Re: [PATCH] drm/bridge: Fix the bridge chain order for pre_enable / post_disable

[Doug Anderson]

Hi,

On Thu, Oct 21, 2021 at 1:21 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> Hi Douglas,
>
> On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
> > Right now, the chaining order of
> > pre_enable/enable/disable/post_disable looks like this:
> >
> > pre_enable:   start from connector and move to encoder
> > enable:       start from encoder and move to connector
> > disable:      start from connector and move to encoder
> > post_disable: start from encoder and move to connector
> >
> > In the above, it can be seen that at least pre_enable() and
> > post_disable() are opposites of each other and enable() and disable()
> > are opposites. However, it seems broken that pre_enable() and enable()
> > would not move in the same direction. In other parts of Linux you can
> > see that various stages move in the same order. For instance, during
> > system suspend the "early" calls run in the same order as the normal
> > calls run in the same order as the "late" calls run in the same order
> > as the "noirq" calls.
> >
> > Let fix the above so that it makes more sense. Now we'll have:
> >
> > pre_enable:   start from encoder and move to connector
> > enable:       start from encoder and move to connector
> > disable:      start from connector and move to encoder
> > post_disable: start from connector and move to encoder
> >
> > This order is chosen because if there are parent-child relationships
> > anywhere I would expect that the encoder would be a parent and the
> > connector a child--not the other way around.
>
> This makes good sense as you describe it. I hope others can add more
> useful feedback.
> Added Andrzej Hajda <andrzej.hajda@xxxxxxxxx> to the mail, as he have
> expressed concerns with the chain of bridges before.
>
> >
> > This can be important when using the DP AUX bus to instantiate a
> > panel. The DP AUX bus is likely part of a bridge driver and is a
> > parent of the panel. We'd like the bridge to be pre_enabled before the
> > panel and the panel to be post_disabled before the
> > bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
> > bridge driver's post_suspend to work properly even a panel is under
> > it.
> >
> > NOTE: it's entirely possible that this change could break someone who
> > was relying on the old order. Hopefully this isn't the case, but if
> > this does break someone it seems like it's better to do it sonner
> > rather than later so we can fix everyone to handle the order that
> > makes the most sense.
> >
> > A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
> > ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
> > inadvertently changed the order of things. The order used to be
> > correct (panel prepare was at the tail of the bridge enable) but it
> > became backwards. We'll restore the original order with this patch.
> >
> > Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>
> To make the patch complete the descriptions in drm_bridge_funcs
> need to be updated to reflect the new reality.

Ah, oops! Sure, I'll plan on a v2 with this but I'll wait for more feedback.



> >  drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index c96847fc0ebc..98808af59afd 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> >  void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>
> If you, or someone else, could r-b or ack the pending patches to remove
> this function, this part of the patch would no longer be needed.

OK. I will likely be able to take a look next week. Given that I'm
helping Philip bringup a board with ps8640 it looks like your patch
series will be quite relevant! I guess it would be good to figure out
what would be the best order to land them. In my case we need this fix
to be easy to pick back to fix the behavior on the Chrome OS 5.4 tree.
My fix is easy to pick back, but perhaps yours is as well. Of course
we could also just make a local divergent change in our tree if need
be, too.


> >  {
> >       struct drm_encoder *encoder;
> > -     struct drm_bridge *iter;
> >
> >       if (!bridge)
> >               return;
> >
> >       encoder = bridge->encoder;
> > -     list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > -             if (iter->funcs->pre_enable)
> > -                     iter->funcs->pre_enable(iter);
> > -
> > -             if (iter == bridge)
> > -                     break;
> > +     list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > +             if (bridge->funcs->pre_enable)
> > +                     bridge->funcs->pre_enable(bridge);
> >       }
> >  }
> >  EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> > @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> >                                         struct drm_atomic_state *old_state)
> >  {
> >       struct drm_encoder *encoder;
> > +     struct drm_bridge *iter;
> s/iter/bridge/ would make the patch simpler
> And then the bridge argument could be last_bridge or something.
> This would IMO increase readability of the code and make the patch smaller.

Yeah, I debated this too. I was trying to match
drm_bridge_chain_disable() and in my mind keeping the two functions
matching is more important than keeping this patch small. Certainly I
could add another patch in the series to rename "bridge" to
"last_bridge" and "iter" to "bridge" in that function, but that
defeats the goal of reducing churn... ...and clearly whoever wrote
drm_bridge_chain_disable() liked "iter" better. :-P

In any case, I'll change it as you say if everyone likes it better,
but otherwise I'll leave it as I have it.


> >       if (!bridge)
> >               return;
> >
> >       encoder = bridge->encoder;
> > -     list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > -             if (bridge->funcs->atomic_post_disable) {
> > +     list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > +             if (iter->funcs->atomic_post_disable) {
> >                       struct drm_bridge_state *old_bridge_state;
> >
> >                       old_bridge_state =
> >                               drm_atomic_get_old_bridge_state(old_state,
> > -                                                             bridge);
> > +                                                             iter);
> >                       if (WARN_ON(!old_bridge_state))
> >                               return;
> >
> > -                     bridge->funcs->atomic_post_disable(bridge,
> > -                                                        old_bridge_state);
> > -             } else if (bridge->funcs->post_disable) {
> > -                     bridge->funcs->post_disable(bridge);
> > +                     iter->funcs->atomic_post_disable(iter,
> > +                                                      old_bridge_state);
> > +             } else if (iter->funcs->post_disable) {
> > +                     iter->funcs->post_disable(iter);
> >               }
> > +
> > +             if (iter == bridge)
> > +                     break;
> I cannot see why this is needed, we are at the end of the list here
> anyway.

It's because you can start at something that's not the first bridge in
the chain. See commit bab5cca7e609 ("drm/bridge: Fix the stop
condition of drm_bridge_chain_pre_enable()")