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

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

[Matthew Auld]

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@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: William Tseng <william.tseng@intel.com>
Cc: <stable@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);
+	}
 }
 
 static inline bool
-- 
2.49.0

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

[Upadhyay, Tejas]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> Matthew Auld
> Sent: 28 May 2025 17:03
> To: intel-xe@lists.freedesktop.org
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Brost, Matthew
> <matthew.brost@intel.com>; Tseng, William <william.tseng@intel.com>;
> stable@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@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: William Tseng <william.tseng@intel.com>
> Cc: <stable@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);!

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

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

[Matthew Auld]

On 28/05/2025 14:06, Upadhyay, Tejas wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
>> Matthew Auld
>> Sent: 28 May 2025 17:03
>> To: intel-xe@lists.freedesktop.org
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Brost, Matthew
>> <matthew.brost@intel.com>; Tseng, William <william.tseng@intel.com>;
>> stable@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@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: William Tseng <william.tseng@intel.com>
>> Cc: <stable@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.

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

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

[Matthew Brost]

On Wed, May 28, 2025 at 03:29:17PM +0100, Matthew Auld wrote:
> On 28/05/2025 14:06, Upadhyay, Tejas wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > Matthew Auld
> > > Sent: 28 May 2025 17:03
> > > To: intel-xe@lists.freedesktop.org
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Brost, Matthew
> > > <matthew.brost@intel.com>; Tseng, William <william.tseng@intel.com>;
> > > stable@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@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: William Tseng <william.tseng@intel.com>
> > > Cc: <stable@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.
> 

Also I the hw_fence will be signaled if in an eror state - see
xe_hw_fence_signaled. I believe we could remove that check from run_job
now but can do that in a follow up.

Anyways, this patch LGTM:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

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

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

[Upadhyay, Tejas]

> -----Original Message-----
> From: Auld, Matthew <matthew.auld@intel.com>
> Sent: 28 May 2025 19:59
> To: Upadhyay, Tejas <tejas.upadhyay@intel.com>; intel-
> xe@lists.freedesktop.org
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Brost, Matthew
> <matthew.brost@intel.com>; Tseng, William <william.tseng@intel.com>;
> stable@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@lists.freedesktop.org> On Behalf Of
> >> Matthew Auld
> >> Sent: 28 May 2025 17:03
> >> To: intel-xe@lists.freedesktop.org
> >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Brost,
> >> Matthew <matthew.brost@intel.com>; Tseng, William
> >> <william.tseng@intel.com>; stable@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@intel.com>
> >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >> Cc: Matthew Brost <matthew.brost@intel.com>
> >> Cc: William Tseng <william.tseng@intel.com>
> >> Cc: <stable@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@intel.com>

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