[PATCH i-g-t 1/2] lib/xe_spin: limit width/pitch for mem-copy

[Francois Dugast]

Hi,

On Fri, May 09, 2025 at 09:46:44AM +0200, Zbigniew Kempczyński wrote:
> Mem-copy for byte copy requires width/pitch to be max 256K
> (higher bits have different meaning and setting them might lead
> to undefined behavior). If witch/pitch are too high limit them to
> the maximum possible for the operation. Asserting might lead to
> test stucking in multithreading scenarios so displaying warning
> was selected instead.

Might be good to add this as a comment in the code to prevent someone
from replacing igt_warn() with igt_assert() in the future.

> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Cc: Francois Dugast <francois.dugast at intel.com>
> ---
>  lib/xe/xe_spin.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/xe/xe_spin.c b/lib/xe/xe_spin.c
> index a92903b6bd..7ff06cab3b 100644
> --- a/lib/xe/xe_spin.c
> +++ b/lib/xe/xe_spin.c
> @@ -126,11 +126,36 @@ void xe_spin_init(struct xe_spin *spin, struct xe_spin_opts *opts)
>  	}
>  
>  	if (opts->mem_copy) {
> +		uint32_t src_width, src_pitch, dst_width, dst_pitch;
> +
> +		src_width = opts->mem_copy->src->width;
> +		src_pitch = opts->mem_copy->src->pitch;
> +		dst_width = opts->mem_copy->dst->width;
> +		dst_pitch = opts->mem_copy->dst->pitch;
> +
> +		if (src_width > dst_width)
> +			igt_warn("src width must be <= dst width\n");

Should we not also override the value in that case?

    src_width = dst_width

> +
> +		if (src_width > SZ_256K) {
> +			igt_warn("src width must be less than 256K, limiting it\n");
> +			src_width = SZ_256K;
> +		}
> +
> +		if (src_pitch > SZ_256K) {
> +			igt_warn("src pitch must be less than 256K, limiting it\n");
> +			src_pitch = SZ_256K;
> +		}
> +
> +		if (dst_pitch > SZ_256K) {
> +			igt_warn("dst pitch must be less than 256K, limiting it\n");
> +			dst_pitch = SZ_256K;
> +		}
> +
>  		spin->batch[b++] = MEM_COPY_CMD;
> -		spin->batch[b++] = opts->mem_copy->src->width - 1;
> -		spin->batch[b++] = opts->mem_copy->src->height - 1;
> -		spin->batch[b++] = opts->mem_copy->src->pitch - 1;
> -		spin->batch[b++] = opts->mem_copy->dst->pitch - 1;
> +		spin->batch[b++] = src_width - 1;
> +		spin->batch[b++] = 1;  /* for byte copying this is ignored */
> +		spin->batch[b++] = src_pitch - 1;
> +		spin->batch[b++] = dst_pitch - 1;
>  		spin->batch[b++] = opts->mem_copy->src_offset;
>  		spin->batch[b++] = opts->mem_copy->src_offset << 32;
>  		spin->batch[b++] = opts->mem_copy->dst_offset;

LGTM, let's wait to see the CI results for v2 with the fix for xe_spin_batch:

    Reviewed-by: Francois Dugast <francois.dugast at intel.com>

Francois

> -- 
> 2.43.0
>