Re: [PATCH v5 1/2] drm/i915/dp: Limit link training clock recovery loop

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

On Thu, Jul 19, 2018 at 02:47:38PM -0700, Nathan Ciobanu wrote:
> Limit the link training clock recovery loop to 10 attempts at
> LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2 and 80 attempts for
> pre-DP 1.4 (4 voltage levels x 4 preemphasis levels x
> x 5 identical voltages tries). Some faulty USB-C MST hubs can
> cause us to get stuck in this loop indefinitely requesting something
> like:
> 
>     voltage swing: 0, pre-emphasis level: 2
>     voltage swing: 1, pre-emphasis level: 2
>     voltage swing: 0, pre-emphasis level: 3
> 
> over and over so max_vswing would never be reached,
> drm_dp_clock_recovery_ok() would never return true and voltage_tries
> would always get reset to 1. The driver sends those values to the hub
> but the hub keeps requesting new values every time.
> 
> Changes in v2:
>     - updated commit message (DK, Manasi)
>     - defined DP_DP14_MAX_CR_TRIES (Marc)
>     - made the loop iterate for max 10 times (Rodrigo, Marc)
> 
> Changes in v3:
>     - changed error message to use DP_DP14_MAX_CR_TRIES
> 
> Changes in v4:
>     - Updated the title to reflect the change
>     - Updated the commit message
>     - Added 80 attempts for pre-DP 1.4 devices
> 
> Changes in v5:
>     - Removed DP_DP14_MAX_CR_TRIES from drm
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Marc Herbert <marc.herbert@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 16 ++++++++++++++--
>  1 file changed, 14 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..7903de7a54c9 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, max_cr_tries;
>  	uint8_t link_config[2];
>  	uint8_t link_bw, rate_select;
>  
> @@ -170,9 +170,19 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
>  		return false;
>  	}
>  
> +	/* DP 1.4 spec clock recovery retries defined but

nip: Preferred kernel multi-line comment is:

/*
 * Start comment..
 * second line..
 */

but yeah, I understand we have many precedent cases using the other style...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
(better with comment updated ;))

> +	 * for devices pre-DP 1.4 we set the retry limit
> +	 * to 4 (voltage levels) x 4 (preemphasis levels) x
> +	 * x 5 (same voltage retries) = 80 (max iterations)
> +	 */
> +	if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
> +		max_cr_tries = 10;
> +	else
> +		max_cr_tries = 80;
> +
>  	voltage_tries = 1;
>  	max_vswing_tries = 0;
> -	for (;;) {
> +	for (cr_tries = 0; cr_tries < 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 +226,8 @@ 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", max_cr_tries);
> +	return false;
>  }
>  
>  /*
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx