Bug 76151

(In reply to comment #0)
> It's a regression bug, the first bad commit is
> 6d88064edcfc5e5893371f7c06b9f3078dc1edf6 

Please always include the first lines of the commit:

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

and for bisected regressions always assign the bug to the author of the bad commit. Done now. Thanks.

Can you please tests the below patch?

http://patchwork.freedesktop.org/patch/21687/

(In reply to comment #2)
> Can you please tests the below patch?
> 
> http://patchwork.freedesktop.org/patch/21687/

It's pass with this patch, thanks.

Created attachment 95781 [details] [review]
fix runtime pm inbalance

Ok, slightly improved patch (Paulo's is just a revert). Please test.

Ugh, that interleaves the lifetime of the pm vs forcewake ref.

reg read: -> deferred forcewake.
gen6_gt_get: -> pm_get, forcewake++
gen6_gt_put: -> --forcewake, pm_put

device is now asleep

forcewake_timer: boom, invalid reg access.


diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b8b59752562a..e92bdcef5342 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5877,6 +5877,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	if (!HAS_RUNTIME_PM(dev))
 		return;
 
+	del_timer_sync(&dev_priv->uncore.force_wake_timer);
+
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_put_autosuspend(device);
 }
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e6bb421a3dbd..527527382361 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -304,8 +304,6 @@ 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);
-
-	intel_runtime_pm_put(dev_priv);
 }
 
 static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
@@ -439,7 +437,6 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 {
 	unsigned long irqflags;
-	bool delayed = false;
 
 	if (!dev_priv->uncore.funcs.force_wake_put)
 		return;
@@ -450,19 +447,17 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 		goto out;
 	}
 
-
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	if (--dev_priv->uncore.forcewake_count == 0) {
 		dev_priv->uncore.forcewake_count++;
-		delayed = true;
 		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
 				 jiffies + 1);
 	}
+
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
 out:
-	if (!delayed)
-		intel_runtime_pm_put(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 }
 
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv)

(In reply to comment #5)
> Ugh, that interleaves the lifetime of the pm vs forcewake ref.
> 
> reg read: -> deferred forcewake.
> gen6_gt_get: -> pm_get, forcewake++
> gen6_gt_put: -> --forcewake, pm_put
> 
> device is now asleep
> 
> forcewake_timer: boom, invalid reg access.

You can only do reg I/O if you already have a runtime pm reference. But yeah I've missed that we don't actually call the full machinery on suspend ...

Both approaches have the ugly issue though that the del_timer_sync will leak the forcewake reference. So I think we need to call the timer function if we've actually deactivated it ... fun stuff.

Is this still an issue on latest -nightly? Please retest.

Created attachment 99548 [details]
dmesg

Booting with HDMI only on latest -nightly(36765340cb068dec1216342bfcdbf2678ec2986). HDMI monitor works well.

(In reply to comment #8)
> Created attachment 99548 [details]
> dmesg
> 
> Booting with HDMI only on latest
> -nightly(36765340cb068dec1216342bfcdbf2678ec2986). HDMI monitor works well.

Closing bug then. If it still happens, please reopen.

(In reply to comment #8)
> Created attachment 99548 [details]
> dmesg
> 
> Booting with HDMI only on latest
> -nightly(36765340cb068dec1216342bfcdbf2678ec2986). HDMI monitor works well.

Closing verified+fixed.