[PATCH v1 00/24] H.264 Field Decoding Support for Frame-based Decoders

[PATCH v1 00/24] H.264 Field Decoding Support for Frame-based Decoders

[Nicolas Dufresne]

Until now, only Cedrus (a slice base decoder) supported interlaced decoding.
In order to support field decoding in our frame-based decoder, the v4l2-h264
library needed adaptation to produce the appropriate reference lists.

This patch extends the v4l2-h264 library to produce the larger references list
needed to represent fields separately. Hantro, MTK-VCODEC and RKVDEC drivers
have been adapted to accommodate the larger lists. Though, only Hantro and
RKVDEC actually have HW support for field decoding. So only these two
have been updated to make use of the larger lists. All this work has been
done using the H.264 specification, LibreELEC downstream kernel patches,
Rockchip MPP reference software and Hantro reference software.

All this work have been tested using GStreamer mainline implementation but also
with FFMPEG LibreELEC fork using the testing tool fluster running through the
ITU-T H.264 (2016-02) AVCv2 set of bitstream. Before this patch, the scores
were:

Hantro:
  FFMPEG:
  GSteamer:
RKVDEC:
  FFMPEG:
  GSteamer:

And after these changes:

Hantro:
  FFMPEG:   118/135
  GSteamer: 129/135
RKVDEC:
  FFMPEG:   118/135
  GSteamer: 129/135

Note that a bug in FFMPEG / LibreELEC fork was noticed and fixed with the
following change:

diff --git a/libavcodec/v4l2_request_h264.c b/libavcodec/v4l2_request_h264.c
index 88da8f0a2d..394bae0550 100644
--- a/libavcodec/v4l2_request_h264.c
+++ b/libavcodec/v4l2_request_h264.c
@@ -66,7 +66,7 @@ static void fill_dpb_entry(struct v4l2_h264_dpb_entry *entry, const H264Picture
 {
     entry->reference_ts = ff_v4l2_request_get_capture_timestamp(pic->f);
     entry->pic_num = pic->pic_id;
-    entry->frame_num = pic->frame_num;
+    entry->frame_num = pic->long_ref ? pic->pic_id : pic->frame_num;
     entry->fields = pic->reference & V4L2_H264_FRAME_REF;
     entry->flags = V4L2_H264_DPB_ENTRY_FLAG_VALID;
     if (entry->fields)

Some useful links:

Detailed Hantro Results:     https://gitlab.freedesktop.org/-/snippets/5189
Detailed RKVDEC Results:     https://gitlab.freedesktop.org/-/snippets/5253
ITU-T H.264 (2016-02) AVCv2: https://www.itu.int/net/itu-t/sigdb/spevideo/VideoForm-s.aspx?val=102002641
Fluster:                     https://github.com/fluendo/fluster
GStreamer:                   https://gitlab.freedesktop.org/gstreamer/gstreamer/
FFMPEG Fork:                 https://github.com/jernejsk/FFmpeg/tree/v4l2-request-hwaccel-4.4
Rockchip MPP:                https://github.com/rockchip-linux/mpp

Alex Bee (1):
  media: rkvdec-h264: Don't hardcode SPS/PPS parameters

Jonas Karlman (5):
  media: rkvdec: h264: Fix reference frame_num wrap for second field
  media: rkvdec: Ensure decoded resolution fit coded resolution
  media: rkvdec: h264: Validate and use pic width and height in mbs
  media: rkvdec: h264: Fix bit depth wrap in pps packet
  media: hantro: h264: Make dpb entry management more robust

Nicolas Dufresne (17):
  media: h264: Increase reference lists size to 32
  media: doc: Document dual use of H.264 pic_num/frame_num
  media: h264: Avoid wrapping long_term_frame_idx
  media: h264: Store current picture fields
  media: h264: Store all fields into the unordered list
  media: v4l2: Trace calculated p/b0/b1 initial reflist
  media: h264: Sort p/b reflist using frame_num
  media: v4l2: Reorder field reflist
  media: v4l2-mem2mem: Fix typo in trace message
  media: v4l2-mem2mem: Trace on implicit un-hold
  media: rkvdec: Stop overclocking the decoder
  media: rkvdec: h264: Fix dpb_valid implementation
  media: rkvdec: Enable capture buffer holding for H264
  media: rkvdec-h264: Add field decoding support
  media: hantro: Enable HOLD_CAPTURE_BUF for H.264
  media: hantro: Stop using H.264 parameter pic_num
  media: hantro: Add H.264 field decoding support

Sebastian Fricke (1):
  media: videobuf2-v4l2: Warn on holding buffers without support

 .../media/v4l/ext-ctrls-codec-stateless.rst   |  10 +-
 .../media/common/videobuf2/videobuf2-v4l2.c   |   7 +-
 drivers/media/v4l2-core/v4l2-h264.c           | 238 +++++++++++++++---
 drivers/media/v4l2-core/v4l2-mem2mem.c        |   3 +-
 drivers/staging/media/hantro/hantro_h264.c    | 119 +++++++--
 drivers/staging/media/hantro/hantro_hw.h      |   7 +-
 drivers/staging/media/hantro/hantro_v4l2.c    |  25 ++
 drivers/staging/media/rkvdec/rkvdec-h264.c    | 104 ++++----
 drivers/staging/media/rkvdec/rkvdec.c         |  22 +-
 drivers/staging/media/rkvdec/rkvdec.h         |   1 +
 include/media/v4l2-h264.h                     |  20 +-
 11 files changed, 431 insertions(+), 125 deletions(-)

base-commit: 51d86122ff02ac2ceef5c0a1cf28f0b5ed580ddd
-- 
2.34.1

[PATCH v1 01/24] media: h264: Increase reference lists size to 32

[Nicolas Dufresne]

This is to accommodate support for field decoding, which splits the top
and the bottom reference into the reference list.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-h264.c        | 6 +++---
 drivers/staging/media/hantro/hantro_hw.h   | 6 +++---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 6 +++---
 include/media/v4l2-h264.h                  | 8 ++++----
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index 0618c6f52214..8d750ee69e74 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -210,7 +210,7 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
  * v4l2_h264_build_p_ref_list() - Build the P reference list
  *
  * @builder: reference list builder context
- * @reflist: 16 sized array used to store the P reference list. Each entry
+ * @reflist: 32 sized array used to store the P reference list. Each entry
  *	     is a v4l2_h264_reference structure
  *
  * This functions builds the P reference lists. This procedure is describe in
@@ -233,9 +233,9 @@ EXPORT_SYMBOL_GPL(v4l2_h264_build_p_ref_list);
  * v4l2_h264_build_b_ref_lists() - Build the B0/B1 reference lists
  *
  * @builder: reference list builder context
- * @b0_reflist: 16 sized array used to store the B0 reference list. Each entry
+ * @b0_reflist: 32 sized array used to store the B0 reference list. Each entry
  *		is a v4l2_h264_reference structure
- * @b1_reflist: 16 sized array used to store the B1 reference list. Each entry
+ * @b1_reflist: 32 sized array used to store the B1 reference list. Each entry
  *		is a v4l2_h264_reference structure
  *
  * This functions builds the B0/B1 reference lists. This procedure is described
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 2bc6b8f088f5..292aaaabaf24 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -69,9 +69,9 @@ struct hantro_h264_dec_ctrls {
  * @b1:		B1 reflist
  */
 struct hantro_h264_dec_reflists {
-	struct v4l2_h264_reference p[HANTRO_H264_DPB_SIZE];
-	struct v4l2_h264_reference b0[HANTRO_H264_DPB_SIZE];
-	struct v4l2_h264_reference b1[HANTRO_H264_DPB_SIZE];
+	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
+	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
+	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
 };
 
 /**
diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 3c7f3d87fab4..dff89732ddd0 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -100,9 +100,9 @@ struct rkvdec_h264_priv_tbl {
 #define RKVDEC_H264_DPB_SIZE 16
 
 struct rkvdec_h264_reflists {
-	struct v4l2_h264_reference p[RKVDEC_H264_DPB_SIZE];
-	struct v4l2_h264_reference b0[RKVDEC_H264_DPB_SIZE];
-	struct v4l2_h264_reference b1[RKVDEC_H264_DPB_SIZE];
+	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
+	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
+	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
 	u8 num_valid;
 };
 
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index ef9a894e3c32..e282fb16ac58 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -37,7 +37,7 @@ struct v4l2_h264_reflist_builder {
 		u16 longterm : 1;
 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
 	s32 cur_pic_order_count;
-	struct v4l2_h264_reference unordered_reflist[V4L2_H264_NUM_DPB_ENTRIES];
+	struct v4l2_h264_reference unordered_reflist[V4L2_H264_REF_LIST_LEN];
 	u8 num_valid;
 };
 
@@ -51,9 +51,9 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
  * v4l2_h264_build_b_ref_lists() - Build the B0/B1 reference lists
  *
  * @builder: reference list builder context
- * @b0_reflist: 16 sized array used to store the B0 reference list. Each entry
+ * @b0_reflist: 32 sized array used to store the B0 reference list. Each entry
  *		is a v4l2_h264_reference structure
- * @b1_reflist: 16 sized array used to store the B1 reference list. Each entry
+ * @b1_reflist: 32 sized array used to store the B1 reference list. Each entry
  *		is a v4l2_h264_reference structure
  *
  * This functions builds the B0/B1 reference lists. This procedure is described
@@ -70,7 +70,7 @@ v4l2_h264_build_b_ref_lists(const struct v4l2_h264_reflist_builder *builder,
  * v4l2_h264_build_p_ref_list() - Build the P reference list
  *
  * @builder: reference list builder context
- * @reflist: 16 sized array used to store the P reference list. Each entry
+ * @reflist: 32 sized array used to store the P reference list. Each entry
  *	     is a v4l2_h264_reference structure
  *
  * This functions builds the P reference lists. This procedure is describe in
-- 
2.34.1

Re: [PATCH v1 01/24] media: h264: Increase reference lists size to 32

[Sebastian Fricke]

Hey Nicolas,

As mentioned in patch 24 as well:
The patch series doesn't seem to apply on the latest media tree master
branch. Looking at your tree I can see that you have commit: ba2c670a
"media: nxp: Restrict VIDEO_IMX_MIPI_CSIS to ARCH_MXC or COMPILE_TEST
Laurent Pinchart authored 1 week ago "

But the current head of the media tree is: 71e6d0608e4d
"media: platform: Remove unnecessary print function dev_err()
Yang Li authored 13 days ago"

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>This is to accommodate support for field decoding, which splits the top
>and the bottom reference into the reference list.

s/and the bottom reference/and the bottom references/

I think it would be helpful to describe with a bit more detail why field
decoding requires the 32 entry array instead of the 16 entry array.

How about:
"""
In field decoding mode a slice is a sequence of macroblock pairs, where
each pair contains a top and a bottom macroblock. To accommodate for
this mode the reference list must be able contain references for both
top and bottom macroblocks. Double the size of the reference list array
accordingly.
"""

I got the info from the specification at 6.3.

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>---
> drivers/media/v4l2-core/v4l2-h264.c        | 6 +++---
> drivers/staging/media/hantro/hantro_hw.h   | 6 +++---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 6 +++---
> include/media/v4l2-h264.h                  | 8 ++++----
> 4 files changed, 13 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index 0618c6f52214..8d750ee69e74 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -210,7 +210,7 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
>  * v4l2_h264_build_p_ref_list() - Build the P reference list
>  *
>  * @builder: reference list builder context
>- * @reflist: 16 sized array used to store the P reference list. Each entry
>+ * @reflist: 32 sized array used to store the P reference list. Each entry
>  *	     is a v4l2_h264_reference structure
>  *
>  * This functions builds the P reference lists. This procedure is describe in

Oh unrelated to the patch but there is a typo: s/describe/described/

Greetings,
Sebastian

>@@ -233,9 +233,9 @@ EXPORT_SYMBOL_GPL(v4l2_h264_build_p_ref_list);
>  * v4l2_h264_build_b_ref_lists() - Build the B0/B1 reference lists
>  *
>  * @builder: reference list builder context
>- * @b0_reflist: 16 sized array used to store the B0 reference list. Each entry
>+ * @b0_reflist: 32 sized array used to store the B0 reference list. Each entry
>  *		is a v4l2_h264_reference structure
>- * @b1_reflist: 16 sized array used to store the B1 reference list. Each entry
>+ * @b1_reflist: 32 sized array used to store the B1 reference list. Each entry
>  *		is a v4l2_h264_reference structure
>  *
>  * This functions builds the B0/B1 reference lists. This procedure is described
>diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
>index 2bc6b8f088f5..292aaaabaf24 100644
>--- a/drivers/staging/media/hantro/hantro_hw.h
>+++ b/drivers/staging/media/hantro/hantro_hw.h
>@@ -69,9 +69,9 @@ struct hantro_h264_dec_ctrls {
>  * @b1:		B1 reflist
>  */
> struct hantro_h264_dec_reflists {
>-	struct v4l2_h264_reference p[HANTRO_H264_DPB_SIZE];
>-	struct v4l2_h264_reference b0[HANTRO_H264_DPB_SIZE];
>-	struct v4l2_h264_reference b1[HANTRO_H264_DPB_SIZE];
>+	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
>+	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
>+	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
> };
>
> /**
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index 3c7f3d87fab4..dff89732ddd0 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -100,9 +100,9 @@ struct rkvdec_h264_priv_tbl {
> #define RKVDEC_H264_DPB_SIZE 16
>
> struct rkvdec_h264_reflists {
>-	struct v4l2_h264_reference p[RKVDEC_H264_DPB_SIZE];
>-	struct v4l2_h264_reference b0[RKVDEC_H264_DPB_SIZE];
>-	struct v4l2_h264_reference b1[RKVDEC_H264_DPB_SIZE];
>+	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
>+	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
>+	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
> 	u8 num_valid;
> };
>
>diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
>index ef9a894e3c32..e282fb16ac58 100644
>--- a/include/media/v4l2-h264.h
>+++ b/include/media/v4l2-h264.h
>@@ -37,7 +37,7 @@ struct v4l2_h264_reflist_builder {
> 		u16 longterm : 1;
> 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
> 	s32 cur_pic_order_count;
>-	struct v4l2_h264_reference unordered_reflist[V4L2_H264_NUM_DPB_ENTRIES];
>+	struct v4l2_h264_reference unordered_reflist[V4L2_H264_REF_LIST_LEN];
> 	u8 num_valid;
> };
>
>@@ -51,9 +51,9 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
>  * v4l2_h264_build_b_ref_lists() - Build the B0/B1 reference lists
>  *
>  * @builder: reference list builder context
>- * @b0_reflist: 16 sized array used to store the B0 reference list. Each entry
>+ * @b0_reflist: 32 sized array used to store the B0 reference list. Each entry
>  *		is a v4l2_h264_reference structure
>- * @b1_reflist: 16 sized array used to store the B1 reference list. Each entry
>+ * @b1_reflist: 32 sized array used to store the B1 reference list. Each entry
>  *		is a v4l2_h264_reference structure
>  *
>  * This functions builds the B0/B1 reference lists. This procedure is described
>@@ -70,7 +70,7 @@ v4l2_h264_build_b_ref_lists(const struct v4l2_h264_reflist_builder *builder,
>  * v4l2_h264_build_p_ref_list() - Build the P reference list
>  *
>  * @builder: reference list builder context
>- * @reflist: 16 sized array used to store the P reference list. Each entry
>+ * @reflist: 32 sized array used to store the P reference list. Each entry
>  *	     is a v4l2_h264_reference structure
>  *
>  * This functions builds the P reference lists. This procedure is describe in
>-- 
>2.34.1
>

[PATCH v1 02/24] media: doc: Document dual use of H.264 pic_num/frame_num

[Nicolas Dufresne]

These two fields needs documentation as they have dual meaning. It is also
confusing since pic_num is a derived value from frame_num, so this should
help application developpers. If we ever need to make a V2 of this API, I
would suggest to remove pic_num entirely.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../media/v4l/ext-ctrls-codec-stateless.rst            | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
index 6541e4c32b26..f634f20bcfbe 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
@@ -649,10 +649,16 @@ Stateless Codec Control ID
         :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64.
     * - __u32
       - ``pic_num``
-      -
+      - For short term reference, this should match the derived value PicNum
+	(8-28) and for long term references it should match the derived value
+	LongTermPicNum (8-29). Note that pic_num is the same as FrameNumWrap
+	for frame decoding.
     * - __u16
       - ``frame_num``
-      -
+      - For short term references, this should match the frame_num value from
+	the slice header syntax (the driver will wrap the value if neeeded). For
+	long term references, this should be set to the value of
+	long_term_frame_idx describes in the dec_ref_pic_marking() syntax.
     * - __u8
       - ``fields``
       - Specifies how the DPB entry is referenced. See :ref:`Reference Fields <h264_ref_fields>`
-- 
2.34.1

Re: [PATCH v1 02/24] media: doc: Document dual use of H.264 pic_num/frame_num

[Sebastian Fricke]

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>These two fields needs documentation as they have dual meaning. It is also

s/needs/need/

>confusing since pic_num is a derived value from frame_num, so this should
>help application developpers. If we ever need to make a V2 of this API, I

s/developpers/developers/
(seems to be a common typo among native French speakers ;))

>would suggest to remove pic_num entirely.

I think that suggestion should be placed as a FIXME/TODO comment into the
source code as it is way easier to find for future developers creating a V2.

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> .../media/v4l/ext-ctrls-codec-stateless.rst            | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
>index 6541e4c32b26..f634f20bcfbe 100644
>--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
>+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
>@@ -649,10 +649,16 @@ Stateless Codec Control ID
>         :c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64.
>     * - __u32
>       - ``pic_num``
>-      -
>+      - For short term reference, this should match the derived value PicNum
>+	(8-28) and for long term references it should match the derived value

Minor suggestion, for short term you use singular (reference) and for
long term you use plural (references), I would stick to one of both.
(Below you only use plural so maybe stick to plural here as well)

>+	LongTermPicNum (8-29). Note that pic_num is the same as FrameNumWrap
>+	for frame decoding.
>     * - __u16
>       - ``frame_num``
>-      -
>+      - For short term references, this should match the frame_num value from
>+	the slice header syntax (the driver will wrap the value if neeeded). For
>+	long term references, this should be set to the value of
>+	long_term_frame_idx describes in the dec_ref_pic_marking() syntax.

s/describes/described/

Greetings,
Sebastian

>     * - __u8
>       - ``fields``
>       - Specifies how the DPB entry is referenced. See :ref:`Reference Fields <h264_ref_fields>`
>-- 
>2.34.1
>

[PATCH v1 03/24] media: h264: Avoid wrapping long_term_frame_idx

[Nicolas Dufresne]

For long term reference, frame_num is set to long_term_frame_idx which
does not require wrapping. This if fixed by observation, no directly
related issue have been found yet.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-h264.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index 8d750ee69e74..aebed1cbe05a 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -57,8 +57,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		 * '8.2.4.1 Decoding process for picture numbers' of the spec.
 		 * TODO: This logic will have to be adjusted when we start
 		 * supporting interlaced content.
+		 * For long term reference, frame_num is set to
+		 * long_term_frame_idx which requires no wrapping.
 		 */
-		if (dpb[i].frame_num > cur_frame_num)
+		if (!b->refs[i].longterm && dpb[i].frame_num > cur_frame_num)
 			b->refs[i].frame_num = (int)dpb[i].frame_num -
 					       max_frame_num;
 		else
-- 
2.34.1

Re: [PATCH v1 03/24] media: h264: Avoid wrapping long_term_frame_idx

[Sebastian Fricke]

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>For long term reference, frame_num is set to long_term_frame_idx which

s/reference/references/

>does not require wrapping. This if fixed by observation, no directly

s/This if/This is/

>related issue have been found yet.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/media/v4l2-core/v4l2-h264.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index 8d750ee69e74..aebed1cbe05a 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -57,8 +57,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> 		 * '8.2.4.1 Decoding process for picture numbers' of the spec.
> 		 * TODO: This logic will have to be adjusted when we start
> 		 * supporting interlaced content.

As you change the logic can't you remove the TODO comment now?

>+		 * For long term reference, frame_num is set to

s/reference/references/

Greetings,
Sebastian

>+		 * long_term_frame_idx which requires no wrapping.
> 		 */
>-		if (dpb[i].frame_num > cur_frame_num)
>+		if (!b->refs[i].longterm && dpb[i].frame_num > cur_frame_num)
> 			b->refs[i].frame_num = (int)dpb[i].frame_num -
> 					       max_frame_num;
> 		else
>-- 
>2.34.1
>

Re: [PATCH v1 03/24] media: h264: Avoid wrapping long_term_frame_idx

[Nicolas Dufresne]

Le mardi 29 mars 2022 à 10:35 +0200, Sebastian Fricke a écrit :
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > For long term reference, frame_num is set to long_term_frame_idx which
> 
> s/reference/references/
> 
> > does not require wrapping. This if fixed by observation, no directly
> 
> s/This if/This is/
> 
> > related issue have been found yet.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> 
> > ---
> > drivers/media/v4l2-core/v4l2-h264.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
> > index 8d750ee69e74..aebed1cbe05a 100644
> > --- a/drivers/media/v4l2-core/v4l2-h264.c
> > +++ b/drivers/media/v4l2-core/v4l2-h264.c
> > @@ -57,8 +57,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> > 		 * '8.2.4.1 Decoding process for picture numbers' of the spec.
> > 		 * TODO: This logic will have to be adjusted when we start
> > 		 * supporting interlaced content.
> 
> As you change the logic can't you remove the TODO comment now?

Not yet, as I'm not fixing it here. Its removed in:

[PATCH v1 05/24] media: h264: Store all fields into the unordered list

> 
> > +		 * For long term reference, frame_num is set to
> 
> s/reference/references/
> 
> Greetings,
> Sebastian
> 
> > +		 * long_term_frame_idx which requires no wrapping.
> > 		 */
> > -		if (dpb[i].frame_num > cur_frame_num)
> > +		if (!b->refs[i].longterm && dpb[i].frame_num > cur_frame_num)
> > 			b->refs[i].frame_num = (int)dpb[i].frame_num -
> > 					       max_frame_num;
> > 		else
> > -- 
> > 2.34.1
> > 

[PATCH v1 04/24] media: h264: Store current picture fields

[Nicolas Dufresne]

This information, also called picture structure will be needed to construct
reference list when decoding field.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-h264.c | 10 +++++++---
 include/media/v4l2-h264.h           |  4 ++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index aebed1cbe05a..4c6bfb057bda 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -34,13 +34,17 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 	cur_frame_num = dec_params->frame_num;
 
 	memset(b, 0, sizeof(*b));
-	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
+	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)) {
 		b->cur_pic_order_count = min(dec_params->bottom_field_order_cnt,
 					     dec_params->top_field_order_cnt);
-	else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
+		b->cur_pic_fields = V4L2_H264_FRAME_REF;
+	} else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD) {
 		b->cur_pic_order_count = dec_params->bottom_field_order_cnt;
-	else
+		b->cur_pic_fields = V4L2_H264_BOTTOM_FIELD_REF;
+	} else {
 		b->cur_pic_order_count = dec_params->top_field_order_cnt;
+		b->cur_pic_fields = V4L2_H264_TOP_FIELD_REF;
+	}
 
 	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
 		u32 pic_order_count;
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index e282fb16ac58..e165a54c68fa 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -21,6 +21,7 @@
  * @refs.longterm: set to true for a long term reference
  * @refs: array of references
  * @cur_pic_order_count: picture order count of the frame being decoded
+ * @cur_pic_fields: fields present in the frame being decoded
  * @unordered_reflist: unordered list of references. Will be used to generate
  *		       ordered P/B0/B1 lists
  * @num_valid: number of valid references in the refs array
@@ -36,7 +37,10 @@ struct v4l2_h264_reflist_builder {
 		u32 pic_num;
 		u16 longterm : 1;
 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
+
 	s32 cur_pic_order_count;
+	u8 cur_pic_fields;
+
 	struct v4l2_h264_reference unordered_reflist[V4L2_H264_REF_LIST_LEN];
 	u8 num_valid;
 };
-- 
2.34.1

Re: [PATCH v1 04/24] media: h264: Store current picture fields

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>This information, also called picture structure will be needed to construct

s/picture structure/picture structure,/
(minor.. but still ;))

>reference list when decoding field.

s/will be needed to construct reference list when decoding field./
   is required in field decoding mode to construct reference lists./

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/media/v4l2-core/v4l2-h264.c | 10 +++++++---
> include/media/v4l2-h264.h           |  4 ++++
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index aebed1cbe05a..4c6bfb057bda 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -34,13 +34,17 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> 	cur_frame_num = dec_params->frame_num;
>
> 	memset(b, 0, sizeof(*b));
>-	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC))
>+	if (!(dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)) {
> 		b->cur_pic_order_count = min(dec_params->bottom_field_order_cnt,
> 					     dec_params->top_field_order_cnt);
>-	else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD)
>+		b->cur_pic_fields = V4L2_H264_FRAME_REF;
>+	} else if (dec_params->flags & V4L2_H264_DECODE_PARAM_FLAG_BOTTOM_FIELD) {
> 		b->cur_pic_order_count = dec_params->bottom_field_order_cnt;
>-	else
>+		b->cur_pic_fields = V4L2_H264_BOTTOM_FIELD_REF;
>+	} else {
> 		b->cur_pic_order_count = dec_params->top_field_order_cnt;
>+		b->cur_pic_fields = V4L2_H264_TOP_FIELD_REF;
>+	}
>
> 	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
> 		u32 pic_order_count;
>diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
>index e282fb16ac58..e165a54c68fa 100644
>--- a/include/media/v4l2-h264.h
>+++ b/include/media/v4l2-h264.h
>@@ -21,6 +21,7 @@
>  * @refs.longterm: set to true for a long term reference
>  * @refs: array of references
>  * @cur_pic_order_count: picture order count of the frame being decoded
>+ * @cur_pic_fields: fields present in the frame being decoded

As there are only three choices:
```
#define V4L2_H264_TOP_FIELD_REF				0x1
#define V4L2_H264_BOTTOM_FIELD_REF			0x2
#define V4L2_H264_FRAME_REF				0x3
```

Maybe it would help to say:
  * @cur_pic_fields: fields present in the frame being decoded (top/bottom/both)

Greetings,
Sebastian

>  * @unordered_reflist: unordered list of references. Will be used to generate
>  *		       ordered P/B0/B1 lists
>  * @num_valid: number of valid references in the refs array
>@@ -36,7 +37,10 @@ struct v4l2_h264_reflist_builder {
> 		u32 pic_num;
> 		u16 longterm : 1;
> 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
>+
> 	s32 cur_pic_order_count;
>+	u8 cur_pic_fields;
>+
> 	struct v4l2_h264_reference unordered_reflist[V4L2_H264_REF_LIST_LEN];
> 	u8 num_valid;
> };
>-- 
>2.34.1
>

[PATCH v1 05/24] media: h264: Store all fields into the unordered list

[Nicolas Dufresne]

When the current picture is a field, store each field into the
unordered_list. Doing this required preserving the top and bottom field
poc.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-h264.c | 65 +++++++++++++++++++++--------
 include/media/v4l2-h264.h           |  6 ++-
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index 4c6bfb057bda..d5698c981973 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -47,8 +47,6 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 	}
 
 	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
-		u32 pic_order_count;
-
 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
 			continue;
 
@@ -59,8 +57,6 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		/*
 		 * Handle frame_num wraparound as described in section
 		 * '8.2.4.1 Decoding process for picture numbers' of the spec.
-		 * TODO: This logic will have to be adjusted when we start
-		 * supporting interlaced content.
 		 * For long term reference, frame_num is set to
 		 * long_term_frame_idx which requires no wrapping.
 		 */
@@ -70,17 +66,33 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		else
 			b->refs[i].frame_num = dpb[i].frame_num;
 
-		if (dpb[i].fields == V4L2_H264_FRAME_REF)
-			pic_order_count = min(dpb[i].top_field_order_cnt,
-					      dpb[i].bottom_field_order_cnt);
-		else if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF)
-			pic_order_count = dpb[i].bottom_field_order_cnt;
-		else
-			pic_order_count = dpb[i].top_field_order_cnt;
+		b->refs[i].top_field_order_cnt = dpb[i].top_field_order_cnt;
+		b->refs[i].bottom_field_order_cnt = dpb[i].bottom_field_order_cnt;
+
+		if (b->cur_pic_fields == V4L2_H264_FRAME_REF) {
+			u8 fields = V4L2_H264_FRAME_REF;
+
+			b->unordered_reflist[b->num_valid].index = i;
+			b->unordered_reflist[b->num_valid].fields = fields;
+			b->num_valid++;
+			continue;
+		}
+
+		if (dpb[i].fields & V4L2_H264_TOP_FIELD_REF) {
+			u8 fields = V4L2_H264_TOP_FIELD_REF;
+
+			b->unordered_reflist[b->num_valid].index = i;
+			b->unordered_reflist[b->num_valid].fields = fields;
+			b->num_valid++;
+		}
 
-		b->refs[i].pic_order_count = pic_order_count;
-		b->unordered_reflist[b->num_valid].index = i;
-		b->num_valid++;
+		if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF) {
+			u8 fields = V4L2_H264_BOTTOM_FIELD_REF;
+
+			b->unordered_reflist[b->num_valid].index = i;
+			b->unordered_reflist[b->num_valid].fields = fields;
+			b->num_valid++;
+		}
 	}
 
 	for (i = b->num_valid; i < ARRAY_SIZE(b->unordered_reflist); i++)
@@ -88,6 +100,23 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 }
 EXPORT_SYMBOL_GPL(v4l2_h264_init_reflist_builder);
 
+static s32 v4l2_h264_get_poc(const struct v4l2_h264_reflist_builder *b,
+			     const struct v4l2_h264_reference *ref)
+{
+	switch (ref->fields) {
+	case V4L2_H264_FRAME_REF:
+		return min(b->refs[ref->index].top_field_order_cnt,
+				b->refs[ref->index].bottom_field_order_cnt);
+	case V4L2_H264_TOP_FIELD_REF:
+		return b->refs[ref->index].top_field_order_cnt;
+	case V4L2_H264_BOTTOM_FIELD_REF:
+		return b->refs[ref->index].bottom_field_order_cnt;
+	}
+
+	/* not reached */
+	return 0;
+}
+
 static int v4l2_h264_p_ref_list_cmp(const void *ptra, const void *ptrb,
 				    const void *data)
 {
@@ -150,8 +179,8 @@ static int v4l2_h264_b0_ref_list_cmp(const void *ptra, const void *ptrb,
 		       builder->refs[idxb].pic_num ?
 		       -1 : 1;
 
-	poca = builder->refs[idxa].pic_order_count;
-	pocb = builder->refs[idxb].pic_order_count;
+	poca = v4l2_h264_get_poc(builder, ptra);
+	pocb = v4l2_h264_get_poc(builder, ptrb);
 
 	/*
 	 * Short term pics with POC < cur POC first in POC descending order
@@ -195,8 +224,8 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
 		       builder->refs[idxb].pic_num ?
 		       -1 : 1;
 
-	poca = builder->refs[idxa].pic_order_count;
-	pocb = builder->refs[idxb].pic_order_count;
+	poca = v4l2_h264_get_poc(builder, ptra);
+	pocb = v4l2_h264_get_poc(builder, ptrb);
 
 	/*
 	 * Short term pics with POC > cur POC first in POC ascending order
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index e165a54c68fa..4cef717b3f18 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -15,7 +15,8 @@
 /**
  * struct v4l2_h264_reflist_builder - Reference list builder object
  *
- * @refs.pic_order_count: reference picture order count
+ * @refs.top_field_order_cnt: top field order count
+ * @refs.bottom_field_order_cnt: bottom field order count
  * @refs.frame_num: reference frame number
  * @refs.pic_num: reference picture number
  * @refs.longterm: set to true for a long term reference
@@ -32,7 +33,8 @@
  */
 struct v4l2_h264_reflist_builder {
 	struct {
-		s32 pic_order_count;
+		s32 top_field_order_cnt;
+		s32 bottom_field_order_cnt;
 		int frame_num;
 		u32 pic_num;
 		u16 longterm : 1;
-- 
2.34.1

Re: [PATCH v1 05/24] media: h264: Store all fields into the unordered list

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>When the current picture is a field, store each field into the
>unordered_list. Doing this required preserving the top and bottom field
>poc.

I would slightly reword this to highlight that we talk about the
reference list and to improve the grammatic of the second sentence:

"""
If the current image is a field, each field is stored in the unordered
reference list, preserving the original picture order count of the
top and bottom fields.
"""

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/media/v4l2-core/v4l2-h264.c | 65 +++++++++++++++++++++--------
> include/media/v4l2-h264.h           |  6 ++-
> 2 files changed, 51 insertions(+), 20 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index 4c6bfb057bda..d5698c981973 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -47,8 +47,6 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> 	}
>
> 	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
>-		u32 pic_order_count;
>-
> 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> 			continue;
>
>@@ -59,8 +57,6 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> 		/*
> 		 * Handle frame_num wraparound as described in section
> 		 * '8.2.4.1 Decoding process for picture numbers' of the spec.
>-		 * TODO: This logic will have to be adjusted when we start
>-		 * supporting interlaced content.

Ah, I see here you remove the TODO comment disregard my comment from
patch 3 please.

Greetings,
Sebastian

> 		 * For long term reference, frame_num is set to
> 		 * long_term_frame_idx which requires no wrapping.
> 		 */
>@@ -70,17 +66,33 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> 		else
> 			b->refs[i].frame_num = dpb[i].frame_num;
>
>-		if (dpb[i].fields == V4L2_H264_FRAME_REF)
>-			pic_order_count = min(dpb[i].top_field_order_cnt,
>-					      dpb[i].bottom_field_order_cnt);
>-		else if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF)
>-			pic_order_count = dpb[i].bottom_field_order_cnt;
>-		else
>-			pic_order_count = dpb[i].top_field_order_cnt;
>+		b->refs[i].top_field_order_cnt = dpb[i].top_field_order_cnt;
>+		b->refs[i].bottom_field_order_cnt = dpb[i].bottom_field_order_cnt;
>+
>+		if (b->cur_pic_fields == V4L2_H264_FRAME_REF) {
>+			u8 fields = V4L2_H264_FRAME_REF;
>+
>+			b->unordered_reflist[b->num_valid].index = i;
>+			b->unordered_reflist[b->num_valid].fields = fields;

>+			b->num_valid++;
>+			continue;
>+		}
>+
>+		if (dpb[i].fields & V4L2_H264_TOP_FIELD_REF) {
>+			u8 fields = V4L2_H264_TOP_FIELD_REF;
>+
>+			b->unordered_reflist[b->num_valid].index = i;
>+			b->unordered_reflist[b->num_valid].fields = fields;
>+			b->num_valid++;
>+		}
>
>-		b->refs[i].pic_order_count = pic_order_count;
>-		b->unordered_reflist[b->num_valid].index = i;
>-		b->num_valid++;
>+		if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF) {
>+			u8 fields = V4L2_H264_BOTTOM_FIELD_REF;
>+
>+			b->unordered_reflist[b->num_valid].index = i;
>+			b->unordered_reflist[b->num_valid].fields = fields;
>+			b->num_valid++;
>+		}
> 	}
>
> 	for (i = b->num_valid; i < ARRAY_SIZE(b->unordered_reflist); i++)
>@@ -88,6 +100,23 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> }
> EXPORT_SYMBOL_GPL(v4l2_h264_init_reflist_builder);
>
>+static s32 v4l2_h264_get_poc(const struct v4l2_h264_reflist_builder *b,
>+			     const struct v4l2_h264_reference *ref)
>+{
>+	switch (ref->fields) {
>+	case V4L2_H264_FRAME_REF:
>+		return min(b->refs[ref->index].top_field_order_cnt,
>+				b->refs[ref->index].bottom_field_order_cnt);
>+	case V4L2_H264_TOP_FIELD_REF:
>+		return b->refs[ref->index].top_field_order_cnt;
>+	case V4L2_H264_BOTTOM_FIELD_REF:
>+		return b->refs[ref->index].bottom_field_order_cnt;
>+	}
>+
>+	/* not reached */
>+	return 0;
>+}
>+
> static int v4l2_h264_p_ref_list_cmp(const void *ptra, const void *ptrb,
> 				    const void *data)
> {
>@@ -150,8 +179,8 @@ static int v4l2_h264_b0_ref_list_cmp(const void *ptra, const void *ptrb,
> 		       builder->refs[idxb].pic_num ?
> 		       -1 : 1;
>
>-	poca = builder->refs[idxa].pic_order_count;
>-	pocb = builder->refs[idxb].pic_order_count;
>+	poca = v4l2_h264_get_poc(builder, ptra);
>+	pocb = v4l2_h264_get_poc(builder, ptrb);
>
> 	/*
> 	 * Short term pics with POC < cur POC first in POC descending order
>@@ -195,8 +224,8 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
> 		       builder->refs[idxb].pic_num ?
> 		       -1 : 1;
>
>-	poca = builder->refs[idxa].pic_order_count;
>-	pocb = builder->refs[idxb].pic_order_count;
>+	poca = v4l2_h264_get_poc(builder, ptra);
>+	pocb = v4l2_h264_get_poc(builder, ptrb);
>
> 	/*
> 	 * Short term pics with POC > cur POC first in POC ascending order
>diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
>index e165a54c68fa..4cef717b3f18 100644
>--- a/include/media/v4l2-h264.h
>+++ b/include/media/v4l2-h264.h
>@@ -15,7 +15,8 @@
> /**
>  * struct v4l2_h264_reflist_builder - Reference list builder object
>  *
>- * @refs.pic_order_count: reference picture order count
>+ * @refs.top_field_order_cnt: top field order count
>+ * @refs.bottom_field_order_cnt: bottom field order count
>  * @refs.frame_num: reference frame number
>  * @refs.pic_num: reference picture number
>  * @refs.longterm: set to true for a long term reference
>@@ -32,7 +33,8 @@
>  */
> struct v4l2_h264_reflist_builder {
> 	struct {
>-		s32 pic_order_count;
>+		s32 top_field_order_cnt;
>+		s32 bottom_field_order_cnt;
> 		int frame_num;
> 		u32 pic_num;
> 		u16 longterm : 1;
>-- 
>2.34.1
>

[PATCH v1 06/24] media: v4l2: Trace calculated p/b0/b1 initial reflist

[Nicolas Dufresne]

This is crucial in verifying that the ordering is right, specially with
more complex sorting needed for field decoding.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-h264.c | 86 +++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index d5698c981973..c9e18fd51d78 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -241,6 +241,87 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
 	return poca < pocb ? -1 : 1;
 }
 
+static char ref_type_to_char (u8 ref_type)
+{
+	switch (ref_type) {
+	case V4L2_H264_FRAME_REF:
+		return 'f';
+	case V4L2_H264_TOP_FIELD_REF:
+		return 't';
+	case V4L2_H264_BOTTOM_FIELD_REF:
+		return 'b';
+	}
+
+	return '?';
+}
+
+static const char *format_ref_list_p(const struct v4l2_h264_reflist_builder *builder,
+				     struct v4l2_h264_reference *reflist,
+				     char *out_str, const int len)
+{
+	int n = 0, i;
+
+	n += snprintf(out_str + n, len - n, "|");
+
+	for (i = 0; i < builder->num_valid; i++) {
+		/* this is pic_num for frame and frame_num (wrapped) for field,
+		 * but for frame pic_num is equal to frame_num (wrapped).
+		 */
+		int frame_num = builder->refs[reflist[i].index].frame_num;
+		bool longterm = builder->refs[reflist[i].index].longterm;
+
+		n += scnprintf(out_str + n, len - n, "%i%c%c|",
+			       frame_num, longterm ? 'l' : 's',
+			       ref_type_to_char (reflist[i].fields));
+	}
+
+	return out_str;
+}
+
+static void print_ref_list_p(const struct v4l2_h264_reflist_builder *builder,
+			     struct v4l2_h264_reference *reflist)
+{
+	char buf[1024];
+
+	pr_debug("ref_pic_list_p (cur_poc %u%c) %s\n",
+		 builder->cur_pic_order_count,
+		 ref_type_to_char(builder->cur_pic_fields),
+		 format_ref_list_p(builder, reflist, buf, sizeof(buf)));
+}
+
+static const char *format_ref_list_b(const struct v4l2_h264_reflist_builder *builder,
+				     struct v4l2_h264_reference *reflist,
+				     char *out_str, const int len)
+{
+	int n = 0, i;
+
+	n += snprintf(out_str + n, len - n, "|");
+
+	for (i = 0; i < builder->num_valid; i++) {
+		int frame_num = builder->refs[reflist[i].index].frame_num;
+		u32 poc = v4l2_h264_get_poc(builder, reflist + i);
+		bool longterm = builder->refs[reflist[i].index].longterm;
+
+		n += scnprintf(out_str + n, len - n, "%i%c%c|",
+			       longterm ? frame_num : poc,
+			       longterm ? 'l' : 's',
+			       ref_type_to_char(reflist[i].fields));
+	}
+
+	return out_str;
+}
+
+static void print_ref_list_b(const struct v4l2_h264_reflist_builder *builder,
+			     struct v4l2_h264_reference *reflist, u8 list_num)
+{
+	char buf[1024];
+
+	pr_debug("ref_pic_list_b%u (cur_poc %u%c) %s",
+		 list_num, builder->cur_pic_order_count,
+		 ref_type_to_char (builder->cur_pic_fields),
+		 format_ref_list_b(builder, reflist, buf, sizeof(buf)));
+}
+
 /**
  * v4l2_h264_build_p_ref_list() - Build the P reference list
  *
@@ -261,6 +342,8 @@ v4l2_h264_build_p_ref_list(const struct v4l2_h264_reflist_builder *builder,
 	       sizeof(builder->unordered_reflist[0]) * builder->num_valid);
 	sort_r(reflist, builder->num_valid, sizeof(*reflist),
 	       v4l2_h264_p_ref_list_cmp, NULL, builder);
+
+	print_ref_list_p(builder, reflist);
 }
 EXPORT_SYMBOL_GPL(v4l2_h264_build_p_ref_list);
 
@@ -296,6 +379,9 @@ v4l2_h264_build_b_ref_lists(const struct v4l2_h264_reflist_builder *builder,
 	if (builder->num_valid > 1 &&
 	    !memcmp(b1_reflist, b0_reflist, builder->num_valid))
 		swap(b1_reflist[0], b1_reflist[1]);
+
+	print_ref_list_b(builder, b0_reflist, 0);
+	print_ref_list_b(builder, b1_reflist, 1);
 }
 EXPORT_SYMBOL_GPL(v4l2_h264_build_b_ref_lists);
 
-- 
2.34.1

Re: [PATCH v1 06/24] media: v4l2: Trace calculated p/b0/b1 initial reflist

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>This is crucial in verifying that the ordering is right, specially with
>more complex sorting needed for field decoding.

How about:

"""
Add debug print statements to print the content of P & B reference
lists, to verify that the ordering of the generated reference lists is
correct. This is escpecially important for the field decoding mode,
where sorting is more complex.
"""

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/media/v4l2-core/v4l2-h264.c | 86 +++++++++++++++++++++++++++++
> 1 file changed, 86 insertions(+)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index d5698c981973..c9e18fd51d78 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -241,6 +241,87 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
> 	return poca < pocb ? -1 : 1;
> }
>
>+static char ref_type_to_char (u8 ref_type)
>+{
>+	switch (ref_type) {
>+	case V4L2_H264_FRAME_REF:
>+		return 'f';
>+	case V4L2_H264_TOP_FIELD_REF:
>+		return 't';
>+	case V4L2_H264_BOTTOM_FIELD_REF:
>+		return 'b';
>+	}
>+
>+	return '?';
>+}
>+
>+static const char *format_ref_list_p(const struct v4l2_h264_reflist_builder *builder,
>+				     struct v4l2_h264_reference *reflist,
>+				     char *out_str, const int len)
>+{
>+	int n = 0, i;
>+
>+	n += snprintf(out_str + n, len - n, "|");
>+
>+	for (i = 0; i < builder->num_valid; i++) {
>+		/* this is pic_num for frame and frame_num (wrapped) for field,
>+		 * but for frame pic_num is equal to frame_num (wrapped).
>+		 */
>+		int frame_num = builder->refs[reflist[i].index].frame_num;
>+		bool longterm = builder->refs[reflist[i].index].longterm;
>+
>+		n += scnprintf(out_str + n, len - n, "%i%c%c|",
>+			       frame_num, longterm ? 'l' : 's',
>+			       ref_type_to_char (reflist[i].fields));
>+	}
>+
>+	return out_str;
>+}
>+
>+static void print_ref_list_p(const struct v4l2_h264_reflist_builder *builder,
>+			     struct v4l2_h264_reference *reflist)
>+{
>+	char buf[1024];
>+
>+	pr_debug("ref_pic_list_p (cur_poc %u%c) %s\n",
>+		 builder->cur_pic_order_count,
>+		 ref_type_to_char(builder->cur_pic_fields),
>+		 format_ref_list_p(builder, reflist, buf, sizeof(buf)));
>+}
>+
>+static const char *format_ref_list_b(const struct v4l2_h264_reflist_builder *builder,
>+				     struct v4l2_h264_reference *reflist,
>+				     char *out_str, const int len)
>+{
>+	int n = 0, i;
>+
>+	n += snprintf(out_str + n, len - n, "|");
>+
>+	for (i = 0; i < builder->num_valid; i++) {
>+		int frame_num = builder->refs[reflist[i].index].frame_num;
>+		u32 poc = v4l2_h264_get_poc(builder, reflist + i);
>+		bool longterm = builder->refs[reflist[i].index].longterm;
>+
>+		n += scnprintf(out_str + n, len - n, "%i%c%c|",
>+			       longterm ? frame_num : poc,
>+			       longterm ? 'l' : 's',
>+			       ref_type_to_char(reflist[i].fields));
>+	}
>+
>+	return out_str;
>+}
>+
>+static void print_ref_list_b(const struct v4l2_h264_reflist_builder *builder,
>+			     struct v4l2_h264_reference *reflist, u8 list_num)
>+{
>+	char buf[1024];
>+
>+	pr_debug("ref_pic_list_b%u (cur_poc %u%c) %s",
>+		 list_num, builder->cur_pic_order_count,
>+		 ref_type_to_char (builder->cur_pic_fields),
>+		 format_ref_list_b(builder, reflist, buf, sizeof(buf)));
>+}
>+
> /**
>  * v4l2_h264_build_p_ref_list() - Build the P reference list
>  *
>@@ -261,6 +342,8 @@ v4l2_h264_build_p_ref_list(const struct v4l2_h264_reflist_builder *builder,
> 	       sizeof(builder->unordered_reflist[0]) * builder->num_valid);
> 	sort_r(reflist, builder->num_valid, sizeof(*reflist),
> 	       v4l2_h264_p_ref_list_cmp, NULL, builder);
>+
>+	print_ref_list_p(builder, reflist);
> }
> EXPORT_SYMBOL_GPL(v4l2_h264_build_p_ref_list);
>
>@@ -296,6 +379,9 @@ v4l2_h264_build_b_ref_lists(const struct v4l2_h264_reflist_builder *builder,
> 	if (builder->num_valid > 1 &&
> 	    !memcmp(b1_reflist, b0_reflist, builder->num_valid))
> 		swap(b1_reflist[0], b1_reflist[1]);
>+
>+	print_ref_list_b(builder, b0_reflist, 0);
>+	print_ref_list_b(builder, b1_reflist, 1);
> }
> EXPORT_SYMBOL_GPL(v4l2_h264_build_b_ref_lists);
>
>-- 
>2.34.1
>

[PATCH v1 07/24] media: h264: Sort p/b reflist using frame_num

[Nicolas Dufresne]

In the reference list builder, frame_num referers to FrameNumWrap
in the spec, which is the same as the pic_num for frame decoding.
The same applied for long_tern_pic_num and long_ter_frame_idx.

Sort all type of references by frame_num so the sort can be reused
for field reflist were the sorting is done using frame_num instead.
In short, pic_num is never actually used for building reference
lists.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-h264.c | 22 ++++++++++++----------
 include/media/v4l2-h264.h           |  2 --
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index c9e18fd51d78..c3fad382882d 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -50,7 +50,6 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
 			continue;
 
-		b->refs[i].pic_num = dpb[i].pic_num;
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
 			b->refs[i].longterm = true;
 
@@ -139,15 +138,18 @@ static int v4l2_h264_p_ref_list_cmp(const void *ptra, const void *ptrb,
 	}
 
 	/*
-	 * Short term pics in descending pic num order, long term ones in
-	 * ascending order.
+	 * For frames, short term pics are in descending pic num order and long
+	 * term ones in ascending order. For field this is the same direction
+	 * but with frame_num (wrapped). As for frame the pic_num and frame_num
+	 * are the same (see formula (8-28) and (8-29)) use the frame_num and share
+	 * this funciton between frame and field reflist.
 	 */
 	if (!builder->refs[idxa].longterm)
 		return builder->refs[idxb].frame_num <
 		       builder->refs[idxa].frame_num ?
 		       -1 : 1;
 
-	return builder->refs[idxa].pic_num < builder->refs[idxb].pic_num ?
+	return builder->refs[idxa].frame_num < builder->refs[idxb].frame_num ?
 	       -1 : 1;
 }
 
@@ -173,10 +175,10 @@ static int v4l2_h264_b0_ref_list_cmp(const void *ptra, const void *ptrb,
 			return 1;
 	}
 
-	/* Long term pics in ascending pic num order. */
+	/* Long term pics in ascending frame num order. */
 	if (builder->refs[idxa].longterm)
-		return builder->refs[idxa].pic_num <
-		       builder->refs[idxb].pic_num ?
+		return builder->refs[idxa].frame_num <
+		       builder->refs[idxb].frame_num ?
 		       -1 : 1;
 
 	poca = v4l2_h264_get_poc(builder, ptra);
@@ -218,10 +220,10 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
 			return 1;
 	}
 
-	/* Long term pics in ascending pic num order. */
+	/* Long term pics in ascending frame num order. */
 	if (builder->refs[idxa].longterm)
-		return builder->refs[idxa].pic_num <
-		       builder->refs[idxb].pic_num ?
+		return builder->refs[idxa].frame_num <
+		       builder->refs[idxb].frame_num ?
 		       -1 : 1;
 
 	poca = v4l2_h264_get_poc(builder, ptra);
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index 4cef717b3f18..0d9eaa956123 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -18,7 +18,6 @@
  * @refs.top_field_order_cnt: top field order count
  * @refs.bottom_field_order_cnt: bottom field order count
  * @refs.frame_num: reference frame number
- * @refs.pic_num: reference picture number
  * @refs.longterm: set to true for a long term reference
  * @refs: array of references
  * @cur_pic_order_count: picture order count of the frame being decoded
@@ -36,7 +35,6 @@ struct v4l2_h264_reflist_builder {
 		s32 top_field_order_cnt;
 		s32 bottom_field_order_cnt;
 		int frame_num;
-		u32 pic_num;
 		u16 longterm : 1;
 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
 
-- 
2.34.1

Re: [PATCH v1 07/24] media: h264: Sort p/b reflist using frame_num

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>In the reference list builder, frame_num referers to FrameNumWrap

s/referers/refers/

>in the spec, which is the same as the pic_num for frame decoding.
>The same applied for long_tern_pic_num and long_ter_frame_idx.

s/same applied/same applies/
s/long_ter_frame_idx/long_term_frame_idx/

>
>Sort all type of references by frame_num so the sort can be reused

s/frame_num so the sort/frame_num, so the order/

>for field reflist were the sorting is done using frame_num instead.

s/field/the field/
s/reflist were the sorting is done using frame_num instead/
   reflist, which is sorted by the frame_num instead/

>In short, pic_num is never actually used for building reference
>lists.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/media/v4l2-core/v4l2-h264.c | 22 ++++++++++++----------
> include/media/v4l2-h264.h           |  2 --
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index c9e18fd51d78..c3fad382882d 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -50,7 +50,6 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> 			continue;
>
>-		b->refs[i].pic_num = dpb[i].pic_num;
> 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> 			b->refs[i].longterm = true;
>
>@@ -139,15 +138,18 @@ static int v4l2_h264_p_ref_list_cmp(const void *ptra, const void *ptrb,
> 	}
>
> 	/*
>-	 * Short term pics in descending pic num order, long term ones in
>-	 * ascending order.
>+	 * For frames, short term pics are in descending pic num order and long
>+	 * term ones in ascending order. For field this is the same direction

s/For field this is the same direction/For fields the same direction is used/

>+	 * but with frame_num (wrapped). As for frame the pic_num and frame_num
>+	 * are the same (see formula (8-28) and (8-29)) use the frame_num and share

s/As for frame the pic_num and frame_num are the same/
   The fields pic_num and frame_num are the same for frames/

>+	 * this funciton between frame and field reflist.

s/funciton/function/

Greetings,
Sebastian

> 	 */
> 	if (!builder->refs[idxa].longterm)
> 		return builder->refs[idxb].frame_num <
> 		       builder->refs[idxa].frame_num ?
> 		       -1 : 1;
>
>-	return builder->refs[idxa].pic_num < builder->refs[idxb].pic_num ?
>+	return builder->refs[idxa].frame_num < builder->refs[idxb].frame_num ?
> 	       -1 : 1;
> }
>
>@@ -173,10 +175,10 @@ static int v4l2_h264_b0_ref_list_cmp(const void *ptra, const void *ptrb,
> 			return 1;
> 	}
>
>-	/* Long term pics in ascending pic num order. */
>+	/* Long term pics in ascending frame num order. */
> 	if (builder->refs[idxa].longterm)
>-		return builder->refs[idxa].pic_num <
>-		       builder->refs[idxb].pic_num ?
>+		return builder->refs[idxa].frame_num <
>+		       builder->refs[idxb].frame_num ?
> 		       -1 : 1;
>
> 	poca = v4l2_h264_get_poc(builder, ptra);
>@@ -218,10 +220,10 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
> 			return 1;
> 	}
>
>-	/* Long term pics in ascending pic num order. */
>+	/* Long term pics in ascending frame num order. */
> 	if (builder->refs[idxa].longterm)
>-		return builder->refs[idxa].pic_num <
>-		       builder->refs[idxb].pic_num ?
>+		return builder->refs[idxa].frame_num <
>+		       builder->refs[idxb].frame_num ?
> 		       -1 : 1;
>
> 	poca = v4l2_h264_get_poc(builder, ptra);
>diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
>index 4cef717b3f18..0d9eaa956123 100644
>--- a/include/media/v4l2-h264.h
>+++ b/include/media/v4l2-h264.h
>@@ -18,7 +18,6 @@
>  * @refs.top_field_order_cnt: top field order count
>  * @refs.bottom_field_order_cnt: bottom field order count
>  * @refs.frame_num: reference frame number
>- * @refs.pic_num: reference picture number
>  * @refs.longterm: set to true for a long term reference
>  * @refs: array of references
>  * @cur_pic_order_count: picture order count of the frame being decoded
>@@ -36,7 +35,6 @@ struct v4l2_h264_reflist_builder {
> 		s32 top_field_order_cnt;
> 		s32 bottom_field_order_cnt;
> 		int frame_num;
>-		u32 pic_num;
> 		u16 longterm : 1;
> 	} refs[V4L2_H264_NUM_DPB_ENTRIES];
>
>-- 
>2.34.1
>

[PATCH v1 08/24] media: v4l2: Reorder field reflist

[Nicolas Dufresne]

As per spec, the field refslist requires interleaving top and bottom
field in a specific way that does not fit inside the sort operation.
Reorder in-place the references so that their parity sart with the same
parity as the current picture and do that for both short and longterm
references separately.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-h264.c | 45 +++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
index c3fad382882d..2f7ee8d5479f 100644
--- a/drivers/media/v4l2-core/v4l2-h264.c
+++ b/drivers/media/v4l2-core/v4l2-h264.c
@@ -243,6 +243,43 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
 	return poca < pocb ? -1 : 1;
 }
 
+/*
+ * The references need to be reorder so that reference are alternating between
+ * top and bottom ref starting with the current picture parity. This have to be
+ * done for short term and long term references separately.
+ */
+static void reorder_field_reflist(const struct v4l2_h264_reflist_builder *b,
+				  struct v4l2_h264_reference *reflist)
+{
+	struct v4l2_h264_reference tmplist[V4L2_H264_REF_LIST_LEN];
+	u8 lt, i = 0, j = 0, k = 0;
+
+	memcpy(tmplist, reflist, sizeof(tmplist[0]) * b->num_valid);
+
+	for (lt = 0; lt <= 1; lt++) {
+		do {
+			for (; i < b->num_valid && b->refs[tmplist[i].index].longterm == lt; i++) {
+				if (tmplist[i].fields == b->cur_pic_fields) {
+					reflist[k] = tmplist[i];
+					k++;
+					i++;
+					break;
+				}
+			}
+
+			for (; j < b->num_valid && b->refs[tmplist[j].index].longterm == lt; j++) {
+				if (tmplist[j].fields != b->cur_pic_fields) {
+					reflist[k] = tmplist[j];
+					k++;
+					j++;
+					break;
+				}
+			}
+		} while ((i < b->num_valid && b->refs[tmplist[i].index].longterm == lt) ||
+			 (j < b->num_valid && b->refs[tmplist[j].index].longterm == lt));
+	}
+}
+
 static char ref_type_to_char (u8 ref_type)
 {
 	switch (ref_type) {
@@ -345,6 +382,9 @@ v4l2_h264_build_p_ref_list(const struct v4l2_h264_reflist_builder *builder,
 	sort_r(reflist, builder->num_valid, sizeof(*reflist),
 	       v4l2_h264_p_ref_list_cmp, NULL, builder);
 
+	if (builder->cur_pic_fields != V4L2_H264_FRAME_REF)
+		reorder_field_reflist(builder, reflist);
+
 	print_ref_list_p(builder, reflist);
 }
 EXPORT_SYMBOL_GPL(v4l2_h264_build_p_ref_list);
@@ -378,6 +418,11 @@ v4l2_h264_build_b_ref_lists(const struct v4l2_h264_reflist_builder *builder,
 	sort_r(b1_reflist, builder->num_valid, sizeof(*b1_reflist),
 	       v4l2_h264_b1_ref_list_cmp, NULL, builder);
 
+	if (builder->cur_pic_fields != V4L2_H264_FRAME_REF) {
+		reorder_field_reflist(builder, b0_reflist);
+		reorder_field_reflist(builder, b1_reflist);
+	}
+
 	if (builder->num_valid > 1 &&
 	    !memcmp(b1_reflist, b0_reflist, builder->num_valid))
 		swap(b1_reflist[0], b1_reflist[1]);
-- 
2.34.1

Re: [PATCH v1 08/24] media: v4l2: Reorder field reflist

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>As per spec, the field refslist requires interleaving top and bottom

In other patches you call it always field reflist, so I'd say let's
stick to that:
s/field refslist/field reflist/

>field in a specific way that does not fit inside the sort operation.
>Reorder in-place the references so that their parity sart with the same
>parity as the current picture and do that for both short and longterm
>references separately.

I find that sentence hard to understand, is this maybe better:

"""
Rearrange the references in place so that their parity matches that of
the current image, and do this separately for both short- and long-term
references.
"""

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/media/v4l2-core/v4l2-h264.c | 45 +++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
>diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c
>index c3fad382882d..2f7ee8d5479f 100644
>--- a/drivers/media/v4l2-core/v4l2-h264.c
>+++ b/drivers/media/v4l2-core/v4l2-h264.c
>@@ -243,6 +243,43 @@ static int v4l2_h264_b1_ref_list_cmp(const void *ptra, const void *ptrb,
> 	return poca < pocb ? -1 : 1;
> }
>
>+/*
>+ * The references need to be reorder so that reference are alternating between

s/to be reorder/to be reordered/
s/ so that reference/, so that references/

>+ * top and bottom ref starting with the current picture parity. This have to be

s/bottom ref/bottom field references/
s/This have to be/This has to be/

>+ * done for short term and long term references separately.
>+ */
>+static void reorder_field_reflist(const struct v4l2_h264_reflist_builder *b,
>+				  struct v4l2_h264_reference *reflist)
>+{
>+	struct v4l2_h264_reference tmplist[V4L2_H264_REF_LIST_LEN];
>+	u8 lt, i = 0, j = 0, k = 0;
>+
>+	memcpy(tmplist, reflist, sizeof(tmplist[0]) * b->num_valid);
>+
>+	for (lt = 0; lt <= 1; lt++) {
>+		do {
>+			for (; i < b->num_valid && b->refs[tmplist[i].index].longterm == lt; i++) {
>+				if (tmplist[i].fields == b->cur_pic_fields) {
>+					reflist[k] = tmplist[i];
>+					k++;
>+					i++;

You can just say: `reflist[k++] = tmplist[i++];`

>+					break;
>+				}
>+			}
>+
>+			for (; j < b->num_valid && b->refs[tmplist[j].index].longterm == lt; j++) {
>+				if (tmplist[j].fields != b->cur_pic_fields) {
>+					reflist[k] = tmplist[j];
>+					k++;
>+					j++;

Same here: `reflist[k++] = tmplist[j++];`

Greetings,
Sebastian

>+					break;
>+				}
>+			}
>+		} while ((i < b->num_valid && b->refs[tmplist[i].index].longterm == lt) ||
>+			 (j < b->num_valid && b->refs[tmplist[j].index].longterm == lt));
>+	}
>+}
>+
> static char ref_type_to_char (u8 ref_type)
> {
> 	switch (ref_type) {
>@@ -345,6 +382,9 @@ v4l2_h264_build_p_ref_list(const struct v4l2_h264_reflist_builder *builder,
> 	sort_r(reflist, builder->num_valid, sizeof(*reflist),
> 	       v4l2_h264_p_ref_list_cmp, NULL, builder);
>
>+	if (builder->cur_pic_fields != V4L2_H264_FRAME_REF)
>+		reorder_field_reflist(builder, reflist);
>+
> 	print_ref_list_p(builder, reflist);
> }
> EXPORT_SYMBOL_GPL(v4l2_h264_build_p_ref_list);
>@@ -378,6 +418,11 @@ v4l2_h264_build_b_ref_lists(const struct v4l2_h264_reflist_builder *builder,
> 	sort_r(b1_reflist, builder->num_valid, sizeof(*b1_reflist),
> 	       v4l2_h264_b1_ref_list_cmp, NULL, builder);
>
>+	if (builder->cur_pic_fields != V4L2_H264_FRAME_REF) {
>+		reorder_field_reflist(builder, b0_reflist);
>+		reorder_field_reflist(builder, b1_reflist);
>+	}
>+
> 	if (builder->num_valid > 1 &&
> 	    !memcmp(b1_reflist, b0_reflist, builder->num_valid))
> 		swap(b1_reflist[0], b1_reflist[1]);
>-- 
>2.34.1
>

[PATCH v1 09/24] media: v4l2-mem2mem: Fix typo in trace message

[Nicolas Dufresne]

On -> One

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 675e22895ebe..53c2332d5cbd 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -316,7 +316,7 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
 	}
 
 	if (m2m_ctx->job_flags & TRANS_QUEUED) {
-		dprintk("On job queue already\n");
+		dprintk("One job queue already\n");
 		goto job_unlock;
 	}
 
-- 
2.34.1

Re: [PATCH v1 09/24] media: v4l2-mem2mem: Fix typo in trace message

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>On -> One

Hmmmm the message "One job queue already" doesn't sound correct. I think
the message wants to say that the buffer is already on the queue.

We could maybe enhance the message like:
"Buffer already found on the job queue\n"

But this is not a typo from my POV.

Greetings,
Sebastian

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>index 675e22895ebe..53c2332d5cbd 100644
>--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>@@ -316,7 +316,7 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
> 	}
>
> 	if (m2m_ctx->job_flags & TRANS_QUEUED) {
>-		dprintk("On job queue already\n");
>+		dprintk("One job queue already\n");
> 		goto job_unlock;
> 	}
>
>-- 
>2.34.1
>

Re: [PATCH v1 09/24] media: v4l2-mem2mem: Fix typo in trace message

[Nicolas Dufresne]

Le mardi 29 mars 2022 à 15:57 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > On -> One
> 
> Hmmmm the message "One job queue already" doesn't sound correct. I think
> the message wants to say that the buffer is already on the queue.
> 
> We could maybe enhance the message like:
> "Buffer already found on the job queue\n"

I think I read queue -> queued. The new message would be inaccurate with this
suggestion. I'll just drop that patch in V2, the fact that this message was
miss-leading to me is irrelevant to the patchset.
> 
> But this is not a typo from my POV.
> 
> Greetings,
> Sebastian
> 
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> > drivers/media/v4l2-core/v4l2-mem2mem.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > index 675e22895ebe..53c2332d5cbd 100644
> > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > @@ -316,7 +316,7 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
> > 	}
> > 
> > 	if (m2m_ctx->job_flags & TRANS_QUEUED) {
> > -		dprintk("On job queue already\n");
> > +		dprintk("One job queue already\n");
> > 		goto job_unlock;
> > 	}
> > 
> > -- 
> > 2.34.1
> > 

[PATCH v1 10/24] media: v4l2-mem2mem: Trace on implicit un-hold

[Nicolas Dufresne]

If the timestamp of the src buffer differs from the timestamp of a held
dst buffer, the held buffer is implicitly removed and marked as done.
Add a trace to help debugging if someone hits that case.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 53c2332d5cbd..d3e739ef16ef 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -336,6 +336,7 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
 	if (src && dst && dst->is_held &&
 	    dst->vb2_buf.copied_timestamp &&
 	    dst->vb2_buf.timestamp != src->vb2_buf.timestamp) {
+		dprintk("src and dst timestamp mismatch, removing held capture buffer.\n");
 		dst->is_held = false;
 		v4l2_m2m_dst_buf_remove(m2m_ctx);
 		v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE);
-- 
2.34.1

Re: [PATCH v1 10/24] media: v4l2-mem2mem: Trace on implicit un-hold

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>If the timestamp of the src buffer differs from the timestamp of a held
>dst buffer, the held buffer is implicitly removed and marked as done.
>Add a trace to help debugging if someone hits that case.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>index 53c2332d5cbd..d3e739ef16ef 100644
>--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>@@ -336,6 +336,7 @@ static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev,
> 	if (src && dst && dst->is_held &&
> 	    dst->vb2_buf.copied_timestamp &&
> 	    dst->vb2_buf.timestamp != src->vb2_buf.timestamp) {
>+		dprintk("src and dst timestamp mismatch, removing held capture buffer.\n");
> 		dst->is_held = false;
> 		v4l2_m2m_dst_buf_remove(m2m_ctx);
> 		v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE);
>-- 
>2.34.1
>

[PATCH v1 11/24] media: videobuf2-v4l2: Warn on holding buffers without support

[Nicolas Dufresne]

From: Sebastian Fricke <sebastian.fricke@collabora.com>

Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
silently ignoring it.
Warn the user via a debug print when the flag is requested but ignored
by the videobuf2 framework.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 6edf4508c636..812c8d1962e0 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -329,8 +329,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
 		 */
 		vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
 		vbuf->field = b->field;
-		if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
+		if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
+			if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
+				dprintk(q, 1,
+					"Request holding buffer (%d), unsupported on output queue\n",
+					b->index);
 			vbuf->flags &= ~V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
+		}
 	} else {
 		/* Zero any output buffer flags as this is a capture buffer */
 		vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS;
-- 
2.34.1

[PATCH v1 12/24] media: rkvdec: Stop overclocking the decoder

[Nicolas Dufresne]

While this overclock hack seems to works on some implementation (some
ChromeBooks, RockPi4) it is also causing instability on other
implementation (notably LibreComputer Renegade, but saw more reports in
the LibreELEC project, were this is already removed). While performance is
indeed affectied (tested with GStreamer), 4K playback still works as long
as you don't operate in lock step and keep at least 1 frame ahead of time
in the decode queue.

After discussion with ChromeOS members, it would seem that their
implementation indeed synchronously decode each frames, so this hack was
simply compensating for their code being less efficienti. This hack
should in my opinion have stayed downstream and I'm removing it to ensure
stability across all RK3399 variants.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index c0cf3488f970..2df8cf4883e2 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -1027,12 +1027,6 @@ static int rkvdec_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/*
-	 * Bump ACLK to max. possible freq. (500 MHz) to improve performance
-	 * When 4k video playback.
-	 */
-	clk_set_rate(rkvdec->clocks[0].clk, 500 * 1000 * 1000);
-
 	rkvdec->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(rkvdec->regs))
 		return PTR_ERR(rkvdec->regs);
-- 
2.34.1

Re: [PATCH v1 12/24] media: rkvdec: Stop overclocking the decoder

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>While this overclock hack seems to works on some implementation (some

s/implementation/implementations/

>ChromeBooks, RockPi4) it is also causing instability on other

s/causing/causes/

>implementation (notably LibreComputer Renegade, but saw more reports in

s/implementation/implementations/

s/but saw more reports/but there were more reports/

>the LibreELEC project, were this is already removed). While performance is

s/were this is/where this is/

>indeed affectied (tested with GStreamer), 4K playback still works as long

s/affectied/affected/

>as you don't operate in lock step and keep at least 1 frame ahead of time

s/lock step/lock step mode/

>in the decode queue.
>
>After discussion with ChromeOS members, it would seem that their
>implementation indeed synchronously decode each frames, so this hack was

s/decode each frames/decodes each frame/

>simply compensating for their code being less efficienti. This hack

s/efficienti/efficient/

>should in my opinion have stayed downstream and I'm removing it to ensure

s/This hack should in my opinion have stayed downstream/
   In my opinion, this hack should not have been included upstream/

>stability across all RK3399 variants.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/staging/media/rkvdec/rkvdec.c | 6 ------
> 1 file changed, 6 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>index c0cf3488f970..2df8cf4883e2 100644
>--- a/drivers/staging/media/rkvdec/rkvdec.c
>+++ b/drivers/staging/media/rkvdec/rkvdec.c
>@@ -1027,12 +1027,6 @@ static int rkvdec_probe(struct platform_device *pdev)
> 	if (ret)
> 		return ret;
>
>-	/*
>-	 * Bump ACLK to max. possible freq. (500 MHz) to improve performance
>-	 * When 4k video playback.
>-	 */
>-	clk_set_rate(rkvdec->clocks[0].clk, 500 * 1000 * 1000);
>-
> 	rkvdec->regs = devm_platform_ioremap_resource(pdev, 0);
> 	if (IS_ERR(rkvdec->regs))
> 		return PTR_ERR(rkvdec->regs);
>-- 
>2.34.1
>

[PATCH v1 13/24] media: rkvdec: h264: Fix reference frame_num wrap for second field

[Nicolas Dufresne]

From: Jonas Karlman <jonas@kwiboo.se>

When decoding the second field in a complementary field pair the second
field is sharing the same frame_num with the first field.

Currently the frame_num for the first field is wrapped when it matches the
field being decoded, this cause issues to decode the second field in a
complementary field pair.

Fix this by using inclusive comparison, less than or equal.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index dff89732ddd0..842d8cd80e90 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -752,7 +752,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			continue;
 
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
-		    dpb[i].frame_num < dec_params->frame_num) {
+		    dpb[i].frame_num <= dec_params->frame_num) {
 			p[i] = dpb[i].frame_num;
 			continue;
 		}
-- 
2.34.1

Re: [PATCH v1 13/24] media: rkvdec: h264: Fix reference frame_num wrap for second field

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Jonas Karlman <jonas@kwiboo.se>
>
>When decoding the second field in a complementary field pair the second
>field is sharing the same frame_num with the first field.
>
>Currently the frame_num for the first field is wrapped when it matches the
>field being decoded, this cause issues to decode the second field in a
>complementary field pair.
>
>Fix this by using inclusive comparison, less than or equal.
>
>Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index dff89732ddd0..842d8cd80e90 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -752,7 +752,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> 			continue;
>
> 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
>-		    dpb[i].frame_num < dec_params->frame_num) {
>+		    dpb[i].frame_num <= dec_params->frame_num) {
> 			p[i] = dpb[i].frame_num;
> 			continue;
> 		}
>-- 
>2.34.1
>

[PATCH v1 14/24] media: rkvdec: h264: Fix dpb_valid implementation

[Nicolas Dufresne]

The ref builder only provided reference that are marked as valid in the
dpb.  Thus the current implementation of dpb_valid would always set the
flag to 1.  This is not representing missing frames (this is called
'non-existing' pictures in the spec). In some context, these non-existing
pictures still need occupy a slot in the reference list according to the
spec.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 842d8cd80e90..db1e762baee5 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -112,6 +112,7 @@ struct rkvdec_h264_run {
 	const struct v4l2_ctrl_h264_sps *sps;
 	const struct v4l2_ctrl_h264_pps *pps;
 	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
+	int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
 };
 
 struct rkvdec_h264_ctx {
@@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 	}
 }
 
+static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
+			       struct rkvdec_h264_run *run)
+{
+	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
+	u32 i;
+
+	for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
+		struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
+		const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
+		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
+		int buf_idx = -1;
+
+		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
+			buf_idx = vb2_find_timestamp(cap_q,
+						     dpb[i].reference_ts, 0);
+
+		run->ref_buf_idx[i] = buf_idx;
+	}
+}
+
 static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			    struct rkvdec_h264_run *run)
 {
@@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 
 	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
 		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
-			u8 dpb_valid = 0;
+			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
 			u8 idx = 0;
 
 			switch (j) {
@@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 
 			if (idx >= ARRAY_SIZE(dec_params->dpb))
 				continue;
-			dpb_valid = !!(dpb[idx].flags &
-				       V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
 
 			set_ps_field(hw_rps, DPB_INFO(i, j),
 				     idx | dpb_valid << 4);
@@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
 	    unsigned int dpb_idx)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
-	const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
 	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
-	int buf_idx = -1;
-
-	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
-		buf_idx = vb2_find_timestamp(cap_q,
-					     dpb[dpb_idx].reference_ts, 0);
+	int buf_idx = run->ref_buf_idx[dpb_idx];
 
 	/*
 	 * If a DPB entry is unused or invalid, address of current destination
@@ -1102,6 +1116,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 
 	assemble_hw_scaling_list(ctx, &run);
 	assemble_hw_pps(ctx, &run);
+	lookup_ref_buf_idx(ctx, &run);
 	assemble_hw_rps(ctx, &run);
 	config_registers(ctx, &run);
 
-- 
2.34.1

Re: [PATCH v1 14/24] media: rkvdec: h264: Fix dpb_valid implementation

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>The ref builder only provided reference that are marked as valid in the

s/reference/references/

>dpb.  Thus the current implementation of dpb_valid would always set the
>flag to 1.  This is not representing missing frames (this is called

s/this is//

>'non-existing' pictures in the spec). In some context, these non-existing
>pictures still need occupy a slot in the reference list according to the

s/occupy/to occupy/

>spec.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index 842d8cd80e90..db1e762baee5 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -112,6 +112,7 @@ struct rkvdec_h264_run {
> 	const struct v4l2_ctrl_h264_sps *sps;
> 	const struct v4l2_ctrl_h264_pps *pps;
> 	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
>+	int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
> };
>
> struct rkvdec_h264_ctx {
>@@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> 	}
> }
>
>+static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
>+			       struct rkvdec_h264_run *run)
>+{
>+	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
>+	u32 i;
>+
>+	for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
>+		struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
>+		const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
>+		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
>+		int buf_idx = -1;
>+
>+		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
>+			buf_idx = vb2_find_timestamp(cap_q,
>+						     dpb[i].reference_ts, 0);
>+
>+		run->ref_buf_idx[i] = buf_idx;
>+	}
>+}
>+
> static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> 			    struct rkvdec_h264_run *run)
> {
>@@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>
> 	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> 		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
>-			u8 dpb_valid = 0;
>+			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> 			u8 idx = 0;
>
> 			switch (j) {
>@@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>
> 			if (idx >= ARRAY_SIZE(dec_params->dpb))
> 				continue;
>-			dpb_valid = !!(dpb[idx].flags &
>-				       V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
>
> 			set_ps_field(hw_rps, DPB_INFO(i, j),
> 				     idx | dpb_valid << 4);
>@@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
> 	    unsigned int dpb_idx)
> {
> 	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
>-	const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> 	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
>-	int buf_idx = -1;
>-
>-	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
>-		buf_idx = vb2_find_timestamp(cap_q,
>-					     dpb[dpb_idx].reference_ts, 0);
>+	int buf_idx = run->ref_buf_idx[dpb_idx];
>
> 	/*
> 	 * If a DPB entry is unused or invalid, address of current destination
>@@ -1102,6 +1116,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>
> 	assemble_hw_scaling_list(ctx, &run);
> 	assemble_hw_pps(ctx, &run);
>+	lookup_ref_buf_idx(ctx, &run);
> 	assemble_hw_rps(ctx, &run);
> 	config_registers(ctx, &run);
>
>-- 
>2.34.1
>

[PATCH v1 15/24] media: rkvdec: Enable capture buffer holding for H264

[Nicolas Dufresne]

In order to support interlaced video decoding, the driver must
allow holding the capture buffer so that the second field can
be decoded into it.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 4 ++++
 drivers/staging/media/rkvdec/rkvdec.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 2df8cf4883e2..05824f1997f7 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -138,6 +138,7 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
 		.ops = &rkvdec_h264_fmt_ops,
 		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
 		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
+		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
 	},
 	{
 		.fourcc = V4L2_PIX_FMT_VP9_FRAME,
@@ -394,6 +395,9 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
 	cap_fmt->fmt.pix_mp.ycbcr_enc = f->fmt.pix_mp.ycbcr_enc;
 	cap_fmt->fmt.pix_mp.quantization = f->fmt.pix_mp.quantization;
 
+	/* Enable format specific queue feature */
+	vq->subsystem_flags |= desc->subsystem_flags;
+
 	return 0;
 }
 
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 2f4ea1786b93..e37f1a015fa0 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -81,6 +81,7 @@ struct rkvdec_coded_fmt_desc {
 	const struct rkvdec_coded_fmt_ops *ops;
 	unsigned int num_decoded_fmts;
 	const u32 *decoded_fmts;
+	u32 subsystem_flags;
 };
 
 struct rkvdec_dev {
-- 
2.34.1

Re: [PATCH v1 15/24] media: rkvdec: Enable capture buffer holding for H264

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>In order to support interlaced video decoding, the driver must
>allow holding the capture buffer so that the second field can
>be decoded into it.
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/staging/media/rkvdec/rkvdec.c | 4 ++++
> drivers/staging/media/rkvdec/rkvdec.h | 1 +
> 2 files changed, 5 insertions(+)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>index 2df8cf4883e2..05824f1997f7 100644
>--- a/drivers/staging/media/rkvdec/rkvdec.c
>+++ b/drivers/staging/media/rkvdec/rkvdec.c
>@@ -138,6 +138,7 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
> 		.ops = &rkvdec_h264_fmt_ops,
> 		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
> 		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
>+		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
> 	},
> 	{
> 		.fourcc = V4L2_PIX_FMT_VP9_FRAME,
>@@ -394,6 +395,9 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
> 	cap_fmt->fmt.pix_mp.ycbcr_enc = f->fmt.pix_mp.ycbcr_enc;
> 	cap_fmt->fmt.pix_mp.quantization = f->fmt.pix_mp.quantization;
>
>+	/* Enable format specific queue feature */

s/feature/features/

Greetings,
Sebastian

>+	vq->subsystem_flags |= desc->subsystem_flags;
>+
> 	return 0;
> }
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>index 2f4ea1786b93..e37f1a015fa0 100644
>--- a/drivers/staging/media/rkvdec/rkvdec.h
>+++ b/drivers/staging/media/rkvdec/rkvdec.h
>@@ -81,6 +81,7 @@ struct rkvdec_coded_fmt_desc {
> 	const struct rkvdec_coded_fmt_ops *ops;
> 	unsigned int num_decoded_fmts;
> 	const u32 *decoded_fmts;
>+	u32 subsystem_flags;
> };
>
> struct rkvdec_dev {
>-- 
>2.34.1
>

[PATCH v1 16/24] media: rkvdec: Ensure decoded resolution fit coded resolution

[Nicolas Dufresne]

From: Jonas Karlman <jonas@kwiboo.se>

Ensure decoded CAPTURE buffer resolution is larger or equal to the coded
OPTUPT buffer resolution.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 05824f1997f7..22c0382c579e 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -269,6 +269,8 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
 		pix_mp->pixelformat = coded_desc->decoded_fmts[0];
 
 	/* Always apply the frmsize constraint of the coded end. */
+	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
+	pix_mp->height = max(pix_mp->height, ctx->coded_fmt.fmt.pix_mp.height);
 	v4l2_apply_frmsize_constraints(&pix_mp->width,
 				       &pix_mp->height,
 				       &coded_desc->frmsize);
-- 
2.34.1

Re: [PATCH v1 16/24] media: rkvdec: Ensure decoded resolution fit coded resolution

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Jonas Karlman <jonas@kwiboo.se>
>
>Ensure decoded CAPTURE buffer resolution is larger or equal to the coded
>OPTUPT buffer resolution.

s/OPTUPT/OUTPUT/

>
>Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/staging/media/rkvdec/rkvdec.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>index 05824f1997f7..22c0382c579e 100644
>--- a/drivers/staging/media/rkvdec/rkvdec.c
>+++ b/drivers/staging/media/rkvdec/rkvdec.c
>@@ -269,6 +269,8 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
> 		pix_mp->pixelformat = coded_desc->decoded_fmts[0];
>
> 	/* Always apply the frmsize constraint of the coded end. */

s/frmsize/framesize/
s/constraint/constraints/
s/coded end/coded format/

Greetings,
Sebastian

>+	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
>+	pix_mp->height = max(pix_mp->height, ctx->coded_fmt.fmt.pix_mp.height);
> 	v4l2_apply_frmsize_constraints(&pix_mp->width,
> 				       &pix_mp->height,
> 				       &coded_desc->frmsize);
>-- 
>2.34.1
>

Re: [PATCH v1 16/24] media: rkvdec: Ensure decoded resolution fit coded resolution

[Sebastian Fricke]

Hey Nicolas,

Also typo in title:
s/Ensure decoded resolution fit coded resolution/
   Ensure decoded resolution fits coded resolution/

Greetings,
Sebastian

On 29.03.2022 17:39, Sebastian Fricke wrote:
>Hey Nicolas,
>
>On 28.03.2022 15:59, Nicolas Dufresne wrote:
>>From: Jonas Karlman <jonas@kwiboo.se>
>>
>>Ensure decoded CAPTURE buffer resolution is larger or equal to the coded
>>OPTUPT buffer resolution.
>
>s/OPTUPT/OUTPUT/
>
>>
>>Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>
>>---
>>drivers/staging/media/rkvdec/rkvdec.c | 2 ++
>>1 file changed, 2 insertions(+)
>>
>>diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>index 05824f1997f7..22c0382c579e 100644
>>--- a/drivers/staging/media/rkvdec/rkvdec.c
>>+++ b/drivers/staging/media/rkvdec/rkvdec.c
>>@@ -269,6 +269,8 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
>>		pix_mp->pixelformat = coded_desc->decoded_fmts[0];
>>
>>	/* Always apply the frmsize constraint of the coded end. */
>
>s/frmsize/framesize/
>s/constraint/constraints/
>s/coded end/coded format/
>
>Greetings,
>Sebastian
>
>>+	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
>>+	pix_mp->height = max(pix_mp->height, ctx->coded_fmt.fmt.pix_mp.height);
>>	v4l2_apply_frmsize_constraints(&pix_mp->width,
>>				       &pix_mp->height,
>>				       &coded_desc->frmsize);
>>-- 
>>2.34.1
>>

[PATCH v1 17/24] media: rkvdec: h264: Validate and use pic width and height in mbs

[Nicolas Dufresne]

From: Jonas Karlman <jonas@kwiboo.se>

The width and height in mbs is currently configured based on OUTPUT buffer
resolution, this works for frame pictures but can cause issues for field
pictures.

When frame_mbs_only_flag is 0 the height in mbs should be height of
the field instead of height of frame.

Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
against OUTPUT buffer resolution and use these values to configure HW.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c |  4 ++--
 drivers/staging/media/rkvdec/rkvdec.c      | 10 ++++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index db1e762baee5..847b8957dad3 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
 		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
+	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
+	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
 		  FRAME_MBS_ONLY_FLAG);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 22c0382c579e..67539f4bf382 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -29,8 +29,11 @@
 
 static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
+	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
+
 	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
+		unsigned int width, height;
 		/*
 		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
 		 * but it's currently broken in the driver.
@@ -45,6 +48,13 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 		if (sps->bit_depth_luma_minus8 != 0)
 			/* Only 8-bit is supported */
 			return -EINVAL;
+
+		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
+		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
+
+		if (width > ctx->coded_fmt.fmt.pix_mp.width ||
+		    height > ctx->coded_fmt.fmt.pix_mp.height)
+			return -EINVAL;
 	}
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v1 17/24] media: rkvdec: h264: Validate and use pic width and height in mbs

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Jonas Karlman <jonas@kwiboo.se>
>
>The width and height in mbs is currently configured based on OUTPUT buffer
>resolution, this works for frame pictures but can cause issues for field
>pictures.

How about:

"""
The width and height measured in macroblocks (mbs) is currently
configured based on the resolution of the OUTPUT buffer, this works for
frame pictures but can cause issues for field pictures..
"""

(I think it improves readability to explain at least once what mbs means
to anyone not aware)

>
>When frame_mbs_only_flag is 0 the height in mbs should be height of
>the field instead of height of frame.
>
>Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
>against OUTPUT buffer resolution and use these values to configure HW.

s/OUTPUT buffer resolution/the resolution of the OUTPUT buffer/

>
>Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian
>---
> drivers/staging/media/rkvdec/rkvdec-h264.c |  4 ++--
> drivers/staging/media/rkvdec/rkvdec.c      | 10 ++++++++++
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index db1e762baee5..847b8957dad3 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> 		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
> 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
> 		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
>-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
>-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
>+	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
>+	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
> 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
> 		  FRAME_MBS_ONLY_FLAG);
> 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
>diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>index 22c0382c579e..67539f4bf382 100644
>--- a/drivers/staging/media/rkvdec/rkvdec.c
>+++ b/drivers/staging/media/rkvdec/rkvdec.c
>@@ -29,8 +29,11 @@
>
> static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> {
>+	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>+
> 	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
> 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>+		unsigned int width, height;
> 		/*
> 		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
> 		 * but it's currently broken in the driver.
>@@ -45,6 +48,13 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> 		if (sps->bit_depth_luma_minus8 != 0)
> 			/* Only 8-bit is supported */
> 			return -EINVAL;
>+
>+		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
>+		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
>+
>+		if (width > ctx->coded_fmt.fmt.pix_mp.width ||
>+		    height > ctx->coded_fmt.fmt.pix_mp.height)
>+			return -EINVAL;
> 	}
> 	return 0;
> }
>-- 
>2.34.1
>

[PATCH v1 18/24] media: rkvdec: h264: Fix bit depth wrap in pps packet

[Nicolas Dufresne]

From: Jonas Karlman <jonas@kwiboo.se>

The luma and chroma bit depth fields in the pps packet is 3 bits wide.
8 is wrongly added to the bit depth value written to these 3-bit fields.
Because only the 3 LSB is written the hardware is configured correctly.

Correct this by not adding 8 to the luma and chroma bit depth value.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 847b8957dad3..ec52b195bbd7 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -662,8 +662,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 	WRITE_PPS(0xff, PROFILE_IDC);
 	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
-	WRITE_PPS(sps->bit_depth_luma_minus8 + 8, BIT_DEPTH_LUMA);
-	WRITE_PPS(sps->bit_depth_chroma_minus8 + 8, BIT_DEPTH_CHROMA);
+	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
+	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
 	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
-- 
2.34.1

Re: [PATCH v1 18/24] media: rkvdec: h264: Fix bit depth wrap in pps packet

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Jonas Karlman <jonas@kwiboo.se>
>
>The luma and chroma bit depth fields in the pps packet is 3 bits wide.

s/is 3 bits wide/are 3 bits wide/

>8 is wrongly added to the bit depth value written to these 3-bit fields.

s/bit depth value/bit depth values/

(as we talk about multiple different values)

>Because only the 3 LSB is written the hardware is configured correctly.

s/Because only the 3 LSB is written the hardware is configured correctly./
   Because only the three least significant bits are written, the hardware will be configured correctly./

(original sentence is very hard to read, the sentence could also mean
something like this:
'Because only the three least significant bits, that are written to the hardware, are configured correctly.')

>
>Correct this by not adding 8 to the luma and chroma bit depth value.
>
>Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index 847b8957dad3..ec52b195bbd7 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -662,8 +662,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> 	WRITE_PPS(0xff, PROFILE_IDC);
> 	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
> 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
>-	WRITE_PPS(sps->bit_depth_luma_minus8 + 8, BIT_DEPTH_LUMA);
>-	WRITE_PPS(sps->bit_depth_chroma_minus8 + 8, BIT_DEPTH_CHROMA);
>+	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
>+	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
> 	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
> 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
> 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
>-- 
>2.34.1
>

[PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Nicolas Dufresne]

This makes use of the new feature in the reference builder to program
up to 32 references when doing field decoding. It also signals the
parity (top of bottom) of the field to the hardware.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 48 ++++++++++------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index ec52b195bbd7..891c48bf6a51 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -97,13 +97,10 @@ struct rkvdec_h264_priv_tbl {
 	u8 err_info[RKV_ERROR_INFO_SIZE];
 };
 
-#define RKVDEC_H264_DPB_SIZE 16
-
 struct rkvdec_h264_reflists {
 	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
 	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
 	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
-	u8 num_valid;
 };
 
 struct rkvdec_h264_run {
@@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
 		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
 		int buf_idx = -1;
 
-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
+		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
 			buf_idx = vb2_find_timestamp(cap_q,
 						     dpb[i].reference_ts, 0);
+			if (buf_idx < 0)
+				pr_debug("No buffer for reference_ts %llu",
+					 dpb[i].reference_ts);
+		}
 
 		run->ref_buf_idx[i] = buf_idx;
 	}
 }
 
 static void assemble_hw_rps(struct rkvdec_ctx *ctx,
+			    struct v4l2_h264_reflist_builder *builder,
 			    struct rkvdec_h264_run *run)
 {
 	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
 	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
 	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
-	const struct v4l2_ctrl_h264_sps *sps = run->sps;
 	struct rkvdec_h264_priv_tbl *priv_tbl = h264_ctx->priv_tbl.cpu;
-	u32 max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
 
 	u32 *hw_rps = priv_tbl->rps;
 	u32 i, j;
@@ -772,37 +772,36 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
 			continue;
 
-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
-		    dpb[i].frame_num <= dec_params->frame_num) {
-			p[i] = dpb[i].frame_num;
-			continue;
-		}
-
-		p[i] = dpb[i].frame_num - max_frame_num;
+		p[i] = builder->refs[i].frame_num;
 	}
 
 	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
-		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
-			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
-			u8 idx = 0;
+		for (i = 0; i < builder->num_valid; i++) {
+			struct v4l2_h264_reference *ref;
+			u8 dpb_valid;
+			u8 bottom;
 
 			switch (j) {
 			case 0:
-				idx = h264_ctx->reflists.p[i].index;
+				ref = &h264_ctx->reflists.p[i];
 				break;
 			case 1:
-				idx = h264_ctx->reflists.b0[i].index;
+				ref = &h264_ctx->reflists.b0[i];
 				break;
 			case 2:
-				idx = h264_ctx->reflists.b1[i].index;
+				ref = &h264_ctx->reflists.b1[i];
 				break;
 			}
 
-			if (idx >= ARRAY_SIZE(dec_params->dpb))
+			if (WARN_ON(ref->index >= ARRAY_SIZE(dec_params->dpb)))
 				continue;
 
+			dpb_valid = run->ref_buf_idx[ref->index] >= 0;
+			bottom = ref->fields == V4L2_H264_BOTTOM_FIELD_REF;
+
 			set_ps_field(hw_rps, DPB_INFO(i, j),
-				     idx | dpb_valid << 4);
+				     ref->index | dpb_valid << 4);
+			set_ps_field(hw_rps, BOTTOM_FLAG(i, j), bottom);
 		}
 	}
 }
@@ -990,10 +989,6 @@ static void config_registers(struct rkvdec_ctx *ctx,
 				       rkvdec->regs + RKVDEC_REG_H264_BASE_REFER15);
 	}
 
-	/*
-	 * Since support frame mode only
-	 * top_field_order_cnt is the same as bottom_field_order_cnt
-	 */
 	reg = RKVDEC_CUR_POC(dec_params->top_field_order_cnt);
 	writel_relaxed(reg, rkvdec->regs + RKVDEC_REG_CUR_POC0);
 
@@ -1109,7 +1104,6 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 	/* Build the P/B{0,1} ref lists. */
 	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
 				       run.sps, run.decode_params->dpb);
-	h264_ctx->reflists.num_valid = reflist_builder.num_valid;
 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
 				    h264_ctx->reflists.b1);
@@ -1117,7 +1111,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 	assemble_hw_scaling_list(ctx, &run);
 	assemble_hw_pps(ctx, &run);
 	lookup_ref_buf_idx(ctx, &run);
-	assemble_hw_rps(ctx, &run);
+	assemble_hw_rps(ctx, &reflist_builder, &run);
 	config_registers(ctx, &run);
 
 	rkvdec_run_postamble(ctx, &run.base);
-- 
2.34.1

Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Dan Carpenter]

On Mon, Mar 28, 2022 at 03:59:31PM -0400, Nicolas Dufresne wrote:
> @@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
>  		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
>  		int buf_idx = -1;
>  
> -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> +		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
>  			buf_idx = vb2_find_timestamp(cap_q,
>  						     dpb[i].reference_ts, 0);
> +			if (buf_idx < 0)
> +				pr_debug("No buffer for reference_ts %llu",
> +					 dpb[i].reference_ts);

pr_debug() is too quiet.  Make it pr_err().  Set buf_idx to zero instead
leaving it as an error code.

> +		}
>  
>  		run->ref_buf_idx[i] = buf_idx;
>  	}
>  }
>  
>  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> +			    struct v4l2_h264_reflist_builder *builder,
>  			    struct rkvdec_h264_run *run)
>  {
>  	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
>  	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
>  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
> -	const struct v4l2_ctrl_h264_sps *sps = run->sps;
>  	struct rkvdec_h264_priv_tbl *priv_tbl = h264_ctx->priv_tbl.cpu;
> -	u32 max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
>  
>  	u32 *hw_rps = priv_tbl->rps;
>  	u32 i, j;
> @@ -772,37 +772,36 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
>  			continue;
>  
> -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> -		    dpb[i].frame_num <= dec_params->frame_num) {
> -			p[i] = dpb[i].frame_num;
> -			continue;
> -		}
> -
> -		p[i] = dpb[i].frame_num - max_frame_num;
> +		p[i] = builder->refs[i].frame_num;
>  	}
>  
>  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> -		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> -			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> -			u8 idx = 0;
> +		for (i = 0; i < builder->num_valid; i++) {
> +			struct v4l2_h264_reference *ref;
> +			u8 dpb_valid;
> +			u8 bottom;

These would be better as type bool.

regards,
dan carpenter

>  
>  			switch (j) {
>  			case 0:
> -				idx = h264_ctx->reflists.p[i].index;
> +				ref = &h264_ctx->reflists.p[i];
>  				break;
>  			case 1:
> -				idx = h264_ctx->reflists.b0[i].index;
> +				ref = &h264_ctx->reflists.b0[i];
>  				break;
>  			case 2:
> -				idx = h264_ctx->reflists.b1[i].index;
> +				ref = &h264_ctx->reflists.b1[i];
>  				break;
>  			}
>  
> -			if (idx >= ARRAY_SIZE(dec_params->dpb))
> +			if (WARN_ON(ref->index >= ARRAY_SIZE(dec_params->dpb)))
>  				continue;
>  
> +			dpb_valid = run->ref_buf_idx[ref->index] >= 0;
> +			bottom = ref->fields == V4L2_H264_BOTTOM_FIELD_REF;
> +
>  			set_ps_field(hw_rps, DPB_INFO(i, j),
> -				     idx | dpb_valid << 4);
> +				     ref->index | dpb_valid << 4);
> +			set_ps_field(hw_rps, BOTTOM_FLAG(i, j), bottom);
>  		}
>  	}
>  }

Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Nicolas Dufresne]

Le mardi 29 mars 2022 à 11:13 +0300, Dan Carpenter a écrit :
> On Mon, Mar 28, 2022 at 03:59:31PM -0400, Nicolas Dufresne wrote:
> > @@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> >  		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> >  		int buf_idx = -1;
> >  
> > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > +		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
> >  			buf_idx = vb2_find_timestamp(cap_q,
> >  						     dpb[i].reference_ts, 0);
> > +			if (buf_idx < 0)
> > +				pr_debug("No buffer for reference_ts %llu",
> > +					 dpb[i].reference_ts);
> 
> pr_debug() is too quiet.  Make it pr_err().  Set buf_idx to zero instead
> leaving it as an error code.

Thanks for the suggestion, I'm just a bit uncomfortable using pr_err() for
something that is not a driver error, but userland error. Perhaps you can
educate me on the policy in this regard, but malicous userland being able to
flood the logs very easily is my main concern here.

About the negative idx, it is being used set dpb_valid later on. H.264 error
resilience requires that these frames should be marked as "unexisting" but still
occupy space in the DPB, this is more or less what I'm trying to implement here.
Setting it to 0 would basically mean to refer to DPB index 0, which is
relatively random pick. I believe your suggestion is not taking into
consideration what the code is doing, but it would fall in some poor-man
concealment which I would rather leave to the userland.

> 
> > +		}
> >  
> >  		run->ref_buf_idx[i] = buf_idx;
> >  	}
> >  }
> >  
> >  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > +			    struct v4l2_h264_reflist_builder *builder,
> >  			    struct rkvdec_h264_run *run)
> >  {
> >  	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> >  	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
> >  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
> > -	const struct v4l2_ctrl_h264_sps *sps = run->sps;
> >  	struct rkvdec_h264_priv_tbl *priv_tbl = h264_ctx->priv_tbl.cpu;
> > -	u32 max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
> >  
> >  	u32 *hw_rps = priv_tbl->rps;
> >  	u32 i, j;
> > @@ -772,37 +772,36 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> >  			continue;
> >  
> > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> > -		    dpb[i].frame_num <= dec_params->frame_num) {
> > -			p[i] = dpb[i].frame_num;
> > -			continue;
> > -		}
> > -
> > -		p[i] = dpb[i].frame_num - max_frame_num;
> > +		p[i] = builder->refs[i].frame_num;
> >  	}
> >  
> >  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > -		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > -			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> > -			u8 idx = 0;
> > +		for (i = 0; i < builder->num_valid; i++) {
> > +			struct v4l2_h264_reference *ref;
> > +			u8 dpb_valid;
> > +			u8 bottom;
> 
> These would be better as type bool.

I never used a bool for bit operations before, but I guess that can work, thanks
for the suggestion. As this deviates from the original code, I suppose I should
make this a separate patch ?

> 
> regards,
> dan carpenter
> 
> >  
> >  			switch (j) {
> >  			case 0:
> > -				idx = h264_ctx->reflists.p[i].index;
> > +				ref = &h264_ctx->reflists.p[i];
> >  				break;
> >  			case 1:
> > -				idx = h264_ctx->reflists.b0[i].index;
> > +				ref = &h264_ctx->reflists.b0[i];
> >  				break;
> >  			case 2:
> > -				idx = h264_ctx->reflists.b1[i].index;
> > +				ref = &h264_ctx->reflists.b1[i];
> >  				break;
> >  			}
> >  
> > -			if (idx >= ARRAY_SIZE(dec_params->dpb))
> > +			if (WARN_ON(ref->index >= ARRAY_SIZE(dec_params->dpb)))
> >  				continue;
> >  
> > +			dpb_valid = run->ref_buf_idx[ref->index] >= 0;
> > +			bottom = ref->fields == V4L2_H264_BOTTOM_FIELD_REF;
> > +
> >  			set_ps_field(hw_rps, DPB_INFO(i, j),
> > -				     idx | dpb_valid << 4);
> > +				     ref->index | dpb_valid << 4);
> > +			set_ps_field(hw_rps, BOTTOM_FLAG(i, j), bottom);
> >  		}
> >  	}
> >  }
> 

Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Dan Carpenter]

On Tue, Mar 29, 2022 at 04:54:55PM -0400, Nicolas Dufresne wrote:
> Le mardi 29 mars 2022 à 11:13 +0300, Dan Carpenter a écrit :
> > On Mon, Mar 28, 2022 at 03:59:31PM -0400, Nicolas Dufresne wrote:
> > > @@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > >  		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > >  		int buf_idx = -1;
> > >  
> > > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > +		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
> > >  			buf_idx = vb2_find_timestamp(cap_q,
> > >  						     dpb[i].reference_ts, 0);
> > > +			if (buf_idx < 0)
> > > +				pr_debug("No buffer for reference_ts %llu",
> > > +					 dpb[i].reference_ts);
> > 
> > pr_debug() is too quiet.  Make it pr_err().  Set buf_idx to zero instead
> > leaving it as an error code.
> 
> Thanks for the suggestion, I'm just a bit uncomfortable using pr_err() for
> something that is not a driver error, but userland error. Perhaps you can
> educate me on the policy in this regard, but malicous userland being able to
> flood the logs very easily is my main concern here.
> 
> About the negative idx, it is being used set dpb_valid later on. H.264 error
> resilience requires that these frames should be marked as "unexisting" but still
> occupy space in the DPB, this is more or less what I'm trying to implement here.
> Setting it to 0 would basically mean to refer to DPB index 0, which is
> relatively random pick. I believe your suggestion is not taking into
> consideration what the code is doing, but it would fall in some poor-man
> concealment which I would rather leave to the userland.
> 

To be honest, I just saw that it was a negative idx and freaked out.  I
didn't look at any context...

You're right that we don't to allow the user to spam the dmesg.  Ideally
we would return an error.  A second best solution is to do a pr_err_once().

> > >  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > > -		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > > -			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> > > -			u8 idx = 0;
> > > +		for (i = 0; i < builder->num_valid; i++) {
> > > +			struct v4l2_h264_reference *ref;
> > > +			u8 dpb_valid;
> > > +			u8 bottom;
> > 
> > These would be better as type bool.
> 
> I never used a bool for bit operations before, but I guess that can work, thanks
> for the suggestion. As this deviates from the original code, I suppose I should
> make this a separate patch ?

I just saw the name and wondered why it was a u8.  bool does make more
sense and works fine for the bitwise stuff.  But I don't really care at
all.

regards,
dan carpenter

Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Nicolas Dufresne]

Le mercredi 30 mars 2022 à 08:15 +0300, Dan Carpenter a écrit :
> On Tue, Mar 29, 2022 at 04:54:55PM -0400, Nicolas Dufresne wrote:
> > Le mardi 29 mars 2022 à 11:13 +0300, Dan Carpenter a écrit :
> > > On Mon, Mar 28, 2022 at 03:59:31PM -0400, Nicolas Dufresne wrote:
> > > > @@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > > >  		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > > >  		int buf_idx = -1;
> > > >  
> > > > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > > +		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
> > > >  			buf_idx = vb2_find_timestamp(cap_q,
> > > >  						     dpb[i].reference_ts, 0);
> > > > +			if (buf_idx < 0)
> > > > +				pr_debug("No buffer for reference_ts %llu",
> > > > +					 dpb[i].reference_ts);
> > > 
> > > pr_debug() is too quiet.  Make it pr_err().  Set buf_idx to zero instead
> > > leaving it as an error code.
> > 
> > Thanks for the suggestion, I'm just a bit uncomfortable using pr_err() for
> > something that is not a driver error, but userland error. Perhaps you can
> > educate me on the policy in this regard, but malicous userland being able to
> > flood the logs very easily is my main concern here.
> > 
> > About the negative idx, it is being used set dpb_valid later on. H.264 error
> > resilience requires that these frames should be marked as "unexisting" but still
> > occupy space in the DPB, this is more or less what I'm trying to implement here.
> > Setting it to 0 would basically mean to refer to DPB index 0, which is
> > relatively random pick. I believe your suggestion is not taking into
> > consideration what the code is doing, but it would fall in some poor-man
> > concealment which I would rather leave to the userland.
> > 
> 
> To be honest, I just saw that it was a negative idx and freaked out.  I
> didn't look at any context...
> 
> You're right that we don't to allow the user to spam the dmesg.  Ideally
> we would return an error.  A second best solution is to do a pr_err_once().

There is two schools in the context of video stream decoding. I'm not saying
this driver is quite there in term of visual corruption reporting, this is
something I expect we'll improve in the long term. But here's the two schools:

- Freeze on the last non-corrupted image (Apple style)
- Display slightly distorted image with movement (the rest of the world)

In order to give users that choice, I must try decoding as much as I can
regardless if there is a missing a reference or not. That wasn't the goal in
this MR, but in the long run we'll remember this and mark the buffer as
corrupted (using the ERROR but setting a payload size to the picture size). This
leaves the users the option to drop or to keep the visually corrupted image. In
video streaming, corrupted stream could look relatively fine, this cannot be
judge noticing 1 reference frame missing (it could be referenced in only 1
macro-block).

Educational bit behind, I think we should keep going and not "error out". Also,
for debugging purpose, it is nicer if we can get a complete report of non-memory
backed references. As a missing reference in 1 frame, may have implication in
later frame, or may cause other frames to be missed.

One thing I was thinking though, is that through using raw pr_debug() I'm not
giving much context to my trace, so it would be hard to associate it with the
driver instance (m2m are multi-instance) it was running against. But I didn't
want to add a new tracing wrapper for that driver in this patchset as it was
already relatively large patchset. Though, these traces have been previous to
make the driver work (as long as you test with a single instance).

If its all right with everyone, I'd leave it like this for this round, we can
dedicated a patchset on improve this driver tracing in the future.

> 
> > > >  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > > > -		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > > > -			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> > > > -			u8 idx = 0;
> > > > +		for (i = 0; i < builder->num_valid; i++) {
> > > > +			struct v4l2_h264_reference *ref;
> > > > +			u8 dpb_valid;
> > > > +			u8 bottom;
> > > 
> > > These would be better as type bool.
> > 
> > I never used a bool for bit operations before, but I guess that can work, thanks
> > for the suggestion. As this deviates from the original code, I suppose I should
> > make this a separate patch ?
> 
> I just saw the name and wondered why it was a u8.  bool does make more
> sense and works fine for the bitwise stuff.  But I don't really care at
> all.

I'll do that in v2, in same patch, looks minor enough. I think if using bool
could guaranty that only 1 or 0 is  possible, it would be even better, but don't
think C works like this.

Thanks again for your comments.

> 
> regards,
> dan carpenter
> 

Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Dan Carpenter]

Yeah.  I'm aboslutely fine with whatever you do.  Some of the questions
you're asking occurred to me too but I don't have the answers.

> > > > > +		for (i = 0; i < builder->num_valid; i++) {
> > > > > +			struct v4l2_h264_reference *ref;
> > > > > +			u8 dpb_valid;
> > > > > +			u8 bottom;
> > > > 
> > > > These would be better as type bool.
> > > 
> > > I never used a bool for bit operations before, but I guess that can work, thanks
> > > for the suggestion. As this deviates from the original code, I suppose I should
> > > make this a separate patch ?
> > 
> > I just saw the name and wondered why it was a u8.  bool does make more
> > sense and works fine for the bitwise stuff.  But I don't really care at
> > all.
> 
> I'll do that in v2, in same patch, looks minor enough. I think if using bool
> could guaranty that only 1 or 0 is  possible, it would be even better, but don't
> think C works like this.

I'm not sure I understand.  If you assign "bool x = <any non-zero>;"
then x is set to true.  Do you want a static checker warning for if
<any non-zero> can be something other than one or zero?  The problem is
that people sometimes deliberately do stuff like "bool x = var & 0xf0;".
Smatch will complain if you assign a negative value to x.

test.c:8 test() warn: assigning (-3) to unsigned variable 'x'

It's supposed to print a warning if you used it to save error codes like:

	x = some_kernel_function();

But it does not.  :/  Something to investigate.

regards,
dan carpenter

Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Nicolas Dufresne]

Le mercredi 30 mars 2022 à 18:16 +0300, Dan Carpenter a écrit :
> Yeah.  I'm aboslutely fine with whatever you do.  Some of the questions
> you're asking occurred to me too but I don't have the answers.
> 
> > > > > > +		for (i = 0; i < builder->num_valid; i++) {
> > > > > > +			struct v4l2_h264_reference *ref;
> > > > > > +			u8 dpb_valid;
> > > > > > +			u8 bottom;
> > > > > 
> > > > > These would be better as type bool.
> > > > 
> > > > I never used a bool for bit operations before, but I guess that can work, thanks
> > > > for the suggestion. As this deviates from the original code, I suppose I should
> > > > make this a separate patch ?
> > > 
> > > I just saw the name and wondered why it was a u8.  bool does make more
> > > sense and works fine for the bitwise stuff.  But I don't really care at
> > > all.
> > 
> > I'll do that in v2, in same patch, looks minor enough. I think if using bool
> > could guaranty that only 1 or 0 is  possible, it would be even better, but don't
> > think C works like this.
> 
> I'm not sure I understand.  If you assign "bool x = <any non-zero>;"
> then x is set to true.  Do you want a static checker warning for if
> <any non-zero> can be something other than one or zero?  The problem is
> that people sometimes deliberately do stuff like "bool x = var & 0xf0;".
> Smatch will complain if you assign a negative value to x.
> 
> test.c:8 test() warn: assigning (-3) to unsigned variable 'x'
> 
> It's supposed to print a warning if you used it to save error codes like:
> 
> 	x = some_kernel_function();
> 
> But it does not.  :/  Something to investigate.

That would be an amazing catch, you might have seen a lot of:

  x = !!(var & 0xf0)

For branches, it does no matter, but if you use x it like this dpb_valid
variable is used, not having 0 or 1 can lead to very surprising results. In the
end its used like this

  set_reg(reg0, val | (x << N))

So using bool type can hint the analyzer that 0 or 1 was likely expected, while
currently an u8 would be ambiguous and lead to false positive if we were to
warn.

> 
> regards,
> dan carpenter
> 

Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>This makes use of the new feature in the reference builder to program
>up to 32 references when doing field decoding. It also signals the
>parity (top of bottom) of the field to the hardware.

s/top of bottom/top or bottom/

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 48 ++++++++++------------
> 1 file changed, 21 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index ec52b195bbd7..891c48bf6a51 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -97,13 +97,10 @@ struct rkvdec_h264_priv_tbl {
> 	u8 err_info[RKV_ERROR_INFO_SIZE];
> };
>
>-#define RKVDEC_H264_DPB_SIZE 16
>-
> struct rkvdec_h264_reflists {
> 	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
> 	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
> 	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
>-	u8 num_valid;
> };
>
> struct rkvdec_h264_run {
>@@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> 		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> 		int buf_idx = -1;
>
>-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
>+		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
> 			buf_idx = vb2_find_timestamp(cap_q,
> 						     dpb[i].reference_ts, 0);
>+			if (buf_idx < 0)
>+				pr_debug("No buffer for reference_ts %llu",
>+					 dpb[i].reference_ts);
>+		}
>
> 		run->ref_buf_idx[i] = buf_idx;
> 	}
> }
>
> static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>+			    struct v4l2_h264_reflist_builder *builder,
> 			    struct rkvdec_h264_run *run)
> {
> 	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> 	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
> 	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
>-	const struct v4l2_ctrl_h264_sps *sps = run->sps;
> 	struct rkvdec_h264_priv_tbl *priv_tbl = h264_ctx->priv_tbl.cpu;
>-	u32 max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
>
> 	u32 *hw_rps = priv_tbl->rps;
> 	u32 i, j;
>@@ -772,37 +772,36 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> 		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> 			continue;
>
>-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
>-		    dpb[i].frame_num <= dec_params->frame_num) {
>-			p[i] = dpb[i].frame_num;
>-			continue;
>-		}
>-
>-		p[i] = dpb[i].frame_num - max_frame_num;
>+		p[i] = builder->refs[i].frame_num;
> 	}
>
> 	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
>-		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
>-			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
>-			u8 idx = 0;
>+		for (i = 0; i < builder->num_valid; i++) {
>+			struct v4l2_h264_reference *ref;
>+			u8 dpb_valid;
>+			u8 bottom;
>
> 			switch (j) {
> 			case 0:
>-				idx = h264_ctx->reflists.p[i].index;
>+				ref = &h264_ctx->reflists.p[i];
> 				break;
> 			case 1:
>-				idx = h264_ctx->reflists.b0[i].index;
>+				ref = &h264_ctx->reflists.b0[i];
> 				break;
> 			case 2:
>-				idx = h264_ctx->reflists.b1[i].index;
>+				ref = &h264_ctx->reflists.b1[i];
> 				break;
> 			}
>
>-			if (idx >= ARRAY_SIZE(dec_params->dpb))
>+			if (WARN_ON(ref->index >= ARRAY_SIZE(dec_params->dpb)))
> 				continue;
>
>+			dpb_valid = run->ref_buf_idx[ref->index] >= 0;
>+			bottom = ref->fields == V4L2_H264_BOTTOM_FIELD_REF;
>+
> 			set_ps_field(hw_rps, DPB_INFO(i, j),
>-				     idx | dpb_valid << 4);
>+				     ref->index | dpb_valid << 4);
>+			set_ps_field(hw_rps, BOTTOM_FLAG(i, j), bottom);
> 		}
> 	}
> }
>@@ -990,10 +989,6 @@ static void config_registers(struct rkvdec_ctx *ctx,
> 				       rkvdec->regs + RKVDEC_REG_H264_BASE_REFER15);
> 	}
>
>-	/*
>-	 * Since support frame mode only
>-	 * top_field_order_cnt is the same as bottom_field_order_cnt
>-	 */
> 	reg = RKVDEC_CUR_POC(dec_params->top_field_order_cnt);
> 	writel_relaxed(reg, rkvdec->regs + RKVDEC_REG_CUR_POC0);
>
>@@ -1109,7 +1104,6 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> 	/* Build the P/B{0,1} ref lists. */
> 	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
> 				       run.sps, run.decode_params->dpb);
>-	h264_ctx->reflists.num_valid = reflist_builder.num_valid;
> 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
> 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> 				    h264_ctx->reflists.b1);
>@@ -1117,7 +1111,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> 	assemble_hw_scaling_list(ctx, &run);
> 	assemble_hw_pps(ctx, &run);
> 	lookup_ref_buf_idx(ctx, &run);
>-	assemble_hw_rps(ctx, &run);
>+	assemble_hw_rps(ctx, &reflist_builder, &run);
> 	config_registers(ctx, &run);
>
> 	rkvdec_run_postamble(ctx, &run.base);
>-- 
>2.34.1
>

[PATCH v1 20/24] media: hantro: Enable HOLD_CAPTURE_BUF for H.264

[Nicolas Dufresne]

This is needed to optimizing field decoding. Each field will be
decoded in the same capture buffer, so to make use of the queues
we need to be able to ask the driver to keep the capture buffer.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/hantro/hantro_v4l2.c | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 67148ba346f5..50d636678ff3 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -409,6 +409,30 @@ hantro_update_requires_request(struct hantro_ctx *ctx, u32 fourcc)
 	}
 }
 
+static void
+hantro_update_requires_hold_capture_buf(struct hantro_ctx *ctx, u32 fourcc)
+{
+	struct vb2_queue *vq;
+
+	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
+
+	switch (fourcc) {
+	case V4L2_PIX_FMT_JPEG:
+	case V4L2_PIX_FMT_MPEG2_SLICE:
+	case V4L2_PIX_FMT_VP8_FRAME:
+	case V4L2_PIX_FMT_HEVC_SLICE:
+	case V4L2_PIX_FMT_VP9_FRAME:
+		vq->subsystem_flags &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
+		break;
+	case V4L2_PIX_FMT_H264_SLICE:
+		vq->subsystem_flags |= VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
+		break;
+	default:
+		break;
+	}
+}
+
 static int hantro_set_fmt_out(struct hantro_ctx *ctx,
 			      struct v4l2_pix_format_mplane *pix_mp)
 {
@@ -472,6 +496,7 @@ static int hantro_set_fmt_out(struct hantro_ctx *ctx,
 	ctx->dst_fmt.quantization = pix_mp->quantization;
 
 	hantro_update_requires_request(ctx, pix_mp->pixelformat);
+	hantro_update_requires_hold_capture_buf(ctx, pix_mp->pixelformat);
 
 	vpu_debug(0, "OUTPUT codec mode: %d\n", ctx->vpu_src_fmt->codec_mode);
 	vpu_debug(0, "fmt - w: %d, h: %d\n",
-- 
2.34.1

Re: [PATCH v1 20/24] media: hantro: Enable HOLD_CAPTURE_BUF for H.264

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>This is needed to optimizing field decoding. Each field will be

s/is needed to optimizing/is needed to optimize/

>decoded in the same capture buffer, so to make use of the queues

s/in the same/into the same/

>we need to be able to ask the driver to keep the capture buffer.

How about:
"""
During field decoding each field will be decoded into the same capture
buffer. Optimise this mode by asking the driver to hold the buffer until
all fields are written into it.
"""

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

>---
> drivers/staging/media/hantro/hantro_v4l2.c | 25 ++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
>diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
>index 67148ba346f5..50d636678ff3 100644
>--- a/drivers/staging/media/hantro/hantro_v4l2.c
>+++ b/drivers/staging/media/hantro/hantro_v4l2.c
>@@ -409,6 +409,30 @@ hantro_update_requires_request(struct hantro_ctx *ctx, u32 fourcc)
> 	}
> }
>
>+static void
>+hantro_update_requires_hold_capture_buf(struct hantro_ctx *ctx, u32 fourcc)
>+{
>+	struct vb2_queue *vq;
>+
>+	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>+			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
>+
>+	switch (fourcc) {
>+	case V4L2_PIX_FMT_JPEG:
>+	case V4L2_PIX_FMT_MPEG2_SLICE:
>+	case V4L2_PIX_FMT_VP8_FRAME:
>+	case V4L2_PIX_FMT_HEVC_SLICE:
>+	case V4L2_PIX_FMT_VP9_FRAME:
>+		vq->subsystem_flags &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
>+		break;

Out of curiosity, why would it be bad for the other codecs to have
support for that feature activated? As this doesn't actually hold the
buffers but only makes sure that they could be held.

>+	case V4L2_PIX_FMT_H264_SLICE:
>+		vq->subsystem_flags |= VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;

I think it is worth it to highlight with a comment why only this one
receives support for holding the buffer. As it is quite confusing
without background info and just the code.

How about:
```
/*
  * During field decoding in H264, all fields are written into the
  * same capture buffer, thus we need to be able to hold the buffer
  * until all fields are written to it
  */
```

>+		break;
>+	default:

The only other decoding formats remaining are:
- V4L2_PIX_FMT_NV12_4L4
- V4L2_PIX_FMT_NV12

Both have codec mode HANTRO_MODE_NONE.

My thought is:
If we don't care for these two, the we might as well disable buffer holding
support for them as well. So, we could make this simplier
(but a bit less descriptive):

```
/*
  * During field decoding in H264, all fields are written into the
  * same capture buffer, thus we need to be able to hold the buffer
  * until all fields are written to it
  */
if (fourcc == V4L2_PIX_FMT_H264_SLICE)
     vq->subsystem_flags |= VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
else 
		vq->subsystem_flags &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
```

Greetings,
Sebastian

>+		break;
>+	}
>+}
>+
> static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> 			      struct v4l2_pix_format_mplane *pix_mp)
> {
>@@ -472,6 +496,7 @@ static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> 	ctx->dst_fmt.quantization = pix_mp->quantization;
>
> 	hantro_update_requires_request(ctx, pix_mp->pixelformat);
>+	hantro_update_requires_hold_capture_buf(ctx, pix_mp->pixelformat);
>
> 	vpu_debug(0, "OUTPUT codec mode: %d\n", ctx->vpu_src_fmt->codec_mode);
> 	vpu_debug(0, "fmt - w: %d, h: %d\n",
>-- 
>2.34.1
>

Re: [PATCH v1 20/24] media: hantro: Enable HOLD_CAPTURE_BUF for H.264

[Nicolas Dufresne]

Le mercredi 30 mars 2022 à 09:36 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > This is needed to optimizing field decoding. Each field will be
> 
> s/is needed to optimizing/is needed to optimize/
> 
> > decoded in the same capture buffer, so to make use of the queues
> 
> s/in the same/into the same/
> 
> > we need to be able to ask the driver to keep the capture buffer.
> 
> How about:
> """
> During field decoding each field will be decoded into the same capture
> buffer. Optimise this mode by asking the driver to hold the buffer until
> all fields are written into it.
> """
> 
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Perhaps avoid giving a reviewed by if you are to comment around modifying the
code ? I will though keep the code as is, I believe there is more good then bad
around the form.

> 
> > ---
> > drivers/staging/media/hantro/hantro_v4l2.c | 25 ++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> > index 67148ba346f5..50d636678ff3 100644
> > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > @@ -409,6 +409,30 @@ hantro_update_requires_request(struct hantro_ctx *ctx, u32 fourcc)
> > 	}
> > }
> > 
> > +static void
> > +hantro_update_requires_hold_capture_buf(struct hantro_ctx *ctx, u32 fourcc)
> > +{
> > +	struct vb2_queue *vq;
> > +
> > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > +			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> > +
> > +	switch (fourcc) {
> > +	case V4L2_PIX_FMT_JPEG:
> > +	case V4L2_PIX_FMT_MPEG2_SLICE:
> > +	case V4L2_PIX_FMT_VP8_FRAME:
> > +	case V4L2_PIX_FMT_HEVC_SLICE:
> > +	case V4L2_PIX_FMT_VP9_FRAME:
> > +		vq->subsystem_flags &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
> > +		break;
> 
> Out of curiosity, why would it be bad for the other codecs to have
> support for that feature activated? As this doesn't actually hold the
> buffers but only makes sure that they could be held.
> 
> > +	case V4L2_PIX_FMT_H264_SLICE:
> > +		vq->subsystem_flags |= VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> 
> I think it is worth it to highlight with a comment why only this one
> receives support for holding the buffer. As it is quite confusing
> without background info and just the code.

As this code is quite separate from the actual codec code, I believe it will be
more robust this way. It could happen in the future that code get modified
without taking into account that a buffer may be held. This also mimic how this
was implemented in Cedrus fwiw.

Note that it needs to be added for MPEG2 field decoding too, but I believe this
is unrelated to this patchset, the form is nice for adding more in the future.

> 
> How about:
> ```
> /*
>   * During field decoding in H264, all fields are written into the
>   * same capture buffer, thus we need to be able to hold the buffer
>   * until all fields are written to it
>   */
> ```
> 
> > +		break;
> > +	default:
> 
> The only other decoding formats remaining are:
> - V4L2_PIX_FMT_NV12_4L4
> - V4L2_PIX_FMT_NV12

You'll never get raw formats in that switch. The cases are exhaustive for the
context, yet the compiler does not understand that context.

> 
> Both have codec mode HANTRO_MODE_NONE.
> 
> My thought is:
> If we don't care for these two, the we might as well disable buffer holding
> support for them as well. So, we could make this simplier
> (but a bit less descriptive):
> 
> ```
> /*
>   * During field decoding in H264, all fields are written into the
>   * same capture buffer, thus we need to be able to hold the buffer
>   * until all fields are written to it
>   */
> if (fourcc == V4L2_PIX_FMT_H264_SLICE)
>      vq->subsystem_flags |= VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> else 
> 		vq->subsystem_flags &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
> ```
> 
> Greetings,
> Sebastian
> 
> > +		break;
> > +	}
> > +}
> > +
> > static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> > 			      struct v4l2_pix_format_mplane *pix_mp)
> > {
> > @@ -472,6 +496,7 @@ static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> > 	ctx->dst_fmt.quantization = pix_mp->quantization;
> > 
> > 	hantro_update_requires_request(ctx, pix_mp->pixelformat);
> > +	hantro_update_requires_hold_capture_buf(ctx, pix_mp->pixelformat);
> > 
> > 	vpu_debug(0, "OUTPUT codec mode: %d\n", ctx->vpu_src_fmt->codec_mode);
> > 	vpu_debug(0, "fmt - w: %d, h: %d\n",
> > -- 
> > 2.34.1
> > 

[PATCH v1 21/24] media: hantro: Stop using H.264 parameter pic_num

[Nicolas Dufresne]

The hardware expects FrameNumWrap or long_term_frame_idx. Picture
numbers are per field, and are mostly used during the memory
management process, which is done in userland. This fixes two
ITU conformance tests:

  - MR6_BT_B
  - MR8_BT_B

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/hantro/hantro_h264.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 0b4d2491be3b..228629fb3cdf 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -354,8 +354,6 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
 
 	if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
 		return 0;
-	if (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
-		return dpb->pic_num;
 	return dpb->frame_num;
 }
 
-- 
2.34.1

Re: [PATCH v1 21/24] media: hantro: Stop using H.264 parameter pic_num

[Sebastian Fricke]

Hey Nicolas,

The term pic_num is now only present in the following files:
```
❯ rg 'pic_num'
staging/media/rkvdec/rkvdec-h264.c
766:	 * Assign an invalid pic_num if DPB entry at that position is inactive.
768:	 * reference picture with pic_num 0, triggering output picture

media/platform/amphion/vpu_windsor.c
485:	u32 pic_num;

media/platform/mediatek/vcodec/vdec/vdec_h264_req_if.c
97:	unsigned short pic_num;
346:		dst_entry->pic_num = src_entry->pic_num;

media/v4l2-core/v4l2-h264.c
143:	 * but with frame_num (wrapped). As for frame the pic_num and frame_num
306:		/* this is pic_num for frame and frame_num (wrapped) for field,
307:		 * but for frame pic_num is equal to frame_num (wrapped).
```

In v4l2-h264 and rkvdec-h264 it is only present as comment and the term
is not part of the specification.
In vpu_windsor it is actually never used.
And for the mediatek driver the same might apply.
It might be worth it to get rid of that term all together while you are
at it.

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>The hardware expects FrameNumWrap or long_term_frame_idx. Picture
>numbers are per field, and are mostly used during the memory
>management process, which is done in userland. This fixes two
>ITU conformance tests:
>
>  - MR6_BT_B
>  - MR8_BT_B
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian
>---
> drivers/staging/media/hantro/hantro_h264.c | 2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
>index 0b4d2491be3b..228629fb3cdf 100644
>--- a/drivers/staging/media/hantro/hantro_h264.c
>+++ b/drivers/staging/media/hantro/hantro_h264.c
>@@ -354,8 +354,6 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
>
> 	if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> 		return 0;
>-	if (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
>-		return dpb->pic_num;
> 	return dpb->frame_num;
> }
>
>-- 
>2.34.1
>

Re: [PATCH v1 21/24] media: hantro: Stop using H.264 parameter pic_num

[Nicolas Dufresne]

Le mercredi 30 mars 2022 à 09:42 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> The term pic_num is now only present in the following files:
> ```
> ❯ rg 'pic_num'
> staging/media/rkvdec/rkvdec-h264.c
> 766:	 * Assign an invalid pic_num if DPB entry at that position is inactive.
> 768:	 * reference picture with pic_num 0, triggering output picture

I should probably translate this one, since the HW uses frame_num, not pic_num.

> 
> media/platform/amphion/vpu_windsor.c
> 485:	u32 pic_num;

Amphion Windsor is a stateful driver, I cannot comment on the user of pic_num
for that type of driver.

> 
> media/platform/mediatek/vcodec/vdec/vdec_h264_req_if.c
> 97:	unsigned short pic_num;
> 346:		dst_entry->pic_num = src_entry->pic_num;

This is being sent to the firmware, so its a difficult change to make without
testing it first. I do have HW to test this, but would prefer doing so in a
seperate patchset. Note that MTK does not support field decoding, so pic_num ==
frame_num. So whatever it does here is likely correct.

> 
> media/v4l2-core/v4l2-h264.c
> 143:	 * but with frame_num (wrapped). As for frame the pic_num and frame_num
> 306:		/* this is pic_num for frame and frame_num (wrapped) for field,
> 307:		 * but for frame pic_num is equal to frame_num (wrapped).
> ```
> 
> In v4l2-h264 and rkvdec-h264 it is only present as comment and the term
> is not part of the specification.
> In vpu_windsor it is actually never used.
> And for the mediatek driver the same might apply.
> It might be worth it to get rid of that term all together while you are
> at it.

Amphion Windsor is a stateful driver, I'd leave it to the maintainer to cleanup
unused variables if there is. In general the term is not invalid, its just that
the value can be trivially deduced from frame_num and the value depends on the
current picture parity, which makes it an unstable identifier.

> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > The hardware expects FrameNumWrap or long_term_frame_idx. Picture
> > numbers are per field, and are mostly used during the memory
> > management process, which is done in userland. This fixes two
> > ITU conformance tests:
> > 
> >  - MR6_BT_B
> >  - MR8_BT_B
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> 
> Greetings,
> Sebastian
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 2 --
> > 1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 0b4d2491be3b..228629fb3cdf 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -354,8 +354,6 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > 
> > 	if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > 		return 0;
> > -	if (dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> > -		return dpb->pic_num;
> > 	return dpb->frame_num;
> > }
> > 
> > -- 
> > 2.34.1
> > 

[PATCH v1 22/24] media: hantro: h264: Make dpb entry management more robust

[Nicolas Dufresne]

From: Jonas Karlman <jonas@kwiboo.se>

The driver maintains stable slot location for reference pictures. This
change makes the code more robust by using the reference_ts as key and
by marking all entries invalid right from the start.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 228629fb3cdf..7377fc26f780 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
 static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
 			    const struct v4l2_h264_dpb_entry *b)
 {
-	return a->top_field_order_cnt == b->top_field_order_cnt &&
-	       a->bottom_field_order_cnt == b->bottom_field_order_cnt;
+	return a->reference_ts == b->reference_ts;
 }
 
 static void update_dpb(struct hantro_ctx *ctx)
@@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
 
 	/* Disable all entries by default. */
 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
-		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
+		ctx->h264_dec.dpb[i].flags = 0;
 
 	/* Try to match new DPB entries with existing ones by their POCs. */
 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
 		const struct v4l2_h264_dpb_entry *ndpb = &dec_param->dpb[i];
 
-		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
+		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
 			continue;
 
 		/*
@@ -290,8 +289,7 @@ static void update_dpb(struct hantro_ctx *ctx)
 			struct v4l2_h264_dpb_entry *cdpb;
 
 			cdpb = &ctx->h264_dec.dpb[j];
-			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
-			    !dpb_entry_match(cdpb, ndpb))
+			if (!dpb_entry_match(cdpb, ndpb))
 				continue;
 
 			*cdpb = *ndpb;
-- 
2.34.1

Re: [PATCH v1 22/24] media: hantro: h264: Make dpb entry management more robust

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Jonas Karlman <jonas@kwiboo.se>
>
>The driver maintains stable slot location for reference pictures. This

s/slot location/slot locations/

>change makes the code more robust by using the reference_ts as key and
>by marking all entries invalid right from the start.
>
>Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>---
> drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
>index 228629fb3cdf..7377fc26f780 100644
>--- a/drivers/staging/media/hantro/hantro_h264.c
>+++ b/drivers/staging/media/hantro/hantro_h264.c
>@@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> 			    const struct v4l2_h264_dpb_entry *b)
> {
>-	return a->top_field_order_cnt == b->top_field_order_cnt &&
>-	       a->bottom_field_order_cnt == b->bottom_field_order_cnt;
>+	return a->reference_ts == b->reference_ts;
> }
>
> static void update_dpb(struct hantro_ctx *ctx)
>@@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
>
> 	/* Disable all entries by default. */
> 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
>-		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
>+		ctx->h264_dec.dpb[i].flags = 0;

Ehm ... we just remove all flags? Can't this have any unwanted side
effects like removing a flag that we actually wanted to keep?
(Like long term or the field flags?)
If we just want to set the DPB entry inactive, then removing the ACTIVE
flag seems like the correct approach to me.
If we want to get rid of the VALID flag as well, then we could just do:
		ctx->h264_dec.dpb[i].flags &=
       ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);

In case we really want to reset all flags, I'd say adjust the comment
above it:
```
- 	/* Disable all entries by default. */
+ 	/* Reset the flags for all entries by default. */
```

Greetings,
Sebastian

>
> 	/* Try to match new DPB entries with existing ones by their POCs. */
> 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> 		const struct v4l2_h264_dpb_entry *ndpb = &dec_param->dpb[i];
>
>-		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
>+		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> 			continue;
>
> 		/*
>@@ -290,8 +289,7 @@ static void update_dpb(struct hantro_ctx *ctx)
> 			struct v4l2_h264_dpb_entry *cdpb;
>
> 			cdpb = &ctx->h264_dec.dpb[j];
>-			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
>-			    !dpb_entry_match(cdpb, ndpb))
>+			if (!dpb_entry_match(cdpb, ndpb))
> 				continue;
>
> 			*cdpb = *ndpb;
>-- 
>2.34.1
>

Re: [PATCH v1 22/24] media: hantro: h264: Make dpb entry management more robust

[Nicolas Dufresne]

Le mercredi 30 mars 2022 à 09:59 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > From: Jonas Karlman <jonas@kwiboo.se>
> > 
> > The driver maintains stable slot location for reference pictures. This
> 
> s/slot location/slot locations/
> 
> > change makes the code more robust by using the reference_ts as key and
> > by marking all entries invalid right from the start.
> > 
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> > 1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 228629fb3cdf..7377fc26f780 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> > 			    const struct v4l2_h264_dpb_entry *b)
> > {
> > -	return a->top_field_order_cnt == b->top_field_order_cnt &&
> > -	       a->bottom_field_order_cnt == b->bottom_field_order_cnt;
> > +	return a->reference_ts == b->reference_ts;
> > }
> > 
> > static void update_dpb(struct hantro_ctx *ctx)
> > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
> > 
> > 	/* Disable all entries by default. */
> > 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
> > -		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
> > +		ctx->h264_dec.dpb[i].flags = 0;
> 
> Ehm ... we just remove all flags? Can't this have any unwanted side
> effects like removing a flag that we actually wanted to keep?
> (Like long term or the field flags?)

This is a local copy of the dpb, the DPB is fully passed for every decode. So
these flags will be fully restored lower when we copy the found entry. In fact,
holding a state here would not represent well the userland intention and can
have negative side effect on the decoding. Flags are not immutable between
decode and can change. This simplify the code, and make things less error prone.
This part of the code is already a bit complex, no need for an extra layer.

> If we just want to set the DPB entry inactive, then removing the ACTIVE
> flag seems like the correct approach to me.
> If we want to get rid of the VALID flag as well, then we could just do:
> 		ctx->h264_dec.dpb[i].flags &=
>        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);
> 
> In case we really want to reset all flags, I'd say adjust the comment
> above it:
> ```
> - 	/* Disable all entries by default. */
> + 	/* Reset the flags for all entries by default. */
> ```

This reads the same to me, but I can do that yes. understand that VALID means
the reference exist and the TS should point to some existing past reference
(unless there was some decode error, which the userland may not be aware yet as
this is asynchronous). While ACTIVE means that it is used as a reference. FFMPEG
is known not to filter inactive references. ACTIVE is just a flag without bunch
of other flags that can change for every decode. So none of this make sense
between 2 decodes.

> 
> Greetings,
> Sebastian
> 
> > 
> > 	/* Try to match new DPB entries with existing ones by their POCs. */
> > 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> > 		const struct v4l2_h264_dpb_entry *ndpb = &dec_param->dpb[i];
> > 
> > -		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > +		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > 			continue;
> > 
> > 		/*
> > @@ -290,8 +289,7 @@ static void update_dpb(struct hantro_ctx *ctx)
> > 			struct v4l2_h264_dpb_entry *cdpb;
> > 
> > 			cdpb = &ctx->h264_dec.dpb[j];
> > -			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
> > -			    !dpb_entry_match(cdpb, ndpb))
> > +			if (!dpb_entry_match(cdpb, ndpb))
> > 				continue;
> > 
> > 			*cdpb = *ndpb;
> > -- 
> > 2.34.1
> > 

Re: [PATCH v1 22/24] media: hantro: h264: Make dpb entry management more robust

[Ezequiel Garcia]

On Wed, Mar 30, 2022 at 12:16 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Le mercredi 30 mars 2022 à 09:59 +0200, Sebastian Fricke a écrit :
> > Hey Nicolas,
> >
> > On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > > From: Jonas Karlman <jonas@kwiboo.se>
> > >
> > > The driver maintains stable slot location for reference pictures. This
> >
> > s/slot location/slot locations/
> >
> > > change makes the code more robust by using the reference_ts as key and
> > > by marking all entries invalid right from the start.
> > >
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > ---
> > > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> > > 1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > > index 228629fb3cdf..7377fc26f780 100644
> > > --- a/drivers/staging/media/hantro/hantro_h264.c
> > > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> > >                         const struct v4l2_h264_dpb_entry *b)
> > > {
> > > -   return a->top_field_order_cnt == b->top_field_order_cnt &&
> > > -          a->bottom_field_order_cnt == b->bottom_field_order_cnt;
> > > +   return a->reference_ts == b->reference_ts;
> > > }
> > >
> > > static void update_dpb(struct hantro_ctx *ctx)
> > > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
> > >
> > >     /* Disable all entries by default. */
> > >     for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
> > > -           ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
> > > +           ctx->h264_dec.dpb[i].flags = 0;
> >
> > Ehm ... we just remove all flags? Can't this have any unwanted side
> > effects like removing a flag that we actually wanted to keep?
> > (Like long term or the field flags?)
>
> This is a local copy of the dpb, the DPB is fully passed for every decode. So
> these flags will be fully restored lower when we copy the found entry. In fact,
> holding a state here would not represent well the userland intention and can
> have negative side effect on the decoding. Flags are not immutable between
> decode and can change. This simplify the code, and make things less error prone.
> This part of the code is already a bit complex, no need for an extra layer.
>
> > If we just want to set the DPB entry inactive, then removing the ACTIVE
> > flag seems like the correct approach to me.
> > If we want to get rid of the VALID flag as well, then we could just do:
> >               ctx->h264_dec.dpb[i].flags &=
> >        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);
> >
> > In case we really want to reset all flags, I'd say adjust the comment
> > above it:
> > ```
> > -     /* Disable all entries by default. */
> > +     /* Reset the flags for all entries by default. */
> > ```
>
> This reads the same to me, but I can do that yes. understand that VALID means
> the reference exist and the TS should point to some existing past reference
> (unless there was some decode error, which the userland may not be aware yet as
> this is asynchronous). While ACTIVE means that it is used as a reference. FFMPEG
> is known not to filter inactive references. ACTIVE is just a flag without bunch
> of other flags that can change for every decode. So none of this make sense
> between 2 decodes.
>

Please leave the comment as-is, 'Disable all entries' is readable.

As much as I'd love to have very clear comments everywhere,
we have to accept that only people with insight on codec details
are expected to make sense of driver details :-)

Thanks,
Ezequiel

Re: [PATCH v1 22/24] media: hantro: h264: Make dpb entry management more robust

[Sebastian Fricke]

Hey Nicolas & Ezequiel,

On 30.03.2022 20:43, Ezequiel Garcia wrote:
>On Wed, Mar 30, 2022 at 12:16 PM Nicolas Dufresne
><nicolas.dufresne@collabora.com> wrote:
>>
>> Le mercredi 30 mars 2022 à 09:59 +0200, Sebastian Fricke a écrit :
>> > Hey Nicolas,
>> >
>> > On 28.03.2022 15:59, Nicolas Dufresne wrote:
>> > > From: Jonas Karlman <jonas@kwiboo.se>
>> > >
>> > > The driver maintains stable slot location for reference pictures. This
>> >
>> > s/slot location/slot locations/
>> >
>> > > change makes the code more robust by using the reference_ts as key and
>> > > by marking all entries invalid right from the start.
>> > >
>> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>> > > ---
>> > > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
>> > > 1 file changed, 4 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
>> > > index 228629fb3cdf..7377fc26f780 100644
>> > > --- a/drivers/staging/media/hantro/hantro_h264.c
>> > > +++ b/drivers/staging/media/hantro/hantro_h264.c
>> > > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
>> > > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
>> > >                         const struct v4l2_h264_dpb_entry *b)
>> > > {
>> > > -   return a->top_field_order_cnt == b->top_field_order_cnt &&
>> > > -          a->bottom_field_order_cnt == b->bottom_field_order_cnt;
>> > > +   return a->reference_ts == b->reference_ts;
>> > > }
>> > >
>> > > static void update_dpb(struct hantro_ctx *ctx)
>> > > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
>> > >
>> > >     /* Disable all entries by default. */
>> > >     for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
>> > > -           ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
>> > > +           ctx->h264_dec.dpb[i].flags = 0;
>> >
>> > Ehm ... we just remove all flags? Can't this have any unwanted side
>> > effects like removing a flag that we actually wanted to keep?
>> > (Like long term or the field flags?)
>>
>> This is a local copy of the dpb, the DPB is fully passed for every decode. So
>> these flags will be fully restored lower when we copy the found entry. In fact,
>> holding a state here would not represent well the userland intention and can
>> have negative side effect on the decoding. Flags are not immutable between
>> decode and can change. This simplify the code, and make things less error prone.
>> This part of the code is already a bit complex, no need for an extra layer.
>>
>> > If we just want to set the DPB entry inactive, then removing the ACTIVE
>> > flag seems like the correct approach to me.
>> > If we want to get rid of the VALID flag as well, then we could just do:
>> >               ctx->h264_dec.dpb[i].flags &=
>> >        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);
>> >
>> > In case we really want to reset all flags, I'd say adjust the comment
>> > above it:
>> > ```
>> > -     /* Disable all entries by default. */
>> > +     /* Reset the flags for all entries by default. */
>> > ```
>>
>> This reads the same to me, but I can do that yes. understand that VALID means
>> the reference exist and the TS should point to some existing past reference
>> (unless there was some decode error, which the userland may not be aware yet as
>> this is asynchronous). While ACTIVE means that it is used as a reference. FFMPEG
>> is known not to filter inactive references. ACTIVE is just a flag without bunch
>> of other flags that can change for every decode. So none of this make sense
>> between 2 decodes.
>>
>
>Please leave the comment as-is, 'Disable all entries' is readable.
>
>As much as I'd love to have very clear comments everywhere,
>we have to accept that only people with insight on codec details
>are expected to make sense of driver details :-)

Fair enough, I just feel like the code should try his best to have as
few hidden details as possible. And in this case our intention is to
disable the entries (removing the active and valid flag) but what we do
is that we remove all flags.

The hidden details at this point are:
- flags will be restored automatically later on and we do not want to
hold the state here
- flags are not immutable between decodes
- this was a decision by the programmer to simplify the code and not a rule in the specification.

All of these things are not obvious from the code (from my POV) and
there are no pointers to the specification, which means the reader is
seemingly expected to know the specification on an expert level. I think
really understanding the H264 specification is hard enough on its own.
And as we do that change right now, I think we might as well take some
time to document as much hidden details as possible and I really think
that it will not lower the code quality when we add another 20-40 lines
of comments.

I might be wrong (as I still learn about H264) but I would love if we
can write code (especially open-source code) in a manner that enables
others to learn quickly.

>
>Thanks,
>Ezequiel

Greetings,
Sebastian

[PATCH v1 23/24] media: hantro: Add H.264 field decoding support

[Nicolas Dufresne]

This adds the required code to support field decoding. While most of
the code is derived from Rockchip and VSI reference code, the
reduction of the reference list to 16 entries has been found by
trial and errors. The list consist of all the references with the
opposite field parity.

The strategy being to deduplicate the reference picture that points
to the same storage (same index). The choice of opposite parity has
been made to keep the other field or a field pair to be kept in the
list. This method may not be robust if a field was lost.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/hantro/hantro_h264.c | 107 ++++++++++++++++++---
 drivers/staging/media/hantro/hantro_hw.h   |   1 +
 2 files changed, 94 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 7377fc26f780..f6fc939aa726 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -22,6 +22,11 @@
 #define POC_BUFFER_SIZE			34
 #define SCALING_LIST_SIZE		(6 * 16 + 2 * 64)
 
+/* For valid and long term reference marking, index are reversed, so bit 31
+ * indicates the status of the picture 0.
+ */
+#define REF_BIT(i)			BIT(32 - 1 - (i))
+
 /* Data structure describing auxiliary buffer format. */
 struct hantro_h264_dec_priv_tbl {
 	u32 cabac_table[CABAC_INIT_BUFFER_SIZE];
@@ -227,6 +232,7 @@ static void prepare_table(struct hantro_ctx *ctx)
 {
 	const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
 	const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
+	const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
 	struct hantro_h264_dec_priv_tbl *tbl = ctx->h264_dec.priv.cpu;
 	const struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
 	u32 dpb_longterm = 0;
@@ -237,20 +243,45 @@ static void prepare_table(struct hantro_ctx *ctx)
 		tbl->poc[i * 2] = dpb[i].top_field_order_cnt;
 		tbl->poc[i * 2 + 1] = dpb[i].bottom_field_order_cnt;
 
+		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
+			continue;
+
 		/*
 		 * Set up bit maps of valid and long term DPBs.
-		 * NOTE: The bits are reversed, i.e. MSb is DPB 0.
+		 * NOTE: The bits are reversed, i.e. MSb is DPB 0. For frame
+		 * decoding, bit 31 to 15 are used, while for field decoding,
+		 * all bits are used, with bit 31 being a top field, 30 a bottom
+		 * field and so on.
 		 */
-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
-			dpb_valid |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
-			dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
+		if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) {
+			if (dpb[i].fields & V4L2_H264_TOP_FIELD_REF)
+				dpb_valid |= REF_BIT(i * 2);
+
+			if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF)
+				dpb_valid |= REF_BIT(i * 2 + 1);
+
+			if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) {
+				dpb_longterm |= REF_BIT(i * 2);
+				dpb_longterm |= REF_BIT(i * 2 + 1);
+			}
+		} else {
+			dpb_valid |= REF_BIT(i);
+
+			if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
+				dpb_longterm |= REF_BIT(i);
+		}
+	}
+	ctx->h264_dec.dpb_valid = dpb_valid;
+	ctx->h264_dec.dpb_longterm = dpb_longterm;
+
+	if ((dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) ||
+	    !(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)) {
+		tbl->poc[32] = ctx->h264_dec.cur_poc;
+		tbl->poc[33] = 0;
+	} else {
+		tbl->poc[32] = dec_param->top_field_order_cnt;
+		tbl->poc[33] = dec_param->bottom_field_order_cnt;
 	}
-	ctx->h264_dec.dpb_valid = dpb_valid << 16;
-	ctx->h264_dec.dpb_longterm = dpb_longterm << 16;
-
-	tbl->poc[32] = dec_param->top_field_order_cnt;
-	tbl->poc[33] = dec_param->bottom_field_order_cnt;
 
 	assemble_scaling_list(ctx);
 }
@@ -326,6 +357,8 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
 {
 	struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
 	dma_addr_t dma_addr = 0;
+	s32 cur_poc = ctx->h264_dec.cur_poc;
+	u32 flags;
 
 	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
 		dma_addr = hantro_get_ref(ctx, dpb[dpb_idx].reference_ts);
@@ -343,7 +376,12 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
 		dma_addr = hantro_get_dec_buf_addr(ctx, buf);
 	}
 
-	return dma_addr;
+	flags = dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD ? 0x2 : 0;
+	flags |= abs(dpb[dpb_idx].top_field_order_cnt - cur_poc) <
+		 abs(dpb[dpb_idx].bottom_field_order_cnt - cur_poc) ?
+		 0x1 : 0;
+
+	return dma_addr | flags;
 }
 
 u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
@@ -355,6 +393,34 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
 	return dpb->frame_num;
 }
 
+static void deduplicate_reflist(struct v4l2_h264_reflist_builder *b,
+				struct v4l2_h264_reference *reflist)
+{
+	int write_idx = 0;
+	int i;
+
+	if (b->cur_pic_fields == V4L2_H264_FRAME_REF) {
+		write_idx = b->num_valid;
+		goto done;
+	}
+
+	for (i = 0; i < b->num_valid; i++) {
+		if (!(b->cur_pic_fields == reflist[i].fields)) {
+			reflist[write_idx++] = reflist[i];
+			continue;
+		}
+	}
+
+done:
+	/* Should not happen unless we have a bug in the reflist builder. */
+	if (WARN_ON(write_idx > 16))
+		write_idx = 16;
+
+	/* Clear the remaining, some streams fails otherwise */
+	for (; write_idx < 16; write_idx++)
+		reflist[write_idx].index = 15;
+}
+
 int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
 {
 	struct hantro_h264_dec_hw_ctx *h264_ctx = &ctx->h264_dec;
@@ -386,15 +452,28 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
 	/* Update the DPB with new refs. */
 	update_dpb(ctx);
 
-	/* Prepare data in memory. */
-	prepare_table(ctx);
-
 	/* Build the P/B{0,1} ref lists. */
 	v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
 				       ctrls->sps, ctx->h264_dec.dpb);
+	h264_ctx->cur_poc = reflist_builder.cur_pic_order_count;
+
+	/* Prepare data in memory. */
+	prepare_table(ctx);
+
 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
 				    h264_ctx->reflists.b1);
+
+	/* Reduce ref lists to at most 16 entries, Hantro hardware will deduce
+	 * the actual picture lists in field through the dpb_valid,
+	 * dpb_longterm bitmap along with the current frame parity.
+	 */
+	if (reflist_builder.cur_pic_fields != V4L2_H264_FRAME_REF) {
+		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.p);
+		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b0);
+		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b1);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 292aaaabaf24..fd869369fb97 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -91,6 +91,7 @@ struct hantro_h264_dec_hw_ctx {
 	struct hantro_h264_dec_ctrls ctrls;
 	u32 dpb_longterm;
 	u32 dpb_valid;
+	s32 cur_poc;
 };
 
 /**
-- 
2.34.1

Re: [PATCH v1 23/24] media: hantro: Add H.264 field decoding support

[Sebastian Fricke]

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>This adds the required code to support field decoding. While most of
>the code is derived from Rockchip and VSI reference code, the
>reduction of the reference list to 16 entries has been found by

s/has been/was/

>trial and errors. The list consist of all the references with the

s/consist/consists/

>opposite field parity.
>
>The strategy being to deduplicate the reference picture that points

s/strategy being to/strategy is to/

>to the same storage (same index). The choice of opposite parity has
>been made to keep the other field or a field pair to be kept in the

Do you mean?

s/keep the other field or a field pair to be kept/
   keep the other field of a field pair/

>list. This method may not be robust if a field was lost.
>
>Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>---
> drivers/staging/media/hantro/hantro_h264.c | 107 ++++++++++++++++++---
> drivers/staging/media/hantro/hantro_hw.h   |   1 +
> 2 files changed, 94 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
>index 7377fc26f780..f6fc939aa726 100644
>--- a/drivers/staging/media/hantro/hantro_h264.c
>+++ b/drivers/staging/media/hantro/hantro_h264.c
>@@ -22,6 +22,11 @@
> #define POC_BUFFER_SIZE			34
> #define SCALING_LIST_SIZE		(6 * 16 + 2 * 64)
>
>+/* For valid and long term reference marking, index are reversed, so bit 31

s/index/indeces/

>+ * indicates the status of the picture 0.
>+ */
>+#define REF_BIT(i)			BIT(32 - 1 - (i))
>+
> /* Data structure describing auxiliary buffer format. */
> struct hantro_h264_dec_priv_tbl {
> 	u32 cabac_table[CABAC_INIT_BUFFER_SIZE];
>@@ -227,6 +232,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> {
> 	const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
> 	const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
>+	const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
> 	struct hantro_h264_dec_priv_tbl *tbl = ctx->h264_dec.priv.cpu;
> 	const struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> 	u32 dpb_longterm = 0;
>@@ -237,20 +243,45 @@ static void prepare_table(struct hantro_ctx *ctx)
> 		tbl->poc[i * 2] = dpb[i].top_field_order_cnt;
> 		tbl->poc[i * 2 + 1] = dpb[i].bottom_field_order_cnt;
>
>+		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
>+			continue;
>+
> 		/*
> 		 * Set up bit maps of valid and long term DPBs.
>-		 * NOTE: The bits are reversed, i.e. MSb is DPB 0.
>+		 * NOTE: The bits are reversed, i.e. MSb is DPB 0. For frame
>+		 * decoding, bit 31 to 15 are used, while for field decoding,

s/bit 31/bits 31/

>+		 * all bits are used, with bit 31 being a top field, 30 a bottom
>+		 * field and so on.
> 		 */
>-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
>-			dpb_valid |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
>-		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
>-			dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
>+		if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) {
>+			if (dpb[i].fields & V4L2_H264_TOP_FIELD_REF)
>+				dpb_valid |= REF_BIT(i * 2);
>+
>+			if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF)
>+				dpb_valid |= REF_BIT(i * 2 + 1);
>+
>+			if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) {
>+				dpb_longterm |= REF_BIT(i * 2);
>+				dpb_longterm |= REF_BIT(i * 2 + 1);
>+			}
>+		} else {
>+			dpb_valid |= REF_BIT(i);
>+
>+			if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
>+				dpb_longterm |= REF_BIT(i);
>+		}
>+	}
>+	ctx->h264_dec.dpb_valid = dpb_valid;
>+	ctx->h264_dec.dpb_longterm = dpb_longterm;
>+
>+	if ((dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) ||
>+	    !(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)) {
>+		tbl->poc[32] = ctx->h264_dec.cur_poc;
>+		tbl->poc[33] = 0;
>+	} else {
>+		tbl->poc[32] = dec_param->top_field_order_cnt;
>+		tbl->poc[33] = dec_param->bottom_field_order_cnt;
> 	}
>-	ctx->h264_dec.dpb_valid = dpb_valid << 16;
>-	ctx->h264_dec.dpb_longterm = dpb_longterm << 16;
>-
>-	tbl->poc[32] = dec_param->top_field_order_cnt;
>-	tbl->poc[33] = dec_param->bottom_field_order_cnt;
>
> 	assemble_scaling_list(ctx);
> }
>@@ -326,6 +357,8 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
> {
> 	struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> 	dma_addr_t dma_addr = 0;
>+	s32 cur_poc = ctx->h264_dec.cur_poc;
>+	u32 flags;
>
> 	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> 		dma_addr = hantro_get_ref(ctx, dpb[dpb_idx].reference_ts);
>@@ -343,7 +376,12 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
> 		dma_addr = hantro_get_dec_buf_addr(ctx, buf);
> 	}
>
>-	return dma_addr;
>+	flags = dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD ? 0x2 : 0;

>+	flags |= abs(dpb[dpb_idx].top_field_order_cnt - cur_poc) <
>+		 abs(dpb[dpb_idx].bottom_field_order_cnt - cur_poc) ?
>+		 0x1 : 0;

You use the magic values `0x1` & `0x2` here, can you replace those with
macros?

It looks 0x2 indicates that we have a field and 0x1 indicates the
distance of the current picture to the bottom field is greater than
the distance of the current picture to the top field. (inidicating that
the order is correct?)

So maybe:
```
#define HANTRO_H264_FIELD_DMA_ADDR              0x1
#define HANTRO_H264_CORRECT_FIELD_ORDER         0x2
```


>+
>+	return dma_addr | flags;
> }
>
> u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
>@@ -355,6 +393,34 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> 	return dpb->frame_num;
> }
>
>+static void deduplicate_reflist(struct v4l2_h264_reflist_builder *b,
>+				struct v4l2_h264_reference *reflist)

Can you add a comment describing why we need to deduplicate the
reference list? And maybe also why we get duplications in the first
place? Why must we limit the size to 16?
This would increase the readability of the code a lot.

>+{
>+	int write_idx = 0;
>+	int i;
>+
>+	if (b->cur_pic_fields == V4L2_H264_FRAME_REF) {
>+		write_idx = b->num_valid;
>+		goto done;
>+	}
>+
>+	for (i = 0; i < b->num_valid; i++) {
>+		if (!(b->cur_pic_fields == reflist[i].fields)) {
>+			reflist[write_idx++] = reflist[i];
>+			continue;
>+		}
>+	}
>+
>+done:
>+	/* Should not happen unless we have a bug in the reflist builder. */
>+	if (WARN_ON(write_idx > 16))
>+		write_idx = 16;
>+
>+	/* Clear the remaining, some streams fails otherwise */

s/the remaining/the remaining entries/
s/fails/fail/

>+	for (; write_idx < 16; write_idx++)
>+		reflist[write_idx].index = 15;
>+}
>+
> int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
> {
> 	struct hantro_h264_dec_hw_ctx *h264_ctx = &ctx->h264_dec;
>@@ -386,15 +452,28 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
> 	/* Update the DPB with new refs. */
> 	update_dpb(ctx);
>
>-	/* Prepare data in memory. */
>-	prepare_table(ctx);
>-
> 	/* Build the P/B{0,1} ref lists. */
> 	v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
> 				       ctrls->sps, ctx->h264_dec.dpb);
>+	h264_ctx->cur_poc = reflist_builder.cur_pic_order_count;
>+
>+	/* Prepare data in memory. */
>+	prepare_table(ctx);
>+
> 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
> 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> 				    h264_ctx->reflists.b1);
>+
>+	/* Reduce ref lists to at most 16 entries, Hantro hardware will deduce
>+	 * the actual picture lists in field through the dpb_valid,

s/in field/in a field/

>+	 * dpb_longterm bitmap along with the current frame parity.

s/bitmap/bitmaps/

Greetings,
Sebastian

>+	 */
>+	if (reflist_builder.cur_pic_fields != V4L2_H264_FRAME_REF) {
>+		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.p);
>+		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b0);
>+		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b1);
>+	}
>+
> 	return 0;
> }
>
>diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
>index 292aaaabaf24..fd869369fb97 100644
>--- a/drivers/staging/media/hantro/hantro_hw.h
>+++ b/drivers/staging/media/hantro/hantro_hw.h
>@@ -91,6 +91,7 @@ struct hantro_h264_dec_hw_ctx {
> 	struct hantro_h264_dec_ctrls ctrls;
> 	u32 dpb_longterm;
> 	u32 dpb_valid;
>+	s32 cur_poc;
> };
>
> /**
>-- 
>2.34.1
>

Re: [PATCH v1 23/24] media: hantro: Add H.264 field decoding support

[Nicolas Dufresne]

Le mercredi 30 mars 2022 à 11:03 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > This adds the required code to support field decoding. While most of
> > the code is derived from Rockchip and VSI reference code, the
> > reduction of the reference list to 16 entries has been found by
> 
> s/has been/was/
> 
> > trial and errors. The list consist of all the references with the
> 
> s/consist/consists/
> 
> > opposite field parity.
> > 
> > The strategy being to deduplicate the reference picture that points
> 
> s/strategy being to/strategy is to/
> 
> > to the same storage (same index). The choice of opposite parity has
> > been made to keep the other field or a field pair to be kept in the
> 
> Do you mean?
> 
> s/keep the other field or a field pair to be kept/
>    keep the other field of a field pair/
> 
> > list. This method may not be robust if a field was lost.
> > 
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 107 ++++++++++++++++++---
> > drivers/staging/media/hantro/hantro_hw.h   |   1 +
> > 2 files changed, 94 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 7377fc26f780..f6fc939aa726 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -22,6 +22,11 @@
> > #define POC_BUFFER_SIZE			34
> > #define SCALING_LIST_SIZE		(6 * 16 + 2 * 64)
> > 
> > +/* For valid and long term reference marking, index are reversed, so bit 31
> 
> s/index/indeces/
> 
> > + * indicates the status of the picture 0.
> > + */
> > +#define REF_BIT(i)			BIT(32 - 1 - (i))
> > +
> > /* Data structure describing auxiliary buffer format. */
> > struct hantro_h264_dec_priv_tbl {
> > 	u32 cabac_table[CABAC_INIT_BUFFER_SIZE];
> > @@ -227,6 +232,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > {
> > 	const struct hantro_h264_dec_ctrls *ctrls = &ctx->h264_dec.ctrls;
> > 	const struct v4l2_ctrl_h264_decode_params *dec_param = ctrls->decode;
> > +	const struct v4l2_ctrl_h264_sps *sps = ctrls->sps;
> > 	struct hantro_h264_dec_priv_tbl *tbl = ctx->h264_dec.priv.cpu;
> > 	const struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> > 	u32 dpb_longterm = 0;
> > @@ -237,20 +243,45 @@ static void prepare_table(struct hantro_ctx *ctx)
> > 		tbl->poc[i * 2] = dpb[i].top_field_order_cnt;
> > 		tbl->poc[i * 2 + 1] = dpb[i].bottom_field_order_cnt;
> > 
> > +		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > +			continue;
> > +
> > 		/*
> > 		 * Set up bit maps of valid and long term DPBs.
> > -		 * NOTE: The bits are reversed, i.e. MSb is DPB 0.
> > +		 * NOTE: The bits are reversed, i.e. MSb is DPB 0. For frame
> > +		 * decoding, bit 31 to 15 are used, while for field decoding,
> 
> s/bit 31/bits 31/
> 
> > +		 * all bits are used, with bit 31 being a top field, 30 a bottom
> > +		 * field and so on.
> > 		 */
> > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > -			dpb_valid |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
> > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> > -			dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE - 1 - i);
> > +		if (dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) {
> > +			if (dpb[i].fields & V4L2_H264_TOP_FIELD_REF)
> > +				dpb_valid |= REF_BIT(i * 2);
> > +
> > +			if (dpb[i].fields & V4L2_H264_BOTTOM_FIELD_REF)
> > +				dpb_valid |= REF_BIT(i * 2 + 1);
> > +
> > +			if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM) {
> > +				dpb_longterm |= REF_BIT(i * 2);
> > +				dpb_longterm |= REF_BIT(i * 2 + 1);
> > +			}
> > +		} else {
> > +			dpb_valid |= REF_BIT(i);
> > +
> > +			if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
> > +				dpb_longterm |= REF_BIT(i);
> > +		}
> > +	}
> > +	ctx->h264_dec.dpb_valid = dpb_valid;
> > +	ctx->h264_dec.dpb_longterm = dpb_longterm;
> > +
> > +	if ((dec_param->flags & V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC) ||
> > +	    !(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)) {
> > +		tbl->poc[32] = ctx->h264_dec.cur_poc;
> > +		tbl->poc[33] = 0;
> > +	} else {
> > +		tbl->poc[32] = dec_param->top_field_order_cnt;
> > +		tbl->poc[33] = dec_param->bottom_field_order_cnt;
> > 	}
> > -	ctx->h264_dec.dpb_valid = dpb_valid << 16;
> > -	ctx->h264_dec.dpb_longterm = dpb_longterm << 16;
> > -
> > -	tbl->poc[32] = dec_param->top_field_order_cnt;
> > -	tbl->poc[33] = dec_param->bottom_field_order_cnt;
> > 
> > 	assemble_scaling_list(ctx);
> > }
> > @@ -326,6 +357,8 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
> > {
> > 	struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
> > 	dma_addr_t dma_addr = 0;
> > +	s32 cur_poc = ctx->h264_dec.cur_poc;
> > +	u32 flags;
> > 
> > 	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > 		dma_addr = hantro_get_ref(ctx, dpb[dpb_idx].reference_ts);
> > @@ -343,7 +376,12 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
> > 		dma_addr = hantro_get_dec_buf_addr(ctx, buf);
> > 	}
> > 
> > -	return dma_addr;
> > +	flags = dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD ? 0x2 : 0;
> 
> > +	flags |= abs(dpb[dpb_idx].top_field_order_cnt - cur_poc) <
> > +		 abs(dpb[dpb_idx].bottom_field_order_cnt - cur_poc) ?
> > +		 0x1 : 0;
> 
> You use the magic values `0x1` & `0x2` here, can you replace those with
> macros?
> 
> It looks 0x2 indicates that we have a field and 0x1 indicates the
> distance of the current picture to the bottom field is greater than
> the distance of the current picture to the top field. (inidicating that
> the order is correct?)
> 
> So maybe:
> ```
> #define HANTRO_H264_FIELD_DMA_ADDR              0x1
> #define HANTRO_H264_CORRECT_FIELD_ORDER         0x2
> ```

Will do, I need to check again in the ref code for the appropriate wording for
the meaning of these bits. I don't like much FIELD_DMA_ADDR, as its not an
address, or an offset to an address. Here the address must be aligned, which
results in these bits always being 0, so they reuse it to pass per-reference
information.

> 
> 
> > +
> > +	return dma_addr | flags;
> > }
> > 
> > u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > @@ -355,6 +393,34 @@ u16 hantro_h264_get_ref_nbr(struct hantro_ctx *ctx, unsigned int dpb_idx)
> > 	return dpb->frame_num;
> > }
> > 
> > +static void deduplicate_reflist(struct v4l2_h264_reflist_builder *b,
> > +				struct v4l2_h264_reference *reflist)
> 
> Can you add a comment describing why we need to deduplicate the
> reference list? And maybe also why we get duplications in the first
> place? Why must we limit the size to 16?
> This would increase the readability of the code a lot.

For readers that did understand the H.264 specification, that a field reflist
has 32 entry should be obvious. That hantro uses 16 entry only is an
undocumented trickery, I'll write something to the best of my knowledge, but
this has been reversed, so don't expect the highest rationale. I will though
document here what I say in the commit, e.g. why this method might not be
robust, so someone can give it a try and make it more robust in the future.

> 
> > +{
> > +	int write_idx = 0;
> > +	int i;
> > +
> > +	if (b->cur_pic_fields == V4L2_H264_FRAME_REF) {
> > +		write_idx = b->num_valid;
> > +		goto done;
> > +	}
> > +
> > +	for (i = 0; i < b->num_valid; i++) {
> > +		if (!(b->cur_pic_fields == reflist[i].fields)) {
> > +			reflist[write_idx++] = reflist[i];
> > +			continue;
> > +		}
> > +	}
> > +
> > +done:
> > +	/* Should not happen unless we have a bug in the reflist builder. */
> > +	if (WARN_ON(write_idx > 16))
> > +		write_idx = 16;
> > +
> > +	/* Clear the remaining, some streams fails otherwise */
> 
> s/the remaining/the remaining entries/
> s/fails/fail/
> 
> > +	for (; write_idx < 16; write_idx++)
> > +		reflist[write_idx].index = 15;
> > +}
> > +
> > int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
> > {
> > 	struct hantro_h264_dec_hw_ctx *h264_ctx = &ctx->h264_dec;
> > @@ -386,15 +452,28 @@ int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
> > 	/* Update the DPB with new refs. */
> > 	update_dpb(ctx);
> > 
> > -	/* Prepare data in memory. */
> > -	prepare_table(ctx);
> > -
> > 	/* Build the P/B{0,1} ref lists. */
> > 	v4l2_h264_init_reflist_builder(&reflist_builder, ctrls->decode,
> > 				       ctrls->sps, ctx->h264_dec.dpb);
> > +	h264_ctx->cur_poc = reflist_builder.cur_pic_order_count;
> > +
> > +	/* Prepare data in memory. */
> > +	prepare_table(ctx);
> > +
> > 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
> > 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> > 				    h264_ctx->reflists.b1);
> > +
> > +	/* Reduce ref lists to at most 16 entries, Hantro hardware will deduce
> > +	 * the actual picture lists in field through the dpb_valid,
> 
> s/in field/in a field/
> 
> > +	 * dpb_longterm bitmap along with the current frame parity.
> 
> s/bitmap/bitmaps/
> 
> Greetings,
> Sebastian
> 
> > +	 */
> > +	if (reflist_builder.cur_pic_fields != V4L2_H264_FRAME_REF) {
> > +		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.p);
> > +		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b0);
> > +		deduplicate_reflist(&reflist_builder, h264_ctx->reflists.b1);
> > +	}
> > +
> > 	return 0;
> > }
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index 292aaaabaf24..fd869369fb97 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -91,6 +91,7 @@ struct hantro_h264_dec_hw_ctx {
> > 	struct hantro_h264_dec_ctrls ctrls;
> > 	u32 dpb_longterm;
> > 	u32 dpb_valid;
> > +	s32 cur_poc;
> > };
> > 
> > /**
> > -- 
> > 2.34.1
> > 

[PATCH v1 24/24] media: rkvdec-h264: Don't hardcode SPS/PPS parameters

[Nicolas Dufresne]

From: Alex Bee <knaerzche@gmail.com>

Some SPS/PPS parameters are currently hardcoded in the driver
even though so do exist in the uapi which is stable by now.

Use them instead of hardcoding them.

Conformance tests have shown there is no difference, but it might
increase decoder performance.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 891c48bf6a51..91f65d78e453 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -655,13 +655,14 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 
 #define WRITE_PPS(value, field) set_ps_field(hw_ps->info, field, value)
 	/* write sps */
-	WRITE_PPS(0xf, SEQ_PARAMETER_SET_ID);
-	WRITE_PPS(0xff, PROFILE_IDC);
-	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
+	WRITE_PPS(sps->seq_parameter_set_id, SEQ_PARAMETER_SET_ID);
+	WRITE_PPS(sps->profile_idc, PROFILE_IDC);
+	WRITE_PPS((sps->constraint_set_flags & 1 << 3) ? 1 : 0, CONSTRAINT_SET3_FLAG);
 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
 	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
 	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
-	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
+	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS),
+		  QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
 	WRITE_PPS(sps->pic_order_cnt_type, PIC_ORDER_CNT_TYPE);
@@ -679,8 +680,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 		  DIRECT_8X8_INFERENCE_FLAG);
 
 	/* write pps */
-	WRITE_PPS(0xff, PIC_PARAMETER_SET_ID);
-	WRITE_PPS(0x1f, PPS_SEQ_PARAMETER_SET_ID);
+	WRITE_PPS(pps->pic_parameter_set_id, PIC_PARAMETER_SET_ID);
+	WRITE_PPS(pps->seq_parameter_set_id, PPS_SEQ_PARAMETER_SET_ID);
 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE),
 		  ENTROPY_CODING_MODE_FLAG);
 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT),
-- 
2.34.1

Re: [PATCH v1 24/24] media: rkvdec-h264: Don't hardcode SPS/PPS parameters

[Sebastian Fricke]

Hey Nicolas,

The patch series doesn't seem to apply on the latest media tree master
branch. Looking at your tree I can see that you have commit: ba2c670a
"media: nxp: Restrict VIDEO_IMX_MIPI_CSIS to ARCH_MXC or COMPILE_TEST
Laurent Pinchart authored 1 week ago "

But the current head of the media tree is: 71e6d0608e4d
"media: platform: Remove unnecessary print function dev_err()
Yang Li authored 13 days ago"

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Alex Bee <knaerzche@gmail.com>
>
>Some SPS/PPS parameters are currently hardcoded in the driver
>even though so do exist in the uapi which is stable by now.

s/even though so/even though they/
>
>Use them instead of hardcoding them.
>
>Conformance tests have shown there is no difference, but it might
>increase decoder performance.

I think it would be great if we could add some performance metrics to
the commit description to have a metric that following patches could
compare themselves with.

Greetings,
Sebastian

>
>Signed-off-by: Alex Bee <knaerzche@gmail.com>
>---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index 891c48bf6a51..91f65d78e453 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -655,13 +655,14 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
>
> #define WRITE_PPS(value, field) set_ps_field(hw_ps->info, field, value)
> 	/* write sps */
>-	WRITE_PPS(0xf, SEQ_PARAMETER_SET_ID);
>-	WRITE_PPS(0xff, PROFILE_IDC);
>-	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
>+	WRITE_PPS(sps->seq_parameter_set_id, SEQ_PARAMETER_SET_ID);
>+	WRITE_PPS(sps->profile_idc, PROFILE_IDC);
>+	WRITE_PPS((sps->constraint_set_flags & 1 << 3) ? 1 : 0, CONSTRAINT_SET3_FLAG);
> 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
> 	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
> 	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
>-	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
>+	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS),
>+		  QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
> 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
> 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
> 	WRITE_PPS(sps->pic_order_cnt_type, PIC_ORDER_CNT_TYPE);
>@@ -679,8 +680,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> 		  DIRECT_8X8_INFERENCE_FLAG);
>
> 	/* write pps */
>-	WRITE_PPS(0xff, PIC_PARAMETER_SET_ID);
>-	WRITE_PPS(0x1f, PPS_SEQ_PARAMETER_SET_ID);
>+	WRITE_PPS(pps->pic_parameter_set_id, PIC_PARAMETER_SET_ID);
>+	WRITE_PPS(pps->seq_parameter_set_id, PPS_SEQ_PARAMETER_SET_ID);
> 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE),
> 		  ENTROPY_CODING_MODE_FLAG);
> 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT),
>-- 
>2.34.1
>

Re: [PATCH v1 24/24] media: rkvdec-h264: Don't hardcode SPS/PPS parameters

[Nicolas Dufresne]

Le mardi 29 mars 2022 à 09:22 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> The patch series doesn't seem to apply on the latest media tree master
> branch. Looking at your tree I can see that you have commit: ba2c670a
> "media: nxp: Restrict VIDEO_IMX_MIPI_CSIS to ARCH_MXC or COMPILE_TEST
> Laurent Pinchart authored 1 week ago "
> 
> But the current head of the media tree is: 71e6d0608e4d
> "media: platform: Remove unnecessary print function dev_err()
> Yang Li authored 13 days ago"
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > From: Alex Bee <knaerzche@gmail.com>
> > 
> > Some SPS/PPS parameters are currently hardcoded in the driver
> > even though so do exist in the uapi which is stable by now.
> 
> s/even though so/even though they/
> > 
> > Use them instead of hardcoding them.
> > 
> > Conformance tests have shown there is no difference, but it might
> > increase decoder performance.
> 
> I think it would be great if we could add some performance metrics to
> the commit description to have a metric that following patches could
> compare themselves with.

Alex, can you extend on this one? I'm not sure how this can impact performance,
so I doubt any mitric will be significant. Can I just drop that part of the
comment ?
> 
> Greetings,
> Sebastian
> 
> > 
> > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > ---
> > drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 891c48bf6a51..91f65d78e453 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -655,13 +655,14 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > 
> > #define WRITE_PPS(value, field) set_ps_field(hw_ps->info, field, value)
> > 	/* write sps */
> > -	WRITE_PPS(0xf, SEQ_PARAMETER_SET_ID);
> > -	WRITE_PPS(0xff, PROFILE_IDC);
> > -	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
> > +	WRITE_PPS(sps->seq_parameter_set_id, SEQ_PARAMETER_SET_ID);
> > +	WRITE_PPS(sps->profile_idc, PROFILE_IDC);
> > +	WRITE_PPS((sps->constraint_set_flags & 1 << 3) ? 1 : 0, CONSTRAINT_SET3_FLAG);
> > 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
> > 	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
> > 	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
> > -	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
> > +	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS),
> > +		  QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
> > 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
> > 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
> > 	WRITE_PPS(sps->pic_order_cnt_type, PIC_ORDER_CNT_TYPE);
> > @@ -679,8 +680,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > 		  DIRECT_8X8_INFERENCE_FLAG);
> > 
> > 	/* write pps */
> > -	WRITE_PPS(0xff, PIC_PARAMETER_SET_ID);
> > -	WRITE_PPS(0x1f, PPS_SEQ_PARAMETER_SET_ID);
> > +	WRITE_PPS(pps->pic_parameter_set_id, PIC_PARAMETER_SET_ID);
> > +	WRITE_PPS(pps->seq_parameter_set_id, PPS_SEQ_PARAMETER_SET_ID);
> > 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE),
> > 		  ENTROPY_CODING_MODE_FLAG);
> > 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT),
> > -- 
> > 2.34.1
> >