[PATCH 01/19] drm/dp: Add drm_dp_max_dprx_data_rate()

[Shankar, Uma]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Friday, January 26, 2024 6:58 PM
> To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Subject: Re: [PATCH 01/19] drm/dp: Add drm_dp_max_dprx_data_rate()
> 
> On Fri, Jan 26, 2024 at 01:36:02PM +0200, Ville Syrjälä wrote:
> > On Tue, Jan 23, 2024 at 12:28:32PM +0200, Imre Deak wrote:
> > > Copy intel_dp_max_data_rate() to DRM core. It will be needed by a
> > > follow-up DP tunnel patch, checking the maximum rate the DPRX (sink)
> > > supports. Accordingly use the drm_dp_max_dprx_data_rate() name for
> > > clarity. This patchset will also switch calling the new DRM function
> > > in i915 instead of intel_dp_max_data_rate().
> > >
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/display/drm_dp_helper.c | 58
> +++++++++++++++++++++++++
> > >  include/drm/display/drm_dp_helper.h     |  2 +
> > >  2 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> > > b/drivers/gpu/drm/display/drm_dp_helper.c
> > > index b1ca3a1100dab..24911243d4d3a 100644
> > > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > > @@ -4058,3 +4058,61 @@ int drm_dp_bw_channel_coding_efficiency(bool
> is_uhbr)
> > >  		return 800000;
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_bw_channel_coding_efficiency);
> > > +
> > > +/*
> > > + * Given a link rate and lanes, get the data bandwidth.
> > > + *
> > > + * Data bandwidth is the actual payload rate, which depends on the
> > > +data
> > > + * bandwidth efficiency and the link rate.
> > > + *
> > > + * For 8b/10b channel encoding, SST and non-FEC, the data bandwidth
> > > +efficiency
> > > + * is 80%. For example, for a 1.62 Gbps link, 1.62*10^9 bps * 0.80
> > > +* (1/8) =
> > > + * 162000 kBps. With 8-bit symbols, we have 162000 kHz symbol
> > > +clock. Just by
> > > + * coincidence, the port clock in kHz matches the data bandwidth in
> > > +kBps, and
> > > + * they equal the link bit rate in Gbps multiplied by 100000. (Note
> > > +that this no
> > > + * longer holds for data bandwidth as soon as FEC or MST is taken
> > > +into account!)
> > > + *
> > > + * For 128b/132b channel encoding, the data bandwidth efficiency is
> > > +96.71%. For
> > > + * example, for a 10 Gbps link, 10*10^9 bps * 0.9671 * (1/8) =
> > > +1208875
> > > + * kBps. With 32-bit symbols, we have 312500 kHz symbol clock. The
> > > +value 1000000
> > > + * does not match the symbol clock, the port clock (not even if you
> > > +think in
> > > + * terms of a byte clock), nor the data bandwidth. It only matches
> > > +the link bit
> > > + * rate in units of 10000 bps.
> > > + *
> > > + * Note that protocol layers above the DPRX link level considered
> > > +here can
> > > + * further limit the maximum data rate. Such layers are the MST
> > > +topology (with
> > > + * limits on the link between the source and first branch device as
> > > +well as on
> > > + * the whole MST path until the DPRX link) and (Thunderbolt) DP
> > > +tunnels -
> > > + * which in turn can encapsulate an MST link with its own limit -
> > > +with each
> > > + * SST or MST encapsulated tunnel sharing the BW of a tunnel group.
> > > + *
> > > + * TODO: Add support for querying the max data rate with the above
> > > +limits as
> > > + * well.
> > > + *
> > > + * Returns the maximum data rate in kBps units.
> > > + */
> > > +int drm_dp_max_dprx_data_rate(int max_link_rate, int max_lanes) {
> > > +	int ch_coding_efficiency =
> > > +
> 	drm_dp_bw_channel_coding_efficiency(drm_dp_is_uhbr_rate(max_link_
> rate));
> > > +	int max_link_rate_kbps = max_link_rate * 10;
> >
> > That x10 value seems rather pointless.
> 
> I suppose the point was to make the units clearer, but it could be clarified instead
> in max_link_rates' documentation, which is missing atm.
> 
> > > +
> > > +	/*
> > > +	 * UHBR rates always use 128b/132b channel encoding, and have
> > > +	 * 97.71% data bandwidth efficiency. Consider max_link_rate the
> > > +	 * link bit rate in units of 10000 bps.
> > > +	 */
> > > +	/*
> > > +	 * Lower than UHBR rates always use 8b/10b channel encoding, and have
> > > +	 * 80% data bandwidth efficiency for SST non-FEC. However, this turns
> > > +	 * out to be a nop by coincidence:
> > > +	 *
> > > +	 *	int max_link_rate_kbps = max_link_rate * 10;
> > > +	 *	max_link_rate_kbps =
> DIV_ROUND_DOWN_ULL(max_link_rate_kbps * 8, 10);
> > > +	 *	max_link_rate = max_link_rate_kbps / 8;
> > > +	 */
> >
> > Not sure why we are repeating the nuts and bolts detils in the
> > comments so much? Doesn't drm_dp_bw_channel_coding_efficiency()
> > explain all this already?
> 
> I simply copied the function, but yes in this context there is duplication, thanks for
> reading through all that. Will consolidate both the above and the bigger comment
> before the function with the existing docs here.

Changes look good to me. With above addressed:
Reviewed-by: Uma Shankar <uma.shankar at intel.com>

> >
> > > +	return DIV_ROUND_DOWN_ULL(mul_u32_u32(max_link_rate_kbps *
> max_lanes,
> > > +					      ch_coding_efficiency),
> > > +				  1000000 * 8);
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_max_dprx_data_rate);
> > > diff --git a/include/drm/display/drm_dp_helper.h
> > > b/include/drm/display/drm_dp_helper.h
> > > index 863b2e7add29e..454ae7517419a 100644
> > > --- a/include/drm/display/drm_dp_helper.h
> > > +++ b/include/drm/display/drm_dp_helper.h
> > > @@ -813,4 +813,6 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
> > >  		       int bpp_x16, unsigned long flags);  int
> > > drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
> > >
> > > +int drm_dp_max_dprx_data_rate(int max_link_rate, int max_lanes);
> > > +
> > >  #endif /* _DRM_DP_HELPER_H_ */
> > > --
> > > 2.39.2
> >
> > --
> > Ville Syrjälä
> > Intel