Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/i915/dp_mst: VC payload left allocated in mst_stream_post_disable() - [Medium] drm/i915/dp_mst: broken retrain in intel_dp_retrain_link() on partial unplug - [Medium] drm/i915/dp_mst: race with delayed connector unregistration in mst_connector_stream_disconnected() --
commit 25f773daa729bda6d48a9b3f730e6fd95cc19639 Author: Xiao Lu <[email protected]> drm/i915/dp_mst: Skip link retrain and ACT for disconnected MST streams This commit skips disconnected MST streams when selecting pipes to modeset in intel_dp_retrain_link() and skips generating and waiting for ACT in mst_stream_post_disable() when drm_dp_remove_payload_part1() did not remove the payload from the branch device. Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/17741 > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > index ac1aff6edf052..41bfb8b1ae0c3 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -2576,6 +2576,9 @@ static int intel_dp_retrain_link(struct intel_encoder > *encoder, > if (ret) > return ret; > > + /* Nothing to retrain for streams whose MST sink has left the topology > */ > + pipe_mask &= ~intel_dp_mst_disconnected_pipes(intel_dp); > + > if (pipe_mask == 0) > return 0; [Severity: Medium] What happens in intel_dp_retrain_link() in drivers/gpu/drm/i915/display/intel_dp_link_training.c when an MST hub has two active streams and only one downstream sink is unplugged (leaving its CRTC enabled until userspace disables it) while the other stream stays connected? In drivers/gpu/drm/i915/display/intel_dp_mst.c, physical DP link teardown and retraining only run when intel_dp->mst.active_streams transitions to 0 in mst_stream_post_disable() and from 0 to 1 in mst_stream_pre_enable(): drivers/gpu/drm/i915/display/intel_dp_mst.c:mst_stream_pre_enable() { ... if (first_mst_stream) { primary_encoder->pre_enable(state, primary_encoder, pipe_config, NULL); intel_mst_reprobe_topology(intel_dp, pipe_config); } ret = drm_dp_add_payload_part1(&intel_dp->mst.mgr, mst_state, drm_atomic_get_mst_payload_state(mst_state, connector->mst.port)); if (ret < 0) intel_dp_queue_modeset_retry_for_link(state, primary_encoder, pipe_config); ... } When pipe_mask masks out only the disconnected pipe while another pipe on the same MST topology remains connected, intel_modeset_commit_pipes_for_atomic_state() only sets uapi.connectors_changed on the connected CRTC. If the disconnected stream is on the MST master transcoder (or on DISPLAY_VER < 12) and link parameters have not changed, intel_crtc_check_fastset() in drivers/gpu/drm/i915/display/intel_display.c clears uapi.mode_changed on the disconnected CRTC. Only the connected stream then undergoes a modeset, so intel_dp->mst.active_streams transitions 2 -> 1 -> 2, first_mst_stream is false, primary_encoder->pre_enable() is never called to retrain the link, and intel_dp_link_training_set_force_retrain() still clears force_retrain. Conversely, if the connected stream is on the MST master transcoder and the disconnected stream is on a slave transcoder on DISPLAY_VER >= 12 (or if link parameters changed), intel_atomic_check() in drivers/gpu/drm/i915/display/intel_display.c still forces a modeset on the disconnected slave CRTC: drivers/gpu/drm/i915/display/intel_display.c:intel_atomic_check() { ... if (intel_dp_mst_is_slave_trans(new_crtc_state)) { enum transcoder master = new_crtc_state->mst_master_transcoder; if (intel_cpu_transcoders_need_modeset(state, BIT(master))) intel_crtc_flag_modeset(new_crtc_state); } ... } Wouldn't that still run mst_stream_pre_enable() and mst_stream_enable() on the disconnected stream, where drm_dp_add_payload_part1() fails (marking link_status bad on the link via intel_dp_queue_modeset_retry_for_link()) and drm_dp_check_act_status() in mst_stream_enable() still stalls for 3000 ms? > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 7023619aaaba9..a1d4c37d001ac 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -1109,6 +1109,7 @@ static void mst_stream_post_disable(struct > intel_atomic_state *state, > drm_atomic_get_mst_payload_state(new_mst_state, > connector->mst.port); > struct intel_crtc *pipe_crtc; > bool last_mst_stream; > + bool payload_removed; > > last_mst_stream = intel_dp_mst_dec_active_streams(intel_dp); > > @@ -1124,16 +1125,20 @@ static void mst_stream_post_disable(struct > intel_atomic_state *state, > > intel_disable_transcoder(old_crtc_state); > > - drm_dp_remove_payload_part1(&intel_dp->mst.mgr, new_mst_state, > new_payload); > + payload_removed = drm_dp_remove_payload_part1(&intel_dp->mst.mgr, > new_mst_state, > + new_payload); > > - intel_ddi_clear_act_sent(encoder, old_crtc_state); > + /* No ACT is needed if the payload table of the branch wasn't changed */ > + if (payload_removed) { > + intel_ddi_clear_act_sent(encoder, old_crtc_state); > > - intel_de_rmw(display, > - TRANS_DDI_FUNC_CTL(display, > old_crtc_state->cpu_transcoder), > - TRANS_DDI_DP_VC_PAYLOAD_ALLOC, 0); > + intel_de_rmw(display, > + TRANS_DDI_FUNC_CTL(display, > old_crtc_state->cpu_transcoder), > + TRANS_DDI_DP_VC_PAYLOAD_ALLOC, 0); > > - intel_ddi_wait_for_act_sent(encoder, old_crtc_state); > - drm_dp_check_act_status(&intel_dp->mst.mgr); > + intel_ddi_wait_for_act_sent(encoder, old_crtc_state); > + drm_dp_check_act_status(&intel_dp->mst.mgr); > + } [Severity: Medium] When payload_removed is false in mst_stream_post_disable() in drivers/gpu/drm/i915/display/intel_dp_mst.c, does skipping the TRANS_DDI_FUNC_CTL update leave TRANS_DDI_DP_VC_PAYLOAD_ALLOC set in hardware? In drivers/gpu/drm/i915/display/intel_ddi.c, intel_ddi_disable_transcoder_func() performs a read-modify-write on TRANS_DDI_FUNC_CTL without clearing TRANS_DDI_DP_VC_PAYLOAD_ALLOC: drivers/gpu/drm/i915/display/intel_ddi.c:intel_ddi_disable_transcoder_func() { ... ctl = intel_de_read(display, TRANS_DDI_FUNC_CTL(display, cpu_transcoder)); drm_WARN_ON(crtc->base.dev, ctl & TRANS_DDI_HDCP_SIGNALLING); ctl &= ~TRANS_DDI_FUNC_ENABLE; if (IS_DISPLAY_VER(display, 8, 10)) ctl &= ~(TRANS_DDI_PORT_SYNC_ENABLE | TRANS_DDI_PORT_SYNC_MASTER_SELECT_MASK); if (DISPLAY_VER(display) >= 12) { if (!intel_dp_mst_is_master_trans(crtc_state)) { ctl &= ~(TGL_TRANS_DDI_PORT_MASK | TRANS_DDI_MODE_SELECT_MASK); } } else { ctl &= ~(TRANS_DDI_PORT_MASK | TRANS_DDI_MODE_SELECT_MASK); } intel_de_write(display, TRANS_DDI_FUNC_CTL(display, cpu_transcoder), ctl); ... } If a downstream branch or sink port was unplugged while other streams on the MST hub remain active (or when the stale CRTC is later disabled by userspace), skipping the clearing of TRANS_DDI_DP_VC_PAYLOAD_ALLOC leaves the VC payload allocated in the source transcoder while drm_dp_remove_payload_part2() shifts the remaining payloads' start slots in software, and violates the transcoder disable sequence requiring TRANS_DDI_DP_VC_PAYLOAD_ALLOC to be cleared before TRANS_DDI_FUNC_ENABLE. Should TRANS_DDI_DP_VC_PAYLOAD_ALLOC always be cleared in mst_stream_post_disable() even when payload_removed is false, only gating the ACT status clear/wait on payload_removed? [ ... ] > @@ -2172,6 +2177,20 @@ bool intel_dp_mst_crtc_needs_modeset(struct > intel_atomic_state *state, > return false; > } > > +static bool mst_connector_stream_disconnected(struct intel_connector > *connector) > +{ > + if (!connector->mst.dp) > + return false; > + > + if (!connector->mst.dp->mst.mgr.mst_state) > + return true; > + > + if (drm_connector_is_unregistered(&connector->base)) > + return true; > + > + return false; > +} [Severity: Medium] Can mst_connector_stream_disconnected() in drivers/gpu/drm/i915/display/intel_dp_mst.c miss a disconnected MST stream in the scenario described by the commit message if intel_dp_retrain_link() runs before drm_dp_delayed_destroy_work() finishes unregistering the old connector? When HPD drops and comes back in intel_ddi_hotplug() -> intel_encoder_hotplug() -> intel_dp_detect(), tearing down the old topology via drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst.mgr, false) queues mgr->delayed_destroy_work to call drm_connector_unregister() asynchronously in drm_dp_delayed_destroy_port(): drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_delayed_destroy_port() { drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE, port->mcs); if (port->connector) { drm_connector_unregister(port->connector); drm_connector_put(port->connector); } drm_dp_mst_put_port_malloc(port); } During the reconnect detection in intel_dp_detect(), intel_dp_mst_configure() immediately calls drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst.mgr, true), which sets mgr->mst_state = true again without flushing mgr->delayed_destroy_work, and intel_ddi_hotplug() then schedules intel_dp_retrain_link() via intel_dp_check_link_state(). (Similarly, if a downstream branch device is unplugged via CSN while the primary hub stays connected, mgr->mst_state remains true while delayed_destroy_work is queued.) If intel_dp_retrain_link() runs before drm_dp_delayed_destroy_work() reaches drm_connector_unregister(&connector->base), won't both !connector->mst.dp->mst.mgr.mst_state and drm_connector_is_unregistered(&connector->base) evaluate to false even though connector->mst.port is no longer in the topology? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
