[PATCH 0/3] BDW runtime PM fixes

[PATCH 0/3] BDW runtime PM fixes

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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.

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(-)

-- 
1.8.5.3

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3fb3bd3..6691970 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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)
-- 
1.8.5.3

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

[Daniel Vetter]

On Tue, Apr 01, 2014 at 07:39:47PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3fb3bd3..6691970 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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)
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Paulo Zanoni]

From: Zhenyu Wang <zhenyuw@linux.intel.com>

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 41da286..8d18455 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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;
 	}
-- 
1.8.5.3

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

[Ben Widawsky]

On Tue, Apr 01, 2014 at 07:39:48PM -0300, Paulo Zanoni wrote:
> From: Zhenyu Wang <zhenyuw@linux.intel.com>
> 
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 41da286..8d18455 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -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;
>  	}
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Daniel Vetter]

On Tue, Apr 01, 2014 at 10:04:15PM -0700, Ben Widawsky wrote:
> On Tue, Apr 01, 2014 at 07:39:48PM -0300, Paulo Zanoni wrote:
> > From: Zhenyu Wang <zhenyuw@linux.intel.com>
> > 
> > Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

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

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cebe0d4..0a0685b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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;
-- 
1.8.5.3

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

[Ben Widawsky]

On Tue, Apr 01, 2014 at 07:39:49PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index cebe0d4..0a0685b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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;

> -	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;
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Daniel Vetter]

On Tue, Apr 01, 2014 at 07:39:49PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/3] BDW runtime PM fixes

[Daniel Vetter]

On Tue, Apr 01, 2014 at 07:39:46PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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.
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch