Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <16e50526255d585bcd5c863755ebb423466d53ff.camel@collabora.com>
Subject: Re: [PATCH v1 4/5] media: rkvdec: Re-enable H.264 error detection
From:   Nicolas Dufresne <nicolas.dufresne@collabora.com>
To:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc:     linux-media <linux-media@vger.kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Collabora Kernel ML <kernel@collabora.com>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        "open list:STAGING SUBSYSTEM" <linux-staging@lists.linux.dev>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Date:   Fri, 10 Jun 2022 12:38:24 -0400
In-Reply-To: <CAAEAJfAdPt59oCi4wPybwn0a4zHq_3x66L5mRSQ54yQezz+ZZA@mail.gmail.com>
References: <20220610125215.240539-1-nicolas.dufresne@collabora.com>
         <20220610125215.240539-5-nicolas.dufresne@collabora.com>
         <CAAEAJfAdPt59oCi4wPybwn0a4zHq_3x66L5mRSQ54yQezz+ZZA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.1 (3.44.1-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

Le vendredi 10 juin 2022 =C3=A0 12:01 -0300, Ezequiel Garcia a =C3=A9crit=
=C2=A0:
> Hi Nicolas,
>=20
> Great stuff! See below for some ideas how to expose errors.
>=20
> On Fri, Jun 10, 2022 at 9:52 AM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> >=20
> > 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.
> >=20
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 23 +++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >=20
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/stagi=
ng/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 *c=
tx)
> >=20
> >         schedule_delayed_work(&rkvdec->watchdog_work, msecs_to_jiffies(=
2000));
> >=20
> > -       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);
> >=20
> >         /* 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);
> >=20
> >         return 0;
> > @@ -1196,10 +1197,26 @@ static int rkvdec_h264_try_ctrl(struct rkvdec_c=
tx *ctx, struct v4l2_ctrl *ctrl)
> >         return 0;
> >  }
> >=20
> > +static int rkvdec_h264_check_error_info(struct rkvdec_ctx *ctx)
> > +{
> > +       struct rkvdec_dev *rkvdec =3D ctx->dev;
> > +       int err;
> > +
> > +       err =3D 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(e=
rr));
>=20
> 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.
>=20
> See:
>=20
> commit b2d3bef1aa7858b2ae5e0d01adb214121ba00b9f
> "media: coda: Add a V4L2 user for control error macroblocks count".
>=20
> 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 ca=
n
flood, but isn't the point of pr_debug() that it won't show if you don't en=
abled
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 floodin=
g 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 t=
he
register isn't complete garbage, and that I'm not basically reading a rando=
m
bit. Leaving the trace there, allow other developer on other variant of the=
se
SoC to also notice if that register becomes garbage. This is in contrast of=
 just
telling others "trust me, I tested it".

Nicolas

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