Re: [PATCH 1/7] drm/encoder: add mutex to protect the bridge chain

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

Hi Maxime,

On Mon, 29 Sep 2025 14:43:46 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Sep 26, 2025 at 05:59:42PM +0200, Luca Ceresoli wrote:
> > The per-encoder bridge chain is currently assumed to be static once it is
> > fully initialized. Work is in progress to add hot-pluggable bridges,
> > breaking that assumption.
> > 
> > With bridge removal, the encoder chain can change without notice, removing
> > tail bridges. This can be problematic while iterating over the chain.
> > 
> > Add a mutex to be taken whenever looping or changing the encoder chain.
> > 
> > Also add two APIs to lock/unlock the mutex without the need to manipulate
> > internal struct drm_encoder fields.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_encoder.c |  2 ++
> >  include/drm/drm_encoder.h     | 18 ++++++++++++++++++
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index 8f2bc6a28482229fd0b030a1958f87753ad7885f..3261f142baea30c516499d23dbf8d0acf5952cd6 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -129,6 +129,7 @@ static int __drm_encoder_init(struct drm_device *dev,
> >  	}
> >  
> >  	INIT_LIST_HEAD(&encoder->bridge_chain);
> > +	mutex_init(&encoder->bridge_chain_mutex);
> >  	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
> >  	encoder->index = dev->mode_config.num_encoder++;
> >  
> > @@ -202,6 +203,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> >  	kfree(encoder->name);
> >  	list_del(&encoder->head);
> >  	dev->mode_config.num_encoder--;
> > +	mutex_destroy(&encoder->bridge_chain_mutex);
> >  
> >  	memset(encoder, 0, sizeof(*encoder));
> >  }
> > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > index 977a9381c8ba943b4d3e021635ea14856df8a17d..6c962de640a345bfbb18308c83076628547c9ab9 100644
> > --- a/include/drm/drm_encoder.h
> > +++ b/include/drm/drm_encoder.h
> > @@ -25,6 +25,7 @@
> >  
> >  #include <linux/list.h>
> >  #include <linux/ctype.h>
> > +#include <linux/mutex.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_mode.h>
> >  #include <drm/drm_mode_object.h>
> > @@ -189,6 +190,9 @@ struct drm_encoder {
> >  	 */
> >  	struct list_head bridge_chain;
> >  
> > +	/** @bridge_chain_mutex: protect bridge_chain from changes while iterating */
> > +	struct mutex bridge_chain_mutex;
> > +
> >  	const struct drm_encoder_funcs *funcs;
> >  	const struct drm_encoder_helper_funcs *helper_private;
> >  
> > @@ -319,6 +323,20 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
> >  	return mo ? obj_to_encoder(mo) : NULL;
> >  }
> >  
> > +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
> > +{
> > +	if (!WARN_ON_ONCE(!encoder))
> > +		mutex_lock(&encoder->bridge_chain_mutex);
> > +
> > +	return encoder;
> > +}
> > +
> > +static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
> > +{
> > +	if (!WARN_ON_ONCE(!encoder))
> > +		mutex_unlock(&encoder->bridge_chain_mutex);
> > +}
> > +  
> 
> Please provide documentation for these two functions.

Ah, sure, here is a draft:

/**
 * drm_encoder_chain_lock - lock the encoder bridge chain
 * @encoder: encoder whose bridge chain must be locked
 *
 * Locks the mutex protecting the bridge chain from concurrent access.
 * To be called by code modifying ot iterating over the bridge chain to
 * prevent the list from changing while iterating over it.
 * Call drm_encoder_chain_unlock() to unlock the mutex.
 *
 * Returns:
 * Pointer to @encoder. Useful to lock the chain and then operate on the
 * in the same statement, e.g.
 * list_first_entry(&drm_encoder_chain_lock(encoder)->bridge_chain)
 */
static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
{
	if (!WARN_ON_ONCE(!encoder))
		mutex_lock(&encoder->bridge_chain_mutex);

	return encoder;
}

/**
 * drm_encoder_chain_unlock - unlock the encoder bridge chain
 * @encoder: encoder whose bridge chain must be unlocked
 *
 * Unlocks the mutex protecting the bridge chain from concurrent access,
 * matching drm_encoder_chain_lock().
 */
static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
{
	if (!WARN_ON_ONCE(!encoder))
		mutex_unlock(&encoder->bridge_chain_mutex);
}

> In particular, why
> do we need to return the encoder pointer?

It allows to lock the encoder chain and then dereference the pointer,
similar to what drm_bridge_get() does. of_node_get(), which I used as a
model for these functions, does the same.

This one in particular is used in patch 4 to write the new scoped
macros. Locking the encoder chain and then dereferencing the encoder
would be very awkward in a macro if drm_encoder_chain_lock() didn't
return the encoder.

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