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

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

Hi Dmitry,

On Tue Oct 7, 2025 at 11:45 AM CEST, Luca Ceresoli wrote:
> Hi Dmitry,
>
> On Sat Oct 4, 2025 at 11:47 AM CEST, Dmitry Baryshkov wrote:
>
>>> @@ -319,6 +323,41 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
>>>  	return mo ? obj_to_encoder(mo) : NULL;
>>>  }
>>>  
>>> +/**
>>> + * 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() when done 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_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain)
>>> + */
>>> +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
>>
>> What is the use case for these wrappers? I'm asking especially since
>> you almost never use the return value of the _lock() one. I think with
>> scoped_guard you can get the same kind of code without needing extra API
>> or extra wrappers.
>
> For two reasons.
>
> One is to avoid drm_encoder users to need to access internal fields
> (encapsulation, in object-oriented jargon). But if I read correctly between
> the lines of your question, it is not worth because drm_bridge and
> drm_encoder are already interdependent?
>
> The second is that the C language spec sets tight constraints to the
> drm_for_each_bridge_in_chain_scoped(). The macro must look like:
>
>   #define drm_for_each_bridge_in_chain_scoped(encoder, bridge) \
>           for (struct drm_bridge *bridge = <FOO>; clause-2; clause-3)
> 	       '----------- clause-1 ----------'
>
> clause-1 must:
>
>  * declare a 'struct drm_bridge *' variable (the loop cursor)
>  * initialize it via <FOO> which thus must be a rvalue of type
>    'struct drm_bridge *' (<FOO> must be a function or a macro, as a
>    variable with the correct value is not available)
>  * use the struct drm_encoder * as its sole input
>  * lock the encoder chain mutex
>  * get a reference to the bridge (as Maxime requested)
>  * ensure the bridge reference is put and the mutex is released on break
>    and return (clause-3 can't do that)
>
> Given the above, we still need a function that locks the encoder chain
> mutex and returns the encoder (bullets 3 and 4), like
> drm_encoder_chain_lock(). I'm OK with removing drm_encoder_chain_lock() and
> replace it with an internal macro or function in drm_bridge.h though.
>
> However I'm not sure how to use scoped_guard here because it doesn't return
> a pointer that can then be passed further. Basically we are constrained to
> have a chain of function or macro calls, each eating the result of the
> inner one, with the outer one returning a bridge pointer for the loop
> cursor variable. There might be some macro magic I'm missing, in such case
> don't hesitate to mention that.

I realized I was not fully clear, sorry about that. The inability to use
scoped_guard refers to the drm_for_each_bridge_in_chain_scoped()
implementation, being a macro defining a for loop. However scoped_guard can
be used in normal locking code such as the changes in patches 3, 6 and 7.

Luca

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