On Tue, Jan 21, 2025 at 01:57:47PM +0200, Dmitry Baryshkov wrote:
> 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.
It's a legitimate subject to discuss, but I'm not sure it's worth
focusing on this at this point though.
We should probably split it out into smaller chunks. Figuring out the
drivers lifetime, and reference counting API is hard enough as it is,
throwing panels into the mix at this point just adds more complexity.
Maxime
Attachment:
signature.asc
Description: PGP signature
