Re: [PATCH v3 2/8] drm/bridge: Document the probe issue with MIPI-DSI bridges

[Maxime Ripard]

Hi Andrzej,

On Mon, Aug 23, 2021 at 06:32:11PM +0200, Andrzej Hajda wrote:
> Hi Maxime,
> 
> On 23.08.2021 10:47, Maxime Ripard wrote:
> 
> > Interactions between bridges, panels, MIPI-DSI host and the component
> > framework are not trivial and can lead to probing issues when
> > implementing a display driver. Let's document the various cases we need
> > too consider, and the solution to support all the cases.
> >
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > ---
> >   Documentation/gpu/drm-kms-helpers.rst |  6 +++
> >   drivers/gpu/drm/drm_bridge.c          | 58 +++++++++++++++++++++++++++
> >   2 files changed, 64 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> > index 10f8df7aecc0..ec2f65b31930 100644
> > --- a/Documentation/gpu/drm-kms-helpers.rst
> > +++ b/Documentation/gpu/drm-kms-helpers.rst
> > @@ -157,6 +157,12 @@ Display Driver Integration
> >   .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> >      :doc: display driver integration
> >   
> > +Special Care with MIPI-DSI bridges
> > +----------------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> > +   :doc: special care dsi
> > +
> >   Bridge Operations
> >   -----------------
> >   
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index baff74ea4a33..794654233cf5 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -96,6 +96,64 @@
> >    * documentation of bridge operations for more details).
> >    */
> >   
> > +/**
> > + * DOC: special care dsi
> > + *
> > + * The interaction between the bridges and other frameworks involved in
> > + * the probing of the display driver and the bridge driver can be
> > + * challenging. Indeed, there's multiple cases that needs to be
> > + * considered:
> > + *
> > + * - The display driver doesn't use the component framework and isn't a
> > + *   MIPI-DSI host. In this case, the bridge driver will probe at some
> > + *   point and the display driver should try to probe again by returning
> > + *   EPROBE_DEFER as long as the bridge driver hasn't probed.
> > + *
> > + * - The display driver doesn't use the component framework, but is a
> > + *   MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be
> > + *   controlled. In this case, the bridge device is a child of the
> > + *   display device and when it will probe it's assured that the display
> > + *   device (and MIPI-DSI host) is present. The display driver will be
> > + *   assured that the bridge driver is connected between the
> > + *   &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations.
> > + *   Therefore, it must run mipi_dsi_host_register() in its probe
> > + *   function, and then run drm_bridge_attach() in its
> > + *   &mipi_dsi_host_ops.attach hook.
> > + *
> > + * - The display driver uses the component framework and is a MIPI-DSI
> > + *   host. The bridge device uses the MIPI-DCS commands to be
> > + *   controlled. This is the same situation than above, and can run
> > + *   mipi_dsi_host_register() in either its probe or bind hooks.
> > + *
> > + * - The display driver uses the component framework and is a MIPI-DSI
> > + *   host. The bridge device uses a separate bus (such as I2C) to be
> > + *   controlled. In this case, there's no correlation between the probe
> > + *   of the bridge and display drivers, so care must be taken to avoid
> > + *   an endless EPROBE_DEFER loop, with each driver waiting for the
> > + *   other to probe.
> > + *
> > + * The ideal pattern to cover the last item (and all the others in the
> > + * display driver case) is to split the operations like this:
> > + *
> > + * - In the display driver must run mipi_dsi_host_register() and
> > + *   component_add in its probe hook. It will make sure that the
> > + *   MIPI-DSI host sticks around, and that the driver's bind can be
> > + *   called.
> 
> I guess component_add is leftover from previous iteration (as you wrote 
> few lines below) component_add should be called from dsi host attach 
> callback.

Indeed, I'll remove it

> > + *
> > + * - In its probe hook, the bridge driver must try to find its MIPI-DSI
> > + *   host, register as a MIPI-DSI device and attach the MIPI-DSI device
> > + *   to its host. The bridge driver is now functional.
> > + *
> > + * - In its &struct mipi_dsi_host_ops.attach hook, the display driver
> > + *   can now add its component. Its bind hook will now be called and
> > + *   since the bridge driver is attached and registered, we can now look
> > + *   for and attach it.
> > + *
> > + * At this point, we're now certain that both the display driver and the
> > + * bridge driver are functional and we can't have a deadlock-like
> > + * situation when probing.
> > + */
> > +
> 
> Beside small mistake the whole patch looks OK for me. Maybe it would be 
> worth to mention what is the real cause of this "special DSI case" - 
> there is mutual dependency between two following entities in display chain:
> 
> 1. display driver - it provides DSI bus, and requires drm_bridge or 
> drm_panel provided by child device.
> 
> 2. bridge or panel with DSI transport - it requires DSI bus provided by 
> display driver, and provides drm_bridge or drm_panel interface required 
> by display driver.

I was trying to explain it in the first part of this patch. Is there
anything misleading there?

> I guess similar issues can appear with other data/control bus-es, 
> apparently DSI case is the most common.

The issue only presents itself when it's using a separate control bus
actually. If it's controlled through DCS, the panel / bridge will be a
children node of the DSI host and will only be probed when the host is
registered, so we don't have this issue.

> And one more thing - you use "display driver" term but this is also case 
> of any bridge providing DSI bus - there are already 3 such bridges in 
> kernel - cdns, nwl, synopsys, tc358768, maybe "DSI host" would be better 
> term.

Good point, I'll change it.

> And another thing - downstream device can be bridge or *panel*, it would 
> be good to mention that panels also should follow this pattern.

We're pretty much forced to do this with panels though. They don't have
an attach hook unlike bridges so we don't have much other options than
putting it in probe.

> Btw this is another place where word bridge can be 1:1 replaced by word 
> panel - it clearly suggest that DRM subsystem waits for brave men who 
> proposes patches unifying them, we would save lot of words, and lines of 
> code if we could use drm_sink instead of "if (sink is bridge) do sth 
> else do sth-similar-but-with-drm_panel-interface".

I agree. In the previous iteration I had this patch that was a step in
this direction:
https://lore.kernel.org/dri-devel/20210728133229.2247965-3-maxime@xxxxxxxxxx/

Even though it's not relevant to this series anymore, I still plan on
submitting it and converting as many users as possible (if my coccinelle
skills allows me to at least).

Maxime

Attachment: signature.asc
Description: PGP signature