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. > 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. > > 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. > 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. -- With best wishes Dmitry
