Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <4740735d92c0dac3708aa922b3d73db7a61fbdda.camel@collabora.com>
Subject: Re: [PATCH v1 22/24] media: hantro: h264: Make dpb entry management
 more robust
From:   Nicolas Dufresne <nicolas.dufresne@collabora.com>
To:     Sebastian Fricke <sebastian.fricke@collabora.com>
Cc:     Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        kernel@collabora.com, Jonas Karlman <jonas@kwiboo.se>,
        linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Date:   Wed, 30 Mar 2022 11:15:58 -0400
In-Reply-To: <20220330075913.wfl3prsyw5fvsv4t@basti-XPS-13-9310>
References: <20220328195936.82552-1-nicolas.dufresne@collabora.com>
         <20220328195936.82552-23-nicolas.dufresne@collabora.com>
         <20220330075913.wfl3prsyw5fvsv4t@basti-XPS-13-9310>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.0 (3.44.0-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

Le mercredi 30 mars 2022 =C3=A0 09:59 +0200, Sebastian Fricke a =C3=A9crit=
=C2=A0:
> Hey Nicolas,
>=20
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > From: Jonas Karlman <jonas@kwiboo.se>
> >=20
> > The driver maintains stable slot location for reference pictures. This
>=20
> s/slot location/slot locations/
>=20
> > change makes the code more robust by using the reference_ts as key and
> > by marking all entries invalid right from the start.
> >=20
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> > 1 file changed, 4 insertions(+), 6 deletions(-)
> >=20
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/stagi=
ng/media/hantro/hantro_h264.c
> > index 228629fb3cdf..7377fc26f780 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> > 			    const struct v4l2_h264_dpb_entry *b)
> > {
> > -	return a->top_field_order_cnt =3D=3D b->top_field_order_cnt &&
> > -	       a->bottom_field_order_cnt =3D=3D b->bottom_field_order_cnt;
> > +	return a->reference_ts =3D=3D b->reference_ts;
> > }
> >=20
> > static void update_dpb(struct hantro_ctx *ctx)
> > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
> >=20
> > 	/* Disable all entries by default. */
> > 	for (i =3D 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
> > -		ctx->h264_dec.dpb[i].flags &=3D ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
> > +		ctx->h264_dec.dpb[i].flags =3D 0;
>=20
> Ehm ... we just remove all flags? Can't this have any unwanted side
> effects like removing a flag that we actually wanted to keep?
> (Like long term or the field flags?)

This is a local copy of the dpb, the DPB is fully passed for every decode. =
So
these flags will be fully restored lower when we copy the found entry. In f=
act,
holding a state here would not represent well the userland intention and ca=
n
have negative side effect on the decoding. Flags are not immutable between
decode and can change. This simplify the code, and make things less error p=
rone.
This part of the code is already a bit complex, no need for an extra layer.

> If we just want to set the DPB entry inactive, then removing the ACTIVE
> flag seems like the correct approach to me.
> If we want to get rid of the VALID flag as well, then we could just do:
> 		ctx->h264_dec.dpb[i].flags &=3D
>        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID=
);
>=20
> In case we really want to reset all flags, I'd say adjust the comment
> above it:
> ```
> - 	/* Disable all entries by default. */
> + 	/* Reset the flags for all entries by default. */
> ```

This reads the same to me, but I can do that yes. understand that VALID mea=
ns
the reference exist and the TS should point to some existing past reference
(unless there was some decode error, which the userland may not be aware ye=
t as
this is asynchronous). While ACTIVE means that it is used as a reference. F=
FMPEG
is known not to filter inactive references. ACTIVE is just a flag without b=
unch
of other flags that can change for every decode. So none of this make sense
between 2 decodes.

>=20
> Greetings,
> Sebastian
>=20
> >=20
> > 	/* Try to match new DPB entries with existing ones by their POCs. */
> > 	for (i =3D 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> > 		const struct v4l2_h264_dpb_entry *ndpb =3D &dec_param->dpb[i];
> >=20
> > -		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > +		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > 			continue;
> >=20
> > 		/*
> > @@ -290,8 +289,7 @@ static void update_dpb(struct hantro_ctx *ctx)
> > 			struct v4l2_h264_dpb_entry *cdpb;
> >=20
> > 			cdpb =3D &ctx->h264_dec.dpb[j];
> > -			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
> > -			    !dpb_entry_match(cdpb, ndpb))
> > +			if (!dpb_entry_match(cdpb, ndpb))
> > 				continue;
> >=20
> > 			*cdpb =3D *ndpb;
> > --=20
> > 2.34.1
> >=20