[PATCH v2 0/5] visl: Adapt output frames for reference comparison

[PATCH v2 0/5] visl: Adapt output frames for reference comparison

[Detlev Casanova]

When using visl in automated tests, we need to have output frames that
can be compared to reference frames or hash of those to validate that
the whole pipeline is working properly.

Add a "stable_output" module parameter to make sure that a given input
stream always outputs the same frames.
This is done by skipping information like queues status and pointer
values.

This also adds some stable variation in the frames so that different
input give more different output.

Changes since v1:
 - Fix typo in parameter documentation

Detlev Casanova (5):
  media: visl: Fix params permissions/defaults mismatch
  media: visl: Add a stable_output parameter
  doc: visl: Document stable_output parameter
  visl: Add a codec specific variability parameter
  doc: visl: Document codec_variability parameter

 Documentation/admin-guide/media/visl.rst    |   9 ++
 drivers/media/test-drivers/visl/visl-core.c |  12 +-
 drivers/media/test-drivers/visl/visl-dec.c  | 152 +++++++++++++-------
 drivers/media/test-drivers/visl/visl.h      |   2 +
 4 files changed, 120 insertions(+), 55 deletions(-)

-- 
2.41.0

[PATCH v2 1/5] media: visl: Fix params permissions/defaults mismatch

[Detlev Casanova]

`false` was used as the keep_bitstream_buffers parameter permissions.
This looks more like a default value for the parameter, so change it to
0 to avoid confusion.

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/media/test-drivers/visl/visl-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
index 9970dc739ca5..df6515530fbf 100644
--- a/drivers/media/test-drivers/visl/visl-core.c
+++ b/drivers/media/test-drivers/visl/visl-core.c
@@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
 		 " the number of frames to trace with dprintk");
 
 bool keep_bitstream_buffers;
-module_param(keep_bitstream_buffers, bool, false);
+module_param(keep_bitstream_buffers, bool, 0);
 MODULE_PARM_DESC(keep_bitstream_buffers,
 		 " keep bitstream buffers in debugfs after streaming is stopped");
 
-- 
2.41.0

[PATCH v2 2/5] media: visl: Add a stable_output parameter

[Detlev Casanova]

This parameter is used to ensure that for a given input, the output
frames are always identical so that it can be compared against
a reference in automatic tests.

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/media/test-drivers/visl/visl-core.c |   5 +
 drivers/media/test-drivers/visl/visl-dec.c  | 125 +++++++++++---------
 drivers/media/test-drivers/visl/visl.h      |   1 +
 3 files changed, 77 insertions(+), 54 deletions(-)

diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
index df6515530fbf..d28d50afec02 100644
--- a/drivers/media/test-drivers/visl/visl-core.c
+++ b/drivers/media/test-drivers/visl/visl-core.c
@@ -88,6 +88,11 @@ module_param(bitstream_trace_nframes, uint, 0);
 MODULE_PARM_DESC(bitstream_trace_nframes,
 		 " the number of frames to dump the bitstream through debugfs");
 
+bool stable_output;
+module_param(stable_output, bool, 0644);
+MODULE_PARM_DESC(stable_output,
+		 " only write stable data for a given input on the output frames");
+
 static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
 	{
 		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
index 318d675e5668..61cfca49ead9 100644
--- a/drivers/media/test-drivers/visl/visl-dec.c
+++ b/drivers/media/test-drivers/visl/visl-dec.c
@@ -197,19 +197,30 @@ static void visl_tpg_fill_sequence(struct visl_ctx *ctx,
 {
 	u32 stream_ms;
 
-	stream_ms = jiffies_to_msecs(get_jiffies_64() - ctx->capture_streamon_jiffies);
-
-	scnprintf(buf, bufsz,
-		  "stream time: %02d:%02d:%02d:%03d sequence:%u timestamp:%lld field:%s",
-		  (stream_ms / (60 * 60 * 1000)) % 24,
-		  (stream_ms / (60 * 1000)) % 60,
-		  (stream_ms / 1000) % 60,
-		  stream_ms % 1000,
-		  run->dst->sequence,
-		  run->dst->vb2_buf.timestamp,
-		  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
-		  (run->dst->field == V4L2_FIELD_TOP ?
-		  " top" : " bottom") : "none");
+	if (!stable_output) {
+		stream_ms = jiffies_to_msecs(get_jiffies_64() - ctx->capture_streamon_jiffies);
+
+		scnprintf(buf, bufsz,
+			  "stream time: %02d:%02d:%02d:%03d sequence:%u timestamp:%lld field:%s",
+			  (stream_ms / (60 * 60 * 1000)) % 24,
+			  (stream_ms / (60 * 1000)) % 60,
+			  (stream_ms / 1000) % 60,
+			  stream_ms % 1000,
+			  run->dst->sequence,
+			  run->dst->vb2_buf.timestamp,
+			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
+			  (run->dst->field == V4L2_FIELD_TOP ?
+			  " top" : " bottom") : "none");
+	} else {
+		scnprintf(buf, bufsz,
+			  "sequence:%u timestamp:%lld field:%s",
+			  run->dst->sequence,
+			  run->dst->vb2_buf.timestamp,
+			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
+			  (run->dst->field == V4L2_FIELD_TOP ?
+			  " top" : " bottom") : "none");
+
+	}
 }
 
 static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
@@ -244,15 +255,17 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
 	frame_dprintk(ctx->dev, run->dst->sequence, "");
 	line++;
 
-	visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
+	if (!stable_output) {
+		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
 
-	while ((line_str = strsep(&tmp, "\n")) && strlen(line_str)) {
-		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, line_str);
-		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", line_str);
-	}
+		while ((line_str = strsep(&tmp, "\n")) && strlen(line_str)) {
+			tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, line_str);
+			frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", line_str);
+		}
 
-	frame_dprintk(ctx->dev, run->dst->sequence, "");
-	line++;
+		frame_dprintk(ctx->dev, run->dst->sequence, "");
+		line++;
+	}
 
 	scnprintf(buf,
 		  TPG_STR_BUF_SZ,
@@ -280,28 +293,30 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
 		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
 	}
 
-	line++;
-	frame_dprintk(ctx->dev, run->dst->sequence, "");
-	scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
-	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
-	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
+	if (!stable_output) {
+		line++;
+		frame_dprintk(ctx->dev, run->dst->sequence, "");
+		scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
+		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
+		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
 
-	len = 0;
-	for (i = 0; i < out_q->num_buffers; i++) {
-		char entry[] = "index: %u, state: %s, request_fd: %d, ";
-		u32 old_len = len;
-		char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
+		len = 0;
+		for (i = 0; i < out_q->num_buffers; i++) {
+			char entry[] = "index: %u, state: %s, request_fd: %d, ";
+			u32 old_len = len;
+			char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
 
-		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
-				 entry, i, q_status,
-				 to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
+			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
+					 entry, i, q_status,
+					 to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
 
-		len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
-					   &buf[len],
-					   TPG_STR_BUF_SZ - len);
+			len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
+						   &buf[len],
+						   TPG_STR_BUF_SZ - len);
 
-		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
-		frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
+			tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
+			frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
+		}
 	}
 
 	line++;
@@ -333,25 +348,27 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
 		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
 	}
 
-	line++;
-	frame_dprintk(ctx->dev, run->dst->sequence, "");
-	scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue status:");
-	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
-	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
+	if (!stable_output) {
+		line++;
+		frame_dprintk(ctx->dev, run->dst->sequence, "");
+		scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue status:");
+		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
+		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
 
-	len = 0;
-	for (i = 0; i < cap_q->num_buffers; i++) {
-		u32 old_len = len;
-		char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
+		len = 0;
+		for (i = 0; i < cap_q->num_buffers; i++) {
+			u32 old_len = len;
+			char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
 
-		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
-				 "index: %u, status: %s, timestamp: %llu, is_held: %d",
-				 cap_q->bufs[i]->index, q_status,
-				 cap_q->bufs[i]->timestamp,
-				 to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
+			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
+					 "index: %u, status: %s, timestamp: %llu, is_held: %d",
+					 cap_q->bufs[i]->index, q_status,
+					 cap_q->bufs[i]->timestamp,
+					 to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
 
-		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
-		frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
+			tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
+			frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
+		}
 	}
 }
 
diff --git a/drivers/media/test-drivers/visl/visl.h b/drivers/media/test-drivers/visl/visl.h
index 31639f2e593d..5a81b493f121 100644
--- a/drivers/media/test-drivers/visl/visl.h
+++ b/drivers/media/test-drivers/visl/visl.h
@@ -85,6 +85,7 @@ extern unsigned int visl_dprintk_nframes;
 extern bool keep_bitstream_buffers;
 extern int bitstream_trace_frame_start;
 extern unsigned int bitstream_trace_nframes;
+extern bool stable_output;
 
 #define frame_dprintk(dev, current, fmt, arg...) \
 	do { \
-- 
2.41.0

[PATCH v2 3/5] doc: visl: Document stable_output parameter

[Detlev Casanova]

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 Documentation/admin-guide/media/visl.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/admin-guide/media/visl.rst b/Documentation/admin-guide/media/visl.rst
index 7d2dc78341c9..5b26fd943571 100644
--- a/Documentation/admin-guide/media/visl.rst
+++ b/Documentation/admin-guide/media/visl.rst
@@ -49,6 +49,10 @@ Module parameters
   visl_dprintk_frame_start, visl_dprintk_nframes, but controls the dumping of
   buffer data through debugfs instead.
 
+- stable_output: Limit the information written on each output frame to make
+  sure that, for a given input, the output frames are always exactly the same.
+  This is useful for automated tests to check that output frames are correct.
+
 What is the default use case for this driver?
 ---------------------------------------------
 
-- 
2.41.0

[PATCH v2 4/5] visl: Add a codec specific variability parameter

[Detlev Casanova]

When running tests with different input data, the stable output frames
could be too similar and hide possible issues.

This commit adds variation by using some codec specific parameters.

Only HEVC and H.264 support this.

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/media/test-drivers/visl/visl-core.c |  5 ++++
 drivers/media/test-drivers/visl/visl-dec.c  | 27 +++++++++++++++++++++
 drivers/media/test-drivers/visl/visl.h      |  1 +
 3 files changed, 33 insertions(+)

diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
index d28d50afec02..e7466f6a91e1 100644
--- a/drivers/media/test-drivers/visl/visl-core.c
+++ b/drivers/media/test-drivers/visl/visl-core.c
@@ -93,6 +93,11 @@ module_param(stable_output, bool, 0644);
 MODULE_PARM_DESC(stable_output,
 		 " only write stable data for a given input on the output frames");
 
+bool codec_variability;
+module_param(codec_variability, bool, 0644);
+MODULE_PARM_DESC(codec_variability,
+		 " add codec specific variability data to generate more unique frames. (Only h.264 and hevc)");
+
 static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
 	{
 		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
index 61cfca49ead9..002d5e3b0ea4 100644
--- a/drivers/media/test-drivers/visl/visl-dec.c
+++ b/drivers/media/test-drivers/visl/visl-dec.c
@@ -223,6 +223,26 @@ static void visl_tpg_fill_sequence(struct visl_ctx *ctx,
 	}
 }
 
+static bool visl_tpg_fill_codec_specific(struct visl_ctx *ctx,
+					 struct visl_run *run,
+					 char buf[], size_t bufsz)
+{
+	switch (ctx->current_codec) {
+	case VISL_CODEC_H264:
+		scnprintf(buf, bufsz,
+			  "H264: %u", run->h264.dpram->pic_order_cnt_lsb);
+		break;
+	case VISL_CODEC_HEVC:
+		scnprintf(buf, bufsz,
+			  "HEVC: %d", run->hevc.dpram->pic_order_cnt_val);
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
 static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
 {
 	u8 *basep[TPG_MAX_PLANES][2];
@@ -255,6 +275,13 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
 	frame_dprintk(ctx->dev, run->dst->sequence, "");
 	line++;
 
+	if (codec_variability && visl_tpg_fill_codec_specific(ctx, run, buf, TPG_STR_BUF_SZ)) {
+		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
+		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
+		frame_dprintk(ctx->dev, run->dst->sequence, "");
+		line++;
+	}
+
 	if (!stable_output) {
 		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
 
diff --git a/drivers/media/test-drivers/visl/visl.h b/drivers/media/test-drivers/visl/visl.h
index 5a81b493f121..4ac2d1783020 100644
--- a/drivers/media/test-drivers/visl/visl.h
+++ b/drivers/media/test-drivers/visl/visl.h
@@ -86,6 +86,7 @@ extern bool keep_bitstream_buffers;
 extern int bitstream_trace_frame_start;
 extern unsigned int bitstream_trace_nframes;
 extern bool stable_output;
+extern bool codec_variability;
 
 #define frame_dprintk(dev, current, fmt, arg...) \
 	do { \
-- 
2.41.0

[PATCH v2 5/5] doc: visl: Document codec_variability parameter

[Detlev Casanova]

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 Documentation/admin-guide/media/visl.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/admin-guide/media/visl.rst b/Documentation/admin-guide/media/visl.rst
index 5b26fd943571..56d2e9ab72cc 100644
--- a/Documentation/admin-guide/media/visl.rst
+++ b/Documentation/admin-guide/media/visl.rst
@@ -53,6 +53,11 @@ Module parameters
   sure that, for a given input, the output frames are always exactly the same.
   This is useful for automated tests to check that output frames are correct.
 
+- codec_variability: Add codec specific variability in the ouput frames. It
+  adds a text line on the ouptut frames containing parameters that is specific
+  to the format of the input stream to ensure that different inputs do not give
+  the same output.
+
 What is the default use case for this driver?
 ---------------------------------------------
 
-- 
2.41.0

Re: [PATCH v2 1/5] media: visl: Fix params permissions/defaults mismatch

[Hans Verkuil]

On 24/10/2023 21:09, Detlev Casanova wrote:
> `false` was used as the keep_bitstream_buffers parameter permissions.
> This looks more like a default value for the parameter, so change it to
> 0 to avoid confusion.
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  drivers/media/test-drivers/visl/visl-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
> index 9970dc739ca5..df6515530fbf 100644
> --- a/drivers/media/test-drivers/visl/visl-core.c
> +++ b/drivers/media/test-drivers/visl/visl-core.c
> @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
>  		 " the number of frames to trace with dprintk");
>  
>  bool keep_bitstream_buffers;
> -module_param(keep_bitstream_buffers, bool, false);
> +module_param(keep_bitstream_buffers, bool, 0);

???

This last parameter is the permission, it makes no sense that this
is either 0 or false: then nobody can see it in /sys/modules/.

Typically this is 0444 if it is readable only, or 0644 if it can be
written by root.

Regards,

	Hans

>  MODULE_PARM_DESC(keep_bitstream_buffers,
>  		 " keep bitstream buffers in debugfs after streaming is stopped");
>  

Re: [PATCH v2 2/5] media: visl: Add a stable_output parameter

[Hans Verkuil]

On 24/10/2023 21:09, Detlev Casanova wrote:
> This parameter is used to ensure that for a given input, the output
> frames are always identical so that it can be compared against
> a reference in automatic tests.
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  drivers/media/test-drivers/visl/visl-core.c |   5 +
>  drivers/media/test-drivers/visl/visl-dec.c  | 125 +++++++++++---------
>  drivers/media/test-drivers/visl/visl.h      |   1 +
>  3 files changed, 77 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
> index df6515530fbf..d28d50afec02 100644
> --- a/drivers/media/test-drivers/visl/visl-core.c
> +++ b/drivers/media/test-drivers/visl/visl-core.c
> @@ -88,6 +88,11 @@ module_param(bitstream_trace_nframes, uint, 0);
>  MODULE_PARM_DESC(bitstream_trace_nframes,
>  		 " the number of frames to dump the bitstream through debugfs");
>  
> +bool stable_output;
> +module_param(stable_output, bool, 0644);
> +MODULE_PARM_DESC(stable_output,
> +		 " only write stable data for a given input on the output frames");
> +
>  static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
>  	{
>  		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> index 318d675e5668..61cfca49ead9 100644
> --- a/drivers/media/test-drivers/visl/visl-dec.c
> +++ b/drivers/media/test-drivers/visl/visl-dec.c
> @@ -197,19 +197,30 @@ static void visl_tpg_fill_sequence(struct visl_ctx *ctx,
>  {
>  	u32 stream_ms;
>  
> -	stream_ms = jiffies_to_msecs(get_jiffies_64() - ctx->capture_streamon_jiffies);
> -
> -	scnprintf(buf, bufsz,
> -		  "stream time: %02d:%02d:%02d:%03d sequence:%u timestamp:%lld field:%s",
> -		  (stream_ms / (60 * 60 * 1000)) % 24,
> -		  (stream_ms / (60 * 1000)) % 60,
> -		  (stream_ms / 1000) % 60,
> -		  stream_ms % 1000,
> -		  run->dst->sequence,
> -		  run->dst->vb2_buf.timestamp,
> -		  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
> -		  (run->dst->field == V4L2_FIELD_TOP ?
> -		  " top" : " bottom") : "none");
> +	if (!stable_output) {
> +		stream_ms = jiffies_to_msecs(get_jiffies_64() - ctx->capture_streamon_jiffies);
> +
> +		scnprintf(buf, bufsz,
> +			  "stream time: %02d:%02d:%02d:%03d sequence:%u timestamp:%lld field:%s",
> +			  (stream_ms / (60 * 60 * 1000)) % 24,
> +			  (stream_ms / (60 * 1000)) % 60,
> +			  (stream_ms / 1000) % 60,
> +			  stream_ms % 1000,

How useful is this 'stream time' anyway? I don't think this adds anything
useful.

> +			  run->dst->sequence,
> +			  run->dst->vb2_buf.timestamp,
> +			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
> +			  (run->dst->field == V4L2_FIELD_TOP ?
> +			  " top" : " bottom") : "none");
> +	} else {
> +		scnprintf(buf, bufsz,
> +			  "sequence:%u timestamp:%lld field:%s",
> +			  run->dst->sequence,
> +			  run->dst->vb2_buf.timestamp,
> +			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
> +			  (run->dst->field == V4L2_FIELD_TOP ?
> +			  " top" : " bottom") : "none");
> +
> +	}
>  }
>  
>  static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> @@ -244,15 +255,17 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
>  	frame_dprintk(ctx->dev, run->dst->sequence, "");
>  	line++;
>  
> -	visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);

This function shows both the ts of the ref frames and the buffer
index. Is it just the buffer index that causes the problem? If so,
then wouldn't it be better to either never show the buffer index
or only if !stable_output.

> +	if (!stable_output) {
> +		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
>  
> -	while ((line_str = strsep(&tmp, "\n")) && strlen(line_str)) {
> -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, line_str);
> -		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", line_str);
> -	}
> +		while ((line_str = strsep(&tmp, "\n")) && strlen(line_str)) {
> +			tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, line_str);
> +			frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", line_str);
> +		}
>  
> -	frame_dprintk(ctx->dev, run->dst->sequence, "");
> -	line++;
> +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> +		line++;
> +	}
>  
>  	scnprintf(buf,
>  		  TPG_STR_BUF_SZ,
> @@ -280,28 +293,30 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
>  		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
>  	}
>  
> -	line++;
> -	frame_dprintk(ctx->dev, run->dst->sequence, "");
> -	scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
> -	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
> -	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
> +	if (!stable_output) {
> +		line++;
> +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> +		scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
> +		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
> +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
>  
> -	len = 0;
> -	for (i = 0; i < out_q->num_buffers; i++) {
> -		char entry[] = "index: %u, state: %s, request_fd: %d, ";
> -		u32 old_len = len;
> -		char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
> +		len = 0;
> +		for (i = 0; i < out_q->num_buffers; i++) {
> +			char entry[] = "index: %u, state: %s, request_fd: %d, ";
> +			u32 old_len = len;
> +			char *q_status = visl_get_vb2_state(out_q->bufs[i]->state);
>  
> -		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> -				 entry, i, q_status,
> -				 to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
> +			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> +					 entry, i, q_status,
> +					 to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
>  
> -		len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
> -					   &buf[len],
> -					   TPG_STR_BUF_SZ - len);
> +			len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
> +						   &buf[len],
> +						   TPG_STR_BUF_SZ - len);
>  
> -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
> -		frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
> +			tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
> +			frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
> +		}
>  	}
>  
>  	line++;
> @@ -333,25 +348,27 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
>  		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
>  	}
>  
> -	line++;
> -	frame_dprintk(ctx->dev, run->dst->sequence, "");
> -	scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue status:");
> -	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
> -	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
> +	if (!stable_output) {
> +		line++;
> +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> +		scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue status:");
> +		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
> +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
>  
> -	len = 0;
> -	for (i = 0; i < cap_q->num_buffers; i++) {
> -		u32 old_len = len;
> -		char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
> +		len = 0;
> +		for (i = 0; i < cap_q->num_buffers; i++) {
> +			u32 old_len = len;
> +			char *q_status = visl_get_vb2_state(cap_q->bufs[i]->state);
>  
> -		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> -				 "index: %u, status: %s, timestamp: %llu, is_held: %d",
> -				 cap_q->bufs[i]->index, q_status,
> -				 cap_q->bufs[i]->timestamp,
> -				 to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
> +			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> +					 "index: %u, status: %s, timestamp: %llu, is_held: %d",
> +					 cap_q->bufs[i]->index, q_status,
> +					 cap_q->bufs[i]->timestamp,
> +					 to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
>  
> -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
> -		frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
> +			tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, &buf[old_len]);
> +			frame_dprintk(ctx->dev, run->dst->sequence, "%s", &buf[old_len]);
> +		}
>  	}
>  }
>  
> diff --git a/drivers/media/test-drivers/visl/visl.h b/drivers/media/test-drivers/visl/visl.h
> index 31639f2e593d..5a81b493f121 100644
> --- a/drivers/media/test-drivers/visl/visl.h
> +++ b/drivers/media/test-drivers/visl/visl.h
> @@ -85,6 +85,7 @@ extern unsigned int visl_dprintk_nframes;
>  extern bool keep_bitstream_buffers;
>  extern int bitstream_trace_frame_start;
>  extern unsigned int bitstream_trace_nframes;
> +extern bool stable_output;
>  
>  #define frame_dprintk(dev, current, fmt, arg...) \
>  	do { \

Should stable_output perhaps be 1 by default?

Regards,

	Hans

Re: [PATCH v2 4/5] visl: Add a codec specific variability parameter

[Hans Verkuil]

On 24/10/2023 21:09, Detlev Casanova wrote:
> When running tests with different input data, the stable output frames
> could be too similar and hide possible issues.
> 
> This commit adds variation by using some codec specific parameters.
> 
> Only HEVC and H.264 support this.
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  drivers/media/test-drivers/visl/visl-core.c |  5 ++++
>  drivers/media/test-drivers/visl/visl-dec.c  | 27 +++++++++++++++++++++
>  drivers/media/test-drivers/visl/visl.h      |  1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
> index d28d50afec02..e7466f6a91e1 100644
> --- a/drivers/media/test-drivers/visl/visl-core.c
> +++ b/drivers/media/test-drivers/visl/visl-core.c
> @@ -93,6 +93,11 @@ module_param(stable_output, bool, 0644);
>  MODULE_PARM_DESC(stable_output,
>  		 " only write stable data for a given input on the output frames");
>  
> +bool codec_variability;
> +module_param(codec_variability, bool, 0644);
> +MODULE_PARM_DESC(codec_variability,
> +		 " add codec specific variability data to generate more unique frames. (Only h.264 and hevc)");

Why make this a module parameter instead of always doing this?

It's not clear from the commit log why a parameter is needed.

> +
>  static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
>  	{
>  		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
> diff --git a/drivers/media/test-drivers/visl/visl-dec.c b/drivers/media/test-drivers/visl/visl-dec.c
> index 61cfca49ead9..002d5e3b0ea4 100644
> --- a/drivers/media/test-drivers/visl/visl-dec.c
> +++ b/drivers/media/test-drivers/visl/visl-dec.c
> @@ -223,6 +223,26 @@ static void visl_tpg_fill_sequence(struct visl_ctx *ctx,
>  	}
>  }
>  
> +static bool visl_tpg_fill_codec_specific(struct visl_ctx *ctx,
> +					 struct visl_run *run,
> +					 char buf[], size_t bufsz)
> +{
> +	switch (ctx->current_codec) {
> +	case VISL_CODEC_H264:
> +		scnprintf(buf, bufsz,
> +			  "H264: %u", run->h264.dpram->pic_order_cnt_lsb);
> +		break;
> +	case VISL_CODEC_HEVC:
> +		scnprintf(buf, bufsz,
> +			  "HEVC: %d", run->hevc.dpram->pic_order_cnt_val);
> +		break;

Perhaps mention here why these specific values are chosen?

> +	default:
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
>  {
>  	u8 *basep[TPG_MAX_PLANES][2];
> @@ -255,6 +275,13 @@ static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
>  	frame_dprintk(ctx->dev, run->dst->sequence, "");
>  	line++;
>  
> +	if (codec_variability && visl_tpg_fill_codec_specific(ctx, run, buf, TPG_STR_BUF_SZ)) {
> +		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
> +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
> +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> +		line++;
> +	}
> +
>  	if (!stable_output) {
>  		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
>  
> diff --git a/drivers/media/test-drivers/visl/visl.h b/drivers/media/test-drivers/visl/visl.h
> index 5a81b493f121..4ac2d1783020 100644
> --- a/drivers/media/test-drivers/visl/visl.h
> +++ b/drivers/media/test-drivers/visl/visl.h
> @@ -86,6 +86,7 @@ extern bool keep_bitstream_buffers;
>  extern int bitstream_trace_frame_start;
>  extern unsigned int bitstream_trace_nframes;
>  extern bool stable_output;
> +extern bool codec_variability;
>  
>  #define frame_dprintk(dev, current, fmt, arg...) \
>  	do { \

Regards,

	Hans

Re: [PATCH v2 1/5] media: visl: Fix params permissions/defaults mismatch

[Detlev Casanova]

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

On Wednesday, November 22, 2023 10:56:20 A.M. EST Hans Verkuil wrote:
> On 24/10/2023 21:09, Detlev Casanova wrote:
> > `false` was used as the keep_bitstream_buffers parameter permissions.
> > This looks more like a default value for the parameter, so change it to
> > 0 to avoid confusion.
> > 
> > Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/media/test-drivers/visl/visl-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl-core.c
> > b/drivers/media/test-drivers/visl/visl-core.c index
> > 9970dc739ca5..df6515530fbf 100644
> > --- a/drivers/media/test-drivers/visl/visl-core.c
> > +++ b/drivers/media/test-drivers/visl/visl-core.c
> > @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
> > 
> >  		 " the number of frames to trace with dprintk");
> >  
> >  bool keep_bitstream_buffers;
> > 
> > -module_param(keep_bitstream_buffers, bool, false);
> > +module_param(keep_bitstream_buffers, bool, 0);
> 
> ???
> 
> This last parameter is the permission, it makes no sense that this
> is either 0 or false: then nobody can see it in /sys/modules/.

It makes sense if we want it set when the module is loaded only. This way, we 
don't have to manage the parameters values changing while work is being done 
and keep it simple.
It could be made readable if that looks better, but there is no real need for 
it to be read either.

> Typically this is 0444 if it is readable only, or 0644 if it can be
> written by root.
> 
> Regards,
> 
> 	Hans
> 
> >  MODULE_PARM_DESC(keep_bitstream_buffers,
> >  
> >  		 " keep bitstream buffers in debugfs after streaming is 
stopped");


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/5] media: visl: Add a stable_output parameter

[Detlev Casanova]

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

On Wednesday, November 22, 2023 11:03:53 A.M. EST Hans Verkuil wrote:
> On 24/10/2023 21:09, Detlev Casanova wrote:
> > This parameter is used to ensure that for a given input, the output
> > frames are always identical so that it can be compared against
> > a reference in automatic tests.
> > 
> > Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/media/test-drivers/visl/visl-core.c |   5 +
> >  drivers/media/test-drivers/visl/visl-dec.c  | 125 +++++++++++---------
> >  drivers/media/test-drivers/visl/visl.h      |   1 +
> >  3 files changed, 77 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl-core.c
> > b/drivers/media/test-drivers/visl/visl-core.c index
> > df6515530fbf..d28d50afec02 100644
> > --- a/drivers/media/test-drivers/visl/visl-core.c
> > +++ b/drivers/media/test-drivers/visl/visl-core.c
> > @@ -88,6 +88,11 @@ module_param(bitstream_trace_nframes, uint, 0);
> > 
> >  MODULE_PARM_DESC(bitstream_trace_nframes,
> >  
> >  		 " the number of frames to dump the bitstream through 
debugfs");
> > 
> > +bool stable_output;
> > +module_param(stable_output, bool, 0644);
> > +MODULE_PARM_DESC(stable_output,
> > +		 " only write stable data for a given input on the 
output frames");
> > +
> > 
> >  static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
> >  
> >  	{
> >  	
> >  		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl-dec.c
> > b/drivers/media/test-drivers/visl/visl-dec.c index
> > 318d675e5668..61cfca49ead9 100644
> > --- a/drivers/media/test-drivers/visl/visl-dec.c
> > +++ b/drivers/media/test-drivers/visl/visl-dec.c
> > @@ -197,19 +197,30 @@ static void visl_tpg_fill_sequence(struct visl_ctx
> > *ctx,> 
> >  {
> >  
> >  	u32 stream_ms;
> > 
> > -	stream_ms = jiffies_to_msecs(get_jiffies_64() -
> > ctx->capture_streamon_jiffies); -
> > -	scnprintf(buf, bufsz,
> > -		  "stream time: %02d:%02d:%02d:%03d sequence:%u 
timestamp:%lld
> > field:%s", -		  (stream_ms / (60 * 60 * 1000)) % 24,
> > -		  (stream_ms / (60 * 1000)) % 60,
> > -		  (stream_ms / 1000) % 60,
> > -		  stream_ms % 1000,
> > -		  run->dst->sequence,
> > -		  run->dst->vb2_buf.timestamp,
> > -		  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
> > -		  (run->dst->field == V4L2_FIELD_TOP ?
> > -		  " top" : " bottom") : "none");
> > +	if (!stable_output) {
> > +		stream_ms = jiffies_to_msecs(get_jiffies_64() -
> > ctx->capture_streamon_jiffies); +
> > +		scnprintf(buf, bufsz,
> > +			  "stream time: %02d:%02d:%02d:%03d 
sequence:%u timestamp:%lld
> > field:%s", +			  (stream_ms / (60 * 60 * 
1000)) % 24,
> > +			  (stream_ms / (60 * 1000)) % 60,
> > +			  (stream_ms / 1000) % 60,
> > +			  stream_ms % 1000,
> 
> How useful is this 'stream time' anyway? I don't think this adds anything
> useful.

I suppose that the more debug information is shown, the better.

> > +			  run->dst->sequence,
> > +			  run->dst->vb2_buf.timestamp,
> > +			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
> > +			  (run->dst->field == V4L2_FIELD_TOP ?
> > +			  " top" : " bottom") : "none");
> > +	} else {
> > +		scnprintf(buf, bufsz,
> > +			  "sequence:%u timestamp:%lld field:%s",
> > +			  run->dst->sequence,
> > +			  run->dst->vb2_buf.timestamp,
> > +			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
> > +			  (run->dst->field == V4L2_FIELD_TOP ?
> > +			  " top" : " bottom") : "none");
> > +
> > +	}
> > 
> >  }
> >  
> >  static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> > 
> > @@ -244,15 +255,17 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
> > struct visl_run *run)> 
> >  	frame_dprintk(ctx->dev, run->dst->sequence, "");
> >  	line++;
> > 
> > -	visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
> 
> This function shows both the ts of the ref frames and the buffer
> index. Is it just the buffer index that causes the problem? If so,
> then wouldn't it be better to either never show the buffer index
> or only if !stable_output.

Indeed, the buffer index is the issue, but I did not check if the ref frames ts 
are stable. I'll do some tests with it and keep the ref frames in stable 
output mode if they are stable.

> > +	if (!stable_output) {
> > +		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
> > 
> > -	while ((line_str = strsep(&tmp, "\n")) && strlen(line_str)) {
> > -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
16, line_str);
> > -		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
line_str);
> > -	}
> > +		while ((line_str = strsep(&tmp, "\n")) && 
strlen(line_str)) {
> > +			tpg_gen_text(&ctx->tpg, basep, line++ * 
line_height, 16, line_str);
> > +			frame_dprintk(ctx->dev, run->dst->sequence, 
"%s\n", line_str);
> > +		}
> > 
> > -	frame_dprintk(ctx->dev, run->dst->sequence, "");
> > -	line++;
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> > +		line++;
> > +	}
> > 
> >  	scnprintf(buf,
> >  	
> >  		  TPG_STR_BUF_SZ,
> > 
> > @@ -280,28 +293,30 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
> > struct visl_run *run)> 
> >  		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
buf);
> >  	
> >  	}
> > 
> > -	line++;
> > -	frame_dprintk(ctx->dev, run->dst->sequence, "");
> > -	scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
> > -	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
> > -	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
> > +	if (!stable_output) {
> > +		line++;
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> > +		scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
> > +		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
16, buf);
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
buf);
> > 
> > -	len = 0;
> > -	for (i = 0; i < out_q->num_buffers; i++) {
> > -		char entry[] = "index: %u, state: %s, request_fd: %d, 
";
> > -		u32 old_len = len;
> > -		char *q_status = visl_get_vb2_state(out_q->bufs[i]-
>state);
> > +		len = 0;
> > +		for (i = 0; i < out_q->num_buffers; i++) {
> > +			char entry[] = "index: %u, state: %s, 
request_fd: %d, ";
> > +			u32 old_len = len;
> > +			char *q_status = visl_get_vb2_state(out_q-
>bufs[i]->state);
> > 
> > -		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> > -				 entry, i, q_status,
> > -				 to_vb2_v4l2_buffer(out_q-
>bufs[i])->request_fd);
> > +			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - 
len,
> > +					 entry, i, q_status,
> > +					 
to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
> > 
> > -		len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q-
>bufs[i]),
> > -					   &buf[len],
> > -					   TPG_STR_BUF_SZ - 
len);
> > +			len += 
visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
> > +						   
&buf[len],
> > +						   
TPG_STR_BUF_SZ - len);
> > 
> > -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
16,
> > &buf[old_len]);
> > -		frame_dprintk(ctx->dev, run->dst->sequence, "%s", 
&buf[old_len]);
> > +			tpg_gen_text(&ctx->tpg, basep, line++ * 
line_height, 16,
> > &buf[old_len]); +			frame_dprintk(ctx->dev, run-
>dst->sequence, "%s",
> > &buf[old_len]); +		}
> > 
> >  	}
> >  	
> >  	line++;
> > 
> > @@ -333,25 +348,27 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
> > struct visl_run *run)> 
> >  		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
buf);
> >  	
> >  	}
> > 
> > -	line++;
> > -	frame_dprintk(ctx->dev, run->dst->sequence, "");
> > -	scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue status:");
> > -	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
> > -	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
> > +	if (!stable_output) {
> > +		line++;
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> > +		scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue 
status:");
> > +		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
16, buf);
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
buf);
> > 
> > -	len = 0;
> > -	for (i = 0; i < cap_q->num_buffers; i++) {
> > -		u32 old_len = len;
> > -		char *q_status = visl_get_vb2_state(cap_q->bufs[i]-
>state);
> > +		len = 0;
> > +		for (i = 0; i < cap_q->num_buffers; i++) {
> > +			u32 old_len = len;
> > +			char *q_status = visl_get_vb2_state(cap_q-
>bufs[i]->state);
> > 
> > -		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
> > -				 "index: %u, status: %s, 
timestamp: %llu, is_held: %d",
> > -				 cap_q->bufs[i]->index, q_status,
> > -				 cap_q->bufs[i]->timestamp,
> > -				 to_vb2_v4l2_buffer(cap_q-
>bufs[i])->is_held);
> > +			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - 
len,
> > +					 "index: %u, status: 
%s, timestamp: %llu, is_held: %d",
> > +					 cap_q->bufs[i]-
>index, q_status,
> > +					 cap_q->bufs[i]-
>timestamp,
> > +					 
to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
> > 
> > -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
16,
> > &buf[old_len]);
> > -		frame_dprintk(ctx->dev, run->dst->sequence, "%s", 
&buf[old_len]);
> > +			tpg_gen_text(&ctx->tpg, basep, line++ * 
line_height, 16,
> > &buf[old_len]); +			frame_dprintk(ctx->dev, run-
>dst->sequence, "%s",
> > &buf[old_len]); +		}
> > 
> >  	}
> >  
> >  }
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl.h
> > b/drivers/media/test-drivers/visl/visl.h index 31639f2e593d..5a81b493f121
> > 100644
> > --- a/drivers/media/test-drivers/visl/visl.h
> > +++ b/drivers/media/test-drivers/visl/visl.h
> > @@ -85,6 +85,7 @@ extern unsigned int visl_dprintk_nframes;
> > 
> >  extern bool keep_bitstream_buffers;
> >  extern int bitstream_trace_frame_start;
> >  extern unsigned int bitstream_trace_nframes;
> > 
> > +extern bool stable_output;
> > 
> >  #define frame_dprintk(dev, current, fmt, arg...) \
> >  
> >  	do { \
> 
> Should stable_output perhaps be 1 by default?

In that case, why not use the visl_debug parameter and show the unstable data 
only when it is set to one ?

--
Detlev

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/5] visl: Add a codec specific variability parameter

[Detlev Casanova]

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

On Wednesday, November 22, 2023 11:07:18 A.M. EST Hans Verkuil wrote:
> On 24/10/2023 21:09, Detlev Casanova wrote:
> > When running tests with different input data, the stable output frames
> > could be too similar and hide possible issues.
> > 
> > This commit adds variation by using some codec specific parameters.
> > 
> > Only HEVC and H.264 support this.
> > 
> > Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  drivers/media/test-drivers/visl/visl-core.c |  5 ++++
> >  drivers/media/test-drivers/visl/visl-dec.c  | 27 +++++++++++++++++++++
> >  drivers/media/test-drivers/visl/visl.h      |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl-core.c
> > b/drivers/media/test-drivers/visl/visl-core.c index
> > d28d50afec02..e7466f6a91e1 100644
> > --- a/drivers/media/test-drivers/visl/visl-core.c
> > +++ b/drivers/media/test-drivers/visl/visl-core.c
> > @@ -93,6 +93,11 @@ module_param(stable_output, bool, 0644);
> > 
> >  MODULE_PARM_DESC(stable_output,
> >  
> >  		 " only write stable data for a given input on the 
output frames");
> > 
> > +bool codec_variability;
> > +module_param(codec_variability, bool, 0644);
> > +MODULE_PARM_DESC(codec_variability,
> > +		 " add codec specific variability data to generate more 
unique frames.
> > (Only h.264 and hevc)");
> Why make this a module parameter instead of always doing this?
> 
> It's not clear from the commit log why a parameter is needed.

I agree with that, I started as a parameter when I wasn't sure what 
variability values or method would be used, but just showing a value can be 
integrated without a parameter and keep it more simple. I'll change that.

> > +
> > 
> >  static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
> >  
> >  	{
> >  	
> >  		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl-dec.c
> > b/drivers/media/test-drivers/visl/visl-dec.c index
> > 61cfca49ead9..002d5e3b0ea4 100644
> > --- a/drivers/media/test-drivers/visl/visl-dec.c
> > +++ b/drivers/media/test-drivers/visl/visl-dec.c
> > @@ -223,6 +223,26 @@ static void visl_tpg_fill_sequence(struct visl_ctx
> > *ctx,> 
> >  	}
> >  
> >  }
> > 
> > +static bool visl_tpg_fill_codec_specific(struct visl_ctx *ctx,
> > +					 struct visl_run *run,
> > +					 char buf[], size_t 
bufsz)
> > +{
> > +	switch (ctx->current_codec) {
> > +	case VISL_CODEC_H264:
> > +		scnprintf(buf, bufsz,
> > +			  "H264: %u", run->h264.dpram-
>pic_order_cnt_lsb);
> > +		break;
> > +	case VISL_CODEC_HEVC:
> > +		scnprintf(buf, bufsz,
> > +			  "HEVC: %d", run->hevc.dpram-
>pic_order_cnt_val);
> > +		break;
> 
> Perhaps mention here why these specific values are chosen?

Will do.

> > +	default:
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > 
> >  static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
> >  {
> >  
> >  	u8 *basep[TPG_MAX_PLANES][2];
> > 
> > @@ -255,6 +275,13 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
> > struct visl_run *run)> 
> >  	frame_dprintk(ctx->dev, run->dst->sequence, "");
> >  	line++;
> > 
> > +	if (codec_variability && visl_tpg_fill_codec_specific(ctx, run, buf,
> > TPG_STR_BUF_SZ)) { +		tpg_gen_text(&ctx->tpg, basep, line++ *
> > line_height, 16, buf);
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
buf);
> > +		frame_dprintk(ctx->dev, run->dst->sequence, "");
> > +		line++;
> > +	}
> > +
> > 
> >  	if (!stable_output) {
> >  	
> >  		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
> > 
> > diff --git a/drivers/media/test-drivers/visl/visl.h
> > b/drivers/media/test-drivers/visl/visl.h index 5a81b493f121..4ac2d1783020
> > 100644
> > --- a/drivers/media/test-drivers/visl/visl.h
> > +++ b/drivers/media/test-drivers/visl/visl.h
> > @@ -86,6 +86,7 @@ extern bool keep_bitstream_buffers;
> > 
> >  extern int bitstream_trace_frame_start;
> >  extern unsigned int bitstream_trace_nframes;
> >  extern bool stable_output;
> > 
> > +extern bool codec_variability;
> > 
> >  #define frame_dprintk(dev, current, fmt, arg...) \
> >  
> >  	do { \
> 
> Regards,
> 
> 	Hans


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/5] media: visl: Fix params permissions/defaults mismatch

[Hans Verkuil]

On 22/11/2023 17:38, Detlev Casanova wrote:
> On Wednesday, November 22, 2023 10:56:20 A.M. EST Hans Verkuil wrote:
>> On 24/10/2023 21:09, Detlev Casanova wrote:
>>> `false` was used as the keep_bitstream_buffers parameter permissions.
>>> This looks more like a default value for the parameter, so change it to
>>> 0 to avoid confusion.
>>>
>>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>
>>>  drivers/media/test-drivers/visl/visl-core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/test-drivers/visl/visl-core.c
>>> b/drivers/media/test-drivers/visl/visl-core.c index
>>> 9970dc739ca5..df6515530fbf 100644
>>> --- a/drivers/media/test-drivers/visl/visl-core.c
>>> +++ b/drivers/media/test-drivers/visl/visl-core.c
>>> @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
>>>
>>>  		 " the number of frames to trace with dprintk");
>>>  
>>>  bool keep_bitstream_buffers;
>>>
>>> -module_param(keep_bitstream_buffers, bool, false);
>>> +module_param(keep_bitstream_buffers, bool, 0);
>>
>> ???
>>
>> This last parameter is the permission, it makes no sense that this
>> is either 0 or false: then nobody can see it in /sys/modules/.
> 
> It makes sense if we want it set when the module is loaded only. This way, we 
> don't have to manage the parameters values changing while work is being done 
> and keep it simple.
> It could be made readable if that looks better, but there is no real need for 
> it to be read either.

Why not? It makes it easy to read what this module option's value is.

I now see that both visl and vidtv uses 0 a lot, so I'm OK with this
patch for consistency.

But I think especially these test-drivers should use 0444 instead of 0
so you can see how the test driver is configured. That might actually
be relevant when writing tests using these drivers.

Perhaps separate patches for visl and vidtv that change 0 to 0444 for all
the module parameters?

Regards,

	Hans

> 
>> Typically this is 0444 if it is readable only, or 0644 if it can be
>> written by root.
>>
>> Regards,
>>
>> 	Hans
>>
>>>  MODULE_PARM_DESC(keep_bitstream_buffers,
>>>  
>>>  		 " keep bitstream buffers in debugfs after streaming is 
> stopped");
> 

Re: [PATCH v2 2/5] media: visl: Add a stable_output parameter

[Hans Verkuil]

On 22/11/2023 17:49, Detlev Casanova wrote:
> On Wednesday, November 22, 2023 11:03:53 A.M. EST Hans Verkuil wrote:
>> On 24/10/2023 21:09, Detlev Casanova wrote:
>>> This parameter is used to ensure that for a given input, the output
>>> frames are always identical so that it can be compared against
>>> a reference in automatic tests.
>>>
>>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>
>>>  drivers/media/test-drivers/visl/visl-core.c |   5 +
>>>  drivers/media/test-drivers/visl/visl-dec.c  | 125 +++++++++++---------
>>>  drivers/media/test-drivers/visl/visl.h      |   1 +
>>>  3 files changed, 77 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/drivers/media/test-drivers/visl/visl-core.c
>>> b/drivers/media/test-drivers/visl/visl-core.c index
>>> df6515530fbf..d28d50afec02 100644
>>> --- a/drivers/media/test-drivers/visl/visl-core.c
>>> +++ b/drivers/media/test-drivers/visl/visl-core.c
>>> @@ -88,6 +88,11 @@ module_param(bitstream_trace_nframes, uint, 0);
>>>
>>>  MODULE_PARM_DESC(bitstream_trace_nframes,
>>>  
>>>  		 " the number of frames to dump the bitstream through 
> debugfs");
>>>
>>> +bool stable_output;
>>> +module_param(stable_output, bool, 0644);
>>> +MODULE_PARM_DESC(stable_output,
>>> +		 " only write stable data for a given input on the 
> output frames");
>>> +
>>>
>>>  static const struct visl_ctrl_desc visl_fwht_ctrl_descs[] = {
>>>  
>>>  	{
>>>  	
>>>  		.cfg.id = V4L2_CID_STATELESS_FWHT_PARAMS,
>>>
>>> diff --git a/drivers/media/test-drivers/visl/visl-dec.c
>>> b/drivers/media/test-drivers/visl/visl-dec.c index
>>> 318d675e5668..61cfca49ead9 100644
>>> --- a/drivers/media/test-drivers/visl/visl-dec.c
>>> +++ b/drivers/media/test-drivers/visl/visl-dec.c
>>> @@ -197,19 +197,30 @@ static void visl_tpg_fill_sequence(struct visl_ctx
>>> *ctx,> 
>>>  {
>>>  
>>>  	u32 stream_ms;
>>>
>>> -	stream_ms = jiffies_to_msecs(get_jiffies_64() -
>>> ctx->capture_streamon_jiffies); -
>>> -	scnprintf(buf, bufsz,
>>> -		  "stream time: %02d:%02d:%02d:%03d sequence:%u 
> timestamp:%lld
>>> field:%s", -		  (stream_ms / (60 * 60 * 1000)) % 24,
>>> -		  (stream_ms / (60 * 1000)) % 60,
>>> -		  (stream_ms / 1000) % 60,
>>> -		  stream_ms % 1000,
>>> -		  run->dst->sequence,
>>> -		  run->dst->vb2_buf.timestamp,
>>> -		  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
>>> -		  (run->dst->field == V4L2_FIELD_TOP ?
>>> -		  " top" : " bottom") : "none");
>>> +	if (!stable_output) {
>>> +		stream_ms = jiffies_to_msecs(get_jiffies_64() -
>>> ctx->capture_streamon_jiffies); +
>>> +		scnprintf(buf, bufsz,
>>> +			  "stream time: %02d:%02d:%02d:%03d 
> sequence:%u timestamp:%lld
>>> field:%s", +			  (stream_ms / (60 * 60 * 
> 1000)) % 24,
>>> +			  (stream_ms / (60 * 1000)) % 60,
>>> +			  (stream_ms / 1000) % 60,
>>> +			  stream_ms % 1000,
>>
>> How useful is this 'stream time' anyway? I don't think this adds anything
>> useful.
> 
> I suppose that the more debug information is shown, the better.
> 
>>> +			  run->dst->sequence,
>>> +			  run->dst->vb2_buf.timestamp,
>>> +			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
>>> +			  (run->dst->field == V4L2_FIELD_TOP ?
>>> +			  " top" : " bottom") : "none");
>>> +	} else {
>>> +		scnprintf(buf, bufsz,
>>> +			  "sequence:%u timestamp:%lld field:%s",
>>> +			  run->dst->sequence,
>>> +			  run->dst->vb2_buf.timestamp,
>>> +			  (run->dst->field == V4L2_FIELD_ALTERNATE) ?
>>> +			  (run->dst->field == V4L2_FIELD_TOP ?
>>> +			  " top" : " bottom") : "none");
>>> +
>>> +	}
>>>
>>>  }
>>>  
>>>  static void visl_tpg_fill(struct visl_ctx *ctx, struct visl_run *run)
>>>
>>> @@ -244,15 +255,17 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
>>> struct visl_run *run)> 
>>>  	frame_dprintk(ctx->dev, run->dst->sequence, "");
>>>  	line++;
>>>
>>> -	visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
>>
>> This function shows both the ts of the ref frames and the buffer
>> index. Is it just the buffer index that causes the problem? If so,
>> then wouldn't it be better to either never show the buffer index
>> or only if !stable_output.
> 
> Indeed, the buffer index is the issue, but I did not check if the ref frames ts 
> are stable. I'll do some tests with it and keep the ref frames in stable 
> output mode if they are stable.
> 
>>> +	if (!stable_output) {
>>> +		visl_get_ref_frames(ctx, buf, TPG_STR_BUF_SZ, run);
>>>
>>> -	while ((line_str = strsep(&tmp, "\n")) && strlen(line_str)) {
>>> -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
> 16, line_str);
>>> -		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
> line_str);
>>> -	}
>>> +		while ((line_str = strsep(&tmp, "\n")) && 
> strlen(line_str)) {
>>> +			tpg_gen_text(&ctx->tpg, basep, line++ * 
> line_height, 16, line_str);
>>> +			frame_dprintk(ctx->dev, run->dst->sequence, 
> "%s\n", line_str);
>>> +		}
>>>
>>> -	frame_dprintk(ctx->dev, run->dst->sequence, "");
>>> -	line++;
>>> +		frame_dprintk(ctx->dev, run->dst->sequence, "");
>>> +		line++;
>>> +	}
>>>
>>>  	scnprintf(buf,
>>>  	
>>>  		  TPG_STR_BUF_SZ,
>>>
>>> @@ -280,28 +293,30 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
>>> struct visl_run *run)> 
>>>  		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
> buf);
>>>  	
>>>  	}
>>>
>>> -	line++;
>>> -	frame_dprintk(ctx->dev, run->dst->sequence, "");
>>> -	scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
>>> -	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
>>> -	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
>>> +	if (!stable_output) {
>>> +		line++;
>>> +		frame_dprintk(ctx->dev, run->dst->sequence, "");
>>> +		scnprintf(buf, TPG_STR_BUF_SZ, "Output queue status:");
>>> +		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
> 16, buf);
>>> +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
> buf);
>>>
>>> -	len = 0;
>>> -	for (i = 0; i < out_q->num_buffers; i++) {
>>> -		char entry[] = "index: %u, state: %s, request_fd: %d, 
> ";
>>> -		u32 old_len = len;
>>> -		char *q_status = visl_get_vb2_state(out_q->bufs[i]-
>> state);
>>> +		len = 0;
>>> +		for (i = 0; i < out_q->num_buffers; i++) {
>>> +			char entry[] = "index: %u, state: %s, 
> request_fd: %d, ";
>>> +			u32 old_len = len;
>>> +			char *q_status = visl_get_vb2_state(out_q-
>> bufs[i]->state);
>>>
>>> -		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
>>> -				 entry, i, q_status,
>>> -				 to_vb2_v4l2_buffer(out_q-
>> bufs[i])->request_fd);
>>> +			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - 
> len,
>>> +					 entry, i, q_status,
>>> +					 
> to_vb2_v4l2_buffer(out_q->bufs[i])->request_fd);
>>>
>>> -		len += visl_fill_bytesused(to_vb2_v4l2_buffer(out_q-
>> bufs[i]),
>>> -					   &buf[len],
>>> -					   TPG_STR_BUF_SZ - 
> len);
>>> +			len += 
> visl_fill_bytesused(to_vb2_v4l2_buffer(out_q->bufs[i]),
>>> +						   
> &buf[len],
>>> +						   
> TPG_STR_BUF_SZ - len);
>>>
>>> -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
> 16,
>>> &buf[old_len]);
>>> -		frame_dprintk(ctx->dev, run->dst->sequence, "%s", 
> &buf[old_len]);
>>> +			tpg_gen_text(&ctx->tpg, basep, line++ * 
> line_height, 16,
>>> &buf[old_len]); +			frame_dprintk(ctx->dev, run-
>> dst->sequence, "%s",
>>> &buf[old_len]); +		}
>>>
>>>  	}
>>>  	
>>>  	line++;
>>>
>>> @@ -333,25 +348,27 @@ static void visl_tpg_fill(struct visl_ctx *ctx,
>>> struct visl_run *run)> 
>>>  		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
> buf);
>>>  	
>>>  	}
>>>
>>> -	line++;
>>> -	frame_dprintk(ctx->dev, run->dst->sequence, "");
>>> -	scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue status:");
>>> -	tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 16, buf);
>>> -	frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", buf);
>>> +	if (!stable_output) {
>>> +		line++;
>>> +		frame_dprintk(ctx->dev, run->dst->sequence, "");
>>> +		scnprintf(buf, TPG_STR_BUF_SZ, "Capture queue 
> status:");
>>> +		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
> 16, buf);
>>> +		frame_dprintk(ctx->dev, run->dst->sequence, "%s\n", 
> buf);
>>>
>>> -	len = 0;
>>> -	for (i = 0; i < cap_q->num_buffers; i++) {
>>> -		u32 old_len = len;
>>> -		char *q_status = visl_get_vb2_state(cap_q->bufs[i]-
>> state);
>>> +		len = 0;
>>> +		for (i = 0; i < cap_q->num_buffers; i++) {
>>> +			u32 old_len = len;
>>> +			char *q_status = visl_get_vb2_state(cap_q-
>> bufs[i]->state);
>>>
>>> -		len += scnprintf(&buf[len], TPG_STR_BUF_SZ - len,
>>> -				 "index: %u, status: %s, 
> timestamp: %llu, is_held: %d",
>>> -				 cap_q->bufs[i]->index, q_status,
>>> -				 cap_q->bufs[i]->timestamp,
>>> -				 to_vb2_v4l2_buffer(cap_q-
>> bufs[i])->is_held);
>>> +			len += scnprintf(&buf[len], TPG_STR_BUF_SZ - 
> len,
>>> +					 "index: %u, status: 
> %s, timestamp: %llu, is_held: %d",
>>> +					 cap_q->bufs[i]-
>> index, q_status,
>>> +					 cap_q->bufs[i]-
>> timestamp,
>>> +					 
> to_vb2_v4l2_buffer(cap_q->bufs[i])->is_held);
>>>
>>> -		tpg_gen_text(&ctx->tpg, basep, line++ * line_height, 
> 16,
>>> &buf[old_len]);
>>> -		frame_dprintk(ctx->dev, run->dst->sequence, "%s", 
> &buf[old_len]);
>>> +			tpg_gen_text(&ctx->tpg, basep, line++ * 
> line_height, 16,
>>> &buf[old_len]); +			frame_dprintk(ctx->dev, run-
>> dst->sequence, "%s",
>>> &buf[old_len]); +		}
>>>
>>>  	}
>>>  
>>>  }
>>>
>>> diff --git a/drivers/media/test-drivers/visl/visl.h
>>> b/drivers/media/test-drivers/visl/visl.h index 31639f2e593d..5a81b493f121
>>> 100644
>>> --- a/drivers/media/test-drivers/visl/visl.h
>>> +++ b/drivers/media/test-drivers/visl/visl.h
>>> @@ -85,6 +85,7 @@ extern unsigned int visl_dprintk_nframes;
>>>
>>>  extern bool keep_bitstream_buffers;
>>>  extern int bitstream_trace_frame_start;
>>>  extern unsigned int bitstream_trace_nframes;
>>>
>>> +extern bool stable_output;
>>>
>>>  #define frame_dprintk(dev, current, fmt, arg...) \
>>>  
>>>  	do { \
>>
>> Should stable_output perhaps be 1 by default?
> 
> In that case, why not use the visl_debug parameter and show the unstable data 
> only when it is set to one ?

I don't think that's a good idea. That parameter enables driver debugging output,
and is meant to track down driver issues. It shouldn't be mixed with changing
driver behavior.

Regards,

	Hans

> 
> --
> Detlev