[PATCH 1/2] drm/i915: remove writeback hook

[PATCH 1/2] drm/i915: remove writeback hook

[Matthew Auld]

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.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.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);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 89b70f5cde7a..820eee5e954e 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);
-
-	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;
 }
 
-- 
2.31.1

[PATCH 2/2] drm/i915: clean up shrinker_release_pages

[Matthew Auld]

Add some proper flags for the different modes, and shorten the name to
something more snappy.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 .../gpu/drm/i915/gem/i915_gem_object_types.h  | 23 ++++++++++++++++---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c     |  8 +++----
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c  | 16 +++++++++----
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 10 ++++----
 4 files changed, 39 insertions(+), 18 deletions(-)

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 00c844caeabd..6f446cca4322 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -57,9 +57,26 @@ 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);
-	int (*shrinker_release_pages)(struct drm_i915_gem_object *obj,
-				      bool no_gpu_wait,
-				      bool should_writeback);
+	/**
+	 * shrink - Perform further backend specific actions to facilate
+	 * shrinking.
+	 * @obj: The gem object
+	 * @flags: Extra flags to control shrinking behaviour in the backend
+	 *
+	 * Possible values for @flags:
+	 *
+	 * I915_GEM_OBJECT_SHRINK_WRITEBACK - Try to perform writeback of the
+	 * backing pages, if supported.
+	 *
+	 * I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT - Don't wait for the object to
+	 * idle.  Active objects can be considered later. The TTM backend for
+	 * example might have aync migrations going on, which don't use any
+	 * i915_vma to track the active GTT binding, and hence having an unbound
+	 * object might not be enough.
+	 */
+#define I915_GEM_OBJECT_SHRINK_WRITEBACK   BIT(0)
+#define I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT BIT(1)
+	int (*shrink)(struct drm_i915_gem_object *obj, unsigned int flags);
 
 	int (*pread)(struct drm_i915_gem_object *obj,
 		     const struct drm_i915_gem_pread *arg);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 7fdf4fa10b0e..6c57b0a79c8a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -331,9 +331,7 @@ 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)
+static int shmem_shrink(struct drm_i915_gem_object *obj, unsigned int flags)
 {
 	switch (obj->mm.madv) {
 	case I915_MADV_DONTNEED:
@@ -342,7 +340,7 @@ static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj,
 		return 0;
 	}
 
-	if (writeback)
+	if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK)
 		shmem_writeback(obj);
 
 	return 0;
@@ -520,7 +518,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,
-	.shrinker_release_pages = shmem_shrinker_release_pages,
+	.shrink = shmem_shrink,
 
 	.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 fd54e05521f6..968ca0fdd57b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -57,10 +57,18 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj,
 
 static int try_to_writeback(struct drm_i915_gem_object *obj, unsigned int flags)
 {
-	if (obj->ops->shrinker_release_pages)
-		return obj->ops->shrinker_release_pages(obj,
-							!(flags & I915_SHRINK_ACTIVE),
-							flags & I915_SHRINK_WRITEBACK);
+	if (obj->ops->shrink) {
+		unsigned int shrink_flags = 0;
+
+		if (!(flags & I915_SHRINK_ACTIVE))
+			shrink_flags |= I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT;
+
+		if (flags & I915_SHRINK_WRITEBACK)
+			shrink_flags |= I915_GEM_OBJECT_SHRINK_WRITEBACK;
+
+		return obj->ops->shrink(obj, shrink_flags);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 923cc7ad8d70..21277f3c64e7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -424,16 +424,14 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
-static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj,
-					   bool no_wait_gpu,
-					   bool should_writeback)
+static int i915_ttm_shrink(struct drm_i915_gem_object *obj, unsigned int flags)
 {
 	struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
 	struct i915_ttm_tt *i915_tt =
 		container_of(bo->ttm, typeof(*i915_tt), ttm);
 	struct ttm_operation_ctx ctx = {
 		.interruptible = true,
-		.no_wait_gpu = no_wait_gpu,
+		.no_wait_gpu = flags & I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT,
 	};
 	struct ttm_placement place = {};
 	int ret;
@@ -467,7 +465,7 @@ static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj,
 		return ret;
 	}
 
-	if (should_writeback)
+	if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK)
 		__shmem_writeback(obj->base.size, i915_tt->filp->f_mapping);
 
 	return 0;
@@ -953,7 +951,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
 	.get_pages = i915_ttm_get_pages,
 	.put_pages = i915_ttm_put_pages,
 	.truncate = i915_ttm_purge,
-	.shrinker_release_pages = i915_ttm_shrinker_release_pages,
+	.shrink = i915_ttm_shrink,
 
 	.adjust_lru = i915_ttm_adjust_lru,
 	.delayed_free = i915_ttm_delayed_free,
-- 
2.31.1

Re: [PATCH 2/2] drm/i915: clean up shrinker_release_pages

[Tvrtko Ursulin]

On 15/12/2021 11:07, Matthew Auld wrote:
> Add some proper flags for the different modes, and shorten the name to
> something more snappy.

Looks good to me - but since it touches TTM I leave for Thomas to approve.

Regards,

Tvrtko

P.S. I hope writing the patch means you thought it is an improvement as 
well, rather than feeling I was asking for it to be done.

> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>   .../gpu/drm/i915/gem/i915_gem_object_types.h  | 23 ++++++++++++++++---
>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c     |  8 +++----
>   drivers/gpu/drm/i915/gem/i915_gem_shrinker.c  | 16 +++++++++----
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 10 ++++----
>   4 files changed, 39 insertions(+), 18 deletions(-)
> 
> 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 00c844caeabd..6f446cca4322 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -57,9 +57,26 @@ 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);
> -	int (*shrinker_release_pages)(struct drm_i915_gem_object *obj,
> -				      bool no_gpu_wait,
> -				      bool should_writeback);
> +	/**
> +	 * shrink - Perform further backend specific actions to facilate
> +	 * shrinking.
> +	 * @obj: The gem object
> +	 * @flags: Extra flags to control shrinking behaviour in the backend
> +	 *
> +	 * Possible values for @flags:
> +	 *
> +	 * I915_GEM_OBJECT_SHRINK_WRITEBACK - Try to perform writeback of the
> +	 * backing pages, if supported.
> +	 *
> +	 * I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT - Don't wait for the object to
> +	 * idle.  Active objects can be considered later. The TTM backend for
> +	 * example might have aync migrations going on, which don't use any
> +	 * i915_vma to track the active GTT binding, and hence having an unbound
> +	 * object might not be enough.
> +	 */
> +#define I915_GEM_OBJECT_SHRINK_WRITEBACK   BIT(0)
> +#define I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT BIT(1)
> +	int (*shrink)(struct drm_i915_gem_object *obj, unsigned int flags);
>   
>   	int (*pread)(struct drm_i915_gem_object *obj,
>   		     const struct drm_i915_gem_pread *arg);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 7fdf4fa10b0e..6c57b0a79c8a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -331,9 +331,7 @@ 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)
> +static int shmem_shrink(struct drm_i915_gem_object *obj, unsigned int flags)
>   {
>   	switch (obj->mm.madv) {
>   	case I915_MADV_DONTNEED:
> @@ -342,7 +340,7 @@ static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj,
>   		return 0;
>   	}
>   
> -	if (writeback)
> +	if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK)
>   		shmem_writeback(obj);
>   
>   	return 0;
> @@ -520,7 +518,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,
> -	.shrinker_release_pages = shmem_shrinker_release_pages,
> +	.shrink = shmem_shrink,
>   
>   	.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 fd54e05521f6..968ca0fdd57b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> @@ -57,10 +57,18 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj,
>   
>   static int try_to_writeback(struct drm_i915_gem_object *obj, unsigned int flags)
>   {
> -	if (obj->ops->shrinker_release_pages)
> -		return obj->ops->shrinker_release_pages(obj,
> -							!(flags & I915_SHRINK_ACTIVE),
> -							flags & I915_SHRINK_WRITEBACK);
> +	if (obj->ops->shrink) {
> +		unsigned int shrink_flags = 0;
> +
> +		if (!(flags & I915_SHRINK_ACTIVE))
> +			shrink_flags |= I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT;
> +
> +		if (flags & I915_SHRINK_WRITEBACK)
> +			shrink_flags |= I915_GEM_OBJECT_SHRINK_WRITEBACK;
> +
> +		return obj->ops->shrink(obj, shrink_flags);
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 923cc7ad8d70..21277f3c64e7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -424,16 +424,14 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj)
>   	return 0;
>   }
>   
> -static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj,
> -					   bool no_wait_gpu,
> -					   bool should_writeback)
> +static int i915_ttm_shrink(struct drm_i915_gem_object *obj, unsigned int flags)
>   {
>   	struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>   	struct i915_ttm_tt *i915_tt =
>   		container_of(bo->ttm, typeof(*i915_tt), ttm);
>   	struct ttm_operation_ctx ctx = {
>   		.interruptible = true,
> -		.no_wait_gpu = no_wait_gpu,
> +		.no_wait_gpu = flags & I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT,
>   	};
>   	struct ttm_placement place = {};
>   	int ret;
> @@ -467,7 +465,7 @@ static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj,
>   		return ret;
>   	}
>   
> -	if (should_writeback)
> +	if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK)
>   		__shmem_writeback(obj->base.size, i915_tt->filp->f_mapping);
>   
>   	return 0;
> @@ -953,7 +951,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
>   	.get_pages = i915_ttm_get_pages,
>   	.put_pages = i915_ttm_put_pages,
>   	.truncate = i915_ttm_purge,
> -	.shrinker_release_pages = i915_ttm_shrinker_release_pages,
> +	.shrink = i915_ttm_shrink,
>   
>   	.adjust_lru = i915_ttm_adjust_lru,
>   	.delayed_free = i915_ttm_delayed_free,
> 

Re: [PATCH 2/2] drm/i915: clean up shrinker_release_pages

[Matthew Auld]

On 15/12/2021 15:55, Tvrtko Ursulin wrote:
> 
> On 15/12/2021 11:07, Matthew Auld wrote:
>> Add some proper flags for the different modes, and shorten the name to
>> something more snappy.
> 
> Looks good to me - but since it touches TTM I leave for Thomas to approve.
> 
> Regards,
> 
> Tvrtko
> 
> P.S. I hope writing the patch means you thought it is an improvement as 
> well, rather than feeling I was asking for it to be done.

Yes, I do see both patches as an improvement :)

> 
>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> ---
>>   .../gpu/drm/i915/gem/i915_gem_object_types.h  | 23 ++++++++++++++++---
>>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c     |  8 +++----
>>   drivers/gpu/drm/i915/gem/i915_gem_shrinker.c  | 16 +++++++++----
>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 10 ++++----
>>   4 files changed, 39 insertions(+), 18 deletions(-)
>>
>> 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 00c844caeabd..6f446cca4322 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> @@ -57,9 +57,26 @@ 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);
>> -    int (*shrinker_release_pages)(struct drm_i915_gem_object *obj,
>> -                      bool no_gpu_wait,
>> -                      bool should_writeback);
>> +    /**
>> +     * shrink - Perform further backend specific actions to facilate
>> +     * shrinking.
>> +     * @obj: The gem object
>> +     * @flags: Extra flags to control shrinking behaviour in the backend
>> +     *
>> +     * Possible values for @flags:
>> +     *
>> +     * I915_GEM_OBJECT_SHRINK_WRITEBACK - Try to perform writeback of 
>> the
>> +     * backing pages, if supported.
>> +     *
>> +     * I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT - Don't wait for the object to
>> +     * idle.  Active objects can be considered later. The TTM backend 
>> for
>> +     * example might have aync migrations going on, which don't use any
>> +     * i915_vma to track the active GTT binding, and hence having an 
>> unbound
>> +     * object might not be enough.
>> +     */
>> +#define I915_GEM_OBJECT_SHRINK_WRITEBACK   BIT(0)
>> +#define I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT BIT(1)
>> +    int (*shrink)(struct drm_i915_gem_object *obj, unsigned int flags);
>>       int (*pread)(struct drm_i915_gem_object *obj,
>>                const struct drm_i915_gem_pread *arg);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> index 7fdf4fa10b0e..6c57b0a79c8a 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -331,9 +331,7 @@ 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)
>> +static int shmem_shrink(struct drm_i915_gem_object *obj, unsigned int 
>> flags)
>>   {
>>       switch (obj->mm.madv) {
>>       case I915_MADV_DONTNEED:
>> @@ -342,7 +340,7 @@ static int shmem_shrinker_release_pages(struct 
>> drm_i915_gem_object *obj,
>>           return 0;
>>       }
>> -    if (writeback)
>> +    if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK)
>>           shmem_writeback(obj);
>>       return 0;
>> @@ -520,7 +518,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,
>> -    .shrinker_release_pages = shmem_shrinker_release_pages,
>> +    .shrink = shmem_shrink,
>>       .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 fd54e05521f6..968ca0fdd57b 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
>> @@ -57,10 +57,18 @@ static bool unsafe_drop_pages(struct 
>> drm_i915_gem_object *obj,
>>   static int try_to_writeback(struct drm_i915_gem_object *obj, 
>> unsigned int flags)
>>   {
>> -    if (obj->ops->shrinker_release_pages)
>> -        return obj->ops->shrinker_release_pages(obj,
>> -                            !(flags & I915_SHRINK_ACTIVE),
>> -                            flags & I915_SHRINK_WRITEBACK);
>> +    if (obj->ops->shrink) {
>> +        unsigned int shrink_flags = 0;
>> +
>> +        if (!(flags & I915_SHRINK_ACTIVE))
>> +            shrink_flags |= I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT;
>> +
>> +        if (flags & I915_SHRINK_WRITEBACK)
>> +            shrink_flags |= I915_GEM_OBJECT_SHRINK_WRITEBACK;
>> +
>> +        return obj->ops->shrink(obj, shrink_flags);
>> +    }
>> +
>>       return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 923cc7ad8d70..21277f3c64e7 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -424,16 +424,14 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj)
>>       return 0;
>>   }
>> -static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object 
>> *obj,
>> -                       bool no_wait_gpu,
>> -                       bool should_writeback)
>> +static int i915_ttm_shrink(struct drm_i915_gem_object *obj, unsigned 
>> int flags)
>>   {
>>       struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>>       struct i915_ttm_tt *i915_tt =
>>           container_of(bo->ttm, typeof(*i915_tt), ttm);
>>       struct ttm_operation_ctx ctx = {
>>           .interruptible = true,
>> -        .no_wait_gpu = no_wait_gpu,
>> +        .no_wait_gpu = flags & I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT,
>>       };
>>       struct ttm_placement place = {};
>>       int ret;
>> @@ -467,7 +465,7 @@ static int i915_ttm_shrinker_release_pages(struct 
>> drm_i915_gem_object *obj,
>>           return ret;
>>       }
>> -    if (should_writeback)
>> +    if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK)
>>           __shmem_writeback(obj->base.size, i915_tt->filp->f_mapping);
>>       return 0;
>> @@ -953,7 +951,7 @@ static const struct drm_i915_gem_object_ops 
>> i915_gem_ttm_obj_ops = {
>>       .get_pages = i915_ttm_get_pages,
>>       .put_pages = i915_ttm_put_pages,
>>       .truncate = i915_ttm_purge,
>> -    .shrinker_release_pages = i915_ttm_shrinker_release_pages,
>> +    .shrink = i915_ttm_shrink,
>>       .adjust_lru = i915_ttm_adjust_lru,
>>       .delayed_free = i915_ttm_delayed_free,
>>

Re: [Intel-gfx] [PATCH 2/2] drm/i915: clean up shrinker_release_pages

[Thomas Hellström (Intel)]

Hi, Matthew

On 12/15/21 12:07, Matthew Auld wrote:
> Add some proper flags for the different modes, and shorten the name to
> something more snappy.
>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>

LGTM.

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>