| Message ID | 1685464318-25031-1-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| Headers | show |
| Series | retrieve DSI DSC through DRM bridge | expand |
>> + 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? > > 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. Do you have other suggestion? >
Generic note: please use reply-to-all instead of any other options to answer the email. You have dropped all recipients (except the freedreno@) in the message <d1a320c4-d851-ba75-ef7b-80dc369d1cfd@quicinc.com> (and it was left unnoticed). On Fri, 2 Jun 2023 at 20:00, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >> 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. > > I am confusing with item 5) > > Currently, msm_dsi_get_dsc_config() of dsi side return dsc pointer if > dsc enabled and NULL if dsc not enabled. > > Should checking dsc == NULL is good enough to differentiate between dsc > is supported and not supported? This is called a "shared memory area". Instead of either providing a dynamic data pointer, one can provide a pointer to the static area which is filled by DP or DSI. If there is no DSC available, one flags 'data not valid' by setting major,minor to 0. > > Why need to set both dsc->dsc_version_major = 0 and > dsc->dsc_version_minor = 0 for dsc is not supported? 6) Another option (which is more in style of what is done in the vendor kernel, if I'm not mistaken): Enhance struct drm_display_mode to contain a pointer to the DSC config. Use this pointer to check whether DSC should be enabled for the particular mode or not. The panels with the static DSC configuration can use a static data pointer.