Re: [PATCH v4 23/24] media: hantro: Add H.264 field decoding support

[Nicolas Dufresne]

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@xxxxxxxxx>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> > ---
> >  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
> 
> >  };
> >  
> >  /**
>