Re: [Intel-gfx] [PATCH] drm/i915/guc: clear stalled request after a reset
On 12/08/2022 16:31, Ceraolo Spurio, Daniele wrote:
On 8/12/2022 12:29 AM, Tvrtko Ursulin wrote:
On 11/08/2022 22:08, Daniele Ceraolo Spurio wrote:
If the GuC CTs are full and we need to stall the request submission
while waiting for space, we save the stalled request and where the stall
occurred; when the CTs have space again we pick up the request
submission
from where we left off.
How serious is it? Statement always was CT buffers can never get full
outside the pathological IGT test cases. So I am wondering if this is
in the category of fix for correctness or actually the CT buffers can
get full in normal use so it is imperative to fix.
The CT buffers being full is indeed something that is normally only
observed with IGTs that hammer the submission path, but it is still
something that a user can do so IMO we do have to fix it. However, the
bug is still extremely unlikely to happen out in the wild as it needs 2
relatively rare things to happen:
- We need to hit the pathological case of the GuC CTs being full and the
stall kicking in
- Something needs to go wrong and escalated to a full GT reset
The bug report that triggered my investigation into this came from what
look like faulty HW: the HW seems to suddenly just stop with no errors
anywhere, which leads to the buffers filling up because the GuC is no
longer processing them, followed by a GT reset as we try to recover the
HW. To replicate this locally I had to add a debugfs to kill the GuC in
the middle of the test to simulate this "HW silently dies" scenario.
Ack. Given the activity around pin/unpin performance issues I was
briefly worried this is another one which was "upgraded" from only IGT
to something in the real world after all. Thanks for confirming that is
not the case.
Regards,
Tvrtko
Daniele
Regards,
Tvrtko
If a full GT reset occurs, the state of all contexts is cleared and all
non-guilty requests are unsubmitted, therefore we need to restart the
stalled request submission from scratch. To make sure that we do so,
clear the saved request after a reset.
Fixes note: the patch that introduced the bug is in 5.15, but no
officially supported platform had GuC submission enabled by default
in that kernel, so the backport to that particular version (and only
that one) can potentially be skipped.
Fixes: 925dc1cf58ed ("drm/i915/guc: Implement GuC submission tasklet")
Signed-off-by: Daniele Ceraolo Spurio
Cc: Matthew Brost
Cc: John Harrison
Cc: # v5.15+
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0d17da77e787..0d56b615bf78 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -4002,6 +4002,13 @@ static inline void guc_init_lrc_mapping(struct
intel_guc *guc)
/* make sure all descriptors are clean... */
xa_destroy(>context_lookup);
+ /*
+ * A reset might have occurred while we had a pending stalled
request,
+ * so make sure we clean that up.
+ */
+ guc->stalled_request = NULL;
+ guc->submission_stall_reason = STALL_NONE;
+
/*
* Some contexts might have been pinned before we enabled GuC
* submission, so we need to add them to the GuC bookeeping.
Re: [Intel-gfx] [PATCH] drm/i915/guc: clear stalled request after a reset
On 8/12/2022 12:29 AM, Tvrtko Ursulin wrote:
On 11/08/2022 22:08, Daniele Ceraolo Spurio wrote:
If the GuC CTs are full and we need to stall the request submission
while waiting for space, we save the stalled request and where the stall
occurred; when the CTs have space again we pick up the request
submission
from where we left off.
How serious is it? Statement always was CT buffers can never get full
outside the pathological IGT test cases. So I am wondering if this is
in the category of fix for correctness or actually the CT buffers can
get full in normal use so it is imperative to fix.
The CT buffers being full is indeed something that is normally only
observed with IGTs that hammer the submission path, but it is still
something that a user can do so IMO we do have to fix it. However, the
bug is still extremely unlikely to happen out in the wild as it needs 2
relatively rare things to happen:
- We need to hit the pathological case of the GuC CTs being full and the
stall kicking in
- Something needs to go wrong and escalated to a full GT reset
The bug report that triggered my investigation into this came from what
look like faulty HW: the HW seems to suddenly just stop with no errors
anywhere, which leads to the buffers filling up because the GuC is no
longer processing them, followed by a GT reset as we try to recover the
HW. To replicate this locally I had to add a debugfs to kill the GuC in
the middle of the test to simulate this "HW silently dies" scenario.
Daniele
Regards,
Tvrtko
If a full GT reset occurs, the state of all contexts is cleared and all
non-guilty requests are unsubmitted, therefore we need to restart the
stalled request submission from scratch. To make sure that we do so,
clear the saved request after a reset.
Fixes note: the patch that introduced the bug is in 5.15, but no
officially supported platform had GuC submission enabled by default
in that kernel, so the backport to that particular version (and only
that one) can potentially be skipped.
Fixes: 925dc1cf58ed ("drm/i915/guc: Implement GuC submission tasklet")
Signed-off-by: Daniele Ceraolo Spurio
Cc: Matthew Brost
Cc: John Harrison
Cc: # v5.15+
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0d17da77e787..0d56b615bf78 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -4002,6 +4002,13 @@ static inline void guc_init_lrc_mapping(struct
intel_guc *guc)
/* make sure all descriptors are clean... */
xa_destroy(>context_lookup);
+ /*
+ * A reset might have occurred while we had a pending stalled
request,
+ * so make sure we clean that up.
+ */
+ guc->stalled_request = NULL;
+ guc->submission_stall_reason = STALL_NONE;
+
/*
* Some contexts might have been pinned before we enabled GuC
* submission, so we need to add them to the GuC bookeeping.
Re: [Intel-gfx] [PATCH] drm/i915/guc: clear stalled request after a reset
On 11/08/2022 22:08, Daniele Ceraolo Spurio wrote:
If the GuC CTs are full and we need to stall the request submission
while waiting for space, we save the stalled request and where the stall
occurred; when the CTs have space again we pick up the request submission
from where we left off.
How serious is it? Statement always was CT buffers can never get full
outside the pathological IGT test cases. So I am wondering if this is in
the category of fix for correctness or actually the CT buffers can get
full in normal use so it is imperative to fix.
Regards,
Tvrtko
If a full GT reset occurs, the state of all contexts is cleared and all
non-guilty requests are unsubmitted, therefore we need to restart the
stalled request submission from scratch. To make sure that we do so,
clear the saved request after a reset.
Fixes note: the patch that introduced the bug is in 5.15, but no
officially supported platform had GuC submission enabled by default
in that kernel, so the backport to that particular version (and only
that one) can potentially be skipped.
Fixes: 925dc1cf58ed ("drm/i915/guc: Implement GuC submission tasklet")
Signed-off-by: Daniele Ceraolo Spurio
Cc: Matthew Brost
Cc: John Harrison
Cc: # v5.15+
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0d17da77e787..0d56b615bf78 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -4002,6 +4002,13 @@ static inline void guc_init_lrc_mapping(struct intel_guc
*guc)
/* make sure all descriptors are clean... */
xa_destroy(>context_lookup);
+ /*
+* A reset might have occurred while we had a pending stalled request,
+* so make sure we clean that up.
+*/
+ guc->stalled_request = NULL;
+ guc->submission_stall_reason = STALL_NONE;
+
/*
* Some contexts might have been pinned before we enabled GuC
* submission, so we need to add them to the GuC bookeeping.
