On Fri, 7 Feb 2025 04:49:21 +0200
Dmitry Baryshkov <dmitry.baryshkov@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(-)
> >
>
> LGTM, minor issue below.
>
> > @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
> > {
> > struct drm_bridge **ptr, *bridge;
> >
> > + if (panel->bridge) {
> > + DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge);
> > + return panel->bridge;
> > + }
>
> Shouldn't the rest of the function also be removed as you do in other
> cases?
Indeed it should.
And even more, I now realize drm_panel_bridge_add_typed() should also
become a simple 'return panel->bridge', like its devm and drmm
variants, and its code, implementing the actual creation of a panel
bridge, move to an internal function. Otherwise this patch is a bug:
existing drivers which do call drm_panel_bridge_add_typed() would end
up in having two panel_bridges for the same panel.
I must say the process of developing this patch together with the
hotplug work was "convoluted" to say the least, which probably explains
why this got unnoticed so far.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
