Message-ID: <CAAOTY_-pOUuM7LQ1jm6gqpg8acMqDWOHxGucY5XOjq0ctGUkzA@mail.gmail.com>
Date:   Thu, 14 May 2020 22:28:33 +0800
From:   Chun-Kuang Hu <chunkuang.hu@...nel.org>
To:     Enric Balletbo Serra <eballetbo@...il.com>
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Collabora Kernel ML <kernel@...labora.com>,
        Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Nicolas Boichat <drinkcat@...omium.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        David Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 7/7] drm/mediatek: mtk_dsi: Create connector for bridges

Hi, Enric:

Enric Balletbo Serra <eballetbo@...il.com> 於 2020年5月14日 週四 上午12:41寫道：
>
> Hi Chun-Kuang,
>
> Missatge de Enric Balletbo i Serra <enric.balletbo@...labora.com> del
> dia dv., 1 de maig 2020 a les 17:25:
> >
> > Use the drm_bridge_connector helper to create a connector for pipelines
> > that use drm_bridge. This allows splitting connector operations across
> > multiple bridges when necessary, instead of having the last bridge in
> > the chain creating the connector and handling all connector operations
> > internally.
> >
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> > Acked-by: Sam Ravnborg <sam@...nborg.org>
>
> A gentle ping on this, I think that this one is the only one that
> still needs a review in the series.

This is what I reply in patch v3:

I think the panel is wrapped into next_bridge here,

if (panel) {
    dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel);

so the next_bridge is a panel_bridge, in its attach function
panel_bridge_attach(),
according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist,
it would create connector and attach connector to panel.

I'm not sure this flag would exist or not, but for both case, it's strange.
If exist, you create connector in this patch but no where to attach
connector to panel.
If not exist, the next_brige would create one connector and this brige
would create another connector.

I think in your case, mtk_dsi does not directly connect to a panel, so
I need a exact explain. Or someone could test this on a
directly-connect-panel platform.

Regards,
Chun-Kuang.

>
> Thanks,
>  Enric
>
> > ---
> >
> > Changes in v4: None
> > Changes in v3:
> > - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart)
> >
> > Changes in v2: None
> >
> >  drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 4f3bd095c1ee..471fcafdf348 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -17,6 +17,7 @@
> >
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_bridge_connector.h>
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> > @@ -183,6 +184,7 @@ struct mtk_dsi {
> >         struct drm_encoder encoder;
> >         struct drm_bridge bridge;
> >         struct drm_bridge *next_bridge;
> > +       struct drm_connector *connector;
> >         struct phy *phy;
> >
> >         void __iomem *regs;
> > @@ -977,10 +979,19 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
> >          */
> >         dsi->encoder.possible_crtcs = 1;
> >
> > -       ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0);
> > +       ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL,
> > +                               DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >         if (ret)
> >                 goto err_cleanup_encoder;
> >
> > +       dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder);
> > +       if (IS_ERR(dsi->connector)) {
> > +               DRM_ERROR("Unable to create bridge connector\n");
> > +               ret = PTR_ERR(dsi->connector);
> > +               goto err_cleanup_encoder;
> > +       }
> > +       drm_connector_attach_encoder(dsi->connector, &dsi->encoder);
> > +
> >         return 0;
> >
> >  err_cleanup_encoder:
> > --
> > 2.26.2
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek

Powered by blists - more mailing lists