[Freedreno] [PATCH v1 2/3] drm/msm/dpu: retrieve DSI DSC struct at atomic_check()

[Dmitry Baryshkov]

On Thu, 1 Jun 2023 at 20:37, Kuogee Hsieh <quic_khsieh at quicinc.com> wrote:
>
>
> On 5/31/2023 10:53 AM, Dmitry Baryshkov wrote:
> > On Wed, 31 May 2023 at 20:29, Kuogee Hsieh <quic_khsieh at quicinc.com> wrote:
> >>
> >> On 5/31/2023 10:12 AM, Dmitry Baryshkov wrote:
> >>> On Wed, 31 May 2023 at 18:41, Kuogee Hsieh <quic_khsieh at quicinc.com> wrote:
> >>>>
> >>>>>>     +    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> >>>>> INTF_DSI
> >>>>>
> >>>>>> +        struct drm_bridge *bridge;
> >>>>>> +
> >>>>>> +        if (!dpu_enc->dsc) {
> >>>>> This condition is not correct. We should be updating the DSC even if
> >>>>> there is one.
> >>>>>
> >>>>>> +            bridge = drm_bridge_chain_get_first_bridge(drm_enc);
> >>>>>> +            dpu_enc->dsc = msm_dsi_bridge_get_dsc_config(bridge);
> >>>>> This approach will not work for the hot-pluggable outputs. The dpu_enc
> >>>>> is not a part of the state. It should not be touched before
> >>>>> atomic_commit actually commits changes.
> >>>> where can drm_dsc_config be stored?
> >>> I'd say, get it during atomic_check (and don't store it anywhere).
> >>> Then get it during atomic_enable (and save in dpu_enc).
> >> got it.
> >>>>> Also, I don't think I like the API. It makes it impossible for the
> >>>>> driver to check that the bridge is the actually our DSI bridge or not.
> >>>>> Once you add DP here, the code will explode.
> >>>>>
> >>>>> I think instead we should extend the drm_bridge API to be able to get
> >>>>> the DSC configuration from it directly. Additional care should be put
> >>>>> to design an assymetrical API. Theoretically a drm_bridge can be both
> >>>>> DSC source and DSC sink. Imagine a DSI-to-DP or DSI-to-HDMI bridge,
> >>>>> supporting DSC on the DSI side too.
> >>>> Form my understanding, a bridge contains two interfaces.
> >>>>
> >>>> Therefore I would think only one bridge for dsi-to-dp bridge? and this
> >>>> bridge should represent the bridge chip?
> >>>>
> >>>> I am thinking adding an ops function, get_bridge_dsc() to struct
> >>>> drm_bridge_funcs to retrieve drm_dsc_config.
> >>> So, for this DSI-to-DP bridge will get_bridge_dsc() return DSC
> >>> configuration for  the DSI or for the DP side of the bridge?
> >> I would think should be DP side. there is no reason to enable dsc on
> >> both DSI and DP fro a bridge chip.
>
> My above statement is not correct. For DSI-to-DP bridge,
> drm_bridge_chain_get_first_bridge(drm_enc) return DSI bridge.
>
> Is possible that DSC feature can be added to DSI-to-DP bridge?
>
> If it is not possible, then we can rule out DSI-to-DP bridge case, then
> use struct drm_bridge to retrieve DSC form controller will work.
>
>
>
> > Well, there can be. E.g. to lower the clock rates of the DSI link.
> >
> >> drm_bridge_chain_get_first_bridge(drm_enc) should return the bridge of
> >> the controller.
> >>
> >> In DSI-to-DP bridge chip case, this controller will be the bridge chip
> >> who configured to perform protocol conversion between DSI and DP.
> >>
> >> If DSC enabled should be at DP size which connect to panel.
> > Ok, so it returns the DSC configuration of the bridge's source side.
> > Now let's consider a panel bridge for the DSC-enabled DSI panel.
> > Should get_bridge_dsc() return a DSC config in this case?
> >
> >>>> Do you have other suggestion?
> >>> Let me think about it for a few days.
>
> There is one option which is keep current
>
> 1) keep struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi
> *msm_dsi) at dsi.c
>
> 2) use  struct msm_display_info *disp_info saved at dpu_enc to locate
> struct msm_dsi from priv->dsi[] list (see item #3)
>
> 3)  info.dsc = msm_dsi_get_dsc_config(priv->dsi[info.h_tile_instance[0]]);
>
> 4) ballistically, keep original code but move  info.dsc =
> msm_dsi_get_dsc_config(priv->dsi[i]); to other place sush as
> atomic_check() and atomic_enable().
>

5) leave drm_dsc_config handling as is, update the dsc config from the
DP driver as suitable. If DSC is not supported, set
dsc->dsc_version_major = 0 and dsc->dsc_version_minor = 0 on the DP
side. In DPU driver verify that dsc->dsc_version_major/_minor != 0.

-- 
With best wishes
Dmitry