On 14.07.23 19:16, Dave Stevenson wrote: > Hi Frieder > > On Mon, 10 Jul 2023 at 08:46, Frieder Schrempf > <frieder.schrempf@xxxxxxxxxx> wrote: >> >> On 07.07.23 21:00, Vladimir Lypak wrote: >>> [Sie erhalten nicht häufig E-Mails von vladimir.lypak@xxxxxxxxx. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ] >>> >>> In function drm_atomic_bridge_chain_post_disable handling of >>> pre_enable_prev_first flag is broken because "next" variable will always >>> end up set to value of "bridge". This breaks loop which should disable >>> bridges in reverse: >>> >>> next = list_next_entry(bridge, chain_node); >>> >>> if (next->pre_enable_prev_first) { >>> /* next bridge had requested that prev >>> * was enabled first, so disabled last >>> */ >>> limit = next; >>> >>> /* Find the next bridge that has NOT requested >>> * prev to be enabled first / disabled last >>> */ >>> list_for_each_entry_from(next, &encoder->bridge_chain, >>> chain_node) { >>> // Next condition is always true. It is likely meant to be inversed >>> // according to comment above. But doing this uncovers another problem: >>> // it won't work if there are few bridges with this flag set at the end. >>> if (next->pre_enable_prev_first) { >>> next = list_prev_entry(next, chain_node); >>> limit = next; >>> // Here we always set next = limit = branch at first iteration. >>> break; >>> } >>> } >>> >>> /* Call these bridges in reverse order */ >>> list_for_each_entry_from_reverse(next, &encoder->bridge_chain, >>> chain_node) { >>> // This loop never executes past this branch. >>> if (next == bridge) >>> break; >>> >>> drm_atomic_bridge_call_post_disable(next, old_state); >>> >>> In this patch logic for handling the flag is simplified. Temporary >>> "iter" variable is introduced instead of "next" which is used only >>> inside inner loops. >>> >>> Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") >>> Signed-off-by: Vladimir Lypak <vladimir.lypak@xxxxxxxxx> >> >> I haven't had a chance to look at this, but I still want to reference >> another patch by Jagan that intends to fix some bug in this area: >> >> https://patchwork.kernel.org/project/dri-devel/patch/20230328170752.1102347-1-jagan@xxxxxxxxxxxxxxxxxxxx/ >> >> +Cc: Jagan >> >> Dave, as you introduced this feature, did you have a chance to look at >> Jagan's and Vladimir's patches? > > Sorry, they'd fallen off my radar. > I'm having a look at the moment, but will probably need to carry it > over to Monday. Sure, take your time. I just wanted to make sure that you are aware of these patches and the existence of a bug in the code. Thanks!
