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

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

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 concealed MB (ERR_MB)

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

-- 
2.27.0

[PATCH 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>
---
 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 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>
---
 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 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>
---
 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 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>
---
 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 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>
---
 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..928a640b0056 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 6/6] coda: Add a V4L2 user for control concealed MB (ERR_MB)

[Ezequiel Garcia]

To avoid potentially overflowing the kernel logs in the case
of corrupted streams, this commit adds a per-stream counter
to be read through a driver-specific control. This allows
applicationsto read the per-stream accumulated concealed
macroblocks count.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 MAINTAINERS                               |  1 +
 drivers/media/platform/coda/coda-bit.c    |  4 +--
 drivers/media/platform/coda/coda-common.c | 40 +++++++++++++++++++++++
 drivers/media/platform/coda/coda.h        |  2 ++
 include/media/drv-intf/coda.h             | 13 ++++++++
 include/uapi/linux/v4l2-controls.h        |  4 +++
 6 files changed, 61 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 928a640b0056..60d8f6f29e97 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2369,9 +2369,7 @@ 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);
+	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..5d63f3687c03 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -28,6 +28,7 @@
 #include <linux/platform_data/media/coda.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 +2063,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 +2164,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 +2309,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 +2484,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	= "Concealed MB 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 +2516,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) {
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index e53f7a65d532..c5b7d2260060 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -242,6 +242,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 +275,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 1/6] coda: Remove redundant ctx->initialized setting

[Philipp Zabel]

Hi Ezequiel,

On Tue, 2020-10-06 at 07:44 -0300, Ezequiel Garcia wrote:
> 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>
> ---
>  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);
>  }

Thank you for the cleanup! In general, I'd put the fixes before the
cosmetics unless they somehow simplify the following steps, but in this
case it doesn't matter much.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

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

[Philipp Zabel]

On Tue, 2020-10-06 at 07:44 -0300, Ezequiel Garcia wrote:
> 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>
> ---
>  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) {

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

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

[Philipp Zabel]

On Tue, 2020-10-06 at 07:44 -0300, Ezequiel Garcia wrote:
> 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>
> ---
>  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;
>  }

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

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

[Philipp Zabel]

On Tue, 2020-10-06 at 07:44 -0300, Ezequiel Garcia wrote:
> 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>
> ---
>  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");

I don't care very much for this, but I don't mind either.

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

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

[Philipp Zabel]

On Tue, 2020-10-06 at 07:44 -0300, Ezequiel Garcia wrote:
> 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>
> ---
>  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..928a640b0056 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)

Let's just call this coda_decoder_drop_used_metas.
Otherwise this looks fine,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

> +{
> +	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);
> +		}

Here it should also be possible to

		  else {
			break;
		}

as meta->end ever increases.

regards
Philipp

Re: [PATCH 6/6] coda: Add a V4L2 user for control concealed MB (ERR_MB)

[Philipp Zabel]

On Tue, 2020-10-06 at 07:44 -0300, Ezequiel Garcia wrote:
> To avoid potentially overflowing the kernel logs in the case
> of corrupted streams, this commit adds a per-stream counter
> to be read through a driver-specific control. This allows
> applicationsto read the per-stream accumulated concealed
> macroblocks count.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  MAINTAINERS                               |  1 +
>  drivers/media/platform/coda/coda-bit.c    |  4 +--
>  drivers/media/platform/coda/coda-common.c | 40 +++++++++++++++++++++++
>  drivers/media/platform/coda/coda.h        |  2 ++
>  include/media/drv-intf/coda.h             | 13 ++++++++
>  include/uapi/linux/v4l2-controls.h        |  4 +++
>  6 files changed, 61 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 928a640b0056..60d8f6f29e97 100644
> --- a/drivers/media/platform/coda/coda-bit.c
> +++ b/drivers/media/platform/coda/coda-bit.c
> @@ -2369,9 +2369,7 @@ 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);

Can we please keep this as a debug message, then?
Maybe rate limited if this causes issues with very broken streams.

> +	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..5d63f3687c03 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -28,6 +28,7 @@
>  #include <linux/platform_data/media/coda.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 +2063,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 +2164,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 +2309,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 +2484,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	= "Concealed MB count",

Is this right? In the old GPL imx-vpu-lib code this was called
numOfErrMBs, but was filled for h.264, MPEG-4, MPEG-2, or JPEG alike.
Not all of them have error concealment.

> +	.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 +2516,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) {
> diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
> index e53f7a65d532..c5b7d2260060 100644
> --- a/drivers/media/platform/coda/coda.h
> +++ b/drivers/media/platform/coda/coda.h
> @@ -242,6 +242,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 +275,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 */

regards
Philipp