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 <[email protected]> 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/[email protected]?part=9
