| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <92ccb38a-5c62-4730-8a41-fa80d2f523a7@amd.com>
Date: Wed, 7 Jan 2026 16:44:11 +0800
From: "Du, Bin" <bin.du@....com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
laurent.pinchart+renesas@...asonboard.com, bryan.odonoghue@...aro.org,
prabhakar.mahadev-lad.rj@...renesas.com, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, sultan@...neltoast.com,
pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com,
mario.limonciello@....com, richard.gong@....com, anson.tsao@....com,
Svetoslav Stoilov <Svetoslav.Stoilov@....com>,
Alexey Zagorodnikov <xglooom@...il.com>
Subject: Re: [PATCH v7 3/7] media: platform: amd: Add isp4 fw and hw interface
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@...neltoast.com>
>> Signed-off-by: Sultan Alsawaf <sultan@...neltoast.com>
>> Co-developed-by: Svetoslav Stoilov <Svetoslav.Stoilov@....com>
>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@....com>
>> Signed-off-by: Bin Du <Bin.Du@....com>
>> Tested-by: Alexey Zagorodnikov <xglooom@...il.com>
>> ---
>> 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@....com>
>> 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
Powered by blists - more mailing lists