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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

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

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
>

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
>

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
>

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
>

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 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 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
>

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
>

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
>

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 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 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 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 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
>>

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 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
> > 

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 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 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
> > 

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
> > 

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

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 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
> >