Message ID | 20231205105341.4100896-3-dario.binacchi@amarulasolutions.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp3344741vqy; Tue, 5 Dec 2023 02:54:22 -0800 (PST) X-Google-Smtp-Source: AGHT+IHHItgDHzzO1PuRPXA6uhCn8u4+g5jO5fkgPOyU2aWmfOgq8RPzy9+8Iwz2XPNtiiSzSdY6 X-Received: by 2002:a17:902:9302:b0:1d0:91a0:a29 with SMTP id bc2-20020a170902930200b001d091a00a29mr2059207plb.6.1701773662507; Tue, 05 Dec 2023 02:54:22 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701773662; cv=none; d=google.com; s=arc-20160816; b=uKk11fbO2D5Ypw9ieJhsRac2zgMbRFAezSji28+i61S8NOcHmeovWdySzNoW7t6VJ3 sBMkBGHHvuZBi7Qhj8sXQEBGw5CsP9Y5nEE3UaY+C22xlDupfR8KMQ05MjWc8ixb3h4f lP0289x2LcOL5GND4cfBVWkLiVfyDrT/OaTGfWN71nXrgjGcBUk2lQLqoQj1O6aiyT1h NWVtSjjlrS6gLvRQzdGlm/B6UHF70//D7+9Xra/rKbAZ2v3di2rVuBL+RaGPMNIlhSGz zitcCy/CpFhHngObGh/KK42ZHOPJsVG0s+L+RsboF1/Jv2mTjnRT31BMYtQLVaioNnZH IcLw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=fu0Px4Tbxip0tKoFWKIyTvbcuMI1IJ2fyAiFtPcJOWc=; fh=yrurfj0wqsNBtuv2/DS7hXM5gjUdNl0RGba7zZuGnHQ=; b=mbvbrZhuvi6XzppftZQ3t38JGaG69Wq2dtCxbYu7+MqJVOZTMaZ51/596ZwsFynYhm 8oJqXhvkXpDVKZJiRbD4PMscuLIP0zUbxRhuhQdMIRWqg/rbL7tmRL7b/LXGK5LIE332 DmUxx7hZp/CXXkbcl6vfgxA1HUmItqyxTKn01D2uCzB30h9WlX+IPBbJg3g9WPPs6FxO hNb3bUAtNIg0zTXWE8ZS3ecgp8G22BfevwHnvXWrqeTWIgb2h7kw+Gyn3pCVxfR4DvRG SXknrf8PFyhvVVRTGJKjN1iPA2wl0Qk2KsxcHNgbCoQ4zYapMJk0rjg4cAhNQ3fLxfte flBw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@amarulasolutions.com header.s=google header.b=b8WPXIpg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=amarulasolutions.com Received: from morse.vger.email (morse.vger.email. [2620:137:e000::3:1]) by mx.google.com with ESMTPS id x12-20020a170902ea8c00b001cff9cd4d6fsi5592846plb.174.2023.12.05.02.54.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 02:54:22 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1; Authentication-Results: mx.google.com; dkim=pass header.i=@amarulasolutions.com header.s=google header.b=b8WPXIpg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=amarulasolutions.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id 415B680E70B1; Tue, 5 Dec 2023 02:54:18 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1376680AbjLEKyD (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Tue, 5 Dec 2023 05:54:03 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50960 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1376606AbjLEKyA (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 5 Dec 2023 05:54:00 -0500 Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 13E17109 for <linux-kernel@vger.kernel.org>; Tue, 5 Dec 2023 02:54:07 -0800 (PST) Received: by mail-lj1-x22d.google.com with SMTP id 38308e7fff4ca-2c9f572c4c5so40939441fa.2 for <linux-kernel@vger.kernel.org>; Tue, 05 Dec 2023 02:54:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amarulasolutions.com; s=google; t=1701773645; x=1702378445; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=fu0Px4Tbxip0tKoFWKIyTvbcuMI1IJ2fyAiFtPcJOWc=; b=b8WPXIpgLqjlmT/oCeG7Bt+qzOSMQ+Urp6mTEYH67oqWB7KgxyrQ/UZbBzMWMoZ50c RerIoC0DV/pO/PViB/q6itggmld6wrBHH3blX+NsXCqBFdinr5VYJ/xQJagDZ8yFAfYA LDms3dqmo5VjM3QUxjzCKS/RuM/1LggQh+TQg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701773645; x=1702378445; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=fu0Px4Tbxip0tKoFWKIyTvbcuMI1IJ2fyAiFtPcJOWc=; b=Ou5uDE3gdMAtM+gESEJNO8cK9Em9YZNeFT8CmQuP2uqs34g4glb4hpb744cIXSzYnm lUX4tKeMQQTGV64PPw1rBzjdN+0QAuAOzHidgHx3nGg/zoFGF5ApYx/2KjpxDjpZkaDs 9PJK0dotcu+CMW70W5Jz+JV20uPe//fOOTnS0K54OY2kiby7V1DhLJTkT4pQoMlOmnEp cAw4cIlwB14ZOF+wfP12o+OBi4rF8iKBA3Av5FG7yW1jELQim9plPHmOU3e4rU00tlSP xlI3EqGb+fspVLBRgh7mJxMpmjpHxwJyMaBsuAkOtW3OAs7VqNEafqVPRPsTja2Z4zyT 7z8w== X-Gm-Message-State: AOJu0YwHpi3sOTXyRQVCyQRfHtfwkqUjqt5mhkPz9GEFO3CAGD+CirsO Dmteh+5eGPnbBmAlmngFSK3WO4K0lHbOL14aHxZgrQ== X-Received: by 2002:ac2:4c8c:0:b0:50b:fe3f:8086 with SMTP id d12-20020ac24c8c000000b0050bfe3f8086mr1104607lfl.53.1701773645010; Tue, 05 Dec 2023 02:54:05 -0800 (PST) Received: from dario-ThinkPad-T14s-Gen-2i.homenet.telecomitalia.it (host-82-54-95-129.retail.telecomitalia.it. [82.54.95.129]) by smtp.gmail.com with ESMTPSA id n23-20020a170906089700b0099297782aa9sm6413491eje.49.2023.12.05.02.54.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 02:54:04 -0800 (PST) From: Dario Binacchi <dario.binacchi@amarulasolutions.com> To: linux-kernel@vger.kernel.org Cc: Amarula patchwork <linux-amarula@amarulasolutions.com>, michael@amarulasolutions.com, Dario Binacchi <dario.binacchi@amarulasolutions.com>, Andrzej Hajda <andrzej.hajda@intel.com>, Daniel Vetter <daniel@ffwll.ch>, Dave Stevenson <dave.stevenson@raspberrypi.com>, David Airlie <airlied@gmail.com>, Frieder Schrempf <frieder.schrempf@kontron.de>, Jernej Skrabec <jernej.skrabec@gmail.com>, Jonas Karlman <jonas@kwiboo.se>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Neil Armstrong <neil.armstrong@linaro.org>, Robert Foss <rfoss@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, dri-devel@lists.freedesktop.org Subject: [PATCH v4 02/10] drm/bridge: Fix a use case in the bridge disable logic Date: Tue, 5 Dec 2023 11:52:49 +0100 Message-ID: <20231205105341.4100896-3-dario.binacchi@amarulasolutions.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20231205105341.4100896-1-dario.binacchi@amarulasolutions.com> References: <20231205105341.4100896-1-dario.binacchi@amarulasolutions.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Tue, 05 Dec 2023 02:54:18 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784439019777522816 X-GMAIL-MSGID: 1784439019777522816 |
Series |
Add displays support for bsh-smm-s2/pro boards
|
|
Commit Message
Dario Binacchi
Dec. 5, 2023, 10:52 a.m. UTC
The patch fixes the code for finding the next bridge with the "pre_enable_prev_first" flag set to false. In case this condition is not verified, i. e. there is no subsequent bridge with the flag set to false, the whole bridge list is traversed, invalidating the "next" variable. The use of a new iteration variable (i. e. "iter") ensures that the value of the "next" variable is not invalidated. Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- (no changes since v1) drivers/gpu/drm/drm_bridge.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Comments
Hi Dario On Tue, 5 Dec 2023 at 10:54, Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > The patch fixes the code for finding the next bridge with the > "pre_enable_prev_first" flag set to false. In case this condition is > not verified, i. e. there is no subsequent bridge with the flag set to > false, the whole bridge list is traversed, invalidating the "next" > variable. > > The use of a new iteration variable (i. e. "iter") ensures that the value > of the "next" variable is not invalidated. We already have https://patchwork.freedesktop.org/patch/529288/ that has been reviewed (but not applied) to resolve this. What does this version do differently and why? Dave > Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") > Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_bridge.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index f66bf4925dd8..2e5781bf192e 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -662,7 +662,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, *next, *limit; > > if (!bridge) > return; > @@ -680,14 +680,15 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge, > * was enabled first, so disabled last > */ > limit = next; > + iter = 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, > + list_for_each_entry_from(iter, &encoder->bridge_chain, > chain_node) { > - if (!next->pre_enable_prev_first) { > - next = list_prev_entry(next, chain_node); > + if (!iter->pre_enable_prev_first) { > + next = list_prev_entry(iter, chain_node); > limit = next; > break; > } > -- > 2.43.0 >
Hi Dave and Jagan, On Tue, Dec 5, 2023 at 4:39 PM Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Dario > > On Tue, 5 Dec 2023 at 10:54, Dario Binacchi > <dario.binacchi@amarulasolutions.com> wrote: > > > > The patch fixes the code for finding the next bridge with the > > "pre_enable_prev_first" flag set to false. In case this condition is > > not verified, i. e. there is no subsequent bridge with the flag set to > > false, the whole bridge list is traversed, invalidating the "next" > > variable. > > > > The use of a new iteration variable (i. e. "iter") ensures that the value > > of the "next" variable is not invalidated. > > We already have https://patchwork.freedesktop.org/patch/529288/ that > has been reviewed (but not applied) to resolve this. What does this > version do differently and why? My patches only affect drm_atomic_bridge_chain_post_disable(), whereas Jagan's patch affects both drm_atomic_bridge_chain_post_disable() and drm_atomic_bridge_chain_pre_enable(). I tested Jagan's patch on my system with success and I reviewed with Michael Trimarchi the drm_atomic_bridge_chain_pre_enable() fixing and we think it's okay. We also believe that our changes to post_disable() are better, as we set the 'next' variable only when required, and the code is more optimized since the list_is_last() is not called within the loop. Would it be possible to use Jagan's patch for fixing drm_atomic_bridge_chain_pre_enable() and mine for fixing drm_atomic_bridge_chain_post_disable()? Thanks and regards, Dario > > Dave > > > Fixes: 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init order") > > Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com> > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > --- > > > > (no changes since v1) > > > > drivers/gpu/drm/drm_bridge.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index f66bf4925dd8..2e5781bf192e 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -662,7 +662,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, *next, *limit; > > > > if (!bridge) > > return; > > @@ -680,14 +680,15 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge, > > * was enabled first, so disabled last > > */ > > limit = next; > > + iter = 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, > > + list_for_each_entry_from(iter, &encoder->bridge_chain, > > chain_node) { > > - if (!next->pre_enable_prev_first) { > > - next = list_prev_entry(next, chain_node); > > + if (!iter->pre_enable_prev_first) { > > + next = list_prev_entry(iter, chain_node); > > limit = next; > > break; > > } > > -- > > 2.43.0 > >
Hi Dario, On Wed, Dec 6, 2023 at 6:57 PM Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > Hi Dave and Jagan, > > On Tue, Dec 5, 2023 at 4:39 PM Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Dario > > > > On Tue, 5 Dec 2023 at 10:54, Dario Binacchi > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > The patch fixes the code for finding the next bridge with the > > > "pre_enable_prev_first" flag set to false. In case this condition is > > > not verified, i. e. there is no subsequent bridge with the flag set to > > > false, the whole bridge list is traversed, invalidating the "next" > > > variable. > > > > > > The use of a new iteration variable (i. e. "iter") ensures that the value > > > of the "next" variable is not invalidated. > > > > We already have https://patchwork.freedesktop.org/patch/529288/ that > > has been reviewed (but not applied) to resolve this. What does this > > version do differently and why? > > My patches only affect drm_atomic_bridge_chain_post_disable(), whereas > Jagan's patch affects both > drm_atomic_bridge_chain_post_disable() and drm_atomic_bridge_chain_pre_enable(). > I tested Jagan's patch on my system with success and I reviewed with > Michael Trimarchi the > drm_atomic_bridge_chain_pre_enable() fixing and we think it's okay. > We also believe that our changes to post_disable() are better, as we > set the 'next' variable only when required, > and the code is more optimized since the list_is_last() is not called > within the loop. > Would it be possible to use Jagan's patch for fixing > drm_atomic_bridge_chain_pre_enable() and mine for > fixing drm_atomic_bridge_chain_post_disable()? > Can you please share the post-disabled bridge chain list with the below example before and after your change? 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 Thanks, Jagan.
Hi Jagan On Wed, Dec 6, 2023 at 2:31 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > Hi Dario, > > On Wed, Dec 6, 2023 at 6:57 PM Dario Binacchi > <dario.binacchi@amarulasolutions.com> wrote: > > > > Hi Dave and Jagan, > > > > On Tue, Dec 5, 2023 at 4:39 PM Dave Stevenson > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > Hi Dario > > > > > > On Tue, 5 Dec 2023 at 10:54, Dario Binacchi > > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > > > The patch fixes the code for finding the next bridge with the > > > > "pre_enable_prev_first" flag set to false. In case this condition is > > > > not verified, i. e. there is no subsequent bridge with the flag set to > > > > false, the whole bridge list is traversed, invalidating the "next" > > > > variable. > > > > > > > > The use of a new iteration variable (i. e. "iter") ensures that the value > > > > of the "next" variable is not invalidated. > > > > > > We already have https://patchwork.freedesktop.org/patch/529288/ that > > > has been reviewed (but not applied) to resolve this. What does this > > > version do differently and why? > > > > My patches only affect drm_atomic_bridge_chain_post_disable(), whereas > > Jagan's patch affects both > > drm_atomic_bridge_chain_post_disable() and drm_atomic_bridge_chain_pre_enable(). > > I tested Jagan's patch on my system with success and I reviewed with > > Michael Trimarchi the > > drm_atomic_bridge_chain_pre_enable() fixing and we think it's okay. > > We also believe that our changes to post_disable() are better, as we > > set the 'next' variable only when required, > > and the code is more optimized since the list_is_last() is not called > > within the loop. > > Would it be possible to use Jagan's patch for fixing > > drm_atomic_bridge_chain_pre_enable() and mine for > > fixing drm_atomic_bridge_chain_post_disable()? > > > > Can you please share the post-disabled bridge chain list with the > below example before and after your change? We have already git commit the description in how the patch affects the post_disable. As Dario reported your patch is ok even in our use case. We don't have a real use case as the one you describe. Can we know how you test it in this use case here? Can you test our patches of post_disable? Thanks Michael > > 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 > > Thanks, > Jagan.
Hi Jagan and Dave, On Wed, Dec 6, 2023 at 2:57 PM Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote: > > Hi Jagan > > On Wed, Dec 6, 2023 at 2:31 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > Hi Dario, > > > > On Wed, Dec 6, 2023 at 6:57 PM Dario Binacchi > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > Hi Dave and Jagan, > > > > > > On Tue, Dec 5, 2023 at 4:39 PM Dave Stevenson > > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > > > Hi Dario > > > > > > > > On Tue, 5 Dec 2023 at 10:54, Dario Binacchi > > > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > > > > > The patch fixes the code for finding the next bridge with the > > > > > "pre_enable_prev_first" flag set to false. In case this condition is > > > > > not verified, i. e. there is no subsequent bridge with the flag set to > > > > > false, the whole bridge list is traversed, invalidating the "next" > > > > > variable. > > > > > > > > > > The use of a new iteration variable (i. e. "iter") ensures that the value > > > > > of the "next" variable is not invalidated. > > > > > > > > We already have https://patchwork.freedesktop.org/patch/529288/ that > > > > has been reviewed (but not applied) to resolve this. What does this > > > > version do differently and why? > > > > > > My patches only affect drm_atomic_bridge_chain_post_disable(), whereas > > > Jagan's patch affects both > > > drm_atomic_bridge_chain_post_disable() and drm_atomic_bridge_chain_pre_enable(). > > > I tested Jagan's patch on my system with success and I reviewed with > > > Michael Trimarchi the > > > drm_atomic_bridge_chain_pre_enable() fixing and we think it's okay. > > > We also believe that our changes to post_disable() are better, as we > > > set the 'next' variable only when required, > > > and the code is more optimized since the list_is_last() is not called > > > within the loop. > > > Would it be possible to use Jagan's patch for fixing > > > drm_atomic_bridge_chain_pre_enable() and mine for > > > fixing drm_atomic_bridge_chain_post_disable()? > > > > > > > Can you please share the post-disabled bridge chain list with the > > below example before and after your change? > > We have already git commit the description in how the patch affects > the post_disable. As Dario > reported your patch is ok even in our use case. We don't have a real > use case as the one you describe. > > Can we know how you test it in this use case here? Can you test our > patches of post_disable? > > Thanks > Michael > > > > > 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 > > > > Thanks, > > Jagan. Starting from my use case: # cat /sys/kernel/debug/dri/32e00000.lcdif/bridge_chains encoder[36] bridge[0] type: 16, ops: 0x0, OF: /soc@0/bus@32c00000/dsi@32e10000:fsl,imx8mn-mipi-dsim bridge[1] type: 16, ops: 0x8, OF: /soc@0/bus@32c00000/dsi@32e10000/panel@0:sharp,ls068b3sx0 I developed a pass through MIPI-DSI bridge driver to try to test your case: # cat /sys/kernel/debug/dri/32e00000.lcdif/bridge_chains encoder[36] bridge[0] type: 16, ops: 0x0, OF: /soc@0/bus@32c00000/dsi@32e10000:fsl,imx8mn-mipi-dsim bridge[1] type: 16, ops: 0x0, OF: /pt_mipi_dsi1:amarula,pt-mipi-dsi bridge[2] type: 16, ops: 0x0, OF: /pt_mipi_dsi2:amarula,pt-mipi-dsi bridge[3] type: 16, ops: 0x0, OF: /pt_mipi_dsi3:amarula,pt-mipi-dsi bridge[4] type: 16, ops: 0x0, OF: /pt_mipi_dsi4:amarula,pt-mipi-dsi bridge[5] type: 16, ops: 0x0, OF: /pt_mipi_dsi5:amarula,pt-mipi-dsi bridge[6] type: 16, ops: 0x8, OF: /pt_mipi_dsi5/panel@0:sharp,ls068b3sx02 The pre_enable_prev_first flag is set through the "amarula,pre_enable_prev_first" dts property I put in my dts. Your and my patches give the same results (result: OK) in both your use case and mine. But If I test my new "enlarged" use case: - Encoder - bridge[0] (samsung-dsim) - bridge[1] pre_enable_prev_first - bridge[2] pre_enable_prev_first - bridge[3] pre_enable_prev_first - bridge[4] pre_enable_prev_first - bridge[5] pre_enable_prev_first - bridge[6] pre_enable_prev_first (Panel) the result is: my patches: KO your patch: OK So, I will remove my patches from the series. Can the driver I implemented to test the use cases (pass through MIPI-DSI) be considered useful for testing these bridge pipelines? Does it make sense to send its patch? Thanks and regards Dario Dario Binacchi Senior Embedded Linux Developer dario.binacchi@amarulasolutions.com
On Wed, Dec 13, 2023 at 5:29 PM Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > Hi Jagan and Dave, > > On Wed, Dec 6, 2023 at 2:57 PM Michael Nazzareno Trimarchi > <michael@amarulasolutions.com> wrote: > > > > Hi Jagan > > > > On Wed, Dec 6, 2023 at 2:31 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > Hi Dario, > > > > > > On Wed, Dec 6, 2023 at 6:57 PM Dario Binacchi > > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > > > Hi Dave and Jagan, > > > > > > > > On Tue, Dec 5, 2023 at 4:39 PM Dave Stevenson > > > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > > > > > Hi Dario > > > > > > > > > > On Tue, 5 Dec 2023 at 10:54, Dario Binacchi > > > > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > > > > > > > The patch fixes the code for finding the next bridge with the > > > > > > "pre_enable_prev_first" flag set to false. In case this condition is > > > > > > not verified, i. e. there is no subsequent bridge with the flag set to > > > > > > false, the whole bridge list is traversed, invalidating the "next" > > > > > > variable. > > > > > > > > > > > > The use of a new iteration variable (i. e. "iter") ensures that the value > > > > > > of the "next" variable is not invalidated. > > > > > > > > > > We already have https://patchwork.freedesktop.org/patch/529288/ that > > > > > has been reviewed (but not applied) to resolve this. What does this > > > > > version do differently and why? > > > > > > > > My patches only affect drm_atomic_bridge_chain_post_disable(), whereas > > > > Jagan's patch affects both > > > > drm_atomic_bridge_chain_post_disable() and drm_atomic_bridge_chain_pre_enable(). > > > > I tested Jagan's patch on my system with success and I reviewed with > > > > Michael Trimarchi the > > > > drm_atomic_bridge_chain_pre_enable() fixing and we think it's okay. > > > > We also believe that our changes to post_disable() are better, as we > > > > set the 'next' variable only when required, > > > > and the code is more optimized since the list_is_last() is not called > > > > within the loop. > > > > Would it be possible to use Jagan's patch for fixing > > > > drm_atomic_bridge_chain_pre_enable() and mine for > > > > fixing drm_atomic_bridge_chain_post_disable()? > > > > > > > > > > Can you please share the post-disabled bridge chain list with the > > > below example before and after your change? > > > > We have already git commit the description in how the patch affects > > the post_disable. As Dario > > reported your patch is ok even in our use case. We don't have a real > > use case as the one you describe. > > > > Can we know how you test it in this use case here? Can you test our > > patches of post_disable? > > > > Thanks > > Michael > > > > > > > > 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 > > > > > > Thanks, > > > Jagan. > > Starting from my use case: > > # cat /sys/kernel/debug/dri/32e00000.lcdif/bridge_chains > encoder[36] > bridge[0] type: 16, ops: 0x0, OF: > /soc@0/bus@32c00000/dsi@32e10000:fsl,imx8mn-mipi-dsim > bridge[1] type: 16, ops: 0x8, OF: > /soc@0/bus@32c00000/dsi@32e10000/panel@0:sharp,ls068b3sx0 > > I developed a pass through MIPI-DSI bridge driver to try to test your case: > # cat /sys/kernel/debug/dri/32e00000.lcdif/bridge_chains > encoder[36] > bridge[0] type: 16, ops: 0x0, OF: > /soc@0/bus@32c00000/dsi@32e10000:fsl,imx8mn-mipi-dsim > bridge[1] type: 16, ops: 0x0, OF: /pt_mipi_dsi1:amarula,pt-mipi-dsi > bridge[2] type: 16, ops: 0x0, OF: /pt_mipi_dsi2:amarula,pt-mipi-dsi > bridge[3] type: 16, ops: 0x0, OF: /pt_mipi_dsi3:amarula,pt-mipi-dsi > bridge[4] type: 16, ops: 0x0, OF: /pt_mipi_dsi4:amarula,pt-mipi-dsi > bridge[5] type: 16, ops: 0x0, OF: /pt_mipi_dsi5:amarula,pt-mipi-dsi > bridge[6] type: 16, ops: 0x8, OF: /pt_mipi_dsi5/panel@0:sharp,ls068b3sx02 > > The pre_enable_prev_first flag is set through the > "amarula,pre_enable_prev_first" dts property I put > in my dts. > Your and my patches give the same results (result: OK) in both your > use case and mine. > But If I test my new "enlarged" use case: > > - Encoder > - bridge[0] (samsung-dsim) > - bridge[1] pre_enable_prev_first > - bridge[2] pre_enable_prev_first > - bridge[3] pre_enable_prev_first > - bridge[4] pre_enable_prev_first > - bridge[5] pre_enable_prev_first > - bridge[6] pre_enable_prev_first (Panel) > > the result is: > my patches: KO > your patch: OK > > So, I will remove my patches from the series. > > Can the driver I implemented to test the use cases (pass through > MIPI-DSI) be considered useful for testing these > bridge pipelines? > Does it make sense to send its patch? I don't think so, I have a similar test bench for chain of bridges. I will try to re-create the chain and update the result. Jagan.
On Wed, Dec 13, 2023 at 12:59:05PM +0100, Dario Binacchi wrote: > Hi Jagan and Dave, > > On Wed, Dec 6, 2023 at 2:57 PM Michael Nazzareno Trimarchi > <michael@amarulasolutions.com> wrote: > > > > Hi Jagan > > > > On Wed, Dec 6, 2023 at 2:31 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > Hi Dario, > > > > > > On Wed, Dec 6, 2023 at 6:57 PM Dario Binacchi > > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > > > Hi Dave and Jagan, > > > > > > > > On Tue, Dec 5, 2023 at 4:39 PM Dave Stevenson > > > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > > > > > Hi Dario > > > > > > > > > > On Tue, 5 Dec 2023 at 10:54, Dario Binacchi > > > > > <dario.binacchi@amarulasolutions.com> wrote: > > > > > > > > > > > > The patch fixes the code for finding the next bridge with the > > > > > > "pre_enable_prev_first" flag set to false. In case this condition is > > > > > > not verified, i. e. there is no subsequent bridge with the flag set to > > > > > > false, the whole bridge list is traversed, invalidating the "next" > > > > > > variable. > > > > > > > > > > > > The use of a new iteration variable (i. e. "iter") ensures that the value > > > > > > of the "next" variable is not invalidated. > > > > > > > > > > We already have https://patchwork.freedesktop.org/patch/529288/ that > > > > > has been reviewed (but not applied) to resolve this. What does this > > > > > version do differently and why? > > > > > > > > My patches only affect drm_atomic_bridge_chain_post_disable(), whereas > > > > Jagan's patch affects both > > > > drm_atomic_bridge_chain_post_disable() and drm_atomic_bridge_chain_pre_enable(). > > > > I tested Jagan's patch on my system with success and I reviewed with > > > > Michael Trimarchi the > > > > drm_atomic_bridge_chain_pre_enable() fixing and we think it's okay. > > > > We also believe that our changes to post_disable() are better, as we > > > > set the 'next' variable only when required, > > > > and the code is more optimized since the list_is_last() is not called > > > > within the loop. > > > > Would it be possible to use Jagan's patch for fixing > > > > drm_atomic_bridge_chain_pre_enable() and mine for > > > > fixing drm_atomic_bridge_chain_post_disable()? > > > > > > > > > > Can you please share the post-disabled bridge chain list with the > > > below example before and after your change? > > > > We have already git commit the description in how the patch affects > > the post_disable. As Dario > > reported your patch is ok even in our use case. We don't have a real > > use case as the one you describe. > > > > Can we know how you test it in this use case here? Can you test our > > patches of post_disable? > > > > Thanks > > Michael > > > > > > > > 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 > > > > > > Thanks, > > > Jagan. > > Starting from my use case: > > # cat /sys/kernel/debug/dri/32e00000.lcdif/bridge_chains > encoder[36] > bridge[0] type: 16, ops: 0x0, OF: > /soc@0/bus@32c00000/dsi@32e10000:fsl,imx8mn-mipi-dsim > bridge[1] type: 16, ops: 0x8, OF: > /soc@0/bus@32c00000/dsi@32e10000/panel@0:sharp,ls068b3sx0 > > I developed a pass through MIPI-DSI bridge driver to try to test your case: > # cat /sys/kernel/debug/dri/32e00000.lcdif/bridge_chains > encoder[36] > bridge[0] type: 16, ops: 0x0, OF: > /soc@0/bus@32c00000/dsi@32e10000:fsl,imx8mn-mipi-dsim > bridge[1] type: 16, ops: 0x0, OF: /pt_mipi_dsi1:amarula,pt-mipi-dsi > bridge[2] type: 16, ops: 0x0, OF: /pt_mipi_dsi2:amarula,pt-mipi-dsi > bridge[3] type: 16, ops: 0x0, OF: /pt_mipi_dsi3:amarula,pt-mipi-dsi > bridge[4] type: 16, ops: 0x0, OF: /pt_mipi_dsi4:amarula,pt-mipi-dsi > bridge[5] type: 16, ops: 0x0, OF: /pt_mipi_dsi5:amarula,pt-mipi-dsi > bridge[6] type: 16, ops: 0x8, OF: /pt_mipi_dsi5/panel@0:sharp,ls068b3sx02 > > The pre_enable_prev_first flag is set through the > "amarula,pre_enable_prev_first" dts property I put > in my dts. > Your and my patches give the same results (result: OK) in both your > use case and mine. > But If I test my new "enlarged" use case: > > - Encoder > - bridge[0] (samsung-dsim) > - bridge[1] pre_enable_prev_first > - bridge[2] pre_enable_prev_first > - bridge[3] pre_enable_prev_first > - bridge[4] pre_enable_prev_first > - bridge[5] pre_enable_prev_first > - bridge[6] pre_enable_prev_first (Panel) > > the result is: > my patches: KO > your patch: OK > > So, I will remove my patches from the series. > > Can the driver I implemented to test the use cases (pass through > MIPI-DSI) be considered useful for testing these > bridge pipelines? > Does it make sense to send its patch? As it is, not really, but kunit tests would be very welcome Maxime
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index f66bf4925dd8..2e5781bf192e 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -662,7 +662,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, *next, *limit; if (!bridge) return; @@ -680,14 +680,15 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge, * was enabled first, so disabled last */ limit = next; + iter = 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, + list_for_each_entry_from(iter, &encoder->bridge_chain, chain_node) { - if (!next->pre_enable_prev_first) { - next = list_prev_entry(next, chain_node); + if (!iter->pre_enable_prev_first) { + next = list_prev_entry(iter, chain_node); limit = next; break; }