linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] CODA timeout and macroblock error control
@ 2020-11-03 21:32 Ezequiel Garcia
  2020-11-03 21:32 ` [PATCH v3 1/2] coda: coda_buffer_meta housekeeping fix Ezequiel Garcia
  2020-11-03 21:32 ` [PATCH v3 2/2] coda: Add a V4L2 user for control error macroblocks count Ezequiel Garcia
  0 siblings, 2 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2020-11-03 21:32 UTC (permalink / raw)
  To: linux-media
  Cc: Hans Verkuil, Philipp Zabel, cphealy, Benjamin.Bara, l.stach,
	Ezequiel Garcia, kernel

Hi all,

I'm sending here the patches that are pending in previous series [2].

The main motivation for this fix 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 "1/2 coda: coda_buffer_meta housekeeping fix".

Also, while testing with corrupted bitstreams we realized
the driver was logging too verbosely, so patch 2 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.

As reported by Benjamin Bara this fix is not sufficient
to fix all timeouts. However, the fix does help to fix
some of the cases.

For instance, videos containing small black frames,
are now fixed. See:

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
[2] https://patchwork.linuxtv.org/project/linux-media/list/?series=3592

Changelog
---------

v3:
* Address Hans' feedback on patch 2.
  In particular, the control is not marked as volatile,
  since that was incorrect.
  Also, move the control CID definition inside the driver header.

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 (2):
  coda: coda_buffer_meta housekeeping fix
  coda: Add a V4L2 user for control error macroblocks count

 drivers/media/platform/coda/coda-bit.c    | 52 ++++++++++++++++++++---
 drivers/media/platform/coda/coda-common.c | 18 ++++++++
 drivers/media/platform/coda/coda.h        | 11 +++++
 include/uapi/linux/v4l2-controls.h        |  4 ++
 4 files changed, 78 insertions(+), 7 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 1/2] coda: coda_buffer_meta housekeeping fix
  2020-11-03 21:32 [PATCH v3 0/2] CODA timeout and macroblock error control Ezequiel Garcia
@ 2020-11-03 21:32 ` Ezequiel Garcia
  2020-11-03 21:32 ` [PATCH v3 2/2] coda: Add a V4L2 user for control error macroblocks count Ezequiel Garcia
  1 sibling, 0 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2020-11-03 21:32 UTC (permalink / raw)
  To: linux-media
  Cc: Hans Verkuil, Philipp Zabel, cphealy, Benjamin.Bara, l.stach,
	Ezequiel Garcia, kernel, Benjamin Bara

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 2/2] coda: Add a V4L2 user for control error macroblocks count
  2020-11-03 21:32 [PATCH v3 0/2] CODA timeout and macroblock error control Ezequiel Garcia
  2020-11-03 21:32 ` [PATCH v3 1/2] coda: coda_buffer_meta housekeeping fix Ezequiel Garcia
@ 2020-11-03 21:32 ` Ezequiel Garcia
  2020-11-04 10:42   ` Hans Verkuil
  2020-11-04 17:43   ` [PATCH v4] " Ezequiel Garcia
  1 sibling, 2 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2020-11-03 21:32 UTC (permalink / raw)
  To: linux-media
  Cc: Hans Verkuil, Philipp Zabel, cphealy, Benjamin.Bara, l.stach,
	Ezequiel Garcia, kernel

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

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

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

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-bit.c    | 10 +++++++---
 drivers/media/platform/coda/coda-common.c | 18 ++++++++++++++++++
 drivers/media/platform/coda/coda.h        | 10 ++++++++++
 include/uapi/linux/v4l2-controls.h        |  4 ++++
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 919b36d753ec..6ff7a001e578 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,12 @@ 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);
+		v4l2_ctrl_s_ctrl(ctx->err_mb_ctrl,
+				 v4l2_ctrl_g_ctrl(ctx->err_mb_ctrl) + 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..d7b066a3300b 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -26,6 +26,7 @@
 #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/v4l2-ctrls.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;
+	v4l2_ctrl_s_ctrl(ctx->err_mb_ctrl, 0);
 
 	ret = ctx->ops->start_streaming(ctx);
 	if (ctx->inst_type == CODA_INST_DECODER) {
@@ -2462,6 +2464,15 @@ 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 = {
+	.id	= V4L2_CID_CODA_MB_ERR_CNT,
+	.name	= "Macroblocks Error Count",
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.min	= 0,
+	.max	= 0x7fffffff,
+	.step	= 1,
+};
+
 static int coda_ctrls_setup(struct coda_ctx *ctx)
 {
 	v4l2_ctrl_handler_init(&ctx->ctrls, 2);
@@ -2484,6 +2495,12 @@ 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;
 	}
 
 	if (ctx->ctrls.error) {
@@ -3202,6 +3219,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..4dd7bdbcde52 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>
@@ -28,6 +29,13 @@
 #define CODA_MAX_FRAMEBUFFERS	19
 #define FMO_SLICE_SAVE_BUF_SIZE	(32)
 
+/*
+ * This control allows aplications to read the per-stream
+ * (i.e. per-context) Macroblocks Error Count. This value
+ * is CODA specific.
+ */
+#define V4L2_CID_CODA_MB_ERR_CNT (V4L2_CID_USER_CODA_BASE + 0)
+
 enum {
 	V4L2_M2M_SRC = 0,
 	V4L2_M2M_DST = 1,
@@ -92,6 +100,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 +251,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;
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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 2/2] coda: Add a V4L2 user for control error macroblocks count
  2020-11-03 21:32 ` [PATCH v3 2/2] coda: Add a V4L2 user for control error macroblocks count Ezequiel Garcia
@ 2020-11-04 10:42   ` Hans Verkuil
  2020-11-04 17:43   ` [PATCH v4] " Ezequiel Garcia
  1 sibling, 0 replies; 5+ messages in thread
From: Hans Verkuil @ 2020-11-04 10:42 UTC (permalink / raw)
  To: Ezequiel Garcia, linux-media
  Cc: Philipp Zabel, cphealy, Benjamin.Bara, l.stach, kernel

Hi Ezequiel,

Some more (small) comments:

On 03/11/2020 22:32, Ezequiel Garcia wrote:
> To avoid potentially overflowing the kernel logs in the case
> of corrupted streams, this commit replaces an error message with
> a per-stream counter to be read through a driver-specific
> control.
> 
> Applications can read the per-stream accumulated
> error macroblocks count.
> 
> The old error message is replaced by a rate-limited debug message.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/platform/coda/coda-bit.c    | 10 +++++++---
>  drivers/media/platform/coda/coda-common.c | 18 ++++++++++++++++++
>  drivers/media/platform/coda/coda.h        | 10 ++++++++++
>  include/uapi/linux/v4l2-controls.h        |  4 ++++
>  4 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
> index 919b36d753ec..6ff7a001e578 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,12 @@ 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);
> +		v4l2_ctrl_s_ctrl(ctx->err_mb_ctrl,
> +				 v4l2_ctrl_g_ctrl(ctx->err_mb_ctrl) + 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..d7b066a3300b 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -26,6 +26,7 @@
>  #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/v4l2-ctrls.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;
> +	v4l2_ctrl_s_ctrl(ctx->err_mb_ctrl, 0);
>  
>  	ret = ctx->ops->start_streaming(ctx);
>  	if (ctx->inst_type == CODA_INST_DECODER) {
> @@ -2462,6 +2464,15 @@ 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 = {

Please also update this struct name to coda_mb_err_cnt_ctrl_config.

> +	.id	= V4L2_CID_CODA_MB_ERR_CNT,
> +	.name	= "Macroblocks Error Count",
> +	.type	= V4L2_CTRL_TYPE_INTEGER,
> +	.min	= 0,
> +	.max	= 0x7fffffff,
> +	.step	= 1,
> +};
> +
>  static int coda_ctrls_setup(struct coda_ctx *ctx)
>  {
>  	v4l2_ctrl_handler_init(&ctx->ctrls, 2);
> @@ -2484,6 +2495,12 @@ 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,

And this to mb_err_cnt_ctrl. This keeps the naming consistent.

> +						&coda_err_mb_ctrl_config,
> +						NULL);
> +		if (ctx->err_mb_ctrl)
> +			ctx->err_mb_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  	}
>  
>  	if (ctx->ctrls.error) {
> @@ -3202,6 +3219,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..4dd7bdbcde52 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>
> @@ -28,6 +29,13 @@
>  #define CODA_MAX_FRAMEBUFFERS	19
>  #define FMO_SLICE_SAVE_BUF_SIZE	(32)
>  
> +/*
> + * This control allows aplications to read the per-stream

Typo: applications

> + * (i.e. per-context) Macroblocks Error Count. This value
> + * is CODA specific.
> + */
> +#define V4L2_CID_CODA_MB_ERR_CNT (V4L2_CID_USER_CODA_BASE + 0)
> +
>  enum {
>  	V4L2_M2M_SRC = 0,
>  	V4L2_M2M_DST = 1,
> @@ -92,6 +100,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 +251,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;
> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v4] coda: Add a V4L2 user for control error macroblocks count
  2020-11-03 21:32 ` [PATCH v3 2/2] coda: Add a V4L2 user for control error macroblocks count Ezequiel Garcia
  2020-11-04 10:42   ` Hans Verkuil
@ 2020-11-04 17:43   ` Ezequiel Garcia
  1 sibling, 0 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2020-11-04 17:43 UTC (permalink / raw)
  To: linux-media
  Cc: Hans Verkuil, Philipp Zabel, cphealy, Benjamin.Bara, l.stach,
	Ezequiel Garcia, kernel

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

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

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

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/coda/coda-bit.c    | 10 +++++++---
 drivers/media/platform/coda/coda-common.c | 18 ++++++++++++++++++
 drivers/media/platform/coda/coda.h        | 10 ++++++++++
 include/uapi/linux/v4l2-controls.h        |  4 ++++
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 919b36d753ec..2f42808c43a4 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,12 @@ 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->mb_err_rs))
+			coda_dbg(1, ctx, "errors in %d macroblocks\n", err_mb);
+		v4l2_ctrl_s_ctrl(ctx->mb_err_cnt_ctrl,
+				 v4l2_ctrl_g_ctrl(ctx->mb_err_cnt_ctrl) + 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..20f70ef3ae7a 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -26,6 +26,7 @@
 #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/v4l2-ctrls.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;
+	v4l2_ctrl_s_ctrl(ctx->mb_err_cnt_ctrl, 0);
 
 	ret = ctx->ops->start_streaming(ctx);
 	if (ctx->inst_type == CODA_INST_DECODER) {
@@ -2462,6 +2464,15 @@ 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_mb_err_cnt_ctrl_config = {
+	.id	= V4L2_CID_CODA_MB_ERR_CNT,
+	.name	= "Macroblocks Error Count",
+	.type	= V4L2_CTRL_TYPE_INTEGER,
+	.min	= 0,
+	.max	= 0x7fffffff,
+	.step	= 1,
+};
+
 static int coda_ctrls_setup(struct coda_ctx *ctx)
 {
 	v4l2_ctrl_handler_init(&ctx->ctrls, 2);
@@ -2484,6 +2495,12 @@ 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->mb_err_cnt_ctrl = v4l2_ctrl_new_custom(&ctx->ctrls,
+						&coda_mb_err_cnt_ctrl_config,
+						NULL);
+		if (ctx->mb_err_cnt_ctrl)
+			ctx->mb_err_cnt_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 	}
 
 	if (ctx->ctrls.error) {
@@ -3202,6 +3219,7 @@ static int coda_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ratelimit_default_init(&dev->mb_err_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..dcf35641c603 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>
@@ -28,6 +29,13 @@
 #define CODA_MAX_FRAMEBUFFERS	19
 #define FMO_SLICE_SAVE_BUF_SIZE	(32)
 
+/*
+ * This control allows applications to read the per-stream
+ * (i.e. per-context) Macroblocks Error Count. This value
+ * is CODA specific.
+ */
+#define V4L2_CID_CODA_MB_ERR_CNT (V4L2_CID_USER_CODA_BASE + 0)
+
 enum {
 	V4L2_M2M_SRC = 0,
 	V4L2_M2M_DST = 1,
@@ -92,6 +100,7 @@ struct coda_dev {
 	struct v4l2_m2m_dev	*m2m_dev;
 	struct ida		ida;
 	struct dentry		*debugfs_root;
+	struct ratelimit_state	mb_err_rs;
 };
 
 struct coda_codec {
@@ -242,6 +251,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		*mb_err_cnt_ctrl;
 	struct v4l2_fh			fh;
 	int				gopcounter;
 	int				runcounter;
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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-04 17:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 21:32 [PATCH v3 0/2] CODA timeout and macroblock error control Ezequiel Garcia
2020-11-03 21:32 ` [PATCH v3 1/2] coda: coda_buffer_meta housekeeping fix Ezequiel Garcia
2020-11-03 21:32 ` [PATCH v3 2/2] coda: Add a V4L2 user for control error macroblocks count Ezequiel Garcia
2020-11-04 10:42   ` Hans Verkuil
2020-11-04 17:43   ` [PATCH v4] " Ezequiel Garcia

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).