| 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: <4d41485bb1452ec6b9dfa0a23a925c5dd2af72da.camel@collabora.com>
Date: Fri, 18 Jun 2021 16:24:32 -0300
From: Ezequiel Garcia <ezequiel@...labora.com>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>,
hverkuil@...all.nl, p.zabel@...gutronix.de, mchehab@...nel.org,
shawnguo@...nel.org, s.hauer@...gutronix.de, festevam@...il.com,
gregkh@...uxfoundation.org, mripard@...nel.org,
paul.kocialkowski@...tlin.com, wens@...e.org,
jernej.skrabec@...l.net, emil.l.velikov@...il.com,
andrzej.p@...labora.com, jc@...esim.co.uk,
jernej.skrabec@...il.com, nicolas@...fresne.ca
Cc: kernel@...gutronix.de, linux-imx@....com,
linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 6/8] media: hantro: enumerate scaled output formats
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@...labora.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
Powered by blists - more mailing lists