On Tue, Jan 21, 2025 at 12:27:18PM +0100, Luca Ceresoli wrote:
> Hi Dmitry,
>
> On Thu, 16 Jan 2025 12:56:25 +0200
> Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> [...]
>
> > > Idea 3:
> > >
> > > The idea is that if the panel driver framework always creates a panel
> > > bridge, it will never need to be created on the fly automagically by
> > > its consumers, so the whole problem would disappear. It also would be
> > > better modeling the hardware: still wrapping a panel with a drm_bridge
> > > that does not exist in the hardware, but at least having it created by
> > > the provider driver and not by the consumer driver which happens to
> > > look for it.
> >
> > I think this is the best option up to now.
>
> Thanks for sharing your opinion. However a few points are not clear to
> me, see below.
>
> > > This looks like a promising and simple idea, so I tried a quick
> > > implementation:
> > >
> > > void drm_panel_init(struct drm_panel *panel, struct device *dev,
> > > const struct drm_panel_funcs *funcs, int connector_type)
> > > {
> > > + struct drm_bridge *bridge;
> > > +
> > > INIT_LIST_HEAD(&panel->list);
> > > INIT_LIST_HEAD(&panel->followers);
> > > mutex_init(&panel->follower_lock);
> > > panel->dev = dev;
> > > panel->funcs = funcs;
> > > panel->connector_type = connector_type;
> > > +
> > > + bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> > > + WARN_ON(!bridge);
> > > }
> > >
> > > This is somewhat working but it requires more work because:
> > >
> > > * as it is, it creates a circular dependency between drm_panel and the
> > > panel bridge, and modular builds will fail:
> > >
> > > depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> > >
> > > * The panel bridge implementation should be made private to the panel
> > > driver only (possibly helping to solve the previous issue?)
> >
> > Or merge drm_panel.c into bridge/panel.c.
>
> Not sure here you mean exactly what you wrote, or the other way around.
> It looks more correct to me that the panel core (drm_panel.c) starts
> exposing a bridge now, and not that the panel bridge which is just one
> of many bridge drivers starts handling all the panel-related stuff.
No, I actually meant what I wrote: merge drm_panel.c into
bridge/panel.c. Indeed we have some drivers that use panel directly.
However drm_bridge is a more generic interface. So, yes, I propose to
have a bridge driver which incorporates panel support.
>
> > The panel bridge already
> > exports non-standard API, so it should be fine to extend / change that
> > API. Likewise we might move drm_panel.c to drm_kms_helper.o, also
> > resolving the loop.
>
> Again I'm not following I'm afraid. It would seem more logical to me to
> move things from the helper into drm.o as they become more necessary
> for common cases, rather than moving things out to a helper module and
> then force all panel drivers to depend on the helper module.
There are a lot of DRM drivers which do not use panels (nor bridges).
Merging that code into drm.ko means that everybody ends up having that
piece of code in memory. Moving drm_panel.o out of drm.ko and
bridge/panel.o out of drm_kms_helper.ko would make the kernel slightly
smaller for those desktop-only users.
>
> Apologies, I'm perhaps not following your reasoning here. :(
>
> > There will be a need for a Kconfig update in both
> > cases.
> >
> > > * Many drivers currently call devm_drm_panel_bridge_add() directly,
> > > they should probably call drm_of_get_bridge instead
> >
> > I'd say, make it a stub that calls drm_of_get_bridge() with a possible
> > deprecation warning.
>
> I get the idea, but it would need to be implemented differently because
> drm_of_get_bridge() calls devm_drm_panel_bridge_add(). They would loop
If the drm_bridge is added during drm_panel_add(), then there is no need
to call devm_drm_panel_bridge_add() from drm_of_get_bridge().
> into each other. I think we might simply add a check at the beginning
> of drm_panel_bridge_add_typed(), as in (pseudocode):
>
> drm_panel_bridge_add_typed(struct drm_panel *panel, ...)
> {
> if (panel_has_a_panel_bridge(panel))
> return panel->panel_bridge.bridge;
>
> // existing code
> }
>
> But that reopens the same issue: drm_panel_bridge_add_typed() would not
> always call drm_bridge_add(), so the caller doesn't know how to dispose
> of the returned pointer.
>
> I think idea 3 can only work if the code understands that the panel
> bridge is created by the panel and they _never_ have to create it
> themselves. So handling the transition for all drivers would be quite
> painful.
That's why I suggested just stubbing existing functions just to lookup
and return a bridge: drivers can transition one-by-one. The bridge will
be already there, created by the panel support code.
>
> > > * drm_of_find_panel_or_bridge() should disappear: other drivers would
> > > just look for a bridge
> >
> > Please keep it for now.
> >
> > Some of the drivers think that it returns literally panel-or-bridge (but
> > not both). However if panel bridge is already created, it will return
> > both. So those drivers need to be updated.
>
> I really believe it never returns both. The *bridge is set only when
> ret != 0 here [0], which can happen only if a panel was not found.
> Despite the slightly intricate logic of that function, I don't see how
> it could return both in its current implementation.
Indeed. You are right, I skipped the if(ret) condition. More boilerplate
code in the drivers :-(
>
> [0]
> https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/gpu/drm/drm_of.c#L273
>
> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
--
With best wishes
Dmitry
