[PATCH v2] drm/i915/gem: Pin the L-shape quirked object as unshrinkable

[PATCH v2] drm/i915/gem: Pin the L-shape quirked object as unshrinkable

[Matthew Auld]

From: Chris Wilson <chris@chris-wilson.co.uk>

When instantiating a tiled object on an L-shaped memory machine, we mark
the object as unshrinkable to prevent the shrinker from trying to swap
out the pages. We have to do this as we do not know the swizzling on the
individual pages, and so the data will be scrambled across swap out/in.

Not only do we need to move the object off the shrinker list, we need to
mark the object with shrink_pin so that the counter is consistent across
calls to madvise.

v2: in the madvise ioctl we need to check if the object is currently
shrinkable/purgeable, not if the object type supports shrinking

Fixes: 0175969e489a ("drm/i915/gem: Use shrinkable status for unknown swizzle quirks")
References: https://gitlab.freedesktop.org/drm/intel/-/issues/3293
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_pages.c |  2 ++
 drivers/gpu/drm/i915/i915_gem.c           | 11 +++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index aed8a37ccdc9..7361971c177d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -63,6 +63,8 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
 	    i915->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
 		GEM_BUG_ON(i915_gem_object_has_tiling_quirk(obj));
 		i915_gem_object_set_tiling_quirk(obj);
+		GEM_BUG_ON(!list_empty(&obj->mm.link));
+		atomic_inc(&obj->mm.shrink_pin);
 		shrinkable = false;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d0018c5f88bd..cffd7f4f87dc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1009,12 +1009,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 		obj->mm.madv = args->madv;
 
 	if (i915_gem_object_has_pages(obj)) {
-		struct list_head *list;
+		unsigned long flags;
 
-		if (i915_gem_object_is_shrinkable(obj)) {
-			unsigned long flags;
-
-			spin_lock_irqsave(&i915->mm.obj_lock, flags);
+		spin_lock_irqsave(&i915->mm.obj_lock, flags);
+		if (!list_empty(&obj->mm.link)) {
+			struct list_head *list;
 
 			if (obj->mm.madv != I915_MADV_WILLNEED)
 				list = &i915->mm.purge_list;
@@ -1022,8 +1021,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 				list = &i915->mm.shrink_list;
 			list_move_tail(&obj->mm.link, list);
 
-			spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
 		}
+		spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
 	}
 
 	/* if the object is no longer attached, discard its backing storage */
-- 
2.26.3

Re: [PATCH v2] drm/i915/gem: Pin the L-shape quirked object as unshrinkable

[Ville Syrjälä]

On Thu, May 13, 2021 at 01:07:56PM +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> When instantiating a tiled object on an L-shaped memory machine, we mark
> the object as unshrinkable to prevent the shrinker from trying to swap
> out the pages. We have to do this as we do not know the swizzling on the
> individual pages, and so the data will be scrambled across swap out/in.
> 
> Not only do we need to move the object off the shrinker list, we need to
> mark the object with shrink_pin so that the counter is consistent across
> calls to madvise.
> 
> v2: in the madvise ioctl we need to check if the object is currently
> shrinkable/purgeable, not if the object type supports shrinking
> 
> Fixes: 0175969e489a ("drm/i915/gem: Use shrinkable status for unknown swizzle quirks")
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/3293
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>

This one seems to fare better than v1.

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_pages.c |  2 ++
>  drivers/gpu/drm/i915/i915_gem.c           | 11 +++++------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index aed8a37ccdc9..7361971c177d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -63,6 +63,8 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
>  	    i915->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
>  		GEM_BUG_ON(i915_gem_object_has_tiling_quirk(obj));
>  		i915_gem_object_set_tiling_quirk(obj);
> +		GEM_BUG_ON(!list_empty(&obj->mm.link));
> +		atomic_inc(&obj->mm.shrink_pin);
>  		shrinkable = false;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d0018c5f88bd..cffd7f4f87dc 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1009,12 +1009,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>  		obj->mm.madv = args->madv;
>  
>  	if (i915_gem_object_has_pages(obj)) {
> -		struct list_head *list;
> +		unsigned long flags;
>  
> -		if (i915_gem_object_is_shrinkable(obj)) {
> -			unsigned long flags;
> -
> -			spin_lock_irqsave(&i915->mm.obj_lock, flags);
> +		spin_lock_irqsave(&i915->mm.obj_lock, flags);
> +		if (!list_empty(&obj->mm.link)) {
> +			struct list_head *list;
>  
>  			if (obj->mm.madv != I915_MADV_WILLNEED)
>  				list = &i915->mm.purge_list;
> @@ -1022,8 +1021,8 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>  				list = &i915->mm.shrink_list;
>  			list_move_tail(&obj->mm.link, list);
>  
> -			spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
>  		}
> +		spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
>  	}
>  
>  	/* if the object is no longer attached, discard its backing storage */
> -- 
> 2.26.3

-- 
Ville Syrjälä
Intel