Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 30 Jun 2022 07:02:32 +0200
From:   Sebastian Fricke <sebastian.fricke@collabora.com>
To:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Nicolas Dufresne <nicolas.dufresne@collabora.com>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>
Subject: Re: [PATCH] hantro: Remove incorrect HEVC SPS validation
Message-ID: <20220630050232.bpntbghouslye3l3@basti-XPS-13-9310>
References: <20220629195624.45745-1-ezequiel@vanguardiasur.com.ar>
 <20220629195624.45745-2-ezequiel@vanguardiasur.com.ar>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
In-Reply-To: <20220629195624.45745-2-ezequiel@vanguardiasur.com.ar>
Precedence: bulk

Hey Ezequiel,

On 29.06.2022 16:56, Ezequiel Garcia wrote:
>Currently, the driver tries to validat the HEVC SPS

s/validat/validate/

>against the CAPTURE queue format (i.e. the decoded format).
>This is not correct, because typically the SPS control is set
>before the CAPTURE queue is negotiated.
>
>In addition to this, a format validation in hantro_hevc_dec_prepare_run()
>is also suboptimal, because hantro_hevc_dec_prepare_run() runs in the context
>of v4l2_m2m_ops.device_run, as part of a decoding job.
>
>Format and control validations should happen before decoding starts,
>in the context of ioctls such as S_CTRL, S_FMT, or STREAMON.
>
>Remove the validation for now.

Couldn't we add a small wrapper around STREAMON to perform that
validation? I feel like "remove the validation for now", seems like a
vague statement.

Greetings,
Sebastian

>
>Fixes: 135ad96cb4d6b ("media: hantro: Be more accurate on pixel formats step_width constraints")
>Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>---
> drivers/staging/media/hantro/hantro_drv.c  | 12 ++++-----
> drivers/staging/media/hantro/hantro_hevc.c | 30 ----------------------
> drivers/staging/media/hantro/hantro_hw.h   |  1 -
> 3 files changed, 6 insertions(+), 37 deletions(-)
>
>diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
>index afddf7ac0731..2387ca85ab54 100644
>--- a/drivers/staging/media/hantro/hantro_drv.c
>+++ b/drivers/staging/media/hantro/hantro_drv.c
>@@ -253,11 +253,6 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>
> static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> {
>-	struct hantro_ctx *ctx;
>-
>-	ctx = container_of(ctrl->handler,
>-			   struct hantro_ctx, ctrl_handler);
>-
> 	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
> 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>
>@@ -273,7 +268,12 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> 	} else if (ctrl->id == V4L2_CID_MPEG_VIDEO_HEVC_SPS) {
> 		const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
>
>-		return hantro_hevc_validate_sps(ctx, sps);
>+		if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
>+			/* Luma and chroma bit depth mismatch */
>+			return -EINVAL;
>+		if (sps->bit_depth_luma_minus8 != 0)
>+			/* Only 8-bit is supported */
>+			return -EINVAL;
> 	} else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
> 		const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame;
>
>diff --git a/drivers/staging/media/hantro/hantro_hevc.c b/drivers/staging/media/hantro/hantro_hevc.c
>index bd924896e409..f86c98e19177 100644
>--- a/drivers/staging/media/hantro/hantro_hevc.c
>+++ b/drivers/staging/media/hantro/hantro_hevc.c
>@@ -154,32 +154,6 @@ static int tile_buffer_reallocate(struct hantro_ctx *ctx)
> 	return -ENOMEM;
> }
>
>-int hantro_hevc_validate_sps(struct hantro_ctx *ctx, const struct v4l2_ctrl_hevc_sps *sps)
>-{
>-	if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
>-		/* Luma and chroma bit depth mismatch */
>-		return -EINVAL;
>-	if (sps->bit_depth_luma_minus8 != 0)
>-		/* Only 8-bit is supported */
>-		return -EINVAL;
>-
>-	/*
>-	 * for tile pixel format check if the width and height match
>-	 * hardware constraints
>-	 */
>-	if (ctx->vpu_dst_fmt->fourcc == V4L2_PIX_FMT_NV12_4L4) {
>-		if (ctx->dst_fmt.width !=
>-		    ALIGN(sps->pic_width_in_luma_samples, ctx->vpu_dst_fmt->frmsize.step_width))
>-			return -EINVAL;
>-
>-		if (ctx->dst_fmt.height !=
>-		    ALIGN(sps->pic_height_in_luma_samples, ctx->vpu_dst_fmt->frmsize.step_height))
>-			return -EINVAL;
>-	}
>-
>-	return 0;
>-}
>-
> int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx)
> {
> 	struct hantro_hevc_dec_hw_ctx *hevc_ctx = &ctx->hevc_dec;
>@@ -203,10 +177,6 @@ int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx)
> 	if (WARN_ON(!ctrls->sps))
> 		return -EINVAL;
>
>-	ret = hantro_hevc_validate_sps(ctx, ctrls->sps);
>-	if (ret)
>-		return ret;
>-
> 	ctrls->pps =
> 		hantro_get_ctrl(ctx, V4L2_CID_MPEG_VIDEO_HEVC_PPS);
> 	if (WARN_ON(!ctrls->pps))
>diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
>index a2e0f0836281..5edff0f0be20 100644
>--- a/drivers/staging/media/hantro/hantro_hw.h
>+++ b/drivers/staging/media/hantro/hantro_hw.h
>@@ -359,7 +359,6 @@ int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx);
> void hantro_hevc_ref_init(struct hantro_ctx *ctx);
> dma_addr_t hantro_hevc_get_ref_buf(struct hantro_ctx *ctx, int poc);
> int hantro_hevc_add_ref_buf(struct hantro_ctx *ctx, int poc, dma_addr_t addr);
>-int hantro_hevc_validate_sps(struct hantro_ctx *ctx, const struct v4l2_ctrl_hevc_sps *sps);
>
>
> static inline unsigned short hantro_vp9_num_sbs(unsigned short dimension)
>-- 
>2.31.1
>