Message ID | 20231221135548.1.I10f326a9305d57ad32cee7f8d9c60518c8be20fb@changeid |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-9106-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2483:b0:fb:cd0c:d3e with SMTP id q3csp708611dyi; Thu, 21 Dec 2023 13:56:35 -0800 (PST) X-Google-Smtp-Source: AGHT+IEhf0Q5ftf2Wp30gWJz+a1koXGP7W1ty4aYQHBe8F1ywZnI4JeqraeU0vAhHUh3k7QIUfDJ X-Received: by 2002:a37:f510:0:b0:77f:3f7d:b4a0 with SMTP id l16-20020a37f510000000b0077f3f7db4a0mr457211qkk.62.1703195795310; Thu, 21 Dec 2023 13:56:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703195795; cv=none; d=google.com; s=arc-20160816; b=EuOfKKAXQOkdHy3Idmk/o1UOVwYRdaoqmGEZrrI+5UwQtiXReqYa72+rzDnQd/WGTc lLTXECgUmbciY9xpHz4Ac0UnhiJ2c9HwCk86r6MprhtnK7idJxpoijgIhlpLL0E31rbW 2vTIKGUHDOOz4566KPyH9V3UFU3fQHof/zz9o0j/CrSPJoM8iPbTEhK/SXHtZa9JrA+g n16GjltrzK7tHoy5UQnGKC99CiViGq5nUBjB/OH/y9Pw2VLnmf9z11O/z0sjFO1x7kA7 A8ipXJ2v+iDu42KZ5ARLoGDe2nP7WuQknrSzmCviCwqc4UUueBmqszywNdKIGVdLYAtQ rP+A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=d5sCfVn0Rl/7lsdspIykh4i2JHByLubzZbSyK4Lu5I0=; fh=ioT16pzLW0piUBBhxXvWxXS2Ou63WpLdKFBE673QY08=; b=VmN1gH0/Aeq6OzhPOJxfs+C18+77kjb8SHEBROQpRhdglLbUa6ppMnqp9GiIYqXmba L70dd2vcWlGUq/Cf+QcR/2fEbgW39rq2BqfnXFh81DaUlwqgeWWpF+/d64TRVxPzLsoP KMv4PqIESHvTKtggIq78lZ2Sy7nUaAlbQcTjR/96jvpWff+xFgVOD79HOpy+r+IKkI1u HBKJcRNINKOv3I+AtakyXjHc5MmE/ewTLIGbAf2WwlarGlHsCE8e3gL83rI56JLSmXc8 uxyOiZXSkYvn+0nktlcvet3VtedB3kVWZsbR0JSKVV+0Nfuu4JlxS9tiWa4ZVcBppyx7 iGhA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=AZqJqpSl; spf=pass (google.com: domain of linux-kernel+bounces-9106-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-9106-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [147.75.199.223]) by mx.google.com with ESMTPS id ay15-20020a05620a178f00b0077d8686cdaesi3239562qkb.307.2023.12.21.13.56.35 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Dec 2023 13:56:35 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-9106-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=AZqJqpSl; spf=pass (google.com: domain of linux-kernel+bounces-9106-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-9106-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 1F8B11C22FDB for <ouuuleilei@gmail.com>; Thu, 21 Dec 2023 21:56:35 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 83F9177F05; Thu, 21 Dec 2023 21:56:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="AZqJqpSl" X-Original-To: linux-kernel@vger.kernel.org Received: from mail-il1-f177.google.com (mail-il1-f177.google.com [209.85.166.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6669876DB6 for <linux-kernel@vger.kernel.org>; Thu, 21 Dec 2023 21:56:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=chromium.org Received: by mail-il1-f177.google.com with SMTP id e9e14a558f8ab-35d57e88c1dso6197735ab.2 for <linux-kernel@vger.kernel.org>; Thu, 21 Dec 2023 13:56:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1703195777; x=1703800577; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=d5sCfVn0Rl/7lsdspIykh4i2JHByLubzZbSyK4Lu5I0=; b=AZqJqpSlkenfYLXXnwXpoaXHnIjw5Z82YofpgZobaUo7/YJQPHX/eiS+Q+TOQIF9yY egaqfAx4BDc7M33MC1DYIh2t0Lk4lYOoDGZIcxNBpSbHRGt7j3n3xbPCWZ9MEYz4BozV EpFbaj8A9Clqr/0yb92lur0VqjbLuYKreibfg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703195777; x=1703800577; 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=d5sCfVn0Rl/7lsdspIykh4i2JHByLubzZbSyK4Lu5I0=; b=WBjVXQeQi6ikSlJAcasIQzeU0I+Zx4bW6SQrm0ETAdO/iLmdrX8VXUZ1LdCqhMe2uJ 1QdUnfPaA5LCc5wQoU6btfPi/Q145T1Pn97+o5qK2nKgM0lfuMr48vEoNeUyG8BwTgKK nDAN5KJnPGvY75ONDS/UNnkDlVy9qRjkUs3uBJiH7VZ+IcCFK0Xrnmyu3UZSuYtYvj/A kaivX9Rhiga/CN2uIYsGYxfhEx32LbriRqxwxdcS3MQeGqEKFzjuGFcCB0vM1QIniBNC QcgWnYaYYL4sygZV148pa0QxqEG//VInJ0DhmYs80zmwReIzhNl49ApcAMVSRzwFVbgX loEg== X-Gm-Message-State: AOJu0YzwGIdVwi3scPSLvSgDU3DhQyyTf2r/ruMAkbKnVE6s4IYAW5q4 Ie9yQJL24wMl2GShzlMi1DQyHArn8Lkj X-Received: by 2002:a05:6e02:2688:b0:35f:b958:29f3 with SMTP id bz8-20020a056e02268800b0035fb95829f3mr299519ilb.124.1703195777479; Thu, 21 Dec 2023 13:56:17 -0800 (PST) Received: from tictac2.mtv.corp.google.com ([2620:15c:9d:2:153:ce57:e73a:ec3a]) by smtp.gmail.com with ESMTPSA id l30-20020a63ba5e000000b005cda2559351sm1756817pgu.88.2023.12.21.13.56.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Dec 2023 13:56:16 -0800 (PST) From: Douglas Anderson <dianders@chromium.org> To: dri-devel@lists.freedesktop.org Cc: treapking@chromium.org, hsinyi@chromium.org, Douglas Anderson <dianders@chromium.org>, Andrzej Hajda <andrzej.hajda@intel.com>, Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, 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>, linux-kernel@vger.kernel.org Subject: [PATCH] drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer Date: Thu, 21 Dec 2023 13:55:48 -0800 Message-ID: <20231221135548.1.I10f326a9305d57ad32cee7f8d9c60518c8be20fb@changeid> X-Mailer: git-send-email 2.43.0.472.g3155946c3a-goog Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785930233964117527 X-GMAIL-MSGID: 1785930233964117527 |
Series |
drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer
|
|
Commit Message
Doug Anderson
Dec. 21, 2023, 9:55 p.m. UTC
Unlike what is claimed in commit f5aa7d46b0ee ("drm/bridge:
parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux"), if
someone manually tries to do an AUX transfer (like via `i2cdump ${bus}
0x50 i`) while the panel is off we don't just get a simple transfer
error. Instead, the whole ps8640 gets thrown for a loop and goes into
a bad state.
Let's put the function to wait for the HPD (and the magical 50 ms
after first reset) back in when we're doing an AUX transfer. This
shouldn't actually make things much slower (assuming the panel is on)
because we should immediately poll and see the HPD high. Mostly this
is just an extra i2c transfer to the bridge.
Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++
1 file changed, 5 insertions(+)
Comments
Hi Douglas, On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> wrote: > > Unlike what is claimed in commit f5aa7d46b0ee ("drm/bridge: > parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux"), if > someone manually tries to do an AUX transfer (like via `i2cdump ${bus} > 0x50 i`) while the panel is off we don't just get a simple transfer > error. Instead, the whole ps8640 gets thrown for a loop and goes into > a bad state. > > Let's put the function to wait for the HPD (and the magical 50 ms > after first reset) back in when we're doing an AUX transfer. This > shouldn't actually make things much slower (assuming the panel is on) > because we should immediately poll and see the HPD high. Mostly this > is just an extra i2c transfer to the bridge. > > Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 541e4f5afc4c..fb5e9ae9ad81 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -346,6 +346,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, > int ret; > > pm_runtime_get_sync(dev); > + ret = _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000); > + if (ret) { > + pm_runtime_put_sync_suspend(dev); > + return ret; > + } > ret = ps8640_aux_transfer_msg(aux, msg); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > -- > 2.43.0.472.g3155946c3a-goog > I think commit 9294914dd550 ("drm/bridge: parade-ps8640: Link device to ensure suspend/resume order") is trying to address the same problem, but we see this issue here because the device link is missing DL_FLAG_PM_RUNTIME. I prefer to add DL_FLAG_PM_RUNTIME here so we don't need to add a _ps8640_wait_hpd_asserted() after every pm_runtime_get_*() call. As a side note, I've verified both this patch and DL_FLAG_PM_RUNTIME in our downstream v5.15 kernel and panel-edp driver. Both of them successfully wait for HPD asserted when the timeout used to happen, but the panel is black in that situation. That being said, this patch still brings us to a better state. Originally, panel_edp_resume() would return an error when the timeout occurs, so the panel-edp driver is stuck at an unexpected state. With this patch or DL_FLAG_PM_RUNTIME, the runtime PM callbacks won't fail and a system suspend/resume brings the panel back. Regards, Pin-yen
Hi, On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@chromium.org> wrote: > > Hi Douglas, > > On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > Unlike what is claimed in commit f5aa7d46b0ee ("drm/bridge: > > parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux"), if > > someone manually tries to do an AUX transfer (like via `i2cdump ${bus} > > 0x50 i`) while the panel is off we don't just get a simple transfer > > error. Instead, the whole ps8640 gets thrown for a loop and goes into > > a bad state. > > > > Let's put the function to wait for the HPD (and the magical 50 ms > > after first reset) back in when we're doing an AUX transfer. This > > shouldn't actually make things much slower (assuming the panel is on) > > because we should immediately poll and see the HPD high. Mostly this > > is just an extra i2c transfer to the bridge. > > > > Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 541e4f5afc4c..fb5e9ae9ad81 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -346,6 +346,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, > > int ret; > > > > pm_runtime_get_sync(dev); > > + ret = _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000); > > + if (ret) { > > + pm_runtime_put_sync_suspend(dev); > > + return ret; > > + } > > ret = ps8640_aux_transfer_msg(aux, msg); > > pm_runtime_mark_last_busy(dev); > > pm_runtime_put_autosuspend(dev); > > -- > > 2.43.0.472.g3155946c3a-goog > > > > I think commit 9294914dd550 ("drm/bridge: parade-ps8640: Link device > to ensure suspend/resume order") is trying to address the same > problem, but we see this issue here because the device link is missing > DL_FLAG_PM_RUNTIME. I prefer to add DL_FLAG_PM_RUNTIME here so we > don't need to add a _ps8640_wait_hpd_asserted() after every > pm_runtime_get_*() call. I disagree. We've had several discussions on the lists about this topic before, though since I'm technically on vacation right now I'm not going to go look them up. In general "pm_runtime" is not sufficient for powering up DRM components. While DRM components can use pm_runtime themselves (as we are doing here), powering up another DRM component by grabbing a pm_runtime reference isn't right. There is a reason for the complexity of the DRM prepare/enable and all the current debates about the right order to call components in prepare() just demonstrates further that a simple pm_runtime reference isn't enough. It can be noted that, with ${SUBJECT} patch we _aren't_ powering up the panel. I actually tested two cases on sc7180-lazor. In one case I just closed the lid, which powered off the panel, but the touchscreen kept the panel power rail on. In this case with my patch I could still read the panel EDID. I then hacked the touchscreen off. Now when I closed the lid I'd get a timeout. This is different than if we tried to get a pm_runtime reference to the panel. > As a side note, I've verified both this patch and DL_FLAG_PM_RUNTIME > in our downstream v5.15 kernel and panel-edp driver. Both of them > successfully wait for HPD asserted when the timeout used to happen, > but the panel is black in that situation. That being said, this patch > still brings us to a better state. Originally, panel_edp_resume() > would return an error when the timeout occurs, so the panel-edp driver > is stuck at an unexpected state. With this patch or > DL_FLAG_PM_RUNTIME, the runtime PM callbacks won't fail and a system > suspend/resume brings the panel back. OK. I'm going to shut off email for real this time while I enjoy some time off. Hopefully the above convinces you. Otherwise I guess we can continue to debate in mid-January. -Doug
Hi, On Fri, Dec 22, 2023 at 11:34 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@chromium.org> wrote: > > > > Hi Douglas, > > > > On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > Unlike what is claimed in commit f5aa7d46b0ee ("drm/bridge: > > > parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux"), if > > > someone manually tries to do an AUX transfer (like via `i2cdump ${bus} > > > 0x50 i`) while the panel is off we don't just get a simple transfer > > > error. Instead, the whole ps8640 gets thrown for a loop and goes into > > > a bad state. > > > > > > Let's put the function to wait for the HPD (and the magical 50 ms > > > after first reset) back in when we're doing an AUX transfer. This > > > shouldn't actually make things much slower (assuming the panel is on) > > > because we should immediately poll and see the HPD high. Mostly this > > > is just an extra i2c transfer to the bridge. > > > > > > Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux") > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > index 541e4f5afc4c..fb5e9ae9ad81 100644 > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > @@ -346,6 +346,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, > > > int ret; > > > > > > pm_runtime_get_sync(dev); > > > + ret = _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000); > > > + if (ret) { > > > + pm_runtime_put_sync_suspend(dev); > > > + return ret; > > > + } > > > ret = ps8640_aux_transfer_msg(aux, msg); > > > pm_runtime_mark_last_busy(dev); > > > pm_runtime_put_autosuspend(dev); > > > -- > > > 2.43.0.472.g3155946c3a-goog > > > > > > > I think commit 9294914dd550 ("drm/bridge: parade-ps8640: Link device > > to ensure suspend/resume order") is trying to address the same > > problem, but we see this issue here because the device link is missing > > DL_FLAG_PM_RUNTIME. I prefer to add DL_FLAG_PM_RUNTIME here so we > > don't need to add a _ps8640_wait_hpd_asserted() after every > > pm_runtime_get_*() call. > > I disagree. We've had several discussions on the lists about this > topic before, though since I'm technically on vacation right now I'm > not going to go look them up. In general "pm_runtime" is not > sufficient for powering up DRM components. While DRM components can > use pm_runtime themselves (as we are doing here), powering up another > DRM component by grabbing a pm_runtime reference isn't right. There is > a reason for the complexity of the DRM prepare/enable and all the > current debates about the right order to call components in prepare() > just demonstrates further that a simple pm_runtime reference isn't > enough. > > It can be noted that, with ${SUBJECT} patch we _aren't_ powering up > the panel. I actually tested two cases on sc7180-lazor. In one case I > just closed the lid, which powered off the panel, but the touchscreen > kept the panel power rail on. In this case with my patch I could still > read the panel EDID. I then hacked the touchscreen off. Now when I > closed the lid I'd get a timeout. This is different than if we tried > to get a pm_runtime reference to the panel. > Okay, thanks for the detailed explanation. Then, let's go with the approach in this patch. So, Tested-by: Pin-yen Lin <treapking@chromium.org> Reviewed-by: Pin-yen Lin <treapking@chromium.org> > > > As a side note, I've verified both this patch and DL_FLAG_PM_RUNTIME > > in our downstream v5.15 kernel and panel-edp driver. Both of them > > successfully wait for HPD asserted when the timeout used to happen, > > but the panel is black in that situation. That being said, this patch > > still brings us to a better state. Originally, panel_edp_resume() > > would return an error when the timeout occurs, so the panel-edp driver > > is stuck at an unexpected state. With this patch or > > DL_FLAG_PM_RUNTIME, the runtime PM callbacks won't fail and a system > > suspend/resume brings the panel back. > > OK. I'm going to shut off email for real this time while I enjoy some > time off. Hopefully the above convinces you. Otherwise I guess we can > continue to debate in mid-January. > > -Doug Happy holiday! Pin-yen
Hi, On Mon, Dec 25, 2023 at 1:08 AM Pin-yen Lin <treapking@chromium.org> wrote: > > Hi, > > On Fri, Dec 22, 2023 at 11:34 PM Doug Anderson <dianders@chromiumorg> wrote: > > > > Hi, > > > > On Fri, Dec 22, 2023 at 2:29 AM Pin-yen Lin <treapking@chromiumorg> wrote: > > > > > > Hi Douglas, > > > > > > On Fri, Dec 22, 2023 at 5:56 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > > > Unlike what is claimed in commit f5aa7d46b0ee ("drm/bridge: > > > > parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux"), if > > > > someone manually tries to do an AUX transfer (like via `i2cdump ${bus} > > > > 0x50 i`) while the panel is off we don't just get a simple transfer > > > > error. Instead, the whole ps8640 gets thrown for a loop and goes into > > > > a bad state. > > > > > > > > Let's put the function to wait for the HPD (and the magical 50 ms > > > > after first reset) back in when we're doing an AUX transfer. This > > > > shouldn't actually make things much slower (assuming the panel is on) > > > > because we should immediately poll and see the HPD high. Mostly this > > > > is just an extra i2c transfer to the bridge. > > > > > > > > Fixes: f5aa7d46b0ee ("drm/bridge: parade-ps8640: Provide wait_hpd_asserted() in struct drm_dp_aux") > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > --- > > > > > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > index 541e4f5afc4c..fb5e9ae9ad81 100644 > > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > > @@ -346,6 +346,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, > > > > int ret; > > > > > > > > pm_runtime_get_sync(dev); > > > > + ret = _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000); > > > > + if (ret) { > > > > + pm_runtime_put_sync_suspend(dev); > > > > + return ret; > > > > + } > > > > ret = ps8640_aux_transfer_msg(aux, msg); > > > > pm_runtime_mark_last_busy(dev); > > > > pm_runtime_put_autosuspend(dev); > > > > -- > > > > 2.43.0.472.g3155946c3a-goog > > > > > > > > > > I think commit 9294914dd550 ("drm/bridge: parade-ps8640: Link device > > > to ensure suspend/resume order") is trying to address the same > > > problem, but we see this issue here because the device link is missing > > > DL_FLAG_PM_RUNTIME. I prefer to add DL_FLAG_PM_RUNTIME here so we > > > don't need to add a _ps8640_wait_hpd_asserted() after every > > > pm_runtime_get_*() call. > > > > I disagree. We've had several discussions on the lists about this > > topic before, though since I'm technically on vacation right now I'm > > not going to go look them up. In general "pm_runtime" is not > > sufficient for powering up DRM components. While DRM components can > > use pm_runtime themselves (as we are doing here), powering up another > > DRM component by grabbing a pm_runtime reference isn't right. There is > > a reason for the complexity of the DRM prepare/enable and all the > > current debates about the right order to call components in prepare() > > just demonstrates further that a simple pm_runtime reference isn't > > enough. > > > > It can be noted that, with ${SUBJECT} patch we _aren't_ powering up > > the panel. I actually tested two cases on sc7180-lazor. In one case I > > just closed the lid, which powered off the panel, but the touchscreen > > kept the panel power rail on. In this case with my patch I could still > > read the panel EDID. I then hacked the touchscreen off. Now when I > > closed the lid I'd get a timeout. This is different than if we tried > > to get a pm_runtime reference to the panel. > > > Okay, thanks for the detailed explanation. Then, let's go with the > approach in this patch. So, > > Tested-by: Pin-yen Lin <treapking@chromium.org> > Reviewed-by: Pin-yen Lin <treapking@chromium.org> Thanks for the tags. I've pushed this to drm-misc-fixes: 024b32db43a3 drm/bridge: parade-ps8640: Wait for HPD when doing an AUX transfer
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 541e4f5afc4c..fb5e9ae9ad81 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -346,6 +346,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, int ret; pm_runtime_get_sync(dev); + ret = _ps8640_wait_hpd_asserted(ps_bridge, 200 * 1000); + if (ret) { + pm_runtime_put_sync_suspend(dev); + return ret; + } ret = ps8640_aux_transfer_msg(aux, msg); pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev);