[Intel-gfx] [PATCH 09/14] drm/dp/i915: Make clock recovery in the link training compliant with DP Spec 1.2

[Pandiyan, Dhinakaran]

On Fri, 2016-09-02 at 12:16 +0300, Mika Kahola wrote:
> On Thu, 2016-09-01 at 15:08 -0700, Manasi Navare wrote:
> > From: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > 
> > This function cleans up clock recovery loop in link training
> > compliant
> > tp Dp Spec 1.2. It tries the clock recovery 5 times for the same
> > voltage
> > or until max voltage swing is reached and removes the additional non
> > compliant retries. This function now returns a boolean values based
> > on
> > if clock recovery passed or failed.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 59 ++++++++++++-----
> > ----------
> >  1 file changed, 26 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 9145e5a..13a0341 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -117,19 +117,19 @@ static bool
> > intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> >  	int lane;
> >  
> >  	for (lane = 0; lane < intel_dp->lane_count; lane++)
> > -		if (intel_dp->train_set[lane] &
> > DP_TRAIN_MAX_SWING_REACHED == 0)
> > +		if ((intel_dp->train_set[lane] &
> > +		     DP_TRAIN_MAX_SWING_REACHED) == 0)
> >  			return false;
> >  
> This function was defined in the previous patch. Do we need to split
> this if(...) line into separate lines in this patch? 
> 

Ah! That was not intended, I will re-submit. 

> >  	return true;
> >  }
> >  
> >  /* Enable corresponding port and start training pattern 1 */
> > -static void
> > +static bool
> >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  {
> > -	int i;
> >  	uint8_t voltage;
> > -	int voltage_tries, loop_tries;
> > +	int voltage_tries, max_vswing_tries;
> >  	uint8_t link_config[2];
> >  	uint8_t link_bw, rate_select;
> >  
> > @@ -145,6 +145,7 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >  	if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> >  		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> >  	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET,
> > link_config, 2);
> > +
> >  	if (intel_dp->num_sink_rates)
> >  		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
> >  				  &rate_select, 1);
> > @@ -160,58 +161,50 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >  				       DP_TRAINING_PATTERN_1 |
> >  				       DP_LINK_SCRAMBLING_DISABLE))
> > {
> >  		DRM_ERROR("failed to enable link training\n");
> > -		return;
> > +		return false;
> >  	}
> >  
> > -	voltage = 0xff;
> > -	voltage_tries = 0;
> > -	loop_tries = 0;
> > +	voltage_tries = 1;
> > +	max_vswing_tries = 0;
> >  	for (;;) {
> >  		uint8_t link_status[DP_LINK_STATUS_SIZE];
> >  
> >  		drm_dp_link_train_clock_recovery_delay(intel_dp-
> > >dpcd);
> > +
> >  		if (!intel_dp_get_link_status(intel_dp,
> > link_status)) {
> >  			DRM_ERROR("failed to get link status\n");
> > -			break;
> > +			return false;
> >  		}
> >  
> >  		if (drm_dp_clock_recovery_ok(link_status, intel_dp-
> > >lane_count)) {
> >  			DRM_DEBUG_KMS("clock recovery OK\n");
> > -			break;
> > +			return true;
> >  		}
> >  
> > -		/* Check to see if we've tried the max voltage */
> > -		if (intel_dp_link_max_vswing_reached(intel_dp)) {
> > -			++loop_tries;
> > -			if (loop_tries == 5) {
> > -				DRM_ERROR("too many full retries,
> > give up\n");
> > -				intel_dp_dump_link_status(link_statu
> > s);
> > -				break;
> > -			}
> > -			intel_dp_reset_link_train(intel_dp,
> > -						  DP_TRAINING_PATTER
> > N_1 |
> > -						  DP_LINK_SCRAMBLING
> > _DISABLE);
> > -			voltage_tries = 0;
> > -			continue;
> > +		if (voltage_tries == 5 || max_vswing_tries == 1) {
> > +			DRM_DEBUG_KMS("Max. vswing reached or same
> > voltage "
> > +				      "tried 5 times\n");
> > +			return false;
> >  		}
> I think it would be beneficial to know which one we hit if the clock
> recovery fails. Therefore, I would split these debug messages so that
> there would be separate debug messages when reaching max vswing or
> trying out the same voltage for five times.

Agreed.

Also, I don't think we need the 'max_vswing_tries' local at all. We
could just call intel_dp_link_max_vswing_reached() here and remove the
call below.



> 
> >  
> > -		/* Check to see if we've tried the same voltage 5
> > times */
> > -		if ((intel_dp->train_set[0] &
> > DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
> > -			++voltage_tries;
> > -			if (voltage_tries == 5) {
> > -				DRM_ERROR("too many voltage retries,
> > give up\n");
> > -				break;
> > -			}
> > -		} else
> > -			voltage_tries = 0;
> >  		voltage = intel_dp->train_set[0] &
> > DP_TRAIN_VOLTAGE_SWING_MASK;
> >  
> >  		/* Update training set as requested by target */
> >  		intel_get_adjust_train(intel_dp, link_status);
> >  		if (!intel_dp_update_link_train(intel_dp)) {
> >  			DRM_ERROR("failed to update link
> > training\n");
> > -			break;
> > +			return false;
> >  		}
> > +
> > +		if ((intel_dp->train_set[0] &
> > DP_TRAIN_VOLTAGE_SWING_MASK) ==
> > +		    voltage)
> > +			++voltage_tries;
> > +		else
> > +			voltage_tries = 1;
> > +
> > +		if (intel_dp_link_max_vswing_reached(intel_dp))
> > +			++max_vswing_tries;
> > +
> >  	}
> >  }
> >