Re: [PATCH 3/3] decoder: Add V4L2 stateless H.264 decoder driver

[Zhentao Guo]

在 2025/10/27 21:10, Krzysztof Kozlowski 写道:
> [You don't often get email from krzk@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> [ EXTERNAL EMAIL ]
>
> On 27/10/2025 06:42, Zhentao Guo via B4 Relay wrote:
> > From: Zhentao Guo <zhentao.guo@xxxxxxxxxxx>
> >
> > This patch introduces initial driver support for Amlogic's new
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94
Got it, I'll refer to the document and revise the commit message.
> > video acceleration hardware architecture, designed for video
> > stream decoding. The driver is designed to support the
> > V4L2 M2M stateless decoder API. In phase 1, it supports H.264
> > bitstreams decoding.
> >
> > Signed-off-by: Zhentao Guo <zhentao.guo@xxxxxxxxxxx>
> ...
>
>
> > +
> > +static int aml_vdec_drv_probe(struct platform_device *pdev)
> > +{
> > +     struct aml_vdec_dev *dev;
> > +     struct video_device *vfd_dec;
> > +     struct aml_vdec_hw *hw;
> > +     int ret = 0;
> > +
> > +     dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> > +     if (!dev)
> > +             return -ENOMEM;
> > +
> > +     dev->plat_dev = pdev;
> > +     mutex_init(&dev->dev_mutex);
> > +
> > +     ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret, "v4l2_device_register err\n");
> > +
> > +     vfd_dec = video_device_alloc();
> > +     if (!vfd_dec) {
> > +             v4l2_err(&dev->v4l2_dev, "Failed to allocate video device\n");
> > +             ret = -ENOMEM;
> > +             goto err_device_alloc;
> > +     }
> > +     *vfd_dec = dec_dev;
> > +     vfd_dec->v4l2_dev = &dev->v4l2_dev;
> > +     vfd_dec->lock = &dev->dev_mutex;
> > +     video_set_drvdata(vfd_dec, dev);
> > +     dev->vfd = vfd_dec;
> > +     platform_set_drvdata(pdev, dev);
> > +
> > +     hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> Why aren't you using devm interfaces?
Thanks for the reminder, It would be better to use the devm interfaces here,
I'll fix this.
> > +     if (!hw) {
> > +             ret = -ENOMEM;
> > +             goto err_dec_mem_init;
> > +     }
> > +     dev->dec_hw = hw;
> > +
> > +     dev->pvdec_data = of_device_get_match_data(&pdev->dev);
> > +     ret = dev->pvdec_data->req_hw_resource(dev);
> > +     if (ret < 0)
> > +             goto err_hw_init;
> > +
> > +     dev->m2m_dev_dec = v4l2_m2m_init(&aml_vdec_m2m_ops);
> > +     if (IS_ERR((__force void *)dev->m2m_dev_dec)) {
> Huh? Why the cast?
The cast seems superfluous here, I'll remove it.
> > +             v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem dec device\n");
> > +             ret = PTR_ERR((__force void *)dev->m2m_dev_dec);
> > +             goto err_hw_init;
> > +     }
> > +
> > +     ret = video_register_device(vfd_dec, VFL_TYPE_VIDEO, -1);
> > +     if (ret) {
> > +             v4l2_err(&dev->v4l2_dev, "Failed to register video device");
> > +             goto err_vid_dev_register;
> > +     }
> > +
> > +     v4l2_info(&dev->v4l2_dev, "Registered %s as /dev/%s\n",
> > +               vfd_dec->name, video_device_node_name(vfd_dec));
> This does not look like useful printk message. Drivers should be silent
> on success:
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
Got it, I'll get rid of it.
> > +
> > +     dev->mdev.dev = &pdev->dev;
> > +     strscpy(dev->mdev.model, AML_VDEC_DRV_NAME, sizeof(dev->mdev.model));
> > +     media_device_init(&dev->mdev);
> > +     dev->mdev.ops = &aml_m2m_media_ops;
> > +     dev->v4l2_dev.mdev = &dev->mdev;
> > +
> > +     ret = v4l2_m2m_register_media_controller(dev->m2m_dev_dec, vfd_dec,
> > +                                              MEDIA_ENT_F_PROC_VIDEO_DECODER);
> > +     if (ret) {
> > +             v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem media controller\n");
> > +             goto error_m2m_mc_register;
> > +     }
> > +
> > +     ret = media_device_register(&dev->mdev);
> > +     if (ret) {
> > +             v4l2_err(&dev->v4l2_dev, "Failed to register media device");
> > +             goto err_media_dev_register;
> > +     }
> > +     vdec_enable(dev->dec_hw);
> > +     return 0;
> > +
> > +err_media_dev_register:
> > +     v4l2_m2m_unregister_media_controller(dev->m2m_dev_dec);
> > +error_m2m_mc_register:
> > +     media_device_cleanup(&dev->mdev);
> > +err_vid_dev_register:
> > +     v4l2_m2m_release(dev->m2m_dev_dec);
> > +err_hw_init:
> > +     kfree(hw);
> > +     dev->dec_hw = NULL;
> > +err_dec_mem_init:
> > +     video_device_release(vfd_dec);
> > +err_device_alloc:
> > +     v4l2_device_unregister(&dev->v4l2_dev);
> > +     return ret;
> > +}
> > +
> > +static void aml_vdec_drv_remove(struct platform_device *pdev)
> > +{
> > +     struct aml_vdec_dev *dev = platform_get_drvdata(pdev);
> > +
> > +     vdec_disable(dev->dec_hw);
> > +
> > +     if (media_devnode_is_registered(dev->mdev.devnode)) {
> > +             media_device_unregister(&dev->mdev);
> > +             media_device_cleanup(&dev->mdev);
> > +     }
> > +
> > +     if (dev->m2m_dev_dec)
> > +             v4l2_m2m_release(dev->m2m_dev_dec);
> > +     if (dev->vfd)
> > +             video_unregister_device(dev->vfd);
> > +     if (dev->dec_hw)
> > +             dev->pvdec_data->destroy_hw_resource(dev);
> > +
> > +     v4l2_device_unregister(&dev->v4l2_dev);
> > +
> > +     pr_debug("aml v4l2 decoder driver remove\n");
>
> This does not look like useful printk message. Drivers should be silent
> on success:
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
I'll remove this message.
> > +}
> > +
> > +static const struct of_device_id aml_vdec_match[] = {
> > +     {.compatible = "amlogic,s4-vcodec-dec", .data = &aml_vdec_s4_pdata},
> > +     {},
> > +};
> > +
> > +static struct platform_driver aml_vcodec_dec_driver = {
> > +     .probe    = aml_vdec_drv_probe,
> > +     .remove    = aml_vdec_drv_remove,
> > +     .driver    = {
> > +             .name    = AML_VDEC_DRV_NAME,
> > +             .of_match_table = aml_vdec_match,
> > +     },
> > +};
> > +
> > +static int __init aml_vdec_init(void)
> > +{
> > +     pr_debug("aml v4l2 decoder module init\n");
> > +
> > +     if (platform_driver_register(&aml_vcodec_dec_driver)) {
> > +             pr_err("failed to register aml v4l2 decoder\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void __exit aml_vdec_exit(void)
> > +{
> > +     pr_debug("aml v4l2 decoder module exit\n");
> > +     platform_driver_unregister(&aml_vcodec_dec_driver);
> No, drop entire init/exit. This is some really odd code, probably
> copy-paste from poor downstream. Please look at upstream drivers - do
> you see anything like that anywhere?
Yes, it should be dropped. I reviewed some other upstream drivers, nobody
add this.
> > +}
> > +
> > +module_init(aml_vdec_init);
> > +module_exit(aml_vdec_exit);
> > +
> Best regards,
> Krzysztof

Thank you

Zhentao