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

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Wed, Apr 29, 2026 at 12:24:31PM -0700, Summers, Stuart wrote:
>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.
>

I understand your concern that GPU job might have started, and have
written to start_addr, but not updated the timestamp yet by the time
we allow switching to happen down below. But I am not sure if that
happening is a real concern here as GPU runs much faster than the
test here.

But I will change the code to read the timestamp until it is non-zero
here instead of xe_spin_wait_started(). That will ensure job has run
past writing the timestamp before waiting on semaphore. Claude generated
code like that to begin with but I changed it to use xe_spin_wait_started()
thinking it will be less confusing for the reader.

>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?

That should be fine as we clear the timestamp below and ensure Q0
is running before we allow queue-switch to happen. That ensures all
queues are waiting on the semaphore at that point. All we need is to
ensure Q0 is running and timestamp of all queues are cleared.

Niranjana

>
>-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,
>>
>