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

[Abhinav Kumar]

On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
> 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.
> 

Got it, thanks

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

Well, we have to figure out if each value matches and if each of them 
come from the spec for us and i915 and from which section. So it is 
unfortunately our problem.

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

Sorry, but I would like to get an ack from i915 folks if this is going
to be useful to them if we move this to helper because we have to look 
at every table. Not just one.

Also, this is just 1.1, we will add more tables for 1.2. So we will have 
to end up changing both 1.1 and 1.2 tables as they are different for QC.

So if you look at the DSC spec from where these tables have come it says

"Common Recommended Rate Control-Related Parameter Values"

Its Recommended but its NOT mandated by the spec to follow every value 
to the dot. I have confirmed this point with more folks.

So, if someone from i915 this is useful and safe to move their code to 
the tables, we can try it.

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