linux-kernel - Re: [PATCH v8 2/3] media: rkvdec: Add get_image

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <dd9ec68a0a36366b85c65060da050e794ed6948c.camel@collabora.com>
Date: Thu, 17 Apr 2025 16:03:24 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>, Mauro Carvalho Chehab
	 <mchehab@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org, 
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	kernel@...labora.com, Jonas Karlman <jonas@...boo.se>, Christopher Obbard
	 <christopher.obbard@...aro.org>
Subject: Re: [PATCH v8 2/3] media: rkvdec: Add get_image_fmt ops

Hi,

Le jeudi 17 avril 2025 à 13:14 -0400, Nicolas Dufresne a écrit :
> From: Jonas Karlman <jonas@...boo.se>
> 
> Add support for a get_image_fmt() ops that returns the required image
> format.
> 
> The CAPTURE format is reset when the required image format changes and
> the buffer queue is not busy.
> 
> Signed-off-by: Jonas Karlman <jonas@...boo.se>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Tested-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Tested-by: Christopher Obbard <chris.obbard@...labora.com>
> Co-developed-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 44 +++++++++++++++++++++++++++++++++--
>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 65c6f1d07a493e017ae941780b823d41314a49b8..db01cc64f1ba2dcd5914537db41e2f51f454b186 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -72,6 +72,15 @@ static bool rkvdec_is_valid_fmt(struct rkvdec_ctx *ctx, u32 fourcc,
>  	return false;
>  }
>  
> +static bool rkvdec_fmt_changed(struct rkvdec_ctx *ctx,
> +			       enum rkvdec_image_fmt image_fmt)
> +{
> +	if (image_fmt == RKVDEC_IMG_FMT_ANY)
> +		return false;
> +
> +	return ctx->image_fmt != image_fmt;
> +}
> +
>  static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
>  				       struct v4l2_pix_format_mplane *pix_mp)
>  {
> @@ -111,15 +120,46 @@ static int rkvdec_try_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;
> +	int ret;
> +
> +	if (desc->ops->try_ctrl) {
> +		ret = desc->ops->try_ctrl(ctx, ctrl);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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;
>  
> -	if (desc->ops->try_ctrl)
> -		return desc->ops->try_ctrl(ctx, ctrl);
> +	/* Check if this change requires a capture format reset */
> +	if (!desc->ops->get_image_fmt)
> +		return 0;
> +
> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> +	if (rkvdec_fmt_changed(ctx, image_fmt)) {
> +		/* Format changes are not allowed when capture queue is busy */
> +		vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> +				     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +		if (vb2_is_busy(vq))
> +			return -EINVAL;

Sad to say, but this patch is bad, I've been testing the wrong kernel
all along. This condition is reached before the queue is set (while the
default control value is set). At the point, vq is null. I also came to
realize the rkvdec_fmt_changed() is quite similar to the format match.
I also assumed that if ctx->image_fmt is ANY, we should always reset,
that's why the old code didn't not stumble on this issue.

Please disregard this series, I'll make a v9.

Nicolas

> +
> +		ctx->image_fmt = image_fmt;
> +		rkvdec_reset_decoded_fmt(ctx);
> +	}
>  
>  	return 0;
>  }
>  
>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>  	.try_ctrl = rkvdec_try_ctrl,
> +	.s_ctrl = rkvdec_s_ctrl,
>  };
>  
>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.h
> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>  		     struct vb2_v4l2_buffer *dst_buf,
>  		     enum vb2_buffer_state result);
>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
> +					       struct v4l2_ctrl *ctrl);
>  };
>  
>  enum rkvdec_image_fmt {

-- 
Nicolas Dufresne
Principal Engineer at Collabora