Re: [PATCH] drm/i915/vdsc: Account for DSC slice overhead in intel_vdsc_min_cdclk()

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

On Tue, Dec 23, 2025 at 11:05:59AM +0530, Nautiyal, Ankit K wrote:
> 
> On 12/22/2025 6:26 PM, Imre Deak wrote:
> > On Mon, Dec 22, 2025 at 10:14:43AM +0530, Ankit Nautiyal wrote:
> > > When DSC is enabled on a pipe, the pipe pixel rate input to the
> > > CDCLK frequency and pipe joining calculation needs an adjustment to
> > > account for compression overhead "bubbles" added at each horizontal
> > > slice boundary.
> > > 
> > > Account for this overhead while computing min cdclk required for DSC.
> > > 
> > > v2: Get rid of the scaling factor and return unchanged pixel-rate
> > > instead of 0.
> > > 
> > > Bspec:68912
> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_vdsc.c | 45 +++++++++++++++++++++--
> > >   1 file changed, 41 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > index ad5fe841e4b3..b91cd009be9d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > @@ -1050,15 +1050,52 @@ void intel_vdsc_state_dump(struct drm_printer *p, int indent,
> > >   	drm_dsc_dump_config(p, indent, &crtc_state->dsc.config);
> > >   }
> > > +static
> > > +int intel_dsc_get_pixel_rate_with_dsc_bubbles(int pixel_rate, int htotal,
> > > +					      int dsc_horizontal_slices)
> > > +{
> > > +	int dsc_slice_bubbles;
> > > +	u64 num;
> > > +
> > > +	if (!htotal)
> > Should this also warn as !slice_width below?
> 
> Yeah can add add warn here..
> 
> > > +		return pixel_rate;
> > > +
> > > +	dsc_slice_bubbles = 14 * dsc_horizontal_slices;
> > > +	num = (u64)pixel_rate * (u64)(htotal + dsc_slice_bubbles);
> > Better to use mul_u32_u32() to avoid the casts and 64-bit x 64-bit
> > multiplication.
> 
> Ok sure will use mul_u32_u32 to avoid casts here.
> 
> > > +
> > > +	return (int)DIV_ROUND_UP_ULL(num, (u64)htotal);
> > Both casts are ensured by the compiler already, so no need for doing
> > them explicitly.
> 
> Got it.
> 
> > > +}
> > > +
> > > +static
> > > +int pixel_rate_with_dsc_bubbles(const struct intel_crtc_state *crtc_state, int pixel_rate)
> > > +{
> > > +	struct intel_display *display = to_intel_display(crtc_state);
> > > +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> > > +	const struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> > > +	int dsc_horizontal_slices;
> > > +
> > > +	if (drm_WARN_ON(display->drm, !vdsc_cfg->slice_width))
> > > +		return pixel_rate;
> > > +
> > > +	dsc_horizontal_slices = vdsc_cfg->pic_width / vdsc_cfg->slice_width;
> > The above looks to be the same as crtc_state->dsc.slice_count, aka the
> > slices per scanline, could you use that instead?
> 
> Apparently we are not filling dsc.slice_count. We are filling
> dsc.slice_width and the pic_width.

dsc.slice_count is computed for the given encoder and dsc.slice_width is
computed based on that (as vdsc->pic_width / crtc_state->dsc.slice_count).

> This parameter seems to be unused, perhaps can be dropped?

It's slice_count what is computed for a particular encoder and the rest
of DSC parameters are only derived from slice_count the same way for all
encoders.

> Regards,
> Ankit
> 
> > > +
> > > +	return intel_dsc_get_pixel_rate_with_dsc_bubbles(pixel_rate,
> > > +							 adjusted_mode->crtc_htotal,
> > > +							 dsc_horizontal_slices);
> > > +}
> > > +
> > >   int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
> > >   {
> > >   	struct intel_display *display = to_intel_display(crtc_state);
> > >   	int num_vdsc_instances = intel_dsc_get_num_vdsc_instances(crtc_state);
> > > +	int pixel_rate;
> > >   	int min_cdclk;
> > >   	if (!crtc_state->dsc.compression_enable)
> > >   		return 0;
> > > +	pixel_rate = pixel_rate_with_dsc_bubbles(crtc_state, crtc_state->pixel_rate);
> > > +
> > >   	/*
> > >   	 * When we decide to use only one VDSC engine, since
> > >   	 * each VDSC operates with 1 ppc throughput, pixel clock
> > > @@ -1066,7 +1103,7 @@ int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
> > >   	 * If there 2 VDSC engines, then pixel clock can't be higher than
> > >   	 * VDSC clock(cdclk) * 2 and so on.
> > >   	 */
> > > -	min_cdclk = DIV_ROUND_UP(crtc_state->pixel_rate, num_vdsc_instances);
> > > +	min_cdclk = DIV_ROUND_UP(pixel_rate, num_vdsc_instances);
> > >   	if (crtc_state->joiner_pipes) {
> > >   		int pixel_clock = intel_dp_mode_to_fec_clock(crtc_state->hw.adjusted_mode.clock);
> > > @@ -1084,9 +1121,9 @@ int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
> > >   		 * => CDCLK >= compressed_bpp * Pixel clock  / 2 * Bigjoiner Interface bits
> > >   		 */
> > >   		int bigjoiner_interface_bits = DISPLAY_VER(display) >= 14 ? 36 : 24;
> > > -		int min_cdclk_bj =
> > > -			(fxp_q4_to_int_roundup(crtc_state->dsc.compressed_bpp_x16) *
> > > -			 pixel_clock) / (2 * bigjoiner_interface_bits);
> > > +		int adjusted_pixel_rate = pixel_rate_with_dsc_bubbles(crtc_state, pixel_clock);
> > > +		int min_cdclk_bj = (fxp_q4_to_int_roundup(crtc_state->dsc.compressed_bpp_x16) *
> > > +				   adjusted_pixel_rate) / (2 * bigjoiner_interface_bits);
> > >   		min_cdclk = max(min_cdclk, min_cdclk_bj);
> > >   	}
> > > -- 
> > > 2.45.2
> > >