Re: [PATCH v2 5/8] media: platform: amd: isp4 subdev and firmware loading handling added

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

On 7/7/2025 4:55 AM, Mario Limonciello wrote:
> On 6/20/2025 5:31 AM, Du, Bin wrote:
>> Thanks Mario, add some comments
>>
>> On 6/19/2025 12:35 AM, Mario Limonciello wrote:
>>> On 6/18/2025 4:19 AM, Bin Du wrote:
>>>> Isp4 sub-device is implementing v4l2 sub-device interface. It has one
>>>> capture video node, and supports only preview stream. It manages
>>>> firmware states, stream configuration and mipi phy configuration.
>>>> This change also adds interrupt handling and notification for isp
>>>> firmware to isp-subdevice.
>>>>
>>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>> ---
>>>>   drivers/media/platform/amd/isp4/Makefile      |    3 +-
>>>>   drivers/media/platform/amd/isp4/isp4.c        |  267 +++-
>>>>   drivers/media/platform/amd/isp4/isp4.h        |    4 +-
>>>>   drivers/media/platform/amd/isp4/isp4_subdev.c | 1192 +++++++++++++ 
>>>> ++++
>>>>   drivers/media/platform/amd/isp4/isp4_subdev.h |  145 ++
>>>>   5 files changed, 1601 insertions(+), 10 deletions(-)
>>>>   create mode 100644 drivers/media/platform/amd/isp4/isp4_subdev.c
>>>>   create mode 100644 drivers/media/platform/amd/isp4/isp4_subdev.h
>>>>
>>>> diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/ 
>>>> media/ platform/amd/isp4/Makefile
>>>> index c0166f954516..52defc06189e 100644
>>>> --- a/drivers/media/platform/amd/isp4/Makefile
>>>> +++ b/drivers/media/platform/amd/isp4/Makefile
>>>> @@ -3,9 +3,10 @@
>>>>   # Copyright (C) 2025 Advanced Micro Devices, Inc.
>>>>   obj-$(CONFIG_AMD_ISP4) += amd_capture.o
>>>> -amd_capture-objs := isp4.o    \
>>>> +amd_capture-objs := isp4_subdev.o \
>>>>               isp4_phy.o \
>>>>               isp4_interface.o \
>>>> +            isp4.o    \
>>>>               isp4_hw.o    \
>>>>   ccflags-y += -I$(srctree)/drivers/media/platform/amd/isp4
>>>> diff --git a/drivers/media/platform/amd/isp4/isp4.c b/drivers/media/ 
>>>> platform/amd/isp4/isp4.c
>>>> index d0be90c5ec3b..c1aca2bd35e3 100644
>>>> --- a/drivers/media/platform/amd/isp4/isp4.c
>>>> +++ b/drivers/media/platform/amd/isp4/isp4.c
>>>> @@ -5,13 +5,20 @@
>>>>   #include <linux/pm_runtime.h>
>>>>   #include <linux/vmalloc.h>
>>>> +#include <media/v4l2-fwnode.h>
>>>>   #include <media/v4l2-ioctl.h>
>>>> -#include "isp4.h"
>>>> +#include "amdgpu_object.h"
>>>> -#define VIDEO_BUF_NUM 5
>>>> +#include "isp4.h"
>>>> +#include "isp4_hw.h"
>>>>   #define ISP4_DRV_NAME "amd_isp_capture"
>>>> +#define ISP4_FW_RESP_RB_IRQ_STATUS_MASK \
>>>> +    (ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT9_INT_MASK  | \
>>>> +     ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT10_INT_MASK | \
>>>> +     ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT11_INT_MASK | \
>>>> +     ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT12_INT_MASK)
>>>>   /* interrupt num */
>>>>   static const u32 isp4_ringbuf_interrupt_num[] = {
>>>> @@ -24,23 +31,233 @@ static const u32 isp4_ringbuf_interrupt_num[] = {
>>>>   #define to_isp4_device(dev) \
>>>>       ((struct isp4_device *)container_of(dev, struct isp4_device, 
>>>> v4l2_dev))
>>>>>>>> [snip]
>>>> >>>> +static int isp4_register_subdev_and_create_links(struct isp4_device
>>>> *isp_dev,
>>>> +                         struct v4l2_subdev *sdev)
>>>> +{
>>>> +    struct device *dev = &isp_dev->pdev->dev;
>>>> +    int ret;
>>>> +
>>>> +    ret = isp4_create_links(isp_dev, sdev);
>>>> +    if (ret)
>>>> +        dev_err(dev, "fail create isp link:%d\n", ret);
>>>
>>> You're discarding ret here.  Shouldn't you return ret in the failure 
>>> path?
>> Even if it fails, some Apps which doesn't depend on the media link can 
>> still work, so just print error message and ignore it so at least 
>> these Apps can still work, do you think it acceptible?>
> 
> I think it should be a warn or an info if it's an acceptable situation 
> along with a comment to help us years later understand why the error 
> handling is that way.
> 
Sure, will make it warn and add comment>>>> +
>>>> +    ret = v4l2_device_register_subdev_nodes(&isp_dev->v4l2_dev);
>>>> +    if (ret != 0) {
>>>> +        dev_warn(dev, "register subdev as nodes fail:%d\n", ret);
>>>> +        ret = 0;
>>>
>>> This isn't fatal?
>>>
>> This call is just expose subdev node to user space, even if it fails, 
>> some Apps can still work, so just print error message and ignore it so 
>> at least these Apps can still work, do you think it acceptible?>> +    }
> 
> I guess same comment as above.
> 
Same as above>>>> +
>>>> +    return ret;
>>>> +}
>>>>
>>>> [snip]>>>>