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
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 <[email protected]>
Signed-off-by: Detlev Casanova <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Detlev Casanova <[email protected]>
---
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
Reviewed-by: Daniel Almeida <[email protected]>
Signed-off-by: Detlev Casanova <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Detlev Casanova <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Detlev Casanova <[email protected]>
> ---
> 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
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 <[email protected]>
> > Signed-off-by: Detlev Casanova <[email protected]>
> > ---
> >
> > 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
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 <[email protected]>
> > Signed-off-by: Detlev Casanova <[email protected]>
> > ---
> >
> > 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
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 <[email protected]>
>>> Signed-off-by: Detlev Casanova <[email protected]>
>>> ---
>>>
>>> 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