Hi Jagan
On Sun, Nov 26, 2023 at 5:11 PM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Nov 13, 2023 at 6:45 PM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Aug 1, 2023 at 11:50 AM Dave Stevenson
> > <dave.stevenson@xxxxxxxxxxxxxxx> 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@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?
> >
> > These still not been picked it yet, can any one pull these two fixes?
>
> Ping!
>
>
We have a similar fix on this. We can test on our test scenario for post disable
https://github.com/torvalds/linux/commit/1883dfbc490cb050999379a180445c1f372ad784
https://github.com/torvalds/linux/commit/a38ac19b1ec96a05963d0160f789fa7e6763dddb
I will ask Dario to give a try to this too
Michael
