[RFC PATCH 2/7] drm/msm/dp: support attaching bridges to the DP encoder

[Dmitry Baryshkov]

On Wed, 12 Jan 2022 at 02:12, Kuogee Hsieh <quic_khsieh at quicinc.com> wrote:
>
>
> On 1/6/2022 9:26 PM, Dmitry Baryshkov wrote:
> > On 07/01/2022 06:42, Stephen Boyd wrote:
> >> Quoting Dmitry Baryshkov (2022-01-06 18:01:27)
> >>> Currently DP driver will allocate panel bridge for eDP panels.
> >>> Simplify this code to just check if there is any next bridge in the
> >>> chain (be it a panel bridge or regular bridge). Rename panel_bridge
> >>> field to next_bridge accordingly.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> >>> ---
> >>>   drivers/gpu/drm/msm/dp/dp_display.c |  2 +-
> >>>   drivers/gpu/drm/msm/dp/dp_display.h |  2 +-
> >>>   drivers/gpu/drm/msm/dp/dp_drm.c     |  4 ++--
> >>>   drivers/gpu/drm/msm/dp/dp_parser.c  | 26 ++++++++------------------
> >>>   drivers/gpu/drm/msm/dp/dp_parser.h  |  2 +-
> >>>   5 files changed, 13 insertions(+), 23 deletions(-)
> >>
> >> I like this one, it certainly makes it easier to understand.
> >>
> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> >>> b/drivers/gpu/drm/msm/dp/dp_parser.c
> >>> index a7acc23f742b..5de21f3d0812 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> >>> @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser
> >>> *parser, int connector_type)
> >>>          if (rc)
> >>>                  return rc;
> >>>
> >>> -       if (connector_type == DRM_MODE_CONNECTOR_eDP) {
> >>
> >> It feels like this is on purpose, but I don't see any comment so I have
> >> no idea. I think qcom folks are concerned about changing how not eDP
> >> works. I'll have to test it out locally.
> >
> > Ah, another thing that should go into the commit message.
> >
> > Current situation:
> > - DP: no external bridges supported.
> > - eDP: only a drm_panel wrapped into the panel bridge
> >
> > After this patch:
> > - both DP and eDP support any chain of bridges attached.
> >
> >
> > While the change means nothing for the DP (IIUC, it will not have any
> > bridges), it simplifies the code path, lowering the amount of checks.
> >
> > And for eDP this means that we can attach any eDP-to-something bridges
> > (e.g. NXP PTN3460).
> >
> >
> > Well... After re-checking the source code for
> > devm_drm_of_get_bridge/drm_of_find_panel_or_bridge I should probably
> > revert removal of the check. The function will return -ENODEV if
> > neither bridge nor panel are specified.
> >
> I am new to drm and  confusing with bridge here.
>
> Isn't bridge used to bridging two different kind of interface together?
>
> for example, dsi <--> bridge <--> dp.
>
> why edp need bridge here?
>
> Can you give me more info regrading what bridge try to do here.

First, there are bridges converting the eDP interface to another
interface. The mentioned NXP PTN3460 converts (embedded) DisplayPort
to LVDS.

Second (and this is the case here) drm_bridge can be used to wrap
drm_panel (panel-bridge), so that the driver doesn't have to care
about the drm_panel interface.

Last, but not least, external display connectors can also be
abstracted as bridges (see display-connector.c).

This becomes even more appealing as the driver can then switch to
drm_bridge_connector, supporting any kinds of pipelines attached to
the encoder, supporting any kind of converters, panel or external
connector pipelines.Think about the following (sometimes crazy, but
possible) examples. With
drm_bridge/panel-bridge/display-connector/drm_bridge_connector there
is no difference for the driver at all:
- DP encoder ⇒ DP port ⇒ DP monitor
- DP encoder ⇒ DP connector supporting generic GPIO as HPD ⇒ DP port ⇒
DP monitor
- eDP encoder ⇒ eDP panel
- eDP encoder ⇒ ptn3460 ⇒ fixed LVDS panel
- eDP encoder ⇒ ptn3460 ⇒ LVDS connector with EDID lines for panel autodetect
- eDP encoder ⇒ ptn3460 ⇒ THC63LVD1024 ⇒ DPI panel.
- eDP encoder ⇒ LT8912 ⇒ DSI panel

> >>
> >>> -               rc = dp_parser_find_panel(parser);
> >>> -               if (rc)
> >>> -                       return rc;
> >>> -       }
> >>> +       rc = dp_parser_find_next_bridge(parser);
> >>> +       if (rc)
> >>> +               return rc;
> >>>
> >>>          /* Map the corresponding regulator information according to
> >>>           * version. Currently, since we only have one supported
> >>> platform,
> >
> >



-- 
With best wishes
Dmitry