Re: [PATCH v2 2/3] media: rkvdec: Add support for the VDPU346 variant

["Diederik de Haas" <diederik@cknow-tech.com>]

Hi Christian,

On Fri Dec 26, 2025 at 12:31 PM CET, Christian Hewitt wrote:
> VDPU346 is similar to VDPU381 but with a single core and limited
> to 4K60 media. It is also limited to H264 L5.1 and omits AV1 and
> AVS2 capabilities. VDPU346 is used with RK3566 and RK3568.
>
> Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
> Reviewed-by: Nicolas Dufresne <nicolas.collabora@collabora.com>
> ---
>  .../media/platform/rockchip/rkvdec/rkvdec.c   | 103 ++++++++++++++++++
>  1 file changed, 103 insertions(+)
>
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> index e547057dc75f..6b39e99d8a8b 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> @@ -236,6 +236,62 @@ static const struct rkvdec_ctrls rkvdec_hevc_ctrls = {
>  	.num_ctrls = ARRAY_SIZE(rkvdec_hevc_ctrl_descs),
>  };
>  
> +static const struct rkvdec_ctrl_desc vdpu346_hevc_ctrl_descs[] = {
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_DECODE_PARAMS,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_SPS,
> +		.cfg.ops = &rkvdec_ctrl_ops,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_PPS,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_SCALING_MATRIX,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_DECODE_MODE,
> +		.cfg.min = V4L2_STATELESS_HEVC_DECODE_MODE_FRAME_BASED,
> +		.cfg.max = V4L2_STATELESS_HEVC_DECODE_MODE_FRAME_BASED,
> +		.cfg.def = V4L2_STATELESS_HEVC_DECODE_MODE_FRAME_BASED,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_START_CODE,
> +		.cfg.min = V4L2_STATELESS_HEVC_START_CODE_ANNEX_B,
> +		.cfg.def = V4L2_STATELESS_HEVC_START_CODE_ANNEX_B,
> +		.cfg.max = V4L2_STATELESS_HEVC_START_CODE_ANNEX_B,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_MPEG_VIDEO_HEVC_PROFILE,
> +		.cfg.min = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN,
> +		.cfg.max = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_10,
> +		.cfg.menu_skip_mask =
> +			BIT(V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN_STILL_PICTURE),
> +		.cfg.def = V4L2_MPEG_VIDEO_HEVC_PROFILE_MAIN,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_MPEG_VIDEO_HEVC_LEVEL,
> +		.cfg.min = V4L2_MPEG_VIDEO_HEVC_LEVEL_1,
> +		.cfg.max = V4L2_MPEG_VIDEO_HEVC_LEVEL_5_1,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_EXT_SPS_ST_RPS,
> +		.cfg.ops = &rkvdec_ctrl_ops,
> +		.cfg.dims = { 65 },
> +	},
> +	{
> +		.cfg.id = V4L2_CID_STATELESS_HEVC_EXT_SPS_LT_RPS,
> +		.cfg.ops = &rkvdec_ctrl_ops,
> +		.cfg.dims = { 65 },
> +	},
> +};
> +
> +static const struct rkvdec_ctrls vdpu346_hevc_ctrls = {
> +	.ctrls = vdpu346_hevc_ctrl_descs,
> +	.num_ctrls = ARRAY_SIZE(vdpu346_hevc_ctrl_descs),
> +};
> +
>  static const struct rkvdec_ctrl_desc vdpu38x_hevc_ctrl_descs[] = {
>  	{
>  		.cfg.id = V4L2_CID_STATELESS_HEVC_DECODE_PARAMS,
> @@ -463,6 +519,41 @@ static const struct rkvdec_coded_fmt_desc rk3288_coded_fmts[] = {
>  	}
>  };
>  
> +static const struct rkvdec_coded_fmt_desc vdpu346_coded_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_HEVC_SLICE,
> +		.frmsize = {
> +			.min_width = 64,
> +			.max_width = 65472,

This should be 4096 according to page 469 of RK3568 TRM Part 2 ...

> +			.step_width = 64,
> +			.min_height = 64,
> +			.max_height = 65472,

... and this 2304.

> +			.step_height = 16,
> +		},
> +		.ctrls = &vdpu346_hevc_ctrls,
> +		.ops = &rkvdec_vdpu381_hevc_fmt_ops,
> +		.num_decoded_fmts = ARRAY_SIZE(rkvdec_hevc_decoded_fmts),
> +		.decoded_fmts = rkvdec_hevc_decoded_fmts,
> +		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_H264_SLICE,
> +		.frmsize = {
> +			.min_width = 64,
> +			.max_width =  65520,

This too should be 4096 according to page 469 of RK3568 TRM Part 2 ...

> +			.step_width = 64,
> +			.min_height = 64,
> +			.max_height =  65520,

... and this 2304.

I guess this also explains the 'green images' Nicolas noticed.

> +			.step_height = 16,
> +		},
> +		.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,
> +	},

I see you've reversed the order of the blocks so that HEVC now comes
before the H264 block. While that makes it consistent with what Detlev
has in their v7 and with the existing code in the driver ... I actually
prefer having H264 before HEVC as the alphabetical sorting order is
H264 before HEVC.
In the existing code the VP9 'stuff' is listed below H264 and HEVC.

But then Detlev should do that too in their patch set ... and 'ideally'
the order of the existing code be updated to be alphabetically too.

OTOH, a consistent order works for me too.

Cheers,
  Diederik

> +};
> +
>  static const struct rkvdec_coded_fmt_desc vdpu381_coded_fmts[] = {
>  	{
>  		.fourcc = V4L2_PIX_FMT_HEVC_SLICE,
> @@ -1643,6 +1734,14 @@ static const struct rkvdec_variant_ops vdpu381_variant_ops = {
>  	.flatten_matrices = transpose_and_flatten_matrices,
>  };
>  
> +static const struct rkvdec_variant vdpu346_variant = {
> +	.coded_fmts = vdpu346_coded_fmts,
> +	.num_coded_fmts = ARRAY_SIZE(vdpu346_coded_fmts),
> +	.rcb_sizes = vdpu381_rcb_sizes,
> +	.num_rcb_sizes = ARRAY_SIZE(vdpu381_rcb_sizes),
> +	.ops = &vdpu381_variant_ops,
> +};
> +
>  static const struct rkvdec_variant vdpu381_variant = {
>  	.coded_fmts = vdpu381_coded_fmts,
>  	.num_coded_fmts = ARRAY_SIZE(vdpu381_coded_fmts),
> @@ -1691,6 +1790,10 @@ static const struct of_device_id of_rkvdec_match[] = {
>  		.compatible = "rockchip,rk3399-vdec",
>  		.data = &rk3399_rkvdec_variant,
>  	},
> +	{
> +		.compatible = "rockchip,rk3568-vdec",
> +		.data = &vdpu346_variant,
> +	},
>  	{
>  		.compatible = "rockchip,rk3588-vdec",
>  		.data = &vdpu381_variant,