On Fri, Mar 14, 2014 at 08:48:46AM +0100, Daniel Vetter wrote: > This regression has been introduced in > > commit 8232644ccf099548710843e97360a3fcd6d28e04 > Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Date: Wed Mar 5 12:00:39 2014 +0000 > > drm/i915: Convert the forcewake worker into a timer func > > which started to use the delayed forcewake put also for the register > I/O forcewake dance. But this conflicts functionally with the > (reviewed in parallel) > > commit 6d88064edcfc5e5893371f7c06b9f3078dc1edf6 > Author: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Date: Fri Feb 21 17:58:29 2014 -0300 > > drm/i915: put runtime PM only when we actually release force_wake > > which moved the runtime pm put calls into the delayed forcewake put > function to avoid an inversion between these. The problem is that the > register I/O function do _not_ grab a runtime pm reference, hence > dropping it is a bug. > > So split the timer into two to re-balance the runtime pm refcounting. > The tricky bit to ensure is that the _raw timer doesn't run after > we've runtime-suspended the device. After all it only has an implicit > runtime pm reference provided by its caller. But the del_timer_sync in > the runtime suspend code will ensure this. > > Add a comment to document this all. Yuck. Why don't we just stick the runtime pm put into gen6_gt_force_wake_put() and let Chris's timer cancellation + forcewake reset fixes take care of things? > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Ben Widawsky <ben@xxxxxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76151 > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++++-- > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 70fbe904016f..c2789702b9a5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -511,7 +511,16 @@ struct intel_uncore { > unsigned fw_rendercount; > unsigned fw_mediacount; > > + /* > + * Delayed forcewake put timers. The _raw one is for register I/O > + * functions which don't grab a runtime pm reference, instead presuming > + * that someone else holds that already. > + * > + * Synchronization with runtime pm actually suspended the device happens > + * in the runtime suspend path in intel_uncore_forcewake_reset. > + */ > struct timer_list force_wake_timer; > + struct timer_list force_wake_timer_raw; > }; > > #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index e6bb421a3dbd..8010e2caf821 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -293,7 +293,7 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv, > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > } > > -static void gen6_force_wake_timer(unsigned long arg) > +static void gen6_force_wake_timer_raw(unsigned long arg) > { > struct drm_i915_private *dev_priv = (void *)arg; > unsigned long irqflags; > @@ -304,6 +304,13 @@ static void gen6_force_wake_timer(unsigned long arg) > if (--dev_priv->uncore.forcewake_count == 0) > dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > +} > + > +static void gen6_force_wake_timer(unsigned long arg) > +{ > + struct drm_i915_private *dev_priv = (void *)arg; > + > + gen6_force_wake_timer_raw(arg); > > intel_runtime_pm_put(dev_priv); > } > @@ -314,6 +321,7 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > unsigned long irqflags; > > del_timer_sync(&dev_priv->uncore.force_wake_timer); > + del_timer_sync(&dev_priv->uncore.force_wake_timer_raw); > > /* Hold uncore.lock across reset to prevent any register access > * with forcewake not set correctly > @@ -542,7 +550,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > FORCEWAKE_ALL); \ > dev_priv->uncore.forcewake_count++; \ > - mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \ > + mod_timer_pinned(&dev_priv->uncore.force_wake_timer_raw, \ > jiffies + 1); \ > } \ > val = __raw_i915_read##x(dev_priv, reg); \ > @@ -726,6 +734,8 @@ void intel_uncore_init(struct drm_device *dev) > > setup_timer(&dev_priv->uncore.force_wake_timer, > gen6_force_wake_timer, (unsigned long)dev_priv); > + setup_timer(&dev_priv->uncore.force_wake_timer_raw, > + gen6_force_wake_timer_raw, (unsigned long)dev_priv); > > if (IS_VALLEYVIEW(dev)) { > dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get; > -- > 1.8.5.2 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
