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 plug and when powered off --
commit 975cd993397b923350a403b7275ed9d5223bb6a9 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 by USB-C AltMode handlers. > 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] Could running msm_dp_hpd_plug_handle() and msm_dp_irq_hpd_handle() sequentially here trigger link maintenance while the display and link clocks are still powered off, or attempt AUX transactions while the PHY is exited? 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_bridge_hpd_notify() now executes msm_dp_hpd_plug_handle(dp) followed unconditionally by msm_dp_irq_hpd_handle(dp). If dp->plugged is false when this runs (for example, when an AltMode driver reports both HPD state and HPD IRQ on initial connect, or when a monitor is plugged into a USB-C dock after msm_dp_hpd_unplug_handle() set dp->plugged to false on sink_count == 0), msm_dp_hpd_plug_handle() initializes the PHY and calls msm_dp_display_process_hpd_high(): drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_process_hpd_high() { ... msm_dp_link_process_request(dp->link); ... } That first call to msm_dp_link_process_request() updates link->prev_sink_count to match link->msm_dp_link.sink_count. However, drm_kms_helper_connector_hotplug_event() and msm_dp_display_atomic_enable() have not enabled the stream yet, so dp->msm_dp_display.power_on is false, ctrl->link_clks_on is false, and the mainlink is untrained. Immediately afterward, msm_dp_irq_hpd_handle() calls msm_dp_link_process_request() a second time: 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); } ... } Because link->prev_sink_count already equals sink_count, if DP_DOWNSTREAM_PORT_STATUS_CHANGED is no longer set in DP_LANE_ALIGN_STATUS_UPDATED, 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; } } ... } Since the link has not been trained yet (or if an out-of-band DRM_CONNECTOR_DP_IRQ_HPD arrives while dp->plugged is true before msm_dp_display_atomic_enable() turns the display on), drm_dp_channel_eq_ok() is false and DP_LINK_STATUS_UPDATED is flagged. That causes msm_dp_display_handle_irq_hpd() -> msm_dp_ctrl_handle_sink_request() -> msm_dp_ctrl_link_maintenance() to program DP_STATE_CTRL_PUSH_IDLE, mainlink setup, and DP_STATE_CTRL_SEND_VIDEO into dp_link registers while dp->msm_dp_display.power_on and ctrl->link_clks_on are false. In addition, if the display was disabled via msm_dp_display_atomic_post_disable() -> msm_dp_display_disable() while a sink remained connected (sink_count != 0): drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_disable() { ... if (dp->link->sink_count == 0) /* re-init the PHY so that we can listen to Dongle disconnect */ msm_dp_ctrl_reinit_phy(dp->ctrl); else msm_dp_display_host_phy_exit(dp); msm_dp_display->power_on = false; ... } dp->phy_initialized is false while dp->plugged stays true. If an out-of-band DRM_CONNECTOR_DP_IRQ_HPD arrives in this state, msm_dp_hpd_plug_handle() returns early because dp->plugged is true, and msm_dp_irq_hpd_handle() then attempts AUX reads while the PHY is powered down. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
