[PATCH 06/19] drm/i915/display: Account for DSC not split case while computing cdclk
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Tue Jul 25 11:22:56 UTC 2023
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 at 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
>>>>
More information about the dri-devel
mailing list