[PATCH] drm/dp: Do not busy-loop during link training

[PATCH] drm/dp: Do not busy-loop during link training

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use microsecond sleeps for the clock recovery and channel equalization
delays during link training. The duration of these delays can be from
100 us up to 16 ms. It is rude to busy-loop for that amount of time.

While at it, also convert to standard coding style by putting the
opening braces in a function definition on a new line.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 73a3e0544f36..2dcc4efcd34c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -125,19 +125,29 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ
 }
 EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
 
-void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
-	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0)
-		udelay(100);
+void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	unsigned int min;
+
+	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] != 0)
+		min = dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4000;
 	else
-		mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4);
+		min = 100;
+
+	usleep_range(min, min * 2);
 }
 EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay);
 
-void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
-	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0)
-		udelay(400);
+void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	unsigned int min;
+
+	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] != 0)
+		min = dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4000;
 	else
-		mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4);
+		min = 400;
+
+	usleep_range(min, min * 2);
 }
 EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
 
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp: Do not busy-loop during link training

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 02:21:56PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Use microsecond sleeps for the clock recovery and channel equalization
> delays during link training. The duration of these delays can be from
> 100 us up to 16 ms. It is rude to busy-loop for that amount of time.

Do you have some numbers on how this affects a typical link training
cycle?

> 
> While at it, also convert to standard coding style by putting the
> opening braces in a function definition on a new line.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 73a3e0544f36..2dcc4efcd34c 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -125,19 +125,29 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ
>  }
>  EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
>  
> -void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
> -	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0)
> -		udelay(100);
> +void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> +	unsigned int min;
> +
> +	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] != 0)
> +		min = dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4000;
>  	else
> -		mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4);
> +		min = 100;
> +
> +	usleep_range(min, min * 2);
>  }
>  EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay);
>  
> -void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) {
> -	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0)
> -		udelay(400);
> +void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> +	unsigned int min;
> +
> +	if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] != 0)
> +		min = dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4000;
>  	else
> -		mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4);
> +		min = 400;
> +
> +	usleep_range(min, min * 2);
>  }
>  EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp: Do not busy-loop during link training

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1347 bytes --]

On Mon, Dec 14, 2015 at 04:48:09PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 14, 2015 at 02:21:56PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Use microsecond sleeps for the clock recovery and channel equalization
> > delays during link training. The duration of these delays can be from
> > 100 us up to 16 ms. It is rude to busy-loop for that amount of time.
> 
> Do you have some numbers on how this affects a typical link training
> cycle?

Not really. Sinks aren't required to provide a value here, in which case
the specification says that a default of 100 us and 400 us should be
used for clock recovery and channel equalization, respectively. If the
sink provides an AUX_RD_INTERVAL value, it is used for both CR and CE
(and is in units of 4 ms). Best case a typical link training cycle would
therefore take something like 0.5 ms and worst case, since the number of
retries should be limited to 5, it'd be around 5 * 16 ms = 80 ms. That's
not counting the actual AUX transactions, though they should be pretty
fast.

Since this patch uses usleep_range(min, min * 2) the worst case now
becomes ~ 160 ms. That's still not very much from a responsiveness point
of view, but the upside is of course that there are no busy loops that
could potentially hog the CPU.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp: Do not busy-loop during link training

[Ville Syrjälä]

On Mon, Dec 14, 2015 at 04:23:41PM +0100, Thierry Reding wrote:
> On Mon, Dec 14, 2015 at 04:48:09PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 14, 2015 at 02:21:56PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Use microsecond sleeps for the clock recovery and channel equalization
> > > delays during link training. The duration of these delays can be from
> > > 100 us up to 16 ms. It is rude to busy-loop for that amount of time.
> > 
> > Do you have some numbers on how this affects a typical link training
> > cycle?
> 
> Not really. Sinks aren't required to provide a value here, in which case
> the specification says that a default of 100 us and 400 us should be
> used for clock recovery and channel equalization, respectively. If the
> sink provides an AUX_RD_INTERVAL value, it is used for both CR and CE
> (and is in units of 4 ms). Best case a typical link training cycle would
> therefore take something like 0.5 ms and worst case, since the number of
> retries should be limited to 5, it'd be around 5 * 16 ms = 80 ms. That's
> not counting the actual AUX transactions, though they should be pretty
> fast.
> 
> Since this patch uses usleep_range(min, min * 2) the worst case now
> becomes ~ 160 ms.

Would be nice to have some *actual* numbers in the commit message,
otherwise it's all just guesswork.

> That's still not very much from a responsiveness point
> of view, but the upside is of course that there are no busy loops that
> could potentially hog the CPU.
> 
> Thierry

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp: Do not busy-loop during link training

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1788 bytes --]

On Mon, Dec 14, 2015 at 06:30:28PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 14, 2015 at 04:23:41PM +0100, Thierry Reding wrote:
> > On Mon, Dec 14, 2015 at 04:48:09PM +0200, Ville Syrjälä wrote:
> > > On Mon, Dec 14, 2015 at 02:21:56PM +0100, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Use microsecond sleeps for the clock recovery and channel equalization
> > > > delays during link training. The duration of these delays can be from
> > > > 100 us up to 16 ms. It is rude to busy-loop for that amount of time.
> > > 
> > > Do you have some numbers on how this affects a typical link training
> > > cycle?
> > 
> > Not really. Sinks aren't required to provide a value here, in which case
> > the specification says that a default of 100 us and 400 us should be
> > used for clock recovery and channel equalization, respectively. If the
> > sink provides an AUX_RD_INTERVAL value, it is used for both CR and CE
> > (and is in units of 4 ms). Best case a typical link training cycle would
> > therefore take something like 0.5 ms and worst case, since the number of
> > retries should be limited to 5, it'd be around 5 * 16 ms = 80 ms. That's
> > not counting the actual AUX transactions, though they should be pretty
> > fast.
> > 
> > Since this patch uses usleep_range(min, min * 2) the worst case now
> > becomes ~ 160 ms.
> 
> Would be nice to have some *actual* numbers in the commit message,
> otherwise it's all just guesswork.

I only have a limited range of test equipment. In the primary test-case,
which is an eDP panel, the difference was ~4.5 ms for udelay()/mdelay()
and ~5 ms for the usleep_range() case. I'll see if I can get one more
test setup running for better comparison.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp: Do not busy-loop during link training

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 09:36:04AM +0100, Thierry Reding wrote:
> On Mon, Dec 14, 2015 at 06:30:28PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 14, 2015 at 04:23:41PM +0100, Thierry Reding wrote:
> > > On Mon, Dec 14, 2015 at 04:48:09PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Dec 14, 2015 at 02:21:56PM +0100, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > 
> > > > > Use microsecond sleeps for the clock recovery and channel equalization
> > > > > delays during link training. The duration of these delays can be from
> > > > > 100 us up to 16 ms. It is rude to busy-loop for that amount of time.
> > > > 
> > > > Do you have some numbers on how this affects a typical link training
> > > > cycle?
> > > 
> > > Not really. Sinks aren't required to provide a value here, in which case
> > > the specification says that a default of 100 us and 400 us should be
> > > used for clock recovery and channel equalization, respectively. If the
> > > sink provides an AUX_RD_INTERVAL value, it is used for both CR and CE
> > > (and is in units of 4 ms). Best case a typical link training cycle would
> > > therefore take something like 0.5 ms and worst case, since the number of
> > > retries should be limited to 5, it'd be around 5 * 16 ms = 80 ms. That's
> > > not counting the actual AUX transactions, though they should be pretty
> > > fast.
> > > 
> > > Since this patch uses usleep_range(min, min * 2) the worst case now
> > > becomes ~ 160 ms.
> > 
> > Would be nice to have some *actual* numbers in the commit message,
> > otherwise it's all just guesswork.
> 
> I only have a limited range of test equipment. In the primary test-case,
> which is an eDP panel, the difference was ~4.5 ms for udelay()/mdelay()
> and ~5 ms for the usleep_range() case. I'll see if I can get one more
> test setup running for better comparison.

Hmm. Depending on how many iterations it took 5ms could be quite a bit,
or not much at all. I think ideally link training shouldn't take very
many milliseconds if you don't have iterate the settings too much. I've
not measured this myself in a while though, and I'm sure we're much
worse at least on some platforms currently.

But at least a few sample numbers in the commit message could then help
people if they later have to look into why link training is taking as
long as it is.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel