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

[Abhinav Kumar]

On 3/16/2023 9:23 AM, Dmitry Baryshkov wrote:
> On 16/03/2023 18:13, Abhinav Kumar wrote:
>>
>>
>> On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote:
>>> Hi,
>>>
>>> [removed previous conversation]
>>>
>>>>
>>>> Hi Dmitry and Abhinav,
>>>>
>>>> Just wanted to follow up on this thread. I've gone over the 
>>>> MSM-specific
>>>> DSC params for DP and DSI and have found a few shared calculations and
>>>> variables between both DSI and DP paths:
>>>>
>>>> - (as mentioned earlier in the thread) almost all the calculations in
>>>> dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
>>>> only difference in the math I'm seeing is initial_scale_value.
>>>
>>> The value in dsi code is valid for initial_offset = 6144. Please use
>>> the formula from the standard (= sde_dsc_populate_dsc_config) and add
>>> it to drm_dsc_helper.c
>>>
>>> If I remember correctly the last remaining item in
>>> dsi_populate_dsc_params() (except mentioned initial_offset) was
>>> line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
>>> According to the standard it should come from a DSC decoder spec,
>>> which means it should be set by the DSI panel driver or via
>>> drm_dp_dsc_sink_line_buf_depth() in the case of DP output.
>>>
>>>> - dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were 
>>>> introduced
>>>> in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing
>>>> engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
>>>> (which is calculated differently between DP and DSI), but
>>>> dce_bytes_per_line is calculated the same way between DP and DSI.
>>>>
>>>> To avoid having to duplicate math in 2 different places, I think it
>>>> would help to have these calculations in some msm_dsc_helper.c file. 
>>>> Any
>>>> thoughts on this?
>>>
>>> dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
>>> code, so they can stay in DPU driver.
>>>
>>
>> They can stay in the dpu driver is fine but where?
>>
>> Like Jessica wrote, this is computed and used in 3 places today :
>>
>> 1) DSI video engine computation
>> 2) DP controller computation
>> 3) timing engine programming
> 
> Please excuse me if I'm wrong. I checked both vendor techpack and the 
> Kuogee's patches. I see them being used only in the SDE / DPU driver 
> code. Could you please point me to the code path that we are discussing?
> 

DSI code :

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dsi/dsi_host.c#L868

DP code:

Refer to dp_panel_dsc_pclk_param_calc in 
https://patchwork.freedesktop.org/patch/519837/?series=113240&rev=1

Timing engine:

refer to https://patchwork.freedesktop.org/patch/519838/?series=113240&rev=1

Probably confusion is due to the naming. bytes_per_line is nothing but 
bytes_per_pkt * pkt_per_line but the concept is common for DP and DSI.

+		if (phys->comp_type == MSM_DISPLAY_COMPRESSION_DSC) {
+			phys->dsc_extra_pclk_cycle_cnt = dsc_info->pclk_per_line;
+			phys->dsc_extra_disp_width = dsc_info->extra_width;
+			phys->dce_bytes_per_line =
+				dsc_info->bytes_per_pkt * dsc_info->pkt_per_line;

> 
>> So either we have a helper in a common location somewhere so that 
>> these 3 modules can call that helper and use it OR each module 
>> duplicates the computation code.
>>
>> What should be the common location is the discussion here.
>>
>> It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder 
>> methods.
>>
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756 
>>>>
>>>>
>>>> [2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1
>>>
>>> [3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
>>>
>>>
>>>
>