| Message ID | 20220107020132.587811-2-dmitry.baryshkov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/7] drm/msm/dp: fix panel bridge attachment | expand |
Quoting Dmitry Baryshkov (2022-01-06 18:01:26) > 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. Can you elaborate here? How does it conflict? Could be as simple as "it attaches before the panel bridge can attach to the root" or something like that. > Change panel_bridge > attachment to come after dp_bridge attachment. >
On 07/01/2022 06:37, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-01-06 18:01:26) >> 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. > > Can you elaborate here? How does it conflict? Could be as simple as "it > attaches before the panel bridge can attach to the root" or something > like that. Actually it would be the other way around: panel bridge attaching before the "dp" one. But yes, you got the idea. I'll extend the patch's description. >> Change panel_bridge >> attachment to come after dp_bridge attachment. >>
On 1/6/2022 6:01 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. 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(-) > > 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); > } > can check connector_type here and if connector_type == DRM_MODE_CONNECTOR_eDP then no drm_bridge add to eDP? So that eDP only has panel_bridge and DP only has drm_bridge? is this fix all your concerns? > + 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; > }
On Wed, 12 Jan 2022 at 20:41, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > On 1/6/2022 6:01 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. 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(-) > > > > 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); > > } > > > > can check connector_type here and if connector_type == > DRM_MODE_CONNECTOR_eDP then no drm_bridge add to eDP? So that eDP only > has panel_bridge and DP only has drm_bridge? No, we still need the DP bridge for the eDP. It handles modesetting, enabling and disabling of the DP controller, etc. > > is this fix all your concerns? > > > > + 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; > > }
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. 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(-)