[PATCH 0/7] drm/i915: Fix PCH eDP support for SNB

[PATCH 0/7] drm/i915: Fix PCH eDP support for SNB

[Keith Packard]

Here's a patch sequence which makes my PCH-connected eDP panel
work. The main bug was a pile of places where the driver was
incorrectly treating a PCH connected eDP panel like a CPU connected
eDP panel, setting incorrect bits in the DP_CTL register and failing
to configure the TRANS_DP_CTL register entirely.

Beyond that, this eDP panel appears very sensitive to panel power
sequencing, and I found a bunch of minor errors there. I switched from
using blind timings to polling the panel power sequencing status
register to make sure we waited until that thought things were done,
and so that any panel power sequencing errors would show up in the
kernel log.

Finally, I noticed that the BIOS tried harder to get the link trained,
by simply starting over when it failed and trying the whole sequence
up to 5 times. This is not part of the DP spec, but given how bad
failing to train a panel is, it seems like it might be a good idea.

The three most interesting patches are the one which handles PCH eDP
more like PCH DP, the one which switches to using the panel power
sequencing hardware for all delays and finally the patch which tries
to do the panel power-up/down in the same order for both
dp_prepare/commit and dp_dpms.

All of these patches are on my pch-edp-fixes branch at

	git://people.freedesktop.org/~keithp/linux

If you've got a PCH connected eDP display, I'd love to know if this
makes it work. If you've got a CPU connected eDP display or PCH
connected DP, please see if this causes any problems.

(I've got several machines failing to resume with this patch, but I've
checked and it's not the fault of anything in the i915 directory;
applying this sequence to v3.1 makes suspend/resume work fine).

[PATCH 1/7] drm/i915: Move common PCH_PP_CONTROL setup to ironlake_get_pp_control

[Keith Packard]

Every usage of PCH_PP_CONTROL sets the PANEL_UNLOCK_REGS value to
ensure that writes will be respected, move this to a common function
to make the driver cleaner.

No functional changes.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   37 +++++++++++++++++++------------------
 1 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7259034..efe5f9e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -906,6 +906,19 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
 	msleep(delay);
 }
 
+/* Read the current pp_control value, unlocking the register if it
+ * is locked
+ */
+
+static  u32 ironlake_get_pp_control(struct drm_i915_private *dev_priv)
+{
+	u32	control = I915_READ(PCH_PP_CONTROL);
+
+	control &= ~PANEL_UNLOCK_MASK;
+	control |= PANEL_UNLOCK_REGS;
+	return control;
+}
+
 static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
@@ -926,9 +939,7 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	}
 
 	ironlake_wait_panel_off(intel_dp);
-	pp = I915_READ(PCH_PP_CONTROL);
-	pp &= ~PANEL_UNLOCK_MASK;
-	pp |= PANEL_UNLOCK_REGS;
+	pp = ironlake_get_pp_control(dev_priv);
 	pp |= EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
@@ -951,9 +962,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
 	u32 pp;
 
 	if (!intel_dp->want_panel_vdd && ironlake_edp_have_panel_vdd(intel_dp)) {
-		pp = I915_READ(PCH_PP_CONTROL);
-		pp &= ~PANEL_UNLOCK_MASK;
-		pp |= PANEL_UNLOCK_REGS;
+		pp = ironlake_get_pp_control(dev_priv);
 		pp &= ~EDP_FORCE_VDD;
 		I915_WRITE(PCH_PP_CONTROL, pp);
 		POSTING_READ(PCH_PP_CONTROL);
@@ -1012,9 +1021,7 @@ static void ironlake_edp_panel_on(struct intel_dp *intel_dp)
 		return;
 
 	ironlake_wait_panel_off(intel_dp);
-	pp = I915_READ(PCH_PP_CONTROL);
-	pp &= ~PANEL_UNLOCK_MASK;
-	pp |= PANEL_UNLOCK_REGS;
+	pp = ironlake_get_pp_control(dev_priv);
 
 	if (IS_GEN5(dev)) {
 		/* ILK workaround: disable reset around power sequence */
@@ -1049,9 +1056,7 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 
 	if (!is_edp(intel_dp))
 		return;
-	pp = I915_READ(PCH_PP_CONTROL);
-	pp &= ~PANEL_UNLOCK_MASK;
-	pp |= PANEL_UNLOCK_REGS;
+	pp = ironlake_get_pp_control(dev_priv);
 
 	if (IS_GEN5(dev)) {
 		/* ILK workaround: disable reset around power sequence */
@@ -1098,9 +1103,7 @@ static void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
 	 * allowing it to appear.
 	 */
 	msleep(intel_dp->backlight_on_delay);
-	pp = I915_READ(PCH_PP_CONTROL);
-	pp &= ~PANEL_UNLOCK_MASK;
-	pp |= PANEL_UNLOCK_REGS;
+	pp = ironlake_get_pp_control(dev_priv);
 	pp |= EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
@@ -1116,9 +1119,7 @@ static void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
 		return;
 
 	DRM_DEBUG_KMS("\n");
-	pp = I915_READ(PCH_PP_CONTROL);
-	pp &= ~PANEL_UNLOCK_MASK;
-	pp |= PANEL_UNLOCK_REGS;
+	pp = ironlake_get_pp_control(dev_priv);
 	pp &= ~EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
-- 
1.7.7

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Move common PCH_PP_CONTROL setup to ironlake_get_pp_control

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On Tue,  1 Nov 2011 23:20:24 -0700
Keith Packard <keithp@keithp.com> wrote:

> Every usage of PCH_PP_CONTROL sets the PANEL_UNLOCK_REGS value to
> ensure that writes will be respected, move this to a common function
> to make the driver cleaner.
> 
> No functional changes.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   37 +++++++++++++++++++------------------
>  1 files changed, 19 insertions(+), 18 deletions(-)

Can't we just set UNLOCK_REGS at load time and have asserts sprinkled
here and there?

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 1/7] drm/i915: Move common PCH_PP_CONTROL setup to ironlake_get_pp_control

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]

On Wed, 2 Nov 2011 09:02:55 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Can't we just set UNLOCK_REGS at load time and have asserts sprinkled
> here and there?

I think we'd need it at resume time as well; it seemed easier to just
set it every time we touch the register.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

[PATCH 2/7] drm/i915: Remove link_status field from intel_dp structure

[Keith Packard]

No persistent data was ever stored here, so link_status is instead
allocated on the stack as needed.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   65 +++++++++++++++++++++-----------------
 1 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index efe5f9e..2c0c482 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -58,7 +58,6 @@ struct intel_dp {
 	struct i2c_algo_dp_aux_data algo;
 	bool is_pch_edp;
 	uint8_t	train_set[4];
-	uint8_t link_status[DP_LINK_STATUS_SIZE];
 	int panel_power_up_delay;
 	int panel_power_down_delay;
 	int panel_power_cycle_delay;
@@ -1285,11 +1284,11 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
  * link status information
  */
 static bool
-intel_dp_get_link_status(struct intel_dp *intel_dp)
+intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
 	return intel_dp_aux_native_read_retry(intel_dp,
 					      DP_LANE0_1_STATUS,
-					      intel_dp->link_status,
+					      link_status,
 					      DP_LINK_STATUS_SIZE);
 }
 
@@ -1301,27 +1300,25 @@ intel_dp_link_status(uint8_t link_status[DP_LINK_STATUS_SIZE],
 }
 
 static uint8_t
-intel_get_adjust_request_voltage(uint8_t link_status[DP_LINK_STATUS_SIZE],
+intel_get_adjust_request_voltage(uint8_t adjust_request[2],
 				 int lane)
 {
-	int	    i = DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1);
 	int	    s = ((lane & 1) ?
 			 DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT :
 			 DP_ADJUST_VOLTAGE_SWING_LANE0_SHIFT);
-	uint8_t l = intel_dp_link_status(link_status, i);
+	uint8_t l = adjust_request[lane>>1];
 
 	return ((l >> s) & 3) << DP_TRAIN_VOLTAGE_SWING_SHIFT;
 }
 
 static uint8_t
-intel_get_adjust_request_pre_emphasis(uint8_t link_status[DP_LINK_STATUS_SIZE],
+intel_get_adjust_request_pre_emphasis(uint8_t adjust_request[2],
 				      int lane)
 {
-	int	    i = DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1);
 	int	    s = ((lane & 1) ?
 			 DP_ADJUST_PRE_EMPHASIS_LANE1_SHIFT :
 			 DP_ADJUST_PRE_EMPHASIS_LANE0_SHIFT);
-	uint8_t l = intel_dp_link_status(link_status, i);
+	uint8_t l = adjust_request[lane>>1];
 
 	return ((l >> s) & 3) << DP_TRAIN_PRE_EMPHASIS_SHIFT;
 }
@@ -1362,15 +1359,18 @@ intel_dp_pre_emphasis_max(uint8_t voltage_swing)
 }
 
 static void
-intel_get_adjust_train(struct intel_dp *intel_dp)
+intel_get_adjust_train(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
+	struct drm_device *dev = intel_dp->base.base.dev;
 	uint8_t v = 0;
 	uint8_t p = 0;
 	int lane;
+	uint8_t	*adjust_request = link_status + (DP_ADJUST_REQUEST_LANE0_1 - DP_LANE0_1_STATUS);
+	int voltage_max;
 
 	for (lane = 0; lane < intel_dp->lane_count; lane++) {
-		uint8_t this_v = intel_get_adjust_request_voltage(intel_dp->link_status, lane);
-		uint8_t this_p = intel_get_adjust_request_pre_emphasis(intel_dp->link_status, lane);
+		uint8_t this_v = intel_get_adjust_request_voltage(adjust_request, lane);
+		uint8_t this_p = intel_get_adjust_request_pre_emphasis(adjust_request, lane);
 
 		if (this_v > v)
 			v = this_v;
@@ -1389,7 +1389,7 @@ intel_get_adjust_train(struct intel_dp *intel_dp)
 }
 
 static uint32_t
-intel_dp_signal_levels(uint8_t train_set, int lane_count)
+intel_dp_signal_levels(uint8_t train_set)
 {
 	uint32_t	signal_levels = 0;
 
@@ -1458,9 +1458,8 @@ static uint8_t
 intel_get_lane_status(uint8_t link_status[DP_LINK_STATUS_SIZE],
 		      int lane)
 {
-	int i = DP_LANE0_1_STATUS + (lane >> 1);
 	int s = (lane & 1) * 4;
-	uint8_t l = intel_dp_link_status(link_status, i);
+	uint8_t l = link_status[lane>>1];
 
 	return (l >> s) & 0xf;
 }
@@ -1485,18 +1484,18 @@ intel_clock_recovery_ok(uint8_t link_status[DP_LINK_STATUS_SIZE], int lane_count
 			 DP_LANE_CHANNEL_EQ_DONE|\
 			 DP_LANE_SYMBOL_LOCKED)
 static bool
-intel_channel_eq_ok(struct intel_dp *intel_dp)
+intel_channel_eq_ok(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
 	uint8_t lane_align;
 	uint8_t lane_status;
 	int lane;
 
-	lane_align = intel_dp_link_status(intel_dp->link_status,
+	lane_align = intel_dp_link_status(link_status,
 					  DP_LANE_ALIGN_STATUS_UPDATED);
 	if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0)
 		return false;
 	for (lane = 0; lane < intel_dp->lane_count; lane++) {
-		lane_status = intel_get_lane_status(intel_dp->link_status, lane);
+		lane_status = intel_get_lane_status(link_status, lane);
 		if ((lane_status & CHANNEL_EQ_BITS) != CHANNEL_EQ_BITS)
 			return false;
 	}
@@ -1569,12 +1568,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 	clock_recovery = false;
 	for (;;) {
 		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
+		uint8_t	    link_status[DP_LINK_STATUS_SIZE];
 		uint32_t    signal_levels;
 		if (IS_GEN6(dev) && is_edp(intel_dp)) {
 			signal_levels = intel_gen6_edp_signal_levels(intel_dp->train_set[0]);
 			DP = (DP & ~EDP_LINK_TRAIN_VOL_EMP_MASK_SNB) | signal_levels;
 		} else {
-			signal_levels = intel_dp_signal_levels(intel_dp->train_set[0], intel_dp->lane_count);
+			signal_levels = intel_dp_signal_levels(intel_dp->train_set[0]);
+			DRM_DEBUG_KMS("training pattern 1 signal levels %08x\n", signal_levels);
 			DP = (DP & ~(DP_VOLTAGE_MASK|DP_PRE_EMPHASIS_MASK)) | signal_levels;
 		}
 
@@ -1590,10 +1591,13 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 		/* Set training pattern 1 */
 
 		udelay(100);
-		if (!intel_dp_get_link_status(intel_dp))
+		if (!intel_dp_get_link_status(intel_dp, link_status)) {
+			DRM_ERROR("failed to get link status\n");
 			break;
+		}
 
-		if (intel_clock_recovery_ok(intel_dp->link_status, intel_dp->lane_count)) {
+		if (intel_clock_recovery_ok(link_status, intel_dp->lane_count)) {
+			DRM_DEBUG_KMS("clock recovery OK\n");
 			clock_recovery = true;
 			break;
 		}
@@ -1615,7 +1619,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
 
 		/* Compute new intel_dp->train_set as requested by target */
-		intel_get_adjust_train(intel_dp);
+		intel_get_adjust_train(intel_dp, link_status);
 	}
 
 	intel_dp->DP = DP;
@@ -1638,6 +1642,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 	for (;;) {
 		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
 		uint32_t    signal_levels;
+		uint8_t	    link_status[DP_LINK_STATUS_SIZE];
 
 		if (cr_tries > 5) {
 			DRM_ERROR("failed to train DP, aborting\n");
@@ -1649,7 +1654,8 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 			signal_levels = intel_gen6_edp_signal_levels(intel_dp->train_set[0]);
 			DP = (DP & ~EDP_LINK_TRAIN_VOL_EMP_MASK_SNB) | signal_levels;
 		} else {
-			signal_levels = intel_dp_signal_levels(intel_dp->train_set[0], intel_dp->lane_count);
+			signal_levels = intel_dp_signal_levels(intel_dp->train_set[0]);
+			DRM_DEBUG_KMS("training pattern 1 signal levels %08x\n", signal_levels);
 			DP = (DP & ~(DP_VOLTAGE_MASK|DP_PRE_EMPHASIS_MASK)) | signal_levels;
 		}
 
@@ -1665,17 +1671,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 			break;
 
 		udelay(400);
-		if (!intel_dp_get_link_status(intel_dp))
+		if (!intel_dp_get_link_status(intel_dp, link_status))
 			break;
 
 		/* Make sure clock is still ok */
-		if (!intel_clock_recovery_ok(intel_dp->link_status, intel_dp->lane_count)) {
+		if (!intel_clock_recovery_ok(link_status, intel_dp->lane_count)) {
 			intel_dp_start_link_train(intel_dp);
 			cr_tries++;
 			continue;
 		}
 
-		if (intel_channel_eq_ok(intel_dp)) {
+		if (intel_channel_eq_ok(intel_dp, link_status)) {
 			channel_eq = true;
 			break;
 		}
@@ -1690,7 +1696,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		}
 
 		/* Compute new intel_dp->train_set as requested by target */
-		intel_get_adjust_train(intel_dp);
+		intel_get_adjust_train(intel_dp, link_status);
 		++tries;
 	}
 
@@ -1822,6 +1828,7 @@ static void
 intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
 	u8 sink_irq_vector;
+	u8 link_status[DP_LINK_STATUS_SIZE];
 
 	if (intel_dp->dpms_mode != DRM_MODE_DPMS_ON)
 		return;
@@ -1830,7 +1837,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 
 	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp)) {
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		intel_dp_link_down(intel_dp);
 		return;
 	}
@@ -1855,7 +1862,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	if (!intel_channel_eq_ok(intel_dp)) {
+	if (!intel_channel_eq_ok(intel_dp, link_status)) {
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      drm_get_encoder_name(&intel_dp->base.base));
 		intel_dp_start_link_train(intel_dp);
-- 
1.7.7

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Remove link_status field from intel_dp structure

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

On Tue,  1 Nov 2011 23:20:25 -0700
Keith Packard <keithp@keithp.com> wrote:

> No persistent data was ever stored here, so link_status is instead
> allocated on the stack as needed.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---

I like this cleanup.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

PCH eDP has many of the same needs as regular PCH DP connections,
including the DP_CTl bit settings, the TRANS_DP_CTL register.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |    3 +-
 drivers/gpu/drm/i915/intel_dp.c      |  112 ++++++++++++++++++++++++----------
 2 files changed, 81 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9fa342e..53eb29e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2933,7 +2933,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 
 	/* For PCH DP, enable TRANS_DP_CTL */
 	if (HAS_PCH_CPT(dev) &&
-	    intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
+	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
+	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
 		u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) >> 5;
 		reg = TRANS_DP_CTL(pipe);
 		temp = I915_READ(reg);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2c0c482..185fae6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -766,10 +766,10 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 			continue;
 
 		intel_dp = enc_to_intel_dp(encoder);
-		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT) {
+		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
 			lane_count = intel_dp->lane_count;
 			break;
-		} else if (is_edp(intel_dp)) {
+		} else if (is_cpu_edp(intel_dp)) {
 			lane_count = dev_priv->edp.lanes;
 			break;
 		}
@@ -808,6 +808,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		  struct drm_display_mode *adjusted_mode)
 {
 	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct drm_crtc *crtc = intel_dp->base.base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -820,18 +821,31 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 			ironlake_edp_pll_off(encoder);
 	}
 
-	intel_dp->DP = DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
-	intel_dp->DP |= intel_dp->color_range;
+	/*
+	 * There are three kinds of DP registers:
+	 *
+	 * 	IBX PCH
+	 * 	CPU
+	 * 	CPT PCH
+	 *
+	 * IBX PCH and CPU are the same for almost everything,
+	 * except that the CPU DP PLL is configured in this
+	 * register
+	 *
+	 * CPT PCH is quite different, having many bits moved
+	 * to the TRANS_DP_CTL register instead. That
+	 * configuration happens (oddly) in ironlake_pch_enable
+	 */
 
-	if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
-		intel_dp->DP |= DP_SYNC_HS_HIGH;
-	if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
-		intel_dp->DP |= DP_SYNC_VS_HIGH;
+	/* Preserve the BIOS-computed detected bit. This is
+	 * supposed to be read-only.
+	 */
+	intel_dp->DP = I915_READ(intel_dp->output_reg) & DP_DETECTED;
+	intel_dp->DP |=  DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
 
-	if (HAS_PCH_CPT(dev) && !is_cpu_edp(intel_dp))
-		intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
-	else
-		intel_dp->DP |= DP_LINK_TRAIN_OFF;
+	/* Handle DP bits in common between all three register formats */
+
+	intel_dp->DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
 
 	switch (intel_dp->lane_count) {
 	case 1:
@@ -850,32 +864,54 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
 		intel_write_eld(encoder, adjusted_mode);
 	}
-
 	memset(intel_dp->link_configuration, 0, DP_LINK_CONFIGURATION_SIZE);
 	intel_dp->link_configuration[0] = intel_dp->link_bw;
 	intel_dp->link_configuration[1] = intel_dp->lane_count;
 	intel_dp->link_configuration[8] = DP_SET_ANSI_8B10B;
-
 	/*
 	 * Check for DPCD version > 1.1 and enhanced framing support
 	 */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    (intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP)) {
 		intel_dp->link_configuration[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
-		intel_dp->DP |= DP_ENHANCED_FRAMING;
 	}
 
-	/* CPT DP's pipe select is decided in TRANS_DP_CTL */
-	if (intel_crtc->pipe == 1 && !HAS_PCH_CPT(dev))
-		intel_dp->DP |= DP_PIPEB_SELECT;
+	/* Split out the IBX/CPU vs CPT settings */
 
-	if (is_cpu_edp(intel_dp)) {
-		/* don't miss out required setting for eDP */
-		intel_dp->DP |= DP_PLL_ENABLE;
-		if (adjusted_mode->clock < 200000)
-			intel_dp->DP |= DP_PLL_FREQ_160MHZ;
-		else
-			intel_dp->DP |= DP_PLL_FREQ_270MHZ;
+	if (!HAS_PCH_CPT(dev) || is_cpu_edp(intel_dp)) {
+		intel_dp->DP |= intel_dp->color_range;
+
+		if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
+			intel_dp->DP |= DP_SYNC_HS_HIGH;
+		if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
+			intel_dp->DP |= DP_SYNC_VS_HIGH;
+		intel_dp->DP |= DP_LINK_TRAIN_OFF;
+
+		if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
+			intel_dp->DP |= DP_ENHANCED_FRAMING;
+
+		/*
+		 * Check for DPCD version > 1.1 and enhanced framing support
+		 */
+		if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
+		    (intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP)) {
+			intel_dp->link_configuration[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
+			intel_dp->DP |= DP_ENHANCED_FRAMING;
+		}
+
+		if (intel_crtc->pipe == 1)
+			intel_dp->DP |= DP_PIPEB_SELECT;
+
+		if (is_cpu_edp(intel_dp)) {
+			/* don't miss out required setting for eDP */
+			intel_dp->DP |= DP_PLL_ENABLE;
+			if (adjusted_mode->clock < 200000)
+				intel_dp->DP |= DP_PLL_FREQ_160MHZ;
+			else
+				intel_dp->DP |= DP_PLL_FREQ_270MHZ;
+		}
+	} else {
+		intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
 	}
 }
 
@@ -1341,6 +1377,7 @@ static char	*link_train_names[] = {
  * a maximum voltage of 800mV and a maximum pre-emphasis of 6dB
  */
 #define I830_DP_VOLTAGE_MAX	    DP_TRAIN_VOLTAGE_SWING_800
+#define I830_DP_VOLTAGE_MAX_CPT	    DP_TRAIN_VOLTAGE_SWING_1200
 
 static uint8_t
 intel_dp_pre_emphasis_max(uint8_t voltage_swing)
@@ -1378,8 +1415,12 @@ intel_get_adjust_train(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_ST
 			p = this_p;
 	}
 
-	if (v >= I830_DP_VOLTAGE_MAX)
-		v = I830_DP_VOLTAGE_MAX | DP_TRAIN_MAX_SWING_REACHED;
+	if (HAS_PCH_CPT(dev) && !is_cpu_edp(intel_dp))
+		voltage_max = I830_DP_VOLTAGE_MAX_CPT;
+	else
+		voltage_max = I830_DP_VOLTAGE_MAX;
+	if (v >= voltage_max)
+		v = voltage_max | DP_TRAIN_MAX_SWING_REACHED;
 
 	if (p >= intel_dp_pre_emphasis_max(v))
 		p = intel_dp_pre_emphasis_max(v) | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
@@ -1570,7 +1611,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
 		uint8_t	    link_status[DP_LINK_STATUS_SIZE];
 		uint32_t    signal_levels;
-		if (IS_GEN6(dev) && is_edp(intel_dp)) {
+
+		if (IS_GEN6(dev) && is_cpu_edp(intel_dp)) {
 			signal_levels = intel_gen6_edp_signal_levels(intel_dp->train_set[0]);
 			DP = (DP & ~EDP_LINK_TRAIN_VOL_EMP_MASK_SNB) | signal_levels;
 		} else {
@@ -1650,12 +1692,11 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 			break;
 		}
 
-		if (IS_GEN6(dev) && is_edp(intel_dp)) {
+		if (IS_GEN6(dev) && is_cpu_edp(intel_dp)) {
 			signal_levels = intel_gen6_edp_signal_levels(intel_dp->train_set[0]);
 			DP = (DP & ~EDP_LINK_TRAIN_VOL_EMP_MASK_SNB) | signal_levels;
 		} else {
 			signal_levels = intel_dp_signal_levels(intel_dp->train_set[0]);
-			DRM_DEBUG_KMS("training pattern 1 signal levels %08x\n", signal_levels);
 			DP = (DP & ~(DP_VOLTAGE_MASK|DP_PRE_EMPHASIS_MASK)) | signal_levels;
 		}
 
@@ -1741,8 +1782,12 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 
 	msleep(17);
 
-	if (is_edp(intel_dp))
-		DP |= DP_LINK_TRAIN_OFF;
+	if (is_edp(intel_dp)) {
+		if (HAS_PCH_CPT(dev) && !is_cpu_edp(intel_dp))
+			DP |= DP_LINK_TRAIN_OFF_CPT;
+		else
+			DP |= DP_LINK_TRAIN_OFF;
+	}
 
 	if (!HAS_PCH_CPT(dev) &&
 	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
@@ -2186,7 +2231,8 @@ intel_trans_dp_port_sel(struct drm_crtc *crtc)
 			continue;
 
 		intel_dp = enc_to_intel_dp(encoder);
-		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT)
+		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT ||
+		    intel_dp->base.type == INTEL_OUTPUT_EDP)
 			return intel_dp->output_reg;
 	}
 
-- 
1.7.7

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Adam Jackson]

On 11/2/11 2:20 AM, Keith Packard wrote:

> +		if (intel_dp->link_configuration [1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
> +			intel_dp->DP |= DP_ENHANCED_FRAMING;
> +
> +		/*
> +		 * Check for DPCD version>  1.1 and enhanced framing support
> +		 */
> +		if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> +		    (intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP)) {
> +			intel_dp->link_configuration[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> +			intel_dp->DP |= DP_ENHANCED_FRAMING;
> +		}

Redundant.  You've already done the link_configuration |= above in the 
common code.  You can drop the second if chunk altogether.

In related news, the corresponding section for this in TRANS_DP_CTL 
setup appears to turn on enhanced framing unconditionally.  This is 
probably not a big deal, I don't think I've ever seen a display not 
support it, but.

- ajax

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Wed, 02 Nov 2011 11:29:53 -0400, Adam Jackson <ajax@redhat.com> wrote:

> Redundant.  You've already done the link_configuration |= above in the 
> common code.  You can drop the second if chunk altogether.

Thanks for catching this mistake; cut&paste programming without the cut part...

> In related news, the corresponding section for this in TRANS_DP_CTL 
> setup appears to turn on enhanced framing unconditionally.  This is 
> probably not a big deal, I don't think I've ever seen a display not 
> support it, but.

Yeah, it's actually a huge pain because TRANS_DP_CTL is set up in
ironlake_pch_enable, which is part of the crtc enable path not the
encoder mode set path, and getting to the appropriate intel_dp structure
takes a walk through all of the encoders to find the matching one.

I think we could move the TRANS_DP_CTL code into intel_dp.c where it
belongs; this chunk was stuck inside ironlake_crtc_dpms by Zhenyu last
year when DP/eDP support for Sandybridge and Cougarpoint was added in
commit e3421a189447c0b8cd0aff5c299f53b5ab7c38f6.

Most of the TRANS_DP_CTL chunk inside ironlake_pch_enable should just
get moved to intel_dp_mode_set, but I don't know if the
TRANS_DP_OUTPUT_ENABLE bit needs to be set before
intel_enable_transcoder is called; if it does, then we'd need to
preserve that piece inside ironlake_pch_enable, otherwise that bit would
move to intel_dp_commit.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]

On Wed, 02 Nov 2011 11:29:53 -0400, Adam Jackson <ajax@redhat.com> wrote:

> Redundant.  You've already done the link_configuration |= above in the 
> common code.  You can drop the second if chunk altogether.

Here's the new version of that chunk of patch:

@@ -850,32 +864,45 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
                intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
                intel_write_eld(encoder, adjusted_mode);
        }
-
        memset(intel_dp->link_configuration, 0, DP_LINK_CONFIGURATION_SIZE);
        intel_dp->link_configuration[0] = intel_dp->link_bw;
        intel_dp->link_configuration[1] = intel_dp->lane_count;
        intel_dp->link_configuration[8] = DP_SET_ANSI_8B10B;
-
        /*
         * Check for DPCD version > 1.1 and enhanced framing support
         */
        if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
            (intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP)) {
                intel_dp->link_configuration[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
-               intel_dp->DP |= DP_ENHANCED_FRAMING;
        }
 
-       /* CPT DP's pipe select is decided in TRANS_DP_CTL */
-       if (intel_crtc->pipe == 1 && !HAS_PCH_CPT(dev))
-               intel_dp->DP |= DP_PIPEB_SELECT;
+       /* Split out the IBX/CPU vs CPT settings */
 
-       if (is_cpu_edp(intel_dp)) {
-               /* don't miss out required setting for eDP */
-               intel_dp->DP |= DP_PLL_ENABLE;
-               if (adjusted_mode->clock < 200000)
-                       intel_dp->DP |= DP_PLL_FREQ_160MHZ;
-               else
-                       intel_dp->DP |= DP_PLL_FREQ_270MHZ;
+       if (!HAS_PCH_CPT(dev) || is_cpu_edp(intel_dp)) {
+               intel_dp->DP |= intel_dp->color_range;
+
+               if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
+                       intel_dp->DP |= DP_SYNC_HS_HIGH;
+               if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
+                       intel_dp->DP |= DP_SYNC_VS_HIGH;
+               intel_dp->DP |= DP_LINK_TRAIN_OFF;
+
+               if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
+                       intel_dp->DP |= DP_ENHANCED_FRAMING;
+
+               if (intel_crtc->pipe == 1)
+                       intel_dp->DP |= DP_PIPEB_SELECT;
+
+               if (is_cpu_edp(intel_dp)) {
+                       /* don't miss out required setting for eDP */
+                       intel_dp->DP |= DP_PLL_ENABLE;
+                       if (adjusted_mode->clock < 200000)
+                               intel_dp->DP |= DP_PLL_FREQ_160MHZ;
+                       else
+                               intel_dp->DP |= DP_PLL_FREQ_270MHZ;
+               }
+       } else {
+               intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
        }
 }
 
-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

On Tue,  1 Nov 2011 23:20:26 -0700
Keith Packard <keithp@keithp.com> wrote:

> PCH eDP has many of the same needs as regular PCH DP connections,
> including the DP_CTl bit settings, the TRANS_DP_CTL register.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    3 +-
>  drivers/gpu/drm/i915/intel_dp.c      |  112 ++++++++++++++++++++++++----------
>  2 files changed, 81 insertions(+), 34 deletions(-)
> 

Might be nice to have some function pointers to handle the different
types better.  But that could be a separate cleanup.  I'd rather have
duplicated code than fragile code...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

But I was curious about this hunk:

@@ -766,10 +766,10 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 			continue;
 
 		intel_dp = enc_to_intel_dp(encoder);
-		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT) {
+		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
 			lane_count = intel_dp->lane_count;
 			break;
-		} else if (is_edp(intel_dp)) {
+		} else if (is_cpu_edp(intel_dp)) {
 			lane_count = dev_priv->edp.lanes;
 			break;
 		}

I guess this means we can't trust the BIOS settings for PCH eDP?

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

On Wed, 2 Nov 2011 09:20:19 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> But I was curious about this hunk:
> 
> @@ -766,10 +766,10 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  			continue;
>  
>  		intel_dp = enc_to_intel_dp(encoder);
> -		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT) {
> +		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
>  			lane_count = intel_dp->lane_count;
>  			break;
> -		} else if (is_edp(intel_dp)) {
> +		} else if (is_cpu_edp(intel_dp)) {
>  			lane_count = dev_priv->edp.lanes;
>  			break;
>  		}
> 
> I guess this means we can't trust the BIOS settings for PCH eDP?

I'm pretty sure this isn't the right place to look at this value in any
case; we're setting the m/n ratios after already deciding how many lanes
to use. Getting this wrong means sending the wrong timing to the
monitor, not setting a different mode.

In any case, my AIO box sets the BIOS value to 1, when it needs 2 lanes
for the mode it uses.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Adam Jackson]

On 11/2/11 12:20 PM, Jesse Barnes wrote:

> @@ -766,10 +766,10 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>   			continue;
>
>   		intel_dp = enc_to_intel_dp(encoder);
> -		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT) {
> +		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
>   			lane_count = intel_dp->lane_count;
>   			break;
> -		} else if (is_edp(intel_dp)) {
> +		} else if (is_cpu_edp(intel_dp)) {
>   			lane_count = dev_priv->edp.lanes;
>   			break;
>   		}
>
> I guess this means we can't trust the BIOS settings for PCH eDP?

Given the choice of trusting DPCD or the VBT, I'd definitely prefer DPCD.

- ajax

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Wed, 02 Nov 2011 13:13:52 -0400, Adam Jackson <ajax@redhat.com> wrote:

> Given the choice of trusting DPCD or the VBT, I'd definitely prefer
> DPCD.

Except that the DPCD is coded into the monitor while the VBT is done by
the platform. And, it's the platform which may neglect to connect some
of the wires. In any case, if we want to look at these values, we should
be doing it before computing the M/N ratio so that we actually set the
hardware up correctly.

Any bets on how long until we find a machine that has right value in the
VBT and the wrong one in DPCD? Or a machine with wrong values in both places?

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Adam Jackson]

On 11/2/11 1:31 PM, Keith Packard wrote:
> On Wed, 02 Nov 2011 13:13:52 -0400, Adam Jackson<ajax@redhat.com>  wrote:
>
>> Given the choice of trusting DPCD or the VBT, I'd definitely prefer
>> DPCD.
>
> Except that the DPCD is coded into the monitor while the VBT is done by
> the platform. And, it's the platform which may neglect to connect some
> of the wires.

My reasoning about this has been:

The maximum link configuration in DPCD is going to fit - and minimally 
fit - the maximum supported configuration (depth/rate/size/etc), because 
otherwise the hardware would have been more expensive to produce.

The VBT is going to be crap.

But as always, "do what the Windows driver does" seems like a good 
strategy.  Do we know?

> Any bets on how long until we find a machine that has right value in the
> VBT and the wrong one in DPCD? Or a machine with wrong values in both places?

I will happily pay $20 to the first person to find a monitor with broken 
link/lane in DPCD, on the understanding that they take it (the $20) to 
the nearest hardware store, buy a hammer, and smash the monitor. 
Preferably with the video uploaded to youtube.

- ajax

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

On Wed, 02 Nov 2011 15:36:20 -0400, Adam Jackson <ajax@redhat.com> wrote:

> The VBT is going to be crap.

The only question then is what to do with hardware that doesn't have the
DPCD value -- that's "new" in revision 0x11, after all.

How about this:

commit 34ebe02cc78f20ae6b7865c5087c3b5ac7810185
Author: Keith Packard <keithp@keithp.com>
Date:   Wed Nov 2 13:03:47 2011 -0700

    drm/i915: Use DPCD value for max DP lanes where possible
    
    Fall back to the VBT value for eDP monitors only when DPCD is missing
    the value.
    
    Signed-off-by: Keith Packard <keithp@keithp.com>

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 02b56ce..93b082a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -154,6 +154,8 @@ intel_edp_link_config(struct intel_encoder *intel_encoder,
 static int
 intel_dp_max_lane_count(struct intel_dp *intel_dp)
 {
+       struct drm_device *dev = intel_dp->base.base.dev;
+       struct drm_i915_private *dev_priv = dev->dev_private;
        int max_lane_count = 4;
 
        if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
@@ -164,6 +166,8 @@ intel_dp_max_lane_count(struct intel_dp *intel_dp)
                default:
                        max_lane_count = 4;
                }
+       } else if (is_edp(intel_dp)) {
+               max_lane_count = dev_priv->edp.lanes;
        }
        return max_lane_count;
 }
@@ -765,12 +769,11 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
                        continue;
 
                intel_dp = enc_to_intel_dp(encoder);
-               if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
+               if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT ||
+                   intel_dp->base.type == INTEL_OUTPUT_EDP)
+               {
                        lane_count = intel_dp->lane_count;
                        break;
-               } else if (is_cpu_edp(intel_dp)) {
-                       lane_count = dev_priv->edp.lanes;
-                       break;
                }
        }
 


-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Adam Jackson]

On 11/2/11 4:05 PM, Keith Packard wrote:
> On Wed, 02 Nov 2011 15:36:20 -0400, Adam Jackson<ajax@redhat.com>  wrote:
>
>> The VBT is going to be crap.
>
> The only question then is what to do with hardware that doesn't have the
> DPCD value -- that's "new" in revision 0x11, after all.

It is?  The DP 1.1a text for lane count is "For Rev.1.1, only the 
following three values are supported. All other values are reserved."  I 
don't think that implies anything about what it meant in 1.0.  It does 
say that bits 7:5 of that register are reserved in 1.0 though; since it 
doesn't have any versioning on bits 4:0 I'd think that means they're 
interpreted the same in 1.0 as in 1.1.

Unless you have a copy of the 1.0 spec?

Again, not that it probably matters much.  I think the installed base of 
DP 1.0 sinks is zero, I've literally never seen one.

> How about this:
>
> commit 34ebe02cc78f20ae6b7865c5087c3b5ac7810185
> Author: Keith Packard<keithp@keithp.com>
> Date:   Wed Nov 2 13:03:47 2011 -0700
>
>      drm/i915: Use DPCD value for max DP lanes where possible
>
>      Fall back to the VBT value for eDP monitors only when DPCD is missing
>      the value.
>
>      Signed-off-by: Keith Packard<keithp@keithp.com>

Reviewed-by: Adam Jackson <ajax@redhat.com>

- ajax

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

On Wed, 02 Nov 2011 16:35:51 -0400, Adam Jackson <ajax@redhat.com> wrote:

> It is?  The DP 1.1a text for lane count is "For Rev.1.1, only the 
> following three values are supported. All other values are reserved."

Yeah, if you look at the MAX_LINK_RATE field, we assume that it has a
useful value. I'll bet they were thinking of letting the spec support
things like alternate clock rates or 3 lanes or something, and the 1.1
version just tied things down to allow only sensible values there.

How about we just always use the DPCD value?

commit e0fafa5dee031ef6174eadb005a5f01d13da931d
Author: Keith Packard <keithp@keithp.com>
Date:   Wed Nov 2 13:03:47 2011 -0700

    drm/i915: Use DPCD value for max DP lanes.
    
    The BIOS VBT value for an eDP panel has been shown to be incorrect on
    one machine, and we haven't found any machines where the DPCD value
    was wrong, so we'll use the DPCD value everywhere.
    
    Signed-off-by: Keith Packard <keithp@keithp.com>

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 02b56ce..5056d29 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -154,16 +154,12 @@ intel_edp_link_config(struct intel_encoder *intel_encoder,
 static int
 intel_dp_max_lane_count(struct intel_dp *intel_dp)
 {
-       int max_lane_count = 4;
-
-       if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
-               max_lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & 0x1f;
-               switch (max_lane_count) {
-               case 1: case 2: case 4:
-                       break;
-               default:
-                       max_lane_count = 4;
-               }
+       int max_lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & 0x1f;
+       switch (max_lane_count) {
+       case 1: case 2: case 4:
+               break;
+       default:
+               max_lane_count = 4;
        }
        return max_lane_count;
 }
@@ -765,12 +761,11 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
                        continue;
 
                intel_dp = enc_to_intel_dp(encoder);
-               if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
+               if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT ||
+                   intel_dp->base.type == INTEL_OUTPUT_EDP)
+               {
                        lane_count = intel_dp->lane_count;
                        break;
-               } else if (is_cpu_edp(intel_dp)) {
-                       lane_count = dev_priv->edp.lanes;
-                       break;
                }
        }

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Adam Jackson]

On 11/2/11 5:13 PM, Keith Packard wrote:
> On Wed, 02 Nov 2011 16:35:51 -0400, Adam Jackson<ajax@redhat.com>  wrote:
>
>> It is?  The DP 1.1a text for lane count is "For Rev.1.1, only the
>> following three values are supported. All other values are reserved."
>
> Yeah, if you look at the MAX_LINK_RATE field, we assume that it has a
> useful value. I'll bet they were thinking of letting the spec support
> things like alternate clock rates or 3 lanes or something, and the 1.1
> version just tied things down to allow only sensible values there.
>
> How about we just always use the DPCD value?

Looks good.

Reviewed-by: Adam Jackson <ajax@redhat.com>

- ajax

Re: [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

On Tue,  1 Nov 2011 23:20:26 -0700, Keith Packard <keithp@keithp.com> wrote:

>  		intel_dp = enc_to_intel_dp(encoder);
> -		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT) {
> +		if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
>  			lane_count = intel_dp->lane_count;
>  			break;
> -		} else if (is_edp(intel_dp)) {
> +		} else if (is_cpu_edp(intel_dp)) {
>  			lane_count = dev_priv->edp.lanes;
>  			break;

Thinking about this one more time -- if we ever want to use
dev_priv->edp.lanes, we should use it in
intel_dp_max_lane_count. intel_dp_set_m_n should use
intel_dp->lane_count unconditionally as that's the value we've used
everywhere else for mode setting.

Perhaps we should use it for monitors that don't include the
MAX_LANE_COUNT field in the dpcd? Perhaps we should use it on all eDP
monitors?

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH 3/7] drm/i915: Treat PCH eDP like DP in most places

[Alex Deucher]

On Wed, Nov 2, 2011 at 2:54 PM, Keith Packard <keithp@keithp.com> wrote:
> On Tue,  1 Nov 2011 23:20:26 -0700, Keith Packard <keithp@keithp.com> wrote:
>
>>               intel_dp = enc_to_intel_dp(encoder);
>> -             if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT) {
>> +             if (intel_dp->base.type == INTEL_OUTPUT_DISPLAYPORT || is_pch_edp(intel_dp)) {
>>                       lane_count = intel_dp->lane_count;
>>                       break;
>> -             } else if (is_edp(intel_dp)) {
>> +             } else if (is_cpu_edp(intel_dp)) {
>>                       lane_count = dev_priv->edp.lanes;
>>                       break;
>
> Thinking about this one more time -- if we ever want to use
> dev_priv->edp.lanes, we should use it in
> intel_dp_max_lane_count. intel_dp_set_m_n should use
> intel_dp->lane_count unconditionally as that's the value we've used
> everywhere else for mode setting.
>
> Perhaps we should use it for monitors that don't include the
> MAX_LANE_COUNT field in the dpcd? Perhaps we should use it on all eDP
> monitors?

FWIW, we rely on the DPCD field for eDP just like DP.  Our vbios LCD
tables don't contain DP lane or rate info.

Alex

[PATCH 4/7] drm/i915: Let panel power sequencing hardware do its job

[Keith Packard]

The panel power sequencing hardware tracks the stages of panel power
sequencing and signals when the panel is completely on or off. Instead
of blindly assuming the panel timings will work, poll the panel power
status register until it shows the correct values.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_reg.h |   17 ++++-
 drivers/gpu/drm/i915/intel_dp.c |  138 +++++++++++++++++++++------------------
 2 files changed, 87 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5a09416..275d149 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1553,12 +1553,21 @@
  */
 #define   PP_READY		(1 << 30)
 #define   PP_SEQUENCE_NONE	(0 << 28)
-#define   PP_SEQUENCE_ON	(1 << 28)
-#define   PP_SEQUENCE_OFF	(2 << 28)
-#define   PP_SEQUENCE_MASK	0x30000000
+#define   PP_SEQUENCE_POWER_UP	(1 << 28)
+#define   PP_SEQUENCE_POWER_DOWN (2 << 28)
+#define   PP_SEQUENCE_MASK	(3 << 28)
+#define   PP_SEQUENCE_SHIFT	28
 #define   PP_CYCLE_DELAY_ACTIVE	(1 << 27)
-#define   PP_SEQUENCE_STATE_ON_IDLE (1 << 3)
 #define   PP_SEQUENCE_STATE_MASK 0x0000000f
+#define   PP_SEQUENCE_STATE_OFF_IDLE	(0x0 << 0)
+#define   PP_SEQUENCE_STATE_OFF_S0_1	(0x1 << 0)
+#define   PP_SEQUENCE_STATE_OFF_S0_2	(0x2 << 0)
+#define   PP_SEQUENCE_STATE_OFF_S0_3	(0x3 << 0)
+#define   PP_SEQUENCE_STATE_ON_IDLE	(0x8 << 0)
+#define   PP_SEQUENCE_STATE_ON_S1_0	(0x9 << 0)
+#define   PP_SEQUENCE_STATE_ON_S1_2	(0xa << 0)
+#define   PP_SEQUENCE_STATE_ON_S1_3	(0xb << 0)
+#define   PP_SEQUENCE_STATE_RESET	(0xf << 0)
 #define PP_CONTROL	0x61204
 #define   POWER_TARGET_ON	(1 << 0)
 #define PP_ON_DELAYS	0x61208
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 185fae6..d6c6608 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -66,7 +66,6 @@ struct intel_dp {
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
-	unsigned long panel_off_jiffies;
 };
 
 /**
@@ -915,32 +914,56 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	}
 }
 
-static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
+#define IDLE_ON_MASK		(PP_ON | PP_READY | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
+#define IDLE_ON_VALUE   	(PP_ON | PP_READY | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
+
+#define IDLE_OFF_MASK		(PP_ON | 0        | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
+#define IDLE_OFF_VALUE		(0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+
+#define IDLE_CYCLE_MASK		(PP_ON | 0        | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
+#define IDLE_CYCLE_VALUE	(0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+
+static void ironlake_wait_panel_status(struct intel_dp *intel_dp,
+				       u32 mask,
+				       u32 value)
 {
-	unsigned long	off_time;
-	unsigned long	delay;
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	DRM_DEBUG_KMS("Wait for panel power off time\n");
+	DRM_DEBUG_KMS("mask %08x value %08x status %08x control %08x\n",
+		      mask, value,
+		      I915_READ(PCH_PP_STATUS),
+		      I915_READ(PCH_PP_CONTROL));
 
-	if (ironlake_edp_have_panel_power(intel_dp) ||
-	    ironlake_edp_have_panel_vdd(intel_dp))
+	if (_wait_for((I915_READ(PCH_PP_STATUS) & mask) == value,
+		      5000,
+		      10))
 	{
-		DRM_DEBUG_KMS("Panel still on, no delay needed\n");
-		return;
+		DRM_ERROR("Panel status timeout: status %08x control %08x\n",
+			  I915_READ(PCH_PP_STATUS),
+			  I915_READ(PCH_PP_CONTROL));
 	}
+}
 
-	off_time = intel_dp->panel_off_jiffies + msecs_to_jiffies(intel_dp->panel_power_down_delay);
-	if (time_after(jiffies, off_time)) {
-		DRM_DEBUG_KMS("Time already passed");
-		return;
-	}
-	delay = jiffies_to_msecs(off_time - jiffies);
-	if (delay > intel_dp->panel_power_down_delay)
-		delay = intel_dp->panel_power_down_delay;
-	DRM_DEBUG_KMS("Waiting an additional %ld ms\n", delay);
-	msleep(delay);
+static void ironlake_wait_panel_on(struct intel_dp *intel_dp)
+{
+	DRM_DEBUG_KMS("Wait for panel power on\n");
+	ironlake_wait_panel_status(intel_dp, IDLE_ON_MASK, IDLE_ON_VALUE);
 }
 
+static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
+{
+	DRM_DEBUG_KMS("Wait for panel power off time\n");
+	ironlake_wait_panel_status(intel_dp, IDLE_OFF_MASK, IDLE_OFF_VALUE);
+}
+
+static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp)
+{
+	DRM_DEBUG_KMS("Wait for panel power cycle\n");
+	ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
+}
+
+
 /* Read the current pp_control value, unlocking the register if it
  * is locked
  */
@@ -968,12 +991,15 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	     "eDP VDD already requested on\n");
 
 	intel_dp->want_panel_vdd = true;
+
 	if (ironlake_edp_have_panel_vdd(intel_dp)) {
 		DRM_DEBUG_KMS("eDP VDD already on\n");
 		return;
 	}
 
-	ironlake_wait_panel_off(intel_dp);
+	if (!ironlake_edp_have_panel_power(intel_dp))
+		ironlake_wait_panel_power_cycle(intel_dp);
+
 	pp = ironlake_get_pp_control(dev_priv);
 	pp |= EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
@@ -1005,7 +1031,8 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
 		/* Make sure sequencer is idle before allowing subsequent activity */
 		DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 			      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-		intel_dp->panel_off_jiffies = jiffies;
+
+		msleep(intel_dp->panel_power_down_delay);
 	}
 }
 
@@ -1043,21 +1070,25 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 	}
 }
 
-/* Returns true if the panel was already on when called */
 static void ironlake_edp_panel_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 pp, idle_on_mask = PP_ON | PP_SEQUENCE_STATE_ON_IDLE;
+	u32 pp;
 
 	if (!is_edp(intel_dp))
 		return;
-	if (ironlake_edp_have_panel_power(intel_dp))
+
+	DRM_DEBUG_KMS("Turn eDP power on\n");
+
+	if (ironlake_edp_have_panel_power(intel_dp)) {
+		DRM_DEBUG_KMS("eDP power already on\n");
 		return;
+	}
 
-	ironlake_wait_panel_off(intel_dp);
-	pp = ironlake_get_pp_control(dev_priv);
+	ironlake_wait_panel_power_cycle(intel_dp);
 
+	pp = ironlake_get_pp_control(dev_priv);
 	if (IS_GEN5(dev)) {
 		/* ILK workaround: disable reset around power sequence */
 		pp &= ~PANEL_POWER_RESET;
@@ -1066,13 +1097,13 @@ static void ironlake_edp_panel_on(struct intel_dp *intel_dp)
 	}
 
 	pp |= POWER_TARGET_ON;
+	if (!IS_GEN5(dev))
+		pp |= PANEL_POWER_RESET;
+
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
 
-	if (wait_for((I915_READ(PCH_PP_STATUS) & idle_on_mask) == idle_on_mask,
-		     5000))
-		DRM_ERROR("panel on wait timed out: 0x%08x\n",
-			  I915_READ(PCH_PP_STATUS));
+	ironlake_wait_panel_on(intel_dp);
 
 	if (IS_GEN5(dev)) {
 		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
@@ -1081,44 +1112,25 @@ static void ironlake_edp_panel_on(struct intel_dp *intel_dp)
 	}
 }
 
-static void ironlake_edp_panel_off(struct drm_encoder *encoder)
+static void ironlake_edp_panel_off(struct intel_dp *intel_dp)
 {
-	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 pp, idle_off_mask = PP_ON | PP_SEQUENCE_MASK |
-		PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK;
+	u32 pp;
 
 	if (!is_edp(intel_dp))
 		return;
-	pp = ironlake_get_pp_control(dev_priv);
 
-	if (IS_GEN5(dev)) {
-		/* ILK workaround: disable reset around power sequence */
-		pp &= ~PANEL_POWER_RESET;
-		I915_WRITE(PCH_PP_CONTROL, pp);
-		POSTING_READ(PCH_PP_CONTROL);
-	}
-
-	intel_dp->panel_off_jiffies = jiffies;
+	DRM_DEBUG_KMS("Turn eDP power off\n");
 
-	if (IS_GEN5(dev)) {
-		pp &= ~POWER_TARGET_ON;
-		I915_WRITE(PCH_PP_CONTROL, pp);
-		POSTING_READ(PCH_PP_CONTROL);
-		pp &= ~POWER_TARGET_ON;
-		I915_WRITE(PCH_PP_CONTROL, pp);
-		POSTING_READ(PCH_PP_CONTROL);
-		msleep(intel_dp->panel_power_cycle_delay);
+	WARN(intel_dp->want_panel_vdd, "Cannot turn power off while VDD is on\n");
 
-		if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
-			DRM_ERROR("panel off wait timed out: 0x%08x\n",
-				  I915_READ(PCH_PP_STATUS));
+	pp = ironlake_get_pp_control(dev_priv);
+	pp &= ~(POWER_TARGET_ON | EDP_FORCE_VDD | PANEL_POWER_RESET | EDP_BLC_ENABLE);
+	I915_WRITE(PCH_PP_CONTROL, pp);
+	POSTING_READ(PCH_PP_CONTROL);
 
-		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
-		I915_WRITE(PCH_PP_CONTROL, pp);
-		POSTING_READ(PCH_PP_CONTROL);
-	}
+	ironlake_wait_panel_off(intel_dp);
 }
 
 static void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
@@ -1232,7 +1244,7 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 	 */
 	ironlake_edp_backlight_off(intel_dp);
 	intel_dp_link_down(intel_dp);
-	ironlake_edp_panel_off(encoder);
+	ironlake_edp_panel_off(intel_dp);
 }
 
 static void intel_dp_commit(struct drm_encoder *encoder)
@@ -1246,7 +1258,6 @@ static void intel_dp_commit(struct drm_encoder *encoder)
 	intel_dp_start_link_train(intel_dp);
 	ironlake_edp_panel_on(intel_dp);
 	ironlake_edp_panel_vdd_off(intel_dp, true);
-
 	intel_dp_complete_link_train(intel_dp);
 	ironlake_edp_backlight_on(intel_dp);
 
@@ -1270,7 +1281,7 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 			ironlake_edp_backlight_off(intel_dp);
 		intel_dp_sink_dpms(intel_dp, mode);
 		intel_dp_link_down(intel_dp);
-		ironlake_edp_panel_off(encoder);
+		ironlake_edp_panel_off(intel_dp);
 		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
 			ironlake_edp_pll_off(encoder);
 		ironlake_edp_panel_vdd_off(intel_dp, false);
@@ -2407,11 +2418,10 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		DRM_DEBUG_KMS("backlight on delay %d, off delay %d\n",
 			      intel_dp->backlight_on_delay, intel_dp->backlight_off_delay);
 
-		intel_dp->panel_off_jiffies = jiffies - intel_dp->panel_power_down_delay;
-
 		ironlake_edp_panel_vdd_on(intel_dp);
 		ret = intel_dp_get_dpcd(intel_dp);
 		ironlake_edp_panel_vdd_off(intel_dp, false);
+
 		if (ret) {
 			if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
 				dev_priv->no_aux_handshake =
-- 
1.7.7

Re: [PATCH 4/7] drm/i915: Let panel power sequencing hardware do its job

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 914 bytes --]

On Tue,  1 Nov 2011 23:20:27 -0700, Keith Packard <keithp@keithp.com> wrote:

> -static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
> +#define IDLE_ON_MASK		(PP_ON | PP_READY | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> +#define IDLE_ON_VALUE   	(PP_ON | PP_READY | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)

A bit more testing -- looks like the MacBook Air doesn't mange to get
PP_READY set when it's time to turn the panel on. I should look at this
a bit more closely; there's no reason it shouldn't be set. But, nothing
bad seems to happen if we simply ignore the PP_READY bit

+#define IDLE_ON_MASK		(PP_ON | 0      | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
+#define IDLE_ON_VALUE   	(PP_ON | 0      | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 4/7] drm/i915: Let panel power sequencing hardware do its job

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On Wed, 02 Nov 2011 00:31:40 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Tue,  1 Nov 2011 23:20:27 -0700, Keith Packard <keithp@keithp.com> wrote:
> 
> > -static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
> > +#define IDLE_ON_MASK		(PP_ON | PP_READY | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> > +#define IDLE_ON_VALUE   	(PP_ON | PP_READY | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
> 
> A bit more testing -- looks like the MacBook Air doesn't mange to get
> PP_READY set when it's time to turn the panel on. I should look at this
> a bit more closely; there's no reason it shouldn't be set. But, nothing
> bad seems to happen if we simply ignore the PP_READY bit
> 
> +#define IDLE_ON_MASK		(PP_ON | 0      | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
> +#define IDLE_ON_VALUE   	(PP_ON | 0      | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)

Note that PP_READY will incorrectly depend on some other register
values, so in some configs the panel will happily power up even if
PP_READY isn't set yet...

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 4/7] drm/i915: Let panel power sequencing hardware do its job

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

On Wed, 2 Nov 2011 09:23:10 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Note that PP_READY will incorrectly depend on some other register
> values, so in some configs the panel will happily power up even if
> PP_READY isn't set yet...

Yeah, I'd like to understand why PP_READY isn't getting set; we should
have all of the other pieces ready before we turn on the panel. But,
given that it's really just a sanity check, it probably doesn't make
sense to spin for 5 seconds waiting for a bit to turn on that won't.

I've just removed that bit from the IDLE_ON_MASK and IDLE_ON_VALUE.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 4/7] drm/i915: Let panel power sequencing hardware do its job

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

On Wed, 2 Nov 2011 09:23:10 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Note that PP_READY will incorrectly depend on some other register
> values, so in some configs the panel will happily power up even if
> PP_READY isn't set yet...

Here's the new version of that chunk:

@@ -906,32 +905,56 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
        }
 }
 
-static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
+#define IDLE_ON_MASK           (PP_ON | 0        | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
+#define IDLE_ON_VALUE          (PP_ON | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_ON_IDLE)
+
+#define IDLE_OFF_MASK          (PP_ON | 0        | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
+#define IDLE_OFF_VALUE         (0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+
+#define IDLE_CYCLE_MASK                (PP_ON | 0        | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
+#define IDLE_CYCLE_VALUE       (0     | 0        | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
+
+static void ironlake_wait_panel_status(struct intel_dp *intel_dp,
+                                      u32 mask,
+                                      u32 value)
 {


-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 4/7] drm/i915: Let panel power sequencing hardware do its job

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Tue,  1 Nov 2011 23:20:27 -0700
Keith Packard <keithp@keithp.com> wrote:

> The panel power sequencing hardware tracks the stages of panel power
> sequencing and signals when the panel is completely on or off. Instead
> of blindly assuming the panel timings will work, poll the panel power
> status register until it shows the correct values.

Modulo what we already discussed on irc about the PP_READY bit, and the
CodingStyle nitpicks (newlines before {?  come on, this isn't X :),

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 4/7] drm/i915: Let panel power sequencing hardware do its job

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

On Thu, 3 Nov 2011 12:57:08 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Modulo what we already discussed on irc about the PP_READY bit, and the

Right, the PP_READY bit requires that everything needed for PCH eDP be
running, even when you're using a CPU connected eDP panel, and so it's
not actually useful.

> CodingStyle nitpicks (newlines before {?  come on, this isn't X :),

Ok, I can fix that at least, even if I think it's ugly in kernel style.

I'll clean up the style and attach your R-b.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

[PATCH 5/7] drm/i915: Make DP prepare/commit consistent with DP dpms

[Keith Packard]

Make sure the sequence of operations in all three functions makes
sense:

 1) The backlight must be off unless the screen is running
 2) The link must be running to turn the eDP panel on/off
 3) The CPU eDP PLL must be running until everything is off

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d6c6608..6be6a04 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1234,17 +1234,18 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
+	ironlake_edp_backlight_off(intel_dp);
+	ironlake_edp_panel_off(intel_dp);
+
 	/* Wake up the sink first */
 	ironlake_edp_panel_vdd_on(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
+	intel_dp_link_down(intel_dp);
 	ironlake_edp_panel_vdd_off(intel_dp, false);
 
 	/* Make sure the panel is off before trying to
 	 * change the mode
 	 */
-	ironlake_edp_backlight_off(intel_dp);
-	intel_dp_link_down(intel_dp);
-	ironlake_edp_panel_off(intel_dp);
 }
 
 static void intel_dp_commit(struct drm_encoder *encoder)
@@ -1276,16 +1277,20 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
 
 	if (mode != DRM_MODE_DPMS_ON) {
+		ironlake_edp_backlight_off(intel_dp);
+		ironlake_edp_panel_off(intel_dp);
+
 		ironlake_edp_panel_vdd_on(intel_dp);
-		if (is_edp(intel_dp))
-			ironlake_edp_backlight_off(intel_dp);
 		intel_dp_sink_dpms(intel_dp, mode);
 		intel_dp_link_down(intel_dp);
-		ironlake_edp_panel_off(intel_dp);
-		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
-			ironlake_edp_pll_off(encoder);
 		ironlake_edp_panel_vdd_off(intel_dp, false);
+
+		if (is_cpu_edp(intel_dp))
+			ironlake_edp_pll_off(encoder);
 	} else {
+		if (is_cpu_edp(intel_dp))
+			ironlake_edp_pll_on(encoder);
+
 		ironlake_edp_panel_vdd_on(intel_dp);
 		intel_dp_sink_dpms(intel_dp, mode);
 		if (!(dp_reg & DP_PORT_EN)) {
@@ -1293,7 +1298,6 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 			ironlake_edp_panel_on(intel_dp);
 			ironlake_edp_panel_vdd_off(intel_dp, true);
 			intel_dp_complete_link_train(intel_dp);
-			ironlake_edp_backlight_on(intel_dp);
 		} else
 			ironlake_edp_panel_vdd_off(intel_dp, false);
 		ironlake_edp_backlight_on(intel_dp);
-- 
1.7.7

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Make DP prepare/commit consistent with DP dpms

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Tue,  1 Nov 2011 23:20:28 -0700
Keith Packard <keithp@keithp.com> wrote:

> Make sure the sequence of operations in all three functions makes
> sense:
> 
>  1) The backlight must be off unless the screen is running
>  2) The link must be running to turn the eDP panel on/off
>  3) The CPU eDP PLL must be running until everything is off

A few comments on this one (also, is it strictly required to fix your
bug)?

> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   22 +++++++++++++---------
>  1 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d6c6608..6be6a04 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1234,17 +1234,18 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> +	ironlake_edp_backlight_off(intel_dp);
> +	ironlake_edp_panel_off(intel_dp);
> +
>  	/* Wake up the sink first */
>  	ironlake_edp_panel_vdd_on(intel_dp);
>  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> +	intel_dp_link_down(intel_dp);
>  	ironlake_edp_panel_vdd_off(intel_dp, false);
>  
>  	/* Make sure the panel is off before trying to
>  	 * change the mode
>  	 */
> -	ironlake_edp_backlight_off(intel_dp);
> -	intel_dp_link_down(intel_dp);
> -	ironlake_edp_panel_off(intel_dp);
>  }

Ok so you're making sure the panel has power to down the link, I think
that's fine.

>  static void intel_dp_commit(struct drm_encoder *encoder)
> @@ -1276,16 +1277,20 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
>  	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
>  
>  	if (mode != DRM_MODE_DPMS_ON) {
> +		ironlake_edp_backlight_off(intel_dp);
> +		ironlake_edp_panel_off(intel_dp);
> +
>  		ironlake_edp_panel_vdd_on(intel_dp);
> -		if (is_edp(intel_dp))
> -			ironlake_edp_backlight_off(intel_dp);
>  		intel_dp_sink_dpms(intel_dp, mode);
>  		intel_dp_link_down(intel_dp);
> -		ironlake_edp_panel_off(intel_dp);
> -		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
> -			ironlake_edp_pll_off(encoder);
>  		ironlake_edp_panel_vdd_off(intel_dp, false);
> +
> +		if (is_cpu_edp(intel_dp))
> +			ironlake_edp_pll_off(encoder);


But here it looks like you're shutting it off, then downing the link?
Should this be reordered?

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Make DP prepare/commit consistent with DP dpms

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1838 bytes --]

On Thu, 3 Nov 2011 13:00:11 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> A few comments on this one (also, is it strictly required to fix your
> bug)?

I think so; the panel definitely was not happy when I turned the link
off while the panel power was on. Having the mode setting and DPMS paths
doing things in different orders definitely doesn't seem reasonable
though. I nearly managed to share code between the two paths, but there
are subtle differences in requirements and so I didn't bother.

> Ok so you're making sure the panel has power to down the link, I think
> that's fine.

No, I'm turning the panel off *before* turning off the link.  The panel
goes nuts if you down the link before turning its power off; lots of
technicolor.

Downing the link doesn't require any communication with the panel, so
there's no issue with losing connectivity at this point:

	ironlake_edp_backlight_off(intel_dp);
	ironlake_edp_panel_off(intel_dp);               <= panel off

	/* Wake up the sink first */
	ironlake_edp_panel_vdd_on(intel_dp);
	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
	intel_dp_link_down(intel_dp);                   <= link down
	ironlake_edp_panel_vdd_off(intel_dp, false);

> But here it looks like you're shutting it off, then downing the link?
> Should this be reordered?

Nope, it's in the same order:

	ironlake_edp_backlight_off(intel_dp);
	ironlake_edp_panel_off(intel_dp);               <= panel off

	ironlake_edp_panel_vdd_on(intel_dp);
	intel_dp_sink_dpms(intel_dp, mode);
	intel_dp_link_down(intel_dp);                   <= link down
	ironlake_edp_panel_vdd_off(intel_dp, false);

The only difference in these two code paths is that dp_prepare turns the
sink to DPMS_ON, while dp_dpms turns the sink to DPMS_OFF.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Make DP prepare/commit consistent with DP dpms

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

On Thu, 03 Nov 2011 15:30:57 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Thu, 3 Nov 2011 13:00:11 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > A few comments on this one (also, is it strictly required to fix your
> > bug)?
> 
> I think so; the panel definitely was not happy when I turned the link
> off while the panel power was on. Having the mode setting and DPMS paths
> doing things in different orders definitely doesn't seem reasonable
> though. I nearly managed to share code between the two paths, but there
> are subtle differences in requirements and so I didn't bother.
> 
> > Ok so you're making sure the panel has power to down the link, I think
> > that's fine.
> 
> No, I'm turning the panel off *before* turning off the link.  The panel
> goes nuts if you down the link before turning its power off; lots of
> technicolor.

Except for VDD??  That does come on... and shouldn't be any different
than a full power sequence as far as link training etc go...

> Downing the link doesn't require any communication with the panel, so
> there's no issue with losing connectivity at this point:
> 
> 	ironlake_edp_backlight_off(intel_dp);
> 	ironlake_edp_panel_off(intel_dp);               <= panel off
> 
> 	/* Wake up the sink first */
> 	ironlake_edp_panel_vdd_on(intel_dp);
> 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> 	intel_dp_link_down(intel_dp);                   <= link down
> 	ironlake_edp_panel_vdd_off(intel_dp, false);
> 
> > But here it looks like you're shutting it off, then downing the link?
> > Should this be reordered?
> 
> Nope, it's in the same order:
> 
> 	ironlake_edp_backlight_off(intel_dp);
> 	ironlake_edp_panel_off(intel_dp);               <= panel off
> 
> 	ironlake_edp_panel_vdd_on(intel_dp);
> 	intel_dp_sink_dpms(intel_dp, mode);
> 	intel_dp_link_down(intel_dp);                   <= link down
> 	ironlake_edp_panel_vdd_off(intel_dp, false);
> 
> The only difference in these two code paths is that dp_prepare turns the
> sink to DPMS_ON, while dp_dpms turns the sink to DPMS_OFF.

Oh missed the vdd on, which is in this path too...  So I'm still
confused by the panel off, vdd on sequence, but at least they're
consistent.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Make DP prepare/commit consistent with DP dpms

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Thu, 3 Nov 2011 15:41:25 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Except for VDD??  That does come on... and shouldn't be any different
> than a full power sequence as far as link training etc go...

Oh, that's a good point. Doing things in this order essentially forces
yet another full panel power sequence delay at this point. Hrm. I'll
have to test again when I get a chance, but perhaps we can turn the sink
DPMS on before we turn the panel power off.

> Oh missed the vdd on, which is in this path too...  So I'm still
> confused by the panel off, vdd on sequence, but at least they're
> consistent.

Right, I'll try doing the sink_dpms before turning the panel off; that
should work just fine, and should make the sequence a bit faster.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

[PATCH 6/7] drm/i915: Try harder during dp pattern 1 link training

[Keith Packard]

Instead of going through the sequence just once, run through the whole
set up to 5 times to see if something can work. This isn't part of the
DP spec, but the BIOS seems to do it, and given that link training
failure is so bad, it seems reasonable to follow suit.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   41 +++++++++++++++++++++++++-------------
 1 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6be6a04..bf20a35 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1576,8 +1576,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 
 	ret = intel_dp_aux_native_write(intel_dp,
 					DP_TRAINING_LANE0_SET,
-					intel_dp->train_set, 4);
-	if (ret != 4)
+					intel_dp->train_set,
+					intel_dp->lane_count);
+	if (ret != intel_dp->lane_count)
 		return false;
 
 	return true;
@@ -1593,7 +1594,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 	int i;
 	uint8_t voltage;
 	bool clock_recovery = false;
-	int tries;
+	int voltage_tries, loop_tries;
 	u32 reg;
 	uint32_t DP = intel_dp->DP;
 
@@ -1620,7 +1621,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 		DP &= ~DP_LINK_TRAIN_MASK;
 	memset(intel_dp->train_set, 0, 4);
 	voltage = 0xff;
-	tries = 0;
+	voltage_tries = 0;
+	loop_tries = 0;
 	clock_recovery = false;
 	for (;;) {
 		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
@@ -1663,17 +1665,28 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 		for (i = 0; i < intel_dp->lane_count; i++)
 			if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
 				break;
-		if (i == intel_dp->lane_count)
-			break;
-
-		/* Check to see if we've tried the same voltage 5 times */
-		if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
-			++tries;
-			if (tries == 5)
+		if (i == intel_dp->lane_count) {
+			++loop_tries;
+			if (loop_tries == 5) {
+				DRM_DEBUG_KMS("too many full retries, give up\n");
 				break;
-		} else
-			tries = 0;
-		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
+			}
+			memset(intel_dp->train_set, 0, 4);
+			voltage_tries = 0;
+			continue;
+		} else {
+
+			/* Check to see if we've tried the same voltage 5 times */
+			if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
+				++voltage_tries;
+				if (voltage_tries == 5) {
+					DRM_DEBUG_KMS("too many voltage retries, give up\n");
+					break;
+				}
+			} else
+				voltage_tries = 0;
+			voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
+		}
 
 		/* Compute new intel_dp->train_set as requested by target */
 		intel_get_adjust_train(intel_dp, link_status);
-- 
1.7.7

Re: [PATCH 6/7] drm/i915: Try harder during dp pattern 1 link training

[Chris Wilson]

On Tue,  1 Nov 2011 23:20:29 -0700, Keith Packard <keithp@keithp.com> wrote:
> Instead of going through the sequence just once, run through the whole
> set up to 5 times to see if something can work. This isn't part of the
> DP spec, but the BIOS seems to do it, and given that link training
> failure is so bad, it seems reasonable to follow suit.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   41 +++++++++++++++++++++++++-------------
>  1 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6be6a04..bf20a35 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1576,8 +1576,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>  
>  	ret = intel_dp_aux_native_write(intel_dp,
>  					DP_TRAINING_LANE0_SET,
> -					intel_dp->train_set, 4);
> -	if (ret != 4)
> +					intel_dp->train_set,
> +					intel_dp->lane_count);
> +	if (ret != intel_dp->lane_count)
>  		return false;

This would seem to be a separate chunk to initiate training on only the
lanes we intend to use.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7] drm/i915: Try harder during dp pattern 1 link training

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

On Wed, 02 Nov 2011 09:12:13 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> This would seem to be a separate chunk to initiate training on only the
> lanes we intend to use.

Yeah, this got left in during some debugging; it might be a good thing
to do, but it's completely separate. I've pulled it out into a separate
patch.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH 6/7] drm/i915: Try harder during dp pattern 1 link training

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

On Wed, 02 Nov 2011 09:12:13 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> This would seem to be a separate chunk to initiate training on only the
> lanes we intend to use.
> -Chris

Here's that patch separated out:

commit e7fecae483754ca9a42312be18f58ceb454702fe
Author: Keith Packard <keithp@keithp.com>
Date:   Wed Nov 2 10:17:59 2011 -0700

    drm/i915: Initiate DP link training only on the lanes we'll be using
    
    Limit the link training setting command to the lanes needed for the
    current mode. It seems vaguely possible that a monitor will try to
    train the other lanes and fail in some way, so this seems like the
    safer plan.
    
    Signed-off-by: Keith Packard <keithp@keithp.com>

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0a4fa64..02b56ce 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1567,8 +1567,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 
        ret = intel_dp_aux_native_write(intel_dp,
                                        DP_TRAINING_LANE0_SET,
-                                       intel_dp->train_set, 4);
-       if (ret != 4)
+                                       intel_dp->train_set,
+                                       intel_dp->lane_count);
+       if (ret != intel_dp->lane_count)
                return false;
 
        return true;


-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 6/7] drm/i915: Try harder during dp pattern 1 link training

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 3453 bytes --]

On Tue,  1 Nov 2011 23:20:29 -0700
Keith Packard <keithp@keithp.com> wrote:

> Instead of going through the sequence just once, run through the whole
> set up to 5 times to see if something can work. This isn't part of the
> DP spec, but the BIOS seems to do it, and given that link training
> failure is so bad, it seems reasonable to follow suit.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   41 +++++++++++++++++++++++++-------------
>  1 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6be6a04..bf20a35 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1576,8 +1576,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>  
>  	ret = intel_dp_aux_native_write(intel_dp,
>  					DP_TRAINING_LANE0_SET,
> -					intel_dp->train_set, 4);
> -	if (ret != 4)
> +					intel_dp->train_set,
> +					intel_dp->lane_count);
> +	if (ret != intel_dp->lane_count)
>  		return false;
>  
>  	return true;

Sneaky putting this bug fix into this patch. :)

> @@ -1593,7 +1594,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>  	int i;
>  	uint8_t voltage;
>  	bool clock_recovery = false;
> -	int tries;
> +	int voltage_tries, loop_tries;
>  	u32 reg;
>  	uint32_t DP = intel_dp->DP;
>  
> @@ -1620,7 +1621,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>  		DP &= ~DP_LINK_TRAIN_MASK;
>  	memset(intel_dp->train_set, 0, 4);
>  	voltage = 0xff;
> -	tries = 0;
> +	voltage_tries = 0;
> +	loop_tries = 0;
>  	clock_recovery = false;
>  	for (;;) {
>  		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
> @@ -1663,17 +1665,28 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>  		for (i = 0; i < intel_dp->lane_count; i++)
>  			if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
>  				break;
> -		if (i == intel_dp->lane_count)
> -			break;
> -
> -		/* Check to see if we've tried the same voltage 5 times */
> -		if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
> -			++tries;
> -			if (tries == 5)
> +		if (i == intel_dp->lane_count) {
> +			++loop_tries;
> +			if (loop_tries == 5) {
> +				DRM_DEBUG_KMS("too many full retries, give up\n");
>  				break;
> -		} else
> -			tries = 0;
> -		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
> +			}
> +			memset(intel_dp->train_set, 0, 4);
> +			voltage_tries = 0;
> +			continue;
> +		} else {
> +
> +			/* Check to see if we've tried the same voltage 5 times */
> +			if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
> +				++voltage_tries;
> +				if (voltage_tries == 5) {
> +					DRM_DEBUG_KMS("too many voltage retries, give up\n");
> +					break;
> +				}
> +			} else
> +				voltage_tries = 0;
> +			voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
> +		}
>  
>  		/* Compute new intel_dp->train_set as requested by target */
>  		intel_get_adjust_train(intel_dp, link_status);

Don't you love the training state machine?  I think this looks ok, and
the DP compliance test should catch any grievous errors, so aside from
the bug fix hunk which should be split out:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 6/7] drm/i915: Try harder during dp pattern 1 link training

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Thu, 3 Nov 2011 13:03:29 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Sneaky putting this bug fix into this patch. :)

Ickle already saw that, and I've split it out into a separate patch. I
don't think this is necessary, but it seems like a sensible change.

> Don't you love the training state machine?  I think this looks ok, and
> the DP compliance test should catch any grievous errors, so aside from
> the bug fix hunk which should be split out:
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

And that chunk has already been split out :-)

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

[PATCH 7/7] drm/i915: Remove trailing white space

[Keith Packard]

Found a couple of bare tabs in intel_dp.c

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bf20a35..7ebeb01 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1054,7 +1054,7 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 
 	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->want_panel_vdd);
 	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
-	
+
 	intel_dp->want_panel_vdd = false;
 
 	if (sync) {
@@ -2402,7 +2402,7 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 
 		cur.t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
 			PANEL_LIGHT_ON_DELAY_SHIFT;
-		
+
 		cur.t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
 			PANEL_LIGHT_OFF_DELAY_SHIFT;
 
-- 
1.7.7

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Remove trailing white space

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On Tue,  1 Nov 2011 23:20:30 -0700
Keith Packard <keithp@keithp.com> wrote:

> Found a couple of bare tabs in intel_dp.c
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bf20a35..7ebeb01 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1054,7 +1054,7 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  
>  	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->want_panel_vdd);
>  	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
> -	
> +
>  	intel_dp->want_panel_vdd = false;
>  
>  	if (sync) {
> @@ -2402,7 +2402,7 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  
>  		cur.t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
>  			PANEL_LIGHT_ON_DELAY_SHIFT;
> -		
> +
>  		cur.t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
>  			PANEL_LIGHT_OFF_DELAY_SHIFT;
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Remove trailing white space

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

On Thu, 3 Nov 2011 13:03:49 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thanks for your careful patch review here.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Remove trailing white space

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

On Thu, 3 Nov 2011 13:03:49 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I've updated the pch-edp-fixes branch with your suggestions and attached
your R-b: to the reviewed patches.

That leaves only the panel power sequencing changes, which could
probably use more testing to make sure no existing systems stop
working. I've got an ILK eDP system here that I haven't tested yet, so I
can do that. I have tested SNB CPU eDP on the MacBook and CPT PCH eDP on
the other system.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]