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

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: Stephen Boyd <swboyd@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Kuogee Hsieh <quic_khsieh@quicinc.com>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [Freedreno] [RFC PATCH v2 1/5] drm/msm/dp: fix panel bridge attachment
Date: Fri, 25 Feb 2022 07:22:57 +0300	[thread overview]
Message-ID: <CAA8EJpp2KrHfNN0F7s6iNOd8Zh1w-1dBMfAXff4Yy4UAdpPGUA@mail.gmail.com> (raw)
In-Reply-To: <9398567b-6517-8eb3-f5d5-d8c88cb10814@quicinc.com>

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.

Thus the dp_bridge should be attached directly to the encoder
(drm_encoder) and panel_bridge should use dp_bridge as the 'previous'
bridge.

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.

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