On Thu, 22 Aug 2019 03:02:17 +0300
Laurent Pinchart <laurent.pinch...@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);
> > + }
> > }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
