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

[Daniel Vetter <daniel@ffwll.ch>]

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