Re: [PATCH v3 2/4] media: rockchip: Introduce the rkvdec2 driver

[Alex Bee <knaerzche@gmail.com>]

Hi Detlev,

Am 20.06.24 um 16:19 schrieb Detlev Casanova:
> This driver supports the second generation of the Rockchip Video
> decoder, also known as vdpu34x.
> It is currently only used on the RK3588(s) SoC.
>
> There are 2 decoders on the RK3588 SoC that can work in pair to decode
> 8K video at 30 FPS but currently, only using one core at a time is
> supported.
>
> Scheduling requests between the two cores will be implemented later.
>
> The core supports H264, HEVC, VP9 and AVS2 decoding but this driver
> currently only supports H264.
>
> The driver is based on rkvdec and they may share some code in the
> future.
> The decision to make a different driver is mainly because rkvdec2 has
> more features and can work with multiple cores.
>
> The registers are mapped in a struct in RAM using bitfields. It is IO
> copied to the HW when all values are configured.
> The decision to use such a struct instead of writing buffers one by one
> is based on the following reasons:
>   - Rockchip cores are known to misbehave when registers are not written
>     in address order,
>   - Those cores also need the software to write all registers, even if
>     they are written their default values or are not related to the task
>     (this core will not start decoding some H264 frames if some VP9
>     registers are not written to 0)
>   - In the future, to support multiple cores, the scheduler could be
>     optimized by storing the precomputed registers values and copy them
>     to the HW as soos as a core becomes available.
>
> This makes the code more readable and may bring performance improvements
> in future features.
>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>   drivers/staging/media/Kconfig                |    1 +
>   drivers/staging/media/Makefile               |    1 +
>   drivers/staging/media/rkvdec2/Kconfig        |   15 +
>   drivers/staging/media/rkvdec2/Makefile       |    3 +
>   drivers/staging/media/rkvdec2/TODO           |    9 +
>   drivers/staging/media/rkvdec2/rkvdec2-h264.c |  739 +++++++++++
>   drivers/staging/media/rkvdec2/rkvdec2-regs.h |  345 +++++
>   drivers/staging/media/rkvdec2/rkvdec2.c      | 1253 ++++++++++++++++++
>   drivers/staging/media/rkvdec2/rkvdec2.h      |  130 ++
>   9 files changed, 2496 insertions(+)
>   create mode 100644 drivers/staging/media/rkvdec2/Kconfig
>   create mode 100644 drivers/staging/media/rkvdec2/Makefile
>   create mode 100644 drivers/staging/media/rkvdec2/TODO
>   create mode 100644 drivers/staging/media/rkvdec2/rkvdec2-h264.c
>   create mode 100644 drivers/staging/media/rkvdec2/rkvdec2-regs.h
>   create mode 100644 drivers/staging/media/rkvdec2/rkvdec2.c
>   create mode 100644 drivers/staging/media/rkvdec2/rkvdec2.h
...
> +static inline void rkvdec2_memcpy_toio(void __iomem *dst, void *src, size_t len)
> +{
> +#ifdef CONFIG_ARM64
> +	__iowrite32_copy(dst, src, len);
> +#elif defined(CONFIG_ARM)
I guess that can get an "#else" since memcpy_toio exists for all archs.
> +	memcpy_toio(dst, src, len);
> +#endif
> +}
> +
...
> +	/* Set timeout threshold */
> +	if (pixels < RKVDEC2_1080P_PIXELS)
> +		regs->common.timeout_threshold = RKVDEC2_TIMEOUT_1080p;
> +	else if (pixels < RKVDEC2_4K_PIXELS)
> +		regs->common.timeout_threshold = RKVDEC2_TIMEOUT_4K;
> +	else if (pixels < RKVDEC2_8K_PIXELS)
> +		regs->common.timeout_threshold = RKVDEC2_TIMEOUT_8K;
> +

Did you test if it works with anything > 8K? If so, you propably want to 
make the check above

+	else
+		regs->common.timeout_threshold = RKVDEC2_TIMEOUT_8K;

Otherwise the timeout may not be set/contain invalid values from any former stream.

...

> +
> +static const struct rkvdec2_coded_fmt_desc rkvdec2_coded_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_H264_SLICE,
> +		.frmsize = {
> +			.min_width = 16,
> +			.max_width =  65520,
> +			.step_width = 16,
> +			.min_height = 16,
> +			.max_height =  65520,
> +			.step_height = 16,
> +		},
> +		.ctrls = &rkvdec2_h264_ctrls,
> +		.ops = &rkvdec2_h264_fmt_ops,
> +		.num_decoded_fmts = ARRAY_SIZE(rkvdec2_h264_decoded_fmts),
> +		.decoded_fmts = rkvdec2_h264_decoded_fmts,
> +		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
> +	},
> +};
> +
Note, that this is also given to userspace (VIDIOC_ENUM_FRAMESIZES) and 
this is already incorrect in the old rkvdec driver (and hantro): From 
userspace perspective we do not have a restriction in 
step_width/step_width, as we are aligning any given width/height to HW 
requirements in the driver - what we should give to userspace is 
fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; fsize->stepwise.min_height = 
1; fsize->stepwise.min_width = 1; fsize->stepwise.max_height = 65520; 
fsize->stepwise.max_width = 65520; I guess this new driver should be an 
opportunity to fix that and distinguish between internal and external 
frame size requirements and the .vidioc_enum_framesizes callback should 
adapted accordingly. Regards, Alex