Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andrzej

On Mon, 28 Feb 2022 at 15:36, Andrzej Hajda <andrzej.hajda@xxxxxxxxx> wrote:
>
>
>
> On 22.02.2022 09:43, Dave Stevenson wrote:
> > Hi Laurent.
> >
> > Thanks for the review.
> >
> > On Tue, 22 Feb 2022 at 06:34, Laurent Pinchart
> > <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> >> Hi Dave,
> >>
> >> Thank you for the patch.
> >>
> >> On Wed, Feb 16, 2022 at 04:59:43PM +0000, Dave Stevenson wrote:
> >>> DSI sink devices typically want the DSI host powered up and configured
> >>> before they are powered up. pre_enable is the place this would normally
> >>> happen, but they are called in reverse order from panel/connector towards
> >>> the encoder, which is the "wrong" order.
> >>>
> >>> Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set
> >>> to swap the order of pre_enable (and post_disable) so that any upstream
> >>> bridges are called first to create the desired state.
> >>>
> >>> eg:
> >>> - Panel
> >>> - Bridge 1
> >>> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >>> - Bridge 3
> >>> - Encoder
> >>> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
> >>> Bridge 2.
> >> If there was a Bridge 4 between Bridge 3 and Encoder, would it be
> >>
> >> Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2
> >>
> >> ? I'd capture that here, to be explicit.
> > No.
> >   - Panel
> >   - Bridge 1
> >   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 3
> >   - Bridge 4
> >    - Encoder
> > Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> > 3, Bridge 2, Bridge 4, Encoder.
> > ie it only swaps the order of bridges 2 & 3.
> >
> >   - Panel
> >   - Bridge 1
> >   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 3 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 4
> >   - Encoder
> > Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> > 4, Bridge 3, Bridge 2, Encoder.
> > (Bridge 2&3 have asked for upstream to be enabled first, which means
> > bridge 4. Bridge 2 wants upstream enabled first, which means bridge
> > 3).
> >
> >   - Panel
> >   - Bridge 1
> >   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 3
> >   - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 5
> >   - Encoder
> > Would result in Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 5, Bridge
> > 4, Encoder.
> >
> > So we only reverse the order whilst the bridges request that they want
> > upstream enabled first, but we can do that multiple times within the
> > chain. I hope that makes sense.
> >
> >>> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> >>> ---
> >>>   drivers/gpu/drm/drm_bridge.c | 197 +++++++++++++++++++++++++++++++++++++------
> >>>   include/drm/drm_bridge.h     |   8 ++
> >>>   2 files changed, 180 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >>> index c96847fc0ebc..7c24e8340efa 100644
> >>> --- a/drivers/gpu/drm/drm_bridge.c
> >>> +++ b/drivers/gpu/drm/drm_bridge.c
> >>> @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
> >>>    * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
> >>>    * encoder chain, starting from the first bridge to the last. These are called
> >>>    * after completing the encoder's prepare op.
> >> Missing blank line, as well as in three locations below.
> >>
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> >>> + * that bridge will be called before the previous one to reverse the pre_enable
> >>> + * calling direction.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>>   void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
> >>>   {
> >>>        struct drm_encoder *encoder;
> >>> +     struct drm_bridge *next, *limit;
> >>>
> >>>        if (!bridge)
> >>>                return;
> >>>
> >>>        encoder = bridge->encoder;
> >>>        list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> >>> +             limit = NULL;
> >>> +
> >>> +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> >>> +                     next = list_next_entry(bridge, chain_node);
> >>> +
> >>> +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                             limit = next;
> >>> +
> >>> +                             list_for_each_entry_from(next, &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                                     if (!(next->ops &
> >>> +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                             next = list_prev_entry(next, chain_node);
> >>> +                                             limit = next;
> >>> +                                             break;
> >>> +                                     }
> >>> +                             }
> >>> +
> >>> +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> >>> +                                                              chain_node) {
> >>> +                                     if (next == bridge)
> >>> +                                             break;
> >>> +
> >>> +                                     if (next->funcs->post_disable)
> >>> +                                             next->funcs->post_disable(next);
> >>> +                             }
> >>> +                     }
> >>> +             }
> >>> +
> >>>                if (bridge->funcs->post_disable)
> >>>                        bridge->funcs->post_disable(bridge);
> >>> +
> >>> +             if (limit)
> >>> +                     bridge = limit;
> >>>        }
> >>> +
> >>>   }
> >>>   EXPORT_SYMBOL(drm_bridge_chain_post_disable);
> >>>
> >>> @@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> >>>    * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
> >>>    * chain, starting from the last bridge to the first. These are called
> >>>    * before calling the encoder's commit op.
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> >>> + * the previous bridge will be called before pre_enable of this bridge.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>>   void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> >>>   {
> >>>        struct drm_encoder *encoder;
> >>> -     struct drm_bridge *iter;
> >>> +     struct drm_bridge *iter, *next, *limit;
> >>>
> >>>        if (!bridge)
> >>>                return;
> >>>
> >>>        encoder = bridge->encoder;
> >>> +
> >>>        list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >>> +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                     next = iter;
> >>> +                     limit = bridge;
> >>> +                     list_for_each_entry_from_reverse(next,
> >>> +                                                      &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                             if (next == bridge)
> >>> +                                     break;
> >>> +
> >>> +                             if (!(next->ops &
> >>> +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                     limit = list_prev_entry(next, chain_node);
> >>> +                                     break;
> >>> +                             }
> >>> +                     }
> >>> +
> >>> +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> >>> +                             if (next == iter)
> >>> +                                     break;
> >>> +
> >>> +                             if (next->funcs->pre_enable)
> >>> +                                     next->funcs->pre_enable(next);
> >>> +                     }
> >>> +             }
> >>> +
> >>>                if (iter->funcs->pre_enable)
> >>>                        iter->funcs->pre_enable(iter);
> >>>
> >>> +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
> >>> +                     iter = limit;
> >>> +
> >>>                if (iter == bridge)
> >>>                        break;
> >>>        }
> >>> @@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
> >>>   }
> >>>   EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
> >>>
> >>> +static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
> >>> +                                             struct drm_atomic_state *old_state)
> >>> +{
> >>> +     if (bridge->funcs->atomic_post_disable) {
> >>> +             struct drm_bridge_state *old_bridge_state;
> >>> +
> >>> +             old_bridge_state =
> >>> +                     drm_atomic_get_old_bridge_state(old_state,
> >>> +                                                     bridge);
> >>> +             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);
> >>> +     }
> >>> +}
> >>> +
> >>>   /**
> >>>    * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
> >>>    *                                     in the encoder chain
> >>> @@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
> >>>    * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
> >>>    * starting from the first bridge to the last. These are called after completing
> >>>    * &drm_encoder_helper_funcs.atomic_disable
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> >>> + * that bridge will be called before the previous one to reverse the pre_enable
> >>> + * calling direction.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>> @@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> >>>                                          struct drm_atomic_state *old_state)
> >>>   {
> >>>        struct drm_encoder *encoder;
> >>> +     struct drm_bridge *next, *limit;
> >>>
> >>>        if (!bridge)
> >>>                return;
> >>>
> >>>        encoder = bridge->encoder;
> >>> +
> >>>        list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> >>> -             if (bridge->funcs->atomic_post_disable) {
> >>> -                     struct drm_bridge_state *old_bridge_state;
> >>> +             limit = NULL;
> >>> +
> >>> +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> >>> +                     next = list_next_entry(bridge, chain_node);
> >>> +
> >>> +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                             limit = next;
> >>> +
> >>> +                             list_for_each_entry_from(next, &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                                     if (!(next->ops &
> >>> +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                             next = list_prev_entry(next, chain_node);
> >>> +                                             limit = next;
> >>> +                                             break;
> >>> +                                     }
> >>> +                             }
> >>> +
> >>> +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> >>> +                                                              chain_node) {
> >>> +                                     if (next == bridge)
> >>> +                                             break;
> >>> +
> >>> +                                     drm_atomic_bridge_call_post_disable(next,
> >>> +                                                                         old_state);
> >>> +                             }
> >>> +                     }
> >>> +             }
> >>>
> >>> -                     old_bridge_state =
> >>> -                             drm_atomic_get_old_bridge_state(old_state,
> >>> -                                                             bridge);
> >>> -                     if (WARN_ON(!old_bridge_state))
> >>> -                             return;
> >>> +             drm_atomic_bridge_call_post_disable(bridge, old_state);
> >>>
> >>> -                     bridge->funcs->atomic_post_disable(bridge,
> >>> -                                                        old_bridge_state);
> >>> -             } else if (bridge->funcs->post_disable) {
> >>> -                     bridge->funcs->post_disable(bridge);
> >>> -             }
> >>> +             if (limit)
> >>> +                     bridge = limit;
> >>>        }
> >>>   }
> >>>   EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> >>>
> >>> +static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
> >>> +                                           struct drm_atomic_state *old_state)
> >>> +{
> >>> +     if (bridge->funcs->atomic_pre_enable) {
> >>> +             struct drm_bridge_state *old_bridge_state;
> >>> +
> >>> +             old_bridge_state =
> >>> +                     drm_atomic_get_old_bridge_state(old_state,
> >>> +                                                     bridge);
> >>> +             if (WARN_ON(!old_bridge_state))
> >>> +                     return;
> >>> +
> >>> +             bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
> >>> +     } else if (bridge->funcs->pre_enable) {
> >>> +             bridge->funcs->pre_enable(bridge);
> >>> +     }
> >>> +}
> >>> +
> >>>   /**
> >>>    * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
> >>>    *                                   the encoder chain
> >>> @@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> >>>    * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
> >>>    * starting from the last bridge to the first. These are called before calling
> >>>    * &drm_encoder_helper_funcs.atomic_enable
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> >>> + * the previous bridge will be called before pre_enable of this bridge.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>> @@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
> >>>                                        struct drm_atomic_state *old_state)
> >>>   {
> >>>        struct drm_encoder *encoder;
> >>> -     struct drm_bridge *iter;
> >>> +     struct drm_bridge *iter, *next, *limit;
> >>>
> >>>        if (!bridge)
> >>>                return;
> >>>
> >>>        encoder = bridge->encoder;
> >>> +
> >>>        list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >>> -             if (iter->funcs->atomic_pre_enable) {
> >>> -                     struct drm_bridge_state *old_bridge_state;
> >>> +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                     next = iter;
> >>> +                     limit = bridge;
> >>> +                     list_for_each_entry_from_reverse(next,
> >>> +                                                      &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                             if (next == bridge)
> >>> +                                     break;
> >>> +
> >>> +                             if (!(next->ops &
> >>> +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                     limit = list_prev_entry(next, chain_node);
> >>> +                                     break;
> >>> +                             }
> >>> +                     }
> >>> +
> >>> +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> >>> +                             if (next == iter)
> >>> +                                     break;
> >>> +
> >>> +                             drm_atomic_bridge_call_pre_enable(next, old_state);
> >>> +                     }
> >>> +             }
> >> This is hard to understand, I have trouble figuring out if it does the
> >> right thing when multiple bridges set the DRM_BRIDGE_OP_UPSTREAM_FIRST
> >> flag (or actually even when a single bridge does so). Comments would
> >> help, but I wonder if it wouldn't be simpler to switch to a recursive
> >> implementation.
> > Recursive - such joys!
> > With the explanation above, I'm not sure that recursive helps, but
> > certainly can add comments.
>
> It could be recursive, or just proper iteration, for example pseudocode:
>
> list_for_each_entry_reverse(...) {
>         if (iter->upstream_first && iter->prev)
>                 continue;
>         tmp = iter;
>         do {
>                 call_op
>                 tmp = tmp->next;
>         } while (tmp && tmp->upstream_first);
> }

You're right, using continue to skip over list entries, and then
having a single loop to work backwards works quite nicely. I'm sorting
a V2 that does that.

  Dave



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux