Re: [PATCH] drm/i915/dp: reduce link M/N parameters

From: Jani Nikula <jani.nikula@intel.com>
To: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915/dp: reduce link M/N parameters
Date: Mon, 27 Mar 2017 22:31:57 +0300	[thread overview]
Message-ID: <877f3a8qj6.fsf@intel.com> (raw)
In-Reply-To: <1490641702.31644.14.camel@dk-H97M-D3H>

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