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: <d58e8624e9703e5dbbc54908aa142d10ef7f5a22.camel@collabora.com>
Subject: Re: [PATCH] media: cedrus: hevc: Add check for invalid timestamp
From:   Nicolas Dufresne <nicolas.dufresne@collabora.com>
To:     Jernej Skrabec <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 13:41:48 -0400
In-Reply-To: <20220718165649.16407-1-jernej.skrabec@gmail.com>
References: <20220718165649.16407-1-jernej.skrabec@gmail.com>
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 18:56 +0200, Jernej Skrabec a =C3=A9crit=C2=
=A0:
> 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>
> ---
>  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/s=
taging/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 c=
edrus_ctx *ctx,
>  			dpb[i].pic_order_cnt_val
>  		};
> =20
> +		if (buffer_index < 0)
> +			continue;

When I compare to other codecs, when the buffer_index does not exist, the a=
ddr 0
is being programmed into the HW. With this implementation is is left to wha=
tever
it was set for the previous decode operation. I think its is nicer done the
other way.

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