Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <298b02d6-135e-70f8-f9b6-8f960e9485cb@kontron.de>
Date:   Mon, 17 Jul 2023 09:09:07 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 1/1] drm/bridge: Fix handling of bridges with
 pre_enable_prev_first flag
To:     Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc:     Vladimir Lypak <vladimir.lypak@gmail.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Jagan Teki <jagan@amarulasolutions.com>
References: <20230707190020.6280-2-vladimir.lypak@gmail.com>
 <7c4affcb-bcb4-5f4b-cc2f-bed2cad9de71@kontron.de>
 <CAPY8ntBWmfDPwPJS_z5tko7PUxHG9pVQ1DOKC3baGDCNzMzykw@mail.gmail.com>
Content-Language: en-US, de-DE
From:   Frieder Schrempf <frieder.schrempf@kontron.de>
In-Reply-To: <CAPY8ntBWmfDPwPJS_z5tko7PUxHG9pVQ1DOKC3baGDCNzMzykw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YTVITjIxRFBiQSt0YnIxVSsrTmtFVjBwWDFoTEVGUCs0amNadlBucnQ0YlVV?=
 =?utf-8?B?UThKdzlWS241NDlHNVhwazlGYnNEaFhudnR0TlhlSjIyVGRtUWgwSG5NU0ho?=
 =?utf-8?B?ZXNyblVNQ3RQN0JzeTRSeDBXSlFCSHZNVW1BYzVodnFPVkJ1em5LRlhRdWhk?=
 =?utf-8?B?VU0wODRXWlJndy80OW5pd0hRTVcvaHBvOVVzWUcwMDFmQkMybEhLRU8yczhY?=
 =?utf-8?B?eWticHhJRGJkeitaSmIydzZSWFM5amUvcy9qVTVZblFDK3dpakVZS0xqQTBi?=
 =?utf-8?B?RTlpSVhYR2k2RURGNGY1eFFaSlcrRDlURVgzYzh1cVVXR0dPeFZ0WHAraGlX?=
 =?utf-8?B?bTVJQmZOa3U3Z25oQ0RqY0U3YTh6Wm1FcmtBd2F5SEZHZVNqNzl6elJoOGFr?=
 =?utf-8?B?ZU8rcGpxVjdsMnFHWktlNHkyNDgwbTNIMHlBTUZ2WDRwek1haVE0OGREa0pu?=
 =?utf-8?B?VndRdjBFMXh3RjltNUwwWSs4dVVtWFZRTXhYR1VYYy9PUndGeUNycElRUGM1?=
 =?utf-8?B?bW9WWEM2aXp6Uk9mVkRmTWdiTkRuSHREamdZNDB3cHhIZi9ZM2JuZ3oyL0cz?=
 =?utf-8?B?d0FLS0Z0VEpPTnNiYit6SGx1MkhNUlFmMFFhUzJKbkE1bXZFaTVQZ0lqMUNW?=
 =?utf-8?B?bUF0aUlySlJmVldHVWt6K201b0RVMVlSRS9Zdmc3NzdPYzF6WXBua0FuMFJn?=
 =?utf-8?B?YnNGVkpzVWxsVVdtNVJhM2s2RkhRcnp1QUQ3VSt2WFFRaC8xalVKMmNwVDAv?=
 =?utf-8?B?SGl5NnhTK0kwbXRkU0VOU0pqQ2h1TzdzRTdFaEdtWmhHRG4yUzNsRE5KWEo2?=
 =?utf-8?B?VnVRbmQrdUNhWWk2bzVyNWZKcTNiWnBjdGJ1Z1F6bDFPTEo5akVKZVhua2hq?=
 =?utf-8?B?Y01sZ3JqcTBMSEtNSjBaNXl4bytUWHZpTXdnaCtEdEUrcDZoVk9tcDg2elRJ?=
 =?utf-8?B?T1BuWjNFeWhaeXdLWjl2Nm9lZjhQcnFXTVhLeDU0OG5tcGcxek1aUlBOc2Fp?=
 =?utf-8?B?TkJ0MENBNUhIeTBVanRxbU42ZWM2STVBdVlKaUVWcFFlVWwyN0R0clJMbkdL?=
 =?utf-8?B?aG1paHBZM0NvYXh2eHJEZXFQTnVuRU1qRTlRYkxWdHNhVEpSd0x1V1FGSGUz?=
 =?utf-8?B?ZWZYNDV6azhERVRrYWhuMmVHSzh3TzFiS1QyMTU1NmduRWtONkxxVDMyOUlu?=
 =?utf-8?B?QW9ZQ3BqL3QrSFdvSThYb2MzZFFweEZWbklVbDBwR3BoMEZ2MmRjbmF2elVx?=
 =?utf-8?B?bWs2bHlIa05rYm9nQ0RvMXdIU28vNjFxY2loalAzMERJUXoyUFp3U0RqbkhV?=
 =?utf-8?B?K1ptbGticWxWek1XcHRxUGtyU2lCMVF0UFRsd242TUZiTCsxMm9VeGJaNzlH?=
 =?utf-8?B?MS9SMFpuUkVZTmU0TXVIVy9VTDFDQ2tWNUVTYW1HMGU3YWc5amJBZ3BBb3Zk?=
 =?utf-8?B?WDhYS2JObUxsaVdkL1Z1V3krcWtIcCtYVWVGdzY0Sm12ek95eVR0SjZGLzli?=
 =?utf-8?B?NGNHNVpaeGxzOG1wV3BsL3A2MVU4bkJYZU5waXZsaFAwWE1kbVltS0o1Y1FU?=
 =?utf-8?B?Mkh1ZDlCTGxMQUtPQ000UFY5eFY1dzFhZVp3N1MyVzZQbGpDRGpuUWVYUXd1?=
 =?utf-8?B?M2hia0tVT1NUbTE5R2hCa3pDWndBSFNtUXlKSmVHUzJpRlQrR05HMCtkMDdZ?=
 =?utf-8?B?aUxKZ2ZkRzlqbDc5M25UWEdJNWNCNWc2Zm9hZjIyazUyMmVIOWVVSlVTWUE4?=
 =?utf-8?B?c3FXMHJjbmM0ZzRXaStBZzJJZ3B6ZHdtWmtMY2wxWHlGYWpXN25zR1lwOHdX?=
 =?utf-8?B?cEJCNGxmQkxhTFNwZzlOam5rZlVSL0lIb3p0MytFcHZPbzAvVmVES2NBaFRa?=
 =?utf-8?B?UmJLcmpmaXlFOEJVZnQ4NldVNVpMUkQwVFduSVl0Q2dxWkhqU0R2T1p4L3g3?=
 =?utf-8?B?cnFwRmNacW9sdjlnQXE1Y25ZZ005ekxJcjZxcUVianM5M01hMjNUQnR2NDBL?=
 =?utf-8?B?QlJiQjVORUVrK3gvTU85UHpZVURacTRwU3hYVzJDdHp2NnNMT1ZydjZlL3Nn?=
 =?utf-8?B?YmRiRUlMWjZsTmlNcWJuMHZPaGNVYi9uZmFCY2JyanAvWlVTUS85OGp4UW5T?=
 =?utf-8?B?NDhJS05hMHQ0S3VMZ1F2V2RWUmgxS0U2elNmc2s4ZzJNSWNNdUdNcTdjYXlh?=
 =?utf-8?B?QkE9PQ==?=
X-OriginatorOrg: kontron.de
X-MS-Exchange-CrossTenant-Network-Message-Id: 5abfffaf-d575-4e8b-f402-08db8694b77f
X-MS-Exchange-CrossTenant-AuthSource: PA4PR10MB5681.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jul 2023 07:09:09.6514
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8c9d3c97-3fd9-41c8-a2b1-646f3942daf1
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Z4pLosAfR3zVLj9y668D2aR5ySZb/vqSGnut+KCEP07zyZmwiNQ713n3JCCk2+WokorEem4lLTnfeecbVQDBAWwzwz1qXDeWfc7CFc2TxhQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR10MB6679
X-Spam-Status: No, score=-1.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,FORGED_SPF_HELO,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,T_SCC_BODY_TEXT_LINE,T_SPF_TEMPERROR
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 14.07.23 19:16, Dave Stevenson wrote:
> Hi Frieder
> 
> On Mon, 10 Jul 2023 at 08:46, Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> On 07.07.23 21:00, Vladimir Lypak wrote:
>>> [Sie erhalten nicht häufig E-Mails von vladimir.lypak@gmail.com. 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@gmail.com>
>>
>> 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@amarulasolutions.com/
>>
>> +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!