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/25/2023 3:40 PM, Lisovskiy, Stanislav wrote:
> On Tue, Jul 25, 2023 at 11:22:52AM +0530, Nautiyal, Ankit K wrote:
>> 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?
> Those are different cases:
>
>
> 1) When we are not using DSC, we are always processing
> 2 pixels per CDCLK, starting from gen 10. It is reflected in both intel_pixel_rate_to_cdclk
> and icl_plane_min_cdclk(which is a bit of a tautology I agree, but anyways we always take
> all limitations and use max(worst case) of them)
>
> 2) When we are using DSC. In that case we could use 1 or VDSC engines, which would set PPC to
> 1 or 2 correspondently. So whenever we happen to use DSC that condition will take max of
> the CDCLK obtained by other requirements and that formula.
> However in non-compressed case when there is no DSC, we should even be insterested in querying
> how many VDSC instances we have, amount of pixels processed per CDCLK isn't related to this in
> that case.
>
> Stan

Alright then I'll drop this change. The existing checks seem sufficient 
to take care of the cdclk for DSC case.

Regards,

Ankit

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