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;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     mripard@kernel.org, paul.kocialkowski@bootlin.com,
        Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc:     mchehab@kernel.org, gregkh@linuxfoundation.org, wens@csie.org,
        samuel@sholland.org, hverkuil-cisco@xs4all.nl,
        ezequiel@vanguardiasur.com.ar, linux-media@vger.kernel.org,
        linux-staging@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: Re: [PATCH] media: cedrus: hevc: Add check for invalid timestamp
Date:   Mon, 18 Jul 2022 19:57:49 +0200
Message-ID: <4725382.GXAFRqVoOG@kista>
In-Reply-To: <d58e8624e9703e5dbbc54908aa142d10ef7f5a22.camel@collabora.com>
References: <20220718165649.16407-1-jernej.skrabec@gmail.com> <d58e8624e9703e5dbbc54908aa142d10ef7f5a22.camel@collabora.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne=20
napisal(a):
> Le lundi 18 juillet 2022 =E0 18:56 +0200, Jernej Skrabec a =E9crit :
> > Not all DPB entries will be used most of the time. Unused entries will
> > thus have invalid timestamps. They will produce negative buffer index
> > which is not specifically handled. This works just by chance in current
> > code. It will even produce bogus pointer, but since it's not used, it
> > won't do any harm.
> >=20
> > Let's fix that brittle design by skipping writing DPB entry altogether
> > if timestamp is invalid.
> >=20
> > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >=20
> >  drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >=20
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > 1afc6797d806..687f87598f78 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > @@ -147,6 +147,9 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > cedrus_ctx *ctx,>=20
> >  			dpb[i].pic_order_cnt_val
> >  	=09
> >  		};
> >=20
> > +		if (buffer_index < 0)
> > +			continue;
>=20
> When I compare to other codecs, when the buffer_index does not exist, the
> addr 0 is being programmed into the HW. With this implementation is is le=
ft
> to whatever it was set for the previous decode operation. I think its is
> nicer done the other way.

It's done the same way as it's done in vendor lib. As I stated in commit=20
message, actual values don't matter for unused entries. If it is used by=20
accident, HW reaction on all zero pointers can only be worse than using old=
,=20
but valid entry.

Due to no real documentation and Allwinner unwillingness to share details,=
=20
we'll probably never know what's best response for each error. Some things =
can=20
be deduced from vendor code, but not all.

I would rather not complicate this fix, especially since it's candidate for=
=20
backporting.

Best regards,
Jernej

>=20
> > +
> >=20
> >  		cedrus_h265_frame_info_write_single(ctx, i,=20
dpb[i].field_pic,
> >  	=09
> >  						   =20
pic_order_cnt,
> >  						   =20
buffer_index);