All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/dp: reduce link M/N parameters
@ 2017-03-27 11:33 Jani Nikula
  2017-03-27 12:04 ` ✗ Fi.CI.BAT: failure for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Jani Nikula @ 2017-03-27 11:33 UTC (permalink / raw)
  To: intel-gfx; +Cc: jani.nikula, Daniel Vetter

Several major vendor USB-C->HDMI converters, in particular the DA200,
fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
0x80000.

The link M and N depend on the pixel clock and link clock ratio. 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. 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.

The DP spec even mentions sources making choices that "allow for static
and relatively small Mvid and Nvid values", thus reducing the link M/N
regardless of the sink in question seems justified.

Everything here is based on the work and information gathered by Clint
Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
the parameters regardless of lane count, link rate, or sink.

Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
Tested-by: Mads <mads@ab3.no>
Tested-by: PJ <foobar@pjmodos.net>
Tested-by: François Guerraz <kubrick@fgv6.net>
Tested-by: Lev Popov <leo@nabam.net>
Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
Cc: Clint Taylor <clinton.a.taylor@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

This is cc: stable material, but due to the slight risk of regressions
(there's always the risk, however small, when you change parameters that
affect all sinks) I'd prefer letting this simmer for a while, and asking
for an explicit stable backport afterwards.
---
 drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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);
-- 
2.1.4

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

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

* ✗ Fi.CI.BAT: failure for drm/i915/dp: reduce link M/N parameters
  2017-03-27 11:33 [PATCH] drm/i915/dp: reduce link M/N parameters Jani Nikula
@ 2017-03-27 12:04 ` Patchwork
  2017-03-27 13:01 ` [PATCH] " Daniel Vetter
  2017-03-27 18:49 ` Pandiyan, Dhinakaran
  2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-03-27 12:04 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/dp: reduce link M/N parameters
URL   : https://patchwork.freedesktop.org/series/21927/
State : failure

== Summary ==

Series 21927v1 drm/i915/dp: reduce link M/N parameters
https://patchwork.freedesktop.org/api/1.0/series/21927/revisions/1/mbox/

Test gem_exec_fence:
        Subgroup nb-await-default:
                pass       -> INCOMPLETE (fi-hsw-4770)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 462s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 458s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 582s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 538s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 596s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 512s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 511s
fi-hsw-4770      total:49   pass:42   dwarn:0   dfail:0   fail:0   skip:6   time: 0s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 427s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 436s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 518s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 493s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 489s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 485s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 605s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 486s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 522s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 457s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 546s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 417s

71f45aec6fccfe64434836abbea0a10cb3b71755 drm-tip: 2017y-03m-27d-08h-58m-18s UTC integration manifest
b79182e drm/i915/dp: reduce link M/N parameters

== Logs ==

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

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

* Re: [PATCH] drm/i915/dp: reduce link M/N parameters
  2017-03-27 11:33 [PATCH] drm/i915/dp: reduce link M/N parameters Jani Nikula
  2017-03-27 12:04 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2017-03-27 13:01 ` Daniel Vetter
  2017-03-27 15:22   ` Jani Nikula
  2017-03-27 18:49 ` Pandiyan, Dhinakaran
  2 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2017-03-27 13:01 UTC (permalink / raw)
  To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx

On Mon, Mar 27, 2017 at 02:33:25PM +0300, Jani Nikula wrote:
> Several major vendor USB-C->HDMI converters, in particular the DA200,
> fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
> 0x80000.
> 
> The link M and N depend on the pixel clock and link clock ratio. 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. 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.
> 
> The DP spec even mentions sources making choices that "allow for static
> and relatively small Mvid and Nvid values", thus reducing the link M/N
> regardless of the sink in question seems justified.
> 
> Everything here is based on the work and information gathered by Clint
> Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
> the parameters regardless of lane count, link rate, or sink.
> 
> Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
> Tested-by: Mads <mads@ab3.no>
> Tested-by: PJ <foobar@pjmodos.net>
> Tested-by: François Guerraz <kubrick@fgv6.net>
> Tested-by: Lev Popov <leo@nabam.net>
> Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> This is cc: stable material, but due to the slight risk of regressions
> (there's always the risk, however small, when you change parameters that
> affect all sinks) I'd prefer letting this simmer for a while, and asking
> for an explicit stable backport afterwards.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> 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);

Can't we push this into reduce_m_n_ratio?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/dp: reduce link M/N parameters
  2017-03-27 13:01 ` [PATCH] " Daniel Vetter
@ 2017-03-27 15:22   ` Jani Nikula
  2017-03-27 15:49     ` Clint Taylor
  0 siblings, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2017-03-27 15:22 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx

On Mon, 27 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Mar 27, 2017 at 02:33:25PM +0300, Jani Nikula wrote:
>> Several major vendor USB-C->HDMI converters, in particular the DA200,
>> fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
>> 0x80000.
>> 
>> The link M and N depend on the pixel clock and link clock ratio. 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. 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.
>> 
>> The DP spec even mentions sources making choices that "allow for static
>> and relatively small Mvid and Nvid values", thus reducing the link M/N
>> regardless of the sink in question seems justified.
>> 
>> Everything here is based on the work and information gathered by Clint
>> Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
>> the parameters regardless of lane count, link rate, or sink.
>> 
>> Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
>> Tested-by: Mads <mads@ab3.no>
>> Tested-by: PJ <foobar@pjmodos.net>
>> Tested-by: François Guerraz <kubrick@fgv6.net>
>> Tested-by: Lev Popov <leo@nabam.net>
>> Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>> 
>> This is cc: stable material, but due to the slight risk of regressions
>> (there's always the risk, however small, when you change parameters that
>> affect all sinks) I'd prefer letting this simmer for a while, and asking
>> for an explicit stable backport afterwards.
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> 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);
>
> Can't we push this into reduce_m_n_ratio?

After *ret_m = div_u64((uint64_t) m * *ret_n, n); it's not guaranteed
that we could shift at all. The reduction needs to be done on the
original m, n being passed in.

BR,
Jani.


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

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

* Re: [PATCH] drm/i915/dp: reduce link M/N parameters
  2017-03-27 15:22   ` Jani Nikula
@ 2017-03-27 15:49     ` Clint Taylor
  2017-03-28 15:27       ` Jani Nikula
  0 siblings, 1 reply; 9+ messages in thread
From: Clint Taylor @ 2017-03-27 15:49 UTC (permalink / raw)
  To: Jani Nikula, Daniel Vetter; +Cc: Daniel Vetter, intel-gfx

On 03/27/2017 08:22 AM, Jani Nikula wrote:
> On Mon, 27 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Mar 27, 2017 at 02:33:25PM +0300, Jani Nikula wrote:
>>> Several major vendor USB-C->HDMI converters, in particular the DA200,
>>> fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
>>> 0x80000.
>>>
>>> The link M and N depend on the pixel clock and link clock ratio. 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. 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.
>>>
>>> The DP spec even mentions sources making choices that "allow for static
>>> and relatively small Mvid and Nvid values", thus reducing the link M/N
>>> regardless of the sink in question seems justified.
>>>
>>> Everything here is based on the work and information gathered by Clint
>>> Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
>>> the parameters regardless of lane count, link rate, or sink.
>>>
>>> Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
>>> Tested-by: Mads <mads@ab3.no>
>>> Tested-by: PJ <foobar@pjmodos.net>
>>> Tested-by: François Guerraz <kubrick@fgv6.net>
>>> Tested-by: Lev Popov <leo@nabam.net>
>>> Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>
>>> ---
>>>
>>> This is cc: stable material, but due to the slight risk of regressions
>>> (there's always the risk, however small, when you change parameters that
>>> affect all sinks) I'd prefer letting this simmer for a while, and asking
>>> for an explicit stable backport afterwards.
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> 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);
>>
>> Can't we push this into reduce_m_n_ratio?
>
> After *ret_m = div_u64((uint64_t) m * *ret_n, n); it's not guaranteed
> that we could shift at all. The reduction needs to be done on the
> original m, n being passed in.
>

Tested-by: Clint Taylor <clinton.a.taylor@intel.com>
Reviewed-by: Clint Taylor <clinton.a.taylor@intel.com>

> BR,
> Jani.
>
>

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

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

* Re: [PATCH] drm/i915/dp: reduce link M/N parameters
  2017-03-27 11:33 [PATCH] drm/i915/dp: reduce link M/N parameters Jani Nikula
  2017-03-27 12:04 ` ✗ Fi.CI.BAT: failure for " Patchwork
  2017-03-27 13:01 ` [PATCH] " Daniel Vetter
@ 2017-03-27 18:49 ` Pandiyan, Dhinakaran
  2017-03-27 19:31   ` Jani Nikula
  2 siblings, 1 reply; 9+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-03-27 18:49 UTC (permalink / raw)
  To: Nikula, Jani; +Cc: daniel.vetter, intel-gfx

On Mon, 2017-03-27 at 14:33 +0300, Jani Nikula wrote:
> Several major vendor USB-C->HDMI converters, in particular the DA200,
> fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
> 0x80000.
> 
> The link M and N depend on the pixel clock and link clock ratio. 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. Just one bit shift right would
> be enough to bring even the DP 1.4 810000 kHz link clock under 0x80000
> link N. 

I don't understand this part, the right shift is applied to 'n' and not
link_clock. For, link_clock=810000kHz, lane=1, n is 62E080h (810000*1*8)
and right-shifting this by 1 does bring it under 80000h. Can you please
clarify this?

The patch itself looks right and is well within the Spec.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> 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.
> 
> The DP spec even mentions sources making choices that "allow for static
> and relatively small Mvid and Nvid values", thus reducing the link M/N
> regardless of the sink in question seems justified.
> 
> Everything here is based on the work and information gathered by Clint
> Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
> the parameters regardless of lane count, link rate, or sink.
> 
> Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
> Tested-by: Mads <mads@ab3.no>
> Tested-by: PJ <foobar@pjmodos.net>
> Tested-by: François Guerraz <kubrick@fgv6.net>
> Tested-by: Lev Popov <leo@nabam.net>
> Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> This is cc: stable material, but due to the slight risk of regressions
> (there's always the risk, however small, when you change parameters that
> affect all sinks) I'd prefer letting this simmer for a while, and asking
> for an explicit stable backport afterwards.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> 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);

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

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

* Re: [PATCH] drm/i915/dp: reduce link M/N parameters
  2017-03-27 18:49 ` Pandiyan, Dhinakaran
@ 2017-03-27 19:31   ` Jani Nikula
  2017-03-27 20:21     ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2017-03-27 19:31 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: daniel.vetter, intel-gfx

On Mon, 27 Mar 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
> On Mon, 2017-03-27 at 14:33 +0300, Jani Nikula wrote:
>> Several major vendor USB-C->HDMI converters, in particular the DA200,
>> fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
>> 0x80000.
>> 
>> The link M and N depend on the pixel clock and link clock ratio. 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. Just one bit shift right would
>> be enough to bring even the DP 1.4 810000 kHz link clock under 0x80000
>> link N. 
>
> I don't understand this part, the right shift is applied to 'n' and not
> link_clock. For, link_clock=810000kHz, lane=1, n is 62E080h (810000*1*8)
> and right-shifting this by 1 does bring it under 80000h. Can you please
> clarify this?

Only *link* N is relevant here, not *data* N. The link M/N =
pixel_clock/link_clock, and bpp and lane count etc. are irrelevant.

BR,
Jani.


>
> The patch itself looks right and is well within the Spec.
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
>
>> 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.
>> 
>> The DP spec even mentions sources making choices that "allow for static
>> and relatively small Mvid and Nvid values", thus reducing the link M/N
>> regardless of the sink in question seems justified.
>> 
>> Everything here is based on the work and information gathered by Clint
>> Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
>> the parameters regardless of lane count, link rate, or sink.
>> 
>> Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
>> Tested-by: Mads <mads@ab3.no>
>> Tested-by: PJ <foobar@pjmodos.net>
>> Tested-by: François Guerraz <kubrick@fgv6.net>
>> Tested-by: Lev Popov <leo@nabam.net>
>> Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>> 
>> This is cc: stable material, but due to the slight risk of regressions
>> (there's always the risk, however small, when you change parameters that
>> affect all sinks) I'd prefer letting this simmer for a while, and asking
>> for an explicit stable backport afterwards.
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> 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);
>

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

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

* Re: [PATCH] drm/i915/dp: reduce link M/N parameters
  2017-03-27 19:31   ` Jani Nikula
@ 2017-03-27 20:21     ` Pandiyan, Dhinakaran
  0 siblings, 0 replies; 9+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-03-27 20:21 UTC (permalink / raw)
  To: Nikula, Jani; +Cc: daniel.vetter, intel-gfx

On Mon, 2017-03-27 at 22:31 +0300, Jani Nikula wrote:
> On Mon, 27 Mar 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
> > On Mon, 2017-03-27 at 14:33 +0300, Jani Nikula wrote:
> >> Several major vendor USB-C->HDMI converters, in particular the DA200,
> >> fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
> >> 0x80000.
> >> 
> >> The link M and N depend on the pixel clock and link clock ratio. 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. Just one bit shift right would
> >> be enough to bring even the DP 1.4 810000 kHz link clock under 0x80000
> >> link N. 
> >
> > I don't understand this part, the right shift is applied to 'n' and not
> > link_clock. For, link_clock=810000kHz, lane=1, n is 62E080h (810000*1*8)
> > and right-shifting this by 1 does bring it under 80000h. Can you please
> > clarify this?
> 
> Only *link* N is relevant here, not *data* N. The link M/N =
> pixel_clock/link_clock, and bpp and lane count etc. are irrelevant.
> 
> BR,
> Jani.
> 
> 
I must have been blind to not see that, thanks for clarifying.
-DK
 
> >
> > The patch itself looks right and is well within the Spec.
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >
> >
> >> 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.
> >> 
> >> The DP spec even mentions sources making choices that "allow for static
> >> and relatively small Mvid and Nvid values", thus reducing the link M/N
> >> regardless of the sink in question seems justified.
> >> 
> >> Everything here is based on the work and information gathered by Clint
> >> Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
> >> the parameters regardless of lane count, link rate, or sink.
> >> 
> >> Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
> >> Tested-by: Mads <mads@ab3.no>
> >> Tested-by: PJ <foobar@pjmodos.net>
> >> Tested-by: François Guerraz <kubrick@fgv6.net>
> >> Tested-by: Lev Popov <leo@nabam.net>
> >> Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
> >> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> 
> >> ---
> >> 
> >> This is cc: stable material, but due to the slight risk of regressions
> >> (there's always the risk, however small, when you change parameters that
> >> affect all sinks) I'd prefer letting this simmer for a while, and asking
> >> for an explicit stable backport afterwards.
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >> 
> >> 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);
> >
> 

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

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

* Re: [PATCH] drm/i915/dp: reduce link M/N parameters
  2017-03-27 15:49     ` Clint Taylor
@ 2017-03-28 15:27       ` Jani Nikula
  0 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2017-03-28 15:27 UTC (permalink / raw)
  To: Clint Taylor, Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, Dhinakaran Pandiyan

On Mon, 27 Mar 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> On 03/27/2017 08:22 AM, Jani Nikula wrote:
>> On Mon, 27 Mar 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Mon, Mar 27, 2017 at 02:33:25PM +0300, Jani Nikula wrote:
>>>> Several major vendor USB-C->HDMI converters, in particular the DA200,
>>>> fail to recover a 5.4 GHz 1 lane signal if the link N is greater than
>>>> 0x80000.
>>>>
>>>> The link M and N depend on the pixel clock and link clock ratio. 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. 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.
>>>>
>>>> The DP spec even mentions sources making choices that "allow for static
>>>> and relatively small Mvid and Nvid values", thus reducing the link M/N
>>>> regardless of the sink in question seems justified.
>>>>
>>>> Everything here is based on the work and information gathered by Clint
>>>> Taylor <clinton.a.taylor@intel.com>. This is just an iteration to reduce
>>>> the parameters regardless of lane count, link rate, or sink.
>>>>
>>>> Reference: http://patchwork.freedesktop.org/patch/msgid/1490225256-11667-1-git-send-email-clinton.a.taylor@intel.com
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93578
>>>> Tested-by: Mads <mads@ab3.no>
>>>> Tested-by: PJ <foobar@pjmodos.net>
>>>> Tested-by: François Guerraz <kubrick@fgv6.net>
>>>> Tested-by: Lev Popov <leo@nabam.net>
>>>> Tested-by: Igor Krivenko <igor.s.krivenko@gmail.com>
>>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>>>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>
>>>> ---
>>>>
>>>> This is cc: stable material, but due to the slight risk of regressions
>>>> (there's always the risk, however small, when you change parameters that
>>>> affect all sinks) I'd prefer letting this simmer for a while, and asking
>>>> for an explicit stable backport afterwards.
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> 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);
>>>
>>> Can't we push this into reduce_m_n_ratio?
>>
>> After *ret_m = div_u64((uint64_t) m * *ret_n, n); it's not guaranteed
>> that we could shift at all. The reduction needs to be done on the
>> original m, n being passed in.
>>
>
> Tested-by: Clint Taylor <clinton.a.taylor@intel.com>
> Reviewed-by: Clint Taylor <clinton.a.taylor@intel.com>

Pushed to drm-intel-next-queued, thanks Clint for your work in getting
at the roots of the problem, and everyone for all the review and
testing!

BR,
Jani.


>
>> BR,
>> Jani.
>>
>>
>

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

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

end of thread, other threads:[~2017-03-28 15:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-27 11:33 [PATCH] drm/i915/dp: reduce link M/N parameters Jani Nikula
2017-03-27 12:04 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-03-27 13:01 ` [PATCH] " Daniel Vetter
2017-03-27 15:22   ` Jani Nikula
2017-03-27 15:49     ` Clint Taylor
2017-03-28 15:27       ` Jani Nikula
2017-03-27 18:49 ` Pandiyan, Dhinakaran
2017-03-27 19:31   ` Jani Nikula
2017-03-27 20:21     ` Pandiyan, Dhinakaran

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.