[PATCH v3] drm/xe/sched: stop re-submitting signalled jobs

[Upadhyay, Tejas]

> -----Original Message-----
> From: Auld, Matthew <matthew.auld at intel.com>
> Sent: 28 May 2025 19:59
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
> xe at lists.freedesktop.org
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>; Brost, Matthew
> <matthew.brost at intel.com>; Tseng, William <william.tseng at intel.com>;
> stable at vger.kernel.org
> Subject: Re: [PATCH v3] drm/xe/sched: stop re-submitting signalled jobs
> 
> On 28/05/2025 14:06, Upadhyay, Tejas wrote:
> >
> >
> >> -----Original Message-----
> >> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> >> Matthew Auld
> >> Sent: 28 May 2025 17:03
> >> To: intel-xe at lists.freedesktop.org
> >> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>; Brost,
> >> Matthew <matthew.brost at intel.com>; Tseng, William
> >> <william.tseng at intel.com>; stable at vger.kernel.org
> >> Subject: [PATCH v3] drm/xe/sched: stop re-submitting signalled jobs
> >>
> >> Customer is reporting a really subtle issue where we get random DMAR
> >> faults, hangs and other nasties for kernel migration jobs when
> >> stressing stuff like s2idle/s3/s4. The explosions seems to happen
> >> somewhere after resuming the system with splats looking something like:
> >>
> >> PM: suspend exit
> >> rfkill: input handler disabled
> >> xe 0000:00:02.0: [drm] GT0: Engine reset: engine_class=bcs, logical_mask:
> >> 0x2, guc_id=0 xe 0000:00:02.0: [drm] GT0: Timedout job: seqno=24496,
> >> lrc_seqno=24496, guc_id=0, flags=0x13 in no process [-1] xe 0000:00:02.0:
> >> [drm] GT0: Kernel-submitted job timed out
> >>
> >> The likely cause appears to be a race between suspend cancelling the
> >> worker that processes the free_job()'s, such that we still have
> >> pending jobs to be freed after the cancel. Following from this, on
> >> resume the pending_list will now contain at least one already
> >> complete job, but it looks like we call drm_sched_resubmit_jobs(),
> >> which will then call
> >> run_job() on everything still on the pending_list. But if the job was
> >> already complete, then all the resources tied to the job, like the bb
> >> itself, any memory that is being accessed, the iommu mappings etc.
> >> might be long gone since those are usually tied to the fence signalling.
> >>
> >> This scenario can be seen in ftrace when running a slightly modified
> >> xe_pm (kernel was only modified to inject artificial latency into
> >> free_job to make the race easier to hit):
> >>
> >> xe_sched_job_run: dev=0000:00:02.0, fence=0xffff888276cc8540,
> >> seqno=0, lrc_seqno=0, gt=0, guc_id=0, batch_addr=0x000000146910 ...
> >> xe_exec_queue_stop:   dev=0000:00:02.0, 3:0x2, gt=0, width=1, guc_id=0,
> >> guc_state=0x0, flags=0x13
> >> xe_exec_queue_stop:   dev=0000:00:02.0, 3:0x2, gt=0, width=1, guc_id=1,
> >> guc_state=0x0, flags=0x4
> >> xe_exec_queue_stop:   dev=0000:00:02.0, 4:0x1, gt=1, width=1, guc_id=0,
> >> guc_state=0x0, flags=0x3
> >> xe_exec_queue_stop:   dev=0000:00:02.0, 1:0x1, gt=1, width=1, guc_id=1,
> >> guc_state=0x0, flags=0x3
> >> xe_exec_queue_stop:   dev=0000:00:02.0, 4:0x1, gt=1, width=1, guc_id=2,
> >> guc_state=0x0, flags=0x3
> >> xe_exec_queue_resubmit: dev=0000:00:02.0, 3:0x2, gt=0, width=1,
> >> guc_id=0, guc_state=0x0, flags=0x13
> >> xe_sched_job_run: dev=0000:00:02.0, fence=0xffff888276cc8540,
> >> seqno=0, lrc_seqno=0, gt=0, guc_id=0, batch_addr=0x000000146910 ...
> >> .....
> >> xe_exec_queue_memory_cat_error: dev=0000:00:02.0, 3:0x2, gt=0,
> >> width=1, guc_id=0, guc_state=0x3, flags=0x13
> >>
> >> So the job_run() is clearly triggered twice for the same job, even
> >> though the first must have already signalled to completion during
> >> suspend. We can also see a CAT error after the re-submit.
> >>
> >> To prevent this try to call xe_sched_stop() to forcefully remove
> >> anything on the pending_list that has already signalled, before we re-
> submit.
> >>
> >> v2:
> >>    - Make sure to re-arm the fence callbacks with sched_start().
> >> v3 (Matt B):
> >>    - Stop using drm_sched_resubmit_jobs(), which appears to be
> deprecated
> >>      and just open-code a simple loop such that we skip calling run_job()
> >>      and anything already signalled.
> >>
> >> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4856
> >> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
> >> GPUs")
> >> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> >> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> >> Cc: Matthew Brost <matthew.brost at intel.com>
> >> Cc: William Tseng <william.tseng at intel.com>
> >> Cc: <stable at vger.kernel.org> # v6.8+
> >> ---
> >>   drivers/gpu/drm/xe/xe_gpu_scheduler.h | 10 +++++++++-
> >>   1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> >> b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> >> index c250ea773491..308061f0cf37 100644
> >> --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> >> +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> >> @@ -51,7 +51,15 @@ static inline void xe_sched_tdr_queue_imm(struct
> >> xe_gpu_scheduler *sched)
> >>
> >>   static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler
> *sched)  {
> >> -	drm_sched_resubmit_jobs(&sched->base);
> >> +	struct drm_sched_job *s_job;
> >> +
> >> +	list_for_each_entry(s_job, &sched->base.pending_list, list) {
> >> +		struct drm_sched_fence *s_fence = s_job->s_fence;
> >> +		struct dma_fence *hw_fence = s_fence->parent;
> >> +
> >> +		if (hw_fence && !dma_fence_is_signaled(hw_fence))
> >> +			sched->base.ops->run_job(s_job);
> >> +	}
> >
> > While this change looks correct, what about those hanging contexts which is
> indicated to waiters by dma_fence_set_error(&s_fence->finished, -
> ECANCELED);!
> 
> I think a hanging context will usually be banned, so we shouldn't reach this
> point AFAICT. Can you share some more info on what your concern is here? I
> don't think we would normally want to call run_job() again on jobs from a
> hanging context. It looks like our run_job() will bail if the hw fence is marked
> with an error.

Ok, I see we detect the hanging and setting jobs through xe_sched_job_set_error() and will be signalled. I am fine here.

Reviewed-by: Tejas Upadhyay <tejas.upadhyay at intel.com>

Tejas 
> 
> >
> > Tejas
> >>   }
> >>
> >>   static inline bool
> >> --
> >> 2.49.0
> >