Re: [PATCH 1/3] drm/bridge: Fix drm_bridge_chain_pre_enable()

[Andrzej Hajda <a.hajda@samsung.com>]

On 06.01.2020 11:29, Laurent Pinchart wrote:
> Hi Boris,
>
> Thank you for the patch.
>
> On Fri, Dec 27, 2019 at 03:41:22PM +0100, Boris Brezillon wrote:
>> Stop iterating on the bridge chain when we reach the bridge element.
>> That's what other helpers do and should allow bridge implementations
>> to execute a pre_enable operation on a sub-chain.
> The code looks fine to me, but I think you should update the
> documentation to explain this. It currently states:
>
>  * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
>  * chain, starting from the last bridge to the first. These are called
>  * before calling the encoder's commit op.
>  *
>  * Note: the bridge passed should be the one closest to the encoder
>
> I suggest stating instead that the operation is called from the last
> bridge to the bridge passed as the argument. The note should then either
> be removed, or updated to state that bridge is usually the bridge
> closest to the encoder, but can be any other bridge if the caller only
> wants to execute the operation on a subset of the chain. It's also
> probably worth it updating the other functions accordingly.


Apparently drm_(atomic_)bridge_chain_* helpers are always called on the
1st bridge so you can try to remove bridge argument, if it is true.

Moreover after patches 2 and 3 drm_bridge_chain_* helpers have no users.


Regards

Andrzej


>
>> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index c2cf0c90fa26..b3b269ec6a39 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -357,6 +357,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>>  	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>  		if (iter->funcs->pre_enable)
>>  			iter->funcs->pre_enable(iter);
>> +
>> +		if (iter == bridge)
>> +			break;
>>  	}
>>  }
>>  EXPORT_SYMBOL(drm_bridge_chain_pre_enable);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel