From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc: "Ezequiel Garcia" <ezequiel@vanguardiasur.com.ar>,
"Philipp Zabel" <p.zabel@pengutronix.de>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Shawn Guo" <shawnguo@kernel.org>,
"Sascha Hauer" <s.hauer@pengutronix.de>,
"Fabio Estevam" <festevam@gmail.com>,
dl-linux-imx <linux-imx@nxp.com>,
"Heiko Stuebner" <heiko@sntech.de>,
"Chen-Yu Tsai" <wens@csie.org>,
"Jernej Škrabec" <jernej.skrabec@gmail.com>,
samuel@sholland.org, "Sascha Hauer" <kernel@pengutronix.de>,
DVB_Linux_Media <linux-media@vger.kernel.org>,
"open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>,
"open list:STAGING SUBSYSTEM" <linux-staging@lists.linux.dev>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
linux-sunxi@lists.linux.dev,
"Collabora Kernel ML" <kernel@collabora.com>
Subject: Re: [PATCH] media: hantro: Be more accurate on pixel formats step_width constraints
Date: Tue, 17 May 2022 09:30:20 -0400 [thread overview]
Message-ID: <4583a1993dd988641e030b811eb94df141c05eef.camel@ndufresne.ca> (raw)
In-Reply-To: <CAKQmDh9qLMQo9XwqSNMknukhc2sEhJfY_gRkUEEr2yJKShWTwQ@mail.gmail.com>
(none-html form, sorry about that)
Le mardi 17 mai 2022 à 06:21 -0400, Nicolas Dufresne a écrit :
>
>
> Le mar. 17 mai 2022, 03 h 38, Benjamin Gaignard
> <benjamin.gaignard@collabora.com> a écrit :
> >
> > Le 16/05/2022 à 21:08, Nicolas Dufresne a écrit :
> > > Hi Benjamin,
> > >
> > > thanks for your patch, but see below, perhaps something you could
> > > improve...
> > >
> > > Le lundi 16 mai 2022 à 16:11 +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.
> > > From v4l2_apply_frmsize_constraints() point of view, 8 bytes is a sub-
> > > set of 4
> > > bytes. The application can request larger then coded size width/height
> > > after
> > > this change and you'd still get a broken render. The reason is that it
> > > looks
> > > like the tile mode has more constraints then what
> > > v4l2_apply_frmsize_constraints(). It seems like from your description that
> > > the
> > > width/height must match the coded size (+plus the alignment). For this
> > > reason, I
> > > think you need when using tile mode validate the that the width/height
> > > passed to
> > > v4l2_apply_frmsize_constraints() matched the coded size from the header
> > > structure rather then user provided size passed in S_FMT.
> >
> > Gstreamer plugins set and get pixel formats on capture and output queues
> > without
> > sending a header when they want to test the driver before register the
> > decoder
> > element. If I do want you want that will be broken and not only for HEVC but
> > for
> > all other codecs.
>
> The validation happens only on s_ctrl and streamon. Check how it's done for
> h264 in media_stage (same happens for vp9).
>
>
> >
> > Benjamin
> >
> > >
> > > regards,
> > > Nicolas
> > >
> > > > 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
> > > > 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@collabora.com>
> > > > ---
> > > > 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 ++++++++
> > > > 5 files changed, 101 insertions(+), 5 deletions(-)
> > > >
> > > > 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,
> > > > + .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,
prev parent reply other threads:[~2022-05-17 13:30 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-16 14:11 Benjamin Gaignard
2022-05-16 19:08 ` Nicolas Dufresne
2022-05-17 7:38 ` Benjamin Gaignard
[not found] ` <CAKQmDh9qLMQo9XwqSNMknukhc2sEhJfY_gRkUEEr2yJKShWTwQ@mail.gmail.com>
2022-05-17 13:30 ` Nicolas Dufresne [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4583a1993dd988641e030b811eb94df141c05eef.camel@ndufresne.ca \
--to=nicolas@ndufresne.ca \
--cc=benjamin.gaignard@collabora.com \
--cc=ezequiel@vanguardiasur.com.ar \
--cc=festevam@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=heiko@sntech.de \
--cc=jernej.skrabec@gmail.com \
--cc=kernel@collabora.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux-sunxi@lists.linux.dev \
--cc=mchehab@kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=s.hauer@pengutronix.de \
--cc=samuel@sholland.org \
--cc=shawnguo@kernel.org \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®