Messages in this thread | | | Date | Thu, 16 Mar 2023 09:36:25 -0700 | Subject | Re: [Freedreno] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions | From | 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 >>> >>> >>> >
| |