Re: [PATCH 2/3] lib/xe/xe_spin: Add option for QUEUE_TIMESTAMP

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

On Thu, 2026-05-07 at 12:41 -0700, Umesh Nerlige Ramappa wrote:
> On Thu, May 07, 2026 at 07:23:50PM +0000, Summers, Stuart wrote:
> > On Wed, 2026-05-06 at 11:31 -0700, Niranjana Vishwanathapura wrote:
> > > From: gta <gta@DUT4637NVLP.fm.intel.com>
> > > 
> > > In multi-queue case, CTX_TIMESTAMP register does not
> > > provide the running time of individual queues of a group.
> > > Provide the option for spinner to read timestamp from the
> > > QUEUE_TIMESTAMP registers instead which is useful in the
> > > multi-queue scenarios.
> > > 
> > > Signed-off-by: Niranjana Vishwanathapura
> > > <niranjana.vishwanathapura@intel.com>
> > > ---
> > >  lib/xe/xe_spin.c | 9 +++++----
> > >  lib/xe/xe_spin.h | 2 ++
> > >  2 files changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/lib/xe/xe_spin.c b/lib/xe/xe_spin.c
> > > index 14952ca90e..8743107898 100644
> > > --- a/lib/xe/xe_spin.c
> > > +++ b/lib/xe/xe_spin.c
> > > @@ -25,7 +25,8 @@
> > >  #define MI_LRI_CS_MMIO                         (1 << 19)
> > >  #define MI_LRR_DST_CS_MMIO                     (1 << 19)
> > >  #define MI_LRR_SRC_CS_MMIO                     (1 << 18)
> > > -#define CTX_TIMESTAMP 0x3a8
> > > +#define CTX_TIMESTAMP          0x3a8
> > > +#define QUEUE_TIMESTAMP                0x4c0
> > >  #define CS_GPR(x) (0x600 + 8 * (x))
> > >  
> > >  enum { START_TS, NOW_TS };
> > > @@ -67,7 +68,7 @@ void xe_spin_init(struct xe_spin *spin, struct
> > > xe_spin_opts *opts)
> > >                 spin->batch[b++] = CS_GPR(START_TS) + 4;
> > >                 spin->batch[b++] = 0;
> > >                 spin->batch[b++] = MI_LOAD_REGISTER_REG |
> > > MI_LRR_DST_CS_MMIO | MI_LRR_SRC_CS_MMIO;
> > > -               spin->batch[b++] = CTX_TIMESTAMP;
> > > +               spin->batch[b++] = opts->use_queue_timestamp ?
> > > QUEUE_TIMESTAMP : CTX_TIMESTAMP;
> > 
> > I see in [1] we are programming this register in the ring also.
> > Will
> > this cause issues if we overwrite this here? I see we do the same
> > thing
> > for the context timestamp, so probably no issue here. But it would
> > be
> > nice to document somehow the expected interaction between the
> > kernel
> > ring programming and the user batch programming if any.
> > 
> > [1]:
> > https://patchwork.freedesktop.org/patch/723502/?series=164654&rev=5
> 
> The MI_LOAD_REGISTER_REG source is TIMESTAMP register and destination
> is 
> the GPR reg, so we should be fine. If we were writing to the
> TIMESTAMP 
> registers, then that's going to mess up counts.

Ok got it. So basically we're only ever reading the timestamp and using
the GPR to update for the test specifically. Yeah makes sense.

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

One other thought I have is if we could make use of both the context
and multi queue timestamps together in some cases - like if we want to
test interaction between context switch and lite restore. Right now we
leave that kind of testing to the UMD side and don't cover in IGT. So
we don't really have any need to do that in the IGT library. And anyway
it would take more than just this one change to implement something
like that. But something that might be interesting to consider for the
future...

-Stuart

> 
> Regards,
> Umesh
> 
> 
> > 
> > >                 spin->batch[b++] = CS_GPR(START_TS);
> > >         }
> > >  
> > > @@ -83,7 +84,7 @@ void xe_spin_init(struct xe_spin *spin, struct
> > > xe_spin_opts *opts)
> > >  
> > >         if (opts->write_timestamp) {
> > >                 spin->batch[b++] = MI_LOAD_REGISTER_REG |
> > > MI_LRR_DST_CS_MMIO | MI_LRR_SRC_CS_MMIO;
> > > -               spin->batch[b++] = CTX_TIMESTAMP;
> > > +               spin->batch[b++] = opts->use_queue_timestamp ?
> > > QUEUE_TIMESTAMP : CTX_TIMESTAMP;
> > >                 spin->batch[b++] = CS_GPR(NOW_TS);
> > >  
> > >                 spin->batch[b++] = MI_STORE_REGISTER_MEM_GEN8 |
> > > MI_SRM_CS_MMIO;
> > > @@ -97,7 +98,7 @@ void xe_spin_init(struct xe_spin *spin, struct
> > > xe_spin_opts *opts)
> > >                 spin->batch[b++] = CS_GPR(NOW_TS) + 4;
> > >                 spin->batch[b++] = 0;
> > >                 spin->batch[b++] = MI_LOAD_REGISTER_REG |
> > > MI_LRR_DST_CS_MMIO | MI_LRR_SRC_CS_MMIO;
> > > -               spin->batch[b++] = CTX_TIMESTAMP;
> > > +               spin->batch[b++] = opts->use_queue_timestamp ?
> > > QUEUE_TIMESTAMP : CTX_TIMESTAMP;
> > >                 spin->batch[b++] = CS_GPR(NOW_TS);
> > >  
> > >                 /* delta = now - start; inverted to match
> > > COND_BBE */
> > > diff --git a/lib/xe/xe_spin.h b/lib/xe/xe_spin.h
> > > index db0febd8ab..75a6ada420 100644
> > > --- a/lib/xe/xe_spin.h
> > > +++ b/lib/xe/xe_spin.h
> > > @@ -51,6 +51,7 @@ struct xe_spin_mem_copy {
> > >   * @multi_queue_switch_on_wait: Add a SEMAPHORE_WAIT multi-queue
> > > switch point
> > >   * and have the queue switch only happen if waiting on the
> > > semaphore.
> > >   * @ctx_ticks: number of ticks after which spinner is stopped,
> > > applied if > 0
> > > + * @use_queue_timestamp: Use QUEUE_TIMESTAMP register instead of
> > > CTX_TIMESTAMP
> > >   * @mem_copy: container of objects used for memory copy
> > > (optional)
> > >   *
> > >   * Used to initialize struct xe_spin spinner behavior.
> > > @@ -62,6 +63,7 @@ struct xe_spin_opts {
> > >         bool multi_queue_switch_on_wait;
> > >         uint32_t ctx_ticks;
> > >         bool write_timestamp;
> > > +       bool use_queue_timestamp;
> > >         struct xe_spin_mem_copy *mem_copy;
> > >  };
> > >  
> >