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

[Jim Bride]

On Wed, Jul 12, 2017 at 02:53:36PM -0700, Manasi Navare wrote:
> 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?

What seems the easiest to me is to set intel_dp->train_set_valid
to false right before the main clock recovery loop (but after calling
intel_dp_reset_link_train(), which is what uses the value) in
intel_dp_link_training_clock_recovery(), and remove the
sets to false for the failing cases.  If clock recovery is ok,
then we set it to true and return.  This makes it much easier
to sort out when we succeed by reading the code without changing
any of the existing functionality.  New patch coming as soon as I
get done testing it.

Jim


> Manasi