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: <b588b53fcfe539692a0a55b9fd0e97528def1d96.camel@collabora.com>
Subject: Re: Re: [PATCH] media: cedrus: hevc: Add check for invalid timestamp
From:   Nicolas Dufresne <nicolas.dufresne@collabora.com>
To:     Jernej =?UTF-8?Q?=C5=A0krabec?= <jernej.skrabec@gmail.com>,
        mripard@kernel.org, paul.kocialkowski@bootlin.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
Date:   Mon, 18 Jul 2022 15:37:31 -0400
In-Reply-To: <4725382.GXAFRqVoOG@kista>
References: <20220718165649.16407-1-jernej.skrabec@gmail.com>
         <d58e8624e9703e5dbbc54908aa142d10ef7f5a22.camel@collabora.com>
         <4725382.GXAFRqVoOG@kista>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

Le lundi 18 juillet 2022 =C3=A0 19:57 +0200, Jernej =C5=A0krabec a =C3=A9cr=
it=C2=A0:
> Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne=20
> napisal(a):
> > Le lundi 18 juillet 2022 =C3=A0 18:56 +0200, Jernej Skrabec a =C3=A9cri=
t :
> > > Not all DPB entries will be used most of the time. Unused entries wil=
l
> > > thus have invalid timestamps. They will produce negative buffer index
> > > which is not specifically handled. This works just by chance in curre=
nt
> > > 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 altogethe=
r
> > > 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(stru=
ct
> > > 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, t=
he
> > addr 0 is being programmed into the HW. With this implementation is is =
left
> > to whatever it was set for the previous decode operation. I think its i=
s
> > nicer done the other way.
>=20
> 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 o=
ld,=20
> but valid entry.
>=20
> 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 thing=
s can=20
> be deduced from vendor code, but not all.
>=20
> I would rather not complicate this fix, especially since it's candidate f=
or=20
> backporting.

I am simply trying to highlight that this is not consistant with how the H2=
64
part is done. Why do we reset the register for one codec and not the other =
?=C2=A0

Perhaps you should sync to your preference the driver as a whole. It also s=
eems
that before your patch, some bits would be 0 and some other would be very l=
arge
values. Between this and leaving random value, I don't really see any gain =
or
reason for a backport. It neither break or fix anything as far as I underst=
and.=C2=A0

My general opinion, is that we fixe the unused address (like to 0) then whe=
n
something goes wrong, as least it will go wrong consistently.

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