Message-ID: <3297d1a4804b20d5d8d32013e5776109fa10c96f.camel@collabora.com>
Date:   Thu, 07 Apr 2022 17:08:22 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        mchehab@...nel.org, hverkuil@...all.nl,
        ezequiel@...guardiasur.com.ar, p.zabel@...gutronix.de,
        gregkh@...uxfoundation.org, mripard@...nel.org,
        paul.kocialkowski@...tlin.com, wens@...e.org,
        jernej.skrabec@...il.com, samuel@...lland.org
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        sebastian.fricke@...labora.com
Subject: Re: [PATCH v5 06/17] media: uapi: HEVC: Change pic_order_cnt
 definition in v4l2_hevc_dpb_entry

Le jeudi 07 avril 2022 à 16:51 -0400, Nicolas Dufresne a écrit :
> Le jeudi 07 avril 2022 à 17:29 +0200, Benjamin Gaignard a écrit :
> > HEVC specifications say that:
> > "PicOrderCntVal is derived as follows:
> > PicOrderCntVal = PicOrderCntMsb + slice_pic_order_cnt_lsb
> > The value of PicOrderCntVal shall be in the range of −231 to 231 − 1, inclusive."
> 
> Did you mean 2^31 ?
> 
> > 
> > To match with these definitions change __u16 pic_order_cnt[2]
> > into __s32 pic_order_cnt_val.
> > 
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> > ---
> > version 5:
> > - change __u16 pic_order_cnt[2] into __s32 pic_order_cnt_val
> >  drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 4 ++--
> >  drivers/staging/media/hantro/hantro_hevc.c        | 2 +-
> >  drivers/staging/media/hantro/hantro_hw.h          | 4 ++--
> >  drivers/staging/media/sunxi/cedrus/cedrus_h265.c  | 4 ++--
> >  include/media/hevc-ctrls.h                        | 2 +-
> >  5 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> > index c524af41baf5..6f3c774aa3d9 100644
> > --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> > +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> > @@ -386,7 +386,7 @@ static int set_ref(struct hantro_ctx *ctx)
> >  	 * pic_order_cnt[0] and ignore pic_order_cnt[1] used in field-coding.
> >  	 */
> >  	for (i = 0; i < decode_params->num_active_dpb_entries && i < ARRAY_SIZE(cur_poc); i++) {
> > -		char poc_diff = decode_params->pic_order_cnt_val - dpb[i].pic_order_cnt[0];
> > +		char poc_diff = decode_params->pic_order_cnt_val - dpb[i].pic_order_cnt_val;
> >  
> >  		hantro_reg_write(vpu, &cur_poc[i], poc_diff);
> >  	}
> > @@ -413,7 +413,7 @@ static int set_ref(struct hantro_ctx *ctx)
> >  	dpb_longterm_e = 0;
> >  	for (i = 0; i < decode_params->num_active_dpb_entries &&
> >  	     i < (V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1); i++) {
> > -		luma_addr = hantro_hevc_get_ref_buf(ctx, dpb[i].pic_order_cnt[0]);
> > +		luma_addr = hantro_hevc_get_ref_buf(ctx, dpb[i].pic_order_cnt_val);
> >  		if (!luma_addr)
> >  			return -ENOMEM;
> >  
> > diff --git a/drivers/staging/media/hantro/hantro_hevc.c b/drivers/staging/media/hantro/hantro_hevc.c
> > index b6ec86d03d91..fadd40768579 100644
> > --- a/drivers/staging/media/hantro/hantro_hevc.c
> > +++ b/drivers/staging/media/hantro/hantro_hevc.c
> > @@ -54,7 +54,7 @@ static void hantro_hevc_ref_init(struct hantro_ctx *ctx)
> >  }
> >  
> >  dma_addr_t hantro_hevc_get_ref_buf(struct hantro_ctx *ctx,
> > -				   int poc)
> > +				   s32 poc)
> >  {
> >  	struct hantro_hevc_dec_hw_ctx *hevc_dec = &ctx->hevc_dec;
> >  	int i;
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index ed018e293ba0..a648c529662b 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -131,7 +131,7 @@ struct hantro_hevc_dec_hw_ctx {
> >  	struct hantro_aux_buf tile_bsd;
> >  	struct hantro_aux_buf ref_bufs[NUM_REF_PICTURES];
> >  	struct hantro_aux_buf scaling_lists;
> > -	int ref_bufs_poc[NUM_REF_PICTURES];
> > +	s32 ref_bufs_poc[NUM_REF_PICTURES];
> 
> Was this strictly needed ? Isn't int always same as s32 ?
> 
> >  	u32 ref_bufs_used;
> >  	struct hantro_hevc_dec_ctrls ctrls;
> >  	unsigned int num_tile_cols_allocated;
> > @@ -337,7 +337,7 @@ int hantro_hevc_dec_init(struct hantro_ctx *ctx);
> >  void hantro_hevc_dec_exit(struct hantro_ctx *ctx);
> >  int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx);
> >  int hantro_hevc_dec_prepare_run(struct hantro_ctx *ctx);
> > -dma_addr_t hantro_hevc_get_ref_buf(struct hantro_ctx *ctx, int poc);
> > +dma_addr_t hantro_hevc_get_ref_buf(struct hantro_ctx *ctx, s32 poc);
> >  int hantro_hevc_add_ref_buf(struct hantro_ctx *ctx, int poc, dma_addr_t addr);
> >  void hantro_hevc_ref_remove_unused(struct hantro_ctx *ctx);
> >  size_t hantro_hevc_chroma_offset(const struct v4l2_ctrl_hevc_sps *sps);
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > index 44f385be9f6c..d04521ffd920 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > @@ -143,8 +143,8 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
> >  	for (i = 0; i < num_active_dpb_entries; i++) {
> >  		int buffer_index = vb2_find_timestamp(vq, dpb[i].timestamp, 0);
> >  		u32 pic_order_cnt[2] = {
> > -			dpb[i].pic_order_cnt[0],
> > -			dpb[i].pic_order_cnt[1]
> > +			dpb[i].pic_order_cnt_val & 0xffff,
> > +			(dpb[i].pic_order_cnt_val >> 16) & 0xffff
> 
> This is confusing, it gives the impression that pic_order_cnt_val contains TOP
> and BOTTOM field pic_order_cnt, which isn't the case. This is just the full pic
> order count value for this reference.
> 
> This is confusing me, most HEVC decoder don't really know about fields. They
> will instead happily produce half height frames, and we should support this in
> the form of ALTERNATE or SEQ interlacing output.
> 
> While it seems like Allwinner HW maybe support interleaved output, there I would
> not find any userland that would implement this, hence proving that it works.
> Overall, interlaced HEVC (a very niche use case) should be studied, and we
> should ensure that alternate/seq interlacing is possible, since a lot of HW will
> only offer this.

I looked a bit more at the reverse-engineering data for that chip, and what
should happen is that the pic_order_cnt_val should be set into pic_order_cnt[0].
The code handling this later on does:

	struct cedrus_h265_sram_frame_info frame_info = {
		.top_pic_order_cnt = cpu_to_le32(pic_order_cnt[0]),
		.bottom_pic_order_cnt = cpu_to_le32(field_pic ?
						    pic_order_cnt[1] :
						    pic_order_cnt[0]),

Now, the pic_oder_cnt[1] is the other field. Things is that the two field won't
endup in the same frame buffer magically. In fact we'll need some tinkering on
how this could possibly be supported. In GStreamer we only have the DXVA based
decoder that supports field decoding, and each field endups in their own surface
(our buffers). This looks more natural for Hantro here at least. Though perhaps
there exist stride trick to interleave them, but then we need special care for
the extra buffers. I think we need to think through what we want to offer here,
anything from SEQ or interleaved buffer will require some more thinking, while
alternate interlacing would be rather simple (handle by userland mostly) and
will be more portable. Even Cedrus should be usable in that mode by ignoring its
interleaving capability. 

> 
> >  		};
> >  
> >  		cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic,
> > diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> > index b3540167df9e..2812778b41f4 100644
> > --- a/include/media/hevc-ctrls.h
> > +++ b/include/media/hevc-ctrls.h
> > @@ -138,7 +138,7 @@ struct v4l2_hevc_dpb_entry {
> >  	__u64	timestamp;
> >  	__u8	flags;
> >  	__u8	field_pic;
> > -	__u16	pic_order_cnt[2];
> > +	__s32	pic_order_cnt_val;
> >  	__u8	padding[2];
> >  };
> >  
> 

Powered by blists - more mailing lists