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

From: Detlev Casanova
Date: Wed Nov 22 2023 - 11:49:20 EST

Next message: Andy Shevchenko: "[PATCH v1 02/17] pinctrl: nuvoton: Convert to use struct pingroup and PINCTRL_PINGROUP()"
Previous message: Pratyush Brahma: "Re: [PATCH v2 0/8] dma-buf: heaps: Add secure heap"
In reply to: Hans Verkuil: "Re: [PATCH v2 2/5] media: visl: Add a stable_output parameter"
Next in thread: Hans Verkuil: "Re: [PATCH v2 2/5] media: visl: Add a stable_output parameter"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx>
> > Signed-off-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>
> > ---
> >
> > 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: signature.asc
Description: This is a digitally signed message part.

Next message: Andy Shevchenko: "[PATCH v1 02/17] pinctrl: nuvoton: Convert to use struct pingroup and PINCTRL_PINGROUP()"
Previous message: Pratyush Brahma: "Re: [PATCH v2 0/8] dma-buf: heaps: Add secure heap"
In reply to: Hans Verkuil: "Re: [PATCH v2 2/5] media: visl: Add a stable_output parameter"
Next in thread: Hans Verkuil: "Re: [PATCH v2 2/5] media: visl: Add a stable_output parameter"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]