[PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[clinton.a.taylor]

From: Clint Taylor <clinton.a.taylor@intel.com>

Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
signal if the Data Link N is greater than 0x80000.
Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
value 20 bit instead of the maximum specification supported 24 bit value.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |    2 ++
 drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04c8f69..838d8d5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4869,6 +4869,8 @@ enum {
 
 #define  DATA_LINK_M_N_MASK	(0xffffff)
 #define  DATA_LINK_N_MAX	(0x800000)
+/* Maximum N value useable on some DP->HDMI converters */
+#define  DATA_LINK_REDUCED_N_MAX (0x80000)
 
 #define _PIPEA_DATA_N_G4X	0x70054
 #define _PIPEB_DATA_N_G4X	0x71054
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 010e5dd..6e1fdd2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 }
 
 static void compute_m_n(unsigned int m, unsigned int n,
-			uint32_t *ret_m, uint32_t *ret_n)
+			uint32_t *ret_m, uint32_t *ret_n,
+			uint32_t max_link_n)
 {
-	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
+	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
 	*ret_m = div_u64((uint64_t) m * *ret_n, n);
 	intel_reduce_m_n_ratio(ret_m, ret_n);
 }
@@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
 		       int pixel_clock, int link_clock,
 		       struct intel_link_m_n *m_n)
 {
+	uint32_t max_link_n = DATA_LINK_N_MAX;
 	m_n->tu = 64;
 
+	if ((nlanes==1) && (link_clock >= 540000))
+		max_link_n = DATA_LINK_REDUCED_N_MAX;
+
 	compute_m_n(bits_per_pixel * pixel_clock,
 		    link_clock * nlanes * 8,
-		    &m_n->gmch_m, &m_n->gmch_n);
+		    &m_n->gmch_m, &m_n->gmch_n,
+		    max_link_n);
 
 	compute_m_n(pixel_clock, link_clock,
-		    &m_n->link_m, &m_n->link_n);
+		    &m_n->link_m, &m_n->link_n,
+		    max_link_n);
 }
 
 static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Srivatsa, Anusha]

>-----Original Message-----
>From: Taylor, Clinton A
>Sent: Wednesday, March 22, 2017 4:28 PM
>To: Intel-gfx@lists.freedesktop.org
>Cc: Taylor, Clinton A <clinton.a.taylor@intel.com>; Nikula, Jani
><jani.nikula@intel.com>; Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Subject: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi
>converters
>
>From: Clint Taylor <clinton.a.taylor@intel.com>
>
>Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
>signal if the Data Link N is greater than 0x80000.
>Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
>value 20 bit instead of the maximum specification supported 24 bit value.

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>Cc: Jani Nikula <jani.nikula@intel.com>
>Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>
>Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>---
> drivers/gpu/drm/i915/i915_reg.h      |    2 ++
> drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 04c8f69..838d8d5 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -4869,6 +4869,8 @@ enum {
>
> #define  DATA_LINK_M_N_MASK	(0xffffff)
> #define  DATA_LINK_N_MAX	(0x800000)
>+/* Maximum N value useable on some DP->HDMI converters */ #define
>+DATA_LINK_REDUCED_N_MAX (0x80000)
>
> #define _PIPEA_DATA_N_G4X	0x70054
> #define _PIPEB_DATA_N_G4X	0x71054
>diff --git a/drivers/gpu/drm/i915/intel_display.c
>b/drivers/gpu/drm/i915/intel_display.c
>index 010e5dd..6e1fdd2 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc
>*crtc,  }
>
> static void compute_m_n(unsigned int m, unsigned int n,
>-			uint32_t *ret_m, uint32_t *ret_n)
>+			uint32_t *ret_m, uint32_t *ret_n,
>+			uint32_t max_link_n)
> {
>-	*ret_n = min_t(unsigned int, roundup_pow_of_two(n),
>DATA_LINK_N_MAX);
>+	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
> 	*ret_m = div_u64((uint64_t) m * *ret_n, n);
> 	intel_reduce_m_n_ratio(ret_m, ret_n);
> }
>@@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned
>int n,
> 		       int pixel_clock, int link_clock,
> 		       struct intel_link_m_n *m_n)
> {
>+	uint32_t max_link_n = DATA_LINK_N_MAX;
> 	m_n->tu = 64;
>
>+	if ((nlanes==1) && (link_clock >= 540000))
>+		max_link_n = DATA_LINK_REDUCED_N_MAX;
>+
> 	compute_m_n(bits_per_pixel * pixel_clock,
> 		    link_clock * nlanes * 8,
>-		    &m_n->gmch_m, &m_n->gmch_n);
>+		    &m_n->gmch_m, &m_n->gmch_n,
>+		    max_link_n);
>
> 	compute_m_n(pixel_clock, link_clock,
>-		    &m_n->link_m, &m_n->link_n);
>+		    &m_n->link_m, &m_n->link_n,
>+		    max_link_n);
> }
>
> static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>--
>1.7.9.5

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

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Manasi Navare]

On Wed, Mar 22, 2017 at 04:27:36PM -0700, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
> signal if the Data Link N is greater than 0x80000.
> Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
> value 20 bit instead of the maximum specification supported 24 bit value.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    2 ++
>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69..838d8d5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4869,6 +4869,8 @@ enum {
>  
>  #define  DATA_LINK_M_N_MASK	(0xffffff)
>  #define  DATA_LINK_N_MAX	(0x800000)
> +/* Maximum N value useable on some DP->HDMI converters */
> +#define  DATA_LINK_REDUCED_N_MAX (0x80000)
>  
>  #define _PIPEA_DATA_N_G4X	0x70054
>  #define _PIPEB_DATA_N_G4X	0x71054
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 010e5dd..6e1fdd2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  }
>  
>  static void compute_m_n(unsigned int m, unsigned int n,
> -			uint32_t *ret_m, uint32_t *ret_n)
> +			uint32_t *ret_m, uint32_t *ret_n,
> +			uint32_t max_link_n)
>  {
> -	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
> +	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
>  	intel_reduce_m_n_ratio(ret_m, ret_n);
>  }
> @@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
>  		       int pixel_clock, int link_clock,
>  		       struct intel_link_m_n *m_n)
>  {
> +	uint32_t max_link_n = DATA_LINK_N_MAX;
>  	m_n->tu = 64;
>  
> +	if ((nlanes==1) && (link_clock >= 540000))
> +		max_link_n = DATA_LINK_REDUCED_N_MAX;
> +

Is this very specific to 1 lane 5.4Gbps configuration or you think
this will be needed for higher rates and 1 lane?

If not why dont we make it very specific link_clock ==540000?

Regards
Manasi


>  	compute_m_n(bits_per_pixel * pixel_clock,
>  		    link_clock * nlanes * 8,
> -		    &m_n->gmch_m, &m_n->gmch_n);
> +		    &m_n->gmch_m, &m_n->gmch_n,
> +		    max_link_n);
>  
>  	compute_m_n(pixel_clock, link_clock,
> -		    &m_n->link_m, &m_n->link_n);
> +		    &m_n->link_m, &m_n->link_n,
> +		    max_link_n);
>  }
>  
>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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

✓ Fi.CI.BAT: success for drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Patchwork]

== Series Details ==

Series: drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters
URL   : https://patchwork.freedesktop.org/series/21724/
State : success

== Summary ==

Series 21724v1 drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters
https://patchwork.freedesktop.org/api/1.0/series/21724/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 468s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 455s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 581s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 551s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 498s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 505s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 437s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 447s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 516s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 496s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 486s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 486s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 600s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 489s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 521s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 458s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 551s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 420s

8229a8c712c22ff8e94e3244d4fd942a7dcd89af drm-tip: 2017y-03m-23d-09h-57m-34s UTC integration manifest
18752d2 drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4272/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Ville Syrjälä]

On Wed, Mar 22, 2017 at 04:27:36PM -0700, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
> signal if the Data Link N is greater than 0x80000.
> Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
> value 20 bit instead of the maximum specification supported 24 bit value.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    2 ++
>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69..838d8d5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4869,6 +4869,8 @@ enum {
>  
>  #define  DATA_LINK_M_N_MASK	(0xffffff)
>  #define  DATA_LINK_N_MAX	(0x800000)
> +/* Maximum N value useable on some DP->HDMI converters */
> +#define  DATA_LINK_REDUCED_N_MAX (0x80000)
>  
>  #define _PIPEA_DATA_N_G4X	0x70054
>  #define _PIPEB_DATA_N_G4X	0x71054
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 010e5dd..6e1fdd2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  }
>  
>  static void compute_m_n(unsigned int m, unsigned int n,
> -			uint32_t *ret_m, uint32_t *ret_n)
> +			uint32_t *ret_m, uint32_t *ret_n,
> +			uint32_t max_link_n)
>  {
> -	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
> +	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
>  	intel_reduce_m_n_ratio(ret_m, ret_n);
>  }
> @@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
>  		       int pixel_clock, int link_clock,
>  		       struct intel_link_m_n *m_n)
>  {
> +	uint32_t max_link_n = DATA_LINK_N_MAX;
>  	m_n->tu = 64;
>  
> +	if ((nlanes==1) && (link_clock >= 540000))
> +		max_link_n = DATA_LINK_REDUCED_N_MAX;

This could use a good comment explaining what's going on. I'd also like
to see a list of affected dongles as part of the comment. Historically
we've had far too many magic quirks for some unspecified piece of
hardware which makes it really difficult to change the code later as
there's no clue how one could even test it.

> +
>  	compute_m_n(bits_per_pixel * pixel_clock,
>  		    link_clock * nlanes * 8,
> -		    &m_n->gmch_m, &m_n->gmch_n);
> +		    &m_n->gmch_m, &m_n->gmch_n,
> +		    max_link_n);
>  
>  	compute_m_n(pixel_clock, link_clock,
> -		    &m_n->link_m, &m_n->link_n);
> +		    &m_n->link_m, &m_n->link_n,
> +		    max_link_n);
>  }
>  
>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Jani Nikula]

On Thu, 23 Mar 2017, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
>
> Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
> signal if the Data Link N is greater than 0x80000.
> Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
> value 20 bit instead of the maximum specification supported 24 bit value.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578

> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    2 ++
>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69..838d8d5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4869,6 +4869,8 @@ enum {
>  
>  #define  DATA_LINK_M_N_MASK	(0xffffff)
>  #define  DATA_LINK_N_MAX	(0x800000)
> +/* Maximum N value useable on some DP->HDMI converters */
> +#define  DATA_LINK_REDUCED_N_MAX (0x80000)
>  
>  #define _PIPEA_DATA_N_G4X	0x70054
>  #define _PIPEB_DATA_N_G4X	0x71054
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 010e5dd..6e1fdd2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  }
>  
>  static void compute_m_n(unsigned int m, unsigned int n,
> -			uint32_t *ret_m, uint32_t *ret_n)
> +			uint32_t *ret_m, uint32_t *ret_n,
> +			uint32_t max_link_n)
>  {
> -	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
> +	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);

If there's evidence suggesting "certain other operating systems" always
use a max (or fixed value) of 0x80000, perhaps we should just follow
suit? Simpler and less magical.

>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
>  	intel_reduce_m_n_ratio(ret_m, ret_n);
>  }
> @@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
>  		       int pixel_clock, int link_clock,
>  		       struct intel_link_m_n *m_n)
>  {
> +	uint32_t max_link_n = DATA_LINK_N_MAX;
>  	m_n->tu = 64;
>  
> +	if ((nlanes==1) && (link_clock >= 540000))

Is the problem really dependent on these conditions? You can get the
same problematic N value with nlanes == 2 && link_clock == 270000 too.

BR,
Jani.

> +		max_link_n = DATA_LINK_REDUCED_N_MAX;
> +
>  	compute_m_n(bits_per_pixel * pixel_clock,
>  		    link_clock * nlanes * 8,
> -		    &m_n->gmch_m, &m_n->gmch_n);
> +		    &m_n->gmch_m, &m_n->gmch_n,
> +		    max_link_n);
>  
>  	compute_m_n(pixel_clock, link_clock,
> -		    &m_n->link_m, &m_n->link_n);
> +		    &m_n->link_m, &m_n->link_n,
> +		    max_link_n);
>  }
>  
>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Clint Taylor]

On 03/23/2017 05:30 AM, Jani Nikula wrote:
> On Thu, 23 Mar 2017, clinton.a.taylor@intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
>> signal if the Data Link N is greater than 0x80000.
>> Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
>> value 20 bit instead of the maximum specification supported 24 bit value.
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578

I will add to the commit message.

>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      |    2 ++
>>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 04c8f69..838d8d5 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4869,6 +4869,8 @@ enum {
>>
>>  #define  DATA_LINK_M_N_MASK	(0xffffff)
>>  #define  DATA_LINK_N_MAX	(0x800000)
>> +/* Maximum N value useable on some DP->HDMI converters */
>> +#define  DATA_LINK_REDUCED_N_MAX (0x80000)
>>
>>  #define _PIPEA_DATA_N_G4X	0x70054
>>  #define _PIPEB_DATA_N_G4X	0x71054
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 010e5dd..6e1fdd2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>  }
>>
>>  static void compute_m_n(unsigned int m, unsigned int n,
>> -			uint32_t *ret_m, uint32_t *ret_n)
>> +			uint32_t *ret_m, uint32_t *ret_n,
>> +			uint32_t max_link_n)
>>  {
>> -	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
>> +	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
>
> If there's evidence suggesting "certain other operating systems" always
> use a max (or fixed value) of 0x80000, perhaps we should just follow
> suit? Simpler and less magical.
>

The other OS's don't appear to be fixed to 0x80000. The calculation in 
i915 rounds up to the nearest power of 2 and the other OS's might have a 
slightly different calculation to the nearest power of 2. Of course I 
haven't seen the other OS's code to know their exact formula. HBR3 will 
cause a higher value to be calculated and having a fixed value may cause 
issues. The i915 formula works and reducing the value can cause 
precision issues in the ratio with the pixel clock.

>>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
>>  	intel_reduce_m_n_ratio(ret_m, ret_n);
>>  }
>> @@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
>>  		       int pixel_clock, int link_clock,
>>  		       struct intel_link_m_n *m_n)
>>  {
>> +	uint32_t max_link_n = DATA_LINK_N_MAX;
>>  	m_n->tu = 64;
>>
>> +	if ((nlanes==1) && (link_clock >= 540000))
>
> Is the problem really dependent on these conditions? You can get the
> same problematic N value with nlanes == 2 && link_clock == 270000 too.
>

The offending device only supports a single DP lane up to HBR2.5. This 
check matches the datasheet for the part. The offending device works 
with our current calculation at 1 lane HBR (270000).

> BR,
> Jani.
>
>> +		max_link_n = DATA_LINK_REDUCED_N_MAX;
>> +
>>  	compute_m_n(bits_per_pixel * pixel_clock,
>>  		    link_clock * nlanes * 8,
>> -		    &m_n->gmch_m, &m_n->gmch_n);
>> +		    &m_n->gmch_m, &m_n->gmch_n,
>> +		    max_link_n);
>>
>>  	compute_m_n(pixel_clock, link_clock,
>> -		    &m_n->link_m, &m_n->link_n);
>> +		    &m_n->link_m, &m_n->link_n,
>> +		    max_link_n);
>>  }
>>
>>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>

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

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Jani Nikula]

On Thu, 23 Mar 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> On 03/23/2017 05:30 AM, Jani Nikula wrote:
>> On Thu, 23 Mar 2017, clinton.a.taylor@intel.com wrote:
>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>>
>>> Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
>>> signal if the Data Link N is greater than 0x80000.
>>> Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
>>> value 20 bit instead of the maximum specification supported 24 bit value.
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>>
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
>
> I will add to the commit message.
>
>>
>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_reg.h      |    2 ++
>>>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
>>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 04c8f69..838d8d5 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -4869,6 +4869,8 @@ enum {
>>>
>>>  #define  DATA_LINK_M_N_MASK	(0xffffff)
>>>  #define  DATA_LINK_N_MAX	(0x800000)
>>> +/* Maximum N value useable on some DP->HDMI converters */
>>> +#define  DATA_LINK_REDUCED_N_MAX (0x80000)
>>>
>>>  #define _PIPEA_DATA_N_G4X	0x70054
>>>  #define _PIPEB_DATA_N_G4X	0x71054
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 010e5dd..6e1fdd2 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>>  }
>>>
>>>  static void compute_m_n(unsigned int m, unsigned int n,
>>> -			uint32_t *ret_m, uint32_t *ret_n)
>>> +			uint32_t *ret_m, uint32_t *ret_n,
>>> +			uint32_t max_link_n)
>>>  {
>>> -	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
>>> +	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
>>
>> If there's evidence suggesting "certain other operating systems" always
>> use a max (or fixed value) of 0x80000, perhaps we should just follow
>> suit? Simpler and less magical.
>>
>
> The other OS's don't appear to be fixed to 0x80000. The calculation in 
> i915 rounds up to the nearest power of 2 and the other OS's might have a 
> slightly different calculation to the nearest power of 2. Of course I 
> haven't seen the other OS's code to know their exact formula. HBR3 will 
> cause a higher value to be calculated and having a fixed value may cause 
> issues. The i915 formula works and reducing the value can cause 
> precision issues in the ratio with the pixel clock.
>
>>>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
>>>  	intel_reduce_m_n_ratio(ret_m, ret_n);
>>>  }
>>> @@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
>>>  		       int pixel_clock, int link_clock,
>>>  		       struct intel_link_m_n *m_n)
>>>  {
>>> +	uint32_t max_link_n = DATA_LINK_N_MAX;
>>>  	m_n->tu = 64;
>>>
>>> +	if ((nlanes==1) && (link_clock >= 540000))
>>
>> Is the problem really dependent on these conditions? You can get the
>> same problematic N value with nlanes == 2 && link_clock == 270000 too.
>>
>
> The offending device only supports a single DP lane up to HBR2.5. This 
> check matches the datasheet for the part. The offending device works 
> with our current calculation at 1 lane HBR (270000).

Okay, so what bugs me about the approach here is that this adds an
arbitrary condition to apply a quirk to a specific device.

Instead of "if device X, then apply restriction A", this adds "if
condition Y, then apply restriction A". If I understand you correctly,
"condition Y" is a superset of "device X", i.e. Y happens also on
devices other than X, but on device X condition Y always holds.

I'd really like it if we could come up with a) a quirk that we apply
only on the affected device(s), or b) rules for M/N that generally make
sense with no need to resort to seeminly arbitrary exceptions.

With the latter I mean things like reducing the M/N before rounding N up
to power of two (M and N are always divisible by 2, for example) or
having intel_reduce_m_n_ratio() shift them right as long as they have
bit 0 unset. At a glance, I'm not sure if this is enough to bring down
the N to within the limits of the device, without intentional loss of
precision.

BR,
Jani.


>
>> BR,
>> Jani.
>>
>>> +		max_link_n = DATA_LINK_REDUCED_N_MAX;
>>> +
>>>  	compute_m_n(bits_per_pixel * pixel_clock,
>>>  		    link_clock * nlanes * 8,
>>> -		    &m_n->gmch_m, &m_n->gmch_n);
>>> +		    &m_n->gmch_m, &m_n->gmch_n,
>>> +		    max_link_n);
>>>
>>>  	compute_m_n(pixel_clock, link_clock,
>>> -		    &m_n->link_m, &m_n->link_n);
>>> +		    &m_n->link_m, &m_n->link_n,
>>> +		    max_link_n);
>>>  }
>>>
>>>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>>
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Clint Taylor]

On 03/23/2017 10:23 AM, Jani Nikula wrote:
> On Thu, 23 Mar 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>> On 03/23/2017 05:30 AM, Jani Nikula wrote:
>>> On Thu, 23 Mar 2017, clinton.a.taylor@intel.com wrote:
>>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>>>
>>>> Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
>>>> signal if the Data Link N is greater than 0x80000.
>>>> Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
>>>> value 20 bit instead of the maximum specification supported 24 bit value.
>>>>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>>>
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
>>
>> I will add to the commit message.
>>
>>>
>>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_reg.h      |    2 ++
>>>>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
>>>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 04c8f69..838d8d5 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -4869,6 +4869,8 @@ enum {
>>>>
>>>>  #define  DATA_LINK_M_N_MASK	(0xffffff)
>>>>  #define  DATA_LINK_N_MAX	(0x800000)
>>>> +/* Maximum N value useable on some DP->HDMI converters */
>>>> +#define  DATA_LINK_REDUCED_N_MAX (0x80000)
>>>>
>>>>  #define _PIPEA_DATA_N_G4X	0x70054
>>>>  #define _PIPEB_DATA_N_G4X	0x71054
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 010e5dd..6e1fdd2 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>>>  }
>>>>
>>>>  static void compute_m_n(unsigned int m, unsigned int n,
>>>> -			uint32_t *ret_m, uint32_t *ret_n)
>>>> +			uint32_t *ret_m, uint32_t *ret_n,
>>>> +			uint32_t max_link_n)
>>>>  {
>>>> -	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
>>>> +	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
>>>
>>> If there's evidence suggesting "certain other operating systems" always
>>> use a max (or fixed value) of 0x80000, perhaps we should just follow
>>> suit? Simpler and less magical.
>>>
>>
>> The other OS's don't appear to be fixed to 0x80000. The calculation in
>> i915 rounds up to the nearest power of 2 and the other OS's might have a
>> slightly different calculation to the nearest power of 2. Of course I
>> haven't seen the other OS's code to know their exact formula. HBR3 will
>> cause a higher value to be calculated and having a fixed value may cause
>> issues. The i915 formula works and reducing the value can cause
>> precision issues in the ratio with the pixel clock.
>>
>>>>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
>>>>  	intel_reduce_m_n_ratio(ret_m, ret_n);
>>>>  }
>>>> @@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
>>>>  		       int pixel_clock, int link_clock,
>>>>  		       struct intel_link_m_n *m_n)
>>>>  {
>>>> +	uint32_t max_link_n = DATA_LINK_N_MAX;
>>>>  	m_n->tu = 64;
>>>>
>>>> +	if ((nlanes==1) && (link_clock >= 540000))
>>>
>>> Is the problem really dependent on these conditions? You can get the
>>> same problematic N value with nlanes == 2 && link_clock == 270000 too.
>>>
>>
>> The offending device only supports a single DP lane up to HBR2.5. This
>> check matches the datasheet for the part. The offending device works
>> with our current calculation at 1 lane HBR (270000).
>
> Okay, so what bugs me about the approach here is that this adds an
> arbitrary condition to apply a quirk to a specific device.
>
> Instead of "if device X, then apply restriction A", this adds "if
> condition Y, then apply restriction A". If I understand you correctly,
> "condition Y" is a superset of "device X", i.e. Y happens also on
> devices other than X, but on device X condition Y always holds.
>
> I'd really like it if we could come up with a) a quirk that we apply
> only on the affected device(s), or b) rules for M/N that generally make
> sense with no need to resort to seeminly arbitrary exceptions.
>

I can detect the specific device through the DP OUI branch value 
returned during DP detect. I can also detect through the device ID 
string DPCD 0x503-0x508 currently not parsed in i915. Either would 
satisfy Device X, Condition Y, then apply workaround A.

I would prefer a solution for B (rules for M/N), but the code doesn't 
appear to be broken and I don't believe we should "Fix" something that 
is working. The device also works by changing the roundup_pow_of_two() 
to rounddown_pow_of_two() however that would apply the change to every 
device connected.


> With the latter I mean things like reducing the M/N before rounding N up
> to power of two (M and N are always divisible by 2, for example) or
> having intel_reduce_m_n_ratio() shift them right as long as they have
> bit 0 unset. At a glance, I'm not sure if this is enough to bring down
> the N to within the limits of the device, without intentional loss of
> precision.
>
> BR,
> Jani.
>
>
>>
>>> BR,
>>> Jani.
>>>
>>>> +		max_link_n = DATA_LINK_REDUCED_N_MAX;
>>>> +
>>>>  	compute_m_n(bits_per_pixel * pixel_clock,
>>>>  		    link_clock * nlanes * 8,
>>>> -		    &m_n->gmch_m, &m_n->gmch_n);
>>>> +		    &m_n->gmch_m, &m_n->gmch_n,
>>>> +		    max_link_n);
>>>>
>>>>  	compute_m_n(pixel_clock, link_clock,
>>>> -		    &m_n->link_m, &m_n->link_n);
>>>> +		    &m_n->link_m, &m_n->link_n,
>>>> +		    max_link_n);
>>>>  }
>>>>
>>>>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>>>
>>
>

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

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Pandiyan, Dhinakaran]

On Thu, 2017-03-23 at 10:59 -0700, Clint Taylor wrote:
> On 03/23/2017 10:23 AM, Jani Nikula wrote:
> > On Thu, 23 Mar 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> >> On 03/23/2017 05:30 AM, Jani Nikula wrote:
> >>> On Thu, 23 Mar 2017, clinton.a.taylor@intel.com wrote:
> >>>> From: Clint Taylor <clinton.a.taylor@intel.com>
> >>>>
> >>>> Several major vendor USB-C->HDMI converters fail to recover a 5.4 GHz 1 lane
> >>>> signal if the Data Link N is greater than 0x80000.
> >>>> Patch detects when 1 lane 5.4 GHz signal is being used and makes the maximum
> >>>> value 20 bit instead of the maximum specification supported 24 bit value.
> >>>>
> >>>> Cc: Jani Nikula <jani.nikula@intel.com>
> >>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >>>>
> >>>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
> >>
> >> I will add to the commit message.
> >>
> >>>
> >>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/i915_reg.h      |    2 ++
> >>>>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++----
> >>>>  2 files changed, 13 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>>> index 04c8f69..838d8d5 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>> @@ -4869,6 +4869,8 @@ enum {
> >>>>
> >>>>  #define  DATA_LINK_M_N_MASK	(0xffffff)
> >>>>  #define  DATA_LINK_N_MAX	(0x800000)
> >>>> +/* Maximum N value useable on some DP->HDMI converters */
> >>>> +#define  DATA_LINK_REDUCED_N_MAX (0x80000)
> >>>>
> >>>>  #define _PIPEA_DATA_N_G4X	0x70054
> >>>>  #define _PIPEB_DATA_N_G4X	0x71054
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index 010e5dd..6e1fdd2 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -6315,9 +6315,10 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> >>>>  }
> >>>>
> >>>>  static void compute_m_n(unsigned int m, unsigned int n,
> >>>> -			uint32_t *ret_m, uint32_t *ret_n)
> >>>> +			uint32_t *ret_m, uint32_t *ret_n,
> >>>> +			uint32_t max_link_n)
> >>>>  {
> >>>> -	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
> >>>> +	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), max_link_n);
> >>>
> >>> If there's evidence suggesting "certain other operating systems" always
> >>> use a max (or fixed value) of 0x80000, perhaps we should just follow
> >>> suit? Simpler and less magical.
> >>>
> >>
> >> The other OS's don't appear to be fixed to 0x80000. The calculation in
> >> i915 rounds up to the nearest power of 2 and the other OS's might have a
> >> slightly different calculation to the nearest power of 2. Of course I
> >> haven't seen the other OS's code to know their exact formula. HBR3 will
> >> cause a higher value to be calculated and having a fixed value may cause
> >> issues. The i915 formula works and reducing the value can cause
> >> precision issues in the ratio with the pixel clock.
> >>
> >>>>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
> >>>>  	intel_reduce_m_n_ratio(ret_m, ret_n);
> >>>>  }
> >>>> @@ -6327,14 +6328,20 @@ static void compute_m_n(unsigned int m, unsigned int n,
> >>>>  		       int pixel_clock, int link_clock,
> >>>>  		       struct intel_link_m_n *m_n)
> >>>>  {
> >>>> +	uint32_t max_link_n = DATA_LINK_N_MAX;
> >>>>  	m_n->tu = 64;
> >>>>
> >>>> +	if ((nlanes==1) && (link_clock >= 540000))
> >>>
> >>> Is the problem really dependent on these conditions? You can get the
> >>> same problematic N value with nlanes == 2 && link_clock == 270000 too.
> >>>
> >>
> >> The offending device only supports a single DP lane up to HBR2.5. This
> >> check matches the datasheet for the part. The offending device works
> >> with our current calculation at 1 lane HBR (270000).
> >
> > Okay, so what bugs me about the approach here is that this adds an
> > arbitrary condition to apply a quirk to a specific device.
> >
> > Instead of "if device X, then apply restriction A", this adds "if
> > condition Y, then apply restriction A". If I understand you correctly,
> > "condition Y" is a superset of "device X", i.e. Y happens also on
> > devices other than X, but on device X condition Y always holds.
> >
> > I'd really like it if we could come up with a) a quirk that we apply
> > only on the affected device(s), or b) rules for M/N that generally make
> > sense with no need to resort to seeminly arbitrary exceptions.
> >
> 
> I can detect the specific device through the DP OUI branch value 
> returned during DP detect. I can also detect through the device ID 
> string DPCD 0x503-0x508 currently not parsed in i915. Either would 
> satisfy Device X, Condition Y, then apply workaround A.
> 
drm_dp_helper.c: drm_dp_downstream_id() does that.

-DK

> I would prefer a solution for B (rules for M/N), but the code doesn't 
> appear to be broken and I don't believe we should "Fix" something that 
> is working. The device also works by changing the roundup_pow_of_two() 
> to rounddown_pow_of_two() however that would apply the change to every 
> device connected.
> 
> 
> > With the latter I mean things like reducing the M/N before rounding N up
> > to power of two (M and N are always divisible by 2, for example) or
> > having intel_reduce_m_n_ratio() shift them right as long as they have
> > bit 0 unset. At a glance, I'm not sure if this is enough to bring down
> > the N to within the limits of the device, without intentional loss of
> > precision.
> >
> > BR,
> > Jani.
> >
> >
> >>
> >>> BR,
> >>> Jani.
> >>>
> >>>> +		max_link_n = DATA_LINK_REDUCED_N_MAX;
> >>>> +
> >>>>  	compute_m_n(bits_per_pixel * pixel_clock,
> >>>>  		    link_clock * nlanes * 8,
> >>>> -		    &m_n->gmch_m, &m_n->gmch_n);
> >>>> +		    &m_n->gmch_m, &m_n->gmch_n,
> >>>> +		    max_link_n);
> >>>>
> >>>>  	compute_m_n(pixel_clock, link_clock,
> >>>> -		    &m_n->link_m, &m_n->link_n);
> >>>> +		    &m_n->link_m, &m_n->link_n,
> >>>> +		    max_link_n);
> >>>>  }
> >>>>
> >>>>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
> >>>
> >>
> >
> 
> _______________________________________________
> 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] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Jani Nikula]

On Thu, 23 Mar 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> I would prefer a solution for B (rules for M/N), but the code doesn't 
> appear to be broken and I don't believe we should "Fix" something that 
> is working. The device also works by changing the roundup_pow_of_two() 
> to rounddown_pow_of_two() however that would apply the change to every 
> device connected.

Looking at this again, the problem must be in the (external) link M/N,
not (internal) data M/N. Thus it's only about the pixel clock / link
clock ratio. Right?

With current code link N exceeds 0x80000 only when link clock >= 540000
kHz. Except for the eDP intermediate link clocks, at least the four
least significant bits are always zero. But just one bit shift right
would be enough to bring even the DP 1.4 810000 kHz link clock under
0x80000 link N. The pixel clock for modes that require a link clock >=
540000 kHz would also have several least significant bits zero.

Unless the user provides a mode with an odd pixel clock value, we can
reduce the numbers to reach the goal, with no loss in precision, and it
doesn't even feel like a hack. The DP spec even mentions sources making
choices that "allow for static and relatively small Mvid and Nvid
values".

Of course, all of this hinges on the problem being specific to the link
M/N, and independent of data M/N.

BR,
Jani.


diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9a28a8917dc1..55bb6cf2a2d3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6337,6 +6337,15 @@ intel_reduce_m_n_ratio(uint32_t *num, uint32_t *den)
 static void compute_m_n(unsigned int m, unsigned int n,
 			uint32_t *ret_m, uint32_t *ret_n)
 {
+	/*
+	 * Reduce M/N as much as possible without loss in precision. Several DP
+	 * dongles in particular seem to be fussy about too large M/N values.
+	 */
+	while ((m & 1) == 0 && (n & 1) == 0) {
+		m >>= 1;
+		n >>= 1;
+	}
+
 	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
 	*ret_m = div_u64((uint64_t) m * *ret_n, n);
 	intel_reduce_m_n_ratio(ret_m, ret_n);
---

And the hack on top that ensures we're below 0x80000 link N independent
of the pixel clock. Note that the only loss in precision here is the one
bit in pixel clock; the other values passed in will always be even.

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 55bb6cf2a2d3..b51b836b9538 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6341,6 +6341,8 @@ static void compute_m_n(unsigned int m, unsigned int n,
 	 * Reduce M/N as much as possible without loss in precision. Several DP
 	 * dongles in particular seem to be fussy about too large M/N values.
 	 */
+	m >>= 1;
+	n >>= 1;
 	while ((m & 1) == 0 && (n & 1) == 0) {
 		m >>= 1;
 		n >>= 1;



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reduce Data Link N value for 1 lane DP->hdmi converters

[Clint Taylor]

On 03/24/2017 04:25 AM, Jani Nikula wrote:
> On Thu, 23 Mar 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>> I would prefer a solution for B (rules for M/N), but the code doesn't
>> appear to be broken and I don't believe we should "Fix" something that
>> is working. The device also works by changing the roundup_pow_of_two()
>> to rounddown_pow_of_two() however that would apply the change to every
>> device connected.
>
> Looking at this again, the problem must be in the (external) link M/N,
> not (internal) data M/N. Thus it's only about the pixel clock / link
> clock ratio. Right?
>

Correct, Pixel/Link clock ratio only.

> With current code link N exceeds 0x80000 only when link clock >= 540000
> kHz. Except for the eDP intermediate link clocks, at least the four
> least significant bits are always zero. But just one bit shift right
> would be enough to bring even the DP 1.4 810000 kHz link clock under
> 0x80000 link N. The pixel clock for modes that require a link clock >=
> 540000 kHz would also have several least significant bits zero.
>
> Unless the user provides a mode with an odd pixel clock value, we can
> reduce the numbers to reach the goal, with no loss in precision, and it
> doesn't even feel like a hack. The DP spec even mentions sources making
> choices that "allow for static and relatively small Mvid and Nvid
> values".

As long as the precision is maintained a lower value should be fine.
>
> Of course, all of this hinges on the problem being specific to the link
> M/N, and independent of data M/N.
>
> BR,
> Jani.
>
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9a28a8917dc1..55bb6cf2a2d3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6337,6 +6337,15 @@ intel_reduce_m_n_ratio(uint32_t *num, uint32_t *den)
>  static void compute_m_n(unsigned int m, unsigned int n,
>  			uint32_t *ret_m, uint32_t *ret_n)
>  {
> +	/*
> +	 * Reduce M/N as much as possible without loss in precision. Several DP
> +	 * dongles in particular seem to be fussy about too large M/N values.
> +	 */
> +	while ((m & 1) == 0 && (n & 1) == 0) {
> +		m >>= 1;
> +		n >>= 1;
> +	}
> +
>  	*ret_n = min_t(unsigned int, roundup_pow_of_two(n), DATA_LINK_N_MAX);
>  	*ret_m = div_u64((uint64_t) m * *ret_n, n);
>  	intel_reduce_m_n_ratio(ret_m, ret_n);
> ---
>
> And the hack on top that ensures we're below 0x80000 link N independent
> of the pixel clock. Note that the only loss in precision here is the one
> bit in pixel clock; the other values passed in will always be even.
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 55bb6cf2a2d3..b51b836b9538 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6341,6 +6341,8 @@ static void compute_m_n(unsigned int m, unsigned int n,
>  	 * Reduce M/N as much as possible without loss in precision. Several DP
>  	 * dongles in particular seem to be fussy about too large M/N values.
>  	 */
> +	m >>= 1;
> +	n >>= 1;
>  	while ((m & 1) == 0 && (n & 1) == 0) {
>  		m >>= 1;
>  		n >>= 1;
>
>
>

The above code at 1 lane @ 5.4 Ghz 1080P@60 (148.400Mhz pixel clock) 
makes GMCH Data N,M = 0x40000, 0x34CCC and Link N,M = 0x40000, 0x11999. 
The offending dongle appears to work fine with the new values as 
expected. I saw Nvid values of 0x10000 at 2048x1152 and the dongle still 
worked.

-Clint


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