Re: [PATCH v4 08/34] drm/xe: Don't change LRC ring head on job resubmission

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

On 05/10/2025 07:53, Matthew Brost wrote:
> On Sat, Oct 04, 2025 at 10:25:34PM -0700, Matthew Brost wrote:
>> On Thu, Oct 02, 2025 at 03:15:13PM +0100, Matthew Auld wrote:
>>> On 02/10/2025 06:53, 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.
>>>>
>>>> v6:
>>>>    - Also set LRC tail to head so queue is idle coming out of reset
>>>>
>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>> Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_guc_submit.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>>>> index 3a534d93505f..70306f902ba5 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>>>> @@ -2008,11 +2008,17 @@ 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) {
>>>> +				q->lrc[i]->ring.tail = job->ptrs[i].head;
>>>> +				xe_lrc_set_ring_tail(q->lrc[i],
>>>> +						     xe_lrc_ring_head(q->lrc[i]));
>>>
>>> IIRC the sched pending_list stuff can also give back pending jobs that have
>>> completed on the hw, but are still kept pending until the final free_job()?
>>>
>>> Suppose we have a pending_list like:
>>>
>>> [pending/complete, pending/complete, actual pending kernel job that never
>>> completed/ran]
>>>
>>> IIUC the sw ring.tail will actually go backwards to the first pending
>>> free/complete job head in the pending_list, with the hw tail being reset to
>>> the current hw head here. But on the next submit the sw ring.tail is where
>>> the commands are emitted to, and on the next update
>>> of the hw tail it will be synced to the sw ring.tail? But if that happens
>>> won't we get hw tail < hw head (since we used the head of an already
>>> complete job for the sw tail), which will make the hw think there is a
>>> massive ring wrap, so it will execute garbage until it wraps back around to
>>> tail?
>>>
>>
>> Let me tweak this flow to use the skip_emit / last_replay flow
>> introduced later in series to avoid this issue.
>>
> 
> Actually this flow works just fine. The GuC state is completely lost
> during this flow - the context is not even registered. By the time
> contect is registered - the LRC head will be at the original postition
> of the first pending job and LRC tail will be at the end of the first
> job.

Can you share some more info here on the flow? I'm seeing LRC hw head 
being at the correct position, which must have moved past anything 
already completed by the hw, right? But here the sw lrc tail is being 
potentially moved backwards to the head of something already complete 
(the job we pick is pending but only because the free_job() has not run 
yet, so the job has already signalled/ran). Below when we do something 
like xe_sched_resubmit_jobs() the hw tail is then updated to the sw 
tail, but now we end up with hw tail < hw head?

Also the flow I'm thinking about here is forced suspend/resume.

> 
> Matt
> 
>> Matt
>>
>>>> +			}
>>>> +		}
>>>>    		xe_sched_resubmit_jobs(sched);
>>>>    	}
>>>