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

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

On Wed, 2026-04-29 at 21:06 -0700, Niranjana Vishwanathapura wrote:
> On Wed, Apr 29, 2026 at 01:52:05PM -0700, Wang, X wrote:
> > 
> > 
> > On 4/29/2026 11:27, 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];
> > > Since we're only really making use of this in the dynamic case,
> > > should
> > > we have "+ !!DYNAMIC" instead of "+ 1" here? I.e. we only care
> > > about
> > > the extra barrier one in the dynamic case?
> > > 
> > > Thanks,
> > > Stuart
> > flags is a runtime parameter, so + !!(flags & DYN_PRIORITY)
> > would make the array size runtime-determined — effectively a VLA.
> > Even in userspace, VLAs are generally discouraged due to
> > unpredictable stack usage. The cost of one extra slot is
> > negligible,
> > so always using + 1 is simpler and avoids introducing a VLA.
> > 
> 
> Yes, we allocate enough space required to handle any scenario.
> That is much better than making the code complex to save an array
> element.

Yeah ok makes sense to me and I agree with the explanations.

-Stuart

> 
> Niranjana
> 
> > Thanks,
> > Xin
> > > >         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);
> > > >         } 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);
> > > > +               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);
> >