Re: [Freedreno] [RFC PATCH v2 1/5] drm/msm/dp: fix panel bridge attachment

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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.
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry