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@xxxxxxxxxxxxx>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@xxxxxxxxxxxxx>
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
> >
