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

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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.

So DP's bridge will still be attached to its encoder's root.

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@quicinc.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>
>> Reviewed-by: Stephen Boyd <swboyd@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.
>