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

[Nathan Ciobanu]

On Mon, Jul 16, 2018 at 03:34:58PM -0700, Rodrigo Vivi wrote:
> On Mon, Jul 16, 2018 at 11:14:45AM -0700, Nathan Ciobanu wrote:
> > Limit the link training clock recovery loop to 10 failed attempts at
> > LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2.
> 
> Thanks... so this made me look to DP 1.3 and DP 1.4 differences here.
> Are we already addressing all new conditions?
> 
> I wonder if that should be at drm level.
> 
> I noticed that one difference, 100us wait difference is already
> addressed there at drm level...
> 
> > Some USB-C MST hubs
> > cause us to get stuck in this loop indefinitely requesting
> > 
> >     voltage swing: 0, pre-emphasis level: 2
> >     voltage swing: 1, pre-emphasis level: 2
> >     voltage swing: 0, pre-emphasis level: 3
> > 
> > over and over: max_vswing is never reached, drm_dp_clock_recovery_ok()
> > never returns true and voltage_tries always gets reset to 1. The driver
> > sends those values to the hub but the hub keeps requesting new values
> > every time in the pattern shown above.
> > 
> > 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
> > 
> > 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) {
> 
> I know that I was the on that asked you to make this global here,
> but just now with better spec pointers and definition is that I noticed
> that this is only for DP 1.4 spec while this code here runs for all
> DP...  So we need to change in a way that we check the spec version somehow...
I think the bug is with this infinite loop which is at the mercy of an external device
and in my case I have this MST hub which appears to be DP 1.2 that triggers the
infinite loop case. We have to limit the number of iterations and
DP 1.4 spec happened to fix this issue. I'm a newbie in this area but in this case
shouldn't we apply the same counter to all <= "DP 1.4" devices?

-Nathan 
> 
> and possibly at drm level?!
> 
> >  		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;
> >  }
> >  
> >  /*
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index c01564991a9f..2303ad8ed24e 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -820,6 +820,7 @@
> >  
> >  #define DP_DP13_DPCD_REV                    0x2200
> >  #define DP_DP13_MAX_LINK_RATE               0x2201
> > +#define DP_DP14_MAX_CR_TRIES                10
> >  
> >  #define DP_DPRX_FEATURE_ENUMERATION_LIST    0x2210  /* DP 1.3 */
> >  # define DP_GTC_CAP					(1 << 0)  /* DP 1.3 */
> > -- 
> > 1.9.1
> > 
>