diff mbox

[v3,2/2] drm/i915: Consolidate ring freespace calculations

Message ID 1417087369-22767-3-git-send-email-david.s.gordon@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dave Gordon Nov. 27, 2014, 11:22 a.m. UTC 
There are numerous places in the code where the driver's idea of
how much space is left in a ring is updated using the driver's
latest notions of the positions of 'head' and 'tail' for the ring.
Among them are some that update one or both of these values before
(re)doing the calculation. In particular, there are four different
places in the code where 'last_retired_head' is copied to 'head'
and then set to -1; and two of these do not have a guard to check
that it has actually been updated since last time it was consumed,
leaving the possibility that the dummy -1 can be transferred from
'last_retired_head' to 'head', causing the space calculation to
produce 'impossible' results (previously seen on Android/VLV).

This code therefore consolidates all the calculation and updating of
these values, such that there is only one place where the ring space
is updated, and it ALWAYS uses (and consumes) 'last_retired_head' if
(and ONLY if) it has been updated since the last call.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |   25 ++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   42 ++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
 3 files changed, 30 insertions(+), 38 deletions(-)

Comments

Shuang He Nov. 27, 2014, 7:20 p.m. UTC | #1 
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              +1-9              365/366              357/366
SNB                                  450/450              450/450
IVB                                  498/498              498/498
BYT                                  289/289              289/289
HSW                                  564/564              564/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_nonexisting-fb      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_pipe_crc_basic_bad-nb-words-3      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_render_direct-render      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_busy-flip-interruptible      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_flip-vs-dpms-interruptible      PASS(2, M26)      NSPT(1, M26)
*ILK  igt_kms_flip_flip-vs-panning      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset      NSPT(1, M26)PASS(1, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(1, M26)PASS(1, M26)      PASS(1, M26)
 ILK  igt_kms_flip_wf_vblank-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(1, M26)      DMESG_WARN(1, M26)
Note: You need to pay more attention to line start with '*'
Daniel Vetter Nov. 28, 2014, 5:51 p.m. UTC | #2 
On Thu, Nov 27, 2014 at 11:22:49AM +0000, Dave Gordon wrote:
> There are numerous places in the code where the driver's idea of
> how much space is left in a ring is updated using the driver's
> latest notions of the positions of 'head' and 'tail' for the ring.
> Among them are some that update one or both of these values before
> (re)doing the calculation. In particular, there are four different
> places in the code where 'last_retired_head' is copied to 'head'
> and then set to -1; and two of these do not have a guard to check
> that it has actually been updated since last time it was consumed,
> leaving the possibility that the dummy -1 can be transferred from
> 'last_retired_head' to 'head', causing the space calculation to
> produce 'impossible' results (previously seen on Android/VLV).
> 
> This code therefore consolidates all the calculation and updating of
> these values, such that there is only one place where the ring space
> is updated, and it ALWAYS uses (and consumes) 'last_retired_head' if
> (and ONLY if) it has been updated since the last call.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Both applied, thanks.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 03b5c04..3d67906 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -920,14 +920,8 @@  static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 	u32 seqno = 0;
 	int ret;
 
-	if (ringbuf->last_retired_head != -1) {
-		ringbuf->head = ringbuf->last_retired_head;
-		ringbuf->last_retired_head = -1;
-
-		ringbuf->space = intel_ring_space(ringbuf);
-		if (ringbuf->space >= bytes)
-			return 0;
-	}
+	if (intel_ring_space(ringbuf) >= bytes)
+		return 0;
 
 	list_for_each_entry(request, &ring->request_list, list) {
 		/*
@@ -955,11 +949,8 @@  static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 		return ret;
 
 	i915_gem_retire_requests_ring(ring);
-	ringbuf->head = ringbuf->last_retired_head;
-	ringbuf->last_retired_head = -1;
 
-	ringbuf->space = intel_ring_space(ringbuf);
-	return 0;
+	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
 }
 
 static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
@@ -985,12 +976,10 @@  static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
 	 * case by choosing an insanely large timeout. */
 	end = jiffies + 60 * HZ;
 
+	ret = 0;
 	do {
-		ringbuf->space = intel_ring_space(ringbuf);
-		if (ringbuf->space >= bytes) {
-			ret = 0;
+		if (intel_ring_space(ringbuf) >= bytes)
 			break;
-		}
 
 		msleep(1);
 
@@ -1031,7 +1020,7 @@  static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf)
 		iowrite32(MI_NOOP, virt++);
 
 	ringbuf->tail = 0;
-	ringbuf->space = intel_ring_space(ringbuf);
+	intel_ring_update_space(ringbuf);
 
 	return 0;
 }
@@ -1881,8 +1870,8 @@  int intel_lr_context_deferred_create(struct intel_context *ctx,
 	ringbuf->effective_size = ringbuf->size;
 	ringbuf->head = 0;
 	ringbuf->tail = 0;
-	ringbuf->space = ringbuf->size;
 	ringbuf->last_retired_head = -1;
+	intel_ring_update_space(ringbuf);
 
 	if (ringbuf->obj == NULL) {
 		ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a5cfaae..19f64da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -58,10 +58,21 @@  int __intel_ring_space(int head, int tail, int size)
 	return space - I915_RING_FREE_SPACE;
 }
 
+void intel_ring_update_space(struct intel_ringbuffer *ringbuf)
+{
+	if (ringbuf->last_retired_head != -1) {
+		ringbuf->head = ringbuf->last_retired_head;
+		ringbuf->last_retired_head = -1;
+	}
+
+	ringbuf->space = __intel_ring_space(ringbuf->head & HEAD_ADDR,
+					    ringbuf->tail, ringbuf->size);
+}
+
 int intel_ring_space(struct intel_ringbuffer *ringbuf)
 {
-	return __intel_ring_space(ringbuf->head & HEAD_ADDR,
-				  ringbuf->tail, ringbuf->size);
+	intel_ring_update_space(ringbuf);
+	return ringbuf->space;
 }
 
 bool intel_ring_stopped(struct intel_engine_cs *ring)
@@ -589,10 +600,10 @@  static int init_ring_common(struct intel_engine_cs *ring)
 		goto out;
 	}
 
+	ringbuf->last_retired_head = -1;
 	ringbuf->head = I915_READ_HEAD(ring);
 	ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
-	ringbuf->space = intel_ring_space(ringbuf);
-	ringbuf->last_retired_head = -1;
+	intel_ring_update_space(ringbuf);
 
 	memset(&ring->hangcheck, 0, sizeof(ring->hangcheck));
 
@@ -1891,14 +1902,8 @@  static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 	u32 seqno = 0;
 	int ret;
 
-	if (ringbuf->last_retired_head != -1) {
-		ringbuf->head = ringbuf->last_retired_head;
-		ringbuf->last_retired_head = -1;
-
-		ringbuf->space = intel_ring_space(ringbuf);
-		if (ringbuf->space >= n)
-			return 0;
-	}
+	if (intel_ring_space(ringbuf) >= n)
+		return 0;
 
 	list_for_each_entry(request, &ring->request_list, list) {
 		if (__intel_ring_space(request->tail, ringbuf->tail,
@@ -1916,10 +1921,7 @@  static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 		return ret;
 
 	i915_gem_retire_requests_ring(ring);
-	ringbuf->head = ringbuf->last_retired_head;
-	ringbuf->last_retired_head = -1;
 
-	ringbuf->space = intel_ring_space(ringbuf);
 	return 0;
 }
 
@@ -1945,14 +1947,14 @@  static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
 	 * case by choosing an insanely large timeout. */
 	end = jiffies + 60 * HZ;
 
+	ret = 0;
 	trace_i915_ring_wait_begin(ring);
 	do {
+		if (intel_ring_space(ringbuf) >= n)
+			break;
 		ringbuf->head = I915_READ_HEAD(ring);
-		ringbuf->space = intel_ring_space(ringbuf);
-		if (ringbuf->space >= n) {
-			ret = 0;
+		if (intel_ring_space(ringbuf) >= n)
 			break;
-		}
 
 		msleep(1);
 
@@ -1993,7 +1995,7 @@  static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
 		iowrite32(MI_NOOP, virt++);
 
 	ringbuf->tail = 0;
-	ringbuf->space = intel_ring_space(ringbuf);
+	intel_ring_update_space(ringbuf);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index fe426cf..1897f21 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -408,6 +408,7 @@  static inline void intel_ring_advance(struct intel_engine_cs *ring)
 	ringbuf->tail &= ringbuf->size - 1;
 }
 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);
 bool intel_ring_stopped(struct intel_engine_cs *ring);
 void __intel_ring_advance(struct intel_engine_cs *ring);