Re: [PATCH 23/37] drm/encoder: add drm_encoder_cleanup_from()

[Maxime Ripard]

On Tue, Jun 09, 2026 at 12:10:39PM +0200, Luca Ceresoli wrote:
> On Mon Jun 8, 2026 at 2:10 PM CEST, Maxime Ripard wrote:
> > On Tue, May 19, 2026 at 12:37:40PM +0200, Luca Ceresoli wrote:
> >> Supporting hardware whose final part of the DRM pipeline can be physically
> >> removed requires the ability to detach all bridges from a given point to
> >> the end of the pipeline.
> >>
> >> Introduce a variant of drm_encoder_cleanup() for this.
> >>
> >> Take care to not try detaching non-attached bridges. This is needed because
> >> when two or more bridges are removed not in the backwards order,
> >> drm_encoder_cleanup_from() is called more than once for bridges closer to
> >> the panel.
> >>
> >> Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
> >>
> >> ---
> >>
> >> Note: in theory drm_encoder_cleanup() is now a superset of
> >> drm_encoder_cleanup_from() and may be simplified to just call
> >> drm_encoder_cleanup_from() and then do the extra actions. However the
> >> common code is subtly different in terms of locking and checks, so this
> >> would complicate the code in this patch and has thus been kept separate for
> >> the time being to make reviewing sompler. Reimplementing
> >> drm_encoder_cleanup() by using drm_encoder_cleanup_from() cvacn be done
> >> later on.
> >>
> >> A much simpler and now obsolete version of this patch (missing locking and
> >> checks) previously appeared in
> >> https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-13-9d6f2c9c3058@xxxxxxxxxxx/
> >> ---
> >>  drivers/gpu/drm/drm_encoder.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_encoder.h     |  1 +
> >>  2 files changed, 39 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> >> index 0d5dbed06db4..40ece477b302 100644
> >> --- a/drivers/gpu/drm/drm_encoder.c
> >> +++ b/drivers/gpu/drm/drm_encoder.c
> >> @@ -179,6 +179,44 @@ int drm_encoder_init(struct drm_device *dev,
> >>  }
> >>  EXPORT_SYMBOL(drm_encoder_init);
> >>
> >> +/**
> >> + * drm_encoder_cleanup_from - remove a given bridge and all the following
> >> + * @encoder: encoder whole list of bridges shall be pruned
> >> + * @bridge: first bridge to remove
> >> + *
> >> + * Removes from an encoder all the bridges starting with a given bridge
> >> + * and until the end of the chain.
> >> + *
> >> + * Does nothing if the bridge is not attached to an encoder chain.
> >> + *
> >> + * This should not be used in "normal" DRM pipelines. It is only useful for
> >> + * devices whose final part of the DRM chain can be physically removed and
> >> + * later reconnected (possibly with different hardware).
> >> + */
> >> +void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge)
> >> +{
> >> +	struct drm_bridge *next;
> >> +	LIST_HEAD(tmplist);
> >> +
> >> +	/*
> >> +	 * We need the bridge_chain_mutex to modify the chain, but
> >> +	 * drm_bridge_detach() will call DRM_MODESET_LOCK_ALL_BEGIN() (in
> >> +	 * drm_modeset_lock_fini()), resulting in a possible ABBA circular
> >> +	 * deadlock. Avoid it by first moving all the bridges to a
> >> +	 * temporary list holding the lock, and then calling
> >> +	 * drm_bridge_detach() without the lock.
> >> +	 */
> >> +	mutex_lock(&encoder->bridge_chain_mutex);
> >> +	if (!list_empty(&bridge->chain_node))
> >> +		list_for_each_entry_safe_from(bridge, next, &encoder->bridge_chain, chain_node)
> >> +			list_move_tail(&bridge->chain_node, &tmplist);
> >> +	mutex_unlock(&encoder->bridge_chain_mutex);
> >> +
> >> +	while (!list_empty(&tmplist))
> >> +		drm_bridge_detach(list_first_entry(&tmplist, struct drm_bridge, chain_node));
> >> +}
> >> +EXPORT_SYMBOL(drm_encoder_cleanup_from);
> >> +
> >
> > The name is super confusing, because it doesn't clean up anything.
> > drm_encoder_cleanup is called that way because it cleans up the encoder.
> > This function doesn't.
> >
> > Unlike what's being documented, it doesn't remove any bridge either. It
> > just detaches bridge, so let's just call it that way?
> 
> Good point.
> 
> What about:
> 
>  * rename to drm_bridge_detach_from()
>  * drop the @encoder argument (get if from bridge->encoder)
>  * maybe even move to drm_bridge.c?

Yeah, sounds good
Maxime

Attachment: signature.asc
Description: PGP signature