[PATCH 0/7] Unclaimed registers and power well V2

[PATCH 0/7] Unclaimed registers and power well V2

[Paulo Zanoni]

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

Hi

So here's the second version of the patches that fix some "unclaimed register"
errors, making our driver good enough to start disabling the power well when
possible (we're still not 100% "unclaimed register" free, recent patches added
some new unclaimed register problems when the power well is disabled, but IMHO
we're good enough).

The changes are based on the reviews and also on some information provided by
the hardware engineers regarding the places where our specification was
confusing.

The patches that were previously patches 9 and 10 were removed, they will be
part of a future patch series only for them.

Thanks,
Paulo

Daniel Vetter (1):
  drm/i915: dynamic Haswell display power well support

Paulo Zanoni (6):
  drm/i915: don't send DP idle pattern before normal pattern on HSW
  drm/i915: fix intel_init_power_wells
  drm/i915: only disable enabled planes on intel_fb_restore_mode
  drm/i915: check the power down well on assert_pipe()
  drm/i915: turn on the power well before suspending
  drm/i915: set TRANSCODER_EDP even earlier

 drivers/gpu/drm/i915/i915_drv.c      |    2 ++
 drivers/gpu/drm/i915/i915_reg.h      |    8 ++---
 drivers/gpu/drm/i915/intel_ddi.c     |    8 ++++-
 drivers/gpu/drm/i915/intel_display.c |   58 +++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_dp.c      |    9 ------
 drivers/gpu/drm/i915/intel_drv.h     |    3 +-
 drivers/gpu/drm/i915/intel_fb.c      |    3 +-
 drivers/gpu/drm/i915/intel_pm.c      |   57 +++++++++++++++++++++++----------
 8 files changed, 103 insertions(+), 45 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] drm/i915: don't send DP idle pattern before normal pattern on HSW

[Paulo Zanoni]

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

Previously I sent "drm/i915: don't read DP_TP_STATUS(PORT_A)", but
after some more discussion I was told by a hardware engineer that we
don't really need to send the idle patterns before the normal pattern
in our current code: we only need this for a DP mode that we currently
don't support. So for now, just kill the whole code. I've already
asked for an update on the documentation, so at some point this code
should match the docs.

This solves "Timed out waiting for DP idle patterns" and "unclaimed
register" messages on eDP.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 51fd797..f2fa219 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1785,16 +1785,7 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 		temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
 		switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
 		case DP_TRAINING_PATTERN_DISABLE:
-			temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
-			I915_WRITE(DP_TP_CTL(port), temp);
-
-			if (wait_for((I915_READ(DP_TP_STATUS(port)) &
-				      DP_TP_STATUS_IDLE_DONE), 1))
-				DRM_ERROR("Timed out waiting for DP idle patterns\n");
-
-			temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
 			temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
-
 			break;
 		case DP_TRAINING_PATTERN_1:
 			temp |= DP_TP_CTL_LINK_TRAIN_PAT1;
-- 
1.7.10.4

[PATCH 2/7] drm/i915: fix intel_init_power_wells

[Paulo Zanoni]

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

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 <paulo.r.zanoni@intel.com>
---
 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(-)

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);
 }
-- 
1.7.10.4

[PATCH 3/7] drm/i915: dynamic Haswell display power well support

[Paulo Zanoni]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

We can disable (almost) all the display hw if we only use pipe A, with
the integrated edp transcoder on port A. Because we don't set the cpu
transcoder that early (yet), we need to help us with a trick to simply
check for any edp encoders.

And wrt the old code: Can anyone explain what that struct mutex
grabbing was supposed to protect?

v2: Paulo Zanoni pointed out that we also need to configure the eDP
cpu transcoder correctly.

v3: Made by Paulo Zanoni
  - Rebase patch to be on top of "fix intel_init_power_wells" patch
  - Fix typos
  - Fix a small bug by adding a "connectors_active" check
  - Restore the initial code that unconditionally enables the power
    well when taking over from the BIOS

v4: Made by Paulo Zanoni
  - One more typo spotted by Jani Nikula

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |    8 +++++++-
 drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_pm.c      |    2 +-
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 33b9112..5b54395 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -988,7 +988,13 @@ void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)
 	if (cpu_transcoder == TRANSCODER_EDP) {
 		switch (pipe) {
 		case PIPE_A:
-			temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
+			/* Can only use the always-on power well for eDP when
+			 * not using the panel fitter, and when not using motion
+			 * blur mitigation (which we don't support). */
+			if (dev_priv->pch_pf_size)
+				temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
+			else
+				temp |= TRANS_DDI_EDP_INPUT_A_ON;
 			break;
 		case PIPE_B:
 			temp |= TRANS_DDI_EDP_INPUT_B_ONOFF;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bfcc199..456da5c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5601,6 +5601,35 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	return fdi_config_ok ? ret : -EINVAL;
 }
 
+static void haswell_modeset_global_resources(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool enable = false;
+	struct intel_crtc *crtc;
+	struct intel_encoder *encoder;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
+		if (crtc->pipe != PIPE_A && crtc->base.enabled)
+			enable = true;
+		/* XXX: Should check for edp transcoder here, but thanks to init
+		 * sequence that's not yet available. Just in case desktop eDP
+		 * on PORT D is possible on haswell, too. */
+	}
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (encoder->type != INTEL_OUTPUT_EDP &&
+		    encoder->connectors_active)
+			enable = true;
+	}
+
+	/* Even the eDP panel fitter is outside the always-on well. */
+	if (dev_priv->pch_pf_size)
+		enable = true;
+
+	intel_set_power_well(dev, enable);
+}
+
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 				 struct drm_display_mode *mode,
 				 struct drm_display_mode *adjusted_mode,
@@ -8485,6 +8514,8 @@ static void intel_init_display(struct drm_device *dev)
 		} else if (IS_HASWELL(dev)) {
 			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 			dev_priv->display.write_eld = haswell_write_eld;
+			dev_priv->display.modeset_global_resources =
+				haswell_modeset_global_resources;
 		}
 	} else if (IS_G4X(dev)) {
 		dev_priv->display.write_eld = g4x_write_eld;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 32c3042..fcdfe42 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -668,6 +668,7 @@ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
 extern void intel_init_power_well(struct drm_device *dev);
+extern void intel_set_power_well(struct drm_device *dev, bool enable);
 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 b8cf16c..c97714e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4043,7 +4043,7 @@ void intel_init_clock_gating(struct drm_device *dev)
 	dev_priv->display.init_clock_gating(dev);
 }
 
-static void intel_set_power_well(struct drm_device *dev, bool enable)
+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;
-- 
1.7.10.4

[PATCH 4/7] drm/i915: only disable enabled planes on intel_fb_restore_mode

[Paulo Zanoni]

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

We should avoid touching registers that are on the power down well
when we don't need to, because if we touch these registers when the
power well is disabled we'll get tons of "unclaimed register"
messages. This commit fixes some of these messages.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fb.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index ce5f544..6591029 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -304,7 +304,8 @@ void intel_fb_restore_mode(struct drm_device *dev)
 
 	/* Be sure to shut off any planes that may be active */
 	list_for_each_entry(plane, &config->plane_list, head)
-		plane->funcs->disable_plane(plane);
+		if (plane->enabled)
+			plane->funcs->disable_plane(plane);
 
 	mutex_unlock(&dev->mode_config.mutex);
 }
-- 
1.7.10.4

[PATCH 5/7] drm/i915: check the power down well on assert_pipe()

[Paulo Zanoni]

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

If the power well is disabled, we should not try to read its
registers, otherwise we'll get "unclaimed register" messages.

V2: Don't check whether the power well is enabled or not, just check
whether we asked it to be enabled or not: if we asked to disable the
power well, don't use the registers on it, even if it's still enabled.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 456da5c..022c59d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
 		state = true;
 
-	reg = PIPECONF(cpu_transcoder);
-	val = I915_READ(reg);
-	cur_state = !!(val & PIPECONF_ENABLE);
+	if (cpu_transcoder == TRANSCODER_EDP ||
+	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
+		reg = PIPECONF(cpu_transcoder);
+		val = I915_READ(reg);
+		cur_state = !!(val & PIPECONF_ENABLE);
+	} else {
+		cur_state = false;
+	}
+
 	WARN(cur_state != state,
 	     "pipe %c assertion failure (expected %s, current %s)\n",
 	     pipe_name(pipe), state_string(state), state_string(cur_state));
-- 
1.7.10.4

[PATCH 6/7] drm/i915: turn on the power well before suspending

[Paulo Zanoni]

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

Our suspend code touches a lot of registers all over the place, so we
need to enable the power well before suspending.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 521a253..9cc8f87 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -470,6 +470,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	intel_set_power_well(dev, true);
+
 	drm_kms_helper_poll_disable(dev);
 
 	pci_save_state(dev->pdev);
-- 
1.7.10.4

[PATCH 7/7] drm/i915: set TRANSCODER_EDP even earlier

[Paulo Zanoni]

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

Instead of setting it at the beginning of haswell_crtc_mode_set, let's
set it at the beginning of intel_crtc_mode_set. When
intel_crt_mode_set calls drm_vblank_pre_modeset we already need to
have the transcoder_edp correctly set, because eventually
drm_vblank_pre_modeset calls functions that call i915_pipe_enabled
from i915_irq.c, which will read PIPECONF(cpu_transcoder).

This is a bug that affects us since we added support for
TRANSCODER_EDP, but I was only able to see the problem after
suspending a machine with the power well disabled (got an "unclaimed
register" error.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 022c59d..dfe8cd7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5668,11 +5668,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	if (is_cpu_edp)
-		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
-	else
-		intel_crtc->cpu_transcoder = pipe;
-
 	/* We are not sure yet this won't happen. */
 	WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
 	     INTEL_PCH_TYPE(dev));
@@ -5737,6 +5732,11 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 	int pipe = intel_crtc->pipe;
 	int ret;
 
+	if (IS_HASWELL(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))
+		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
+	else
+		intel_crtc->cpu_transcoder = pipe;
+
 	drm_vblank_pre_modeset(dev, pipe);
 
 	ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
-- 
1.7.10.4

Re: [PATCH 1/7] drm/i915: don't send DP idle pattern before normal pattern on HSW

[Daniel Vetter]

On Fri, Jan 25, 2013 at 04:59:10PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Previously I sent "drm/i915: don't read DP_TP_STATUS(PORT_A)", but
> after some more discussion I was told by a hardware engineer that we
> don't really need to send the idle patterns before the normal pattern
> in our current code: we only need this for a DP mode that we currently
> don't support. So for now, just kill the whole code. I've already
> asked for an update on the documentation, so at some point this code
> should match the docs.
> 
> This solves "Timed out waiting for DP idle patterns" and "unclaimed
> register" messages on eDP.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Since we already have the code, and it should be harmless to run this on
DP ports B-D, can't we just disable this on port A? I fear that if we ever
get around to enable that funky other DP mode, we'll miss this and suffer
through some ugly bugs ...
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c |    9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 51fd797..f2fa219 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1785,16 +1785,7 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>  		temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
>  		switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
>  		case DP_TRAINING_PATTERN_DISABLE:
> -			temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
> -			I915_WRITE(DP_TP_CTL(port), temp);
> -
> -			if (wait_for((I915_READ(DP_TP_STATUS(port)) &
> -				      DP_TP_STATUS_IDLE_DONE), 1))
> -				DRM_ERROR("Timed out waiting for DP idle patterns\n");
> -
> -			temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
>  			temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
> -
>  			break;
>  		case DP_TRAINING_PATTERN_1:
>  			temp |= DP_TP_CTL_LINK_TRAIN_PAT1;
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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 2/7] drm/i915: fix intel_init_power_wells

[Daniel Vetter]

On Fri, Jan 25, 2013 at 04:59:11PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

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

Re: [PATCH 7/7] drm/i915: set TRANSCODER_EDP even earlier

[Daniel Vetter]

On Fri, Jan 25, 2013 at 04:59:16PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Instead of setting it at the beginning of haswell_crtc_mode_set, let's
> set it at the beginning of intel_crtc_mode_set. When
> intel_crt_mode_set calls drm_vblank_pre_modeset we already need to
> have the transcoder_edp correctly set, because eventually
> drm_vblank_pre_modeset calls functions that call i915_pipe_enabled
> from i915_irq.c, which will read PIPECONF(cpu_transcoder).
> 
> This is a bug that affects us since we added support for
> TRANSCODER_EDP, but I was only able to see the problem after
> suspending a machine with the power well disabled (got an "unclaimed
> register" error.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

I've picked up patches 4-7 from this series. I'm leaving patch 3 out for
now until that entire "unclaimed register write business" with the dynamic
power down well code is settled.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/7] drm/i915: check the power down well on assert_pipe()

[Daniel Vetter]

On Fri, Jan 25, 2013 at 04:59:14PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If the power well is disabled, we should not try to read its
> registers, otherwise we'll get "unclaimed register" messages.
> 
> V2: Don't check whether the power well is enabled or not, just check
> whether we asked it to be enabled or not: if we asked to disable the
> power well, don't use the registers on it, even if it's still enabled.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 456da5c..022c59d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>  	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>  		state = true;
>  
> -	reg = PIPECONF(cpu_transcoder);
> -	val = I915_READ(reg);
> -	cur_state = !!(val & PIPECONF_ENABLE);
> +	if (cpu_transcoder == TRANSCODER_EDP ||
> +	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
> +		reg = PIPECONF(cpu_transcoder);
> +		val = I915_READ(reg);
> +		cur_state = !!(val & PIPECONF_ENABLE);
> +	} else {
> +		cur_state = false;
> +	}

This blows up on !hsw in big ways, since it makes the paranoid modeset
state checker really unhappy. I've dropped the patch for now again from
dinq. Thanks to Sedat Dilek for reporting.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch