Re: [PATCH i-g-t 2/2] tests/gem|xe_ccs: Check surface ccs data instead of naive comparison

[Karolina Stolarek <karolina.stolarek@intel.com>]

On 29.03.2024 10:16, Zbigniew Kempczyński wrote:
> Start using function which extracts ccs data from the surface and
> determines its compression.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Karolina Stolarek <karolina.stolarek@intel.com>
> Cc: Akshata Jahagirdar <akshata.jahagirdar@intel.com>
> ---
>   tests/intel/gem_ccs.c | 15 ++++++++++++---
>   tests/intel/xe_ccs.c  |  9 +++------
>   2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/intel/gem_ccs.c b/tests/intel/gem_ccs.c
> index e4126bcaa8..c59fc69fb7 100644
> --- a/tests/intel/gem_ccs.c
> +++ b/tests/intel/gem_ccs.c
> @@ -305,6 +305,10 @@ static int blt_block_copy3(int i915,
>   	return ret;
>   }
>   
> +#define CHECK_FROM_WIDTH 256
> +#define CHECK_FROM_HEIGHT 256

I'm not a fan of adding another magic number to the mix. I know it's 
hardly magic, you found it by testing, but could we at least define it 
as a half of default dimensions (512x512)?

> +#define FROM_EXP_WH(w, h) ((w) >= CHECK_FROM_WIDTH && (h) >= CHECK_FROM_HEIGHT)
> +
>   static void block_copy(int i915,
>   		       const intel_ctx_t *ctx,
>   		       const struct intel_execution_engine2 *e,
> @@ -359,9 +363,14 @@ static void block_copy(int i915,
>   	blt_block_copy(i915, ctx, e, ahnd, &blt, pext);
>   	gem_sync(i915, mid->handle);
>   
> -	/* We expect mid != src if there's compression */
> -	if (mid->compression)
> -		igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> +	/*
> +	 * We expect mid != src if there's compression. Ignore this for small
> +	 * width x height for linear as compression for gradient occurs in the
> +	 * middle for bigger sizes.
> +	 */

You mention we ignore this check for small surfaces with no tiling, but 
there's no check for mid_tiling. Is that on purpose? This comment 
applies to comments both in gem_ccs and xe_ccs.

Many thanks,
Karolina

> +	if (mid->compression && FROM_EXP_WH(width, height))
> +		igt_assert(blt_surface_is_compressed(i915, (intel_ctx_t *)ctx, e,
> +						     ahnd, mid));
>   
>   	WRITE_PNG(i915, run_id, "mid", &blt.dst, width, height, bpp);
>   
> diff --git a/tests/intel/xe_ccs.c b/tests/intel/xe_ccs.c
> index 7d0f2f2a11..5525aaed65 100644
> --- a/tests/intel/xe_ccs.c
> +++ b/tests/intel/xe_ccs.c
> @@ -361,13 +361,10 @@ static void block_copy(int xe,
>   	/*
>   	 * We expect mid != src if there's compression. Ignore this for small
>   	 * width x height for linear as compression for gradient occurs in the
> -	 * middle for bigger sizes. We also ignore 1x1 as this looks same for
> -	 * xmajor.
> +	 * middle for bigger sizes.
>   	 */
> -	if (mid->compression && MIN_EXP_WH(width, height)) {
> -		if (mid_tiling != T_LINEAR || FROM_EXP_WH(width, height))
> -			igt_assert(memcmp(src->ptr, mid->ptr, src->size) != 0);
> -	}
> +	if (mid->compression && FROM_EXP_WH(width, height))
> +		igt_assert(blt_surface_is_compressed(xe, ctx, NULL, ahnd, mid));
>   
>   	WRITE_PNG(xe, run_id, "mid", &blt.dst, width, height, bpp);
>