From 5c37cf7ee6101a9cdf3d3c87d5bc34e2ef4a02d4 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Fri, 14 Mar 2014 08:41:33 +0100 Subject: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This regression has been introduced in commit 8232644ccf099548710843e97360a3fcd6d28e04 Author: Chris Wilson 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 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. Cc: Chris Wilson Cc: Ben Widawsky Cc: Ville Syrjälä Cc: Paulo Zanoni Cc: Jesse Barnes Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76151 Signed-off-by: Daniel Vetter --- 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