Message-ID: <YtXVeAChyKlfqD8O@eze-laptop>
Date:   Mon, 18 Jul 2022 18:49:44 -0300
From:   Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
To:     Jernej Škrabec <jernej.skrabec@...il.com>
Cc:     mripard@...nel.org, paul.kocialkowski@...tlin.com,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        mchehab@...nel.org, gregkh@...uxfoundation.org, wens@...e.org,
        samuel@...lland.org, hverkuil-cisco@...all.nl,
        linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: cedrus: hevc: Add check for invalid timestamp

Hi Jernej,

On Mon, Jul 18, 2022 at 10:34:37PM +0200, Jernej Škrabec wrote:
> 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@...il.com>
> > > > > ---
> > > > > 
> > > > >  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 think this makes sense, since it allows to fix the bug for the time
being.

Reviewed-by: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>

Thanks!
Ezequiel

> > 
> > 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);
> 
> 
> 
> 

Powered by blists - more mailing lists