[PATCH RFC v2 00/10] media: hantro: H264 fixes and improvements

[PATCH RFC v2 00/10] media: hantro: H264 fixes and improvements

[Jonas Karlman]

This series contains fixes and improvements for the hantro H264 decoder.

Patch 1-3 is updated patches from the "media: hantro: Collected fixes for v5.4"
series.

Patch 4-8 fixes issues and limitations observed when preparing support
for field encoded content.

Patch 9 introduce new DPB entry flags that is used to signal how a reference
frame is referenced. This information is needed to correctly build a
reference list for field encoded content.

Patch 10 adds bits to handle field encoded content, this is a rough patch
and should be reworked with proper code style and formatting.
Please get back with feedback on how to improve this.

The following samples from [1] are now playable with this series
- H264_1080i-25-interlace_Kaesescheibchen.mkv
- H264_10_1080i_50_AC3-Astra19.2_ProSieben_HD.ts
- big_buck_bunny_1080p_H264_AAC_25fps_7200K.mp4
- h264_tivo_sample.ts

This series has been tested using ffmpeg v4l2 request hwaccel at [2]

[1] http://kwiboo.libreelec.tv/test/samples/
[2] https://github.com/Kwiboo/FFmpeg/compare/4.0.4-Leia-18.4...v4l2-request-hwaccel-4.0.4-hantro

Changes in v2:
  - scaling list changes split to its own series
  - address feedback from Philipp and Ezequiel

Regards,
Jonas

Francois Buergisser (2):
  media: hantro: Fix motion vectors usage condition
  media: hantro: Fix picture order count table enable

Jonas Karlman (8):
  media: hantro: Fix H264 max frmsize supported on RK3288
  media: hantro: Fix H264 motion vector buffer offset
  media: hantro: Reduce H264 extra space for motion vectors
  media: hantro: Use capture buffer width and height for H264 decoding
  media: hantro: Remove now unused H264 pic_size
  media: hantro: Set H264 FIELDPIC_FLAG_E flag correctly
  media: uapi: h264: Add DPB entry field reference flags
  media: hantro: Fix H264 decoding of field encoded content

 .../media/uapi/v4l/ext-ctrls-codec.rst        |  12 ++
 .../staging/media/hantro/hantro_g1_h264_dec.c |  62 ++++-----
 drivers/staging/media/hantro/hantro_h264.c    | 126 +++++++++++-------
 drivers/staging/media/hantro/hantro_hw.h      |   5 +-
 drivers/staging/media/hantro/hantro_v4l2.c    |   6 +-
 drivers/staging/media/hantro/rk3288_vpu_hw.c  |   4 +-
 include/media/h264-ctrls.h                    |   4 +
 7 files changed, 135 insertions(+), 84 deletions(-)

-- 
2.17.1

[PATCH v2 02/10] media: hantro: Fix motion vectors usage condition

[Jonas Karlman]

From: Francois Buergisser <fbuergisser@chromium.org>

The setting of the motion vectors usage and the setting of motion
vectors address are currently done under different conditions.

When decoding pre-recorded videos, this results of leaving the motion
vectors address unset, resulting in faulty memory accesses. Fix it
by using the same condition everywhere, which matches the profiles
that support motion vectors.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 29130946dea4..a1cb18680200 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
 	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
 		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
 	reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
-	if (dec_param->nal_ref_idc)
+	if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
 		reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
 
 	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
@@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
 
 	/* Higher profiles require DMV buffer appended to reference frames. */
-	if (ctrls->sps->profile_idc > 66) {
+	if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {
 		size_t pic_size = ctx->h264_dec.pic_size;
 		size_t mv_offset = round_up(pic_size, 8);
 
-- 
2.17.1

[PATCH v2 01/10] media: hantro: Fix H264 max frmsize supported on RK3288

[Jonas Karlman]

TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
change frmsize max_width/max_height to match TRM at [1].

This patch makes it possible to decode the 4096x2304 sample at [2].

[1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
[2] https://4ksamples.com/puppies-bath-in-4k/

Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
  - updated commit message with reference to TRM and sample video
---
 drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
index c0bdd6c02520..f8db6fcaad73 100644
--- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
+++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
@@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
 		.max_depth = 2,
 		.frmsize = {
 			.min_width = 48,
-			.max_width = 3840,
+			.max_width = 4096,
 			.step_width = MB_DIM,
 			.min_height = 48,
-			.max_height = 2160,
+			.max_height = 2304,
 			.step_height = MB_DIM,
 		},
 	},
-- 
2.17.1

[PATCH v2 03/10] media: hantro: Fix picture order count table enable

[Jonas Karlman]

From: Francois Buergisser <fbuergisser@chromium.org>

The picture order count table only makes sense for profiles
higher than Baseline. This is confirmed by the H.264 specification
(See 8.2.1 Decoding process for picture order count), which
clarifies how POC are used for features not present in Baseline.

"""
Picture order counts are used to determine initial picture orderings
for reference pictures in the decoding of B slices, to represent picture
order differences between frames or fields for motion vector derivation
in temporal direct mode, for implicit mode weighted prediction in B slices,
and for decoder conformance checking.
"""

As a side note, this change matches various vendors downstream codebases,
including ChromiumOS and IMX VPU libraries.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index a1cb18680200..70a6b5b26477 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -34,9 +34,11 @@ static void set_params(struct hantro_ctx *ctx)
 	reg = G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(0x0);
 	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
 		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
-	reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
-	if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
-		reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
+	if (sps->profile_idc > 66) {
+		reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
+		if (dec_param->nal_ref_idc)
+			reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
+	}
 
 	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
 	    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD ||
-- 
2.17.1

[PATCH v2 04/10] media: hantro: Fix H264 motion vector buffer offset

[Jonas Karlman]

A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
macroblock and is laid out in memory as follow:

+---------------------------+
| Y-plane   256 bytes x MBs |
+---------------------------+
| UV-plane  128 bytes x MBs |
+---------------------------+
| MV buffer  64 bytes x MBs |
+---------------------------+

The motion vector buffer offset is currently correct for 4:2:0 because the
extra space for motion vectors is overallocated with an extra 64 bytes x MBs.

Wrong offset for both destination and motion vector buffer are used
for the bottom field of field encoded content, wrong offset is
also used for 4:0:0 (monochrome) content.

Fix this by setting the motion vector address to the expected 384 bytes x MBs
offset for 4:2:0 and 256 bytes x MBs offset for 4:0:0 content.

Also use correct destination and motion vector buffer offset
for the bottom field of field encoded content.

While at it also extend the check for 4:0:0 (monochrome) to include an
additional check for High Profile (100).

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
  * address remarks from Philipp and Ezequiel
  - update commit message
  - rename offset to bytes_per_mb
  - remove MV_OFFSET macros
  - move PIC_MB_WIDTH/HEIGHT_P change to separate patch
---
 .../staging/media/hantro/hantro_g1_h264_dec.c | 29 +++++++++++++------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 70a6b5b26477..71bf162eaf73 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -81,7 +81,7 @@ static void set_params(struct hantro_ctx *ctx)
 		reg |= G1_REG_DEC_CTRL4_CABAC_E;
 	if (sps->flags & V4L2_H264_SPS_FLAG_DIRECT_8X8_INFERENCE)
 		reg |= G1_REG_DEC_CTRL4_DIR_8X8_INFER_E;
-	if (sps->chroma_format_idc == 0)
+	if (sps->profile_idc >= 100 && sps->chroma_format_idc == 0)
 		reg |= G1_REG_DEC_CTRL4_BLACKWHITE_E;
 	if (pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED)
 		reg |= G1_REG_DEC_CTRL4_WEIGHT_PRED_E;
@@ -234,6 +234,7 @@ static void set_buffers(struct hantro_ctx *ctx)
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	struct hantro_dev *vpu = ctx->dev;
 	dma_addr_t src_dma, dst_dma;
+	size_t offset = 0;
 
 	src_buf = hantro_get_src_buf(ctx);
 	dst_buf = hantro_get_dst_buf(ctx);
@@ -244,18 +245,28 @@ static void set_buffers(struct hantro_ctx *ctx)
 
 	/* Destination (decoded frame) buffer. */
 	dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
-	vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
+	/* Adjust dma addr to start at second line for bottom field */
+	if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
+		offset = ALIGN(ctx->dst_fmt.width, MB_DIM);
+	vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
 
 	/* Higher profiles require DMV buffer appended to reference frames. */
 	if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {
-		size_t pic_size = ctx->h264_dec.pic_size;
-		size_t mv_offset = round_up(pic_size, 8);
-
+		unsigned int bytes_per_mb = 384;
+		/* DMV buffer for monochrome start directly after Y-plane */
+		if (ctrls->sps->profile_idc >= 100 &&
+		    ctrls->sps->chroma_format_idc == 0)
+			bytes_per_mb = 256;
+		offset = bytes_per_mb * MB_WIDTH(ctx->dst_fmt.width) *
+			 MB_HEIGHT(ctx->dst_fmt.height);
+
+		/* DMV buffer is split in two for field encoded frames,
+		 * adjust offset for bottom field
+		 */
 		if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
-			mv_offset += 32 * MB_WIDTH(ctx->dst_fmt.width);
-
-		vdpu_write_relaxed(vpu, dst_dma + mv_offset,
-				   G1_REG_ADDR_DIR_MV);
+			offset += 32 * MB_WIDTH(ctx->dst_fmt.width) *
+				  MB_HEIGHT(ctx->dst_fmt.height);
+		vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
 	}
 
 	/* Auxiliary buffer prepared in hantro_g1_h264_dec_prepare_table(). */
-- 
2.17.1

[PATCH v2 05/10] media: hantro: Reduce H264 extra space for motion vectors

[Jonas Karlman]

A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
macroblock with additional 32 bytes on multi-core variants.

Memory layout is as follow:

+---------------------------+
| Y-plane   256 bytes x MBs |
+---------------------------+
| UV-plane  128 bytes x MBs |
+---------------------------+
| MV buffer  64 bytes x MBs |
+---------------------------+
| MC sync          32 bytes |
+---------------------------+

Reduce the extra space allocated now that motion vector buffer offset no
longer is based on the extra space.

Only allocate extra space for 64 bytes x MBs of motion vector buffer
and 32 bytes for multi-core sync.

Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
  - updated commit message
---
 drivers/staging/media/hantro/hantro_v4l2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 3dae52abb96c..b3a4368b37de 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -242,12 +242,12 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
 		/*
 		 * The H264 decoder needs extra space on the output buffers
 		 * to store motion vectors. This is needed for reference
-		 * frames.
+		 * frames. 32 extra bytes is used for multi-core sync.
 		 */
 		if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
 			pix_mp->plane_fmt[0].sizeimage +=
-				128 * DIV_ROUND_UP(pix_mp->width, 16) *
-				      DIV_ROUND_UP(pix_mp->height, 16);
+				64 * MB_WIDTH(pix_mp->width) *
+				     MB_WIDTH(pix_mp->height) + 32;
 	} else if (!pix_mp->plane_fmt[0].sizeimage) {
 		/*
 		 * For coded formats the application can specify
-- 
2.17.1

[PATCH v2 06/10] media: hantro: Use capture buffer width and height for H264 decoding

[Jonas Karlman]

Calculations for motion vector buffer offset is based on width and height
from the configured capture format, lets use the same values for macroblock
width and height hw regs.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
  - new patch split from "media: hantro: Fix H264 motion vector buffer offset"
---
 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 71bf162eaf73..eeed11366135 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
 
 	/* Decoder control register 1. */
-	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
-	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
+	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
+	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |
 	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);
 
-- 
2.17.1

[PATCH v2 07/10] media: hantro: Remove now unused H264 pic_size

[Jonas Karlman]

pic_size in hantro_h264_dec_hw_ctx struct is no longer used,
lets remove it.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/hantro/hantro_h264.c | 5 -----
 drivers/staging/media/hantro/hantro_hw.h   | 3 ---
 2 files changed, 8 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 694a330f508e..568640eab3a6 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -618,7 +618,6 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx)
 	struct hantro_h264_dec_hw_ctx *h264_dec = &ctx->h264_dec;
 	struct hantro_aux_buf *priv = &h264_dec->priv;
 	struct hantro_h264_dec_priv_tbl *tbl;
-	struct v4l2_pix_format_mplane pix_mp;
 
 	priv->cpu = dma_alloc_coherent(vpu->dev, sizeof(*tbl), &priv->dma,
 				       GFP_KERNEL);
@@ -629,9 +628,5 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx)
 	tbl = priv->cpu;
 	memcpy(tbl->cabac_table, h264_cabac_table, sizeof(tbl->cabac_table));
 
-	v4l2_fill_pixfmt_mp(&pix_mp, ctx->dst_fmt.pixelformat,
-			    ctx->dst_fmt.width, ctx->dst_fmt.height);
-	h264_dec->pic_size = pix_mp.plane_fmt[0].sizeimage;
-
 	return 0;
 }
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 69b88f4d3fb3..fa91dd1848b7 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -80,15 +80,12 @@ struct hantro_h264_dec_reflists {
  * @dpb:	DPB
  * @reflists:	P/B0/B1 reflists
  * @ctrls:	V4L2 controls attached to a run
- * @pic_size:	Size in bytes of decoded picture, this is needed
- *		to pass the location of motion vectors.
  */
 struct hantro_h264_dec_hw_ctx {
 	struct hantro_aux_buf priv;
 	struct v4l2_h264_dpb_entry dpb[HANTRO_H264_DPB_SIZE];
 	struct hantro_h264_dec_reflists reflists;
 	struct hantro_h264_dec_ctrls ctrls;
-	size_t pic_size;
 };
 
 /**
-- 
2.17.1

[PATCH v2 08/10] media: hantro: Set H264 FIELDPIC_FLAG_E flag correctly

[Jonas Karlman]

The FIELDPIC_FLAG_E bit should be set when field_pic_flag exists in stream,
it is currently set based on field_pic_flag of current frame.
The PIC_FIELDMODE_E bit is correctly set based on the field_pic_flag.

Fix this by setting the FIELDPIC_FLAG_E bit when frame_mbs_only is not set.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index eeed11366135..c07da0ee4973 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -63,7 +63,7 @@ static void set_params(struct hantro_ctx *ctx)
 	/* always use the matrix sent from userspace */
 	reg |= G1_REG_DEC_CTRL2_TYPE1_QUANT_E;
 
-	if (slices[0].flags &  V4L2_H264_SLICE_FLAG_FIELD_PIC)
+	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
 		reg |= G1_REG_DEC_CTRL2_FIELDPIC_FLAG_E;
 	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL2);
 
-- 
2.17.1

[RFC v2 09/10] media: uapi: h264: Add DPB entry field reference flags

[Jonas Karlman]

Add DPB entry flags to help indicate when a reference frame is a field picture
and how the DPB entry is referenced, top or bottom field or full frame.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++++++
 include/media/h264-ctrls.h                       |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index 28313c0f4e7c..d472a54d1c4d 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -2028,6 +2028,18 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
       - 0x00000004
       - The DPB entry is a long term reference frame
+    * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE``
+      - 0x00000008
+      - The DPB entry is a field picture
+    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_TOP``
+      - 0x00000010
+      - The DPB entry is a top field reference
+    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM``
+      - 0x00000020
+      - The DPB entry is a bottom field reference
+    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME``
+      - 0x00000030
+      - The DPB entry is a reference frame
 
 ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
     Specifies the decoding mode to use. Currently exposes slice-based and
diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
index e877bf1d537c..76020ebd1e6c 100644
--- a/include/media/h264-ctrls.h
+++ b/include/media/h264-ctrls.h
@@ -185,6 +185,10 @@ struct v4l2_ctrl_h264_slice_params {
 #define V4L2_H264_DPB_ENTRY_FLAG_VALID		0x01
 #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
 #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
+#define V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE	0x08
+#define V4L2_H264_DPB_ENTRY_FLAG_REF_TOP	0x10
+#define V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM	0x20
+#define V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME	0x30
 
 struct v4l2_h264_dpb_entry {
 	__u64 reference_ts;
-- 
2.17.1

[RFC v2 10/10] media: hantro: Fix H264 decoding of field encoded content

[Jonas Karlman]

This still need code cleanup and formatting

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 .../staging/media/hantro/hantro_g1_h264_dec.c |  17 +--
 drivers/staging/media/hantro/hantro_h264.c    | 121 ++++++++++++------
 drivers/staging/media/hantro/hantro_hw.h      |   2 +
 3 files changed, 85 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index c07da0ee4973..473d7a0d7338 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -132,25 +132,12 @@ static void set_ref(struct hantro_ctx *ctx)
 	struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
 	const u8 *b0_reflist, *b1_reflist, *p_reflist;
 	struct hantro_dev *vpu = ctx->dev;
-	u32 dpb_longterm = 0;
-	u32 dpb_valid = 0;
 	int reg_num;
 	u32 reg;
 	int i;
 
-	/*
-	 * Set up bit maps of valid and long term DPBs.
-	 * NOTE: The bits are reversed, i.e. MSb is DPB 0.
-	 */
-	for (i = 0; i < HANTRO_H264_DPB_SIZE; ++i) {
-		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);
-	}
-	vdpu_write_relaxed(vpu, dpb_valid << 16, G1_REG_VALID_REF);
-	vdpu_write_relaxed(vpu, dpb_longterm << 16, G1_REG_LT_REF);
+	vdpu_write_relaxed(vpu, ctx->h264_dec.dpb_valid, G1_REG_VALID_REF);
+	vdpu_write_relaxed(vpu, ctx->h264_dec.dpb_longterm, G1_REG_LT_REF);
 
 	/*
 	 * Set up reference frame picture numbers.
diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 568640eab3a6..a405d0f5a0a1 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -225,17 +225,65 @@ 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_slice_params *slices = ctrls->slices;
 	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;
+	u32 dpb_valid = 0;
 	int i;
 
+	/*
+	 * Set up bit maps of valid and long term DPBs.
+	 * NOTE: The bits are reversed, i.e. MSb is DPB 0.
+	 */
+	if ((slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) || (slices[0].flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)) {
+		for (i = 0; i < HANTRO_H264_DPB_SIZE * 2; ++i) {
+			// check for correct reference use
+			u32 flag = (i & 0x1) ? V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM : V4L2_H264_DPB_ENTRY_FLAG_REF_TOP;
+			if (dpb[i / 2].flags & flag)
+				dpb_valid |= BIT(HANTRO_H264_DPB_SIZE * 2 - 1 - i);
+
+			if (dpb[i / 2].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
+				dpb_longterm |= BIT(HANTRO_H264_DPB_SIZE * 2 - 1 - i);
+		}
+
+		ctx->h264_dec.dpb_valid = dpb_valid;
+		ctx->h264_dec.dpb_longterm = dpb_longterm;
+	} else {
+		for (i = 0; i < HANTRO_H264_DPB_SIZE; ++i) {
+			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);
+		}
+
+		ctx->h264_dec.dpb_valid = dpb_valid << 16;
+		ctx->h264_dec.dpb_longterm = dpb_longterm << 16;
+	}
+
 	for (i = 0; i < HANTRO_H264_DPB_SIZE; ++i) {
-		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_ACTIVE) {
+			tbl->poc[i * 2] = dpb[i].top_field_order_cnt;
+			tbl->poc[i * 2 + 1] = dpb[i].bottom_field_order_cnt;
+		} else {
+			tbl->poc[i * 2] = 0;
+			tbl->poc[i * 2 + 1] = 0;
+		}
 	}
 
-	tbl->poc[32] = dec_param->top_field_order_cnt;
-	tbl->poc[33] = dec_param->bottom_field_order_cnt;
+	if ((slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC) || !(slices[0].flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)) {
+		if ((slices[0].flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
+			tbl->poc[32] = (slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD) ?
+					dec_param->bottom_field_order_cnt :
+					dec_param->top_field_order_cnt;
+		else
+			tbl->poc[32] = min(dec_param->top_field_order_cnt, dec_param->bottom_field_order_cnt);
+		tbl->poc[33] = 0;
+	} else {
+		tbl->poc[32] = dec_param->top_field_order_cnt;
+		tbl->poc[33] = dec_param->bottom_field_order_cnt;
+	}
 
 	reorder_scaling_list(ctx);
 }
@@ -249,21 +297,6 @@ struct hantro_h264_reflist_builder {
 	u8 num_valid;
 };
 
-static s32 get_poc(enum v4l2_field field, s32 top_field_order_cnt,
-		   s32 bottom_field_order_cnt)
-{
-	switch (field) {
-	case V4L2_FIELD_TOP:
-		return top_field_order_cnt;
-	case V4L2_FIELD_BOTTOM:
-		return bottom_field_order_cnt;
-	default:
-		break;
-	}
-
-	return min(top_field_order_cnt, bottom_field_order_cnt);
-}
-
 static void
 init_reflist_builder(struct hantro_ctx *ctx,
 		     struct hantro_h264_reflist_builder *b)
@@ -273,7 +306,6 @@ init_reflist_builder(struct hantro_ctx *ctx,
 	const struct v4l2_ctrl_h264_sps *sps;
 	struct vb2_v4l2_buffer *buf = hantro_get_dst_buf(ctx);
 	const struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
-	struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q;
 	int cur_frame_num, max_frame_num;
 	unsigned int i;
 
@@ -285,21 +317,15 @@ init_reflist_builder(struct hantro_ctx *ctx,
 
 	memset(b, 0, sizeof(*b));
 	b->dpb = dpb;
-	b->curpoc = get_poc(buf->field, dec_param->top_field_order_cnt,
-			    dec_param->bottom_field_order_cnt);
+	b->curpoc = (slice_params->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD) ?
+		    dec_param->bottom_field_order_cnt :
+		    dec_param->top_field_order_cnt;
 
 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++) {
-		int buf_idx;
-
-		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
+		u32 ref_flag = dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME;
+		if (!ref_flag)
 			continue;
 
-		buf_idx = vb2_find_timestamp(cap_q, dpb[i].reference_ts, 0);
-		if (buf_idx < 0)
-			continue;
-
-		buf = to_vb2_v4l2_buffer(vb2_get_buffer(cap_q, buf_idx));
-
 		/*
 		 * Handle frame_num wraparound as described in section
 		 * '8.2.4.1 Decoding process for picture numbers' of the spec.
@@ -311,8 +337,13 @@ init_reflist_builder(struct hantro_ctx *ctx,
 		else
 			b->frame_nums[i] = dpb[i].frame_num;
 
-		b->pocs[i] = get_poc(buf->field, dpb[i].top_field_order_cnt,
-				     dpb[i].bottom_field_order_cnt);
+		if (ref_flag == V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME)
+			b->pocs[i] = min(dpb[i].bottom_field_order_cnt, dpb[i].top_field_order_cnt);
+		else if (ref_flag == V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM)
+			b->pocs[i] = dpb[i].bottom_field_order_cnt;
+		else if (ref_flag == V4L2_H264_DPB_ENTRY_FLAG_REF_TOP)
+			b->pocs[i] = dpb[i].top_field_order_cnt;
+
 		b->unordered_reflist[b->num_valid] = i;
 		b->num_valid++;
 	}
@@ -466,8 +497,7 @@ build_b_ref_lists(const struct hantro_h264_reflist_builder *builder,
 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)
@@ -481,13 +511,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;
 
 		/*
@@ -498,8 +528,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;
@@ -535,7 +564,11 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
 				   unsigned int dpb_idx)
 {
 	struct v4l2_h264_dpb_entry *dpb = ctx->h264_dec.dpb;
+	const struct v4l2_ctrl_h264_decode_params *dec_param = ctx->h264_dec.ctrls.decode;
+	const struct v4l2_ctrl_h264_slice_params *slices = ctx->h264_dec.ctrls.slices;
 	dma_addr_t dma_addr = 0;
+	s32 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);
@@ -553,7 +586,15 @@ dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx,
 		dma_addr = vb2_dma_contig_plane_dma_addr(buf, 0);
 	}
 
-	return dma_addr;
+	cur_poc = slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD ?
+		  dec_param->bottom_field_order_cnt :
+		  dec_param->top_field_order_cnt;
+	flags = dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE ? 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;
 }
 
 int hantro_h264_dec_prepare_run(struct hantro_ctx *ctx)
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index fa91dd1848b7..fb1451e8a80b 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -86,6 +86,8 @@ struct hantro_h264_dec_hw_ctx {
 	struct v4l2_h264_dpb_entry dpb[HANTRO_H264_DPB_SIZE];
 	struct hantro_h264_dec_reflists reflists;
 	struct hantro_h264_dec_ctrls ctrls;
+	u32 dpb_longterm;
+	u32 dpb_valid;
 };
 
 /**
-- 
2.17.1

Re: [PATCH v2 01/10] media: hantro: Fix H264 max frmsize supported on RK3288

[Boris Brezillon]

On Tue, 29 Oct 2019 01:24:47 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> change frmsize max_width/max_height to match TRM at [1].
> 
> This patch makes it possible to decode the 4096x2304 sample at [2].
> 
> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
> [2] https://4ksamples.com/puppies-bath-in-4k/
> 
> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Boris Brezillon <boris.brezillon@collabora.com>

Let's also add

Cc: <stable@vger.kernel.org>

just in case this patch doesn't make it to 5.4.


> ---
> Changes in v2:
>   - updated commit message with reference to TRM and sample video
> ---
>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> index c0bdd6c02520..f8db6fcaad73 100644
> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
>  		.max_depth = 2,
>  		.frmsize = {
>  			.min_width = 48,
> -			.max_width = 3840,
> +			.max_width = 4096,
>  			.step_width = MB_DIM,
>  			.min_height = 48,
> -			.max_height = 2160,
> +			.max_height = 2304,
>  			.step_height = MB_DIM,

Hans, Mauro, we were intending to have this fix merged in 5.4 or at
the very least be backported to the 5.4 stable branch at some point,
the problem is, this patch is based on media/master which contains the
s/MB_DIM_H264/MB_DIM/ change. I can send a new version based on
media/fixes, but that means Linus will have a conflict when merging the
media 5.5 PR in his tree. Are you fine dealing with this conflict
(letting Linus know about the expected resolution or backmerging the -rc
containing the fix in media/master so that he doesn't even have to deal
with it), or should we just let this patch go in media/master and
backport it later?

>  		},
>  	},

Re: [PATCH v2 02/10] media: hantro: Fix motion vectors usage condition

[Boris Brezillon]

On Tue, 29 Oct 2019 01:24:47 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> From: Francois Buergisser <fbuergisser@chromium.org>
> 
> The setting of the motion vectors usage and the setting of motion
> vectors address are currently done under different conditions.
> 
> When decoding pre-recorded videos, this results of leaving the motion
> vectors address unset, resulting in faulty memory accesses. Fix it
> by using the same condition everywhere, which matches the profiles
> that support motion vectors.
> 
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Boris Brezillon <boris.brezillon@collabora.com>

This fix should apply cleanly to media/fixes. Would be great to have it
queued for 5.4-rc6 so we don't have to backport it manually.

> ---
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 29130946dea4..a1cb18680200 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
>  	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>  		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
>  	reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> -	if (dec_param->nal_ref_idc)
> +	if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
>  		reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
>  
>  	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
> @@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
>  	vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
>  
>  	/* Higher profiles require DMV buffer appended to reference frames. */
> -	if (ctrls->sps->profile_idc > 66) {
> +	if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {
>  		size_t pic_size = ctx->h264_dec.pic_size;
>  		size_t mv_offset = round_up(pic_size, 8);
>  

Re: [PATCH v2 03/10] media: hantro: Fix picture order count table enable

[Boris Brezillon]

On Tue, 29 Oct 2019 01:24:48 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> From: Francois Buergisser <fbuergisser@chromium.org>
> 
> The picture order count table only makes sense for profiles
> higher than Baseline. This is confirmed by the H.264 specification
> (See 8.2.1 Decoding process for picture order count), which
> clarifies how POC are used for features not present in Baseline.
> 
> """
> Picture order counts are used to determine initial picture orderings
> for reference pictures in the decoding of B slices, to represent picture
> order differences between frames or fields for motion vector derivation
> in temporal direct mode, for implicit mode weighted prediction in B slices,
> and for decoder conformance checking.
> """
> 
> As a side note, this change matches various vendors downstream codebases,
> including ChromiumOS and IMX VPU libraries.
> 
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Boris Brezillon <boris.brezillon@collabora.com>

Same as for patch 2, it would be great to have this fix queued for
5.4-rc6 so we don't have to backport it manually.

> ---
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index a1cb18680200..70a6b5b26477 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -34,9 +34,11 @@ static void set_params(struct hantro_ctx *ctx)
>  	reg = G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(0x0);
>  	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>  		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
> -	reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> -	if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
> -		reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
> +	if (sps->profile_idc > 66) {
> +		reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> +		if (dec_param->nal_ref_idc)
> +			reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
> +	}
>  
>  	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
>  	    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD ||

Re: [PATCH v2 06/10] media: hantro: Use capture buffer width and height for H264 decoding

[Boris Brezillon]

On Tue, 29 Oct 2019 01:24:50 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> Calculations for motion vector buffer offset is based on width and height
> from the configured capture format, lets use the same values for macroblock
> width and height hw regs.
> 
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v2:
>   - new patch split from "media: hantro: Fix H264 motion vector buffer offset"
> ---
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 71bf162eaf73..eeed11366135 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
>  
>  	/* Decoder control register 1. */
> -	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
> -	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
> +	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
> +	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |

I'm just curious, do they differ in practice? Also not sure what's been
decided for the "G1 post-proc", but if the dst/capture format res set
by the user is the scaled res (PP can scale IIRC), then you probably
shouldn't use those values here.

>  	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);
>  

Re: [PATCH v2 04/10] media: hantro: Fix H264 motion vector buffer offset

[Boris Brezillon]

On Tue, 29 Oct 2019 01:24:49 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
> macroblock and is laid out in memory as follow:
> 
> +---------------------------+
> | Y-plane   256 bytes x MBs |
> +---------------------------+
> | UV-plane  128 bytes x MBs |
> +---------------------------+
> | MV buffer  64 bytes x MBs |
> +---------------------------+

Would be great to also have this sort of diagram as a comment in the
code.

> 
> The motion vector buffer offset is currently correct for 4:2:0 because the
> extra space for motion vectors is overallocated with an extra 64 bytes x MBs.
> 
> Wrong offset for both destination and motion vector buffer are used
> for the bottom field of field encoded content, wrong offset is
> also used for 4:0:0 (monochrome) content.
> 
> Fix this by setting the motion vector address to the expected 384 bytes x MBs
> offset for 4:2:0 and 256 bytes x MBs offset for 4:0:0 content.
> 
> Also use correct destination and motion vector buffer offset
> for the bottom field of field encoded content.
> 
> While at it also extend the check for 4:0:0 (monochrome) to include an
> additional check for High Profile (100).
> 
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Just 2 nitpicks (see below).

> ---
> Changes in v2:
>   * address remarks from Philipp and Ezequiel
>   - update commit message
>   - rename offset to bytes_per_mb
>   - remove MV_OFFSET macros
>   - move PIC_MB_WIDTH/HEIGHT_P change to separate patch
> ---
>  .../staging/media/hantro/hantro_g1_h264_dec.c | 29 +++++++++++++------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 70a6b5b26477..71bf162eaf73 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -81,7 +81,7 @@ static void set_params(struct hantro_ctx *ctx)
>  		reg |= G1_REG_DEC_CTRL4_CABAC_E;
>  	if (sps->flags & V4L2_H264_SPS_FLAG_DIRECT_8X8_INFERENCE)
>  		reg |= G1_REG_DEC_CTRL4_DIR_8X8_INFER_E;
> -	if (sps->chroma_format_idc == 0)
> +	if (sps->profile_idc >= 100 && sps->chroma_format_idc == 0)
>  		reg |= G1_REG_DEC_CTRL4_BLACKWHITE_E;
>  	if (pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED)
>  		reg |= G1_REG_DEC_CTRL4_WEIGHT_PRED_E;
> @@ -234,6 +234,7 @@ static void set_buffers(struct hantro_ctx *ctx)
>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
>  	struct hantro_dev *vpu = ctx->dev;
>  	dma_addr_t src_dma, dst_dma;
> +	size_t offset = 0;
>  
>  	src_buf = hantro_get_src_buf(ctx);
>  	dst_buf = hantro_get_dst_buf(ctx);
> @@ -244,18 +245,28 @@ static void set_buffers(struct hantro_ctx *ctx)
>  
>  	/* Destination (decoded frame) buffer. */
>  	dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> -	vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
> +	/* Adjust dma addr to start at second line for bottom field */
> +	if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> +		offset = ALIGN(ctx->dst_fmt.width, MB_DIM);
> +	vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DST);
>  
>  	/* Higher profiles require DMV buffer appended to reference frames. */
>  	if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {
> -		size_t pic_size = ctx->h264_dec.pic_size;
> -		size_t mv_offset = round_up(pic_size, 8);
> -
> +		unsigned int bytes_per_mb = 384;

Nitpick: can you add a blank line here?

> +		/* DMV buffer for monochrome start directly after Y-plane */
> +		if (ctrls->sps->profile_idc >= 100 &&
> +		    ctrls->sps->chroma_format_idc == 0)
> +			bytes_per_mb = 256;
> +		offset = bytes_per_mb * MB_WIDTH(ctx->dst_fmt.width) *
> +			 MB_HEIGHT(ctx->dst_fmt.height);
> +
> +		/* DMV buffer is split in two for field encoded frames,

Nitpick again: can you use non-net-style comments.

		/*
		 * Blabla
		 */

> +		 * adjust offset for bottom field
> +		 */
>  		if (ctrls->slices[0].flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> -			mv_offset += 32 * MB_WIDTH(ctx->dst_fmt.width);
> -
> -		vdpu_write_relaxed(vpu, dst_dma + mv_offset,
> -				   G1_REG_ADDR_DIR_MV);
> +			offset += 32 * MB_WIDTH(ctx->dst_fmt.width) *
> +				  MB_HEIGHT(ctx->dst_fmt.height);
> +		vdpu_write_relaxed(vpu, dst_dma + offset, G1_REG_ADDR_DIR_MV);
>  	}
>  
>  	/* Auxiliary buffer prepared in hantro_g1_h264_dec_prepare_table(). */

Re: [PATCH v2 05/10] media: hantro: Reduce H264 extra space for motion vectors

[Boris Brezillon]

On Tue, 29 Oct 2019 01:24:50 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
> macroblock with additional 32 bytes on multi-core variants.
> 
> Memory layout is as follow:
> 
> +---------------------------+
> | Y-plane   256 bytes x MBs |
> +---------------------------+
> | UV-plane  128 bytes x MBs |
> +---------------------------+
> | MV buffer  64 bytes x MBs |
> +---------------------------+
> | MC sync          32 bytes |
> +---------------------------+

As for patch 4, can we point this diagram as a comment in the code too?

> 
> Reduce the extra space allocated now that motion vector buffer offset no
> longer is based on the extra space.
> 
> Only allocate extra space for 64 bytes x MBs of motion vector buffer
> and 32 bytes for multi-core sync.
> 
> Fixes: a9471e25629b ("media: hantro: Add core bits to support H264 decoding")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
> Changes in v2:
>   - updated commit message
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 3dae52abb96c..b3a4368b37de 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -242,12 +242,12 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f,
>  		/*
>  		 * The H264 decoder needs extra space on the output buffers
>  		 * to store motion vectors. This is needed for reference
> -		 * frames.
> +		 * frames. 32 extra bytes is used for multi-core sync.
>  		 */
>  		if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
>  			pix_mp->plane_fmt[0].sizeimage +=
> -				128 * DIV_ROUND_UP(pix_mp->width, 16) *
> -				      DIV_ROUND_UP(pix_mp->height, 16);
> +				64 * MB_WIDTH(pix_mp->width) *
> +				     MB_WIDTH(pix_mp->height) + 32;
>  	} else if (!pix_mp->plane_fmt[0].sizeimage) {
>  		/*
>  		 * For coded formats the application can specify

Re: [PATCH v2 07/10] media: hantro: Remove now unused H264 pic_size

[Boris Brezillon]

On Tue, 29 Oct 2019 01:24:51 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> pic_size in hantro_h264_dec_hw_ctx struct is no longer used,
> lets remove it.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/staging/media/hantro/hantro_h264.c | 5 -----
>  drivers/staging/media/hantro/hantro_hw.h   | 3 ---
>  2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> index 694a330f508e..568640eab3a6 100644
> --- a/drivers/staging/media/hantro/hantro_h264.c
> +++ b/drivers/staging/media/hantro/hantro_h264.c
> @@ -618,7 +618,6 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx)
>  	struct hantro_h264_dec_hw_ctx *h264_dec = &ctx->h264_dec;
>  	struct hantro_aux_buf *priv = &h264_dec->priv;
>  	struct hantro_h264_dec_priv_tbl *tbl;
> -	struct v4l2_pix_format_mplane pix_mp;
>  
>  	priv->cpu = dma_alloc_coherent(vpu->dev, sizeof(*tbl), &priv->dma,
>  				       GFP_KERNEL);
> @@ -629,9 +628,5 @@ int hantro_h264_dec_init(struct hantro_ctx *ctx)
>  	tbl = priv->cpu;
>  	memcpy(tbl->cabac_table, h264_cabac_table, sizeof(tbl->cabac_table));
>  
> -	v4l2_fill_pixfmt_mp(&pix_mp, ctx->dst_fmt.pixelformat,
> -			    ctx->dst_fmt.width, ctx->dst_fmt.height);
> -	h264_dec->pic_size = pix_mp.plane_fmt[0].sizeimage;
> -
>  	return 0;
>  }
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index 69b88f4d3fb3..fa91dd1848b7 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -80,15 +80,12 @@ struct hantro_h264_dec_reflists {
>   * @dpb:	DPB
>   * @reflists:	P/B0/B1 reflists
>   * @ctrls:	V4L2 controls attached to a run
> - * @pic_size:	Size in bytes of decoded picture, this is needed
> - *		to pass the location of motion vectors.
>   */
>  struct hantro_h264_dec_hw_ctx {
>  	struct hantro_aux_buf priv;
>  	struct v4l2_h264_dpb_entry dpb[HANTRO_H264_DPB_SIZE];
>  	struct hantro_h264_dec_reflists reflists;
>  	struct hantro_h264_dec_ctrls ctrls;
> -	size_t pic_size;
>  };
>  
>  /**

Re: [PATCH v2 08/10] media: hantro: Set H264 FIELDPIC_FLAG_E flag correctly

[Boris Brezillon]

On Tue, 29 Oct 2019 01:26:00 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> The FIELDPIC_FLAG_E bit should be set when field_pic_flag exists in stream,
> it is currently set based on field_pic_flag of current frame.
> The PIC_FIELDMODE_E bit is correctly set based on the field_pic_flag.
> 
> Fix this by setting the FIELDPIC_FLAG_E bit when frame_mbs_only is not set.
> 
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index eeed11366135..c07da0ee4973 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -63,7 +63,7 @@ static void set_params(struct hantro_ctx *ctx)
>  	/* always use the matrix sent from userspace */
>  	reg |= G1_REG_DEC_CTRL2_TYPE1_QUANT_E;
>  
> -	if (slices[0].flags &  V4L2_H264_SLICE_FLAG_FIELD_PIC)
> +	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
>  		reg |= G1_REG_DEC_CTRL2_FIELDPIC_FLAG_E;
>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL2);
>  

Re: [PATCH v2 06/10] media: hantro: Use capture buffer width and height for H264 decoding

[Jonas Karlman]

On 2019-10-31 10:21, Boris Brezillon wrote:
> On Tue, 29 Oct 2019 01:24:50 +0000
> Jonas Karlman <jonas@kwiboo.se> wrote:
>
>> Calculations for motion vector buffer offset is based on width and height
>> from the configured capture format, lets use the same values for macroblock
>> width and height hw regs.
>>
>> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> Changes in v2:
>>   - new patch split from "media: hantro: Fix H264 motion vector buffer offset"
>> ---
>>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>> index 71bf162eaf73..eeed11366135 100644
>> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>> @@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
>>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
>>  
>>  	/* Decoder control register 1. */
>> -	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
>> -	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
>> +	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
>> +	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |
> I'm just curious, do they differ in practice? Also not sure what's been
> decided for the "G1 post-proc", but if the dst/capture format res set
> by the user is the scaled res (PP can scale IIRC), then you probably
> shouldn't use those values here.

You are correct, I wanted to use the same source for both size and offsets, looking at this again
both here and where is it used for offsets this probably need to change.

Do you think we can use src_fmt.width/height for size and offsets? It looks like that is what cedrus is using.

Regards,
Jonas

>
>>  	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
>>  	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);
>>  
>>

Re: [RFC v2 09/10] media: uapi: h264: Add DPB entry field reference flags

[Boris Brezillon]

On Tue, 29 Oct 2019 01:26:01 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> Add DPB entry flags to help indicate when a reference frame is a field picture
> and how the DPB entry is referenced, top or bottom field or full frame.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++++++
>  include/media/h264-ctrls.h                       |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> index 28313c0f4e7c..d472a54d1c4d 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> @@ -2028,6 +2028,18 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
>        - 0x00000004
>        - The DPB entry is a long term reference frame
> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE``
> +      - 0x00000008
> +      - The DPB entry is a field picture
> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_TOP``
> +      - 0x00000010
> +      - The DPB entry is a top field reference
> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM``
> +      - 0x00000020
> +      - The DPB entry is a bottom field reference
> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME``
> +      - 0x00000030
> +      - The DPB entry is a reference frame
>  
>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
>      Specifies the decoding mode to use. Currently exposes slice-based and
> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> index e877bf1d537c..76020ebd1e6c 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/media/h264-ctrls.h
> @@ -185,6 +185,10 @@ struct v4l2_ctrl_h264_slice_params {
>  #define V4L2_H264_DPB_ENTRY_FLAG_VALID		0x01
>  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
>  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
> +#define V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE	0x08
> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_TOP	0x10
> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM	0x20
> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME	0x30

I don't remember all the details, but do we really need 3 flags?
Maybe I'm wrong, but it looks like the following combination doesn't
make sense:

- FIELD_PICTURE + REF_FRAME: if it's a full frame ref it should
  contain both top and bottom fields right, so it's no longer a
  FIELD_PICTURE, is it?

Can't we just have 2 flags?

FIELD_PICTURE		0x08
FIELD_REF_TOP		0x10 (meaning that FIELD_REF_BOTTOM is
			      0x00)

and then have the following combinations:

top field ref => FIELD_PICTURE | FIELD_REF_TOP
bottom field ref => FIELD_PICTURE
full frame ref => 0x0

>  
>  struct v4l2_h264_dpb_entry {
>  	__u64 reference_ts;

Re: [PATCH v2 01/10] media: hantro: Fix H264 max frmsize supported on RK3288

[Hans Verkuil]

On 10/31/19 9:52 AM, Boris Brezillon wrote:
> On Tue, 29 Oct 2019 01:24:47 +0000
> Jonas Karlman <jonas@kwiboo.se> wrote:
> 
>> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
>> change frmsize max_width/max_height to match TRM at [1].
>>
>> This patch makes it possible to decode the 4096x2304 sample at [2].
>>
>> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
>> [2] https://4ksamples.com/puppies-bath-in-4k/
>>
>> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Tested-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Let's also add
> 
> Cc: <stable@vger.kernel.org>
> 
> just in case this patch doesn't make it to 5.4.
> 
> 
>> ---
>> Changes in v2:
>>   - updated commit message with reference to TRM and sample video
>> ---
>>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>> index c0bdd6c02520..f8db6fcaad73 100644
>> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
>> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
>>  		.max_depth = 2,
>>  		.frmsize = {
>>  			.min_width = 48,
>> -			.max_width = 3840,
>> +			.max_width = 4096,
>>  			.step_width = MB_DIM,
>>  			.min_height = 48,
>> -			.max_height = 2160,
>> +			.max_height = 2304,
>>  			.step_height = MB_DIM,
> 
> Hans, Mauro, we were intending to have this fix merged in 5.4 or at
> the very least be backported to the 5.4 stable branch at some point,
> the problem is, this patch is based on media/master which contains the
> s/MB_DIM_H264/MB_DIM/ change. I can send a new version based on
> media/fixes, but that means Linus will have a conflict when merging the
> media 5.5 PR in his tree. Are you fine dealing with this conflict
> (letting Linus know about the expected resolution or backmerging the -rc
> containing the fix in media/master so that he doesn't even have to deal
> with it), or should we just let this patch go in media/master and
> backport it later?

Backport it later once it is merged in mainline.

This patch doesn't fix a bug, it is really an enhancement, so I think this
can safely be delayed.

Regards,

	Hans

> 
>>  		},
>>  	},
> 

Re: [PATCH v2 02/10] media: hantro: Fix motion vectors usage condition

[Ezequiel Garcia]

On Tue, 29 Oct 2019 at 02:25, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> From: Francois Buergisser <fbuergisser@chromium.org>
>
> The setting of the motion vectors usage and the setting of motion
> vectors address are currently done under different conditions.
>
> When decoding pre-recorded videos, this results of leaving the motion
> vectors address unset, resulting in faulty memory accesses. Fix it
> by using the same condition everywhere, which matches the profiles
> that support motion vectors.
>
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 29130946dea4..a1cb18680200 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
>         if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>                 reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
>         reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> -       if (dec_param->nal_ref_idc)
> +       if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
>                 reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
>
>         if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
> @@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
>         vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
>
>         /* Higher profiles require DMV buffer appended to reference frames. */
> -       if (ctrls->sps->profile_idc > 66) {
> +       if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {

How about a one-line function (purposely not a macro,
to have type-checking) ? I think this should emphasize the fact that
the condition
needs to be the same.

Thanks,
Ezequiel

>                 size_t pic_size = ctx->h264_dec.pic_size;
>                 size_t mv_offset = round_up(pic_size, 8);
>
> --
> 2.17.1
>

Re: [PATCH v2 02/10] media: hantro: Fix motion vectors usage condition

[Ezequiel Garcia]

On Sun, 3 Nov 2019 at 00:09, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> On Tue, 29 Oct 2019 at 02:25, Jonas Karlman <jonas@kwiboo.se> wrote:
> >
> > From: Francois Buergisser <fbuergisser@chromium.org>
> >
> > The setting of the motion vectors usage and the setting of motion
> > vectors address are currently done under different conditions.
> >
> > When decoding pre-recorded videos, this results of leaving the motion
> > vectors address unset, resulting in faulty memory accesses. Fix it
> > by using the same condition everywhere, which matches the profiles
> > that support motion vectors.
> >
> > Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> > Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> >  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > index 29130946dea4..a1cb18680200 100644
> > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
> >         if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
> >                 reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
> >         reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> > -       if (dec_param->nal_ref_idc)
> > +       if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
> >                 reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
> >
> >         if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
> > @@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx)
> >         vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
> >
> >         /* Higher profiles require DMV buffer appended to reference frames. */
> > -       if (ctrls->sps->profile_idc > 66) {
> > +       if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) {
>
> How about a one-line function (purposely not a macro,
> to have type-checking) ? I think this should emphasize the fact that
> the condition
> needs to be the same.
>

Oops, just saw the next patch. Nevermind.

Re: [PATCH v2 01/10] media: hantro: Fix H264 max frmsize supported on RK3288

[Boris Brezillon]

On Fri, 1 Nov 2019 09:36:55 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> On 10/31/19 9:52 AM, Boris Brezillon wrote:
> > On Tue, 29 Oct 2019 01:24:47 +0000
> > Jonas Karlman <jonas@kwiboo.se> wrote:
> >   
> >> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> >> change frmsize max_width/max_height to match TRM at [1].
> >>
> >> This patch makes it possible to decode the 4096x2304 sample at [2].
> >>
> >> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
> >> [2] https://4ksamples.com/puppies-bath-in-4k/
> >>
> >> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>  
> > 
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Tested-by: Boris Brezillon <boris.brezillon@collabora.com>
> > 
> > Let's also add
> > 
> > Cc: <stable@vger.kernel.org>
> > 
> > just in case this patch doesn't make it to 5.4.
> > 
> >   
> >> ---
> >> Changes in v2:
> >>   - updated commit message with reference to TRM and sample video
> >> ---
> >>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >> index c0bdd6c02520..f8db6fcaad73 100644
> >> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> >>  		.max_depth = 2,
> >>  		.frmsize = {
> >>  			.min_width = 48,
> >> -			.max_width = 3840,
> >> +			.max_width = 4096,
> >>  			.step_width = MB_DIM,
> >>  			.min_height = 48,
> >> -			.max_height = 2160,
> >> +			.max_height = 2304,
> >>  			.step_height = MB_DIM,  
> > 
> > Hans, Mauro, we were intending to have this fix merged in 5.4 or at
> > the very least be backported to the 5.4 stable branch at some point,
> > the problem is, this patch is based on media/master which contains the
> > s/MB_DIM_H264/MB_DIM/ change. I can send a new version based on
> > media/fixes, but that means Linus will have a conflict when merging the
> > media 5.5 PR in his tree. Are you fine dealing with this conflict
> > (letting Linus know about the expected resolution or backmerging the -rc
> > containing the fix in media/master so that he doesn't even have to deal
> > with it), or should we just let this patch go in media/master and
> > backport it later?  
> 
> Backport it later once it is merged in mainline.
> 
> This patch doesn't fix a bug, it is really an enhancement, so I think this
> can safely be delayed.

Okay, let's make things simple. Can we have patches 1 to 3 applied to
media/master? I'll take care of backporting those patches to 5.4 once
5.5-rc1 is out.

Thanks,

Boris

Re: [RFC v2 09/10] media: uapi: h264: Add DPB entry field reference flags

[Jonas Karlman]

On 2019-10-31 11:20, Boris Brezillon wrote:
> On Tue, 29 Oct 2019 01:26:01 +0000
> Jonas Karlman <jonas@kwiboo.se> wrote:
>
>> Add DPB entry flags to help indicate when a reference frame is a field picture
>> and how the DPB entry is referenced, top or bottom field or full frame.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++++++
>>  include/media/h264-ctrls.h                       |  4 ++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>> index 28313c0f4e7c..d472a54d1c4d 100644
>> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>> @@ -2028,6 +2028,18 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
>>        - 0x00000004
>>        - The DPB entry is a long term reference frame
>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE``
>> +      - 0x00000008
>> +      - The DPB entry is a field picture
>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_TOP``
>> +      - 0x00000010
>> +      - The DPB entry is a top field reference
>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM``
>> +      - 0x00000020
>> +      - The DPB entry is a bottom field reference
>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME``
>> +      - 0x00000030
>> +      - The DPB entry is a reference frame
>>  
>>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
>>      Specifies the decoding mode to use. Currently exposes slice-based and
>> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
>> index e877bf1d537c..76020ebd1e6c 100644
>> --- a/include/media/h264-ctrls.h
>> +++ b/include/media/h264-ctrls.h
>> @@ -185,6 +185,10 @@ struct v4l2_ctrl_h264_slice_params {
>>  #define V4L2_H264_DPB_ENTRY_FLAG_VALID		0x01
>>  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
>>  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
>> +#define V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE	0x08
>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_TOP	0x10
>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM	0x20
>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME	0x30
> I don't remember all the details, but do we really need 3 flags?
> Maybe I'm wrong, but it looks like the following combination doesn't
> make sense:
>
> - FIELD_PICTURE + REF_FRAME: if it's a full frame ref it should
>   contain both top and bottom fields right, so it's no longer a
>   FIELD_PICTURE, is it?
>
> Can't we just have 2 flags?
>
> FIELD_PICTURE		0x08
> FIELD_REF_TOP		0x10 (meaning that FIELD_REF_BOTTOM is
> 			      0x00)
>
> and then have the following combinations:
>
> top field ref => FIELD_PICTURE | FIELD_REF_TOP
> bottom field ref => FIELD_PICTURE
> full frame ref => 0x0

I am not sure and will need to look closer at spec and what ffmpeg is doing.
These flags was mostly inspired by the information ffmpeg stores in
H264Picture->reference and H264Picture->field_picture.

I also believe that the new FLAG_REF_TOP/BOTTOM may make FLAG_ACTIVE obsolete.

active => flags & FLAG_REF_FRAME

Hopefully I will have some time to rework and respin this at the end of next week.

Regards,
Jonas

>
>>  
>>  struct v4l2_h264_dpb_entry {
>>  	__u64 reference_ts;

Re: [RFC v2 09/10] media: uapi: h264: Add DPB entry field reference flags

[Boris Brezillon]

On Fri, 15 Nov 2019 06:26:54 +0000
Jonas Karlman <jonas@kwiboo.se> wrote:

> On 2019-10-31 11:20, Boris Brezillon wrote:
> > On Tue, 29 Oct 2019 01:26:01 +0000
> > Jonas Karlman <jonas@kwiboo.se> wrote:
> >  
> >> Add DPB entry flags to help indicate when a reference frame is a field picture
> >> and how the DPB entry is referenced, top or bottom field or full frame.
> >>
> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >> ---
> >>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++++++
> >>  include/media/h264-ctrls.h                       |  4 ++++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> >> index 28313c0f4e7c..d472a54d1c4d 100644
> >> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> >> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> >> @@ -2028,6 +2028,18 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >>      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
> >>        - 0x00000004
> >>        - The DPB entry is a long term reference frame
> >> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE``
> >> +      - 0x00000008
> >> +      - The DPB entry is a field picture
> >> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_TOP``
> >> +      - 0x00000010
> >> +      - The DPB entry is a top field reference
> >> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM``
> >> +      - 0x00000020
> >> +      - The DPB entry is a bottom field reference
> >> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME``
> >> +      - 0x00000030
> >> +      - The DPB entry is a reference frame
> >>  
> >>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
> >>      Specifies the decoding mode to use. Currently exposes slice-based and
> >> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> >> index e877bf1d537c..76020ebd1e6c 100644
> >> --- a/include/media/h264-ctrls.h
> >> +++ b/include/media/h264-ctrls.h
> >> @@ -185,6 +185,10 @@ struct v4l2_ctrl_h264_slice_params {
> >>  #define V4L2_H264_DPB_ENTRY_FLAG_VALID		0x01
> >>  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
> >>  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
> >> +#define V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE	0x08
> >> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_TOP	0x10
> >> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM	0x20
> >> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME	0x30  
> > I don't remember all the details, but do we really need 3 flags?
> > Maybe I'm wrong, but it looks like the following combination doesn't
> > make sense:
> >
> > - FIELD_PICTURE + REF_FRAME: if it's a full frame ref it should
> >   contain both top and bottom fields right, so it's no longer a
> >   FIELD_PICTURE, is it?
> >
> > Can't we just have 2 flags?
> >
> > FIELD_PICTURE		0x08
> > FIELD_REF_TOP		0x10 (meaning that FIELD_REF_BOTTOM is
> > 			      0x00)
> >
> > and then have the following combinations:
> >
> > top field ref => FIELD_PICTURE | FIELD_REF_TOP
> > bottom field ref => FIELD_PICTURE
> > full frame ref => 0x0  
> 
> I am not sure and will need to look closer at spec and what ffmpeg is doing.
> These flags was mostly inspired by the information ffmpeg stores in
> H264Picture->reference and H264Picture->field_picture.
> 
> I also believe that the new FLAG_REF_TOP/BOTTOM may make FLAG_ACTIVE obsolete.
> 
> active => flags & FLAG_REF_FRAME

Can't we keep the ACTIVE flag and drop the REF_FRAME one then? AFAIU,
all we need to know in addition to what we already have is:

* is the reference a full-frame or a field?
* if it is a field, which one (top or bottom)?

Hence my suggestion to keep only 2 flags.

Re: [RFC v2 09/10] media: uapi: h264: Add DPB entry field reference flags

[Jonas Karlman]

On 2019-11-15 08:45, Boris Brezillon wrote:
> On Fri, 15 Nov 2019 06:26:54 +0000
> Jonas Karlman <jonas@kwiboo.se> wrote:
>
>> On 2019-10-31 11:20, Boris Brezillon wrote:
>>> On Tue, 29 Oct 2019 01:26:01 +0000
>>> Jonas Karlman <jonas@kwiboo.se> wrote:
>>>  
>>>> Add DPB entry flags to help indicate when a reference frame is a field picture
>>>> and how the DPB entry is referenced, top or bottom field or full frame.
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++++++
>>>>  include/media/h264-ctrls.h                       |  4 ++++
>>>>  2 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>>> index 28313c0f4e7c..d472a54d1c4d 100644
>>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>>> @@ -2028,6 +2028,18 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>>      * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
>>>>        - 0x00000004
>>>>        - The DPB entry is a long term reference frame
>>>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE``
>>>> +      - 0x00000008
>>>> +      - The DPB entry is a field picture
>>>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_TOP``
>>>> +      - 0x00000010
>>>> +      - The DPB entry is a top field reference
>>>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM``
>>>> +      - 0x00000020
>>>> +      - The DPB entry is a bottom field reference
>>>> +    * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME``
>>>> +      - 0x00000030
>>>> +      - The DPB entry is a reference frame
>>>>  
>>>>  ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
>>>>      Specifies the decoding mode to use. Currently exposes slice-based and
>>>> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
>>>> index e877bf1d537c..76020ebd1e6c 100644
>>>> --- a/include/media/h264-ctrls.h
>>>> +++ b/include/media/h264-ctrls.h
>>>> @@ -185,6 +185,10 @@ struct v4l2_ctrl_h264_slice_params {
>>>>  #define V4L2_H264_DPB_ENTRY_FLAG_VALID		0x01
>>>>  #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE		0x02
>>>>  #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM	0x04
>>>> +#define V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE	0x08
>>>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_TOP	0x10
>>>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM	0x20
>>>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME	0x30  
>>> I don't remember all the details, but do we really need 3 flags?
>>> Maybe I'm wrong, but it looks like the following combination doesn't
>>> make sense:
>>>
>>> - FIELD_PICTURE + REF_FRAME: if it's a full frame ref it should
>>>   contain both top and bottom fields right, so it's no longer a
>>>   FIELD_PICTURE, is it?
>>>
>>> Can't we just have 2 flags?
>>>
>>> FIELD_PICTURE		0x08
>>> FIELD_REF_TOP		0x10 (meaning that FIELD_REF_BOTTOM is
>>> 			      0x00)
>>>
>>> and then have the following combinations:
>>>
>>> top field ref => FIELD_PICTURE | FIELD_REF_TOP
>>> bottom field ref => FIELD_PICTURE
>>> full frame ref => 0x0  
>> I am not sure and will need to look closer at spec and what ffmpeg is doing.
>> These flags was mostly inspired by the information ffmpeg stores in
>> H264Picture->reference and H264Picture->field_picture.
>>
>> I also believe that the new FLAG_REF_TOP/BOTTOM may make FLAG_ACTIVE obsolete.
>>
>> active => flags & FLAG_REF_FRAME
> Can't we keep the ACTIVE flag and drop the REF_FRAME one then? AFAIU,
> all we need to know in addition to what we already have is:
>
> * is the reference a full-frame or a field?
> * if it is a field, which one (top or bottom)?
>
> Hence my suggestion to keep only 2 flags.

I think we also need to know if the reference frame was a field or a full-frame picture
not just how it is referenced, I think there was an issue when I originally tried a similar
approach as you suggested, but it was too long time ago as I seem to have forgotten the details.

Will do more testing and see if I can refresh my memory.

Best regards,
Jonas