Re: [PATCH 1/2] tests/intel/xe_exec_multi_queue: use timestamp to check job start

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

On Wed, 2026-04-29 at 19:18 +0000, Summers, Stuart wrote:
> On Tue, 2026-04-28 at 19:08 -0700, Niranjana Vishwanathapura wrote:
> > In __test_priority(), enable write_timestamp in xe_spin_init_opts()
> > and use timestamp value to determine the queue switch order. This
> > replaces the indeterminate sleep() with a more deterministic wait
> > based on the GPU timestamp.
> > 
> > Pre-set all spinners to preempt-wait before submission so each
> > queue, once scheduled by HW, blocks at the semaphore. This ensures
> > all queues are running on HW before testing priority-based
> > scheduling.
> > 
> > Assisted-by: GitHub Copilot:claude-sonnet-4.6
> > Signed-off-by: Niranjana Vishwanathapura
> > <niranjana.vishwanathapura@intel.com>
> > ---
> >  tests/intel/xe_exec_multi_queue.c | 51 ++++++++++++++++++++++-----
> > --
> > --
> >  1 file changed, 37 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tests/intel/xe_exec_multi_queue.c
> > b/tests/intel/xe_exec_multi_queue.c
> > index ca96099d36..382705d065 100644
> > --- a/tests/intel/xe_exec_multi_queue.c
> > +++ b/tests/intel/xe_exec_multi_queue.c
> > @@ -454,25 +454,24 @@ __test_priority(int fd, struct
> > drm_xe_engine_class_instance *eci,
> >         for (i = 0; i < num_queues; i++) {
> >                 uint64_t spin_addr = addr + i * sizeof(struct
> > xe_spin);
> >  
> > -               xe_spin_init_opts(spin[i], .addr = spin_addr,
> > .multi_queue_switch = true);
> > +               xe_spin_init_opts(spin[i], .addr = spin_addr,
> > .multi_queue_switch = true,
> > +                                 .write_timestamp = true);
> > +               /*
> > +                * Pre-set all spinners to preempt-wait so each
> > queue, once
> > +                * scheduled, immediately blocks at the
> > QUEUE_SWITCH_MODE semaphore
> > +                * after writing its timestamp. The HW switches
> > between queues at
> > +                * this point, allowing all of them to schedule
> > deterministically.
> > +                */
> > +               xe_spin_preempt_wait(spin[i]);
> >                 sync.addr = spin_addr + (char *)&spin[i]->exec_sync
> > -
> > (char *)spin[i];
> >                 exec.exec_queue_id = exec_queues[i];
> >                 exec.address = spin_addr;
> >                 xe_exec(fd, &exec);
> > -
> > -               /* Wait for job on Q0 to start, other queues block
> > behind Q0 */
> > -               if (!i)
> > -                       xe_spin_wait_started(spin[i]);
> >         }
> >  
> > -       sleep(sleep_duration);
> > -
> > -       /*
> > -        * Expect the job on other queue to not get scheduled while
> > the spinner
> > -        * on q0 is not waiting on preempt condition.
> > -        */
> > -       for (i = 1; i < num_queues; i++)
> > -               igt_assert(!xe_spin_started(spin[i]));
> > +       /* Wait for all queues to start */
> > +       for (i = 0; i < num_queues; i++)
> > +               xe_spin_wait_started(spin[i]);
> 
> So I see in the spinner batch that the c0ffee value is written before
> the timestamp... should we change that order in the batch to account
> for this scenario? And should we have a check before we go into the
> for
> loop to clear the timestamp below that all of the timestamps are non-
> zero first?
> 
> I'm wondering if we can hit some race condition where we mark
> everything as started, but some of the queues (well.. at least one of
> hte queues) hasn't actually hit the semaphore wait yet.
> 
> That's the only issue I see here really, everything else looks ok to
> me. And maybe I'm just being paranoid about the case above, just
> trying
> to see where we could have some hole here if things are running
> sufficiently slow for some reason.

And it could also be, of course, that the timestamp gets written but
the semaphore wait hasn't been seen yet... so maybe in addition to the
non-zero check, we should make sure it increments at least once
somehow?

-Stuart

> 
> Thanks,
> Stuart
> 
> >  
> >         if (flags & DYN_PRIORITY) {
> >                 /* Assign increasing order of priority for
> > secondary
> > queues */
> > @@ -485,6 +484,30 @@ __test_priority(int fd, struct
> > drm_xe_engine_class_instance *eci,
> >                 sleep(sleep_duration);
> >         }
> >  
> > +       /*
> > +        * Clear timestamps and release all queues from the
> > semaphore
> > wait.
> > +        * The order in which they next write a timestamp reveals
> > the
> > +        * priority-based scheduling order.
> > +        */
> > +       for (i = 0; i < num_queues; i++) {
> > +               WRITE_ONCE(spin[i]->timestamp, 0);
> > +               xe_spin_preempt_nowait(spin[i]);
> > +
> > +               /*
> > +                * For Q0, wait until it is running again to ensure
> > it holds the engine
> > +                * when priority arbitration is triggered.
> > +                */
> > +               if (!i)
> > +                       while (!READ_ONCE(spin[i]->timestamp));
> > +       }
> > +
> > +       /*
> > +        * Verify that secondary queues have not been scheduled
> > while
> > Q0
> > +        * holds the engine.
> > +        */
> > +       for (i = 1; i < num_queues; i++)
> > +               igt_assert(!READ_ONCE(spin[i]->timestamp));
> > +
> >         /*
> >          * Trigger a queue switch by making the spinner on q0 to
> > wait
> > on preempt
> >          * condition, allowing job on q1 to get scheduled and
> > finish.
> > When we end
> > @@ -501,7 +524,7 @@ __test_priority(int fd, struct
> > drm_xe_engine_class_instance *eci,
> >         i = 1;
> >         while (i < num_queues) {
> >                 for (j = 1; j < num_queues; j++) {
> > -                       if (xe_spin_started(spin[j]) &&
> > ((already_in_order & (1 << j)) == 0)) {
> > +                       if (READ_ONCE(spin[j]->timestamp) &&
> > ((already_in_order & (1 << j)) == 0)) {
> >                                 start_order[i] = j;
> >                                 xe_spin_end(spin[j]);
> >                                 xe_wait_ufence(fd, &spin[j]-
> > > exec_sync, USER_FENCE_VALUE,
>