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