* [PATCH v4 0/2] media: HEVC: RPS clean up @ 2022-01-04 7:38 ` Benjamin Gaignard 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-04 7:38 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard This series aims to clean up Reference Picture Set usage and flags. Long term flag was named with RPS prefix while it is not used for RPS but for mark long term references in DBP. Remane it and remove the two other useless RPS flags. Clarify documentation about RPS lists content and make sure that Hantro driver use them correctly (i.e without look up in DBP). version 4: - check flags with & and not == - remove the 2 last patches which becomes useless since VPU ctrl-blk driver introduction. version 3: - rebased on top of v5.16-rc1 version 2: - change DPB field name from rps to flags GStreamer HEVC plugin merge request can be found here: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079 With those piece of code fluster score is 77/147. Benjamin Gaignard (2): media: hevc: Remove RPS named flags media: hevc: Embedded indexes in RPS .../media/v4l/ext-ctrls-codec.rst | 14 +++++----- .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++++-------------- .../staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- include/media/hevc-ctrls.h | 6 ++--- 4 files changed, 16 insertions(+), 33 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 0/2] media: HEVC: RPS clean up @ 2022-01-04 7:38 ` Benjamin Gaignard 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-04 7:38 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard This series aims to clean up Reference Picture Set usage and flags. Long term flag was named with RPS prefix while it is not used for RPS but for mark long term references in DBP. Remane it and remove the two other useless RPS flags. Clarify documentation about RPS lists content and make sure that Hantro driver use them correctly (i.e without look up in DBP). version 4: - check flags with & and not == - remove the 2 last patches which becomes useless since VPU ctrl-blk driver introduction. version 3: - rebased on top of v5.16-rc1 version 2: - change DPB field name from rps to flags GStreamer HEVC plugin merge request can be found here: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079 With those piece of code fluster score is 77/147. Benjamin Gaignard (2): media: hevc: Remove RPS named flags media: hevc: Embedded indexes in RPS .../media/v4l/ext-ctrls-codec.rst | 14 +++++----- .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++++-------------- .../staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- include/media/hevc-ctrls.h | 6 ++--- 4 files changed, 16 insertions(+), 33 deletions(-) -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 1/2] media: hevc: Remove RPS named flags 2022-01-04 7:38 ` Benjamin Gaignard @ 2022-01-04 7:38 ` Benjamin Gaignard -1 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-04 7:38 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard Marking a picture as long-term reference is valid for DPB but not for RPS. Change flag name to match with it description in HEVC spec chapiter "8.3.2 Decoding process for reference picture set". Remove the other unused RPS flags. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- version 4: - check flags with & and not == Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++----- drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +- drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- include/media/hevc-ctrls.h | 6 ++---- 4 files changed, 7 insertions(+), 11 deletions(-) diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index e141f0e4eec9..38da33e61c3d 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - :c:func:`v4l2_timeval_to_ns()` function to convert the struct :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64. * - __u8 - - ``rps`` - - The reference set for the reference frame - (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE, - V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or - V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) + - ``flags`` + - Long term flag for the reference frame + (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) * - __u8 - ``field_pic`` - Whether the reference is a field picture or a frame. diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c index 99d8ea7543da..14e0e6414100 100644 --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c @@ -433,7 +433,7 @@ static int set_ref(struct hantro_ctx *ctx) chroma_addr = luma_addr + cr_offset; mv_addr = luma_addr + mv_offset; - if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) + if (dpb[i].flags & V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i); hantro_write_addr(vpu, G2_REF_LUMA_ADDR(i), luma_addr); diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index 8829a7bab07e..8ab2d9c6f048 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c @@ -169,7 +169,7 @@ static void cedrus_h265_ref_pic_list_write(struct cedrus_dev *dev, unsigned int index = list[i]; u8 value = list[i]; - if (dpb[index].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) + if (dpb[index].flags & V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) value |= VE_DEC_H265_SRAM_REF_PIC_LIST_LT_REF; /* Each SRAM word gathers up to 4 references. */ diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h index ef63bc205756..01ccda48d8c5 100644 --- a/include/media/hevc-ctrls.h +++ b/include/media/hevc-ctrls.h @@ -127,15 +127,13 @@ struct v4l2_ctrl_hevc_pps { __u64 flags; }; -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE 0x01 -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER 0x02 -#define V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR 0x03 +#define V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE 0x01 #define V4L2_HEVC_DPB_ENTRIES_NUM_MAX 16 struct v4l2_hevc_dpb_entry { __u64 timestamp; - __u8 rps; + __u8 flags; __u8 field_pic; __u16 pic_order_cnt[2]; __u8 padding[2]; -- 2.30.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4 1/2] media: hevc: Remove RPS named flags @ 2022-01-04 7:38 ` Benjamin Gaignard 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-04 7:38 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard Marking a picture as long-term reference is valid for DPB but not for RPS. Change flag name to match with it description in HEVC spec chapiter "8.3.2 Decoding process for reference picture set". Remove the other unused RPS flags. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- version 4: - check flags with & and not == Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++----- drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +- drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- include/media/hevc-ctrls.h | 6 ++---- 4 files changed, 7 insertions(+), 11 deletions(-) diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index e141f0e4eec9..38da33e61c3d 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - :c:func:`v4l2_timeval_to_ns()` function to convert the struct :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64. * - __u8 - - ``rps`` - - The reference set for the reference frame - (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE, - V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or - V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) + - ``flags`` + - Long term flag for the reference frame + (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) * - __u8 - ``field_pic`` - Whether the reference is a field picture or a frame. diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c index 99d8ea7543da..14e0e6414100 100644 --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c @@ -433,7 +433,7 @@ static int set_ref(struct hantro_ctx *ctx) chroma_addr = luma_addr + cr_offset; mv_addr = luma_addr + mv_offset; - if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) + if (dpb[i].flags & V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i); hantro_write_addr(vpu, G2_REF_LUMA_ADDR(i), luma_addr); diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index 8829a7bab07e..8ab2d9c6f048 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c @@ -169,7 +169,7 @@ static void cedrus_h265_ref_pic_list_write(struct cedrus_dev *dev, unsigned int index = list[i]; u8 value = list[i]; - if (dpb[index].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) + if (dpb[index].flags & V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) value |= VE_DEC_H265_SRAM_REF_PIC_LIST_LT_REF; /* Each SRAM word gathers up to 4 references. */ diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h index ef63bc205756..01ccda48d8c5 100644 --- a/include/media/hevc-ctrls.h +++ b/include/media/hevc-ctrls.h @@ -127,15 +127,13 @@ struct v4l2_ctrl_hevc_pps { __u64 flags; }; -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE 0x01 -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER 0x02 -#define V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR 0x03 +#define V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE 0x01 #define V4L2_HEVC_DPB_ENTRIES_NUM_MAX 16 struct v4l2_hevc_dpb_entry { __u64 timestamp; - __u8 rps; + __u8 flags; __u8 field_pic; __u16 pic_order_cnt[2]; __u8 padding[2]; -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags 2022-01-04 7:38 ` Benjamin Gaignard @ 2022-01-04 15:34 ` Jernej Škrabec -1 siblings, 0 replies; 14+ messages in thread From: Jernej Škrabec @ 2022-01-04 15:34 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, nicolas.dufresne, Benjamin Gaignard Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard Hi Benjamin! Dne torek, 04. januar 2022 ob 08:38:41 CET je Benjamin Gaignard napisal(a): > Marking a picture as long-term reference is valid for DPB but not for RPS. > Change flag name to match with it description in HEVC spec chapiter > "8.3.2 Decoding process for reference picture set". > Remove the other unused RPS flags. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags @ 2022-01-04 15:34 ` Jernej Škrabec 0 siblings, 0 replies; 14+ messages in thread From: Jernej Škrabec @ 2022-01-04 15:34 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, nicolas.dufresne, Benjamin Gaignard Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard Hi Benjamin! Dne torek, 04. januar 2022 ob 08:38:41 CET je Benjamin Gaignard napisal(a): > Marking a picture as long-term reference is valid for DPB but not for RPS. > Change flag name to match with it description in HEVC spec chapiter > "8.3.2 Decoding process for reference picture set". > Remove the other unused RPS flags. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags 2022-01-04 7:38 ` Benjamin Gaignard @ 2022-01-05 12:26 ` Ezequiel Garcia -1 siblings, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2022-01-05 12:26 UTC (permalink / raw) To: Benjamin Gaignard Cc: mchehab, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne, linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel Hi Benjamin, On Tue, Jan 04, 2022 at 08:38:41AM +0100, Benjamin Gaignard wrote: > Marking a picture as long-term reference is valid for DPB but not for RPS. > Change flag name to match with it description in HEVC spec chapiter > "8.3.2 Decoding process for reference picture set". > Remove the other unused RPS flags. > A change like this, which is affecting a kernel interface and has impact on userspace and drivers, requires a lot more attention in the commit description. If I understand correctly, this change is related to how HEVC was first introduced and how the DPB was originally represented in V4L2. Originally, a DPB entry struct v4l2_hevc_dpb_entry had an rps field which can be: V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR Perhaps this idea followed libva, where a VAPictureHEVC has flags field which can be: VA_PICTURE_HEVC_RPS_ST_CURR_BEFORE, VA_PICTURE_HEVC_RPS_ST_CURR_AFTER, VA_PICTURE_HEVC_RPS_LT_CURR, VA_PICTURE_HEVC_LONG_TERM_REFERENCE In this representation, the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr are implicit, and must be built by the kernel from the DPB entries struct v4l2_hevc_dpb_entry, using the information in the rps field. Last year, we changed this with your commit: commit d395a78db9eabd12633b39e05c80e803543b6590 Author: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: Thu Jun 3 13:49:57 2021 +0200 media: hevc: Add decode params control Which added the control v4l2_ctrl_hevc_decode_params, which includes the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr explicitly and therefore moved the responsability of creating and maintaining the sets to userspace. This effectively made the rps field in the struct v4l2_hevc_dpb_entry useless. The longterm flag is still needed though for a DPB entry. With this in mind, you could even say this commit is doing actually two things: 1. Removes the unused rps field. 2. Adds a flag field, for the longterm DPB entry boolean. Do you think a single byte of flags will be OK for a DPB entry? I think so, but I'd love yours/others input here. If the above is more or less accurate then, and provided you submit a new version with a more detailed commit description: Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Also, a small question: > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > version 4: > - check flags with & and not == > > Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++----- > drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +- > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- > include/media/hevc-ctrls.h | 6 ++---- > 4 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > index e141f0e4eec9..38da33e61c3d 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > :c:func:`v4l2_timeval_to_ns()` function to convert the struct > :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64. > * - __u8 > - - ``rps`` > - - The reference set for the reference frame > - (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE, > - V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or > - V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) > + - ``flags`` > + - Long term flag for the reference frame > + (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) Is this longterm flag associated in any way to a syntax element or an HEVC process? If so, please document that. Thanks, Ezequiel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags @ 2022-01-05 12:26 ` Ezequiel Garcia 0 siblings, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2022-01-05 12:26 UTC (permalink / raw) To: Benjamin Gaignard Cc: mchehab, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne, linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel Hi Benjamin, On Tue, Jan 04, 2022 at 08:38:41AM +0100, Benjamin Gaignard wrote: > Marking a picture as long-term reference is valid for DPB but not for RPS. > Change flag name to match with it description in HEVC spec chapiter > "8.3.2 Decoding process for reference picture set". > Remove the other unused RPS flags. > A change like this, which is affecting a kernel interface and has impact on userspace and drivers, requires a lot more attention in the commit description. If I understand correctly, this change is related to how HEVC was first introduced and how the DPB was originally represented in V4L2. Originally, a DPB entry struct v4l2_hevc_dpb_entry had an rps field which can be: V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR Perhaps this idea followed libva, where a VAPictureHEVC has flags field which can be: VA_PICTURE_HEVC_RPS_ST_CURR_BEFORE, VA_PICTURE_HEVC_RPS_ST_CURR_AFTER, VA_PICTURE_HEVC_RPS_LT_CURR, VA_PICTURE_HEVC_LONG_TERM_REFERENCE In this representation, the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr are implicit, and must be built by the kernel from the DPB entries struct v4l2_hevc_dpb_entry, using the information in the rps field. Last year, we changed this with your commit: commit d395a78db9eabd12633b39e05c80e803543b6590 Author: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: Thu Jun 3 13:49:57 2021 +0200 media: hevc: Add decode params control Which added the control v4l2_ctrl_hevc_decode_params, which includes the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr explicitly and therefore moved the responsability of creating and maintaining the sets to userspace. This effectively made the rps field in the struct v4l2_hevc_dpb_entry useless. The longterm flag is still needed though for a DPB entry. With this in mind, you could even say this commit is doing actually two things: 1. Removes the unused rps field. 2. Adds a flag field, for the longterm DPB entry boolean. Do you think a single byte of flags will be OK for a DPB entry? I think so, but I'd love yours/others input here. If the above is more or less accurate then, and provided you submit a new version with a more detailed commit description: Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Also, a small question: > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > version 4: > - check flags with & and not == > > Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++----- > drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +- > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- > include/media/hevc-ctrls.h | 6 ++---- > 4 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > index e141f0e4eec9..38da33e61c3d 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > :c:func:`v4l2_timeval_to_ns()` function to convert the struct > :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64. > * - __u8 > - - ``rps`` > - - The reference set for the reference frame > - (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE, > - V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or > - V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) > + - ``flags`` > + - Long term flag for the reference frame > + (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) Is this longterm flag associated in any way to a syntax element or an HEVC process? If so, please document that. Thanks, Ezequiel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags 2022-01-05 12:26 ` Ezequiel Garcia @ 2022-01-07 16:02 ` Benjamin Gaignard -1 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-07 16:02 UTC (permalink / raw) To: Ezequiel Garcia Cc: mchehab, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne, linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel Le 05/01/2022 à 13:26, Ezequiel Garcia a écrit : > Hi Benjamin, > > On Tue, Jan 04, 2022 at 08:38:41AM +0100, Benjamin Gaignard wrote: >> Marking a picture as long-term reference is valid for DPB but not for RPS. >> Change flag name to match with it description in HEVC spec chapiter >> "8.3.2 Decoding process for reference picture set". >> Remove the other unused RPS flags. >> > A change like this, which is affecting a kernel interface and has impact > on userspace and drivers, requires a lot more attention in the commit description. > > If I understand correctly, this change is related to how HEVC was first > introduced and how the DPB was originally represented in V4L2. > > Originally, a DPB entry struct v4l2_hevc_dpb_entry had an rps field > which can be: > > V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE > V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER > V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR > > Perhaps this idea followed libva, where a VAPictureHEVC has flags field > which can be: > > VA_PICTURE_HEVC_RPS_ST_CURR_BEFORE, > VA_PICTURE_HEVC_RPS_ST_CURR_AFTER, > VA_PICTURE_HEVC_RPS_LT_CURR, > VA_PICTURE_HEVC_LONG_TERM_REFERENCE > > In this representation, the sets PocStCurrBefore, PocStCurrAfter, > PocLtCurr are implicit, and must be built by the kernel from the DPB > entries struct v4l2_hevc_dpb_entry, using the information in the rps field. > > Last year, we changed this with your commit: > > commit d395a78db9eabd12633b39e05c80e803543b6590 > Author: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Date: Thu Jun 3 13:49:57 2021 +0200 > > media: hevc: Add decode params control > > Which added the control v4l2_ctrl_hevc_decode_params, which includes > the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr explicitly and therefore > moved the responsability of creating and maintaining the sets to userspace. > > This effectively made the rps field in the struct v4l2_hevc_dpb_entry > useless. The longterm flag is still needed though for a DPB entry. > > With this in mind, you could even say this commit is doing actually two > things: > > 1. Removes the unused rps field. > 2. Adds a flag field, for the longterm DPB entry boolean. > > Do you think a single byte of flags will be OK for a DPB entry? > I think so, but I'd love yours/others input here. In ITU HEVC spec it is said that: "When a picture is referred to as being marked as "used for reference", this collectively refers to the picture being marked as "used for short-term reference" or "used for long-term reference" (but not both)." So I believe that a single if enough for DPB entry. > > If the above is more or less accurate then, and provided you > submit a new version with a more detailed commit description: I will follow your suggestions and improve it in v5 > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > Also, a small question: > >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> version 4: >> - check flags with & and not == >> >> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++----- >> drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +- >> drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- >> include/media/hevc-ctrls.h | 6 ++---- >> 4 files changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> index e141f0e4eec9..38da33e61c3d 100644 >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - >> :c:func:`v4l2_timeval_to_ns()` function to convert the struct >> :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64. >> * - __u8 >> - - ``rps`` >> - - The reference set for the reference frame >> - (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE, >> - V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or >> - V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) >> + - ``flags`` >> + - Long term flag for the reference frame >> + (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) > Is this longterm flag associated in any way to a syntax element > or an HEVC process? If so, please document that. done in v5. Thanks for your review. Benjamin > > Thanks, > Ezequiel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags @ 2022-01-07 16:02 ` Benjamin Gaignard 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-07 16:02 UTC (permalink / raw) To: Ezequiel Garcia Cc: mchehab, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne, linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel Le 05/01/2022 à 13:26, Ezequiel Garcia a écrit : > Hi Benjamin, > > On Tue, Jan 04, 2022 at 08:38:41AM +0100, Benjamin Gaignard wrote: >> Marking a picture as long-term reference is valid for DPB but not for RPS. >> Change flag name to match with it description in HEVC spec chapiter >> "8.3.2 Decoding process for reference picture set". >> Remove the other unused RPS flags. >> > A change like this, which is affecting a kernel interface and has impact > on userspace and drivers, requires a lot more attention in the commit description. > > If I understand correctly, this change is related to how HEVC was first > introduced and how the DPB was originally represented in V4L2. > > Originally, a DPB entry struct v4l2_hevc_dpb_entry had an rps field > which can be: > > V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE > V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER > V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR > > Perhaps this idea followed libva, where a VAPictureHEVC has flags field > which can be: > > VA_PICTURE_HEVC_RPS_ST_CURR_BEFORE, > VA_PICTURE_HEVC_RPS_ST_CURR_AFTER, > VA_PICTURE_HEVC_RPS_LT_CURR, > VA_PICTURE_HEVC_LONG_TERM_REFERENCE > > In this representation, the sets PocStCurrBefore, PocStCurrAfter, > PocLtCurr are implicit, and must be built by the kernel from the DPB > entries struct v4l2_hevc_dpb_entry, using the information in the rps field. > > Last year, we changed this with your commit: > > commit d395a78db9eabd12633b39e05c80e803543b6590 > Author: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Date: Thu Jun 3 13:49:57 2021 +0200 > > media: hevc: Add decode params control > > Which added the control v4l2_ctrl_hevc_decode_params, which includes > the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr explicitly and therefore > moved the responsability of creating and maintaining the sets to userspace. > > This effectively made the rps field in the struct v4l2_hevc_dpb_entry > useless. The longterm flag is still needed though for a DPB entry. > > With this in mind, you could even say this commit is doing actually two > things: > > 1. Removes the unused rps field. > 2. Adds a flag field, for the longterm DPB entry boolean. > > Do you think a single byte of flags will be OK for a DPB entry? > I think so, but I'd love yours/others input here. In ITU HEVC spec it is said that: "When a picture is referred to as being marked as "used for reference", this collectively refers to the picture being marked as "used for short-term reference" or "used for long-term reference" (but not both)." So I believe that a single if enough for DPB entry. > > If the above is more or less accurate then, and provided you > submit a new version with a more detailed commit description: I will follow your suggestions and improve it in v5 > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > Also, a small question: > >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> version 4: >> - check flags with & and not == >> >> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++----- >> drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +- >> drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- >> include/media/hevc-ctrls.h | 6 ++---- >> 4 files changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> index e141f0e4eec9..38da33e61c3d 100644 >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - >> :c:func:`v4l2_timeval_to_ns()` function to convert the struct >> :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64. >> * - __u8 >> - - ``rps`` >> - - The reference set for the reference frame >> - (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE, >> - V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or >> - V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) >> + - ``flags`` >> + - Long term flag for the reference frame >> + (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) > Is this longterm flag associated in any way to a syntax element > or an HEVC process? If so, please document that. done in v5. Thanks for your review. Benjamin > > Thanks, > Ezequiel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] media: hevc: Embedded indexes in RPS 2022-01-04 7:38 ` Benjamin Gaignard @ 2022-01-04 7:38 ` Benjamin Gaignard -1 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-04 7:38 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard Reference Picture Set lists provide indexes of short and long term reference in DBP array. Fix Hantro to not do a look up in DBP entries. Make documentation more clear about it. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- .../media/v4l/ext-ctrls-codec.rst | 6 ++--- .../staging/media/hantro/hantro_g2_hevc_dec.c | 25 +++++-------------- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index 38da33e61c3d..b12ad5b3eaba 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst @@ -3381,15 +3381,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - * - __u8 - ``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` - PocStCurrBefore as described in section 8.3.2 "Decoding process for reference - picture set. + picture set": provides the index of the short term before references in DPB array. * - __u8 - ``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` - PocStCurrAfter as described in section 8.3.2 "Decoding process for reference - picture set. + picture set": provides the index of the short term after references in DPB array. * - __u8 - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` - PocLtCurr as described in section 8.3.2 "Decoding process for reference - picture set. + picture set": provides the index of the long term references in DPB array. * - __u64 - ``flags`` - See :ref:`Decode Parameters Flags <hevc_decode_params_flags>` diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c index 14e0e6414100..c524af41baf5 100644 --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c @@ -255,24 +255,11 @@ static void set_params(struct hantro_ctx *ctx) hantro_reg_write(vpu, &g2_apf_threshold, 8); } -static int find_ref_pic_index(const struct v4l2_hevc_dpb_entry *dpb, int pic_order_cnt) -{ - int i; - - for (i = 0; i < V4L2_HEVC_DPB_ENTRIES_NUM_MAX; i++) { - if (dpb[i].pic_order_cnt[0] == pic_order_cnt) - return i; - } - - return 0x0; -} - static void set_ref_pic_list(struct hantro_ctx *ctx) { const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls; struct hantro_dev *vpu = ctx->dev; const struct v4l2_ctrl_hevc_decode_params *decode_params = ctrls->decode_params; - const struct v4l2_hevc_dpb_entry *dpb = decode_params->dpb; u32 list0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; u32 list1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; static const struct hantro_reg ref_pic_regs0[] = { @@ -316,11 +303,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) /* List 0 contains: short term before, short term after and long term */ j = 0; for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list0); i++) - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); + list0[j++] = decode_params->poc_st_curr_before[i]; for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list0); i++) - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); + list0[j++] = decode_params->poc_st_curr_after[i]; for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list0); i++) - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); + list0[j++] = decode_params->poc_lt_curr[i]; /* Fill the list, copying over and over */ i = 0; @@ -329,11 +316,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) j = 0; for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list1); i++) - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); + list1[j++] = decode_params->poc_st_curr_after[i]; for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list1); i++) - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); + list1[j++] = decode_params->poc_st_curr_before[i]; for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list1); i++) - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); + list1[j++] = decode_params->poc_lt_curr[i]; i = 0; while (j < ARRAY_SIZE(list1)) -- 2.30.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] media: hevc: Embedded indexes in RPS @ 2022-01-04 7:38 ` Benjamin Gaignard 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2022-01-04 7:38 UTC (permalink / raw) To: mchehab, ezequiel, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne Cc: linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel, Benjamin Gaignard Reference Picture Set lists provide indexes of short and long term reference in DBP array. Fix Hantro to not do a look up in DBP entries. Make documentation more clear about it. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- .../media/v4l/ext-ctrls-codec.rst | 6 ++--- .../staging/media/hantro/hantro_g2_hevc_dec.c | 25 +++++-------------- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index 38da33e61c3d..b12ad5b3eaba 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst @@ -3381,15 +3381,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - * - __u8 - ``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` - PocStCurrBefore as described in section 8.3.2 "Decoding process for reference - picture set. + picture set": provides the index of the short term before references in DPB array. * - __u8 - ``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` - PocStCurrAfter as described in section 8.3.2 "Decoding process for reference - picture set. + picture set": provides the index of the short term after references in DPB array. * - __u8 - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` - PocLtCurr as described in section 8.3.2 "Decoding process for reference - picture set. + picture set": provides the index of the long term references in DPB array. * - __u64 - ``flags`` - See :ref:`Decode Parameters Flags <hevc_decode_params_flags>` diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c index 14e0e6414100..c524af41baf5 100644 --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c @@ -255,24 +255,11 @@ static void set_params(struct hantro_ctx *ctx) hantro_reg_write(vpu, &g2_apf_threshold, 8); } -static int find_ref_pic_index(const struct v4l2_hevc_dpb_entry *dpb, int pic_order_cnt) -{ - int i; - - for (i = 0; i < V4L2_HEVC_DPB_ENTRIES_NUM_MAX; i++) { - if (dpb[i].pic_order_cnt[0] == pic_order_cnt) - return i; - } - - return 0x0; -} - static void set_ref_pic_list(struct hantro_ctx *ctx) { const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls; struct hantro_dev *vpu = ctx->dev; const struct v4l2_ctrl_hevc_decode_params *decode_params = ctrls->decode_params; - const struct v4l2_hevc_dpb_entry *dpb = decode_params->dpb; u32 list0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; u32 list1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; static const struct hantro_reg ref_pic_regs0[] = { @@ -316,11 +303,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) /* List 0 contains: short term before, short term after and long term */ j = 0; for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list0); i++) - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); + list0[j++] = decode_params->poc_st_curr_before[i]; for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list0); i++) - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); + list0[j++] = decode_params->poc_st_curr_after[i]; for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list0); i++) - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); + list0[j++] = decode_params->poc_lt_curr[i]; /* Fill the list, copying over and over */ i = 0; @@ -329,11 +316,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) j = 0; for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list1); i++) - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); + list1[j++] = decode_params->poc_st_curr_after[i]; for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list1); i++) - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); + list1[j++] = decode_params->poc_st_curr_before[i]; for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list1); i++) - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); + list1[j++] = decode_params->poc_lt_curr[i]; i = 0; while (j < ARRAY_SIZE(list1)) -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] media: hevc: Embedded indexes in RPS 2022-01-04 7:38 ` Benjamin Gaignard @ 2022-01-05 12:28 ` Ezequiel Garcia -1 siblings, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2022-01-05 12:28 UTC (permalink / raw) To: Benjamin Gaignard Cc: mchehab, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne, linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel On Tue, Jan 04, 2022 at 08:38:42AM +0100, Benjamin Gaignard wrote: > Reference Picture Set lists provide indexes of short and long term > reference in DBP array. > Fix Hantro to not do a look up in DBP entries. > Make documentation more clear about it. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > .../media/v4l/ext-ctrls-codec.rst | 6 ++--- > .../staging/media/hantro/hantro_g2_hevc_dec.c | 25 +++++-------------- > 2 files changed, 9 insertions(+), 22 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > index 38da33e61c3d..b12ad5b3eaba 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -3381,15 +3381,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > * - __u8 > - ``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > - PocStCurrBefore as described in section 8.3.2 "Decoding process for reference > - picture set. > + picture set": provides the index of the short term before references in DPB array. > * - __u8 > - ``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > - PocStCurrAfter as described in section 8.3.2 "Decoding process for reference > - picture set. > + picture set": provides the index of the short term after references in DPB array. > * - __u8 > - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > - PocLtCurr as described in section 8.3.2 "Decoding process for reference > - picture set. > + picture set": provides the index of the long term references in DPB array. > * - __u64 > - ``flags`` > - See :ref:`Decode Parameters Flags <hevc_decode_params_flags>` > diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c > index 14e0e6414100..c524af41baf5 100644 > --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c > +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c > @@ -255,24 +255,11 @@ static void set_params(struct hantro_ctx *ctx) > hantro_reg_write(vpu, &g2_apf_threshold, 8); > } > > -static int find_ref_pic_index(const struct v4l2_hevc_dpb_entry *dpb, int pic_order_cnt) > -{ > - int i; > - > - for (i = 0; i < V4L2_HEVC_DPB_ENTRIES_NUM_MAX; i++) { > - if (dpb[i].pic_order_cnt[0] == pic_order_cnt) > - return i; > - } > - > - return 0x0; > -} > - > static void set_ref_pic_list(struct hantro_ctx *ctx) > { > const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls; > struct hantro_dev *vpu = ctx->dev; > const struct v4l2_ctrl_hevc_decode_params *decode_params = ctrls->decode_params; > - const struct v4l2_hevc_dpb_entry *dpb = decode_params->dpb; > u32 list0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; > u32 list1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; > static const struct hantro_reg ref_pic_regs0[] = { > @@ -316,11 +303,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) > /* List 0 contains: short term before, short term after and long term */ > j = 0; > for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list0); i++) > - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); > + list0[j++] = decode_params->poc_st_curr_before[i]; > for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list0); i++) > - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); > + list0[j++] = decode_params->poc_st_curr_after[i]; > for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list0); i++) > - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); > + list0[j++] = decode_params->poc_lt_curr[i]; > > /* Fill the list, copying over and over */ > i = 0; > @@ -329,11 +316,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) > > j = 0; > for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list1); i++) > - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); > + list1[j++] = decode_params->poc_st_curr_after[i]; > for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list1); i++) > - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); > + list1[j++] = decode_params->poc_st_curr_before[i]; > for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list1); i++) > - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); > + list1[j++] = decode_params->poc_lt_curr[i]; > > i = 0; > while (j < ARRAY_SIZE(list1)) > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] media: hevc: Embedded indexes in RPS @ 2022-01-05 12:28 ` Ezequiel Garcia 0 siblings, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2022-01-05 12:28 UTC (permalink / raw) To: Benjamin Gaignard Cc: mchehab, p.zabel, gregkh, hverkuil-cisco, jernej.skrabec, nicolas.dufresne, linux-media, linux-kernel, linux-arm-kernel, linux-staging, kernel On Tue, Jan 04, 2022 at 08:38:42AM +0100, Benjamin Gaignard wrote: > Reference Picture Set lists provide indexes of short and long term > reference in DBP array. > Fix Hantro to not do a look up in DBP entries. > Make documentation more clear about it. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > .../media/v4l/ext-ctrls-codec.rst | 6 ++--- > .../staging/media/hantro/hantro_g2_hevc_dec.c | 25 +++++-------------- > 2 files changed, 9 insertions(+), 22 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > index 38da33e61c3d..b12ad5b3eaba 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -3381,15 +3381,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - > * - __u8 > - ``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > - PocStCurrBefore as described in section 8.3.2 "Decoding process for reference > - picture set. > + picture set": provides the index of the short term before references in DPB array. > * - __u8 > - ``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > - PocStCurrAfter as described in section 8.3.2 "Decoding process for reference > - picture set. > + picture set": provides the index of the short term after references in DPB array. > * - __u8 > - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]`` > - PocLtCurr as described in section 8.3.2 "Decoding process for reference > - picture set. > + picture set": provides the index of the long term references in DPB array. > * - __u64 > - ``flags`` > - See :ref:`Decode Parameters Flags <hevc_decode_params_flags>` > diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c > index 14e0e6414100..c524af41baf5 100644 > --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c > +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c > @@ -255,24 +255,11 @@ static void set_params(struct hantro_ctx *ctx) > hantro_reg_write(vpu, &g2_apf_threshold, 8); > } > > -static int find_ref_pic_index(const struct v4l2_hevc_dpb_entry *dpb, int pic_order_cnt) > -{ > - int i; > - > - for (i = 0; i < V4L2_HEVC_DPB_ENTRIES_NUM_MAX; i++) { > - if (dpb[i].pic_order_cnt[0] == pic_order_cnt) > - return i; > - } > - > - return 0x0; > -} > - > static void set_ref_pic_list(struct hantro_ctx *ctx) > { > const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls; > struct hantro_dev *vpu = ctx->dev; > const struct v4l2_ctrl_hevc_decode_params *decode_params = ctrls->decode_params; > - const struct v4l2_hevc_dpb_entry *dpb = decode_params->dpb; > u32 list0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; > u32 list1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {}; > static const struct hantro_reg ref_pic_regs0[] = { > @@ -316,11 +303,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) > /* List 0 contains: short term before, short term after and long term */ > j = 0; > for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list0); i++) > - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); > + list0[j++] = decode_params->poc_st_curr_before[i]; > for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list0); i++) > - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); > + list0[j++] = decode_params->poc_st_curr_after[i]; > for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list0); i++) > - list0[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); > + list0[j++] = decode_params->poc_lt_curr[i]; > > /* Fill the list, copying over and over */ > i = 0; > @@ -329,11 +316,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx) > > j = 0; > for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list1); i++) > - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]); > + list1[j++] = decode_params->poc_st_curr_after[i]; > for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list1); i++) > - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]); > + list1[j++] = decode_params->poc_st_curr_before[i]; > for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list1); i++) > - list1[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]); > + list1[j++] = decode_params->poc_lt_curr[i]; > > i = 0; > while (j < ARRAY_SIZE(list1)) > -- > 2.30.2 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-01-07 16:04 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-04 7:38 [PATCH v4 0/2] media: HEVC: RPS clean up Benjamin Gaignard 2022-01-04 7:38 ` Benjamin Gaignard 2022-01-04 7:38 ` [PATCH v4 1/2] media: hevc: Remove RPS named flags Benjamin Gaignard 2022-01-04 7:38 ` Benjamin Gaignard 2022-01-04 15:34 ` Jernej Škrabec 2022-01-04 15:34 ` Jernej Škrabec 2022-01-05 12:26 ` Ezequiel Garcia 2022-01-05 12:26 ` Ezequiel Garcia 2022-01-07 16:02 ` Benjamin Gaignard 2022-01-07 16:02 ` Benjamin Gaignard 2022-01-04 7:38 ` [PATCH v4 2/2] media: hevc: Embedded indexes in RPS Benjamin Gaignard 2022-01-04 7:38 ` Benjamin Gaignard 2022-01-05 12:28 ` Ezequiel Garcia 2022-01-05 12:28 ` Ezequiel Garcia
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.