Re: [PATCH 2/2] tests/intel/xe_exec_multi_queue: replace sleep with barrier queue

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

On Wed, Apr 29, 2026 at 12:28:09PM -0700, Summers, Stuart wrote:
>On Tue, 2026-04-28 at 19:08 -0700, Niranjana Vishwanathapura wrote:
>> In __test_priority() DYN_PRIORITY case, replace sleep() with a
>> deterministic barrier using an extra queue in the same multi-queue
>> group. After assigning priorities, submit a spinner to the extra
>> queue, end it immediately and wait for its user fence to signal.
>> This guarantees a full scheduler round-trip confirming the priority
>> updates have taken effect before releasing the other queues.
>>
>> Increase exec_queues[] and spin[] array sizes by 1 to accommodate
>> the extra barrier queue slot at index num_queues.
>>
>> Assisted-by: GitHub Copilot:claude-sonnet-4.6
>> Signed-off-by: Niranjana Vishwanathapura
>> <niranjana.vishwanathapura@intel.com>
>> ---
>>  tests/intel/xe_exec_multi_queue.c | 37 +++++++++++++++++++++++++----
>> --
>>  1 file changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/intel/xe_exec_multi_queue.c
>> b/tests/intel/xe_exec_multi_queue.c
>> index 382705d065..8c6fbb2d18 100644
>> --- a/tests/intel/xe_exec_multi_queue.c
>> +++ b/tests/intel/xe_exec_multi_queue.c
>> @@ -381,8 +381,8 @@ __test_priority(int fd, struct
>> drm_xe_engine_class_instance *eci,
>>                 .syncs = to_user_pointer(&sync),
>>         };
>>         uint64_t vm_sync = 0, addr = BASE_ADDRESS;
>> -       uint32_t exec_queues[XE_EXEC_QUEUE_PRIORITY_N];
>> -       struct xe_spin *spin[XE_EXEC_QUEUE_PRIORITY_N];
>> +       uint32_t exec_queues[XE_EXEC_QUEUE_PRIORITY_N + 1];
>> +       struct xe_spin *spin[XE_EXEC_QUEUE_PRIORITY_N + 1];
>>         uint32_t vm, num_queues, num_queue_priorities, bo = 0;
>>         uint32_t start_order[XE_EXEC_QUEUE_PRIORITY_N] = { 0 };
>>         int64_t fence_timeout = NSEC_PER_SEC;
>> @@ -403,7 +403,7 @@ __test_priority(int fd, struct
>> drm_xe_engine_class_instance *eci,
>>                 .value = DRM_XE_MULTI_GROUP_CREATE,
>>         };
>>         uint64_t ext = to_user_pointer(&multi_queue);
>> -       int i, j, sleep_duration = 1;
>> +       int i, j;
>>         void *bo_map;
>>  
>>         num_queue_priorities = XE_EXEC_QUEUE_NUM_PRIORITIES;
>> @@ -415,12 +415,12 @@ __test_priority(int fd, struct
>> drm_xe_engine_class_instance *eci,
>>                  eci[0].engine_class, eci[0].engine_instance);
>>  
>>         vm = xe_vm_create(fd, DRM_XE_VM_CREATE_FLAG_LR_MODE, 0);
>> -       bo_size = xe_bb_size(fd, sizeof(*spin[0]) * num_queues);
>> +       bo_size = xe_bb_size(fd, sizeof(*spin[0]) * (num_queues +
>> 1));
>>  
>>         bo = xe_bo_create(fd, vm, bo_size, vram_if_possible(fd,
>> eci[0].gt_id),
>>                           DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
>>         bo_map = xe_bo_map(fd, bo, bo_size);
>> -       for (i = 0; i < num_queues; i++)
>> +       for (i = 0; i < num_queues + 1; i++)
>>                 spin[i] = bo_map + i * sizeof(*spin[0]);
>>  
>>         /* Use the default priority for Q0 because we are explicitly
>> waiting for it below */
>> @@ -430,6 +430,11 @@ __test_priority(int fd, struct
>> drm_xe_engine_class_instance *eci,
>>         if (flags & DYN_PRIORITY) {
>>                 for (i = 1; i < num_queues; i++)
>>                         exec_queues[i] = xe_exec_queue_create(fd, vm,
>> eci, ext);
>> +               /*
>> +                * Create an extra queue in the same multi-queue
>> group, used as
>> +                * a barrier to confirm priority updates have taken
>> effect.
>> +                */
>> +               exec_queues[num_queues] = xe_exec_queue_create(fd,
>> vm, eci, ext);
>
>Sorry for the multiple responses here...
>
>I realize you're doing this separate line explicitly so it's clear what
>and why, etc, but we're really just duplicating code here when we could
>have a num_queues + 1 in the for loop here. The comment here is the
>interesting part that will let us know in the future why we have that
>extra one (in addition to the commit message of course).
>
>Not a blocker, but I'd prefer to not change the inner portion of the
>for loop and just add the + 1 plus the comment above the loop...

Claude genreated this way and I kind of like it as it has required
comment above and a matching xe_exec_queue_destroy() below.

>
>>         } else {
>>                 struct drm_xe_ext_set_property mq_priority = {
>>                         .base.name =
>> DRM_XE_EXEC_QUEUE_EXTENSION_SET_PROPERTY,
>> @@ -474,14 +479,28 @@ __test_priority(int fd, struct
>> drm_xe_engine_class_instance *eci,
>>                 xe_spin_wait_started(spin[i]);
>>  
>>         if (flags & DYN_PRIORITY) {
>> +               uint64_t barrier_spin_addr = addr + num_queues *
>> sizeof(struct xe_spin);
>> +
>>                 /* Assign increasing order of priority for secondary
>> queues */
>>                 for (i = 1; i < num_queues; i++)
>>                         xe_exec_queue_set_property(fd,
>> exec_queues[i],
>>                                                   
>> DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_QUEUE_PRIORITY,
>>                                                    i %
>> num_queue_priorities);
>>  
>> -               /* Wait for priorities to take effect */
>> -               sleep(sleep_duration);
>> +               /*
>> +                * Submit a barrier job on the extra queue to ensure
>> priority
>> +                * updates have taken effect before releasing the
>> other queues.
>> +                */
>> +               xe_spin_init_opts(spin[num_queues], .addr =
>> barrier_spin_addr,
>> +                                 .preempt = true);
>
>Why are you setting preempt mode here?
>

Claude added it and I thought I removed it, but obviously I did not.
Let me remove it.

Niranjana

>-Stuart
>
>> +               sync.addr = barrier_spin_addr +
>> +                       ((char *)&spin[num_queues]->exec_sync - (char
>> *)spin[num_queues]);
>> +               exec.exec_queue_id = exec_queues[num_queues];
>> +               exec.address = barrier_spin_addr;
>> +               xe_exec(fd, &exec);
>> +               xe_spin_end(spin[num_queues]);
>> +               xe_wait_ufence(fd, &spin[num_queues]->exec_sync,
>> USER_FENCE_VALUE,
>> +                              exec_queues[num_queues],
>> fence_timeout);
>>         }
>>  
>>         /*
>> @@ -566,6 +585,10 @@ __test_priority(int fd, struct
>> drm_xe_engine_class_instance *eci,
>>         for (i = 0; i < num_queues; i++)
>>                 xe_exec_queue_destroy(fd, exec_queues[i]);
>>  
>> +       /* Destroy the extra queue */
>> +       if (flags & DYN_PRIORITY)
>> +               xe_exec_queue_destroy(fd, exec_queues[num_queues]);
>> +
>>         munmap(bo_map, bo_size);
>>         gem_close(fd, bo);
>>  
>