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

[Zbigniew Kempczyński]

On Fri, Mar 29, 2024 at 02:40:50PM +0100, Karolina Stolarek wrote:
> 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 at intel.com>
> > Cc: Karolina Stolarek <karolina.stolarek at intel.com>
> > Cc: Akshata Jahagirdar <akshata.jahagirdar at 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)?

But this might stop being true if someone will decrease width/height
to 256/256. So defining as a half will decrease to 128/128 what is not
my intention.

> 
> > +#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.

Agree, with the proposed code above comment is inadequate. I'll rephrase
it.

Thank you very much for the review.

--
Zbigniew

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