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

[PATCH v2] 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>
---
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);
@@ -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);
+
 	return status;
 }
 
-- 
2.43.0

Re: [PATCH v2] 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: unplug teardown bypassed when detect() clears priv->plugged
--

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

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

This commit adjusts PM runtime references in the DisplayPort driver to hold
exactly one reference per plugged state. It takes a reference on plug and
drops it when detect() notices the sink is gone or when the unplug handler
runs.

Link: https://lore.kernel.org/all/20260929130627.959-1-26rote@gmail.com/

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 6abe30bdff120..4059cb9dba64b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -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);
> +
>  	return status;
>  }

[Severity: High]
Does this cement a hardware teardown bypass on unplug?

When a physical unplug occurs, msm_dp_bridge_detect() fails to read DPCD
caps and sets priv->plugged = false. By dropping the PM runtime reference
here using the newly added was_plugged flag, it completes the state
transition early.

The DRM core subsequently calls .hpd_notify() which invokes
msm_dp_hpd_unplug_handle(). Because the plugged flag was already cleared in
detect(), the unplug handler immediately early-returns:

msm_dp_hpd_unplug_handle() {
        guard(mutex)(&dp->plugged_lock);
        if (!dp->plugged)
                return 0;
        // Teardown code is skipped
        ...
}

This entirely bypasses drm_edid_connector_update(..., NULL),
drm_dp_set_subconnector_property(), and
msm_dp_display_handle_plugged_change(..., false). As a result, the EDID is
left stale, and the audio subsystem is never notified of the unplug (unless
an active audio stream was running).

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