Re: [PATCH v3] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jul 16, 2018 at 04:27:47PM -0700, Nathan Ciobanu wrote:
> On Mon, Jul 16, 2018 at 03:30:49PM -0700, Rodrigo Vivi wrote:
> > On Mon, Jul 16, 2018 at 12:22:13PM -0700, Marc Herbert wrote:
> > > 
> > > While the shortness of v3 is really nice, I think it's rather a good
> > > opportunity to make much clearer (as a separate, no functional change
> > > patch?)  its existing terminating condition(s) and what the existing loop
> > > iterates on. I mean it's painful and risky enough to _combine multiple
> > > counters in the same loop_, so at least these different counters should be
> > > _invidually_ crystal-clear with enough comments. For instance let's pretend
> > > there are 4 possible voltage values and that each value is tried 4 times -
> > > then the last value will never be tried! Can this ever happen? If not then
> > > why not? Not even with a buggy sink?  If it can happen then is it fine to
> > > give up before trying some values? Is it compliant with the spec? Etc.
> > 
> > hm.. it seems that that code has room for improvements to make it more
> > clear and easier to map with the spec...
> > but yeap, separated patches.
> Do we like to add macros for the other counters? I can try to clarify this a bit.
> 
> > 
> > > 
> > > This should incidentally help clarify why and how the current logic allows
> > > infinite loops.
> > > 
> > > BTW "max_vswing_tries" looks like a boolean, correct? If correct then please
> > > remove this confusing "increment from false to true":
> > > 
> > > - if (max_vswing_tries == 1)
> > > + if (max_vswing_tries)
> > > 
> > > - max_vswing_tries++;
> > > + max_vwing_tries = true;
> > 
> > if we change to boolean it is better to really change the type
> > and remove "_tries" from the name....
> Yup, will do!

please note I'm not telling that we "should do"... but only "if we do" ;)

> 
> > 
> > > 
> > > Marc
> > > 
> > > 
> > > On 16/07/2018 11:14, Nathan Ciobanu wrote:
> > > > Signed-off-by: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 7 +++++--
> > > >  include/drm/drm_dp_helper.h                   | 1 +
> > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > index 4da6e33c7fa1..4bfba65c431c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > @@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> > > >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  {
> > > >  	uint8_t voltage;
> > > > -	int voltage_tries, max_vswing_tries;
> > > > +	int voltage_tries, max_vswing_tries, cr_tries;
> > > >  	uint8_t link_config[2];
> > > >  	uint8_t link_bw, rate_select;
> > > >  
> > > > @@ -172,7 +172,7 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> > > >  
> > > >  	voltage_tries = 1;
> > > >  	max_vswing_tries = 0;
> > > > -	for (;;) {
> > > > +	for (cr_tries = 0; cr_tries < DP_DP14_MAX_CR_TRIES; ++cr_tries) {
> > > >  		uint8_t link_status[DP_LINK_STATUS_SIZE];
> > > >  
> > > >  		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> > > > @@ -216,6 +216,9 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> > > >  			++max_vswing_tries;
> > > >  
> > > >  	}
> > > > +	DRM_ERROR("Failed clock recovery %d times, giving up!\n",
> > > > +		  DP_DP14_MAX_CR_TRIES);
> > > > +	return false;
> > > >  }
> > > >  
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx