| Message ID | 20180830111507.7859-1-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: clear error registers after error capture | expand |
Quoting Lionel Landwerlin (2018-08-30 12:15:07) > We need to clear the register in order to get correct value after the > next potential hang. > > v2: Centralize error register clearing in i915_irq.c (Chris) > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Ok, I was thinking of move the code around the files a bit more, but agree with what you've done as being the smallest change possible. > @@ -3238,6 +3238,22 @@ static void i915_clear_error_registers(struct drm_i915_private *dev_priv) > I915_WRITE(EMR, I915_READ(EMR) | eir); > I915_WRITE(IIR, I915_MASTER_ERROR_INTERRUPT); > } > + > + if (INTEL_GEN(dev_priv) < 8) { > + struct intel_engine_cs *engine; > + enum intel_engine_id id; > + > + for_each_engine(engine, dev_priv, id) { > + I915_WRITE(RING_FAULT_REG(engine), > + I915_READ(RING_FAULT_REG(engine)) & > + ~RING_FAULT_VALID); > + } > + POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS])); Memory says this is gen6+, and i915_gpu_error.c has the same opinion. > + } else { > + I915_WRITE(GEN8_RING_FAULT_REG, > + I915_READ(GEN8_RING_FAULT_REG) & ~RING_FAULT_VALID); > + POSTING_READ(GEN8_RING_FAULT_REG); > + } I think you want: if (INTEL_GEN > 8) { ... } else if (INTEL_GEN > 6) { ... } else { /* Are there any fault regs for earlier? */ } With that tweak, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 30/08/2018 12:23, Chris Wilson wrote: > Quoting Lionel Landwerlin (2018-08-30 12:15:07) >> We need to clear the register in order to get correct value after the >> next potential hang. >> >> v2: Centralize error register clearing in i915_irq.c (Chris) >> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Ok, I was thinking of move the code around the files a bit more, but > agree with what you've done as being the smallest change possible. If you have a suggestion, I can do a v3. > >> @@ -3238,6 +3238,22 @@ static void i915_clear_error_registers(struct drm_i915_private *dev_priv) >> I915_WRITE(EMR, I915_READ(EMR) | eir); >> I915_WRITE(IIR, I915_MASTER_ERROR_INTERRUPT); >> } >> + >> + if (INTEL_GEN(dev_priv) < 8) { >> + struct intel_engine_cs *engine; >> + enum intel_engine_id id; >> + >> + for_each_engine(engine, dev_priv, id) { >> + I915_WRITE(RING_FAULT_REG(engine), >> + I915_READ(RING_FAULT_REG(engine)) & >> + ~RING_FAULT_VALID); >> + } >> + POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS])); > Memory says this is gen6+, and i915_gpu_error.c has the same opinion. What is? The post read? > >> + } else { >> + I915_WRITE(GEN8_RING_FAULT_REG, >> + I915_READ(GEN8_RING_FAULT_REG) & ~RING_FAULT_VALID); >> + POSTING_READ(GEN8_RING_FAULT_REG); >> + } > I think you want: > > if (INTEL_GEN > 8) { > ... > } else if (INTEL_GEN > 6) { > ... > } else { > /* Are there any fault regs for earlier? */ > } > > With that tweak, Thanks, done locally. > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris >
Quoting Lionel Landwerlin (2018-08-30 12:41:42) > On 30/08/2018 12:23, Chris Wilson wrote: > > Quoting Lionel Landwerlin (2018-08-30 12:15:07) > >> We need to clear the register in order to get correct value after the > >> next potential hang. > >> > >> v2: Centralize error register clearing in i915_irq.c (Chris) > >> > >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > Ok, I was thinking of move the code around the files a bit more, but > > agree with what you've done as being the smallest change possible. > > > If you have a suggestion, I can do a v3. Basically pulling the fault reg out of i915_gem_gtt.c and everything into i915_reset.c. And then removing the struct_mutex. > >> @@ -3238,6 +3238,22 @@ static void i915_clear_error_registers(struct drm_i915_private *dev_priv) > >> I915_WRITE(EMR, I915_READ(EMR) | eir); > >> I915_WRITE(IIR, I915_MASTER_ERROR_INTERRUPT); > >> } > >> + > >> + if (INTEL_GEN(dev_priv) < 8) { > >> + struct intel_engine_cs *engine; > >> + enum intel_engine_id id; > >> + > >> + for_each_engine(engine, dev_priv, id) { > >> + I915_WRITE(RING_FAULT_REG(engine), > >> + I915_READ(RING_FAULT_REG(engine)) & > >> + ~RING_FAULT_VALID); > >> + } > >> + POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS])); > > Memory says this is gen6+, and i915_gpu_error.c has the same opinion. > > > What is? The post read? The comment below :) > >> + } else { > >> + I915_WRITE(GEN8_RING_FAULT_REG, > >> + I915_READ(GEN8_RING_FAULT_REG) & ~RING_FAULT_VALID); > >> + POSTING_READ(GEN8_RING_FAULT_REG); > >> + } > > I think you want: > > > > if (INTEL_GEN > 8) { > > ... > > } else if (INTEL_GEN > 6) { > > ... > > } else { > > /* Are there any fault regs for earlier? */ > > } > > > > With that tweak, > > > Thanks, done locally. Hopefully with >= :) -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 40c93a37e385..34cca15be926 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2817,6 +2817,8 @@ extern void intel_irq_fini(struct drm_i915_private *dev_priv); int intel_irq_install(struct drm_i915_private *dev_priv); void intel_irq_uninstall(struct drm_i915_private *dev_priv); +void i915_clear_error_registers(struct drm_i915_private *dev_priv); + static inline bool intel_gvt_active(struct drm_i915_private *dev_priv) { return dev_priv->gvt; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 4137af4bd8f5..d9d44639ba26 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2337,7 +2337,7 @@ static bool needs_idle_maps(struct drm_i915_private *dev_priv) return IS_GEN5(dev_priv) && IS_MOBILE(dev_priv) && intel_vtd_active(); } -static void gen6_check_and_clear_faults(struct drm_i915_private *dev_priv) +static void gen6_check_faults(struct drm_i915_private *dev_priv) { struct intel_engine_cs *engine; enum intel_engine_id id; @@ -2355,15 +2355,11 @@ static void gen6_check_and_clear_faults(struct drm_i915_private *dev_priv) fault & RING_FAULT_GTTSEL_MASK ? "GGTT" : "PPGTT", RING_FAULT_SRCID(fault), RING_FAULT_FAULT_TYPE(fault)); - I915_WRITE(RING_FAULT_REG(engine), - fault & ~RING_FAULT_VALID); } } - - POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS])); } -static void gen8_check_and_clear_faults(struct drm_i915_private *dev_priv) +static void gen8_check_faults(struct drm_i915_private *dev_priv) { u32 fault = I915_READ(GEN8_RING_FAULT_REG); @@ -2388,22 +2384,20 @@ static void gen8_check_and_clear_faults(struct drm_i915_private *dev_priv) GEN8_RING_FAULT_ENGINE_ID(fault), RING_FAULT_SRCID(fault), RING_FAULT_FAULT_TYPE(fault)); - I915_WRITE(GEN8_RING_FAULT_REG, - fault & ~RING_FAULT_VALID); } - - POSTING_READ(GEN8_RING_FAULT_REG); } void i915_check_and_clear_faults(struct drm_i915_private *dev_priv) { /* From GEN8 onwards we only have one 'All Engine Fault Register' */ if (INTEL_GEN(dev_priv) >= 8) - gen8_check_and_clear_faults(dev_priv); + gen8_check_faults(dev_priv); else if (INTEL_GEN(dev_priv) >= 6) - gen6_check_and_clear_faults(dev_priv); + gen6_check_faults(dev_priv); else return; + + i915_clear_error_registers(dev_priv); } void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 8084e35b25c5..61a01584eae2 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3215,7 +3215,7 @@ static void i915_reset_device(struct drm_i915_private *dev_priv, kobject_uevent_env(kobj, KOBJ_CHANGE, reset_done_event); } -static void i915_clear_error_registers(struct drm_i915_private *dev_priv) +void i915_clear_error_registers(struct drm_i915_private *dev_priv) { u32 eir; @@ -3238,6 +3238,22 @@ static void i915_clear_error_registers(struct drm_i915_private *dev_priv) I915_WRITE(EMR, I915_READ(EMR) | eir); I915_WRITE(IIR, I915_MASTER_ERROR_INTERRUPT); } + + if (INTEL_GEN(dev_priv) < 8) { + struct intel_engine_cs *engine; + enum intel_engine_id id; + + for_each_engine(engine, dev_priv, id) { + I915_WRITE(RING_FAULT_REG(engine), + I915_READ(RING_FAULT_REG(engine)) & + ~RING_FAULT_VALID); + } + POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS])); + } else { + I915_WRITE(GEN8_RING_FAULT_REG, + I915_READ(GEN8_RING_FAULT_REG) & ~RING_FAULT_VALID); + POSTING_READ(GEN8_RING_FAULT_REG); + } } /**
We need to clear the register in order to get correct value after the next potential hang. v2: Centralize error register clearing in i915_irq.c (Chris) Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem_gtt.c | 18 ++++++------------ drivers/gpu/drm/i915/i915_irq.c | 18 +++++++++++++++++- 3 files changed, 25 insertions(+), 13 deletions(-)