| Message ID | 1444141613-11152-8-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 06, 2015 at 05:26:48PM +0300, Mika Kuoppala wrote: > @@ -6973,8 +6989,12 @@ void intel_init_clock_gating(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + intel_wa_init(&dev_priv->mmio_workarounds); > + > if (dev_priv->display.init_clock_gating) > dev_priv->display.init_clock_gating(dev); > + > + intel_wa_write_mmio(dev_priv, &dev_priv->mmio_workarounds); > } Given that we have this function which is in charge of setting the w/a regs and is supposed to be called when initialising the hw after load/reset/resume, why? -Chris
On Wed, Oct 07, 2015 at 09:43:07AM +0100, Chris Wilson wrote: > On Tue, Oct 06, 2015 at 05:26:48PM +0300, Mika Kuoppala wrote: > > @@ -6973,8 +6989,12 @@ void intel_init_clock_gating(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > + intel_wa_init(&dev_priv->mmio_workarounds); > > + > > if (dev_priv->display.init_clock_gating) > > dev_priv->display.init_clock_gating(dev); > > + > > + intel_wa_write_mmio(dev_priv, &dev_priv->mmio_workarounds); > > } > > Given that we have this function which is in charge of setting the w/a > regs and is supposed to be called when initialising the hw after > load/reset/resume, why? I think ideally we should move all the GT related w/as, or at least the ones clobbered by a GPU reset into ring init, or somewhere close by. The display stuff (and UCGCTL at least) could stay where they are. Although I suspect we should move the init_clock_gating earlier in the init/resume sequence anyway. I think now we might be a bit late in doing it.
On Tue, Oct 06, 2015 at 05:26:48PM +0300, Mika Kuoppala wrote: > From: Mika Kuoppala <miku@testikku.fi.intel.com> > > Rewrite everything in mmio workaround list right after > gpu reset. This ensures that we start the reinitialization > with proper mmio workarounds in place, before we > start the rings. > > This commit just adds the mechanism, the list itself > is still empty. Following commits will add registers. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > drivers/gpu/drm/i915/i915_irq.c | 2 ++ > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- > drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++++++++ > 5 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ae5b6b3..d41808a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2732,6 +2732,7 @@ void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv, > void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv, > enum forcewake_domains domains); > void assert_forcewakes_inactive(struct drm_i915_private *dev_priv); > +void assert_forcewakes_active(struct drm_i915_private *dev_priv); > static inline bool intel_vgpu_active(struct drm_device *dev) > { > return to_i915(dev)->vgpu.active; > @@ -3545,7 +3546,14 @@ static inline void i915_trace_irq_get(struct intel_engine_cs *ring, > #define WA_CLR_BIT(t, addr, mask) \ > WA_REG_##t(addr, mask, I915_READ((addr)) & ~(mask)) > > +static inline void intel_wa_init(struct i915_workarounds *w) > +{ > + w->count = 0; > +} > + > int intel_wa_add(struct i915_workarounds *w, > const u32 addr, const u32 mask, const u32 val); > > +void intel_wa_write_mmio(struct drm_i915_private *dev_priv, > + const struct i915_workarounds *w); > #endif > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 8ca772d..e686f78 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2441,6 +2441,8 @@ static void i915_reset_and_wakeup(struct drm_device *dev) > */ > ret = i915_reset(dev); > > + intel_wa_write_mmio(dev_priv, &dev_priv->mmio_workarounds); > + > intel_finish_reset(dev); > > intel_runtime_pm_put(dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 60d120c..e97f271 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -52,6 +52,22 @@ > #define INTEL_RC6p_ENABLE (1<<1) > #define INTEL_RC6pp_ENABLE (1<<2) > > +void intel_wa_write_mmio(struct drm_i915_private *dev_priv, > + const struct i915_workarounds *w) > +{ > + int i; > + > + if (WARN_ON_ONCE(w->count == 0)) > + return; > + > + assert_forcewakes_active(dev_priv); > + > + for (i = 0; i < w->count; i++) > + I915_WRITE_FW(w->reg[i].addr, w->reg[i].value); > + > + DRM_DEBUG_DRIVER("Number of Workarounds written: %d\n", w->count); > +} > + > static void gen9_init_clock_gating(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -6973,8 +6989,12 @@ void intel_init_clock_gating(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + intel_wa_init(&dev_priv->mmio_workarounds); > + > if (dev_priv->display.init_clock_gating) > dev_priv->display.init_clock_gating(dev); > + > + intel_wa_write_mmio(dev_priv, &dev_priv->mmio_workarounds); init_clock_gating is for display block workarounds, not for anything in the GT. That should be done by either the various engine/ring init hooks or by new code. The reason for that is that display and GT can be reset separately (well GT can be reset without display going down, at least on modern hw). -Daniel > } > > void intel_suspend_hw(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index c9d3489e..3667dd9 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1074,7 +1074,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring) > > WARN_ON(ring->id != RCS); > > - dev_priv->lri_workarounds.count = 0; > + intel_wa_init(&dev_priv->lri_workarounds); > > if (IS_BROADWELL(dev)) > return bdw_init_workarounds(ring); > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index b43c6d0..3f8d1f6 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -524,6 +524,18 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv) > WARN_ON(domain->wake_count); > } > > +void assert_forcewakes_active(struct drm_i915_private *dev_priv) > +{ > + struct intel_uncore_forcewake_domain *domain; > + enum forcewake_domain_id id; > + > + if (!dev_priv->uncore.funcs.force_wake_get) > + return; > + > + for_each_fw_domain(domain, dev_priv, id) > + WARN_ON(domain->wake_count == 0); > +} > + > /* We give fast paths for the really cool registers */ > #define NEEDS_FORCE_WAKE(dev_priv, reg) \ > ((reg) < 0x40000 && (reg) != FORCEWAKE) > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 07, 2015 at 04:52:54PM +0300, Ville Syrjälä wrote: > On Wed, Oct 07, 2015 at 09:43:07AM +0100, Chris Wilson wrote: > > On Tue, Oct 06, 2015 at 05:26:48PM +0300, Mika Kuoppala wrote: > > > @@ -6973,8 +6989,12 @@ void intel_init_clock_gating(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > > + intel_wa_init(&dev_priv->mmio_workarounds); > > > + > > > if (dev_priv->display.init_clock_gating) > > > dev_priv->display.init_clock_gating(dev); > > > + > > > + intel_wa_write_mmio(dev_priv, &dev_priv->mmio_workarounds); > > > } > > > > Given that we have this function which is in charge of setting the w/a > > regs and is supposed to be called when initialising the hw after > > load/reset/resume, why? > > I think ideally we should move all the GT related w/as, or at least the > ones clobbered by a GPU reset into ring init, or somewhere close by. > The display stuff (and UCGCTL at least) could stay where they are. > Although I suspect we should move the init_clock_gating earlier in the > init/resume sequence anyway. I think now we might be a bit late in doing > it. Yes, this is how it's supposed to work. Any w/a which gets clobbered by GT reset and which is still in the init_clock_gating code is just plain a bug in our driver. We have them aplenty unfortunately :( Maybe we should add a big kerneldoc to intel_init_clock_gating explaining that. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ae5b6b3..d41808a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2732,6 +2732,7 @@ void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv, void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv, enum forcewake_domains domains); void assert_forcewakes_inactive(struct drm_i915_private *dev_priv); +void assert_forcewakes_active(struct drm_i915_private *dev_priv); static inline bool intel_vgpu_active(struct drm_device *dev) { return to_i915(dev)->vgpu.active; @@ -3545,7 +3546,14 @@ static inline void i915_trace_irq_get(struct intel_engine_cs *ring, #define WA_CLR_BIT(t, addr, mask) \ WA_REG_##t(addr, mask, I915_READ((addr)) & ~(mask)) +static inline void intel_wa_init(struct i915_workarounds *w) +{ + w->count = 0; +} + int intel_wa_add(struct i915_workarounds *w, const u32 addr, const u32 mask, const u32 val); +void intel_wa_write_mmio(struct drm_i915_private *dev_priv, + const struct i915_workarounds *w); #endif diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 8ca772d..e686f78 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2441,6 +2441,8 @@ static void i915_reset_and_wakeup(struct drm_device *dev) */ ret = i915_reset(dev); + intel_wa_write_mmio(dev_priv, &dev_priv->mmio_workarounds); + intel_finish_reset(dev); intel_runtime_pm_put(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 60d120c..e97f271 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -52,6 +52,22 @@ #define INTEL_RC6p_ENABLE (1<<1) #define INTEL_RC6pp_ENABLE (1<<2) +void intel_wa_write_mmio(struct drm_i915_private *dev_priv, + const struct i915_workarounds *w) +{ + int i; + + if (WARN_ON_ONCE(w->count == 0)) + return; + + assert_forcewakes_active(dev_priv); + + for (i = 0; i < w->count; i++) + I915_WRITE_FW(w->reg[i].addr, w->reg[i].value); + + DRM_DEBUG_DRIVER("Number of Workarounds written: %d\n", w->count); +} + static void gen9_init_clock_gating(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -6973,8 +6989,12 @@ void intel_init_clock_gating(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + intel_wa_init(&dev_priv->mmio_workarounds); + if (dev_priv->display.init_clock_gating) dev_priv->display.init_clock_gating(dev); + + intel_wa_write_mmio(dev_priv, &dev_priv->mmio_workarounds); } void intel_suspend_hw(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index c9d3489e..3667dd9 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1074,7 +1074,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring) WARN_ON(ring->id != RCS); - dev_priv->lri_workarounds.count = 0; + intel_wa_init(&dev_priv->lri_workarounds); if (IS_BROADWELL(dev)) return bdw_init_workarounds(ring); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index b43c6d0..3f8d1f6 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -524,6 +524,18 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv) WARN_ON(domain->wake_count); } +void assert_forcewakes_active(struct drm_i915_private *dev_priv) +{ + struct intel_uncore_forcewake_domain *domain; + enum forcewake_domain_id id; + + if (!dev_priv->uncore.funcs.force_wake_get) + return; + + for_each_fw_domain(domain, dev_priv, id) + WARN_ON(domain->wake_count == 0); +} + /* We give fast paths for the really cool registers */ #define NEEDS_FORCE_WAKE(dev_priv, reg) \ ((reg) < 0x40000 && (reg) != FORCEWAKE)