Re: [PATCH v3 07/21] drm/bridge: Make the bridge chain a double-linked list

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi Neil,

Sorry for the late reply.

On Tue, 5 Nov 2019 17:02:30 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

> Hi,
> 
> 
> On 25/10/2019 15:29, Neil Armstrong wrote:
> > On 23/10/2019 17:44, Boris Brezillon wrote:  
> >> So that each element in the chain can easily access its predecessor.
> >> This will be needed to support bus format negotiation between elements
> >> of the bridge chain.
> >>
> >> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> >> ---
> >> Changes in v3:
> >> * None
> >>
> >> Changes in v2:
> >> * Adjust things to the "dummy encoder bridge" change (patch 2 in this
> >>   series)
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c  | 171 ++++++++++++++++++++++------------
> >>  drivers/gpu/drm/drm_encoder.c |  16 +---
> >>  include/drm/drm_bridge.h      |  12 ++-
> >>  include/drm/drm_encoder.h     |   9 +-
> >>  4 files changed, 135 insertions(+), 73 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 54c874493c57..c5cf8a9c4237 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c  
> 
> [...]
> 
> >>  
> >> @@ -426,15 +471,23 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> >>  void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
> >>  					struct drm_atomic_state *state)
> >>  {
> >> +	struct drm_encoder *encoder;
> >> +	struct drm_bridge *iter;
> >> +
> >>  	if (!bridge)
> >>  		return;
> >>  
> >> -	drm_atomic_bridge_chain_pre_enable(bridge->next, state);
> >> +	encoder = bridge->encoder;
> >> +	list_for_each_entry_reverse(iter, &bridge->encoder->bridge_chain,
> >> +				    chain_node) {  
> 
> This should use the encoder local variable in list_for_each_entry_reverse()
> 
> >> +		if (iter->funcs->atomic_pre_enable)
> >> +			iter->funcs->atomic_pre_enable(iter, state);
> >> +		else if (iter->funcs->pre_enable)
> >> +			iter->funcs->pre_enable(iter);
> >>  
> >> -	if (bridge->funcs->atomic_pre_enable)
> >> -		bridge->funcs->atomic_pre_enable(bridge, state);
> >> -	else if (bridge->funcs->pre_enable)
> >> -		bridge->funcs->pre_enable(bridge);
> >> +		if (iter == bridge)
> >> +			break;
> >> +	}
> >>  }
> >>  EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
> >>  
> >> @@ -453,15 +506,19 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
> >>  void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
> >>  				    struct drm_atomic_state *state)
> >>  {
> >> +	struct drm_encoder *encoder;
> >> +
> >>  	if (!bridge)
> >>  		return;
> >>  
> >> -	if (bridge->funcs->atomic_enable)
> >> -		bridge->funcs->atomic_enable(bridge, state);
> >> -	else if (bridge->funcs->enable)
> >> -		bridge->funcs->enable(bridge);
> >> -
> >> -	drm_atomic_bridge_chain_enable(bridge->next, state);
> >> +	encoder = bridge->encoder;
> >> +	list_for_each_entry_from(bridge, &bridge->encoder->bridge_chain,
> >> +				 chain_node) {  
> 
> This should use encoder instead of bridge->encoder otherwise bridge will
> change and bridge->encoder->bridge_chain won't be valid during the for_each and
> cause the following :

Oops, indeed. I fixed the very same problem in another helper but
somehow missed those 2. Thanks for testing/reporting the bug.

Boris