On Thu, 09 Oct 2014, Todd Previte <tprevite@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
