Re: [PATCH 1/3] lib/xe/xe_spin: Enhance multi-queue switch option

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

On Thu, May 07, 2026 at 12:15:10PM -0700, Summers, Stuart wrote:
>On Wed, 2026-05-06 at 11:31 -0700, Niranjana Vishwanathapura wrote:
>> From: gta <gta@DUT4637NVLP.fm.intel.com>
>>
>> Allow user to control whether the multi-queue switch should
>> happen after parsing the MI_SEMAPHORE_WAIT instruction or
>> only if the instruction is unsuccessful in getting the
>> semaphore, thus having to wait.
>>
>> Ensure that in xe_exec_multi_queue@priority test, the multi-queue
>> switch happens only when the spinner has to wait for the semaphore.
>>
>> Signed-off-by: Niranjana Vishwanathapura
>> <niranjana.vishwanathapura@intel.com>
>> ---
>>  lib/xe/xe_spin.c                  | 13 +++++++------
>>  lib/xe/xe_spin.h                  |  6 +++++-
>>  tests/intel/xe_exec_multi_queue.c |  2 +-
>>  3 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/xe/xe_spin.c b/lib/xe/xe_spin.c
>> index 4dc110c222..14952ca90e 100644
>> --- a/lib/xe/xe_spin.c
>> +++ b/lib/xe/xe_spin.c
>> @@ -179,14 +179,15 @@ void xe_spin_init(struct xe_spin *spin, struct
>> xe_spin_opts *opts)
>>          * Insert a MI_SEMAPHORE_WAIT_CMD instruction with condition
>> controlled
>>          * by the user which acts as a queue switch point in multi
>> queue mode.
>>          */
>> -       if (opts->multi_queue_switch) {
>> +       if (opts->multi_queue_switch || opts-
>> >multi_queue_switch_on_wait) {
>
>I don't have any problem with the way you're doing this. I can see we
>might want to use this without multi queue though at some point, so it
>might be nice to have the switch_on_wait as an option on top of
>multi_queue_switch - basically rename multi_queue_switch to
>semaphore_wait or something and then add the multi_queue_switch_on_wait
>as an additional option. But that also means we have to keep track of a
>separate parameter in xe_spin_init_opts() and since multi queue is the
>only user right now, it might be overkill.
>

Note that we use the bit MI_SEMAPHORE_QUEUE_SWITCH_MODE to indicate
whether multi-queue switch should happen on wait or after parsing
the command. It is a multi-queue specific bit. I don't see if that
option is available for non-multi-queue case. Yah, we can always
revisit if we get that option for non-multi-queue case also.

>Otherwise LGTM. Please apply the author related changes Xin had
>requested before merging:
>Reviewed-by: Stuart Summers <stuart.summers@intel.com>

Sure, Thanks,
Niranjana

>
>>                 uint64_t wait_addr = opts->addr + offsetof(struct
>> xe_spin, wait_cond);
>> +               uint32_t sema_cmd = MI_SEMAPHORE_WAIT_CMD |
>> MI_SEMAPHORE_POLL |
>> +                                   MI_SEMAPHORE_SAD_EQ_SDD | 3;
>>  
>> -               spin->batch[b++] = MI_SEMAPHORE_WAIT_CMD |
>> -                               MI_SEMAPHORE_POLL |
>> -                               MI_SEMAPHORE_QUEUE_SWITCH_MODE |
>> -                               MI_SEMAPHORE_SAD_EQ_SDD |
>> -                               3;
>> +               if (opts->multi_queue_switch_on_wait)
>> +                       sema_cmd |= MI_SEMAPHORE_QUEUE_SWITCH_MODE;
>> +
>> +               spin->batch[b++] = sema_cmd;
>>                 spin->batch[b++] = 0;
>>                 spin->batch[b++] = wait_addr;
>>                 spin->batch[b++] = wait_addr >> 32;
>> diff --git a/lib/xe/xe_spin.h b/lib/xe/xe_spin.h
>> index 31154997b9..db0febd8ab 100644
>> --- a/lib/xe/xe_spin.h
>> +++ b/lib/xe/xe_spin.h
>> @@ -46,7 +46,10 @@ struct xe_spin_mem_copy {
>>   * struct xe_spin_opts
>>   * @addr: offset of spinner within vm
>>   * @preempt: allow spinner to be preempted or not
>> - * @multi_queue_switch: Add a multi-queue switch point
>> + * @multi_queue_switch: Add a SEMAPHORE_WAIT multi-queue switch
>> point
>> + * and have the queue switch happen after command is parsed.
>> + * @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
>>   * @mem_copy: container of objects used for memory copy (optional)
>>   *
>> @@ -56,6 +59,7 @@ struct xe_spin_opts {
>>         uint64_t addr;
>>         bool preempt;
>>         bool multi_queue_switch;
>> +       bool multi_queue_switch_on_wait;
>>         uint32_t ctx_ticks;
>>         bool write_timestamp;
>>         struct xe_spin_mem_copy *mem_copy;
>> diff --git a/tests/intel/xe_exec_multi_queue.c
>> b/tests/intel/xe_exec_multi_queue.c
>> index 0479554bb6..6f90c3e4e8 100644
>> --- a/tests/intel/xe_exec_multi_queue.c
>> +++ b/tests/intel/xe_exec_multi_queue.c
>> @@ -459,7 +459,7 @@ __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_on_wait = true,
>>                                   .write_timestamp = true);
>>                 /*
>>                  * Pre-set all spinners to preempt-wait so each
>> queue, once
>