diff mbox

drm/i915: Move CSB MMIO reads out of the execlists lock

Message ID 1456838737-6669-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin March 1, 2016, 1:25 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

By reading the CSB (slow MMIO accesses) into a temporary local
buffer we can decrease the duration of holding the execlist
lock.

Main advantage is that during heavy batch buffer submission we
reduce the execlist lock contention, which should decrease the
latency and CPU usage between the submitting userspace process
and interrupt handling.

Downside is that we need to grab and relase the forcewake twice,
but as the below numbers will show this is completely hidden
by the primary gains.

Testing with "gem_latency -n 100" (submit batch buffers with a
hundred nops each) shows more than doubling of the throughput
and more than halving of the dispatch latency, overall latency
and CPU time spend in the submitting process.

Submitting empty batches ("gem_latency -n 0") does not seem
significantly affected by this change with throughput and CPU
time improving by half a percent, and overall latency worsening
by the same amount.

Above tests were done in a hundred runs on a big core Broadwell.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 73 ++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 37 deletions(-)

Comments

Tvrtko Ursulin March 3, 2016, 5:47 p.m. UTC | #1 
On 03/03/16 11:02, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915: Move CSB MMIO reads out of the execlists lock (rev2)
> URL   : https://patchwork.freedesktop.org/series/3973/
> State : warning
>
> == Summary ==
>
> Series 3973v2 drm/i915: Move CSB MMIO reads out of the execlists lock
> http://patchwork.freedesktop.org/api/1.0/series/3973/revisions/2/mbox/
>
> Test drv_module_reload_basic:
>                  incomplete -> PASS       (snb-dellxps)
> Test gem_storedw_loop:
>          Subgroup basic-blt:
>                  pass       -> DMESG-WARN (hsw-brixbox)

Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=94307

> Test kms_flip:
>          Subgroup basic-flip-vs-wf_vblank:
>                  pass       -> DMESG-WARN (hsw-gt2)

Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=94384

> Test pm_rpm:
>          Subgroup basic-pci-d3-state:
>                  pass       -> DMESG-WARN (bsw-nuc-2)

Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=94164

>
> bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11
> bdw-ultra        total:169  pass:155  dwarn:0   dfail:0   fail:0   skip:14
> bsw-nuc-2        total:169  pass:136  dwarn:2   dfail:0   fail:1   skip:30
> byt-nuc          total:169  pass:144  dwarn:0   dfail:0   fail:0   skip:25
> hsw-brixbox      total:169  pass:153  dwarn:1   dfail:0   fail:0   skip:15
> hsw-gt2          total:169  pass:158  dwarn:1   dfail:0   fail:0   skip:10
> ilk-hp8440p      total:169  pass:119  dwarn:0   dfail:0   fail:0   skip:50
> ivb-t430s        total:169  pass:154  dwarn:0   dfail:0   fail:0   skip:15
> skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16
> skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16
> snb-dellxps      total:169  pass:145  dwarn:1   dfail:0   fail:0   skip:23
> snb-x220t        total:169  pass:145  dwarn:0   dfail:1   fail:1   skip:22
>
> Results at /archive/results/CI_IGT_test/Patchwork_1521/
>
> 99ba2daf15b76f01916dc645b8d1b03dbbabf13c drm-intel-nightly: 2016y-03m-03d-08h-27m-26s UTC integration manifest
> 999801d1943ae1642f4908e7ded8ee20a5699bcf drm/i915: Move CSB MMIO reads out of the execlists lock

So just the usual unrelated random fails.

Otherwise patch seems stable when I bash it with GLmark, SynMark2, OCL 
and some random Linux games.

Regards,

Tvrtko
Chris Wilson March 3, 2016, 8:50 p.m. UTC | #2 
On Thu, Mar 03, 2016 at 05:47:14PM +0000, Tvrtko Ursulin wrote:
> 
> On 03/03/16 11:02, Patchwork wrote:
> >== Series Details ==
> >
> >Series: drm/i915: Move CSB MMIO reads out of the execlists lock (rev2)
> >URL   : https://patchwork.freedesktop.org/series/3973/
> >State : warning
> >
> >== Summary ==
> >
> >Series 3973v2 drm/i915: Move CSB MMIO reads out of the execlists lock
> >http://patchwork.freedesktop.org/api/1.0/series/3973/revisions/2/mbox/
> >
> >Test drv_module_reload_basic:
> >                 incomplete -> PASS       (snb-dellxps)
> >Test gem_storedw_loop:
> >         Subgroup basic-blt:
> >                 pass       -> DMESG-WARN (hsw-brixbox)
> 
> Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=94307
> 
> >Test kms_flip:
> >         Subgroup basic-flip-vs-wf_vblank:
> >                 pass       -> DMESG-WARN (hsw-gt2)
> 
> Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=94384
> 
> >Test pm_rpm:
> >         Subgroup basic-pci-d3-state:
> >                 pass       -> DMESG-WARN (bsw-nuc-2)
> 
> Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=94164
> 
> >
> >bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11
> >bdw-ultra        total:169  pass:155  dwarn:0   dfail:0   fail:0   skip:14
> >bsw-nuc-2        total:169  pass:136  dwarn:2   dfail:0   fail:1   skip:30
> >byt-nuc          total:169  pass:144  dwarn:0   dfail:0   fail:0   skip:25
> >hsw-brixbox      total:169  pass:153  dwarn:1   dfail:0   fail:0   skip:15
> >hsw-gt2          total:169  pass:158  dwarn:1   dfail:0   fail:0   skip:10
> >ilk-hp8440p      total:169  pass:119  dwarn:0   dfail:0   fail:0   skip:50
> >ivb-t430s        total:169  pass:154  dwarn:0   dfail:0   fail:0   skip:15
> >skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16
> >skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16
> >snb-dellxps      total:169  pass:145  dwarn:1   dfail:0   fail:0   skip:23
> >snb-x220t        total:169  pass:145  dwarn:0   dfail:1   fail:1   skip:22
> >
> >Results at /archive/results/CI_IGT_test/Patchwork_1521/
> >
> >99ba2daf15b76f01916dc645b8d1b03dbbabf13c drm-intel-nightly: 2016y-03m-03d-08h-27m-26s UTC integration manifest
> >999801d1943ae1642f4908e7ded8ee20a5699bcf drm/i915: Move CSB MMIO reads out of the execlists lock
> 
> So just the usual unrelated random fails.
> 
> Otherwise patch seems stable when I bash it with GLmark, SynMark2,
> OCL and some random Linux games.

Yes, patch is sane, I'm just messing around with my Braswell at the
moment and then I'll try again at getting some numbers. First glance
said 10% in reducing latency (with a 100% throughput improvement in one
particular small copy scenario, that I want to reprdouce and do some back
of the envelope calculations to check that it is sane), but the machine
(thanks to execlists) just dies as soon as I try some more interesting
benchmarks.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 27c9ee3f7372..9000d6ac8d65 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -416,15 +416,23 @@  static void execlists_update_context(struct drm_i915_gem_request *rq)
 static void execlists_submit_requests(struct drm_i915_gem_request *rq0,
 				      struct drm_i915_gem_request *rq1)
 {
+	struct drm_i915_private *dev_priv = rq0->ring->dev->dev_private;
+
 	execlists_update_context(rq0);
 
 	if (rq1)
 		execlists_update_context(rq1);
 
+	spin_lock(&dev_priv->uncore.lock);
+	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
+
 	execlists_elsp_write(rq0, rq1);
+
+	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
+	spin_unlock(&dev_priv->uncore.lock);
 }
 
-static void execlists_context_unqueue__locked(struct intel_engine_cs *ring)
+static void execlists_context_unqueue(struct intel_engine_cs *ring)
 {
 	struct drm_i915_gem_request *req0 = NULL, *req1 = NULL;
 	struct drm_i915_gem_request *cursor, *tmp;
@@ -478,19 +486,6 @@  static void execlists_context_unqueue__locked(struct intel_engine_cs *ring)
 	execlists_submit_requests(req0, req1);
 }
 
-static void execlists_context_unqueue(struct intel_engine_cs *ring)
-{
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-
-	spin_lock(&dev_priv->uncore.lock);
-	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
-
-	execlists_context_unqueue__locked(ring);
-
-	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
-	spin_unlock(&dev_priv->uncore.lock);
-}
-
 static unsigned int
 execlists_check_remove_request(struct intel_engine_cs *ring, u32 request_id)
 {
@@ -551,12 +546,10 @@  void intel_lrc_irq_handler(struct intel_engine_cs *ring)
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	u32 status_pointer;
 	unsigned int read_pointer, write_pointer;
-	u32 status = 0;
-	u32 status_id;
+	u32 csb[GEN8_CSB_ENTRIES][2];
+	unsigned int csb_read = 0, i;
 	unsigned int submit_contexts = 0;
 
-	spin_lock(&ring->execlist_lock);
-
 	spin_lock(&dev_priv->uncore.lock);
 	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
 
@@ -568,39 +561,45 @@  void intel_lrc_irq_handler(struct intel_engine_cs *ring)
 		write_pointer += GEN8_CSB_ENTRIES;
 
 	while (read_pointer < write_pointer) {
-		status = get_context_status(ring, ++read_pointer, &status_id);
+		csb[csb_read][0] = get_context_status(ring, ++read_pointer,
+						      &csb[csb_read][1]);
+		csb_read++;
+	}
 
-		if (unlikely(status & GEN8_CTX_STATUS_PREEMPTED)) {
-			if (status & GEN8_CTX_STATUS_LITE_RESTORE) {
-				if (execlists_check_remove_request(ring, status_id))
+	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
+
+	/* Update the read pointer to the old write pointer. Manual ringbuffer
+	 * management ftw </sarcasm> */
+	I915_WRITE_FW(RING_CONTEXT_STATUS_PTR(ring),
+		      _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK,
+				    ring->next_context_status_buffer << 8));
+
+	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
+	spin_unlock(&dev_priv->uncore.lock);
+
+	spin_lock(&ring->execlist_lock);
+
+	for (i = 0; i < csb_read; i++) {
+		if (unlikely(csb[i][0] & GEN8_CTX_STATUS_PREEMPTED)) {
+			if (csb[i][0] & GEN8_CTX_STATUS_LITE_RESTORE) {
+				if (execlists_check_remove_request(ring, csb[i][1]))
 					WARN(1, "Lite Restored request removed from queue\n");
 			} else
 				WARN(1, "Preemption without Lite Restore\n");
 		}
 
-		if (status & (GEN8_CTX_STATUS_ACTIVE_IDLE |
+		if (csb[i][0] & (GEN8_CTX_STATUS_ACTIVE_IDLE |
 		    GEN8_CTX_STATUS_ELEMENT_SWITCH))
 			submit_contexts +=
-				execlists_check_remove_request(ring, status_id);
+				execlists_check_remove_request(ring, csb[i][1]);
 	}
 
 	if (submit_contexts) {
 		if (!ring->disable_lite_restore_wa ||
-		    (status & GEN8_CTX_STATUS_ACTIVE_IDLE))
-			execlists_context_unqueue__locked(ring);
+		    (csb[i][0] & GEN8_CTX_STATUS_ACTIVE_IDLE))
+			execlists_context_unqueue(ring);
 	}
 
-	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
-
-	/* Update the read pointer to the old write pointer. Manual ringbuffer
-	 * management ftw </sarcasm> */
-	I915_WRITE_FW(RING_CONTEXT_STATUS_PTR(ring),
-		      _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK,
-				    ring->next_context_status_buffer << 8));
-
-	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
-	spin_unlock(&dev_priv->uncore.lock);
-
 	spin_unlock(&ring->execlist_lock);
 
 	if (unlikely(submit_contexts > 2))