Hi Doug,
I see this patch fixes the order for
drm_bridge_chain_pre_enable() and drm_atomic_bridge_chain_post_disable().
For completeness, shouldn't we also fix the order for
drm_atomic_bridge_chain_pre_enable() and drm_bridge_chain_post_disable()?
Surely, if Sam's pending patches will land first, there is no need to
fix the non_atomic versions.
On Thu, Oct 21, 2021 at 1:41 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> 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
