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