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).
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);
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?
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.
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);
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
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; }
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
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.
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; } }
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?
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.
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
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?
On 11/2/11 1:31 PM, Keith Packard wrote:
On Wed, 02 Nov 2011 13:13:52 -0400, Adam Jacksonajax@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
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; } }
On 11/2/11 4:05 PM, Keith Packard wrote:
On Wed, 02 Nov 2011 15:36:20 -0400, Adam Jacksonajax@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 Packardkeithp@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
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; } }
On 11/2/11 5:13 PM, Keith Packard wrote:
On Wed, 02 Nov 2011 16:35:51 -0400, Adam Jacksonajax@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
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?
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
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 =
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)
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...
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.
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) {
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
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.
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);
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:
- The backlight must be off unless the screen is running
- The link must be running to turn the eDP panel on/off
- 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);ironlake_edp_panel_off(intel_dp);if (is_edp(intel_dp) && !is_pch_edp(intel_dp))ironlake_edp_pll_off(encoder);
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?
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.
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.
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.
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);
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
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.
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;
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;
} elsetries = 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;}} elsevoltage_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
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 :-)
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;
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
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.
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.
dri-devel@lists.freedesktop.org
-
Adam Jackson -
Alex Deucher -
Chris Wilson -
Jesse Barnes -
Keith Packard