Re: [PATCH 05/37] drm/display: bridge-connector: split code creating the connector to a subfunction

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5552 bytes --]

On Fri, Jul 17, 2026 at 11:41:33AM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Thu Jul 16, 2026 at 3:22 PM CEST, Maxime Ripard wrote:
> > On Thu, Jul 16, 2026 at 10:37:24AM +0200, Luca Ceresoli wrote:
> >> >> > Now if the bridges start doing it themselves we should go back to
> >> >> > those encoder drivers and ditch all the drm_bridge_connector from
> >> >> > there?
> >> >> >
> >> >> > I must be missing something. Can you elaborate on this?
> >> >>
> >> >> drm_bridge_connectors bring together a (complete) bridge chain and a
> >> >> connector. If you don't have either anymore, then we shouldn't keep it
> >> >> around.
> >> >>
> >> >> What I was suggesting before was only a suggestion. I guess we could
> >> >> also make the encoder own the hotplug handling code and create the
> >> >> drm_bridge_connector when the chain is complete, and remove it when it's
> >> >> no longer the case.
> >> >
> >> > That's an interesting option. We don't have to keep drm_bridge_connector
> >> > in its current form, but I don't think we should go back to individual
> >> > bridge driver creating connectors, especially now that we have bridge
> >> > chains where the connector ops are implemented collectively by multiple
> >> > bridges.
> >>
> >> I definitely agree we don't want to add burden back on the encoder.
> >
> > I don't think Laurent mentioned the encoder anywhere.
> 
> Ah, indeed, sorry! However, I think both the bridges and the encoder
> drivers should equally have the minimum burden on them.
> 
> Right now the recommended practice is:
> 
>  - bridges do not create connectors (thanks to DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>  - encoders just call drm_bridge_connector_init(), which does all the
>    common operations to populate a suitable drm_connector
> 
> So all common operations involved in connector creation and bridge chain
> analysis are implemented in common code, not per-bridge or
> per-encoder. That's good.

I agree, but another way to phrase it is: bridges aren't aware of how
the chain is setup, the encoder ties it all together.

> >> >> We can discuss alternatives too. But either way, we shouldn't have it
> >> >> stick around.
> >>
> >> Bottom line, I roughly see three ideas mentioned:
> >>
> >>  1. (this series) extend the drm_bridge_connector to create the
> >>     drm_connector based on bridge hotplug events [+rename it]
> >>  2. - keep the drm_bridge_connector (mostly) as is
> >>     - let each encoder driver add/remove it based on bridge hotplug events
> >>     => more burden on encoder drivers -> no
> >
> > Can you motivate that with *any* reason? Because I really feel like it's
> > the best solution going forward.
> 
> My understanding of your idea (maybe a bit overstressed just to ensure it's
> clear) is that:
> 
>  - the drm_bridge_connector should stay (almost) unmodified

Yes, and bridges should ideally remain as lightly affected as possible.
We have probably around 100 bridge drivers at the moment, having some
kind of opt-in to enable hotplug would mean that we can't expect hotplug
to work on a new platform, which should be a last resort.

>  - there should be no new "manager" component (not sure this is actually
>    your opinion, can you comment on this specifically?)
>  - every encoder driver would have to:
>    - register to receive hotplug events
>    - when receiving one such event, find out whether the hardware is
>      complete or not (by calling drm_bridge_connector_pipeline_is_complete()
>      or so)
>    - create/destroy a drm_bridge_connector based on hotplug events
> 
> Is this somewhat close to what you have in mind?

Yes. To make things a bit more precise, we need two things: the encoder
to put the chain together, and "something" (that you used to call
manager) to react to hotplug events and handle the bridge
detach/destruction, connector creation/destruction, etc and should stick
around when we enable hotplug.

What I'm suggesting is that, since the encoder already owns and creates
the chain in the first place, and is there forever, it's only natural
for the encoder to be that "something", and we don't necessarily mean
creating a new entity or piece of code. A bunch of helpers and hooks a
probably going to be enough.

This is where the opt-in part should be, and I'd like, if possible, for
hotplug-enabled encoders to work with any bridge.

> To me the best solution to add hotplug support is that encoder drivers
> replace the single drm_bridge_connector_init() call with a single call to
> something new (let's call it a hotplug manager), which takes care of all
> the common aspects: registering to receive bridge hotplug events, finding
> out whether the hardware pipeline is complete or not, and add/remove the
> drm_connector based on that.
> 
> In other words, the changes on encoder drivers would be similar to patch
> 37. In a nutshell:
> 
>   -	connector = drm_bridge_connector_init(lcdif->drm, encoder);
>   +	drm_hotplug_manager = drm_hotplug_manager_init(lcdif->drm, encoder);
> 
> All the hotplug logic would be in common code, and any maintenance and
> future improvements to it would stay in a single place, benefitting all
> encoders at once.
> 
> What do you think about this?

From a high level point-of-view, I think we mostly agree. We can argue
on the name, and if we should merge it with something else
(drm_encoder_init, drm_bridge_attach, something else?) but that's the
path forward I think.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]