[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

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