Re: [PATCH v2] drm/xe: Assign queue name in time for drm_sched_init

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Sat, May 23, 2026 at 11:34:18AM +0100, Tvrtko Ursulin wrote:
> Currently the queue name is only assigned after the drm scheduler instance
> has been created. This loses information with all logging or debug
> workqueue facilities so lets re-order things a bit so the name gets
> assigned in time.
> 
> To be able to assign a GuC ID early we split the allocation into
> reservation and publish phases.
> 
> First, with the submission state lock held, we reserve the ID in the GuC
> ID manager, which serves as an authoritative source of truth. Then we can
> drop the lock and reserve entries in the exec queue lookup XArray. This
> can be lockless since the NULL entries are invisible both to the kernel
> and userspace. Only after the queue has been fully created we replace the
> reserved entries with the queue pointer, which can be done locklessly for
> single width queues.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Thomas Hellstrom <thomas.hellstrom@linux.intel.com>
> ---
> v2:
>  * Split the id allocation into reserve and publish. (Rodrigo)


Thank you

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

pushing right now...

> ---
>  drivers/gpu/drm/xe/xe_guc_submit.c | 72 +++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index afd8cc7bd231..ab501513d806 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -408,46 +408,43 @@ void xe_guc_submit_disable(struct xe_guc *guc)
>  	guc->submission_state.enabled = false;
>  }
>  
> -static void __release_guc_id(struct xe_guc *guc, struct xe_exec_queue *q, u32 xa_count)
> +static void __release_guc_id(struct xe_guc *guc, struct xe_exec_queue *q,
> +			     int count)
>  {
>  	int i;
>  
> -	lockdep_assert_held(&guc->submission_state.lock);
> +	mutex_lock(&guc->submission_state.lock);
>  
> -	for (i = 0; i < xa_count; ++i)
> -		xa_erase(&guc->submission_state.exec_queue_lookup, q->guc->id + i);
> +	for (i = 0; i < count; ++i)
> +		xa_erase(&guc->submission_state.exec_queue_lookup,
> +			 q->guc->id + i);
>  
>  	xe_guc_id_mgr_release_locked(&guc->submission_state.idm,
>  				     q->guc->id, q->width);
>  
>  	if (xa_empty(&guc->submission_state.exec_queue_lookup))
>  		wake_up(&guc->submission_state.fini_wq);
> +
> +	mutex_unlock(&guc->submission_state.lock);
>  }
>  
>  static int alloc_guc_id(struct xe_guc *guc, struct xe_exec_queue *q)
>  {
> -	int ret;
> -	int i;
> -
> -	/*
> -	 * Must use GFP_NOWAIT as this lock is in the dma fence signalling path,
> -	 * worse case user gets -ENOMEM on engine create and has to try again.
> -	 *
> -	 * FIXME: Have caller pre-alloc or post-alloc /w GFP_KERNEL to prevent
> -	 * failure.
> -	 */
> -	lockdep_assert_held(&guc->submission_state.lock);
> +	int ret, i;
>  
> +	mutex_lock(&guc->submission_state.lock);
>  	ret = xe_guc_id_mgr_reserve_locked(&guc->submission_state.idm,
>  					   q->width);
> +	mutex_unlock(&guc->submission_state.lock);
>  	if (ret < 0)
>  		return ret;
>  
>  	q->guc->id = ret;
>  
> +	/* Reserve empty slots. */
>  	for (i = 0; i < q->width; ++i) {
> -		ret = xa_err(xa_store(&guc->submission_state.exec_queue_lookup,
> -				      q->guc->id + i, q, GFP_NOWAIT));
> +		ret = xa_insert(&guc->submission_state.exec_queue_lookup,
> +				 q->guc->id + i, NULL, GFP_KERNEL);
>  		if (ret)
>  			goto err_release;
>  	}
> @@ -460,11 +457,24 @@ static int alloc_guc_id(struct xe_guc *guc, struct xe_exec_queue *q)
>  	return ret;
>  }
>  
> +static void publish_guc_id(struct xe_guc *guc, struct xe_exec_queue *q)
> +{
> +	int i;
> +
> +	lockdep_assert_held(&guc->submission_state.lock);
> +
> +	for (i = 0; i < q->width; ++i) {
> +		void *old;
> +
> +		old = xa_store(&guc->submission_state.exec_queue_lookup,
> +			       q->guc->id + i, q, GFP_NOWAIT);
> +		XE_WARN_ON(old || xa_is_err(old));
> +	}
> +}
> +
>  static void release_guc_id(struct xe_guc *guc, struct xe_exec_queue *q)
>  {
> -	mutex_lock(&guc->submission_state.lock);
>  	__release_guc_id(guc, q, q->width);
> -	mutex_unlock(&guc->submission_state.lock);
>  }
>  
>  struct exec_queue_policy {
> @@ -1961,6 +1971,12 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
>  	timeout = (q->vm && xe_vm_in_lr_mode(q->vm)) ? MAX_SCHEDULE_TIMEOUT :
>  		  msecs_to_jiffies(q->sched_props.job_timeout_ms);
>  
> +	err = alloc_guc_id(guc, q);
> +	if (err)
> +		goto err_free;
> +
> +	xe_exec_queue_assign_name(q, q->guc->id);
> +
>  	/*
>  	 * Use primary queue's submit_wq for all secondary queues of a
>  	 * multi queue group. This serialization avoids any locking around
> @@ -1977,28 +1993,21 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
>  			    timeout, guc_to_gt(guc)->ordered_wq, NULL,
>  			    q->name, gt_to_xe(q->gt)->drm.dev);
>  	if (err)
> -		goto err_free;
> +		goto err_release_id;
>  
>  	sched = &ge->sched;
>  	err = xe_sched_entity_init(&ge->entity, sched);
>  	if (err)
>  		goto err_sched;
>  
> -	mutex_lock(&guc->submission_state.lock);
> -
> -	err = alloc_guc_id(guc, q);
> -	if (err)
> -		goto err_entity;
> -
>  	q->entity = &ge->entity;
>  
> +	mutex_lock(&guc->submission_state.lock);
>  	if (xe_guc_read_stopped(guc) || vf_recovery(guc))
>  		xe_sched_stop(sched);
> -
> +	publish_guc_id(guc, q);
>  	mutex_unlock(&guc->submission_state.lock);
>  
> -	xe_exec_queue_assign_name(q, q->guc->id);
> -
>  	/*
>  	 * Maintain secondary queues of the multi queue group in a list
>  	 * for handling dependencies across the queues in the group.
> @@ -2021,11 +2030,10 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
>  
>  	return 0;
>  
> -err_entity:
> -	mutex_unlock(&guc->submission_state.lock);
> -	xe_sched_entity_fini(&ge->entity);
>  err_sched:
>  	xe_sched_fini(&ge->sched);
> +err_release_id:
> +	release_guc_id(guc, q);
>  err_free:
>  	kfree(ge);
>  
> -- 
> 2.54.0
>