[PATCH 1/2] drm/i915: check the power well when redisabling VGA

[PATCH 1/2] drm/i915: check the power well when redisabling VGA

[Paulo Zanoni]

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

If the power well is disabled VGA is guaranteed to be disabled.

This fixes unclaimed register messages that happen on suspend/resume.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67517
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1e50df..3765d0b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10097,6 +10097,17 @@ void i915_redisable_vga(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 vga_reg = i915_vgacntrl_reg(dev);
 
+	/* This function can be called both from intel_modeset_setup_hw_state or
+	 * at a very early point in our resume sequence, where the power well
+	 * structures are not yet restored. Since this function is at a very
+	 * paranoid "someone might have enabled VGA while we were not looking"
+	 * level, just check if the power well is enabled instead of trying to
+	 * follow the "don't touch the power well if we don't need it" policy
+	 * the rest of the driver uses. */
+	if (HAS_POWER_WELL(dev) &&
+	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE) == 0)
+		return;
+
 	if (I915_READ(vga_reg) != VGA_DISP_DISABLE) {
 		DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n");
 		i915_disable_vga(dev);
-- 
1.8.1.2

[PATCH 2/2] drm/i915: clarify Haswell power well bit names

[Paulo Zanoni]

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

Whenever I need to work with the HSW_PWER_WELL_* register bits I have
to look at the documentation to find out which bit is to request the
power well and which one shows its current state. Rename the bits so I
won't need to look the docs every time.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  4 ++--
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 13 +++++++------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 19941c6..9c4140d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4840,8 +4840,8 @@
 #define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
 #define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
 #define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
-#define   HSW_PWR_WELL_ENABLE			(1<<31)
-#define   HSW_PWR_WELL_STATE			(1<<30)
+#define   HSW_PWR_WELL_ENABLE_REQUEST		(1<<31)
+#define   HSW_PWR_WELL_STATE_ENABLED		(1<<30)
 #define HSW_PWR_WELL_CTL5			0x45410
 #define   HSW_PWR_WELL_ENABLE_SINGLE_STEP	(1<<31)
 #define   HSW_PWR_WELL_PWR_GATE_OVERRIDE	(1<<20)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3765d0b..0b7796d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10105,7 +10105,7 @@ void i915_redisable_vga(struct drm_device *dev)
 	 * follow the "don't touch the power well if we don't need it" policy
 	 * the rest of the driver uses. */
 	if (HAS_POWER_WELL(dev) &&
-	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE) == 0)
+	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE_ENABLED) == 0)
 		return;
 
 	if (I915_READ(vga_reg) != VGA_DISP_DISABLE) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0a5ba92..44c0030 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5030,7 +5030,7 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	case POWER_DOMAIN_TRANSCODER_B:
 	case POWER_DOMAIN_TRANSCODER_C:
 		return I915_READ(HSW_PWR_WELL_DRIVER) ==
-		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
+		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
 	default:
 		BUG();
 	}
@@ -5043,17 +5043,18 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 	uint32_t tmp;
 
 	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
-	is_enabled = tmp & HSW_PWR_WELL_STATE;
-	enable_requested = tmp & HSW_PWR_WELL_ENABLE;
+	is_enabled = tmp & HSW_PWR_WELL_STATE_ENABLED;
+	enable_requested = tmp & HSW_PWR_WELL_ENABLE_REQUEST;
 
 	if (enable) {
 		if (!enable_requested)
-			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
+			I915_WRITE(HSW_PWR_WELL_DRIVER,
+				   HSW_PWR_WELL_ENABLE_REQUEST);
 
 		if (!is_enabled) {
 			DRM_DEBUG_KMS("Enabling power well\n");
 			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
-				      HSW_PWR_WELL_STATE), 20))
+				      HSW_PWR_WELL_STATE_ENABLED), 20))
 				DRM_ERROR("Timeout enabling power well\n");
 		}
 	} else {
@@ -5155,7 +5156,7 @@ void intel_init_power_well(struct drm_device *dev)
 
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
-	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
+	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
 		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
 
-- 
1.8.1.2

Re: [PATCH 1/2] drm/i915: check the power well when redisabling VGA

[Daniel Vetter]

On Fri, Aug 02, 2013 at 04:22:24PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If the power well is disabled VGA is guaranteed to be disabled.
> 
> This fixes unclaimed register messages that happen on suspend/resume.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67517
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Have you seen my other comment asking whether we could just force-enable
the power well a bit earlier in the resume sequence instead? Imo that
feels a notch more robust, since we didn't sprinkle tons of power well
checks in all the other hw readout paths either.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e1e50df..3765d0b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10097,6 +10097,17 @@ void i915_redisable_vga(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 vga_reg = i915_vgacntrl_reg(dev);
>  
> +	/* This function can be called both from intel_modeset_setup_hw_state or
> +	 * at a very early point in our resume sequence, where the power well
> +	 * structures are not yet restored. Since this function is at a very
> +	 * paranoid "someone might have enabled VGA while we were not looking"
> +	 * level, just check if the power well is enabled instead of trying to
> +	 * follow the "don't touch the power well if we don't need it" policy
> +	 * the rest of the driver uses. */
> +	if (HAS_POWER_WELL(dev) &&
> +	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE) == 0)
> +		return;
> +
>  	if (I915_READ(vga_reg) != VGA_DISP_DISABLE) {
>  		DRM_DEBUG_KMS("Something enabled VGA plane, disabling it\n");
>  		i915_disable_vga(dev);
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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

Re: [PATCH 1/2] drm/i915: check the power well when redisabling VGA

[Daniel Vetter]

On Mon, Aug 05, 2013 at 05:46:12PM +0200, Daniel Vetter wrote:
> On Fri, Aug 02, 2013 at 04:22:24PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > If the power well is disabled VGA is guaranteed to be disabled.
> > 
> > This fixes unclaimed register messages that happen on suspend/resume.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67517
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Have you seen my other comment asking whether we could just force-enable
> the power well a bit earlier in the resume sequence instead? Imo that
> feels a notch more robust, since we didn't sprinkle tons of power well
> checks in all the other hw readout paths either.

Ok, I've merged this one here (plus comments and the follow-up patch)
instead of the other one.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch