* [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking @ 2017-03-27 13:00 Chris Wilson 2017-03-27 13:00 ` [PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring Chris Wilson ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Chris Wilson @ 2017-03-27 13:00 UTC (permalink / raw) To: intel-gfx Cc: mika.kuoppala, tvrtko.ursulin, Chris Wilson, Mika Kuoppala, # v4 . 10+ 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring 2017-03-27 13:00 [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking Chris Wilson @ 2017-03-27 13:00 ` Chris Wilson 2017-03-27 13:57 ` Mika Kuoppala 2017-03-27 13:00 ` [PATCH v2 3/3] drm/i915: Refactor tests for validity of RING_TAIL Chris Wilson ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2017-03-27 13:00 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring 2017-03-27 13:00 ` [PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring Chris Wilson @ 2017-03-27 13:57 ` Mika Kuoppala 0 siblings, 0 replies; 10+ messages in thread From: Mika Kuoppala @ 2017-03-27 13:57 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] drm/i915: Refactor tests for validity of RING_TAIL 2017-03-27 13:00 [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking Chris Wilson 2017-03-27 13:00 ` [PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring Chris Wilson @ 2017-03-27 13:00 ` Chris Wilson 2017-03-27 13:14 ` [PATCH v3] " Chris Wilson 2017-03-27 13:05 ` Mika Kuoppala 2017-03-27 14:11 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2) Patchwork 3 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2017-03-27 13:00 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3] drm/i915: Refactor tests for validity of RING_TAIL 2017-03-27 13:00 ` [PATCH v2 3/3] drm/i915: Refactor tests for validity of RING_TAIL Chris Wilson @ 2017-03-27 13:14 ` Chris Wilson 2017-03-27 13:57 ` Mika Kuoppala 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2017-03-27 13:14 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Refactor tests for validity of RING_TAIL 2017-03-27 13:14 ` [PATCH v3] " Chris Wilson @ 2017-03-27 13:57 ` Mika Kuoppala 0 siblings, 0 replies; 10+ messages in thread From: Mika Kuoppala @ 2017-03-27 13:57 UTC (permalink / raw) To: Chris Wilson, intel-gfx 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking 2017-03-27 13:00 [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking Chris Wilson @ 2017-03-27 13:05 ` Mika Kuoppala 2017-03-27 13:00 ` [PATCH v2 3/3] drm/i915: Refactor tests for validity of RING_TAIL Chris Wilson ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Mika Kuoppala @ 2017-03-27 13:05 UTC (permalink / raw) To: Chris Wilson, intel-gfx; +Cc: tvrtko.ursulin, Chris Wilson, # v4 . 10+ 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking @ 2017-03-27 13:05 ` Mika Kuoppala 0 siblings, 0 replies; 10+ messages in thread From: Mika Kuoppala @ 2017-03-27 13:05 UTC (permalink / raw) To: Chris Wilson, intel-gfx; +Cc: # v4 . 10+ 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2) 2017-03-27 13:00 [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking Chris Wilson ` (2 preceding siblings ...) 2017-03-27 13:05 ` Mika Kuoppala @ 2017-03-27 14:11 ` Patchwork 2017-03-27 14:20 ` Chris Wilson 3 siblings, 1 reply; 10+ messages in thread From: Patchwork @ 2017-03-27 14:11 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2) 2017-03-27 14:11 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2) Patchwork @ 2017-03-27 14:20 ` Chris Wilson 0 siblings, 0 replies; 10+ messages in thread From: Chris Wilson @ 2017-03-27 14:20 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-27 14:20 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-27 13:00 [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking Chris Wilson 2017-03-27 13:00 ` [PATCH v2 2/3] drm/i915: Assert that the request->tail fits within the ring Chris Wilson 2017-03-27 13:57 ` Mika Kuoppala 2017-03-27 13:00 ` [PATCH v2 3/3] drm/i915: Refactor tests for validity of RING_TAIL Chris Wilson 2017-03-27 13:14 ` [PATCH v3] " Chris Wilson 2017-03-27 13:57 ` Mika Kuoppala 2017-03-27 13:05 ` [PATCH v2 1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking Mika Kuoppala 2017-03-27 13:05 ` Mika Kuoppala 2017-03-27 14:11 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/execlists: Wrap tail pointer after reset tweaking (rev2) Patchwork 2017-03-27 14:20 ` Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.