Message-Id: <DDBZS1BBBKSN.ZHCLP3O5T1HI@bootlin.com>
Date: Tue, 07 Oct 2025 11:45:05 +0200
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: "Dmitry Baryshkov" <dmitry.baryshkov@....qualcomm.com>
Cc: "Maarten Lankhorst" <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard"
 <mripard@...nel.org>, "Thomas Zimmermann" <tzimmermann@...e.de>, "David
 Airlie" <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Andrzej
 Hajda" <andrzej.hajda@...el.com>, "Neil Armstrong"
 <neil.armstrong@...aro.org>, "Robert Foss" <rfoss@...nel.org>, "Laurent
 Pinchart" <Laurent.pinchart@...asonboard.com>, "Jonas Karlman"
 <jonas@...boo.se>, "Jernej Skrabec" <jernej.skrabec@...il.com>, "Hui Pu"
 <Hui.Pu@...ealthcare.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/7] drm/encoder: add mutex to protect the bridge
 chain

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.

Best regards,
Luca

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

Powered by blists - more mailing lists