Re: [PATCH v2 1/8] media: platform: amd: Introduce amd isp4 capture driver

From: Du, Bin
Date: Mon Jul 28 2025 - 05:01:04 EST

Many thanks Askari Alius for your careful review.

On 7/28/2025 1:54 PM, Sakari Ailus wrote:
Hi Bin,

On Wed, Jun 18, 2025 at 05:19:52PM +0800, Bin Du wrote:
Amd isp4 capture is a v4l2 media device which implements media controller
interface.
It has one sub-device (amd ISP4 sub-device) endpoint which can be connected
to a remote CSI2 TX endpoint. It supports only one physical interface for
now.
Also add ISP4 driver related entry info into the MAINAINERS file

You could rewrap the text and use the full lines here -- up to 75
characters per line.

Sure, will do in the next patch>>
Signed-off-by: Bin Du <Bin.Du@xxxxxxx>
Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@xxxxxxx>
---
MAINTAINERS | 10 ++
drivers/media/platform/Kconfig | 1 +
drivers/media/platform/Makefile | 1 +
drivers/media/platform/amd/Kconfig | 17 +++
drivers/media/platform/amd/Makefile | 5 +
drivers/media/platform/amd/isp4/Makefile | 21 ++++
drivers/media/platform/amd/isp4/isp4.c | 139 +++++++++++++++++++++++
drivers/media/platform/amd/isp4/isp4.h | 35 ++++++
8 files changed, 229 insertions(+)
create mode 100644 drivers/media/platform/amd/Kconfig
create mode 100644 drivers/media/platform/amd/Makefile
create mode 100644 drivers/media/platform/amd/isp4/Makefile
create mode 100644 drivers/media/platform/amd/isp4/isp4.c
create mode 100644 drivers/media/platform/amd/isp4/isp4.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 10893c91b1c1..15070afb14b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1107,6 +1107,16 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux.git
F: drivers/iommu/amd/
F: include/linux/amd-iommu.h
+AMD ISP4 DRIVER
+M: Bin Du <bin.du@xxxxxxx>
+M: Nirujogi Pratap <pratap.nirujogi@xxxxxxx>
+L: linux-media@xxxxxxxxxxxxxxx
+S: Maintained
+T: git git://linuxtv.org/media.git
+F: drivers/media/platform/amd/Kconfig
+F: drivers/media/platform/amd/Makefile
+F: drivers/media/platform/amd/isp4/*
+
AMD KFD
M: Felix Kuehling <Felix.Kuehling@xxxxxxx>
L: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 85d2627776b6..d525c2262a7d 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -89,5 +89,6 @@ source "drivers/media/platform/ti/Kconfig"
source "drivers/media/platform/verisilicon/Kconfig"
source "drivers/media/platform/via/Kconfig"
source "drivers/media/platform/xilinx/Kconfig"
+source "drivers/media/platform/amd/Kconfig"
endif # MEDIA_PLATFORM_DRIVERS
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index ace4e34483dd..9f3d1693868d 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -32,6 +32,7 @@ obj-y += ti/
obj-y += verisilicon/
obj-y += via/
obj-y += xilinx/
+obj-y += amd/
# Please place here only ancillary drivers that aren't SoC-specific
# Please keep it alphabetically sorted by Kconfig name
diff --git a/drivers/media/platform/amd/Kconfig b/drivers/media/platform/amd/Kconfig
new file mode 100644
index 000000000000..3b1dba0400a0
--- /dev/null
+++ b/drivers/media/platform/amd/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: MIT
+
+config AMD_ISP4
+ tristate "AMD ISP4 and camera driver"
+ default y
+ depends on VIDEO_DEV && VIDEO_V4L2_SUBDEV_API
+ select VIDEOBUF2_CORE
+ select VIDEOBUF2_V4L2
+ select VIDEOBUF2_MEMOPS
+ select VIDEOBUF2_VMALLOC
+ select VIDEOBUF2_DMA_CONTIG
+ select VIDEOBUF2_DMA_SG

Do you need all these three? Most drivers need only one.

After double check, yes, none of theam are used in our code, will remove them and also the header files included>> + help
+ This is support for AMD ISP4 and camera subsystem driver.
+ Say Y here to enable the ISP4 and camera device for video capture.
+ To compile this driver as a module, choose M here. The module will
+ be called amd_capture.
diff --git a/drivers/media/platform/amd/Makefile b/drivers/media/platform/amd/Makefile
new file mode 100644
index 000000000000..76146efcd2bf
--- /dev/null
+++ b/drivers/media/platform/amd/Makefile
@@ -0,0 +1,5 @@
+# Copyright 2024 Advanced Micro Devices, Inc.
+# add isp block
+ifneq ($(CONFIG_AMD_ISP4),)
+obj-y += isp4/
+endif
diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/media/platform/amd/isp4/Makefile
new file mode 100644
index 000000000000..e9e84160517d
--- /dev/null
+++ b/drivers/media/platform/amd/isp4/Makefile
@@ -0,0 +1,21 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (C) 2025 Advanced Micro Devices, Inc.
+
+obj-$(CONFIG_AMD_ISP4) += amd_capture.o
+amd_capture-objs := isp4.o
+
+ccflags-y += -I$(srctree)/drivers/media/platform/amd/isp4
+ccflags-y += -I$(srctree)/include
+
+ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
+ cc_stack_align := -mpreferred-stack-boundary=4
+endif

Uh... does the driver actually depend on this?

Yes, other comments also mentioned this, will update in the next patch>> +
+ccflags-y += $(cc_stack_align)
+ccflags-y += -DCONFIG_COMPAT
+ccflags-y += -Wunused-but-set-variable
+ccflags-y += -Wmissing-include-dirs
+ccflags-y += -Wunused-const-variable
+ccflags-y += -Wmaybe-uninitialized
+ccflags-y += -Wunused-value
diff --git a/drivers/media/platform/amd/isp4/isp4.c b/drivers/media/platform/amd/isp4/isp4.c
new file mode 100644
index 000000000000..d0be90c5ec3b
--- /dev/null
+++ b/drivers/media/platform/amd/isp4/isp4.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/pm_runtime.h>
+#include <linux/vmalloc.h>
+#include <media/v4l2-ioctl.h>

Alphabetic order, please.

Sorry, seems it's already in alphabetic order, an example in drivers/media/v4l2-core/v4l2-ioctl.c is like
#include <linux/v4l2-subdev.h>
#include <linux/videodev2.h>

#include <media/media-device.h> /* for media_set_bus_info() */
#include <media/v4l2-common.h>
Suppose i can add an empty line before #include <media/v4l2-ioctl.h>>> +
+#include "isp4.h"
+
+#define VIDEO_BUF_NUM 5

Unused.

Yes, other comments also mentioned this, will remove it in the next patch>> +
+#define ISP4_DRV_NAME "amd_isp_capture"
+
+/* interrupt num */
+static const u32 isp4_ringbuf_interrupt_num[] = {
+ 0, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT9 */
+ 1, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT10 */
+ 3, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT11 */
+ 4, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT12 */
+};
+
+#define to_isp4_device(dev) \
+ ((struct isp4_device *)container_of(dev, struct isp4_device, v4l2_dev))

No need for the cast.

Sure, will remove the cast in the next patch>> +
+static irqreturn_t isp4_irq_handler(int irq, void *arg)
+{
+ struct isp4_device *isp_dev = dev_get_drvdata((struct device *)arg);
+
+ if (!isp_dev)
+ goto error_drv_data;
+
+error_drv_data:
+ return IRQ_HANDLED;
+}
+
+/*
+ * amd capture module
+ */
+static int isp4_capture_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct isp4_device *isp_dev;
+

Extra newline.

Yes, other comments also mentioned this, will remove it in the next patch>> + int i, irq, ret;

unsigned int i?

Weired, no compile warning about this, will change it to size_t i in the next patch.>> +
+ isp_dev = devm_kzalloc(&pdev->dev, sizeof(*isp_dev), GFP_KERNEL);
+ if (!isp_dev)
+ return -ENOMEM;
+
+ isp_dev->pdev = pdev;
+ dev->init_name = ISP4_DRV_NAME;
+
+ for (i = 0; i < ARRAY_SIZE(isp4_ringbuf_interrupt_num); i++) {
+ irq = platform_get_irq(pdev, isp4_ringbuf_interrupt_num[i]);
+ if (irq < 0)
+ return dev_err_probe(dev, -ENODEV,
+ "fail to get irq %d\n",
+ isp4_ringbuf_interrupt_num[i]);
+ ret = devm_request_irq(&pdev->dev, irq, isp4_irq_handler, 0,
+ "ISP_IRQ", &pdev->dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "fail to req irq %d\n",
+ irq);
+ }
+
+ isp_dev->pltf_data = pdev->dev.platform_data;
+
+ dev_dbg(dev, "isp irq registration successful\n");

Please leave this out.

Yes, other comments also mentioned this, will remove it in the next patch>> +
+ /* Link the media device within the v4l2_device */
+ isp_dev->v4l2_dev.mdev = &isp_dev->mdev;
+
+ /* Initialize media device */
+ strscpy(isp_dev->mdev.model, "amd_isp41_mdev",
+ sizeof(isp_dev->mdev.model));
+ snprintf(isp_dev->mdev.bus_info, sizeof(isp_dev->mdev.bus_info),
+ "platform:%s", ISP4_DRV_NAME);
+ isp_dev->mdev.dev = &pdev->dev;
+ media_device_init(&isp_dev->mdev);
+
+ /* register v4l2 device */
+ snprintf(isp_dev->v4l2_dev.name, sizeof(isp_dev->v4l2_dev.name),
+ "AMD-V4L2-ROOT");
+ ret = v4l2_device_register(&pdev->dev, &isp_dev->v4l2_dev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "fail register v4l2 device\n");
+
+ dev_dbg(dev, "AMD ISP v4l2 device registered\n");

This doesn't seem useful.

Yes, other comments also mentioned this, will remove it in the next patch>> +
+ ret = media_device_register(&isp_dev->mdev);
+ if (ret) {
+ dev_err(dev, "fail to register media device %d\n", ret);
+ goto err_unreg_v4l2;
+ }
+
+ platform_set_drvdata(pdev, isp_dev);
+
+ pm_runtime_set_suspended(dev);
+ pm_runtime_enable(dev);

You'll need to enable runtime PM before registering any interfaces on UAPI.
Same goes for setting driver data for the device.

Sure, will modify in the next patch>> +
+ return 0;
+
+err_unreg_v4l2:
+ v4l2_device_unregister(&isp_dev->v4l2_dev);
+
+ return dev_err_probe(dev, ret, "isp probe fail\n");
+}
+
+static void isp4_capture_remove(struct platform_device *pdev)
+{
+ struct isp4_device *isp_dev = platform_get_drvdata(pdev);
+ struct device *dev = &pdev->dev;
+
+ media_device_unregister(&isp_dev->mdev);
+ v4l2_device_unregister(&isp_dev->v4l2_dev);
+ dev_dbg(dev, "AMD ISP v4l2 device unregistered\n");

I'd say this is redundant.

Yes, other comments also mentioned this, will remove it in the next patch>> +}
+
+static struct platform_driver isp4_capture_drv = {
+ .probe = isp4_capture_probe,
+ .remove = isp4_capture_remove,
+ .driver = {
+ .name = ISP4_DRV_NAME,
+ .owner = THIS_MODULE,
+ }
+};
+
+module_platform_driver(isp4_capture_drv);
+
+MODULE_ALIAS("platform:" ISP4_DRV_NAME);
+MODULE_IMPORT_NS("DMA_BUF");
+
+MODULE_DESCRIPTION("AMD ISP4 Driver");
+MODULE_AUTHOR("Bin Du <bin.du@xxxxxxx>");
+MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@xxxxxxx>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/amd/isp4/isp4.h b/drivers/media/platform/amd/isp4/isp4.h
new file mode 100644
index 000000000000..27a7362ce6f9
--- /dev/null
+++ b/drivers/media/platform/amd/isp4/isp4.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2025 Advanced Micro Devices, Inc.
+ */
+
+#ifndef _ISP4_H_
+#define _ISP4_H_
+
+#include <linux/mutex.h>
+#include <media/v4l2-device.h>
+#include <media/videobuf2-memops.h>
+#include <media/videobuf2-vmalloc.h>
+
+#define ISP4_GET_ISP_REG_BASE(isp4sd) (((isp4sd))->mmio)
+
+struct isp4_platform_data {
+ void *adev;
+ void *bo;
+ void *cpu_ptr;
+ u64 gpu_addr;
+ u32 size;
+ u32 asic_type;
+ resource_size_t base_rmmio_size;
+};
+
+struct isp4_device {
+ struct v4l2_device v4l2_dev;
+ struct media_device mdev;
+
+ struct isp4_platform_data *pltf_data;
+ struct platform_device *pdev;
+ struct notifier_block i2c_nb;
+};
+
+#endif /* isp4.h */

Use the same name as for the macro here, please.

Yes, other comments also mentioned this, will modify in the next patch