| Message ID | 20220211224006.1797846-2-dmitry.baryshkov@linaro.org |
|---|---|
| State | Accepted |
| Commit | 4d793a02c4967ab14d4ae5e86a51ee02ed78921a |
| Headers | show |
| Series | Simplify and correct msm/dp bridge implementation | expand |
On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote: > 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 > panel_bridge attachment to come after dp_bridge attachment. > > 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> Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > 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; > }
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. > > 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.
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. > >> >> 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.
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; }
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 panel_bridge attachment to come after dp_bridge attachment. 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> --- drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)