Re: [Intel-gfx] [PATCH v6] drm/i915/dp: Limit link training clock recovery loop
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
> Cc: Rodrigo Vivi
> Cc: Marc Herbert
> Cc: Manasi Navare
> Signed-off-by: Nathan Ciobanu
> ---
> 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: [Intel-gfx] [PATCH v6] drm/i915/dp: Limit link training clock recovery loop
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 */ 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: [Intel-gfx] [PATCH v6] drm/i915/dp: Limit link training clock recovery loop
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
> > Cc: Rodrigo Vivi
> > Cc: Marc Herbert
> > Cc: Manasi Navare
> > Signed-off-by: Nathan Ciobanu
>
> anyway
>
> Reviewed-by: Rodrigo Vivi
>
> > ---
> > 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: [Intel-gfx] [PATCH v6] drm/i915/dp: Limit link training clock recovery loop
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
> Cc: Rodrigo Vivi
> Cc: Marc Herbert
> Cc: Manasi Navare
> Signed-off-by: Nathan Ciobanu
anyway
Reviewed-by: Rodrigo Vivi
> ---
> 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
