linux-kernel - Re: [PATCH v1 20/24] media: hantro: Enable HOLD_CAPTURE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8a59875f62ac1a60aec91c9170c1471234e1b4db.camel@collabora.com>
Date:   Thu, 31 Mar 2022 14:25:37 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Sebastian Fricke <sebastian.fricke@...labora.com>
Cc:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel@...labora.com, linux-media@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 20/24] media: hantro: Enable HOLD_CAPTURE_BUF for
 H.264

Le mercredi 30 mars 2022 à 09:36 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > This is needed to optimizing field decoding. Each field will be
> 
> s/is needed to optimizing/is needed to optimize/
> 
> > decoded in the same capture buffer, so to make use of the queues
> 
> s/in the same/into the same/
> 
> > we need to be able to ask the driver to keep the capture buffer.
> 
> 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.
> """
> 
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@...labora.com>

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

> 
> > ---
> > drivers/staging/media/hantro/hantro_v4l2.c | 25 ++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/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)
> > 	}
> > }
> > 
> > +static void
> > +hantro_update_requires_hold_capture_buf(struct hantro_ctx *ctx, u32 fourcc)
> > +{
> > +	struct vb2_queue *vq;
> > +
> > +	vq = 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 &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
> > +		break;
> 
> 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.
> 
> > +	case V4L2_PIX_FMT_H264_SLICE:
> > +		vq->subsystem_flags |= VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> 
> 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 will 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 future.

> 
> 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
>   */
> ```
> 
> > +		break;
> > +	default:
> 
> 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 the
context, yet the compiler does not understand that context.

> 
> Both have codec mode HANTRO_MODE_NONE.
> 
> My thought is:
> If we don't care for these two, the we might as well disable buffer holding
> support for them as well. So, we could make this simplier
> (but a bit less descriptive):
> 
> ```
> /*
>   * 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 == V4L2_PIX_FMT_H264_SLICE)
>      vq->subsystem_flags |= VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> else 
> 		vq->subsystem_flags &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
> ```
> 
> Greetings,
> Sebastian
> 
> > +		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 *ctx,
> > 	ctx->dst_fmt.quantization = pix_mp->quantization;
> > 
> > 	hantro_update_requires_request(ctx, pix_mp->pixelformat);
> > +	hantro_update_requires_hold_capture_buf(ctx, pix_mp->pixelformat);
> > 
> > 	vpu_debug(0, "OUTPUT codec mode: %d\n", ctx->vpu_src_fmt->codec_mode);
> > 	vpu_debug(0, "fmt - w: %d, h: %d\n",
> > -- 
> > 2.34.1
> >