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

[Rodrigo Vivi]

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.

> 
> 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....

> 
> Marc
> 
> 
> On 16/07/2018 11:14, Nathan Ciobanu wrote:
> > Signed-off-by: Nathan Ciobanu <nathan.d.ciobanu at 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;
> >  }
> >