Re: [PATCH v6 4/6] decoder: Add V4L2 stateless H.264 decoder driver

[Zhentao Guo <zhentao.guo@amlogic.com>]

Hi Krzysztof,

> On 24/07/2026 09:04, Zhentao Guo via B4 Relay wrote:
>> From: Zhentao Guo <zhentao.guo@amlogic.com>
>>
>> Add initial support for V4L2 stateless video decoder
>> driver on Amlogic S4(S805X2) platform. In phase 1,
>> it supports 8bit H.264 bitstreams decoding. Currently
>> only progressive streams are supported.
>>
>> Signed-off-by: Zhentao Guo <zhentao.guo@amlogic.com>
>> ---
>>   MAINTAINERS                                        |    7 +
>>   drivers/media/platform/amlogic/Kconfig             |    1 +
>>   drivers/media/platform/amlogic/Makefile            |    1 +
>>   drivers/media/platform/amlogic/vdec/Kconfig        |   18 +
>>   drivers/media/platform/amlogic/vdec/Makefile       |    4 +
>>   drivers/media/platform/amlogic/vdec/TODO           |    7 +
>>   drivers/media/platform/amlogic/vdec/aml_vdec.c     |  736 +++++++
>>   drivers/media/platform/amlogic/vdec/aml_vdec.h     |   33 +
>>   drivers/media/platform/amlogic/vdec/aml_vdec_drv.c |  239 +++
>>   drivers/media/platform/amlogic/vdec/aml_vdec_drv.h |  172 ++
>>   drivers/media/platform/amlogic/vdec/aml_vdec_hw.c  |  538 +++++
>>   drivers/media/platform/amlogic/vdec/aml_vdec_hw.h  |  157 ++
>>   .../platform/amlogic/vdec/aml_vdec_platform.c      |   81 +
>>   .../platform/amlogic/vdec/aml_vdec_platform.h      |   46 +
>>   .../media/platform/amlogic/vdec/aml_vdec_tee_fw.c  |  240 +++
>>   .../media/platform/amlogic/vdec/aml_vdec_tee_fw.h  |   27 +
>>   drivers/media/platform/amlogic/vdec/h264.c         | 2067 ++++++++++++++++++++
>>   drivers/media/platform/amlogic/vdec/h264.h         |  299 +++
>>   drivers/media/platform/amlogic/vdec/reg_defines.h  |  177 ++
>>   19 files changed, 4850 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a112ce9f0fa0..a27ea19907f9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1423,6 +1423,13 @@ S:     Maintained
>>   F:   Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
>>   F:   drivers/spi/spi-amlogic-spisg.c
>>
>> +AMLOGIC VDEC DRIVER
>> +M:   Zhentao Guo <zhentao.guo@amlogic.com>
>> +L:   linux-media@vger.kernel.org
> You can drop the L:. Subsystem defines it.
OK, got it. I'll drop this.
>
>> +S:   Maintained
>> +F:   Documentation/devicetree/bindings/media/amlogic,s4-vcodec-dec.yaml
>> +F:   drivers/media/platform/amlogic/vdec/
>> +
>>   AMPHENOL CHIPCAP 2 DRIVER
>>   M:   Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>   L:   linux-hwmon@vger.kernel.org
>> diff --git a/drivers/media/platform/amlogic/Kconfig b/drivers/media/platform/amlogic/Kconfig
>> index 458acf3d5fa8..7c541ac0d0c3 100644
>> --- a/drivers/media/platform/amlogic/Kconfig
>> +++ b/drivers/media/platform/amlogic/Kconfig
>> @@ -4,3 +4,4 @@ comment "Amlogic media platform drivers"
>>
>>   source "drivers/media/platform/amlogic/c3/Kconfig"
>>   source "drivers/media/platform/amlogic/meson-ge2d/Kconfig"
>> +source "drivers/media/platform/amlogic/vdec/Kconfig"
>> diff --git a/drivers/media/platform/amlogic/Makefile b/drivers/media/platform/amlogic/Makefile
>> index c744afcd1b9e..7409de674c0b 100644
>> --- a/drivers/media/platform/amlogic/Makefile
>> +++ b/drivers/media/platform/amlogic/Makefile
>> @@ -2,3 +2,4 @@
>>
>>   obj-y += c3/
>>   obj-y += meson-ge2d/
>> +obj-y += vdec/
>> diff --git a/drivers/media/platform/amlogic/vdec/Kconfig b/drivers/media/platform/amlogic/vdec/Kconfig
>> new file mode 100644
>> index 000000000000..d392967c7743
>> --- /dev/null
>> +++ b/drivers/media/platform/amlogic/vdec/Kconfig
>> @@ -0,0 +1,18 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR MIT)
>> +
>> +config VIDEO_AMLOGIC_VDEC
>> +    tristate "Amlogic Video Decoder Driver"
>> +    depends on ARCH_MESON || COMPILE_TEST
>> +    depends on VIDEO_DEV
>> +    depends on V4L_MEM2MEM_DRIVERS
>> +    depends on TEE
>> +    select VIDEOBUF2_DMA_CONTIG
>> +    select V4L2_H264
>> +    select V4L2_MEM2MEM_DEV
>> +    select MESON_CANVAS
>> +    select MESON_SM
>> +    help
>> +      This is a v4l2 driver for Amlogic video decoder driver.
>> +       This driver is designed to support V4L2 M2M STATELESS
>> +       interface.
> Somehow messed indentation.
I'll fix the indentation problem in the next revision.
> ...
>
>> +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:
>> +     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) {
> I do not understand why do you release resources conditionally. Where
> they allocated conditionally? Why probe is not releasing them the same
> way in error paths?

These resources are allocated unconditionally in probe, so they should 
be released unconditionally as well.

I was being overly cautious here to avoid potential double-free 
scenarios. But in fact these resources are only released in remove(), so 
the conditional is redundant. I'll drop it in the next revision.

>
>> +             dev->pvdec_data->destroy_hw_resource(dev);
>> +             dev->dec_hw = NULL;
> What's the purpose of this? If this is needed, then your driver has
> serious concurrency issues which are not solved with it.
>
Do you mean the statement 'dev->dec_hw = NULL;'? Actuallyin the remove 
callback, all resources are at the end of their lifecycle and no other 
path will access this pointer. This was just my coding habit, nulling 
out pointers after freeing them. I'll drop it if you think this is 
unnecessary.
>> +     }
>> +     v4l2_device_unregister(&dev->v4l2_dev);
>> +}
>> +
>> +static const struct of_device_id aml_vdec_match[] = {
>> +     {.compatible = "amlogic,s4-vdec", .data = &aml_vdec_s4_pdata},
> Fix the style, so you will not re-introduce issue Uwe is fixing.
Thanks for the reminder. I will refer to Uwe's patch and improve this.
>
>
> Best regards,
> Krzysztof

BRs

Zhentao


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic