Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/bridge: analogix_dp: force_connected_mode breaks analogix_dp_detect_hpd() - [High] drm/bridge: analogix_dp: unclocked MMIO access in analogix_dp_bridge_detect() --
commit 3f124dede2fd813765bd376dfd69c3031ce42a2c Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Restrict forced connected status to panel endpoints This commit restricts the forced connected status in the Analogix DP bridge driver so that it only applies when the bridge chain ends with a panel and no downstream bridge supports DRM_BRIDGE_OP_DETECT, relying on analogix_dp_detect_hpd() for other configurations. Link: https://lore.kernel.org/all/5209243.oAaWjsssbx@diego/ > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 853b6a6a7fe4d..df898f3142549 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -912,15 +912,30 @@ analogix_dp_bridge_detect(struct drm_bridge *bridge, > struct drm_connector *conne > struct analogix_dp_device *dp = to_dp(bridge); > enum drm_connector_status status = connector_status_disconnected; > > - if (dp->bridge.next_bridge) > - return connector_status_connected; > - > if (!analogix_dp_detect_hpd(dp)) > status = connector_status_connected; > > return status; > } > > +static bool analogix_dp_bridge_force_connected(struct analogix_dp_device *dp) > +{ > + struct drm_bridge *last_bridge; > + bool force_connected = false; > + > + drm_for_each_bridge_in_chain_from(dp->bridge.next_bridge, bridge) { > + if (bridge->ops & DRM_BRIDGE_OP_DETECT) > + return false; > + } > + > + last_bridge = drm_bridge_chain_get_last_bridge(dp->encoder); > + if (last_bridge) > + force_connected = drm_bridge_is_panel(last_bridge); > + drm_bridge_put(last_bridge); [Severity: High] What happens on Exynos boards with a legacy display-timings node (such as exynos5250-smdk5250.dts and exynos5420-smdk5420.dts) or on non-panel endpoints when analogix_dp_bridge_detect() runs while the controller is runtime-suspended? In analogix_dp_aux_done_probing(), Exynos boards with display-timings attach a bridge created via devm_drm_of_display_mode_bridge(): drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_aux_done_probing() { if (plat_data->dev_type == EXYNOS_DP && of_get_display_timings(dp->dev->of_node)) { next_bridge = devm_drm_of_display_mode_bridge(dp->dev, dp->dev->of_node, DRM_MODE_CONNECTOR_eDP); if (IS_ERR(next_bridge)) return PTR_ERR(next_bridge); bridge->next_bridge = drm_bridge_get(next_bridge); return component_add(dp->dev, plat_data->ops); } } Because devm_drm_of_display_mode_bridge() uses of_display_mode_bridge_funcs rather than panel_bridge_bridge_funcs, drm_bridge_is_panel(last_bridge) returns false and dp->force_connected_mode is set to false. In analogix_dp_bind(), a runtime PM reference is only kept when analogix_dp_require_pm_for_hpd_irq(dp) is true: drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_require_pm_for_hpd_irq() { return analogix_dp_is_rockchip(dp->plat_data->dev_type) && !dp->hpd_gpiod && !dp->force_hpd; } On Exynos platforms (and on any non-panel endpoint with force-hpd), the DP controller remains runtime-suspended with dp->clock disabled when the display is off. Unlike analogix_dp_set_bridge(), analogix_dpaux_transfer(), and analogix_dpaux_wait_hpd_asserted(), analogix_dp_bridge_detect() does not call pm_runtime_get_sync(dp->dev). When drm_bridge_connector_detect_ctx() calls analogix_dp_bridge_detect() -> analogix_dp_detect_hpd() -> analogix_dp_get_plug_in_status() while the display is disabled, won't readl(dp->reg_base + ANALOGIX_DP_SYS_CTL_3) (and analogix_dp_force_hpd()) access unclocked MMIO registers and cause an external abort? [ ... ] > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > index af5e5a8ec88d6..abbcdba1a4385 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > @@ -533,6 +533,9 @@ int analogix_dp_get_plug_in_status(struct > analogix_dp_device *dp) > { > u32 reg; > > + if (dp->force_connected_mode) > + return 0; > + > if (dp->hpd_gpiod) { > if (gpiod_get_value(dp->hpd_gpiod)) > return 0; [Severity: High] Does returning 0 early in analogix_dp_get_plug_in_status() when dp->force_connected_mode is true break hardware HPD handling in analogix_dp_detect_hpd() and analogix_dpaux_wait_hpd_asserted() for eDP panels? In analogix_dp_detect_hpd() (called from analogix_dp_set_bridge() during bridge .atomic_enable and from analogix_dpaux_transfer() during AUX transfers), the loop checks analogix_dp_get_plug_in_status(dp) before reaching the dp->force_hpd fallback: drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_detect_hpd() { while (timeout_loop < DP_TIMEOUT_LOOP_COUNT) { if (analogix_dp_get_plug_in_status(dp) == 0) return 0; timeout_loop++; usleep_range(1000, 1100); } ... if (!dp->force_hpd) return -ETIMEDOUT; ... analogix_dp_force_hpd(dp); } Because dp->force_connected_mode is true for panel endpoints, analogix_dp_get_plug_in_status(dp) now always returns 0 on the first iteration. As a result, analogix_dp_detect_hpd() returns 0 immediately without ever calling analogix_dp_force_hpd(dp) to set F_HPD | HPD_CTRL in ANALOGIX_DP_SYS_CTL_3. On eDP panel boards that use the force-hpd DT property, hardware HPD_STATUS(RO) remains 0 after analogix_dp_init_dp(), which prevents the AUX channel hardware from working. In addition, for eDP panels with a physical HPD line (!dp->force_hpd), both analogix_dp_detect_hpd() and analogix_dpaux_wait_hpd_asserted(): drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dpaux_wait_hpd_asserted() { ... ret = readx_poll_timeout(analogix_dp_get_plug_in_status, dp, val, !val, wait_us / 100, wait_us); } will return immediately without polling dp->hpd_gpiod or HPD_STATUS to wait for the panel to assert HPD after power-on. Should the force_connected_mode check be kept in analogix_dp_bridge_detect() instead of analogix_dp_get_plug_in_status()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
