On Thu, Oct 15, 2026 at 01:08:23PM +0530, Jayant Shekhar wrote:
> DCP firmware owns the display hardware, so the MSM DRM driver needs a
> transport for HFI commands, responses, and asynchronous display events.
>
> Add the VirtIO command and response queues used to communicate with DCP.
> Allocate command, response, and capability buffers through the DMA API
> using the firmware-visible IOVA window. Track synchronous commands and
> asynchronous responses, dispatch HFI callbacks, and synchronize queue
> teardown with in-flight transactions.
>
> Signed-off-by: Jayant Shekhar <[email protected]>
> Assisted-by: LLM
You haven't read Documentation/process/. Sad.
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_drv.h
> b/drivers/gpu/drm/msm/dcp/dcp_drv.h
> new file mode 100644
> index 000000000000..bf6c34af43a0
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_drv.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#ifndef __DCP_DRV_H__
> +#define __DCP_DRV_H__
> +
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/notifier.h>
> +#include <linux/platform_device.h>
> +#include <linux/virtio.h>
Are all these headers required for this header?
> +
> +#include "dcp_hfi.h"
> +
> +struct dcp_dpu_kms;
> +
> +struct dcp_drv {
> + struct virtio_device *vdev;
> + struct platform_device *dcp_subsystem_dev;
> + struct device_dma_parameters dcp_subsystem_dma_parms;
> + struct device_node *dcp_subsystem_node;
> + struct dcp_hfi *hfi;
> + struct dcp_dpu_kms *kms;
> + struct dcp_caps caps;
> + struct notifier_block panic_notifier;
> +};
> +
> +#endif /* __DCP_DRV_H__ */
> +
> +#define DCP_MAX_DISPLAYS 4
> +
> +#define DCP_FEAT_DSC BIT(0)
> +#define DCP_FEAT_HDR BIT(1)
> +#define DCP_FEAT_SRC_SPLIT BIT(3)
> +#define DCP_FEAT_CURSOR BIT(4)
What kind of features are those?
> +
> +#define DCP_MAX_SSPP_COUNT 16
> +#define DCP_MAX_VIG_PIPES DCP_MAX_SSPP_COUNT
> +#define DCP_MAX_DMA_PIPES DCP_MAX_SSPP_COUNT
> +
> +#define DCP_MAX_BLOCKS 8
> +
> +/* Pipe indices encode the rectangle ID in bits 31:24. */
> +#define DCP_REC_ID(x) (((x) & 0xff000000) >> 24)
> +
> +/* Capabilities parsed from DEVICE_INIT responses. */
> +struct dcp_caps {
> + u32 dcp_hw_rev;
> + u32 hw_rev;
> + u32 fw_rev;
> +
> + u32 num_vig_pipes;
> + u32 vig_pipe_ids[DCP_MAX_SSPP_COUNT];
> + u32 virt_vig_count;
> + u32 vig_r1_indices[DCP_MAX_SSPP_COUNT];
> +
> + u32 num_dma_pipes;
> + u32 dma_pipe_ids[DCP_MAX_SSPP_COUNT];
> + u32 virt_dma_count;
> + u32 dma_r1_indices[DCP_MAX_SSPP_COUNT];
> +
> + u32 num_crtcs;
CRTCs? Or mixers?
> +
> + u32 dsi_count;
> + u32 dsi_indices[DCP_MAX_BLOCKS];
> + u32 max_dsi_resolution;
If these are parsed, why is it still u32 rather than two resolutions?
> +
> + u32 dp_count;
> + u32 dp_indices[DCP_MAX_BLOCKS];
Unused, drop
> +
> + u32 active_pipes_mask[DCP_MAX_DISPLAYS];
Unused. Drop all unused fields.
> +
> + u32 max_width;
> + u32 max_height;
max width/height of... what?
> + u32 max_planes;
> + u32 features;
> + bool valid;
How can it be invalid?
> +
> + bool dyn_clk_support;
??
> +};
> +
> +struct dcp_hfi_buf {
> + void *data;
> + dma_addr_t dma_addr;
> + size_t size;
> + struct scatterlist sg;
> + struct dcp_hfi_completion *comp;
> + struct list_head node;
> +};
> +
> +struct dcp_hfi_completion {
> + struct completion done;
> + int status;
> + void *resp_data;
> + size_t resp_len;
> + u32 packet_id;
> +};
> +
> +#define DCP_HFI_RESOURCE_VOTE_BUF_SIZE 256
> +
> +/* Wire-format resource vote entry. */
> +struct dcp_resource_vote_pair {
> + u32 key;
> + u64 value;
> +};
> +
> +struct dcp_resource_vote_wire {
> + __le32 key;
> + __le64 value;
> +} __packed;
> +
> +struct dcp_hfi {
> + struct device *dev;
> + struct dcp_drv *drv;
> +
> + struct device *dma_dev;
> +
> + struct virtqueue *cmd_vq;
> + struct virtqueue *resp_vq;
> +
> + struct dcp_hfi_buf *cmd_pool;
> + struct list_head cmd_free_list;
> + /* Protects cmd_free_list and command buffer ownership. */
> + spinlock_t cmd_lock;
> +
> + struct dcp_hfi_buf *resp_pool;
> + /* Protects response queue draining and power callback publication. */
> + spinlock_t resp_drain_lock;
> + struct list_head resp_repost_list;
> + struct delayed_work resp_repost_work;
> + struct xarray pending_xa;
> + /* Protects pending_xa. */
> + spinlock_t pending_lock;
> + atomic_t packet_id;
> + atomic_t shutdown_in_progress;
This all demands at least some description or kernel docs.
> +
> + struct work_struct resource_vote_work;
> + /* Protects resource_vote_buf, resource_vote_len, and callback state. */
> + spinlock_t resource_vote_lock;
> + u8 resource_vote_buf[DCP_HFI_RESOURCE_VOTE_BUF_SIZE];
> + u32 resource_vote_len;
> + u32 resource_vote_generation;
> + void (*resource_vote_cb)(struct dcp_drv *drv,
> + const struct dcp_resource_vote_pair *pairs,
> + u32 count);
> +
> + /* Protects caps_cache pointer and its lifetime. */
> + spinlock_t caps_lock;
> + /* Serializes capability transactions and cache detach/free. */
> + struct mutex caps_cache_lock;
> + struct dcp_hfi_caps_cache *caps_cache;
What's the point of keeping it as a pointer? Just embed it here and drop
the locks. Read the caps while the devices is not visible so that there
are no concurrent users.
> +
> + void (*hotplug_cb)(struct dcp_drv *drv, u32 intf_idx, bool connected);
Why do we need these callbacks? Can you call the functions directly?
> + void (*error_cb)(struct dcp_drv *drv, u32 display_id, u32 error_code);
> +
> + void *power_control_priv;
> + void (*power_control_cb)(void *priv, u32 display_id,
> + u32 power_mask, bool enable, u32 packet_id);
> +};
> +
> +int dcp_hfi_init(struct dcp_drv *drv, struct virtio_device *vdev,
> + struct virtqueue *cmd_vq, struct virtqueue *resp_vq,
> + struct device *dma_dev);
> +void dcp_hfi_set_power_control_cb(struct dcp_hfi *hfi, void *priv,
> + void (*cb)(void *priv, u32 display_id,
> + u32 power_mask, bool enable,
> + u32 packet_id));
> +void dcp_hfi_clear_power_control_cb(struct dcp_hfi *hfi, void *priv);
> +void dcp_hfi_stop(struct dcp_hfi *hfi);
> +void dcp_hfi_quiesce_queues(struct dcp_hfi *hfi);
> +void dcp_hfi_deinit(struct dcp_hfi *hfi);
> +
> +void dcp_hfi_resp_callback(struct virtqueue *vq);
> +void dcp_hfi_cmd_callback(struct virtqueue *vq);
> +
> +int dcp_hfi_send_sync(struct dcp_hfi *hfi, u32 cmd,
> + enum hfi_cmd_buff_type cmd_type, u32 object_id,
> + enum hfi_packet_payload_type payload_type, void *payload,
> + u32 payload_size, u32 tx_flags, void *resp_data,
> + size_t resp_len, unsigned int timeout_ms);
> +
> +int dcp_hfi_send_no_wait(struct dcp_hfi *hfi, u32 cmd,
> + enum hfi_cmd_buff_type cmd_type, u32 object_id,
> + enum hfi_packet_payload_type payload_type, void
> *payload,
> + u32 payload_size);
> +
> +int dcp_hfi_send_no_wait_flags(struct dcp_hfi *hfi, u32 cmd,
> + enum hfi_cmd_buff_type cmd_type, u32 object_id,
> + enum hfi_packet_payload_type payload_type,
> + void *payload, u32 payload_size, u32 tx_flags);
> +int dcp_hfi_send_no_wait_atomic_flags(struct dcp_hfi *hfi, u32 cmd,
> + enum hfi_cmd_buff_type cmd_type, u32
> object_id,
> + enum hfi_packet_payload_type
> payload_type,
> + void *payload, u32 payload_size, u32
> tx_flags);
You haven't really followed the internal review feedback. You have been
asked to review the indentation. You didnt.
> +#endif /* __DCP_HFI_H__ */
> diff --git a/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c
> b/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c
> new file mode 100644
> index 000000000000..bf27cbf9b8a4
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_hfi_core.c
> @@ -0,0 +1,1198 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * DCP HFI core transport.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/unaligned.h>
> +#include <linux/slab.h>
> +#include <linux/scatterlist.h>
> +#include <linux/spinlock.h>
> +#include <linux/virtio.h>
> +#include <linux/workqueue.h>
> +#include <uapi/linux/virtio_mmio.h>
> +
> +#include "dcp_hfi.h"
> +#include "dcp_hfi_priv.h"
> +#include "dcp_drv.h"
> +#include "hfi/hfi_packet.h"
> +#include "hfi/hfi_packer.h"
> +#include "hfi/hfi_unpacker.h"
> +#include "hfi/hfi_commands_device.h"
> +#include "hfi/hfi_commands_display.h"
> +#include "hfi/hfi_commands_display_events.h"
> +#include "hfi/hfi_defs_common.h"
> +#include "hfi/hfi_defs_display.h"
> +
> +static void dcp_hfi_resource_vote_work(struct work_struct *work);
> +static int dcp_hfi_prepost_resp_bufs(struct dcp_hfi *hfi);
> +
> +u32 dcp_hfi_next_packet_id(struct dcp_hfi *hfi)
> +{
> + u32 id;
> +
> + id = (u32)atomic_inc_return(&hfi->packet_id);
> + if (id == 0)
> + id = (u32)atomic_inc_return(&hfi->packet_id);
What if something looks at your counter inbetween? Two atomic operations
aren't atomic.
> + return id;
> +}
> +
> +#define DCP_HFI_BUF_ALLOC_RETRIES 3 /* Bound command-path stalls. */
> +#define DCP_HFI_BUF_ALLOC_SLEEP_US_MIN 30000
> +#define DCP_HFI_BUF_ALLOC_SLEEP_US_MAX 40000
> +
> +static struct dcp_hfi_buf *_dcp_hfi_buf_alloc(struct dcp_hfi *hfi,
> + struct list_head *free_list,
> + spinlock_t *lock,
> + const char *pool_name)
> +{
> + struct dcp_hfi_buf *buf = NULL;
> + unsigned long flags;
> + int retry;
> +
> + if (atomic_read(&hfi->shutdown_in_progress)) {
This is called drm_dev_get().
> + dev_dbg(hfi->dev,
> + "HFI shutdown in progress — refusing %s buffer
> allocation\n",
> + pool_name);
> + return NULL;
> + }
> +
> + for (retry = 0; retry <= DCP_HFI_BUF_ALLOC_RETRIES; retry++) {
> + if (atomic_read(&hfi->shutdown_in_progress))
> + return NULL;
> +
> + spin_lock_irqsave(lock, flags);
> + if (!list_empty(free_list)) {
> + buf = list_first_entry(free_list,
> + struct dcp_hfi_buf, node);
> + list_del(&buf->node);
> + }
> + spin_unlock_irqrestore(lock, flags);
> +
> + if (buf)
> + return buf;
> +
> + if (retry < DCP_HFI_BUF_ALLOC_RETRIES)
> + usleep_range(DCP_HFI_BUF_ALLOC_SLEEP_US_MIN,
> + DCP_HFI_BUF_ALLOC_SLEEP_US_MAX);
> + }
> +
> + dev_warn_ratelimited(hfi->dev,
> + "HFI %s TX pool exhausted after %d retries —
> command dropped\n",
> + pool_name, DCP_HFI_BUF_ALLOC_RETRIES);
> + return NULL;
> +}
> +
> +static void _dcp_hfi_buf_free(struct dcp_hfi *hfi, struct dcp_hfi_buf *buf,
> + struct list_head *free_list, spinlock_t *lock)
> +{
> + unsigned long flags;
> +
> + buf->comp = NULL;
> + spin_lock_irqsave(lock, flags);
> + list_add_tail(&buf->node, free_list);
> + spin_unlock_irqrestore(lock, flags);
Just call _dcp_hfi_buf_free_locked() under the lock.
> +}
> +
> +static void _dcp_hfi_buf_free_locked(struct dcp_hfi_buf *buf,
> + struct list_head *free_list)
> +{
> + buf->comp = NULL;
> + list_add_tail(&buf->node, free_list);
> +}
> +
[..]
> +
> + case HFI_COMMAND_DISPLAY_EVENT_HPD_STATUS: {
> + u32 intf_idx = pkt_info->id & 0xFFFF;
> + bool connected = false;
> +
> + if (pkt_info->payload_ptr &&
> + pkt_info->payload_size >=
> + sizeof(struct hfi_display_hpd_status)) {
> + struct hfi_display_hpd_status *hpd =
> + pkt_info->payload_ptr;
> + connected = (hpd->dp_state ==
> + HFI_DP_STATE_CONNECTED);
> + }
> + if (hfi->hotplug_cb)
> + hfi->hotplug_cb(hfi->drv, intf_idx, connected);
> + return true;
> + }
> +
> + case HFI_COMMAND_DISPLAY_EVENT_PANEL_DEAD:
> + dev_err(hfi->dev,
> + "DCP: panel dead display %u\n",
> + pkt_info->id & 0xFFFF);
> + if (hfi->error_cb)
> + hfi->error_cb(hfi->drv, pkt_info->id & 0xFFFF, 0);
> + return true;
> +
> + case HFI_COMMAND_DISPLAY_EVENT_HW_RECOVERY:
> + dev_err(hfi->dev,
> + "DCP: HW recovery display %u\n",
> + pkt_info->id & 0xFFFF);
> + if (hfi->error_cb)
> + hfi->error_cb(hfi->drv, pkt_info->id & 0xFFFF, 1);
Yep... Drop the callbacks. I think it was also a part of the review.
> + return true;
> +
Can you split the patches? It really becomes troublesome to review.
--
With best wishes
Dmitry
