[PATCH 0/8] Gen9 HW whitelist and Preemption WA patches

[PATCH 0/8] Gen9 HW whitelist and Preemption WA patches

[Arun Siluvery]

Set of patches that add HW whitelist framework and Preemption WA patches.

HW whitelist patch is already sent to list and reviewed[1], it is just
rebased in this version. This was not merged before as there was no user; this
is still the case but since Preemption patches[2] are already on the list this is
going to be required sooner or later.

Please review the remaining patches.

[1] https://patchwork.freedesktop.org/patch/60937/
[2] http://lists.freedesktop.org/archives/intel-gfx/2015-November/080965.html

Arun Siluvery (8):
  drm/i915/gen9: Add framework to whitelist specific GPU registers
  drm/i915/gen9: Add GEN8_CS_CHICKEN1 to HW whitelist
  drm/i915/gen9: Add HDC_CHICKEN1 to HW whitelist
  drm/i915/bxt: Add GEN9_CS_DEBUG_MODE1 to HW whitelist
  drm/i915/bxt: Add GEN8_L3SQCREG4 to HW whitelist
  drm/i915/skl: Add GEN8_L3SQCREG4 to HW whitelist
  drm/i915/skl: Enable Per context Preemption granularity control
  drm/i915/gen9: Add WaOCLCoherentLineFlush

 drivers/gpu/drm/i915/i915_drv.h         |  9 ++++-
 drivers/gpu/drm/i915/i915_reg.h         | 11 ++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 62 +++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 1 deletion(-)

-- 
1.9.1

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

[PATCH 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

Some of the HW registers are privileged and cannot be written to from a
non-privileged batch buffers coming from userspace unless they are on whitelist.
Userspace need write access to them to implement preemption related WA.

The reason for using this approach is, the register bits that control preemption
granularity at the HW level are not context save/restored; so even if we set these
bits always in kernel they are going to change once the context is switched out.
We can consider making them non-privileged by default but these registers also
contain other chicken bits which should not be allowed to be modified.

In the later revisions controlling bits are save/restored at context level but
in the existing revisions these are exported via other debug registers and should
be on the whitelist. This patch adds changes to provide HW with a list of registers
to be whitelisted. HW checks this list during execution and provides access accordingly.

HW imposes a limit on the number of registers on whitelist and it is per-engine.
At this point we are only enabling whitelist for RCS and we don't foresee any
requirement for other engines.

The registers to be whitelisted are added using generic workaround list mechanism,
even these are only enablers for userspace workarounds. But by sharing this
mechanism we get some test assets without additional cost (Mika).

v2: rebase

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
 drivers/gpu/drm/i915/i915_reg.h         |  3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 104bd18..660afe1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1653,11 +1653,18 @@ struct i915_wa_reg {
 	u32 mask;
 };
 
-#define I915_MAX_WA_REGS 16
+/*
+ * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
+ * allowing it for RCS as we don't foresee any requirement of having
+ * a whitelist for other engines. When it is really required for
+ * other engines then the limit need to be increased.
+ */
+#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
 
 struct i915_workarounds {
 	struct i915_wa_reg reg[I915_MAX_WA_REGS];
 	u32 count;
+	u32 hw_whitelist_index[I915_NUM_RINGS];
 };
 
 struct i915_virtual_gpu {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0a98889..6668bb0 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
 #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
 #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
 
+#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0)
+#define   RING_MAX_NONPRIV_SLOTS  12
+
 #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
 
 #if 0
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4060acf..354da81 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -787,6 +787,23 @@ static int wa_add(struct drm_i915_private *dev_priv,
 
 #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
 
+static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
+					i915_reg_t reg_addr)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct i915_workarounds *wa = &dev_priv->workarounds;
+	const uint32_t index = wa->hw_whitelist_index[ring->id];
+
+	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
+		return -EINVAL;
+
+	WA_WRITE(_MMIO(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4),
+		 reg_addr.reg);
+	wa->hw_whitelist_index[ring->id]++;
+
+	return 0;
+}
+
 static int gen8_init_workarounds(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
@@ -1115,6 +1132,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
 	WARN_ON(ring->id != RCS);
 
 	dev_priv->workarounds.count = 0;
+	dev_priv->workarounds.hw_whitelist_index[ring->id] = 0;
 
 	if (IS_BROADWELL(dev))
 		return bdw_init_workarounds(ring);
-- 
1.9.1

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

[PATCH 2/8] drm/i915/gen9: Add GEN8_CS_CHICKEN1 to HW whitelist

[Arun Siluvery]

Required for WaEnablePreemptionGranularityControlByUMD:skl,bxt

Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6668bb0..1067ff0 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5998,6 +5998,8 @@ enum skl_disp_power_wells {
 #define FF_SLICE_CS_CHICKEN2			_MMIO(0x20e4)
 #define  GEN9_TSG_BARRIER_ACK_DISABLE		(1<<8)
 
+#define GEN8_CS_CHICKEN1		_MMIO(0x2580)
+
 /* GEN7 chicken */
 #define GEN7_COMMON_SLICE_CHICKEN1		_MMIO(0x7010)
 # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC	((1<<10) | (1<<26))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 354da81..35e78ed 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -909,6 +909,7 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
+	int ret;
 
 	/* WaEnableLbsSlaRetryTimerDecrement:skl */
 	I915_WRITE(BDW_SCRATCH1, I915_READ(BDW_SCRATCH1) |
@@ -979,6 +980,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
 	/* WaDisableSTUnitPowerOptimization:skl,bxt */
 	WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN2, GEN8_ST_PO_DISABLE);
 
+	/* WaEnablePreemptionGranularityControlByUMD:skl,bxt */
+	ret= wa_ring_whitelist_reg(ring, GEN8_CS_CHICKEN1);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
1.9.1

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

[PATCH 3/8] drm/i915/gen9: Add HDC_CHICKEN1 to HW whitelist

[Arun Siluvery]

Required for WaAllowUMDToModifyHDCChicken1:skl,bxt

Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 2 ++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1067ff0..16ef377 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6045,6 +6045,8 @@ enum skl_disp_power_wells {
 #define  HDC_FORCE_NON_COHERENT			(1<<4)
 #define  HDC_BARRIER_PERFORMANCE_DISABLE	(1<<10)
 
+#define GEN8_HDC_CHICKEN1			_MMIO(0x7304)
+
 /* GEN9 chicken */
 #define SLICE_ECO_CHICKEN0			_MMIO(0x7308)
 #define   PIXEL_MASK_CAMMING_DISABLE		(1 << 14)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 35e78ed..2241a92 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -985,6 +985,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
 	if (ret)
 		return ret;
 
+	/* WaAllowUMDToModifyHDCChicken1:skl,bxt */
+	ret = wa_ring_whitelist_reg(ring, GEN8_HDC_CHICKEN1);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
1.9.1

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

[PATCH 4/8] drm/i915/bxt: Add GEN9_CS_DEBUG_MODE1 to HW whitelist

[Arun Siluvery]

Required for,
WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt
WaDisableObjectLevelPreemptionForInstancedDraw:bxt
WaDisableObjectLevelPreemtionForInstanceId:bxt

According to WA database these are only applicable for BXT:A0 but since
A0 and A1 shares the same GT these are extended for A1 as well.

These are also required for SKL until B0 but not adding them because they
are pre-production steppings.

Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 16ef377..eabd2af 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5998,6 +5998,7 @@ enum skl_disp_power_wells {
 #define FF_SLICE_CS_CHICKEN2			_MMIO(0x20e4)
 #define  GEN9_TSG_BARRIER_ACK_DISABLE		(1<<8)
 
+#define GEN9_CS_DEBUG_MODE1		_MMIO(0x20EC)
 #define GEN8_CS_CHICKEN1		_MMIO(0x2580)
 
 /* GEN7 chicken */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2241a92..7a46cf1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1132,6 +1132,15 @@ static int bxt_init_workarounds(struct intel_engine_cs *ring)
 			GEN7_SBE_SS_CACHE_DISPATCH_PORT_SHARING_DISABLE);
 	}
 
+	/* WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt */
+	/* WaDisableObjectLevelPreemptionForInstancedDraw:bxt */
+	/* WaDisableObjectLevelPreemtionForInstanceId:bxt */
+	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
+		ret = wa_ring_whitelist_reg(ring, GEN9_CS_DEBUG_MODE1);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
1.9.1

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

[PATCH 5/8] drm/i915/bxt: Add GEN8_L3SQCREG4 to HW whitelist

[Arun Siluvery]

Required for WaDisableLSQCROPERFforOCL:bxt

According to WA database these are only applicable for BXT:A0 but since
A0 and A1 shares the same GT these are extended for A1 as well.

Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7a46cf1..5eb4eea 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1135,10 +1135,15 @@ static int bxt_init_workarounds(struct intel_engine_cs *ring)
 	/* WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt */
 	/* WaDisableObjectLevelPreemptionForInstancedDraw:bxt */
 	/* WaDisableObjectLevelPreemtionForInstanceId:bxt */
+	/* WaDisableLSQCROPERFforOCL:bxt */
 	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
 		ret = wa_ring_whitelist_reg(ring, GEN9_CS_DEBUG_MODE1);
 		if (ret)
 			return ret;
+
+		ret = wa_ring_whitelist_reg(ring, GEN8_L3SQCREG4);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
-- 
1.9.1

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

[PATCH 6/8] drm/i915/skl: Add GEN8_L3SQCREG4 to HW whitelist

[Arun Siluvery]

Required for WaDisableLSQCROPERFforOCL:skl

Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5eb4eea..b8dbd2c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1097,6 +1097,11 @@ static int skl_init_workarounds(struct intel_engine_cs *ring)
 			GEN7_HALF_SLICE_CHICKEN1,
 			GEN7_SBE_SS_CACHE_DISPATCH_PORT_SHARING_DISABLE);
 
+	/* WaDisableLSQCROPERFforOCL:skl */
+	ret = wa_ring_whitelist_reg(ring, GEN8_L3SQCREG4);
+	if (ret)
+		return ret;
+
 	return skl_tune_iz_hashing(ring);
 }
 
-- 
1.9.1

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

[PATCH 7/8] drm/i915/skl: Enable Per context Preemption granularity control

[Arun Siluvery]

Per context preemption granularity control is only available from SKL:E0+

Cc: Dave Gordon <david.s.gordon@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |  3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index eabd2af..97774a3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5995,6 +5995,9 @@ enum skl_disp_power_wells {
 #define SKL_DFSM_CDCLK_LIMIT_450	(2 << 23)
 #define SKL_DFSM_CDCLK_LIMIT_337_5	(3 << 23)
 
+#define GEN7_FF_SLICE_CS_CHICKEN1	_MMIO(0x20E0)
+#define   GEN9_FFSC_PERCTX_PREEMPT_CTRL	(1<<14)
+
 #define FF_SLICE_CS_CHICKEN2			_MMIO(0x20e4)
 #define  GEN9_TSG_BARRIER_ACK_DISABLE		(1<<8)
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b8dbd2c..5a2ad10 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1045,6 +1045,16 @@ static int skl_init_workarounds(struct intel_engine_cs *ring)
 	if (ret)
 		return ret;
 
+	/*
+	 * Actual WA is to disable percontext preemption granularity control
+	 * until D0 which is the default case so this is equivalent to
+	 * !WaDisablePerCtxtPreemptionGranularityControl:skl
+	 */
+	if (IS_SKL_REVID(dev, SKL_REVID_E0, REVID_FOREVER)) {
+		I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
+			   _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
+	}
+
 	if (IS_SKL_REVID(dev, 0, SKL_REVID_D0)) {
 		/* WaDisableChickenBitTSGBarrierAckForFFSliceCS:skl */
 		I915_WRITE(FF_SLICE_CS_CHICKEN2,
-- 
1.9.1

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

[PATCH 8/8] drm/i915/gen9: Add WaOCLCoherentLineFlush

[Arun Siluvery]

This is mainly required for preemption.

Cc: Dave Gordon <david.s.gordon@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5a2ad10..af95091 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -980,6 +980,10 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
 	/* WaDisableSTUnitPowerOptimization:skl,bxt */
 	WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN2, GEN8_ST_PO_DISABLE);
 
+	/* WaOCLCoherentLineFlush:skl,bxt */
+	I915_WRITE(GEN8_L3SQCREG4, (I915_READ(GEN8_L3SQCREG4) |
+				    GEN8_LQSC_FLUSH_COHERENT_LINES));
+
 	/* WaEnablePreemptionGranularityControlByUMD:skl,bxt */
 	ret= wa_ring_whitelist_reg(ring, GEN8_CS_CHICKEN1);
 	if (ret)
-- 
1.9.1

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

Re: [PATCH 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Ville Syrjälä]

On Wed, Jan 13, 2016 at 10:06:26AM +0000, Arun Siluvery wrote:
> Some of the HW registers are privileged and cannot be written to from a
> non-privileged batch buffers coming from userspace unless they are on whitelist.
> Userspace need write access to them to implement preemption related WA.
> 
> The reason for using this approach is, the register bits that control preemption
> granularity at the HW level are not context save/restored; so even if we set these
> bits always in kernel they are going to change once the context is switched out.
> We can consider making them non-privileged by default but these registers also
> contain other chicken bits which should not be allowed to be modified.
> 
> In the later revisions controlling bits are save/restored at context level but
> in the existing revisions these are exported via other debug registers and should
> be on the whitelist. This patch adds changes to provide HW with a list of registers
> to be whitelisted. HW checks this list during execution and provides access accordingly.
> 
> HW imposes a limit on the number of registers on whitelist and it is per-engine.
> At this point we are only enabling whitelist for RCS and we don't foresee any
> requirement for other engines.
> 
> The registers to be whitelisted are added using generic workaround list mechanism,
> even these are only enablers for userspace workarounds. But by sharing this
> mechanism we get some test assets without additional cost (Mika).
> 
> v2: rebase
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 104bd18..660afe1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1653,11 +1653,18 @@ struct i915_wa_reg {
>  	u32 mask;
>  };
>  
> -#define I915_MAX_WA_REGS 16
> +/*
> + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
> + * allowing it for RCS as we don't foresee any requirement of having
> + * a whitelist for other engines. When it is really required for
> + * other engines then the limit need to be increased.
> + */
> +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
>  
>  struct i915_workarounds {
>  	struct i915_wa_reg reg[I915_MAX_WA_REGS];
>  	u32 count;
> +	u32 hw_whitelist_index[I915_NUM_RINGS];
>  };
>  
>  struct i915_virtual_gpu {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0a98889..6668bb0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
>  #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
>  #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
>  
> +#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0)
> +#define   RING_MAX_NONPRIV_SLOTS  12
> +
>  #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
>  
>  #if 0
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4060acf..354da81 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -787,6 +787,23 @@ static int wa_add(struct drm_i915_private *dev_priv,
>  
>  #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
>  
> +static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
> +					i915_reg_t reg_addr)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct i915_workarounds *wa = &dev_priv->workarounds;
> +	const uint32_t index = wa->hw_whitelist_index[ring->id];
> +
> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> +		return -EINVAL;
> +
> +	WA_WRITE(_MMIO(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4),
> +		 reg_addr.reg);

No _MMIO junk outside of i915_reg.h please. Just parametrize the reg define
properly.

> +	wa->hw_whitelist_index[ring->id]++;
> +
> +	return 0;
> +}
> +
>  static int gen8_init_workarounds(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
> @@ -1115,6 +1132,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
>  	WARN_ON(ring->id != RCS);
>  
>  	dev_priv->workarounds.count = 0;
> +	dev_priv->workarounds.hw_whitelist_index[ring->id] = 0;
>  
>  	if (IS_BROADWELL(dev))
>  		return bdw_init_workarounds(ring);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ success: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on dd4a7926b4118f72b7ae0f7b97e9644172df472c drm-intel-nightly: 2016y-01m-13d-09h-05m-34s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i5k-2) UNSTABLE

bdw-nuci7        total:138  pass:129  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:132  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:141  pass:115  dwarn:2   dfail:0   fail:0   skip:24 
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:141  pass:137  dwarn:0   dfail:0   fail:0   skip:4  
hsw-xps12        total:138  pass:133  dwarn:1   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:141  pass:101  dwarn:3   dfail:0   fail:0   skip:37 
ivb-t430s        total:135  pass:122  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:141  pass:122  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1161/

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

Re: [PATCH 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Mika Kuoppala]

Arun Siluvery <arun.siluvery@linux.intel.com> writes:

> Some of the HW registers are privileged and cannot be written to from a
> non-privileged batch buffers coming from userspace unless they are on whitelist.
> Userspace need write access to them to implement preemption related WA.
>
> The reason for using this approach is, the register bits that control preemption
> granularity at the HW level are not context save/restored; so even if we set these
> bits always in kernel they are going to change once the context is switched out.
> We can consider making them non-privileged by default but these registers also
> contain other chicken bits which should not be allowed to be modified.
>
> In the later revisions controlling bits are save/restored at context level but
> in the existing revisions these are exported via other debug registers and should
> be on the whitelist. This patch adds changes to provide HW with a list of registers
> to be whitelisted. HW checks this list during execution and provides access accordingly.
>
> HW imposes a limit on the number of registers on whitelist and it is per-engine.
> At this point we are only enabling whitelist for RCS and we don't foresee any
> requirement for other engines.
>
> The registers to be whitelisted are added using generic workaround list mechanism,
> even these are only enablers for userspace workarounds. But by sharing this
> mechanism we get some test assets without additional cost (Mika).
>
> v2: rebase
>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 104bd18..660afe1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1653,11 +1653,18 @@ struct i915_wa_reg {
>  	u32 mask;
>  };
>  
> -#define I915_MAX_WA_REGS 16
> +/*
> + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
> + * allowing it for RCS as we don't foresee any requirement of having
> + * a whitelist for other engines. When it is really required for
> + * other engines then the limit need to be increased.
> + */
> +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
>  
>  struct i915_workarounds {
>  	struct i915_wa_reg reg[I915_MAX_WA_REGS];
>  	u32 count;
> +	u32 hw_whitelist_index[I915_NUM_RINGS];
>  };
>  
>  struct i915_virtual_gpu {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0a98889..6668bb0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
>  #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
>  #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
>  
> +#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0)
> +#define   RING_MAX_NONPRIV_SLOTS  12
> +
>  #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
>  
>  #if 0
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4060acf..354da81 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -787,6 +787,23 @@ static int wa_add(struct drm_i915_private *dev_priv,
>  
>  #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
>  
> +static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
> +					i915_reg_t reg_addr)

When you remove the _MMIO Ville noted, please also
remove the inline here too.

-Mika

> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct i915_workarounds *wa = &dev_priv->workarounds;
> +	const uint32_t index = wa->hw_whitelist_index[ring->id];
> +
> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> +		return -EINVAL;
> +
> +	WA_WRITE(_MMIO(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4),
> +		 reg_addr.reg);
> +	wa->hw_whitelist_index[ring->id]++;
> +
> +	return 0;
> +}
> +
>  static int gen8_init_workarounds(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
> @@ -1115,6 +1132,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
>  	WARN_ON(ring->id != RCS);
>  
>  	dev_priv->workarounds.count = 0;
> +	dev_priv->workarounds.hw_whitelist_index[ring->id] = 0;
>  
>  	if (IS_BROADWELL(dev))
>  		return bdw_init_workarounds(ring);
> -- 
> 1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

Some of the HW registers are privileged and cannot be written to from a
non-privileged batch buffers coming from userspace unless they are on whitelist.
Userspace need write access to them to implement preemption related WA.

The reason for using this approach is, the register bits that control preemption
granularity at the HW level are not context save/restored; so even if we set these
bits always in kernel they are going to change once the context is switched out.
We can consider making them non-privileged by default but these registers also
contain other chicken bits which should not be allowed to be modified.

In the later revisions controlling bits are save/restored at context level but
in the existing revisions these are exported via other debug registers and should
be on the whitelist. This patch adds changes to provide HW with a list of registers
to be whitelisted. HW checks this list during execution and provides access accordingly.

HW imposes a limit on the number of registers on whitelist and it is per-engine.
At this point we are only enabling whitelist for RCS and we don't foresee any
requirement for other engines.

The registers to be whitelisted are added using generic workaround list mechanism,
even these are only enablers for userspace workarounds. But by sharing this
mechanism we get some test assets without additional cost (Mika).

v2: rebase

v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
 drivers/gpu/drm/i915/i915_reg.h         |  3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 104bd18..660afe1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1653,11 +1653,18 @@ struct i915_wa_reg {
 	u32 mask;
 };
 
-#define I915_MAX_WA_REGS 16
+/*
+ * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
+ * allowing it for RCS as we don't foresee any requirement of having
+ * a whitelist for other engines. When it is really required for
+ * other engines then the limit need to be increased.
+ */
+#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
 
 struct i915_workarounds {
 	struct i915_wa_reg reg[I915_MAX_WA_REGS];
 	u32 count;
+	u32 hw_whitelist_index[I915_NUM_RINGS];
 };
 
 struct i915_virtual_gpu {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0a98889..7938814 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
 #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
 #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
 
+#define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base)+0x4D0) + (i)*4)
+#define   RING_MAX_NONPRIV_SLOTS  12
+
 #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
 
 #if 0
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4060acf..279dd51 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
 
 #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
 
+static int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
+				 i915_reg_t reg_addr)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct i915_workarounds *wa = &dev_priv->workarounds;
+	const uint32_t index = wa->hw_whitelist_index[ring->id];
+
+	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
+		return -EINVAL;
+
+	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg);
+	wa->hw_whitelist_index[ring->id]++;
+
+	return 0;
+}
+
 static int gen8_init_workarounds(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
@@ -1115,6 +1131,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
 	WARN_ON(ring->id != RCS);
 
 	dev_priv->workarounds.count = 0;
+	dev_priv->workarounds.hw_whitelist_index[ring->id] = 0;
 
 	if (IS_BROADWELL(dev))
 		return bdw_init_workarounds(ring);
-- 
1.9.1

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

Re: [PATCH v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Chris Wilson]

On Wed, Jan 13, 2016 at 12:28:17PM +0000, Arun Siluvery wrote:
> Some of the HW registers are privileged and cannot be written to from a
> non-privileged batch buffers coming from userspace unless they are on whitelist.
> Userspace need write access to them to implement preemption related WA.

You need to be clear that this is the hw whitelist and not our sw
whitelist.
 
> The reason for using this approach is, the register bits that control preemption
> granularity at the HW level are not context save/restored; so even if we set these
> bits always in kernel they are going to change once the context is switched out.
> We can consider making them non-privileged by default but these registers also
> contain other chicken bits which should not be allowed to be modified.
> 
> In the later revisions controlling bits are save/restored at context level but
> in the existing revisions these are exported via other debug registers and should
> be on the whitelist. This patch adds changes to provide HW with a list of registers
> to be whitelisted. HW checks this list during execution and provides access accordingly.
> 
> HW imposes a limit on the number of registers on whitelist and it is per-engine.
> At this point we are only enabling whitelist for RCS and we don't foresee any
> requirement for other engines.
> 
> The registers to be whitelisted are added using generic workaround list mechanism,
> even these are only enablers for userspace workarounds. But by sharing this
> mechanism we get some test assets without additional cost (Mika).
> 
> v2: rebase
> 
> v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
> i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 104bd18..660afe1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1653,11 +1653,18 @@ struct i915_wa_reg {
>  	u32 mask;
>  };
>  
> -#define I915_MAX_WA_REGS 16
> +/*
> + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
> + * allowing it for RCS as we don't foresee any requirement of having
> + * a whitelist for other engines. When it is really required for
> + * other engines then the limit need to be increased.
> + */
> +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
>  
>  struct i915_workarounds {
>  	struct i915_wa_reg reg[I915_MAX_WA_REGS];
>  	u32 count;
> +	u32 hw_whitelist_index[I915_NUM_RINGS];

This is a counter only to be used whilst constructing the reg list. It
also implies that this list of wa_reg is now engine specific. Hmm, looks
like it always has been, just been relying on RCS running first.

I would fix that up first.
-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 v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

On 13/01/2016 13:03, Chris Wilson wrote:
> On Wed, Jan 13, 2016 at 12:28:17PM +0000, Arun Siluvery wrote:
>> Some of the HW registers are privileged and cannot be written to from a
>> non-privileged batch buffers coming from userspace unless they are on whitelist.
>> Userspace need write access to them to implement preemption related WA.
>
> You need to be clear that this is the hw whitelist and not our sw
> whitelist.
>
ok, I will update the commit msg to clarify this.

>> The reason for using this approach is, the register bits that control preemption
>> granularity at the HW level are not context save/restored; so even if we set these
>> bits always in kernel they are going to change once the context is switched out.
>> We can consider making them non-privileged by default but these registers also
>> contain other chicken bits which should not be allowed to be modified.
>>
>> In the later revisions controlling bits are save/restored at context level but
>> in the existing revisions these are exported via other debug registers and should
>> be on the whitelist. This patch adds changes to provide HW with a list of registers
>> to be whitelisted. HW checks this list during execution and provides access accordingly.
>>
>> HW imposes a limit on the number of registers on whitelist and it is per-engine.
>> At this point we are only enabling whitelist for RCS and we don't foresee any
>> requirement for other engines.
>>
>> The registers to be whitelisted are added using generic workaround list mechanism,
>> even these are only enablers for userspace workarounds. But by sharing this
>> mechanism we get some test assets without additional cost (Mika).
>>
>> v2: rebase
>>
>> v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
>> i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
>>
>> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
>>   drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 104bd18..660afe1 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1653,11 +1653,18 @@ struct i915_wa_reg {
>>   	u32 mask;
>>   };
>>
>> -#define I915_MAX_WA_REGS 16
>> +/*
>> + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
>> + * allowing it for RCS as we don't foresee any requirement of having
>> + * a whitelist for other engines. When it is really required for
>> + * other engines then the limit need to be increased.
>> + */
>> +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
>>
>>   struct i915_workarounds {
>>   	struct i915_wa_reg reg[I915_MAX_WA_REGS];
>>   	u32 count;
>> +	u32 hw_whitelist_index[I915_NUM_RINGS];
>
> This is a counter only to be used whilst constructing the reg list. It
> also implies that this list of wa_reg is now engine specific. Hmm, looks
> like it always has been, just been relying on RCS running first.
>
> I would fix that up first.
We are initializing all WA only for RCS, similarly HW whitelist also 
which is engine specific.

HW whitelist is initialized along with WA so essentially we are doing it 
only for RCS. Afaik there is no requirement to have it for other 
engines, we can actually limit hw_whitelist_index to RCS. I only allowed 
for all rings because spec defined it for other engines and also we 
don't have to change this when required. It is not clear what needs 
fixing up.

regards
Arun


> -Chris
>

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

Re: [PATCH v2 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Chris Wilson]

On Wed, Jan 13, 2016 at 01:41:47PM +0000, Arun Siluvery wrote:
> On 13/01/2016 13:03, Chris Wilson wrote:
> >On Wed, Jan 13, 2016 at 12:28:17PM +0000, Arun Siluvery wrote:
> >>Some of the HW registers are privileged and cannot be written to from a
> >>non-privileged batch buffers coming from userspace unless they are on whitelist.
> >>Userspace need write access to them to implement preemption related WA.
> >
> >You need to be clear that this is the hw whitelist and not our sw
> >whitelist.
> >
> ok, I will update the commit msg to clarify this.
> 
> >>The reason for using this approach is, the register bits that control preemption
> >>granularity at the HW level are not context save/restored; so even if we set these
> >>bits always in kernel they are going to change once the context is switched out.
> >>We can consider making them non-privileged by default but these registers also
> >>contain other chicken bits which should not be allowed to be modified.
> >>
> >>In the later revisions controlling bits are save/restored at context level but
> >>in the existing revisions these are exported via other debug registers and should
> >>be on the whitelist. This patch adds changes to provide HW with a list of registers
> >>to be whitelisted. HW checks this list during execution and provides access accordingly.
> >>
> >>HW imposes a limit on the number of registers on whitelist and it is per-engine.
> >>At this point we are only enabling whitelist for RCS and we don't foresee any
> >>requirement for other engines.
> >>
> >>The registers to be whitelisted are added using generic workaround list mechanism,
> >>even these are only enablers for userspace workarounds. But by sharing this
> >>mechanism we get some test assets without additional cost (Mika).
> >>
> >>v2: rebase
> >>
> >>v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
> >>i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
> >>
> >>Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >>Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >>Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
> >>  drivers/gpu/drm/i915/i915_reg.h         |  3 +++
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
> >>  3 files changed, 28 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 104bd18..660afe1 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -1653,11 +1653,18 @@ struct i915_wa_reg {
> >>  	u32 mask;
> >>  };
> >>
> >>-#define I915_MAX_WA_REGS 16
> >>+/*
> >>+ * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
> >>+ * allowing it for RCS as we don't foresee any requirement of having
> >>+ * a whitelist for other engines. When it is really required for
> >>+ * other engines then the limit need to be increased.
> >>+ */
> >>+#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
> >>
> >>  struct i915_workarounds {
> >>  	struct i915_wa_reg reg[I915_MAX_WA_REGS];
> >>  	u32 count;
> >>+	u32 hw_whitelist_index[I915_NUM_RINGS];
> >
> >This is a counter only to be used whilst constructing the reg list. It
> >also implies that this list of wa_reg is now engine specific. Hmm, looks
> >like it always has been, just been relying on RCS running first.
> >
> >I would fix that up first.
> We are initializing all WA only for RCS, similarly HW whitelist also
> which is engine specific.
> 
> HW whitelist is initialized along with WA so essentially we are
> doing it only for RCS. Afaik there is no requirement to have it for
> other engines, we can actually limit hw_whitelist_index to RCS. I
> only allowed for all rings because spec defined it for other engines
> and also we don't have to change this when required. It is not clear
> what needs fixing up.

You can at least remove the appearance of it doing performing cross-engine
setup. That mistake has already been observed by users where we have
used RCS to initialise the other rings, and this patch looks like it
could be doing exactly the same - or enabling others to make such a
mistake.
-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

[PATCH v3 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

Some of the HW registers are privileged and cannot be written to from
non-privileged batch buffers coming from userspace unless they are added to
the HW whitelist. This whitelist is maintained by HW and it is different from
SW whitelist. Userspace need write access to them to implement preemption
related WA.

The reason for using this approach is, the register bits that control
preemption granularity at the HW level are not context save/restored; so even
if we set these bits always in kernel they are going to change once the
context is switched out.  We can consider making them non-privileged by
default but these registers also contain other chicken bits which should not
be allowed to be modified.

In the later revisions controlling bits are save/restored at context level but
in the existing revisions these are exported via other debug registers and
should be on the whitelist. This patch adds changes to provide HW with a list
of registers to be whitelisted. HW checks this list during execution and
provides access accordingly.

HW imposes a limit on the number of registers on whitelist and it is
per-engine.  At this point we are only enabling whitelist for RCS and we don't
foresee any requirement for other engines.

The registers to be whitelisted are added using generic workaround list
mechanism, even these are only enablers for userspace workarounds. But by
sharing this mechanism we get some test assets without additional cost (Mika).

v2: rebase

v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).

v4: Clarify that this is HW whitelist and different from the one maintained in
driver. This list is engine specific but it gets initialized along with other
WA which is RCS specific thing, so make it clear that we are not doing any
cross engine setup during initialization (Chris).

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
 drivers/gpu/drm/i915/i915_reg.h         |  3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 104bd18..83fccc0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1653,11 +1653,18 @@ struct i915_wa_reg {
 	u32 mask;
 };
 
-#define I915_MAX_WA_REGS 16
+/*
+ * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
+ * allowing it for RCS as we don't foresee any requirement of having
+ * a whitelist for other engines. When it is really required for
+ * other engines then the limit need to be increased.
+ */
+#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
 
 struct i915_workarounds {
 	struct i915_wa_reg reg[I915_MAX_WA_REGS];
 	u32 count;
+	u32 hw_whitelist_count[I915_NUM_RINGS];
 };
 
 struct i915_virtual_gpu {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0a98889..7938814 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
 #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
 #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
 
+#define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base)+0x4D0) + (i)*4)
+#define   RING_MAX_NONPRIV_SLOTS  12
+
 #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
 
 #if 0
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4060acf..4fe3240 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
 
 #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
 
+static int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
+				 i915_reg_t reg_addr)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct i915_workarounds *wa = &dev_priv->workarounds;
+	const uint32_t index = wa->hw_whitelist_count[ring->id];
+
+	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
+		return -EINVAL;
+
+	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg);
+	wa->hw_whitelist_count[ring->id]++;
+
+	return 0;
+}
+
 static int gen8_init_workarounds(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
@@ -1115,6 +1131,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
 	WARN_ON(ring->id != RCS);
 
 	dev_priv->workarounds.count = 0;
+	dev_priv->workarounds.hw_whitelist_count[RCS] = 0;
 
 	if (IS_BROADWELL(dev))
 		return bdw_init_workarounds(ring);
-- 
1.9.1

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

Re: [PATCH v3 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Chris Wilson]

On Wed, Jan 13, 2016 at 03:38:15PM +0000, Arun Siluvery wrote:
> Some of the HW registers are privileged and cannot be written to from
> non-privileged batch buffers coming from userspace unless they are added to
> the HW whitelist. This whitelist is maintained by HW and it is different from
> SW whitelist. Userspace need write access to them to implement preemption
> related WA.
> 
> The reason for using this approach is, the register bits that control
> preemption granularity at the HW level are not context save/restored; so even
> if we set these bits always in kernel they are going to change once the
> context is switched out.  We can consider making them non-privileged by
> default but these registers also contain other chicken bits which should not
> be allowed to be modified.
> 
> In the later revisions controlling bits are save/restored at context level but
> in the existing revisions these are exported via other debug registers and
> should be on the whitelist. This patch adds changes to provide HW with a list
> of registers to be whitelisted. HW checks this list during execution and
> provides access accordingly.
> 
> HW imposes a limit on the number of registers on whitelist and it is
> per-engine.  At this point we are only enabling whitelist for RCS and we don't
> foresee any requirement for other engines.
> 
> The registers to be whitelisted are added using generic workaround list
> mechanism, even these are only enablers for userspace workarounds. But by
> sharing this mechanism we get some test assets without additional cost (Mika).
> 
> v2: rebase
> 
> v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
> i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
> 
> v4: Clarify that this is HW whitelist and different from the one maintained in
> driver. This list is engine specific but it gets initialized along with other
> WA which is RCS specific thing, so make it clear that we are not doing any
> cross engine setup during initialization (Chris).

Those name work much better for me, so thanks for clearing them up and
allaying my fears.

Would it not also make sense to expose hw_whitelist_count[] in
i915_wa_registers (debugfs)?

> +static int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
> +				 i915_reg_t reg_addr)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct i915_workarounds *wa = &dev_priv->workarounds;
> +	const uint32_t index = wa->hw_whitelist_count[ring->id];
> +
> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> +		return -EINVAL;
> +
> +	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg);

WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
         i915_mmio_reg_offset(reg_addr));

And just call it reg. (reg_addr would imply that you applied the mmio
offset, i.e were about to call ioread32(reg_addr)).
-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 v3 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

On 13/01/2016 19:01, Chris Wilson wrote:
> On Wed, Jan 13, 2016 at 03:38:15PM +0000, Arun Siluvery wrote:
>> Some of the HW registers are privileged and cannot be written to from
>> non-privileged batch buffers coming from userspace unless they are added to
>> the HW whitelist. This whitelist is maintained by HW and it is different from
>> SW whitelist. Userspace need write access to them to implement preemption
>> related WA.
>>
>> The reason for using this approach is, the register bits that control
>> preemption granularity at the HW level are not context save/restored; so even
>> if we set these bits always in kernel they are going to change once the
>> context is switched out.  We can consider making them non-privileged by
>> default but these registers also contain other chicken bits which should not
>> be allowed to be modified.
>>
>> In the later revisions controlling bits are save/restored at context level but
>> in the existing revisions these are exported via other debug registers and
>> should be on the whitelist. This patch adds changes to provide HW with a list
>> of registers to be whitelisted. HW checks this list during execution and
>> provides access accordingly.
>>
>> HW imposes a limit on the number of registers on whitelist and it is
>> per-engine.  At this point we are only enabling whitelist for RCS and we don't
>> foresee any requirement for other engines.
>>
>> The registers to be whitelisted are added using generic workaround list
>> mechanism, even these are only enablers for userspace workarounds. But by
>> sharing this mechanism we get some test assets without additional cost (Mika).
>>
>> v2: rebase
>>
>> v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
>> i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
>>
>> v4: Clarify that this is HW whitelist and different from the one maintained in
>> driver. This list is engine specific but it gets initialized along with other
>> WA which is RCS specific thing, so make it clear that we are not doing any
>> cross engine setup during initialization (Chris).
>
> Those name work much better for me, so thanks for clearing them up and
> allaying my fears.
>
> Would it not also make sense to expose hw_whitelist_count[] in
> i915_wa_registers (debugfs)?
>
It is already is part of i915_wa_registers, each HW whitelist entry is 
just another entry in i915_workarounds. Mika suggested to add this using 
workaround list mechanism so that we get this without additional cost.

>> +static int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
>> +				 i915_reg_t reg_addr)
>> +{
>> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>> +	struct i915_workarounds *wa = &dev_priv->workarounds;
>> +	const uint32_t index = wa->hw_whitelist_count[ring->id];
>> +
>> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
>> +		return -EINVAL;
>> +
>> +	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg);
>
> WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
>           i915_mmio_reg_offset(reg_addr));
>
> And just call it reg. (reg_addr would imply that you applied the mmio
> offset, i.e were about to call ioread32(reg_addr)).
the thought of using i915_mmio_reg_offset() came to me after sending v3 
but thought no one would notice :)
Do you want me to change this and send again?

-       WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), 
reg_addr.reg);
+       WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
+                i915_mmio_reg_offset(reg));

regards
Arun


> -Chris
>

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

Re: [PATCH v3 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Chris Wilson]

On Wed, Jan 13, 2016 at 07:14:56PM +0000, Arun Siluvery wrote:
> On 13/01/2016 19:01, Chris Wilson wrote:
> >On Wed, Jan 13, 2016 at 03:38:15PM +0000, Arun Siluvery wrote:
> >>Some of the HW registers are privileged and cannot be written to from
> >>non-privileged batch buffers coming from userspace unless they are added to
> >>the HW whitelist. This whitelist is maintained by HW and it is different from
> >>SW whitelist. Userspace need write access to them to implement preemption
> >>related WA.
> >>
> >>The reason for using this approach is, the register bits that control
> >>preemption granularity at the HW level are not context save/restored; so even
> >>if we set these bits always in kernel they are going to change once the
> >>context is switched out.  We can consider making them non-privileged by
> >>default but these registers also contain other chicken bits which should not
> >>be allowed to be modified.
> >>
> >>In the later revisions controlling bits are save/restored at context level but
> >>in the existing revisions these are exported via other debug registers and
> >>should be on the whitelist. This patch adds changes to provide HW with a list
> >>of registers to be whitelisted. HW checks this list during execution and
> >>provides access accordingly.
> >>
> >>HW imposes a limit on the number of registers on whitelist and it is
> >>per-engine.  At this point we are only enabling whitelist for RCS and we don't
> >>foresee any requirement for other engines.
> >>
> >>The registers to be whitelisted are added using generic workaround list
> >>mechanism, even these are only enablers for userspace workarounds. But by
> >>sharing this mechanism we get some test assets without additional cost (Mika).
> >>
> >>v2: rebase
> >>
> >>v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
> >>i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
> >>
> >>v4: Clarify that this is HW whitelist and different from the one maintained in
> >>driver. This list is engine specific but it gets initialized along with other
> >>WA which is RCS specific thing, so make it clear that we are not doing any
> >>cross engine setup during initialization (Chris).
> >
> >Those name work much better for me, so thanks for clearing them up and
> >allaying my fears.
> >
> >Would it not also make sense to expose hw_whitelist_count[] in
> >i915_wa_registers (debugfs)?
> >
> It is already is part of i915_wa_registers, each HW whitelist entry
> is just another entry in i915_workarounds. Mika suggested to add
> this using workaround list mechanism so that we get this without
> additional cost.

I just mean that is much easier to sanity check kernel internals if we
print them out in debugfs as well. Yes, you can inspect the value of
each register and determine which are whitelist enabling - but wouldn't
it also be good to check that you have the same count as the kernel. For
mere mortals, we can just look at the count and be happy that
whitelisting is taking effect.
 
> >>+static int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
> >>+				 i915_reg_t reg_addr)
> >>+{
> >>+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >>+	struct i915_workarounds *wa = &dev_priv->workarounds;
> >>+	const uint32_t index = wa->hw_whitelist_count[ring->id];
> >>+
> >>+	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> >>+		return -EINVAL;
> >>+
> >>+	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg);
> >
> >WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
> >          i915_mmio_reg_offset(reg_addr));
> >
> >And just call it reg. (reg_addr would imply that you applied the mmio
> >offset, i.e were about to call ioread32(reg_addr)).
> the thought of using i915_mmio_reg_offset() came to me after sending
> v3 but thought no one would notice :)
> Do you want me to change this and send again?

If you have a moment, do so with my
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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

[PATCH v4 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

Some of the HW registers are privileged and cannot be written to from
non-privileged batch buffers coming from userspace unless they are added to
the HW whitelist. This whitelist is maintained by HW and it is different from
SW whitelist. Userspace need write access to them to implement preemption
related WA.

The reason for using this approach is, the register bits that control
preemption granularity at the HW level are not context save/restored; so even
if we set these bits always in kernel they are going to change once the
context is switched out.  We can consider making them non-privileged by
default but these registers also contain other chicken bits which should not
be allowed to be modified.

In the later revisions controlling bits are save/restored at context level but
in the existing revisions these are exported via other debug registers and
should be on the whitelist. This patch adds changes to provide HW with a list
of registers to be whitelisted. HW checks this list during execution and
provides access accordingly.

HW imposes a limit on the number of registers on whitelist and it is
per-engine.  At this point we are only enabling whitelist for RCS and we don't
foresee any requirement for other engines.

The registers to be whitelisted are added using generic workaround list
mechanism, even these are only enablers for userspace workarounds. But by
sharing this mechanism we get some test assets without additional cost (Mika).

v2: rebase

v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).

v4: improvements suggested by Chris Wilson.
Clarify that this is HW whitelist and different from the one maintained in
driver. This list is engine specific but it gets initialized along with other
WA which is RCS specific thing, so make it clear that we are not doing any
cross engine setup during initialization.
Make HW whitelist count of each engine available in debugfs.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 15 ++++++++++-----
 drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
 drivers/gpu/drm/i915/i915_reg.h         |  3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
 4 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e3377ab..7eb002c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3229,9 +3229,11 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
 {
 	int i;
 	int ret;
+	struct intel_engine_cs *ring;
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_workarounds *workarounds = &dev_priv->workarounds;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
@@ -3239,15 +3241,18 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
 
 	intel_runtime_pm_get(dev_priv);
 
-	seq_printf(m, "Workarounds applied: %d\n", dev_priv->workarounds.count);
-	for (i = 0; i < dev_priv->workarounds.count; ++i) {
+	seq_printf(m, "Workarounds applied: %d\n", workarounds->count);
+	for_each_ring(ring, dev_priv, i)
+		seq_printf(m, "HW whitelist count for %s: %d\n",
+			   ring->name, workarounds->hw_whitelist_count[i]);
+	for (i = 0; i < workarounds->count; ++i) {
 		i915_reg_t addr;
 		u32 mask, value, read;
 		bool ok;
 
-		addr = dev_priv->workarounds.reg[i].addr;
-		mask = dev_priv->workarounds.reg[i].mask;
-		value = dev_priv->workarounds.reg[i].value;
+		addr = workarounds->reg[i].addr;
+		mask = workarounds->reg[i].mask;
+		value = workarounds->reg[i].value;
 		read = I915_READ(addr);
 		ok = (value & mask) == (read & mask);
 		seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X, read: 0x%08x, status: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 104bd18..83fccc0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1653,11 +1653,18 @@ struct i915_wa_reg {
 	u32 mask;
 };
 
-#define I915_MAX_WA_REGS 16
+/*
+ * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
+ * allowing it for RCS as we don't foresee any requirement of having
+ * a whitelist for other engines. When it is really required for
+ * other engines then the limit need to be increased.
+ */
+#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
 
 struct i915_workarounds {
 	struct i915_wa_reg reg[I915_MAX_WA_REGS];
 	u32 count;
+	u32 hw_whitelist_count[I915_NUM_RINGS];
 };
 
 struct i915_virtual_gpu {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0a98889..7938814 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
 #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
 #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
 
+#define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base)+0x4D0) + (i)*4)
+#define   RING_MAX_NONPRIV_SLOTS  12
+
 #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
 
 #if 0
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4060acf..56af736 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
 
 #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
 
+static int wa_ring_whitelist_reg(struct intel_engine_cs *ring, i915_reg_t reg)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct i915_workarounds *wa = &dev_priv->workarounds;
+	const uint32_t index = wa->hw_whitelist_count[ring->id];
+
+	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
+		return -EINVAL;
+
+	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
+		 i915_mmio_reg_offset(reg));
+	wa->hw_whitelist_count[ring->id]++;
+
+	return 0;
+}
+
 static int gen8_init_workarounds(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
@@ -1115,6 +1131,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
 	WARN_ON(ring->id != RCS);
 
 	dev_priv->workarounds.count = 0;
+	dev_priv->workarounds.hw_whitelist_count[RCS] = 0;
 
 	if (IS_BROADWELL(dev))
 		return bdw_init_workarounds(ring);
-- 
1.9.1

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

Re: [PATCH v4 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Daniel Vetter]

On Thu, Jan 14, 2016 at 03:27:35PM +0000, Arun Siluvery wrote:
> Some of the HW registers are privileged and cannot be written to from
> non-privileged batch buffers coming from userspace unless they are added to
> the HW whitelist. This whitelist is maintained by HW and it is different from
> SW whitelist. Userspace need write access to them to implement preemption
> related WA.
> 
> The reason for using this approach is, the register bits that control
> preemption granularity at the HW level are not context save/restored; so even
> if we set these bits always in kernel they are going to change once the
> context is switched out.  We can consider making them non-privileged by
> default but these registers also contain other chicken bits which should not
> be allowed to be modified.
> 
> In the later revisions controlling bits are save/restored at context level but
> in the existing revisions these are exported via other debug registers and
> should be on the whitelist. This patch adds changes to provide HW with a list
> of registers to be whitelisted. HW checks this list during execution and
> provides access accordingly.
> 
> HW imposes a limit on the number of registers on whitelist and it is
> per-engine.  At this point we are only enabling whitelist for RCS and we don't
> foresee any requirement for other engines.
> 
> The registers to be whitelisted are added using generic workaround list
> mechanism, even these are only enablers for userspace workarounds. But by
> sharing this mechanism we get some test assets without additional cost (Mika).
> 
> v2: rebase
> 
> v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
> i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
> 
> v4: improvements suggested by Chris Wilson.
> Clarify that this is HW whitelist and different from the one maintained in
> driver. This list is engine specific but it gets initialized along with other
> WA which is RCS specific thing, so make it clear that we are not doing any
> cross engine setup during initialization.
> Make HW whitelist count of each engine available in debugfs.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>

If you resend just single patches to a series you must --in-reply-to the
individual patch, not the cover letter. Otherwise patchwork won't pick it
up, which means we don't have CI results for this.

Since it's been a while probably best to just resend the entire pile.

Also we seem to be missing r-b tags for the actual w/a changes.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 15 ++++++++++-----
>  drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
>  4 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e3377ab..7eb002c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3229,9 +3229,11 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>  {
>  	int i;
>  	int ret;
> +	struct intel_engine_cs *ring;
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_workarounds *workarounds = &dev_priv->workarounds;
>  
>  	ret = mutex_lock_interruptible(&dev->struct_mutex);
>  	if (ret)
> @@ -3239,15 +3241,18 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>  
>  	intel_runtime_pm_get(dev_priv);
>  
> -	seq_printf(m, "Workarounds applied: %d\n", dev_priv->workarounds.count);
> -	for (i = 0; i < dev_priv->workarounds.count; ++i) {
> +	seq_printf(m, "Workarounds applied: %d\n", workarounds->count);
> +	for_each_ring(ring, dev_priv, i)
> +		seq_printf(m, "HW whitelist count for %s: %d\n",
> +			   ring->name, workarounds->hw_whitelist_count[i]);
> +	for (i = 0; i < workarounds->count; ++i) {
>  		i915_reg_t addr;
>  		u32 mask, value, read;
>  		bool ok;
>  
> -		addr = dev_priv->workarounds.reg[i].addr;
> -		mask = dev_priv->workarounds.reg[i].mask;
> -		value = dev_priv->workarounds.reg[i].value;
> +		addr = workarounds->reg[i].addr;
> +		mask = workarounds->reg[i].mask;
> +		value = workarounds->reg[i].value;
>  		read = I915_READ(addr);
>  		ok = (value & mask) == (read & mask);
>  		seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X, read: 0x%08x, status: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 104bd18..83fccc0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1653,11 +1653,18 @@ struct i915_wa_reg {
>  	u32 mask;
>  };
>  
> -#define I915_MAX_WA_REGS 16
> +/*
> + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
> + * allowing it for RCS as we don't foresee any requirement of having
> + * a whitelist for other engines. When it is really required for
> + * other engines then the limit need to be increased.
> + */
> +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
>  
>  struct i915_workarounds {
>  	struct i915_wa_reg reg[I915_MAX_WA_REGS];
>  	u32 count;
> +	u32 hw_whitelist_count[I915_NUM_RINGS];
>  };
>  
>  struct i915_virtual_gpu {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0a98889..7938814 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
>  #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
>  #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
>  
> +#define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base)+0x4D0) + (i)*4)
> +#define   RING_MAX_NONPRIV_SLOTS  12
> +
>  #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
>  
>  #if 0
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4060acf..56af736 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
>  
>  #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
>  
> +static int wa_ring_whitelist_reg(struct intel_engine_cs *ring, i915_reg_t reg)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct i915_workarounds *wa = &dev_priv->workarounds;
> +	const uint32_t index = wa->hw_whitelist_count[ring->id];
> +
> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> +		return -EINVAL;
> +
> +	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
> +		 i915_mmio_reg_offset(reg));
> +	wa->hw_whitelist_count[ring->id]++;
> +
> +	return 0;
> +}
> +
>  static int gen8_init_workarounds(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
> @@ -1115,6 +1131,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
>  	WARN_ON(ring->id != RCS);
>  
>  	dev_priv->workarounds.count = 0;
> +	dev_priv->workarounds.hw_whitelist_count[RCS] = 0;
>  
>  	if (IS_BROADWELL(dev))
>  		return bdw_init_workarounds(ring);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Arun Siluvery]

On 19/01/2016 09:00, Daniel Vetter wrote:
> On Thu, Jan 14, 2016 at 03:27:35PM +0000, Arun Siluvery wrote:
>> Some of the HW registers are privileged and cannot be written to from
>> non-privileged batch buffers coming from userspace unless they are added to
>> the HW whitelist. This whitelist is maintained by HW and it is different from
>> SW whitelist. Userspace need write access to them to implement preemption
>> related WA.
>>
>> The reason for using this approach is, the register bits that control
>> preemption granularity at the HW level are not context save/restored; so even
>> if we set these bits always in kernel they are going to change once the
>> context is switched out.  We can consider making them non-privileged by
>> default but these registers also contain other chicken bits which should not
>> be allowed to be modified.
>>
>> In the later revisions controlling bits are save/restored at context level but
>> in the existing revisions these are exported via other debug registers and
>> should be on the whitelist. This patch adds changes to provide HW with a list
>> of registers to be whitelisted. HW checks this list during execution and
>> provides access accordingly.
>>
>> HW imposes a limit on the number of registers on whitelist and it is
>> per-engine.  At this point we are only enabling whitelist for RCS and we don't
>> foresee any requirement for other engines.
>>
>> The registers to be whitelisted are added using generic workaround list
>> mechanism, even these are only enablers for userspace workarounds. But by
>> sharing this mechanism we get some test assets without additional cost (Mika).
>>
>> v2: rebase
>>
>> v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
>> i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
>>
>> v4: improvements suggested by Chris Wilson.
>> Clarify that this is HW whitelist and different from the one maintained in
>> driver. This list is engine specific but it gets initialized along with other
>> WA which is RCS specific thing, so make it clear that we are not doing any
>> cross engine setup during initialization.
>> Make HW whitelist count of each engine available in debugfs.
>>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>
> If you resend just single patches to a series you must --in-reply-to the
> individual patch, not the cover letter. Otherwise patchwork won't pick it
> up, which means we don't have CI results for this.

Hi Daniel,

Yes I did use --in-reply-to but probably not the correct message-id, 
will keep this in mind.

>
> Since it's been a while probably best to just resend the entire pile.
>
> Also we seem to be missing r-b tags for the actual w/a changes.
yes, actual w/a are yet to be reviewed, I can resend all of them once 
they are reviewed or you want me to send it now?

regards
Arun

> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c     | 15 ++++++++++-----
>>   drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
>>   drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
>>   4 files changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index e3377ab..7eb002c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3229,9 +3229,11 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>>   {
>>   	int i;
>>   	int ret;
>> +	struct intel_engine_cs *ring;
>>   	struct drm_info_node *node = (struct drm_info_node *) m->private;
>>   	struct drm_device *dev = node->minor->dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct i915_workarounds *workarounds = &dev_priv->workarounds;
>>
>>   	ret = mutex_lock_interruptible(&dev->struct_mutex);
>>   	if (ret)
>> @@ -3239,15 +3241,18 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>>
>>   	intel_runtime_pm_get(dev_priv);
>>
>> -	seq_printf(m, "Workarounds applied: %d\n", dev_priv->workarounds.count);
>> -	for (i = 0; i < dev_priv->workarounds.count; ++i) {
>> +	seq_printf(m, "Workarounds applied: %d\n", workarounds->count);
>> +	for_each_ring(ring, dev_priv, i)
>> +		seq_printf(m, "HW whitelist count for %s: %d\n",
>> +			   ring->name, workarounds->hw_whitelist_count[i]);
>> +	for (i = 0; i < workarounds->count; ++i) {
>>   		i915_reg_t addr;
>>   		u32 mask, value, read;
>>   		bool ok;
>>
>> -		addr = dev_priv->workarounds.reg[i].addr;
>> -		mask = dev_priv->workarounds.reg[i].mask;
>> -		value = dev_priv->workarounds.reg[i].value;
>> +		addr = workarounds->reg[i].addr;
>> +		mask = workarounds->reg[i].mask;
>> +		value = workarounds->reg[i].value;
>>   		read = I915_READ(addr);
>>   		ok = (value & mask) == (read & mask);
>>   		seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X, read: 0x%08x, status: %s\n",
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 104bd18..83fccc0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1653,11 +1653,18 @@ struct i915_wa_reg {
>>   	u32 mask;
>>   };
>>
>> -#define I915_MAX_WA_REGS 16
>> +/*
>> + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
>> + * allowing it for RCS as we don't foresee any requirement of having
>> + * a whitelist for other engines. When it is really required for
>> + * other engines then the limit need to be increased.
>> + */
>> +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
>>
>>   struct i915_workarounds {
>>   	struct i915_wa_reg reg[I915_MAX_WA_REGS];
>>   	u32 count;
>> +	u32 hw_whitelist_count[I915_NUM_RINGS];
>>   };
>>
>>   struct i915_virtual_gpu {
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 0a98889..7938814 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
>>   #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
>>   #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
>>
>> +#define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base)+0x4D0) + (i)*4)
>> +#define   RING_MAX_NONPRIV_SLOTS  12
>> +
>>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
>>
>>   #if 0
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 4060acf..56af736 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
>>
>>   #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
>>
>> +static int wa_ring_whitelist_reg(struct intel_engine_cs *ring, i915_reg_t reg)
>> +{
>> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>> +	struct i915_workarounds *wa = &dev_priv->workarounds;
>> +	const uint32_t index = wa->hw_whitelist_count[ring->id];
>> +
>> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
>> +		return -EINVAL;
>> +
>> +	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
>> +		 i915_mmio_reg_offset(reg));
>> +	wa->hw_whitelist_count[ring->id]++;
>> +
>> +	return 0;
>> +}
>> +
>>   static int gen8_init_workarounds(struct intel_engine_cs *ring)
>>   {
>>   	struct drm_device *dev = ring->dev;
>> @@ -1115,6 +1131,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
>>   	WARN_ON(ring->id != RCS);
>>
>>   	dev_priv->workarounds.count = 0;
>> +	dev_priv->workarounds.hw_whitelist_count[RCS] = 0;
>>
>>   	if (IS_BROADWELL(dev))
>>   		return bdw_init_workarounds(ring);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH v4 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Daniel Vetter]

On Tue, Jan 19, 2016 at 10:16:52AM +0000, Arun Siluvery wrote:
> On 19/01/2016 09:00, Daniel Vetter wrote:
> >On Thu, Jan 14, 2016 at 03:27:35PM +0000, Arun Siluvery wrote:
> >>Some of the HW registers are privileged and cannot be written to from
> >>non-privileged batch buffers coming from userspace unless they are added to
> >>the HW whitelist. This whitelist is maintained by HW and it is different from
> >>SW whitelist. Userspace need write access to them to implement preemption
> >>related WA.
> >>
> >>The reason for using this approach is, the register bits that control
> >>preemption granularity at the HW level are not context save/restored; so even
> >>if we set these bits always in kernel they are going to change once the
> >>context is switched out.  We can consider making them non-privileged by
> >>default but these registers also contain other chicken bits which should not
> >>be allowed to be modified.
> >>
> >>In the later revisions controlling bits are save/restored at context level but
> >>in the existing revisions these are exported via other debug registers and
> >>should be on the whitelist. This patch adds changes to provide HW with a list
> >>of registers to be whitelisted. HW checks this list during execution and
> >>provides access accordingly.
> >>
> >>HW imposes a limit on the number of registers on whitelist and it is
> >>per-engine.  At this point we are only enabling whitelist for RCS and we don't
> >>foresee any requirement for other engines.
> >>
> >>The registers to be whitelisted are added using generic workaround list
> >>mechanism, even these are only enablers for userspace workarounds. But by
> >>sharing this mechanism we get some test assets without additional cost (Mika).
> >>
> >>v2: rebase
> >>
> >>v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
> >>i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
> >>
> >>v4: improvements suggested by Chris Wilson.
> >>Clarify that this is HW whitelist and different from the one maintained in
> >>driver. This list is engine specific but it gets initialized along with other
> >>WA which is RCS specific thing, so make it clear that we are not doing any
> >>cross engine setup during initialization.
> >>Make HW whitelist count of each engine available in debugfs.
> >>
> >>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >>Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >>Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >
> >If you resend just single patches to a series you must --in-reply-to the
> >individual patch, not the cover letter. Otherwise patchwork won't pick it
> >up, which means we don't have CI results for this.
> 
> Hi Daniel,
> 
> Yes I did use --in-reply-to but probably not the correct message-id, will
> keep this in mind.
> 
> >
> >Since it's been a while probably best to just resend the entire pile.
> >
> >Also we seem to be missing r-b tags for the actual w/a changes.
> yes, actual w/a are yet to be reviewed, I can resend all of them once they
> are reviewed or you want me to send it now?

Either way is fine I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915/gen9: Add GEN8_CS_CHICKEN1 to HW whitelist

[Nick Hoath]

On 13/01/2016 10:06, Arun Siluvery wrote:
> Required for WaEnablePreemptionGranularityControlByUMD:skl,bxt
>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_reg.h         | 2 ++
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++++++
>   2 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6668bb0..1067ff0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5998,6 +5998,8 @@ enum skl_disp_power_wells {
>   #define FF_SLICE_CS_CHICKEN2			_MMIO(0x20e4)
>   #define  GEN9_TSG_BARRIER_ACK_DISABLE		(1<<8)
>
> +#define GEN8_CS_CHICKEN1		_MMIO(0x2580)
> +
>   /* GEN7 chicken */
>   #define GEN7_COMMON_SLICE_CHICKEN1		_MMIO(0x7010)
>   # define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC	((1<<10) | (1<<26))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 354da81..35e78ed 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -909,6 +909,7 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
>   	struct drm_device *dev = ring->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	uint32_t tmp;
> +	int ret;
>
>   	/* WaEnableLbsSlaRetryTimerDecrement:skl */
>   	I915_WRITE(BDW_SCRATCH1, I915_READ(BDW_SCRATCH1) |
> @@ -979,6 +980,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
>   	/* WaDisableSTUnitPowerOptimization:skl,bxt */
>   	WA_SET_BIT_MASKED(HALF_SLICE_CHICKEN2, GEN8_ST_PO_DISABLE);
>
> +	/* WaEnablePreemptionGranularityControlByUMD:skl,bxt */
> +	ret= wa_ring_whitelist_reg(ring, GEN8_CS_CHICKEN1);
> +	if (ret)
> +		return ret;
> +
>   	return 0;
>   }
>
>

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

Re: [PATCH 3/8] drm/i915/gen9: Add HDC_CHICKEN1 to HW whitelist

[Nick Hoath]

On 13/01/2016 10:06, Arun Siluvery wrote:
> Required for WaAllowUMDToModifyHDCChicken1:skl,bxt
>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_reg.h         | 2 ++
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
>   2 files changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1067ff0..16ef377 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6045,6 +6045,8 @@ enum skl_disp_power_wells {
>   #define  HDC_FORCE_NON_COHERENT			(1<<4)
>   #define  HDC_BARRIER_PERFORMANCE_DISABLE	(1<<10)
>
> +#define GEN8_HDC_CHICKEN1			_MMIO(0x7304)
> +
>   /* GEN9 chicken */
>   #define SLICE_ECO_CHICKEN0			_MMIO(0x7308)
>   #define   PIXEL_MASK_CAMMING_DISABLE		(1 << 14)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 35e78ed..2241a92 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -985,6 +985,11 @@ static int gen9_init_workarounds(struct intel_engine_cs *ring)
>   	if (ret)
>   		return ret;
>
> +	/* WaAllowUMDToModifyHDCChicken1:skl,bxt */
> +	ret = wa_ring_whitelist_reg(ring, GEN8_HDC_CHICKEN1);
> +	if (ret)
> +		return ret;
> +
>   	return 0;
>   }
>
>

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

Re: [PATCH 4/8] drm/i915/bxt: Add GEN9_CS_DEBUG_MODE1 to HW whitelist

[Nick Hoath]

On 13/01/2016 10:06, Arun Siluvery wrote:
> Required for,
> WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt
> WaDisableObjectLevelPreemptionForInstancedDraw:bxt
> WaDisableObjectLevelPreemtionForInstanceId:bxt
>
> According to WA database these are only applicable for BXT:A0 but since
> A0 and A1 shares the same GT these are extended for A1 as well.
>
> These are also required for SKL until B0 but not adding them because they
> are pre-production steppings.
>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         | 1 +
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 9 +++++++++
>   2 files changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 16ef377..eabd2af 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5998,6 +5998,7 @@ enum skl_disp_power_wells {
>   #define FF_SLICE_CS_CHICKEN2			_MMIO(0x20e4)
>   #define  GEN9_TSG_BARRIER_ACK_DISABLE		(1<<8)
>
> +#define GEN9_CS_DEBUG_MODE1		_MMIO(0x20EC)

The pattern seems to be lc for hex (0x20ec)

>   #define GEN8_CS_CHICKEN1		_MMIO(0x2580)
>
>   /* GEN7 chicken */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2241a92..7a46cf1 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1132,6 +1132,15 @@ static int bxt_init_workarounds(struct intel_engine_cs *ring)
>   			GEN7_SBE_SS_CACHE_DISPATCH_PORT_SHARING_DISABLE);
>   	}
>
> +	/* WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt */
> +	/* WaDisableObjectLevelPreemptionForInstancedDraw:bxt */
> +	/* WaDisableObjectLevelPreemtionForInstanceId:bxt */
> +	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> +		ret = wa_ring_whitelist_reg(ring, GEN9_CS_DEBUG_MODE1);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	return 0;
>   }
>
>

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

Re: [PATCH 5/8] drm/i915/bxt: Add GEN8_L3SQCREG4 to HW whitelist

[Nick Hoath]

On 13/01/2016 10:06, Arun Siluvery wrote:
> Required for WaDisableLSQCROPERFforOCL:bxt
>
> According to WA database these are only applicable for BXT:A0 but since
> A0 and A1 shares the same GT these are extended for A1 as well.
>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 7a46cf1..5eb4eea 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1135,10 +1135,15 @@ static int bxt_init_workarounds(struct intel_engine_cs *ring)
>   	/* WaDisableObjectLevelPreemptionForTrifanOrPolygon:bxt */
>   	/* WaDisableObjectLevelPreemptionForInstancedDraw:bxt */
>   	/* WaDisableObjectLevelPreemtionForInstanceId:bxt */
> +	/* WaDisableLSQCROPERFforOCL:bxt */
>   	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
>   		ret = wa_ring_whitelist_reg(ring, GEN9_CS_DEBUG_MODE1);
>   		if (ret)
>   			return ret;
> +
> +		ret = wa_ring_whitelist_reg(ring, GEN8_L3SQCREG4);
> +		if (ret)
> +			return ret;
>   	}
>
>   	return 0;
>

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

Re: [PATCH 6/8] drm/i915/skl: Add GEN8_L3SQCREG4 to HW whitelist

[Nick Hoath]

On 13/01/2016 10:06, Arun Siluvery wrote:
> Required for WaDisableLSQCROPERFforOCL:skl
>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>

> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 5eb4eea..b8dbd2c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1097,6 +1097,11 @@ static int skl_init_workarounds(struct intel_engine_cs *ring)
>   			GEN7_HALF_SLICE_CHICKEN1,
>   			GEN7_SBE_SS_CACHE_DISPATCH_PORT_SHARING_DISABLE);
>
> +	/* WaDisableLSQCROPERFforOCL:skl */
> +	ret = wa_ring_whitelist_reg(ring, GEN8_L3SQCREG4);
> +	if (ret)
> +		return ret;
> +
>   	return skl_tune_iz_hashing(ring);
>   }
>
>

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

Re: [PATCH 7/8] drm/i915/skl: Enable Per context Preemption granularity control

[Nick Hoath]

On 13/01/2016 10:06, Arun Siluvery wrote:
> Per context preemption granularity control is only available from SKL:E0+
>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         |  3 +++
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++++++
>   2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index eabd2af..97774a3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5995,6 +5995,9 @@ enum skl_disp_power_wells {
>   #define SKL_DFSM_CDCLK_LIMIT_450	(2 << 23)
>   #define SKL_DFSM_CDCLK_LIMIT_337_5	(3 << 23)
>
> +#define GEN7_FF_SLICE_CS_CHICKEN1	_MMIO(0x20E0)

0x20e0?

> +#define   GEN9_FFSC_PERCTX_PREEMPT_CTRL	(1<<14)
> +
>   #define FF_SLICE_CS_CHICKEN2			_MMIO(0x20e4)
>   #define  GEN9_TSG_BARRIER_ACK_DISABLE		(1<<8)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index b8dbd2c..5a2ad10 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1045,6 +1045,16 @@ static int skl_init_workarounds(struct intel_engine_cs *ring)
>   	if (ret)
>   		return ret;
>
> +	/*
> +	 * Actual WA is to disable percontext preemption granularity control
> +	 * until D0 which is the default case so this is equivalent to
> +	 * !WaDisablePerCtxtPreemptionGranularityControl:skl
> +	 */
> +	if (IS_SKL_REVID(dev, SKL_REVID_E0, REVID_FOREVER)) {
> +		I915_WRITE(GEN7_FF_SLICE_CS_CHICKEN1,
> +			   _MASKED_BIT_ENABLE(GEN9_FFSC_PERCTX_PREEMPT_CTRL));
> +	}
> +
>   	if (IS_SKL_REVID(dev, 0, SKL_REVID_D0)) {
>   		/* WaDisableChickenBitTSGBarrierAckForFFSliceCS:skl */
>   		I915_WRITE(FF_SLICE_CS_CHICKEN2,
>

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