[Intel-gfx] [PATCH] drm/i915: Improve reliability for Displayport link training
Todd Previte
tprevite at gmail.com
Thu Oct 23 18:56:53 CEST 2014
On 10/22/2014 7:22 AM, Jani Nikula wrote:
> On Thu, 09 Oct 2014, Todd Previte <tprevite at 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 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list