Re: [PATCH v4 5/7] media: platform: amd: isp4 video node and buffers handling added

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

Hi Sultan,

On 10/11/2025 5:30 PM, Du, Bin wrote:
> Many thanks Sultan for your review, that's really helpful.
> 
> On 10/1/2025 2:53 PM, Sultan Alsawaf wrote:
>> Hi Bin,
>>
>> On Thu, Sep 11, 2025 at 06:08:45PM +0800, Bin Du wrote:
>>> Isp video implements v4l2 video interface and supports NV12 and YUYV. It
>>> manages buffers, pipeline power and state. Cherry-picked Sultan's DMA
>>> buffer related fix from branch v6.16-drm-tip-isp4-for-amd on
>>> https://github.com/kerneltoast/kernel_x86_laptop.git
>>>
>>> Co-developed-by: Sultan Alsawaf <sultan@kerneltoast.com>
>>> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
>>> Co-developed-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>> Tested-by: Alexey Zagorodnikov <xglooom@gmail.com>
>>
>> [snip]
>>
>>> +++ b/drivers/media/platform/amd/isp4/isp4.c
>>> @@ -178,6 +178,16 @@ static int isp4_capture_probe(struct 
>>> platform_device *pdev)
>>>           goto err_isp4_deinit;
>>>       }
>>> +    ret = media_create_pad_link(&isp_dev->isp_sdev.sdev.entity,
>>> +                    0, &isp_dev->isp_sdev.isp_vdev.vdev.entity,
>>> +                    0,
>>> +                    MEDIA_LNK_FL_ENABLED |
>>> +                    MEDIA_LNK_FL_IMMUTABLE);
>>> +    if (ret) {
>>> +        dev_err(dev, "fail to create pad link %d\n", ret);
>>> +        goto err_isp4_deinit;
>>> +    }
>>> +
>>
>> Two problems with this hunk:
>>
>> 1. According to the comment in include/media/media-device.h [1],
>>     media_create_pad_link() should be called before 
>> media_device_register():
>>
>>      * So drivers need to first initialize the media device, register 
>> any entity
>>      * within the media device, create pad to pad links and then 
>> finally register
>>      * the media device by calling media_device_register() as a final 
>> step.
>>
>> 2. Missing call to media_device_unregister() on error when
>>     media_create_pad_link() fails.
>>
>> Since the media_create_pad_link() will be moved before 
>> media_device_register(),
>> we will need to clean up media_create_pad_link() when 
>> media_device_register()
>> fails.
>>
>> The clean-up function for media_create_pad_link() is 
>> media_device_unregister().
>> According to the comment for media_device_unregister() [2], it is safe 
>> to call
>> media_device_unregister() on an unregistered media device that is 
>> initialized
>> (through media_device_init()).
>>
>> In addition, this made me realize that there's no call to 
>> media_device_cleanup()
>> in the entire driver too. This is the cleanup function for 
>> media_device_init(),
>> so it should be called on error and on module unload.
>>
>> To summarize, make the following changes:
>>
>> 1. Move the media_create_pad_link() up, right before 
>> media_device_register().
>>
>> 2. When media_device_register() fails, clean up 
>> media_create_pad_link() by
>>     calling media_device_unregister().
>>
>> 3. Add a missing call to media_device_cleanup() on error and module 
>> unload to
>>     clean up media_device_init().
>>
> 
> Very clear guide, will follow your advice.
> 
>>>       platform_set_drvdata(pdev, isp_dev);
>>>       return 0;

For 2, we found when media_device_register() fails, calling 
media_device_unregister() won't clean up media_create_pad_link() because 
it simply returns without doing anything(see 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/mc/mc-device.c?h=v6.17-rc7#n797). 
Therefore like other kernel drivers, we'd rather not call 
media_device_unregister() in this scenario, it doesn't cause issues, but 
it's not logically correct. Cleanup for media_create_pad_link() occurs 
during error handling via 
isp4sd_deinit()->isp4vid_dev_deinit()->vb2_video_unregister_device()->...->media_entity_remove_link(). 
What do you think?

[snip]

-- 
Regards,
Bin