| Message ID | 20221212171958.82593-2-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/migrate: fix corner case in CCS aux copying | expand |
On 12/12/2022 6:19 PM, Matthew Auld wrote: > It seems we can have one or more framebuffers that are still pinned when > suspending lmem, in such a case we end up creating a shmem backup > object, instead of evicting the object directly, but this will skip > copying the CCS aux state, since we don't allocate the extra storage for > the CCS pages as part of the ttm_tt construction. Since we can already > deal with pinned objects just fine, it doesn't seem too nasty to just > extend to support dealing with the CCS aux state, if the object is a > pinned framebuffer. This fixes display corruption (like in gnome-shell) > seen on DG2 when returning from suspend. This patch fixes resume corruption with full-bar as well, tested with sway. Tested-by: Nirmoy Das <nirmoy.das@intel.com> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com> Thanks, Nirmoy > > Fixes: da0595ae91da ("drm/i915/migrate: Evict and restore the flatccs capable lmem obj") > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Shuicheng Lin <shuicheng.lin@intel.com> > Cc: <stable@vger.kernel.org> # v5.19+ > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 +++ > .../gpu/drm/i915/gem/i915_gem_object_types.h | 10 ++++++---- > drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c | 18 +++++++++++++++++- > 3 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index 733696057761..1a0886b8aaa1 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -785,6 +785,9 @@ bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj) > if (!HAS_FLAT_CCS(to_i915(obj->base.dev))) > return false; > > + if (obj->flags & I915_BO_ALLOC_CCS_AUX) > + return true; > + > for (i = 0; i < obj->mm.n_placements; i++) { > /* Compression is not allowed for the objects with smem placement */ > if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM) > 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 a7b70701617a..19c9bdd8f905 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > @@ -327,16 +327,18 @@ struct drm_i915_gem_object { > * dealing with userspace objects the CPU fault handler is free to ignore this. > */ > #define I915_BO_ALLOC_GPU_ONLY BIT(6) > +#define I915_BO_ALLOC_CCS_AUX BIT(7) > #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \ > I915_BO_ALLOC_VOLATILE | \ > I915_BO_ALLOC_CPU_CLEAR | \ > I915_BO_ALLOC_USER | \ > I915_BO_ALLOC_PM_VOLATILE | \ > I915_BO_ALLOC_PM_EARLY | \ > - I915_BO_ALLOC_GPU_ONLY) > -#define I915_BO_READONLY BIT(7) > -#define I915_TILING_QUIRK_BIT 8 /* unknown swizzling; do not release! */ > -#define I915_BO_PROTECTED BIT(9) > + I915_BO_ALLOC_GPU_ONLY | \ > + I915_BO_ALLOC_CCS_AUX) > +#define I915_BO_READONLY BIT(8) > +#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not release! */ > +#define I915_BO_PROTECTED BIT(10) > /** > * @mem_flags - Mutable placement-related flags > * > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c > index 07e49f22f2de..7e67742bc65e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c > @@ -50,6 +50,7 @@ static int i915_ttm_backup(struct i915_gem_apply_to_region *apply, > container_of(bo->bdev, typeof(*i915), bdev); > struct drm_i915_gem_object *backup; > struct ttm_operation_ctx ctx = {}; > + unsigned int flags; > int err = 0; > > if (bo->resource->mem_type == I915_PL_SYSTEM || obj->ttm.backup) > @@ -65,7 +66,22 @@ static int i915_ttm_backup(struct i915_gem_apply_to_region *apply, > if (obj->flags & I915_BO_ALLOC_PM_VOLATILE) > return 0; > > - backup = i915_gem_object_create_shmem(i915, obj->base.size); > + /* > + * It seems that we might have some framebuffers still pinned at this > + * stage, but for such objects we might also need to deal with the CCS > + * aux state. Make sure we force the save/restore of the CCS state, > + * otherwise we might observe display corruption, when returning from > + * suspend. > + */ > + flags = 0; > + if (i915_gem_object_needs_ccs_pages(obj)) { > + WARN_ON_ONCE(!i915_gem_object_is_framebuffer(obj)); > + WARN_ON_ONCE(!pm_apply->allow_gpu); > + > + flags = I915_BO_ALLOC_CCS_AUX; > + } > + backup = i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_SMEM], > + obj->base.size, 0, flags); > if (IS_ERR(backup)) > return PTR_ERR(backup); >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 733696057761..1a0886b8aaa1 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -785,6 +785,9 @@ bool i915_gem_object_needs_ccs_pages(struct drm_i915_gem_object *obj) if (!HAS_FLAT_CCS(to_i915(obj->base.dev))) return false; + if (obj->flags & I915_BO_ALLOC_CCS_AUX) + return true; + for (i = 0; i < obj->mm.n_placements; i++) { /* Compression is not allowed for the objects with smem placement */ if (obj->mm.placements[i]->type == INTEL_MEMORY_SYSTEM) 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 a7b70701617a..19c9bdd8f905 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -327,16 +327,18 @@ struct drm_i915_gem_object { * dealing with userspace objects the CPU fault handler is free to ignore this. */ #define I915_BO_ALLOC_GPU_ONLY BIT(6) +#define I915_BO_ALLOC_CCS_AUX BIT(7) #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \ I915_BO_ALLOC_VOLATILE | \ I915_BO_ALLOC_CPU_CLEAR | \ I915_BO_ALLOC_USER | \ I915_BO_ALLOC_PM_VOLATILE | \ I915_BO_ALLOC_PM_EARLY | \ - I915_BO_ALLOC_GPU_ONLY) -#define I915_BO_READONLY BIT(7) -#define I915_TILING_QUIRK_BIT 8 /* unknown swizzling; do not release! */ -#define I915_BO_PROTECTED BIT(9) + I915_BO_ALLOC_GPU_ONLY | \ + I915_BO_ALLOC_CCS_AUX) +#define I915_BO_READONLY BIT(8) +#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not release! */ +#define I915_BO_PROTECTED BIT(10) /** * @mem_flags - Mutable placement-related flags * diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c index 07e49f22f2de..7e67742bc65e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c @@ -50,6 +50,7 @@ static int i915_ttm_backup(struct i915_gem_apply_to_region *apply, container_of(bo->bdev, typeof(*i915), bdev); struct drm_i915_gem_object *backup; struct ttm_operation_ctx ctx = {}; + unsigned int flags; int err = 0; if (bo->resource->mem_type == I915_PL_SYSTEM || obj->ttm.backup) @@ -65,7 +66,22 @@ static int i915_ttm_backup(struct i915_gem_apply_to_region *apply, if (obj->flags & I915_BO_ALLOC_PM_VOLATILE) return 0; - backup = i915_gem_object_create_shmem(i915, obj->base.size); + /* + * It seems that we might have some framebuffers still pinned at this + * stage, but for such objects we might also need to deal with the CCS + * aux state. Make sure we force the save/restore of the CCS state, + * otherwise we might observe display corruption, when returning from + * suspend. + */ + flags = 0; + if (i915_gem_object_needs_ccs_pages(obj)) { + WARN_ON_ONCE(!i915_gem_object_is_framebuffer(obj)); + WARN_ON_ONCE(!pm_apply->allow_gpu); + + flags = I915_BO_ALLOC_CCS_AUX; + } + backup = i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_SMEM], + obj->base.size, 0, flags); if (IS_ERR(backup)) return PTR_ERR(backup);
It seems we can have one or more framebuffers that are still pinned when suspending lmem, in such a case we end up creating a shmem backup object, instead of evicting the object directly, but this will skip copying the CCS aux state, since we don't allocate the extra storage for the CCS pages as part of the ttm_tt construction. Since we can already deal with pinned objects just fine, it doesn't seem too nasty to just extend to support dealing with the CCS aux state, if the object is a pinned framebuffer. This fixes display corruption (like in gnome-shell) seen on DG2 when returning from suspend. Fixes: da0595ae91da ("drm/i915/migrate: Evict and restore the flatccs capable lmem obj") Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Nirmoy Das <nirmoy.das@intel.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: Shuicheng Lin <shuicheng.lin@intel.com> Cc: <stable@vger.kernel.org> # v5.19+ --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 +++ .../gpu/drm/i915/gem/i915_gem_object_types.h | 10 ++++++---- drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c | 18 +++++++++++++++++- 3 files changed, 26 insertions(+), 5 deletions(-)