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@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi Dave,
>
> Added Maxime, Laurent [which I thought I added before]
>
> On Tue, Mar 28, 2023 at 10:38 PM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
Thanks for investigating and sorting this.
Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> > Changes for v2:
> > - add missing dri-devel in CC
>
> Would you please look into this issue?
>
> Thanks,
> Jagan.
