[Intel-gfx] [PATCH] drm/i915/dp: Don't stop the link when retraining

[Ville Syrjälä]

On Mon, Nov 10, 2014 at 10:01:56AM -0800, Jesse Barnes wrote:
> On Mon,  3 Nov 2014 11:39:24 +0100
> Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> 
> > On pre-ddi platforms we don't shut down the link when changing link
> > training parameters. Except when clock recovery fails too hard and we
> > restart with channel eq training. Which doesn't make a lot of sense
> > really, since just stopping/restarting the DP port at this point
> > violates the modeset sequence documented in the Bspec.
> > 
> > So let's tempt fate and try this.
> > 
> > This patch is motivated by a WARN_ON triggered by
> > 
> > commit bc76e320f21f8bd790a72bd5dc06909617432352
> > Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Date:   Tue May 20 22:46:50 2014 +0200
> > 
> >     drm/i915: Drop now misleading DDI comment from dp_link_down
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=85670
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index f6a3fdd5589e..e48ca3a87199
> > 100644 --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3598,7 +3598,6 @@ intel_dp_complete_link_train(struct intel_dp
> > *intel_dp) 
> >  		/* Try 5 times, then try clock recovery if that
> > fails */ if (tries > 5) {
> > -			intel_dp_link_down(intel_dp);
> >  			intel_dp_start_link_train(intel_dp);
> >  			intel_dp_set_link_train(intel_dp, &DP,
> >  						training_pattern |
> 
> 
> Didn't look like it helped the reporter?  Or at least I didn't see it
> tried in the bug above...
> 
> I'm a bit worried about this because istr the spec indicating that we
> do need to down the link when retrying clock recovery.  I guess I'll
> need to check again.

I had a similar notion in my head. Can't recall where I saw it exactly.

But the current code is a bit inconistent anyway since it doesn't call
intel_dp_link_down() in all the failure cases. Which combined with an
initially stuck power sequencer on chv sometimes resulted in success
on the first retry and sometimes all the retries would just fail,
entirely depending on which codepath it used when restarting the link
training. I never bothered figuring out what really made it pick one
path over the other, but IIRC it was totally consistent in its
choice for a given combination of modeset operations.

Anyway, I suppose if we really want to follow the correct restart
procedure we might have to shut down _everything_ and start again from
the top. But that seems rather difficult to do given that the link
training isn't driving the entire modeset sequence for us.

-- 
Ville Syrjälä
Intel OTC