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

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

On 12/24/2025 12:15 AM, Imre Deak wrote:
> On Tue, Dec 23, 2025 at 08:38:26PM +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.
>> v3:
>>   - Use mul_u32_u32() for the bubble-adjusted pixel rate to avoid 64x64
>>     multiplication and drop redundant casts in DIV_ROUND_UP_ULL(). (Imre)
>>
>> Bspec:68912
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vdsc.c | 35 ++++++++++++++++++++---
>>   1 file changed, 31 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..5493082f30a7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -1050,15 +1050,40 @@ 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(struct intel_display *display,
>> +					      int pixel_rate, int htotal,
>> +					      int dsc_horizontal_slices)
>> +{
>> +	int dsc_slice_bubbles;
>> +	u64 num;
>> +
>> +	if (drm_WARN_ON(display->drm, !htotal))
>> +		return pixel_rate;
>> +
>> +	dsc_slice_bubbles = 14 * dsc_horizontal_slices;
>> +	num = mul_u32_u32(pixel_rate, (htotal + dsc_slice_bubbles));
>> +
>> +	return DIV_ROUND_UP_ULL(num, htotal);
>> +}
>> +
>>   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 htotal = crtc_state->hw.adjusted_mode.crtc_htotal;
>> +	int dsc_slices = crtc_state->dsc.slice_count;
>> +	int pixel_rate;
>>   	int min_cdclk;
>>   
>>   	if (!crtc_state->dsc.compression_enable)
>>   		return 0;
>>   
>> +	pixel_rate = intel_dsc_get_pixel_rate_with_dsc_bubbles(display,
>> +							       crtc_state->pixel_rate,
>> +							       htotal,
>> +							       dsc_slices);
>> +
>>   	/*
>>   	 * When we decide to use only one VDSC engine, since
>>   	 * each VDSC operates with 1 ppc throughput, pixel clock
>> @@ -1066,7 +1091,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 +1109,11 @@ 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 =
>> +			intel_dsc_get_pixel_rate_with_dsc_bubbles(display, pixel_clock,
>> +								  htotal, dsc_slices);
>> +		int min_cdclk_bj = (fxp_q4_to_int_roundup(crtc_state->dsc.compressed_bpp_x16) *
>> +				   adjusted_pixel_rate) / (2 * bigjoiner_interface_bits);
> The patch looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
> Fwiw: I suppose when calculating min_cdclk_bj instead of
> fxp_q4_to_int_roundup() the x16 adjustment could be in the divider for
> more precision and the division should round up not down. However neither
> of these are related to your changes, they can be revised later.

Agreed. I guess when this change started, we were not having the 
granularity of 1/16.

I will add the suggested change with my other DSC cdclk series.

Thanks for the review!


Regards,

Ankit

>
>>   
>>   		min_cdclk = max(min_cdclk, min_cdclk_bj);
>>   	}
>> -- 
>> 2.45.2
>>