Re: [PATCH v4 10/14] drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/bridge: analogix_dp: AUX powered down after analogix_dp_bridge_disable()
- [High] drm/bridge: analogix_dp: unmasked native IRQ active across runtime suspend
- [Medium] drm/bridge: analogix_dp: analogix_dp_require_pm_for_hpd_irq() blocks eDP runtime suspend
- [Medium] drm/bridge: analogix_dp: missing DRM_BRIDGE_OP_HPD in analogix_dp_bind()
--

commit 084692aff33ec03020fcf182af076977c2df993a
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection

This commit acquires a runtime PM reference in analogix_dp_bind() and
releases it in analogix_dp_unbind() for Rockchip native HPD pin mode so
the DP controller stays powered and clocked to detect hotplug interrupts
while the display pipeline is inactive.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index addd319719de4..38857508d9392 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -39,6 +39,12 @@
>  
>  static const bool verify_fast_training;
>  
> +static bool analogix_dp_require_pm_for_hpd_irq(struct analogix_dp_device *dp)
> +{
> +	return analogix_dp_is_rockchip(dp->plat_data->dev_type) && !dp->hpd_gpiod &&
> +	       !dp->force_hpd;
> +}

[Severity: Medium]
Does analogix_dp_require_pm_for_hpd_irq() also return true on Rockchip boards
with a fixed eDP panel that do not specify force-hpd or hpd-gpios in Device
Tree (such as rk3399-gru-kevin.dts, rk3399-gru-bob.dts,
rk3399-sapphire-excavator.dts, and rk3288-veyron-jaq/jerry/minnie/tiger.dts)?

On those systems, analogix_dp_bridge_detect() unconditionally returns
connector_status_connected when dp->plat_data->next_bridge is present:

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

Later in the series, commit "drm/bridge: analogix_dp: Handle HPD notification
from downstream bridge" adds !dp->hpd_notify_mode to
analogix_dp_require_pm_for_hpd_irq(), and commit "drm/bridge: analogix_dp:
Restrict forced connected status to panel endpoints" introduces
dp->force_connected_mode for fixed panel endpoints where
analogix_dp_get_plug_in_status() unconditionally returns 0. However,
analogix_dp_require_pm_for_hpd_irq() is not updated to exclude
dp->force_connected_mode (or dp->plat_data->next_bridge here).

Because analogix_dp_bind() holds a runtime PM reference until
analogix_dp_unbind(), pm_runtime_put_sync(dp->dev) in
analogix_dp_bridge_disable() only drops the usage counter from 2 to 1 during
DPMS off. Doesn't this prevent rockchip_dp_suspend() and
analogix_dp_suspend() from running when the screen is blanked, leaving the DP
PHY, dp->clock, dp->pclk, and the SoC power domain (RK3399_PD_EDP /
RK3288_PD_VIO) powered on and leaving DP_IRQ_TYPE_HP_CHANGE unmuted while
panel_edp_suspend() turns off the panel power supply?

> +
>  static void analogix_dp_init_dp(struct analogix_dp_device *dp)
>  {
>  	analogix_dp_reset(dp);
> @@ -1571,6 +1577,12 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  		goto err_unregister_aux;
>  	}
>  
> +	if (analogix_dp_require_pm_for_hpd_irq(dp)) {
> +		ret = pm_runtime_resume_and_get(dp->dev);
> +		if (ret)
> +			goto err_unregister_aux;
> +	}

[Severity: High]
When analogix_dp_require_pm_for_hpd_irq(dp) is true, can DisplayPort AUX
transactions fail after the bridge has been enabled and then disabled (for
example, on DPMS off or cable unplug and replug)?

In analogix_dp_bridge_disable(), disabling the bridge powers down all
internal analog blocks before dropping its runtime PM reference:

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_bridge_disable() {
	...
	analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);

	pm_runtime_put_sync(dp->dev);
	...
}

drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c:analogix_dp_set_analog_power_down() {
	...
	case POWER_ALL:
		if (enable) {
			reg = DP_ALL_PD;
			writel(reg, dp->reg_base + phy_pd_addr);
		} else {
	...
}

Writing DP_ALL_PD (0xff) to ANALOGIX_DP_PD asserts both RK_AUX_PD (BIT(5))
and DP_INC_BG (BIT(7), the internal PHY bandgap on Rockchip).

Previously, pm_runtime_put_sync(dp->dev) in analogix_dp_bridge_disable()
dropped the usage count to 0 and suspended the device, so the next AUX
transfer in analogix_dpaux_transfer() called pm_runtime_get_sync(dp->dev),
running analogix_dp_resume() -> rockchip_dp_poweron() and
analogix_dp_init_dp() -> analogix_dp_init_aux() to reset the controller and
clear ANALOGIX_DP_PD.

With the persistent runtime PM reference held in analogix_dp_bind(),
pm_runtime_put_sync(dp->dev) in analogix_dp_bridge_disable() only decrements
the usage count from 2 to 1, keeping dp->dev in RPM_ACTIVE. When a sink is
subsequently probed while dpms_mode is DRM_MODE_DPMS_OFF:

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dpaux_transfer() {
	...
	pm_runtime_get_sync(dp->dev);

	ret = analogix_dp_detect_hpd(dp);
	if (ret)
		goto out;

	ret = analogix_dp_transfer(dp, msg);
	...
}

pm_runtime_get_sync(dp->dev) is a no-op because dp->dev is already active, so
analogix_dp_transfer() attempts AUX transactions while ANALOGIX_DP_PD is still
DP_ALL_PD (0xff). Even when analogix_dp_transfer() times out after 500 ms and
calls analogix_dp_init_aux(dp) in its aux_error path, analogix_dp_init_aux()
only clears AUX_BLOCK (RK_AUX_PD) and leaves DP_INC_BG asserted. Should
analogix_dp_bridge_disable() or analogix_dpaux_transfer() keep the AUX block
and bandgap powered when the controller remains runtime-active?

[Severity: Medium]
Also, will native HPD interrupts actually be processed by
drm_helper_hpd_irq_event() without DRM_BRIDGE_OP_HPD set in bridge->ops?

Earlier in analogix_dp_bind(), bridge->ops is initialized to:

	bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;

Both rockchip_dp_bind() and exynos_dp_bind() create the connector via
drm_bridge_connector_init():

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;
	...
}

In native HPD pin mode (and internal hpd_gpiod mode), no bridge in the chain
sets DRM_BRIDGE_OP_HPD, so bridge_connector->bridge_hpd is NULL and
connector->polled does not have DRM_CONNECTOR_POLL_HPD set. When the HPD IRQ
fires and analogix_dp_irq_thread() calls drm_helper_hpd_irq_event(dp->drm_dev),
drm_helper_hpd_irq_event() skips any connector without DRM_CONNECTOR_POLL_HPD:

drivers/gpu/drm/drm_probe_helper.c:drm_helper_hpd_irq_event() {
	...
	drm_for_each_connector_iter(connector, &conn_iter) {
		/* Only handle HPD capable connectors. */
		if (!(connector->polled & DRM_CONNECTOR_POLL_HPD))
			continue;
	...
}

Doesn't this cause native HPD interrupts to be ignored and fall back to the
10-second periodic poll worker?

> +
>  	enable_irq(dp->irq);

[Severity: High]
Is it safe to unconditionally call enable_irq(dp->irq) here when
analogix_dp_require_pm_for_hpd_irq(dp) is false and !dp->hpd_gpiod (for
example, when dp->force_hpd is true on Rockchip, or on EXYNOS_DP without
hpd-gpios)?

In those configurations, dp->irq is the native DP controller interrupt, and
dp->dev is allowed to runtime-suspend. Whenever dp->dev runtime-resumes (such
as in analogix_dpaux_transfer() or analogix_dp_set_bridge()),
analogix_dp_init_dp() calls analogix_dp_config_interrupt():

drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c:analogix_dp_config_interrupt() {
	if (dp->hpd_gpiod) {
		analogix_dp_mute_hpd_interrupt(dp, HPD_IRQ_ALL);
		return;
	}

	if (analogix_dp_is_rockchip(dp->plat_data->dev_type))
		analogix_dp_unmute_hpd_interrupt(dp, DP_IRQ_TYPE_HP_CHANGE);
	else
		analogix_dp_unmute_hpd_interrupt(dp, HPD_IRQ_ALL);
}

Because analogix_dp_config_interrupt() does not check dp->force_hpd, it
unmasks the controller's HPD interrupts. In force-HPD mode,
analogix_dp_init_hpd() clears F_HPD | HPD_CTRL in ANALOGIX_DP_SYS_CTL_3 and
analogix_dp_detect_hpd() calls analogix_dp_force_hpd() to set F_HPD |
HPD_CTRL, generating HPD transitions that fire the unmasked controller
interrupt.

Furthermore, because analogix_dp_suspend() does not disable or synchronize
dp->irq before disabling dp->pclk and dp->clock, if analogix_dp_hardirq()
wakes analogix_dp_irq_thread() right before dp->dev runtime-suspends (at the
end of analogix_dpaux_transfer() or in analogix_dp_bridge_disable()),
analogix_dp_irq_thread() will access the controller registers while unclocked:

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_irq_thread() {
	...
	irq_type = analogix_dp_get_irq_type(dp);
	if (!dp->hpd_gpiod && irq_type)
		analogix_dp_clear_hotplug_interrupts(dp, irq_type);
	...
}

Can this cause unclocked MMIO reads and writes to ANALOGIX_DP_COMMON_INT_STA_4
and ANALOGIX_DP_INT_STA (resulting in a synchronous external abort / SError
on ARM/ARM64) as well as spurious hotplug events?

>  
>  	return 0;

[ ... ]

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