Re: [PATCH i-g-t 1/2] lib/xe/xe_spin: initialize ticks_delta to ~0u

On Thu, Jul 30, 2026 at 12:05:10PM +0200, Bernatowicz, Marcin wrote:
> 
> On 7/29/2026 11:42 AM, Zbigniew Kempczyński wrote:
> > On Tue, Jul 28, 2026 at 12:26:58PM +0200, Marcin Bernatowicz wrote:
> > > Initialize ticks_delta to ~0u instead of 0. This value does not satisfy
> > > the COND_BBE exit condition and prevents false batch termination.
> > May you elaborate? Does it mean STORE-DWORD (loop) is not visible in the memory
> > before COND_BBE is executed? IIUC with ~0 we could also drop this loop.

> The issue is not about the STORE-DWORDs (loop) visibility within a single
> batch; that's what the pad is for and that path works fine.

I see two scenarios here:

- loop causes delay to allow write to land in memory, where COND_BBE can
  read it. If there's stale read that means loop is too short.

- we got two competing writes to same memory - one from cpu side
  (spin->ticks_delta = 0) and one from gpu - SRM. If this is possible
  COND_BBE could read cpu write (0 in this case, we don't know which
  will be visible at comparison time) so this causes premature spinner termination.

> The problem is a cross-iteration potential stale cache read: when the same
> BO is re-submitted, COND_BBE can read the ticks_delta value leftover
> from the previous batch execution - which is <= ctx_ticks (since that's the
> exit condition) - and immediately exit before the new SRM write propagates.
> 
> With ticks_delta = 0 (or any value <= ctx_ticks at prev-iteration exit), a
> stale read satisfies COND_BBE.
> With ~0u, even a stale read of the sentinel gives 0xFFFFFFFF <= ctx_ticks ->
> false -> batch continues safely.

I did some experiments, MI_MATH_STOREINV() stores negative value to
memory, so I assumed we're comparing signed values (and then 0 is fine
as it larger than negative opts->ctx_ticks). But this comparison is done
on unsigned values and 0 in ticks_delta is smaller than opts->ctx_ticks
casted to unsigned and spinner is terminating.

I think for sake of clearity instead of initializing ticks_delta to ~0u
I would change MI_MATH_STOREINV() to MI_MATH_STORE(), replace ~opts->ctx_ticks
to opts->ctx_ticks and add MAD_LT_IDD in COND_BBE.

--
Zbigniew

> 
> To reproduce: each gem_wsim invocation does 4000 submit/wait cycles on the
> same BO (5 engines × 5ms each).
> The failure appears only after many outer iterations under sustained load:
> 
> i=1; while true; do
>     echo "Iteration: $i"
>     gem_wsim -w
> "1.RCS.5000.0.0,1.BCS.5000.0.0,1.CCS.5000.0.1,1.VCS.5000.0.0,1.VECS.5000.0.0"
> -VV -r 4000 || break
>     ((i++))
> done
> 
> Typical failure after ~60 outer iterations:
> 
> CRITICAL: Failed assertion: w->duration.requested_ticks <=
> ~w->xe.data->spin.ticks_delta
> CRITICAL: error: 96000 > 2 (sometimes 0, 1)
> 
> With the sentinel reset before each submission, ran two days without
> failure.
> 
> --
> 
> Marcin
> 
> > 
> > --
> > Zbigniew
> > 
> > > Signed-off-by: Marcin Bernatowicz <[email protected]>
> > > Cc: Adam Miszczak <[email protected]>
> > > Cc: Kamil Konieczny <[email protected]>
> > > Cc: Lukasz Laguna <[email protected]>
> > > ---
> > >   lib/xe/xe_spin.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/xe/xe_spin.c b/lib/xe/xe_spin.c
> > > index 874310789..331bed1d2 100644
> > > --- a/lib/xe/xe_spin.c
> > > +++ b/lib/xe/xe_spin.c
> > > @@ -60,7 +60,7 @@ void xe_spin_init(struct xe_spin *spin, struct xe_spin_opts *opts)
> > >   	spin->start = 0;
> > >   	spin->end = 0xffffffff;
> > >   	spin->wait_cond = 0;
> > > -	spin->ticks_delta = 0;
> > > +	spin->ticks_delta = ~0u;
> > >   	if (opts->ctx_ticks) {
> > >   		/* store start timestamp */
> > > -- 
> > > 2.43.0
> > >