drm/i915: There is only one fault register from Gen8 onwards

Submitted by Michel Thierry on June 23, 2017, 11:17 p.m.

Details

Message ID 20170623231729.35240-1-michel.thierry@intel.com
State New
Headers show
Series "drm/i915: There is only one fault register from Gen8 onwards" ( rev: 1 ) in Intel GFX

Browsing this patch as part of:
"drm/i915: There is only one fault register from Gen8 onwards" rev 1 in Intel GFX
<< prev patch [1/1] next patch >>

Commit Message

Michel Thierry June 23, 2017, 11:17 p.m.
Until Haswell/Baytrail, the hardware used to have a per engine fault
register (e.g. 0x4094 - render fault register, 0x4194 - media fault
register, etc). But since Broadwell, all these registers were combined
into a singe one, which specifies the engine id in bits 14:12.

Luckily, the additional register addresses haven't been reused, but we
should not been reading (and writing to) registers that do not exist.

References: IHD-OS-BDW-Vol 2c-11.15, page 75.
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c   | 29 ++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_gpu_error.c |  8 +++++---
 drivers/gpu/drm/i915/i915_reg.h       |  2 ++
 3 files changed, 31 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index de67084d5fcf..83deb004368c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2000,22 +2000,41 @@  void i915_check_and_clear_faults(struct drm_i915_private *dev_priv)
 	if (INTEL_INFO(dev_priv)->gen < 6)
 		return;
 
-	for_each_engine(engine, dev_priv, id) {
+	/* From GEN8 onwards we only have one 'All Engine Fault Register' */
+	if (INTEL_INFO(dev_priv)->gen >= 8) {
 		u32 fault_reg;
-		fault_reg = I915_READ(RING_FAULT_REG(engine));
+		fault_reg = I915_READ(GEN8_RING_FAULT_REG);
 		if (fault_reg & RING_FAULT_VALID) {
 			DRM_DEBUG_DRIVER("Unexpected fault\n"
 					 "\tAddr: 0x%08lx\n"
-					 "\tAddress space: %s\n"
+					 "\tEngine ID: %d\n"
 					 "\tSource ID: %d\n"
 					 "\tType: %d\n",
 					 fault_reg & PAGE_MASK,
-					 fault_reg & RING_FAULT_GTTSEL_MASK ? "GGTT" : "PPGTT",
+					 GEN8_RING_FAULT_ENGINE_ID(fault_reg),
 					 RING_FAULT_SRCID(fault_reg),
 					 RING_FAULT_FAULT_TYPE(fault_reg));
-			I915_WRITE(RING_FAULT_REG(engine),
+			I915_WRITE(GEN8_RING_FAULT_REG,
 				   fault_reg & ~RING_FAULT_VALID);
 		}
+	} else {
+		for_each_engine(engine, dev_priv, id) {
+			u32 fault_reg;
+			fault_reg = I915_READ(RING_FAULT_REG(engine));
+			if (fault_reg & RING_FAULT_VALID) {
+				DRM_DEBUG_DRIVER("Unexpected fault\n"
+						 "\tAddr: 0x%08lx\n"
+						 "\tAddress space: %s\n"
+						 "\tSource ID: %d\n"
+						 "\tType: %d\n",
+						 fault_reg & PAGE_MASK,
+						 fault_reg & RING_FAULT_GTTSEL_MASK ? "GGTT" : "PPGTT",
+						 RING_FAULT_SRCID(fault_reg),
+						 RING_FAULT_FAULT_TYPE(fault_reg));
+				I915_WRITE(RING_FAULT_REG(engine),
+					   fault_reg & ~RING_FAULT_VALID);
+			}
+		}
 	}
 
 	/* Engine specific init may not have been done till this point. */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index ae70283470a6..b7f147b1b6dd 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1167,11 +1167,13 @@  static void error_record_engine_registers(struct i915_gpu_state *error,
 
 	if (INTEL_GEN(dev_priv) >= 6) {
 		ee->rc_psmi = I915_READ(RING_PSMI_CTL(engine->mmio_base));
-		ee->fault_reg = I915_READ(RING_FAULT_REG(engine));
-		if (INTEL_GEN(dev_priv) >= 8)
+		if (INTEL_GEN(dev_priv) >= 8) {
 			gen8_record_semaphore_state(error, engine, ee);
-		else
+			ee->fault_reg = I915_READ(GEN8_RING_FAULT_REG);
+		} else {
 			gen6_record_semaphore_state(engine, ee);
+			ee->fault_reg = I915_READ(RING_FAULT_REG(engine));
+		}
 	}
 
 	if (INTEL_GEN(dev_priv) >= 4) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c8647cfa81ba..54a828aad777 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2149,6 +2149,8 @@  enum skl_disp_power_wells {
 #define   ARB_MODE_SWIZZLE_BDW	(1<<1)
 #define RENDER_HWS_PGA_GEN7	_MMIO(0x04080)
 #define RING_FAULT_REG(engine)	_MMIO(0x4094 + 0x100*(engine)->hw_id)
+#define GEN8_RING_FAULT_REG	_MMIO(0x4094)
+#define   GEN8_RING_FAULT_ENGINE_ID(x)	(((x) >> 12) & 0x7)
 #define   RING_FAULT_GTTSEL_MASK (1<<11)
 #define   RING_FAULT_SRCID(x)	(((x) >> 3) & 0xff)
 #define   RING_FAULT_FAULT_TYPE(x) (((x) >> 1) & 0x3)

Comments

Quoting Michel Thierry (2017-06-24 00:17:29)
> Until Haswell/Baytrail, the hardware used to have a per engine fault
> register (e.g. 0x4094 - render fault register, 0x4194 - media fault
> register, etc). But since Broadwell, all these registers were combined
> into a singe one, which specifies the engine id in bits 14:12.
> 
> Luckily, the additional register addresses haven't been reused, but we
> should not been reading (and writing to) registers that do not exist.
> 
> References: IHD-OS-BDW-Vol 2c-11.15, page 75.
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c   | 29 ++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_gpu_error.c |  8 +++++---
>  drivers/gpu/drm/i915/i915_reg.h       |  2 ++
>  3 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index de67084d5fcf..83deb004368c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2000,22 +2000,41 @@ void i915_check_and_clear_faults(struct drm_i915_private *dev_priv)
>         if (INTEL_INFO(dev_priv)->gen < 6)
>                 return;
>  
> -       for_each_engine(engine, dev_priv, id) {
> +       /* From GEN8 onwards we only have one 'All Engine Fault Register' */
> +       if (INTEL_INFO(dev_priv)->gen >= 8) {
INTEL_GEN() >= 8

>                 u32 fault_reg;

Newline after variable blocks (might as well fix up the coding style in
passing).

u32 fault;

foo_reg is more commonly used to refer to the register offset itself,
i.e. i915_reg_t.  Might as well move it to the start of the function as
it is shared between all branches, and I don't think we are overly
concerned with aiding gcc's liverange analysis here.
-Chris
On 23/06/17 16:35, Chris Wilson wrote:
> Quoting Michel Thierry (2017-06-24 00:17:29)
>> Until Haswell/Baytrail, the hardware used to have a per engine fault
>> register (e.g. 0x4094 - render fault register, 0x4194 - media fault
>> register, etc). But since Broadwell, all these registers were combined
>> into a singe one, which specifies the engine id in bits 14:12.
>>
>> Luckily, the additional register addresses haven't been reused, but we
>> should not been reading (and writing to) registers that do not exist.
>>
>> References: IHD-OS-BDW-Vol 2c-11.15, page 75.
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c   | 29 ++++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/i915_gpu_error.c |  8 +++++---
>>  drivers/gpu/drm/i915/i915_reg.h       |  2 ++
>>  3 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index de67084d5fcf..83deb004368c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2000,22 +2000,41 @@ void i915_check_and_clear_faults(struct drm_i915_private *dev_priv)
>>         if (INTEL_INFO(dev_priv)->gen < 6)
>>                 return;
>>
>> -       for_each_engine(engine, dev_priv, id) {
>> +       /* From GEN8 onwards we only have one 'All Engine Fault Register' */
>> +       if (INTEL_INFO(dev_priv)->gen >= 8) {
> INTEL_GEN() >= 8
>

Oops, busted copy-pasting old code, I'll also change it 
(INTEL_INFO(dev_priv)->gen < 6).

>>                 u32 fault_reg;
>
> Newline after variable blocks (might as well fix up the coding style in
> passing).
>
> u32 fault;
>
> foo_reg is more commonly used to refer to the register offset itself,
> i.e. i915_reg_t.  Might as well move it to the start of the function as
> it is shared between all branches, and I don't think we are overly
> concerned with aiding gcc's liverange analysis here.

I thought about moving it to the start of the function. Let me make 
these changes.

Thanks,

-Michel