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@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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
