diff mbox

[v4,4/6] drm/i915/gen8: Add WaFlushCoherentL3CacheLinesAtContextSwitch workaround

Message ID 1434482725-21823-5-git-send-email-arun.siluvery@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

arun.siluvery@linux.intel.com June 16, 2015, 7:25 p.m. UTC 
In Indirect context w/a batch buffer,
+WaFlushCoherentL3CacheLinesAtContextSwitch:bdw

v2: Add LRI commands to set/reset bit that invalidates coherent lines,
update WA to include programming restrictions and exclude CHV as
it is not required (Ville)

Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  2 ++
 drivers/gpu/drm/i915/intel_lrc.c | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Chris Wilson June 16, 2015, 8:38 p.m. UTC | #1 
On Tue, Jun 16, 2015 at 08:25:23PM +0100, Arun Siluvery wrote:
> +	/* WaFlushCoherentL3CacheLinesAtContextSwitch:bdw */
> +	if (IS_BROADWELL(ring->dev)) {
> +		struct drm_i915_private *dev_priv = ring->dev->dev_private;

dev_priv = to_i915(ring->dev);

> +
> +		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
> +		cmd[index++] = GEN8_L3SQCREG4;
> +		cmd[index++] = I915_READ(GEN8_L3SQCREG4) |
> +			GEN8_LQSC_FLUSH_COHERENT_LINES;

Read the reg once, it is clearer that way.

> +
> +		cmd[index++] = GFX_OP_PIPE_CONTROL(6);
> +		cmd[index++] = PIPE_CONTROL_CS_STALL |
> +			PIPE_CONTROL_DC_FLUSH_ENABLE;
> +		cmd[index++] = 0;
> +		cmd[index++] = 0;
> +		cmd[index++] = 0;
> +		cmd[index++] = 0;
> +
> +		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
> +		cmd[index++] = GEN8_L3SQCREG4;
> +		cmd[index++] = I915_READ(GEN8_L3SQCREG4) &
> +			~GEN8_LQSC_FLUSH_COHERENT_LINES;
-Chris
Dave Gordon June 26, 2015, 4:45 p.m. UTC | #2 
On 16/06/15 21:38, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 08:25:23PM +0100, Arun Siluvery wrote:
>> +	/* WaFlushCoherentL3CacheLinesAtContextSwitch:bdw */
>> +	if (IS_BROADWELL(ring->dev)) {
>> +		struct drm_i915_private *dev_priv = ring->dev->dev_private;
> 
> dev_priv = to_i915(ring->dev);
> 
>> +
>> +		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
>> +		cmd[index++] = GEN8_L3SQCREG4;
>> +		cmd[index++] = I915_READ(GEN8_L3SQCREG4) |
>> +			GEN8_LQSC_FLUSH_COHERENT_LINES;
> 
> Read the reg once, it is clearer that way.
> 
>> +
>> +		cmd[index++] = GFX_OP_PIPE_CONTROL(6);
>> +		cmd[index++] = PIPE_CONTROL_CS_STALL |
>> +			PIPE_CONTROL_DC_FLUSH_ENABLE;
>> +		cmd[index++] = 0;
>> +		cmd[index++] = 0;
>> +		cmd[index++] = 0;
>> +		cmd[index++] = 0;
>> +
>> +		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
>> +		cmd[index++] = GEN8_L3SQCREG4;
>> +		cmd[index++] = I915_READ(GEN8_L3SQCREG4) &
>> +			~GEN8_LQSC_FLUSH_COHERENT_LINES;
> -Chris

What Chris said. But also, is it even meaningful to read a h/w register
now (when?) and use its value as the basis for future LRI instructions?
How (and when) does this register get its initial value, and does it get
changed at any other time? If the value we put in the register is a
run-time constant, there's really no need to read it back even once.

.Dave.
Chris Wilson June 26, 2015, 4:56 p.m. UTC | #3 
On Fri, Jun 26, 2015 at 05:45:08PM +0100, Dave Gordon wrote:
> On 16/06/15 21:38, Chris Wilson wrote:
> > On Tue, Jun 16, 2015 at 08:25:23PM +0100, Arun Siluvery wrote:
> >> +	/* WaFlushCoherentL3CacheLinesAtContextSwitch:bdw */
> >> +	if (IS_BROADWELL(ring->dev)) {
> >> +		struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > 
> > dev_priv = to_i915(ring->dev);
> > 
> >> +
> >> +		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
> >> +		cmd[index++] = GEN8_L3SQCREG4;
> >> +		cmd[index++] = I915_READ(GEN8_L3SQCREG4) |
> >> +			GEN8_LQSC_FLUSH_COHERENT_LINES;
> > 
> > Read the reg once, it is clearer that way.
> > 
> >> +
> >> +		cmd[index++] = GFX_OP_PIPE_CONTROL(6);
> >> +		cmd[index++] = PIPE_CONTROL_CS_STALL |
> >> +			PIPE_CONTROL_DC_FLUSH_ENABLE;
> >> +		cmd[index++] = 0;
> >> +		cmd[index++] = 0;
> >> +		cmd[index++] = 0;
> >> +		cmd[index++] = 0;
> >> +
> >> +		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
> >> +		cmd[index++] = GEN8_L3SQCREG4;
> >> +		cmd[index++] = I915_READ(GEN8_L3SQCREG4) &
> >> +			~GEN8_LQSC_FLUSH_COHERENT_LINES;
> > -Chris
> 
> What Chris said. But also, is it even meaningful to read a h/w register
> now (when?) and use its value as the basis for future LRI instructions?
> How (and when) does this register get its initial value, and does it get
> changed at any other time? If the value we put in the register is a
> run-time constant, there's really no need to read it back even once.

True. To be generic, we need to do STORE_REG_MEM, LRI constant value,
then LOAD_REG_MEM. Hopefully, no userspace ever actually need to twiddle
that register and we can just load a constant value.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 84af255..d14ad20 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -426,6 +426,7 @@ 
 #define   PIPE_CONTROL_INDIRECT_STATE_DISABLE		(1<<9)
 #define   PIPE_CONTROL_NOTIFY				(1<<8)
 #define   PIPE_CONTROL_FLUSH_ENABLE			(1<<7) /* gen7+ */
+#define   PIPE_CONTROL_DC_FLUSH_ENABLE			(1<<5)
 #define   PIPE_CONTROL_VF_CACHE_INVALIDATE		(1<<4)
 #define   PIPE_CONTROL_CONST_CACHE_INVALIDATE		(1<<3)
 #define   PIPE_CONTROL_STATE_CACHE_INVALIDATE		(1<<2)
@@ -5788,6 +5789,7 @@  enum skl_disp_power_wells {
 
 #define GEN8_L3SQCREG4				0xb118
 #define  GEN8_LQSC_RO_PERF_DIS			(1<<27)
+#define  GEN8_LQSC_FLUSH_COHERENT_LINES		(1<<21)
 
 /* GEN8 chicken */
 #define HDC_CHICKEN0				0x7300
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c9875f6..92556b9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1094,6 +1094,29 @@  static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
 	cmd[index++] = MI_ARB_ON_OFF | MI_ARB_DISABLE;
 
+	/* WaFlushCoherentL3CacheLinesAtContextSwitch:bdw */
+	if (IS_BROADWELL(ring->dev)) {
+		struct drm_i915_private *dev_priv = ring->dev->dev_private;
+
+		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
+		cmd[index++] = GEN8_L3SQCREG4;
+		cmd[index++] = I915_READ(GEN8_L3SQCREG4) |
+			GEN8_LQSC_FLUSH_COHERENT_LINES;
+
+		cmd[index++] = GFX_OP_PIPE_CONTROL(6);
+		cmd[index++] = PIPE_CONTROL_CS_STALL |
+			PIPE_CONTROL_DC_FLUSH_ENABLE;
+		cmd[index++] = 0;
+		cmd[index++] = 0;
+		cmd[index++] = 0;
+		cmd[index++] = 0;
+
+		cmd[index++] = MI_LOAD_REGISTER_IMM(1);
+		cmd[index++] = GEN8_L3SQCREG4;
+		cmd[index++] = I915_READ(GEN8_L3SQCREG4) &
+			~GEN8_LQSC_FLUSH_COHERENT_LINES;
+	}
+
 	/* padding */
         while (((unsigned long) (cmd + index) % CACHELINE_BYTES) != 0)
 		cmd[index++] = MI_NOOP;