[Intel-gfx] [PATCH] drm/i915: Refactor tests for validity of RING_TAIL

[Chris Wilson]

On Mon, Mar 27, 2017 at 11:17:20AM +0100, Tvrtko Ursulin wrote:
> 
> On 27/03/2017 10:50, Chris Wilson wrote:
> >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 at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/intel_lrc.c        | 12 ++++--------
> > drivers/gpu/drm/i915/intel_ringbuffer.c |  9 +++------
> > drivers/gpu/drm/i915/intel_ringbuffer.h |  7 +++++++
> > 3 files changed, 14 insertions(+), 14 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 3b84fbb7da5d..ccbe1a16f3c3 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
> >@@ -1304,8 +1303,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
> > 	/* Reset WaIdleLiteRestore:bdw,skl as well */
> > 	request->tail = request->wa_tail - WA_TAIL_DWORDS * sizeof(u32);
> > 	request->tail &= request->ring->size - 1;
> >-	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)
> >@@ -1516,8 +1514,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);
> > }
> >@@ -1545,8 +1542,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 2ecb41788fb6..6bc89719250a 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >@@ -523,6 +523,13 @@ intel_ring_offset(struct drm_i915_gem_request *req, void *addr)
> > 	return offset & (req->ring->size - 1);
> > }
> >
> >+static inline void
> >+assert_ring_tail_valid(const struct intel_ring *ring, unsigned int tail)
> >+{
> >+	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);
> >
> 
> Looking at the call sites, how about assert_request_tail_valid(req)?
> And squash with the one where you are adding the second check.

I considered it, but I thought using request distracted from the
intention of checking against the current hardware limits (hence
intel_ringbuffer.h for lack of a better home rather than
i915_gem_request.h)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre