diff mbox series

[v5,06/17] media: uapi: HEVC: Change pic_order_cnt definition in v4l2_hevc_dpb_entry

Message ID 20220407152940.738159-7-benjamin.gaignard@collabora.com (mailing list archive)
State New, archived
Headers show
Series Move HEVC stateless controls out of staging | expand

Commit Message

Benjamin Gaignard April 7, 2022, 3:29 p.m. UTC
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."

To match with these definitions change __u16 pic_order_cnt[2]
into __s32 pic_order_cnt_val.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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(-)

Comments

Nicolas Dufresne April 7, 2022, 8:51 p.m. UTC | #1
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@collabora.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.

>  		};
>  
>  		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];
>  };
>
Nicolas Dufresne April 7, 2022, 9:08 p.m. UTC | #2
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@collabora.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];
> >  };
> >  
>
Benjamin Gaignard April 8, 2022, 7:10 a.m. UTC | #3
Le 07/04/2022 à 22:51, 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 ?

yes it is 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@collabora.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 ?

could be, but like this I'm sure it is fine in all cases

>
>>   	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.

In GST code pic_order_cnt[0] and pic_order_cnt[1] had the same value so Cedrus
driver always put the same value in this register.
I haven't any clue of want the hardware expect here.
Maybe some maintainers can explain what we should set on these fields.

Benjamin

>
>>   		};
>>   
>>   		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];
>>   };
>>
Nicolas Dufresne April 8, 2022, 4:33 p.m. UTC | #4
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."
> 
> To match with these definitions change __u16 pic_order_cnt[2]
> into __s32 pic_order_cnt_val.

You forgot to update the slice_params->slice_pic_order_count.

Nicolas

> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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];
>  	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
>  		};
>  
>  		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];
>  };
>
Benjamin Gaignard April 14, 2022, 8:02 a.m. UTC | #5
Le 08/04/2022 à 18:33, 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."
>>
>> To match with these definitions change __u16 pic_order_cnt[2]
>> into __s32 pic_order_cnt_val.
> You forgot to update the slice_params->slice_pic_order_count.

Thanks it will be in v6

Benjamin

>
> Nicolas
>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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];
>>   	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
>>   		};
>>   
>>   		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];
>>   };
>>
Sebastian Fricke April 25, 2022, 3:16 p.m. UTC | #6
Hey Benjamin,

On 07.04.2022 17:29, Benjamin Gaignard wrote:
>HEVC specifications say that:

s/HEVC specifications say that:/
   The HEVC specification describes the following:/

Greetings,
Sebastian

>"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."
>
>To match with these definitions change __u16 pic_order_cnt[2]
>into __s32 pic_order_cnt_val.
>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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];
> 	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
> 		};
>
> 		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];
> };
>
>-- 
>2.32.0
>
diff mbox series

Patch

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];
 	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
 		};
 
 		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];
 };