Re: [PATCH 03/13] drm/msm/dcp: Add HFI transport over VirtIO

[Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>]

On 15-10-2026 18:06, Dmitry Baryshkov wrote:
> 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 <jayant.shekhar@oss.qualcomm.com>
>> Assisted-by: LLM
> You haven't read Documentation/process/. Sad.


Will move the assisted by above signed off.


>
>> 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?
>

Unused headers to be removed in v2.


>> +
>> +#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?


Will be removed


>
>> +
>> +#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?


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?


to be addressed in v2


>
>> +
>> +	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;
> ??
>

All above to be addressed in v2.


>> +};
>> +
>> +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?
>

Will be removed in v2.


>> +	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.
>

This indentation was not caught by b4 prep --check

Anyways, will address across files.


>> +#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().
>
>> +	


There is no drm_dev_get() call in this path. HFI command paths allocate 
buffers directly and use shutdown_in_progress to reject new work during 
teardown.
dcp_hfi_stop() sets shutdown_in_progress while holding cmd_lock. 
dcp_hfi_send_cmd_async() performs the final shutdown check under the 
same lock before adding the buffer to the VirtIO queue. Therefore, if 
allocation races with teardown, the buffer is returned and cannot be 
submitted after shutdown begins.


>> 	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.


Above comments to be addressed in v2.