Re: [PATCH v5 16/18] drm/bridge: analogix_dp: Declare DRM_BRIDGE_OP_HPD for hotplug reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/bridge: analogix_dp: analogix_dp_irq_thread() ignores force_connected_mode
- [Medium] drm/bridge: analogix_dp: unconditional DRM_BRIDGE_OP_HPD breaks polling
--

commit 38abbb3331a9d5db0a9b89496e297a81a0f82400
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Declare DRM_BRIDGE_OP_HPD for hotplug reporting

This commit declares DRM_BRIDGE_OP_HPD on the Analogix DP bridge, adds no-op
.hpd_enable and .hpd_disable callbacks, and switches analogix_dp_irq_thread()
from drm_helper_hpd_irq_event() to drm_bridge_hpd_notify().

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index fdf3edfd298a2..44d431a89ff6f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c

[ ... ]

> @@ -741,9 +740,13 @@ static irqreturn_t analogix_dp_irq_thread(int irq, void *arg)
>  		hpd_detected = (irq_type & DP_IRQ_TYPE_HP_CABLE_IN) ||
>  			       (irq_type & DP_IRQ_TYPE_HP_CABLE_OUT);
>  	if (hpd_detected) {
> +		enum drm_connector_status status;
> +
>  		dev_dbg(dp->dev, "Detected cable status changed!\n");
> -		if (dp->drm_dev)
> -			drm_helper_hpd_irq_event(dp->drm_dev);
> +
> +		status = analogix_dp_get_plug_in_status(dp) == 0 ? connector_status_connected :
> +								   connector_status_disconnected;
> +		drm_bridge_hpd_notify(&dp->bridge, status);
>  	}

[Severity: High]
Does reporting raw analogix_dp_get_plug_in_status() via
drm_bridge_hpd_notify() here bypass dp->force_connected_mode and
dp->force_hpd?

Previously, drm_helper_hpd_irq_event() routed through
analogix_dp_bridge_detect(), which honours both flags:

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_bridge_detect() {
	if (dp->force_connected_mode)
		return connector_status_connected;

	ret = pm_runtime_resume_and_get(dp->dev);
	if (ret)
		return connector_status_unknown;

	if (!analogix_dp_detect_hpd(dp))
		status = connector_status_connected;
	...
}

In contrast, drm_bridge_hpd_notify() calls drm_bridge_connector_handle_hpd(),
which directly overwrites connector->status without calling .detect():

drivers/gpu/drm/display/drm_bridge_connector.c:drm_bridge_connector_handle_hpd() {
	mutex_lock(&dev->mode_config.mutex);
	connector->status = status;
	mutex_unlock(&dev->mode_config.mutex);

	drm_bridge_connector_hpd_notify(connector, status);

	drm_kms_helper_connector_hotplug_event(connector);
}

On boards where the bridge chain ends with a fixed panel (where
dp->force_connected_mode is true) and HPD is wired up, powering down the
panel or downstream bridge (such as nxp,ptn3460 or parade,ps8625) during DPMS
off or after ptn3460_edid_read() drops the HPD line. When
analogix_dp_irq_thread() runs, won't it report connector_status_disconnected
for the built-in panel and, on PTN3460 boards, trigger a continuous hotplug
and EDID read loop?

[ ... ]

> @@ -1611,7 +1624,8 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  		return ret;
>  	}
>  
> -	bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
> +	bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
> +		      DRM_BRIDGE_OP_HPD;

[Severity: Medium]
What happens on platforms where hardware HPD interrupts cannot fire while the
DP controller is runtime-suspended if DRM_BRIDGE_OP_HPD is set
unconditionally?

In drm_bridge_connector_init(), setting DRM_BRIDGE_OP_HPD switches
connector->polled from DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT to DRM_CONNECTOR_POLL_HPD, which disables
periodic polling in output_poll_execute():

drivers/gpu/drm/display/drm_bridge_connector.c:drm_bridge_connector_init() {
	if (bridge_connector->bridge_hpd)
		connector->polled = DRM_CONNECTOR_POLL_HPD;
	else if (bridge_connector->bridge_detect)
		connector->polled = DRM_CONNECTOR_POLL_CONNECT
				  | DRM_CONNECTOR_POLL_DISCONNECT;
}

However, without dp->hpd_gpiod, the Analogix DP controller requires its clocks
and PHY power to remain active to generate native HPD interrupts, and
analogix_dp_bind() only holds a runtime PM reference when
analogix_dp_require_pm_for_hpd_irq() is true:

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_require_pm_for_hpd_irq() {
	return (dp->plat_data->dev_type == RK3576_EDP || dp->plat_data->dev_type == RK3588_EDP) &&
	       !dp->hpd_gpiod && !dp->force_hpd && !dp->force_connected_mode;
}

On RK3288_DP, RK3399_EDP, and EXYNOS_DP without hpd-gpios (or when
dp->force_hpd is set), the device stays runtime-suspended when idle and cannot
raise HPD interrupts. Won't advertising DRM_BRIDGE_OP_HPD unconditionally
stop hotplug detection from working on those configurations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027132810.1746118-1-damon.ding@rock-chips.com?part=16