[Freedreno] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions

[Dmitry Baryshkov]

On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
> > 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar <quic_abhinavk at quicinc.com> пишет:
> >> On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
> >>> On 24/02/2023 21:40, Kuogee Hsieh wrote:
> >>>> Add DSC helper functions based on DSC configuration profiles to produce
> >>>> DSC related runtime parameters through both table look up and runtime
> >>>> calculation to support DSC on DPU.
> >>>>
> >>>> There are 6 different DSC configuration profiles are supported currently.
> >>>> DSC configuration profiles are differiented by 5 keys, DSC version (V1.1),
> >>>> chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
> >>>> bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
> >>>>
> >>>> Only DSC version V1.1 added and V1.2 will be added later.
> >>>
> >>> These helpers should go to drivers/gpu/drm/display/drm_dsc_helper.c
> >>> Also please check that they can be used for i915 or for amdgpu (ideally for both of them).
> >>>
> >>
> >> No, it cannot. So each DSC encoder parameter is calculated based on the HW core which is being used.
> >>
> >> They all get packed to the same DSC structure which is the struct drm_dsc_config but the way the parameters are computed is specific to the HW.
> >>
> >> This DPU file helper still uses the drm_dsc_helper's drm_dsc_compute_rc_parameters() like all other vendors do but the parameters themselves are very HW specific and belong to each vendor's dir.
> >>
> >> This is not unique to MSM.
> >>
> >> Lets take a few other examples:
> >>
> >> AMD: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
> >>
> >> i915: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
> >
> > I checked several values here. Intel driver defines more bpc/bpp combinations, but the ones which are defined in intel_vdsc and in this patch seem to match. If there are major differences there, please point me to the exact case.
> >
> > I remember that AMD driver might have different values.
> >
>
> Some values in the rc_params table do match. But the rc_buf_thresh[] doesnt.

Because later they do:

vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;

>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
>
> Vs
>
> +static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
> +               0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
> +               0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
> +};

I'd prefer to have 896, 1792, etc. here, as those values come from the
standard. As it's done in the Intel driver.

> I dont know the AMD calculation very well to say that moving this to the
> helper is going to help.

Those calculations correspond (more or less) at the first glance to
what intel does for their newer generations. I think that's not our
problem for now.

>
> Also, i think its too risky to change other drivers to use whatever math
> we put in the drm_dsc_helper to compute thr RC params because their code
> might be computing and using this tables differently.
>
> Its too much ownership for MSM developers to move this to drm_dsc_helper
> and own that as it might cause breakage of basic DSC even if some values
> are repeated.

It's time to stop thinking about ownership and start thinking about
shared code. We already have two instances of DSC tables. I don't
think having a third instance, which is a subset of an existing
dataset, would be beneficial to anybody.
AMD has complicated code which supports half-bit bpp and calculates
some of the parameters. But sharing data with the i915 driver is
straightforward.

> I would prefer to keep it in the msm code but in a top level directory
> so that we dont have to make DSI dependent on DPU.

I haven't changed my opinion. Please move relevant i915's code to
helpers, verify data against standards and reuse it.

> >> All vendors compute the values differently and eventually call drm_dsc_compute_rc_parameters()
> >>
> >>> I didn't check the tables against the standard (or against the current source code), will do that later.

-- 
With best wishes
Dmitry