Hi Neil,
Sorry for the late reply.
On Tue, 5 Nov 2019 17:02:30 +0100
Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> ---
> >> 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
