* [PATCH] drm/i915: Improve reliability for Displayport link training
@ 2014-10-09 16:39 Todd Previte
2014-10-21 14:45 ` Daniel Vetter
2014-10-22 14:22 ` Jani Nikula
0 siblings, 2 replies; 6+ messages in thread
From: Todd Previte @ 2014-10-09 16:39 UTC (permalink / raw)
To: intel-gfx
Link training for Displayport can fail in many ways and at multiple different points
during the training process. Previously, errors were logged but no additional action
was taken based on them. Consequently, training attempts could continue even after
errors have occured that would prevent successful link training. This patch updates
the link training functions and where/how they're used to be more intelligent about
failures and to stop trying to train the link when it's a lost cause.
Signed-off-by: Todd Previte <tprevite@gmail.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++--
drivers/gpu/drm/i915/intel_dp.c | 88 ++++++++++++++++++++++++++++++----------
drivers/gpu/drm/i915/intel_drv.h | 10 +++--
3 files changed, 95 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index cb5367c..718240f 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1119,6 +1119,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
enum port port = intel_ddi_get_encoder_port(intel_encoder);
int type = intel_encoder->type;
+ uint8_t fail_code = 0;
+ char *msg;
if (crtc->config.has_audio) {
DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
@@ -1143,10 +1145,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
intel_ddi_init_dp_buf_reg(intel_encoder);
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
- intel_dp_start_link_train(intel_dp);
- intel_dp_complete_link_train(intel_dp);
+ if (!intel_dp_start_link_train(intel_dp)) {
+ fail_code = INTEL_DP_CLOCKREC_FAILED;
+ msg = "clock recovery";
+ goto failed;
+ }
+ if (!intel_dp_complete_link_train(intel_dp)) {
+ fail_code = INTEL_DP_CHANNELEQ_FAILED;
+ msg = "channel equalization";
+ goto failed;
+ }
if (port != PORT_A)
- intel_dp_stop_link_train(intel_dp);
+ if (!intel_dp_stop_link_train(intel_dp)) {
+ fail_code = INTEL_DP_STOPTRAIN_FAILED;
+ msg = "stop training";
+ goto failed;
+ }
} else if (type == INTEL_OUTPUT_HDMI) {
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
@@ -1154,6 +1168,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
crtc->config.has_hdmi_sink,
&crtc->config.adjusted_mode);
}
+
+ return;
+
+failed:
+ DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
}
static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..a8352c4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2538,18 +2538,38 @@ static void intel_enable_dp(struct intel_encoder *encoder)
struct drm_device *dev = encoder->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
uint32_t dp_reg = I915_READ(intel_dp->output_reg);
+ uint8_t fail_code = 0;
+ char *msg;
if (WARN_ON(dp_reg & DP_PORT_EN))
- return;
+ return false;
intel_dp_enable_port(intel_dp);
intel_edp_panel_vdd_on(intel_dp);
intel_edp_panel_on(intel_dp);
intel_edp_panel_vdd_off(intel_dp, true);
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
- intel_dp_start_link_train(intel_dp);
- intel_dp_complete_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
+ if (!intel_dp_start_link_train(intel_dp)) {
+ fail_code = INTEL_DP_CLOCKREC_FAILED;
+ msg = "clock recovery";
+ goto failed;
+ }
+ if (!intel_dp_complete_link_train(intel_dp)) {
+ fail_code = INTEL_DP_CHANNELEQ_FAILED;
+ msg = "channel equalization";
+ goto failed;
+ }
+ if (!intel_dp_stop_link_train(intel_dp)) {
+ fail_code = INTEL_DP_STOPTRAIN_FAILED;
+ msg = "stop training";
+ goto failed;
+ }
+
+ return true;
+
+failed:
+ DRM_DEBUG_KMS("Failed to train DP link - %s failed\n", msg);
+ return false;
}
static void g4x_enable_dp(struct intel_encoder *encoder)
@@ -3513,7 +3533,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
return ret == intel_dp->lane_count;
}
-static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
+void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
{
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = intel_dig_port->base.base.dev;
@@ -3545,7 +3565,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
}
/* Enable corresponding port and start training pattern 1 */
-void
+bool
intel_dp_start_link_train(struct intel_dp *intel_dp)
{
struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
@@ -3564,11 +3584,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
link_config[1] = intel_dp->lane_count;
if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
- drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
+ if (drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2) != 2) {
+ DRM_DEBUG_KMS("Failed to write sink DPCD for link rate and lane count\n");
+ goto failed;
+ }
link_config[0] = 0;
link_config[1] = DP_SET_ANSI_8B10B;
- drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
+ if (drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2) != 2) {
+ DRM_DEBUG_KMS("Failed to write sink DPCD for downspread control\n");
+ goto failed;
+ }
DP |= DP_PORT_EN;
@@ -3577,7 +3603,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to enable link training\n");
- return;
+ goto failed;
}
voltage = 0xff;
@@ -3589,12 +3615,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
if (!intel_dp_get_link_status(intel_dp, link_status)) {
DRM_ERROR("failed to get link status\n");
- break;
+ goto failed;
}
if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
DRM_DEBUG_KMS("clock recovery OK\n");
- break;
+ goto cr_done;
}
/* Check to see if we've tried the max voltage */
@@ -3605,7 +3631,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
++loop_tries;
if (loop_tries == 5) {
DRM_ERROR("too many full retries, give up\n");
- break;
+ goto failed;
}
intel_dp_reset_link_train(intel_dp, &DP,
DP_TRAINING_PATTERN_1 |
@@ -3619,7 +3645,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
++voltage_tries;
if (voltage_tries == 5) {
DRM_ERROR("too many voltage retries, give up\n");
- break;
+ goto failed;
}
} else
voltage_tries = 0;
@@ -3628,14 +3654,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
/* Update training set as requested by target */
if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
DRM_ERROR("failed to update link training\n");
- break;
+ goto failed;
}
}
+cr_done:
intel_dp->DP = DP;
+ return true;
+
+failed:
+ DRM_DEBUG_KMS("Failed to initiate link training\n");
+ return false;
}
-void
+bool
intel_dp_complete_link_train(struct intel_dp *intel_dp)
{
bool channel_eq = false;
@@ -3652,7 +3684,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
training_pattern |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to start channel equalization\n");
- return;
+ return false;
}
tries = 0;
@@ -3711,14 +3743,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
intel_dp->DP = DP;
- if (channel_eq)
+ if (channel_eq) {
DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
+ return true;
+ }
+ return false;
}
-void intel_dp_stop_link_train(struct intel_dp *intel_dp)
+bool intel_dp_stop_link_train(struct intel_dp *intel_dp)
{
- intel_dp_set_link_train(intel_dp, &intel_dp->DP,
+ return intel_dp_set_link_train(intel_dp, &intel_dp->DP,
DP_TRAINING_PATTERN_DISABLE);
}
@@ -4077,9 +4112,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
intel_encoder->base.name);
- intel_dp_start_link_train(intel_dp);
- intel_dp_complete_link_train(intel_dp);
- intel_dp_stop_link_train(intel_dp);
+ if (!intel_dp_start_link_train(intel_dp)) {
+ DRM_DEBUG_KMS("Start link training failed\n");
+ return;
+ }
+ if (!intel_dp_complete_link_train(intel_dp)) {
+ DRM_DEBUG_KMS("Complete link training failed\n");
+ return;
+ }
+ if (!intel_dp_stop_link_train(intel_dp)) {
+ DRM_DEBUG_KMS("Stop link training failed\n");
+ return;
+ }
}
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e8f4839..dc80444 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -949,12 +949,16 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
/* intel_dp.c */
+#define INTEL_DP_CLOCKREC_FAILED 1
+#define INTEL_DP_CHANNELEQ_FAILED 2
+#define INTEL_DP_STOPTRAIN_FAILED 3
void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
struct intel_connector *intel_connector);
-void intel_dp_start_link_train(struct intel_dp *intel_dp);
-void intel_dp_complete_link_train(struct intel_dp *intel_dp);
-void intel_dp_stop_link_train(struct intel_dp *intel_dp);
+bool intel_dp_start_link_train(struct intel_dp *intel_dp);
+bool intel_dp_complete_link_train(struct intel_dp *intel_dp);
+bool intel_dp_stop_link_train(struct intel_dp *intel_dp);
+void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
void intel_dp_encoder_destroy(struct drm_encoder *encoder);
void intel_dp_check_link_status(struct intel_dp *intel_dp);
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Improve reliability for Displayport link training
2014-10-09 16:39 [PATCH] drm/i915: Improve reliability for Displayport link training Todd Previte
@ 2014-10-21 14:45 ` Daniel Vetter
2014-10-23 13:58 ` Todd Previte
2014-10-22 14:22 ` Jani Nikula
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-10-21 14:45 UTC (permalink / raw)
To: Todd Previte; +Cc: intel-gfx
On Thu, Oct 09, 2014 at 09:39:01AM -0700, Todd Previte wrote:
> Link training for Displayport can fail in many ways and at multiple different points
> during the training process. Previously, errors were logged but no additional action
> was taken based on them. Consequently, training attempts could continue even after
> errors have occured that would prevent successful link training. This patch updates
> the link training functions and where/how they're used to be more intelligent about
> failures and to stop trying to train the link when it's a lost cause.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++--
> drivers/gpu/drm/i915/intel_dp.c | 88 ++++++++++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 10 +++--
> 3 files changed, 95 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cb5367c..718240f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1119,6 +1119,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> enum port port = intel_ddi_get_encoder_port(intel_encoder);
> int type = intel_encoder->type;
> + uint8_t fail_code = 0;
> + char *msg;
>
> if (crtc->config.has_audio) {
> DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
> @@ -1143,10 +1145,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> intel_ddi_init_dp_buf_reg(intel_encoder);
>
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_complete_link_train(intel_dp);
> + if (!intel_dp_start_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CLOCKREC_FAILED;
> + msg = "clock recovery";
> + goto failed;
> + }
I think all the additional debug output lines here just confuse the
control flow. All the functions augment with return values already have
debug output, so I don't think we need this. And if we've missed
something, then maybe we should add the debug output in the function, not
at every call site.
Otherwise seems to make sense, but I'm not a DP expert.
-Daniel
> + if (!intel_dp_complete_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
> + msg = "channel equalization";
> + goto failed;
> + }
> if (port != PORT_A)
> - intel_dp_stop_link_train(intel_dp);
> + if (!intel_dp_stop_link_train(intel_dp)) {
> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
> + msg = "stop training";
> + goto failed;
> + }
> } else if (type == INTEL_OUTPUT_HDMI) {
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>
> @@ -1154,6 +1168,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> crtc->config.has_hdmi_sink,
> &crtc->config.adjusted_mode);
> }
> +
> + return;
> +
> +failed:
> + DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
> }
>
> static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 64c8e04..a8352c4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2538,18 +2538,38 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> struct drm_device *dev = encoder->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> + uint8_t fail_code = 0;
> + char *msg;
>
> if (WARN_ON(dp_reg & DP_PORT_EN))
> - return;
> + return false;
>
> intel_dp_enable_port(intel_dp);
> intel_edp_panel_vdd_on(intel_dp);
> intel_edp_panel_on(intel_dp);
> intel_edp_panel_vdd_off(intel_dp, true);
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_complete_link_train(intel_dp);
> - intel_dp_stop_link_train(intel_dp);
> + if (!intel_dp_start_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CLOCKREC_FAILED;
> + msg = "clock recovery";
> + goto failed;
> + }
> + if (!intel_dp_complete_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
> + msg = "channel equalization";
> + goto failed;
> + }
> + if (!intel_dp_stop_link_train(intel_dp)) {
> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
> + msg = "stop training";
> + goto failed;
> + }
> +
> + return true;
> +
> +failed:
> + DRM_DEBUG_KMS("Failed to train DP link - %s failed\n", msg);
> + return false;
> }
>
> static void g4x_enable_dp(struct intel_encoder *encoder)
> @@ -3513,7 +3533,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> return ret == intel_dp->lane_count;
> }
>
> -static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> {
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct drm_device *dev = intel_dig_port->base.base.dev;
> @@ -3545,7 +3565,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> }
>
> /* Enable corresponding port and start training pattern 1 */
> -void
> +bool
> intel_dp_start_link_train(struct intel_dp *intel_dp)
> {
> struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
> @@ -3564,11 +3584,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> link_config[1] = intel_dp->lane_count;
> if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> - drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2) != 2) {
> + DRM_DEBUG_KMS("Failed to write sink DPCD for link rate and lane count\n");
> + goto failed;
> + }
>
> link_config[0] = 0;
> link_config[1] = DP_SET_ANSI_8B10B;
> - drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2) != 2) {
> + DRM_DEBUG_KMS("Failed to write sink DPCD for downspread control\n");
> + goto failed;
> + }
>
> DP |= DP_PORT_EN;
>
> @@ -3577,7 +3603,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> DP_TRAINING_PATTERN_1 |
> DP_LINK_SCRAMBLING_DISABLE)) {
> DRM_ERROR("failed to enable link training\n");
> - return;
> + goto failed;
> }
>
> voltage = 0xff;
> @@ -3589,12 +3615,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> if (!intel_dp_get_link_status(intel_dp, link_status)) {
> DRM_ERROR("failed to get link status\n");
> - break;
> + goto failed;
> }
>
> if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> DRM_DEBUG_KMS("clock recovery OK\n");
> - break;
> + goto cr_done;
> }
>
> /* Check to see if we've tried the max voltage */
> @@ -3605,7 +3631,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> ++loop_tries;
> if (loop_tries == 5) {
> DRM_ERROR("too many full retries, give up\n");
> - break;
> + goto failed;
> }
> intel_dp_reset_link_train(intel_dp, &DP,
> DP_TRAINING_PATTERN_1 |
> @@ -3619,7 +3645,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> ++voltage_tries;
> if (voltage_tries == 5) {
> DRM_ERROR("too many voltage retries, give up\n");
> - break;
> + goto failed;
> }
> } else
> voltage_tries = 0;
> @@ -3628,14 +3654,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> /* Update training set as requested by target */
> if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
> DRM_ERROR("failed to update link training\n");
> - break;
> + goto failed;
> }
> }
>
> +cr_done:
> intel_dp->DP = DP;
> + return true;
> +
> +failed:
> + DRM_DEBUG_KMS("Failed to initiate link training\n");
> + return false;
> }
>
> -void
> +bool
> intel_dp_complete_link_train(struct intel_dp *intel_dp)
> {
> bool channel_eq = false;
> @@ -3652,7 +3684,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> training_pattern |
> DP_LINK_SCRAMBLING_DISABLE)) {
> DRM_ERROR("failed to start channel equalization\n");
> - return;
> + return false;
> }
>
> tries = 0;
> @@ -3711,14 +3743,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>
> intel_dp->DP = DP;
>
> - if (channel_eq)
> + if (channel_eq) {
> DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
> + return true;
> + }
>
> + return false;
> }
>
> -void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp)
> {
> - intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> + return intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> DP_TRAINING_PATTERN_DISABLE);
> }
>
> @@ -4077,9 +4112,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> intel_encoder->base.name);
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_complete_link_train(intel_dp);
> - intel_dp_stop_link_train(intel_dp);
> + if (!intel_dp_start_link_train(intel_dp)) {
> + DRM_DEBUG_KMS("Start link training failed\n");
> + return;
> + }
> + if (!intel_dp_complete_link_train(intel_dp)) {
> + DRM_DEBUG_KMS("Complete link training failed\n");
> + return;
> + }
> + if (!intel_dp_stop_link_train(intel_dp)) {
> + DRM_DEBUG_KMS("Stop link training failed\n");
> + return;
> + }
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e8f4839..dc80444 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -949,12 +949,16 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
> void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>
> /* intel_dp.c */
> +#define INTEL_DP_CLOCKREC_FAILED 1
> +#define INTEL_DP_CHANNELEQ_FAILED 2
> +#define INTEL_DP_STOPTRAIN_FAILED 3
> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> struct intel_connector *intel_connector);
> -void intel_dp_start_link_train(struct intel_dp *intel_dp);
> -void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> -void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_start_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_complete_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> void intel_dp_check_link_status(struct intel_dp *intel_dp);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Improve reliability for Displayport link training
2014-10-09 16:39 [PATCH] drm/i915: Improve reliability for Displayport link training Todd Previte
2014-10-21 14:45 ` Daniel Vetter
@ 2014-10-22 14:22 ` Jani Nikula
2014-10-23 16:56 ` Todd Previte
1 sibling, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2014-10-22 14:22 UTC (permalink / raw)
To: Todd Previte, intel-gfx
On Thu, 09 Oct 2014, Todd Previte <tprevite@gmail.com> wrote:
> Link training for Displayport can fail in many ways and at multiple different points
> during the training process. Previously, errors were logged but no additional action
> was taken based on them. Consequently, training attempts could continue even after
> errors have occured that would prevent successful link training. This patch updates
> the link training functions and where/how they're used to be more intelligent about
> failures and to stop trying to train the link when it's a lost cause.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++--
> drivers/gpu/drm/i915/intel_dp.c | 88 ++++++++++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 10 +++--
> 3 files changed, 95 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cb5367c..718240f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1119,6 +1119,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> enum port port = intel_ddi_get_encoder_port(intel_encoder);
> int type = intel_encoder->type;
> + uint8_t fail_code = 0;
> + char *msg;
>
> if (crtc->config.has_audio) {
> DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
> @@ -1143,10 +1145,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> intel_ddi_init_dp_buf_reg(intel_encoder);
>
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_complete_link_train(intel_dp);
> + if (!intel_dp_start_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CLOCKREC_FAILED;
> + msg = "clock recovery";
> + goto failed;
> + }
> + if (!intel_dp_complete_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
> + msg = "channel equalization";
> + goto failed;
> + }
> if (port != PORT_A)
> - intel_dp_stop_link_train(intel_dp);
> + if (!intel_dp_stop_link_train(intel_dp)) {
> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
> + msg = "stop training";
> + goto failed;
> + }
> } else if (type == INTEL_OUTPUT_HDMI) {
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>
> @@ -1154,6 +1168,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> crtc->config.has_hdmi_sink,
> &crtc->config.adjusted_mode);
> }
> +
> + return;
> +
> +failed:
> + DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
> }
>
> static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 64c8e04..a8352c4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2538,18 +2538,38 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> struct drm_device *dev = encoder->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> + uint8_t fail_code = 0;
> + char *msg;
>
> if (WARN_ON(dp_reg & DP_PORT_EN))
> - return;
> + return false;
>
> intel_dp_enable_port(intel_dp);
> intel_edp_panel_vdd_on(intel_dp);
> intel_edp_panel_on(intel_dp);
> intel_edp_panel_vdd_off(intel_dp, true);
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_complete_link_train(intel_dp);
> - intel_dp_stop_link_train(intel_dp);
> + if (!intel_dp_start_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CLOCKREC_FAILED;
> + msg = "clock recovery";
> + goto failed;
> + }
> + if (!intel_dp_complete_link_train(intel_dp)) {
> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
> + msg = "channel equalization";
> + goto failed;
> + }
> + if (!intel_dp_stop_link_train(intel_dp)) {
> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
> + msg = "stop training";
> + goto failed;
> + }
In general I like failing fast and bailing out. However the users
probably like it better if we limp on and keep trying to get a picture
on screen. It's also much less stressful to work on bugs that cause a
warn in the logs instead of a black screen.
Case in point [1] which would end up in a black screen with this
patch. Unless we've managed to fix it by now.
BR,
Jani.
[1] https://bugs.freedesktop.org/show_bug.cgi?id=70117
> +
> + return true;
> +
> +failed:
> + DRM_DEBUG_KMS("Failed to train DP link - %s failed\n", msg);
> + return false;
> }
>
> static void g4x_enable_dp(struct intel_encoder *encoder)
> @@ -3513,7 +3533,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> return ret == intel_dp->lane_count;
> }
>
> -static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> {
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct drm_device *dev = intel_dig_port->base.base.dev;
> @@ -3545,7 +3565,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> }
>
> /* Enable corresponding port and start training pattern 1 */
> -void
> +bool
> intel_dp_start_link_train(struct intel_dp *intel_dp)
> {
> struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
> @@ -3564,11 +3584,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> link_config[1] = intel_dp->lane_count;
> if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> - drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2) != 2) {
> + DRM_DEBUG_KMS("Failed to write sink DPCD for link rate and lane count\n");
> + goto failed;
> + }
>
> link_config[0] = 0;
> link_config[1] = DP_SET_ANSI_8B10B;
> - drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2) != 2) {
> + DRM_DEBUG_KMS("Failed to write sink DPCD for downspread control\n");
> + goto failed;
> + }
>
> DP |= DP_PORT_EN;
>
> @@ -3577,7 +3603,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> DP_TRAINING_PATTERN_1 |
> DP_LINK_SCRAMBLING_DISABLE)) {
> DRM_ERROR("failed to enable link training\n");
> - return;
> + goto failed;
> }
>
> voltage = 0xff;
> @@ -3589,12 +3615,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> if (!intel_dp_get_link_status(intel_dp, link_status)) {
> DRM_ERROR("failed to get link status\n");
> - break;
> + goto failed;
> }
>
> if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> DRM_DEBUG_KMS("clock recovery OK\n");
> - break;
> + goto cr_done;
> }
>
> /* Check to see if we've tried the max voltage */
> @@ -3605,7 +3631,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> ++loop_tries;
> if (loop_tries == 5) {
> DRM_ERROR("too many full retries, give up\n");
> - break;
> + goto failed;
> }
> intel_dp_reset_link_train(intel_dp, &DP,
> DP_TRAINING_PATTERN_1 |
> @@ -3619,7 +3645,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> ++voltage_tries;
> if (voltage_tries == 5) {
> DRM_ERROR("too many voltage retries, give up\n");
> - break;
> + goto failed;
> }
> } else
> voltage_tries = 0;
> @@ -3628,14 +3654,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> /* Update training set as requested by target */
> if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
> DRM_ERROR("failed to update link training\n");
> - break;
> + goto failed;
> }
> }
>
> +cr_done:
> intel_dp->DP = DP;
> + return true;
> +
> +failed:
> + DRM_DEBUG_KMS("Failed to initiate link training\n");
> + return false;
> }
>
> -void
> +bool
> intel_dp_complete_link_train(struct intel_dp *intel_dp)
> {
> bool channel_eq = false;
> @@ -3652,7 +3684,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> training_pattern |
> DP_LINK_SCRAMBLING_DISABLE)) {
> DRM_ERROR("failed to start channel equalization\n");
> - return;
> + return false;
> }
>
> tries = 0;
> @@ -3711,14 +3743,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>
> intel_dp->DP = DP;
>
> - if (channel_eq)
> + if (channel_eq) {
> DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
> + return true;
> + }
>
> + return false;
> }
>
> -void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp)
> {
> - intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> + return intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> DP_TRAINING_PATTERN_DISABLE);
> }
>
> @@ -4077,9 +4112,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> intel_encoder->base.name);
> - intel_dp_start_link_train(intel_dp);
> - intel_dp_complete_link_train(intel_dp);
> - intel_dp_stop_link_train(intel_dp);
> + if (!intel_dp_start_link_train(intel_dp)) {
> + DRM_DEBUG_KMS("Start link training failed\n");
> + return;
> + }
> + if (!intel_dp_complete_link_train(intel_dp)) {
> + DRM_DEBUG_KMS("Complete link training failed\n");
> + return;
> + }
> + if (!intel_dp_stop_link_train(intel_dp)) {
> + DRM_DEBUG_KMS("Stop link training failed\n");
> + return;
> + }
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e8f4839..dc80444 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -949,12 +949,16 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
> void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>
> /* intel_dp.c */
> +#define INTEL_DP_CLOCKREC_FAILED 1
> +#define INTEL_DP_CHANNELEQ_FAILED 2
> +#define INTEL_DP_STOPTRAIN_FAILED 3
> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> struct intel_connector *intel_connector);
> -void intel_dp_start_link_train(struct intel_dp *intel_dp);
> -void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> -void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_start_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_complete_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> void intel_dp_check_link_status(struct intel_dp *intel_dp);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Improve reliability for Displayport link training
2014-10-21 14:45 ` Daniel Vetter
@ 2014-10-23 13:58 ` Todd Previte
0 siblings, 0 replies; 6+ messages in thread
From: Todd Previte @ 2014-10-23 13:58 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On 10/21/2014 7:45 AM, Daniel Vetter wrote:
> On Thu, Oct 09, 2014 at 09:39:01AM -0700, Todd Previte wrote:
>> Link training for Displayport can fail in many ways and at multiple different points
>> during the training process. Previously, errors were logged but no additional action
>> was taken based on them. Consequently, training attempts could continue even after
>> errors have occured that would prevent successful link training. This patch updates
>> the link training functions and where/how they're used to be more intelligent about
>> failures and to stop trying to train the link when it's a lost cause.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>> drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++--
>> drivers/gpu/drm/i915/intel_dp.c | 88 ++++++++++++++++++++++++++++++----------
>> drivers/gpu/drm/i915/intel_drv.h | 10 +++--
>> 3 files changed, 95 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index cb5367c..718240f 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1119,6 +1119,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>> enum port port = intel_ddi_get_encoder_port(intel_encoder);
>> int type = intel_encoder->type;
>> + uint8_t fail_code = 0;
>> + char *msg;
>>
>> if (crtc->config.has_audio) {
>> DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
>> @@ -1143,10 +1145,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> intel_ddi_init_dp_buf_reg(intel_encoder);
>>
>> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>> - intel_dp_start_link_train(intel_dp);
>> - intel_dp_complete_link_train(intel_dp);
>> + if (!intel_dp_start_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CLOCKREC_FAILED;
>> + msg = "clock recovery";
>> + goto failed;
>> + }
> I think all the additional debug output lines here just confuse the
> control flow. All the functions augment with return values already have
> debug output, so I don't think we need this. And if we've missed
> something, then maybe we should add the debug output in the function, not
> at every call site.
>
> Otherwise seems to make sense, but I'm not a DP expert.
> -Daniel
There's enough debug messaging in the training functions that the extra
at the top level isn't necessary, nor are the unconditional jumps. Those
will be removed for version 2. Thanks Daniel.
>> + if (!intel_dp_complete_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
>> + msg = "channel equalization";
>> + goto failed;
>> + }
>> if (port != PORT_A)
>> - intel_dp_stop_link_train(intel_dp);
>> + if (!intel_dp_stop_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
>> + msg = "stop training";
>> + goto failed;
>> + }
>> } else if (type == INTEL_OUTPUT_HDMI) {
>> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>>
>> @@ -1154,6 +1168,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> crtc->config.has_hdmi_sink,
>> &crtc->config.adjusted_mode);
>> }
>> +
>> + return;
>> +
>> +failed:
>> + DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
>> }
>>
>> static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 64c8e04..a8352c4 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2538,18 +2538,38 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>> struct drm_device *dev = encoder->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> uint32_t dp_reg = I915_READ(intel_dp->output_reg);
>> + uint8_t fail_code = 0;
>> + char *msg;
>>
>> if (WARN_ON(dp_reg & DP_PORT_EN))
>> - return;
>> + return false;
>>
>> intel_dp_enable_port(intel_dp);
>> intel_edp_panel_vdd_on(intel_dp);
>> intel_edp_panel_on(intel_dp);
>> intel_edp_panel_vdd_off(intel_dp, true);
>> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>> - intel_dp_start_link_train(intel_dp);
>> - intel_dp_complete_link_train(intel_dp);
>> - intel_dp_stop_link_train(intel_dp);
>> + if (!intel_dp_start_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CLOCKREC_FAILED;
>> + msg = "clock recovery";
>> + goto failed;
>> + }
>> + if (!intel_dp_complete_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
>> + msg = "channel equalization";
>> + goto failed;
>> + }
>> + if (!intel_dp_stop_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
>> + msg = "stop training";
>> + goto failed;
>> + }
>> +
>> + return true;
>> +
>> +failed:
>> + DRM_DEBUG_KMS("Failed to train DP link - %s failed\n", msg);
>> + return false;
>> }
>>
>> static void g4x_enable_dp(struct intel_encoder *encoder)
>> @@ -3513,7 +3533,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>> return ret == intel_dp->lane_count;
>> }
>>
>> -static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>> {
>> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> struct drm_device *dev = intel_dig_port->base.base.dev;
>> @@ -3545,7 +3565,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>> }
>>
>> /* Enable corresponding port and start training pattern 1 */
>> -void
>> +bool
>> intel_dp_start_link_train(struct intel_dp *intel_dp)
>> {
>> struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
>> @@ -3564,11 +3584,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> link_config[1] = intel_dp->lane_count;
>> if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>> link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> - drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2) != 2) {
>> + DRM_DEBUG_KMS("Failed to write sink DPCD for link rate and lane count\n");
>> + goto failed;
>> + }
>>
>> link_config[0] = 0;
>> link_config[1] = DP_SET_ANSI_8B10B;
>> - drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2) != 2) {
>> + DRM_DEBUG_KMS("Failed to write sink DPCD for downspread control\n");
>> + goto failed;
>> + }
>>
>> DP |= DP_PORT_EN;
>>
>> @@ -3577,7 +3603,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> DP_TRAINING_PATTERN_1 |
>> DP_LINK_SCRAMBLING_DISABLE)) {
>> DRM_ERROR("failed to enable link training\n");
>> - return;
>> + goto failed;
>> }
>>
>> voltage = 0xff;
>> @@ -3589,12 +3615,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
>> if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> DRM_ERROR("failed to get link status\n");
>> - break;
>> + goto failed;
>> }
>>
>> if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
>> DRM_DEBUG_KMS("clock recovery OK\n");
>> - break;
>> + goto cr_done;
>> }
>>
>> /* Check to see if we've tried the max voltage */
>> @@ -3605,7 +3631,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> ++loop_tries;
>> if (loop_tries == 5) {
>> DRM_ERROR("too many full retries, give up\n");
>> - break;
>> + goto failed;
>> }
>> intel_dp_reset_link_train(intel_dp, &DP,
>> DP_TRAINING_PATTERN_1 |
>> @@ -3619,7 +3645,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> ++voltage_tries;
>> if (voltage_tries == 5) {
>> DRM_ERROR("too many voltage retries, give up\n");
>> - break;
>> + goto failed;
>> }
>> } else
>> voltage_tries = 0;
>> @@ -3628,14 +3654,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> /* Update training set as requested by target */
>> if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
>> DRM_ERROR("failed to update link training\n");
>> - break;
>> + goto failed;
>> }
>> }
>>
>> +cr_done:
>> intel_dp->DP = DP;
>> + return true;
>> +
>> +failed:
>> + DRM_DEBUG_KMS("Failed to initiate link training\n");
>> + return false;
>> }
>>
>> -void
>> +bool
>> intel_dp_complete_link_train(struct intel_dp *intel_dp)
>> {
>> bool channel_eq = false;
>> @@ -3652,7 +3684,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>> training_pattern |
>> DP_LINK_SCRAMBLING_DISABLE)) {
>> DRM_ERROR("failed to start channel equalization\n");
>> - return;
>> + return false;
>> }
>>
>> tries = 0;
>> @@ -3711,14 +3743,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>>
>> intel_dp->DP = DP;
>>
>> - if (channel_eq)
>> + if (channel_eq) {
>> DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
>> + return true;
>> + }
>>
>> + return false;
>> }
>>
>> -void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp)
>> {
>> - intel_dp_set_link_train(intel_dp, &intel_dp->DP,
>> + return intel_dp_set_link_train(intel_dp, &intel_dp->DP,
>> DP_TRAINING_PATTERN_DISABLE);
>> }
>>
>> @@ -4077,9 +4112,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>> if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
>> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> intel_encoder->base.name);
>> - intel_dp_start_link_train(intel_dp);
>> - intel_dp_complete_link_train(intel_dp);
>> - intel_dp_stop_link_train(intel_dp);
>> + if (!intel_dp_start_link_train(intel_dp)) {
>> + DRM_DEBUG_KMS("Start link training failed\n");
>> + return;
>> + }
>> + if (!intel_dp_complete_link_train(intel_dp)) {
>> + DRM_DEBUG_KMS("Complete link training failed\n");
>> + return;
>> + }
>> + if (!intel_dp_stop_link_train(intel_dp)) {
>> + DRM_DEBUG_KMS("Stop link training failed\n");
>> + return;
>> + }
>> }
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e8f4839..dc80444 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -949,12 +949,16 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>> void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>>
>> /* intel_dp.c */
>> +#define INTEL_DP_CLOCKREC_FAILED 1
>> +#define INTEL_DP_CHANNELEQ_FAILED 2
>> +#define INTEL_DP_STOPTRAIN_FAILED 3
>> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
>> bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>> struct intel_connector *intel_connector);
>> -void intel_dp_start_link_train(struct intel_dp *intel_dp);
>> -void intel_dp_complete_link_train(struct intel_dp *intel_dp);
>> -void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>> +bool intel_dp_start_link_train(struct intel_dp *intel_dp);
>> +bool intel_dp_complete_link_train(struct intel_dp *intel_dp);
>> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp);
>> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
>> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>> void intel_dp_encoder_destroy(struct drm_encoder *encoder);
>> void intel_dp_check_link_status(struct intel_dp *intel_dp);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Improve reliability for Displayport link training
2014-10-22 14:22 ` Jani Nikula
@ 2014-10-23 16:56 ` Todd Previte
2014-10-24 8:27 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Todd Previte @ 2014-10-23 16:56 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On 10/22/2014 7:22 AM, Jani Nikula wrote:
> On Thu, 09 Oct 2014, Todd Previte <tprevite@gmail.com> wrote:
>> Link training for Displayport can fail in many ways and at multiple different points
>> during the training process. Previously, errors were logged but no additional action
>> was taken based on them. Consequently, training attempts could continue even after
>> errors have occured that would prevent successful link training. This patch updates
>> the link training functions and where/how they're used to be more intelligent about
>> failures and to stop trying to train the link when it's a lost cause.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>> drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++--
>> drivers/gpu/drm/i915/intel_dp.c | 88 ++++++++++++++++++++++++++++++----------
>> drivers/gpu/drm/i915/intel_drv.h | 10 +++--
>> 3 files changed, 95 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index cb5367c..718240f 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1119,6 +1119,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>> enum port port = intel_ddi_get_encoder_port(intel_encoder);
>> int type = intel_encoder->type;
>> + uint8_t fail_code = 0;
>> + char *msg;
>>
>> if (crtc->config.has_audio) {
>> DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
>> @@ -1143,10 +1145,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> intel_ddi_init_dp_buf_reg(intel_encoder);
>>
>> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>> - intel_dp_start_link_train(intel_dp);
>> - intel_dp_complete_link_train(intel_dp);
>> + if (!intel_dp_start_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CLOCKREC_FAILED;
>> + msg = "clock recovery";
>> + goto failed;
>> + }
>> + if (!intel_dp_complete_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
>> + msg = "channel equalization";
>> + goto failed;
>> + }
>> if (port != PORT_A)
>> - intel_dp_stop_link_train(intel_dp);
>> + if (!intel_dp_stop_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
>> + msg = "stop training";
>> + goto failed;
>> + }
>> } else if (type == INTEL_OUTPUT_HDMI) {
>> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>>
>> @@ -1154,6 +1168,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> crtc->config.has_hdmi_sink,
>> &crtc->config.adjusted_mode);
>> }
>> +
>> + return;
>> +
>> +failed:
>> + DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
>> }
>>
>> static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 64c8e04..a8352c4 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2538,18 +2538,38 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>> struct drm_device *dev = encoder->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> uint32_t dp_reg = I915_READ(intel_dp->output_reg);
>> + uint8_t fail_code = 0;
>> + char *msg;
>>
>> if (WARN_ON(dp_reg & DP_PORT_EN))
>> - return;
>> + return false;
>>
>> intel_dp_enable_port(intel_dp);
>> intel_edp_panel_vdd_on(intel_dp);
>> intel_edp_panel_on(intel_dp);
>> intel_edp_panel_vdd_off(intel_dp, true);
>> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>> - intel_dp_start_link_train(intel_dp);
>> - intel_dp_complete_link_train(intel_dp);
>> - intel_dp_stop_link_train(intel_dp);
>> + if (!intel_dp_start_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CLOCKREC_FAILED;
>> + msg = "clock recovery";
>> + goto failed;
>> + }
>> + if (!intel_dp_complete_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_CHANNELEQ_FAILED;
>> + msg = "channel equalization";
>> + goto failed;
>> + }
>> + if (!intel_dp_stop_link_train(intel_dp)) {
>> + fail_code = INTEL_DP_STOPTRAIN_FAILED;
>> + msg = "stop training";
>> + goto failed;
>> + }
> In general I like failing fast and bailing out. However the users
> probably like it better if we limp on and keep trying to get a picture
> on screen. It's also much less stressful to work on bugs that cause a
> warn in the logs instead of a black screen.
>
> Case in point [1] which would end up in a black screen with this
> patch. Unless we've managed to fix it by now.
>
>
> BR,
> Jani.
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=70117
>
Instead of relying on hitting the failure case during channel
equalization (where we notice CR is gone and execute start_link_training
again), it's probably a better solution to restart link training again
from the top if we end up failing early on. I can modify this patch to
do that, since it seems like that's still within the scope of the
operations here. Provided we put a cap on the number of attempts (2 or 3
is what I had in mind, if you think it should be more/less we can
discuss that), I think that's going to be the best way to avoid a black
screen. In this instance it may delay bringing up the panel by a few
seconds, but that seems like a reasonable tradeoff.
-T
>
>
>> +
>> + return true;
>> +
>> +failed:
>> + DRM_DEBUG_KMS("Failed to train DP link - %s failed\n", msg);
>> + return false;
>> }
>>
>> static void g4x_enable_dp(struct intel_encoder *encoder)
>> @@ -3513,7 +3533,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>> return ret == intel_dp->lane_count;
>> }
>>
>> -static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>> {
>> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> struct drm_device *dev = intel_dig_port->base.base.dev;
>> @@ -3545,7 +3565,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>> }
>>
>> /* Enable corresponding port and start training pattern 1 */
>> -void
>> +bool
>> intel_dp_start_link_train(struct intel_dp *intel_dp)
>> {
>> struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
>> @@ -3564,11 +3584,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> link_config[1] = intel_dp->lane_count;
>> if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>> link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> - drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2) != 2) {
>> + DRM_DEBUG_KMS("Failed to write sink DPCD for link rate and lane count\n");
>> + goto failed;
>> + }
>>
>> link_config[0] = 0;
>> link_config[1] = DP_SET_ANSI_8B10B;
>> - drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>> + if (drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2) != 2) {
>> + DRM_DEBUG_KMS("Failed to write sink DPCD for downspread control\n");
>> + goto failed;
>> + }
>>
>> DP |= DP_PORT_EN;
>>
>> @@ -3577,7 +3603,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> DP_TRAINING_PATTERN_1 |
>> DP_LINK_SCRAMBLING_DISABLE)) {
>> DRM_ERROR("failed to enable link training\n");
>> - return;
>> + goto failed;
>> }
>>
>> voltage = 0xff;
>> @@ -3589,12 +3615,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
>> if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> DRM_ERROR("failed to get link status\n");
>> - break;
>> + goto failed;
>> }
>>
>> if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
>> DRM_DEBUG_KMS("clock recovery OK\n");
>> - break;
>> + goto cr_done;
>> }
>>
>> /* Check to see if we've tried the max voltage */
>> @@ -3605,7 +3631,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> ++loop_tries;
>> if (loop_tries == 5) {
>> DRM_ERROR("too many full retries, give up\n");
>> - break;
>> + goto failed;
>> }
>> intel_dp_reset_link_train(intel_dp, &DP,
>> DP_TRAINING_PATTERN_1 |
>> @@ -3619,7 +3645,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> ++voltage_tries;
>> if (voltage_tries == 5) {
>> DRM_ERROR("too many voltage retries, give up\n");
>> - break;
>> + goto failed;
>> }
>> } else
>> voltage_tries = 0;
>> @@ -3628,14 +3654,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>> /* Update training set as requested by target */
>> if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
>> DRM_ERROR("failed to update link training\n");
>> - break;
>> + goto failed;
>> }
>> }
>>
>> +cr_done:
>> intel_dp->DP = DP;
>> + return true;
>> +
>> +failed:
>> + DRM_DEBUG_KMS("Failed to initiate link training\n");
>> + return false;
>> }
>>
>> -void
>> +bool
>> intel_dp_complete_link_train(struct intel_dp *intel_dp)
>> {
>> bool channel_eq = false;
>> @@ -3652,7 +3684,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>> training_pattern |
>> DP_LINK_SCRAMBLING_DISABLE)) {
>> DRM_ERROR("failed to start channel equalization\n");
>> - return;
>> + return false;
>> }
>>
>> tries = 0;
>> @@ -3711,14 +3743,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>>
>> intel_dp->DP = DP;
>>
>> - if (channel_eq)
>> + if (channel_eq) {
>> DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
>> + return true;
>> + }
>>
>> + return false;
>> }
>>
>> -void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp)
>> {
>> - intel_dp_set_link_train(intel_dp, &intel_dp->DP,
>> + return intel_dp_set_link_train(intel_dp, &intel_dp->DP,
>> DP_TRAINING_PATTERN_DISABLE);
>> }
>>
>> @@ -4077,9 +4112,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>> if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
>> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> intel_encoder->base.name);
>> - intel_dp_start_link_train(intel_dp);
>> - intel_dp_complete_link_train(intel_dp);
>> - intel_dp_stop_link_train(intel_dp);
>> + if (!intel_dp_start_link_train(intel_dp)) {
>> + DRM_DEBUG_KMS("Start link training failed\n");
>> + return;
>> + }
>> + if (!intel_dp_complete_link_train(intel_dp)) {
>> + DRM_DEBUG_KMS("Complete link training failed\n");
>> + return;
>> + }
>> + if (!intel_dp_stop_link_train(intel_dp)) {
>> + DRM_DEBUG_KMS("Stop link training failed\n");
>> + return;
>> + }
>> }
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e8f4839..dc80444 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -949,12 +949,16 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>> void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>>
>> /* intel_dp.c */
>> +#define INTEL_DP_CLOCKREC_FAILED 1
>> +#define INTEL_DP_CHANNELEQ_FAILED 2
>> +#define INTEL_DP_STOPTRAIN_FAILED 3
>> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
>> bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>> struct intel_connector *intel_connector);
>> -void intel_dp_start_link_train(struct intel_dp *intel_dp);
>> -void intel_dp_complete_link_train(struct intel_dp *intel_dp);
>> -void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>> +bool intel_dp_start_link_train(struct intel_dp *intel_dp);
>> +bool intel_dp_complete_link_train(struct intel_dp *intel_dp);
>> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp);
>> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
>> void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>> void intel_dp_encoder_destroy(struct drm_encoder *encoder);
>> void intel_dp_check_link_status(struct intel_dp *intel_dp);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Improve reliability for Displayport link training
2014-10-23 16:56 ` Todd Previte
@ 2014-10-24 8:27 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-10-24 8:27 UTC (permalink / raw)
To: Todd Previte; +Cc: intel-gfx
On Thu, Oct 23, 2014 at 09:56:53AM -0700, Todd Previte wrote:
>
> On 10/22/2014 7:22 AM, Jani Nikula wrote:
> >On Thu, 09 Oct 2014, Todd Previte <tprevite@gmail.com> wrote:
> >>Link training for Displayport can fail in many ways and at multiple different points
> >>during the training process. Previously, errors were logged but no additional action
> >>was taken based on them. Consequently, training attempts could continue even after
> >>errors have occured that would prevent successful link training. This patch updates
> >>the link training functions and where/how they're used to be more intelligent about
> >>failures and to stop trying to train the link when it's a lost cause.
> >>
> >>Signed-off-by: Todd Previte <tprevite@gmail.com>
> >>---
> >> drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++--
> >> drivers/gpu/drm/i915/intel_dp.c | 88 ++++++++++++++++++++++++++++++----------
> >> drivers/gpu/drm/i915/intel_drv.h | 10 +++--
> >> 3 files changed, 95 insertions(+), 28 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>index cb5367c..718240f 100644
> >>--- a/drivers/gpu/drm/i915/intel_ddi.c
> >>+++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>@@ -1119,6 +1119,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >> struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> >> enum port port = intel_ddi_get_encoder_port(intel_encoder);
> >> int type = intel_encoder->type;
> >>+ uint8_t fail_code = 0;
> >>+ char *msg;
> >> if (crtc->config.has_audio) {
> >> DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
> >>@@ -1143,10 +1145,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >> intel_ddi_init_dp_buf_reg(intel_encoder);
> >> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>- intel_dp_start_link_train(intel_dp);
> >>- intel_dp_complete_link_train(intel_dp);
> >>+ if (!intel_dp_start_link_train(intel_dp)) {
> >>+ fail_code = INTEL_DP_CLOCKREC_FAILED;
> >>+ msg = "clock recovery";
> >>+ goto failed;
> >>+ }
> >>+ if (!intel_dp_complete_link_train(intel_dp)) {
> >>+ fail_code = INTEL_DP_CHANNELEQ_FAILED;
> >>+ msg = "channel equalization";
> >>+ goto failed;
> >>+ }
> >> if (port != PORT_A)
> >>- intel_dp_stop_link_train(intel_dp);
> >>+ if (!intel_dp_stop_link_train(intel_dp)) {
> >>+ fail_code = INTEL_DP_STOPTRAIN_FAILED;
> >>+ msg = "stop training";
> >>+ goto failed;
> >>+ }
> >> } else if (type == INTEL_OUTPUT_HDMI) {
> >> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >>@@ -1154,6 +1168,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >> crtc->config.has_hdmi_sink,
> >> &crtc->config.adjusted_mode);
> >> }
> >>+
> >>+ return;
> >>+
> >>+failed:
> >>+ DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
> >> }
> >> static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> >>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>index 64c8e04..a8352c4 100644
> >>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>@@ -2538,18 +2538,38 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >> struct drm_device *dev = encoder->base.dev;
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> >>+ uint8_t fail_code = 0;
> >>+ char *msg;
> >> if (WARN_ON(dp_reg & DP_PORT_EN))
> >>- return;
> >>+ return false;
> >> intel_dp_enable_port(intel_dp);
> >> intel_edp_panel_vdd_on(intel_dp);
> >> intel_edp_panel_on(intel_dp);
> >> intel_edp_panel_vdd_off(intel_dp, true);
> >> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >>- intel_dp_start_link_train(intel_dp);
> >>- intel_dp_complete_link_train(intel_dp);
> >>- intel_dp_stop_link_train(intel_dp);
> >>+ if (!intel_dp_start_link_train(intel_dp)) {
> >>+ fail_code = INTEL_DP_CLOCKREC_FAILED;
> >>+ msg = "clock recovery";
> >>+ goto failed;
> >>+ }
> >>+ if (!intel_dp_complete_link_train(intel_dp)) {
> >>+ fail_code = INTEL_DP_CHANNELEQ_FAILED;
> >>+ msg = "channel equalization";
> >>+ goto failed;
> >>+ }
> >>+ if (!intel_dp_stop_link_train(intel_dp)) {
> >>+ fail_code = INTEL_DP_STOPTRAIN_FAILED;
> >>+ msg = "stop training";
> >>+ goto failed;
> >>+ }
> >In general I like failing fast and bailing out. However the users
> >probably like it better if we limp on and keep trying to get a picture
> >on screen. It's also much less stressful to work on bugs that cause a
> >warn in the logs instead of a black screen.
> >
> >Case in point [1] which would end up in a black screen with this
> >patch. Unless we've managed to fix it by now.
> >
> >
> >BR,
> >Jani.
> >
> >[1] https://bugs.freedesktop.org/show_bug.cgi?id=70117
> >
> Instead of relying on hitting the failure case during channel equalization
> (where we notice CR is gone and execute start_link_training again), it's
> probably a better solution to restart link training again from the top if we
> end up failing early on. I can modify this patch to do that, since it seems
> like that's still within the scope of the operations here. Provided we put a
> cap on the number of attempts (2 or 3 is what I had in mind, if you think it
> should be more/less we can discuss that), I think that's going to be the
> best way to avoid a black screen. In this instance it may delay bringing up
> the panel by a few seconds, but that seems like a reasonable tradeoff.
Sounds like a neat idea. If you can throw this at a few bug reporters
would be even more awesome I think, especially those where we have lots of
link-training errors but still succeed in displaying a picture somehow.
If that doesn't cut it then I guess we need an option for "standards
compliant link training" where we don't enable the link if the training
fails. And then never enable it in real-world. We already have similar
stupid requirements on the hdmi side, so one global "standards, but broken
mode" would be good.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-24 8:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-09 16:39 [PATCH] drm/i915: Improve reliability for Displayport link training Todd Previte
2014-10-21 14:45 ` Daniel Vetter
2014-10-23 13:58 ` Todd Previte
2014-10-22 14:22 ` Jani Nikula
2014-10-23 16:56 ` Todd Previte
2014-10-24 8:27 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.