[PATCH] drm/i915/execlists: Always reset the context's RING registers

* [PATCH] drm/i915/execlists: Always reset the context's RING registers
@ 2019-04-08 13:15 Chris Wilson
  2019-04-08 13:40 ` Chris Wilson
                   ` (11 more replies)
  0 siblings, 12 replies; 14+ messages in thread
From: Chris Wilson @ 2019-04-08 13:15 UTC (permalink / raw)
  To: intel-gfx

During reset, we try and stop the active ring. This has the consequence
that we often clobber the RING registers within the context image. When
we find an active request, we update the context image to rerun that
request (if it was guilty, we replace the hanging user payload with
NOPs). However, we were ignoring an active context if the request had
completed, with the consequence that the next submission on that request
would start with RING_HEAD==0 and not the tail of the previous request,
causing all requests still in the ring to be rerun. Rare, but
occasionally seen within CI where we would spot that the context seqno
would reverse and complain that we were retiring an incomplete request.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 46 +++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6931dbb2888c..76095f40cdd9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1904,7 +1904,6 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
 
 	/* And flush any current direct submission. */
 	spin_lock_irqsave(&engine->timeline.lock, flags);
-	process_csb(engine); /* drain preemption events */
 	spin_unlock_irqrestore(&engine->timeline.lock, flags);
 }
 
@@ -1928,12 +1927,29 @@ static bool lrc_regs_ok(const struct i915_request *rq)
 static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
 {
 	struct intel_engine_execlists * const execlists = &engine->execlists;
-	struct i915_request *rq;
+	struct i915_request *rq = NULL;
+	struct intel_context *ce;
 	unsigned long flags;
 	u32 *regs;
 
 	spin_lock_irqsave(&engine->timeline.lock, flags);
 
+	GEM_TRACE("%s\n", engine->name);
+	process_csb(engine); /* drain preemption events */
+
+	/* Following the reset, we need to reload the CSB read/write pointers */
+	reset_csb_pointers(&engine->execlists);
+
+	/*
+	 * Save the currently executing context, even if we completed
+	 * it's request, it was still running at the time of the
+	 * reset and will have been clobbered.
+	 */
+	if (!port_isset(execlists->port))
+		goto out_unlock;
+
+	ce = port_request(execlists->port)->hw_context;
+
 	/*
 	 * Catch up with any missed context-switch interrupts.
 	 *
@@ -1947,12 +1963,10 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
 
 	/* Push back any incomplete requests for replay after the reset. */
 	rq = __unwind_incomplete_requests(engine);
-
-	/* Following the reset, we need to reload the CSB read/write pointers */
-	reset_csb_pointers(&engine->execlists);
-
 	if (!rq)
-		goto out_unlock;
+		goto out_replay;
+
+	GEM_BUG_ON(rq->hw_context != ce);
 
 	/*
 	 * If this request hasn't started yet, e.g. it is waiting on a
@@ -1967,7 +1981,7 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	 * perfectly and we do not need to flag the result as being erroneous.
 	 */
 	if (!i915_request_started(rq) && lrc_regs_ok(rq))
-		goto out_unlock;
+		goto out_replay;
 
 	/*
 	 * If the request was innocent, we leave the request in the ELSP
@@ -1982,7 +1996,7 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	 */
 	i915_reset_request(rq, stalled);
 	if (!stalled && lrc_regs_ok(rq))
-		goto out_unlock;
+		goto out_replay;
 
 	/*
 	 * We want a simple context + ring to execute the breadcrumb update.
@@ -1992,21 +2006,23 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
 	 * future request will be after userspace has had the opportunity
 	 * to recreate its own state.
 	 */
-	regs = rq->hw_context->lrc_reg_state;
+	regs = ce->lrc_reg_state;
 	if (engine->pinned_default_state) {
 		memcpy(regs, /* skip restoring the vanilla PPHWSP */
 		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
 		       engine->context_size - PAGE_SIZE);
 	}
+	execlists_init_reg_state(regs, ce, engine, ce->ring);
 
 	/* Rerun the request; its payload has been neutered (if guilty). */
-	rq->ring->head = intel_ring_wrap(rq->ring, rq->head);
-	intel_ring_update_space(rq->ring);
-
-	execlists_init_reg_state(regs, rq->hw_context, engine, rq->ring);
-	__execlists_update_reg_state(rq->hw_context, engine);
+out_replay:
+	ce->ring->head =
+		intel_ring_wrap(ce->ring, rq ? rq->head : ce->ring->tail);
+	intel_ring_update_space(ce->ring);
+	__execlists_update_reg_state(ce, engine);
 
 out_unlock:
+	execlists_clear_all_active(execlists);
 	spin_unlock_irqrestore(&engine->timeline.lock, flags);
 }
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread