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

Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> · Wed, 15 Dec 2021 15:55:09 +0000

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@xxxxxxxxxxxxxxx>
Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
---
  .../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,







