diff mbox

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

Message ID 1472767699-31211-10-git-send-email-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Navare, Manasi Sept. 1, 2016, 10:08 p.m. UTC 
From: Dhinakaran Pandiyan <dhinakaran.pandiyan@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@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 59 ++++++++++++---------------
 1 file changed, 26 insertions(+), 33 deletions(-)

Comments

Kahola, Mika Sept. 2, 2016, 9:16 a.m. UTC | #1 
On Thu, 2016-09-01 at 15:08 -0700, Manasi Navare wrote:
> From: Dhinakaran Pandiyan <dhinakaran.pandiyan@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@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? 

>  	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.

>  
> -		/* 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;
> +
>  	}
>  }
>
Dhinakaran Pandiyan Sept. 2, 2016, 5:55 p.m. UTC | #2 
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@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@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;

> > +

> >  	}

> >  }

> >
diff mbox

Patch

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;
 
 	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_status);
-				break;
-			}
-			intel_dp_reset_link_train(intel_dp,
-						  DP_TRAINING_PATTERN_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;
 		}
 
-		/* 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;
+
 	}
 }