Re: [PATCH v3 14/18] drm/xe/multi_queue: Support active group after primary is destroyed

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

On Thu, Nov 20, 2025 at 07:51:48PM -0800, Niranjana Vishwanathapura wrote:
> Add support to keep the group active after the primary queue is
> destroyed. Instead of killing the primary queue during exec_queue
> destroy ioctl, kill it when all the secondary queues of the group
> are killed.
> 
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c           |  7 ++-
>  drivers/gpu/drm/xe/xe_exec_queue.c       | 55 +++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_exec_queue.h       |  2 +
>  drivers/gpu/drm/xe/xe_exec_queue_types.h |  4 ++
>  include/uapi/drm/xe_drm.h                |  4 ++
>  5 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 037206c8b543..e72294a7d4b1 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -177,7 +177,12 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>  	xa_for_each(&xef->exec_queue.xa, idx, q) {
>  		if (q->vm && q->hwe->hw_engine_group)
>  			xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> -		xe_exec_queue_kill(q);
> +
> +		if (xe_exec_queue_is_multi_queue_primary(q))
> +			xe_exec_queue_group_kill_put(q->multi_queue.group);
> +		else
> +			xe_exec_queue_kill(q);
> +
>  		xe_exec_queue_put(q);
>  	}
>  	xa_for_each(&xef->vm.xa, idx, vm)
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index ab161b74fef0..839fb08f09e7 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -417,6 +417,26 @@ struct xe_exec_queue *xe_exec_queue_create_bind(struct xe_device *xe,
>  }
>  ALLOW_ERROR_INJECTION(xe_exec_queue_create_bind, ERRNO);
>  
> +static void xe_exec_queue_group_kill(struct kref *ref)
> +{
> +	struct xe_exec_queue_group *group = container_of(ref, struct xe_exec_queue_group,
> +							 kill_refcount);
> +	xe_exec_queue_kill(group->primary);
> +}
> +
> +static inline void xe_exec_queue_group_kill_get(struct xe_exec_queue_group *group)
> +{
> +	kref_get(&group->kill_refcount);
> +}
> +
> +void xe_exec_queue_group_kill_put(struct xe_exec_queue_group *group)
> +{
> +	if (!group)
> +		return;
> +
> +	kref_put(&group->kill_refcount, xe_exec_queue_group_kill);
> +}
> +
>  void xe_exec_queue_destroy(struct kref *ref)
>  {
>  	struct xe_exec_queue *q = container_of(ref, struct xe_exec_queue, refcount);
> @@ -629,6 +649,7 @@ static int xe_exec_queue_group_init(struct xe_device *xe, struct xe_exec_queue *
>  	group->primary = q;
>  	group->cgp_bo = bo;
>  	INIT_LIST_HEAD(&group->list);
> +	kref_init(&group->kill_refcount);
>  	xa_init_flags(&group->xa, XA_FLAGS_ALLOC1);
>  	mutex_init(&group->list_lock);
>  	q->multi_queue.group = group;
> @@ -704,6 +725,11 @@ static int xe_exec_queue_group_add(struct xe_device *xe, struct xe_exec_queue *q
>  
>  	q->multi_queue.pos = pos;
>  
> +	if (group->primary->multi_queue.keep_active) {
> +		xe_exec_queue_group_kill_get(group);
> +		q->multi_queue.keep_active = true;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -717,6 +743,11 @@ static void xe_exec_queue_group_delete(struct xe_device *xe, struct xe_exec_queu
>  	lrc = xa_erase(&group->xa, q->multi_queue.pos);
>  	xe_assert(xe, lrc);
>  	xe_lrc_put(lrc);
> +
> +	if (q->multi_queue.keep_active) {
> +		xe_exec_queue_group_kill_put(group);
> +		q->multi_queue.keep_active = false;
> +	}
>  }
>  
>  static int exec_queue_set_multi_group(struct xe_device *xe, struct xe_exec_queue *q,
> @@ -735,12 +766,24 @@ static int exec_queue_set_multi_group(struct xe_device *xe, struct xe_exec_queue
>  		return -EINVAL;
>  
>  	if (value & DRM_XE_MULTI_GROUP_CREATE) {
> -		if (XE_IOCTL_DBG(xe, value & ~DRM_XE_MULTI_GROUP_CREATE))
> +		if (XE_IOCTL_DBG(xe, value & ~(DRM_XE_MULTI_GROUP_CREATE |
> +					       DRM_XE_MULTI_GROUP_KEEP_ACTIVE)))
> +			return -EINVAL;
> +
> +		/*
> +		 * KEEP_ACTIVE is not supported in preempt fence mode as in that mode,
> +		 * VM_DESTROY ioctl expects all exec queues of that VM are already killed.
> +		 */

Is the compute UMD ok with this? I really hope by the time multi-queue
is enabled in the KMD the compute UMD is only using faulting VMs. Not
blocker, we likely could actually fix preempt fence mode to avoid this
restriction, more something we need communicate to the UMDs teams / keep
an on eye if this becomes an issue.

Anyways patch LGTM:

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +		if (XE_IOCTL_DBG(xe, (value & DRM_XE_MULTI_GROUP_KEEP_ACTIVE) &&
> +				 xe_vm_in_preempt_fence_mode(q->vm)))
>  			return -EINVAL;
>  
>  		q->multi_queue.valid = true;
>  		q->multi_queue.is_primary = true;
>  		q->multi_queue.pos = 0;
> +		if (value & DRM_XE_MULTI_GROUP_KEEP_ACTIVE)
> +			q->multi_queue.keep_active = true;
> +
>  		return 0;
>  	}
>  
> @@ -1286,6 +1329,11 @@ void xe_exec_queue_kill(struct xe_exec_queue *q)
>  
>  	q->ops->kill(q);
>  	xe_vm_remove_compute_exec_queue(q->vm, q);
> +
> +	if (!xe_exec_queue_is_multi_queue_primary(q) && q->multi_queue.keep_active) {
> +		xe_exec_queue_group_kill_put(q->multi_queue.group);
> +		q->multi_queue.keep_active = false;
> +	}
>  }
>  
>  int xe_exec_queue_destroy_ioctl(struct drm_device *dev, void *data,
> @@ -1312,7 +1360,10 @@ int xe_exec_queue_destroy_ioctl(struct drm_device *dev, void *data,
>  	if (q->vm && q->hwe->hw_engine_group)
>  		xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
>  
> -	xe_exec_queue_kill(q);
> +	if (xe_exec_queue_is_multi_queue_primary(q))
> +		xe_exec_queue_group_kill_put(q->multi_queue.group);
> +	else
> +		xe_exec_queue_kill(q);
>  
>  	trace_xe_exec_queue_close(q);
>  	xe_exec_queue_put(q);
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h b/drivers/gpu/drm/xe/xe_exec_queue.h
> index ffcc1feb879e..10abed98fb6b 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.h
> @@ -113,6 +113,8 @@ static inline struct xe_exec_queue *xe_exec_queue_multi_queue_primary(struct xe_
>  	return xe_exec_queue_is_multi_queue(q) ? q->multi_queue.group->primary : q;
>  }
>  
> +void xe_exec_queue_group_kill_put(struct xe_exec_queue_group *group);
> +
>  bool xe_exec_queue_is_lr(struct xe_exec_queue *q);
>  
>  bool xe_exec_queue_is_idle(struct xe_exec_queue *q);
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> index 5721fb4bad1a..c6b80977ff4f 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> @@ -62,6 +62,8 @@ struct xe_exec_queue_group {
>  	struct list_head list;
>  	/** @list_lock: Secondary queue list lock */
>  	struct mutex list_lock;
> +	/** @kill_refcount: ref count to kill primary queue */
> +	struct kref kill_refcount;
>  	/** @sync_pending: CGP_SYNC_DONE g2h response pending */
>  	bool sync_pending;
>  };
> @@ -159,6 +161,8 @@ struct xe_exec_queue {
>  		u8 valid:1;
>  		/** @multi_queue.is_primary: Is primary queue (Q0) of the group */
>  		u8 is_primary:1;
> +		/** @multi_queue.keep_active: Keep the group active after primary is destroyed */
> +		u8 keep_active:1;
>  	} multi_queue;
>  
>  	/** @sched_props: scheduling properties */
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 71a851615876..74cbebb24aa5 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -1260,6 +1260,9 @@ struct drm_xe_vm_bind {
>   *    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's exec_queue_id is specified in the lower 32 bits of the 'value' field.
> + *    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.
>   *    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.
>   *  - %DRM_XE_EXEC_QUEUE_SET_PROPERTY_MULTI_QUEUE_PRIORITY - Set the queue
> @@ -1307,6 +1310,7 @@ struct drm_xe_exec_queue_create {
>  #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;
> -- 
> 2.43.0
>