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: <f3bf3ba7cc27f4d8eddeec730fe588a7a32c0465.camel@collabora.com>
Subject: Re: [PATCH v2 3/4] media: rkvdec: Re-enable H.264 error detection
From:   Nicolas Dufresne <nicolas.dufresne@collabora.com>
To:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        kernel@collabora.com, linux-media@vger.kernel.org,
        linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Date:   Mon, 09 Jan 2023 15:00:39 -0500
In-Reply-To: <Y6oTEkEwuGISwr+z@eze-laptop>
References: <20221223193807.914935-1-nicolas.dufresne@collabora.com>
         <20221223193807.914935-4-nicolas.dufresne@collabora.com>
         <Y6oTEkEwuGISwr+z@eze-laptop>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.2 (3.46.2-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

Le lundi 26 d=C3=A9cembre 2022 =C3=A0 18:33 -0300, Ezequiel Garcia a =C3=A9=
crit=C2=A0:
> Hi Nicolas,
>=20
> I'm still unsure about this patchset.
> It sounds like a good approach and a nice
> improvement, but I want to make sure I think through it.
>=20
> Meanwhile, a small comment...
>=20
> On Fri, Dec 23, 2022 at 02:38:05PM -0500, Nicolas Dufresne wrote:
> > This re-enable 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 state, and allow reporting that some corruption
> > may be present in the buffer 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 4fc167b42cf0c..dfe3e235f099a 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -1162,14 +1162,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;
> > @@ -1183,10 +1184,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",
>=20
> ... still uses pr_debug. I would change it so it uses v4l2_dbg,
> and can be controlled using the same debug parameter
> as you use in patch 4/4.

Good catch, I missed that one, will fix.

>=20
> > +			 RKVDEC_ERR_PKT_NUM(err), RKVDEC_SLICEDEC_NUM(err));
> > +		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,
> >  };
> > --=20
> > 2.38.1
> >=20