From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3A48DC433FE for ; Tue, 15 Feb 2022 22:52:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244665AbiBOWwO (ORCPT ); Tue, 15 Feb 2022 17:52:14 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:59644 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244260AbiBOWwM (ORCPT ); Tue, 15 Feb 2022 17:52:12 -0500 Received: from mail-oo1-xc30.google.com (mail-oo1-xc30.google.com [IPv6:2607:f8b0:4864:20::c30]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CA0F78BF3D for ; Tue, 15 Feb 2022 14:52:01 -0800 (PST) Received: by mail-oo1-xc30.google.com with SMTP id c7-20020a4ad207000000b002e7ab4185d2so429361oos.6 for ; Tue, 15 Feb 2022 14:52:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=1FB9vEgE7XNXDYnECZNocmKPYP8d5C7TWz0lgDEn7xk=; b=fjG712BlrOTYrfwnSRNCBRY8yRQW9o6qR1hiZlZWQq91Sm5fVmIhzlRa+s7C/PBz2T eJ4cgsB9JemF8urXL4CrzCBOFzhtwz5N7W5Kotgar0OKQBgyUhdOiRujxY9FKvRk0roG NZoBf1wVhyOnoD6NCrsjPrRQ5hEQ7qliayxC0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1FB9vEgE7XNXDYnECZNocmKPYP8d5C7TWz0lgDEn7xk=; b=ONlcvcM9aPzMATiNM1DJ+qTjzCRy+d26KLBK3rmNYTMTtRcKHbVWLJ+H4KFZCHNvNR 4YioEaZSyZMFwJBn5d/RuKKhEBOw+V2sZqS+zG2UC8D4oQGMwIDZANY0YBFpdCUcoKoI /rGEI0lQ6o7X9kzuWX7NDTasbeuqvYvS61kkn+cDoPX3Pu5xvhJa3GoIYXngqnDQoQ/A l1WqEtEwkbU2UYjaPVk/vOajh/6GhEz6vX9uIoQ7dnyzLcBU5m18sirPi9H4+N0qjk2o 9qaVLXQruCwQl8NRa3zUPMEziYhQLpbJc93X7/zuabG1vijSrK8GG0Ql/eg1jjdx2p/w Yesw== X-Gm-Message-State: AOAM531QMmkNw6xohSwzQHxKDA7Ekndp380eJKowAa1cTxSYaYIarcmF zBExCOd1nP3vGizE9lkjChZk3qaJt1iwjA== X-Google-Smtp-Source: ABdhPJy1i9d7rM8n6qaOjVGE6QYXMaIUdRc0QAoi1xtpZBKyGm7ZCKnrt+P1mVMCV+rYNrnvpVNBXA== X-Received: by 2002:a05:6870:e982:b0:d2:a01f:562d with SMTP id r2-20020a056870e98200b000d2a01f562dmr57162oao.242.1644965520031; Tue, 15 Feb 2022 14:52:00 -0800 (PST) Received: from mail-ot1-f48.google.com (mail-ot1-f48.google.com. [209.85.210.48]) by smtp.gmail.com with ESMTPSA id y1sm17117368oad.38.2022.02.15.14.51.58 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 15 Feb 2022 14:51:58 -0800 (PST) Received: by mail-ot1-f48.google.com with SMTP id s6-20020a0568301e0600b0059ea5472c98so293981otr.11 for ; Tue, 15 Feb 2022 14:51:58 -0800 (PST) X-Received: by 2002:a9d:755a:: with SMTP id b26mr513845otl.230.1644965517513; Tue, 15 Feb 2022 14:51:57 -0800 (PST) MIME-Version: 1.0 References: <20211001144212.v2.1.I773a08785666ebb236917b0c8e6c05e3de471e75@changeid> In-Reply-To: From: Brian Norris Date: Tue, 15 Feb 2022 14:51:46 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX To: Doug Anderson Cc: Andrzej Hajda , Neil Armstrong , Sean Paul , Jonas Karlman , LKML , Jernej Skrabec , Laurent Pinchart , dri-devel , "open list:ARM/Rockchip SoC..." , "# 4.0+" , Tomeu Vizoso Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 15, 2022 at 1:31 PM Doug Anderson wrote: > > Hi, Hi! > On Fri, Oct 1, 2021 at 2:50 PM Brian Norris wrote: > > > > If the display is not enable()d, then we aren't holding a runtime PM > > reference here. Thus, it's easy to accidentally cause a hang, if user > > space is poking around at /dev/drm_dp_aux0 at the "wrong" time. > > > > Let's get the panel and PM state right before trying to talk AUX. > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > index b7d2e4449cfa..6fc46ac93ef8 100644 > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > @@ -1632,8 +1632,27 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux, ... > > + pm_runtime_get_sync(dp->dev); > > + ret = analogix_dp_transfer(dp, msg); > > + pm_runtime_put(dp->dev); > > I've spent an unfortunate amount of time digging around the DP AUX bus > recently, so I can at least say that I have some experience and some > opinions here. Thanks! Experience is welcome, and opinions too sometimes ;) > IMO: > > 1. Don't power the panel on. If the panel isn't powered on then the DP > AUX transfer will timeout. Tough nuggies. Think of yourself more like > an i2c controller and of this as an i2c transfer implementation. The > i2c controller isn't in charge of powering up the i2c devices on the > bus. If userspace does an "i2c detect" on an i2c bus and some of the > devices aren't powered then they won't be found. If you try to > read/write from a powered off device that won't work either. I guess this, paired with the driver examples below (ti-sn65dsi86.c, especially, which specifically throws errors if the panel isn't on), makes some sense. It's approximately (but more verbosely) what Andrzej was recommending too, I guess. It still makes me wonder what the point of the /dev/drm_dp_aux interface is though, because it seems like you're pretty much destined to not have reliable operation through that means. Also note: I found that the AUX bus is really not working properly at all (even with this patch) in some cases due to self-refresh. Not only do we need the panel enabled, but we need to not be in self-refresh mode. Self-refresh is not currently exposed to user space, so user space has no way of knowing the panel is currently active, aside from racily inducing artificial display activity. But if we're OK with "just throw errors" or "just let stuff time out", then I guess that's not a big deal. My purpose is to avoid hanging the system, not to make /dev/drm_dp_aux useful. > 2. In theory if the DP driver can read HPD (I haven't looked through > the analogix code to see how it handles it) then you can fail an AUX > transfer right away if HPD isn't asserted instead of timing out. If > this is hard, it's probably fine to just time out though. This driver does handle HPD, but it also has overrides because apparently it doesn't work on some systems. I might see if we can leverage it, or I might just follow the bridge-enabled state (similar to ti-sn65dsi86.c's 'comms_enabled'). > 3. Do the "pm_runtime" calls, but enable "autosuspend" with something > ~1 second autosuspend delay. When using the AUX bus to read an EDID > the underlying code will call your function 16 times in quick > succession. If you're powering up and down constantly that'll be a bit > of a waste. Does this part really matter? For properly active cases, the bridge remains enabled, and it holds a runtime PM reference. For "maybe active" (your "tough nuggies" situation above), you're probably right that it's inefficient, but does it matter, when it's going to be a slow timed-out operation anyway? The AUX failure will be much slower than the PM transition. I guess I can do this anyway, but frankly, I'll just be copy/pasting stuff from other drivers, because the runtime PM documentation still confuses me, and moreso once you involve autosuspend. > For a reference, you could look at > `drivers/gpu/drm/bridge/ti-sn65dsi86.c`. Also > `drivers/gpu/drm/bridge/parade-ps8640.c` Thanks for these. They look like reasonable patterns to follow. Brian From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D6687C433F5 for ; Tue, 15 Feb 2022 22:52:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=AILAOaSFTEKdg6mTkA7KT94LMIjvcHvM99qCL+rV7j4=; b=YM+U7GLOoHTBzb nmbIuCzN0XIHvDwOkpfp8D6v9BQB++lO0DnV7fKgdEoXcqCwHO8k5Vpd9n+MqbitNXbQEV3jQvTpx 3mI+FJvQZd5eH2gdvCFoZ1W3q1ysf8y3STkXTGmRZ/xmQYHaKDsc3uUkQvg1sVAt1LZ4qxwGlIiwh 5jYOu+2SsE6Pz+KlxJ+z8egBFPEp3R303yigTkonQbOBLBcIgQZ6cdN0reto96rw2es7imy/G1UQz fqqg8N656EsaSbTxMjbNf1lo/FVzwkcPLEv11tMT6/UErLJmWwuONM5fd6PG9FXX+jwdXSDYAh60B r/7QtrH4Wnl3QmhBxuIA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nK6g9-004oi2-MF; Tue, 15 Feb 2022 22:52:13 +0000 Received: from mail-oo1-xc34.google.com ([2607:f8b0:4864:20::c34]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nK6g6-004ofz-Ad for linux-rockchip@lists.infradead.org; Tue, 15 Feb 2022 22:52:12 +0000 Received: by mail-oo1-xc34.google.com with SMTP id c7-20020a4ad207000000b002e7ab4185d2so429424oos.6 for ; Tue, 15 Feb 2022 14:52:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=1FB9vEgE7XNXDYnECZNocmKPYP8d5C7TWz0lgDEn7xk=; b=fjG712BlrOTYrfwnSRNCBRY8yRQW9o6qR1hiZlZWQq91Sm5fVmIhzlRa+s7C/PBz2T eJ4cgsB9JemF8urXL4CrzCBOFzhtwz5N7W5Kotgar0OKQBgyUhdOiRujxY9FKvRk0roG NZoBf1wVhyOnoD6NCrsjPrRQ5hEQ7qliayxC0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1FB9vEgE7XNXDYnECZNocmKPYP8d5C7TWz0lgDEn7xk=; b=K65/utCPMc9GBCPkPUZdTn8ifLzm6UrBcVOW660z4JHcus0IShgfbTxq7Vl49JVPoc dXzSMifjct1lVEN2DiP19jh6fR+GHV8cpvB6EpKfZeXKat52oNpbid5yMk+CywOfUSCS adF0Y1NqmCByByOg7kq9bt3jLxTEWMj62gwj+mD63R11OsIpLnjtYHW67vDjJFc6OCwy uxI6Tgh5LjGSGIx0qzD920MLdW8HFbIoAvv7GD6lIjjcehe6ymDQ6yjO7ooF0vUqhi9w uRqV4vL3JOqFi/vkgfWFbxYDIgIPelK1MjdY+HLzsyj0gkcMg0uU2uTrKp3Gcdp2wZv3 o9Ow== X-Gm-Message-State: AOAM530tdkB+5t4hFaIOuG0dBS/3d9u2aIrYVMGv6IGrsKdng7ihyH1a 5YbAnpuJ8IipWiWsQce/ngl2JvFZNzYVxA== X-Google-Smtp-Source: ABdhPJx1UwS8ZZoViE2wLokEBX1LqepfymZkMPUB0OHualyIJ8O6c2Amd1XCX2MzTYq0Cw2CFOhy9Q== X-Received: by 2002:a05:6870:ed92:b0:d2:9e7c:903d with SMTP id fz18-20020a056870ed9200b000d29e7c903dmr52817oab.350.1644965521005; Tue, 15 Feb 2022 14:52:01 -0800 (PST) Received: from mail-ot1-f41.google.com (mail-ot1-f41.google.com. [209.85.210.41]) by smtp.gmail.com with ESMTPSA id s64sm14339316oos.0.2022.02.15.14.51.58 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 15 Feb 2022 14:51:58 -0800 (PST) Received: by mail-ot1-f41.google.com with SMTP id a12-20020a05683012cc00b005acf7c91097so296801otq.10 for ; Tue, 15 Feb 2022 14:51:58 -0800 (PST) X-Received: by 2002:a9d:755a:: with SMTP id b26mr513845otl.230.1644965517513; Tue, 15 Feb 2022 14:51:57 -0800 (PST) MIME-Version: 1.0 References: <20211001144212.v2.1.I773a08785666ebb236917b0c8e6c05e3de471e75@changeid> In-Reply-To: From: Brian Norris Date: Tue, 15 Feb 2022 14:51:46 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX To: Doug Anderson Cc: Andrzej Hajda , Neil Armstrong , Sean Paul , Jonas Karlman , LKML , Jernej Skrabec , Laurent Pinchart , dri-devel , "open list:ARM/Rockchip SoC..." , "# 4.0+" , Tomeu Vizoso X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220215_145210_391493_69A5731C X-CRM114-Status: GOOD ( 34.38 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org On Tue, Feb 15, 2022 at 1:31 PM Doug Anderson wrote: > > Hi, Hi! > On Fri, Oct 1, 2021 at 2:50 PM Brian Norris wrote: > > > > If the display is not enable()d, then we aren't holding a runtime PM > > reference here. Thus, it's easy to accidentally cause a hang, if user > > space is poking around at /dev/drm_dp_aux0 at the "wrong" time. > > > > Let's get the panel and PM state right before trying to talk AUX. > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > index b7d2e4449cfa..6fc46ac93ef8 100644 > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > @@ -1632,8 +1632,27 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux, ... > > + pm_runtime_get_sync(dp->dev); > > + ret = analogix_dp_transfer(dp, msg); > > + pm_runtime_put(dp->dev); > > I've spent an unfortunate amount of time digging around the DP AUX bus > recently, so I can at least say that I have some experience and some > opinions here. Thanks! Experience is welcome, and opinions too sometimes ;) > IMO: > > 1. Don't power the panel on. If the panel isn't powered on then the DP > AUX transfer will timeout. Tough nuggies. Think of yourself more like > an i2c controller and of this as an i2c transfer implementation. The > i2c controller isn't in charge of powering up the i2c devices on the > bus. If userspace does an "i2c detect" on an i2c bus and some of the > devices aren't powered then they won't be found. If you try to > read/write from a powered off device that won't work either. I guess this, paired with the driver examples below (ti-sn65dsi86.c, especially, which specifically throws errors if the panel isn't on), makes some sense. It's approximately (but more verbosely) what Andrzej was recommending too, I guess. It still makes me wonder what the point of the /dev/drm_dp_aux interface is though, because it seems like you're pretty much destined to not have reliable operation through that means. Also note: I found that the AUX bus is really not working properly at all (even with this patch) in some cases due to self-refresh. Not only do we need the panel enabled, but we need to not be in self-refresh mode. Self-refresh is not currently exposed to user space, so user space has no way of knowing the panel is currently active, aside from racily inducing artificial display activity. But if we're OK with "just throw errors" or "just let stuff time out", then I guess that's not a big deal. My purpose is to avoid hanging the system, not to make /dev/drm_dp_aux useful. > 2. In theory if the DP driver can read HPD (I haven't looked through > the analogix code to see how it handles it) then you can fail an AUX > transfer right away if HPD isn't asserted instead of timing out. If > this is hard, it's probably fine to just time out though. This driver does handle HPD, but it also has overrides because apparently it doesn't work on some systems. I might see if we can leverage it, or I might just follow the bridge-enabled state (similar to ti-sn65dsi86.c's 'comms_enabled'). > 3. Do the "pm_runtime" calls, but enable "autosuspend" with something > ~1 second autosuspend delay. When using the AUX bus to read an EDID > the underlying code will call your function 16 times in quick > succession. If you're powering up and down constantly that'll be a bit > of a waste. Does this part really matter? For properly active cases, the bridge remains enabled, and it holds a runtime PM reference. For "maybe active" (your "tough nuggies" situation above), you're probably right that it's inefficient, but does it matter, when it's going to be a slow timed-out operation anyway? The AUX failure will be much slower than the PM transition. I guess I can do this anyway, but frankly, I'll just be copy/pasting stuff from other drivers, because the runtime PM documentation still confuses me, and moreso once you involve autosuspend. > For a reference, you could look at > `drivers/gpu/drm/bridge/ti-sn65dsi86.c`. Also > `drivers/gpu/drm/bridge/parade-ps8640.c` Thanks for these. They look like reasonable patterns to follow. Brian _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 36BFCC433F5 for ; Tue, 15 Feb 2022 22:52:04 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3833810E5CD; Tue, 15 Feb 2022 22:52:03 +0000 (UTC) Received: from mail-oo1-xc32.google.com (mail-oo1-xc32.google.com [IPv6:2607:f8b0:4864:20::c32]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7EED210E5CD for ; Tue, 15 Feb 2022 22:52:02 +0000 (UTC) Received: by mail-oo1-xc32.google.com with SMTP id u47-20020a4a9732000000b00316d0257de0so425067ooi.7 for ; Tue, 15 Feb 2022 14:52:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=1FB9vEgE7XNXDYnECZNocmKPYP8d5C7TWz0lgDEn7xk=; b=fjG712BlrOTYrfwnSRNCBRY8yRQW9o6qR1hiZlZWQq91Sm5fVmIhzlRa+s7C/PBz2T eJ4cgsB9JemF8urXL4CrzCBOFzhtwz5N7W5Kotgar0OKQBgyUhdOiRujxY9FKvRk0roG NZoBf1wVhyOnoD6NCrsjPrRQ5hEQ7qliayxC0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1FB9vEgE7XNXDYnECZNocmKPYP8d5C7TWz0lgDEn7xk=; b=AFU+qyHjxCjRzjaXAkgyWGC6WijEu0PRqJwphGJPvcGr6EfnGkkP8D99GGJltJpv7Y 0GKMIITSj7Iz5G6H5lBpBiCyvZpUTe1lWEt/HI5XE/tm2bgLqM9Z3PFKF7z4n/ty9ClZ iVtNPuVEIA+Txg7KDbJmG8HJBs02hh3x031ooEYt/+XMT2XY0PiRylUvYOaXzR67YxaH sp1uaa3DdSCkWHJQK2scgP8UG6lH/tYV8Kts4wubK7qB3NpKwapBDpKH1UH5wgv4zShf XHW8XzjAZveEgwU4GLvXEECaY65NlIWRnbYMZ1x3nOJSNfuZqSp811t9o/o9GZ9Vaox5 bW4Q== X-Gm-Message-State: AOAM532AMpgjFXI49ObgNAXpzBnOk4xaPxWboFPu8XoxmNql60qTO0mM QEv9F3hnOMfXznb/PzEkPyLyy6AgiOiGqg== X-Google-Smtp-Source: ABdhPJyxA41fUh2+MrbDFR+m6TY9J3aL+g7t9RK83Uy/MlGIF+MfRz34UVkO2K8R60SqQfITeUwlJg== X-Received: by 2002:a05:6870:3a35:b0:d2:c787:ceea with SMTP id du53-20020a0568703a3500b000d2c787ceeamr2278422oab.96.1644965521388; Tue, 15 Feb 2022 14:52:01 -0800 (PST) Received: from mail-ot1-f42.google.com (mail-ot1-f42.google.com. [209.85.210.42]) by smtp.gmail.com with ESMTPSA id t192sm893840oie.14.2022.02.15.14.51.58 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 15 Feb 2022 14:51:58 -0800 (PST) Received: by mail-ot1-f42.google.com with SMTP id l12-20020a0568302b0c00b005a4856ff4ceso286404otv.13 for ; Tue, 15 Feb 2022 14:51:58 -0800 (PST) X-Received: by 2002:a9d:755a:: with SMTP id b26mr513845otl.230.1644965517513; Tue, 15 Feb 2022 14:51:57 -0800 (PST) MIME-Version: 1.0 References: <20211001144212.v2.1.I773a08785666ebb236917b0c8e6c05e3de471e75@changeid> In-Reply-To: From: Brian Norris Date: Tue, 15 Feb 2022 14:51:46 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX To: Doug Anderson Content-Type: text/plain; charset="UTF-8" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Tomeu Vizoso , Jonas Karlman , dri-devel , Neil Armstrong , LKML , Jernej Skrabec , Andrzej Hajda , Laurent Pinchart , "open list:ARM/Rockchip SoC..." , "# 4.0+" , Sean Paul Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, Feb 15, 2022 at 1:31 PM Doug Anderson wrote: > > Hi, Hi! > On Fri, Oct 1, 2021 at 2:50 PM Brian Norris wrote: > > > > If the display is not enable()d, then we aren't holding a runtime PM > > reference here. Thus, it's easy to accidentally cause a hang, if user > > space is poking around at /dev/drm_dp_aux0 at the "wrong" time. > > > > Let's get the panel and PM state right before trying to talk AUX. > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > index b7d2e4449cfa..6fc46ac93ef8 100644 > > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > > @@ -1632,8 +1632,27 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux, ... > > + pm_runtime_get_sync(dp->dev); > > + ret = analogix_dp_transfer(dp, msg); > > + pm_runtime_put(dp->dev); > > I've spent an unfortunate amount of time digging around the DP AUX bus > recently, so I can at least say that I have some experience and some > opinions here. Thanks! Experience is welcome, and opinions too sometimes ;) > IMO: > > 1. Don't power the panel on. If the panel isn't powered on then the DP > AUX transfer will timeout. Tough nuggies. Think of yourself more like > an i2c controller and of this as an i2c transfer implementation. The > i2c controller isn't in charge of powering up the i2c devices on the > bus. If userspace does an "i2c detect" on an i2c bus and some of the > devices aren't powered then they won't be found. If you try to > read/write from a powered off device that won't work either. I guess this, paired with the driver examples below (ti-sn65dsi86.c, especially, which specifically throws errors if the panel isn't on), makes some sense. It's approximately (but more verbosely) what Andrzej was recommending too, I guess. It still makes me wonder what the point of the /dev/drm_dp_aux interface is though, because it seems like you're pretty much destined to not have reliable operation through that means. Also note: I found that the AUX bus is really not working properly at all (even with this patch) in some cases due to self-refresh. Not only do we need the panel enabled, but we need to not be in self-refresh mode. Self-refresh is not currently exposed to user space, so user space has no way of knowing the panel is currently active, aside from racily inducing artificial display activity. But if we're OK with "just throw errors" or "just let stuff time out", then I guess that's not a big deal. My purpose is to avoid hanging the system, not to make /dev/drm_dp_aux useful. > 2. In theory if the DP driver can read HPD (I haven't looked through > the analogix code to see how it handles it) then you can fail an AUX > transfer right away if HPD isn't asserted instead of timing out. If > this is hard, it's probably fine to just time out though. This driver does handle HPD, but it also has overrides because apparently it doesn't work on some systems. I might see if we can leverage it, or I might just follow the bridge-enabled state (similar to ti-sn65dsi86.c's 'comms_enabled'). > 3. Do the "pm_runtime" calls, but enable "autosuspend" with something > ~1 second autosuspend delay. When using the AUX bus to read an EDID > the underlying code will call your function 16 times in quick > succession. If you're powering up and down constantly that'll be a bit > of a waste. Does this part really matter? For properly active cases, the bridge remains enabled, and it holds a runtime PM reference. For "maybe active" (your "tough nuggies" situation above), you're probably right that it's inefficient, but does it matter, when it's going to be a slow timed-out operation anyway? The AUX failure will be much slower than the PM transition. I guess I can do this anyway, but frankly, I'll just be copy/pasting stuff from other drivers, because the runtime PM documentation still confuses me, and moreso once you involve autosuspend. > For a reference, you could look at > `drivers/gpu/drm/bridge/ti-sn65dsi86.c`. Also > `drivers/gpu/drm/bridge/parade-ps8640.c` Thanks for these. They look like reasonable patterns to follow. Brian