Message ID | 20230113155547.RFT.2.I4cfeab9d0e07e98ead23dd0736ab4461e6c69002@changeid |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp28517wrn; Fri, 13 Jan 2023 16:03:46 -0800 (PST) X-Google-Smtp-Source: AMrXdXu6OBEn06kG7MQjDwV/7Y1JU8rozdNL4vjqJfwdCeuP4bhuPslZ4l8fUBmtYTAQiNKdIHNw X-Received: by 2002:a17:907:6f09:b0:7c1:277:cb00 with SMTP id sy9-20020a1709076f0900b007c10277cb00mr72055524ejc.23.1673654626693; Fri, 13 Jan 2023 16:03:46 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673654626; cv=none; d=google.com; s=arc-20160816; b=C0v89FhuJS+nT7fEFzH9H7+QQ5JhpTVjwbJpxTlAEoK+uPzdqaU5ZlAFca6vWq1Ot9 S8Km9w3s7xBIGpY1Sd5r+VljeB+6WFKFs2CCB2z9vhkZ76FKiQvg2/dIuQ6xzxjGlEX2 ki3nf5g2AsCIlMnRU1SYlgbBwrKzj4/dNF5hxUcGpolW6LRlOcuZyVUiNNnp+PpRbnL6 Rv58YQzTH0Cs0EuSszeQfOngTlhoCAYdSu5nxFwLg3y+ebE9+a4tAKyZRarxJRhoZhHT Yc3v2Qt3YzTPYGYwpLlU5SVY9Ldgo3NVZOBdHXeM19aF8dfsQZNLE0sYZyTVsEuJV4xa S7jA== 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=HBRAbkUwcVHYnpIoYhFS8Gcgp00+RQqgzwfjMcTRfn8=; b=KhOiCYkDs8C6UkosRrnpWwMZhnbyXnINk+45CmDyG4PXPjNedLbEEooIIsJSK/dqkn XP27VbaFl64U4AoseATdl9RuO9WC+Ig0bo+swih+2tKnjemR0VQYBcsblh3+46isINCR SqUxxnRhvxfxLi7iSAqrULxj9NnQ1g2IMTQrumFleA6bRafiQgHDG/Zu6smynd43AHZI VFTCqLn3GR854p/HzD8wuXS11NzLodc6WaqGl0tmXBfOdK8fN8yo1zVYgcChiiorUlXl t5pr8/76IVVbyAx89Yj6bblCvIoDdgThpO8EluEK/9U6rA2lZEkHKiBIfjNIIhCBi9zK CO0A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=CBsLcSyA; 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 sa4-20020a1709076d0400b0078fcfc5d24esi24904548ejc.618.2023.01.13.16.03.21; Fri, 13 Jan 2023 16:03:46 -0800 (PST) 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=CBsLcSyA; 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 S231214AbjAMX4p (ORCPT <rfc822;stefanalexe48@gmail.com> + 99 others); Fri, 13 Jan 2023 18:56:45 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48066 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230347AbjAMX4h (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 13 Jan 2023 18:56:37 -0500 Received: from mail-pg1-x536.google.com (mail-pg1-x536.google.com [IPv6:2607:f8b0:4864:20::536]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C0CEE7466F for <linux-kernel@vger.kernel.org>; Fri, 13 Jan 2023 15:56:36 -0800 (PST) Received: by mail-pg1-x536.google.com with SMTP id 141so16041572pgc.0 for <linux-kernel@vger.kernel.org>; Fri, 13 Jan 2023 15:56:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; 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=HBRAbkUwcVHYnpIoYhFS8Gcgp00+RQqgzwfjMcTRfn8=; b=CBsLcSyAmbSqlhmELttzLHiRFV0Pbee815YWapJN6gF0sd2nxMN5ceXTUwfPWY1vb6 tQQUO8D+NzkbrARK/MSF+73CIpPAtT7QqL2Di6XBOZcjUCUevQh67WapL9qMwFjKqkIJ ZHcoRYRPJ2BuMgAhKxtLI4kUOmqUNo1orQW1k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=HBRAbkUwcVHYnpIoYhFS8Gcgp00+RQqgzwfjMcTRfn8=; b=jUn2Vvf2kDAN53FIMDjsaYE5SvqgiYYGWwl5IK9GoGXz+JwS/u4WyfWdKNPyNWImNF f8dt/iOggA5CzF6GWTxvjCcbw9EyZe4C7j8qw81WferVsUTkUjK71zRyCyqu702ZSmfr 3r+qSaDPMboLALHF6+TkOO+sxY6rr+tKxjee4W8OoFqDbrsvsMGZ485r3dY53Y5ArW7J uT+k3AKFjo7orqpM3wF5905Z2qsRnTEbooIoT4rdqmJxw2ez+cLSyOkQidLqsgjZuJ0F l2DDZHK+UefRrMm9BAM69NwfeXI/HiO0Kxn5ePKQEs0U4LLvqycJZgmgIQk8nFyJuQAT v73A== X-Gm-Message-State: AFqh2kpstvfX1fIR0T/7IaC2g3BBcK4y8zLNOuxw2QdlbvtgmEKopJAg GyCjRP37Eksd3Yy/rOsi51IVW7uC0Je++XFh X-Received: by 2002:a62:1693:0:b0:583:9b05:d1f0 with SMTP id 141-20020a621693000000b005839b05d1f0mr25361391pfw.33.1673654196266; Fri, 13 Jan 2023 15:56:36 -0800 (PST) Received: from tictac2.mtv.corp.google.com ([2620:15c:9d:2:b4ad:3c49:1195:a01e]) by smtp.gmail.com with ESMTPSA id f10-20020aa79d8a000000b005823b7da05asm10002737pfq.122.2023.01.13.15.56.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 13 Jan 2023 15:56:35 -0800 (PST) From: Douglas Anderson <dianders@chromium.org> To: dri-devel@lists.freedesktop.org, Rob Clark <robdclark@gmail.com>, Abhinav Kumar <quic_abhinavk@quicinc.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: Daniel Vetter <daniel@ffwll.ch>, Andrzej Hajda <andrzej.hajda@intel.com>, Dave Stevenson <dave.stevenson@raspberrypi.com>, Robert Foss <robert.foss@linaro.org>, Sean Paul <sean@poorly.run>, Jonas Karlman <jonas@kwiboo.se>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Neil Armstrong <neil.armstrong@linaro.org>, David Airlie <airlied@gmail.com>, linux-arm-msm@vger.kernel.org, Stephen Boyd <swboyd@chromium.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Vinod Koul <vkoul@kernel.org>, Douglas Anderson <dianders@chromium.org>, freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org, ye xingchen <ye.xingchen@zte.com.cn> Subject: [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset Date: Fri, 13 Jan 2023 15:56:04 -0800 Message-Id: <20230113155547.RFT.2.I4cfeab9d0e07e98ead23dd0736ab4461e6c69002@changeid> X-Mailer: git-send-email 2.39.0.314.g84b9a713c41-goog In-Reply-To: <20230113155547.RFT.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid> References: <20230113155547.RFT.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid> 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 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?1754954073892310588?= X-GMAIL-MSGID: =?utf-8?q?1754954073892310588?= |
Series |
[RFT,1/2] drm/bridge: tc358762: Set pre_enable_prev_first
|
|
Commit Message
Doug Anderson
Jan. 13, 2023, 11:56 p.m. UTC
In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time"), we moved powering up DSI hosts to modeset time. This wasn't
because it was an elegant design, but there were no better options.
That commit actually ended up breaking ps8640, and thus was born
commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
the old way of doing things. It turns out that ps8640 _really_ doesn't
like its pre_enable() function to be called after
dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
because I have any inside knowledge), it looks like the assertion of
"RST#" in the ps8640 runtime resume handler seems like it's not
allowed to happen after dsi_mgr_bridge_power_on()
Recently, Dave Stevenson's series landed allowing bridges some control
over pre_enable ordering. The meaty commit for our purposes is commit
4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
bridge init order"). As documented by that series, if a bridge doesn't
set "pre_enable_prev_first" then we should use the old ordering.
Now that we have the commit ("drm/bridge: tc358762: Set
pre_enable_prev_first") we can go back to the old ordering, which also
allows us to remove the ps8640 special case.
One last note is that even without reverting commit 7d8e9a90509f
("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
revert the ps8640 special case and try it out then it doesn't seem to
fail anymore. I spent time bisecting / debugging this and it turns out
to be mostly luck, so we still want this patch to make sure it's
solid. Specifically the reason it sorta works these days is because
we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
"pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
implemented means that we actually power the bridge chip up just a wee
bit earlier and then the bridge happens to stay on because of
autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++----------------------
1 file changed, 11 insertions(+), 57 deletions(-)
Comments
Hi, On Fri, Jan 13, 2023 at 3:56 PM Douglas Anderson <dianders@chromium.org> wrote: > > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > time"), we moved powering up DSI hosts to modeset time. This wasn't > because it was an elegant design, but there were no better options. > > That commit actually ended up breaking ps8640, and thus was born > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to > the old way of doing things. It turns out that ps8640 _really_ doesn't > like its pre_enable() function to be called after > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not > because I have any inside knowledge), it looks like the assertion of > "RST#" in the ps8640 runtime resume handler seems like it's not > allowed to happen after dsi_mgr_bridge_power_on() > > Recently, Dave Stevenson's series landed allowing bridges some control > over pre_enable ordering. The meaty commit for our purposes is commit > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter > bridge init order"). As documented by that series, if a bridge doesn't > set "pre_enable_prev_first" then we should use the old ordering. > > Now that we have the commit ("drm/bridge: tc358762: Set > pre_enable_prev_first") we can go back to the old ordering, which also > allows us to remove the ps8640 special case. > > One last note is that even without reverting commit 7d8e9a90509f > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ > revert the ps8640 special case and try it out then it doesn't seem to > fail anymore. I spent time bisecting / debugging this and it turns out > to be mostly luck, so we still want this patch to make sure it's > solid. Specifically the reason it sorta works these days is because > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() > implemented means that we actually power the bridge chip up just a wee > bit earlier and then the bridge happens to stay on because of > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). > > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++---------------------- > 1 file changed, 11 insertions(+), 57 deletions(-) Does anyone have any comments on this patch series? It would probably make sense to wait to land until early in a kernel's release cycle, so perhaps there is no hurry. That being said, it would still be good to know what the plan is. Abhinav: I think you're the one that was most concerned with removing the special case for ps8640. Does that mean you'd be willing to review this patch? For whether or not the "tc358762" panel works with the MSM display driver after this series, are the correct people on this thread? Thanks! -Doug
Hi, On Sat, 14 Jan 2023 at 01:56, Douglas Anderson <dianders@chromium.org> wrote: > > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > time"), we moved powering up DSI hosts to modeset time. This wasn't > because it was an elegant design, but there were no better options. > > That commit actually ended up breaking ps8640, and thus was born > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to > the old way of doing things. It turns out that ps8640 _really_ doesn't > like its pre_enable() function to be called after > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not > because I have any inside knowledge), it looks like the assertion of > "RST#" in the ps8640 runtime resume handler seems like it's not > allowed to happen after dsi_mgr_bridge_power_on() > > Recently, Dave Stevenson's series landed allowing bridges some control > over pre_enable ordering. The meaty commit for our purposes is commit > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter > bridge init order"). As documented by that series, if a bridge doesn't > set "pre_enable_prev_first" then we should use the old ordering. > > Now that we have the commit ("drm/bridge: tc358762: Set > pre_enable_prev_first") we can go back to the old ordering, which also > allows us to remove the ps8640 special case. > > One last note is that even without reverting commit 7d8e9a90509f > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ > revert the ps8640 special case and try it out then it doesn't seem to > fail anymore. I spent time bisecting / debugging this and it turns out > to be mostly luck, so we still want this patch to make sure it's > solid. Specifically the reason it sorta works these days is because > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() > implemented means that we actually power the bridge chip up just a wee > bit earlier and then the bridge happens to stay on because of > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). I had a small comment on your patch, but then I was distracted and forgot to send it. See below. > > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++---------------------- > 1 file changed, 11 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index 3a1417397283..5e6b8d423b96 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb; > #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) > #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) > > -#ifdef CONFIG_OF > -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge) > -{ > - struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge); > - > - /* > - * If the next bridge in the chain is the Parade ps8640 bridge chip > - * then don't power on early since it seems to violate the expectations > - * of the firmware that the bridge chip is running. > - * > - * NOTE: this is expected to be a temporary special case. It's expected > - * that we'll eventually have a framework that allows the next level > - * bridge to indicate whether it needs us to power on before it or > - * after it. When that framework is in place then we'll use it and > - * remove this special case. > - */ > - return !(next_bridge && next_bridge->of_node && > - of_device_is_compatible(next_bridge->of_node, "parade,ps8640")); > -} > -#else > -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge) > -{ > - return true; > -} > -#endif > - > static inline struct msm_dsi *dsi_mgr_get_dsi(int id) > { > return msm_dsim_glb.dsi[id]; > @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) > } > } > > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) Can you please keep the dsi_mgr_bridge_power_on() as is and just remove the now-legacy dsi_mgr_power_on_early(). > { > int id = dsi_mgr_bridge_get_id(bridge); > struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > @@ -300,36 +274,10 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > if (is_bonded_dsi && msm_dsi1) > msm_dsi_host_enable_irq(msm_dsi1->host); > > - return; > - > -host1_on_fail: > - msm_dsi_host_power_off(host); > -host_on_fail: > - dsi_mgr_phy_disable(id); > -phy_en_fail: > - return; > -} > - > -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > -{ > - int id = dsi_mgr_bridge_get_id(bridge); > - struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > - struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > - struct mipi_dsi_host *host = msm_dsi->host; > - bool is_bonded_dsi = IS_BONDED_DSI(); > - int ret; > - > - DBG("id=%d", id); > - if (!msm_dsi_device_connected(msm_dsi)) > - return; > - > /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ > if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) > return; > > - if (!dsi_mgr_power_on_early(bridge)) > - dsi_mgr_bridge_power_on(bridge); > - > ret = msm_dsi_host_enable(host); > if (ret) { > pr_err("%s: enable host %d failed, %d\n", __func__, id, ret); > @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > host1_en_fail: > msm_dsi_host_disable(host); > host_en_fail: > - > + msm_dsi_host_disable_irq(host); > + if (is_bonded_dsi && msm_dsi1) { > + msm_dsi_host_disable_irq(msm_dsi1->host); > + msm_dsi_host_power_off(msm_dsi1->host); > + } > +host1_on_fail: > + msm_dsi_host_power_off(host); > +host_on_fail: > + dsi_mgr_phy_disable(id); > +phy_en_fail: > return; > } > > @@ -438,9 +395,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, > msm_dsi_host_set_display_mode(host, adjusted_mode); > if (is_bonded_dsi && other_dsi) > msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); > - > - if (dsi_mgr_power_on_early(bridge)) > - dsi_mgr_bridge_power_on(bridge); > } > > static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge, > -- > 2.39.0.314.g84b9a713c41-goog >
Hi, On Thu, Jan 26, 2023 at 9:49 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > Hi, > > On Sat, 14 Jan 2023 at 01:56, Douglas Anderson <dianders@chromium.org> wrote: > > > > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > > time"), we moved powering up DSI hosts to modeset time. This wasn't > > because it was an elegant design, but there were no better options. > > > > That commit actually ended up breaking ps8640, and thus was born > > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for > > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to > > the old way of doing things. It turns out that ps8640 _really_ doesn't > > like its pre_enable() function to be called after > > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not > > because I have any inside knowledge), it looks like the assertion of > > "RST#" in the ps8640 runtime resume handler seems like it's not > > allowed to happen after dsi_mgr_bridge_power_on() > > > > Recently, Dave Stevenson's series landed allowing bridges some control > > over pre_enable ordering. The meaty commit for our purposes is commit > > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter > > bridge init order"). As documented by that series, if a bridge doesn't > > set "pre_enable_prev_first" then we should use the old ordering. > > > > Now that we have the commit ("drm/bridge: tc358762: Set > > pre_enable_prev_first") we can go back to the old ordering, which also > > allows us to remove the ps8640 special case. > > > > One last note is that even without reverting commit 7d8e9a90509f > > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ > > revert the ps8640 special case and try it out then it doesn't seem to > > fail anymore. I spent time bisecting / debugging this and it turns out > > to be mostly luck, so we still want this patch to make sure it's > > solid. Specifically the reason it sorta works these days is because > > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of > > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() > > implemented means that we actually power the bridge chip up just a wee > > bit earlier and then the bridge happens to stay on because of > > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). > > I had a small comment on your patch, but then I was distracted and > forgot to send it. See below. > > > > > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++---------------------- > > 1 file changed, 11 insertions(+), 57 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > index 3a1417397283..5e6b8d423b96 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb; > > #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) > > #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) > > > > -#ifdef CONFIG_OF > > -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge) > > -{ > > - struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge); > > - > > - /* > > - * If the next bridge in the chain is the Parade ps8640 bridge chip > > - * then don't power on early since it seems to violate the expectations > > - * of the firmware that the bridge chip is running. > > - * > > - * NOTE: this is expected to be a temporary special case. It's expected > > - * that we'll eventually have a framework that allows the next level > > - * bridge to indicate whether it needs us to power on before it or > > - * after it. When that framework is in place then we'll use it and > > - * remove this special case. > > - */ > > - return !(next_bridge && next_bridge->of_node && > > - of_device_is_compatible(next_bridge->of_node, "parade,ps8640")); > > -} > > -#else > > -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge) > > -{ > > - return true; > > -} > > -#endif > > - > > static inline struct msm_dsi *dsi_mgr_get_dsi(int id) > > { > > return msm_dsim_glb.dsi[id]; > > @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) > > } > > } > > > > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > > Can you please keep the dsi_mgr_bridge_power_on() as is and just > remove the now-legacy dsi_mgr_power_on_early(). By this, I assume you mean keep the function separate but still remove the call to it from "modeset" and unconditionally call it from dsi_mgr_bridge_pre_enable(), right? > > { > > int id = dsi_mgr_bridge_get_id(bridge); > > struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > > @@ -300,36 +274,10 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > if (is_bonded_dsi && msm_dsi1) > > msm_dsi_host_enable_irq(msm_dsi1->host); > > > > - return; > > - > > -host1_on_fail: > > - msm_dsi_host_power_off(host); > > -host_on_fail: > > - dsi_mgr_phy_disable(id); > > -phy_en_fail: > > - return; > > -} > > - > > -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > > -{ > > - int id = dsi_mgr_bridge_get_id(bridge); > > - struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > > - struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > > - struct mipi_dsi_host *host = msm_dsi->host; > > - bool is_bonded_dsi = IS_BONDED_DSI(); > > - int ret; > > - > > - DBG("id=%d", id); > > - if (!msm_dsi_device_connected(msm_dsi)) > > - return; > > - > > /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ > > if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) > > return; > > > > - if (!dsi_mgr_power_on_early(bridge)) > > - dsi_mgr_bridge_power_on(bridge); > > - > > ret = msm_dsi_host_enable(host); > > if (ret) { > > pr_err("%s: enable host %d failed, %d\n", __func__, id, ret); > > @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > > host1_en_fail: > > msm_dsi_host_disable(host); > > host_en_fail: > > - > > + msm_dsi_host_disable_irq(host); > > + if (is_bonded_dsi && msm_dsi1) { > > + msm_dsi_host_disable_irq(msm_dsi1->host); > > + msm_dsi_host_power_off(msm_dsi1->host); > > + } > > +host1_on_fail: > > + msm_dsi_host_power_off(host); > > +host_on_fail: > > + dsi_mgr_phy_disable(id); > > +phy_en_fail: > > return; > > } > > > > @@ -438,9 +395,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, > > msm_dsi_host_set_display_mode(host, adjusted_mode); > > if (is_bonded_dsi && other_dsi) > > msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); > > - > > - if (dsi_mgr_power_on_early(bridge)) > > - dsi_mgr_bridge_power_on(bridge); > > } > > > > static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge, > > -- > > 2.39.0.314.g84b9a713c41-goog > > > > > -- > With best wishes > Dmitry
On 27/01/2023 18:32, Doug Anderson wrote: > Hi, > > On Thu, Jan 26, 2023 at 9:49 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> Hi, >> >> On Sat, 14 Jan 2023 at 01:56, Douglas Anderson <dianders@chromium.org> wrote: >>> >>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset >>> time"), we moved powering up DSI hosts to modeset time. This wasn't >>> because it was an elegant design, but there were no better options. >>> >>> That commit actually ended up breaking ps8640, and thus was born >>> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for >>> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to >>> the old way of doing things. It turns out that ps8640 _really_ doesn't >>> like its pre_enable() function to be called after >>> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not >>> because I have any inside knowledge), it looks like the assertion of >>> "RST#" in the ps8640 runtime resume handler seems like it's not >>> allowed to happen after dsi_mgr_bridge_power_on() >>> >>> Recently, Dave Stevenson's series landed allowing bridges some control >>> over pre_enable ordering. The meaty commit for our purposes is commit >>> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter >>> bridge init order"). As documented by that series, if a bridge doesn't >>> set "pre_enable_prev_first" then we should use the old ordering. >>> >>> Now that we have the commit ("drm/bridge: tc358762: Set >>> pre_enable_prev_first") we can go back to the old ordering, which also >>> allows us to remove the ps8640 special case. >>> >>> One last note is that even without reverting commit 7d8e9a90509f >>> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ >>> revert the ps8640 special case and try it out then it doesn't seem to >>> fail anymore. I spent time bisecting / debugging this and it turns out >>> to be mostly luck, so we still want this patch to make sure it's >>> solid. Specifically the reason it sorta works these days is because >>> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of >>> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() >>> implemented means that we actually power the bridge chip up just a wee >>> bit earlier and then the bridge happens to stay on because of >>> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). >> >> I had a small comment on your patch, but then I was distracted and >> forgot to send it. See below. >> >>> >>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> >>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> --- >>> >>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++---------------------- >>> 1 file changed, 11 insertions(+), 57 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> index 3a1417397283..5e6b8d423b96 100644 >>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb; >>> #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) >>> #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) >>> >>> -#ifdef CONFIG_OF >>> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge) >>> -{ >>> - struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge); >>> - >>> - /* >>> - * If the next bridge in the chain is the Parade ps8640 bridge chip >>> - * then don't power on early since it seems to violate the expectations >>> - * of the firmware that the bridge chip is running. >>> - * >>> - * NOTE: this is expected to be a temporary special case. It's expected >>> - * that we'll eventually have a framework that allows the next level >>> - * bridge to indicate whether it needs us to power on before it or >>> - * after it. When that framework is in place then we'll use it and >>> - * remove this special case. >>> - */ >>> - return !(next_bridge && next_bridge->of_node && >>> - of_device_is_compatible(next_bridge->of_node, "parade,ps8640")); >>> -} >>> -#else >>> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge) >>> -{ >>> - return true; >>> -} >>> -#endif >>> - >>> static inline struct msm_dsi *dsi_mgr_get_dsi(int id) >>> { >>> return msm_dsim_glb.dsi[id]; >>> @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) >>> } >>> } >>> >>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) >>> +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >> >> Can you please keep the dsi_mgr_bridge_power_on() as is and just >> remove the now-legacy dsi_mgr_power_on_early(). > > By this, I assume you mean keep the function separate but still remove > the call to it from "modeset" and unconditionally call it from > dsi_mgr_bridge_pre_enable(), right? Yes, of course. > > >>> { >>> int id = dsi_mgr_bridge_get_id(bridge); >>> struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); >>> @@ -300,36 +274,10 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) >>> if (is_bonded_dsi && msm_dsi1) >>> msm_dsi_host_enable_irq(msm_dsi1->host); >>> >>> - return; >>> - >>> -host1_on_fail: >>> - msm_dsi_host_power_off(host); >>> -host_on_fail: >>> - dsi_mgr_phy_disable(id); >>> -phy_en_fail: >>> - return; >>> -} >>> - >>> -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>> -{ >>> - int id = dsi_mgr_bridge_get_id(bridge); >>> - struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); >>> - struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); >>> - struct mipi_dsi_host *host = msm_dsi->host; >>> - bool is_bonded_dsi = IS_BONDED_DSI(); >>> - int ret; >>> - >>> - DBG("id=%d", id); >>> - if (!msm_dsi_device_connected(msm_dsi)) >>> - return; >>> - >>> /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ >>> if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) >>> return; >>> >>> - if (!dsi_mgr_power_on_early(bridge)) >>> - dsi_mgr_bridge_power_on(bridge); >>> - >>> ret = msm_dsi_host_enable(host); >>> if (ret) { >>> pr_err("%s: enable host %d failed, %d\n", __func__, id, ret); >>> @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>> host1_en_fail: >>> msm_dsi_host_disable(host); >>> host_en_fail: >>> - >>> + msm_dsi_host_disable_irq(host); >>> + if (is_bonded_dsi && msm_dsi1) { >>> + msm_dsi_host_disable_irq(msm_dsi1->host); >>> + msm_dsi_host_power_off(msm_dsi1->host); >>> + } >>> +host1_on_fail: >>> + msm_dsi_host_power_off(host); >>> +host_on_fail: >>> + dsi_mgr_phy_disable(id); >>> +phy_en_fail: >>> return; >>> } >>> >>> @@ -438,9 +395,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, >>> msm_dsi_host_set_display_mode(host, adjusted_mode); >>> if (is_bonded_dsi && other_dsi) >>> msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); >>> - >>> - if (dsi_mgr_power_on_early(bridge)) >>> - dsi_mgr_bridge_power_on(bridge); >>> } >>> >>> static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge, >>> -- >>> 2.39.0.314.g84b9a713c41-goog >>> >> >> >> -- >> With best wishes >> Dmitry
On 1/13/2023 3:56 PM, Douglas Anderson wrote: > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > time"), we moved powering up DSI hosts to modeset time. This wasn't > because it was an elegant design, but there were no better options. > > That commit actually ended up breaking ps8640, and thus was born > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to > the old way of doing things. It turns out that ps8640 _really_ doesn't > like its pre_enable() function to be called after > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not > because I have any inside knowledge), it looks like the assertion of > "RST#" in the ps8640 runtime resume handler seems like it's not > allowed to happen after dsi_mgr_bridge_power_on() > > Recently, Dave Stevenson's series landed allowing bridges some control > over pre_enable ordering. The meaty commit for our purposes is commit > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter > bridge init order"). As documented by that series, if a bridge doesn't > set "pre_enable_prev_first" then we should use the old ordering. > > Now that we have the commit ("drm/bridge: tc358762: Set > pre_enable_prev_first") we can go back to the old ordering, which also > allows us to remove the ps8640 special case. > > One last note is that even without reverting commit 7d8e9a90509f > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ > revert the ps8640 special case and try it out then it doesn't seem to > fail anymore. I spent time bisecting / debugging this and it turns out > to be mostly luck, so we still want this patch to make sure it's > solid. Specifically the reason it sorta works these days is because > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() > implemented means that we actually power the bridge chip up just a wee > bit earlier and then the bridge happens to stay on because of > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). > > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> Why is the patch title showing 2/2? I am not seeing any 1/2 here. > --- > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++---------------------- > 1 file changed, 11 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index 3a1417397283..5e6b8d423b96 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb; > #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) > #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) > > -#ifdef CONFIG_OF > -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge) > -{ > - struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge); > - > - /* > - * If the next bridge in the chain is the Parade ps8640 bridge chip > - * then don't power on early since it seems to violate the expectations > - * of the firmware that the bridge chip is running. > - * > - * NOTE: this is expected to be a temporary special case. It's expected > - * that we'll eventually have a framework that allows the next level > - * bridge to indicate whether it needs us to power on before it or > - * after it. When that framework is in place then we'll use it and > - * remove this special case. > - */ > - return !(next_bridge && next_bridge->of_node && > - of_device_is_compatible(next_bridge->of_node, "parade,ps8640")); > -} > -#else > -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge) > -{ > - return true; > -} > -#endif > - > static inline struct msm_dsi *dsi_mgr_get_dsi(int id) > { > return msm_dsim_glb.dsi[id]; > @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) > } > } > > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > { > int id = dsi_mgr_bridge_get_id(bridge); > struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > @@ -300,36 +274,10 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > if (is_bonded_dsi && msm_dsi1) > msm_dsi_host_enable_irq(msm_dsi1->host); > > - return; > - > -host1_on_fail: > - msm_dsi_host_power_off(host); > -host_on_fail: > - dsi_mgr_phy_disable(id); > -phy_en_fail: > - return; > -} > - > -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > -{ > - int id = dsi_mgr_bridge_get_id(bridge); > - struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > - struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > - struct mipi_dsi_host *host = msm_dsi->host; > - bool is_bonded_dsi = IS_BONDED_DSI(); > - int ret; > - > - DBG("id=%d", id); > - if (!msm_dsi_device_connected(msm_dsi)) > - return; > - > /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ > if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) > return; > > - if (!dsi_mgr_power_on_early(bridge)) > - dsi_mgr_bridge_power_on(bridge); > - > ret = msm_dsi_host_enable(host); > if (ret) { > pr_err("%s: enable host %d failed, %d\n", __func__, id, ret); > @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > host1_en_fail: > msm_dsi_host_disable(host); > host_en_fail: > - > + msm_dsi_host_disable_irq(host); > + if (is_bonded_dsi && msm_dsi1) { > + msm_dsi_host_disable_irq(msm_dsi1->host); > + msm_dsi_host_power_off(msm_dsi1->host); > + } In addition to Dmitry's comment of keeping the bridge_power_on() name, this part of the change seems independent of the patch. This was missing cleanup for DSI1 (esp the disable_irq part). So can we break it up into two parts. 1) Add missing cleanup for DSI1 2) Just get rid of dsi_mgr_power_on_early() and keep the call dsi_mgr_bridge_power_on() in dsi_mgr_bridge_pre_enable() unconditionally. > +host1_on_fail: > + msm_dsi_host_power_off(host); > +host_on_fail: > + dsi_mgr_phy_disable(id); > +phy_en_fail: > return; > } > > @@ -438,9 +395,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, > msm_dsi_host_set_display_mode(host, adjusted_mode); > if (is_bonded_dsi && other_dsi) > msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); > - > - if (dsi_mgr_power_on_early(bridge)) > - dsi_mgr_bridge_power_on(bridge); > } > > static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
Hi, On Fri, Jan 27, 2023 at 10:54 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > On 1/13/2023 3:56 PM, Douglas Anderson wrote: > > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > > time"), we moved powering up DSI hosts to modeset time. This wasn't > > because it was an elegant design, but there were no better options. > > > > That commit actually ended up breaking ps8640, and thus was born > > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for > > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to > > the old way of doing things. It turns out that ps8640 _really_ doesn't > > like its pre_enable() function to be called after > > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not > > because I have any inside knowledge), it looks like the assertion of > > "RST#" in the ps8640 runtime resume handler seems like it's not > > allowed to happen after dsi_mgr_bridge_power_on() > > > > Recently, Dave Stevenson's series landed allowing bridges some control > > over pre_enable ordering. The meaty commit for our purposes is commit > > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter > > bridge init order"). As documented by that series, if a bridge doesn't > > set "pre_enable_prev_first" then we should use the old ordering. > > > > Now that we have the commit ("drm/bridge: tc358762: Set > > pre_enable_prev_first") we can go back to the old ordering, which also > > allows us to remove the ps8640 special case. > > > > One last note is that even without reverting commit 7d8e9a90509f > > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ > > revert the ps8640 special case and try it out then it doesn't seem to > > fail anymore. I spent time bisecting / debugging this and it turns out > > to be mostly luck, so we still want this patch to make sure it's > > solid. Specifically the reason it sorta works these days is because > > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of > > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() > > implemented means that we actually power the bridge chip up just a wee > > bit earlier and then the bridge happens to stay on because of > > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). > > > > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Why is the patch title showing 2/2? I am not seeing any 1/2 here. Is it a problem with your mail filters? You can see it at: https://lore.kernel.org/r/20230113155547.RFT.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid/ You are listed on the "To:" line. ;-) > > @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > > host1_en_fail: > > msm_dsi_host_disable(host); > > host_en_fail: > > - > > + msm_dsi_host_disable_irq(host); > > + if (is_bonded_dsi && msm_dsi1) { > > + msm_dsi_host_disable_irq(msm_dsi1->host); > > + msm_dsi_host_power_off(msm_dsi1->host); > > + } > > In addition to Dmitry's comment of keeping the bridge_power_on() name, > > this part of the change seems independent of the patch. This was missing > cleanup for DSI1 (esp the disable_irq part). > > So can we break it up into two parts. > > 1) Add missing cleanup for DSI1 > 2) Just get rid of dsi_mgr_power_on_early() and keep the call > dsi_mgr_bridge_power_on() in dsi_mgr_bridge_pre_enable() unconditionally. I didn't intentionally fix any bug in my patch--I just reverted it all back to how it was before. ;-) So looking more closely, it looks like overall the current code (AKA what's landed today and without ${SUBJECT} patch) doesn't really handle errors with dsi_mgr_bridge_power_on() very well. The normal case of calling dsi_mgr_bridge_power_on() from modeset is totally ignored because modeset returns no error. Then the special workaround for ps8640 just followed the same pattern and assumed that dsi_mgr_bridge_power_on() succeeded. It also assumed that if the rest of dsi_mgr_bridge_pre_enable() failed that it didn't need to undo dsi_mgr_bridge_power_on() because it wouldn't have undone it in the modeset case. While the current code isn't the best, it's not like the pre_enable() call could have returned errors anyway. It probably wasn't truly the end of the world to behave the way it did. With all that, I guess my plan would be to do as Dmitry says and just always call dsi_mgr_bridge_power_on() from dsi_mgr_bridge_pre_enable(). In the first patch I'll just do that and remove the ps8640 workaround. Then I can add a 2nd patch that improves the error handling by having dsi_mgr_bridge_power_on() return an error code and then adding a matching dsi_mgr_bridge_power_off() that will undo it and include the extra cleanup. -Doug
On 1/27/2023 2:33 PM, Doug Anderson wrote: > Hi, > > On Fri, Jan 27, 2023 at 10:54 AM Abhinav Kumar > <quic_abhinavk@quicinc.com> wrote: >> >> On 1/13/2023 3:56 PM, Douglas Anderson wrote: >>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset >>> time"), we moved powering up DSI hosts to modeset time. This wasn't >>> because it was an elegant design, but there were no better options. >>> >>> That commit actually ended up breaking ps8640, and thus was born >>> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for >>> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to >>> the old way of doing things. It turns out that ps8640 _really_ doesn't >>> like its pre_enable() function to be called after >>> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not >>> because I have any inside knowledge), it looks like the assertion of >>> "RST#" in the ps8640 runtime resume handler seems like it's not >>> allowed to happen after dsi_mgr_bridge_power_on() >>> >>> Recently, Dave Stevenson's series landed allowing bridges some control >>> over pre_enable ordering. The meaty commit for our purposes is commit >>> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter >>> bridge init order"). As documented by that series, if a bridge doesn't >>> set "pre_enable_prev_first" then we should use the old ordering. >>> >>> Now that we have the commit ("drm/bridge: tc358762: Set >>> pre_enable_prev_first") we can go back to the old ordering, which also >>> allows us to remove the ps8640 special case. >>> >>> One last note is that even without reverting commit 7d8e9a90509f >>> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_ >>> revert the ps8640 special case and try it out then it doesn't seem to >>> fail anymore. I spent time bisecting / debugging this and it turns out >>> to be mostly luck, so we still want this patch to make sure it's >>> solid. Specifically the reason it sorta works these days is because >>> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of >>> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted() >>> implemented means that we actually power the bridge chip up just a wee >>> bit earlier and then the bridge happens to stay on because of >>> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on(). >>> >>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> >>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> >> Why is the patch title showing 2/2? I am not seeing any 1/2 here. > > Is it a problem with your mail filters? You can see it at: > > https://lore.kernel.org/r/20230113155547.RFT.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid/ > > You are listed on the "To:" line. ;-) Ah, I see what happened. The first patch did not have freedreno CCed but the second one did. So freedreno PW got confused thinking , hey where is the first patch? :) https://patchwork.freedesktop.org/series/112824/ And so did I :) Perhaps freedreno should be CCed on both patches because its a series. > > >>> @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>> host1_en_fail: >>> msm_dsi_host_disable(host); >>> host_en_fail: >>> - >>> + msm_dsi_host_disable_irq(host); >>> + if (is_bonded_dsi && msm_dsi1) { >>> + msm_dsi_host_disable_irq(msm_dsi1->host); >>> + msm_dsi_host_power_off(msm_dsi1->host); >>> + } >> >> In addition to Dmitry's comment of keeping the bridge_power_on() name, >> >> this part of the change seems independent of the patch. This was missing >> cleanup for DSI1 (esp the disable_irq part). >> >> So can we break it up into two parts. >> >> 1) Add missing cleanup for DSI1 >> 2) Just get rid of dsi_mgr_power_on_early() and keep the call >> dsi_mgr_bridge_power_on() in dsi_mgr_bridge_pre_enable() unconditionally. > > I didn't intentionally fix any bug in my patch--I just reverted it all > back to how it was before. ;-) > No sure what I am missing here but I certainly dont see msm_dsi_host_disable_irq() being part of any error handling labels which made me think you fixed that. > So looking more closely, it looks like overall the current code (AKA > what's landed today and without ${SUBJECT} patch) doesn't really > handle errors with dsi_mgr_bridge_power_on() very well. The normal > case of calling dsi_mgr_bridge_power_on() from modeset is totally > ignored because modeset returns no error. Then the special workaround > for ps8640 just followed the same pattern and assumed that > dsi_mgr_bridge_power_on() succeeded. It also assumed that if the rest > of dsi_mgr_bridge_pre_enable() failed that it didn't need to undo > dsi_mgr_bridge_power_on() because it wouldn't have undone it in the > modeset case. > Yes thats right. > While the current code isn't the best, it's not like the pre_enable() > call could have returned errors anyway. It probably wasn't truly the > end of the world to behave the way it did. > > With all that, I guess my plan would be to do as Dmitry says and just > always call dsi_mgr_bridge_power_on() from > dsi_mgr_bridge_pre_enable(). In the first patch I'll just do that and > remove the ps8640 workaround. Then I can add a 2nd patch that improves > the error handling by having dsi_mgr_bridge_power_on() return an error > code and then adding a matching dsi_mgr_bridge_power_off() that will > undo it and include the extra cleanup. > Sounds good to me. > -Doug
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 3a1417397283..5e6b8d423b96 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb; #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed) #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id) -#ifdef CONFIG_OF -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge) -{ - struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge); - - /* - * If the next bridge in the chain is the Parade ps8640 bridge chip - * then don't power on early since it seems to violate the expectations - * of the firmware that the bridge chip is running. - * - * NOTE: this is expected to be a temporary special case. It's expected - * that we'll eventually have a framework that allows the next level - * bridge to indicate whether it needs us to power on before it or - * after it. When that framework is in place then we'll use it and - * remove this special case. - */ - return !(next_bridge && next_bridge->of_node && - of_device_is_compatible(next_bridge->of_node, "parade,ps8640")); -} -#else -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge) -{ - return true; -} -#endif - static inline struct msm_dsi *dsi_mgr_get_dsi(int id) { return msm_dsim_glb.dsi[id]; @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) } } -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) { int id = dsi_mgr_bridge_get_id(bridge); struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); @@ -300,36 +274,10 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) if (is_bonded_dsi && msm_dsi1) msm_dsi_host_enable_irq(msm_dsi1->host); - return; - -host1_on_fail: - msm_dsi_host_power_off(host); -host_on_fail: - dsi_mgr_phy_disable(id); -phy_en_fail: - return; -} - -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) -{ - int id = dsi_mgr_bridge_get_id(bridge); - struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); - struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); - struct mipi_dsi_host *host = msm_dsi->host; - bool is_bonded_dsi = IS_BONDED_DSI(); - int ret; - - DBG("id=%d", id); - if (!msm_dsi_device_connected(msm_dsi)) - return; - /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) return; - if (!dsi_mgr_power_on_early(bridge)) - dsi_mgr_bridge_power_on(bridge); - ret = msm_dsi_host_enable(host); if (ret) { pr_err("%s: enable host %d failed, %d\n", __func__, id, ret); @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) host1_en_fail: msm_dsi_host_disable(host); host_en_fail: - + msm_dsi_host_disable_irq(host); + if (is_bonded_dsi && msm_dsi1) { + msm_dsi_host_disable_irq(msm_dsi1->host); + msm_dsi_host_power_off(msm_dsi1->host); + } +host1_on_fail: + msm_dsi_host_power_off(host); +host_on_fail: + dsi_mgr_phy_disable(id); +phy_en_fail: return; } @@ -438,9 +395,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, msm_dsi_host_set_display_mode(host, adjusted_mode); if (is_bonded_dsi && other_dsi) msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); - - if (dsi_mgr_power_on_early(bridge)) - dsi_mgr_bridge_power_on(bridge); } static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,