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

[Marc Herbert]

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.

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;

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;
>  }
>