Re: [igt-dev] [PATCH i-g-t 4/4] tests/i915/gem_linear_blits: Enable XY_FAST_COPY_BLT copy instruction

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

On 24.03.2023 14:33, Vikas Srivastava wrote:
> From: Arjun Melkaveri <arjun.melkaveri@intel.com>
> 
> Test case uses legacy command XY_SRC_COPY_BLT_CMD which is
> not supported on newer platforms. Modified test to
> use XY_FAST_COPY_BLT.
> 
> Signed-off-by: Arjun Melkaveri <arjun.melkaveri@intel.com>
> Co-developed-by: Vikas Srivastava <vikas.srivastava@intel.com>

Nice use of the tag, I should probably start using it in my patches as
well...

> Signed-off-by: Vikas Srivastava <vikas.srivastava@intel.com>
> ---
>   tests/i915/gem_linear_blits.c | 66 +++++++++++++++++++++++------------
>   1 file changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/i915/gem_linear_blits.c b/tests/i915/gem_linear_blits.c
> index fac25095f5..6fa6cd4976 100644
> --- a/tests/i915/gem_linear_blits.c
> +++ b/tests/i915/gem_linear_blits.c
> @@ -48,6 +48,7 @@
>   #include "i915/gem_create.h"
>   #include "igt.h"
>   #include "igt_types.h"
> +#include "i915/i915_blt.h"
>   
>   IGT_TEST_DESCRIPTION("Test doing many blits with a working set larger than the"
>   		     " aperture size.");
> @@ -67,6 +68,7 @@ static void copy(int fd, uint64_t ahnd, uint32_t dst, uint32_t src,
>   	struct drm_i915_gem_relocation_entry reloc[2];
>   	struct drm_i915_gem_exec_object2 obj[3];
>   	struct drm_i915_gem_execbuffer2 exec;
> +	static uint32_t devid;
>   	int i = 0;
>   
>   	memset(obj, 0, sizeof(obj));
> @@ -83,29 +85,47 @@ static void copy(int fd, uint64_t ahnd, uint32_t dst, uint32_t src,
>   	obj[2].offset = CANONICAL(obj[2].offset);
>   	obj[2].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>   
> -	batch[i++] = XY_SRC_COPY_BLT_CMD |
> -		  XY_SRC_COPY_BLT_WRITE_ALPHA |
> -		  XY_SRC_COPY_BLT_WRITE_RGB;
> -	if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> -		batch[i - 1] |= 8;
> -	else
> -		batch[i - 1] |= 6;
> -
> -	batch[i++] = (3 << 24) | /* 32 bits */
> -		  (0xcc << 16) | /* copy ROP */
> -		  WIDTH*4;
> -	batch[i++] = 0; /* dst x1,y1 */
> -	batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */
> -	batch[i++] = obj[0].offset;
> -	if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> -		batch[i++] = obj[0].offset >> 32;
> -	batch[i++] = 0; /* src x1,y1 */
> -	batch[i++] = WIDTH*4;
> -	batch[i++] = obj[1].offset;
> -	if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> -		batch[i++] = obj[1].offset >> 32;
> -	batch[i++] = MI_BATCH_BUFFER_END;
> -	batch[i++] = MI_NOOP;
> +	devid = intel_get_drm_devid(fd);
> +
> +	if (blt_has_fast_copy(fd)) {

Don't forget to check the order, like I said in 1/4.

> +		batch[i++] = XY_FAST_COPY_BLT;
> +		batch[i++] = XY_FAST_COPY_COLOR_DEPTH_32 | WIDTH * 4;
> +		batch[i++] = 0; /* dst x1,y1 */
> +		batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */
> +		batch[i++] = obj[0].offset; /* dst address lower bits */
> +		batch[i++] = obj[0].offset >> 32; /* dst address upper bits */
> +		batch[i++] = 0; /* src x1,y1 */
> +		batch[i++] = WIDTH * 4; /* src pitch */

Just wanted to thank you for addressing my comments, even these for such 
minor changes :)

> +		batch[i++] = obj[1].offset; /* src address lower bits */
> +		batch[i++] = obj[1].offset >> 32; /* src address upper bits */
> +		batch[i++] = MI_BATCH_BUFFER_END;
> +		batch[i++] = MI_NOOP;
> +	} else if (blt_has_xy_src_copy(fd)) {
> +		batch[i++] = XY_SRC_COPY_BLT_CMD |
> +			  XY_SRC_COPY_BLT_WRITE_ALPHA |
> +			  XY_SRC_COPY_BLT_WRITE_RGB;
> +		if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> +			batch[i - 1] |= 8;
> +		else
> +			batch[i - 1] |= 6;
> +
> +		batch[i++] = (3 << 24) | /* 32 bits */
> +			  (0xcc << 16) | /* copy ROP */
> +			  WIDTH * 4;
> +		batch[i++] = 0; /* dst x1,y1 */
> +		batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */
> +		batch[i++] = obj[0].offset;
> +		if (intel_gen(devid) >= 8)
> +			batch[i++] = obj[0].offset >> 32;
> +		batch[i++] = 0; /* src x1,y1 */
> +		batch[i++] = WIDTH * 4;
> +		batch[i++] = obj[1].offset;
> +		if (intel_gen(devid) >= 8)
> +			batch[i++] = obj[1].offset >> 32;
> +		batch[i++] = MI_BATCH_BUFFER_END;
> +		batch[i++] = MI_NOOP;
> +	} else
> +		igt_assert_f(0, "No supported blit command found\n");

My comment on the brackets applies here as well.

So, let's wait for the CI results (or check this and 3/4 locally), and 
if everything is fine and brackets are balanced, I can r-b these patches.

Many thanks,
Karolina

>   
>   	gem_write(fd, obj[2].handle, 0, batch, i * sizeof(batch[0]));
>