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

[Dmitry Baryshkov]

27 февраля 2023 г. 19:59:35 GMT+02:00, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> пишет:
>
>
>On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
>> On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>>> 
>>> 
>>> 
>>> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
>>>> On 26/02/2023 02:47, Abhinav Kumar wrote:
>>>>> Hi Dmitry
>>>>> 
>>>>> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>>>>>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
>>>>>>>> <quic_abhinavk@xxxxxxxxxxx> wrote:
>>>>>>>>> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
>>>>>>>>>> <quic_abhinavk@xxxxxxxxxxx> пишет:
>>>>>>>>>>> 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.
>>>>>> 
>>>>>> Otherwise it will have to be handled by Marijn, me or anybody else
>>>>>> wanting to hack up the DSC code. Or by anybody adding DSC support to
>>>>>> the next platform and having to figure out the difference between
>>>>>> i915, msm and their platform.
>>>>>> 
>>>>> 
>>>>> Yes, I wonder why the same doubt didn't arise when the other vendors
>>>>> added their support both from other maintainers and others.
>>>>> 
>>>>> Which makes me think that like I wrote in my previous response, these
>>>>> are "recommended" values in the spec but its not mandatory.
>>>> 
>>>> I think, it is because there were no other drivers to compare. In other
>>>> words, for a first driver it is pretty logical to have everything
>>>> handled on its own. As soon as we start getting other implementations of
>>>> a feature, it becomes logical to think if the code can be generalized.
>>>> This is what we see we with the HDCP series or with the code being moved
>>>> to DP helpers.
>>>> 
>>> 
>>> We were not the second, MSM was/is the third to add support for DSC afer
>>> i915 and AMD. Thats what made me think why whoever was the second didnt
>>> end up generalizing. Was it just missed out or was it intentionally left
>>> in the vendor driver.
>> 
>> I didn't count AMD here, since it calculates some of the params rather
>> than using the fixed ones from the model.
>> 
>>> 
>>>>> 
>>>>> Moving this to the drm_dsc_helper is generalizing the tables and not
>>>>> giving room for the vendors to customize even if they want to (which
>>>>> the spec does allow).
>>>> 
>>>> That depends on the API you select. For example, in
>>>> intel_dsc_compute_params() I see customization being applied to
>>>> rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
>>>> 
>>> 
>>> Thanks for going through the i915 to figure out that the 6bpp is handled
>>> in a customized way. So what you are saying is let the helper first fill
>>> up the recommended values of the spec, whatever is changed from that let
>>> the vendor driver override that.
>>> 
>>> Thats where the case-by-case handling comes.
>>> 
>>> Why not we do this way? Like you mentioned lets move these tables to the
>>> drm_dsc_helper and let MSM driver first use those.
>>> 
>>> Then in a separate patchset if i915 and AMD would like to move to that,
>>> let them handle it for their respective drivers instead of MSM going
>>> through whats customized for each calculation and doing it.
>>> 
>>> I am hesitant to take up that effort.
>> 
>> Writing a tool to convert model's rc_Nbpc_Mbpp_foo.cfg into C
>> languages structures used by Intel code took 15-20 minutes. Plugging
>> generated structures took another 5 minutes. I will send the patches
>> later today or tomorrow, as I find a time slot to clean them. Thank
>> you for spending more time on arguing than it took me to generate &
>> verify the data.
>> 
>
>Great, we will wait for your patches. We didnt intend to spend time on this at this point. We always wanted to take it up in a separate series of moving the tables.

Getting rid of msm_display_dsc_config and then making use of drm_dsc_compute_rc_parameters() was bad enough. So, let's get things done in a good way now, rather than at some random point later.


>
>You preferred not to wait. Upto you.
>
>So thanks for doing it.
>
>>> 
>>> If the recommended values work for the vendor, they can clean it up and
>>> move to the drm_dsc_helper themselves and preserving their
>>> customizations rather than one vendor doing it for all of them.
>>> 
>>>> In case the driver needs to perform customization of the params, nothing
>>>> stops it drop applying after filling all the RC params in the
>>>> drm_dsc_config struct via the generic helper.
>>>> 
>>>> 
>>>>> So if this has any merit and if you or Marijn would like to take it
>>>>> up, go for it. We would do the same thing as either of you would have
>>>>> to in terms of figuring out the difference between msm and the i915 code.
>>>>> 
>>>>> This is not a generic API we are trying to put in a helper, these are
>>>>> hard-coded tables so there is a difference between looking at these Vs
>>>>> looking at some common code which can move to the core.
>>>>> 
>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 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.
>>>>>> 
>>>>>> Added i915 maintainers to the CC list for them to be able to answer.
>>>>>> 
>>>>> 
>>>>> Thanks, lets wait to hear from them about where finally these tables
>>>>> should go but thats can be taken up as a separate effort too.
>>>>> 
>>>>>>> 
>>>>>>> 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.
>>>>>> 
>>>>>> I haven't heard back from Kuogee about the possible causes of using
>>>>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on
>>>>>> that? In other words, can we always stick to the values from 1.2
>>>>>> standard? What will be the drawback?
>>>>>> 
>>>>>> Otherwise, we'd have to have two different sets of values, like you
>>>>>> do in your vendor driver.
>>>>>> 
>>>>> 
>>>>> I have responded to this in the other email.
>>>>> 
>>>>> All this being said, even if the rc tables move the drm_dsc_helper
>>>>> either now or later on, we will still need MSM specific calculations
>>>>> for many of the other encoder parameters (which are again either
>>>>> hard-coded or calculated). Please refer to the
>>>>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find
>>>>> those in the DP spec directly.
>>>>> 
>>>>> So we will still need a dsc helper for MSM calculations to be common
>>>>> for DSI / DP irrespective of where the tables go.
>>>>> 
>>>>> So, lets finalize that first.
>>>> 
>>>> I went on and trimmed sde_dsc_populate_dsc_config() to remove
>>>> duplication with the drm_dsc_compute_rc_parameters() (which we already
>>>> use for the MSM DSI DSC).
>>>> 
>>>> Not much is left:
>>>> 
>>>> dsc->first_line_bpg_offset set via the switch
>>>> 
>>>> dsc->line_buf_depth = bpc + 1;
>>>> dsc->mux_word_size = bpc > 10 ? DSC_MUX_WORD_SIZE_12_BPC:
>>>>           DSC_MUX_WORD_SIZE_8_10_BPC;
>>>> 
>>>> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420))
>>>>       dsc->nsl_bpg_offset = (2048 *
>>>>                (DIV_ROUND_UP(dsc->second_line_bpg_offset,
>>>>                                   (dsc->slice_height - 1))));
>>>> 
>>>> dsc->initial_scale_value = 8 * dsc->rc_model_size /
>>>>                           (dsc->rc_model_size - dsc->initial_offset);
>>>> 
>>>> 
>>>> mux_word_size comes from the standard (must)
>>>> initial_scale_value calculation is recommended, but not required
>>>> nsl_bpg_offset follows the standard (must), also see below (*).
>>>> 
>>>> first_line_bpg_offset calculation differs between three drivers. The
>>>> standard also provides a recommended formulas. I think we can leave it
>>>> as is for now.
>>>> 
>>>> I think, that mux_word_size and nsl_bpg_offset calculation should be
>>>> moved to drm_dsc_compute_rc_parameters(), while leaving
>>>> initial_scale_value in place (in the driver code).
>>>> 
>>>> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard
>>>> demands that it is set to 'second_line_bpg_offset / (slice_height - 1),
>>>> rounded up to 16 fraction bits', while SDE driver code sets it to the
>>>> value rounded up to the next integer (having 16 fraction bits
>>>> representation).
>>>> 
>>>> In my opinion correct calculation should be:
>>>> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset,
>>>>                                   (dsc->slice_height - 1));
>>>> 
>>>> Could you please check, which one is correct according to the standard?
>>>> 
>>>> 
>>> 
>>> Sure, i will check about nsl_bpg_offset. But sorry if I was not more
>>> clear about this but sde_dsc_populate_dsc_config() is only one example
>>> which from your analysis can be moved to the drm_dsc_helper() but not
>>> the initial line calculation _dce_dsc_initial_line_calc(),
>>> _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper().
>> 
>> The initial_line is already calculated in dpu_encoder.c. As for the
>> _dce_dsc_ich_reset_override_needed(), I don't think we support partial
>> updates in the upstream driver.
>> 
>>> 
>>> All of these are again common between DSI and DP.
>>> 
>>> So in addition to thinking about what can be moved to the drm_dsc_helper
>>> also think about what is specific to MSM but common to DSI and DP modules.
>>> 
>>> That was the bigger picture I was trying to convey.
>> 
>
>_dce_dsc_initial_line_calc which will get expanded with v1.2 gets added has much more than whats there in upstream today.
>
>Dumping everything in dpu_encoder is not the solution. Sorry.

But it is still the DPU thing. So, no problems.

>
>> 
>>