Re: [PATCH] media: cedrus: hevc: Add check for invalid timestamp

[Jernej Škrabec]

Dne ponedeljek, 18. julij 2022 ob 21:37:31 CEST je Nicolas Dufresne 
napisal(a):
> Le lundi 18 juillet 2022 à 19:57 +0200, Jernej Škrabec a écrit :
> > Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne
> > 
> > napisal(a):
> > > Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit :
> > > > 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.
> > > > 
> > > > Let's fix that brittle design by skipping writing DPB entry altogether
> > > > if timestamp is invalid.
> > > > 
> > > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support")
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> > > > ---
> > > > 
> > > >  drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > 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,>
> > > > 
> > > >  			dpb[i].pic_order_cnt_val
> > > >  		
> > > >  		};
> > > > 
> > > > +		if (buffer_index < 0)
> > > > +			continue;
> > > 
> > > 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
> > > left
> > > 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
> > message, actual values don't matter for unused entries. If it is used by
> > accident, HW reaction on all zero pointers can only be worse than using
> > old, but valid entry.
> > 
> > Due to no real documentation and Allwinner unwillingness to share details,
> > we'll probably never know what's best response for each error. Some things
> > can be deduced from vendor code, but not all.
> > 
> > I would rather not complicate this fix, especially since it's candidate
> > for
> > backporting.
> 
> I am simply trying to highlight that this is not consistant with how the
> H264 part is done. Why do we reset the register for one codec and not the
> other ?

While H264 and HEVC are similar in many ways, Cedrus uses two different cores 
or in Cedrus slang, engines, for them. They have their own quirks. One of the 
most apparent is handling of DPB array. H264 requires that same entry is 
always at the same position in HW DPB. That's not required by HEVC.

Additional reasons for differences come from the fact that it's from two 
different authors (Maxime and Paul). Those differences were created at the 
beginning and it is what it is.

> 
> Perhaps you should sync to your preference the driver as a whole. It also
> seems that before your patch, some bits would be 0 and some other would be
> very large 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 understand.

Maybe there is no need to backport, but the change is nevertheless useful. As 
I explained, current code works only by chance, as we noticed with Ezequiel's 
rework. It's certainly worthwhile to make code less brittle. As far as I'm 
concerned, fixes tag can be dropped or even Ezequiel can squash this change 
into his commit, with appropriate adjustments, of course.

I'm not completely sure what do you mean by syncing driver preference. Code 
changes always need a good reason to be accepted. Moving code around and 
renaming things just to be similar with something else is not.

Best regards,
Jernej

> 
> My general opinion, is that we fixe the unused address (like to 0) then when
> something goes wrong, as least it will go wrong consistently.
> 
> > Best regards,
> > Jernej
> > 
> > > > +
> > > > 
> > > >  		cedrus_h265_frame_info_write_single(ctx, i,
> > 
> > dpb[i].field_pic,
> > 
> > 
> > pic_order_cnt,
> > 
> > buffer_index);