From: Ezequiel Garcia <ezequiel@collabora.com>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>,
hverkuil@xs4all.nl, p.zabel@pengutronix.de, mchehab@kernel.org,
shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com,
gregkh@linuxfoundation.org, mripard@kernel.org,
paul.kocialkowski@bootlin.com, wens@csie.org,
jernej.skrabec@siol.net, emil.l.velikov@gmail.com,
andrzej.p@collabora.com, jc@kynesim.co.uk,
jernej.skrabec@gmail.com, nicolas@ndufresne.ca
Cc: kernel@pengutronix.de, linux-imx@nxp.com,
linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 6/8] media: hantro: enumerate scaled output formats
Date: Fri, 18 Jun 2021 16:24:32 -0300 [thread overview]
Message-ID: <4d41485bb1452ec6b9dfa0a23a925c5dd2af72da.camel@collabora.com> (raw)
In-Reply-To: <20210618131526.566762-7-benjamin.gaignard@collabora.com>
Hi Benjamin,
Thanks for working on this.
On Fri, 2021-06-18 at 15:15 +0200, Benjamin Gaignard wrote:
> When enumerating the output formats take care of the hardware scaling
> capabilities.
> For a given input size G2 hardware block is capable of down scale the
> output by 2, 4 or 8 factor. When decoding 4K streams that to be could
> helpful to save memory bandwidth.
>
Looking at https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html
I see that this case should be covered by the spec.
If I understand correctly, it would be:
1. VIDIOC_S_FMT(OUTPUT)
2. VIDIOC_ENUM_FMT(CAPTURE) / VIDIOC_ENUM_FRAMESIZES(CAPTURE)
3. VIDIOC_S_FMT(CAPTURE)
4. VIDIOC_G_FMT(CAPTURE) again to get buffer information.
Does v4l2codecs support this case as-is, if changes are needed,
I'd like to have the MR ready and reviewed by Nicolas.
I know it's a staging driver, but I believe it's important
to have users for new cases/feature to avoid bitrotting.
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> drivers/staging/media/hantro/hantro.h | 4 ++
> .../staging/media/hantro/hantro_g2_hevc_dec.c | 46 ++++++++++++++++++-
> drivers/staging/media/hantro/hantro_g2_regs.h | 6 +++
> drivers/staging/media/hantro/hantro_hw.h | 1 +
> drivers/staging/media/hantro/hantro_v4l2.c | 10 ++--
> drivers/staging/media/hantro/imx8m_vpu_hw.c | 1 +
> 6 files changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index 6a21d1e95b34..ca9038b0384a 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -71,6 +71,9 @@ struct hantro_irq {
> * @reg_names: array of register range names
> * @num_regs: number of register range names in the array
> * @postproc_regs: &struct hantro_postproc_regs pointer
> + * @scaling: Set possible scaled output formats.
> + * Returns zero if OK, a negative value in error cases.
> + * Optional.
> */
> struct hantro_variant {
> unsigned int enc_offset;
> @@ -92,6 +95,7 @@ struct hantro_variant {
> const char * const *reg_names;
> int num_regs;
> const struct hantro_postproc_regs *postproc_regs;
> + int (*scaling)(struct hantro_ctx *ctx, struct v4l2_frmsizeenum *fsize);
Please add some .ops field, so we can put this
and move init and runtime_resume as well.
> };
>
> /**
> diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> index 41dc89ec926c..3a8aa2ff109c 100644
> --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> @@ -396,6 +396,17 @@ static void set_ref_pic_list(struct hantro_ctx *ctx)
> }
> }
>
> +static int down_scale_factor(struct hantro_ctx *ctx)
> +{
> + const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls;
> + const struct v4l2_ctrl_hevc_sps *sps = ctrls->sps;
> +
> + if (sps->pic_width_in_luma_samples == ctx->dst_fmt.width)
> + return 0;
> +
> + return DIV_ROUND_CLOSEST(sps->pic_width_in_luma_samples, ctx->dst_fmt.width);
> +}
> +
> static int set_ref(struct hantro_ctx *ctx)
> {
> const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls;
> @@ -409,6 +420,7 @@ static int set_ref(struct hantro_ctx *ctx)
> size_t mv_offset = hantro_hevc_motion_vectors_offset(sps);
> size_t compress_luma_offset = hantro_hevc_luma_compress_offset(sps);
> size_t compress_chroma_offset = hantro_hevc_chroma_compress_offset(sps);
> + int down_scale = down_scale_factor(ctx);
> u32 max_ref_frames;
> u16 dpb_longterm_e;
> static const struct hantro_reg cur_poc[] = {
> @@ -521,8 +533,18 @@ static int set_ref(struct hantro_ctx *ctx)
> hantro_write_addr(vpu, G2_REG_CHR_REF(i), chroma_addr);
> hantro_write_addr(vpu, G2_REG_DMV_REF(i++), mv_addr);
>
> - hantro_write_addr(vpu, G2_ADDR_DST, luma_addr);
> - hantro_write_addr(vpu, G2_ADDR_DST_CHR, chroma_addr);
> + if (down_scale) {
> + chroma_addr = luma_addr + (cr_offset >> down_scale);
> + hantro_reg_write(vpu, &g2_down_scale_e, 1);
> + hantro_reg_write(vpu, &g2_down_scale_y, down_scale >> 2);
> + hantro_reg_write(vpu, &g2_down_scale_x, down_scale >> 2);
> + hantro_write_addr(vpu, G2_DS_DST, luma_addr);
> + hantro_write_addr(vpu, G2_DS_DST_CHR, chroma_addr);
> + } else {
> + hantro_write_addr(vpu, G2_ADDR_DST, luma_addr);
> + hantro_write_addr(vpu, G2_ADDR_DST_CHR, chroma_addr);
> + }
> +
> hantro_write_addr(vpu, G2_ADDR_DST_MV, mv_addr);
> hantro_write_addr(vpu, G2_COMP_ADDR_DST, compress_luma_addr);
> hantro_write_addr(vpu, G2_COMP_CHR, compress_chroma_addr);
> @@ -603,6 +625,26 @@ static void hantro_g2_check_idle(struct hantro_dev *vpu)
> }
> }
>
> +int hantro_g2_hevc_dec_scaling(struct hantro_ctx *ctx,
> + struct v4l2_frmsizeenum *fsize)
Maybe
s/hantro_g2_hevc_dec_scaling/hantro_g2_hevc_enum_framesizes
would be clear?
Is this restricted to HEVC or is it something that will
work on VP9 as well?
> +{
> + /**
> + * G2 scaler can scale down by 0, 2, 4 or 8
> + * use fsize->index has power of 2 diviser
> + **/
Please use
/*
*
*/
style.
> + if (fsize->index > 3)
> + return -EINVAL;
> +
> + if (!ctx->src_fmt.width || !ctx->src_fmt.height)
> + return -EINVAL;
> +
> + fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> + fsize->discrete.width = ctx->src_fmt.width >> fsize->index;
> + fsize->discrete.height = ctx->src_fmt.height >> fsize->index;
> +
> + return 0;
> +}
> +
[..]
> - /* This only makes sense for coded formats */
> - if (fmt->codec_mode == HANTRO_MODE_NONE)
> + /* For non-coded formats check if scaling is possible */
> + if (fmt->codec_mode == HANTRO_MODE_NONE) {
> + if (ctx->dev->variant->scaling)
> + return ctx->dev->variant->scaling(ctx, fsize);
> +
> return -EINVAL;
I wonder why we are returning EINVAL here. Can we support
.vidioc_enum_framesizes for coded and non-coded?
Thanks,
Ezequiel
next prev parent reply other threads:[~2021-06-18 19:24 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-18 13:15 [PATCH v3 0/8] Additional features for Hantro HEVC Benjamin Gaignard
2021-06-18 13:15 ` [PATCH v3 1/8] media: hantro: Trace hevc hw cycles performance register Benjamin Gaignard
2021-06-18 18:58 ` Ezequiel Garcia
2021-06-22 12:23 ` Benjamin Gaignard
2021-06-22 12:48 ` Ezequiel Garcia
2021-06-18 13:15 ` [PATCH v3 2/8] media: hantro: Add support of compressed reference buffers Benjamin Gaignard
2021-06-18 13:15 ` [PATCH v3 3/8] media: hantro: hevc: Allow 10-bits encoded streams Benjamin Gaignard
2021-06-18 13:15 ` [PATCH v3 4/8] media: Add P010 video format Benjamin Gaignard
2021-06-18 19:38 ` Ezequiel Garcia
2021-06-22 21:24 ` Nicolas Dufresne
2021-06-22 22:21 ` Ezequiel Garcia
2021-06-25 19:33 ` Nicolas Dufresne
2021-06-28 13:37 ` Benjamin Gaignard
2021-06-18 13:15 ` [PATCH v3 5/8] media: hantro: hevc: Allow to produce 10-bit frames Benjamin Gaignard
2021-06-22 13:26 ` Ezequiel Garcia
2021-06-18 13:15 ` [PATCH v3 6/8] media: hantro: enumerate scaled output formats Benjamin Gaignard
2021-06-18 19:24 ` Ezequiel Garcia [this message]
2021-06-22 21:45 ` Nicolas Dufresne
2021-06-18 13:15 ` [PATCH v3 7/8] media: hevc: Add scaling matrix control Benjamin Gaignard
2021-06-18 13:15 ` [PATCH v3 8/8] media: hantro: Add scaling lists feature Benjamin Gaignard
2021-06-22 12:54 ` [PATCH v3 0/8] Additional features for Hantro HEVC Ezequiel Garcia
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=4d41485bb1452ec6b9dfa0a23a925c5dd2af72da.camel@collabora.com \
--to=ezequiel@collabora.com \
--cc=andrzej.p@collabora.com \
--cc=benjamin.gaignard@collabora.com \
--cc=emil.l.velikov@gmail.com \
--cc=festevam@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=hverkuil@xs4all.nl \
--cc=jc@kynesim.co.uk \
--cc=jernej.skrabec@gmail.com \
--cc=jernej.skrabec@siol.net \
--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=mchehab@kernel.org \
--cc=mripard@kernel.org \
--cc=nicolas@ndufresne.ca \
--cc=p.zabel@pengutronix.de \
--cc=paul.kocialkowski@bootlin.com \
--cc=s.hauer@pengutronix.de \
--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;
as well as URLs for NNTP newsgroup(s).