Re: [Intel-gfx] [PATCH 06/19] drm/i915/display: Account for DSC not split case while computing cdclk

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

On 7/20/2023 2:46 PM, Lisovskiy, Stanislav wrote:
> On Thu, Jul 13, 2023 at 04:03:33PM +0530, Ankit Nautiyal wrote:
>> Currently we assume 2 Pixels Per Clock (PPC) while computing
>> plane cdclk and min_cdlck. In cases where DSC single engine
>> is used the throughput is 1 PPC.
>>
>> So account for the above case, while computing cdclk.
>>
>> v2: Use helper to get the adjusted pixel rate.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_cdclk.c         |  2 +-
>>   drivers/gpu/drm/i915/display/intel_vdsc.c          | 12 ++++++++++++
>>   drivers/gpu/drm/i915/display/intel_vdsc.h          |  2 ++
>>   drivers/gpu/drm/i915/display/skl_universal_plane.c |  4 ++--
>>   4 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> index dcc1f6941b60..701909966545 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> @@ -2508,7 +2508,7 @@ static int intel_pixel_rate_to_cdclk(const struct intel_crtc_state *crtc_state)
>>   	int pixel_rate = crtc_state->pixel_rate;
>>   
>>   	if (DISPLAY_VER(dev_priv) >= 10)
>> -		return DIV_ROUND_UP(pixel_rate, 2);
>> +		return intel_dsc_get_adjusted_pixel_rate(crtc_state, pixel_rate);
>>   	else if (DISPLAY_VER(dev_priv) == 9 ||
>>   		 IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
>>   		return pixel_rate;
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> index 9d76c2756784..bbfdbf06da68 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -1038,3 +1038,15 @@ void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
>>   out:
>>   	intel_display_power_put(dev_priv, power_domain, wakeref);
>>   }
>> +
>> +int intel_dsc_get_adjusted_pixel_rate(const struct intel_crtc_state *crtc_state, int pixel_rate)
>> +{
>> +	/*
>> +	 * If single VDSC engine is used, it uses one pixel per clock
>> +	 * otherwise we use two pixels per clock.
>> +	 */
>> +	if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)
>> +		return pixel_rate;
>> +
>> +	return DIV_ROUND_UP(pixel_rate, 2);
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.h b/drivers/gpu/drm/i915/display/intel_vdsc.h
>> index 2cc41ff08909..3bb4b1980b6b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h
>> @@ -28,4 +28,6 @@ void intel_dsc_dsi_pps_write(struct intel_encoder *encoder,
>>   void intel_dsc_dp_pps_write(struct intel_encoder *encoder,
>>   			    const struct intel_crtc_state *crtc_state);
>>   
>> +int intel_dsc_get_adjusted_pixel_rate(const struct intel_crtc_state *crtc_state, int pixel_rate);
>> +
>>   #endif /* __INTEL_VDSC_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index 6b01a0b68b97..9eeb25ec4be9 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -17,6 +17,7 @@
>>   #include "intel_fb.h"
>>   #include "intel_fbc.h"
>>   #include "intel_psr.h"
>> +#include "intel_vdsc.h"
>>   #include "skl_scaler.h"
>>   #include "skl_universal_plane.h"
>>   #include "skl_watermark.h"
>> @@ -263,8 +264,7 @@ static int icl_plane_min_cdclk(const struct intel_crtc_state *crtc_state,
>>   {
>>   	unsigned int pixel_rate = intel_plane_pixel_rate(crtc_state, plane_state);
>>   
>> -	/* two pixels per clock */
>> -	return DIV_ROUND_UP(pixel_rate, 2);
>> +	return intel_dsc_get_adjusted_pixel_rate(crtc_state, pixel_rate);
> Hi Ankit,
>
> I think the thing what you are taking of is already handled here in intel_cdclk.c:
>
> 	/*
>           * When we decide to use only one VDSC engine, since
>           * each VDSC operates with 1 ppc throughput, pixel clock
>           * cannot be higher than the VDSC clock (cdclk)
>           * If there 2 VDSC engines, then pixel clock can't be higher than
>           * VDSC clock(cdclk) * 2 and so on.
>           */
>          if (crtc_state->dsc.compression_enable) {
>                  int num_vdsc_instances = intel_dsc_get_num_vdsc_instances(crtc_state);
>
>                  min_cdclk = max_t(int, min_cdclk,
>                                    DIV_ROUND_UP(crtc_state->pixel_rate,
>                                                 num_vdsc_instances));
>          }

As far as I understand this condition is coming from the pixel clock 
limitation as an input to the splitter (Bspec: 49259):

Pipe BW check:

Pixel clock < PPC * CDCLK frequency * Number of pipes joined

PPC = 1 or 2 depending on number of DSC engines used within the pipe.

So it implies CDCLK frequency > Pixel clock / (PPC * Number of pipes joined)

num_vdsc_instances is actually giving us (PPC * number of pipes joined).


I completely agree that there will be no effect of the change on the 
min_cdclk that gets computed in any case, whether DSC, 1 engine, 2 
engine,  bigjoiner or no DSC.

Only thing is for the case where 1 DSC engine is used, what 
intel_pixel_rate_to_cdclk return will be different, and its due to the 
fact that pipe is driven with 1PPC.

But as I said, the min_cdclk computed will be same as without patch. So 
we can certainly do away with this change, and I can drop this from the 
patch.


But in case of icl_plane_min_cdclk, currently we are dividing the 
plane_pixel_rate by 2, citing that we use 2 pixel per clock, to get the 
plane_min_cdclk.

Should this not be 1 PPC in case where single DSC engine is used? In 
that case plane_min_cdclk will be double. Should we keep the change only 
for plane_min_cdclk then?


Regards,

Ankit


>
> Also even if something still have to be done here, I think we should preferrably
> deal with anything related to DSC in a single place, to prevent any kind of
> confusion(when those checks are scattered in different places, it is way more easy to forget/not notice something)
>
> I think intel_pixel_rate_to_cdclk isn't supposed to know anything about DSC or any other specifics like audio checks and etc - it is
> just dealing with the "default" uncompressed case.
> Any other additional limitations or checks we apply after those, as there are
> quite many anyway.
>
> Stan
>
>
>>   }
>>   
>>   static void
>> -- 
>> 2.40.1
>>