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
