Re: [PATCH v2 13/23] drm/msm/dcp: Route HFI display events

[sashiko-bot@kernel.org]

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 <jayant.shekhar@oss.qualcomm.com>

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/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=13