Re: [PATCH v2 01/19] drm-uapi/xe: Sync with Multi-Queue uapi

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

On Thu, 2025-12-04 at 12:58 -0800, Niranjana Vishwanathapura wrote:
> On Thu, Dec 04, 2025 at 11:19:47AM -0800, Summers, Stuart wrote:
> > On Thu, 2025-11-20 at 19:57 -0800, Niranjana Vishwanathapura wrote:
> > > Sync kernel header to get Multi-Queue uapi definitions.
> > > 
> > > Signed-off-by: Niranjana Vishwanathapura
> > > <niranjana.vishwanathapura@intel.com>
> > > ---
> > >  include/drm-uapi/xe_drm.h | 40
> > > +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 40 insertions(+)
> > > 
> > > diff --git a/include/drm-uapi/xe_drm.h b/include/drm-
> > > uapi/xe_drm.h
> > > index 89ab54935..d1cb08dc7 100644
> > > --- a/include/drm-uapi/xe_drm.h
> > > +++ b/include/drm-uapi/xe_drm.h
> > > @@ -106,6 +106,7 @@ extern "C" {
> > >  #define DRM_XE_OBSERVATION             0x0b
> > >  #define DRM_XE_MADVISE                 0x0c
> > >  #define DRM_XE_VM_QUERY_MEM_RANGE_ATTRS        0x0d
> > > +#define DRM_XE_EXEC_QUEUE_SET_PROPERTY 0x0e
> > >  
> > >  /* Must be kept compact -- no holes */
> > >  
> > > @@ -123,6 +124,7 @@ extern "C" {
> > >  #define
> > > DRM_IOCTL_XE_OBSERVATION               DRM_IOW(DRM_COMMAND_BASE +
> > > DRM_XE_OBSERVATION, struct drm_xe_observation_param)
> > >  #define
> > > DRM_IOCTL_XE_MADVISE                   DRM_IOW(DRM_COMMAND_BASE +
> > > DRM_XE_MADVISE, struct drm_xe_madvise)
> > >  #define
> > > DRM_IOCTL_XE_VM_QUERY_MEM_RANGE_ATTRS  DRM_IOWR(DRM_COMMAND_BASE
> > > +
> > > DRM_XE_VM_QUERY_MEM_RANGE_ATTRS, struct
> > > drm_xe_vm_query_mem_range_attr)
> > > +#define
> > > DRM_IOCTL_XE_EXEC_QUEUE_SET_PROPERTY   DRM_IOW(DRM_COMMAND_BASE +
> > > DRM_XE_EXEC_QUEUE_SET_PROPERTY, struct
> > > drm_xe_exec_queue_set_property)
> > >  
> > >  /**
> > >   * DOC: Xe IOCTL Extensions
> > > @@ -1233,6 +1235,18 @@ struct drm_xe_vm_bind {
> > >   *    Given that going into a power-saving state kills PXP HWDRM
> > > sessions,
> > >   *    runtime PM will be blocked while queues of this type are
> > > alive.
> > >   *    All PXP queues will be killed if a PXP invalidation event
> > > occurs.
> > > + *  - %DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_GROUP - Create a
> > > multi-
> > > queue group
> > > + *    or add secondary queues to a multi-queue group.
> > > + *    If the extension's 'value' field has
> > > %DRM_XE_MULTI_GROUP_CREATE flag set,
> > > + *    then a new multi-queue group is created with this queue as
> > > the
> > > primary queue
> > > + *    (Q0). Otherwise, the queue gets added to the multi-queue
> > > group
> > > whose primary
> > > + *    queue id is specified in the 'value' field.
> > 
> > Missing documentation, although compared to the ones below this is
> > much
> > less interesting:
> > 
> > + *    All the other non-relevant bits of extension's 'value' field
> > while adding the
> > + *    primary or the secondary queues of the group must be set to
> > 0.
> > 
> > > + *    If the extension's 'value' field has
> > > %DRM_XE_MULTI_GROUP_KEEP_ACTIVE flag
> > > + *    set, then the multi-queue group is kept active after the
> > > primary queue is
> > > + *    destroyed.
> > > + *
> > > + *  - %DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_QUEUE_PRIORITY - Set
> > > the
> > > queue
> > > + *    priority within the multi-queue group.
> > 
> > I see a little more detail in the kernel patch here:
> > 
> > + *    priority within the multi-queue group. Current valid
> > priority
> > values are 0–2
> > + *    (default is 1), with higher values indicating higher
> > priority.
> > 
> > Why not include this documentation here?
> > 
> 
> The updates to uapi documentation on kernel side was done as part of
> review comment fix, which I forgot to import it here. Will import
> them. I applies to below comments also.

Makes sense and sounds great. Thanks!

-Stuart

> 
> Niranjana
> 
> > >   *
> > >   * The example below shows how to use @drm_xe_exec_queue_create
> > > to
> > > create
> > >   * a simple exec_queue (no parallel submission) of class
> > > @@ -1273,6 +1287,10 @@ struct drm_xe_exec_queue_create {
> > >  #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY              0
> > >  #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_TIMESLICE             1
> > >  #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PXP_TYPE              2
> > > +#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_GROUP           3
> > > +#define    
> > > DRM_XE_MULTI_GROUP_CREATE                          (1ull
> > > << 63)
> > > +#define    
> > > DRM_XE_MULTI_GROUP_KEEP_ACTIVE                     (1ull
> > > << 62)
> > > +#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_QUEUE_PRIORITY  4
> > >         /** @extensions: Pointer to the first extension struct,
> > > if
> > > any */
> > >         __u64 extensions;
> > >  
> > > @@ -2254,6 +2272,28 @@ struct drm_xe_vm_query_mem_range_attr {
> > >  
> > >  };
> > >  
> > > +/**
> > > + * struct drm_xe_exec_queue_set_property - exec queue set
> > > property
> > > + *
> > > + * Sets execution queue properties dynamically.
> > 
> > Why not include the bit about the multi queue property being the
> > only
> > one here?
> > 
> > +/**
> > + * struct drm_xe_exec_queue_set_property - exec queue set property
> > + *
> > + * Sets execution queue properties dynamically.
> > + * Currently only
> > %DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_QUEUE_PRIORITY
> > + * property can be dynamically set.
> > + */
> > 
> > Anyway these aren't function. From my perspective it's more nice to
> > have to align with the kernel. An IGT developer (and UMD dev) can
> > and
> > should look at the documentation in the kernel itself anyway. For
> > consistency though it would be nice to keep this the same.
> > 
> > Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> > 
> > Thanks,
> > Stuart
> > 
> > 
> > > + */
> > > +struct drm_xe_exec_queue_set_property {
> > > +       /** @extensions: Pointer to the first extension struct,
> > > if
> > > any */
> > > +       __u64 extensions;
> > > +
> > > +       /** @exec_queue_id: Exec queue ID */
> > > +       __u32 exec_queue_id;
> > > +
> > > +       /** @property: property to set */
> > > +       __u32 property;
> > > +
> > > +       /** @value: property value */
> > > +       __u64 value;
> > > +
> > > +       /** @reserved: Reserved */
> > > +       __u64 reserved[2];
> > > +};
> > > +
> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif
> >