| 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: <cf05c8031345341caf641d37aeeb58bd506b624a.camel@ndufresne.ca>
Date: Thu, 28 Apr 2022 09:10:20 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Hans Verkuil <hverkuil@...all.nl>,
Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
Philipp Zabel <p.zabel@...gutronix.de>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-media@...r.kernel.org, Jonas Karlman <jonas@...boo.se>,
linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 23/24] media: hantro: Add H.264 field decoding support
Le jeudi 28 avril 2022 à 10:50 +0200, Hans Verkuil a écrit :
> On 26/04/2022 14:57, Nicolas Dufresne wrote:
> > This adds the required code to support field decoding. While most of
> > the code is derived from Rockchip and VSI reference code, the
> > reduction of the reference list to 16 entries was found by
> > trial and errors. The list consists of all the references with the
> > opposite field parity.
> >
> > The strategy is to deduplicate the reference picture that points
> > to the same storage (same index). The choice of opposite parity has
> > been made to keep the other field of the current field pair in the
> > list. This method may not be robust if a field was lost.
> >
> > Signed-off-by: Jonas Karlman <jonas@...boo.se>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 122 ++++++++++++++++++---
> > drivers/staging/media/hantro/hantro_hw.h | 1 +
> > 2 files changed, 109 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 7377fc26f780..7502dddb324c 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -22,6 +22,12 @@
> > #define POC_BUFFER_SIZE 34
> > #define SCALING_LIST_SIZE (6 * 16 + 2 * 64)
> >
> > +/*
> > + * For valid and long term reference marking, index are reversed, so bit 31
> > + * indicates the status of the picture 0.
> > + */
> > +#define REF_BIT(i) BIT(32 - 1 - (i))
> > +
> > /* Data structure describing auxiliary buffer format. */
> > struct hantro_h264_dec_priv_tbl {
> > u32 cabac_table[CABAC_INIT_BUFFER_SIZE];
> > @@ -227,6 +233,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > {
> > const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
> > const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
> > + const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
> > struct hantro_h264_dec_priv_tbl *tbl = ctx->h264_dec.priv.cpu;
> > const struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> > u32 dpb_longterm = 0;
> > @@ -237,20 +244,45 @@ static void prepare_table(struct hantro_ctx *ctx)
> > tbl->poc[i * 2] = dpb[i].top_field_order_cnt;
> > tbl->poc[i * 2 + 1] = dpb[i].bottom_field_order_cnt;
> >
> > + if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > + continue;
> > +
> > /*
> > * Set up bit maps of valid and long term DPBs.
> > - * NOTE: The bits are reversed, i.e. MSb is DPB 0.
> > + * NOTE: The bits are reversed, i.e. MSb is DPB 0. For frame
> > + * decoding, bit 31 to 15 are used, while for field decoding,
> > + * all bits are used, with bit 31 being a top field, 30 a bottom
> > + * field and so on.
> > */
> > - if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > - dpb_valid |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
> > - if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> > - dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
> > + if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) {
> > + if (dpb[i].fields & V4L2_H264_TOP_FIELD_REF)
> > + dpb_valid |= REF_BIT(i * 2);
> > +
> > + if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF)
> > + dpb_valid |= REF_BIT(i * 2 + 1);
> > +
> > + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) {
> > + dpb_longterm |= REF_BIT(i * 2);
> > + dpb_longterm |= REF_BIT(i * 2 + 1);
> > + }
> > + } else {
> > + dpb_valid |= REF_BIT(i);
> > +
> > + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> > + dpb_longterm |= REF_BIT(i);
> > + }
> > + }
> > + ctx->h264_dec.dpb_valid = dpb_valid;
> > + ctx->h264_dec.dpb_longterm = dpb_longterm;
> > +
> > + if ((dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) ||
> > + !(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)) {
> > + tbl->poc[32] = ctx->h264_dec.cur_poc;
> > + tbl->poc[33] = 0;
> > + } else {
> > + tbl->poc[32] = dec_param->top_field_order_cnt;
> > + tbl->poc[33] = dec_param->bottom_field_order_cnt;
> > }
> > - ctx->h264_dec.dpb_valid = dpb_valid << 16;
> > - ctx->h264_dec.dpb_longterm = dpb_longterm << 16;
> > -
> > - tbl->poc[32] = dec_param->top_field_order_cnt;
> > - tbl->poc[33] = dec_param->bottom_field_order_cnt;
> >
> > assemble_scaling_list(ctx);
> > }
> > @@ -326,6 +358,8 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
> > {
> > struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> > dma_addr_t dma_addr = 0;
> > + s32 cur_poc = ctx->h264_dec.cur_poc;
> > + u32 flags;
> >
> > if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > dma_addr = hantro_get_ref(ctx, dpb[dpb_idx].reference_ts);
> > @@ -343,7 +377,12 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
> > dma_addr = hantro_get_dec_buf_addr(ctx, buf);
> > }
> >
> > - return dma_addr;
> > + flags = dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD ? 0x2 : 0;
> > + flags |= abs(dpb[dpb_idx].top_field_order_cnt - cur_poc) <
> > + abs(dpb[dpb_idx].bottom_field_order_cnt - cur_poc) ?
> > + 0x1 : 0;
> > +
> > + return dma_addr | flags;
> > }
> >
> > u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > @@ -355,6 +394,47 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > return dpb->frame_num;
> > }
> >
> > +/*
> > + * Removes all references with he same parity as current picture from the
> > + * reference list. The remaining list will have references with the opposite
> > + * parity. This is effectively a deduplication of references since each buffer
> > + * stores two fields. For this eason, each buffer are found twice in the
> > + * reference list.
> > + *
> > + * This technique has been chosen through trial and error. This simple approach
> > + * resulted in the highest conformance score. Note that this method may suffer
> > + * worse quality in the case an opposite reference frame has been lost. If this
> > + * becomes a problem in the future, it should be possible to add a preprocessing
> > + * to identify un-paired fields and avoid removing them.
> > + */
> > +static void deduplicate_reflist(struct v4l2_h264_reflist_builder *b,
> > + struct v4l2_h264_reference *reflist)
> > +{
> > + int write_idx = 0;
> > + int i;
> > +
> > + if (b->cur_pic_fields == V4L2_H264_FRAME_REF) {
> > + write_idx = b->num_valid;
> > + goto done;
> > + }
> > +
> > + for (i = 0; i < b->num_valid; i++) {
> > + if (!(b->cur_pic_fields == reflist[i].fields)) {
> > + reflist[write_idx++] = reflist[i];
> > + continue;
> > + }
> > + }
> > +
> > +done:
> > + /* Should not happen unless we have a bug in the reflist builder. */
> > + if (WARN_ON(write_idx > 16))
> > + write_idx = 16;
> > +
> > + /* Clear the remaining, some streams fails otherwise */
> > + for (; write_idx < 16; write_idx++)
> > + reflist[write_idx].index = 15;
> > +}
> > +
> > int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
> > {
> > struct hantro_h264_dec_hw_ctx *h264_ctx = &ctx->h264_dec;
> > @@ -386,15 +466,29 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
> > /* Update the DPB with new refs. */
> > update_dpb(ctx);
> >
> > - /* Prepare data in memory. */
> > - prepare_table(ctx);
> > -
> > /* Build the P/B{0,1} ref lists. */
> > v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
> > ctrls->sps, ctx->h264_dec.dpb);
> > + h264_ctx->cur_poc = reflist_builder.cur_pic_order_count;
> > +
> > + /* Prepare data in memory. */
> > + prepare_table(ctx);
> > +
> > v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
> > v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> > h264_ctx->reflists.b1);
> > +
> > + /*
> > + * Reduce ref lists to at most 16 entries, Hantro hardware will deduce
> > + * the actual picture lists in field through the dpb_valid,
> > + * dpb_longterm bitmap along with the current frame parity.
> > + */
> > + if (reflist_builder.cur_pic_fields != V4L2_H264_FRAME_REF) {
> > + deduplicate_reflist(&reflist_builder, h264_ctx->reflists.p);
> > + deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b0);
> > + deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b1);
> > + }
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index 292aaaabaf24..fd869369fb97 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -91,6 +91,7 @@ struct hantro_h264_dec_hw_ctx {
> > struct hantro_h264_dec_ctrls ctrls;
> > u32 dpb_longterm;
> > u32 dpb_valid;
> > + s32 cur_poc;
>
> This field isn't documented in kerneldoc.
>
> I've added this:
>
> + * @cur_poc: Current picture order count
>
> Is that a correct description? If not, let me know what it should be.
>
> I'll update the patch manually, no need to repost.
Yes, this is entirely correct, thanks for catching this.
>
> Regards,
>
> Hans
>
> > };
> >
> > /**
>
Powered by blists - more mailing lists