Message ID | 20230314110043.2139111-1-treapking@chromium.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp1691266wrd; Tue, 14 Mar 2023 04:11:06 -0700 (PDT) X-Google-Smtp-Source: AK7set8xtn+maMsEWVhXinM0HEoOrhHDtiEyFnSZPHdr7476qNVvJxEHnKSjSyUhVDeJSPsCtjTq X-Received: by 2002:a17:90a:1b85:b0:23d:1f95:de1c with SMTP id w5-20020a17090a1b8500b0023d1f95de1cmr4509441pjc.28.1678792266433; Tue, 14 Mar 2023 04:11:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678792266; cv=none; d=google.com; s=arc-20160816; b=IVWVWK7o7LqfyYep75hcLErL754Z4fLuqPbH6L0c9k8IKcLdgxwahouPCBENAd10Hc RYsZWzQerqXiTPSPwcEs/aavkqZQn6aeUMxmsDPrWFA8OBeUdMVFm7usSAk36eaaDJne tnlGd1hJGsTw7fWzQt5rFash+q2n9x51U+GPlXKcWscpQNvS0Xhea7R79f73SDBvMZlm VUGahv7R9WQxSajBpN4enNGqPuKsWfOnZc1r+mzZC5pHC7tf06MyXYMUv7dBRezmhVw/ MrFwz7jg6eSQL7SB1vp4TbfDgFRPNxK7Rng0gvJz66u0EmzGB0xvLLQpwWCd2grEjjfD yg+Q== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=Js/tHDer/Yr5TRaIcV1cVBSaKjsn5E+7833Z9Y2Z5oc=; b=lRhsxRYkubwLSV+UwtZrJw00kX92CJQhRQXjaeb0hSo+uOUwLCF8taEzOkewb8O0O8 R7MuNR6PBbsXqSeArjnJCMlT08V4z8gjpH+kYUXTcCZumGna0je0+O6/RYPWJ8JbzEoZ 4ZwetaSXRhaYZPQ+WyD96EagJjjIy9ItFbcu1y+Fhcu/E/iTO/eIFzxAcAVygZxzv08r +/EhzkLKs3gXHl5vZq8OM6ONn+ijJ7H2ypMogfhoEG8lQz3d7+ZpkcmXi7cgucOeTgzw 5e0hsKZgj1lAr5NhVrKL4C0rGt87c4f9COk+x5xyn+V7/frqQ8112+LGUmJf2XpdxKat yNGQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=gInm2BDd; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id v5-20020a632f05000000b00503e9b0490fsi2016342pgv.512.2023.03.14.04.10.53; Tue, 14 Mar 2023 04:11:06 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=gInm2BDd; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229456AbjCNLAz (ORCPT <rfc822;realc9580@gmail.com> + 99 others); Tue, 14 Mar 2023 07:00:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51272 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229592AbjCNLAy (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 14 Mar 2023 07:00:54 -0400 Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 031FA5DC80 for <linux-kernel@vger.kernel.org>; Tue, 14 Mar 2023 04:00:53 -0700 (PDT) Received: by mail-pf1-x42f.google.com with SMTP id fd25so9448868pfb.1 for <linux-kernel@vger.kernel.org>; Tue, 14 Mar 2023 04:00:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1678791652; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=Js/tHDer/Yr5TRaIcV1cVBSaKjsn5E+7833Z9Y2Z5oc=; b=gInm2BDdjwVTeHhR+bNfsdsL2pH8+qdgFc7TzHVJ4615z6q0HAoAtyj5pze2EYkGTz 1jpip1tf8MIJ7tJqh0VMGZ7Zr0N3ZwCVJ1+5nwgQapiJNp7s27Y/MRo2oYpVreB3IqiZ sIto1wJGlyvGFDTiMNxtX2WNThMiOKoVNitN8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678791652; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Js/tHDer/Yr5TRaIcV1cVBSaKjsn5E+7833Z9Y2Z5oc=; b=2DGdVD7cN1L4nFl879ilLKfrkSQ/LesjOu7RecDnvwjCVgFJNF9En2lzcMmt7HxDHx 8UitGkhBkN3aCnQQMQR4Vdzk/e4x1RdWTOk1gXYaZ9gutZhqcM5rYV77ygjYvbapWFC0 1INaq/r62UCavTk7T0B/VolS7Tn3tay4u1GXLflFFnt4EAdSlTgcetGpHL5PpdxqMhAH ReLX9YIKT1ZCMAHuQajCJhaJVUFxeREnLvOxYI4dwtKgKXkAoSxYsao9ZKjal0my4jHw 5Zs/xcNxWmHqiAFPbP71yLG60GseMcATOI4L3xd9pMod5QGj2X0eme9wsGTcDYTposBO IJTQ== X-Gm-Message-State: AO0yUKWl5Pmd2YcqLu2LC5zFpyA12i7IdpV+uDJU+LfAf1Et1zpvRC6P OaXYwu/OAVcA/jfJzQW+6vbcdQ== X-Received: by 2002:aa7:9511:0:b0:625:4d29:7390 with SMTP id b17-20020aa79511000000b006254d297390mr3123574pfp.13.1678791652359; Tue, 14 Mar 2023 04:00:52 -0700 (PDT) Received: from treapking.tpe.corp.google.com ([2401:fa00:1:10:3718:fdeb:7d7e:b6c0]) by smtp.gmail.com with ESMTPSA id x2-20020a62fb02000000b005a909290425sm1362028pfm.172.2023.03.14.04.00.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Mar 2023 04:00:51 -0700 (PDT) From: Pin-yen Lin <treapking@chromium.org> To: Douglas Anderson <dianders@chromium.org>, 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> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Pin-yen Lin <treapking@chromium.org> Subject: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable Date: Tue, 14 Mar 2023 19:00:42 +0800 Message-Id: <20230314110043.2139111-1-treapking@chromium.org> X-Mailer: git-send-email 2.40.0.rc1.284.g88254d51c5-goog MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760341279603671995?= X-GMAIL-MSGID: =?utf-8?q?1760341279603671995?= |
Series |
[1/2] drm/bridge: ps8640: Skip redundant bridge enable
|
|
Commit Message
Pin-yen Lin
March 14, 2023, 11 a.m. UTC
Skip the drm_bridge_chain_pre_enable call when the bridge is already
pre_enabled. This make pre_enable and post_disable (thus
pm_runtime_get/put) symmetric.
Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Comments
On Tue, Mar 14, 2023 at 12:01 PM Pin-yen Lin <treapking@chromium.org> wrote: > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > pre_enabled. This make pre_enable and post_disable (thus > pm_runtime_get/put) symmetric. > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > --- > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 4b361d7d5e44..08de501c436e 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > * EDID, for this chip, we need to do a full poweron, otherwise it will > * fail. > */ > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > + if (poweroff) > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > edid = drm_get_edid(connector, > ps_bridge->page[PAGE0_DP_CNTL]->adapter); > -- > 2.40.0.rc1.284.g88254d51c5-goog > Reviewed-by: Robert Foss <rfoss@kernel.org>
Hi, On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > pre_enabled. This make pre_enable and post_disable (thus > pm_runtime_get/put) symmetric. > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > --- > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 4b361d7d5e44..08de501c436e 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > * EDID, for this chip, we need to do a full poweron, otherwise it will > * fail. > */ > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > + if (poweroff) > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); It always seemed weird to me that this function was asymmetric, so I like this change, thanks! I also remember wondering before how this function was safe, though. The callpath for getting here from the ioctl is documented in the function and when I look at it I wonder if anything is preventing the bridge from being enabled / disabled through normal means at the same time your function is running. That could cause all sorts of badness if it is indeed possible. Does anyone reading this know if that's indeed a problem? I suppose that, if this is unsafe, it's no more unsafe now than it was before your patch, so I guess: Reviewed-by: Douglas Anderson <dianders@chromium.org> If there are no issues, I'll plan to land this patch and the next one to drm-misc-next sometime late-ish next week.
Hi Doug, On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > > pre_enabled. This make pre_enable and post_disable (thus > > pm_runtime_get/put) symmetric. > > > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > --- > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 4b361d7d5e44..08de501c436e 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > > * EDID, for this chip, we need to do a full poweron, otherwise it will > > * fail. > > */ > > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > + if (poweroff) > > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > It always seemed weird to me that this function was asymmetric, so I > like this change, thanks! > > I also remember wondering before how this function was safe, though. > The callpath for getting here from the ioctl is documented in the > function and when I look at it I wonder if anything is preventing the > bridge from being enabled / disabled through normal means at the same > time your function is running. That could cause all sorts of badness > if it is indeed possible. Does anyone reading this know if that's > indeed a problem? If the "normal mean" is disabling the bridge, then we are probably disabling the whole display pipeline. If so, is the EDID still relevant in this case? drm_get_edid returns NULL whenever error happens, and the helpers seem to handle this case properly. > > I suppose that, if this is unsafe, it's no more unsafe now than it was > before your patch, so I guess: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > If there are no issues, I'll plan to land this patch and the next one > to drm-misc-next sometime late-ish next week. Thanks for the review. I'll submit a v2 to collect the review tags and fix up the nit in patch 2/2. Best regards, Pin-yen
Hi, On Tue, Mar 14, 2023 at 8:28 PM Pin-yen Lin <treapking@chromium.org> wrote: > > Hi Doug, > > On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > > > pre_enabled. This make pre_enable and post_disable (thus > > > pm_runtime_get/put) symmetric. > > > > > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > --- > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > index 4b361d7d5e44..08de501c436e 100644 > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > > > * EDID, for this chip, we need to do a full poweron, otherwise it will > > > * fail. > > > */ > > > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > + if (poweroff) > > > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > > It always seemed weird to me that this function was asymmetric, so I > > like this change, thanks! > > > > I also remember wondering before how this function was safe, though. > > The callpath for getting here from the ioctl is documented in the > > function and when I look at it I wonder if anything is preventing the > > bridge from being enabled / disabled through normal means at the same > > time your function is running. That could cause all sorts of badness > > if it is indeed possible. Does anyone reading this know if that's > > indeed a problem? > > If the "normal mean" is disabling the bridge, then we are probably > disabling the whole display pipeline. If so, is the EDID still > relevant in this case? In general when we do a "modeset" I believe that the display pipeline is disabled and re-enabled. On a Chromebook test image you can see this disable / re-enable happen when you switch between "VT2" and the main login screen. If the display pipeline is disabled / re-enabled then it should still be fine to keep the EDID cached, so that's not what I'm worried about. I'm more worried that someone could be querying the EDID at the same time that someone else was turning the screen off. In that case it would be possible for "poweroff" to be true (because the screen was on when we started reading the EDID) and then partway through the screen could get turned off. -Doug
Hi, On Thu, Mar 16, 2023 at 5:34 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Mar 14, 2023 at 8:28 PM Pin-yen Lin <treapking@chromium.org> wrote: > > > > Hi Doug, > > > > On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > > > > > Skip the drm_bridge_chain_pre_enable call when the bridge is already > > > > pre_enabled. This make pre_enable and post_disable (thus > > > > pm_runtime_get/put) symmetric. > > > > > > > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling") > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > > --- > > > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > index 4b361d7d5e44..08de501c436e 100644 > > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, > > > > * EDID, for this chip, we need to do a full poweron, otherwise it will > > > > * fail. > > > > */ > > > > - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > > + if (poweroff) > > > > + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); > > > > > > It always seemed weird to me that this function was asymmetric, so I > > > like this change, thanks! > > > > > > I also remember wondering before how this function was safe, though. > > > The callpath for getting here from the ioctl is documented in the > > > function and when I look at it I wonder if anything is preventing the > > > bridge from being enabled / disabled through normal means at the same > > > time your function is running. That could cause all sorts of badness > > > if it is indeed possible. Does anyone reading this know if that's > > > indeed a problem? > > > > If the "normal mean" is disabling the bridge, then we are probably > > disabling the whole display pipeline. If so, is the EDID still > > relevant in this case? > > In general when we do a "modeset" I believe that the display pipeline > is disabled and re-enabled. On a Chromebook test image you can see > this disable / re-enable happen when you switch between "VT2" and the > main login screen. > > If the display pipeline is disabled / re-enabled then it should still > be fine to keep the EDID cached, so that's not what I'm worried about. > I'm more worried that someone could be querying the EDID at the same > time that someone else was turning the screen off. In that case it > would be possible for "poweroff" to be true (because the screen was on You mean "poweroff" to be "false", right? That is, "ps_bridge->pre_enabled" is true. So the .get_edid function assumes that the pipeline is enabled, but another thread is turning off the screen. > when we started reading the EDID) and then partway through the screen > could get turned off. Thanks for the detailed explanation. In that case, we probably get an error and return a NULL EDID. But do we need the EDID when the screen is turned off? And the EDID should be re-read if the screen is turned back on. However, in a reversed setting, if the .get_edid is reading EDID when the pipeline is disabled (poweroff=true), but someone enables the pipeline in between. In that case, .get_edid might disable the bridge and panel after the pipeline is enabled. Anyway, the function is not safe, but it's no more unsafe than before. Patch 2/2 should lower the chance for anything bad to happen by adding a cache by only read EDID once. > > -Doug Thanks and regards, Pin-yen
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 4b361d7d5e44..08de501c436e 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * EDID, for this chip, we need to do a full poweron, otherwise it will * fail. */ - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); + if (poweroff) + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state); edid = drm_get_edid(connector, ps_bridge->page[PAGE0_DP_CNTL]->adapter);