Re: [PATCH v2 09/23] drm/msm/dcp: Add generic HFI display commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dcp: completion corruption and unlocked caps_cache in dcp_hfi_kms.c
- [High] drm/msm/dcp: pipe count clobbering in dcp_hfi_parse_init_caps_kv()
--

commit 90bc8a62bf35e8d6f13a5451d4e21b3d608a5097
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

drm/msm/dcp: Add generic HFI display commands

This commit adds generic HFI display command helpers and capability parsing
for the firmware-owned DCP display pipeline, translating DRM state for
resources, CRTCs, planes, bandwidth, and frame sequencing into HFI commands
and properties.

> diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi_kms.c b/drivers/gpu/drm/msm/dcp/dcp_hfi_kms.c
> new file mode 100644
> index 0000000000000..ac8000a8ee900
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi_kms.c
> @@ -0,0 +1,366 @@

[ ... ]

> +static void dcp_hfi_caps_cache_init(struct dcp_hfi_caps_cache *cache)
> +{
> +	init_completion(&cache->device_done);
> +	init_completion(&cache->display_done);
> +}
> +
> +static void dcp_hfi_caps_cache_start(struct dcp_hfi *hfi)
> +{
> +	memset(&hfi->caps_cache, 0, sizeof(hfi->caps_cache));
> +	dcp_hfi_caps_cache_init(&hfi->caps_cache);
> +}
> +
> +static void dcp_hfi_caps_cache_stop(struct dcp_hfi *hfi)
> +{
> +	memset(&hfi->caps_cache, 0, sizeof(hfi->caps_cache));
> +}
> +
> +int dcp_hfi_send_device_init(struct dcp_hfi *hfi)
> +{
> +	int ret;
> +
> +	mutex_lock(&hfi->caps_cache_lock);
> +	dcp_hfi_caps_cache_start(hfi);
> +
> +	/* Payload and flags follow the HFI device-command definition. */
> +	dev_dbg(hfi->dev,
> +		"HFI DEVICE_INIT cmd=0x%x flags=0x%x timeout=%ums\n",
> +		HFI_COMMAND_DEVICE_INIT,
> +		HFI_TX_FLAGS_RESPONSE_REQUIRED | HFI_TX_FLAGS_NON_DISCARDABLE,
> +		DCP_HFI_TIMEOUT_INIT_MS);
> +
> +	ret = dcp_hfi_send_sync(hfi,
> +				HFI_COMMAND_DEVICE_INIT,
> +				HFI_CMD_BUFF_DEVICE, 0,
> +				HFI_PAYLOAD_NONE, NULL, 0,
> +				HFI_TX_FLAGS_RESPONSE_REQUIRED |
> +				HFI_TX_FLAGS_NON_DISCARDABLE,
> +				NULL, 0,
> +				DCP_HFI_TIMEOUT_INIT_MS);
> +	if (ret) {
> +		dev_err(hfi->dev, "HFI DEVICE_INIT failed: %d\n", ret);
> +		dcp_hfi_caps_cache_stop(hfi);
> +	} else {
> +		dev_dbg(hfi->dev, "HFI DEVICE_INIT succeeded\n");
> +	}
> +
> +	mutex_unlock(&hfi->caps_cache_lock);
> +	return ret;
> +}
> +
> +static void dcp_hfi_parse_init_caps_kv(struct dcp_caps *caps,
> +				       u32 prop_key,
> +				       const u32 *payload,
> +				       u32 max_words)
> +{
> +	u32 prop_id = HFI_PROP_ID(prop_key);
> +	u32 count, i;
> +
> +	if (!payload || !max_words)
> +		return;
> +
> +	switch (prop_id) {

[ ... ]

> +	case HFI_PROPERTY_DEVICE_INIT_VIG_INDICES:
> +		if (max_words < 1)
> +			break;
> +		count = payload[0];
> +		if (count > max_words - 1)
> +			count = max_words - 1;
> +		caps->num_vig_pipes = 0;
> +		caps->virt_vig_count = 0;
> +		for (i = 0; i < count; i++) {
> +			u32 idx = payload[1 + i];
> +
> +			if (DCP_REC_ID(idx) == 0) {
> +				if (caps->num_vig_pipes < DCP_MAX_SSPP_COUNT)
> +					caps->vig_pipe_ids[caps->num_vig_pipes++] = idx;
> +			} else {
> +				if (caps->virt_vig_count < DCP_MAX_SSPP_COUNT)
> +					caps->vig_r1_indices[caps->virt_vig_count++] = idx;
> +			}
> +		}
> +		break;
> +	case HFI_PROPERTY_DEVICE_INIT_DMA_INDICES:
> +		if (max_words < 1)
> +			break;
> +		count = payload[0];
> +		if (count > max_words - 1)
> +			count = max_words - 1;
> +		caps->num_dma_pipes = 0;
> +		caps->virt_dma_count = 0;
> +		for (i = 0; i < count; i++) {
> +			u32 idx = payload[1 + i];
> +
> +			if (DCP_REC_ID(idx) == 0) {
> +				if (caps->num_dma_pipes < DCP_MAX_SSPP_COUNT)
> +					caps->dma_pipe_ids[caps->num_dma_pipes++] = idx;
> +			} else {
> +				if (caps->virt_dma_count < DCP_MAX_SSPP_COUNT)
> +					caps->dma_r1_indices[caps->virt_dma_count++] = idx;
> +			}
> +		}
> +		break;

[Severity: High]
Won't resetting both the primary and rect-1 pipe counters here clobber the
previously parsed pipe counts when dcp_hfi_get_capabilities() parses multiple
capability buffers into the same struct dcp_caps?

In drivers/gpu/drm/msm/dcp/hfi/hfi_properties_device.h, both rect-0 pipes
(DCP_REC_ID(idx) == 0) and rect-1 multirect pipes (DCP_REC_ID(idx) != 0) use
HFI_PROPERTY_DEVICE_INIT_VIG_INDICES and HFI_PROPERTY_DEVICE_INIT_DMA_INDICES.

dcp_hfi_get_capabilities() calls dcp_hfi_parse_kv_buf() sequentially on
cache->device_buf, cache->display_buf, cache->vig_buf, cache->dma_buf,
cache->vig_r1_buf, and cache->dma_r1_buf.

When cache->vig_r1_buf and cache->dma_r1_buf are parsed after the primary
pipe buffers, their HFI_PROPERTY_DEVICE_INIT_VIG_INDICES and
HFI_PROPERTY_DEVICE_INIT_DMA_INDICES properties contain only rect-1 indices,
which unconditionally resets caps->num_vig_pipes and caps->num_dma_pipes back
to 0. As a result, caps->max_planes evaluates to 0 and
dcp_hfi_get_capabilities() fails with -EINVAL ("invalid DCP caps: no scanout
pipes"). Conversely, if device_buf contains both rect-0 and rect-1 indices
and vig_buf/dma_buf contain only rect-0 indices, parsing vig_buf/dma_buf
resets caps->virt_vig_count and caps->virt_dma_count to 0.

[ ... ]

> +int dcp_hfi_get_capabilities(struct dcp_hfi *hfi, struct dcp_caps *caps)
> +{
> +	struct dcp_hfi_caps_cache *cache;
> +	int ret = 0, opt_ret;
> +
> +	memset(caps, 0, sizeof(*caps));
> +
> +	mutex_lock(&hfi->caps_cache_lock);
> +	cache = &hfi->caps_cache;
> +
> +	if (!wait_for_completion_timeout(&cache->device_done,
> +					 msecs_to_jiffies(DCP_HFI_TIMEOUT_CAPS_MS))) {
> +		dev_err(hfi->dev,
> +			"DEVICE_INIT_DEVICE_CAPS timeout after %ums\n",
> +			DCP_HFI_TIMEOUT_CAPS_MS);
> +		ret = -ETIMEDOUT;
> +		goto out_free_cache;
> +	}
> +
> +	if (!cache->device_len) {
> +		dev_err(hfi->dev, "DEVICE_INIT_DEVICE_CAPS: empty payload\n");
> +		ret = -ENODATA;
> +		goto out_free_cache;
> +	}
> +
> +	ret = dcp_hfi_parse_kv_buf(hfi, caps, cache->device_buf,
> +				   cache->device_len);
> +	if (ret)
> +		goto out_free_cache;
> +
> +	if (!wait_for_completion_timeout(&cache->display_done,
> +					 msecs_to_jiffies(DCP_HFI_TIMEOUT_CAPS_MS))) {
> +		dev_warn(hfi->dev,
> +			 "DEVICE_INIT_DISPLAY_CAPS not received\n");
> +	} else if (cache->display_len > 0) {
> +		opt_ret = dcp_hfi_parse_kv_buf(hfi, caps, cache->display_buf,
> +					       cache->display_len);
> +		if (opt_ret) {
> +			dev_warn(hfi->dev,
> +				 "DISPLAY_CAPS parse failed: %d (non-fatal)\n",
> +				 opt_ret);
> +		}
> +	}
> +	if (cache->vig_len > 0) {
> +		opt_ret = dcp_hfi_parse_kv_buf(hfi, caps, cache->vig_buf,
> +					       cache->vig_len);
> +		if (opt_ret)
> +			dev_warn(hfi->dev,
> +				 "VIG_CAPS parse failed: %d (non-fatal)\n",
> +				 opt_ret);
> +		else
> +			dcp_hfi_log_pipe_ids(hfi->dev, "VIG_CAPS",
> +					     caps->vig_pipe_ids,
> +					     caps->num_vig_pipes);
> +	}

[ ... ]

> +out_free_cache:
> +	dcp_hfi_caps_cache_stop(hfi);
> +	mutex_unlock(&hfi->caps_cache_lock);
> +	return ret;
> +}

[Severity: High]
Can zeroing hfi->caps_cache in dcp_hfi_caps_cache_stop() corrupt
device_done and display_done while capability events are still in flight, and
does caps_cache access here race with dcp_hfi_dispatch_event()?

struct dcp_hfi_caps_cache embeds the device_done and display_done struct
completion objects. Calling memset(&hfi->caps_cache, 0,
sizeof(hfi->caps_cache)) in dcp_hfi_caps_cache_stop() (and in
dcp_hfi_caps_cache_start() before dcp_hfi_caps_cache_init(), as well as prior
to the first dcp_hfi_send_device_init() call after dcp_hfi_init() posts
response buffers) zeroes wait.lock and sets wait.task_list.next and
wait.task_list.prev to NULL.

In dcp_hfi_get_capabilities(), a timeout on display_done is treated as a
non-fatal warning and execution proceeds to out_free_cache, where
dcp_hfi_caps_cache_stop() zeroes hfi->caps_cache. The same happens if
device_done times out, if dcp_hfi_parse_kv_buf() fails on device_buf, or if
dcp_hfi_send_sync() times out in dcp_hfi_send_device_init().

If HFI_COMMAND_DEVICE_INIT_DEVICE_CAPS or
HFI_COMMAND_DEVICE_INIT_DISPLAY_CAPS then arrives in
dcp_hfi_resp_callback()->dcp_hfi_dispatch_event()->dcp_hfi_store_caps_payload(),
complete() calls swake_up_locked() on the zeroed completion. Because
list_empty(&q->task_list) checks READ_ONCE(head->next) == head, a NULL next
pointer evaluates as non-empty and causes a NULL pointer dereference in
interrupt context (along with operating on a zeroed raw_spinlock_t).

In addition, dcp_hfi_dispatch_event() in dcp_hfi_core.c protects writes to
hfi->caps_cache with spin_lock_irqsave(&hfi->caps_lock, flags), whereas
dcp_hfi_caps_cache_start(), dcp_hfi_caps_cache_stop(), and
dcp_hfi_get_capabilities() only hold the mutex hfi->caps_cache_lock and never
acquire hfi->caps_lock. Because VIG_CAPS, DMA_CAPS, VIG_R1_CAPS, and
DMA_R1_CAPS have no completion wait in dcp_hfi_get_capabilities(), reading
cache->vig_buf, cache->dma_buf, cache->vig_r1_buf, and cache->dma_r1_buf (and
zeroing hfi->caps_cache at out_free_cache) can also race concurrently with
dcp_hfi_store_caps_payload() on another CPU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=9