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

[Zbigniew Kempczyński]

On Mon, May 12, 2025 at 09:48:35AM +0200, Francois Dugast wrote:
> 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.

This depends how library is used. I've noticed that in render-copy
stress scenarios - if during xe_spin_init_opts() we assert pthread
won't be unlocked and we stuck in main thread which just waits for
unlocking the mutex. I sometimes use assertions in library code
(if arguments won't allow to work code properly or just ends with
sigsegv). So I would like to keep code without this comment -
it might be confusing and I think proper review is better for catching
such cases.

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

Hmm, that's not bad idea. Soon or later we will need to fix the code
but at least spinner should work fine with this mem-copy code.

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

Thanks for the review, but I had to extend the series so I'll ask
you for the review one more time.

--
Zbigniew

> 
> > -- 
> > 2.43.0
> >