Re: [PATCH v6 06/17] drm/xe/multi_queue: Add exec_queue set_property ioctl support

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Tue, 2026-01-20 at 14:20 -0800, Matthew Brost wrote:
> On Tue, Jan 20, 2026 at 01:06:07PM -0800, Niranjana Vishwanathapura
> wrote:
> > On Mon, Jan 19, 2026 at 05:57:29PM +0100, Thomas Hellström wrote:
> > > Hi,
> > > 
> > > On Wed, 2025-12-10 at 17:02 -0800, Niranjana Vishwanathapura
> > > wrote:
> > > > This patch adds support for exec_queue set_property ioctl.
> > > > It is derived from the original work which is part of
> > > > https://patchwork.freedesktop.org/series/112188/
> > > > 
> > > > Currently only
> > > > DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_QUEUE_PRIORITY
> > > > property can be dynamically set.
> > > > 
> > > > v2: Check for and update kernel-doc which property this ioctl
> > > >     supports (Matt Brost)
> > > > 
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com>
> > > > Signed-off-by: Niranjana Vishwanathapura
> > > > <niranjana.vishwanathapura@intel.com>
> > > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > 
> > > Two questions on this patch:
> > > 
> > > 1) I don't see any locking to protect the exec-queue / lrc state.
> > > Perhaps I missed it, but couldn't this blow up if someone runs
> > > two
> > > set_property ioctls in parallel on the same exec-queue? Looks
> > > like at
> > > least the set_priority is doing non-atomic RMW assignments?
> > > 
> 
> I don't think we'd blow up. At worst q->multi_queue.priority would
> race
> between a set in guc_exec_queue_set_priority (parallel across queues)
> and read of that value in
> __guc_exec_queue_process_msg_set_sched_props
> (serialize across queues). Eventually we'd stablize on the most
> recently
> set value in __guc_exec_queue_process_msg_set_sched_props.
> 
> > 
> > Hmm...I think only checking and setting of 'q-
> > >multi_queue.priority'
> > in guc_exec_queue_set_multi_queue_priority() is a potential issue
> > (as the rest is handled by message handling queue and guc ct
> > backend).
> > I think we can fix it with a simple spicklock in this function.
> > 
> 
> Hmm, I'm not sure what a lock here buys us as if two user threads try
> to
> set q->multi_queue.priority we'd never really knows who should win
> the
> race of saying of setting.
> 
> > > 2) Do we really need this to be mutable? In i915 assuming that
> > > lrc /
> > > context configuration could change created horrendous locking
> > > constructs that were later fixed by making it immutable at first
> > > command sumbission. I don't exactly remember the details but do
> > > we need
> > > to change properties after first submission? If not I suggest
> > > blocking
> > > that.
> 
> I don't think this is overly complicated here given the actual queue
> interaction with the GuC is serialized by the DRM scheduler.
> 
> My thinking is this patch is probably fine as is, albiet a little
> harmlessly racey.

I think even if it's possible to reason that this is harmless in its
current form, state that can be modified by my multiple threads
simultaneously must really be protected by locks.

Otherwise the code will be fragile: Any future updates where the author
doesn't realize the state is not properly protected might break it.

Also any person trying to familiarize himself with the code and noting
that the data is not protected from racing will have to dig down and
verify that each and every concurrent access is actually ok taking the
lack of barriers and hardware reordering into account.

So please, let's adhere to "data with concurrent accesses must be
protected unless immutable".

Thanks,
Thomas



> 
> Matt
> 
> > 
> > Yah, ability to dynamically change multi-queue priority is a
> > requirement
> > from the Compute UMD team.
> > 
> > Niranjana
> > 
> > > 
> > > Thanks,
> > > Thomas
> > > 
> > > 
> > > 
> > > 
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_device.c     |  2 ++
> > > >  drivers/gpu/drm/xe/xe_exec_queue.c | 35
> > > > ++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/xe/xe_exec_queue.h |  2 ++
> > > >  include/uapi/drm/xe_drm.h          | 26 ++++++++++++++++++++++
> > > >  4 files changed, 65 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > > > b/drivers/gpu/drm/xe/xe_device.c
> > > > index 1197f914ef77..7a498c8db7b1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > > @@ -207,6 +207,8 @@ static const struct drm_ioctl_desc
> > > > xe_ioctls[] =
> > > > {
> > > >  	DRM_IOCTL_DEF_DRV(XE_MADVISE, xe_vm_madvise_ioctl,
> > > > DRM_RENDER_ALLOW),
> > > >  	DRM_IOCTL_DEF_DRV(XE_VM_QUERY_MEM_RANGE_ATTRS,
> > > > xe_vm_query_vmas_attrs_ioctl,
> > > >  			  DRM_RENDER_ALLOW),
> > > > +	DRM_IOCTL_DEF_DRV(XE_EXEC_QUEUE_SET_PROPERTY,
> > > > xe_exec_queue_set_property_ioctl,
> > > > +			  DRM_RENDER_ALLOW),
> > > >  };
> > > >  
> > > >  static long xe_drm_ioctl(struct file *file, unsigned int cmd,
> > > > unsigned long arg)
> > > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > index d0082eb45a4a..d738a9fea1e1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > @@ -790,6 +790,41 @@ static const xe_exec_queue_set_property_fn
> > > > exec_queue_set_property_funcs[] = {
> > > >  							exec_q
> > > > ueue_s
> > > > et_multi_queue_priority,
> > > >  };
> > > >  
> > > > +int xe_exec_queue_set_property_ioctl(struct drm_device *dev,
> > > > void
> > > > *data,
> > > > +				     struct drm_file *file)
> > > > +{
> > > > +	struct xe_device *xe = to_xe_device(dev);
> > > > +	struct xe_file *xef = to_xe_file(file);
> > > > +	struct drm_xe_exec_queue_set_property *args = data;
> > > > +	struct xe_exec_queue *q;
> > > > +	int ret;
> > > > +	u32 idx;
> > > > +
> > > > +	if (XE_IOCTL_DBG(xe, args->reserved[0] || args-
> > > > > reserved[1]))
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (XE_IOCTL_DBG(xe, args->property !=
> > > > +			
> > > > DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_QUEUE_PRIORITY))
> > > > +		return -EINVAL;
> > > > +
> > > > +	q = xe_exec_queue_lookup(xef, args->exec_queue_id);
> > > > +	if (XE_IOCTL_DBG(xe, !q))
> > > > +		return -ENOENT;
> > > > +
> > > > +	idx = array_index_nospec(args->property,
> > > > +				
> > > > ARRAY_SIZE(exec_queue_set_property_funcs));
> > > > +	ret = exec_queue_set_property_funcs[idx](xe, q, args-
> > > > > value);
> > > > +	if (XE_IOCTL_DBG(xe, ret))
> > > > +		goto err_post_lookup;
> > > > +
> > > > +	xe_exec_queue_put(q);
> > > > +	return 0;
> > > > +
> > > > + err_post_lookup:
> > > > +	xe_exec_queue_put(q);
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  static int exec_queue_user_ext_check(struct xe_exec_queue *q,
> > > > u64
> > > > properties)
> > > >  {
> > > >  	u64 secondary_queue_valid_props =
> > > > BIT_ULL(DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_GROUP) |
> > > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h
> > > > b/drivers/gpu/drm/xe/xe_exec_queue.h
> > > > index e6daa40003f2..ffcc1feb879e 100644
> > > > --- a/drivers/gpu/drm/xe/xe_exec_queue.h
> > > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.h
> > > > @@ -125,6 +125,8 @@ int xe_exec_queue_destroy_ioctl(struct
> > > > drm_device
> > > > *dev, void *data,
> > > >  				struct drm_file *file);
> > > >  int xe_exec_queue_get_property_ioctl(struct drm_device *dev,
> > > > void
> > > > *data,
> > > >  				     struct drm_file *file);
> > > > +int xe_exec_queue_set_property_ioctl(struct drm_device *dev,
> > > > void
> > > > *data,
> > > > +				     struct drm_file *file);
> > > >  enum xe_exec_queue_priority
> > > > xe_exec_queue_device_get_max_priority(struct xe_device *xe);
> > > >  
> > > >  void xe_exec_queue_last_fence_put(struct xe_exec_queue *e,
> > > > struct
> > > > xe_vm *vm);
> > > > diff --git a/include/uapi/drm/xe_drm.h
> > > > b/include/uapi/drm/xe_drm.h
> > > > index fd79d78de2e9..705081bf0d81 100644
> > > > --- a/include/uapi/drm/xe_drm.h
> > > > +++ b/include/uapi/drm/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_BA
> > > > SE + DRM_XE_OBSERVATION,structdrm_xe_observation_param)
> > > >  #define
> > > > DRM_IOCTL_XE_MADVISE			DRM_IOW(DRM_COMMAND_BA
> > > > SE + DRM_XE_MADVISE, structdrm_xe_madvise)
> > > >  #define
> > > > DRM_IOCTL_XE_VM_QUERY_MEM_RANGE_ATTRS	DRM_IOWR(DRM_COMMAND_B
> > > > ASE +
> > > > DRM_XE_VM_QUERY_MEM_RANGE_ATTRS,structdrm_xe_vm_query_mem_range
> > > > _attr)
> > > > +#define
> > > > DRM_IOCTL_XE_EXEC_QUEUE_SET_PROPERTY	DRM_IOW(DRM_COMMAND_BA
> > > > SE +
> > > > DRM_XE_EXEC_QUEUE_SET_PROPERTY,structdrm_xe_exec_queue_set_prop
> > > > erty)
> > > >  
> > > >  /**
> > > >   * DOC: Xe IOCTL Extensions
> > > > @@ -2315,6 +2317,30 @@ struct drm_xe_vm_query_mem_range_attr {
> > > >  
> > > >  };
> > > >  
> > > > +/**
> > > > + * 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.
> > > > + */
> > > > +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
> > >