Re: [PATCH v2 14/19] lib/xe/xe_spin: Add switch point for preemptible spinner

["Summers, Stuart" <stuart.summers@intel.com>]

On Thu, 2025-12-04 at 13:42 -0800, Niranjana Vishwanathapura wrote:
> On Thu, Dec 04, 2025 at 01:03:25PM -0800, Summers, Stuart wrote:
> > On Thu, 2025-11-20 at 19:57 -0800, Niranjana Vishwanathapura wrote:
> > > When 'preempt' option is specified, insert a MI_SEMAPHORE_WAIT
> > > command in the spinner batch buffer. This provides the switch
> > > point for preemption to happen in multi queue mode.
> > > Allow user to control preempt wait condition.
> > > 
> > > Signed-off-by: Niranjana Vishwanathapura
> > > <niranjana.vishwanathapura@intel.com>
> > > ---
> > >  include/intel_gpu_commands.h |  1 +
> > >  lib/xe/xe_spin.c             | 41
> > > ++++++++++++++++++++++++++++++++++++
> > >  lib/xe/xe_spin.h             |  3 +++
> > >  3 files changed, 45 insertions(+)
> > > 
> > > diff --git a/include/intel_gpu_commands.h
> > > b/include/intel_gpu_commands.h
> > > index cd281ba89..5158bb0ea 100644
> > > --- a/include/intel_gpu_commands.h
> > > +++ b/include/intel_gpu_commands.h
> > > @@ -123,6 +123,7 @@
> > >  #define MI_SEMAPHORE_WAIT      MI_INSTR(0x1c, 2) /* GEN8+ */
> > >  #define MI_SEMAPHORE_WAIT_TOKEN        MI_INSTR(0x1c, 3) /*
> > > GEN12+
> > > */
> > >  #define   MI_SEMAPHORE_POLL            (1 << 15)
> > > +#define   MI_SEMAPHORE_QUEUE_SWITCH_MODE       (1 << 11)
> > >  #define   MI_SEMAPHORE_SAD_GT_SDD      (0 << 12)
> > >  #define   MI_SEMAPHORE_SAD_GTE_SDD     (1 << 12)
> > >  #define   MI_SEMAPHORE_SAD_LT_SDD      (2 << 12)
> > > diff --git a/lib/xe/xe_spin.c b/lib/xe/xe_spin.c
> > > index 72b13ef42..174c5f7ec 100644
> > > --- a/lib/xe/xe_spin.c
> > > +++ b/lib/xe/xe_spin.c
> > > @@ -57,6 +57,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;
> > >  
> > >         if (opts->ctx_ticks) {
> > > @@ -167,6 +168,24 @@ void xe_spin_init(struct xe_spin *spin,
> > > struct
> > > xe_spin_opts *opts)
> > >                                    opts->mem_copy->dst-
> > > >mocs_index;
> > >         }
> > >  
> > > +       /*
> > > +        * Insert a MI_SEMAPHORE_WAIT_CMD instruction with
> > > condition
> > > controlled
> > > +        * by the user. It also acts as a queue switch point in
> > > multi
> > > queue mode.
> > > +        */
> > > +       if (opts->preempt) {
> > > +               uint64_t wait_addr = opts->addr + offsetof(struct
> > > xe_spin, wait_cond);
> > > +
> > > +               spin->batch[b++] = MI_SEMAPHORE_WAIT_CMD |
> > > +                               MI_SEMAPHORE_POLL |
> > > +                               MI_SEMAPHORE_QUEUE_SWITCH_MODE |
> > > +                               MI_SEMAPHORE_SAD_EQ_SDD |
> > > +                               3;
> > > +               spin->batch[b++] = 0;
> > 
> > Should this be set to spin->wait_cond in case we ever want to
> > change
> > the default value here?
> > 
> 
> wait_cond is already part of wait_addr. So, user can control the
> wait using xe_spin_preempt_wait()/nowait() functions below.

Ah right and this is just the value we're using to compare against the
user value. So yeah makes sense what we have.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

> 
> Niranjana
> 
> > Otherwise generally this is a great addition to the spinner
> > toolkit:
> > Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> > 
> > > +               spin->batch[b++] = wait_addr;
> > > +               spin->batch[b++] = wait_addr >> 32;
> > > +               spin->batch[b++] = 0;
> > > +       }
> > > +
> > >         spin->batch[b++] = MI_COND_BATCH_BUFFER_END |
> > > MI_DO_COMPARE |
> > > 2;
> > >         spin->batch[b++] = 0;
> > >         spin->batch[b++] = end_addr;
> > > @@ -207,6 +226,28 @@ void xe_spin_end(struct xe_spin *spin)
> > >         WRITE_ONCE(spin->end, 0);
> > >  }
> > >  
> > > +/**
> > > + * xe_spin_preempt_wait:
> > > + * @spin: pointer to spinner mapped bo
> > > + *
> > > + * Make the spinner wait on the preemption semaphore.
> > > + */
> > > +void xe_spin_preempt_wait(struct xe_spin *spin)
> > > +{
> > > +       spin->wait_cond = 1;
> > > +}
> > > +
> > > +/**
> > > + * xe_spin_preempt_nowait:
> > > + * @spin: pointer to spinner mapped bo
> > > + *
> > > + * Make the spinner do not wait on the preemption semaphore.
> > > + */
> > > +void xe_spin_preempt_nowait(struct xe_spin *spin)
> > > +{
> > > +       spin->wait_cond = 0;
> > > +}
> > > +
> > >  /**
> > >   * xe_spin_create:
> > >   * @opt: controlling options such as allocator handle,
> > > exec_queue,
> > > vm etc
> > > diff --git a/lib/xe/xe_spin.h b/lib/xe/xe_spin.h
> > > index c2199d0aa..eaf2a409e 100644
> > > --- a/lib/xe/xe_spin.h
> > > +++ b/lib/xe/xe_spin.h
> > > @@ -64,6 +64,7 @@ struct xe_spin {
> > >         uint64_t pad;
> > >         uint32_t start;
> > >         uint32_t end;
> > > +       uint32_t wait_cond;
> > >         uint32_t ticks_delta;
> > >         uint64_t exec_sync;
> > >         uint32_t timestamp;
> > > @@ -76,6 +77,8 @@ void xe_spin_init(struct xe_spin *spin, struct
> > > xe_spin_opts *opts);
> > >  bool xe_spin_started(struct xe_spin *spin);
> > >  void xe_spin_wait_started(struct xe_spin *spin);
> > >  void xe_spin_end(struct xe_spin *spin);
> > > +void xe_spin_preempt_wait(struct xe_spin *spin);
> > > +void xe_spin_preempt_nowait(struct xe_spin *spin);
> > >  
> > >  /*
> > >   * xe_cork: higher level API that simplifies exec'ing an xe_spin
> > > by
> > > taking care
> >