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

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

On Wed, Jan 21, 2026 at 04:29:34PM +0100, Thomas Hellström wrote:
>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".
>

Hi Thomas, Matt,

I have posted a patch to fix the issue.
https://patchwork.freedesktop.org/series/160447/

Can you take a look?

Thanks,
Niranjana

>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
>> > >