Re: [PATCH v8 19/29] drm/bridge_connector: suppress hotplug for IRQ_HPD without status changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/bridge_connector: unconditional detect() after hpd_notify()
  breaks msm_dp
- [Medium] drm/bridge_connector: suppressed IRQ_HPD uevent on downstream
  EDID change
--

commit f0f1fcb0e2054ca27f091a1526051359ce518167
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/bridge_connector: suppress hotplug for IRQ_HPD without status changes

This commit updates drm_bridge_connector_handle_hpd() to notify the bridge
chain first, invoke the detect bridge callback to update connector->status,
and send a hotplug event on IRQ_HPD only when the connector status changes.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 4ade63306653f..1d4502126bff3 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -160,15 +160,25 @@ static void drm_bridge_connector_handle_hpd(struct drm_bridge_connector *drm_bri
>  					    enum drm_connector_dp_event event)
>  {
>  	struct drm_connector *connector = &drm_bridge_connector->base;
> +	struct drm_bridge *detect = drm_bridge_connector->bridge_detect;
>  	struct drm_device *dev = connector->dev;
> +	enum drm_connector_status old_status, new_status;
> +	bool level_hpd;
> +	bool changed;
>  
> -	mutex_lock(&dev->mode_config.mutex);
> -	connector->status = status;
> -	mutex_unlock(&dev->mode_config.mutex);
> +	level_hpd = event == DRM_CONNECTOR_NO_EXTRA_STATUS;
>  
>  	drm_bridge_connector_hpd_notify(connector, status, event);
>  
> -	drm_kms_helper_connector_hotplug_event(connector);
> +	mutex_lock(&dev->mode_config.mutex);
> +	old_status = connector->status;
> +	new_status = detect ? detect->funcs->detect(detect, connector) : status;

[Severity: High]
Does calling detect->funcs->detect() unconditionally after
drm_bridge_connector_hpd_notify() leak runtime PM references and leave AUX
transfers enabled after unplug in msm_dp?

On HPD plug and IRQ_HPD events, msm_dp_bridge_hpd_notify() ->
msm_dp_hpd_plug_handle() calls pm_runtime_resume_and_get() and sets
dp->plugged = true.  Calling msm_dp_bridge_detect() right after acquires
another runtime PM reference unconditionally and skips
pm_runtime_put_sync() when priv->plugged is true:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_detect() {
	guard(mutex)(&priv->plugged_lock);
	ret = pm_runtime_resume_and_get(&dp->pdev->dev);
	...
	status = connector_status_connected;
	priv->plugged = true;
	...
end:
	if (!priv->plugged) {
		if (phy_deinit) {
			msm_dp_aux_enable_xfers(priv->aux, false);
			msm_dp_display_host_phy_exit(priv);
		}

		pm_runtime_put_sync(&dp->pdev->dev);
	}
}

Because msm_dp_hpd_unplug_handle() only drops one reference when clearing
dp->plugged, every HPD plug and IRQ_HPD event leaks a runtime PM reference
on dp->pdev->dev, preventing the controller from runtime suspending.  If a
DPCD read fails in msm_dp_bridge_detect() while plugged, it clears
priv->plugged and drops only its own reference, causing a subsequent
msm_dp_hpd_unplug_handle() to return early without tearing down the session
or dropping the plug reference.

On unplug (status == connector_status_disconnected), calling
detect->funcs->detect() after msm_dp_hpd_unplug_handle() re-enables AUX
transfers (leaving them enabled when dp->link->sink_count > 0 kept the PHY
initialized so phy_deinit is false) or re-initializes the PHY when
sink_count == 0.  In addition, for bridges whose detect callback does not
sense HPD directly (such as lt9611uxc_bridge_detect() when hpd_supported is
false, which returns connector_status_connected), calling detect() on an
unplug notification overwrites connector_status_disconnected with
connector_status_connected.

> +	connector->status = new_status;
> +	changed = new_status != old_status;
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	if (changed || level_hpd)
> +		drm_kms_helper_connector_hotplug_event(connector);
>  }

[Severity: Medium]
Can this suppress needed hotplug events when a downstream sink or EDID
changes on a DP branch device during IRQ_HPD while connector->status stays
connector_status_connected?

When a branch device signals DS_PORT_STATUS_CHANGED via IRQ_HPD without
transitioning through disconnected, msm_dp_irq_hpd_handle() ->
msm_dp_display_process_hpd_high() reads the new EDID and calls
drm_edid_connector_update(), which increments connector->epoch_counter.
Because level_hpd is false and old_status and new_status are both
connector_status_connected, changed is false and userspace is not notified
to reprobe the updated modes.  Checking whether connector->epoch_counter
changed across drm_bridge_connector_hpd_notify() and detect() would
preserve the hotplug event when the EDID changes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-msm-dp-mst-v8-0-d040fb33db5f@oss.qualcomm.com?part=19