diff mbox series

drm/i915/gt: reset RING_HEAD during intel_gt_unset_wedged

Message ID 20211220120030.4116079-1-tejaskumarx.surendrakumar.upadhyay@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/gt: reset RING_HEAD during intel_gt_unset_wedged | expand

Commit Message

Tejas Upadhyay Dec. 20, 2021, noon UTC
During repeated wedged-unwedged, it is
found that i915_request_retire zaps the old
request with 0x6b6b6b6b.

On unwedged, we write a new request at RING_TAIL,
expecting to start executuing from that position,
but execution resumes from RING_HEAD (preserved
from an earlier wakeup before wedging) and
consumes the 0x6b.

Resetting kernel/user context setup enables
RING_HEAD to use RING_TAIL for submitting new
requests which resolves issue. Normally this reset
is applied when unpinning a user context, or for
kernel_contexts upon waking up the device. But fast
wedged-unwedged sequence will keep the device awake,
preserving the RING_HEAD from before.

Testcase: igt@gem_eio@unwedge-stress

Note : Current user impact is assessed to be low, as
this only affects intel_gt_unset_wedged which is
currently only used during testing and upon suspend
resume (where the device was already flushed and will
reset the kernel_contexts on waking up). In the
future though, this will present an issue for PCI
error recovery.

Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 8 ++++++++
 drivers/gpu/drm/i915/gt/intel_reset.c     | 3 +++
 2 files changed, 11 insertions(+)

Comments

Thomas Hellström (Intel) Jan. 21, 2022, 10:04 a.m. UTC | #1
On 12/20/21 13:00, Tejas Upadhyay wrote:
> During repeated wedged-unwedged, it is
> found that i915_request_retire zaps the old
> request with 0x6b6b6b6b.
>
> On unwedged, we write a new request at RING_TAIL,
> expecting to start executuing from that position,
> but execution resumes from RING_HEAD (preserved
> from an earlier wakeup before wedging) and
> consumes the 0x6b.
>
> Resetting kernel/user context setup enables
> RING_HEAD to use RING_TAIL for submitting new
> requests which resolves issue. Normally this reset
> is applied when unpinning a user context, or for
> kernel_contexts upon waking up the device. But fast
> wedged-unwedged sequence will keep the device awake,
> preserving the RING_HEAD from before.
>
> Testcase: igt@gem_eio@unwedge-stress
>
> Note : Current user impact is assessed to be low, as
> this only affects intel_gt_unset_wedged which is
> currently only used during testing and upon suspend
> resume (where the device was already flushed and will
> reset the kernel_contexts on waking up). In the
> future though, this will present an issue for PCI
> error recovery.
>
> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 8 ++++++++
>   drivers/gpu/drm/i915/gt/intel_reset.c     | 3 +++
>   2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 352254e001b4..7e1c561bce69 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1455,9 +1455,17 @@ void intel_engines_reset_default_submission(struct intel_gt *gt)
>   	enum intel_engine_id id;
>   
>   	for_each_engine(engine, gt, id) {
> +		struct intel_context *ce = engine->kernel_context;
> +
>   		if (engine->sanitize)
>   			engine->sanitize(engine);
>   
> +		/* Reset RING_HEAD so we don't consume the old
> +		 * poisoned request on unwedging
> +		 */
> +		if (ce)
> +			ce->ops->reset(ce);
> +

Would it make sense to instead stop excluding the kernel context in 
intel_engine_reset_pinned_contexts, which is called from 
engine->sanitize(). That would lead to a double kernel context reset in 
some situations, though, but I figure the above would as well if the 
engine was parked before unwedging?

How is the PCI error recovery affected? Do we have have a plan to 
address that?

Thanks,

/Thomas


>   		engine->set_default_submission(engine);
>   	}
>   }
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 7be0002d9d70..1c26e936e699 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -961,6 +961,9 @@ static bool __intel_gt_unset_wedged(struct intel_gt *gt)
>   	}
>   	spin_unlock(&timelines->lock);
>   
> +	/* Ensure that all non-kernel contexts are unpinned as well */
> +	intel_gt_retire_requests(gt);
> +
>   	/* We must reset pending GPU events before restoring our submission */
>   	ok = !HAS_EXECLISTS(gt->i915); /* XXX better agnosticism desired */
>   	if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 352254e001b4..7e1c561bce69 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1455,9 +1455,17 @@  void intel_engines_reset_default_submission(struct intel_gt *gt)
 	enum intel_engine_id id;
 
 	for_each_engine(engine, gt, id) {
+		struct intel_context *ce = engine->kernel_context;
+
 		if (engine->sanitize)
 			engine->sanitize(engine);
 
+		/* Reset RING_HEAD so we don't consume the old
+		 * poisoned request on unwedging
+		 */
+		if (ce)
+			ce->ops->reset(ce);
+
 		engine->set_default_submission(engine);
 	}
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 7be0002d9d70..1c26e936e699 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -961,6 +961,9 @@  static bool __intel_gt_unset_wedged(struct intel_gt *gt)
 	}
 	spin_unlock(&timelines->lock);
 
+	/* Ensure that all non-kernel contexts are unpinned as well */
+	intel_gt_retire_requests(gt);
+
 	/* We must reset pending GPU events before restoring our submission */
 	ok = !HAS_EXECLISTS(gt->i915); /* XXX better agnosticism desired */
 	if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)