Re: [PATCH] drm/i915: Improve reliability for Displayport link training

[Todd Previte <tprevite@xxxxxxxxx>]

On 10/22/2014 7:22 AM, Jani Nikula wrote:
> 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
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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx