[Intel-gfx] [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP

[Manasi Navare]

On Wed, Jul 12, 2017 at 10:38:03PM +0100, Chris Wilson wrote:
> Quoting Manasi Navare (2017-07-12 22:36:49)
> > On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> > > Quoting Jim Bride (2017-07-11 23:19:56)
> > > > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  
> > > >                 if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > >                         DRM_ERROR("failed to get link status\n");
> > > > +                       intel_dp->train_set_valid = false;
> > > >                         return false;
> > > >                 }
> > > >  
> > > >                 if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> > > >                         DRM_DEBUG_KMS("clock recovery OK\n");
> > > > +                       intel_dp->train_set_valid = is_edp(intel_dp);
> > > 
> > > Ouch, that was hard to spot amongst the decoys. How about setting
> > > intel_dp->train_set_valid = false at the very start of training, and
> > > only on success set it to true, something like
> > >
> > 
> > Or like I suggested, just set train_set_valid to false in the
> > failure_handling and set it to true only on success.
> 
> It just looked a little crowded in the failure_handling: whereas at the
> start of the function, there was plenty of whitespace for it to stand
> out. That was all I was thinking.
> -Chris

But at the beginning of the function, it is anyway set to False since
intel_dp_init_connector sets it to false.
So we dont need to again set it to false at the beginning of the function right?
Then we only set it to true when it succeeds so after 1 successful link training
it will be set to true.
And now since this code will also be used fro DP case, we need to make sure
we set this to false in failure handling so that it resets link train during
link train fallback.
Makes sense?

Manasi