[PATCH 0/3] Three small patches to ringbuffer calculations

[PATCH 0/3] Three small patches to ringbuffer calculations

[Dave Gordon]

[PATCH 1/3] drm/i915: use effective_size for ringbuffer calculations

    When calculating the available space in a ringbuffer, we should
    use the effective_size rather than the true size of the ring.

[PATCH 2/3] drm/i915: recheck ringspace after wait+retire

    When a ringbuffer is nearly full, we often wait for some request
    to complete and so free up some ringspace. This code checks that
    we actually got at least as much space as we expected and need.

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

    Check that these functions are used in a logically consistent manner.

I've dropped the patch to allow nested begin/advance pairs; instead
we will create a pre-allocate/reserve call as part of the scheduler
or preemption work.


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

[PATCH 1/3] drm/i915: use effective_size for ringbuffer calculations

[Dave Gordon]

When calculating the available space in a ringbuffer, we should
use the effective_size rather than the true size of the ring.

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

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index a82020e..8545dbd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -942,7 +942,7 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 
 		/* Would completion of this request free enough space? */
 		if (__intel_ring_space(request->tail, ringbuf->tail,
-				       ringbuf->size) >= bytes) {
+				       ringbuf->effective_size) >= bytes) {
 			break;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 83accb7..9def83c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -66,7 +66,8 @@ void intel_ring_update_space(struct intel_ringbuffer *ringbuf)
 	}
 
 	ringbuf->space = __intel_ring_space(ringbuf->head & HEAD_ADDR,
-					    ringbuf->tail, ringbuf->size);
+					    ringbuf->tail,
+					    ringbuf->effective_size);
 }
 
 int intel_ring_space(struct intel_ringbuffer *ringbuf)
@@ -1912,7 +1913,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 
 	list_for_each_entry(request, &ring->request_list, list) {
 		if (__intel_ring_space(request->tail, ringbuf->tail,
-				       ringbuf->size) >= n) {
+				       ringbuf->effective_size) >= n) {
 			break;
 		}
 	}
-- 
1.7.9.5

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

[PATCH 2/3] drm/i915: recheck ringspace after wait+retire

[Dave Gordon]

In {intel,logical}_ring_wait_request(), we try to find a request
whose completion will release the amount of ring space required.
If we find such a request, we wait for it, and then retire it, in
the expectation that we will now have at least the required amount
of free space in the ring. But it's good to check that this has
actually happened, so we can back out with a meaningful error
code if something unexpected has happened, such as wait_request
returning early.

This code was already in the execlist version, so the change to
intel_lrc.c is just to add a comment; but we want the same check
in the legacy ringbuffer mode too.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |    9 +++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |   12 +++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8545dbd..69b042f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -956,6 +956,15 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 
 	i915_gem_retire_requests_ring(ring);
 
+	/*
+	 * According to our calculation above, retiring the request we just
+	 * waited for should have resulted in there being enough space in
+	 * the ringbuffer; but let's check.
+	 *
+	 * If there is not now enough space, something has gone horribly worng
+	 * (such as wait_request returning early, but with no error, or
+	 * retire_requests failing to retire the request we expected it to).
+	 */
 	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 9def83c..660d10d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1912,6 +1912,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->effective_size) >= n) {
 			break;
@@ -1927,7 +1928,16 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 
 	i915_gem_retire_requests_ring(ring);
 
-	return 0;
+	/*
+	 * According to our calculation above, retiring the request we just
+	 * waited for should have resulted in there being enough space in
+	 * the ringbuffer; but let's check.
+	 *
+	 * If there is not now enough space, something has gone horribly worng
+	 * (such as wait_request returning early, but with no error, or
+	 * retire_requests failing to retire the request we expected it to).
+	 */
+	return intel_ring_space(ringbuf) >= n ? 0 : -ENOSPC;
 }
 
 static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
-- 
1.7.9.5

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

[PATCH 3/3] 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 |    9 ++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h |   54 ++++++++++++++++++++++++++++++-
 4 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 69b042f..422f3b9 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;
@@ -1093,7 +1094,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 660d10d..7ffad3a 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;
 }
 
@@ -85,7 +86,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);
@@ -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..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

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

[Chris Wilson]

On Fri, Dec 12, 2014 at 04:13:03PM +0000, Dave Gordon wrote:
>  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;

Urm. No. If you never write into the reserved pair of cachelines at the
end of the ringbuffer but the hw reads garbage from it, you lose.
tail &= size - 1; is a nice description of how the hw works that is
suitable for inlining, with all the magic in begin().

The goal is to remove the duplicated logic from intel_lrc.c, use
requests completely, and remove the dri1 hangover. To repeat what I said
last time:
http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c?h=requests#n2447
is where I want us to go, a single piece of logic for ring management.
-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 3/3] drm/i915: Track & check calls to intel(_logical)_ring_{begin, advance}

[Dave Gordon]

On 12/12/14 17:12, Chris Wilson wrote:
> On Fri, Dec 12, 2014 at 04:13:03PM +0000, Dave Gordon wrote:
>>  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;
> 
> Urm. No. If you never write into the reserved pair of cachelines at the
> end of the ringbuffer but the hw reads garbage from it, you lose.

This won't happen, because (with the first patch of this set applied)
the check for sufficient space uses effective_size, but the wrap code
uses the actual size. Thus, once the next chunk won't fit in the
available space up to the effective_size, we write NOPs all over
whatever is left up to the total size. The part between the effective
and the actual sizes is therefore always (and only) written with NOPs.

> tail &= size - 1; is a nice description of how the hw works that is
> suitable for inlining, with all the magic in begin().
> 
> The goal is to remove the duplicated logic from intel_lrc.c, use
> requests completely, and remove the dri1 hangover. To repeat what I said
> last time:
> http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c?h=requests#n2447
> is where I want us to go, a single piece of logic for ring management.
> -Chris

I agree with the deduplication, which is why nearly all these changes
are in functions operating on ringbuffers (not rings), and located in
intel_ringbuffer.h, with only minimal changes in the execlist and legacy
paths to use these common functions rather than having two implementations.

So, what I think I'll do is rework this third patch to merge the
tail-masking into the common function; rename it to something other than
/check/; and then the legacy and LRC versions become trivial wrappers
round this.

Are you happy with the first two patches?

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

Re: [PATCH 3/3] drm/i915: Track & check 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                                  364/364              364/364
ILK              +3                 362/366              365/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_nonexisting-fb      DMESG_WARN(1, M26)PASS(3, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_rcs-flip-vs-panning-interruptible      DMESG_WARN(1, M26)PASS(3, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_rcs-wf_vblank-vs-dpms-interruptible      DMESG_WARN(1, M26)PASS(2, M26M37)      PASS(1, M37)
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

[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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx