Re: [PATCH v2 09/12] media: rkvdec: Add H264 support for the VDPU381 variant

["Diederik de Haas" <didi.debian@cknow.org>]

[-- Attachment #1: Type: text/plain, Size: 7068 bytes --]

Hi Detlev,

Thanks a LOT for this patch set!
As already reported the results were quite impressive :-D

To figure out how to make VDPU346 for rk3568 work, I had a close look at
the TRM and compared that to rk3588's TRM and compared it to your code.
I think I may have found a few potential issue, but I may also not
understand things correctly.

On Fri Aug 8, 2025 at 10:03 PM CEST, Detlev Casanova wrote:
> This decoder variant is found in Rockchip RK3588 SoC family.
>
> Like for rkvdec on rk3399, it supports the NV12, NV15, NV16 and NV20
> output formats and level up to 5.1.
>
> The maximum width and height have been significantly increased
> supporting up to 65520 pixels for both.
>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  .../media/platform/rockchip/rkvdec/Makefile   |   1 +
>  .../rockchip/rkvdec/rkvdec-h264-common.h      |   2 +
>  .../platform/rockchip/rkvdec/rkvdec-h264.c    |  36 --
>  .../rockchip/rkvdec/rkvdec-vdpu381-h264.c     | 469 ++++++++++++++++++
>  .../rockchip/rkvdec/rkvdec-vdpu381-regs.h     | 427 ++++++++++++++++
>  .../media/platform/rockchip/rkvdec/rkvdec.c   | 164 +++++-
>  .../media/platform/rockchip/rkvdec/rkvdec.h   |   6 +
>  7 files changed, 1067 insertions(+), 38 deletions(-)
>  create mode 100644 drivers/media/platform/rockchip/rkvdec/rkvdec-vdpu381-h264.c
>  create mode 100644 drivers/media/platform/rockchip/rkvdec/rkvdec-vdpu381-regs.h
>
> ...
>
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec-vdpu381-regs.h b/drivers/media/platform/rockchip/rkvdec/rkvdec-vdpu381-regs.h
> new file mode 100644
> index 0000000000000..11b545e9ee7ea
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec-vdpu381-regs.h
> @@ -0,0 +1,427 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Rockchip VDPU381 Video Decoder driver registers description
> + *
> + * Copyright (C) 2024 Collabora, Ltd.
> + *  Detlev Casanova <detlev.casanova@collabora.com>
> + */
> +
> +#include <linux/types.h>
> +
> +#ifndef _RKVDEC_REGS_H_
> +#define _RKVDEC_REGS_H_
> +
> +#define OFFSET_COMMON_REGS		(8 * sizeof(u32))
> +#define OFFSET_CODEC_PARAMS_REGS	(64 * sizeof(u32))
> +#define OFFSET_COMMON_ADDR_REGS		(128 * sizeof(u32))
> +#define OFFSET_CODEC_ADDR_REGS		(160 * sizeof(u32))
> +#define OFFSET_POC_HIGHBIT_REGS		(200 * sizeof(u32))
> +
> +#define VDPU381_MODE_HEVC	0
> +#define VDPU381_MODE_H264	1
> +#define VDPU381_MODE_VP9	2
> +#define VDPU381_MODE_AVS2	3
> +
> +#define MAX_SLICE_NUMBER	0x3fff
> +
> +#define RKVDEC_1080P_PIXELS		(1920 * 1080)
> +#define RKVDEC_4K_PIXELS		(4096 * 2304)
> +#define RKVDEC_8K_PIXELS		(7680 * 4320)

In the RK3588 TRM Part 1 paragraph 5.4.3 I can find the values for 4K
and 8K, but the 1080P resolution seems to be 1920x1088 (not 1080).

> +#define RKVDEC_TIMEOUT_1080p		(0xefffff)
> +#define RKVDEC_TIMEOUT_4K		(0x2cfffff)
> +#define RKVDEC_TIMEOUT_8K		(0x4ffffff)
> +#define RKVDEC_TIMEOUT_MAX		(0xffffffff)
> +
> +#define VDPU381_REG_DEC_E		0x028
> +#define VDPU381_DEC_E_BIT		1
> +
> +#define VDPU381_REG_IMPORTANT_EN	0x02c
> +#define VDPU381_DEC_IRQ_DISABLE	BIT(4)
> +
> +#define VDPU381_REG_STA_INT		0x380
> +#define VDPU381_STA_INT_DEC_RDY_STA	BIT(2)
> +#define VDPU381_STA_INT_ERROR		BIT(4)
> +#define VDPU381_STA_INT_TIMEOUT		BIT(5)
> +#define VDPU381_STA_INT_SOFTRESET_RDY	BIT(9)
> +
> +/* base: OFFSET_COMMON_REGS */
>
> ...
>
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> index 0ccf1ba81958a..1b55fe4ff2baf 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> @@ -28,6 +28,7 @@
>  
>  #include "rkvdec.h"
>  #include "rkvdec-regs.h"
> +#include "rkvdec-vdpu381-regs.h"
>  #include "rkvdec-rcb.h"
>  
>  static bool rkvdec_image_fmt_match(enum rkvdec_image_fmt fmt1,
> @@ -84,11 +85,50 @@ static bool rkvdec_is_valid_fmt(struct rkvdec_ctx *ctx, u32 fourcc,
>  	return false;
>  }
>  
> +#define VDPU38X_STRIDE_ALIGN	16
> +
> +/**
> + * The default v4l2_fill_pixfmt_mp() function doesn't allow for specific alignment values.
> + * As the VDPU381 and VDPU383 need lines to be aligned on 16, use our own implementation here.
> + */
> +static int vdpu38x_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pix_mp, u32 pixelformat,
> +				  u32 width, u32 height)
> +{
> +	const struct v4l2_format_info *info = v4l2_format_info(pix_mp->pixelformat);
> +	struct v4l2_plane_pix_format *plane = &pix_mp->plane_fmt[0];
> +
> +	if (!info)
> +		return -EINVAL;
> +
> +	pix_mp->num_planes = 1;
> +
> +	memset(plane, 0, sizeof(*plane));
> +
> +	plane->bytesperline = pix_mp->width * info->bpp[0] / info->bpp_div[0];
> +	plane->bytesperline = ALIGN(plane->bytesperline, VDPU38X_STRIDE_ALIGN);
> +
> +	for (int i = 0; i < info->comp_planes; i++) {
> +		unsigned int vdiv = i ? info->vdiv : 1;
> +		unsigned int hdiv = i ? info->hdiv : 1;
> +		unsigned int stride = DIV_ROUND_UP(pix_mp->width, hdiv)
> +				    * info->bpp[i] / info->bpp_div[i];
> +		unsigned int height = DIV_ROUND_UP(pix_mp->height, vdiv);
> +
> +		plane->sizeimage += ALIGN(stride, VDPU38X_STRIDE_ALIGN) * height;
> +	}
> +
> +	return 0;
> +}
> +
>  static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
>  				       struct v4l2_pix_format_mplane *pix_mp)
>  {
> -	v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
> -			    pix_mp->width, pix_mp->height);
> +	struct rkvdec_config *cfg = ctx->dev->config;
> +
> +	cfg->fill_pixfmt_mp(pix_mp, pix_mp->pixelformat, pix_mp->width, pix_mp->height);
> +
> +	ctx->colmv_offset = pix_mp->plane_fmt[0].sizeimage;
> +
>  	pix_mp->plane_fmt[0].sizeimage += 128 *
>  		DIV_ROUND_UP(pix_mp->width, 16) *
>  		DIV_ROUND_UP(pix_mp->height, 16);
> @@ -287,6 +327,25 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
>  	}
>  };
>  
> +static const struct rkvdec_coded_fmt_desc vdpu381_coded_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_H264_SLICE,
> +		.frmsize = {
> +			.min_width = 64,
> +			.max_width =  65520,
> +			.step_width = 64,
> +			.min_height = 16,
> +			.max_height =  65520,
> +			.step_height = 16,
> +		},

Also in the RK3588 TRM Part 1 paragraph 5.4.3, I see "Supported image size" :
16x16 to 65520x65520; step size 16 pixels

I interpret that that .min_width and .step_width should both be 16.
(.min_height and .step_height are correct at 16; if I read the TRM right)

> +		.ctrls = &rkvdec_h264_ctrls,
> +		.ops = &rkvdec_vdpu381_h264_fmt_ops,
> +		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_decoded_fmts),
> +		.decoded_fmts = rkvdec_h264_decoded_fmts,
> +		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
> +	},
> +};
> +
>  static const struct rkvdec_coded_fmt_desc *
>  rkvdec_find_coded_fmt_desc(struct rkvdec_ctx *ctx, u32 fourcc)
>  {
> @@ -1125,6 +1184,35 @@ static irqreturn_t rk3399_irq_handler(struct rkvdec_ctx *ctx)

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]