Re: [PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol cycles

[Imre Deak <imre.deak@intel.com>]

On Tue, Apr 22, 2025 at 08:03:52AM +0300, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun R
> > Murthy
> > Sent: Thursday, April 17, 2025 4:22 PM
> > To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> > xe@lists.freedesktop.org
> > Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Deak, Imre
> > <imre.deak@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>
> > Subject: [PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol
> > cycles
> > 
> > Unify the function to calculate the link symbol cycles for both dsc and non-dsc
> > case and export the function so that it can be used in the respective platform
> > display drivers for other calculations.
> > 
> > v2: unify the fn for both dsc and non-dsc case (Imre)
> > v3: rename drm_dp_link_symbol_cycles to drm_dp_link_data_symbol_cycles
> >     retain slice_eoc_cycles as is (Imre)
> > 
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_helper.c | 53 +++++++++++++++++--------------
> > --
> >  include/drm/display/drm_dp_helper.h     |  2 ++
> >  2 files changed, 29 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> > b/drivers/gpu/drm/display/drm_dp_helper.c
> > index
> > 57828f2b7b5a0582ca4a6f2a9be2d5909fe8ad24..5ce8ccc3310fb71b39ea5f74c4
> > 022474c180f727 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -4392,26 +4392,33 @@ EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> > 
> >  #endif
> > 
> > -/* See DP Standard v2.1 2.6.4.4.1.1, 2.8.4.4, 2.8.7 */ -static int
> > drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> > -				     int symbol_size, bool is_mst)
> > -{
> > -	int cycles = DIV_ROUND_UP(pixels * bpp_x16, 16 * symbol_size *
> > lane_count);
> > -	int align = is_mst ? 4 / lane_count : 1;
> > -
> > -	return ALIGN(cycles, align);
> > -}
> > -
> > -static int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int
> > slice_count,
> > -					 int bpp_x16, int symbol_size, bool
> > is_mst)
> > -{
> > -	int slice_pixels = DIV_ROUND_UP(pixels, slice_count);
> > -	int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count,
> > slice_pixels,
> > -							  bpp_x16,
> > symbol_size, is_mst);
> > +/**
> > + * drm_dp_link_data_symbol_cycles - calculate the link symbol count
> > + * @lane_coount: DP link lane count
>  
> Typo "lane_count"
> 
> > + * @pixels: horizontal active pixels
> > + * @bpp_x16: bits per pixel in .4 binary fixed format
> > + * @symbol_size: DP symbol size
> > + * @is_mst: is mst or sst
> > + * @slice_count: number of slices
> > + *
> > + * Calculate the link symbol cycles for both dsc and non dsc case and
> > + * return the count.
> 
> Lets add the DP spec to be referred seems like it was missed
> 
> > + */
> > +int drm_dp_link_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> > +				   int symbol_size, bool is_mst, int slice_count)
> > {
> > +	int slice_pixels = slice_count ? DIV_ROUND_UP(pixels, slice_count) :
> > +					 pixels;
> > +	int cycles = DIV_ROUND_UP(slice_pixels * bpp_x16,
> > +				  (6 * symbol_size * lane_count));
> 
> Shouldn't this be 16
> Also one thing I see which was there previously to while calculating is we ignore the two ceils 
> Inside the function and merge it into a single div_round_up which may bring as slight variation in calculation
> For example for non dsc case
> Spec says
> HACT_LL_SYM_CYC_CNT
> = CEIL(CEIL(HACT_WIDTH / 4) × PIX_BPP / SYMBOL_SIZE)
> HACT_ML_SYM_CYC_CNT
> = HACT_LL_SYM_CYC_CNT × 4 / PHY_LANE_CNT
> 
> But we do
> DIV_ROUND_UP(slice_pixels * bpp_x16,	  (6 * symbol_size * lane_count));

and we align this to 4 / lane_count. So the code does what the DP
standard describes.

> Which translates to 
> CEIL( (HACT_WIDTH* BPP*4)/(16 *SYMBOL_SIZE *LANECOUNT))
> 
> Which does not seem to match the calculation exactly as what was said in the spec
> Lets have an intermediate ll_symbol_cycle variable too should make the calculations
> More clearer and precise according to me.
>
> Also for dsc case lets have chunk size instead of reusing slice pixels.

Let's not add now other changes besides the original request of one
thing, that is to make drm_dp_link_symbol_cycles() handle both the
DSC and non-DSC cases without changing the calculation, see [1]. That
corresponds to the diff I added in [2].

[1] https://lore.kernel.org/all/Z_UvdB05S0sPbs6l@ideak-desk.fi.intel.com
[2] https://lore.kernel.org/all/aAdiU3K5EV6Oq81a@ideak-desk.fi.intel.com

> Regards,
> Suraj Kandpal
> 
> > +	int slice_data_cycles = ALIGN(cycles, is_mst ? (4 / lane_count) : 1);
> >  	int slice_eoc_cycles = is_mst ? 4 / lane_count : 1;
> > 
> > -	return slice_count * (slice_data_cycles + slice_eoc_cycles);
> > +	return slice_count ? (slice_count *
> > +			      (slice_data_cycles + slice_eoc_cycles)) :
> > +			      slice_data_cycles;
> >  }
> > +EXPORT_SYMBOL(drm_dp_link_data_symbol_cycles);
> > 
> >  /**
> >   * drm_dp_bw_overhead - Calculate the BW overhead of a DP link stream @@
> > -4486,15 +4493,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
> >  	WARN_ON((flags & DRM_DP_BW_OVERHEAD_UHBR) &&
> >  		(flags & DRM_DP_BW_OVERHEAD_FEC));
> > 
> > -	if (flags & DRM_DP_BW_OVERHEAD_DSC)
> > -		symbol_cycles = drm_dp_link_dsc_symbol_cycles(lane_count,
> > hactive,
> > -							      dsc_slice_count,
> > -							      bpp_x16,
> > symbol_size,
> > -							      is_mst);
> > -	else
> > -		symbol_cycles = drm_dp_link_symbol_cycles(lane_count,
> > hactive,
> > -							  bpp_x16,
> > symbol_size,
> > -							  is_mst);
> > +	symbol_cycles = drm_dp_link_data_symbol_cycles(lane_count,
> > hactive,
> > +						       bpp_x16, symbol_size,
> > +						       is_mst, dsc_slice_count);
> > 
> >  	return DIV_ROUND_UP_ULL(mul_u32_u32(symbol_cycles *
> > symbol_size * lane_count,
> >  					    overhead * 16),
> > diff --git a/include/drm/display/drm_dp_helper.h
> > b/include/drm/display/drm_dp_helper.h
> > index
> > d9614e2c89397536f44bb7258e894628ae1dccc9..98bbbe98e5bc0ce0f9cdf513b
> > 2c5ea90bb5caffb 100644
> > --- a/include/drm/display/drm_dp_helper.h
> > +++ b/include/drm/display/drm_dp_helper.h
> > @@ -971,5 +971,7 @@ int drm_dp_bw_channel_coding_efficiency(bool
> > is_uhbr);  int drm_dp_max_dprx_data_rate(int max_link_rate, int max_lanes);
> > 
> >  ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, struct
> > dp_sdp *sdp);
> > +int drm_dp_link_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> > +				   int symbol_size, bool is_mst, int slice_count);
> > 
> >  #endif /* _DRM_DP_HELPER_H_ */
> > 
> > --
> > 2.25.1
>