On Thu, 22 Aug 2019 03:02:17 +0300 Laurent Pinchart laurent.pinchart@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);}