Re: [PATCH v2 7/8] drm/vmwgfx: Abstract placement selection
> >
> > Problem with explicit placement selection in vmwgfx is that by the time
> > the buffer object needs to be validated the information about which
> > placement was supposed to be used is lost. To workaround this the driver
> > had a bunch of state in various places e.g. as_mob or cpu_blit to
> > somehow convey the information on which placement was intended.
> >
> > Fix it properly by allowing the buffer objects to hold their preferred
> > placement so it can be reused whenever needed. This makes the entire
> > validation pipeline a lot easier both to understand and maintain.
I think this patch mixes up src/dst_pitch in the stdu code.
CC [M] drivers/gpu/drm/etnaviv/etnaviv_gem_submit.o
/home/airlied/devel/kernel/dim/src/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c:509:29:
error: variable 'dst_pitch' is uninitialized when used here
[-Werror,-Wuninitialized]
src_offset = ddirty->top * dst_pitch + ddirty->left * stdu->cpp;
^
/home/airlied/devel/kernel/dim/src/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c:492:26:
note: initialize the variable 'dst_pitch' to silence this warning
s32 src_pitch, dst_pitch;
^
= 0
Dave.
> >
> > Signed-off-by: Zack Rusin
> > ---
> > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c| 145 +++--
> > drivers/gpu/drm/vmwgfx/vmwgfx_bo.h| 25 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 9 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 11 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 -
> > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 35 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 5 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 21 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 11 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 3 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 13 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_shader.c| 15 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_so.c| 4 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 304 ++
> > drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c | 3 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 6 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c| 47 ---
> > drivers/gpu/drm/vmwgfx/vmwgfx_va.c| 4 +-
> > drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 74 ++---
> > drivers/gpu/drm/vmwgfx/vmwgfx_validation.h| 6 +-
> > 22 files changed, 312 insertions(+), 456 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> > index c6dc733f6d45..d8f6ccecf4bf 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> > @@ -135,11 +135,17 @@ int vmw_bo_pin_in_vram_or_gmr(struct vmw_private
> > *dev_priv,
> > goto out_unreserve;
> > }
> >
> > - ret = ttm_bo_validate(bo, &vmw_vram_gmr_placement, &ctx);
> > + vmw_bo_placement_set(buf,
> > + VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM,
> > + VMW_BO_DOMAIN_GMR);
> > + ret = ttm_bo_validate(bo, &buf->placement, &ctx);
> > if (likely(ret == 0) || ret == -ERESTARTSYS)
> > goto out_unreserve;
> >
> > - ret = ttm_bo_validate(bo, &vmw_vram_placement, &ctx);
> > + vmw_bo_placement_set(buf,
> > + VMW_BO_DOMAIN_VRAM,
> > + VMW_BO_DOMAIN_VRAM);
> > + ret = ttm_bo_validate(bo, &buf->placement, &ctx);
> >
> > out_unreserve:
> > if (!ret)
> > @@ -190,17 +196,8 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
> > *dev_priv,
> > {
> > struct ttm_operation_ctx ctx = {interruptible, false };
> > struct ttm_buffer_object *bo = &buf->base;
> > - struct ttm_placement placement;
> > - struct ttm_place place;
> > int ret = 0;
> >
> > - place = vmw_vram_placement.placement[0];
> > - place.lpfn = PFN_UP(bo->resource->size);
> > - placement.num_placement = 1;
> > - placement.placement = &place;
> > - placement.num_busy_placement = 1;
> > - placement.busy_placement = &place;
> > -
> > vmw_execbuf_release_pinned_bo(dev_priv);
> > ret = ttm_bo_reserve(bo, interruptible, false, NULL);
> > if (unlikely(ret != 0))
> > @@ -216,14 +213,21 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
> > *dev_priv,
> > bo->resource->start > 0 &&
> > buf->base.pin_count == 0) {
> > ctx.interruptible = false;
> > - (void) ttm_bo_validate(bo, &vmw_sys_placement, &ctx);
> > + vmw_bo_placement_set(buf,
> > + VMW_BO_DOMAIN_SYS,
> > + VMW_BO_DOMAIN_SYS);
> > + (void)ttm_bo_vali
Re: [PATCH v2 7/8] drm/vmwgfx: Abstract placement selection
On 1/30/23 19:35, Zack Rusin wrote:
> From: Zack Rusin
>
> Problem with explicit placement selection in vmwgfx is that by the time
> the buffer object needs to be validated the information about which
> placement was supposed to be used is lost. To workaround this the driver
> had a bunch of state in various places e.g. as_mob or cpu_blit to
> somehow convey the information on which placement was intended.
>
> Fix it properly by allowing the buffer objects to hold their preferred
> placement so it can be reused whenever needed. This makes the entire
> validation pipeline a lot easier both to understand and maintain.
>
> Signed-off-by: Zack Rusin
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c| 145 +++--
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.h| 25 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 9 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 11 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 -
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 35 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 5 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 21 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 11 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 3 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 13 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_shader.c| 15 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_so.c| 4 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 304 ++
> drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c | 3 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 6 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c| 47 ---
> drivers/gpu/drm/vmwgfx/vmwgfx_va.c| 4 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 74 ++---
> drivers/gpu/drm/vmwgfx/vmwgfx_validation.h| 6 +-
> 22 files changed, 312 insertions(+), 456 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index c6dc733f6d45..d8f6ccecf4bf 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -135,11 +135,17 @@ int vmw_bo_pin_in_vram_or_gmr(struct vmw_private
> *dev_priv,
> goto out_unreserve;
> }
>
> - ret = ttm_bo_validate(bo, &vmw_vram_gmr_placement, &ctx);
> + vmw_bo_placement_set(buf,
> + VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM,
> + VMW_BO_DOMAIN_GMR);
> + ret = ttm_bo_validate(bo, &buf->placement, &ctx);
> if (likely(ret == 0) || ret == -ERESTARTSYS)
> goto out_unreserve;
>
> - ret = ttm_bo_validate(bo, &vmw_vram_placement, &ctx);
> + vmw_bo_placement_set(buf,
> + VMW_BO_DOMAIN_VRAM,
> + VMW_BO_DOMAIN_VRAM);
> + ret = ttm_bo_validate(bo, &buf->placement, &ctx);
>
> out_unreserve:
> if (!ret)
> @@ -190,17 +196,8 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
> *dev_priv,
> {
> struct ttm_operation_ctx ctx = {interruptible, false };
> struct ttm_buffer_object *bo = &buf->base;
> - struct ttm_placement placement;
> - struct ttm_place place;
> int ret = 0;
>
> - place = vmw_vram_placement.placement[0];
> - place.lpfn = PFN_UP(bo->resource->size);
> - placement.num_placement = 1;
> - placement.placement = &place;
> - placement.num_busy_placement = 1;
> - placement.busy_placement = &place;
> -
> vmw_execbuf_release_pinned_bo(dev_priv);
> ret = ttm_bo_reserve(bo, interruptible, false, NULL);
> if (unlikely(ret != 0))
> @@ -216,14 +213,21 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
> *dev_priv,
> bo->resource->start > 0 &&
> buf->base.pin_count == 0) {
> ctx.interruptible = false;
> - (void) ttm_bo_validate(bo, &vmw_sys_placement, &ctx);
> + vmw_bo_placement_set(buf,
> + VMW_BO_DOMAIN_SYS,
> + VMW_BO_DOMAIN_SYS);
> + (void)ttm_bo_validate(bo, &buf->placement, &ctx);
> }
>
> + vmw_bo_placement_set(buf,
> + VMW_BO_DOMAIN_VRAM,
> + VMW_BO_DOMAIN_VRAM);
> + buf->places[0].lpfn = PFN_UP(bo->resource->size);
> if (buf->base.pin_count > 0)
> - ret = ttm_resource_compat(bo->resource, &placement)
> + ret = ttm_resource_compat(bo->resource, &buf->placement)
> ? 0 : -EINVAL;
> else
> - ret = ttm_bo_validate(bo, &placement, &ctx);
> + ret = ttm_bo_validate(bo, &buf->placement, &ctx);
>
> /* For some reason we didn't end up at the start of vram */
> WARN_ON(ret == 0 && bo->resource->start != 0);
> @@ -431,7 +435,7 @@ int vmw_bo_create_kernel(struct
Re: [PATCH v2 7/8] drm/vmwgfx: Abstract placement selection
From: Martin Krastev
LGTM
Reviewed-by: Martin Krastev
Regards,
Martin
On 31.01.23 г. 5:35 ч., Zack Rusin wrote:
From: Zack Rusin
Problem with explicit placement selection in vmwgfx is that by the time
the buffer object needs to be validated the information about which
placement was supposed to be used is lost. To workaround this the driver
had a bunch of state in various places e.g. as_mob or cpu_blit to
somehow convey the information on which placement was intended.
Fix it properly by allowing the buffer objects to hold their preferred
placement so it can be reused whenever needed. This makes the entire
validation pipeline a lot easier both to understand and maintain.
Signed-off-by: Zack Rusin
---
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c| 145 +++--
drivers/gpu/drm/vmwgfx/vmwgfx_bo.h| 25 +-
drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 9 +-
drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 11 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 -
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 35 +-
drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 5 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 21 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 11 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 13 +-
drivers/gpu/drm/vmwgfx/vmwgfx_shader.c| 15 +-
drivers/gpu/drm/vmwgfx/vmwgfx_so.c| 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 304 ++
drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c | 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 6 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c| 47 ---
drivers/gpu/drm/vmwgfx/vmwgfx_va.c| 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 74 ++---
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h| 6 +-
22 files changed, 312 insertions(+), 456 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index c6dc733f6d45..d8f6ccecf4bf 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -135,11 +135,17 @@ int vmw_bo_pin_in_vram_or_gmr(struct vmw_private
*dev_priv,
goto out_unreserve;
}
- ret = ttm_bo_validate(bo, &vmw_vram_gmr_placement, &ctx);
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM,
+VMW_BO_DOMAIN_GMR);
+ ret = ttm_bo_validate(bo, &buf->placement, &ctx);
if (likely(ret == 0) || ret == -ERESTARTSYS)
goto out_unreserve;
- ret = ttm_bo_validate(bo, &vmw_vram_placement, &ctx);
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_VRAM,
+VMW_BO_DOMAIN_VRAM);
+ ret = ttm_bo_validate(bo, &buf->placement, &ctx);
out_unreserve:
if (!ret)
@@ -190,17 +196,8 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
*dev_priv,
{
struct ttm_operation_ctx ctx = {interruptible, false };
struct ttm_buffer_object *bo = &buf->base;
- struct ttm_placement placement;
- struct ttm_place place;
int ret = 0;
- place = vmw_vram_placement.placement[0];
- place.lpfn = PFN_UP(bo->resource->size);
- placement.num_placement = 1;
- placement.placement = &place;
- placement.num_busy_placement = 1;
- placement.busy_placement = &place;
-
vmw_execbuf_release_pinned_bo(dev_priv);
ret = ttm_bo_reserve(bo, interruptible, false, NULL);
if (unlikely(ret != 0))
@@ -216,14 +213,21 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
*dev_priv,
bo->resource->start > 0 &&
buf->base.pin_count == 0) {
ctx.interruptible = false;
- (void) ttm_bo_validate(bo, &vmw_sys_placement, &ctx);
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_SYS,
+VMW_BO_DOMAIN_SYS);
+ (void)ttm_bo_validate(bo, &buf->placement, &ctx);
}
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_VRAM,
+VMW_BO_DOMAIN_VRAM);
+ buf->places[0].lpfn = PFN_UP(bo->resource->size);
if (buf->base.pin_count > 0)
- ret = ttm_resource_compat(bo->resource, &placement)
+ ret = ttm_resource_compat(bo->resource, &buf->placement)
? 0 : -EINVAL;
else
- ret = ttm_bo_validate(bo, &placement, &ctx);
+ ret = ttm_bo_validate(bo, &buf->placement, &ctx);
/* For some reason we didn't end up at the start of vram */
WARN_ON(ret == 0 && bo->resource->start != 0);
@@ -431,7 +435,7 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv,
[PATCH v2 7/8] drm/vmwgfx: Abstract placement selection
From: Zack Rusin
Problem with explicit placement selection in vmwgfx is that by the time
the buffer object needs to be validated the information about which
placement was supposed to be used is lost. To workaround this the driver
had a bunch of state in various places e.g. as_mob or cpu_blit to
somehow convey the information on which placement was intended.
Fix it properly by allowing the buffer objects to hold their preferred
placement so it can be reused whenever needed. This makes the entire
validation pipeline a lot easier both to understand and maintain.
Signed-off-by: Zack Rusin
---
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c| 145 +++--
drivers/gpu/drm/vmwgfx/vmwgfx_bo.h| 25 +-
drivers/gpu/drm/vmwgfx/vmwgfx_context.c | 9 +-
drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 11 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 -
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 35 +-
drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 5 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 21 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 11 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 13 +-
drivers/gpu/drm/vmwgfx/vmwgfx_shader.c| 15 +-
drivers/gpu/drm/vmwgfx/vmwgfx_so.c| 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 304 ++
drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c | 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 6 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c| 47 ---
drivers/gpu/drm/vmwgfx/vmwgfx_va.c| 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 74 ++---
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h| 6 +-
22 files changed, 312 insertions(+), 456 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index c6dc733f6d45..d8f6ccecf4bf 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -135,11 +135,17 @@ int vmw_bo_pin_in_vram_or_gmr(struct vmw_private
*dev_priv,
goto out_unreserve;
}
- ret = ttm_bo_validate(bo, &vmw_vram_gmr_placement, &ctx);
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM,
+VMW_BO_DOMAIN_GMR);
+ ret = ttm_bo_validate(bo, &buf->placement, &ctx);
if (likely(ret == 0) || ret == -ERESTARTSYS)
goto out_unreserve;
- ret = ttm_bo_validate(bo, &vmw_vram_placement, &ctx);
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_VRAM,
+VMW_BO_DOMAIN_VRAM);
+ ret = ttm_bo_validate(bo, &buf->placement, &ctx);
out_unreserve:
if (!ret)
@@ -190,17 +196,8 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
*dev_priv,
{
struct ttm_operation_ctx ctx = {interruptible, false };
struct ttm_buffer_object *bo = &buf->base;
- struct ttm_placement placement;
- struct ttm_place place;
int ret = 0;
- place = vmw_vram_placement.placement[0];
- place.lpfn = PFN_UP(bo->resource->size);
- placement.num_placement = 1;
- placement.placement = &place;
- placement.num_busy_placement = 1;
- placement.busy_placement = &place;
-
vmw_execbuf_release_pinned_bo(dev_priv);
ret = ttm_bo_reserve(bo, interruptible, false, NULL);
if (unlikely(ret != 0))
@@ -216,14 +213,21 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private
*dev_priv,
bo->resource->start > 0 &&
buf->base.pin_count == 0) {
ctx.interruptible = false;
- (void) ttm_bo_validate(bo, &vmw_sys_placement, &ctx);
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_SYS,
+VMW_BO_DOMAIN_SYS);
+ (void)ttm_bo_validate(bo, &buf->placement, &ctx);
}
+ vmw_bo_placement_set(buf,
+VMW_BO_DOMAIN_VRAM,
+VMW_BO_DOMAIN_VRAM);
+ buf->places[0].lpfn = PFN_UP(bo->resource->size);
if (buf->base.pin_count > 0)
- ret = ttm_resource_compat(bo->resource, &placement)
+ ret = ttm_resource_compat(bo->resource, &buf->placement)
? 0 : -EINVAL;
else
- ret = ttm_bo_validate(bo, &placement, &ctx);
+ ret = ttm_bo_validate(bo, &buf->placement, &ctx);
/* For some reason we didn't end up at the start of vram */
WARN_ON(ret == 0 && bo->resource->start != 0);
@@ -431,7 +435,7 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv,
unsigned long size,
}
int vmw_bo_create(struct vmw_private *vmw,
- size_t size, struct ttm_placement *placemen
