[PATCH] media: hantro: relax s_fmt(out) busy error

[PATCH] media: hantro: relax s_fmt(out) busy error

[Philipp Zabel]

Setting the output format resets the capture queue, so we return -EBUSY
while the capture queue has buffers allocated. If capture dimensions
and pixel format don't change though, we can allow setting the output
format without reallocating the capture queue.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
This applies on top of https://patchwork.linuxtv.org/patch/59337/
("media: hantro: Fix s_fmt for dynamic resolution changes").
---
 drivers/staging/media/hantro/hantro_v4l2.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 586d243cc3cc..05c3edce27a9 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -368,7 +368,7 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
 	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	struct hantro_ctx *ctx = fh_to_ctx(priv);
 	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
-	const struct hantro_fmt *formats;
+	const struct hantro_fmt *raw_vpu_fmt, *formats;
 	unsigned int num_fmts;
 	int ret;
 
@@ -394,8 +394,16 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
 		 */
 		peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
 					  V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
-		if (vb2_is_busy(peer_vq))
-			return -EBUSY;
+		if (vb2_is_busy(peer_vq)) {
+			formats = hantro_get_formats(ctx, &num_fmts);
+			raw_vpu_fmt = hantro_get_default_fmt(formats, num_fmts,
+							     false);
+
+			if (ctx->dst_fmt.width != pix_mp->width ||
+			    ctx->dst_fmt.height != pix_mp->height ||
+			    ctx->dst_fmt.pixelformat != raw_vpu_fmt->fourcc)
+				return -EBUSY;
+		}
 	} else {
 		/*
 		 * The encoder doesn't admit a format change if
-- 
2.20.1

Re: [PATCH] media: hantro: relax s_fmt(out) busy error

[Tomasz Figa]

Hi Philipp,

On Tue, Oct 8, 2019 at 9:38 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Setting the output format resets the capture queue, so we return -EBUSY
> while the capture queue has buffers allocated. If capture dimensions
> and pixel format don't change though, we can allow setting the output
> format without reallocating the capture queue.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> This applies on top of https://patchwork.linuxtv.org/patch/59337/
> ("media: hantro: Fix s_fmt for dynamic resolution changes").
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 586d243cc3cc..05c3edce27a9 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -368,7 +368,7 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>         struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>         struct hantro_ctx *ctx = fh_to_ctx(priv);
>         struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> -       const struct hantro_fmt *formats;
> +       const struct hantro_fmt *raw_vpu_fmt, *formats;
>         unsigned int num_fmts;
>         int ret;
>
> @@ -394,8 +394,16 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>                  */
>                 peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>                                           V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> -               if (vb2_is_busy(peer_vq))
> -                       return -EBUSY;
> +               if (vb2_is_busy(peer_vq)) {
> +                       formats = hantro_get_formats(ctx, &num_fmts);
> +                       raw_vpu_fmt = hantro_get_default_fmt(formats, num_fmts,
> +                                                            false);
> +
> +                       if (ctx->dst_fmt.width != pix_mp->width ||
> +                           ctx->dst_fmt.height != pix_mp->height ||
> +                           ctx->dst_fmt.pixelformat != raw_vpu_fmt->fourcc)

First of all, thanks for the patch! I'd like to ask you to clarify a few things:
1) What's the use case for S_FMT(OUTPUT) without changing neither
resolution nor pixelformat?
2) Is it correct to compare dst_fmt with pix_fmt? My understanding is
that width/height of the OUTPUT queue is the coded size of the stream
(a stream parameter), while width/height of the CAPTURE queue is the
frame buffer size, which can be different from the stream coded size.
Perhaps we should compare with ctx->src_fmt instead?

Best regards,
Tomasz

Re: [PATCH] media: hantro: relax s_fmt(out) busy error

[Philipp Zabel]

Hi Tomasz,

On Tue, 2019-10-08 at 23:05 +0900, Tomasz Figa wrote:
> Hi Philipp,
> 
> On Tue, Oct 8, 2019 at 9:38 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > Setting the output format resets the capture queue, so we return -EBUSY
> > while the capture queue has buffers allocated. If capture dimensions
> > and pixel format don't change though, we can allow setting the output
> > format without reallocating the capture queue.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > This applies on top of https://patchwork.linuxtv.org/patch/59337/
> > ("media: hantro: Fix s_fmt for dynamic resolution changes").
> > ---
> >  drivers/staging/media/hantro/hantro_v4l2.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> > index 586d243cc3cc..05c3edce27a9 100644
> > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > @@ -368,7 +368,7 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> >         struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> >         struct hantro_ctx *ctx = fh_to_ctx(priv);
> >         struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > -       const struct hantro_fmt *formats;
> > +       const struct hantro_fmt *raw_vpu_fmt, *formats;
> >         unsigned int num_fmts;
> >         int ret;
> > 
> > @@ -394,8 +394,16 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> >                  */
> >                 peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> >                                           V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > -               if (vb2_is_busy(peer_vq))
> > -                       return -EBUSY;
> > +               if (vb2_is_busy(peer_vq)) {
> > +                       formats = hantro_get_formats(ctx, &num_fmts);
> > +                       raw_vpu_fmt = hantro_get_default_fmt(formats, num_fmts,
> > +                                                            false);
> > +
> > +                       if (ctx->dst_fmt.width != pix_mp->width ||
> > +                           ctx->dst_fmt.height != pix_mp->height ||
> > +                           ctx->dst_fmt.pixelformat != raw_vpu_fmt->fourcc)
> 
> First of all, thanks for the patch! I'd like to ask you to clarify a few things:
> 1) What's the use case for S_FMT(OUTPUT) without changing neither
> resolution nor pixelformat?

Userspace that currently does not follow the stateless codec spec
correctly, and allocates capture buffers before setting the output
format because libva:

https://github.com/bootlin/libva-v4l2-request/pull/26

It would be better to lazily allocate the capture buffers when the
context is spun up there, it just seemed strange that S_FMT(OUTPUT)
would error even with identical parameters.

> 2) Is it correct to compare dst_fmt with pix_fmt? My understanding is
> that width/height of the OUTPUT queue is the coded size of the stream
> (a stream parameter), while width/height of the CAPTURE queue is the
> frame buffer size, which can be different from the stream coded size.
> Perhaps we should compare with ctx->src_fmt instead?

A call to hantro_reset_raw_fmt() will set dst_fmt width/height to
src_fmt width/height later in this function, so this should make no
difference.

regards
Philipp

Re: [PATCH] media: hantro: relax s_fmt(out) busy error

[Tomasz Figa]

On Tue, Oct 8, 2019 at 11:44 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Tomasz,
>
> On Tue, 2019-10-08 at 23:05 +0900, Tomasz Figa wrote:
> > Hi Philipp,
> >
> > On Tue, Oct 8, 2019 at 9:38 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > >
> > > Setting the output format resets the capture queue, so we return -EBUSY
> > > while the capture queue has buffers allocated. If capture dimensions
> > > and pixel format don't change though, we can allow setting the output
> > > format without reallocating the capture queue.
> > >
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > This applies on top of https://patchwork.linuxtv.org/patch/59337/
> > > ("media: hantro: Fix s_fmt for dynamic resolution changes").
> > > ---
> > >  drivers/staging/media/hantro/hantro_v4l2.c | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> > > index 586d243cc3cc..05c3edce27a9 100644
> > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > @@ -368,7 +368,7 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> > >         struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > >         struct hantro_ctx *ctx = fh_to_ctx(priv);
> > >         struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > > -       const struct hantro_fmt *formats;
> > > +       const struct hantro_fmt *raw_vpu_fmt, *formats;
> > >         unsigned int num_fmts;
> > >         int ret;
> > >
> > > @@ -394,8 +394,16 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> > >                  */
> > >                 peer_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > >                                           V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > > -               if (vb2_is_busy(peer_vq))
> > > -                       return -EBUSY;
> > > +               if (vb2_is_busy(peer_vq)) {
> > > +                       formats = hantro_get_formats(ctx, &num_fmts);
> > > +                       raw_vpu_fmt = hantro_get_default_fmt(formats, num_fmts,
> > > +                                                            false);
> > > +
> > > +                       if (ctx->dst_fmt.width != pix_mp->width ||
> > > +                           ctx->dst_fmt.height != pix_mp->height ||
> > > +                           ctx->dst_fmt.pixelformat != raw_vpu_fmt->fourcc)
> >
> > First of all, thanks for the patch! I'd like to ask you to clarify a few things:
> > 1) What's the use case for S_FMT(OUTPUT) without changing neither
> > resolution nor pixelformat?
>
> Userspace that currently does not follow the stateless codec spec
> correctly, and allocates capture buffers before setting the output
> format because libva:
>
> https://github.com/bootlin/libva-v4l2-request/pull/26
>
> It would be better to lazily allocate the capture buffers when the
> context is spun up there, it just seemed strange that S_FMT(OUTPUT)
> would error even with identical parameters.
>

How does the userspace know the right resolution of buffers to
allocate? Note that in general there is no guarantee that it's equal
to stream coded size, as there may be driver-specific alignments
involved.

Regardless of that, in the stateful spec the resolution of the CAPTURE
queue can change even if the queue has buffers allocated already, i.e.
when there is a dynamic resolution change. Maybe we should be
consistent with that behavior and disallow only pixelformat changes?
That would require careful synchronization in the driver, though, to
reject any already queued incompatible buffers, but could speed up
handling of resolution changes thanks to the ability to have big
enough buffers preallocated.

> > 2) Is it correct to compare dst_fmt with pix_fmt? My understanding is
> > that width/height of the OUTPUT queue is the coded size of the stream
> > (a stream parameter), while width/height of the CAPTURE queue is the
> > frame buffer size, which can be different from the stream coded size.
> > Perhaps we should compare with ctx->src_fmt instead?
>
> A call to hantro_reset_raw_fmt() will set dst_fmt width/height to
> src_fmt width/height later in this function, so this should make no
> difference.

Okay, so basically the failure condition is whether the destination
format would change after this function. I guess it makes sense if we
decide to go with such behavior.

Comparing source and destination formats in the code is at least
confusing, though. It relies on the current driver behavior to use the
same framebuffer size as stream coded size, but they are not
equivalent in general. Perhaps we could have a comment there?

Best regards,
Tomasz