[PATCH] hantro: Fix RK3399 H.264 format advertising

[PATCH] hantro: Fix RK3399 H.264 format advertising

[Ezequiel Garcia]

Commit 1f82f2df523cb ("media: hantro: Enable H.264 on Rockchip VDPU2")
enabled H.264 on some SoCs with VDPU2 cores. This had the side-effect
of exposing H.264 coded format as supported on RK3399.

Fix this and clarify how the codec is explicitly disabled on RK399 on
this driver.

Fixes: 1f82f2df523cb ("media: hantro: Enable H.264 on Rockchip VDPU2")
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 .../staging/media/hantro/rockchip_vpu_hw.c    | 60 ++++++++++++++++---
 1 file changed, 53 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c
index 098486b9ec27..26e16b5a6a70 100644
--- a/drivers/staging/media/hantro/rockchip_vpu_hw.c
+++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c
@@ -182,7 +182,7 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
 	},
 };
 
-static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
+static const struct hantro_fmt rockchip_vdpu2_dec_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_NV12,
 		.codec_mode = HANTRO_MODE_NONE,
@@ -236,6 +236,47 @@ static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
 	},
 };
 
+static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12,
+		.codec_mode = HANTRO_MODE_NONE,
+		.frmsize = {
+			.min_width = FMT_MIN_WIDTH,
+			.max_width = FMT_FHD_WIDTH,
+			.step_width = MB_DIM,
+			.min_height = FMT_MIN_HEIGHT,
+			.max_height = FMT_FHD_HEIGHT,
+			.step_height = MB_DIM,
+		},
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_MPEG2_SLICE,
+		.codec_mode = HANTRO_MODE_MPEG2_DEC,
+		.max_depth = 2,
+		.frmsize = {
+			.min_width = FMT_MIN_WIDTH,
+			.max_width = FMT_FHD_WIDTH,
+			.step_width = MB_DIM,
+			.min_height = FMT_MIN_HEIGHT,
+			.max_height = FMT_FHD_HEIGHT,
+			.step_height = MB_DIM,
+		},
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_VP8_FRAME,
+		.codec_mode = HANTRO_MODE_VP8_DEC,
+		.max_depth = 2,
+		.frmsize = {
+			.min_width = FMT_MIN_WIDTH,
+			.max_width = FMT_UHD_WIDTH,
+			.step_width = MB_DIM,
+			.min_height = FMT_MIN_HEIGHT,
+			.max_height = FMT_UHD_HEIGHT,
+			.step_height = MB_DIM,
+		},
+	},
+};
+
 static irqreturn_t rockchip_vpu1_vepu_irq(int irq, void *dev_id)
 {
 	struct hantro_dev *vpu = dev_id;
@@ -548,8 +589,8 @@ const struct hantro_variant rk3288_vpu_variant = {
 
 const struct hantro_variant rk3328_vpu_variant = {
 	.dec_offset = 0x400,
-	.dec_fmts = rk3399_vpu_dec_fmts,
-	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
+	.dec_fmts = rockchip_vdpu2_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
 	.codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
 		 HANTRO_H264_DECODER,
 	.codec_ops = rk3399_vpu_codec_ops,
@@ -560,6 +601,11 @@ const struct hantro_variant rk3328_vpu_variant = {
 	.num_clocks = ARRAY_SIZE(rockchip_vpu_clk_names),
 };
 
+/*
+ * H.264 decoding explicitly disabled in RK3399.
+ * This ensures userspace applications use the Rockchip VDEC core,
+ * which has better performance.
+ */
 const struct hantro_variant rk3399_vpu_variant = {
 	.enc_offset = 0x0,
 	.enc_fmts = rockchip_vpu_enc_fmts,
@@ -579,8 +625,8 @@ const struct hantro_variant rk3399_vpu_variant = {
 
 const struct hantro_variant rk3568_vpu_variant = {
 	.dec_offset = 0x400,
-	.dec_fmts = rk3399_vpu_dec_fmts,
-	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
+	.dec_fmts = rockchip_vdpu2_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
 	.codec = HANTRO_MPEG2_DECODER |
 		 HANTRO_VP8_DECODER | HANTRO_H264_DECODER,
 	.codec_ops = rk3399_vpu_codec_ops,
@@ -596,8 +642,8 @@ const struct hantro_variant px30_vpu_variant = {
 	.enc_fmts = rockchip_vpu_enc_fmts,
 	.num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
 	.dec_offset = 0x400,
-	.dec_fmts = rk3399_vpu_dec_fmts,
-	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
+	.dec_fmts = rockchip_vdpu2_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
 	.codec = HANTRO_JPEG_ENCODER | HANTRO_MPEG2_DECODER |
 		 HANTRO_VP8_DECODER | HANTRO_H264_DECODER,
 	.codec_ops = rk3399_vpu_codec_ops,
-- 
2.31.1

[PATCH] hantro: Remove incorrect HEVC SPS validation

[Ezequiel Garcia]

Currently, the driver tries to validat the HEVC SPS
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.

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

Re: [PATCH] hantro: Remove incorrect HEVC SPS validation

[Sebastian Fricke]

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
>

Re: [PATCH] hantro: Remove incorrect HEVC SPS validation

[Hans Verkuil]

On 6/30/22 07:02, Sebastian Fricke wrote:
> 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.

I agree. Basically two things happen in this patch: two sanity checks
for the SPS control are moved to try_ctrl, and that part looks good.

So that can be a separate patch.

The second part is the removal of the format+control validation, but it
is not clear why removing it altogether is wrong. Shouldn't it still be
done somewhere? And if not, why not?

Regards,

	Hans

> 
> 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
>>

Re: [PATCH] hantro: Remove incorrect HEVC SPS validation

[Ezequiel Garcia]

Hi Hans,

On Fri, Jul 08, 2022 at 10:42:43AM +0200, Hans Verkuil wrote:
> 
> 
> On 6/30/22 07:02, Sebastian Fricke wrote:
> > 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.
> 
> I agree. Basically two things happen in this patch: two sanity checks
> for the SPS control are moved to try_ctrl, and that part looks good.
> 
> So that can be a separate patch.
> 
> The second part is the removal of the format+control validation, but it
> is not clear why removing it altogether is wrong. Shouldn't it still be
> done somewhere? And if not, why not?
> 

Sorry for the delayed reply. After giving this format+control validation
more thought, I believe the V4L2 API provides a natural way of handling.

When the format is negotiated in TRY_FMT, the control needs to be used
to offer a valid resolution.

The driver is capable of negotiating the resolution of the stream
using the control information, instead of failing.

I'll send a v2 with a new proposal.

Thanks,
Ezequiel

> Regards,
> 
> 	Hans
> 
> > 
> > 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
> >>

Re: [PATCH] hantro: Fix RK3399 H.264 format advertising

[Nicolas Dufresne]

Hi Ezequiel,

Le mercredi 29 juin 2022 à 16:56 -0300, Ezequiel Garcia a écrit :
> Commit 1f82f2df523cb ("media: hantro: Enable H.264 on Rockchip VDPU2")
> enabled H.264 on some SoCs with VDPU2 cores. This had the side-effect
> of exposing H.264 coded format as supported on RK3399.
> 
> Fix this and clarify how the codec is explicitly disabled on RK399 on

RK399 -> RK3399

> this driver.
> 
> Fixes: 1f82f2df523cb ("media: hantro: Enable H.264 on Rockchip VDPU2")
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks for fixing this. The bogus format (without associated controls) is not
gone. With the right video node value:

  v4l2-ctl -D -dN --list-formats-out

Tested-By: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  .../staging/media/hantro/rockchip_vpu_hw.c    | 60 ++++++++++++++++---
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> index 098486b9ec27..26e16b5a6a70 100644
> --- a/drivers/staging/media/hantro/rockchip_vpu_hw.c
> +++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> @@ -182,7 +182,7 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
>  	},
>  };
>  
> -static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
> +static const struct hantro_fmt rockchip_vdpu2_dec_fmts[] = {
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.codec_mode = HANTRO_MODE_NONE,
> @@ -236,6 +236,47 @@ static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
>  	},
>  };
>  
> +static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV12,
> +		.codec_mode = HANTRO_MODE_NONE,
> +		.frmsize = {
> +			.min_width = FMT_MIN_WIDTH,
> +			.max_width = FMT_FHD_WIDTH,
> +			.step_width = MB_DIM,
> +			.min_height = FMT_MIN_HEIGHT,
> +			.max_height = FMT_FHD_HEIGHT,
> +			.step_height = MB_DIM,
> +		},
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_MPEG2_SLICE,
> +		.codec_mode = HANTRO_MODE_MPEG2_DEC,
> +		.max_depth = 2,
> +		.frmsize = {
> +			.min_width = FMT_MIN_WIDTH,
> +			.max_width = FMT_FHD_WIDTH,
> +			.step_width = MB_DIM,
> +			.min_height = FMT_MIN_HEIGHT,
> +			.max_height = FMT_FHD_HEIGHT,
> +			.step_height = MB_DIM,
> +		},
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_VP8_FRAME,
> +		.codec_mode = HANTRO_MODE_VP8_DEC,
> +		.max_depth = 2,
> +		.frmsize = {
> +			.min_width = FMT_MIN_WIDTH,
> +			.max_width = FMT_UHD_WIDTH,
> +			.step_width = MB_DIM,
> +			.min_height = FMT_MIN_HEIGHT,
> +			.max_height = FMT_UHD_HEIGHT,
> +			.step_height = MB_DIM,
> +		},
> +	},
> +};
> +
>  static irqreturn_t rockchip_vpu1_vepu_irq(int irq, void *dev_id)
>  {
>  	struct hantro_dev *vpu = dev_id;
> @@ -548,8 +589,8 @@ const struct hantro_variant rk3288_vpu_variant = {
>  
>  const struct hantro_variant rk3328_vpu_variant = {
>  	.dec_offset = 0x400,
> -	.dec_fmts = rk3399_vpu_dec_fmts,
> -	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
> +	.dec_fmts = rockchip_vdpu2_dec_fmts,
> +	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
>  	.codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
>  		 HANTRO_H264_DECODER,
>  	.codec_ops = rk3399_vpu_codec_ops,
> @@ -560,6 +601,11 @@ const struct hantro_variant rk3328_vpu_variant = {
>  	.num_clocks = ARRAY_SIZE(rockchip_vpu_clk_names),
>  };
>  
> +/*
> + * H.264 decoding explicitly disabled in RK3399.
> + * This ensures userspace applications use the Rockchip VDEC core,
> + * which has better performance.
> + */
>  const struct hantro_variant rk3399_vpu_variant = {
>  	.enc_offset = 0x0,
>  	.enc_fmts = rockchip_vpu_enc_fmts,
> @@ -579,8 +625,8 @@ const struct hantro_variant rk3399_vpu_variant = {
>  
>  const struct hantro_variant rk3568_vpu_variant = {
>  	.dec_offset = 0x400,
> -	.dec_fmts = rk3399_vpu_dec_fmts,
> -	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
> +	.dec_fmts = rockchip_vdpu2_dec_fmts,
> +	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
>  	.codec = HANTRO_MPEG2_DECODER |
>  		 HANTRO_VP8_DECODER | HANTRO_H264_DECODER,
>  	.codec_ops = rk3399_vpu_codec_ops,
> @@ -596,8 +642,8 @@ const struct hantro_variant px30_vpu_variant = {
>  	.enc_fmts = rockchip_vpu_enc_fmts,
>  	.num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
>  	.dec_offset = 0x400,
> -	.dec_fmts = rk3399_vpu_dec_fmts,
> -	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
> +	.dec_fmts = rockchip_vdpu2_dec_fmts,
> +	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
>  	.codec = HANTRO_JPEG_ENCODER | HANTRO_MPEG2_DECODER |
>  		 HANTRO_VP8_DECODER | HANTRO_H264_DECODER,
>  	.codec_ops = rk3399_vpu_codec_ops,

And code looks good (just found a small commit message typo).

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

regards,
Nicolas

Re: [PATCH] hantro: Fix RK3399 H.264 format advertising

[Nicolas Dufresne]

Le mercredi 29 juin 2022 à 16:07 -0400, Nicolas Dufresne a écrit :
> Hi Ezequiel,
> 
> Le mercredi 29 juin 2022 à 16:56 -0300, Ezequiel Garcia a écrit :
> > Commit 1f82f2df523cb ("media: hantro: Enable H.264 on Rockchip VDPU2")
> > enabled H.264 on some SoCs with VDPU2 cores. This had the side-effect
> > of exposing H.264 coded format as supported on RK3399.
> > 
> > Fix this and clarify how the codec is explicitly disabled on RK399 on
> 
> RK399 -> RK3399
> 
> > this driver.
> > 
> > Fixes: 1f82f2df523cb ("media: hantro: Enable H.264 on Rockchip VDPU2")
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> Thanks for fixing this. The bogus format (without associated controls) is not

Sorry:
* is now

> gone. With the right video node value:
> 
>   v4l2-ctl -D -dN --list-formats-out
> 
> Tested-By: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> > ---
> >  .../staging/media/hantro/rockchip_vpu_hw.c    | 60 ++++++++++++++++---
> >  1 file changed, 53 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> > index 098486b9ec27..26e16b5a6a70 100644
> > --- a/drivers/staging/media/hantro/rockchip_vpu_hw.c
> > +++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> > @@ -182,7 +182,7 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> >  	},
> >  };
> >  
> > -static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
> > +static const struct hantro_fmt rockchip_vdpu2_dec_fmts[] = {
> >  	{
> >  		.fourcc = V4L2_PIX_FMT_NV12,
> >  		.codec_mode = HANTRO_MODE_NONE,
> > @@ -236,6 +236,47 @@ static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
> >  	},
> >  };
> >  
> > +static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_NV12,
> > +		.codec_mode = HANTRO_MODE_NONE,
> > +		.frmsize = {
> > +			.min_width = FMT_MIN_WIDTH,
> > +			.max_width = FMT_FHD_WIDTH,
> > +			.step_width = MB_DIM,
> > +			.min_height = FMT_MIN_HEIGHT,
> > +			.max_height = FMT_FHD_HEIGHT,
> > +			.step_height = MB_DIM,
> > +		},
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_MPEG2_SLICE,
> > +		.codec_mode = HANTRO_MODE_MPEG2_DEC,
> > +		.max_depth = 2,
> > +		.frmsize = {
> > +			.min_width = FMT_MIN_WIDTH,
> > +			.max_width = FMT_FHD_WIDTH,
> > +			.step_width = MB_DIM,
> > +			.min_height = FMT_MIN_HEIGHT,
> > +			.max_height = FMT_FHD_HEIGHT,
> > +			.step_height = MB_DIM,
> > +		},
> > +	},
> > +	{
> > +		.fourcc = V4L2_PIX_FMT_VP8_FRAME,
> > +		.codec_mode = HANTRO_MODE_VP8_DEC,
> > +		.max_depth = 2,
> > +		.frmsize = {
> > +			.min_width = FMT_MIN_WIDTH,
> > +			.max_width = FMT_UHD_WIDTH,
> > +			.step_width = MB_DIM,
> > +			.min_height = FMT_MIN_HEIGHT,
> > +			.max_height = FMT_UHD_HEIGHT,
> > +			.step_height = MB_DIM,
> > +		},
> > +	},
> > +};
> > +
> >  static irqreturn_t rockchip_vpu1_vepu_irq(int irq, void *dev_id)
> >  {
> >  	struct hantro_dev *vpu = dev_id;
> > @@ -548,8 +589,8 @@ const struct hantro_variant rk3288_vpu_variant = {
> >  
> >  const struct hantro_variant rk3328_vpu_variant = {
> >  	.dec_offset = 0x400,
> > -	.dec_fmts = rk3399_vpu_dec_fmts,
> > -	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
> > +	.dec_fmts = rockchip_vdpu2_dec_fmts,
> > +	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
> >  	.codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> >  		 HANTRO_H264_DECODER,
> >  	.codec_ops = rk3399_vpu_codec_ops,
> > @@ -560,6 +601,11 @@ const struct hantro_variant rk3328_vpu_variant = {
> >  	.num_clocks = ARRAY_SIZE(rockchip_vpu_clk_names),
> >  };
> >  
> > +/*
> > + * H.264 decoding explicitly disabled in RK3399.
> > + * This ensures userspace applications use the Rockchip VDEC core,
> > + * which has better performance.
> > + */
> >  const struct hantro_variant rk3399_vpu_variant = {
> >  	.enc_offset = 0x0,
> >  	.enc_fmts = rockchip_vpu_enc_fmts,
> > @@ -579,8 +625,8 @@ const struct hantro_variant rk3399_vpu_variant = {
> >  
> >  const struct hantro_variant rk3568_vpu_variant = {
> >  	.dec_offset = 0x400,
> > -	.dec_fmts = rk3399_vpu_dec_fmts,
> > -	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
> > +	.dec_fmts = rockchip_vdpu2_dec_fmts,
> > +	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
> >  	.codec = HANTRO_MPEG2_DECODER |
> >  		 HANTRO_VP8_DECODER | HANTRO_H264_DECODER,
> >  	.codec_ops = rk3399_vpu_codec_ops,
> > @@ -596,8 +642,8 @@ const struct hantro_variant px30_vpu_variant = {
> >  	.enc_fmts = rockchip_vpu_enc_fmts,
> >  	.num_enc_fmts = ARRAY_SIZE(rockchip_vpu_enc_fmts),
> >  	.dec_offset = 0x400,
> > -	.dec_fmts = rk3399_vpu_dec_fmts,
> > -	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
> > +	.dec_fmts = rockchip_vdpu2_dec_fmts,
> > +	.num_dec_fmts = ARRAY_SIZE(rockchip_vdpu2_dec_fmts),
> >  	.codec = HANTRO_JPEG_ENCODER | HANTRO_MPEG2_DECODER |
> >  		 HANTRO_VP8_DECODER | HANTRO_H264_DECODER,
> >  	.codec_ops = rk3399_vpu_codec_ops,
> 
> And code looks good (just found a small commit message typo).
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> regards,
> Nicolas
> 
>