[Intel-gfx] [PATCH 3/3, v2] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance}

[Dave Gordon]

When adding instructions to a legacy or LRC ringbuffer, the sequence of
emit() calls must be preceded by a call to intel(_logical)_ring_begin()
to reserve the required amount of space, and followed by a matching call
to intel(_logical)_ring_advance() (note that this used to trigger
immediate submission to the h/w, but now actual submission is deferred
until all the instructions for a single batch submission have been
assembled). Historically some (display) code didn't use begin/advance,
but just inserted instructions ad hoc, which would then be sent to the
hardware along with the current or next batch, but this is not supported
and is now regarded as incorrect.

This commit therefore adds begin/advance tracking, with WARNings where
various forms of misuse are detected. These include:
* advance without begin
* begin without advance before submission to h/w
* multiple begins without an advance between
* exceeding the space reserved by begin
* leaving the ring misaligned
* ring buffer overrun (negative freespace)

v2: more deduplication (Chris)

Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |    3 +-
 drivers/gpu/drm/i915/intel_lrc.h        |    3 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    9 +++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |   57 +++++++++++++++++++++++++++++--
 4 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 69b042f..4222174 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1093,7 +1093,8 @@ int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, int num_dwords)
 	if (ret)
 		return ret;
 
-	ringbuf->space -= num_dwords * sizeof(uint32_t);
+	__intel_ringbuffer_begin(ringbuf, num_dwords);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 14b216b..4b29ed6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -48,8 +48,9 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf);
  */
 static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf)
 {
-	ringbuf->tail &= ringbuf->size - 1;
+	__intel_ringbuffer_advance(ringbuf);
 }
+
 /**
  * intel_logical_ring_emit() - write a DWORD to the ringbuffer.
  * @ringbuf: Ringbuffer to write to.
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 660d10d..0de9065 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -55,6 +55,7 @@ int __intel_ring_space(int head, int tail, int size)
 	int space = head - tail;
 	if (space <= 0)
 		space += size;
+	WARN_ON(space < I915_RING_FREE_SPACE);
 	return space - I915_RING_FREE_SPACE;
 }
 
@@ -82,10 +83,13 @@ bool intel_ring_stopped(struct intel_engine_cs *ring)
 	return dev_priv->gpu_error.stop_rings & intel_ring_flag(ring);
 }
 
+/* advance(), and then also submit to hardware */
 void __intel_ring_advance(struct intel_engine_cs *ring)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
-	ringbuf->tail &= ringbuf->size - 1;
+
+	intel_ring_advance(ring);
+
 	if (intel_ring_stopped(ring))
 		return;
 	ring->write_tail(ring, ringbuf->tail);
@@ -2107,7 +2111,8 @@ int intel_ring_begin(struct intel_engine_cs *ring,
 	if (ret)
 		return ret;
 
-	ring->buffer->space -= num_dwords * sizeof(uint32_t);
+	__intel_ringbuffer_begin(ring->buffer, num_dwords);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 6dbb6f4..b002056 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -112,6 +112,11 @@ struct intel_ringbuffer {
 	int size;
 	int effective_size;
 
+	/* these let begin/advance() check for misuse */
+	int rsv_level;	/* reservation nesting level */
+	int rsv_size;	/* size passed to begin() */
+	int rsv_start;	/* tail when begin() last returned */
+
 	/** We track the position of the requests in the ring buffer, and
 	 * when each is retired we increment last_retired_head as the GPU
 	 * must have finished processing the request and so we know we
@@ -401,11 +406,59 @@ static inline void intel_ring_emit(struct intel_engine_cs *ring,
 	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
 	ringbuf->tail += 4;
 }
+
+static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf,
+					    int num_dwords)
+{
+	int nbytes = num_dwords * sizeof(uint32_t);
+
+	/* First check the incoming parameter */
+	WARN_ON(num_dwords & 1);
+	WARN_ON(nbytes <= 0);
+
+	/* Check that 'tail' is sane */
+	WARN_ON(ringbuf->tail & 7);
+	WARN_ON(ringbuf->tail > ringbuf->effective_size-8);
+	WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size);
+
+	/* Check for begin() called twice or more without advance() */
+	WARN_ON(ringbuf->rsv_level++);
+
+	/* Record the start and size, then decrement the remaining space */
+	ringbuf->rsv_start = ringbuf->tail;
+	ringbuf->rsv_size = nbytes;
+	ringbuf->space -= nbytes;
+
+	WARN_ON(ringbuf->space < 0);
+}
+
+static inline void __intel_ringbuffer_advance(struct intel_ringbuffer *ringbuf)
+{
+	/* Check that 'space' and 'tail' are still sane */
+	WARN_ON(ringbuf->space < 0);
+	WARN_ON(ringbuf->tail & 7);
+	WARN_ON(ringbuf->tail > ringbuf->effective_size);
+
+	/* Check for advance() called more times than begin() */
+	WARN_ON(--ringbuf->rsv_level);
+
+	if (!WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0)) {
+		WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size);
+
+		/* Negate start and size to show that they've been used up */
+		ringbuf->rsv_start = -ringbuf->rsv_size;
+		ringbuf->rsv_size = -ringbuf->rsv_size;
+	}
+
+	/* Wrap the tail offset back to the start (size is a power of 2) */
+	ringbuf->tail &= ringbuf->size-1;
+}
+
 static inline void intel_ring_advance(struct intel_engine_cs *ring)
 {
-	struct intel_ringbuffer *ringbuf = ring->buffer;
-	ringbuf->tail &= ringbuf->size - 1;
+	__intel_ringbuffer_advance(ring->buffer);
 }
+
 int __intel_ring_space(int head, int tail, int size);
 void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
 int intel_ring_space(struct intel_ringbuffer *ringbuf);
-- 
1.7.9.5