On Fri, 25 Feb 2022 at 07:45, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 2/24/2022 8:22 PM, Dmitry Baryshkov wrote: > > 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. > > 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@xxxxxxxxxxx> > > 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@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