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 <[email protected]>
---
drivers/gpu/drm/drm_bridge.c | 46 ++++++++++++++----------------------
1 file changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c3d69af02e79..7a5b39a46325 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -660,7 +660,7 @@ 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;
+ struct drm_bridge *iter, *limit;
if (!bridge)
return;
@@ -670,36 +670,26 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
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->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) {
- if (next->pre_enable_prev_first) {
- next = list_prev_entry(next, chain_node);
- limit = next;
- break;
- }
- }
+ /* Find sequence of bridges (bridge, limit] which requested prev to
+ * be enabled first and disabled last
+ */
+ iter = list_next_entry(bridge, chain_node);
+ list_for_each_entry_from(iter, &encoder->bridge_chain, chain_node) {
+ if (!iter->pre_enable_prev_first)
+ break;
+
+ limit = iter;
+ }
- /* Call these bridges in reverse order */
- 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);
- }
+ if (limit) {
+ /* Call these bridges in reverse order */
+ iter = limit;
+ list_for_each_entry_from_reverse(iter,
+ &encoder->bridge_chain, chain_node) {
+ if (iter == bridge)
+ break;
+
+ drm_atomic_bridge_call_post_disable(iter, old_state);
}
}
--
2.41.0
On 07.07.23 21:00, Vladimir Lypak wrote:
> [Sie erhalten nicht häufig E-Mails von [email protected]. 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 <[email protected]>
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/[email protected]/
+Cc: Jagan
Dave, as you introduced this feature, did you have a chance to look at
Jagan's and Vladimir's patches?
Hi Frieder
On Mon, 10 Jul 2023 at 08:46, Frieder Schrempf
<[email protected]> wrote:
>
> On 07.07.23 21:00, Vladimir Lypak wrote:
> > [Sie erhalten nicht häufig E-Mails von [email protected]. 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 <[email protected]>
>
> 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/[email protected]/
>
> +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.
Dave
On 14.07.23 19:16, Dave Stevenson wrote:
> Hi Frieder
>
> On Mon, 10 Jul 2023 at 08:46, Frieder Schrempf
> <[email protected]> wrote:
>>
>> On 07.07.23 21:00, Vladimir Lypak wrote:
>>> [Sie erhalten nicht häufig E-Mails von [email protected]. 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 <[email protected]>
>>
>> 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/[email protected]/
>>
>> +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!