[PATCH RFC 11/19] drm/bridge: Patch atomic hooks to take a drm_bridge_state

[Boris Brezillon]

On Thu, 22 Aug 2019 03:02:17 +0300
Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> >  }
> >  EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> > @@ -518,10 +535,18 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_encoder *encoder,
> >  
> >  	list_for_each_entry_reverse(bridge, &encoder->bridge_chain,
> >  				    chain_node) {
> > -		if (bridge->funcs->atomic_pre_enable)
> > -			bridge->funcs->atomic_pre_enable(bridge, state);
> > -		else if (bridge->funcs->pre_enable)
> > +		if (bridge->funcs->atomic_pre_enable) {
> > +			struct drm_bridge_state *bridge_state;
> > +
> > +			bridge_state = drm_atomic_get_new_bridge_state(state,
> > +								       bridge);  
> 
> Shouldn't you get the old state here ? The new state in commit-related
> operations is supposed to be obtained from the object itself, and the
> old state is passed to the function. See how the CRTC and encoder
> .atomic_enable() are called in drm_atomic_helper_commit_modeset_enables
> (drm_atomic_helper.c) for instance.
> 
> You should update the documentation of the bridge atomic operations to
> makes this explicit. The documentation of the bridge helpers
> (drm_atomic_bridge_enable() & co.) is also misleading, they get passed
> the old state, while the documentation states "atomic state being
> committed". I think you should rename all those state parameters to
> old_state to make this clearer.
> 
> Last but not least, the implementation in the analogix bridge driver
> seems to expect the new state, so I think it's buggy.

I based that decision on the only driver implementing those hooks. I
can pass the old_state instead.

> 
> > +			if (WARN_ON(!bridge_state))
> > +				return;
> > +
> > +			bridge->funcs->atomic_pre_enable(bridge, bridge_state);
> > +		} else if (bridge->funcs->pre_enable) {
> >  			bridge->funcs->pre_enable(bridge);
> > +		}
> >  	}