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

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

[Nathan Ciobanu]

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
+	 * 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

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

[Rodrigo Vivi]

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

[PATCH v6] drm/i915/dp: Limit link training clock recovery loop

[Nathan Ciobanu]

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

v6: Updated comment to match kernel style (Rodrigo)

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 | 17 +++++++++++++++--
 1 file changed, 15 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..299cad5632ed 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,20 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	/*
+	 * DP 1.4 spec clock recovery retries defined but
+	 * 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 +227,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

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

[Nathan Ciobanu]

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

Re: [PATCH v6] drm/i915/dp: Limit link training clock recovery loop

[Rodrigo Vivi]

On Fri, Jul 20, 2018 at 01:15:59PM -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
> 
> v6: Updated comment to match kernel style (Rodrigo)

thanks... you could've added my rv-b directly as well ;)

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

anyway

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++++++++++++++--
>  1 file changed, 15 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..299cad5632ed 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,20 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
>  		return false;
>  	}
>  
> +	/*
> +	 * DP 1.4 spec clock recovery retries defined but
> +	 * 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 +227,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

Re: [PATCH v6] drm/i915/dp: Limit link training clock recovery loop

[Nathan Ciobanu]

On Fri, Jul 20, 2018 at 01:28:44PM -0700, Rodrigo Vivi wrote:
> On Fri, Jul 20, 2018 at 01:15:59PM -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
> > 
> > v6: Updated comment to match kernel style (Rodrigo)
> 
> thanks... you could've added my rv-b directly as well ;)
Why should it be simple when I can make it complicated, right? :D. 
Hopefully less noob mistakes next time.
> 
> > 
> > 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>
> 
> anyway
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++++++++++++++--
> >  1 file changed, 15 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..299cad5632ed 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,20 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> >  		return false;
> >  	}
> >  
> > +	/*
> > +	 * DP 1.4 spec clock recovery retries defined but
> > +	 * 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 +227,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

Re: [PATCH v6] drm/i915/dp: Limit link training clock recovery loop

[Marc Herbert]

Compliance aside I had a really hard time understanding the gap between 80
and 10. I mean if up to 80 tries might be needed pre-1.4, then how come 1.4
is supposed to succeed in less than 10? Confusing.

After asking Nathan and DK (thx) I suggest rephrasing the comment below with
something like this:

/* For devices pre-DP 1.4 we set the retry limit to an extremely lenient 80
  max iterations = 4 (voltage levels) x 4 (preemphasis levels) x 5 (same
  voltage retries). It should never take that many attempts (TODO: why not?)
  but we let's be lax and tolerate all kinds of sinks because the specs
  don't specify any limit (and allow infinite loops).
  On the other hand, 1.4 devices should be smart and/or compliant enough and
  the expectations are now higher; they're must succeed in less than 10 tries
  because they know how <TODO>
*/

My 2 cents - you get the idea.

BTW I'm curious how long 80 tries typically take.


On 20/07/2018 13:15, 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:

I heard that loop was not just the hub's fault after all.

Of course no hub should be able to trigger an infinite loop anyway,
not even a hypothetical hub.


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

> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c

>  
> @@ -170,9 +170,20 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
>  		return false;
>  	}
>  
> +	/*
> +	 * DP 1.4 spec clock recovery retries defined but
> +	 * 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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6] drm/i915/dp: Limit link training clock recovery loop

[Rodrigo Vivi]

On Fri, Jul 20, 2018 at 01:15:59PM -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
> 
> v6: Updated comment to match kernel style (Rodrigo)

series pushed to dinq. Thanks.

> 
> 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 | 17 +++++++++++++++--
>  1 file changed, 15 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..299cad5632ed 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,20 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
>  		return false;
>  	}
>  
> +	/*
> +	 * DP 1.4 spec clock recovery retries defined but
> +	 * 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 +227,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