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

["Du, Bin" <bin.du@amd.com>]

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