drm/i915: check the power well when redisabling VGA

PATCH In review Paulo Zanoni 2 participants 4 messages Aug 2, 2013 – Aug 14, 2013

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2013-08-02 19:22 UTC #0 lore raw

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

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_display.c +11 -0

index e1e50df..3765d0b 100644
@@ -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);

Signature · 1 line

1.8.1.2

Paulo Zanoni

author

2013-08-02 19:22 UTC #1 lore raw

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

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

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

drivers/gpu/drm/i915/i915_reg.h +2 -2

index 19941c6..9c4140d 100644
@@ -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)

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

index 3765d0b..0b7796d 100644
@@ -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) {

drivers/gpu/drm/i915/intel_pm.c +7 -6

index 0a5ba92..44c0030 100644
@@ -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);
 }

Signature · 1 line

1.8.1.2

Daniel Vetter

2013-08-05 15:46 UTC #2 lore raw

In reply to Paulo Zanoni

Quoted 8 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
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 <[email protected]>

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

Quoted 33 lines

---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/intel_display.c +11 -0

index e1e50df..3765d0b 100644
@@ -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);

Signature · 6 lines

1.8.1.2

_______________________________________________
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

Daniel Vetter

2013-08-14 14:50 UTC #3 lore raw

Quoted 7 lines · Daniel Vetter

Quoted 6 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If the power well is disabled VGA is guaranteed to be disabled.
This fixes unclaimed register messages that happen on suspend/resume.

Quoted 7 lines

Quoted 2 lines
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67517
Signed-off-by Paulo Zanoni <[email protected]>
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

Signature · 3 lines

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