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

[Krzysztof Kozlowski]

On 24/07/2026 09:04, Zhentao Guo via B4 Relay wrote:
> From: Zhentao Guo <zhentao.guo@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxx>
> +L:	linux-media@xxxxxxxxxxxxxxx

You can drop the L:. Subsystem defines it.

> +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@xxxxxxxxx>
>  L:	linux-hwmon@xxxxxxxxxxxxxxx
> 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.

...

> +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?

> +		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.


> +	}
> +	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.


Best regards,
Krzysztof