[Intel-gfx] [PATCH 1/2] drm/dp/i915: Fix DP link rate math

Fri Nov 11 03:03:32 UTC 2016

On Thu, 2016-11-10 at 15:55 -0800, Manasi Navare wrote:
> On Wed, Nov 09, 2016 at 09:32:29PM -0800, Dhinakaran Pandiyan wrote:
> > We store DP link rates as link clock frequencies in kHz, just like all
> > other clock values. But, DP link rates in the DP Spec are expressed in
> > Gbps/lane, which seems to have led to some confusion.
> > 
> > E.g., for HBR2
> > Max. data rate = 5.4 Gbps/lane x 4 lane x 8/10 x 1/8 = 2160000 kBps
> > where, 8/10 is for channel encoding and 1/8 is for bit to Byte conversion
> > 
> > Using link clock frequency, like we do
> > Max. data rate = 540000 kHz * 4 lanes = 2160000 kSymbols/s
> > Because, each symbol has 8 bit of data, this is 2160000 kBps
> > and there is no need to account for channel encoding here.
> > 
> > But, currently we do 540000 kHz * 4 lanes * (8/10) = 1728000 kBps
> > 
> > Similarly, while computing the required link bandwidth for a mode,
> > there is a mysterious 1/10 term.
> > This should simply be pixel_clock kHz * bpp * 1/8  to give the final
> > result in kBps
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++-------------------
> >  1 file changed, 9 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 8f313c1..7a9e122 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -161,33 +161,23 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
> >  	return min(source_max, sink_max);
> >  }
> >  
> > -/*
> > - * The units on the numbers in the next two are... bizarre.  Examples will
> > - * make it clearer; this one parallels an example in the eDP spec.
> > - *
> > - * intel_dp_max_data_rate for one lane of 2.7GHz evaluates as:
> > - *
> > - *     270000 * 1 * 8 / 10 == 216000
> > - *
> > - * The actual data capacity of that configuration is 2.16Gbit/s, so the
> > - * units are decakilobits.  ->clock in a drm_display_mode is in kilohertz -
> > - * or equivalently, kilopixels per second - so for 1680x1050R it'd be
> > - * 119000.  At 18bpp that's 2142000 kilobits per second.
> > - *
> > - * Thus the strange-looking division by 10 in intel_dp_link_required, to
> > - * get the result in decakilobits instead of kilobits.
> > - */
> > -
> >  static int
> >  intel_dp_link_required(int pixel_clock, int bpp)
> >  {
> > -	return (pixel_clock * bpp + 9) / 10;
> > +	/* pixel_clock is in kHz, divide bpp by 8 to return the value in kBps*/
> > +	return (pixel_clock * bpp + 7) / 8;
> >  }
> >  
> >  static int
> >  intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> >  {
> > -	return (max_link_clock * max_lanes * 8) / 10;
> > +	/* max_link_clock is the link symbol clock (LS_Clk) in kHz and not the
> > +	 * link rate that is generally expressed in Gbps. Since, 8 bits data is
> > +	 * transmitted every LS_Clk per lane, there is no need to account for
> > +	 * the channel encoding that is done in the PHY layer here.
> > +	 */
> > +
> 
> Max Link is the max link rate for the actual physical link of the DP cable.

The units are quite a mess, as the existing comments indicate.
For e.g., see
1. intel_dp_link_required()
""* The units on the numbers in the next two are... bizarre. ..."
2. intel_read_sink_rates()
" /* Value read is in kHz while drm clock is saved in deca-kHz */"

This has led to confusing /10's to just match the units to deca Hz and
deca kHz.

I believe, the confusion is due to the fact that,
1. eDP DPCD registers have link rates that have to be multiplied with
200kHz to get the final value in kHz. This is NOT the link symbol clock.
	e.g., DPCD SUPPORTED_LINK_RATES
	for HBR2, the value will be 27000 or 6978h (from DP Spec)

	To get the bit rate capacity
	= Link rate * Num. lanes * 8/10
	= 27000*200kHz * 4 * 8/10 = 17280000 kbits per second or 
	= 2160000 kBytes per second

	But, we arbitrarily divide by 10 in intel_edp_init_dpcd()
	"/* Value read is in kHz while drm clock is saved in deca-kHz */
 	intel_dp->sink_rates[i] = (val * 200) / 10; "

2. Let's look at source rates.

static const int bxt_rates[] = { 162000, 216000, 243000, 270000,
                                  324000, 432000, 540000 };
static const int skl_rates[] = { 162000, 216000, 270000,
                                  324000, 432000, 540000 };
static const int default_rates[] = { 162000, 270000, 540000 };

These cannot be link rates in Gbps nor link rate expressed in kHz.
If it is the link rate, HBR2 should be
5400000 Gbps or 54000000 kHz
But, the value is 540000. That's because, this is the Link Symbol Clock
(LS_Clk) in kHz

If you look at 

commit a4fc5ed69817c73e32571ad7837bb707f9890009
Author: Keith Packard <keithp at keithp.com>
Date:   Tue Apr 7 16:16:42 2009 -0700

    drm/i915: Add Display Port support

you will see that the units all make sense, but somewhere in between we
have messed up the conversions.

To check if a mode is valid, the comparison should be between,
mode_clock(kHz)*bpp/8  and link_clock(kHz)*lanes , the units being kBps 
since 1 symbol corresponds to 1 byte of data.

-DK

> So eventually PHY layer is going to encode the bits and generate 10 bits
> for every 8 bits, so the code rate will be 8/10 and the useful net rate (sending
> useful bits) will be link_rate * code rate = link_rate * 8/10. 
> So the max available link rate at the link layer should be this useful net rate
> and so IMHO we should consider this channel encoding into account.
> 
> Manasi
> > +	return (max_link_clock * max_lanes);
> >  }
> >  
> >  static int
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx