All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm: Make the bw/link rate calculations more forgiving
@ 2019-07-17 16:01 Sean Paul
  2019-07-17 16:20 ` Ville Syrjälä
  0 siblings, 1 reply; 3+ messages in thread
From: Sean Paul @ 2019-07-17 16:01 UTC (permalink / raw
  To: dri-devel; +Cc: Sean Paul

From: Sean Paul <seanpaul@chromium.org>

Although the DisplayPort spec explicitly calls out the 1.62/2.7/5.4/8.1
link rates, the value of LINK_BW_SET is calculated.  The DisplayPort
spec says "Main-Link Bandwidth Setting = Value x 0.27Gbps/lane".

A bridge that we're looking to upstream uses 6.75Gbps rate (value 0x19)
[1], and that precludes it from using these functions.

This patch calculates the values according to spec instead of
restricting these values to one of the DP_LINK_BW_* #defines.

No functional change for the well-defined values, but we lose the
warning for ill-defined bw values.

Signed-off-by: Sean Paul <seanpaul@chromium.org>

[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1689251/2/drivers/gpu/drm/bridge/analogix/anx7625.c#636
---
 drivers/gpu/drm/drm_dp_helper.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 0b994d083a89..ffc68d305afe 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -152,38 +152,15 @@ EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
 
 u8 drm_dp_link_rate_to_bw_code(int link_rate)
 {
-	switch (link_rate) {
-	default:
-		WARN(1, "unknown DP link rate %d, using %x\n", link_rate,
-		     DP_LINK_BW_1_62);
-		/* fall through */
-	case 162000:
-		return DP_LINK_BW_1_62;
-	case 270000:
-		return DP_LINK_BW_2_7;
-	case 540000:
-		return DP_LINK_BW_5_4;
-	case 810000:
-		return DP_LINK_BW_8_1;
-	}
+	/* Spec says link_bw = link_rate / 0.27Gbps */
+	return link_rate / 27000;
 }
 EXPORT_SYMBOL(drm_dp_link_rate_to_bw_code);
 
 int drm_dp_bw_code_to_link_rate(u8 link_bw)
 {
-	switch (link_bw) {
-	default:
-		WARN(1, "unknown DP link BW code %x, using 162000\n", link_bw);
-		/* fall through */
-	case DP_LINK_BW_1_62:
-		return 162000;
-	case DP_LINK_BW_2_7:
-		return 270000;
-	case DP_LINK_BW_5_4:
-		return 540000;
-	case DP_LINK_BW_8_1:
-		return 810000;
-	}
+	/* Spec says link_rate = link_bw * 0.27Gbps */
+	return link_bw * 27000;
 }
 EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] drm: Make the bw/link rate calculations more forgiving
  2019-07-17 16:01 [PATCH] drm: Make the bw/link rate calculations more forgiving Sean Paul
@ 2019-07-17 16:20 ` Ville Syrjälä
  2019-07-17 16:31   ` Sean Paul
  0 siblings, 1 reply; 3+ messages in thread
From: Ville Syrjälä @ 2019-07-17 16:20 UTC (permalink / raw
  To: Sean Paul; +Cc: Sean Paul, dri-devel

On Wed, Jul 17, 2019 at 12:01:48PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Although the DisplayPort spec explicitly calls out the 1.62/2.7/5.4/8.1
> link rates, the value of LINK_BW_SET is calculated.  The DisplayPort
> spec says "Main-Link Bandwidth Setting = Value x 0.27Gbps/lane".
> 
> A bridge that we're looking to upstream uses 6.75Gbps rate (value 0x19)
> [1], and that precludes it from using these functions.

The DP spec has this note:
"A MyDP Source device, upon reading the MAX_LINK_RATE register of the
 downstream DPRX programmed to 19h (which can be the case only for a
 MyDP-to-Legacy or MyDP-to-DP lane count converter) can program the
 LINK_BW_SET register (DPCD Address 00100h) to 19h to enable 6.75Gbps/lane."

Which I guess is the thing you're seeing.

But I think the patch makes sense in any case. Otherwise anyone who
plugs in some new display to a machine with an old kernel will likely
get that WARN. Assuming the driver correctly limits the max link rate
based on the source device capabilities there's no harm in letting
the sink declare higher limits.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> This patch calculates the values according to spec instead of
> restricting these values to one of the DP_LINK_BW_* #defines.
> 
> No functional change for the well-defined values, but we lose the
> warning for ill-defined bw values.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> 
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1689251/2/drivers/gpu/drm/bridge/analogix/anx7625.c#636
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 31 ++++---------------------------
>  1 file changed, 4 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 0b994d083a89..ffc68d305afe 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -152,38 +152,15 @@ EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
>  
>  u8 drm_dp_link_rate_to_bw_code(int link_rate)
>  {
> -	switch (link_rate) {
> -	default:
> -		WARN(1, "unknown DP link rate %d, using %x\n", link_rate,
> -		     DP_LINK_BW_1_62);
> -		/* fall through */
> -	case 162000:
> -		return DP_LINK_BW_1_62;
> -	case 270000:
> -		return DP_LINK_BW_2_7;
> -	case 540000:
> -		return DP_LINK_BW_5_4;
> -	case 810000:
> -		return DP_LINK_BW_8_1;
> -	}
> +	/* Spec says link_bw = link_rate / 0.27Gbps */
> +	return link_rate / 27000;
>  }
>  EXPORT_SYMBOL(drm_dp_link_rate_to_bw_code);
>  
>  int drm_dp_bw_code_to_link_rate(u8 link_bw)
>  {
> -	switch (link_bw) {
> -	default:
> -		WARN(1, "unknown DP link BW code %x, using 162000\n", link_bw);
> -		/* fall through */
> -	case DP_LINK_BW_1_62:
> -		return 162000;
> -	case DP_LINK_BW_2_7:
> -		return 270000;
> -	case DP_LINK_BW_5_4:
> -		return 540000;
> -	case DP_LINK_BW_8_1:
> -		return 810000;
> -	}
> +	/* Spec says link_rate = link_bw * 0.27Gbps */
> +	return link_bw * 27000;
>  }
>  EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
>  
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] drm: Make the bw/link rate calculations more forgiving
  2019-07-17 16:20 ` Ville Syrjälä
@ 2019-07-17 16:31   ` Sean Paul
  0 siblings, 0 replies; 3+ messages in thread
From: Sean Paul @ 2019-07-17 16:31 UTC (permalink / raw
  To: Ville Syrjälä; +Cc: Sean Paul, Sean Paul, dri-devel

On Wed, Jul 17, 2019 at 07:20:29PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 17, 2019 at 12:01:48PM -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > Although the DisplayPort spec explicitly calls out the 1.62/2.7/5.4/8.1
> > link rates, the value of LINK_BW_SET is calculated.  The DisplayPort
> > spec says "Main-Link Bandwidth Setting = Value x 0.27Gbps/lane".
> > 
> > A bridge that we're looking to upstream uses 6.75Gbps rate (value 0x19)
> > [1], and that precludes it from using these functions.
> 
> The DP spec has this note:
> "A MyDP Source device, upon reading the MAX_LINK_RATE register of the
>  downstream DPRX programmed to 19h (which can be the case only for a
>  MyDP-to-Legacy or MyDP-to-DP lane count converter) can program the
>  LINK_BW_SET register (DPCD Address 00100h) to 19h to enable 6.75Gbps/lane."
> 
> Which I guess is the thing you're seeing.

Thanks for digging this up. The spec I reviewed didn't have this, but it did
mention some more esoteric exceptions to the Big 4 (it might not even have HBR3
either, I'm not sure. /me really needs to get up-to-date specs) defined rates. I'll
add this to my commit message when I apply it.

> 
> But I think the patch makes sense in any case. Otherwise anyone who
> plugs in some new display to a machine with an old kernel will likely
> get that WARN. Assuming the driver correctly limits the max link rate
> based on the source device capabilities there's no harm in letting
> the sink declare higher limits.

Yep, and not only the WARN, but they'll also get 1.62Gbps rate/code. So
calculating the value seems like the safer route to take.

> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks! I'll apply it to drm-misc-next.

Sean

> 
> > 
> > This patch calculates the values according to spec instead of
> > restricting these values to one of the DP_LINK_BW_* #defines.
> > 
> > No functional change for the well-defined values, but we lose the
> > warning for ill-defined bw values.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > 
> > [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1689251/2/drivers/gpu/drm/bridge/analogix/anx7625.c#636
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 31 ++++---------------------------
> >  1 file changed, 4 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 0b994d083a89..ffc68d305afe 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -152,38 +152,15 @@ EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay);
> >  
> >  u8 drm_dp_link_rate_to_bw_code(int link_rate)
> >  {
> > -	switch (link_rate) {
> > -	default:
> > -		WARN(1, "unknown DP link rate %d, using %x\n", link_rate,
> > -		     DP_LINK_BW_1_62);
> > -		/* fall through */
> > -	case 162000:
> > -		return DP_LINK_BW_1_62;
> > -	case 270000:
> > -		return DP_LINK_BW_2_7;
> > -	case 540000:
> > -		return DP_LINK_BW_5_4;
> > -	case 810000:
> > -		return DP_LINK_BW_8_1;
> > -	}
> > +	/* Spec says link_bw = link_rate / 0.27Gbps */
> > +	return link_rate / 27000;
> >  }
> >  EXPORT_SYMBOL(drm_dp_link_rate_to_bw_code);
> >  
> >  int drm_dp_bw_code_to_link_rate(u8 link_bw)
> >  {
> > -	switch (link_bw) {
> > -	default:
> > -		WARN(1, "unknown DP link BW code %x, using 162000\n", link_bw);
> > -		/* fall through */
> > -	case DP_LINK_BW_1_62:
> > -		return 162000;
> > -	case DP_LINK_BW_2_7:
> > -		return 270000;
> > -	case DP_LINK_BW_5_4:
> > -		return 540000;
> > -	case DP_LINK_BW_8_1:
> > -		return 810000;
> > -	}
> > +	/* Spec says link_rate = link_bw * 0.27Gbps */
> > +	return link_bw * 27000;
> >  }
> >  EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
> >  
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-07-17 16:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-17 16:01 [PATCH] drm: Make the bw/link rate calculations more forgiving Sean Paul
2019-07-17 16:20 ` Ville Syrjälä
2019-07-17 16:31   ` Sean Paul

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.