Date:   Wed, 30 Mar 2022 11:25:53 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Sebastian Fricke <sebastian.fricke@...labora.com>
Cc:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel@...labora.com, Jonas Karlman <jonas@...boo.se>,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 23/24] media: hantro: Add H.264 field decoding support

Le mercredi 30 mars 2022 à 11:03 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, 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 has been found by
> 
> s/has been/was/
> 
> > trial and errors. The list consist of all the references with the
> 
> s/consist/consists/
> 
> > opposite field parity.
> > 
> > The strategy being to deduplicate the reference picture that points
> 
> s/strategy being to/strategy is to/
> 
> > to the same storage (same index). The choice of opposite parity has
> > been made to keep the other field or a field pair to be kept in the
> 
> Do you mean?
> 
> s/keep the other field or a field pair to be kept/
>    keep the other field of a field pair/
> 
> > 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 | 107 ++++++++++++++++++---
> > drivers/staging/media/hantro/hantro_hw.h   |   1 +
> > 2 files changed, 94 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 7377fc26f780..f6fc939aa726 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -22,6 +22,11 @@
> > #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
> 
> s/index/indeces/
> 
> > + * 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 +232,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 +243,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,
> 
> s/bit 31/bits 31/
> 
> > +		 * 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 +357,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 +376,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;
> 
> You use the magic values `0x1` & `0x2` here, can you replace those with
> macros?
> 
> It looks 0x2 indicates that we have a field and 0x1 indicates the
> distance of the current picture to the bottom field is greater than
> the distance of the current picture to the top field. (inidicating that
> the order is correct?)
> 
> So maybe:
> ```
> #define HANTRO_H264_FIELD_DMA_ADDR              0x1
> #define HANTRO_H264_CORRECT_FIELD_ORDER         0x2
> ```

Will do, I need to check again in the ref code for the appropriate wording for
the meaning of these bits. I don't like much FIELD_DMA_ADDR, as its not an
address, or an offset to an address. Here the address must be aligned, which
results in these bits always being 0, so they reuse it to pass per-reference
information.

> 
> 
> > +
> > +	return dma_addr | flags;
> > }
> > 
> > u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > @@ -355,6 +393,34 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > 	return dpb->frame_num;
> > }
> > 
> > +static void deduplicate_reflist(struct v4l2_h264_reflist_builder *b,
> > +				struct v4l2_h264_reference *reflist)
> 
> Can you add a comment describing why we need to deduplicate the
> reference list? And maybe also why we get duplications in the first
> place? Why must we limit the size to 16?
> This would increase the readability of the code a lot.

For readers that did understand the H.264 specification, that a field reflist
has 32 entry should be obvious. That hantro uses 16 entry only is an
undocumented trickery, I'll write something to the best of my knowledge, but
this has been reversed, so don't expect the highest rationale. I will though
document here what I say in the commit, e.g. why this method might not be
robust, so someone can give it a try and make it more robust in the future.

> 
> > +{
> > +	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 */
> 
> s/the remaining/the remaining entries/
> s/fails/fail/
> 
> > +	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 +452,28 @@ 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,
> 
> s/in field/in a field/
> 
> > +	 * dpb_longterm bitmap along with the current frame parity.
> 
> s/bitmap/bitmaps/
> 
> Greetings,
> Sebastian
> 
> > +	 */
> > +	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;
> > };
> > 
> > /**
> > -- 
> > 2.34.1
> > 

Powered by blists - more mailing lists