[PATCH v2 1/2] drm/bridge: Fix improper bridge init order with pre_enable_prev_first

[Jagan Teki]

On Tue, Aug 1, 2023 at 11:50 AM Dave Stevenson
<dave.stevenson at raspberrypi.com> wrote:
>
> Hi Jagan
>
> My apologies for dropping the ball on this one, and thanks to Frieder
> for the nudge.
>
> On Wed, 12 Apr 2023 at 07:25, Jagan Teki <jagan at amarulasolutions.com> wrote:
> >
> > Hi Dave,
> >
> > Added Maxime, Laurent [which I thought I added before]
> >
> > On Tue, Mar 28, 2023 at 10:38 PM Jagan Teki <jagan at amarulasolutions.com> wrote:
> > >
> > > For a given bridge pipeline if any bridge sets pre_enable_prev_first
> > > flag then the pre_enable for the previous bridge will be called before
> > > pre_enable of this bridge and opposite is done for post_disable.
> > >
> > > These are the potential bridge flags to alter bridge init order in order
> > > to satisfy the MIPI DSI host and downstream panel or bridge to function.
> > > However the existing pre_enable_prev_first logic with associated bridge
> > > ordering has broken for both pre_enable and post_disable calls.
> > >
> > > [pre_enable]
> > >
> > > The altered bridge ordering has failed if two consecutive bridges on a
> > > given pipeline enables the pre_enable_prev_first flag.
> > >
> > > Example:
> > > - Panel
> > > - Bridge 1
> > > - Bridge 2 pre_enable_prev_first
> > > - Bridge 3
> > > - Bridge 4 pre_enable_prev_first
> > > - Bridge 5 pre_enable_prev_first
> > > - Bridge 6
> > > - Encoder
> > >
> > > In this example, Bridge 4 and Bridge 5 have pre_enable_prev_first.
> > >
> > > The logic looks for a bridge which enabled pre_enable_prev_first flag
> > > on each iteration and assigned the previou bridge to limit pointer
> > > if the bridge doesn't enable pre_enable_prev_first flags.
> > >
> > > If control found Bridge 2 is pre_enable_prev_first then the iteration
> > > looks for Bridge 3 and found it is not pre_enable_prev_first and assigns
> > > it's previous Bridge 4 to limit pointer and calls pre_enable of Bridge 3
> > > and Bridge 2 and assign iter pointer with limit which is Bridge 4.
> > >
> > > Here is the actual problem, for the next iteration control look for
> > > Bridge 5 instead of Bridge 4 has iter pointer in previous iteration
> > > moved to Bridge 4 so this iteration skips the Bridge 4. The iteration
> > > found Bridge 6 doesn't pre_enable_prev_first flags so the limit assigned
> > > to Encoder. From next iteration Encoder skips as it is the last bridge
> > > for reverse order pipeline.
> > >
> > > So, the resulting pre_enable bridge order would be,
> > > - Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 6, Bridge 5.
> > >
> > > This patch fixes this by assigning limit to next pointer instead of
> > > previous bridge since the iteration always looks for bridge that does
> > > NOT request prev so assigning next makes sure the last bridge on a
> > > given iteration what exactly the limit bridge is.
> > >
> > > So, the resulting pre_enable bridge order with fix would be,
> > > - Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 6, Bridge 5, Bridge 4,
> > >   Encoder.
> > >
> > > [post_disable]
> > >
> > > The altered bridge ordering has failed if two consecutive bridges on a
> > > given pipeline enables the pre_enable_prev_first flag.
> > >
> > > Example:
> > > - Panel
> > > - Bridge 1
> > > - Bridge 2 pre_enable_prev_first
> > > - Bridge 3
> > > - Bridge 4 pre_enable_prev_first
> > > - Bridge 5 pre_enable_prev_first
> > > - Bridge 6
> > > - Encoder
> > >
> > > In this example Bridge 5 and Bridge 4 have pre_enable_prev_first.
> > >
> > > The logic looks for a bridge which enabled pre_enable_prev_first flags
> > > on each iteration and assigned the previou bridge to next and next to
> > > limit pointer if the bridge does enable pre_enable_prev_first flag.
> > >
> > > If control starts from Bridge 6 then it found next Bridge 5 is
> > > pre_enable_prev_first and immediately the next assigned to previous
> > > Bridge 6 and limit assignments to next Bridge 6 and call post_enable
> > > of Bridge 6 even though the next consecutive Bridge 5 is enabled with
> > > pre_enable_prev_first. This clearly misses the logic to find the state
> > > of next conducive bridge as everytime the next and limit assigns
> > > previous bridge if given bridge enabled pre_enable_prev_first.
> > >
> > > So, the resulting post_disable bridge order would be,
> > > - Encoder, Bridge 6, Bridge 5, Bridge 4, Bridge 3, Bridge 2, Bridge 1,
> > >   Panel.
> > >
> > > This patch fixes this by assigning next with previou bridge only if the
> > > bridge doesn't enable pre_enable_prev_first flag and the next further
> > > assign it to limit. This way we can find the bridge that NOT requested
> > > prev to disable last.
> > >
> > > So, the resulting pre_enable bridge order with fix would be,
> > > - Encoder, Bridge 4, Bridge 5, Bridge 6, Bridge 2, Bridge 3, Bridge 1,
> > >   Panel.
> > >
> > > Validated the bridge init ordering by incorporating dummy bridges in
> > > the sun6i-mipi-dsi pipeline
> > >
> > > Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to
> > > alter bridge init order")
> > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
>
> Thanks for investigating and sorting this.
>
> Reviewed-by: Dave Stevenson <dave.stevenson at raspberrypi.com>
>
> > > ---
> > > Changes for v2:
> > > - add missing dri-devel in CC
> >
> > Would you please look into this issue?

These still not been picked it yet, can any one pull these two fixes?

Thanks,
Jagan.