On Fri, 25 Feb 2022 at 20:11, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 2/25/2022 1:04 AM, Dmitry Baryshkov wrote: > > 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. > > > > 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@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