[v2] Add AMD ISP4 driver

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

Posted by Bin Du 7 months, 3 weeks ago

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

Signed-off-by: Bin Du <Bin.Du@amd.com>
Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
---
 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@amd.com>
+M:	Nirujogi Pratap <pratap.nirujogi@amd.com>
+L:	linux-media@vger.kernel.org
+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@amd.com>
 L:	amd-gfx@lists.freedesktop.org
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
+	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
+
+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>
+
+#include "isp4.h"
+
+#define VIDEO_BUF_NUM 5
+
+#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))
+
+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;
+
+	int i, irq, ret;
+
+	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");
+
+	/* 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");
+
+	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);
+
+	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");
+}
+
+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@amd.com>");
+MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
+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 */
-- 
2.34.1

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

Posted by Sakari Ailus 6 months, 1 week ago

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.

> 
> Signed-off-by: Bin Du <Bin.Du@amd.com>
> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
> ---
>  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@amd.com>
> +M:	Nirujogi Pratap <pratap.nirujogi@amd.com>
> +L:	linux-media@vger.kernel.org
> +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@amd.com>
>  L:	amd-gfx@lists.freedesktop.org
> 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.

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

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

> +
> +#include "isp4.h"
> +
> +#define VIDEO_BUF_NUM 5

Unused.

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

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

> +	int i, irq, ret;

unsigned int i?

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

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

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

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

> +}
> +
> +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@amd.com>");
> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
> +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.

-- 
Kind regards,

Sakari Ailus

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

Posted by Du, Bin 6 months, 1 week ago

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@amd.com>
>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>> ---
>>   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@amd.com>
>> +M:	Nirujogi Pratap <pratap.nirujogi@amd.com>
>> +L:	linux-media@vger.kernel.org
>> +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@amd.com>
>>   L:	amd-gfx@lists.freedesktop.org
>> 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@amd.com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>> +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

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

Posted by Mario Limonciello 7 months, 3 weeks ago

On 6/18/2025 4:19 AM, Bin Du wrote:
> Amd isp4 capture is a v4l2 media device which implements media controller

AMD

> interface.
> It has one sub-device (amd ISP4 sub-device) endpoint which can be connected

AMD

> 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

MAINTAINERS

> 
> Signed-off-by: Bin Du <Bin.Du@amd.com>
> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>

Who actually developed?  If both are developers there should be a 
Co-developed-by tag.

> ---
>   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@amd.com>
> +M:	Nirujogi Pratap <pratap.nirujogi@amd.com>
> +L:	linux-media@vger.kernel.org
> +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@amd.com>
>   L:	amd-gfx@lists.freedesktop.org
> 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/

Is this whole file alphabetical?  If so this is the wrong place.

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

I don't believe this should default 'y'.  Normally drivers need to be 
opt in.

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

2025

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

As the directory is already conditional on CONFIG_AMD_ISP4 is this 
obj-$() needed?  Or should it really be obj-y?

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

Do you really need to enforce all these flags just for this driver?

Was this just for development to avoid having to remember to call the 
build with W=1 or CONFIG_WERROR?

> 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>
> +
> +#include "isp4.h"
> +
> +#define VIDEO_BUF_NUM 5
> +
> +#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))
> +
> +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;

In patch 5 you change this function, including dropping the goto and label.

So I suggest that for patch 1 you KISS:

static irqreturn_t isp4_irq_handler(int irq, void *arg)
{
	return IRQ_HANDLED;
}

Then in patch 5 add the extra conditional code and real handling.

> +}
> +
> +/*
> + * amd capture module
> + */

Pointless comment, no?

> +static int isp4_capture_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct isp4_device *isp_dev;
> +

Why newline here?

> +	int i, irq, ret;
> +
> +	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");

As you have error handling in place with dev_err_probe() I think the 
lack of an error implies this message.  I'd say drop it.

> +
> +	/* 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");

As you have error handling in place with dev_err_probe() I think the 
lack of an error implies this message.  I'd say drop it.

> +
> +	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);
> +
> +	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");

Probably not needed message anymore, right?

> +}
> +
> +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@amd.com>");
> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
> +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;

Will touch more on these in later patches, but I would say don't 
introduce them until the patch they're needed.

> +	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 */

/* ISP4_H */

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

Posted by Du, Bin 7 months, 3 weeks ago

Many thx Mario for your comments, really helpful, will address all of 
them in the next patch.
Add inline for some of your comments, pls check

On 6/18/2025 11:58 PM, Mario Limonciello wrote:
> On 6/18/2025 4:19 AM, Bin Du wrote:
>> Amd isp4 capture is a v4l2 media device which implements media controller
> 
> AMD
> 
>> interface.
>> It has one sub-device (amd ISP4 sub-device) endpoint which can be 
>> connected
> 
> AMD
> 
>> 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
> 
> MAINTAINERS
> 
>>
>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
> 
> Who actually developed?  If both are developers there should be a Co- 
> developed-by tag.
> 
>> ---
>>   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@amd.com>
>> +M:    Nirujogi Pratap <pratap.nirujogi@amd.com>
>> +L:    linux-media@vger.kernel.org
>> +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@amd.com>
>>   L:    amd-gfx@lists.freedesktop.org
>> 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/
> 
> Is this whole file alphabetical?  If so this is the wrong place.
> 
>>   # 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
> 
> I don't believe this should default 'y'.  Normally drivers need to be 
> opt in.
> 
>> +    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
>> +    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.
> 
> 2025
> 
>> +# 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
> 
> As the directory is already conditional on CONFIG_AMD_ISP4 is this obj- 
> $() needed?  Or should it really be obj-y?
>
Yes, it is needed, because AMD_ISP4 is trisate in Kconfig, it can be y or m

>> +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
>> +
>> +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
> 
> Do you really need to enforce all these flags just for this driver?
> 
> Was this just for development to avoid having to remember to call the 
> build with W=1 or CONFIG_WERROR?
> 
We found after patch submission, Media CI robot test will be triggered, 
when it builds the patch it will set these flags, so adding these flags 
to align with Media CI robot test to discover potential issue before 
submission.

>> 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>
>> +
>> +#include "isp4.h"
>> +
>> +#define VIDEO_BUF_NUM 5
>> +
>> +#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))
>> +
>> +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;
> 
> In patch 5 you change this function, including dropping the goto and label.
> 
> So I suggest that for patch 1 you KISS:
> 
> static irqreturn_t isp4_irq_handler(int irq, void *arg)
> {
>      return IRQ_HANDLED;
> }
> 
> Then in patch 5 add the extra conditional code and real handling.
> 
>> +}
>> +
>> +/*
>> + * amd capture module
>> + */
> 
> Pointless comment, no?
> 
>> +static int isp4_capture_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct isp4_device *isp_dev;
>> +
> 
> Why newline here?
> 
>> +    int i, irq, ret;
>> +
>> +    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");
> 
> As you have error handling in place with dev_err_probe() I think the 
> lack of an error implies this message.  I'd say drop it.
> 
>> +
>> +    /* 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");
> 
> As you have error handling in place with dev_err_probe() I think the 
> lack of an error implies this message.  I'd say drop it.
> 
>> +
>> +    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);
>> +
>> +    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");
> 
> Probably not needed message anymore, right?
> 
>> +}
>> +
>> +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@amd.com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>> +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;
> 
> Will touch more on these in later patches, but I would say don't 
> introduce them until the patch they're needed.
> 
>> +    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 */
> 
> /* ISP4_H */
>

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

Posted by Mario Limonciello 7 months, 3 weeks ago

On 6/19/2025 2:46 AM, Du, Bin wrote:
> Many thx Mario for your comments, really helpful, will address all of 
> them in the next patch.
> Add inline for some of your comments, pls check
> 
> On 6/18/2025 11:58 PM, Mario Limonciello wrote:
>> On 6/18/2025 4:19 AM, Bin Du wrote:
>>> Amd isp4 capture is a v4l2 media device which implements media 
>>> controller
>>
>> AMD
>>
>>> interface.
>>> It has one sub-device (amd ISP4 sub-device) endpoint which can be 
>>> connected
>>
>> AMD
>>
>>> 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
>>
>> MAINTAINERS
>>
>>>
>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>
>> Who actually developed?  If both are developers there should be a Co- 
>> developed-by tag.
>>
>>> ---
>>>   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@amd.com>
>>> +M:    Nirujogi Pratap <pratap.nirujogi@amd.com>
>>> +L:    linux-media@vger.kernel.org
>>> +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@amd.com>
>>>   L:    amd-gfx@lists.freedesktop.org
>>> 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/
>>
>> Is this whole file alphabetical?  If so this is the wrong place.
>>
>>>   # 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
>>
>> I don't believe this should default 'y'.  Normally drivers need to be 
>> opt in.
>>
>>> +    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
>>> +    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.
>>
>> 2025
>>
>>> +# 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
>>
>> As the directory is already conditional on CONFIG_AMD_ISP4 is this 
>> obj- $() needed?  Or should it really be obj-y?
>>
> Yes, it is needed, because AMD_ISP4 is trisate in Kconfig, it can be y or m

Got it, thanks for clarification.

> 
>>> +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
>>> +
>>> +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
>>
>> Do you really need to enforce all these flags just for this driver?
>>
>> Was this just for development to avoid having to remember to call the 
>> build with W=1 or CONFIG_WERROR?
>>
> We found after patch submission, Media CI robot test will be triggered, 
> when it builds the patch it will set these flags, so adding these flags 
> to align with Media CI robot test to discover potential issue before 
> submission.

I believe you can just compile with CONFIG_WERROR and get same result, 
no?  If I'm wrong, nonetheless this should be set in your external build 
environment script not in the Makefile to be going upstream.

> 
>>> 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>
>>> +
>>> +#include "isp4.h"
>>> +
>>> +#define VIDEO_BUF_NUM 5
>>> +
>>> +#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))
>>> +
>>> +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;
>>
>> In patch 5 you change this function, including dropping the goto and 
>> label.
>>
>> So I suggest that for patch 1 you KISS:
>>
>> static irqreturn_t isp4_irq_handler(int irq, void *arg)
>> {
>>      return IRQ_HANDLED;
>> }
>>
>> Then in patch 5 add the extra conditional code and real handling.
>>
>>> +}
>>> +
>>> +/*
>>> + * amd capture module
>>> + */
>>
>> Pointless comment, no?
>>
>>> +static int isp4_capture_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct isp4_device *isp_dev;
>>> +
>>
>> Why newline here?
>>
>>> +    int i, irq, ret;
>>> +
>>> +    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");
>>
>> As you have error handling in place with dev_err_probe() I think the 
>> lack of an error implies this message.  I'd say drop it.
>>
>>> +
>>> +    /* 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");
>>
>> As you have error handling in place with dev_err_probe() I think the 
>> lack of an error implies this message.  I'd say drop it.
>>
>>> +
>>> +    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);
>>> +
>>> +    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");
>>
>> Probably not needed message anymore, right?
>>
>>> +}
>>> +
>>> +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@amd.com>");
>>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>>> +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;
>>
>> Will touch more on these in later patches, but I would say don't 
>> introduce them until the patch they're needed.
>>
>>> +    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 */
>>
>> /* ISP4_H */
>>
>

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

Posted by Du, Bin 7 months, 3 weeks ago

Thanks Mario, just add one more comment for the CONFIG_WERROR

On 6/19/2025 9:00 PM, Mario Limonciello wrote:
> On 6/19/2025 2:46 AM, Du, Bin wrote:
>> Many thx Mario for your comments, really helpful, will address all of 
>> them in the next patch.
>> Add inline for some of your comments, pls check
>>
>> On 6/18/2025 11:58 PM, Mario Limonciello wrote:
>>> On 6/18/2025 4:19 AM, Bin Du wrote:
>>>> Amd isp4 capture is a v4l2 media device which implements media 
>>>> controller
>>>
>>> AMD
>>>
>>>> interface.
>>>> It has one sub-device (amd ISP4 sub-device) endpoint which can be 
>>>> connected
>>>
>>> AMD
>>>
>>>> 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
>>>
>>> MAINTAINERS
>>>
>>>>
>>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>
>>> Who actually developed?  If both are developers there should be a Co- 
>>> developed-by tag.
>>>
>>>> ---
>>>>   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@amd.com>
>>>> +M:    Nirujogi Pratap <pratap.nirujogi@amd.com>
>>>> +L:    linux-media@vger.kernel.org
>>>> +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@amd.com>
>>>>   L:    amd-gfx@lists.freedesktop.org
>>>> 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/
>>>
>>> Is this whole file alphabetical?  If so this is the wrong place.
>>>
>>>>   # 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
>>>
>>> I don't believe this should default 'y'.  Normally drivers need to be 
>>> opt in.
>>>
>>>> +    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
>>>> +    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.
>>>
>>> 2025
>>>
>>>> +# 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
>>>
>>> As the directory is already conditional on CONFIG_AMD_ISP4 is this 
>>> obj- $() needed?  Or should it really be obj-y?
>>>
>> Yes, it is needed, because AMD_ISP4 is trisate in Kconfig, it can be y 
>> or m
> 
> Got it, thanks for clarification.
> 
>>
>>>> +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
>>>> +
>>>> +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
>>>
>>> Do you really need to enforce all these flags just for this driver?
>>>
>>> Was this just for development to avoid having to remember to call the 
>>> build with W=1 or CONFIG_WERROR?
>>>
>> We found after patch submission, Media CI robot test will be 
>> triggered, when it builds the patch it will set these flags, so adding 
>> these flags to align with Media CI robot test to discover potential 
>> issue before submission.
> 
> I believe you can just compile with CONFIG_WERROR and get same result, 
> no?  If I'm wrong, nonetheless this should be set in your external build 
> environment script not in the Makefile to be going upstream.
> 
IMO, CONFIG_WERROR just treats warning as error, won't add other extra 
flags.
Sure, will remove them from the makefile and only add them to the local 
build environment.

>>
>>>> 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>
>>>> +
>>>> +#include "isp4.h"
>>>> +
>>>> +#define VIDEO_BUF_NUM 5
>>>> +
>>>> +#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))
>>>> +
>>>> +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;
>>>
>>> In patch 5 you change this function, including dropping the goto and 
>>> label.
>>>
>>> So I suggest that for patch 1 you KISS:
>>>
>>> static irqreturn_t isp4_irq_handler(int irq, void *arg)
>>> {
>>>      return IRQ_HANDLED;
>>> }
>>>
>>> Then in patch 5 add the extra conditional code and real handling.
>>>
>>>> +}
>>>> +
>>>> +/*
>>>> + * amd capture module
>>>> + */
>>>
>>> Pointless comment, no?
>>>
>>>> +static int isp4_capture_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct isp4_device *isp_dev;
>>>> +
>>>
>>> Why newline here?
>>>
>>>> +    int i, irq, ret;
>>>> +
>>>> +    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");
>>>
>>> As you have error handling in place with dev_err_probe() I think the 
>>> lack of an error implies this message.  I'd say drop it.
>>>
>>>> +
>>>> +    /* 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");
>>>
>>> As you have error handling in place with dev_err_probe() I think the 
>>> lack of an error implies this message.  I'd say drop it.
>>>
>>>> +
>>>> +    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);
>>>> +
>>>> +    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");
>>>
>>> Probably not needed message anymore, right?
>>>
>>>> +}
>>>> +
>>>> +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@amd.com>");
>>>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>>>> +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;
>>>
>>> Will touch more on these in later patches, but I would say don't 
>>> introduce them until the patch they're needed.
>>>
>>>> +    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 */
>>>
>>> /* ISP4_H */
>>>
>>
>

[PATCH v2 1/8] media: platform: amd: Introduce amd isp4 capture driver
[PATCH v2 2/8] media: platform: amd: low level support for isp4 firmware
[PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface
[PATCH v2 5/8] media: platform: amd: isp4 subdev and firmware loading handling added
[PATCH v2 6/8] media: platform: amd: isp4 video node and buffers handling added
[PATCH v2 7/8] media: platform: amd: isp4 debug fs logging and more descriptive errors
[PATCH v2 8/8] Documentation: add documentation of AMD isp 4 driver