Re: [PATCH 04/11] drm/panel: merge the drm_kms_helper module into the drm module

[Maxime Ripard]

On Wed, Sep 02, 2026 at 03:44:55PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Wed Sep 2, 2026 at 2:36 PM CEST, Maxime Ripard wrote:
> > On Tue, Sep 01, 2026 at 04:30:40PM +0200, Luca Ceresoli wrote:
> >> On Tue Sep 1, 2026 at 2:40 PM CEST, Luca Ceresoli wrote:
> >>
> >> [...]
> >>
> >> >>> >> > And we'd essentially move drm_panel_bridge into drm_panel.c, and make it
> >> >>> >> > private.
> >> >>> >>
> >> >>> >> Yes in theory, but the panel_bridge code uses other parts of the
> >> >>> >> drm_kms_helper module: drm_atomic_helper and drm_probe_helper, maybe more,
> >> >>> >> so we'd have to move them into the drm module too.
> >> >>> >
> >> >>> > Ah, right. What would happen if we were doing it the other way around
> >> >>> > then? Move drm_panel out of the main drm module?
> >> >>>
> >> >>> Into the drm_kms_helper module?
> >> >>>
> >> >>> I had a look and did some experiments and I found at least one user in the drm
> >> >>> module calling a drm_panel API, and guess who:
> >> >>>
> >> >>>  drm_of_find_panel_or_bridge() (in drm_of.c, drm module)
> >> >>>     -> calls of_drm_find_panel (in drm_panel.c, drm_kms_helper module)
> >> >>>
> >> >>> Based on our discussion after patch 3, I'm not sure
> >> >>> drm_of_find_panel_or_bridge() will disappear soon. If it doesn't, I guess
> >> >>> we can try to move drm_of_find_panel_or_bridge() into bridge/panel.c which
> >> >>> is in the drm_kms_helper module (and from drm_of.h info
> >> >>> drm_bridge.h?). That however might trigger build failures for drivers which
> >> >>> currently don't select DRM_KMS_HELPER and which would have to select it.
> >> >>>
> >> >>> I'll give it a try, and if I see major drawbacks I will get back to moving
> >> >>> all the drm_kms_helper code into the main drm module.
> >> >>
> >> >> No, I meant into its own module.
> >> >
> >> > Looks like a good idea indeed, making the design more modular and
> >> > dependencies cleaner.
> >> >
> >> >> Do we have any dependency from the main
> >> >> drm module into drm_panel?
> >> >
> >> > As far as I can see there is only the one mentioned above:
> >> >
> >> >   drm_of_find_panel_or_bridge() (in drm_of.c, drm module)
> >> >      -> calls of_drm_find_panel (in drm_panel.c, would-be the new drm_panel module)
> >> >
> >> > And while drm_of_find_panel_or_bridge() is possibly going to disappear at
> >> > the end of my series, moving drm_panel.c to its own module would make the
> >> > series non-build-bisectable, which would be very annoying.
> >> >
> >> > However there seem to be no calls from the drm module to
> >> > drm_of_find_panel_or_bridge(), so we could move it out as well. What about
> >> > a series doing, in this order:
> >> >
> >> >  - move drm_of_find_panel_or_bridge() from the drm module to the
> >> >    drm_kms_helper module
> >> >  - move drm_panel.c to a new drm_panel module
> >> >  - Main change: embed a drm_bridge into every panel
> >> >  - convert drivers to stop using the panel_bridge, hopefully removing all
> >> >    calls to drm_of_find_panel_or_bridge()
> >> >  - remove drm_of_find_panel_or_bridge()
> >>
> >> Ah, no, that won't work. There would be a circular module dependency loop
> >> later on when we embed a drm_bridge into evern drm_panel: indeed at that
> >> point drm_panel will use the atomic and probe helpers to implement the
> >> embedded drm_bridge, resulting in:
> >>
> >>  * the panel_bridge code in bridge/panel.c [drm_kms_helper module]
> >>    already depends on the drm_panel.c code (it manipulates a drm_panel, OK)
> >>  * additionally, the drm_panel.c code, in order to create a drm_bridge,
> >>    will depend on the helpers in drm_atomic_helper and drm_probe_helper
> >>    code [drm_kms_helper module]
> >>
> >> The loop is only between kernel modules (.ko), not in actual code. So I
> >> think this revised plan should work (the 2nd bullet is key):
> >>
> >>  - move drm_of_find_panel_or_bridge() from the drm module to
> >>    bridge/panel.c [currently drm_kms_helper module]
> >>  - move bridge/panel.o to a new drm_panel_bridge module (NEW)
> >>  - move drm_panel.c to a new drm_panel module
> >
> > Looks good on principle, but iirc the starting point of that discussion
> > was to move bridge panel into the new panel module,
> 
> That was the starting point, but working on it I realized it was very
> tricky. So I tried instead copying the relevant bridge/panel.c code into
> drm_panel.c, but with more suitable function names, and the result is way
> cleaner.

I don't see the difference, but I guess that's a good thing anyway :)

> This changed the steps, but not the end result: remove
> drm_of_find_panel_or_bridge() and bridge/panel.c entirely.
> 
> > so I guess we could:
> >
> > 1) Move drm_of_find_panel_or_bridge() to drm_panel.c. Both are still in
> > the drm module at this point, so it should be ok.
> > 2) Create a new panel module, with a dependency on bridge
> > 3) Move the bridge/panel.c code into the new panel module to create the
> > bridge at the same time we create the panel.
> >
> > Would that work?
> 
> Maybe, but as above, my plan has changed a bit.
> 
> My opinion is that for this tricky task we should focus more on the end
> result (embed a bridge into every panel, remove
> drm_of_find_panel_or_bridge() and bridge/panel.c) than on finding the most
> optimal path to get there. So I'm aiming at a path that is
> build-bisectable, clean and understandable.
> 
> What do you think about this principle?

I somewhat agree, the somewhat being I don't think we should go for that
end result at all cost. We don't know how long it's going to complete,
how much you're going to be involved in this going forward, etc. so I
think the intermediate steps should be reasonable (even if not perfect)
and ok to carry for an indefinite amount of time.

Maxime

Attachment: signature.asc
Description: PGP signature