On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote:
> > On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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.
No. It is not.
For both DP and eDP the chain should be:
dpu_encoder -> dp_bridge -> external (panel) bridge, optional for DP
-> [other bridges] -> connector
Otherwise drm_bridge_chain_foo() functions would be called in the
incorrect order.
Thus the dp_bridge should be attached directly to the encoder
(drm_encoder) and panel_bridge should use dp_bridge as the 'previous'
bridge.
For example, for the DP port one can use a display-connector (which
actually implements drm_bridge) as an external bridge to provide hpd
or dp power GPIOs.
Note, that the current dp_connector breaks layering. It makes calls
directly into dp_display, not allowing external bridge (and other
bridges) to override get_modes/mode_valid and other callbacks.
Thus one of the next patches in series (the one that Kuogee had issues
with) tries to replace the chain with the following one:
dpu_encoder -> dp_bridge -> external (panel) bridge -> [other bridges]
-> drm_bridge_connector.
>
> 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,
as you see, NULL is incorrect. It should be a pointer to dp_bridge instead
> 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);
And this bridge should be attached before the external bridge.
>
> 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@xxxxxxxxxxx>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >>>>> ---
> >>>>
> >>>> Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> >>>>
> >>>>> 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.
> >>>
> >
> >
> >
--
With best wishes
Dmitry
