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 <[email protected]> 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/[email protected]?part=10
