Re: [PATCH] drm/i915: remove writeback hook

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/12/2021 18:07, Matthew Auld wrote:
> Ditch the writeback hook and drop i915_gem_object_writeback(). We
> already support the shrinker_release_pages hook which can just call
> shmem_writeback directly.

Looks like a good cleanup to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A couple of bike shedding comments/question only below.

> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_object.h    |  1 -
>   .../gpu/drm/i915/gem/i915_gem_object_types.h  |  1 -
>   drivers/gpu/drm/i915/gem/i915_gem_pages.c     | 10 ----------
>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c     | 19 ++++++++++++++++++-
>   drivers/gpu/drm/i915/gem/i915_gem_shrinker.c  | 12 ------------
>   5 files changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 66f20b803b01..aaf9183e601b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -455,7 +455,6 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>   
>   int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>   int i915_gem_object_truncate(struct drm_i915_gem_object *obj);
> -void i915_gem_object_writeback(struct drm_i915_gem_object *obj);
>   
>   /**
>    * i915_gem_object_pin_map - return a contiguous mapping of the entire object
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index f9f7e44099fe..00c844caeabd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -57,7 +57,6 @@ struct drm_i915_gem_object_ops {
>   	void (*put_pages)(struct drm_i915_gem_object *obj,
>   			  struct sg_table *pages);
>   	int (*truncate)(struct drm_i915_gem_object *obj);
> -	void (*writeback)(struct drm_i915_gem_object *obj);
>   	int (*shrinker_release_pages)(struct drm_i915_gem_object *obj,
>   				      bool no_gpu_wait,
>   				      bool should_writeback);

Perhaps a simple shrink for the vfunc name would suffice and match 
better with the neighbouring names?

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 49c6e55c68ce..52e975f57956 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -168,16 +168,6 @@ int i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>   	return 0;
>   }
>   
> -/* Try to discard unwanted pages */
> -void i915_gem_object_writeback(struct drm_i915_gem_object *obj)
> -{
> -	assert_object_held_shared(obj);
> -	GEM_BUG_ON(i915_gem_object_has_pages(obj));
> -
> -	if (obj->ops->writeback)
> -		obj->ops->writeback(obj);
> -}
> -
>   static void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj)
>   {
>   	struct radix_tree_iter iter;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index cc9fe258fba7..7fdf4fa10b0e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -331,6 +331,23 @@ shmem_writeback(struct drm_i915_gem_object *obj)
>   	__shmem_writeback(obj->base.size, obj->base.filp->f_mapping);
>   }
>   
> +static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj,
> +					bool no_gpu_wait,
> +					bool writeback)
> +{
> +	switch (obj->mm.madv) {
> +	case I915_MADV_DONTNEED:
> +		return i915_gem_object_truncate(obj);
> +	case __I915_MADV_PURGED:
> +		return 0;
> +	}
> +
> +	if (writeback)
> +		shmem_writeback(obj);
> +
> +	return 0;
> +}
> +
>   void
>   __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
>   				struct sg_table *pages,
> @@ -503,7 +520,7 @@ const struct drm_i915_gem_object_ops i915_gem_shmem_ops = {
>   	.get_pages = shmem_get_pages,
>   	.put_pages = shmem_put_pages,
>   	.truncate = shmem_truncate,
> -	.writeback = shmem_writeback,
> +	.shrinker_release_pages = shmem_shrinker_release_pages,
>   
>   	.pwrite = shmem_pwrite,
>   	.pread = shmem_pread,
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> index 157a9765f483..fd54e05521f6 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> @@ -61,18 +61,6 @@ static int try_to_writeback(struct drm_i915_gem_object *obj, unsigned int flags)
>   		return obj->ops->shrinker_release_pages(obj,
>   							!(flags & I915_SHRINK_ACTIVE),
>   							flags & I915_SHRINK_WRITEBACK);

Maybe flags would be better than two booleans? (Usually more than one is 
one two many when readability is concerned.) Although they couldn't be 
from I915_SHRINK_ namespace since that "belongs" to the shrinker, and 
granted, there wouldn't be ideal component separation since caller still 
has to have knowledge of shrinking mechanics to decide which ones is 
safe to pass in. So don't know, just thinking out loud.

Regards,

Tvrtko

> -
> -	switch (obj->mm.madv) {
> -	case I915_MADV_DONTNEED:
> -		i915_gem_object_truncate(obj);
> -		return 0;
> -	case __I915_MADV_PURGED:
> -		return 0;
> -	}
> -
> -	if (flags & I915_SHRINK_WRITEBACK)
> -		i915_gem_object_writeback(obj);
> -
>   	return 0;
>   }
>   
>