BDW runtime PM fixes

PATCH Applied Paulo Zanoni 3 participants 10 messages Apr 1, 2014 – Apr 2, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2014-04-01 22:40 UTC #0 lore raw

cover BDW runtime PM fixes

From: Paulo Zanoni <[email protected]>

I got a little surprised to see the BDW patches merged so fast, I thought I was going to have time to resend the last series with these last patches in.

The first patch replaces a HSW-specific WARN that was happening all the time on BDW with a gen-agnostic WARN. The second patch is just a debugfs fix. The third patch fixes an infinite loop, but it does not fix the undelying reason why the infinite loop happens, so maybe we don't want to merge it so we don't hide the real bug.

With these last patches on top of -nightly, my BDW machine passes pm_pc8 without errors.

Thanks,
Paulo

Paulo Zanoni (2):
drm/i915: only check for irqs_disabled when disabling LCPLL drm/i915: fix infinite loop at gen6_update_ring_freq

Zhenyu Wang (1):
drm/i915: Allow i915_pc8_status debug info on BDW

 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++----------
 drivers/gpu/drm/i915/intel_pm.c      |  3 ++-
 3 files changed, 10 insertions(+), 12 deletions(-)

Signature · 1 line

1.8.5.3

Paulo Zanoni

author

2014-04-01 22:40 UTC #1 lore raw

PATCH 1/3 drm/i915: only check for irqs_disabled when disabling LCPLL

From: Paulo Zanoni <[email protected]>

Because if we keep the current code, we'll get tons of WARNs on Broadwell, since the code is Haswell-specific.

We could have also added a Broadwell-specific code there, but it's not really needed since we never disable LCPLL with the hotplug interrupts still enabled. So keep the easy-and-simple-to-maintain solution until we actually need something else.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_display.c +7 ~~-10~~

index 3fb3bd3..6691970 100644
@@ -6861,8 +6861,6 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
 	struct intel_crtc *crtc;
-	unsigned long irqflags;
-	uint32_t val;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
 		WARN(crtc->active, "CRTC for pipe %c enabled\n",

@@ -6883,14 +6881,13 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	     "Utility pin enabled\n");
 	WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");
 
-	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	val = I915_READ(DEIMR);
-	WARN((val | DE_PCH_EVENT_IVB) != 0xffffffff,
-	     "Unexpected DEIMR bits enabled: 0x%x\n", val);
-	val = I915_READ(SDEIMR);
-	WARN((val | SDE_HOTPLUG_MASK_CPT) != 0xffffffff,
-	     "Unexpected SDEIMR bits enabled: 0x%x\n", val);
-	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+	/*
+	 * In theory we can still leave IRQs enabled, as long as only the HPD
+	 * interrupts remain enabled. We used to check for that, but since it's
+	 * gen-specific and since we only disable LCPLL after we fully disable
+	 * the interrupts, the check below should be enough.
+	 */
+	WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
 }
 
 static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)

Signature · 1 line

1.8.5.3

Daniel Vetter

2014-04-02 07:28 UTC #2 lore raw

Re: [PATCH 1/3] drm/i915: only check for irqs_disabled when disabling LCPLL

Quoted 11 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Because if we keep the current code, we'll get tons of WARNs on Broadwell, since the code is Haswell-specific.
We could have also added a Broadwell-specific code there, but it's not really needed since we never disable LCPLL with the hotplug interrupts still enabled. So keep the easy-and-simple-to-maintain solution until we actually need something else.
Signed-off-by Paulo Zanoni <[email protected]>

I'll see how quickly QA reports this before merging it. If you spot the bug please reply with the Bugzilla: tag, thanks.
-Daniel

Quoted 46 lines

---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_display.c +7 ~~-10~~

index 3fb3bd3..6691970 100644
@@ -6861,8 +6861,6 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
 	struct intel_crtc *crtc;
-	unsigned long irqflags;
-	uint32_t val;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
 		WARN(crtc->active, "CRTC for pipe %c enabled\n",

@@ -6883,14 +6881,13 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	     "Utility pin enabled\n");
 	WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");
 
-	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	val = I915_READ(DEIMR);
-	WARN((val | DE_PCH_EVENT_IVB) != 0xffffffff,
-	     "Unexpected DEIMR bits enabled: 0x%x\n", val);
-	val = I915_READ(SDEIMR);
-	WARN((val | SDE_HOTPLUG_MASK_CPT) != 0xffffffff,
-	     "Unexpected SDEIMR bits enabled: 0x%x\n", val);
-	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+	/*
+	 * In theory we can still leave IRQs enabled, as long as only the HPD
+	 * interrupts remain enabled. We used to check for that, but since it's
+	 * gen-specific and since we only disable LCPLL after we fully disable
+	 * the interrupts, the check below should be enough.
+	 */
+	WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
 }
 
 static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)

Signature · 6 lines

1.8.5.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Paulo Zanoni

author

2014-04-01 22:40 UTC #3 lore raw

PATCH 2/3 drm/i915: Allow i915_pc8_status debug info on BDW

In reply to Paulo Zanoni

From: Zhenyu Wang <[email protected]>

Signed-off-by Zhenyu Wang <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_debugfs.c +1 -1

index 41da286..8d18455 100644
@@ -2058,7 +2058,7 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!IS_HASWELL(dev)) {
+	if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
 		seq_puts(m, "not supported\n");
 		return 0;
 	}

Signature · 1 line

1.8.5.3

Ben Widawsky

Reviewed-by on v1 2/3

2014-04-02 05:04 UTC #4 lore raw

Re: [PATCH 2/3] drm/i915: Allow i915_pc8_status debug info on BDW

Quoted 3 lines · Paulo Zanoni

From: Zhenyu Wang <[email protected]>
Signed-off-by Zhenyu Wang <[email protected]>

Reviewed-by Ben Widawsky <[email protected]>

Quoted 24 lines

---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_debugfs.c +1 -1

index 41da286..8d18455 100644
@@ -2058,7 +2058,7 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!IS_HASWELL(dev)) {
+	if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
 		seq_puts(m, "not supported\n");
 		return 0;
 	}

Signature · 6 lines

1.8.5.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Daniel Vetter

Applied

2014-04-02 07:27 UTC #5 lore raw

Re: [PATCH 2/3] drm/i915: Allow i915_pc8_status debug info on BDW

Quoted 6 lines · Ben Widawsky

Quoted 3 lines · Paulo Zanoni
From: Zhenyu Wang <[email protected]>
Signed-off-by Zhenyu Wang <[email protected]>
Reviewed-by Ben Widawsky <[email protected]>

Queued for -next, thanks for the patch.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Paulo Zanoni

author

2014-04-01 22:40 UTC #6 lore raw

PATCH 3/3 drm/i915: fix infinite loop at gen6_update_ring_freq

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

If I boot my Broadwell machine to X on a system with Mesa Gallium llvmpipe instead of i965, then kill X and try to run pm_pc8.c, when we disable PC8 and call gen6_update_ring_freq(), we will get stuck on an infinite loop because the frequencies are zero and the variables are unsigned. This happens because we never ran any batch, so we did not enable RC6, so the variables are zero. If I run gem_exec_nop before running pm_pc8, everything works as expected because gem_exec_nop makes RC6 be enabled.

This commit should prevent the infinite loop, which IMHO is already a good reason to be merged, but it is not the proper fix to the "RC6 is not being enabled" problem.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_pm.c +2 -1

index cebe0d4..0a0685b 100644
@@ -3487,7 +3487,8 @@ void gen6_update_ring_freq(struct drm_device *dev)
 	 * to use for memory access.  We do this by specifying the IA frequency
 	 * the PCU should use as a reference to determine the ring frequency.
 	 */
-	for (gpu_freq = dev_priv->rps.max_freq_softlimit; gpu_freq >= dev_priv->rps.min_freq_softlimit;
+	for (gpu_freq = dev_priv->rps.max_freq_softlimit;
+	     gpu_freq >= dev_priv->rps.min_freq_softlimit && gpu_freq != 0;
 	     gpu_freq--) {
 		int diff = dev_priv->rps.max_freq_softlimit - gpu_freq;
 		unsigned int ia_freq = 0, ring_freq = 0;

Signature · 1 line

1.8.5.3

Ben Widawsky

2014-04-02 05:03 UTC #7 lore raw

Re: [PATCH 3/3] drm/i915: fix infinite loop at gen6_update_ring_freq

Quoted 11 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
If I boot my Broadwell machine to X on a system with Mesa Gallium llvmpipe instead of i965, then kill X and try to run pm_pc8.c, when we disable PC8 and call gen6_update_ring_freq(), we will get stuck on an infinite loop because the frequencies are zero and the variables are unsigned. This happens because we never ran any batch, so we did not enable RC6, so the variables are zero. If I run gem_exec_nop before running pm_pc8, everything works as expected because gem_exec_nop makes RC6 be enabled.

Quoted 5 lines

This commit should prevent the infinite loop, which IMHO is already a good reason to be merged, but it is not the proper fix to the "RC6 is not being enabled" problem.
Signed-off-by Paulo Zanoni <[email protected]>

You should determine if it's possible to hit this without my patch. I convinced myself pretty thoroughly that it isn't possible to enter pc8 (and thus get to this point) without actually turning on RC6. One theory I had which I'm pretty sure I disproved is if BIOS leaves RC6 on, and we get into PC8 before the deferred rc6 workqueue runs. I'm pretty certain however that we forcibly disable rc6 first, so it can't happen.

drivers/gpu/drm/i915/intel_pm.c void gen6_update_ring_freq(struct drm_device *dev) · line 3489

Quoted 12 lines

---
 drivers/gpu/drm/i915/intel_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_pm.c +0 -0

index cebe0d4..0a0685b 100644
@@ -3487,7 +3487,8 @@ void gen6_update_ring_freq(struct drm_device *dev)
 	 * to use for memory access.  We do this by specifying the IA frequency
 	 * the PCU should use as a reference to determine the ring frequency.
 	 */

So I think it should really be (if anything at all):
if (WARN_ON(!dev_priv->rps.max_freq_softlimit))

	return;

Quoted 13 lines

drivers/gpu/drm/i915/intel_pm.c +2 -1

-	for (gpu_freq = dev_priv->rps.max_freq_softlimit; gpu_freq >= dev_priv->rps.min_freq_softlimit;
+	for (gpu_freq = dev_priv->rps.max_freq_softlimit;
+	     gpu_freq >= dev_priv->rps.min_freq_softlimit && gpu_freq != 0;
 	     gpu_freq--) {
 		int diff = dev_priv->rps.max_freq_softlimit - gpu_freq;
 		unsigned int ia_freq = 0, ring_freq = 0;

Signature · 6 lines

1.8.5.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Daniel Vetter

2014-04-02 07:26 UTC #8 lore raw

Re: [PATCH 3/3] drm/i915: fix infinite loop at gen6_update_ring_freq

In reply to Paulo Zanoni

Quoted 11 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
If I boot my Broadwell machine to X on a system with Mesa Gallium llvmpipe instead of i965, then kill X and try to run pm_pc8.c, when we disable PC8 and call gen6_update_ring_freq(), we will get stuck on an infinite loop because the frequencies are zero and the variables are unsigned. This happens because we never ran any batch, so we did not enable RC6, so the variables are zero. If I run gem_exec_nop before running pm_pc8, everything works as expected because gem_exec_nop makes RC6 be enabled.

Quoted 5 lines

This commit should prevent the infinite loop, which IMHO is already a good reason to be merged, but it is not the proper fix to the "RC6 is not being enabled" problem.
Signed-off-by Paulo Zanoni <[email protected]>

This will be fixed (I hope at least) once we have the in-kernel 3d state emission stuff.

But there's still a race when we go into runtime suspend right after coming out of system resume. I think we need to grab a runtime pm reference when we launch the delayed rps work and drop it once that has run. Or at least we need to have a flush_delayed_work in the runtime pm suspend function.

There's probably piles more stuff like this.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Daniel Vetter

2014-04-02 14:58 UTC #9 lore raw

In reply to Paulo Zanoni

Quoted 16 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Hi
I got a little surprised to see the BDW patches merged so fast, I thought I was going to have time to resend the last series with these last patches in.
The first patch replaces a HSW-specific WARN that was happening all the time on BDW with a gen-agnostic WARN. The second patch is just a debugfs fix. The third patch fixes an infinite loop, but it does not fix the undelying reason why the infinite loop happens, so maybe we don't want to merge it so we don't hide the real bug.
With these last patches on top of -nightly, my BDW machine passes pm_pc8 without errors.

Quoted 6 lines

Thanks,
Paulo
Paulo Zanoni (2):
drm/i915: only check for irqs_disabled when disabling LCPLL drm/i915: fix infinite loop at gen6_update_ring_freq

Apparently management isn't fully appeased yet, so I've pulled these two in, too.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch