Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le vendredi 10 juin 2022 à 12:01 -0300, Ezequiel Garcia a écrit :
> Hi Nicolas,
> 
> Great stuff! See below for some ideas how to expose errors.
> 
> On Fri, Jun 10, 2022 at 9:52 AM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > 
> > This re-enables H.264 error detection, but using the other error mode.
> > In that mode, the decoder will skip over the error macro-block or
> > slices and complete the decoding. As a side effect, the error status
> > is not set in the interrupt status register, and instead errors are
> > detected per format. Using this mode workaround the issue that the
> > HW get stuck in error stated and allow reporting that some corruption
> > may be present in the buffer returned to userland.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index 55596ce6bb6e..60a89918e2c1 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -1175,14 +1175,15 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> > 
> >         schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(2000));
> > 
> > -       writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > -       writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> > +       writel(0xffffffff, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > +       writel(0xffffffff, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> >         writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
> >         writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
> > 
> >         /* Start decoding! */
> >         writel(RKVDEC_INTERRUPT_DEC_E | RKVDEC_CONFIG_DEC_CLK_GATE_E |
> > -              RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E,
> > +              RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E |
> > +              RKVDEC_H264ORVP9_ERR_MODE,
> >                rkvdec->regs + RKVDEC_REG_INTERRUPT);
> > 
> >         return 0;
> > @@ -1196,10 +1197,26 @@ static int rkvdec_h264_try_ctrl(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
> >         return 0;
> >  }
> > 
> > +static int rkvdec_h264_check_error_info(struct rkvdec_ctx *ctx)
> > +{
> > +       struct rkvdec_dev *rkvdec = ctx->dev;
> > +       int err;
> > +
> > +       err = readl(rkvdec->regs + RKVDEC_REG_H264_ERRINFO_NUM);
> > +       if (err & RKVDEC_STRMD_DECT_ERR_FLAG) {
> > +               pr_debug("Decoded picture have %i/%i slices with errors.\n",
> > +                        RKVDEC_ERR_PKT_NUM(err), RKVDEC_SLICEDEC_NUM(err));
> 
> It's more useful friendly to just keep a counter somewhere. In the past,
> we've created a user control, which has the advantage of leveraging
> an existing mechanism, and already being per-fd.
> 
> See:
> 
> commit b2d3bef1aa7858b2ae5e0d01adb214121ba00b9f
> "media: coda: Add a V4L2 user for control error macroblocks count".
> 
> I would drop the pr_debug, or if you think it's really useful for users
> and developers, go with v4l2_dbg. In which case, how do you ensure
> a corrupted stream won't flood the logs?

There is no use case to make this a control, but yes, a corrupted stream can
flood, but isn't the point of pr_debug() that it won't show if you don't enabled
it ?

As for v4l2_dbg I'm not familiar with that and its not used in this driver for
traces. I've use pr_debug for reference list tracing previously and flooding was
not considered a problem despite being a per-frame trace. You can even out-
compile these if you need to.

Let me know if you have further rationale in the suggestion direction. The
rationale in my coding for such trace is that if I read 1 bit of a register, and
trace the surrounding value, I can validate (as a human) that the rest of the
register isn't complete garbage, and that I'm not basically reading a random
bit. Leaving the trace there, allow other developer on other variant of these
SoC to also notice if that register becomes garbage. This is in contrast of just
telling others "trust me, I tested it".

Nicolas

> 
> Thanks,
> Ezequiel
> 
> 
> > +               return VB2_BUF_STATE_ERROR;
> > +       }
> > +
> > +       return VB2_BUF_STATE_DONE;
> > +}
> > +
> >  const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
> >         .adjust_fmt = rkvdec_h264_adjust_fmt,
> >         .start = rkvdec_h264_start,
> >         .stop = rkvdec_h264_stop,
> >         .run = rkvdec_h264_run,
> >         .try_ctrl = rkvdec_h264_try_ctrl,
> > +       .check_error_info = rkvdec_h264_check_error_info,
> >  };
> > --
> > 2.36.1
> >