| 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
| ||
|
Message-ID: <db9149100bbed9efecc54d9a39dc57862d49cd23.camel@collabora.com>
Date: Tue, 17 May 2022 13:42:11 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>,
ezequiel@...guardiasur.com.ar, p.zabel@...gutronix.de,
mchehab@...nel.org, gregkh@...uxfoundation.org,
shawnguo@...nel.org, s.hauer@...gutronix.de, festevam@...il.com,
linux-imx@....com, heiko@...ech.de, wens@...e.org,
jernej.skrabec@...il.com, samuel@...lland.org
Cc: kernel@...gutronix.de, linux-media@...r.kernel.org,
linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-sunxi@...ts.linux.dev, kernel@...labora.com
Subject: Re: [PATCH v2] media: hantro: Be more accurate on pixel formats
step_width constraints
Le mardi 17 mai 2022 à 14:26 +0200, Benjamin Gaignard a écrit :
> On Hantro G2 decoder on IMX8MQ strides requirements aren't the same
> for NV12_4L4 and NV12 pixel formats. The first one use a 4 bytes padding
> while the last one needs 8 bytes.
> To be sure to provide the correct stride in all cases we need:
> - to relax the constraints on codec formats so set step_width to 4
> - use capture queue format and not the output queue format when applying
> the pixel format constraints.
> - put the correct step_width constraints on each pixel format.
>
> Move HEVC SPS validation in hantro_hevc.c to be able to perform it
> when setting sps control and when starting to decode the bitstream.
> Add a new test in HEVC SPS validation function to check if resolution
> is still matching the hardware constraints.
>
> With this SAODBLK_A_MainConcept_4 and SAODBLK_B_MainConcept_4 conformance
> tests files are correctly decoded with both NV12 and NV12_4L4 pixel formats.
> These two files have a resolution of 1016x760.
> If step_width = 16 for the both pixel formats the selected capture
Did you mean is instead of = ? Are you missing "and" somewhere in this sentence
?
> resolution is 1024x768 which is wrong for NV12_4L4 (which expect 1016x760)
> on Hantro G2 on IMX8MQ (but correct for NV12).
>
> For other variants than Hantro G2 on IMX8M keep the same step_width to avoid
> regressions.
>
> Fluster HEVC test score is now 128/147 vs 126/147 with the both pixel
> formats as decoder output.
> Fluster VP9 test score stay at 147/303.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
> version 2:
> - Add a HEVC SPS validation function to be used when
> setting the control and start decoding.
> I hope that is what Nicolas expects in his remark on v1.
>
> drivers/staging/media/hantro/hantro_drv.c | 12 +++---
> drivers/staging/media/hantro/hantro_hevc.c | 28 +++++++++++++
> drivers/staging/media/hantro/hantro_hw.h | 2 +
> drivers/staging/media/hantro/hantro_v4l2.c | 2 +-
> drivers/staging/media/hantro/imx8m_vpu_hw.c | 40 +++++++++++++++++--
> .../staging/media/hantro/rockchip_vpu_hw.c | 32 +++++++++++++++
> .../staging/media/hantro/sama5d4_vdec_hw.c | 16 ++++++++
> drivers/staging/media/hantro/sunxi_vpu_hw.c | 16 ++++++++
> 8 files changed, 137 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 377dcc1d19de..5aac3a090480 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -253,6 +253,11 @@ 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;
>
> @@ -268,12 +273,7 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> } else if (ctrl->id == V4L2_CID_STATELESS_HEVC_SPS) {
> const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_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;
> + return hantro_hevc_validate_sps(ctx, sps);
> } 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 7fdec50dc853..6abef810b285 100644
> --- a/drivers/staging/media/hantro/hantro_hevc.c
> +++ b/drivers/staging/media/hantro/hantro_hevc.c
> @@ -154,6 +154,30 @@ 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;
No action needed, just a question. Can we output tiled out of the PP ? If so,
perhaps we could handle the alignment difference by enabling the PP even if
tiled. That would greatly help to handle compatibility between decoders and
encoders in the future.
> + }
> +
> + return 0;
> +}
> +
> int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx)
> {
> struct hantro_hevc_dec_hw_ctx *hevc_ctx = &ctx->hevc_dec;
> @@ -177,6 +201,10 @@ 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_STATELESS_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 994547fe41b9..0bba6378212d 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -341,6 +341,8 @@ 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, s32 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)
> {
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 71a6279750bf..93d0dcf69f4a 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -260,7 +260,7 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
> } else if (ctx->is_encoder) {
> vpu_fmt = ctx->vpu_dst_fmt;
> } else {
> - vpu_fmt = ctx->vpu_src_fmt;
> + vpu_fmt = fmt;
> /*
> * Width/height on the CAPTURE end of a decoder are ignored and
> * replaced by the OUTPUT ones.
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index 9802508bade2..b6b2bf65e56d 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -83,6 +83,14 @@ static const struct hantro_fmt imx8m_vpu_postproc_fmts[] = {
> .fourcc = V4L2_PIX_FMT_YUYV,
> .codec_mode = HANTRO_MODE_NONE,
> .postprocessed = true,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 3840,
I remember seeing limits to 4096 instead in VSI documentation (UWHD). Just like
many hardcoded limits in this driver, perhaps the limits was copied from the
white paper performance example. It would be weird that we can decode portrait
UHD/FHD.
I think for this patch, what I'd like to see is the UDH and FHD limits to be set
as a define. Since if we got them wrong, fixing them later after this patch
becomes a lot more work, as its copied over and over.
Everything else looks good to me know, thanks for the update.
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 2160,
> + .step_height = MB_DIM,
> + },
> },
> };
>
> @@ -90,6 +98,14 @@ static const struct hantro_fmt imx8m_vpu_dec_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 3840,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 2160,
> + .step_height = MB_DIM,
> + },
> },
> {
> .fourcc = V4L2_PIX_FMT_MPEG2_SLICE,
> @@ -137,6 +153,14 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> .postprocessed = true,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 3840,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 2160,
> + .step_height = MB_DIM,
> + },
> },
> };
>
> @@ -144,6 +168,14 @@ static const struct hantro_fmt imx8m_vpu_g2_dec_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12_4L4,
> .codec_mode = HANTRO_MODE_NONE,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 3840,
> + .step_width = 4,
> + .min_height = 48,
> + .max_height = 2160,
> + .step_height = 4,
> + },
> },
> {
> .fourcc = V4L2_PIX_FMT_HEVC_SLICE,
> @@ -152,10 +184,10 @@ static const struct hantro_fmt imx8m_vpu_g2_dec_fmts[] = {
> .frmsize = {
> .min_width = 48,
> .max_width = 3840,
> - .step_width = MB_DIM,
> + .step_width = 4,
> .min_height = 48,
> .max_height = 2160,
> - .step_height = MB_DIM,
> + .step_height = 4,
> },
> },
> {
> @@ -165,10 +197,10 @@ static const struct hantro_fmt imx8m_vpu_g2_dec_fmts[] = {
> .frmsize = {
> .min_width = 48,
> .max_width = 3840,
> - .step_width = MB_DIM,
> + .step_width = 4,
> .min_height = 48,
> .max_height = 2160,
> - .step_height = MB_DIM,
> + .step_height = 4,
> },
> },
> };
> diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> index fc96501f3bc8..efba7fcdf207 100644
> --- a/drivers/staging/media/hantro/rockchip_vpu_hw.c
> +++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c
> @@ -63,6 +63,14 @@ static const struct hantro_fmt rockchip_vpu1_postproc_fmts[] = {
> .fourcc = V4L2_PIX_FMT_YUYV,
> .codec_mode = HANTRO_MODE_NONE,
> .postprocessed = true,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 1920,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 1088,
> + .step_height = MB_DIM,
> + },
> },
> };
>
> @@ -70,6 +78,14 @@ static const struct hantro_fmt rk3066_vpu_dec_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 1920,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 1088,
> + .step_height = MB_DIM,
> + },
> },
> {
> .fourcc = V4L2_PIX_FMT_H264_SLICE,
> @@ -116,6 +132,14 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 4096,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 2304,
> + .step_height = MB_DIM,
> + },
> },
> {
> .fourcc = V4L2_PIX_FMT_H264_SLICE,
> @@ -162,6 +186,14 @@ static const struct hantro_fmt rk3399_vpu_dec_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 1920,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 1088,
> + .step_height = MB_DIM,
> + },
> },
> {
> .fourcc = V4L2_PIX_FMT_H264_SLICE,
> diff --git a/drivers/staging/media/hantro/sama5d4_vdec_hw.c b/drivers/staging/media/hantro/sama5d4_vdec_hw.c
> index b2fc1c5613e1..07ee804e706b 100644
> --- a/drivers/staging/media/hantro/sama5d4_vdec_hw.c
> +++ b/drivers/staging/media/hantro/sama5d4_vdec_hw.c
> @@ -16,6 +16,14 @@ static const struct hantro_fmt sama5d4_vdec_postproc_fmts[] = {
> .fourcc = V4L2_PIX_FMT_YUYV,
> .codec_mode = HANTRO_MODE_NONE,
> .postprocessed = true,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 1280,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 720,
> + .step_height = MB_DIM,
> + },
> },
> };
>
> @@ -23,6 +31,14 @@ static const struct hantro_fmt sama5d4_vdec_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 1280,
> + .step_width = MB_DIM,
> + .min_height = 48,
> + .max_height = 720,
> + .step_height = MB_DIM,
> + },
> },
> {
> .fourcc = V4L2_PIX_FMT_MPEG2_SLICE,
> diff --git a/drivers/staging/media/hantro/sunxi_vpu_hw.c b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> index c0edd5856a0c..c2392c08febb 100644
> --- a/drivers/staging/media/hantro/sunxi_vpu_hw.c
> +++ b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> @@ -14,6 +14,14 @@ static const struct hantro_fmt sunxi_vpu_postproc_fmts[] = {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> .postprocessed = true,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 3840,
> + .step_width = 32,
> + .min_height = 48,
> + .max_height = 2160,
> + .step_height = 32,
> + },
> },
> };
>
> @@ -21,6 +29,14 @@ static const struct hantro_fmt sunxi_vpu_dec_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12_4L4,
> .codec_mode = HANTRO_MODE_NONE,
> + .frmsize = {
> + .min_width = 48,
> + .max_width = 3840,
> + .step_width = 32,
> + .min_height = 48,
> + .max_height = 2160,
> + .step_height = 32,
> + },
> },
> {
> .fourcc = V4L2_PIX_FMT_VP9_FRAME,
> --
> 2.32.0
>
>
Powered by blists - more mailing lists