[PATCH] drm/msm/dp: hold one runtime PM reference per plugged state

[PATCH] drm/msm/dp: hold one runtime PM reference per plugged state

[Joonhoe Kim]

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>
---
Related pending work: "drm: handle IRQ_HPD events correctly" v5
(https://lore.kernel.org/all/20260920-hpd-irq-events-v5-0-ff1de0090c72@oss.qualcomm.com/)
removes the IRQ_HPD-as-plug path, and MST v7 26/35
(https://lore.kernel.org/all/20260929-msm-dp-mst-v7-26-4b81473185e7@oss.qualcomm.com/)
the detect-based hpd_notify for DP; the reference that
msm_dp_bridge_detect() keeps is leaked either way. MST v7 27/35 touches
the same lines as the first hunk; happy to rebase on top of it.

 drivers/gpu/drm/msm/dp/dp_display.c | 23 +++++++++++++++++++----
 1 file changed, 19 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..4e47b659c71d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -412,10 +412,17 @@ 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;
+	/*
+	 * The plugged state holds one runtime PM reference, dropped by the
+	 * unplug handler. The Type-C side repeats "connected" notifications,
+	 * and detect() may have seen the sink first: take it only once.
+	 */
+	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);
@@ -897,6 +904,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 +918,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);
@@ -960,6 +969,12 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
 		}
 
 		pm_runtime_put_sync(&dp->pdev->dev);
+		/* the sink is gone: drop the plugged state's reference too */
+		if (was_plugged)
+			pm_runtime_put_sync(&dp->pdev->dev);
+	} else if (was_plugged) {
+		/* the plugged state already holds its reference */
+		pm_runtime_put_sync(&dp->pdev->dev);
 	}
 
 	return status;

base-commit: 6375e61c01e93e35ee7acd336a689ac1fae4b509
-- 
2.43.0

Re: [PATCH] drm/msm/dp: hold one runtime PM reference per plugged state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dp: dropping plugged PM ref in detect() breaks PHY init on replug
--

commit c051f74425731ad4ec4c7067fd0c9ebc38bc16ab
Author: Joonhoe Kim <26rote@gmail.com>

drm/msm/dp: hold one runtime PM reference per plugged state

This commit modifies the DP runtime PM handling to take exactly one reference
for the plugged state, avoiding leaks where multiple HPD events or detect()
calls accumulate references and prevent the controller from suspending.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 6abe30bdff120..4e47b659c71df 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> @@ -960,6 +969,12 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
>  		}
>  
>  		pm_runtime_put_sync(&dp->pdev->dev);
> +		/* the sink is gone: drop the plugged state's reference too */
> +		if (was_plugged)
> +			pm_runtime_put_sync(&dp->pdev->dev);

[Severity: High]
Does dropping the plugged state PM reference here allow the controller to
suspend while the PHY software state still thinks it is initialized?

If msm_dp_bridge_detect() fails during a physical unplug (for example,
failing to read DPCD caps), it sets priv->plugged to false and jumps here to
drop both its own PM reference and the plugged state's PM reference.

The hardware unplug interrupt would then trigger msm_dp_hpd_unplug_handle(),
which has an early return:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_hpd_unplug_handle() {
    ...
    guard(mutex)(&dp->plugged_lock);
    if (!dp->plugged)
        return 0;
}

Because detect() already cleared dp->plugged, this early return skips tearing
down the PHY state with msm_dp_display_host_phy_exit().

Then, on the next plug, msm_dp_display_host_phy_init() will silently skip
PHY initialization because dp->phy_initialized was never reset to false:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_host_phy_init() {
    ...
    if (!dp->phy_initialized) {
        msm_dp_ctrl_phy_init(dp->ctrl);
        dp->phy_initialized = true;
        return true;
    }
    return false;
}

Can this sequence leave the hardware PHY uninitialized on replug, causing
link training to fail and the display to remain broken until reboot?

> +	} else if (was_plugged) {
> +		/* the plugged state already holds its reference */
> +		pm_runtime_put_sync(&dp->pdev->dev);
>  	}
>  
>  	return status;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929130627.959-1-26rote@gmail.com?part=1

Re: [PATCH] drm/msm/dp: hold one runtime PM reference per plugged state

[Joonhoe Kim]

On Tue, 29 Sep 2026 13:20:29 +0000, sashiko-bot@kernel.org wrote:
> Then, on the next plug, msm_dp_display_host_phy_init() will silently skip
> PHY initialization because dp->phy_initialized was never reset to false:
[...]
> Can this sequence leave the hardware PHY uninitialized on replug, causing
> link training to fail and the display to remain broken until reboot?

I tried it on the device (Lenovo TB323FU, a DP monitor over USB-C) by
delaying the unplug handler by 5 s and forcing a detect in that window.

- A normal unplug already leaves the PHY initialized across the
  controller's runtime suspend: msm_dp_hpd_unplug_handle() only calls
  msm_dp_display_host_phy_exit() when sink_count == 0, which it was not
  here, and msm_dp_pm_runtime_suspend() does not touch the PHY for DP.
  With this patch the controller suspended after the unplug, and a
  replug 20 s later trained the link with phy_init=1 and brought the
  picture back. The controller's runtime suspend does not power the PHY
  down, so the flag still describes the PHY correctly.

- Tearing the PHY down in detect() when the sink is gone (the natural
  fix for the scenario in the review) made it worse: the next detect()
  initialized the PHY again and hit

    phy phy-88e8000.phy.3: phy_power_on was called before phy_init

  and the SoC reset shortly after.

So I would keep this patch as it is. detect() clearing plugged so that
the unplug handler returns early is older than this change and is not
affected by it.

Thanks,
Joonhoe

Re: drm/msm/dp: hold one runtime PM reference per plugged state

[Val Packett]

On 9/29/26 10:06 AM, 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>
> ---
> Related pending work: "drm: handle IRQ_HPD events correctly" v5
> (https://lore.kernel.org/all/20260920-hpd-irq-events-v5-0-ff1de0090c72@oss.qualcomm.com/)
> removes the IRQ_HPD-as-plug path, and MST v7 26/35
> (https://lore.kernel.org/all/20260929-msm-dp-mst-v7-26-4b81473185e7@oss.qualcomm.com/)
> the detect-based hpd_notify for DP; the reference that
> msm_dp_bridge_detect() keeps is leaked either way. MST v7 27/35 touches
> the same lines as the first hunk; happy to rebase on top of it.

Also kinda related / touching the same places, tho not yet sent to the 
lists AFAIK:

"drm/msm/dp: Mark the link bad when an active sink is replugged"
https://github.com/radxa/kernel/commit/eb3247f6f9a39dcb15cf4b1df6d4732b1489b410

>   drivers/gpu/drm/msm/dp/dp_display.c | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
>
>
> base-commit: 6375e61c01e93e35ee7acd336a689ac1fae4b509
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 6abe30bdff12..4e47b659c71d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -412,10 +412,17 @@ 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;
> +	/*
> +	 * The plugged state holds one runtime PM reference, dropped by the
> +	 * unplug handler. The Type-C side repeats "connected" notifications,
> +	 * and detect() may have seen the sink first: take it only once.
> +	 */
> +	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);
> @@ -897,6 +904,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 +918,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);
> @@ -960,6 +969,12 @@ enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
>   		}
>   
>   		pm_runtime_put_sync(&dp->pdev->dev);
> +		/* the sink is gone: drop the plugged state's reference too */
> +		if (was_plugged)
> +			pm_runtime_put_sync(&dp->pdev->dev);
> +	} else if (was_plugged) {
> +		/* the plugged state already holds its reference */
> +		pm_runtime_put_sync(&dp->pdev->dev);
>   	}
>   
>   	return status;
This last chunk looks odd, is that not equivalent to just doing

if (was_plugged) pm_runtime_put_sync(&dp->pdev->dev);

on the outer level, outside of the outer if/else?


~val