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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240315094706.xcpjy5s4fjtjvn7j@basti-XPS-13-9310>
Date: Fri, 15 Mar 2024 11:30:17 +0100
From: Sebastian Fricke <sebastian.fricke@...labora.com>
To: Yunfei Dong <yunfei.dong@...iatek.com>
Cc: NĂ­colas F . R . A . Prado <nfraprado@...labora.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Benjamin Gaignard <benjamin.gaignard@...labora.com>,
	Nathan Hebert <nhebert@...omium.org>,
	Hsin-Yi Wang <hsinyi@...omium.org>,
	Fritz Koenig <frkoenig@...omium.org>,
	Daniel Vetter <daniel@...ll.ch>, Steve Cho <stevecho@...omium.org>,
	linux-media@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH] media: mediatek: vcodec: add decoder command to support
 stateless decoder

Hey Yunfei,

On 15.03.2024 15:26, Yunfei Dong wrote:
>The supported decoder commands are different for stateless and
>stateful architecture. Adding stateless decoder commands to fix

s/Adding/Add/

>below v4l2-compliance test error.

s/below v4l2-compliance test error./the v4l2-compliance test error below./

>
>Codec ioctls:
>    VIDIOC_ENCODER_CMD returned -1 (Inappropriate ioctl for device)
>    VIDIOC_TRY_ENCODER_CMD returned -1 (Inappropriate ioctl for device)
> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
>    VIDIOC_G_ENC_INDEX returned -1 (Inappropriate ioctl for device)
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
>    VIDIOC_DECODER_CMD returned -1 (Invalid argument)
>    VIDIOC_TRY_DECODER_CMD returned -1 (Invalid argument)
>    VIDIOC_TRY_DECODER_CMD returned -1 (Invalid argument)
>    fail: v4l2-test-codecs.cpp(126): ret
> test VIDIOC_(TRY_)DECODER_CMD: FAIL
>
>Signed-off-by: Yunfei Dong <yunfei.dong@...iatek.com>
>---
> .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 65 +++++++++++++++++--
> 1 file changed, 59 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>index ba742f0e391d..90579dd92cae 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>@@ -80,21 +80,20 @@ static struct mtk_q_data *mtk_vdec_get_q_data(struct mtk_vcodec_dec_ctx *ctx,
> 	return &ctx->q_data[MTK_Q_DATA_DST];
> }
>
>-static int vidioc_try_decoder_cmd(struct file *file, void *priv,
>-				struct v4l2_decoder_cmd *cmd)
>+static int mtk_vcodec_stateful_try_decoder_cmd(struct file *file, void *priv,
>+					       struct v4l2_decoder_cmd *cmd)

In some cases you seem to name these functions with the prefix
`mtk_vdec` and sometimes with the prefix `mtk_vcodec` but all of these
are for decoders, so could you settle for one naming scheme? Also as
these functions are static I don't think it is strictly necessary to add
a prefix for each function.

> {
> 	return v4l2_m2m_ioctl_try_decoder_cmd(file, priv, cmd);
> }
>
>-
>-static int vidioc_decoder_cmd(struct file *file, void *priv,
>-				struct v4l2_decoder_cmd *cmd)
>+static int mtk_vcodec_stateful_decoder_cmd(struct file *file, void *priv,
>+					   struct v4l2_decoder_cmd *cmd)
> {
> 	struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
> 	struct vb2_queue *src_vq, *dst_vq;
> 	int ret;
>
>-	ret = vidioc_try_decoder_cmd(file, priv, cmd);
>+	ret = mtk_vcodec_stateful_try_decoder_cmd(file, priv, cmd);
> 	if (ret)
> 		return ret;
>
>@@ -128,6 +127,60 @@ static int vidioc_decoder_cmd(struct file *file, void *priv,
> 	return 0;
> }
>
>+static int mtk_vcodec_stateless_try_decoder_cmd(struct file *file, void *priv,
>+						struct v4l2_decoder_cmd *cmd)
>+{
>+	return v4l2_m2m_ioctl_stateless_try_decoder_cmd(file, priv, cmd);
>+}
>+
>+static int mtk_vcodec_stateless_decoder_cmd(struct file *file, void *priv,
>+					    struct v4l2_decoder_cmd *cmd)
>+{
>+	struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
>+	int ret;
>+
>+	ret = v4l2_m2m_ioctl_stateless_try_decoder_cmd(file, priv, cmd);
>+	if (ret)
>+		return ret;
>+
>+	mtk_v4l2_vdec_dbg(3, ctx, "decoder cmd=%u", cmd->cmd);
>+	switch (cmd->cmd) {
>+	case V4L2_DEC_CMD_FLUSH:
>+		/*
>+		 * If the flag of output buffer is set with V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF,

s/output/the output/
s/is set with/equals/

>+		 * this command will prevent dequeueing the capture buffer containing the last
>+		 * decoded frame. Or do nothing
>+		 */
>+		break;
>+

Please remove this newline.

>+	default:
>+		mtk_v4l2_vdec_err(ctx, "invalid stateless decoder cmd=%u", cmd->cmd);
>+		return -EINVAL;
>+	}
>+
>+	return 0;
>+}
>+
>+static int vidioc_try_decoder_cmd(struct file *file, void *priv, struct v4l2_decoder_cmd *cmd)
>+{
>+	struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
>+
>+	if (ctx->dev->vdec_pdata->uses_stateless_api)
>+		return mtk_vcodec_stateless_try_decoder_cmd(file, priv, cmd);
>+	else

As these conditional branches contain return statements you can skip the
else.
E.g.
	if (ctx->dev->vdec_pdata->uses_stateless_api)
		return mtk_vcodec_stateless_try_decoder_cmd(file, priv, cmd);
   return mtk_vcodec_stateful_try_decoder_cmd(file, priv, cmd);

>+		return mtk_vcodec_stateful_try_decoder_cmd(file, priv, cmd);
>+}
>+
>+static int vidioc_decoder_cmd(struct file *file, void *priv, struct v4l2_decoder_cmd *cmd)
>+{
>+	struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
>+
>+	if (ctx->dev->vdec_pdata->uses_stateless_api)
>+		return mtk_vcodec_stateless_decoder_cmd(file, priv, cmd);
>+	else

Same as mentioned above.

>+		return mtk_vcodec_stateful_decoder_cmd(file, priv, cmd);
>+}
>+
> void mtk_vdec_unlock(struct mtk_vcodec_dec_ctx *ctx)
> {
> 	mutex_unlock(&ctx->dev->dec_mutex[ctx->hw_id]);
>-- 
>2.18.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ