| Message ID | 20191008123850.641-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hantro: relax s_fmt(out) busy error | expand |
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
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
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
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
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(-)