Re: [PATCH v3 2/3] drm/vram: Add infrastructure for move_notify()

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote:
> This patch prepares VRAM helpers for lazy unmapping of buffer objects.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++
>  include/drm/drm_vram_mm_helper.h     |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c
> index c911781d6728..31984690d5f3 100644
> --- a/drivers/gpu/drm/drm_vram_mm_helper.c
> +++ b/drivers/gpu/drm/drm_vram_mm_helper.c
> @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo,
>  	return vmm->funcs->verify_access(bo, filp);
>  }
>  
> +static void bo_driver_move_notify(struct ttm_buffer_object *bo,
> +				  bool evict,
> +				  struct ttm_mem_reg *new_mem)
> +{
> +	struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev);
> +
> +	if (!vmm->funcs || !vmm->funcs->move_notify)
> +		return;
> +	vmm->funcs->move_notify(bo, evict, new_mem);
> +}
> +
>  static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev,
>  				    struct ttm_mem_reg *mem)
>  {
> @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = {
>  	.eviction_valuable = ttm_bo_eviction_valuable,
>  	.evict_flags = bo_driver_evict_flags,
>  	.verify_access = bo_driver_verify_access,
> +	.move_notify = bo_driver_move_notify,
>  	.io_mem_reserve = bo_driver_io_mem_reserve,
>  	.io_mem_free = bo_driver_io_mem_free,
>  };
> diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h
> index 2aacfb1ccfae..7fb8700f45fe 100644
> --- a/include/drm/drm_vram_mm_helper.h
> +++ b/include/drm/drm_vram_mm_helper.h
> @@ -15,6 +15,8 @@ struct drm_device;
>  	&ttm_bo_driver.evict_flags
>   * @verify_access:	Provides an implementation for \
>  	struct &ttm_bo_driver.verify_access
> + * @move_notify:	Provides an implementation for
> + *			struct &ttm_bo_driver.move_notify
>   *
>   * These callback function integrate VRAM MM with TTM buffer objects. New
>   * functions can be added if necessary.
> @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs {
>  	void (*evict_flags)(struct ttm_buffer_object *bo,
>  			    struct ttm_placement *placement);
>  	int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp);
> +	void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
> +			    struct ttm_mem_reg *new_mem);

Is this indirection really worth it? We have a grand total of 1 driver
which isn't using gem (vmwgfx), and I don't think that one will ever
switch over to vram helpers.

I'd fold that all in. Helpers don't need to be flexible enough to support
every possible use-case (that's just the job of the core), they can be
opinionated to get cleaner code for most cases.

For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs
(4 with this patch here), which I think is a neat simplification of the
complexity exposed to driver writers. Just put the necessary declarations
into a drm_vram_helper_internal.h so that drivers don't get at it by
accident (like the other drm*internal.h helpers we have).
-Daniel

>  };
>  
>  /**
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel