[Freedreno] [RFC PATCH v2 1/5] drm/msm/dp: fix panel bridge attachment

[Abhinav Kumar]

On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote:
> On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 2/18/2022 6:26 PM, Dmitry Baryshkov wrote:
>>> On 19/02/2022 02:56, Stephen Boyd wrote:
>>>> Quoting Dmitry Baryshkov (2022-02-11 14:40:02)
>>>>> In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display
>>>>> enable and disable") the DP driver received a drm_bridge instance, which
>>>>> is always attached to the encoder as a root bridge. However it conflicts
>>>>> with the panel_bridge support for eDP panels. The panel bridge attaches
>>>>> to the encoder before the "dp" bridge has a chace to do so. Change
>>>>
>>>> s/chace/chance/
>>>>
>>>>> panel_bridge attachment to come after dp_bridge attachment.
>>>>
>>>> s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge
>>>> is the "DP driver's drm_bridge instance created in
>>>> msm_dp_bridge_init()"?
>>>>
>>>> My understanding is that we want to pass the bridge created in
>>>> msm_dp_bridge_init() as the 'previous' bridge so that it attaches the
>>>> panel bridge to the output of the DP bridge that's attached to the
>>>> encoder.
>>>
>>> Thanks! I'll update the commit log when pushing the patches.
>>
>> Please correct if i am missing something here.
>>
>> You are right that the eDP panel's panel bridge attaches to the encoder
>> in dp_drm_connector_init() which happens before msm_dp_bridge_init() and
>> hence it will attach directly to the encoder.
>>
>> But we are talking about different encoders here. DP's dp_display has a
>> different encoder compared to the EDP's dp_display.
> 
> The encoders are different. However both encoders use the same
> codepath, which includes dp_bridge. It controls dp_display by calling
> msm_dp_display_foo() functions.
> 
>> So DP's bridge will still be attached to its encoder's root.
> 
> There is one dp_bridge per each encoder. Consider sc8180x which has 3
> DP controllers (and thus 3 dp_bridges).
> 

Sorry, but I still didnt follow this.

So for eDP, dp_drm_connector_init() will attach the panel_bridge
and then msm_dp_bridge_init() will add a drm_bridge.

And yes in that case, the drm_bridge will be after the panel_bridge

But since panel_bridge is at the root for eDP it should be okay.

Your commit text was mentioning about DP.

For DP, for each controller in the catalog, we will call modeset_init() 
which should skip this part for DP

   if (dp_display->panel_bridge) {
         ret = drm_bridge_attach(dp_display->encoder,
                     dp_display->panel_bridge, NULL,
                     DRM_BRIDGE_ATTACH_NO_CONNECTOR);
         if (ret < 0) {
             DRM_ERROR("failed to attach panel bridge: %d\n", ret);
             return ERR_PTR(ret);
         }
     }

Followed by calling msm_dp_bridge_init() which will actually attach the 
bridge:

drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);

Now, even for 3 DP controllers, this shall be true as there will be 3 
separate encoders and 3 dp_displays and hence 3 drm_bridges.

What am i missing here?

>>
>> So what are we achieving with this change?
>>
>>>
>>>>
>>>>>
>>>>> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display
>>>>> enable and disable")
>>>>> Cc: Kuogee Hsieh <quic_khsieh at quicinc.com>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>> ---
>>>>
>>>> Reviewed-by: Stephen Boyd <swboyd at chromium.org>
>>>>
>>>>>    drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++----------
>>>>>    1 file changed, 11 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>> index d4d360d19eba..26ef41a4c1b6 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>> @@ -169,16 +169,6 @@ struct drm_connector
>>>>> *dp_drm_connector_init(struct msm_dp *dp_display)
>>>>>
>>>>>           drm_connector_attach_encoder(connector, dp_display->encoder);
>>>>>
>>>>> -       if (dp_display->panel_bridge) {
>>>>> -               ret = drm_bridge_attach(dp_display->encoder,
>>>>> -                                       dp_display->panel_bridge, NULL,
>>>>> -                                       DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>>> -               if (ret < 0) {
>>>>> -                       DRM_ERROR("failed to attach panel bridge:
>>>>> %d\n", ret);
>>>>> -                       return ERR_PTR(ret);
>>>>> -               }
>>>>> -       }
>>>>> -
>>>>>           return connector;
>>>>>    }
>>>>>
>>>>> @@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct
>>>>> msm_dp *dp_display, struct drm_devi
>>>>>                   return ERR_PTR(rc);
>>>>>           }
>>>>>
>>>>> +       if (dp_display->panel_bridge) {
>>>>> +               rc = drm_bridge_attach(dp_display->encoder,
>>>>> +                                       dp_display->panel_bridge,
>>>>> bridge,
>>>>> +                                       DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>>> +               if (rc < 0) {
>>>>> +                       DRM_ERROR("failed to attach panel bridge:
>>>>> %d\n", rc);
>>>>> +                       drm_bridge_remove(bridge);
>>>>> +                       return ERR_PTR(rc);
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>>           return bridge;
>>>>
>>>> Not a problem with this patch, but what is this pointer used for? I see
>>>> it's assigned to priv->bridges and num_bridges is incremented but nobody
>>>> seems to look at that.
>>>
>>>
>>> That's on my todo list. to remove connectors array and to destroy
>>> created bridges during drm device destruction.
>>>
> 
> 
>