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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hi Maxime,

On Mon Jul 20, 2026 at 4:28 PM CEST, Maxime Ripard wrote:
> 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.

I fully agree.

> 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.

A few changes to each bridge wanting to support hotplug will unavoidably be
needed. The .get_next_bridge callback we mentioned in the discussion for
patch 30 at least. I'm keeping any other changes, if any, to a minimum.

>>  - 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.

That's the idea I had reached too, yes. Except the "bunch of helpers and
hooks" could be perhaps as small as one single helper function or little
more.

> 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.

Good.

> 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.

OK, let's see what I can come up with in v2.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com