[2/7] drm/i915: fix intel_init_power_wells

Commit Message

Paulo Zanoni Jan. 25, 2013, 6:59 p.m. UTC

From: Paulo Zanoni <[e-mail]>

The current code was wrong in many different ways, so this is a full
rewrite. We don't have "different power wells for different parts of
the GPU", we have a single power well, but we have multiple registers
that can be used to request enabling/disabling the power well. So
let's be a good citizen and only use the register we're suppose to
use, except when we're loading the driver, where we clear the request
made by the BIOS.

If any of the registers is requesting the power well to be enabled, it
will be enabled. If none of the registers is requesting the power well
to be enabled, it will be disabled.

For now we're just forcing the power well to be enabled, but in the
next commits we'll change this.

V2:
  - Remove debug messages that could be misleading due to possible
    race conditions with KVMr, Debug and BIOS.
  - Don't wait on disabling: after a conversaion with a hardware
    engineer we discovered that the "restriction" on bit 31 is just
    for the "enable" case, and we don't even need to wait on the
    "disable" case.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 drivers/gpu/drm/i915/i915_reg.h      |    8 ++---
 drivers/gpu/drm/i915/intel_display.c |    5 +--
 drivers/gpu/drm/i915/intel_drv.h     |    2 +-
 drivers/gpu/drm/i915/intel_pm.c      |   57 ++++++++++++++++++++++++----------
 4 files changed, 46 insertions(+), 26 deletions(-)

Comments

Daniel Vetter Jan. 26, 2013, 4:54 p.m. UTC | #1

On Fri, Jan 25, 2013 at 04:59:11PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> The current code was wrong in many different ways, so this is a full
> rewrite. We don't have "different power wells for different parts of
> the GPU", we have a single power well, but we have multiple registers
> that can be used to request enabling/disabling the power well. So
> let's be a good citizen and only use the register we're suppose to
> use, except when we're loading the driver, where we clear the request
> made by the BIOS.
> 
> If any of the registers is requesting the power well to be enabled, it
> will be enabled. If none of the registers is requesting the power well
> to be enabled, it will be disabled.
> 
> For now we're just forcing the power well to be enabled, but in the
> next commits we'll change this.
> 
> V2:
>   - Remove debug messages that could be misleading due to possible
>     race conditions with KVMr, Debug and BIOS.
>   - Don't wait on disabling: after a conversaion with a hardware
>     engineer we discovered that the "restriction" on bit 31 is just
>     for the "enable" case, and we don't even need to wait on the
>     "disable" case.
> 
> Signed-off-by: Paulo Zanoni <[e-mail]>

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

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 94a08b9..15c15a5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4416,10 +4416,10 @@ 
 #define   AUDIO_CP_READY_C		(1<<9)
 
 /* HSW Power Wells */
-#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
-#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
-#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
-#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
+#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
+#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_CTL5			0x45410
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 886124a..bfcc199 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8664,10 +8664,7 @@  static void i915_disable_vga(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
-	/* We attempt to init the necessary power wells early in the initialization
-	 * time, so the subsystems that expect power to be enabled can work.
-	 */
-	intel_init_power_wells(dev);
+	intel_init_power_well(dev);
 
 	intel_prepare_ddi(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 66619d8..32c3042 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -667,7 +667,7 @@  extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
-extern void intel_init_power_wells(struct drm_device *dev);
+extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
 extern void intel_disable_gt_powersave(struct drm_device *dev);
 extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5a8a72c..b8cf16c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4043,33 +4043,56 @@  void intel_init_clock_gating(struct drm_device *dev)
 	dev_priv->display.init_clock_gating(dev);
 }
 
-/* Starting with Haswell, we have different power wells for
- * different parts of the GPU. This attempts to enable them all.
+static void intel_set_power_well(struct drm_device *dev, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool is_enabled, enable_requested;
+	uint32_t tmp;
+
+	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
+	is_enabled = tmp & HSW_PWR_WELL_STATE;
+	enable_requested = tmp & HSW_PWR_WELL_ENABLE;
+
+	if (enable) {
+		if (!enable_requested)
+			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
+
+		if (!is_enabled) {
+			DRM_DEBUG_KMS("Enabling power well\n");
+			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
+				      HSW_PWR_WELL_STATE), 20))
+				DRM_ERROR("Timeout enabling power well\n");
+		}
+	} else {
+		if (enable_requested) {
+			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
+			DRM_DEBUG_KMS("Requesting to disable the power well\n");
+		}
+	}
+}
+
+/*
+ * Starting with Haswell, we have a "Power Down Well" that can be turned off
+ * when not needed anymore. We have 4 registers that can request the power well
+ * to be enabled, and it will only be disabled if none of the registers is
+ * requesting it to be enabled.
  */
-void intel_init_power_wells(struct drm_device *dev)
+void intel_init_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long power_wells[] = {
-		HSW_PWR_WELL_CTL1,
-		HSW_PWR_WELL_CTL2,
-		HSW_PWR_WELL_CTL4
-	};
-	int i;
 
 	if (!IS_HASWELL(dev))
 		return;
 
 	mutex_lock(&dev->struct_mutex);
 
-	for (i = 0; i < ARRAY_SIZE(power_wells); i++) {
-		int well = I915_READ(power_wells[i]);
+	/* For now, we need the power well to be always enabled. */
+	intel_set_power_well(dev, true);
 
-		if ((well & HSW_PWR_WELL_STATE) == 0) {
-			I915_WRITE(power_wells[i], well & HSW_PWR_WELL_ENABLE);
-			if (wait_for((I915_READ(power_wells[i]) & HSW_PWR_WELL_STATE), 20))
-				DRM_ERROR("Error enabling power well %lx\n", power_wells[i]);
-		}
-	}
+	/* 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)
+		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 
 	mutex_unlock(&dev->struct_mutex);
 }