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

[Sam Ravnborg <sam@ravnborg.org>]

Hi Douglas,

> > >  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.
I do not mind the order - so whatever works for you guys.
The only concern here is that we should not gain new users.

> 
> > >  {
> > >       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.
Well, drm_bridge_chain_disable() is about to be deleted. So that the
wrong one to look at.

> 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.
I see, please include this change in your changelog and add it to the
documentation in drm_bridge_h.

	Sam