Message-ID: <68f481ff-3b99-4c21-964d-edb81b34452e@amd.com>
Date: Thu, 19 Jun 2025 15:46:54 +0800
From: "Du, Bin" <bin.du@....com>
To: Mario Limonciello <mario.limonciello@....com>, mchehab@...nel.org,
 hverkuil@...all.nl, laurent.pinchart+renesas@...asonboard.com,
 bryan.odonoghue@...aro.org, sakari.ailus@...ux.intel.com,
 prabhakar.mahadev-lad.rj@...renesas.com, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
 gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com,
 Svetoslav.Stoilov@....com
Subject: Re: [PATCH v2 1/8] media: platform: amd: Introduce amd isp4 capture
 driver

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@....com>
>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@....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@....com>
>> +M:    Nirujogi Pratap <pratap.nirujogi@....com>
>> +L:    linux-media@...r.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@....com>
>>   L:    amd-gfx@...ts.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@....com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@....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 */
> 

Powered by blists - more mailing lists