Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm/dp: msm_dp_bridge_hpd_notify() runs irq_hpd on unpowered link --
commit 378c862ef1c761eb90130da10b12f8747c15b1a2 Author: Dmitry Baryshkov <[email protected]> drm/msm/dp: handle the IRQ_HPD events reported by USB-C This commit updates msm_dp_bridge_hpd_notify() to handle out-of-band DRM_CONNECTOR_DP_IRQ_HPD events reported via USB-C AltMode notifications and restructures the connected branch so that msm_dp_hpd_plug_handle() runs prior to msm_dp_irq_hpd_handle(). > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index ce919a26fba28..771c8b0744aee 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1566,11 +1566,13 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge > *bridge, > if (hpd_link_status == ISR_HPD_REPLUG_COUNT) { > msm_dp_hpd_unplug_handle(dp); > msm_dp_hpd_plug_handle(dp); > - } else if (hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT) { > - msm_dp_irq_hpd_handle(dp); > } else { > msm_dp_hpd_plug_handle(dp); > } > + > + if (hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT || > + event == DRM_CONNECTOR_DP_IRQ_HPD) > + msm_dp_irq_hpd_handle(dp); [Severity: High] Can calling msm_dp_irq_hpd_handle(dp) unconditionally after msm_dp_hpd_plug_handle(dp) trigger link maintenance while the mainlink and link clocks are still off? Previously, msm_dp_hpd_plug_handle(dp) and msm_dp_irq_hpd_handle(dp) were in mutually exclusive branches in msm_dp_bridge_hpd_notify(). With this change, when status is connector_status_connected and either event is DRM_CONNECTOR_DP_IRQ_HPD or hpd_link_status is ISR_IRQ_HPD_PULSE_COUNT, msm_dp_hpd_plug_handle(dp) runs first and is immediately followed by msm_dp_irq_hpd_handle(dp). When dp->plugged is false on entry (such as when a USB-C AltMode driver reports both HPD_STATE and IRQ_HPD together on initial plug, or when a monitor is plugged into a connected USB-C branch device whose prior sink_count == 0 caused msm_dp_hpd_unplug_handle() to clear dp->plugged), or after msm_dp_hpd_unplug_handle(dp) in the ISR_HPD_REPLUG_COUNT path, msm_dp_hpd_plug_handle(dp) runs: msm_dp_hpd_plug_handle() -> msm_dp_display_process_hpd_high() -> msm_dp_link_process_request() -> msm_dp_link_process_ds_port_status_change() At this point, msm_dp_hpd_plug_handle(dp) only enables AUX transfers and initializes the PHY; dp->msm_dp_display.power_on and ctrl->link_clks_on remain false until msm_dp_display_atomic_enable() is called. However, msm_dp_link_process_ds_port_status_change() has already updated link->prev_sink_count to match link->msm_dp_link.sink_count. When msm_dp_irq_hpd_handle(dp) then runs immediately afterward (or when an IRQ_HPD arrives while dp->plugged is true before atomic_enable() has enabled the mainlink): drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_irq_hpd_handle() { rc = msm_dp_link_process_request(dp->link); if (!rc) { sink_request = dp->link->sink_request; drm_dbg_dp(dp->drm_dev, "sink_request=%d\n", sink_request); if (sink_request & DS_PORT_STATUS_CHANGED) rc = msm_dp_display_process_hpd_high(dp); else rc = msm_dp_display_handle_irq_hpd(dp); } } If DP_DOWNSTREAM_PORT_STATUS_CHANGED is still set in DP_LANE_ALIGN_STATUS_UPDATED, msm_dp_display_process_hpd_high(dp) is called a second time, redundantly re-initializing LTTPRs and re-reading DPCD and EDID. Otherwise, because link->prev_sink_count already equals sink_count, msm_dp_link_process_ds_port_status_change() returns -EINVAL and msm_dp_link_process_request() falls through to msm_dp_link_process_link_status_update(): drivers/gpu/drm/msm/dp/dp_link.c:msm_dp_link_process_request() { ... } else { ret = msm_dp_link_process_link_status_update(link); if (!ret) { msm_dp_link->sink_request |= DP_LINK_STATUS_UPDATED; } Because the link has not been trained yet (dp->msm_dp_display.power_on is false and DP_INTERLANE_ALIGN_DONE is 0), drm_dp_channel_eq_ok() is false, so msm_dp_link_process_link_status_update() returns 0 and flags DP_LINK_STATUS_UPDATED. msm_dp_irq_hpd_handle(dp) then takes the else branch into msm_dp_display_handle_irq_hpd(dp) -> msm_dp_ctrl_handle_sink_request() -> msm_dp_ctrl_link_maintenance(): drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_link_maintenance() { int ret = 0; int training_step = DP_TRAINING_NONE; msm_dp_ctrl_push_idle(&ctrl->msm_dp_ctrl); ctrl->link->phy_params.p_level = 0; ctrl->link->phy_params.v_level = 0; ret = msm_dp_ctrl_setup_main_link(ctrl, panel, &training_step); if (ret) goto end; msm_dp_ctrl_clear_training_pattern(ctrl, panel, DP_PHY_DPRX); msm_dp_write_link(ctrl, REG_DP_STATE_CTRL, DP_STATE_CTRL_SEND_VIDEO); ret = msm_dp_ctrl_wait4video_ready(ctrl); } This drives msm_dp_ctrl_push_idle() and writes to link_base registers while !dp->msm_dp_display.power_on and !ctrl->link_clks_on, which causes unclocked register accesses, PUSH_IDLE timeouts, or a TrustZone force-stop and SoC reset (as noted in msm_dp_display_atomic_disable() where msm_dp_ctrl_push_idle() is guarded by dp->power_on). Should msm_dp_irq_hpd_handle(dp) be skipped when msm_dp_hpd_plug_handle(dp) just handled a fresh plug, and should link maintenance in the IRQ_HPD path check that the mainlink is powered on before attempting to retrain the link? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
