Re: [PATCH v7 3/7] media: platform: amd: Add isp4 fw and hw interface
From: Du, Bin
Date: Wed Jan 07 2026 - 03:44:24 EST
Thank you, Sakari for the feedback.
On 12/22/2025 5:37 PM, Sakari Ailus wrote:
Hi Bin,
On Tue, Dec 16, 2025 at 05:13:22PM +0800, Bin Du wrote:
ISP firmware controls ISP HW pipeline using dedicated embedded processor
called ccpu. The communication between ISP FW and driver is using commands
and response messages sent through the ring buffer. Command buffers support
either global setting that is not specific to the steam and support stream
specific parameters. Response buffers contain ISP FW notification
information such as frame buffer done and command done. IRQ is used for
receiving response buffer from ISP firmware, which is handled in the main
isp4 media device. ISP ccpu is booted up through the firmware loading
helper function prior to stream start. Memory used for command buffer and
response buffer needs to be allocated from amdgpu buffer manager because
isp4 is a child device of amdgpu.
Co-developed-by: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>
Signed-off-by: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>
Co-developed-by: Svetoslav Stoilov <Svetoslav.Stoilov@xxxxxxx>
Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@xxxxxxx>
Signed-off-by: Bin Du <Bin.Du@xxxxxxx>
Tested-by: Alexey Zagorodnikov <xglooom@xxxxxxxxx>
---
MAINTAINERS | 3 +
drivers/media/platform/amd/isp4/Makefile | 3 +-
.../platform/amd/isp4/isp4_fw_cmd_resp.h | 314 +++++++
.../media/platform/amd/isp4/isp4_interface.c | 786 ++++++++++++++++++
.../media/platform/amd/isp4/isp4_interface.h | 141 ++++
5 files changed, 1246 insertions(+), 1 deletion(-)
create mode 100644 drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h
create mode 100644 drivers/media/platform/amd/isp4/isp4_interface.c
create mode 100644 drivers/media/platform/amd/isp4/isp4_interface.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 7aa17c7e71d6..cccae369c876 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1145,7 +1145,10 @@ F: drivers/media/platform/amd/isp4/Kconfig
F: drivers/media/platform/amd/isp4/Makefile
F: drivers/media/platform/amd/isp4/isp4.c
F: drivers/media/platform/amd/isp4/isp4.h
+F: drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h
F: drivers/media/platform/amd/isp4/isp4_hw_reg.h
+F: drivers/media/platform/amd/isp4/isp4_interface.c
+F: drivers/media/platform/amd/isp4/isp4_interface.h
AMD KFD
M: Felix Kuehling <Felix.Kuehling@xxxxxxx>
diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/media/platform/amd/isp4/Makefile
index de0092dad26f..a2a5bf98e912 100644
--- a/drivers/media/platform/amd/isp4/Makefile
+++ b/drivers/media/platform/amd/isp4/Makefile
@@ -3,4 +3,5 @@
# Copyright (C) 2025 Advanced Micro Devices, Inc.
obj-$(CONFIG_AMD_ISP4) += amd_capture.o
-amd_capture-objs := isp4.o
+amd_capture-objs := isp4.o \
+ isp4_interface.o
diff --git a/drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h b/drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h
new file mode 100644
index 000000000000..d571b3873edb
--- /dev/null
+++ b/drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h
@@ -0,0 +1,314 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#ifndef _ISP4_CMD_RESP_H_
+#define _ISP4_CMD_RESP_H_
+
+/*
+ * @brief Host and Firmware command & response channel.
+ * Two types of command/response channel.
+ * Type Global Command has one command/response channel.
+ * Type Stream Command has one command/response channel.
+ *----------- ------------
+ *| | --------------------------- | |
+ *| | ---->| Global Command |----> | |
+ *| | --------------------------- | |
+ *| | | |
+ *| | | |
+ *| | --------------------------- | |
+ *| | ---->| Stream Command |----> | |
+ *| | --------------------------- | |
+ *| | | |
+ *| | | |
+ *| | | |
+ *| HOST | | Firmware |
+ *| | | |
+ *| | | |
+ *| | -------------------------- | |
+ *| | <----| Global Response |<---- | |
+ *| | -------------------------- | |
+ *| | | |
+ *| | | |
+ *| | -------------------------- | |
+ *| | <----| Stream Response |<---- | |
+ *| | -------------------------- | |
+ *| | | |
+ *| | | |
+ *----------- ------------
+ */
+
+/*
+ * @brief command ID format
+ * cmd_id is in the format of following type:
+ * type: indicate command type, global/stream commands.
+ * group: indicate the command group.
+ * id: A unique command identification in one type and group.
+ * |<-Bit31 ~ Bit24->|<-Bit23 ~ Bit16->|<-Bit15 ~ Bit0->|
+ * | type | group | id |
+ */
+
+#define CMD_TYPE_SHIFT 24
+#define CMD_GROUP_SHIFT 16
+#define CMD_TYPE_STREAM_CTRL (0x2U << CMD_TYPE_SHIFT)
+
+#define CMD_GROUP_STREAM_CTRL (0x1U << CMD_GROUP_SHIFT)
+#define CMD_GROUP_STREAM_BUFFER (0x4U << CMD_GROUP_SHIFT)
+
+/* Stream Command */
+#define CMD_ID_SET_STREAM_CONFIG (CMD_TYPE_STREAM_CTRL | CMD_GROUP_STREAM_CTRL | 0x1)
+#define CMD_ID_SET_OUT_CHAN_PROP (CMD_TYPE_STREAM_CTRL | CMD_GROUP_STREAM_CTRL | 0x3)
+#define CMD_ID_ENABLE_OUT_CHAN (CMD_TYPE_STREAM_CTRL | CMD_GROUP_STREAM_CTRL | 0x5)
+#define CMD_ID_START_STREAM (CMD_TYPE_STREAM_CTRL | CMD_GROUP_STREAM_CTRL | 0x7)
+#define CMD_ID_STOP_STREAM (CMD_TYPE_STREAM_CTRL | CMD_GROUP_STREAM_CTRL | 0x8)
+
+/* Stream Buffer Command */
+#define CMD_ID_SEND_BUFFER (CMD_TYPE_STREAM_CTRL | CMD_GROUP_STREAM_BUFFER | 0x1)
+
+/*
+ * @brief response ID format
+ * resp_id is in the format of following type:
+ * type: indicate command type, global/stream commands.
+ * group: indicate the command group.
+ * id: A unique command identification in one type and group.
+ * |<-Bit31 ~ Bit24->|<-Bit23 ~ Bit16->|<-Bit15 ~ Bit0->|
+ * | type | group | id |
+ */
+
+#define RESP_GROUP_SHIFT 16
+#define RESP_GROUP_MASK (0xff << RESP_GROUP_SHIFT)
+
+#define GET_RESP_GROUP_VALUE(resp_id) (((resp_id) & RESP_GROUP_MASK) >> RESP_GROUP_SHIFT)
+#define GET_RESP_ID_VALUE(resp_id) ((resp_id) & 0xffff)
+
+#define RESP_GROUP_GENERAL (0x1 << RESP_GROUP_SHIFT)
+#define RESP_GROUP_NOTIFICATION (0x3 << RESP_GROUP_SHIFT)
+
+/* General Response */
+#define RESP_ID_CMD_DONE (RESP_GROUP_GENERAL | 0x1)
+
+/* Notification */
+#define RESP_ID_NOTI_FRAME_DONE (RESP_GROUP_NOTIFICATION | 0x1)
+
+#define CMD_STATUS_SUCCESS 0
+#define CMD_STATUS_FAIL 1
+#define CMD_STATUS_SKIPPED 2
+
+#define ADDR_SPACE_TYPE_GPU_VA 4
+
+#define FW_MEMORY_POOL_SIZE (100 * 1024 * 1024)
+
+/*
+ * standard ISP mipicsi=>isp
+ */
+#define MIPI0_ISP_PIPELINE_ID 0x5f91
+
+enum isp4fw_sensor_id {
+ SENSOR_ID_ON_MIPI0 = 0, /* Sensor id for ISP input from MIPI port 0 */
+};
+
+enum isp4fw_stream_id {
+ STREAM_ID_INVALID = -1, /* STREAM_ID_INVALID. */
+ STREAM_ID_1 = 0, /* STREAM_ID_1. */
+ STREAM_ID_2 = 1, /* STREAM_ID_2. */
+ STREAM_ID_3 = 2, /* STREAM_ID_3. */
+ STREAM_ID_MAXIMUM /* STREAM_ID_MAXIMUM. */
+};
+
+enum isp4fw_image_format {
+ IMAGE_FORMAT_NV12 = 1, /* 4:2:0,semi-planar, 8-bit */
+ IMAGE_FORMAT_YUV422INTERLEAVED = 7, /* interleave, 4:2:2, 8-bit */
+};
+
+enum isp4fw_pipe_out_ch {
+ ISP_PIPE_OUT_CH_PREVIEW = 0,
+};
+
+enum isp4fw_yuv_range {
+ ISP_YUV_RANGE_FULL = 0, /* YUV value range in 0~255 */
+ ISP_YUV_RANGE_NARROW = 1, /* YUV value range in 16~235 */
+ ISP_YUV_RANGE_MAX
+};
+
+enum isp4fw_buffer_type {
+ BUFFER_TYPE_PREVIEW = 8,
+ BUFFER_TYPE_META_INFO = 10,
+ BUFFER_TYPE_MEM_POOL = 15,
+};
+
+enum isp4fw_buffer_status {
+ BUFFER_STATUS_INVALID, /* The buffer is INVALID */
+ BUFFER_STATUS_SKIPPED, /* The buffer is not filled with image data */
+ BUFFER_STATUS_EXIST, /* The buffer is exist and waiting for filled */
+ BUFFER_STATUS_DONE, /* The buffer is filled with image data */
+ BUFFER_STATUS_LACK, /* The buffer is unavailable */
+ BUFFER_STATUS_DIRTY, /* The buffer is dirty, probably caused by
+ * LMI leakage
+ */
+ BUFFER_STATUS_MAX /* The buffer STATUS_MAX */
+};
+
+enum isp4fw_buffer_source {
+ /* The buffer is from the stream buffer queue */
+ BUFFER_SOURCE_STREAM,
+};
Could you also use the ISP4 (or ISP4IF) prefix for these, please? Many look
rather generic.
Thank you for highlighting this matter, since these definitions are located in isp4_fw_cmd_resp.h, ISP4_FW may be a more appropriate prefix. Just to confirm: are you suggesting that we should add this prefix to all macros and enums? For example, changing CMD_ID_SET_STREAM_CONFIG to ISP4_FW_CMD_ID_SET_STREAM_CONFIG, and BUFFER_SOURCE_STREAM to ISP4_FW _BUFFER_SOURCE_STREAM? Our initial thought was that these would only be used within ISP and shouldn't lead to any confusion.
...
diff --git a/drivers/media/platform/amd/isp4/isp4_interface.h b/drivers/media/platform/amd/isp4/isp4_interface.h
new file mode 100644
index 000000000000..01d5268f7d4c
--- /dev/null
+++ b/drivers/media/platform/amd/isp4/isp4_interface.h
@@ -0,0 +1,141 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#ifndef _ISP4_INTERFACE_H_
+#define _ISP4_INTERFACE_H_
+
+#include <drm/amd/isp.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+struct isp4fw_resp;
+
+#define ISP4IF_RB_MAX 25
+#define ISP4IF_RESP_CHAN_TO_RB_OFFSET 9
+#define ISP4IF_RB_PMBMAP_MEM_SIZE (SZ_16M - 1)
+#define ISP4IF_RB_PMBMAP_MEM_CHUNK \
+ (ISP4IF_RB_PMBMAP_MEM_SIZE / (ISP4IF_RB_MAX - 1))
+#define ISP4IF_HOST2FW_COMMAND_SIZE sizeof(struct isp4fw_cmd)
+#define ISP4IF_MAX_NUM_HOST2FW_COMMAND 40
+#define ISP4IF_FW_CMD_BUF_SIZE \
+ (ISP4IF_MAX_NUM_HOST2FW_COMMAND * ISP4IF_HOST2FW_COMMAND_SIZE)
+#define ISP4IF_RB_FULL_SLEEP_US (33 * USEC_PER_MSEC)
+#define ISP4IF_RB_FULL_TIMEOUT_US (10 * ISP4IF_RB_FULL_SLEEP_US)
+
+#define ISP4IF_META_INFO_BUF_SIZE ALIGN(sizeof(struct isp4fw_meta_info), 0x8000)
+#define ISP4IF_MAX_STREAM_BUF_COUNT 8
+
+#define ISP4IF_FW_LOG_RINGBUF_SIZE SZ_2M
+
+enum isp4if_stream_id {
+ ISP4IF_STREAM_ID_GLOBAL = 0,
+ ISP4IF_STREAM_ID_1 = 1,
+ ISP4IF_STREAM_ID_MAX = 4
+};
+
+enum isp4if_status {
+ ISP4IF_STATUS_PWR_OFF,
+ ISP4IF_STATUS_PWR_ON,
+ ISP4IF_STATUS_FW_RUNNING,
+ ISP4IF_FSM_STATUS_MAX
+};
+
+struct isp4if_gpu_mem_info {
+ u64 mem_size;
+ u64 gpu_mc_addr;
+ void *sys_addr;
+ void *mem_handle;
+};
+
+struct isp4if_img_buf_info {
+ struct {
+ void *sys_addr;
+ u64 mc_addr;
+ u32 len;
+ } planes[3];
+};
+
+struct isp4if_img_buf_node {
+ struct list_head node;
+ struct isp4if_img_buf_info buf_info;
+};
+
+struct isp4if_cmd_element {
+ struct list_head list;
+ u32 seq_num;
+ u32 cmd_id;
+ struct completion cmd_done;
+ atomic_t refcnt;
+};
+
+struct isp4_interface {
How about just "isp4if"? Up to you.
If that's the case, do you mind if i keep it, so i can save a lot of conflict and alignment fix efforts in the subsequent patches in the set, LOL
+ struct device *dev;
+ void __iomem *mmio;
+
+ spinlock_t cmdq_lock; /* used for cmdq access */
+ spinlock_t bufq_lock; /* used for bufq access */
+ struct mutex isp4if_mutex; /* used to send fw cmd and read fw log */
+
+ struct list_head cmdq; /* commands sent to fw */
+ struct list_head bufq; /* buffers sent to fw */
+
+ enum isp4if_status status;
+ u32 host2fw_seq_num;
+
+ /* ISP fw buffers */
+ struct isp4if_gpu_mem_info *fw_log_buf;
+ struct isp4if_gpu_mem_info *fw_cmd_resp_buf;
+ struct isp4if_gpu_mem_info *fw_mem_pool;
+ struct isp4if_gpu_mem_info *meta_info_buf[ISP4IF_MAX_STREAM_BUF_COUNT];
+};
+
+static inline void isp4if_split_addr64(u64 addr, u32 *lo, u32 *hi)
+{
+ if (lo)
+ *lo = addr & 0xffffffff;
+
+ if (hi)
+ *hi = addr >> 32;
+}
+
+static inline u64 isp4if_join_addr64(u32 lo, u32 hi)
+{
+ return (((u64)hi) << 32) | (u64)lo;
+}
+
+int isp4if_f2h_resp(struct isp4_interface *ispif, enum isp4if_stream_id stream,
+ struct isp4fw_resp *resp);
+
+int isp4if_send_command(struct isp4_interface *ispif, u32 cmd_id, const void *package,
+ u32 package_size);
+
+int isp4if_send_command_sync(struct isp4_interface *ispif, u32 cmd_id, const void *package,
+ u32 package_size);
+
+struct isp4if_cmd_element *isp4if_rm_cmd_from_cmdq(struct isp4_interface *ispif, u32 seq_num,
+ u32 cmd_id);
+
+void isp4if_clear_cmdq(struct isp4_interface *ispif);
+
+void isp4if_clear_bufq(struct isp4_interface *ispif);
+
+void isp4if_dealloc_buffer_node(struct isp4if_img_buf_node *buf_node);
+
+struct isp4if_img_buf_node *isp4if_alloc_buffer_node(struct isp4if_img_buf_info *buf_info);
+
+struct isp4if_img_buf_node *isp4if_dequeue_buffer(struct isp4_interface *ispif);
+
+int isp4if_queue_buffer(struct isp4_interface *ispif, struct isp4if_img_buf_node *buf_node);
+
+int isp4if_stop(struct isp4_interface *ispif);
+
+int isp4if_start(struct isp4_interface *ispif);
+
+int isp4if_deinit(struct isp4_interface *ispif);
+
+int isp4if_init(struct isp4_interface *ispif, struct device *dev, void __iomem *isp_mmio);
Could you run
$ ./scripts/checkpatch.pl --strict --max-line-length=80
on the set, please?
Oh, I will change --max-line-length in our checkpatch.pl from 100 to 80 and fix all the violations. I used to think that the requirement has been relaxed to 100 columns based on https://www.phoronix.com/news/Linux-Kernel-Deprecates-80-Col
+
+#endif /* _ISP4_INTERFACE_H_ */
--
Regards,
Bin