From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=91.218.175.103 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1792810720; b=j5A93T1zwYa/zKSAA6y8gHrIWedc0ZZx257N/gZlrqw2NDTs5cQ1h4kI+i3CsUgcNAfU 8zOEgX28QbpVo1v6qzL6BDzaCY2NspTrCGZeHhWNeq/O4YNa/r8rmTOiqR1XbGR5wzuU0 QVZbmpFp1VpXeHr6BkGxBZiJxlltC9gNXUEuNmOfEwmR34VjPAR+ujqRFwqz/I9hRsCw5 EoNKJqA0fzL28xq01nVLj78PfBloAXwqWox5jjeeeLNZqVjou2hkp7luSL+s0J8yzZkxK 5KvTqnfJo7EhxWNQPCNdd3InGsF0gKxJPZiD27A2XZq69Vq0LzB7bcLusYop7vzuMpw== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1792810720; h=DMARC-Filter:X-Envelope-To:DKIM-Signature:X-Envelope-To:Received: X-Mizu-Trace-ID:X-Migadu-Flow:Message-ID:Date:MIME-Version:User-Agent: Subject:To:Cc:References:Content-Language:From:In-Reply-To: Content-Type:Content-Transfer-Encoding; bh=DMCCmtvheCPZ6z25FzUW0Qd9uAafljfKefGPpydFs30=; b=RSZQ+rS5CvvBh7Z3lQhBD8/VLJrEmSXoUnrGJgcDzO+h1R4LcuAAA7JFWVGwhHfluFo7 2tgo8tzOYcPSABj3iqZeg4OYhq4VN6H3k5ePeptumTLvZ+/g1TZJPhrMCJgZzjMbBXNQ/ 6Lne9QNF0jQSItkbrBA8JiHidmqSkIU+OeJ8cisqEgfPfy9vBXHVNicByG3MnNWMrOzjl t8wlUEwtzzo1YMRvAlA+MH5F6DESETs4q8NqHie8/RVApqSfmPeLwqcjfqJVVLuVV0eEA dz1aRF9V3+7E5jy7jYsdHJPHcm0c0XRk2/5nJPXXHGUe3nFGUt5RqjW4CiIj1TQOs1Q== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=packett.cool; spf=pass smtp.mailfrom=packett.cool; dkim=pass header.d=packett.cool header.i=@packett.cool header.a=rsa-sha256 header.s=key1 header.b=PlWGrR3n; arc=none smtp.remote-ip=91.218.175.103 Received: from mta0.migadu.com (out-103.mta0.migadu.com [91.218.175.103]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 46F26606A31D for ; Sat, 24 Oct 2026 02:58:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org 46F26606A31D Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=packett.cool Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=packett.cool Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=packett.cool header.i=@packett.cool header.a=rsa-sha256 header.s=key1 header.b=PlWGrR3n X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=gMqBedMqV6/PbEo4P/C/wuOOsirk4D+GGPXOjLgDTws=; c=simple/simple; d=packett.cool; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1792810716; v=1; x=1793415516; b=PlWGrR3n5CnVaJDtS1BCZrLEZV3BF47ZC06Vma1LNJdgfCCgzrXTMjkFGKt+sa0M4rJgIwaK k9ZAf95fFi3BfwFIZ9Jy5AxUE8QAd35QRYOnQTb77MZggU2qQ2iAhPZ5E5j+/wKgAud/knZx/Lr v3F7bd7suXx+ICwQZp79cus9p8Ke6NTVM4dkrUUe8pRkQHoxmBE8UZmM5r8oaD5vUPSTsAqnva+ ck4gYX12rZE9HpeiCRmiL0cH1HyQM+UZ8vg48r/NeghbdUC80hpGVYnnu+0XyJxrFAt4ZHYCW0v ynhE94sD95pd3aJWyXGcJT9k0yYW8Wr11RuiOpVnC4oHQ== X-Envelope-To: linux-kernel@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id 16e8be6ff7b3bc0f; Sat, 24 Oct 2026 02:58:36 +0000 X-Mizu-Trace-ID: 16e8be6ff7b3bc0f X-Migadu-Flow: FLOW_OUT Message-ID: Date: Fri, 23 Oct 2026 23:58:25 -0300 X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [v2] drm/msm/dp: hold one runtime PM reference per plugged state To: Joonhoe Kim <26rote@gmail.com>, robin.clark@oss.qualcomm.com, Dmitry Baryshkov Cc: abhinav.kumar@linux.dev, jesszhan0024@gmail.com, sean@poorly.run, marijn.suijten@somainline.org, airlied@gmail.com, simona@ffwll.ch, yongxing.mou@oss.qualcomm.com, linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20261007155848.106-1-26rote@gmail.com> Content-Language: en-US From: Val Packett In-Reply-To: <20261007155848.106-1-26rote@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 10/7/26 12:58 PM, Joonhoe Kim wrote: > The plug handler takes a runtime PM reference every time it runs and > sets dp->plugged; the unplug handler drops a single reference when > dp->plugged is set. msm_dp_bridge_detect() also takes a reference and > marks the sink plugged, and keeps that reference as well -- on every > detect while the sink stays connected. On top of that, in the current > tree the plug handler runs more than once per plug: > drm_bridge_connector_detect() calls .detect() and then .hpd_notify(), > and pmic_glink altmode reports an IRQ_HPD as "connected" too. Every > extra reference is leaked on unplug. > > On a Lenovo TB323FU (SM8850, DP over the USB-C port via pmic_glink > altmode) the DP controller's usage count reached 28-34 after a few > plugs (rpm_usage tracepoint), so the controller never runtime-suspended > after the display was unplugged. It in turn kept the MDSS core GDSC on > while all displays were off. > > Make the plugged state own exactly one reference: the plug handler > only takes it when the sink was not plugged yet, and detect() returns > its own reference when the plugged state already holds one, or drops > both when it finds the sink gone. > > With this the count stays at 1-3 while a display is connected, the > controller suspends 3-4 s after unplug, MDSS suspends and its GDSC > powers off on DPMS off, and replugging (quickly or after 20 s) still > brings the picture back. Only tested on this device. > > Fixes: 3ea2d1c3d154 ("drm/msm/dp: turn link_ready into plugged") > Assisted-by: LLM > Signed-off-by: Joonhoe Kim <26rote@gmail.com> > --- > Changes in v2: > - Drop the plugged state's reference once, after the if/else, instead > of in both branches [Val] > - Drop the comments > - Link to v1: https://lore.kernel.org/all/20260929130627.959-1-26rote@gmail.com/ > > drivers/gpu/drm/msm/dp/dp_display.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 6abe30bdff12..4059cb9dba64 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -412,10 +412,12 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp) > > guard(mutex)(&dp->plugged_lock); > > - ret = pm_runtime_resume_and_get(&pdev->dev); > - if (ret) { > - DRM_ERROR("failed to pm_runtime_resume\n"); > - return ret; > + if (!dp->plugged) { > + ret = pm_runtime_resume_and_get(&pdev->dev); > + if (ret) { > + DRM_ERROR("failed to pm_runtime_resume\n"); > + return ret; > + } > } > > msm_dp_aux_enable_xfers(dp->aux, true); This part (msm_dp_hpd_plug_handle) is now also handled by the IRQ_HPD series: https://lore.kernel.org/all/20261024-hpd-irq-events-v8-6-6ee949e7a384@oss.qualcomm.com/ > @@ -897,6 +899,7 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge, > u8 dpcd[DP_RECEIVER_CAP_SIZE]; > struct drm_dp_desc desc; > bool phy_deinit; > + bool was_plugged; > int ret; > > dp = to_dp_bridge(bridge)->msm_dp_display; > @@ -910,6 +913,7 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge, > return status; > } > > + was_plugged = priv->plugged; > phy_deinit = msm_dp_display_host_phy_init(priv); > > msm_dp_aux_enable_xfers(priv->aux, true); > @@ -962,6 +966,9 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge, > pm_runtime_put_sync(&dp->pdev->dev); > } > > + if (was_plugged) > + pm_runtime_put_sync(&dp->pdev->dev); Thinking about this again, this flow of the reference(s) seems confusing. If I understand it correctly now, * there's a long-term reference associated with priv->plugged; * and this function needs the device to not be suspended because it's doing transfers to check if there's something plugged into the port. With the current flow (fixed by this v2 patch to not leak, or at least leak less): * we always take +1 reference; * if we "find a plug" (!was_plugged && priv->plugged): no drop, our reference "becomes" the long-term one; * if we "find an unplug" (was_plugged && !priv->plugged): we drop our own reference, as well as the long-term one; * if we "find no change while plugged" (was_plugged && priv->plugged): we only drop our own reference; * if we "find no change while unplugged" (!was_plugged && !priv->plugged): no drop, is this intended or a leak still? Not clear who needs the reference to persist in this case? **If** that last case is actually **not** intended to keep the reference, I think it would be clearer to, instead: * use a guard (probably PM_RUNTIME_ACQUIRE_IF_ENABLED to keep the error check) for this function's own reference; * and use a plain get an extra time when setting priv->plugged = true..? > + > return status; > } > Thanks, ~val