Re: [PATCH v6 08/26] drm/bridge: panel: add a panel_bridge to every panel

[Chen-Yu Tsai <wenst@chromium.org>]

On Tue, Feb 11, 2025 at 2:34 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote:
> > Adding a panel does currently not add a panel_bridge wrapping it. Usually
> > the panel_bridge creation happens when some other driver (e.g. the previous
> > bridge or the encoder) calls *_of_get_bridge() and the following element in
> > the pipeline is a panel.
> >
> > This has some drawbacks:
> >
> >  * the panel_bridge is not created in the context of the driver of the
> >    underlying physical device (the panel driver), but of some other driver
> >  * that "other driver" is not aware of whether the returned drm_bridge
> >    pointer is a panel_bridge created on the fly, a pre-existing
> >    panel_bridge or a non-panel bridge
> >  * removal of a panel_bridge requires calling drm_panel_bridge_remove(),
> >    but the "other driver" doesn't know whether this is needed because it
> >    doesn't know whether it has created a panel_bridge or not
> >
> > So far this approach has been working because devm and drmm ensure the
> > panel bridge would be dealloacted at some later point. However with the
> > upcoming implementation of dynamic bridge lifetime this will get more
> > complicated.
> >
> > Correct removal of a panel_bridge might possibly be obtained by adding more
> > devm/drmm technology to have it freed correctly at all times. However this
> > would add more complexity and not help making lifetime more understandable.
> >
> > Use a different approach instead: always create a panel_bridge with a
> > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge
> > wrapping it. This makes lifetime much more straightforward to understand
> > and to further develop on.
> >
> > With the panel_bridge always created, the functions to get a bridge
> > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because
> > the bridge they are looking for exists already (if it can exist at all). In
> > turn, this is implemented based on a variant of
> > drm_of_find_panel_or_bridge() that only looks for panels:
> > of_drm_find_bridge_by_endpoint(). In the future
> > drm_of_find_panel_or_bridge() can be progressively removed because there
> > will never be a panel not exposing a bridge.
> >
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >
> > ---
> >
> > This patch was added in v6.
> > ---
> >  drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++---------
> >  include/drm/drm_panel.h        |  8 +++++
> >  2 files changed, 66 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > index 58570ff6952ca313b3def084262c9bb3772272ef..6995de605e7317dd1eb153afd475746ced764712 100644
> > --- a/drivers/gpu/drm/bridge/panel.c
> > +++ b/drivers/gpu/drm/bridge/panel.c
> > @@ -69,6 +69,9 @@ EXPORT_SYMBOL(drm_panel_init);
> >   */
> >  void drm_panel_add(struct drm_panel *panel)
> >  {
> > +     panel->bridge = drm_panel_bridge_add(panel);
> > +     WARN_ON(!panel->bridge);
> > +
> >       mutex_lock(&panel_lock);
> >       list_add_tail(&panel->list, &panel_list);
> >       mutex_unlock(&panel_lock);
> > @@ -86,6 +89,9 @@ void drm_panel_remove(struct drm_panel *panel)
> >       mutex_lock(&panel_lock);
> >       list_del_init(&panel->list);
> >       mutex_unlock(&panel_lock);
> > +
> > +     drm_panel_bridge_remove(panel->bridge);
> > +     panel->bridge = NULL;
> >  }
> >  EXPORT_SYMBOL(drm_panel_remove);
>
> Given that drm_panel_add and drm_panel_remove are typically called at
> probe/remove, it's pretty much equivalent to using devm. Both of these
> solutions aren't safe, and the drm_panel lifetime is still broken.

FWIW I believe this solves the panel vs panel_bridge lifetime
inconsistencies we previously reported [1]. Of course, as you rightly
point out, any pointers to the bridge become stale if the panel gets
removed.

> I'd rather work on a solution that actually fixes those lifetime issues.

I think that can happen once the bridges are ref-counted?

Instead of removing the bridge from the list, it can just clear the
panel pointer and have all the callbacks skip any operations involving
the panel.

The other option is to have the panel itself be ref-counted. I don't
think that's worth pursuing if the idea is to move everything over to
panel_bridge and things are somewhat ready.

ChenYu

[1] https://lore.kernel.org/dri-devel/20241009052402.411978-1-fshao@chromium.org/