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

[Kuogee Hsieh]

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().