[PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance}

[PATCH 0/2] Track 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().  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.

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

[PATCH 1/2] 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)

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |    4 ++-
 drivers/gpu/drm/i915/intel_lrc.h        |   11 ++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.c |   10 ++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |   55 ++++++++++++++++++++++++++++++-
 4 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a82020e..56e3636 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -825,6 +825,7 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf)
 	struct intel_context *ctx = ringbuf->FIXME_lrc_ctx;
 
 	intel_logical_ring_advance(ringbuf);
+	WARN_ON(ringbuf->rsv_level != 0);
 
 	if (intel_ring_stopped(ring))
 		return;
@@ -1084,7 +1085,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..9a0457e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -48,8 +48,17 @@ 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_check(ringbuf);
+
+	/*
+	 * Tail == effecive_size is legitimate (buffer exactly full).
+	 * Tail > effective_size is not, and should give a warning,
+	 * but we'll reset tail in both cases to prevent further chaos
+	 */
+	if (ringbuf->tail >= ringbuf->effective_size)
+		ringbuf->tail -= ringbuf->effective_size;
 }
+
 /**
  * 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 83accb7..5874eab 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;
 }
 
@@ -84,7 +85,10 @@ bool intel_ring_stopped(struct intel_engine_cs *ring)
 void __intel_ring_advance(struct intel_engine_cs *ring)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
-	ringbuf->tail &= ringbuf->size - 1;
+
+	intel_ring_advance(ring);
+	WARN_ON(ringbuf->rsv_level != 0);
+
 	if (intel_ring_stopped(ring))
 		return;
 	ring->write_tail(ring, ringbuf->tail);
@@ -1911,6 +1915,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 		return 0;
 
 	list_for_each_entry(request, &ring->request_list, list) {
+		/* Would completion of this request free enough space? */
 		if (__intel_ring_space(request->tail, ringbuf->tail,
 				       ringbuf->size) >= n) {
 			break;
@@ -2096,7 +2101,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..a10d271 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 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);
+#if	1	/* DEBUG CODE */
+	WARN_ON(num_dwords & 1);
+	WARN_ON(nbytes <= 0);
+
+	if (ringbuf->rsv_level++) {
+		/* begin() called twice or more without advance() */
+		WARN_ON(1);
+	} else {
+		/*
+		 * A new reservation; validate and record the start and
+		 * size, then deduct the size from the remaining space
+		 */
+		WARN_ON(ringbuf->tail & 7);
+		WARN_ON(ringbuf->tail > ringbuf->effective_size-8);
+		WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size);
+		WARN_ON(ringbuf->space < nbytes);
+	}
+#endif
+	ringbuf->rsv_start = ringbuf->tail;
+	ringbuf->rsv_size = nbytes;
+	ringbuf->space -= nbytes;
+}
+
+static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf)
+{
+#if	1
+	WARN_ON(ringbuf->rsv_level-- != 1);
+	WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0);
+	WARN_ON(ringbuf->tail & 7);
+	WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size);
+	WARN_ON(ringbuf->tail > ringbuf->effective_size);
+#endif
+}
 static inline void intel_ring_advance(struct intel_engine_cs *ring)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
-	ringbuf->tail &= ringbuf->size - 1;
+
+	__intel_ringbuffer_check(ringbuf);
+
+	/*
+	 * Tail == effecive_size is legitimate (buffer exactly full).
+	 * Tail > effective_size is not, and should give a warning,
+	 * but we'll reset tail in both cases to prevent further chaos
+	 */
+	if (ringbuf->tail >= ringbuf->effective_size)
+		ringbuf->tail -= ringbuf->effective_size;
 }
+
 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

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

[PATCH 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance}

[Dave Gordon]

With the current deferred-submission model, if a problem arises part-way
through the insertion of instructions into the ringbuffer (e.g. due to
one of the begin() calls finding there's not enough space), we avoid
sending the incomplete sequence to the h/w; but currently have no means
of undoing the work so far, which will lead to undefined behaviour when
the next batch is submitted (probably TDR will trigger a reset first,
though, and clean up the ring state).

A future idea is to move to an atomic-submission model, where all the
space required for a batch submission is reserved up front, and in the
event of failure partway through, the work can be abandoned without
side-effects. This will be required for the forthcoming GPU scheduler
(specifically, for preemption).

To support this, we allow nested begin/advance pairs.  Specifically,
the outermost pair defines the total space reservation; inner pairs
can be nested ad lib, but all inner reservations at any level must
fit entirely within the outermost one.  Thus, this is permitted:

	begin(128) - guarantees that up to 128 dwords can now be
			emitted without waiting for more freespace
		begin(6)
		advance
		begin(10)
		advance
		begin(8)
		advance
		etc, as long as the total is no more than 128 dwords
	advance-and-submit

The execbuffer code will later be enhanced to use this approach. In the
mean time, the traditional single-level begin/advance mechanism remains
fully supported.

This commit changes only the begin/advance checking code, to permit (but
not require) nested begin/advance pairs.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.h |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index a10d271..71cb3ef 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -416,8 +416,17 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf,
 	WARN_ON(nbytes <= 0);
 
 	if (ringbuf->rsv_level++) {
-		/* begin() called twice or more without advance() */
-		WARN_ON(1);
+		/*
+		 * A nested reservation; check that it falls entirely
+		 * within the outer block. Don't adjust remaining space.
+		 */
+		WARN_ON(ringbuf->rsv_start < 0);
+		WARN_ON(ringbuf->rsv_start & 7);
+		WARN_ON(ringbuf->tail & 7);
+		WARN_ON(ringbuf->tail > ringbuf->effective_size);
+		WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size);
+		WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size);
+		WARN_ON(ringbuf->tail + nbytes > ringbuf->rsv_start + ringbuf->rsv_size);
 	} else {
 		/*
 		 * A new reservation; validate and record the start and
@@ -437,7 +446,7 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf,
 static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf)
 {
 #if	1
-	WARN_ON(ringbuf->rsv_level-- != 1);
+	WARN_ON(ringbuf->rsv_level-- <= 0);
 	WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0);
 	WARN_ON(ringbuf->tail & 7);
 	WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size);
-- 
1.7.9.5

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

Re: [PATCH 1/2] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance}

[Daniel Vetter]

On Wed, Dec 10, 2014 at 03:07:08PM +0000, Dave Gordon wrote:
> @@ -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);
> +#if	1	/* DEBUG CODE */

No #ifdef's in code please, they tend to be either just go stale or just
obfuscate the code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 0/2] Track 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().  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.

v2: remove redundant "#if 1" markers

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

[PATCH v2 1/2] 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)

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |    4 ++-
 drivers/gpu/drm/i915/intel_lrc.h        |   11 ++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.c |   10 ++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |   54 ++++++++++++++++++++++++++++++-
 4 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a82020e..56e3636 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -825,6 +825,7 @@ void intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf)
 	struct intel_context *ctx = ringbuf->FIXME_lrc_ctx;
 
 	intel_logical_ring_advance(ringbuf);
+	WARN_ON(ringbuf->rsv_level != 0);
 
 	if (intel_ring_stopped(ring))
 		return;
@@ -1084,7 +1085,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..9a0457e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -48,8 +48,17 @@ 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_check(ringbuf);
+
+	/*
+	 * Tail == effecive_size is legitimate (buffer exactly full).
+	 * Tail > effective_size is not, and should give a warning,
+	 * but we'll reset tail in both cases to prevent further chaos
+	 */
+	if (ringbuf->tail >= ringbuf->effective_size)
+		ringbuf->tail -= ringbuf->effective_size;
 }
+
 /**
  * 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 83accb7..5874eab 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;
 }
 
@@ -84,7 +85,10 @@ bool intel_ring_stopped(struct intel_engine_cs *ring)
 void __intel_ring_advance(struct intel_engine_cs *ring)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
-	ringbuf->tail &= ringbuf->size - 1;
+
+	intel_ring_advance(ring);
+	WARN_ON(ringbuf->rsv_level != 0);
+
 	if (intel_ring_stopped(ring))
 		return;
 	ring->write_tail(ring, ringbuf->tail);
@@ -1911,6 +1915,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 		return 0;
 
 	list_for_each_entry(request, &ring->request_list, list) {
+		/* Would completion of this request free enough space? */
 		if (__intel_ring_space(request->tail, ringbuf->tail,
 				       ringbuf->size) >= n) {
 			break;
@@ -2096,7 +2101,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..a6660c1 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 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,58 @@ 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);
+
+	WARN_ON(num_dwords & 1);
+	WARN_ON(nbytes <= 0);
+
+	if (ringbuf->rsv_level++) {
+		/* begin() called twice or more without advance() */
+		WARN_ON(1);
+	} else {
+		/*
+		 * A new reservation; validate and record the start and
+		 * size, then deduct the size from the remaining space
+		 */
+		WARN_ON(ringbuf->tail & 7);
+		WARN_ON(ringbuf->tail > ringbuf->effective_size-8);
+		WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size);
+		WARN_ON(ringbuf->space < nbytes);
+	}
+
+	ringbuf->rsv_start = ringbuf->tail;
+	ringbuf->rsv_size = nbytes;
+	ringbuf->space -= nbytes;
+}
+
+static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf)
+{
+	WARN_ON(ringbuf->rsv_level-- != 1);
+	WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0);
+	WARN_ON(ringbuf->tail & 7);
+	WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size);
+	WARN_ON(ringbuf->tail > ringbuf->effective_size);
+}
+
 static inline void intel_ring_advance(struct intel_engine_cs *ring)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
-	ringbuf->tail &= ringbuf->size - 1;
+
+	__intel_ringbuffer_check(ringbuf);
+
+	/*
+	 * Tail == effecive_size is legitimate (buffer exactly full).
+	 * Tail > effective_size is not, and should give a warning,
+	 * but we'll reset tail in both cases to prevent further chaos
+	 */
+	if (ringbuf->tail >= ringbuf->effective_size)
+		ringbuf->tail -= ringbuf->effective_size;
 }
+
 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

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

[PATCH v2 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance}

[Dave Gordon]

With the current deferred-submission model, if a problem arises part-way
through the insertion of instructions into the ringbuffer (e.g. due to
one of the begin() calls finding there's not enough space), we avoid
sending the incomplete sequence to the h/w; but currently have no means
of undoing the work so far, which will lead to undefined behaviour when
the next batch is submitted (probably TDR will trigger a reset first,
though, and clean up the ring state).

A future idea is to move to an atomic-submission model, where all the
space required for a batch submission is reserved up front, and in the
event of failure partway through, the work can be abandoned without
side-effects. This will be required for the forthcoming GPU scheduler
(specifically, for preemption).

To support this, we allow nested begin/advance pairs.  Specifically,
the outermost pair defines the total space reservation; inner pairs
can be nested ad lib, but all inner reservations at any level must
fit entirely within the outermost one.  Thus, this is permitted:

	begin(128) - guarantees that up to 128 dwords can now be
			emitted without waiting for more freespace
		begin(6)
		advance
		begin(10)
		advance
		begin(8)
		advance
		etc, as long as the total is no more than 128 dwords
	advance-and-submit

The execbuffer code will later be enhanced to use this approach. In the
mean time, the traditional single-level begin/advance mechanism remains
fully supported.

This commit changes only the begin/advance checking code, to permit (but
not require) nested begin/advance pairs.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.h |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index a6660c1..68665c7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -416,8 +416,17 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf,
 	WARN_ON(nbytes <= 0);
 
 	if (ringbuf->rsv_level++) {
-		/* begin() called twice or more without advance() */
-		WARN_ON(1);
+		/*
+		 * A nested reservation; check that it falls entirely
+		 * within the outer block. Don't adjust remaining space.
+		 */
+		WARN_ON(ringbuf->rsv_start < 0);
+		WARN_ON(ringbuf->rsv_start & 7);
+		WARN_ON(ringbuf->tail & 7);
+		WARN_ON(ringbuf->tail > ringbuf->effective_size);
+		WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size);
+		WARN_ON(ringbuf->tail + nbytes > ringbuf->effective_size);
+		WARN_ON(ringbuf->tail + nbytes > ringbuf->rsv_start + ringbuf->rsv_size);
 	} else {
 		/*
 		 * A new reservation; validate and record the start and
@@ -436,7 +445,7 @@ static inline void __intel_ringbuffer_begin(struct intel_ringbuffer *ringbuf,
 
 static inline void __intel_ringbuffer_check(struct intel_ringbuffer *ringbuf)
 {
-	WARN_ON(ringbuf->rsv_level-- != 1);
+	WARN_ON(ringbuf->rsv_level-- <= 0);
 	WARN_ON(ringbuf->rsv_start < 0 || ringbuf->rsv_size < 0);
 	WARN_ON(ringbuf->tail & 7);
 	WARN_ON(ringbuf->tail > ringbuf->rsv_start + ringbuf->rsv_size);
-- 
1.7.9.5

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

Re: [PATCH 0/2] Track calls to intel(_logical)_ring_{begin, advance}

[Chris Wilson]

On Wed, Dec 10, 2014 at 03:07:07PM +0000, Dave Gordon wrote:
> 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().  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.

Please review the suggested approach I made months ago which avoid all
this ad hoc hilarity.
-Chris

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

Re: [PATCH 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance}

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              364/364              362/364
ILK                 -11              364/366              353/366
SNB                 -13              448/450              435/450
IVB                 -11              497/498              486/498
BYT                 -5              289/289              284/289
HSW                 -10              563/564              553/564
BDW                 -7              417/417              410/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_ringfill_blitter      PASS(3, M23M25)      DMESG_WARN(1, M25)
*PNV  igt_gem_ringfill_blitter-interruptible      PASS(3, M23M25)      DMESG_WARN(1, M25)
*ILK  igt_gem_concurrent_blit_gpu-bcs-gpu-read-after-write-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_concurrent_blit_gpu-bcs-overwrite-source-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_concurrent_blit_gpuX-bcs-gpu-read-after-write-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_concurrent_blit_gpuX-bcs-overwrite-source-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_concurrent_blit_prw-bcs-gpu-read-after-write-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_nonexisting-fb      DMESG_WARN(1, M26)PASS(4, M26)      NSPT(1, M26)
*ILK  igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_ringfill_blitter      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_gem_ringfill_blitter-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      PASS(5, M26)      DMESG_WARN(1, M26)
*SNB  igt_gem_concurrent_blit_cpu-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-forked      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_concurrent_blit_gpuX-rcs-gpu-read-after-write-forked      PASS(2, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_concurrent_blit_gpuX-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-forked      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_ring_sync_copy_sync-render-blitter-read-write      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_ring_sync_copy_sync-render-blitter-write-read      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_ring_sync_copy_sync-render-blitter-write-write      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_userptr_blits_coherency-sync      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_pm_rps_min-max-config-loaded      PASS(3, M35M22)      DMESG_WARN(1, M22)
*SNB  igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M22)
*IVB  igt_gem_linear_blits_interruptible      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_linear_blits_normal      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_render_linear_blits      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_render_tiled_blits      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_tiled_blits_interruptible      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_tiled_blits_normal      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_tiled_fence_blits      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_userptr_blits_coherency-sync      PASS(2, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_userptr_blits_coherency-unsync      PASS(2, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_ringfill_blitter      PASS(3, M4M34)      DMESG_WARN(1, M34)
*IVB  igt_gem_ringfill_blitter-interruptible      PASS(3, M4M34)      DMESG_WARN(1, M34)
*BYT  igt_gem_gtt_hog      PASS(3, M48M51)      DMESG_WARN(1, M51)
*BYT  igt_gem_ringfill_blitter      PASS(3, M48M51)      DMESG_WARN(1, M51)
*BYT  igt_gem_ringfill_blitter-interruptible      PASS(3, M48M51)      DMESG_WARN(1, M51)
*BYT  igt_gem_ringfill_render      PASS(3, M48M51)      DMESG_WARN(1, M51)
*BYT  igt_gem_ringfill_render-interruptible      PASS(3, M48M51)      DMESG_WARN(1, M51)
*HSW  igt_gem_linear_blits_interruptible      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_linear_blits_normal      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_render_linear_blits      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_render_tiled_blits      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_tiled_blits_interruptible      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_tiled_blits_normal      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_tiled_fence_blits      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_pm_rps_min-max-config-loaded      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_ringfill_blitter      PASS(3, M40M20)      DMESG_WARN(1, M40)
*HSW  igt_gem_ringfill_blitter-interruptible      PASS(3, M40M20)      DMESG_WARN(1, M40)
*BDW  igt_gem_render_linear_blits      TIMEOUT(1, M30)PASS(3, M30M28)      DMESG_WARN(1, M28)
*BDW  igt_gem_render_tiled_blits      TIMEOUT(1, M30)PASS(3, M30M28)      DMESG_WARN(1, M28)
*BDW  igt_gem_gtt_hog      PASS(3, M30M28)      DMESG_WARN(1, M28)
*BDW  igt_gem_ringfill_blitter      PASS(3, M30M28)      DMESG_WARN(1, M28)
*BDW  igt_gem_ringfill_blitter-interruptible      PASS(3, M30M28)      DMESG_WARN(1, M28)
*BDW  igt_gem_ringfill_render      PASS(3, M30M28)      DMESG_WARN(1, M28)
*BDW  igt_gem_ringfill_render-interruptible      PASS(3, M30M28)      DMESG_WARN(1, M28)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Track nested calls to intel(_logical)_ring_{begin, advance}

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              364/364              362/364
ILK              +1-9              364/366              356/366
SNB                 -12              448/450              436/450
IVB                 -9              497/498              488/498
BYT                 -5              289/289              284/289
HSW                 -11              563/564              552/564
BDW                 -5              417/417              412/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_ringfill_blitter      PASS(3, M23M25)      DMESG_WARN(1, M23)
*PNV  igt_gem_ringfill_blitter-interruptible      PASS(3, M23M25)      DMESG_WARN(1, M23)
*ILK  igt_gem_concurrent_blit_gpu-bcs-gpu-read-after-write-interruptible      PASS(3, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_concurrent_blit_gpu-bcs-overwrite-source      PASS(2, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_concurrent_blit_gpu-bcs-overwrite-source-interruptible      PASS(3, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_concurrent_blit_gpuX-bcs-gpu-read-after-write-interruptible      PASS(3, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_concurrent_blit_gpuX-bcs-overwrite-source-interruptible      PASS(3, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible      PASS(3, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible      PASS(3, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_ringfill_blitter      PASS(3, M26M37)      DMESG_WARN(1, M37)
*ILK  igt_gem_ringfill_blitter-interruptible      PASS(3, M26M37)      DMESG_WARN(1, M37)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(3, M26)PASS(19, M26M37)      PASS(1, M37)
*SNB  igt_gem_concurrent_blit_cpu-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-forked      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_concurrent_blit_gpu-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_concurrent_blit_gpuX-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-forked      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_concurrent_blit_prw-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_ring_sync_copy_sync-render-blitter-read-write      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_ring_sync_copy_sync-render-blitter-write-read      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_ring_sync_copy_sync-render-blitter-write-write      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_userptr_blits_coherency-sync      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_pm_rps_min-max-config-loaded      PASS(3, M35M22)      DMESG_WARN(1, M35)
*SNB  igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-interruptible      PASS(3, M35M22)      DMESG_WARN(1, M35)
*IVB  igt_gem_linear_blits_interruptible      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_linear_blits_normal      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_render_linear_blits      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_render_tiled_blits      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_tiled_blits_interruptible      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_tiled_blits_normal      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_tiled_fence_blits      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_ringfill_blitter      PASS(3, M4M34)      DMESG_WARN(1, M4)
*IVB  igt_gem_ringfill_blitter-interruptible      PASS(3, M4M34)      DMESG_WARN(1, M4)
*BYT  igt_gem_gtt_hog      PASS(3, M48M51)      DMESG_WARN(1, M48)
*BYT  igt_gem_ringfill_blitter      PASS(3, M48M51)      DMESG_WARN(1, M48)
*BYT  igt_gem_ringfill_blitter-interruptible      PASS(3, M48M51)      DMESG_WARN(1, M48)
*BYT  igt_gem_ringfill_render      PASS(3, M48M51)      DMESG_WARN(1, M48)
*BYT  igt_gem_ringfill_render-interruptible      PASS(3, M48M51)      DMESG_WARN(1, M48)
*HSW  igt_gem_linear_blits_interruptible      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_linear_blits_normal      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_render_linear_blits      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_render_tiled_blits      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_tiled_blits_interruptible      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_tiled_blits_normal      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_tiled_fence_blits      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_userptr_blits_coherency-unsync      PASS(2, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_pm_rps_min-max-config-loaded      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_ringfill_blitter      PASS(3, M40M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_ringfill_blitter-interruptible      PASS(3, M40M20)      DMESG_WARN(1, M20)
*BDW  igt_gem_gtt_hog      PASS(3, M30M28)      DMESG_WARN(1, M30)
*BDW  igt_gem_ringfill_blitter      PASS(3, M30M28)      DMESG_WARN(1, M30)
*BDW  igt_gem_ringfill_blitter-interruptible      PASS(3, M30M28)      DMESG_WARN(1, M30)
*BDW  igt_gem_ringfill_render      PASS(3, M30M28)      DMESG_WARN(1, M30)
*BDW  igt_gem_ringfill_render-interruptible      PASS(3, M30M28)      DMESG_WARN(1, M30)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx