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

From: Kuogee Hsieh
Date: Wed May 31 2023 - 11:41:17 EST

  +    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_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?