| Message ID | 1490614405-23337-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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. > >
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);
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); >
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); > > >
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. >> >> >
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);