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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 30, 2014 at 11:07:34AM -0300, Paulo Zanoni wrote:
> 2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> > 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.
> 
> I agree we need to do something about this problem, but I'm not sure
> how this patch improves the situation. Can you please describe more
> how exactly the changes you did are getting us towards the solution of
> the problem? Of course, the points where you start signaling
> previously-unsignaled errors are obviously an improvement.
> 
> Anyway, this patch should probably be split in 3:
> - A patch to add the boolean return values and change
> intel_dp_check_link_status() + intel_enable_dp() +
> intel_ddi_pre_enable().
> - A patch to signal dpcd error cases we were previously ignoring.
> - A patch to that changes how intel_dp_start_link_train() and
> intel_dp_stop_link_train() currently behave (the "goto"s replacing
> "break" statements).
> Se below for better explanations.
> 
> 
> The big problem here is that these encoder callbacks never fail, so
> there's not really much to do after we detect a sink failure.
> 
> In the current code (without your patch), we already clearly signal
> the link training failures with debug+error messages, so the new debug
> messages at places linke intel_enable_dp() are not much of an
> improvement. Also, we already run intel_dp_set_idle_link_train() at
> the end of intel_dp_complete_link_train(), and we do additional things
> such as calling intel_dp_stop_link_train(). And I guess we do the
> non-DDI equivalent steps at some point too, so I'm not sure how
> jumping straight to intel_dp_set_idle_link_train() helps, since we do
> it anyway as part of the normal sequence. Also, our mode set sequence
> is currently completely followed - even though the sink fails to
> understand what we throw at it - and I'm always afraid of not
> following the sequence exactly as described in the spec, since it
> could lead to unpredicted bugs (we had this problem dozens of times in
> the past).
> 
> I think the real cool solution would be to retry link training with
> different parameters (different clock and number of lanes), but I
> imagine this would require a lot of code refactoring since we probably
> need to go back to the compute_config stages of the modeset sequence.
> Or maybe just finding a way to tell the user-space modesetting app
> that it has a black screen would already be helpful.
> 
> Other people may think that the real-real long-term solution would be
> to fix our code so it never fails link training or gives black screens
> :)

DP spec says that we need to be able to retrain. My rough idea was that we
set some flags in pipe config (or wire up return values) which signals
that we need to rerun the modeset (including compute_config and all that).

To keep the link bw/count limits around we could add some flags to the
pipe config or something (and not completely clear it ofc).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch