Attachment 95781 Details for Bug 76151 – fix runtime pm inbalance

[patch] fix runtime pm inbalance

0001-drm-i915-Fix-runtime-pm-inbalance-due-to-reg-I-O-for.patch (text/plain), 4.80 KB, created by Daniel Vetter on 2014-03-14 07:48:37 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Vetter

Created: 2014-03-14 07:48:37 UTC

Size: 4.80 KB

patch

obsolete

>From 5c37cf7ee6101a9cdf3d3c87d5bc34e2ef4a02d4 Mon Sep 17 00:00:00 2001
>From: Daniel Vetter <daniel.vetter@ffwll.ch>
>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 <chris@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@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@chris-wilson.co.uk>
>Cc: Ben Widawsky <ben@bwidawsk.net>
>Cc: Ville SyrjÃ¤lÃ¤ <ville.syrjala@linux.intel.com>
>Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76151
>Signed-off-by: Daniel Vetter <daniel.vetter@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
>

Actions: View | Splinter Review

Attachments on bug 76151: 95768 | 95781 | 99548

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.