[PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking

[PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking

[Chris Wilson]

If the request->wa_tail is 0 (because it landed exactly on the end of
the ringbuffer), when we reconstruct request->tail following a reset we
fill in an illegal value (-8 or 0x001ffff8). As a result, RING_HEAD is
never able to catch up with RING_TAIL and the GPU spins endlessly. If
the ring contains a couple of breadcrumbs, even our hangcheck is unable
to catch the busy-looping as the ACTHD and seqno continually advance.

v2: Move the wrap into a common intel_ring_wrap().

Fixes: a3aabe86a340 ("drm/i915/execlists: Reinitialise context image after GPU hang")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: <stable@vger.kernel.org> # v4.10+
---
 drivers/gpu/drm/i915/intel_lrc.c        |  4 +++-
 drivers/gpu/drm/i915/intel_ringbuffer.h | 10 ++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b75df70e8e0e..32fb8ad3fd36 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1278,7 +1278,9 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	GEM_BUG_ON(request->ctx != port[0].request->ctx);
 
 	/* Reset WaIdleLiteRestore:bdw,skl as well */
-	request->tail = request->wa_tail - WA_TAIL_DWORDS * sizeof(u32);
+	request->tail =
+		intel_ring_wrap(request->ring,
+				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
 	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 166aa1ae65cf..17ac44980d84 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -515,12 +515,18 @@ intel_ring_advance(struct drm_i915_gem_request *req, u32 *cs)
 }
 
 static inline u32
-intel_ring_offset(struct drm_i915_gem_request *req, void *addr)
+intel_ring_wrap(const struct intel_ring *ring, u32 pos)
+{
+	return pos & (ring->size - 1);
+}
+
+static inline u32
+intel_ring_offset(const struct drm_i915_gem_request *req, void *addr)
 {
 	/* Don't write ring->size (equivalent to 0) as that hangs some GPUs. */
 	u32 offset = addr - req->ring->vaddr;
 	GEM_BUG_ON(offset > req->ring->size);
-	return offset & (req->ring->size - 1);
+	return intel_ring_wrap(req->ring, offset);
 }
 
 void intel_ring_update_space(struct intel_ring *ring);
-- 
2.11.0

[PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring

[Chris Wilson]

In addition to being qword-aligned, the RING_TAIL offset must be within
the ring!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 4 ++++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 32fb8ad3fd36..39cb84a5d4e4 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -327,6 +327,7 @@ static u64 execlists_update_context(struct drm_i915_gem_request *rq)
 	u32 *reg_state = ce->lrc_reg_state;
 
 	GEM_BUG_ON(!IS_ALIGNED(rq->tail, 8));
+	GEM_BUG_ON(rq->tail >= rq->ring->size);
 	reg_state[CTX_RING_TAIL+1] = rq->tail;
 
 	/* True 32b PPGTT with dynamic page allocation: update PDP
@@ -1282,6 +1283,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 		intel_ring_wrap(request->ring,
 				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
 	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
+	GEM_BUG_ON(request->tail >= request->ring->size);
 }
 
 static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
@@ -1493,6 +1495,7 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
 	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
+	GEM_BUG_ON(request->tail >= request->ring->size);
 
 	gen8_emit_wa_tail(request, cs);
 }
@@ -1521,6 +1524,7 @@ static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
 	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
+	GEM_BUG_ON(request->tail >= request->ring->size);
 
 	gen8_emit_wa_tail(request, cs);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4729ac7ac122..47921dcbedb3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -775,6 +775,7 @@ static void i9xx_submit_request(struct drm_i915_gem_request *request)
 	i915_gem_request_submit(request);
 
 	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
+	GEM_BUG_ON(request->tail >= request->ring->size);
 	I915_WRITE_TAIL(request->engine, request->tail);
 }
 
@@ -787,6 +788,7 @@ static void i9xx_emit_breadcrumb(struct drm_i915_gem_request *req, u32 *cs)
 
 	req->tail = intel_ring_offset(req, cs);
 	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
+	GEM_BUG_ON(req->tail >= req->ring->size);
 }
 
 static const int i9xx_emit_breadcrumb_sz = 4;
@@ -826,6 +828,7 @@ static void gen8_render_emit_breadcrumb(struct drm_i915_gem_request *req,
 
 	req->tail = intel_ring_offset(req, cs);
 	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
+	GEM_BUG_ON(req->tail >= req->ring->size);
 }
 
 static const int gen8_render_emit_breadcrumb_sz = 8;
-- 
2.11.0

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

Re: [PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> In addition to being qword-aligned, the RING_TAIL offset must be within
> the ring!
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_lrc.c        | 4 ++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 32fb8ad3fd36..39cb84a5d4e4 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -327,6 +327,7 @@ static u64 execlists_update_context(struct drm_i915_gem_request *rq)
>  	u32 *reg_state = ce->lrc_reg_state;
>  
>  	GEM_BUG_ON(!IS_ALIGNED(rq->tail, 8));
> +	GEM_BUG_ON(rq->tail >= rq->ring->size);
>  	reg_state[CTX_RING_TAIL+1] = rq->tail;
>  
>  	/* True 32b PPGTT with dynamic page allocation: update PDP
> @@ -1282,6 +1283,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>  		intel_ring_wrap(request->ring,
>  				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
>  	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> +	GEM_BUG_ON(request->tail >= request->ring->size);
>  }
>  
>  static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
> @@ -1493,6 +1495,7 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
>  	*cs++ = MI_NOOP;
>  	request->tail = intel_ring_offset(request, cs);
>  	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> +	GEM_BUG_ON(request->tail >= request->ring->size);
>  
>  	gen8_emit_wa_tail(request, cs);
>  }
> @@ -1521,6 +1524,7 @@ static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
>  	*cs++ = MI_NOOP;
>  	request->tail = intel_ring_offset(request, cs);
>  	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> +	GEM_BUG_ON(request->tail >= request->ring->size);
>  
>  	gen8_emit_wa_tail(request, cs);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4729ac7ac122..47921dcbedb3 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -775,6 +775,7 @@ static void i9xx_submit_request(struct drm_i915_gem_request *request)
>  	i915_gem_request_submit(request);
>  
>  	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> +	GEM_BUG_ON(request->tail >= request->ring->size);
>  	I915_WRITE_TAIL(request->engine, request->tail);
>  }
>  
> @@ -787,6 +788,7 @@ static void i9xx_emit_breadcrumb(struct drm_i915_gem_request *req, u32 *cs)
>  
>  	req->tail = intel_ring_offset(req, cs);
>  	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
> +	GEM_BUG_ON(req->tail >= req->ring->size);
>  }
>  
>  static const int i9xx_emit_breadcrumb_sz = 4;
> @@ -826,6 +828,7 @@ static void gen8_render_emit_breadcrumb(struct drm_i915_gem_request *req,
>  
>  	req->tail = intel_ring_offset(req, cs);
>  	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
> +	GEM_BUG_ON(req->tail >= req->ring->size);
>  }
>  
>  static const int gen8_render_emit_breadcrumb_sz = 8;
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm/i915: Refactor tests for validity of RING_TAIL

[Chris Wilson]

Whilst I like having the assertions clearly visible in the code, they
are quite repetitious! As we find new limits we want to incorporate into
the set of assertions, it make sense to refactor them to a common
routine.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 12 ++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.c |  9 +++------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 11 +++++++++++
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 39cb84a5d4e4..b0c3a029b592 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -326,8 +326,7 @@ static u64 execlists_update_context(struct drm_i915_gem_request *rq)
 		rq->ctx->ppgtt ?: rq->i915->mm.aliasing_ppgtt;
 	u32 *reg_state = ce->lrc_reg_state;
 
-	GEM_BUG_ON(!IS_ALIGNED(rq->tail, 8));
-	GEM_BUG_ON(rq->tail >= rq->ring->size);
+	assert_ring_tail_valid(rq->ring, rq->tail);
 	reg_state[CTX_RING_TAIL+1] = rq->tail;
 
 	/* True 32b PPGTT with dynamic page allocation: update PDP
@@ -1282,8 +1281,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	request->tail =
 		intel_ring_wrap(request->ring,
 				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 }
 
 static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
@@ -1494,8 +1492,7 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
 	*cs++ = MI_USER_INTERRUPT;
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 
 	gen8_emit_wa_tail(request, cs);
 }
@@ -1523,8 +1520,7 @@ static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
 	*cs++ = MI_USER_INTERRUPT;
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 
 	gen8_emit_wa_tail(request, cs);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 47921dcbedb3..66a2b8b83972 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -774,8 +774,7 @@ static void i9xx_submit_request(struct drm_i915_gem_request *request)
 
 	i915_gem_request_submit(request);
 
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 	I915_WRITE_TAIL(request->engine, request->tail);
 }
 
@@ -787,8 +786,7 @@ static void i9xx_emit_breadcrumb(struct drm_i915_gem_request *req, u32 *cs)
 	*cs++ = MI_USER_INTERRUPT;
 
 	req->tail = intel_ring_offset(req, cs);
-	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
-	GEM_BUG_ON(req->tail >= req->ring->size);
+	assert_ring_tail_valid(req->ring, req->tail);
 }
 
 static const int i9xx_emit_breadcrumb_sz = 4;
@@ -827,8 +825,7 @@ static void gen8_render_emit_breadcrumb(struct drm_i915_gem_request *req,
 	*cs++ = MI_NOOP;
 
 	req->tail = intel_ring_offset(req, cs);
-	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
-	GEM_BUG_ON(req->tail >= req->ring->size);
+	assert_ring_tail_valid(req->ring, req->tail);
 }
 
 static const int gen8_render_emit_breadcrumb_sz = 8;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 17ac44980d84..a82a0807f64d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -529,6 +529,17 @@ intel_ring_offset(const struct drm_i915_gem_request *req, void *addr)
 	return intel_ring_wrap(req->ring, offset);
 }
 
+static inline void
+assert_ring_tail_valid(const struct intel_ring *ring, unsigned int tail)
+{
+	/* We could combine these into a single tail operation, but keeping
+	 * them as seperate tests will help identify the cause should one
+	 * ever fire.
+	 */
+	GEM_BUG_ON(!IS_ALIGNED(tail, 8));
+	GEM_BUG_ON(tail >= ring->size);
+}
+
 void intel_ring_update_space(struct intel_ring *ring);
 
 void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno);
-- 
2.11.0

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

[PATCH v3] drm/i915: Refactor tests for validity of RING_TAIL

[Chris Wilson]

Whilst I like having the assertions clearly visible in the code, they
are quite repetitious! As we find new limits we want to incorporate into
the set of assertions, it make sense to refactor them to a common
routine.

v2: Add a guc holdout.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c           | 12 ++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  9 +++------
 drivers/gpu/drm/i915/intel_ringbuffer.h    | 11 +++++++++++
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 991e76e10f82..6193ad7edcf4 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -481,7 +481,7 @@ static void guc_wq_item_append(struct i915_guc_client *client,
 
 	/* The GuC firmware wants the tail index in QWords, not bytes */
 	tail = rq->tail;
-	GEM_BUG_ON(tail & 7);
+	assert_ring_tail_valid(rq->ring, rq->tail);
 	tail >>= 3;
 	GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 39cb84a5d4e4..b0c3a029b592 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -326,8 +326,7 @@ static u64 execlists_update_context(struct drm_i915_gem_request *rq)
 		rq->ctx->ppgtt ?: rq->i915->mm.aliasing_ppgtt;
 	u32 *reg_state = ce->lrc_reg_state;
 
-	GEM_BUG_ON(!IS_ALIGNED(rq->tail, 8));
-	GEM_BUG_ON(rq->tail >= rq->ring->size);
+	assert_ring_tail_valid(rq->ring, rq->tail);
 	reg_state[CTX_RING_TAIL+1] = rq->tail;
 
 	/* True 32b PPGTT with dynamic page allocation: update PDP
@@ -1282,8 +1281,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	request->tail =
 		intel_ring_wrap(request->ring,
 				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 }
 
 static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
@@ -1494,8 +1492,7 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
 	*cs++ = MI_USER_INTERRUPT;
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 
 	gen8_emit_wa_tail(request, cs);
 }
@@ -1523,8 +1520,7 @@ static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
 	*cs++ = MI_USER_INTERRUPT;
 	*cs++ = MI_NOOP;
 	request->tail = intel_ring_offset(request, cs);
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 
 	gen8_emit_wa_tail(request, cs);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 47921dcbedb3..66a2b8b83972 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -774,8 +774,7 @@ static void i9xx_submit_request(struct drm_i915_gem_request *request)
 
 	i915_gem_request_submit(request);
 
-	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
-	GEM_BUG_ON(request->tail >= request->ring->size);
+	assert_ring_tail_valid(request->ring, request->tail);
 	I915_WRITE_TAIL(request->engine, request->tail);
 }
 
@@ -787,8 +786,7 @@ static void i9xx_emit_breadcrumb(struct drm_i915_gem_request *req, u32 *cs)
 	*cs++ = MI_USER_INTERRUPT;
 
 	req->tail = intel_ring_offset(req, cs);
-	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
-	GEM_BUG_ON(req->tail >= req->ring->size);
+	assert_ring_tail_valid(req->ring, req->tail);
 }
 
 static const int i9xx_emit_breadcrumb_sz = 4;
@@ -827,8 +825,7 @@ static void gen8_render_emit_breadcrumb(struct drm_i915_gem_request *req,
 	*cs++ = MI_NOOP;
 
 	req->tail = intel_ring_offset(req, cs);
-	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
-	GEM_BUG_ON(req->tail >= req->ring->size);
+	assert_ring_tail_valid(req->ring, req->tail);
 }
 
 static const int gen8_render_emit_breadcrumb_sz = 8;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 17ac44980d84..a82a0807f64d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -529,6 +529,17 @@ intel_ring_offset(const struct drm_i915_gem_request *req, void *addr)
 	return intel_ring_wrap(req->ring, offset);
 }
 
+static inline void
+assert_ring_tail_valid(const struct intel_ring *ring, unsigned int tail)
+{
+	/* We could combine these into a single tail operation, but keeping
+	 * them as seperate tests will help identify the cause should one
+	 * ever fire.
+	 */
+	GEM_BUG_ON(!IS_ALIGNED(tail, 8));
+	GEM_BUG_ON(tail >= ring->size);
+}
+
 void intel_ring_update_space(struct intel_ring *ring);
 
 void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno);
-- 
2.11.0

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

Re: [PATCH v3] drm/i915: Refactor tests for validity of RING_TAIL

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Whilst I like having the assertions clearly visible in the code, they
> are quite repetitious! As we find new limits we want to incorporate into
> the set of assertions, it make sense to refactor them to a common
> routine.
>
> v2: Add a guc holdout.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
>  drivers/gpu/drm/i915/intel_lrc.c           | 12 ++++--------
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  9 +++------
>  drivers/gpu/drm/i915/intel_ringbuffer.h    | 11 +++++++++++
>  4 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 991e76e10f82..6193ad7edcf4 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -481,7 +481,7 @@ static void guc_wq_item_append(struct i915_guc_client *client,
>  
>  	/* The GuC firmware wants the tail index in QWords, not bytes */
>  	tail = rq->tail;
> -	GEM_BUG_ON(tail & 7);
> +	assert_ring_tail_valid(rq->ring, rq->tail);
>  	tail >>= 3;
>  	GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 39cb84a5d4e4..b0c3a029b592 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -326,8 +326,7 @@ static u64 execlists_update_context(struct drm_i915_gem_request *rq)
>  		rq->ctx->ppgtt ?: rq->i915->mm.aliasing_ppgtt;
>  	u32 *reg_state = ce->lrc_reg_state;
>  
> -	GEM_BUG_ON(!IS_ALIGNED(rq->tail, 8));
> -	GEM_BUG_ON(rq->tail >= rq->ring->size);
> +	assert_ring_tail_valid(rq->ring, rq->tail);
>  	reg_state[CTX_RING_TAIL+1] = rq->tail;
>  
>  	/* True 32b PPGTT with dynamic page allocation: update PDP
> @@ -1282,8 +1281,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>  	request->tail =
>  		intel_ring_wrap(request->ring,
>  				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
> -	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> -	GEM_BUG_ON(request->tail >= request->ring->size);
> +	assert_ring_tail_valid(request->ring, request->tail);
>  }
>  
>  static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
> @@ -1494,8 +1492,7 @@ static void gen8_emit_breadcrumb(struct drm_i915_gem_request *request, u32 *cs)
>  	*cs++ = MI_USER_INTERRUPT;
>  	*cs++ = MI_NOOP;
>  	request->tail = intel_ring_offset(request, cs);
> -	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> -	GEM_BUG_ON(request->tail >= request->ring->size);
> +	assert_ring_tail_valid(request->ring, request->tail);
>  
>  	gen8_emit_wa_tail(request, cs);
>  }
> @@ -1523,8 +1520,7 @@ static void gen8_emit_breadcrumb_render(struct drm_i915_gem_request *request,
>  	*cs++ = MI_USER_INTERRUPT;
>  	*cs++ = MI_NOOP;
>  	request->tail = intel_ring_offset(request, cs);
> -	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> -	GEM_BUG_ON(request->tail >= request->ring->size);
> +	assert_ring_tail_valid(request->ring, request->tail);
>  
>  	gen8_emit_wa_tail(request, cs);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 47921dcbedb3..66a2b8b83972 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -774,8 +774,7 @@ static void i9xx_submit_request(struct drm_i915_gem_request *request)
>  
>  	i915_gem_request_submit(request);
>  
> -	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
> -	GEM_BUG_ON(request->tail >= request->ring->size);
> +	assert_ring_tail_valid(request->ring, request->tail);
>  	I915_WRITE_TAIL(request->engine, request->tail);
>  }
>  
> @@ -787,8 +786,7 @@ static void i9xx_emit_breadcrumb(struct drm_i915_gem_request *req, u32 *cs)
>  	*cs++ = MI_USER_INTERRUPT;
>  
>  	req->tail = intel_ring_offset(req, cs);
> -	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
> -	GEM_BUG_ON(req->tail >= req->ring->size);
> +	assert_ring_tail_valid(req->ring, req->tail);
>  }
>  
>  static const int i9xx_emit_breadcrumb_sz = 4;
> @@ -827,8 +825,7 @@ static void gen8_render_emit_breadcrumb(struct drm_i915_gem_request *req,
>  	*cs++ = MI_NOOP;
>  
>  	req->tail = intel_ring_offset(req, cs);
> -	GEM_BUG_ON(!IS_ALIGNED(req->tail, 8));
> -	GEM_BUG_ON(req->tail >= req->ring->size);
> +	assert_ring_tail_valid(req->ring, req->tail);
>  }
>  
>  static const int gen8_render_emit_breadcrumb_sz = 8;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 17ac44980d84..a82a0807f64d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -529,6 +529,17 @@ intel_ring_offset(const struct drm_i915_gem_request *req, void *addr)
>  	return intel_ring_wrap(req->ring, offset);
>  }
>  
> +static inline void
> +assert_ring_tail_valid(const struct intel_ring *ring, unsigned int tail)
> +{
> +	/* We could combine these into a single tail operation, but keeping
> +	 * them as seperate tests will help identify the cause should one
> +	 * ever fire.
> +	 */
> +	GEM_BUG_ON(!IS_ALIGNED(tail, 8));
> +	GEM_BUG_ON(tail >= ring->size);
> +}
> +
>  void intel_ring_update_space(struct intel_ring *ring);
>  
>  void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno);
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> If the request->wa_tail is 0 (because it landed exactly on the end of
> the ringbuffer), when we reconstruct request->tail following a reset we
> fill in an illegal value (-8 or 0x001ffff8). As a result, RING_HEAD is
> never able to catch up with RING_TAIL and the GPU spins endlessly. If
> the ring contains a couple of breadcrumbs, even our hangcheck is unable
> to catch the busy-looping as the ACTHD and seqno continually advance.
>
> v2: Move the wrap into a common intel_ring_wrap().
>
> Fixes: a3aabe86a340 ("drm/i915/execlists: Reinitialise context image after GPU hang")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: <stable@vger.kernel.org> # v4.10+

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_lrc.c        |  4 +++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 10 ++++++++--
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b75df70e8e0e..32fb8ad3fd36 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1278,7 +1278,9 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>  	GEM_BUG_ON(request->ctx != port[0].request->ctx);
>  
>  	/* Reset WaIdleLiteRestore:bdw,skl as well */
> -	request->tail = request->wa_tail - WA_TAIL_DWORDS * sizeof(u32);
> +	request->tail =
> +		intel_ring_wrap(request->ring,
> +				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
>  	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 166aa1ae65cf..17ac44980d84 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -515,12 +515,18 @@ intel_ring_advance(struct drm_i915_gem_request *req, u32 *cs)
>  }
>  
>  static inline u32
> -intel_ring_offset(struct drm_i915_gem_request *req, void *addr)
> +intel_ring_wrap(const struct intel_ring *ring, u32 pos)
> +{
> +	return pos & (ring->size - 1);
> +}
> +
> +static inline u32
> +intel_ring_offset(const struct drm_i915_gem_request *req, void *addr)
>  {
>  	/* Don't write ring->size (equivalent to 0) as that hangs some GPUs. */
>  	u32 offset = addr - req->ring->vaddr;
>  	GEM_BUG_ON(offset > req->ring->size);
> -	return offset & (req->ring->size - 1);
> +	return intel_ring_wrap(req->ring, offset);
>  }
>  
>  void intel_ring_update_space(struct intel_ring *ring);
> -- 
> 2.11.0

Re: [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> If the request->wa_tail is 0 (because it landed exactly on the end of
> the ringbuffer), when we reconstruct request->tail following a reset we
> fill in an illegal value (-8 or 0x001ffff8). As a result, RING_HEAD is
> never able to catch up with RING_TAIL and the GPU spins endlessly. If
> the ring contains a couple of breadcrumbs, even our hangcheck is unable
> to catch the busy-looping as the ACTHD and seqno continually advance.
>
> v2: Move the wrap into a common intel_ring_wrap().
>
> Fixes: a3aabe86a340 ("drm/i915/execlists: Reinitialise context image after GPU hang")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: <stable@vger.kernel.org> # v4.10+

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_lrc.c        |  4 +++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 10 ++++++++--
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b75df70e8e0e..32fb8ad3fd36 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1278,7 +1278,9 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>  	GEM_BUG_ON(request->ctx != port[0].request->ctx);
>  
>  	/* Reset WaIdleLiteRestore:bdw,skl as well */
> -	request->tail = request->wa_tail - WA_TAIL_DWORDS * sizeof(u32);
> +	request->tail =
> +		intel_ring_wrap(request->ring,
> +				request->wa_tail - WA_TAIL_DWORDS*sizeof(u32));
>  	GEM_BUG_ON(!IS_ALIGNED(request->tail, 8));
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 166aa1ae65cf..17ac44980d84 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -515,12 +515,18 @@ intel_ring_advance(struct drm_i915_gem_request *req, u32 *cs)
>  }
>  
>  static inline u32
> -intel_ring_offset(struct drm_i915_gem_request *req, void *addr)
> +intel_ring_wrap(const struct intel_ring *ring, u32 pos)
> +{
> +	return pos & (ring->size - 1);
> +}
> +
> +static inline u32
> +intel_ring_offset(const struct drm_i915_gem_request *req, void *addr)
>  {
>  	/* Don't write ring->size (equivalent to 0) as that hangs some GPUs. */
>  	u32 offset = addr - req->ring->vaddr;
>  	GEM_BUG_ON(offset > req->ring->size);
> -	return offset & (req->ring->size - 1);
> +	return intel_ring_wrap(req->ring, offset);
>  }
>  
>  void intel_ring_update_space(struct intel_ring *ring);
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2)
URL   : https://patchwork.freedesktop.org/series/21930/
State : success

== Summary ==

Series 21930v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/21930/revisions/2/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 460s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 453s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 582s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 543s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 558s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 509s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 502s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 441s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 436s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 509s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 489s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 484s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 596s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 497s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 603s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 496s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 516s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 463s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 556s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 422s

379767e3f8c8b89b43cfb07d6fa9e6fbcb2a028b drm-tip: 2017y-03m-27d-13h-31m-17s UTC integration manifest
3f7e423 drm/i915: Refactor tests for validity of RING_TAIL
6a34c78 drm/i915: Assert that the request->tail fits within the ring
1d79e91 drm/i915/execlists: Wrap tail pointer after reset tweaking

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4314/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2)

[Chris Wilson]

On Mon, Mar 27, 2017 at 02:11:27PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2)
> URL   : https://patchwork.freedesktop.org/series/21930/
> State : success
> 
> == Summary ==
> 
> Series 21930v2 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/21930/revisions/2/mbox/

Thanks for the review. For the record, this is the first time
gem_exec_whisper/hang has run to completion!

Subtest hang-normal: SUCCESS (10256.387s)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx