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

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Jan 20, 2026 at 02:20:22PM -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.

Actually maybe we need WRITE_ONCE / READ_ONCE semantics here to prevent
data tearing.

Matt

> 
> 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_queue_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_BASE + DRM_XE_OBSERVATION,structdrm_xe_observation_param)
> > > >  #define
> > > > DRM_IOCTL_XE_MADVISE			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_MADVISE, structdrm_xe_madvise)
> > > >  #define
> > > > DRM_IOCTL_XE_VM_QUERY_MEM_RANGE_ATTRS	DRM_IOWR(DRM_COMMAND_BASE + 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_BASE + DRM_XE_EXEC_QUEUE_SET_PROPERTY,structdrm_xe_exec_queue_set_property)
> > > >  
> > > >  /**
> > > >   * 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
> > >