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
