On Thu, 1 Jun 2023 at 20:37, Kuogee Hsieh <[email protected]> wrote: > > > On 5/31/2023 10:53 AM, Dmitry Baryshkov wrote: > > On Wed, 31 May 2023 at 20:29, Kuogee Hsieh <[email protected]> wrote: > >> > >> On 5/31/2023 10:12 AM, Dmitry Baryshkov wrote: > >>> On Wed, 31 May 2023 at 18:41, Kuogee Hsieh <[email protected]> > >>> 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
