[igt-dev] [PATCH i-g-t 1/4] lib/intel_batchbuffer: Enable XY_FAST_COPY_BLT support for api_intel_bb

[Kamil Konieczny]

Hi Vikas,

On 2023-03-24 at 19:03:43 +0530, Vikas Srivastava wrote:
> 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.
> 

Put here Cc like:

Cc: Karolina Stolarek <karolina.stolarek at intel.com>
> Signed-off-by: Vikas Srivastava <vikas.srivastava at intel.com>
> ---
>  lib/intel_batchbuffer.c | 63 +++++++++++++++++++++++++++--------------
>  1 file changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index da4c238cae..b1eff42fcb 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -2407,11 +2407,17 @@ uint32_t intel_bb_copy_data(struct intel_bb *ibb,
>   */
>  void intel_bb_blit_start(struct intel_bb *ibb, uint32_t flags)
>  {
> -	intel_bb_out(ibb, XY_SRC_COPY_BLT_CMD |
> -		     XY_SRC_COPY_BLT_WRITE_ALPHA |
> -		     XY_SRC_COPY_BLT_WRITE_RGB |
> -		     flags |
> -		     (6 + 2 * (ibb->gen >= 8)));
> +	if (blt_has_fast_copy(ibb->i915))
> +		intel_bb_out(ibb, XY_FAST_COPY_BLT | flags);
> +	else if (blt_has_xy_src_copy(ibb->i915))
> +
> +		intel_bb_out(ibb, XY_SRC_COPY_BLT_CMD |
> +			     XY_SRC_COPY_BLT_WRITE_ALPHA |
> +			     XY_SRC_COPY_BLT_WRITE_RGB |
> +			     flags |
> +			     (6 + 2 * (ibb->gen >= 8)));
> +	else
> +		igt_assert_f(0, "No supported blit command found\n");
>  }
>  
>  /*
> @@ -2449,12 +2455,22 @@ void intel_bb_emit_blt_copy(struct intel_bb *ibb,
>  
>  	if (gen >= 4 && src->tiling != I915_TILING_NONE) {
>  		src_pitch /= 4;
> -		cmd_bits |= XY_SRC_COPY_BLT_SRC_TILED;
> +		if (blt_has_fast_copy(ibb->i915))
> +			cmd_bits |= fast_copy_dword0(src->tiling, dst->tiling);
> +		else if (blt_has_xy_src_copy(ibb->i915))
> +			cmd_bits |= XY_SRC_COPY_BLT_SRC_TILED;
> +		else
> +			igt_assert_f(0, "No supported blit command found\n");
>  	}
>  
>  	if (gen >= 4 && dst->tiling != I915_TILING_NONE) {
>  		dst_pitch /= 4;
> -		cmd_bits |= XY_SRC_COPY_BLT_DST_TILED;
> +		if (blt_has_fast_copy(ibb->i915))
> +			cmd_bits |= fast_copy_dword0(src->tiling, dst->tiling);
> +		else if (blt_has_xy_src_copy(ibb->i915))
-------------------- ^
Here you can use else without if() as it was already checked above.

> +			cmd_bits |= XY_SRC_COPY_BLT_DST_TILED;
> +		else
> +			igt_assert_f(0, "No supported blit command found\n");

This was ruled out with SRC_TILED above so no need to repeat
"else igt_assert" here.

>  	}
>  
>  	CHECK_RANGE(src_x1); CHECK_RANGE(src_y1);
> @@ -2465,20 +2481,25 @@ void intel_bb_emit_blt_copy(struct intel_bb *ibb,
>  	CHECK_RANGE(src_pitch); CHECK_RANGE(dst_pitch);
>  
>  	br13_bits = 0;
> -	switch (bpp) {
> -	case 8:
> -		break;
> -	case 16:		/* supporting only RGB565, not ARGB1555 */
> -		br13_bits |= 1 << 24;
> -		break;
> -	case 32:
> -		br13_bits |= 3 << 24;
> -		cmd_bits |= (XY_SRC_COPY_BLT_WRITE_ALPHA |
> -			     XY_SRC_COPY_BLT_WRITE_RGB);
> -		break;
> -	default:
> -		igt_fail(IGT_EXIT_FAILURE);
> -	}
> +	if (blt_has_xy_src_copy(ibb->i915)) {

The instruction for dword0 and dword1 should be of the same kind,
so if you will use fast_copy if it is supported then it should
be used for both dw0 and dw1 as Karolina observed.

> +		switch (bpp) {
> +		case 8:
> +			break;
> +		case 16:		/* supporting only RGB565, not ARGB1555 */
> +			br13_bits |= 1 << 24;
> +			break;
> +		case 32:
> +			br13_bits |= 3 << 24;
> +			cmd_bits |= (XY_SRC_COPY_BLT_WRITE_ALPHA |
> +				     XY_SRC_COPY_BLT_WRITE_RGB);
> +			break;
> +		default:
> +			igt_fail(IGT_EXIT_FAILURE);
> +		}
> +	} else if (blt_has_fast_copy(ibb->i915)) {
-------------- ^
Here you can use else without if().

> +		br13_bits = fast_copy_dword1(ibb->i915, src->tiling, dst->tiling, bpp);
> +	} else
> +		igt_assert_f(0, "No supported blit command found\n");

Same here, no need for that final else.

Regards,
Kamil

>  
>  	if ((src->tiling | dst->tiling) >= I915_TILING_Y) {
>  		intel_bb_out(ibb, MI_LOAD_REGISTER_IMM(1));
> -- 
> 2.25.1
>