Re: [PATCH] drm/vc4: dsi: Only register our component once a DSI device is attached

[Dave Stevenson]

Hi Laurent

On Sun, 20 Jun 2021 at 04:26, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Maxime,
>
> I'm testing this, and I'm afraid it causes an issue with all the
> I2C-controlled bridges. I'm focussing on the newly merged ti-sn65dsi83
> driver at the moment, but other are affected the same way.
>
> With this patch, the DSI component is only added when the DSI device is
> attached to the host with mipi_dsi_attach(). In the ti-sn65dsi83 driver,
> this happens in the bridge attach callback, which is called when the
> bridge is attached by a call to drm_bridge_attach() in vc4_dsi_bind().
> This creates a circular dependency, and the DRM/KMS device is never
> created.
>
> How should this be solved ? Dave, I think you have shown an interest in
> the sn65dsi83 recently, any help would be appreciated. On a side note,
> I've tested the ti-sn65dsi83 driver on a v5.10 RPi kernel, without much
> success (on top of commit e1499baa0b0c I get a very weird frame rate -
> 147 fps of 99 fps instead of 60 fps - and nothing on the screen, and on
> top of the latest v5.10 RPi branch, I get lock-related warnings at every
> page flip), which is why I tried v5.12 and noticed this patch. Is it
> worth trying to bring up the display on the v5.10 RPi kernel in parallel
> to fixing the issue introduced in this patch, or is DSI known to be
> broken there ?

I've been looking at SN65DSI83/4, but as I don't have any hardware
I've largely been suggesting things to try to those on the forums who
do [1].

My branch at https://github.com/6by9/linux/tree/rpi-5.10.y-sn65dsi8x-marek
is the latest one I've worked on. It's rpi-5.10.y with Marek's driver
cherry-picked, and an overlay and simple-panel definition by others.
It also has a rework for vc4_dsi to use pm_runtime, instead of
breaking up the DSI bridge chain (which is flawed as it never calls
the bridge mode_set or mode_valid functions which sn65dsi83 relies
on).

I ran it on Friday in the lab and encountered an issue with vc4_dsi
should vc4_dsi_encoder_mode_fixup wish for a divider of 7 (required
for this 800x1280 panel over 4 lanes) where it resulted in an invalid
mode configuration. That resulted in patch [2] which then gave me
sensible numbers.

That branch with dtoverlay=vc4-kms-v3d and
dtoverlay=vc4-kms-dsi-ti-sn65dsi83 created all the expected devices,
and everything came up normally.
It was a busy day, but I think I even stuck a scope on the clock lanes
at that point and confirmed that they were at the link frequency
expected.


Coming back to this patch though, it isn't in 5.10 so I'm not seeing
the issues. As to the exact ordering of attaches, I can't claim
sufficient knowledge on that front.
I can try a cherry-pick of this patch to see what goes on, but it
won't be for a day or two.

Hope that helps
  Dave

[1] Largely https://www.raspberrypi.org/forums/viewtopic.php?f=44&t=305690,
but ignore about the first 5 pages of the thread as different driver
versions were floating about. Most stuff after that is based on
Marek's driver.
[2] https://github.com/6by9/linux/commit/c3c774136a1e946109048711d16974be8d520aaa

> On Tue, Jul 07, 2020 at 12:19:12PM +0200, Maxime Ripard wrote:
> > If the DSI driver is the last to probe, component_add will try to run all
> > the bind callbacks straight away and return the error code.
> >
> > However, since we depend on a power domain, we're pretty much guaranteed to
> > be in that case on the BCM2711, and are just lucky on the previous SoCs
> > since the v3d also depends on that power domain and is further in the probe
> > order.
> >
> > In that case, the DSI host will not stick around in the system: the DSI
> > bind callback will be executed, will not find any DSI device attached and
> > will return EPROBE_DEFER, and we will then remove the DSI host and ask to
> > be probed later on.
> >
> > But since that host doesn't stick around, DSI devices like the RaspberryPi
> > touchscreen whose probe is not linked to the DSI host (unlike the usual DSI
> > devices that will be probed through the call to mipi_dsi_host_register)
> > cannot attach to the DSI host, and we thus end up in a situation where the
> > DSI host cannot probe because the panel hasn't probed yet, and the panel
> > cannot probe because the DSI host hasn't yet.
> >
> > In order to break this cycle, let's wait until there's a DSI device that
> > attaches to the DSI host to register the component and allow to progress
> > further.
> >
> > Suggested-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/vc4/vc4_dsi.c | 25 ++++++++-----------------
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > index eaf276978ee7..19aab4e7e209 100644
> > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > @@ -1246,10 +1246,12 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
> >       return ret;
> >  }
> >
> > +static const struct component_ops vc4_dsi_ops;
> >  static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
> >                              struct mipi_dsi_device *device)
> >  {
> >       struct vc4_dsi *dsi = host_to_dsi(host);
> > +     int ret;
> >
> >       dsi->lanes = device->lanes;
> >       dsi->channel = device->channel;
> > @@ -1284,6 +1286,12 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
> >               return 0;
> >       }
> >
> > +     ret = component_add(&dsi->pdev->dev, &vc4_dsi_ops);
> > +     if (ret) {
> > +             mipi_dsi_host_unregister(&dsi->dsi_host);
> > +             return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -1662,7 +1670,6 @@ static int vc4_dsi_dev_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct vc4_dsi *dsi;
> > -     int ret;
> >
> >       dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> >       if (!dsi)
> > @@ -1670,26 +1677,10 @@ static int vc4_dsi_dev_probe(struct platform_device *pdev)
> >       dev_set_drvdata(dev, dsi);
> >
> >       dsi->pdev = pdev;
> > -
> > -     /* Note, the initialization sequence for DSI and panels is
> > -      * tricky.  The component bind above won't get past its
> > -      * -EPROBE_DEFER until the panel/bridge probes.  The
> > -      * panel/bridge will return -EPROBE_DEFER until it has a
> > -      * mipi_dsi_host to register its device to.  So, we register
> > -      * the host during pdev probe time, so vc4 as a whole can then
> > -      * -EPROBE_DEFER its component bind process until the panel
> > -      * successfully attaches.
> > -      */
> >       dsi->dsi_host.ops = &vc4_dsi_host_ops;
> >       dsi->dsi_host.dev = dev;
> >       mipi_dsi_host_register(&dsi->dsi_host);
> >
> > -     ret = component_add(&pdev->dev, &vc4_dsi_ops);
> > -     if (ret) {
> > -             mipi_dsi_host_unregister(&dsi->dsi_host);
> > -             return ret;
> > -     }
> > -
> >       return 0;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart