Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <4d0ec577fdeb01fa232ffa743fde06129353396a.1634126587.git.agx@sigxcpu.org>
In-Reply-To: <4d0ec577fdeb01fa232ffa743fde06129353396a.1634126587.git.agx@sigxcpu.org>
From:   Jagan Teki <jagan@amarulasolutions.com>
Date:   Wed, 13 Oct 2021 17:54:36 +0530
Message-ID: <CAMty3ZAsn4K0WFRC_FNN2Mina0gSu4Nc1y1zVsoZ8GuSqcQFsA@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: nwl-dsi: Move bridge add/remove to dsi callbacks
To:     =?UTF-8?Q?Guido_G=C3=BCnther?= <agx@sigxcpu.org>
Cc:     Andrzej Hajda <andrzej.hajda@gmail.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Robert Foss <robert.foss@linaro.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 13, 2021 at 5:44 PM Guido G=C3=BCnther <agx@sigxcpu.org> wrote:
>
> Move the panel and bridge_{add,remove} from the bridge callbacks to the
> DSI callbacks to make sure we don't indicate readiness to participate in
> the display pipeline before the panel is attached.
>
> This was prompted by commit fb8d617f8fd6 ("drm/bridge: Centralize error
> message when bridge attach fails") which triggered
>
>   [drm:drm_bridge_attach] *ERROR* failed to attach bridge /soc@0/bus@3080=
0000/mipi-dsi@30a0 0000 to encoder None-34: -517
>
> during boot.
>
> Signed-off-by: Guido G=C3=BCnther <agx@sigxcpu.org>
> ---
> This was prompted by the discussion at
> https://lore.kernel.org/dri-devel/00493cc61d1443dab1c131c46c5890f95f6f9b2=
5.1634068657.git.agx@sigxcpu.org/
>
>  drivers/gpu/drm/bridge/nwl-dsi.c | 64 ++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nw=
l-dsi.c
> index a7389a0facfb..77aa6f13afef 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -355,6 +355,9 @@ static int nwl_dsi_host_attach(struct mipi_dsi_host *=
dsi_host,
>  {
>         struct nwl_dsi *dsi =3D container_of(dsi_host, struct nwl_dsi, ds=
i_host);
>         struct device *dev =3D dsi->dev;
> +       struct drm_bridge *panel_bridge;
> +       struct drm_panel *panel;
> +       int ret;
>
>         DRM_DEV_INFO(dev, "lanes=3D%u, format=3D0x%x flags=3D0x%lx\n", de=
vice->lanes,
>                      device->format, device->mode_flags);
> @@ -362,10 +365,43 @@ static int nwl_dsi_host_attach(struct mipi_dsi_host=
 *dsi_host,
>         if (device->lanes < 1 || device->lanes > 4)
>                 return -EINVAL;
>
> +       ret =3D drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &pan=
el,
> +                                         &panel_bridge);
> +       if (ret)
> +               return ret;
> +
> +       if (panel) {
> +               panel_bridge =3D drm_panel_bridge_add(panel);
> +               if (IS_ERR(panel_bridge))
> +                       return PTR_ERR(panel_bridge);
> +       }
> +       if (!panel_bridge)
> +               return -EPROBE_DEFER;
> +
> +       dsi->panel_bridge =3D panel_bridge;
>         dsi->lanes =3D device->lanes;
>         dsi->format =3D device->format;
>         dsi->dsi_mode_flags =3D device->mode_flags;
>
> +       /*
> +        * The DSI output has been properly configured, we can now safely
> +        * register the input to the bridge framework so that it can take=
 place
> +        * in a display pipeline.
> +        */
> +       drm_bridge_add(&dsi->bridge);
> +
> +       return 0;
> +}
> +
> +static int nwl_dsi_host_detach(struct mipi_dsi_host *dsi_host,
> +                              struct mipi_dsi_device *dev)
> +{
> +       struct nwl_dsi *dsi =3D container_of(dsi_host, struct nwl_dsi, ds=
i_host);
> +
> +       drm_bridge_remove(&dsi->bridge);
> +       if (dsi->panel_bridge)
> +               drm_panel_bridge_remove(dsi->panel_bridge);
> +

If I'm correct this logic will failed to find the direct and I2C based
bridges. As these peripheral bridges are trying to find the bridge
device from bridge_attach unlike DSI panels are trying to find the
panel via host attach directly from probe.

Similar issue we have encounters with dw-mipi-dsi bridge.
c206c7faeb3263a7cc7b4de443a3877cd7a5e74b

Jagan.