Re: [PATCH v7 05/32] drm/xe: Don't change LRC ring head on job resubmission

[Matthew Auld <matthew.auld@intel.com>]

On 07/10/2025 12:26, Matthew Brost wrote:
> Now that we save the job's head during submission, it's no longer
> necessary to adjust the LRC ring head during resubmission. Instead, a
> software-based adjustment of the tail will overwrite the old jobs in
> place. For some odd reason, adjusting the LRC ring head didn't work on
> parallel queues, which was causing issues in our CI.
> 
> v5:
>   - Add comment in guc_exec_queue_start explaning why the function works
>     (Auld)
> v7:
>   - Only adjust first state on first unsignaled job (Auld)
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_gpu_scheduler.h | 21 +++++++++++++++++----
>   drivers/gpu/drm/xe/xe_guc_submit.c    | 18 ++++++++++++++++--
>   2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> index e548b2aed95a..3a9ff78d9346 100644
> --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> @@ -77,17 +77,30 @@ static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
>   	spin_unlock(&sched->base.job_list_lock);
>   }
>   
> +/**
> + * xe_sched_first_pending_job() - Find first pending job which is unsignaled
> + * @sched: Xe GPU scheduler
> + *
> + * Return first unsignaled job in pending list or NULL
> + */
>   static inline
>   struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
>   {
> -	struct xe_sched_job *job;
> +	struct xe_sched_job *job, *r_job = NULL;
>   
>   	spin_lock(&sched->base.job_list_lock);
> -	job = list_first_entry_or_null(&sched->base.pending_list,
> -				       struct xe_sched_job, drm.list);
> +	list_for_each_entry(job, &sched->base.pending_list, drm.list) {
> +		struct drm_sched_fence *s_fence = job->drm.s_fence;
> +		struct dma_fence *hw_fence = s_fence->parent;
> +
> +		if (hw_fence && !dma_fence_is_signaled(hw_fence)) {
> +			r_job = job;
> +			break;
> +		}
> +	}
>   	spin_unlock(&sched->base.job_list_lock);

I think it might make sense to split this change out, since it also 
changes the behaviour of the other existing caller, and that might want 
to be treated as a potential fix. Or commit needs to be reworded to make 
this more clear.

Otherwise lgtm.

>   
> -	return job;
> +	return r_job;
>   }
>   
>   static inline int
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 3a534d93505f..d123bdb63369 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2008,11 +2008,25 @@ static void guc_exec_queue_start(struct xe_exec_queue *q)
>   	struct xe_gpu_scheduler *sched = &q->guc->sched;
>   
>   	if (!exec_queue_killed_or_banned_or_wedged(q)) {
> +		struct xe_sched_job *job = xe_sched_first_pending_job(sched);
>   		int i;
>   
>   		trace_xe_exec_queue_resubmit(q);
> -		for (i = 0; i < q->width; ++i)
> -			xe_lrc_set_ring_head(q->lrc[i], q->lrc[i]->ring.tail);
> +		if (job) {
> +			for (i = 0; i < q->width; ++i) {
> +				/*
> +				 * The GuC context is unregistered at this point
> +				 * time, adjusting software ring tail ensures
> +				 * jobs are rewritten in original placement,
> +				 * adjusting LRC tail ensures the newly loaded
> +				 * GuC / contexts only view the LRC tail
> +				 * increasing as jobs are written out.
> +				 */
> +				q->lrc[i]->ring.tail = job->ptrs[i].head;
> +				xe_lrc_set_ring_tail(q->lrc[i],
> +						     xe_lrc_ring_head(q->lrc[i]));
> +			}
> +		}
>   		xe_sched_resubmit_jobs(sched);
>   	}
>