[PATCH 0/2] media: HEVC: RPS clean up

[PATCH 0/2] media: HEVC: RPS clean up

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

Benjamin

Benjamin Gaignard (2):
  media: hevc: Remove RPS named flags
  media: hevc: Embedded indexes in RPS

 .../media/v4l/ext-ctrls-codec.rst             | 12 ++++-----
 .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++++--------------
 .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
 include/media/hevc-ctrls.h                    |  4 +--
 4 files changed, 14 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH 1/2] media: hevc: Remove RPS named flags

[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>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++----
 drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
 drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
 include/media/hevc-ctrls.h                                | 4 +---
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 3865acb9e0fd..eff33c511090 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -3138,10 +3138,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
 	: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)
+      - 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 9ea864ca5625..be46b3c28b17 100644
--- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
@@ -503,7 +503,7 @@ static int set_ref(struct hantro_ctx *ctx)
 		compress_luma_addr = luma_addr + compress_luma_offset;
 		compress_chroma_addr = luma_addr + compress_chroma_offset;
 
-		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
+		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
 			dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i);
 
 		/*
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index ef0311a16d01..6086cc35e8cc 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].rps == 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..f587448ef495 100644
--- a/include/media/hevc-ctrls.h
+++ b/include/media/hevc-ctrls.h
@@ -127,9 +127,7 @@ 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
 
-- 
2.25.1

Re: [PATCH 1/2] media: hevc: Remove RPS named flags

[John Cox]

>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>
>---
> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++----
> drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
> include/media/hevc-ctrls.h                                | 4 +---
> 4 files changed, 5 insertions(+), 9 deletions(-)
>
>diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>index 3865acb9e0fd..eff33c511090 100644
>--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>@@ -3138,10 +3138,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> 	: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)
>+      - 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.

If you are going to remove all the RPS values except for Long Term
wouldn't it be better to rename the field too, either to "flags" or a
bool "is_long_term"?  If we have a field called RPS it really should be
able to have a value for any of the 5 valid Reference Picture Sets that
a DPB entry can belong to.

As a side note, it is important to my code that the DPB array contains
all the DPB entries not just the ones that are in use in this frame.  I
need them so I can track which frames have left the DPB so I can
reuse/free the MV tables associated with them (yes I could keep one for
every entry in the capture Q but that is generally wasteful on memory
and the Pi is often memory constrained). So maybe update the docn on DPB
to make this explicit please? (I suspect that current code does this
anyway as it is generally easier to do than to not.)

John Cox

>diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>index 9ea864ca5625..be46b3c28b17 100644
>--- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>+++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>@@ -503,7 +503,7 @@ static int set_ref(struct hantro_ctx *ctx)
> 		compress_luma_addr = luma_addr + compress_luma_offset;
> 		compress_chroma_addr = luma_addr + compress_chroma_offset;
> 
>-		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
>+		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
> 			dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i);
> 
> 		/*
>diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>index ef0311a16d01..6086cc35e8cc 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].rps == 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..f587448ef495 100644
>--- a/include/media/hevc-ctrls.h
>+++ b/include/media/hevc-ctrls.h
>@@ -127,9 +127,7 @@ 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
> 

Re: [PATCH 1/2] media: hevc: Remove RPS named flags

[Benjamin Gaignard]

Le 31/08/2021 à 13:08, John Cox a écrit :
>> 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>
>> ---
>> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++----
>> drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
>> drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
>> include/media/hevc-ctrls.h                                | 4 +---
>> 4 files changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index 3865acb9e0fd..eff33c511090 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -3138,10 +3138,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>> 	: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)
>> +      - 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.
> If you are going to remove all the RPS values except for Long Term
> wouldn't it be better to rename the field too, either to "flags" or a
> bool "is_long_term"?  If we have a field called RPS it really should be
> able to have a value for any of the 5 valid Reference Picture Sets that
> a DPB entry can belong to.

I will send a v2 and rename rps into flags.

>
> As a side note, it is important to my code that the DPB array contains
> all the DPB entries not just the ones that are in use in this frame.  I
> need them so I can track which frames have left the DPB so I can
> reuse/free the MV tables associated with them (yes I could keep one for
> every entry in the capture Q but that is generally wasteful on memory
> and the Pi is often memory constrained). So maybe update the docn on DPB
> to make this explicit please? (I suspect that current code does this
> anyway as it is generally easier to do than to not.)

That should be in another patch :-)

Benjamin

>
> John Cox
>
>> diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>> index 9ea864ca5625..be46b3c28b17 100644
>> --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>> +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>> @@ -503,7 +503,7 @@ static int set_ref(struct hantro_ctx *ctx)
>> 		compress_luma_addr = luma_addr + compress_luma_offset;
>> 		compress_chroma_addr = luma_addr + compress_chroma_offset;
>>
>> -		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
>> +		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
>> 			dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i);
>>
>> 		/*
>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>> index ef0311a16d01..6086cc35e8cc 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].rps == 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..f587448ef495 100644
>> --- a/include/media/hevc-ctrls.h
>> +++ b/include/media/hevc-ctrls.h
>> @@ -127,9 +127,7 @@ 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
>>

Re: [PATCH 1/2] media: hevc: Remove RPS named flags

[John Cox]

>Le 31/08/2021 à 13:08, John Cox a écrit :
>>> 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>
>>> ---
>>> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++----
>>> drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
>>> drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
>>> include/media/hevc-ctrls.h                                | 4 +---
>>> 4 files changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index 3865acb9e0fd..eff33c511090 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -3138,10 +3138,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>> 	: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)
>>> +      - 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.
>> If you are going to remove all the RPS values except for Long Term
>> wouldn't it be better to rename the field too, either to "flags" or a
>> bool "is_long_term"?  If we have a field called RPS it really should be
>> able to have a value for any of the 5 valid Reference Picture Sets that
>> a DPB entry can belong to.
>
>I will send a v2 and rename rps into flags.

OK. I was going to say that you should merge the "field_pic" entry into
a flags bitfield, but then I remembered that H265 doesn't have the
concept of field_pics in the way that H264 does and I believe that both
it and pic_order_count[1] are redundant (i.e. can never be used). But I
guess that is the subject of yet another patch.

Regards

John Cox

>> As a side note, it is important to my code that the DPB array contains
>> all the DPB entries not just the ones that are in use in this frame.  I
>> need them so I can track which frames have left the DPB so I can
>> reuse/free the MV tables associated with them (yes I could keep one for
>> every entry in the capture Q but that is generally wasteful on memory
>> and the Pi is often memory constrained). So maybe update the docn on DPB
>> to make this explicit please? (I suspect that current code does this
>> anyway as it is generally easier to do than to not.)
>
>That should be in another patch :-)
>
>Benjamin
>
>>
>> John Cox
>>
>>> diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>>> index 9ea864ca5625..be46b3c28b17 100644
>>> --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>>> +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
>>> @@ -503,7 +503,7 @@ static int set_ref(struct hantro_ctx *ctx)
>>> 		compress_luma_addr = luma_addr + compress_luma_offset;
>>> 		compress_chroma_addr = luma_addr + compress_chroma_offset;
>>>
>>> -		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
>>> +		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
>>> 			dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i);
>>>
>>> 		/*
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>>> index ef0311a16d01..6086cc35e8cc 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].rps == 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..f587448ef495 100644
>>> --- a/include/media/hevc-ctrls.h
>>> +++ b/include/media/hevc-ctrls.h
>>> @@ -127,9 +127,7 @@ 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
>>>

[PATCH 2/2] media: hevc: Embedded indexes in RPS

[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 eff33c511090..4e4892c37723 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -3352,15 +3352,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 be46b3c28b17..76386e9eb8f4 100644
--- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
@@ -306,24 +306,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[] = {
@@ -367,11 +354,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;
@@ -380,11 +367,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.25.1

Re: [PATCH 0/2] media: HEVC: RPS clean up

[Nicolas Dufresne]

As per discussion with Ezequiel, I think this patch should perhaps be made an
RFC titled "media: uapi: clean and make HEVC uAPI public". Keep it RFC until we
have collected all the needed changes.

Opinion ?

Le mardi 31 août 2021 à 11:48 +0200, Benjamin Gaignard a écrit :
> 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).
> 
> Benjamin
> 
> Benjamin Gaignard (2):
>   media: hevc: Remove RPS named flags
>   media: hevc: Embedded indexes in RPS
> 
>  .../media/v4l/ext-ctrls-codec.rst             | 12 ++++-----
>  .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++++--------------
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>  include/media/hevc-ctrls.h                    |  4 +--
>  4 files changed, 14 insertions(+), 31 deletions(-)
> 

Re: [PATCH 0/2] media: HEVC: RPS clean up

[Ezequiel Garcia]

Hans, Nicolas,

On Tue, 31 Aug 2021 at 16:19, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> As per discussion with Ezequiel, I think this patch should perhaps be made an
> RFC titled "media: uapi: clean and make HEVC uAPI public". Keep it RFC until we
> have collected all the needed changes.
>
> Opinion ?

I agree.

IMO, it makes no sense to keep merging anything on Hantro G2 HEVC
support, until the BLK-CTRL part is ready [1]. Without that, we can't
test anything
so we may introduce regressions without knowing.

Also, I think it's time to start discussing the HEVC uAPI as a whole,
with the goal is having a stable interface that can be used by userspace
frameworks (GStreamer).

[1] https://www.spinics.net/lists/arm-kernel/msg913881.html

Thanks,
Ezequiel