Re: [PATCH 00/12] media: rkvdec: Add support for VDPU381 and VDPU383

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

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

Le vendredi 18 juillet 2025 à 17:37 +0800, Jianfeng Liu a écrit :
> Hi,
> 
> On Mon, 14 Jul 2025 22:46:10 +0800, Jianfeng Liu wrote:
> > You are right, the code of chromium should be fixed for frame size type
> > V4L2_FRMSIZE_TYPE_CONTINUOUS.
> 
> I have just sent a cr at chromium[1] to fix this.
> 
> > I have checked that this issue is not introduced by your series. After
> > reverting this commit[2] which adds Support High 10 and 4:2:2 profiles,
> > chromium can play video well on rk3399. I will investigate further.
> 
> I found that this issue is caused by this code block[2]. Before adding
> .get_image_fmt, rkvdec_s_ctrl will just return 0. But now when detecting
> image format change(usually from RKVDEC_IMG_FMT_ANY to real video format
> like RKVDEC_IMG_FMT_420_8BIT), it will return -EBUSY, then I get green
> frame at chromium.
> 
> After taking a look at hantro's code, I find that it is not necessary to
> let .s_ctrl return -EBUSY when format changes, here is a commit[3]
> disabling this check in hantro_set_fmt_cap. I have written a patch that
> can fix my issue with chromium, you can see it at the bottom of my mail.
> 
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/6767118
> [2]
> https://github.com/torvalds/linux/blob/v6.16-rc6/drivers/staging/media/rkvdec/rkvdec.c#L143-L146
> [3]
> https://github.com/torvalds/linux/commit/bbd267daf4fc831f58bf4a2530a8b64881779e6a
> 
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> index 5d86fb7cdd6..7800d159fad 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> @@ -185,7 +185,6 @@ static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct
> rkvdec_ctx, ctrl_hdl);
>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>  	enum rkvdec_image_fmt image_fmt;
> -	struct vb2_queue *vq;
>  
>  	/* Check if this change requires a capture format reset */
>  	if (!desc->ops->get_image_fmt)
> @@ -193,11 +192,6 @@ static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>  	if (rkvdec_image_fmt_changed(ctx, image_fmt)) {
> -		vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> -				     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> -		if (vb2_is_busy(vq))
> -			return -EBUSY;
> -

Hantro driver have extra code to protect against the fact that the queue format
may not match the currently allocated buffer size. This change alone seems
unsafe and may allow tricking the driver into buffer overflow. It believe some
further thought and care need to be put into this.

Nicolas

>  		ctx->image_fmt = image_fmt;
>  		rkvdec_reset_decoded_fmt(ctx);
>  	}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]