[PATCH] lib/xe/xe_spin: Add additional MI_ARB_CHECK to preemptable spin init

[Zbigniew Kempczyński]

On Thu, Aug 14, 2025 at 12:24:11PM -0700, Matthew Brost wrote:
> On Thu, Aug 14, 2025 at 01:11:39PM -0600, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Brost, Matthew <matthew.brost at intel.com> 
> > Sent: Thursday, August 14, 2025 12:05 PM
> > To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
> > Cc: igt-dev at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>; Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>
> > Subject: Re: [PATCH] lib/xe/xe_spin: Add additional MI_ARB_CHECK to preemptable spin init
> > > 
> > > On Thu, Aug 14, 2025 at 06:50:10PM +0000, Jonathan Cavitt wrote:
> > > > Add an additional MI_ARB_CHECK to the spin initialization function,
> > > > just before the conditional batch buffer end, when preemption is
> > > > enabled.  The MI_ARB_CHECK forces a preemption in this case, which can
> > > > allow additionally scheduled exec queues to make independent progress
> > > > earlier than if we relied on the automatic timeout of the exec queue.
> > > > 
> > > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/5783
> > > > Suggested-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > > > Cc: Matthew Brost <matthew.brost at intel.com>
> > > > Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
> > > > ---
> > > >  lib/xe/xe_spin.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/xe/xe_spin.c b/lib/xe/xe_spin.c
> > > > index 25f7555e27..b3a937fe80 100644
> > > > --- a/lib/xe/xe_spin.c
> > > > +++ b/lib/xe/xe_spin.c
> > > > @@ -30,7 +30,6 @@
> > > >  
> > > >  enum { START_TS, NOW_TS };
> > > >  
> > > > -
> > > >  uint32_t xe_spin_nsec_to_ticks(int fd, int gt_id, uint64_t nsec)
> > > >  {
> > > >  	uint32_t ticks = xe_nsec_to_ticks(fd, gt_id, nsec);
> > > > @@ -167,6 +166,9 @@ void xe_spin_init(struct xe_spin *spin, struct xe_spin_opts *opts)
> > > >  				   opts->mem_copy->dst->mocs_index;
> > > >  	}
> > > >  
> > > > +	if (opts->preempt)
> > > > +		spin->batch[b++] = MI_ARB_CHECK;
> > > 
> > > But we already have one these at the top of the loop:
> > > 
> > >  72         loop_addr = opts->addr + b * sizeof(uint32_t);
> > >  73
> > >  74         spin->batch[b++] = MI_STORE_DWORD_IMM_GEN4;
> > >  75         spin->batch[b++] = start_addr;
> > >  76         spin->batch[b++] = start_addr >> 32;
> > >  77         spin->batch[b++] = 0xc0ffee;
> > >  78
> > >  79         if (opts->preempt)
> > >  80                 spin->batch[b++] = MI_ARB_CHECK;
> > > 
> > > Line 74 is here the ring loop restarts.
> > 
> > I tried removing the top MI_ARB_CHECK, but that caused the test to start
> > failing again, so it looks like both MI_ARB_CHECK calls are necessary.
> > -Jonathan Cavitt
> 
> It shouldn’t be, though. A couple of instructions later we should jump
> to loop_addr (that is line 73 ring instruction) and then execute the
> MI_ARB_CHECK programmed in the ring at line 80.
> 
> I’m all for fixing the tests, but I want to understand what’s going on
> here.

We (me and Sai) noticed that adding additional MI_ARB_CHECK increases
the preemption probability. I totally agree we already have one MI_ARB_CHECK
in a loop but from some unknown reason even it if is run in a loop it
mostly has no effect. Why I claim there's no effect? We tried to
increase preempt_timeout_us to 2000000, and preemption still didn't
happened during this period.

--
Zbigniew

> 
> Can you reliably reproduce this issue and confirm that it fails before
> this change and works after?
> 
> Matt
> 
> > 
> > > 
> > > Matt
> > > 
> > > > +
> > > >  	spin->batch[b++] = MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2;
> > > >  	spin->batch[b++] = 0;
> > > >  	spin->batch[b++] = end_addr;
> > > > -- 
> > > > 2.43.0
> > > > 
> > >