[Intel-gfx] [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

[Daniel Vetter]

This regression has been introduced in

commit 8232644ccf099548710843e97360a3fcd6d28e04
Author: Chris Wilson <chris at chris-wilson.co.uk>
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 at intel.com>
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 <chris at chris-wilson.co.uk>
Cc: Ben Widawsky <ben at bwidawsk.net>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76151
Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
---
 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