Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm/dcp: unsynchronized frame done state in dcp_crtc_hfi_frame_done_cb() - [Medium] drm/msm/dcp: missing payload validation in dcp_hfi_dispatch_frame_done() --
commit 7606ba3e12102c6ab2bc26f57c18f121dc3e91f7 Author: Jayant Shekhar <[email protected]> drm/msm/dcp: Route HFI display events This commit routes asynchronous HFI VSYNC and frame-scan completion display events from DCP firmware to the corresponding DRM CRTC callbacks, and adds a NULL check for dma_dev in dcp_hfi_init(). > diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c > b/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c > index 774a5afac0f0a..e54ccfafdae0f 100644 > --- a/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c > +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c > @@ -19,6 +19,7 @@ > #include "dcp_hfi.h" > #include "dcp_hfi_priv.h" > #include "dcp_drv.h" > +#include "dcp_crtc.h" > #include "hfi/hfi_packet.h" > #include "hfi/hfi_packer.h" > #include "hfi/hfi_unpacker.h" > @@ -260,18 +261,35 @@ static void dcp_hfi_store_caps_payload(struct dcp_hfi > *hfi, > dev_dbg(hfi->dev, "%s: stored %u bytes\n", name, n); > } > > +static void dcp_hfi_dispatch_frame_done(struct dcp_hfi *hfi, > + const struct hfi_packet_info *pkt_info) > +{ > + const struct hfi_display_frame_event_data *frame = NULL; > + u32 sequence_id = 0; > + > + if (pkt_info->payload_ptr && > + pkt_info->payload_size >= sizeof(*frame)) { > + frame = pkt_info->payload_ptr; > + sequence_id = frame->bufferflip_index; > + } > + dcp_crtc_hfi_frame_done_cb(hfi->drv, pkt_info->id & 0xFFFF, > + sequence_id); > +} [Severity: Medium] Should dcp_hfi_dispatch_frame_done() drop the packet and return early when pkt_info->payload_ptr is NULL or pkt_info->payload_size is smaller than sizeof(*frame)? As written, a truncated or empty HFI_COMMAND_DISPLAY_EVENT_FRAME_SCAN_COMPLETE packet falls through and invokes dcp_crtc_hfi_frame_done_cb() with a fabricated sequence_id of 0: drivers/gpu/drm/msm/dcp/dcp_crtc.c:dcp_crtc_hfi_frame_done_cb() { ... if (crtc) { struct dcp_crtc *dcp_crtc = to_dcp_crtc(crtc); /* A single pending commit makes equality valid across sequence wrap. */ if (sequence_id != dcp_crtc->pending_seq_id) { drm_warn(crtc->dev, "CRTC%u: ignoring stale FRAME_SCAN_COMPLETE seq=%u expected=%u\n", dcp_crtc->display_id, sequence_id, dcp_crtc->pending_seq_id); return; } dcp_crtc_frame_done_callback(crtc); } ... } If dcp_crtc->pending_seq_id happens to be 0 (such as before the first commit sequence is latched or when the 32-bit commit_seq_id counter wraps around to 0), this will falsely match pending_seq_id and prematurely complete the frame. When pending_seq_id is non-zero, it logs an unratelimited warning for a bogus sequence 0 event. > + > /* Route asynchronous events and capability packets before flag handling. */ > static bool dcp_hfi_dispatch_event(struct dcp_hfi *hfi, > const struct hfi_packet_info *pkt_info) > { > switch (pkt_info->cmd) { > case HFI_COMMAND_DISPLAY_EVENT_VSYNC: > + dcp_crtc_hfi_vblank_cb(hfi->drv, pkt_info->id & 0xFFFF); > return true; > > case HFI_COMMAND_DISPLAY_EVENT_FRAME_SCAN_START: > return true; > > case HFI_COMMAND_DISPLAY_EVENT_FRAME_SCAN_COMPLETE: > + dcp_hfi_dispatch_frame_done(hfi, pkt_info); > return true; [Severity: High] With HFI_COMMAND_DISPLAY_EVENT_FRAME_SCAN_COMPLETE now routed from the asynchronous VirtIO response callback (dcp_hfi_resp_callback() -> dcp_hfi_dispatch_event() -> dcp_hfi_dispatch_frame_done()) into dcp_crtc_hfi_frame_done_cb(), can concurrent access to dcp_crtc->pending_seq_id and dcp_crtc->pending_kickoff_cnt race with the commit path? In drivers/gpu/drm/msm/dcp/dcp_crtc.c, dcp_crtc_commit_kickoff() updates pending_kickoff_cnt and pending_seq_id without lock synchronization against dcp_crtc_hfi_frame_done_cb(): if (atomic_cmpxchg(&dcp_crtc->pending_kickoff_cnt, 0, 1)) { drm_err(crtc->dev, "CRTC%u: frame completion already pending\n", dcp_crtc->display_id); cstate->hfi_error = -EBUSY; return; } dcp_crtc->pending_seq_id = dcp_crtc->commit_seq_id; ret = dcp_hfi_display_commit(dcp_kms->hfi, session_id); if (ret) { drm_err(crtc->dev, "HFI commit failed for CRTC%u (session=%u): %d\n", dcp_crtc->display_id, session_id, ret); atomic_dec(&dcp_crtc->pending_kickoff_cnt); wake_up(&dcp_crtc->frame_done_wq); } This appears to allow several races when asynchronous frame-done events arrive: 1. pending_kickoff_cnt is set to 1 before dcp_crtc->pending_seq_id is updated to dcp_crtc->commit_seq_id. If a FRAME_SCAN_COMPLETE event carrying the previous sequence ID arrives in that window (for example after a prior commit timeout in dcp_crtc_wait_for_commit_done() reset pending_kickoff_cnt to 0 while leaving pending_seq_id at the old value, or from periodic scanout), dcp_crtc_hfi_frame_done_cb() sees sequence_id == dcp_crtc->pending_seq_id and calls dcp_crtc_frame_done_callback(), decrementing pending_kickoff_cnt back to 0 before dcp_hfi_display_commit() even runs. 2. If dcp_crtc_frame_done_callback() decrements pending_kickoff_cnt from 1 to 0 (either via the window above or because firmware sent FRAME_SCAN_COMPLETE while dcp_hfi_display_commit() timed out waiting for the command response) and dcp_hfi_display_commit() returns an error, the error path unconditionally runs atomic_dec(&dcp_crtc->pending_kickoff_cnt), underflowing pending_kickoff_cnt to -1. Because dcp_crtc_frame_done_callback() uses atomic_add_unless(&dcp_crtc->pending_kickoff_cnt, -1, 0), subsequent frame-done events will decrement it further (-2, -3, ...) and cause future commits to stall until DCP_FRAME_DONE_TIMEOUT_MS expires. 3. In dcp_crtc_atomic_flush(), ++dcp_crtc->commit_seq_id is sent to firmware via dcp_hfi_crtc_set_scan_sequence_id() and autorefresh is enabled via dcp_hfi_crtc_set_autorefresh() before dcp_crtc_commit_kickoff() sets pending_seq_id and pending_kickoff_cnt. If firmware emits a FRAME_SCAN_COMPLETE event for the new sequence ID before dcp_crtc_commit_kickoff() updates pending_seq_id, dcp_crtc_hfi_frame_done_cb() will drop it as a stale sequence mismatch. Should pending_seq_id and pending_kickoff_cnt updates and checks be synchronized under a spinlock across the flush, kickoff, and HFI callback paths? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
