[PATCH 1/7] drm/i915/bdw: Always issue a force restore

[PATCH 1/7] drm/i915/bdw: Always issue a force restore

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The PDPs seem to get screwed up otherwise, specifically PDP0. I am not
really clear why this is required, it just works with full PPGTT.

v2: Only do it for gen8, to limit regression potential

v3: Fix the bugzilla links

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78891
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78935
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78936
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78937
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78938

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3b99390..56f7b1e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -586,6 +586,9 @@ mi_set_context(struct intel_engine_cs *ring,
 	else
 		intel_ring_emit(ring, MI_NOOP);
 
+	if (INTEL_INFO(ring->dev)->gen == 8)
+		hw_flags |= MI_FORCE_RESTORE;
+
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_emit(ring, MI_SET_CONTEXT);
 	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->legacy_hw_ctx.rcs_state) |
-- 
1.9.3

[PATCH 2/7] drm/i915: Add thread stall DOP clock gating workaround on Broadwell.

[Rodrigo Vivi]

From: Kenneth Graunke <kenneth@whitecape.org>

Ben and I believe this will be necessary on production hardware.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 684dc5f..f919596 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5412,6 +5412,10 @@ static void gen8_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(GEN8_ROW_CHICKEN,
 		   _MASKED_BIT_ENABLE(STALL_DOP_GATING_DISABLE));
 
+	/* WaDisableThreadStallDopClockGating:bdw */
+	I915_WRITE(GEN8_ROW_CHICKEN,
+		   _MASKED_BIT_ENABLE(STALL_DOP_GATING_DISABLE));
+
 	/*
 	 * This GEN8_CENTROID_PIXEL_OPT_DIS W/A is only needed for
 	 * pre-production hardware
-- 
1.9.3

[PATCH 3/7] drm/i915/bdw: MI_FLUSH_DW a qword instead of dword

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The actual post sync op is "Write Immediate Data QWord." It is therefore
arguable that we should have always done a qword write.

The actual impetus for this patch is our decoder complains when we write
a dword and I was trying to eliminate the spurious errors. With this
patch, I've noticed a really strange reproducible error turns into a
different strange reproducible error - so it does indeed have some
effect of some sort.

This was also recommended to me by someone that is familiar with the
Windows driver.

It's based on top of the semaphore series, so it won't be easily
applied, I'd guess.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 95 +++++++++++++++++++++++++--------
 1 file changed, 74 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2908896..9a562b5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -727,7 +727,7 @@ static int gen8_rcs_signal(struct intel_engine_cs *signaller,
 static int gen8_xcs_signal(struct intel_engine_cs *signaller,
 			   unsigned int num_dwords)
 {
-#define MBOX_UPDATE_DWORDS 6
+#define MBOX_UPDATE_DWORDS 8
 	struct drm_device *dev = signaller->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *waiter;
@@ -746,15 +746,18 @@ static int gen8_xcs_signal(struct intel_engine_cs *signaller,
 		if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
 			continue;
 
-		intel_ring_emit(signaller, (MI_FLUSH_DW + 1) |
+		intel_ring_emit(signaller, (MI_FLUSH_DW + 2) |
 					   MI_FLUSH_DW_OP_STOREDW);
 		intel_ring_emit(signaller, lower_32_bits(gtt_offset) |
 					   MI_FLUSH_DW_USE_GTT);
 		intel_ring_emit(signaller, upper_32_bits(gtt_offset));
 		intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
+		intel_ring_emit(signaller, 0); /* upper dword */
+
 		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
 					   MI_SEMAPHORE_TARGET(waiter->id));
 		intel_ring_emit(signaller, 0);
+		intel_ring_emit(signaller, MI_NOOP);
 	}
 
 	return 0;
@@ -1939,8 +1942,6 @@ static int gen6_bsd_ring_flush(struct intel_engine_cs *ring,
 		return ret;
 
 	cmd = MI_FLUSH_DW;
-	if (INTEL_INFO(ring->dev)->gen >= 8)
-		cmd += 1;
 	/*
 	 * Bspec vol 1c.5 - video engine command streamer:
 	 * "If ENABLED, all TLBs will be invalidated once the flush
@@ -1952,13 +1953,38 @@ static int gen6_bsd_ring_flush(struct intel_engine_cs *ring,
 			MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
 	intel_ring_emit(ring, cmd);
 	intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
-	if (INTEL_INFO(ring->dev)->gen >= 8) {
-		intel_ring_emit(ring, 0); /* upper addr */
-		intel_ring_emit(ring, 0); /* value */
-	} else  {
-		intel_ring_emit(ring, 0);
-		intel_ring_emit(ring, MI_NOOP);
-	}
+	intel_ring_emit(ring, 0);
+	intel_ring_emit(ring, MI_NOOP);
+	intel_ring_advance(ring);
+	return 0;
+}
+
+static int gen8_bsd_ring_flush(struct intel_engine_cs *ring,
+			       u32 invalidate, u32 flush)
+{
+	uint32_t cmd;
+	int ret;
+
+	ret = intel_ring_begin(ring, 6);
+	if (ret)
+		return ret;
+
+	cmd = MI_FLUSH_DW + 2;
+	/*
+	 * Bspec vol 1c.5 - video engine command streamer:
+	 * "If ENABLED, all TLBs will be invalidated once the flush
+	 * operation is complete. This bit is only valid when the
+	 * Post-Sync Operation field is a value of 1h or 3h."
+	 */
+	if (invalidate & I915_GEM_GPU_DOMAINS)
+		cmd |= MI_INVALIDATE_TLB | MI_INVALIDATE_BSD |
+			MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
+	intel_ring_emit(ring, cmd);
+	intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
+	intel_ring_emit(ring, 0); /* upper addr */
+	intel_ring_emit(ring, 0); /* value */
+	intel_ring_emit(ring, 0); /* value */
+	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
 	return 0;
 }
@@ -2029,8 +2055,38 @@ gen6_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
 	return 0;
 }
 
-/* Blitter support (SandyBridge+) */
+static int gen8_ring_flush(struct intel_engine_cs *ring,
+			   u32 invalidate, u32 flush)
+{
+	uint32_t cmd;
+	int ret;
+
+	ret = intel_ring_begin(ring, 6);
+	if (ret)
+		return ret;
+
+	cmd = MI_FLUSH_DW + 2;
+	/*
+	 * Bspec vol 1c.3 - blitter engine command streamer:
+	 * "If ENABLED, all TLBs will be invalidated once the flush
+	 * operation is complete. This bit is only valid when the
+	 * Post-Sync Operation field is a value of 1h or 3h."
+	 */
+	if (invalidate & I915_GEM_DOMAIN_RENDER)
+		cmd |= MI_INVALIDATE_TLB | MI_FLUSH_DW_STORE_INDEX |
+			MI_FLUSH_DW_OP_STOREDW;
+	intel_ring_emit(ring, cmd);
+	intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
+	intel_ring_emit(ring, 0); /* upper addr */
+	intel_ring_emit(ring, 0); /* value */
+	intel_ring_emit(ring, 0); /* value */
+	intel_ring_emit(ring, MI_NOOP);
+	intel_ring_advance(ring);
+
+	return 0;
+}
 
+/* Blitter support (SandyBridge+) */
 static int gen6_ring_flush(struct intel_engine_cs *ring,
 			   u32 invalidate, u32 flush)
 {
@@ -2043,8 +2099,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
 		return ret;
 
 	cmd = MI_FLUSH_DW;
-	if (INTEL_INFO(ring->dev)->gen >= 8)
-		cmd += 1;
+
 	/*
 	 * Bspec vol 1c.3 - blitter engine command streamer:
 	 * "If ENABLED, all TLBs will be invalidated once the flush
@@ -2056,13 +2111,8 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
 			MI_FLUSH_DW_OP_STOREDW;
 	intel_ring_emit(ring, cmd);
 	intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
-	if (INTEL_INFO(ring->dev)->gen >= 8) {
-		intel_ring_emit(ring, 0); /* upper addr */
-		intel_ring_emit(ring, 0); /* value */
-	} else  {
-		intel_ring_emit(ring, 0);
-		intel_ring_emit(ring, MI_NOOP);
-	}
+	intel_ring_emit(ring, 0);
+	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
 
 	if (IS_GEN7(dev) && !invalidate && flush)
@@ -2314,6 +2364,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 		if (INTEL_INFO(dev)->gen >= 8) {
 			ring->irq_enable_mask =
 				GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
+			ring->flush = gen8_bsd_ring_flush;
 			ring->irq_get = gen8_ring_get_irq;
 			ring->irq_put = gen8_ring_put_irq;
 			ring->dispatch_execbuffer =
@@ -2422,6 +2473,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 8) {
 		ring->irq_enable_mask =
 			GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
+		ring->flush = gen8_ring_flush;
 		ring->irq_get = gen8_ring_get_irq;
 		ring->irq_put = gen8_ring_put_irq;
 		ring->dispatch_execbuffer = gen8_ring_dispatch_execbuffer;
@@ -2480,6 +2532,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 8) {
 		ring->irq_enable_mask =
 			GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
+		ring->flush = gen8_ring_flush;
 		ring->irq_get = gen8_ring_get_irq;
 		ring->irq_put = gen8_ring_put_irq;
 		ring->dispatch_execbuffer = gen8_ring_dispatch_execbuffer;
-- 
1.9.3

[PATCH 4/7] drm/i915/bdw: cs-stall before state cache invld w/a

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

We do this already for previous GENs. I guess we must do it for BDW too
according to DOCS.

"Pipe_control with CS-stall bit set must be issued before a
pipe-control command that has the State Cache Invalidate bit set."

This does not solve the problem I have unfortunately.

I didn't check if this was in Ville's CHV series. If it was, I
apologize.

NOTE: I tried to use smaller lengths for the command, but nothing made
it happy except 6.

Cc: Kenneth Graunke <kenneth@whitecape.org>
Cc: Jordan Justen <jljusten@gmail.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 9a562b5..2e566e0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -279,17 +279,25 @@ gen6_render_ring_flush(struct intel_engine_cs *ring,
 static int
 gen7_render_ring_cs_stall_wa(struct intel_engine_cs *ring)
 {
-	int ret;
+	int ret, size = 4;
 
-	ret = intel_ring_begin(ring, 4);
+	if (IS_BROADWELL(ring->dev))
+		size += 2;
+
+	ret = intel_ring_begin(ring, size);
 	if (ret)
 		return ret;
 
-	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4));
+	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(size));
 	intel_ring_emit(ring, PIPE_CONTROL_CS_STALL |
 			      PIPE_CONTROL_STALL_AT_SCOREBOARD);
 	intel_ring_emit(ring, 0);
 	intel_ring_emit(ring, 0);
+	if (IS_BROADWELL(ring->dev)) {
+		intel_ring_emit(ring, 0);
+		intel_ring_emit(ring, 0);
+	}
+
 	intel_ring_advance(ring);
 
 	return 0;
@@ -422,6 +430,11 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,
 		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
 		flags |= PIPE_CONTROL_QW_WRITE;
 		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
+
+		/* Workaround: we must issue a pipe_control with CS-stall bit
+		 * set before a pipe_control command that has the state cache
+		 * invalidate bit set. */
+		gen7_render_ring_cs_stall_wa(ring);
 	}
 
 	return gen8_emit_pipe_control(ring, flags, scratch_addr);
-- 
1.9.3

[PATCH 5/7] drm/i915: avoid emiting semaphore wait on GEN8 when seqno wrap happened.

[Rodrigo Vivi]

C: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2e566e0..cd30b39 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -867,15 +867,26 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
 	if (ret)
 		return ret;
 
-	intel_ring_emit(waiter, MI_SEMAPHORE_WAIT |
-				MI_SEMAPHORE_GLOBAL_GTT |
-				MI_SEMAPHORE_POLL |
-				MI_SEMAPHORE_SAD_GTE_SDD);
-	intel_ring_emit(waiter, seqno);
-	intel_ring_emit(waiter,
-			lower_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
-	intel_ring_emit(waiter,
-			upper_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
+	/* If seqno wrap happened, omit the wait with no-ops */
+	if (likely(!i915_gem_has_seqno_wrapped(waiter->dev, seqno))) {
+		intel_ring_emit(waiter, MI_SEMAPHORE_WAIT |
+					MI_SEMAPHORE_GLOBAL_GTT |
+					MI_SEMAPHORE_POLL |
+					MI_SEMAPHORE_SAD_GTE_SDD);
+		intel_ring_emit(waiter, seqno);
+		intel_ring_emit(waiter,
+				lower_32_bits(GEN8_WAIT_OFFSET(waiter,
+							       signaller->id)));
+		intel_ring_emit(waiter,
+				upper_32_bits(GEN8_WAIT_OFFSET(waiter,
+							       signaller->id)));
+	} else {
+		intel_ring_emit(waiter, MI_NOOP);
+		intel_ring_emit(waiter, MI_NOOP);
+		intel_ring_emit(waiter, MI_NOOP);
+		intel_ring_emit(waiter, MI_NOOP);
+	}
+
 	intel_ring_advance(waiter);
 	return 0;
 }
@@ -2572,6 +2583,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 			ring->semaphore.mbox.signal[BCS] = GEN6_BVESYNC;
 			ring->semaphore.mbox.signal[VECS] = GEN6_NOSYNC;
 			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
+
 		}
 	}
 	ring->init = init_ring_common;
-- 
1.9.3

[PATCH 6/7] drm/i915: BDW Semaphore signal with Post Sync

[Rodrigo Vivi]

With this bit set MI_SEMAPHORE_SIGNAL command is executed as a pipelined
PIPE_CONTROL flush command with Semaphore Signal as post sync operation.
However this can only be set only when "Fixed Function DOP Clock Gate Disable"
is set.

This brought a bit of stability on Semaphores minimizing the hangs.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 1 +
 drivers/gpu/drm/i915/intel_pm.c         | 3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
 3 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dc13961..27a54a0 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -269,6 +269,7 @@
 #define MI_SEMAPHORE_SIGNAL	MI_INSTR(0x1b, 0) /* GEN8+ */
 #define   MI_SEMAPHORE_TARGET(engine)	((engine)<<15)
 #define MI_SEMAPHORE_WAIT	MI_INSTR(0x1c, 2) /* GEN8+ */
+#define   MI_SEMAPHORE_POST_SYNC	(1<<21)
 #define   MI_SEMAPHORE_POLL		(1<<15)
 #define   MI_SEMAPHORE_SAD_GTE_SDD	(1<<12)
 #define MI_STORE_DWORD_IMM	MI_INSTR(0x20, 1)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f919596..ab8cbda 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5481,6 +5481,9 @@ static void gen8_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL,
 		   _MASKED_BIT_ENABLE(GEN8_RC_SEMA_IDLE_MSG_DISABLE));
 
+	I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL,
+		   _MASKED_BIT_ENABLE(GEN8_FF_DOP_CLOCK_GATE_DISABLE));
+
 	/* WaDisableSDEUnitClockGating:bdw */
 	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
 		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index cd30b39..edb8234 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -730,6 +730,7 @@ static int gen8_rcs_signal(struct intel_engine_cs *signaller,
 		intel_ring_emit(signaller, signaller->outstanding_lazy_seqno);
 		intel_ring_emit(signaller, 0);
 		intel_ring_emit(signaller, MI_SEMAPHORE_SIGNAL |
+					   MI_SEMAPHORE_POST_SYNC |
 					   MI_SEMAPHORE_TARGET(waiter->id));
 		intel_ring_emit(signaller, 0);
 	}
-- 
1.9.3

[PATCH 7/7] Revert "drm/i915: Enable semaphores on BDW"

[Rodrigo Vivi]

This reverts commit 521e62e49a42661a4ee0102644517dbe2f100a23.

Although POST_SYNC brought a bit of stability to Semaphores on BDW
it didn't solved all issues and some hungs can still occour when
semaphores are enabled on BDW. Also some sloweness can be found on some
igt tests, althoguth it apparently doesn't affect real workloads.

Besides that, no real performance gain was found on our tests with different
and even multiple workloads.

Let's disable it again for now. At least until we are sure it is safe
to re-enable it.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b25c..ec96f9a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -481,6 +481,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	if (i915.semaphores >= 0)
 		return i915.semaphores;
 
+	/* Until we get further testing... */
+	if (IS_GEN8(dev))
+		return false;
+
 #ifdef CONFIG_INTEL_IOMMU
 	/* Enable semaphores on SNB when IO remapping is off */
 	if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
-- 
1.9.3

Re: [PATCH 1/7] drm/i915/bdw: Always issue a force restore

[Ben Widawsky]

On Mon, Aug 04, 2014 at 11:15:13AM -0700, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> The PDPs seem to get screwed up otherwise, specifically PDP0. I am not
> really clear why this is required, it just works with full PPGTT.
> 
> v2: Only do it for gen8, to limit regression potential
> 
> v3: Fix the bugzilla links
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78891
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78935
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78936
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78937
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78938
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Hi Rodrigo. This patch  was superseded by my [partially merged] series
here:
http://lists.freedesktop.org/archives/intel-gfx/2014-July/048311.html

In that series I went through very thoroughly with design why we need to
do this, and verified those patches do address this issue.

So yeah, NAK from me - and please get those reworked or merged or
whatever.

Thanks

[snip]


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/7] drm/i915/bdw: MI_FLUSH_DW a qword instead of dword

[Ben Widawsky]

On Mon, Aug 04, 2014 at 11:15:15AM -0700, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> The actual post sync op is "Write Immediate Data QWord." It is therefore
> arguable that we should have always done a qword write.
> 
> The actual impetus for this patch is our decoder complains when we write
> a dword and I was trying to eliminate the spurious errors. With this
> patch, I've noticed a really strange reproducible error turns into a
> different strange reproducible error - so it does indeed have some
> effect of some sort.
> 
> This was also recommended to me by someone that is familiar with the
> Windows driver.
> 
> It's based on top of the semaphore series, so it won't be easily
> applied, I'd guess.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Did we determine this was needed for anything other than shutting up the
instruction decoder, for debug? Seems like if the existing stuff ain't
broke, don't fix it.

[snip]


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 5/7] drm/i915: avoid emiting semaphore wait on GEN8 when seqno wrap happened.

[Daniel Vetter]

On Mon, Aug 04, 2014 at 11:15:17AM -0700, Rodrigo Vivi wrote:

This commit message is too thin.

> C: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Does this blow up with Mika's seqno wrap testcase? If so please add the
right Testcase: line, if not, how can we repro this?
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2e566e0..cd30b39 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -867,15 +867,26 @@ gen8_ring_sync(struct intel_engine_cs *waiter,
>  	if (ret)
>  		return ret;
>  
> -	intel_ring_emit(waiter, MI_SEMAPHORE_WAIT |
> -				MI_SEMAPHORE_GLOBAL_GTT |
> -				MI_SEMAPHORE_POLL |
> -				MI_SEMAPHORE_SAD_GTE_SDD);
> -	intel_ring_emit(waiter, seqno);
> -	intel_ring_emit(waiter,
> -			lower_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
> -	intel_ring_emit(waiter,
> -			upper_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
> +	/* If seqno wrap happened, omit the wait with no-ops */
> +	if (likely(!i915_gem_has_seqno_wrapped(waiter->dev, seqno))) {
> +		intel_ring_emit(waiter, MI_SEMAPHORE_WAIT |
> +					MI_SEMAPHORE_GLOBAL_GTT |
> +					MI_SEMAPHORE_POLL |
> +					MI_SEMAPHORE_SAD_GTE_SDD);
> +		intel_ring_emit(waiter, seqno);
> +		intel_ring_emit(waiter,
> +				lower_32_bits(GEN8_WAIT_OFFSET(waiter,
> +							       signaller->id)));
> +		intel_ring_emit(waiter,
> +				upper_32_bits(GEN8_WAIT_OFFSET(waiter,
> +							       signaller->id)));
> +	} else {
> +		intel_ring_emit(waiter, MI_NOOP);
> +		intel_ring_emit(waiter, MI_NOOP);
> +		intel_ring_emit(waiter, MI_NOOP);
> +		intel_ring_emit(waiter, MI_NOOP);
> +	}
> +
>  	intel_ring_advance(waiter);
>  	return 0;
>  }
> @@ -2572,6 +2583,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  			ring->semaphore.mbox.signal[BCS] = GEN6_BVESYNC;
>  			ring->semaphore.mbox.signal[VECS] = GEN6_NOSYNC;
>  			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
> +
>  		}
>  	}
>  	ring->init = init_ring_common;
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/7] drm/i915/bdw: Always issue a force restore

[Daniel Vetter]

On Mon, Aug 04, 2014 at 06:20:00PM -0700, Ben Widawsky wrote:
> On Mon, Aug 04, 2014 at 11:15:13AM -0700, Rodrigo Vivi wrote:
> > From: Ben Widawsky <benjamin.widawsky@intel.com>
> > 
> > The PDPs seem to get screwed up otherwise, specifically PDP0. I am not
> > really clear why this is required, it just works with full PPGTT.
> > 
> > v2: Only do it for gen8, to limit regression potential
> > 
> > v3: Fix the bugzilla links
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78891
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78935
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78936
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78937
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78938
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Hi Rodrigo. This patch  was superseded by my [partially merged] series
> here:
> http://lists.freedesktop.org/archives/intel-gfx/2014-July/048311.html
> 
> In that series I went through very thoroughly with design why we need to
> do this, and verified those patches do address this issue.
> 
> So yeah, NAK from me - and please get those reworked or merged or
> whatever.

Iirc I've stopped merging on that series since it mixes up gen8 specific
fixes with generic ppgtt fixes. Specifically for the ppgtt_release issues
we now have a patch (plus fixups) which is almost ready (just stalling for
review). Can you pls go through that series with Rodrigo so that he knows
which parts are the gen8 fixes?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/7] drm/i915/bdw: cs-stall before state cache invld w/a

[Ville Syrjälä]

On Mon, Aug 04, 2014 at 11:15:16AM -0700, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> We do this already for previous GENs. I guess we must do it for BDW too
> according to DOCS.
> 
> "Pipe_control with CS-stall bit set must be issued before a
> pipe-control command that has the State Cache Invalidate bit set."
> 
> This does not solve the problem I have unfortunately.
> 
> I didn't check if this was in Ville's CHV series. If it was, I
> apologize.

Ken's version from long ago was included there. It's just waiting
to be merged.

> 
> NOTE: I tried to use smaller lengths for the command, but nothing made
> it happy except 6.
> 
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Jordan Justen <jljusten@gmail.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 9a562b5..2e566e0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -279,17 +279,25 @@ gen6_render_ring_flush(struct intel_engine_cs *ring,
>  static int
>  gen7_render_ring_cs_stall_wa(struct intel_engine_cs *ring)
>  {
> -	int ret;
> +	int ret, size = 4;
>  
> -	ret = intel_ring_begin(ring, 4);
> +	if (IS_BROADWELL(ring->dev))
> +		size += 2;
> +
> +	ret = intel_ring_begin(ring, size);
>  	if (ret)
>  		return ret;
>  
> -	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4));
> +	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(size));
>  	intel_ring_emit(ring, PIPE_CONTROL_CS_STALL |
>  			      PIPE_CONTROL_STALL_AT_SCOREBOARD);
>  	intel_ring_emit(ring, 0);
>  	intel_ring_emit(ring, 0);
> +	if (IS_BROADWELL(ring->dev)) {
> +		intel_ring_emit(ring, 0);
> +		intel_ring_emit(ring, 0);
> +	}
> +
>  	intel_ring_advance(ring);
>  
>  	return 0;
> @@ -422,6 +430,11 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,
>  		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
>  		flags |= PIPE_CONTROL_QW_WRITE;
>  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> +
> +		/* Workaround: we must issue a pipe_control with CS-stall bit
> +		 * set before a pipe_control command that has the state cache
> +		 * invalidate bit set. */
> +		gen7_render_ring_cs_stall_wa(ring);
>  	}
>  
>  	return gen8_emit_pipe_control(ring, flags, scratch_addr);
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/7] drm/i915: Add thread stall DOP clock gating workaround on Broadwell.

[Ville Syrjälä]

On Mon, Aug 04, 2014 at 11:15:14AM -0700, Rodrigo Vivi wrote:
> From: Kenneth Graunke <kenneth@whitecape.org>
> 
> Ben and I believe this will be necessary on production hardware.
> 
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 684dc5f..f919596 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5412,6 +5412,10 @@ static void gen8_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(GEN8_ROW_CHICKEN,
>  		   _MASKED_BIT_ENABLE(STALL_DOP_GATING_DISABLE));
>  
> +	/* WaDisableThreadStallDopClockGating:bdw */
> +	I915_WRITE(GEN8_ROW_CHICKEN,
> +		   _MASKED_BIT_ENABLE(STALL_DOP_GATING_DISABLE));

It's already there just above.

> +
>  	/*
>  	 * This GEN8_CENTROID_PIXEL_OPT_DIS W/A is only needed for
>  	 * pre-production hardware
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 7/7] Revert "drm/i915: Enable semaphores on BDW"

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 1840 bytes --]

The rest of the series was a reference for the records of what I had and
let semaphores on bdw a bit more stable. But even with them we still get
hungs so please consider only to get the revert for now.


On Mon, Aug 4, 2014 at 11:15 AM, Rodrigo Vivi <rodrigo.vivi@intel.com>
wrote:

> This reverts commit 521e62e49a42661a4ee0102644517dbe2f100a23.
>
> Although POST_SYNC brought a bit of stability to Semaphores on BDW
> it didn't solved all issues and some hungs can still occour when
> semaphores are enabled on BDW. Also some sloweness can be found on some
> igt tests, althoguth it apparently doesn't affect real workloads.
>
> Besides that, no real performance gain was found on our tests with
> different
> and even multiple workloads.
>
> Let's disable it again for now. At least until we are sure it is safe
> to re-enable it.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b25c..ec96f9a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -481,6 +481,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>         if (i915.semaphores >= 0)
>                 return i915.semaphores;
>
> +       /* Until we get further testing... */
> +       if (IS_GEN8(dev))
> +               return false;
> +
>  #ifdef CONFIG_INTEL_IOMMU
>         /* Enable semaphores on SNB when IO remapping is off */
>         if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 2693 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 7/7] Revert "drm/i915: Enable semaphores on BDW"

[Daniel Vetter]

On Thu, Aug 07, 2014 at 01:05:52PM -0700, Rodrigo Vivi wrote:
> The rest of the series was a reference for the records of what I had and
> let semaphores on bdw a bit more stable. But even with them we still get
> hungs so please consider only to get the revert for now.

Thanks for the reminder, picked up for -fixes, thanks for the patch.
-Daniel
> 
> 
> On Mon, Aug 4, 2014 at 11:15 AM, Rodrigo Vivi <rodrigo.vivi@intel.com>
> wrote:
> 
> > This reverts commit 521e62e49a42661a4ee0102644517dbe2f100a23.
> >
> > Although POST_SYNC brought a bit of stability to Semaphores on BDW
> > it didn't solved all issues and some hungs can still occour when
> > semaphores are enabled on BDW. Also some sloweness can be found on some
> > igt tests, althoguth it apparently doesn't affect real workloads.
> >
> > Besides that, no real performance gain was found on our tests with
> > different
> > and even multiple workloads.
> >
> > Let's disable it again for now. At least until we are sure it is safe
> > to re-enable it.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 6c4b25c..ec96f9a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -481,6 +481,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> >         if (i915.semaphores >= 0)
> >                 return i915.semaphores;
> >
> > +       /* Until we get further testing... */
> > +       if (IS_GEN8(dev))
> > +               return false;
> > +
> >  #ifdef CONFIG_INTEL_IOMMU
> >         /* Enable semaphores on SNB when IO remapping is off */
> >         if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/7] Revert "drm/i915: Enable semaphores on BDW"

[Daniel Vetter]

On Mon, Aug 04, 2014 at 11:15:19AM -0700, Rodrigo Vivi wrote:
> This reverts commit 521e62e49a42661a4ee0102644517dbe2f100a23.
> 
> Although POST_SYNC brought a bit of stability to Semaphores on BDW
> it didn't solved all issues and some hungs can still occour when
> semaphores are enabled on BDW. Also some sloweness can be found on some
> igt tests, althoguth it apparently doesn't affect real workloads.
> 
> Besides that, no real performance gain was found on our tests with different
> and even multiple workloads.
> 
> Let's disable it again for now. At least until we are sure it is safe
> to re-enable it.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b25c..ec96f9a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -481,6 +481,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  	if (i915.semaphores >= 0)
>  		return i915.semaphores;
>  
> +	/* Until we get further testing... */
> +	if (IS_GEN8(dev))
> +		return false;

Aside: With this we can't test the code any more at all. The usual
approach for adjusting defaults when it's not the same on all platforms is
to set the module option to -1 (per-platform defaults) and have a
sanitize_foo function call to set it to the correct default. Would be nice
on top of the revert.
-Daniel

> +
>  #ifdef CONFIG_INTEL_IOMMU
>  	/* Enable semaphores on SNB when IO remapping is off */
>  	if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/7] Revert "drm/i915: Enable semaphores on BDW"

[Chris Wilson]

On Fri, Aug 08, 2014 at 09:13:06AM +0200, Daniel Vetter wrote:
> On Mon, Aug 04, 2014 at 11:15:19AM -0700, Rodrigo Vivi wrote:
> > Besides that, no real performance gain was found on our tests with different
> > and even multiple workloads.

That either means you didn't try hard enough, or that the implementation
is extremely inefficient.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre