| Message ID | 20220407152940.738159-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | Move HEVC stateless controls out of staging | expand |
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]; > }; >
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]; > > }; > > >
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]; >> }; >>
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]; > }; >
Le jeudi 07 avril 2022 à 17:29 +0200, Benjamin Gaignard a écrit : > Make explicit that V4L2_CID_STATELESS_HEVC_SLICE_PARAMS control is > a dynamic array control type. > Some drivers may be able to receive multiple slices in one control > to improve decoding performance. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 2 ++ > include/media/hevc-ctrls.h | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > index 7fcc6c2bac4c..1aaf3b84b584 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -2964,6 +2964,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > These bitstream parameters are defined according to :ref:`hevc`. > They are described in section 7.4.7 "General slice segment header > semantics" of the specification. > + This control is a dynamically sized 1-dimensional array, > + V4L2_CTRL_FLAG_DYNAMIC_ARRAY flag must be set when using it. > > .. c:type:: v4l2_ctrl_hevc_slice_params > > diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h > index 96a9b71d1def..28049e1a8a07 100644 > --- a/include/media/hevc-ctrls.h > +++ b/include/media/hevc-ctrls.h > @@ -314,6 +314,9 @@ struct v4l2_hevc_pred_weight_table { > /** > * v4l2_ctrl_hevc_slice_params - HEVC slice parameters > * > + * This control is a dynamically sized 1-dimensional array, > + * V4L2_CTRL_FLAG_DYNAMIC_ARRAY flag must be set when using it. > + * I'm not sure about this comment, but I'm sure you forgot: --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c @@ -1505,6 +1505,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, break; case V4L2_CID_STATELESS_HEVC_SLICE_PARAMS: *type = V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS; + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; break; case V4L2_CID_STATELESS_HEVC_SCALING_MATRIX: *type = V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX; Also, in AV1 we have V4L2_AV1_MAX_TILE_COUNT, which can be used in cfg.dims, something the driver will actually set. We could have #define V4L2_HEVC_MAX_TILE_COUNT 600 And drives can set: .dims = { V4L2_HEVC_MAX_TILE_COUNT }, In the control config. > * @bit_size: size (in bits) of the current slice data > * @data_bit_offset: offset (in bits) to the video data in the current slice data > * @nal_unit_type: specifies the coding type of the slice (B, P or I)
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]; >> }; >>
Le 08/04/2022 à 20:53, Nicolas Dufresne a écrit : > Le jeudi 07 avril 2022 à 17:29 +0200, Benjamin Gaignard a écrit : >> Make explicit that V4L2_CID_STATELESS_HEVC_SLICE_PARAMS control is >> a dynamic array control type. >> Some drivers may be able to receive multiple slices in one control >> to improve decoding performance. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 2 ++ >> include/media/hevc-ctrls.h | 3 +++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> index 7fcc6c2bac4c..1aaf3b84b584 100644 >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> @@ -2964,6 +2964,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - >> These bitstream parameters are defined according to :ref:`hevc`. >> They are described in section 7.4.7 "General slice segment header >> semantics" of the specification. >> + This control is a dynamically sized 1-dimensional array, >> + V4L2_CTRL_FLAG_DYNAMIC_ARRAY flag must be set when using it. >> >> .. c:type:: v4l2_ctrl_hevc_slice_params >> >> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h >> index 96a9b71d1def..28049e1a8a07 100644 >> --- a/include/media/hevc-ctrls.h >> +++ b/include/media/hevc-ctrls.h >> @@ -314,6 +314,9 @@ struct v4l2_hevc_pred_weight_table { >> /** >> * v4l2_ctrl_hevc_slice_params - HEVC slice parameters >> * >> + * This control is a dynamically sized 1-dimensional array, >> + * V4L2_CTRL_FLAG_DYNAMIC_ARRAY flag must be set when using it. >> + * > I'm not sure about this comment, but I'm sure you forgot: > > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c > @@ -1505,6 +1505,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum > v4l2_ctrl_type *type, > break; > case V4L2_CID_STATELESS_HEVC_SLICE_PARAMS: > *type = V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS; > + *flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY; > break; > case V4L2_CID_STATELESS_HEVC_SCALING_MATRIX: > *type = V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX; > > Also, in AV1 we have V4L2_AV1_MAX_TILE_COUNT, which can be used in cfg.dims, > something the driver will actually set. We could have > > #define V4L2_HEVC_MAX_TILE_COUNT 600 > > And drives can set: > > .dims = { V4L2_HEVC_MAX_TILE_COUNT }, > > In the control config. Ok that will be in v6 > >> * @bit_size: size (in bits) of the current slice data >> * @data_bit_offset: offset (in bits) to the video data in the current slice data >> * @nal_unit_type: specifies the coding type of the slice (B, P or I)
This series aims to make HEVC uapi stable and usable for hardware decoder. HEVC uapi is used by 2 mainlined drivers (Cedrus and Hantro) and 2 out of the tree drivers (rkvdec and RPI). version 5: - Change __u16 pic_order_cnt[2] into __s32 pic_order_cnt_val in hevc_dpb_entry structure - Add defines for SEI pic_struct values (patch 4) - Fix numbers of bits computation in cedrus_h265_skip_bits() parameters - Fix num_short_term_ref_pic_sets and num_long_term_ref_pics_sps documentation (patch 8) - Rebased on v5-18-rc1 GStreamer H265 decoder plugin aligned with HEVC uAPI v5: https://gitlab.freedesktop.org/benjamin.gaignard1/gstreamer/-/tree/HEVC_aligned_with_kernel_5.15 Version 4: - Add num_entry_point_offsets field in struct v4l2_ctrl_hevc_slice_params - Fix V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS name - Initialize control V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS - Fix space/tab issue in kernel-doc - Add patch to change data_bit_offset definition - Fix hantro-media SPDX license - put controls under stateless section in v4l2-ctrls-defs.c At the end fluster tests results on IMX8MQ is 77/147 for HEVC codec. Benjamin Gaignard (14): media: uapi: HEVC: Add missing fields in HEVC controls media: uapi: HEVC: Rename HEVC stateless controls with STATELESS prefix media: uapi: HEVC: Change pic_order_cnt definition in v4l2_hevc_dpb_entry media: uapi: HEVC: Add SEI pic struct flags media: uapi: HEVC: Add document uAPI structure media: uapi: HEVC: Define V4L2_CID_STATELESS_HEVC_SLICE_PARAMS as a dynamic array media: uapi: Move parsed HEVC pixel format out of staging media: uapi: Add V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS control media: uapi: Move the HEVC stateless control type out of staging media: controls: Log HEVC stateless control in .std_log media: uapi: Create a dedicated header for Hantro control media: uapi: HEVC: fix padding in v4l2 control structures media: uapi: Change data_bit_offset definition media: uapi: move HEVC stateless controls out of staging Hans Verkuil (3): videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY v4l2-ctrls: add support for dynamically allocated arrays. vivid: add dynamic array test control .../userspace-api/media/drivers/hantro.rst | 5 - .../media/v4l/ext-ctrls-codec-stateless.rst | 882 ++++++++++++++++++ .../media/v4l/ext-ctrls-codec.rst | 780 ---------------- .../media/v4l/pixfmt-compressed.rst | 7 +- .../media/v4l/vidioc-g-ext-ctrls.rst | 20 + .../media/v4l/vidioc-queryctrl.rst | 8 + .../media/videodev2.h.rst.exceptions | 5 + .../media/test-drivers/vivid/vivid-ctrls.c | 15 + drivers/media/v4l2-core/v4l2-ctrls-api.c | 103 +- drivers/media/v4l2-core/v4l2-ctrls-core.c | 198 +++- drivers/media/v4l2-core/v4l2-ctrls-defs.c | 37 +- drivers/media/v4l2-core/v4l2-ctrls-priv.h | 3 +- drivers/media/v4l2-core/v4l2-ctrls-request.c | 13 +- drivers/staging/media/hantro/hantro_drv.c | 27 +- .../staging/media/hantro/hantro_g2_hevc_dec.c | 4 +- drivers/staging/media/hantro/hantro_hevc.c | 10 +- drivers/staging/media/hantro/hantro_hw.h | 4 +- drivers/staging/media/sunxi/cedrus/cedrus.c | 24 +- .../staging/media/sunxi/cedrus/cedrus_dec.c | 10 +- .../staging/media/sunxi/cedrus/cedrus_h265.c | 6 +- include/media/hevc-ctrls.h | 250 ----- include/media/v4l2-ctrls.h | 48 +- include/uapi/linux/hantro-media.h | 19 + include/uapi/linux/v4l2-controls.h | 453 +++++++++ include/uapi/linux/videodev2.h | 13 + 25 files changed, 1767 insertions(+), 1177 deletions(-) delete mode 100644 include/media/hevc-ctrls.h create mode 100644 include/uapi/linux/hantro-media.h