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: <8a59875f62ac1a60aec91c9170c1471234e1b4db.camel@collabora.com>
Subject: Re: [PATCH v1 20/24] media: hantro: Enable HOLD_CAPTURE_BUF for
 H.264
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, linux-media@vger.kernel.org,
        linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Date:   Thu, 31 Mar 2022 14:25:37 -0400
In-Reply-To: <20220330073600.crbi5wqlp4rimx2a@basti-XPS-13-9310>
References: <20220328195936.82552-1-nicolas.dufresne@collabora.com>
         <20220328195936.82552-21-nicolas.dufresne@collabora.com>
         <20220330073600.crbi5wqlp4rimx2a@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:36 +0200, Sebastian Fricke a =C3=A9crit=
=C2=A0:
> Hey Nicolas,
>=20
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > This is needed to optimizing field decoding. Each field will be
>=20
> s/is needed to optimizing/is needed to optimize/
>=20
> > decoded in the same capture buffer, so to make use of the queues
>=20
> s/in the same/into the same/
>=20
> > we need to be able to ask the driver to keep the capture buffer.
>=20
> How about:
> """
> During field decoding each field will be decoded into the same capture
> buffer. Optimise this mode by asking the driver to hold the buffer until
> all fields are written into it.
> """
>=20
> >=20
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Perhaps avoid giving a reviewed by if you are to comment around modifying t=
he
code ? I will though keep the code as is, I believe there is more good then=
 bad
around the form.

>=20
> > ---
> > drivers/staging/media/hantro/hantro_v4l2.c | 25 ++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >=20
> > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/stagi=
ng/media/hantro/hantro_v4l2.c
> > index 67148ba346f5..50d636678ff3 100644
> > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > @@ -409,6 +409,30 @@ hantro_update_requires_request(struct hantro_ctx *=
ctx, u32 fourcc)
> > 	}
> > }
> >=20
> > +static void
> > +hantro_update_requires_hold_capture_buf(struct hantro_ctx *ctx, u32 fo=
urcc)
> > +{
> > +	struct vb2_queue *vq;
> > +
> > +	vq =3D v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > +			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> > +
> > +	switch (fourcc) {
> > +	case V4L2_PIX_FMT_JPEG:
> > +	case V4L2_PIX_FMT_MPEG2_SLICE:
> > +	case V4L2_PIX_FMT_VP8_FRAME:
> > +	case V4L2_PIX_FMT_HEVC_SLICE:
> > +	case V4L2_PIX_FMT_VP9_FRAME:
> > +		vq->subsystem_flags &=3D ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF=
);
> > +		break;
>=20
> Out of curiosity, why would it be bad for the other codecs to have
> support for that feature activated? As this doesn't actually hold the
> buffers but only makes sure that they could be held.
>=20
> > +	case V4L2_PIX_FMT_H264_SLICE:
> > +		vq->subsystem_flags |=3D VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
>=20
> I think it is worth it to highlight with a comment why only this one
> receives support for holding the buffer. As it is quite confusing
> without background info and just the code.

As this code is quite separate from the actual codec code, I believe it wil=
l be
more robust this way. It could happen in the future that code get modified
without taking into account that a buffer may be held. This also mimic how =
this
was implemented in Cedrus fwiw.

Note that it needs to be added for MPEG2 field decoding too, but I believe =
this
is unrelated to this patchset, the form is nice for adding more in the futu=
re.

>=20
> How about:
> ```
> /*
>   * During field decoding in H264, all fields are written into the
>   * same capture buffer, thus we need to be able to hold the buffer
>   * until all fields are written to it
>   */
> ```
>=20
> > +		break;
> > +	default:
>=20
> The only other decoding formats remaining are:
> - V4L2_PIX_FMT_NV12_4L4
> - V4L2_PIX_FMT_NV12

You'll never get raw formats in that switch. The cases are exhaustive for t=
he
context, yet the compiler does not understand that context.

>=20
> Both have codec mode HANTRO_MODE_NONE.
>=20
> My thought is:
> If we don't care for these two, the we might as well disable buffer holdi=
ng
> support for them as well. So, we could make this simplier
> (but a bit less descriptive):
>=20
> ```
> /*
>   * During field decoding in H264, all fields are written into the
>   * same capture buffer, thus we need to be able to hold the buffer
>   * until all fields are written to it
>   */
> if (fourcc =3D=3D V4L2_PIX_FMT_H264_SLICE)
>      vq->subsystem_flags |=3D VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> else=20
> 		vq->subsystem_flags &=3D ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
> ```
>=20
> Greetings,
> Sebastian
>=20
> > +		break;
> > +	}
> > +}
> > +
> > static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> > 			      struct v4l2_pix_format_mplane *pix_mp)
> > {
> > @@ -472,6 +496,7 @@ static int hantro_set_fmt_out(struct hantro_ctx *ct=
x,
> > 	ctx->dst_fmt.quantization =3D pix_mp->quantization;
> >=20
> > 	hantro_update_requires_request(ctx, pix_mp->pixelformat);
> > +	hantro_update_requires_hold_capture_buf(ctx, pix_mp->pixelformat);
> >=20
> > 	vpu_debug(0, "OUTPUT codec mode: %d\n", ctx->vpu_src_fmt->codec_mode);
> > 	vpu_debug(0, "fmt - w: %d, h: %d\n",
> > --=20
> > 2.34.1
> >=20