[PATCH v2 0/6] CODA timeout fix & assorted changes

[PATCH v2 0/6] CODA timeout fix & assorted changes

[Ezequiel Garcia]

Hi all,

The main motivation for this series is to address a PIC_RUN
timeout, which we managed to link with a hardware bitstream
buffer underrun condition.

Upon further investigation we discovered that the underrun
was produced by a subtle issue in the way buffer_meta's were
being tracked.

The issue is fixed by patch "5/6 coda: coda_buffer_meta housekeeping fix".

Patches 1 to 4 are mostly cleanups and minor cosmetic changes.

Finally, while testing with corrupted bitstreams we realized
the driver was logging too verbosely, so patch 6 addresses
this by introducing a private control to read an macroblock-error
counter.

These patches have been tested against media's upstream
and v5.4-based, on i.MX6 (Wandboard) with H.264 and MPEG-2
streams.

In particular, this series manages to fix the PIC_RUN
timeout reported by Benjamin Bara [1].

I believe this timeout can be systematically reproduced with
a video containing small black frames, which can be generated with:

gst-launch-1.0 videotestsrc pattern=black num-buffers=300 ! \
video/x-raw,format=I420,width=176,height=120 ! avenc_mpeg2video ! \
mpegvideoparse ! mpegtsmux ! filesink location=black-qcif-10s.ts

Reviews and feedback are appreciated, as always.

[1] https://lkml.org/lkml/2020/8/21/495

Changelog
---------

v2:
* Keep the error MB message, but move it to coda_dbg(1, ctx).
* Add per-device rate limitting for the error MB message.
* Rename V4L2_CID_CODA_ERR_MB description.
* s/__coda_decoder_drop_used_metas/coda_decoder_drop_used_metas

Ezequiel Garcia (6):
  coda: Remove redundant ctx->initialized setting
  coda: Simplify H.264 small buffer padding logic
  coda: Clarify device registered log
  coda: Clarify interrupt registered name
  coda: coda_buffer_meta housekeeping fix
  coda: Add a V4L2 user for control error macroblocks count

 MAINTAINERS                               |  1 +
 drivers/media/platform/coda/coda-bit.c    | 72 ++++++++++++++++-------
 drivers/media/platform/coda/coda-common.c | 57 +++++++++++++++---
 drivers/media/platform/coda/coda.h        |  5 ++
 include/media/drv-intf/coda.h             | 13 ++++
 include/uapi/linux/v4l2-controls.h        |  4 ++
 6 files changed, 122 insertions(+), 30 deletions(-)
 create mode 100644 include/media/drv-intf/coda.h

-- 
2.27.0

[PATCH v2 1/6] coda: Remove redundant ctx->initialized setting

[Ezequiel Garcia]

The ctx->initialized flag is set in __coda_decoder_seq_init,
so it's redundant to set it in coda_dec_seq_init_work.
Remove the redundant set, which allows to simplify the
implementation quite a bit.

This change shouldn't affect functionality as it's just
cosmetics.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-bit.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index bf75927bac4e..aa0a47c34413 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2005,21 +2005,13 @@ static void coda_dec_seq_init_work(struct work_struct *work)
 	struct coda_ctx *ctx = container_of(work,
 					    struct coda_ctx, seq_init_work);
 	struct coda_dev *dev = ctx->dev;
-	int ret;
 
 	mutex_lock(&ctx->buffer_mutex);
 	mutex_lock(&dev->coda_mutex);
 
-	if (ctx->initialized == 1)
-		goto out;
-
-	ret = __coda_decoder_seq_init(ctx);
-	if (ret < 0)
-		goto out;
-
-	ctx->initialized = 1;
+	if (!ctx->initialized)
+		__coda_decoder_seq_init(ctx);
 
-out:
 	mutex_unlock(&dev->coda_mutex);
 	mutex_unlock(&ctx->buffer_mutex);
 }
-- 
2.27.0

[PATCH v2 2/6] coda: Simplify H.264 small buffer padding logic

[Ezequiel Garcia]

The H.264 small buffer padding is done under
the (ctx->qsequence == 0 && payload < 512) condition.

Given this is the exact same condition immediately
above, we can move it right there, making the code
slightly clearer.

This change shouldn't affect functionality as it's just
cosmetics.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-bit.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index aa0a47c34413..659fcf77b0ed 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -293,12 +293,11 @@ static bool coda_bitstream_try_queue(struct coda_ctx *ctx,
 			coda_dbg(1, ctx,
 				 "could not parse header, sequence initialization might fail\n");
 		}
-	}
 
-	/* Add padding before the first buffer, if it is too small */
-	if (ctx->qsequence == 0 && payload < 512 &&
-	    ctx->codec->src_fourcc == V4L2_PIX_FMT_H264)
-		coda_h264_bitstream_pad(ctx, 512 - payload);
+		/* Add padding before the first buffer, if it is too small */
+		if (ctx->codec->src_fourcc == V4L2_PIX_FMT_H264)
+			coda_h264_bitstream_pad(ctx, 512 - payload);
+	}
 
 	ret = coda_bitstream_queue(ctx, vaddr, payload);
 	if (ret < 0) {
-- 
2.27.0

[PATCH v2 3/6] coda: Clarify device registered log

[Ezequiel Garcia]

Instead of printing just the device type, let's use
the device name, which makes the message more useful.

With this commit, the messages shown when the driver
is registered are:

coda 2040000.vpu: Firmware code revision: 570363
coda 2040000.vpu: Initialized CODA960.
coda 2040000.vpu: Firmware version: 3.1.1
coda 2040000.vpu: coda-jpeg-encoder registered as video0
coda 2040000.vpu: coda-jpeg-decoder registered as video1
coda 2040000.vpu: coda-video-encoder registered as video2
coda 2040000.vpu: coda-video-decoder registered as video3

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 87a2c706f747..a72ea4bb37d7 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -172,7 +172,7 @@ struct coda_video_device {
 };
 
 static const struct coda_video_device coda_bit_encoder = {
-	.name = "coda-encoder",
+	.name = "coda-video-encoder",
 	.type = CODA_INST_ENCODER,
 	.ops = &coda_bit_encode_ops,
 	.src_formats = {
@@ -202,7 +202,7 @@ static const struct coda_video_device coda_bit_jpeg_encoder = {
 };
 
 static const struct coda_video_device coda_bit_decoder = {
-	.name = "coda-decoder",
+	.name = "coda-video-decoder",
 	.type = CODA_INST_DECODER,
 	.ops = &coda_bit_decode_ops,
 	.src_formats = {
@@ -2851,12 +2851,12 @@ static int coda_hw_init(struct coda_dev *dev)
 static int coda_register_device(struct coda_dev *dev, int i)
 {
 	struct video_device *vfd = &dev->vfd[i];
-	enum coda_inst_type type;
+	const char *name;
 	int ret;
 
 	if (i >= dev->devtype->num_vdevs)
 		return -EINVAL;
-	type = dev->devtype->vdevs[i]->type;
+	name = dev->devtype->vdevs[i]->name;
 
 	strscpy(vfd->name, dev->devtype->vdevs[i]->name, sizeof(vfd->name));
 	vfd->fops	= &coda_fops;
@@ -2876,8 +2876,7 @@ static int coda_register_device(struct coda_dev *dev, int i)
 	ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0);
 	if (!ret)
 		v4l2_info(&dev->v4l2_dev, "%s registered as %s\n",
-			  type == CODA_INST_ENCODER ? "encoder" : "decoder",
-			  video_device_node_name(vfd));
+			  name, video_device_node_name(vfd));
 	return ret;
 }
 
-- 
2.27.0

[PATCH v2 4/6] coda: Clarify interrupt registered name

[Ezequiel Garcia]

Make interrupt naming more consistent by using a common
pattern for video and jpeg interrupts.

With this commit, interrupts are shown as:

29:          0          0       GPC  12 Level     coda-video
30:          0          0       GPC   3 Level     coda-jpeg

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index a72ea4bb37d7..487dd653b24a 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -3153,7 +3153,7 @@ static int coda_probe(struct platform_device *pdev)
 		return irq;
 
 	ret = devm_request_irq(&pdev->dev, irq, coda_irq_handler, 0,
-			       dev_name(&pdev->dev), dev);
+			       CODA_NAME "-video", dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to request irq: %d\n", ret);
 		return ret;
@@ -3167,7 +3167,7 @@ static int coda_probe(struct platform_device *pdev)
 
 		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
 						coda9_jpeg_irq_handler,
-						IRQF_ONESHOT, CODA_NAME " jpeg",
+						IRQF_ONESHOT, CODA_NAME "-jpeg",
 						dev);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to request jpeg irq\n");
-- 
2.27.0

[PATCH v2 5/6] coda: coda_buffer_meta housekeeping fix

[Ezequiel Garcia]

It's possible that the VPU was initialized using just one buffer,
containing only codec headers.

In this case, right after the initialization and after updating
the FIFO read pointer, we need to iterate through all the coda_buffer_meta
and release any metas that have been already used by the VPU.

This issue is affecting indirectly the bitstream buffer fill
threshold, which depends on the meta end position of the first
queued meta, which is passed to coda_bitstream_can_fetch_past().

Without this fix, it's possible that for certain videos, the
bitstream buffer level is not filled properly, resulting in a PIC_RUN
timeout.

Reported-by: Benjamin Bara <benjamin.bara@skidata.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-bit.c | 42 +++++++++++++++++++++++---
 drivers/media/platform/coda/coda.h     |  1 +
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 659fcf77b0ed..919b36d753ec 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -1836,6 +1836,29 @@ static bool coda_reorder_enable(struct coda_ctx *ctx)
 	return profile > V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE;
 }
 
+static void coda_decoder_drop_used_metas(struct coda_ctx *ctx)
+{
+	struct coda_buffer_meta *meta, *tmp;
+
+	/*
+	 * All metas that end at or before the RD pointer (fifo out),
+	 * are now consumed by the VPU and should be released.
+	 */
+	spin_lock(&ctx->buffer_meta_lock);
+	list_for_each_entry_safe(meta, tmp, &ctx->buffer_meta_list, list) {
+		if (ctx->bitstream_fifo.kfifo.out >= meta->end) {
+			coda_dbg(2, ctx, "releasing meta: seq=%d start=%d end=%d\n",
+				 meta->sequence, meta->start, meta->end);
+
+			list_del(&meta->list);
+			ctx->num_metas--;
+			ctx->first_frame_sequence++;
+			kfree(meta);
+		}
+	}
+	spin_unlock(&ctx->buffer_meta_lock);
+}
+
 static int __coda_decoder_seq_init(struct coda_ctx *ctx)
 {
 	struct coda_q_data *q_data_src, *q_data_dst;
@@ -1921,10 +1944,17 @@ static int __coda_decoder_seq_init(struct coda_ctx *ctx)
 	}
 	ctx->sequence_offset = ~0U;
 	ctx->initialized = 1;
+	ctx->first_frame_sequence = 0;
 
 	/* Update kfifo out pointer from coda bitstream read pointer */
 	coda_kfifo_sync_from_device(ctx);
 
+	/*
+	 * After updating the read pointer, we need to check if
+	 * any metas are consumed and should be released.
+	 */
+	coda_decoder_drop_used_metas(ctx);
+
 	if (coda_read(dev, CODA_RET_DEC_SEQ_SUCCESS) == 0) {
 		v4l2_err(&dev->v4l2_dev,
 			"CODA_COMMAND_SEQ_INIT failed, error code = 0x%x\n",
@@ -2395,12 +2425,16 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 		v4l2_err(&dev->v4l2_dev,
 			 "decoded frame index out of range: %d\n", decoded_idx);
 	} else {
+		int sequence;
+
 		decoded_frame = &ctx->internal_frames[decoded_idx];
 
 		val = coda_read(dev, CODA_RET_DEC_PIC_FRAME_NUM);
 		if (ctx->sequence_offset == -1)
 			ctx->sequence_offset = val;
-		val -= ctx->sequence_offset;
+
+		sequence = val + ctx->first_frame_sequence
+			       - ctx->sequence_offset;
 		spin_lock(&ctx->buffer_meta_lock);
 		if (!list_empty(&ctx->buffer_meta_list)) {
 			meta = list_first_entry(&ctx->buffer_meta_list,
@@ -2415,10 +2449,10 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 			 * should be enough to detect most errors and saves us
 			 * from doing different things based on the format.
 			 */
-			if ((val & 0xffff) != (meta->sequence & 0xffff)) {
+			if ((sequence & 0xffff) != (meta->sequence & 0xffff)) {
 				v4l2_err(&dev->v4l2_dev,
 					 "sequence number mismatch (%d(%d) != %d)\n",
-					 val, ctx->sequence_offset,
+					 sequence, ctx->sequence_offset,
 					 meta->sequence);
 			}
 			decoded_frame->meta = *meta;
@@ -2428,7 +2462,7 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 			v4l2_err(&dev->v4l2_dev, "empty timestamp list!\n");
 			memset(&decoded_frame->meta, 0,
 			       sizeof(struct coda_buffer_meta));
-			decoded_frame->meta.sequence = val;
+			decoded_frame->meta.sequence = sequence;
 			decoded_frame->meta.last = false;
 			ctx->sequence_offset++;
 		}
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index b81f3aca9209..e53f7a65d532 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -259,6 +259,7 @@ struct coda_ctx {
 	struct list_head		buffer_meta_list;
 	spinlock_t			buffer_meta_lock;
 	int				num_metas;
+	unsigned int			first_frame_sequence;
 	struct coda_aux_buf		workbuf;
 	int				num_internal_frames;
 	int				idx;
-- 
2.27.0

[PATCH v2 6/6] coda: Add a V4L2 user for control error macroblocks count

[Ezequiel Garcia]

To avoid potentially overflowing the kernel logs in the case
of corrupted streams, this commit replaces an error log with
a per-stream counter to be read through a driver-specific
control.

Applications can read the per-stream accumulated
error macroblocks count.

The error message is moved to the driver-specific debug log,
and rate-limitting is added to make sure it doesn't
spam the log.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 MAINTAINERS                               |  1 +
 drivers/media/platform/coda/coda-bit.c    |  9 +++--
 drivers/media/platform/coda/coda-common.c | 42 +++++++++++++++++++++++
 drivers/media/platform/coda/coda.h        |  4 +++
 include/media/drv-intf/coda.h             | 13 +++++++
 include/uapi/linux/v4l2-controls.h        |  4 +++
 6 files changed, 70 insertions(+), 3 deletions(-)
 create mode 100644 include/media/drv-intf/coda.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ba5eb1dff9c2..4c7a59a4dda3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4334,6 +4334,7 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/media/coda.txt
 F:	drivers/media/platform/coda/
+F:	include/media/drv-intf/coda.h
 
 CODE OF CONDUCT
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 919b36d753ec..dca6d1ee5744 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/log2.h>
 #include <linux/platform_device.h>
+#include <linux/ratelimit.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
@@ -2369,9 +2370,11 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 	}
 
 	err_mb = coda_read(dev, CODA_RET_DEC_PIC_ERR_MB);
-	if (err_mb > 0)
-		v4l2_err(&dev->v4l2_dev,
-			 "errors in %d macroblocks\n", err_mb);
+	if (err_mb > 0) {
+		if (__ratelimit(&dev->err_mb_rs))
+			coda_dbg(1, ctx, "errors in %d macroblocks\n", err_mb);
+		ctx->err_mb += err_mb;
+	}
 
 	if (dev->devtype->product == CODA_HX4 ||
 	    dev->devtype->product == CODA_7541) {
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 487dd653b24a..498563bc9a66 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -26,8 +26,10 @@
 #include <linux/videodev2.h>
 #include <linux/of.h>
 #include <linux/platform_data/media/coda.h>
+#include <linux/ratelimit.h>
 #include <linux/reset.h>
 
+#include <media/drv-intf/coda.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -2062,6 +2064,7 @@ static int coda_start_streaming(struct vb2_queue *q, unsigned int count)
 	if (q_data_dst->fourcc == V4L2_PIX_FMT_JPEG)
 		ctx->params.gop_size = 1;
 	ctx->gopcounter = ctx->params.gop_size - 1;
+	ctx->err_mb = 0;
 
 	ret = ctx->ops->start_streaming(ctx);
 	if (ctx->inst_type == CODA_INST_DECODER) {
@@ -2162,6 +2165,22 @@ static const struct vb2_ops coda_qops = {
 	.wait_finish		= vb2_ops_wait_finish,
 };
 
+static int coda_g_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct coda_ctx *ctx =
+			container_of(ctrl->handler, struct coda_ctx, ctrls);
+	switch (ctrl->id) {
+	case V4L2_CID_CODA_ERR_MB:
+		ctrl->val = ctx->err_mb;
+		break;
+	default:
+		coda_dbg(1, ctx, "Invalid control, id=%d, val=%d\n",
+			 ctrl->id, ctrl->val);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int coda_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	const char * const *val_names = v4l2_ctrl_get_menu(ctrl->id);
@@ -2291,6 +2310,10 @@ static int coda_s_ctrl(struct v4l2_ctrl *ctrl)
 	return 0;
 }
 
+static const struct v4l2_ctrl_ops coda_err_mb_ctrl_ops = {
+	.g_volatile_ctrl = coda_g_ctrl,
+};
+
 static const struct v4l2_ctrl_ops coda_ctrl_ops = {
 	.s_ctrl = coda_s_ctrl,
 };
@@ -2462,6 +2485,16 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
 		ctx->mpeg4_level_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 }
 
+static const struct v4l2_ctrl_config coda_err_mb_ctrl_config = {
+	.ops	= &coda_err_mb_ctrl_ops,
+	.id	= V4L2_CID_CODA_ERR_MB,
+	.name	= "Error macroblocks count",
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.min	= 0,
+	.max	= 0xffffffff,
+	.step	= 1,
+};
+
 static int coda_ctrls_setup(struct coda_ctx *ctx)
 {
 	v4l2_ctrl_handler_init(&ctx->ctrls, 2);
@@ -2484,6 +2517,14 @@ static int coda_ctrls_setup(struct coda_ctx *ctx)
 				  1, 1, 1, 1);
 		if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_H264)
 			coda_decode_ctrls(ctx);
+
+		ctx->err_mb_ctrl = v4l2_ctrl_new_custom(&ctx->ctrls,
+						&coda_err_mb_ctrl_config,
+						NULL);
+		if (ctx->err_mb_ctrl)
+			ctx->err_mb_ctrl->flags |=
+				V4L2_CTRL_FLAG_READ_ONLY |
+				V4L2_CTRL_FLAG_VOLATILE;
 	}
 
 	if (ctx->ctrls.error) {
@@ -3202,6 +3243,7 @@ static int coda_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ratelimit_default_init(&dev->err_mb_rs);
 	mutex_init(&dev->dev_mutex);
 	mutex_init(&dev->coda_mutex);
 	ida_init(&dev->ida);
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index e53f7a65d532..517c47e6e1b3 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -17,6 +17,7 @@
 #include <linux/mutex.h>
 #include <linux/kfifo.h>
 #include <linux/videodev2.h>
+#include <linux/ratelimit.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -92,6 +93,7 @@ struct coda_dev {
 	struct v4l2_m2m_dev	*m2m_dev;
 	struct ida		ida;
 	struct dentry		*debugfs_root;
+	struct ratelimit_state	err_mb_rs;
 };
 
 struct coda_codec {
@@ -242,6 +244,7 @@ struct coda_ctx {
 	struct v4l2_ctrl		*mpeg2_level_ctrl;
 	struct v4l2_ctrl		*mpeg4_profile_ctrl;
 	struct v4l2_ctrl		*mpeg4_level_ctrl;
+	struct v4l2_ctrl		*err_mb_ctrl;
 	struct v4l2_fh			fh;
 	int				gopcounter;
 	int				runcounter;
@@ -274,6 +277,7 @@ struct coda_ctx {
 	struct dentry			*debugfs_entry;
 	bool				use_bit;
 	bool				use_vdoa;
+	unsigned int			err_mb;
 	struct vdoa_ctx			*vdoa;
 	/*
 	 * wakeup mutex used to serialize encoder stop command and finish_run,
diff --git a/include/media/drv-intf/coda.h b/include/media/drv-intf/coda.h
new file mode 100644
index 000000000000..1d767bec4c4a
--- /dev/null
+++ b/include/media/drv-intf/coda.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef VIDEO_CODA_INTF_H
+#define VIDEO_CODA_INTF_H
+
+#include <linux/types.h>
+#include <linux/videodev2.h>
+
+enum coda_ctrl_id {
+	V4L2_CID_CODA_ERR_MB = (V4L2_CID_USER_CODA_BASE + 0),
+};
+
+#endif /* VIDEO_CODA_INTF_H */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a184c4939438..b4481d9579e7 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -198,6 +198,10 @@ enum v4l2_colorfx {
  */
 #define V4L2_CID_USER_ATMEL_ISC_BASE		(V4L2_CID_USER_BASE + 0x10c0)
 
+/* The base for the CODA driver controls.
+ * We reserve 16 controls for this driver. */
+#define V4L2_CID_USER_CODA_BASE			(V4L2_CID_USER_BASE + 0x10e0)
+
 /* MPEG-class control IDs */
 /* The MPEG controls are applicable to all codec controls
  * and the 'MPEG' part of the define is historical */
-- 
2.27.0

Re: [PATCH v2 0/6] CODA timeout fix & assorted changes

[Fabio Estevam]

Hi Ezequiel,

On Wed, Oct 7, 2020 at 8:01 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Hi all,
>
> The main motivation for this series is to address a PIC_RUN
> timeout, which we managed to link with a hardware bitstream
> buffer underrun condition.
>
> Upon further investigation we discovered that the underrun
> was produced by a subtle issue in the way buffer_meta's were
> being tracked.
>
> The issue is fixed by patch "5/6 coda: coda_buffer_meta housekeeping fix".
>
> Patches 1 to 4 are mostly cleanups and minor cosmetic changes.

Shouldn't the fix be the first patch in the series so that it can be
backported to stable?

Thanks

Re: [PATCH v2 0/6] CODA timeout fix & assorted changes

[Ezequiel Garcia]

Hi Fabio,

On Wed, 2020-10-07 at 08:13 -0300, Fabio Estevam wrote:
> Hi Ezequiel,
> 
> On Wed, Oct 7, 2020 at 8:01 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > Hi all,
> > 
> > The main motivation for this series is to address a PIC_RUN
> > timeout, which we managed to link with a hardware bitstream
> > buffer underrun condition.
> > 
> > Upon further investigation we discovered that the underrun
> > was produced by a subtle issue in the way buffer_meta's were
> > being tracked.
> > 
> > The issue is fixed by patch "5/6 coda: coda_buffer_meta housekeeping fix".
> > 
> > Patches 1 to 4 are mostly cleanups and minor cosmetic changes.
> 
> Shouldn't the fix be the first patch in the series so that it can be
> backported to stable?
> 

While that is typically the case, it won't change much.

You'll find that the fix in patch 5 can be applied cleanly
on top of v5.4 and v5.8 :-)

Now that you mention this, I believe that the patch
should carry:

Cc: stable@vger.kernel.org # v5.4

Thanks,
Ezequiel

Re: [PATCH v2 0/6] CODA timeout fix & assorted changes

[Fabio Estevam]

On Wed, Oct 7, 2020 at 8:34 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:

> While that is typically the case, it won't change much.
>
> You'll find that the fix in patch 5 can be applied cleanly
> on top of v5.4 and v5.8 :-)
>
> Now that you mention this, I believe that the patch
> should carry:
>
> Cc: stable@vger.kernel.org # v5.4

And a Fixes tag too :-)

RE: [PATCH v2 0/6] CODA timeout fix & assorted changes

[Benjamin Bara - SKIDATA]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

Hi Ezequiel!

I applied your series on top of bbf5c979011a099af5dc76498918ed7df445635b (5.9 tag).
Additionally, I applied the attached patch to print the active metas during coda_fill_bitstream():
0001-DEBUG-coda-print-list-of-current-active-metas.patch

For verification, I used the attached single-color video:
MPEG4, 512x512, ~135 kb/s, 20 FPS, GoP = 5; see below for reproduction steps.
This might be a very "hard" and "theoretical" sample.
However, with this video, I still get a timeout (see error log below).

When applying my "simplistic approach" (second patch) on top of that, the video is working.
I saw a padding to 512 byte in the H.264 implementation, therefore I assumed it's the same here.
Since I don't know how to pad the MPEG4 stream, I decided to keep adding frames
until the "requirement" (I have no documentation for that) is reached.
Therefore, this patch ensures that there are at least two buffers queued
which reach the 512 byte threshold (for simplistic reasons 3, since "beyond the current one").

Note that the second patch is also just a proof of concept, there might be more efficient solutions.

If your reproduction attempts lead to different results, please write me an email.

Regards & many thanks!
Benjamin


*Video creation:*
1.) Create single color video:
gst-launch-1.0 videotestsrc num-buffers=100 pattern=blue ! \
video/x-raw,format=I420,width=512,height=512 ! v4l2mpeg4enc output-io-mode=4 ! \
avimux ! filesink location=temp.avi

2.) Re-encode with a GoP (IIRC this option is not provided by v4l2mpeg4enc,
even though enabled in the driver; I usually do this on my test pc, not the SuT):
ffmpeg -i temp.avi -vf scale=512:512 -vtag "xvid" -g 5 mpeg4.avi

*Error log:*
[ 1067.150368] coda 2040000.vpu: 0: start streaming vid-cap
[ 1067.164871] coda 2040000.vpu: 0: not ready: need 2 buffers available (queue:1 + bitstream:0)
[ 1067.165490] coda 2040000.vpu: 0: job ready
[ 1067.166682] coda 2040000.vpu: 0: active metas:
[ 1067.166692] coda 2040000.vpu: 0: - payload: 60
[ 1067.166697] coda 2040000.vpu: 0: - payload: 2913
[ 1067.166702] coda 2040000.vpu: 0: - payload: 155
[ 1067.166706] coda 2040000.vpu: 0: - payload: 155
[ 1067.166711] coda 2040000.vpu: 0: - payload: 155
[ 1067.166715] coda 2040000.vpu: 0: want to queue: payload: 155
[ 1067.166973] coda 2040000.vpu: 0: active metas:
[ 1067.166982] coda 2040000.vpu: 0: - payload: 60
[ 1067.166987] coda 2040000.vpu: 0: - payload: 2913
[ 1067.166992] coda 2040000.vpu: 0: - payload: 155
[ 1067.166996] coda 2040000.vpu: 0: - payload: 155
[ 1067.167000] coda 2040000.vpu: 0: - payload: 155
[ 1067.167005] coda 2040000.vpu: 0: want to queue: payload: 155
[ 1068.168449] coda 2040000.vpu: CODA PIC_RUN timeout


[-- Attachment #2: video.avi --]
[-- Type: video/avi, Size: 120588 bytes --]

[-- Attachment #3: 0002-media-coda-avoid-starvation-on-well-compressed-data.patch --]
[-- Type: application/octet-stream, Size: 3110 bytes --]

From 1f534d84dd1ed4c586c37551d64a070160e88626 Mon Sep 17 00:00:00 2001
From: Benjamin Bara <benjamin.bara@skidata.com>
Date: Mon, 12 Oct 2020 15:44:59 +0200
Subject: [PATCH 2/2] media: coda: avoid starvation on well-compressed data

The prefetcher requires two 512 byte periods beyond the current one.
However, currently it is only checked if there are at least 512 bytes
beyond the current meta available.
This only works under the assumption that every buffer has a size of
at least 512 bytes.

To ensure that the requirement is fulfilled with buffers < 512 bytes,
at least two must not be below this threshold.
Otherwise, additional buffers are enqueued to ensure a full window.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/media/platform/coda/coda-bit.c | 7 ++++++-
 drivers/media/platform/coda/coda.h     | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index de7ac35ade9b..95e3193c88ed 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -326,6 +326,7 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 	struct coda_buffer_meta *meta, *meta_tmp;
 	u32 start;
 	struct list_head *this;
+	u32 count;
 
 	if (ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG)
 		return;
@@ -347,11 +348,14 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 			meta = list_first_entry(&ctx->buffer_meta_list,
 						struct coda_buffer_meta, list);
 
+			count = 0;
 			coda_dbg(1, ctx, "active metas:\n");
 			list_for_each(this, &ctx->buffer_meta_list) {
 				meta_tmp = list_entry(this, struct coda_buffer_meta, list);
 				coda_dbg(1, ctx, "- payload: %u\n", 
 					 meta_tmp->end - meta_tmp->start);
+				if (!meta_tmp->below_threshold)
+					count++;
 			}
 
 			coda_dbg(1, ctx, "want to queue: payload: %lu\n", 
@@ -365,7 +369,7 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 			 * the first buffer to fetch, we can safely stop queuing
 			 * in order to limit the decoder drain latency.
 			 */
-			if (coda_bitstream_can_fetch_past(ctx, meta->end))
+			if (count > 2)
 				break;
 		}
 
@@ -414,6 +418,7 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 				meta->start = start;
 				meta->end = ctx->bitstream_fifo.kfifo.in;
 				meta->last = src_buf->flags & V4L2_BUF_FLAG_LAST;
+				meta->below_threshold = (meta->end - meta->start) < 512;
 				if (meta->last)
 					coda_dbg(1, ctx, "marking last meta");
 				spin_lock(&ctx->buffer_meta_lock);
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index 517c47e6e1b3..e887e08e9115 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -162,6 +162,7 @@ struct coda_buffer_meta {
 	unsigned int		start;
 	unsigned int		end;
 	bool			last;
+	bool			below_threshold;
 };
 
 /* Per-queue, driver-specific private data */
-- 
2.26.2


[-- Attachment #4: 0001-DEBUG-coda-print-list-of-current-active-metas.patch --]
[-- Type: application/octet-stream, Size: 2343 bytes --]

From 59b28f83eff60c00d4a3e0dfabb73c7ad2923fdb Mon Sep 17 00:00:00 2001
From: Benjamin Bara <benjamin.bara@skidata.com>
Date: Mon, 12 Oct 2020 14:07:08 +0200
Subject: [PATCH 1/2] [DEBUG] coda: print list of current active metas

This commit prints the active metas every time a new
meta should be added.
This allows to identify the state during a timeout.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/media/platform/coda/coda-bit.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 377ae64c09cf..de7ac35ade9b 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -323,8 +323,9 @@ static bool coda_bitstream_try_queue(struct coda_ctx *ctx,
 void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 {
 	struct vb2_v4l2_buffer *src_buf;
-	struct coda_buffer_meta *meta;
+	struct coda_buffer_meta *meta, *meta_tmp;
 	u32 start;
+	struct list_head *this;
 
 	if (ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG)
 		return;
@@ -339,11 +340,23 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 		    ctx->num_metas > 1)
 			break;
 
+		src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
+
 		if (ctx->num_internal_frames &&
 		    ctx->num_metas >= ctx->num_internal_frames) {
 			meta = list_first_entry(&ctx->buffer_meta_list,
 						struct coda_buffer_meta, list);
 
+			coda_dbg(1, ctx, "active metas:\n");
+			list_for_each(this, &ctx->buffer_meta_list) {
+				meta_tmp = list_entry(this, struct coda_buffer_meta, list);
+				coda_dbg(1, ctx, "- payload: %u\n", 
+					 meta_tmp->end - meta_tmp->start);
+			}
+
+			coda_dbg(1, ctx, "want to queue: payload: %lu\n", 
+				 vb2_get_plane_payload(&src_buf->vb2_buf, 0));
+
 			/*
 			 * If we managed to fill in at least a full reorder
 			 * window of buffers (num_internal_frames is a
@@ -356,8 +369,6 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 				break;
 		}
 
-		src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
-
 		/* Drop frames that do not start/end with a SOI/EOI markers */
 		if (ctx->codec->src_fourcc == V4L2_PIX_FMT_JPEG &&
 		    !coda_jpeg_check_buffer(ctx, &src_buf->vb2_buf)) {
-- 
2.26.2

Re: [PATCH v2 6/6] coda: Add a V4L2 user for control error macroblocks count

[Hans Verkuil]

On 07/10/2020 12:35, Ezequiel Garcia wrote:
> To avoid potentially overflowing the kernel logs in the case
> of corrupted streams, this commit replaces an error log with

log -> message

> a per-stream counter to be read through a driver-specific
> control.
> 
> Applications can read the per-stream accumulated
> error macroblocks count.
> 
> The error message is moved to the driver-specific debug log,
> and rate-limitting is added to make sure it doesn't
> spam the log.

This is a confusing sentence. I'd write:

"The old error message is replaced by a rate-limited debug message."

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  MAINTAINERS                               |  1 +
>  drivers/media/platform/coda/coda-bit.c    |  9 +++--
>  drivers/media/platform/coda/coda-common.c | 42 +++++++++++++++++++++++
>  drivers/media/platform/coda/coda.h        |  4 +++
>  include/media/drv-intf/coda.h             | 13 +++++++
>  include/uapi/linux/v4l2-controls.h        |  4 +++
>  6 files changed, 70 insertions(+), 3 deletions(-)
>  create mode 100644 include/media/drv-intf/coda.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ba5eb1dff9c2..4c7a59a4dda3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4334,6 +4334,7 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/media/coda.txt
>  F:	drivers/media/platform/coda/
> +F:	include/media/drv-intf/coda.h
>  
>  CODE OF CONDUCT
>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
> index 919b36d753ec..dca6d1ee5744 100644
> --- a/drivers/media/platform/coda/coda-bit.c
> +++ b/drivers/media/platform/coda/coda-bit.c
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/log2.h>
>  #include <linux/platform_device.h>
> +#include <linux/ratelimit.h>
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
> @@ -2369,9 +2370,11 @@ static void coda_finish_decode(struct coda_ctx *ctx)
>  	}
>  
>  	err_mb = coda_read(dev, CODA_RET_DEC_PIC_ERR_MB);
> -	if (err_mb > 0)
> -		v4l2_err(&dev->v4l2_dev,
> -			 "errors in %d macroblocks\n", err_mb);
> +	if (err_mb > 0) {
> +		if (__ratelimit(&dev->err_mb_rs))
> +			coda_dbg(1, ctx, "errors in %d macroblocks\n", err_mb);
> +		ctx->err_mb += err_mb;

Use v4l2_ctrl_s_ctrl.

> +	}
>  
>  	if (dev->devtype->product == CODA_HX4 ||
>  	    dev->devtype->product == CODA_7541) {
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 487dd653b24a..498563bc9a66 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -26,8 +26,10 @@
>  #include <linux/videodev2.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/media/coda.h>
> +#include <linux/ratelimit.h>
>  #include <linux/reset.h>
>  
> +#include <media/drv-intf/coda.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -2062,6 +2064,7 @@ static int coda_start_streaming(struct vb2_queue *q, unsigned int count)
>  	if (q_data_dst->fourcc == V4L2_PIX_FMT_JPEG)
>  		ctx->params.gop_size = 1;
>  	ctx->gopcounter = ctx->params.gop_size - 1;
> +	ctx->err_mb = 0;

Ditto.

>  
>  	ret = ctx->ops->start_streaming(ctx);
>  	if (ctx->inst_type == CODA_INST_DECODER) {
> @@ -2162,6 +2165,22 @@ static const struct vb2_ops coda_qops = {
>  	.wait_finish		= vb2_ops_wait_finish,
>  };
>  
> +static int coda_g_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct coda_ctx *ctx =
> +			container_of(ctrl->handler, struct coda_ctx, ctrls);
> +	switch (ctrl->id) {
> +	case V4L2_CID_CODA_ERR_MB:
> +		ctrl->val = ctx->err_mb;
> +		break;
> +	default:
> +		coda_dbg(1, ctx, "Invalid control, id=%d, val=%d\n",
> +			 ctrl->id, ctrl->val);

Just drop this message, you can't actually get in the default case.

> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int coda_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	const char * const *val_names = v4l2_ctrl_get_menu(ctrl->id);
> @@ -2291,6 +2310,10 @@ static int coda_s_ctrl(struct v4l2_ctrl *ctrl)
>  	return 0;
>  }
>  
> +static const struct v4l2_ctrl_ops coda_err_mb_ctrl_ops = {
> +	.g_volatile_ctrl = coda_g_ctrl,
> +};

This control isn't a volatile control at all. Just use v4l2_ctrl_s_ctrl to
set it.

> +
>  static const struct v4l2_ctrl_ops coda_ctrl_ops = {
>  	.s_ctrl = coda_s_ctrl,
>  };
> @@ -2462,6 +2485,16 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
>  		ctx->mpeg4_level_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  }
>  
> +static const struct v4l2_ctrl_config coda_err_mb_ctrl_config = {
> +	.ops	= &coda_err_mb_ctrl_ops,
> +	.id	= V4L2_CID_CODA_ERR_MB,

Call it V4L2_CID_CODA_MB_ERR_CNT to clearly signal that this is a counter.
Rename related variables as well (e.g. coda_mb_err_cnt_ctrl_config).

> +	.name	= "Error macroblocks count",

"Macroblocks Error Count"

> +	.type	= V4L2_CTRL_TYPE_INTEGER,
> +	.min	= 0,
> +	.max	= 0xffffffff,

This is a s32, so max should be 0x7fffffff.

> +	.step	= 1,
> +};
> +
>  static int coda_ctrls_setup(struct coda_ctx *ctx)
>  {
>  	v4l2_ctrl_handler_init(&ctx->ctrls, 2);
> @@ -2484,6 +2517,14 @@ static int coda_ctrls_setup(struct coda_ctx *ctx)
>  				  1, 1, 1, 1);
>  		if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_H264)
>  			coda_decode_ctrls(ctx);
> +
> +		ctx->err_mb_ctrl = v4l2_ctrl_new_custom(&ctx->ctrls,
> +						&coda_err_mb_ctrl_config,
> +						NULL);
> +		if (ctx->err_mb_ctrl)
> +			ctx->err_mb_ctrl->flags |=
> +				V4L2_CTRL_FLAG_READ_ONLY |
> +				V4L2_CTRL_FLAG_VOLATILE;

Drop volatile.

>  	}
>  
>  	if (ctx->ctrls.error) {
> @@ -3202,6 +3243,7 @@ static int coda_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ratelimit_default_init(&dev->err_mb_rs);
>  	mutex_init(&dev->dev_mutex);
>  	mutex_init(&dev->coda_mutex);
>  	ida_init(&dev->ida);
> diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
> index e53f7a65d532..517c47e6e1b3 100644
> --- a/drivers/media/platform/coda/coda.h
> +++ b/drivers/media/platform/coda/coda.h
> @@ -17,6 +17,7 @@
>  #include <linux/mutex.h>
>  #include <linux/kfifo.h>
>  #include <linux/videodev2.h>
> +#include <linux/ratelimit.h>
>  
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> @@ -92,6 +93,7 @@ struct coda_dev {
>  	struct v4l2_m2m_dev	*m2m_dev;
>  	struct ida		ida;
>  	struct dentry		*debugfs_root;
> +	struct ratelimit_state	err_mb_rs;
>  };
>  
>  struct coda_codec {
> @@ -242,6 +244,7 @@ struct coda_ctx {
>  	struct v4l2_ctrl		*mpeg2_level_ctrl;
>  	struct v4l2_ctrl		*mpeg4_profile_ctrl;
>  	struct v4l2_ctrl		*mpeg4_level_ctrl;
> +	struct v4l2_ctrl		*err_mb_ctrl;
>  	struct v4l2_fh			fh;
>  	int				gopcounter;
>  	int				runcounter;
> @@ -274,6 +277,7 @@ struct coda_ctx {
>  	struct dentry			*debugfs_entry;
>  	bool				use_bit;
>  	bool				use_vdoa;
> +	unsigned int			err_mb;
>  	struct vdoa_ctx			*vdoa;
>  	/*
>  	 * wakeup mutex used to serialize encoder stop command and finish_run,
> diff --git a/include/media/drv-intf/coda.h b/include/media/drv-intf/coda.h

Why here? This is not a public header. It should either be in include/uapi/linux
or just keep it in the driver.

> new file mode 100644
> index 000000000000..1d767bec4c4a
> --- /dev/null
> +++ b/include/media/drv-intf/coda.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef VIDEO_CODA_INTF_H
> +#define VIDEO_CODA_INTF_H
> +
> +#include <linux/types.h>
> +#include <linux/videodev2.h>
> +
> +enum coda_ctrl_id {
> +	V4L2_CID_CODA_ERR_MB = (V4L2_CID_USER_CODA_BASE + 0),
> +};

Just use #define.

You must document this control here.

> +
> +#endif /* VIDEO_CODA_INTF_H */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a184c4939438..b4481d9579e7 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -198,6 +198,10 @@ enum v4l2_colorfx {
>   */
>  #define V4L2_CID_USER_ATMEL_ISC_BASE		(V4L2_CID_USER_BASE + 0x10c0)
>  
> +/* The base for the CODA driver controls.
> + * We reserve 16 controls for this driver. */
> +#define V4L2_CID_USER_CODA_BASE			(V4L2_CID_USER_BASE + 0x10e0)
> +
>  /* MPEG-class control IDs */
>  /* The MPEG controls are applicable to all codec controls
>   * and the 'MPEG' part of the define is historical */
> 

Regards,

	Hans

Re: [PATCH v2 0/6] CODA timeout fix & assorted changes

[Ezequiel Garcia]

Hi Benjamin,

On Mon, 2020-10-12 at 14:47 +0000, Benjamin Bara - SKIDATA wrote:
> Hi Ezequiel!
> 
> I applied your series on top of bbf5c979011a099af5dc76498918ed7df445635b (5.9 tag).
> Additionally, I applied the attached patch to print the active metas during coda_fill_bitstream():
> 0001-DEBUG-coda-print-list-of-current-active-metas.patch
> 
> For verification, I used the attached single-color video:
> MPEG4, 512x512, ~135 kb/s, 20 FPS, GoP = 5; see below for reproduction steps.
> This might be a very "hard" and "theoretical" sample.
> However, with this video, I still get a timeout (see error log below).
> 
> When applying my "simplistic approach" (second patch) on top of that, the video is working.
> I saw a padding to 512 byte in the H.264 implementation, therefore I assumed it's the same here.
> Since I don't know how to pad the MPEG4 stream, I decided to keep adding frames
> until the "requirement" (I have no documentation for that) is reached.
> Therefore, this patch ensures that there are at least two buffers queued
> which reach the 512 byte threshold (for simplistic reasons 3, since "beyond the current one").
> 
> Note that the second patch is also just a proof of concept, there might be more efficient solutions.
> 
> If your reproduction attempts lead to different results, please write me an email.
> 
> Regards & many thanks!
> Benjamin
> 
> 
> *Video creation:*
> 1.) Create single color video:
> gst-launch-1.0 videotestsrc num-buffers=100 pattern=blue ! \
> video/x-raw,format=I420,width=512,height=512 ! v4l2mpeg4enc output-io-mode=4 ! \
> avimux ! filesink location=temp.avi
> 
> 2.) Re-encode with a GoP (IIRC this option is not provided by v4l2mpeg4enc,
> even though enabled in the driver; I usually do this on my test pc, not the SuT):
> ffmpeg -i temp.avi -vf scale=512:512 -vtag "xvid" -g 5 mpeg4.avi
> 
> *Error log:*
> [ 1067.150368] coda 2040000.vpu: 0: start streaming vid-cap
> [ 1067.164871] coda 2040000.vpu: 0: not ready: need 2 buffers available (queue:1 + bitstream:0)
> [ 1067.165490] coda 2040000.vpu: 0: job ready
> [ 1067.166682] coda 2040000.vpu: 0: active metas:
> [ 1067.166692] coda 2040000.vpu: 0: - payload: 60
> [ 1067.166697] coda 2040000.vpu: 0: - payload: 2913
> [ 1067.166702] coda 2040000.vpu: 0: - payload: 155
> [ 1067.166706] coda 2040000.vpu: 0: - payload: 155
> [ 1067.166711] coda 2040000.vpu: 0: - payload: 155
> [ 1067.166715] coda 2040000.vpu: 0: want to queue: payload: 155
> [ 1067.166973] coda 2040000.vpu: 0: active metas:
> [ 1067.166982] coda 2040000.vpu: 0: - payload: 60
> [ 1067.166987] coda 2040000.vpu: 0: - payload: 2913
> [ 1067.166992] coda 2040000.vpu: 0: - payload: 155
> [ 1067.166996] coda 2040000.vpu: 0: - payload: 155
> [ 1067.167000] coda 2040000.vpu: 0: - payload: 155
> [ 1067.167005] coda 2040000.vpu: 0: want to queue: payload: 155
> [ 1068.168449] coda 2040000.vpu: CODA PIC_RUN timeout
> 

Many thanks for your report. Indeed you managed to create
a video that is quite problematic for CODA.

Adding some debugging code to dump the metas, and then 
inspect the bitstream payload before/after a sync,
we can see that the hardware buffer has 690 bytes.

This seems a bit confusing, since the driver considers
it's enough. From the log below, isn't this supposed
to be enough metas?

[  274.087643] coda 2040000.vpu: CODA PIC_RUN timeout
[  274.092655] coda 2040000.vpu: 	meta 2: 432 - 804 (00 00 01 b0 01)
[  274.099133] coda 2040000.vpu: 	meta 3: 804 - 1176 (00 00 01 b0 01)
[  274.105551] coda 2040000.vpu: 	meta 4: 1176 - 1548 (00 00 01 b0 01)
[  274.112200] coda 2040000.vpu: 	meta 5: 1548 - 1920 (00 00 01 b0 01)
[  274.118840] coda 2040000.vpu: 	meta 6: 1920 - 2292 (00 00 01 b0 01)
[  274.125347] coda 2040000.vpu: bitstream payload: 690 (before sync)
[  274.130627] coda 2040000.vpu: bitstream payload: 690 (after sync)

Philipp, what do you think?

I must admit I can't fully wrap my head around
your prefetch fix, and how the new condition
would fix this issue. Could you explain it in more detail?

Why wouldn't the prefetcher count chunks smaller than 512?
Do you have some documentation for that?

Thanks,
Ezequiel

RE: [PATCH v2 0/6] CODA timeout fix & assorted changes

[Benjamin Bara - SKIDATA]

> -----Original Message-----
> From: Ezequiel Garcia <ezequiel@collabora.com>
> Sent: Mittwoch, 4. November 2020 18:01

Hi again!
 
> Many thanks for your report. Indeed you managed to create a video that is quite
> problematic for CODA.

Thanks for looking at it!

> I must admit I can't fully wrap my head around your prefetch fix, and how the new
> condition would fix this issue. Could you explain it in more detail?

I don't have access to the CODA960 documentation, so all my findings are based on tests
or code documentation.

My starting point was the inline documentation [1]:
"If we managed to fill in at least a full reorder window of buffers and the bitstream
 prefetcher has at least 2 256 bytes periods beyond the first buffer to fetch (...)".

The current code checks if there are 512 bytes after the current meta.
When I comment out the following break, my test videos are working.
So for me, starvation is caused by this statement, because it hinders follow-up metas
for performance reasons (the decoder might also start hiccuping with too much data enqueued,
but not sure).

I did some tests, and basically I found out that the sum doesn't matter, so something like
- Meta 2: 1024
- Meta 3: 100
- Meta 4: 100
starves, even if it passes the current check.

Next, I thought about "2x 256" is actually not the same as "1x 512".
With 2x 256, I could actually got a couple more test videos running, something like:
- Meta 2: 650
- Meta 3: 50
- Meta 4: 650

The crafting of such video is quite easy by varying the Group-of-Picture and the resolution.

However, then I tried some smaller videos (with meta 2 & 4: 256 < meta < 512) 
which lead to starvation again.
I haven't done additional tests, instead I shared my findings and asked for further documentation
hints about the actual starvation criteria.

So my solution is just a Proof-of-Concept, found by testing and is not backed by documentation - 
therefore I am also not sure if it is sufficient or has any side effects.

Best regards
Benjamin 

[1] https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/media/platform/coda/coda-bit.c#L352

Re: [PATCH v2 0/6] CODA timeout fix & assorted changes

[Ezequiel Garcia]

Hi Benjamin,

On Wed, 2020-11-04 at 17:49 +0000, Benjamin Bara - SKIDATA wrote:
> > -----Original Message-----
> > From: Ezequiel Garcia <ezequiel@collabora.com>
> > Sent: Mittwoch, 4. November 2020 18:01
> 
> Hi again!
>  
> > Many thanks for your report. Indeed you managed to create a video that is quite
> > problematic for CODA.
> 
> Thanks for looking at it!
> 
> > I must admit I can't fully wrap my head around your prefetch fix, and how the new
> > condition would fix this issue. Could you explain it in more detail?
> 
> I don't have access to the CODA960 documentation, so all my findings are based on tests
> or code documentation.
> 
> My starting point was the inline documentation [1]:
> "If we managed to fill in at least a full reorder window of buffers and the bitstream
>  prefetcher has at least 2 256 bytes periods beyond the first buffer to fetch (...)".
> 
> The current code checks if there are 512 bytes after the current meta.
> When I comment out the following break, my test videos are working.
> So for me, starvation is caused by this statement, because it hinders follow-up metas
> for performance reasons (the decoder might also start hiccuping with too much data enqueued,
> but not sure).
> 
> I did some tests, and basically I found out that the sum doesn't matter, so something like
> - Meta 2: 1024
> - Meta 3: 100
> - Meta 4: 100
> starves, even if it passes the current check.
> 
> Next, I thought about "2x 256" is actually not the same as "1x 512".
> With 2x 256, I could actually got a couple more test videos running, something like:
> - Meta 2: 650
> - Meta 3: 50
> - Meta 4: 650
> 
> The crafting of such video is quite easy by varying the Group-of-Picture and the resolution.
> 
> However, then I tried some smaller videos (with meta 2 & 4: 256 < meta < 512) 
> which lead to starvation again.
> I haven't done additional tests, instead I shared my findings and asked for further documentation
> hints about the actual starvation criteria.
> 
> So my solution is just a Proof-of-Concept, found by testing and is not backed by documentation - 
> therefore I am also not sure if it is sufficient or has any side effects.
> 

Please submit your patch on top of latest media master branch and let's
start discussing it.

BTW, Nicolas and I have started using https://github.com/fluendo/fluster/
conformance testing. Perhaps you can run the H264 test suite, with & without
your patch (assuming it affects H264 in any way) and include that information
in the cover letter.

Thanks!
Ezequiel