On Tue, Feb 18, 2025 at 05:43:43PM +0800, Chen-Yu Tsai wrote:
> On Tue, Feb 11, 2025 at 2:34 AM Maxime Ripard <mripard@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > >
> > > ---
> > >
> > > 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?
Not all panel users use a bridge, some are using the panel API directly.
> 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.
Yeah, maybe. I'm kind of skeptical it would be easier than just doing
the work we're doing here though. Most of the "easy" users have been
converted already, and only the problematic ones remain, which will be
difficult to move over. sun4i_rgb and sun6i_dsi, for example, won't be
trivial to switch to panel_bridge. And given that sun4i_rgb is mostly
obsolete, and sun6i_dsi unmaintainable, I don't think it's likely to
happen.
So I don't think it's fair to expect that work from such a series, or to
rely on it eventually being fixed.
I think a solution where we have a bridge in drm_panel is a better
solution (and easier to convert to), but that requires a new panel
allocation API too. We've started to work on it as well, so everything
might just click.
Maxime
Attachment:
signature.asc
Description: PGP signature
