| 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.
On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote: > On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar <quic_abhinavk@quicinc.com> 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. 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, 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); 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@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. >>> > > >
On Fri, 25 Feb 2022 at 07:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/24/2022 8:22 PM, Dmitry Baryshkov wrote: > > On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote: > >>> On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar <quic_abhinavk@quicinc.com> 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. > > Agreed. For drm_bridge_chain_foo() ordering to be correct, for eDP chain > the order should be what you mentioned and panel_bridge should be at the > end ( should be the last one ). > > For the above reason, > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > I still didnt understand what gets affected by this for the > msm_dp_display_foo() functions you mentioned earlier and wanted to get > some clarity on that. They should be called at the correct time. > > > > Thus the dp_bridge should be attached directly to the encoder > > (drm_encoder) and panel_bridge should use dp_bridge as the 'previous' > > bridge. > > > > Agreed. > > > 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. > > > >> > > So originally the plan was always that the DP connector layer intercepts > the call because panel-eDP file did not support reading of the EDID ( we > have not provided the aux bus ). So it was intended that we did not want > to goto the eDP panel to get the modes. Not an error but something which > we wanted to cleanup later when we moved to panel-eDP completely. panel_edp_get_modes() correctly handles this case and returns modes specified in the panel description. So the code should work even with panel-eDP and w/o the AUX bus. > > Till then we wanted the dp_connector to read the EDID and get the modes. > > So this was actually intended to happen till the point where we moved to > panel-eDP to get the modes. > > Hence what you have mentioned in your cover letter is right that the > chain was broken but there was no loss of functionality due to that today. > > Now that these changes are made, we can still goto panel-eDP file for > the modes because of the below change from Sankeerth where the mode is > hard-coded: > > https://patchwork.freedesktop.org/patch/473431/ > > With this change cherry-picked it should work for kuogee. We will > re-test that with this change. I suppose he had both of the changes in the tree. Otherwise the panel_edp_get_modes() wouldn't be called. > >> 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@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. > >>>>> > >>> > >>> > >>> > > > > > >
On Fri, 25 Feb 2022 at 20:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/25/2022 1:04 AM, Dmitry Baryshkov wrote: > > On Fri, 25 Feb 2022 at 07:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 2/24/2022 8:22 PM, Dmitry Baryshkov wrote: > >>> On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote: > >>>>> On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar <quic_abhinavk@quicinc.com> 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. > >> > >> Agreed. For drm_bridge_chain_foo() ordering to be correct, for eDP chain > >> the order should be what you mentioned and panel_bridge should be at the > >> end ( should be the last one ). > >> > >> For the above reason, > >> > >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> > >> I still didnt understand what gets affected by this for the > >> msm_dp_display_foo() functions you mentioned earlier and wanted to get > >> some clarity on that. > > > > They should be called at the correct time. > > > >>> > >>> Thus the dp_bridge should be attached directly to the encoder > >>> (drm_encoder) and panel_bridge should use dp_bridge as the 'previous' > >>> bridge. > >>> > >> > >> Agreed. > >> > >>> 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. > >>> > >>>> > >> > >> So originally the plan was always that the DP connector layer intercepts > >> the call because panel-eDP file did not support reading of the EDID ( we > >> have not provided the aux bus ). So it was intended that we did not want > >> to goto the eDP panel to get the modes. Not an error but something which > >> we wanted to cleanup later when we moved to panel-eDP completely. > > > > panel_edp_get_modes() correctly handles this case and returns modes > > specified in the panel description. So the code should work even with > > panel-eDP and w/o the AUX bus. > > > > If hard-coded modes are not present, it will fail in below case: > > /* > * Add hard-coded panel modes. Don't call this if there are no timings > * and no modes (the generic edp-panel case) because it will clobber > * the display_info that was already set by drm_add_edid_modes(). > */ > if (p->desc->num_timings || p->desc->num_modes) > num += panel_edp_get_non_edid_modes(p, connector); > else if (!num) > dev_warn(p->base.dev, "No display modes\n"); > > Thats exactly the error he was seeing. Ah, so he had neither timings nor modes. The rest of the panels does. > > >> > >> Till then we wanted the dp_connector to read the EDID and get the modes. > >> > >> So this was actually intended to happen till the point where we moved to > >> panel-eDP to get the modes. > >> > >> Hence what you have mentioned in your cover letter is right that the > >> chain was broken but there was no loss of functionality due to that today. > >> > >> Now that these changes are made, we can still goto panel-eDP file for > >> the modes because of the below change from Sankeerth where the mode is > >> hard-coded: > >> > >> https://patchwork.freedesktop.org/patch/473431/ > >> > >> With this change cherry-picked it should work for kuogee. We will > >> re-test that with this change. > > > > I suppose he had both of the changes in the tree. Otherwise the > > panel_edp_get_modes() wouldn't be called. > > No he did not. > > That brings up another point. > > Even Bjorn was relying on the DP connector's get_modes() for his eDP > panel if I am not mistaken. > > Unless he makes an equivalent change for his panel OR supports reading > EDID in panel-edp, then he will hit the same error. > > Given that your changes were only compile tested, lets wait for both > Kuogee and Bjorn to validate their resp setups. Sure, anyway I think it's too late to bring in this patch during this cycle. > > > > >>>> 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@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(-)