Re: [Intel-gfx] [PATCH 2/2] drm/i915/ttm: consider CCS for backup objects

["Das, Nirmoy" <nirmoy.das@linux.intel.com>]

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);
>