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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 12/23/2025 3:16 PM, Imre Deak wrote:
> 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).

Sorry I got confused between dsc.slice_count and dsc.config.slice_count 
(we do not fill this).

I was also assuming we already got 
intel_dsc_line_slice_count(dsc.slice_config) merged, but that is still 
to be reviewed.

So yes you are right we can use dsc.slice_count, which will be later 
replaced by the intel_dsc_line_slice_count() AFAIU.

Thanks for clearing this up. I will make the suggested change.


Regards,

Ankit

>
>> 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
>>>>