Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/msm/dcp: dcp_crtc_atomic_begin() opens session on non-modeset commits --
commit 9b8003ef1934260f1a1ec514fc980b7068b0a204 Author: Jayant Shekhar <[email protected]> drm/msm/dcp: Integrate the DSI bridge with KMS This commit connects the DCP DSI bridge to the DCP KMS pipeline by adding helpers to prepare the CRTC display session during atomic_begin and check whether planes require dirtyfb updates during atomic_check. > diff --git a/drivers/gpu/drm/msm/dcp/dcp_crtc.c > b/drivers/gpu/drm/msm/dcp/dcp_crtc.c > index ec4f74c2920f8..173a07df71c5f 100644 > --- a/drivers/gpu/drm/msm/dcp/dcp_crtc.c > +++ b/drivers/gpu/drm/msm/dcp/dcp_crtc.c > @@ -147,6 +147,12 @@ static void dcp_crtc_atomic_begin(struct drm_crtc *crtc, > return; > } > > + ret = dcp_dpu_kms_prepare_crtc_session(dcp_kms, crtc); > + if (ret) { > + cstate->hfi_error = ret; > + return; > + } [Severity: Medium] Should dcp_dpu_kms_prepare_crtc_session() only open a new session when drm_atomic_crtc_needs_modeset(crtc->state) is true, or can calling it unconditionally in dcp_crtc_atomic_begin() leave the session partially initialized on non-modeset commits? In msm_atomic_commit_tail(), drm_atomic_helper_commit_planes() (which calls dcp_crtc_atomic_begin()) runs before drm_atomic_helper_commit_modeset_enables() (which calls dcp_crtc_atomic_enable(), dcp_dsi_bridge_atomic_pre_enable(), and dcp_dsi_bridge_atomic_enable(), and only runs when drm_atomic_crtc_needs_modeset(crtc->state) is true). If an initial modeset commit fails in dcp_dsi_bridge_prepare_session() or in dcp_dsi_bridge_atomic_enable(): drivers/gpu/drm/msm/dcp/dcp_dsi_bridge.c:dcp_dsi_bridge_atomic_enable() { ... ret = dcp_hfi_display_post_enable(kms->hfi, dsi_bridge->session_id); if (ret) { drm_err(bridge->dev, "DSI%u: HFI DISPLAY_POST_ENABLE failed: %d\n", dsi_bridge->intf_idx, ret); goto err_close; } ... err_close: dcp_hfi_clear_power_control_cb(kms->hfi, kms); dcp_hfi_display_close(kms->hfi, dsi_bridge->session_id); dsi_bridge->session_id = 0; dsi_bridge->session_open = false; dcp_dsi_bridge_clear_session(dsi_bridge); } the session is closed and dsi_bridge->session_open is reset to false, while crtc->state->active remains true. On the next non-modeset plane update or pageflip where drm_atomic_crtc_needs_modeset(crtc->state) is false, dcp_crtc_atomic_begin() -> dcp_dpu_kms_prepare_crtc_session() -> dcp_dsi_bridge_prepare_session() sees !dsi_bridge->session_open, opens a new HFI session, sets dsi_bridge->session_open = true, and publishes session_id to the CRTC: drivers/gpu/drm/msm/dcp/dcp_dsi_bridge.c:dcp_dsi_bridge_prepare_session() { ... dsi_bridge->session_id = opened_session_id; dsi_bridge->session_open = true; dcp_dsi_bridge_publish_session(dsi_bridge); ... } Because drm_atomic_helper_commit_modeset_enables() skips the CRTC and bridge enable callbacks on a non-modeset commit, dcp_crtc_atomic_enable() and dcp_dsi_bridge_atomic_enable() never run, skipping HFI_MODE_DPMS_ON, drm_crtc_vblank_on(), panel bridge enable, HFI_COMMAND_DISPLAY_POST_ENABLE, and HFI_EVENT_FRAME_SCAN_COMPLETE registration. When dcp_crtc_commit_kickoff() then submits frame triggers for that session, dcp_crtc_wait_for_commit_done() will wait on frame_done_wq for an event that was never registered and time out after 500ms on every subsequent commit: drivers/gpu/drm/msm/dcp/dcp_crtc.c:dcp_crtc_wait_for_commit_done() { ... if (!wait_event_timeout(dcp_crtc->frame_done_wq, atomic_read(&dcp_crtc->pending_kickoff_cnt) == 0, timeout)) { drm_err(crtc->dev, "frame done timeout for CRTC%u\n", dcp_crtc->display_id); atomic_set(&dcp_crtc->pending_kickoff_cnt, 0); } } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
