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

[Nathan Ciobanu <nathan.d.ciobanu@linux.intel.com>]

On Fri, Jul 20, 2018 at 12:22:15PM -0700, Rodrigo Vivi wrote:
> 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 ;))
Absolutely. I made the change and sent the patch again. 
Thanks, Nathan.
> 
> > +	 * 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