Messages in this thread | | | Date | Thu, 16 Mar 2023 16:40:08 -0700 | Subject | Re: [Freedreno] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions | From | Abhinav Kumar <> |
| |
On 3/16/2023 9:36 AM, Abhinav Kumar wrote: > > > 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 >>>>
Yes, I agree with this part. for rc_model_size we can use DSC_RC_MODEL_SIZE_CONST.
initial_offset is already handled in https://patchwork.freedesktop.org/patch/525424/?series=114472&rev=2
Then we can use this math:
rc_model_size / (rc_model_size - initial_offset), keeping in mind that initial_scale_value has three fractional bits.
So this would be 8192 / (8192 - 6144) = 4
Then << 3 for 3 fractional bits = 32.
>>>> 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 >>>> >>>> >>>> >>
| |