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

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

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@bootlin.com>
>>
>> ---
>>
>> 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@bootlin.com/
>> ---
>>  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?

Luca

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