| Message ID | 20191007174505.10681-2-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hantro: Collected fixes for v5.4 | expand |
On Mon, 7 Oct 2019 14:45:02 -0300 Ezequiel Garcia <ezequiel@collabora.com> wrote: > Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > changed the conditions under S_FMT was allowed for OUTPUT > CAPTURE buffers. > > However, and according to the mem-to-mem stateless decoder specification, > in order to support dynamic resolution changes, S_FMT should be allowed > even if OUTPUT buffers have been allocated. > > Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution > modification, provided the pixel format stays the same. > > Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests. > > Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > -- > v2: > * Call try_fmt_out before using the format, > pointed out by Philipp. > > drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c > index 3dae52abb96c..586d243cc3cc 100644 > --- a/drivers/staging/media/hantro/hantro_v4l2.c > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > @@ -367,19 +367,26 @@ 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; > unsigned int num_fmts; > - struct vb2_queue *vq; > int ret; > > - /* Change not allowed if queue is busy. */ > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); > - if (vb2_is_busy(vq)) > - return -EBUSY; > + ret = vidioc_try_fmt_out_mplane(file, priv, f); > + if (ret) > + return ret; > > if (!hantro_is_encoder_ctx(ctx)) { > struct vb2_queue *peer_vq; > > + /* > + * In other to support dynamic resolution change, ^ order > + * the decoder admits a resolution change, as long > + * as the pixelformat remains. Can't be done if streaming. > + */ > + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) && > + pix_mp->pixelformat != ctx->src_fmt.pixelformat)) > + return -EBUSY; Sorry to chime in only now, but I'm currently looking at the VP9 spec and it seems the resolution is allowed to change dynamically [1] (I guess the same applies to VP8). IIU the spec correctly, coded frames using the new resolution can reference decoded frames using the old one (can be higher or lower res BTW). If we force a streamoff to change the resolution (as seems to be the case here), we'll lose those ref frames (see the hantro_return_bufs() in stop streaming), right? Hans, Tomasz, any idea how this dynamic resolution change could/should be supported? > /* > * Since format change on the OUTPUT queue will reset > * the CAPTURE queue, we can't allow doing so > @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f) > V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > if (vb2_is_busy(peer_vq)) > return -EBUSY; > + } else { > + /* > + * The encoder doesn't admit a format change if > + * there are OUTPUT buffers allocated. > + */ > + if (vb2_is_busy(vq)) > + return -EBUSY; > } > > - ret = vidioc_try_fmt_out_mplane(file, priv, f); > - if (ret) > - return ret; > - > formats = hantro_get_formats(ctx, &num_fmts); > ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts, > pix_mp->pixelformat); [1] Section "5.16 Reference frame scaling" of https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf
On Fri, Nov 8, 2019 at 7:20 PM Boris Brezillon <boris.brezillon@collabora.com> wrote: > > On Mon, 7 Oct 2019 14:45:02 -0300 > Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > > changed the conditions under S_FMT was allowed for OUTPUT > > CAPTURE buffers. > > > > However, and according to the mem-to-mem stateless decoder specification, > > in order to support dynamic resolution changes, S_FMT should be allowed > > even if OUTPUT buffers have been allocated. > > > > Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution > > modification, provided the pixel format stays the same. > > > > Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests. > > > > Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > -- > > v2: > > * Call try_fmt_out before using the format, > > pointed out by Philipp. > > > > drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++------- > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c > > index 3dae52abb96c..586d243cc3cc 100644 > > --- a/drivers/staging/media/hantro/hantro_v4l2.c > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > > @@ -367,19 +367,26 @@ 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; > > unsigned int num_fmts; > > - struct vb2_queue *vq; > > int ret; > > > > - /* Change not allowed if queue is busy. */ > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); > > - if (vb2_is_busy(vq)) > > - return -EBUSY; > > + ret = vidioc_try_fmt_out_mplane(file, priv, f); > > + if (ret) > > + return ret; > > > > if (!hantro_is_encoder_ctx(ctx)) { > > struct vb2_queue *peer_vq; > > > > + /* > > + * In other to support dynamic resolution change, > > ^ order > > > + * the decoder admits a resolution change, as long > > + * as the pixelformat remains. Can't be done if streaming. > > + */ > > + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) && > > + pix_mp->pixelformat != ctx->src_fmt.pixelformat)) > > + return -EBUSY; > > Sorry to chime in only now, but I'm currently looking at the VP9 spec > and it seems the resolution is allowed to change dynamically [1] (I > guess the same applies to VP8). IIU the spec correctly, coded frames > using the new resolution can reference decoded frames using the old > one (can be higher or lower res BTW). If we force a streamoff to change > the resolution (as seems to be the case here), we'll lose those ref > frames (see the hantro_return_bufs() in stop streaming), right? > Hans, Tomasz, any idea how this dynamic resolution change could/should > be supported? The same problem applies to stateful decoders as well. This is an inherent limitation of the current V4L2 API. To handle this kind of streams we would have to make the format a per-buffer parameter, rather than per-queue as it is defined currently. Best regards, Tomasz > > > /* > > * Since format change on the OUTPUT queue will reset > > * the CAPTURE queue, we can't allow doing so > > @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f) > > V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > > if (vb2_is_busy(peer_vq)) > > return -EBUSY; > > + } else { > > + /* > > + * The encoder doesn't admit a format change if > > + * there are OUTPUT buffers allocated. > > + */ > > + if (vb2_is_busy(vq)) > > + return -EBUSY; > > } > > > > - ret = vidioc_try_fmt_out_mplane(file, priv, f); > > - if (ret) > > - return ret; > > - > > formats = hantro_get_formats(ctx, &num_fmts); > > ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts, > > pix_mp->pixelformat); > > [1] Section "5.16 Reference frame scaling" of > https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf
On 11/8/19 12:52 PM, Tomasz Figa wrote: > On Fri, Nov 8, 2019 at 7:20 PM Boris Brezillon > <boris.brezillon@collabora.com> wrote: >> >> On Mon, 7 Oct 2019 14:45:02 -0300 >> Ezequiel Garcia <ezequiel@collabora.com> wrote: >> >>> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") >>> changed the conditions under S_FMT was allowed for OUTPUT >>> CAPTURE buffers. >>> >>> However, and according to the mem-to-mem stateless decoder specification, >>> in order to support dynamic resolution changes, S_FMT should be allowed >>> even if OUTPUT buffers have been allocated. >>> >>> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution >>> modification, provided the pixel format stays the same. >>> >>> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests. >>> >>> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> -- >>> v2: >>> * Call try_fmt_out before using the format, >>> pointed out by Philipp. >>> >>> drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++------- >>> 1 file changed, 19 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c >>> index 3dae52abb96c..586d243cc3cc 100644 >>> --- a/drivers/staging/media/hantro/hantro_v4l2.c >>> +++ b/drivers/staging/media/hantro/hantro_v4l2.c >>> @@ -367,19 +367,26 @@ 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; >>> unsigned int num_fmts; >>> - struct vb2_queue *vq; >>> int ret; >>> >>> - /* Change not allowed if queue is busy. */ >>> - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); >>> - if (vb2_is_busy(vq)) >>> - return -EBUSY; >>> + ret = vidioc_try_fmt_out_mplane(file, priv, f); >>> + if (ret) >>> + return ret; >>> >>> if (!hantro_is_encoder_ctx(ctx)) { >>> struct vb2_queue *peer_vq; >>> >>> + /* >>> + * In other to support dynamic resolution change, >> >> ^ order >> >>> + * the decoder admits a resolution change, as long >>> + * as the pixelformat remains. Can't be done if streaming. >>> + */ >>> + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) && >>> + pix_mp->pixelformat != ctx->src_fmt.pixelformat)) >>> + return -EBUSY; >> >> Sorry to chime in only now, but I'm currently looking at the VP9 spec >> and it seems the resolution is allowed to change dynamically [1] (I >> guess the same applies to VP8). IIU the spec correctly, coded frames >> using the new resolution can reference decoded frames using the old >> one (can be higher or lower res BTW). If we force a streamoff to change >> the resolution (as seems to be the case here), we'll lose those ref >> frames (see the hantro_return_bufs() in stop streaming), right? >> Hans, Tomasz, any idea how this dynamic resolution change could/should >> be supported? > > The same problem applies to stateful decoders as well. This is an > inherent limitation of the current V4L2 API. To handle this kind of > streams we would have to make the format a per-buffer parameter, > rather than per-queue as it is defined currently. This would be interesting to implement in new streaming ioctls. There are more reasons besides codec support why you would want that (i.e. a resolution change on an HDMI input). It's kind of supported since you can allocate larger buffers than needed for the current format, but currently there is no way to see when a new resolution is received. Related to this is the fact that while you can add new buffers on the fly (CREATE_BUFS), you can't delete unused buffers. Also, the max number of buffers (32) is too small for some of the more advanced scenarios. This really needs to be addressed as well in new streaming ioctls. Regards, Hans > > Best regards, > Tomasz > >> >>> /* >>> * Since format change on the OUTPUT queue will reset >>> * the CAPTURE queue, we can't allow doing so >>> @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f) >>> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >>> if (vb2_is_busy(peer_vq)) >>> return -EBUSY; >>> + } else { >>> + /* >>> + * The encoder doesn't admit a format change if >>> + * there are OUTPUT buffers allocated. >>> + */ >>> + if (vb2_is_busy(vq)) >>> + return -EBUSY; >>> } >>> >>> - ret = vidioc_try_fmt_out_mplane(file, priv, f); >>> - if (ret) >>> - return ret; >>> - >>> formats = hantro_get_formats(ctx, &num_fmts); >>> ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts, >>> pix_mp->pixelformat); >> >> [1] Section "5.16 Reference frame scaling" of >> https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf
On 11/8/19 11:19 AM, Boris Brezillon wrote: > On Mon, 7 Oct 2019 14:45:02 -0300 > Ezequiel Garcia <ezequiel@collabora.com> wrote: > >> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") >> changed the conditions under S_FMT was allowed for OUTPUT >> CAPTURE buffers. >> >> However, and according to the mem-to-mem stateless decoder specification, >> in order to support dynamic resolution changes, S_FMT should be allowed >> even if OUTPUT buffers have been allocated. >> >> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution >> modification, provided the pixel format stays the same. >> >> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests. >> >> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >> -- >> v2: >> * Call try_fmt_out before using the format, >> pointed out by Philipp. >> >> drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++------- >> 1 file changed, 19 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c >> index 3dae52abb96c..586d243cc3cc 100644 >> --- a/drivers/staging/media/hantro/hantro_v4l2.c >> +++ b/drivers/staging/media/hantro/hantro_v4l2.c >> @@ -367,19 +367,26 @@ 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; >> unsigned int num_fmts; >> - struct vb2_queue *vq; >> int ret; >> >> - /* Change not allowed if queue is busy. */ >> - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); >> - if (vb2_is_busy(vq)) >> - return -EBUSY; >> + ret = vidioc_try_fmt_out_mplane(file, priv, f); >> + if (ret) >> + return ret; >> >> if (!hantro_is_encoder_ctx(ctx)) { >> struct vb2_queue *peer_vq; >> >> + /* >> + * In other to support dynamic resolution change, > > ^ order > >> + * the decoder admits a resolution change, as long >> + * as the pixelformat remains. Can't be done if streaming. >> + */ >> + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) && >> + pix_mp->pixelformat != ctx->src_fmt.pixelformat)) >> + return -EBUSY; > > Sorry to chime in only now, but I'm currently looking at the VP9 spec > and it seems the resolution is allowed to change dynamically [1] (I > guess the same applies to VP8). IIU the spec correctly, coded frames > using the new resolution can reference decoded frames using the old > one (can be higher or lower res BTW). If we force a streamoff to change > the resolution (as seems to be the case here), we'll lose those ref > frames (see the hantro_return_bufs() in stop streaming), right? > Hans, Tomasz, any idea how this dynamic resolution change could/should > be supported? As Tomasz also mentioned, supporting this is much more work, and probably requires new streaming ioctls. In the meantime I think this patch is fine (with the typo fixed, I can do that), so is it OK if I merge this? Regards, Hans > >> /* >> * Since format change on the OUTPUT queue will reset >> * the CAPTURE queue, we can't allow doing so >> @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f) >> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> if (vb2_is_busy(peer_vq)) >> return -EBUSY; >> + } else { >> + /* >> + * The encoder doesn't admit a format change if >> + * there are OUTPUT buffers allocated. >> + */ >> + if (vb2_is_busy(vq)) >> + return -EBUSY; >> } >> >> - ret = vidioc_try_fmt_out_mplane(file, priv, f); >> - if (ret) >> - return ret; >> - >> formats = hantro_get_formats(ctx, &num_fmts); >> ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts, >> pix_mp->pixelformat); > > [1] Section "5.16 Reference frame scaling" of > https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf >
Hi Hans, On Sat, 2019-11-09 at 13:25 +0100, Hans Verkuil wrote: > On 11/8/19 11:19 AM, Boris Brezillon wrote: > > On Mon, 7 Oct 2019 14:45:02 -0300 > > Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > > > Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > > > changed the conditions under S_FMT was allowed for OUTPUT > > > CAPTURE buffers. > > > > > > However, and according to the mem-to-mem stateless decoder specification, > > > in order to support dynamic resolution changes, S_FMT should be allowed > > > even if OUTPUT buffers have been allocated. > > > > > > Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution > > > modification, provided the pixel format stays the same. > > > > > > Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests. > > > > > > Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > -- > > > v2: > > > * Call try_fmt_out before using the format, > > > pointed out by Philipp. > > > > > > drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++------- > > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c > > > index 3dae52abb96c..586d243cc3cc 100644 > > > --- a/drivers/staging/media/hantro/hantro_v4l2.c > > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > > > @@ -367,19 +367,26 @@ 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; > > > unsigned int num_fmts; > > > - struct vb2_queue *vq; > > > int ret; > > > > > > - /* Change not allowed if queue is busy. */ > > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); > > > - if (vb2_is_busy(vq)) > > > - return -EBUSY; > > > + ret = vidioc_try_fmt_out_mplane(file, priv, f); > > > + if (ret) > > > + return ret; > > > > > > if (!hantro_is_encoder_ctx(ctx)) { > > > struct vb2_queue *peer_vq; > > > > > > + /* > > > + * In other to support dynamic resolution change, > > > > ^ order > > > > > + * the decoder admits a resolution change, as long > > > + * as the pixelformat remains. Can't be done if streaming. > > > + */ > > > + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) && > > > + pix_mp->pixelformat != ctx->src_fmt.pixelformat)) > > > + return -EBUSY; > > > > Sorry to chime in only now, but I'm currently looking at the VP9 spec > > and it seems the resolution is allowed to change dynamically [1] (I > > guess the same applies to VP8). IIU the spec correctly, coded frames > > using the new resolution can reference decoded frames using the old > > one (can be higher or lower res BTW). If we force a streamoff to change > > the resolution (as seems to be the case here), we'll lose those ref > > frames (see the hantro_return_bufs() in stop streaming), right? > > Hans, Tomasz, any idea how this dynamic resolution change could/should > > be supported? > > As Tomasz also mentioned, supporting this is much more work, and probably > requires new streaming ioctls. > > In the meantime I think this patch is fine (with the typo fixed, I can do > that), so is it OK if I merge this? > Yes, please. I originally posted this as a v5.4 fix, so it would be nice if we can mark this as stable material. Thanks, Ezequiel
On Sat, 9 Nov 2019 13:25:18 +0100 Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 11/8/19 11:19 AM, Boris Brezillon wrote: > > On Mon, 7 Oct 2019 14:45:02 -0300 > > Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > >> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > >> changed the conditions under S_FMT was allowed for OUTPUT > >> CAPTURE buffers. > >> > >> However, and according to the mem-to-mem stateless decoder specification, > >> in order to support dynamic resolution changes, S_FMT should be allowed > >> even if OUTPUT buffers have been allocated. > >> > >> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution > >> modification, provided the pixel format stays the same. > >> > >> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests. > >> > >> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") > >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > >> -- > >> v2: > >> * Call try_fmt_out before using the format, > >> pointed out by Philipp. > >> > >> drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++------- > >> 1 file changed, 19 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c > >> index 3dae52abb96c..586d243cc3cc 100644 > >> --- a/drivers/staging/media/hantro/hantro_v4l2.c > >> +++ b/drivers/staging/media/hantro/hantro_v4l2.c > >> @@ -367,19 +367,26 @@ 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; > >> unsigned int num_fmts; > >> - struct vb2_queue *vq; > >> int ret; > >> > >> - /* Change not allowed if queue is busy. */ > >> - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); > >> - if (vb2_is_busy(vq)) > >> - return -EBUSY; > >> + ret = vidioc_try_fmt_out_mplane(file, priv, f); > >> + if (ret) > >> + return ret; > >> > >> if (!hantro_is_encoder_ctx(ctx)) { > >> struct vb2_queue *peer_vq; > >> > >> + /* > >> + * In other to support dynamic resolution change, > > > > ^ order > > > >> + * the decoder admits a resolution change, as long > >> + * as the pixelformat remains. Can't be done if streaming. > >> + */ > >> + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) && > >> + pix_mp->pixelformat != ctx->src_fmt.pixelformat)) > >> + return -EBUSY; > > > > Sorry to chime in only now, but I'm currently looking at the VP9 spec > > and it seems the resolution is allowed to change dynamically [1] (I > > guess the same applies to VP8). IIU the spec correctly, coded frames > > using the new resolution can reference decoded frames using the old > > one (can be higher or lower res BTW). If we force a streamoff to change > > the resolution (as seems to be the case here), we'll lose those ref > > frames (see the hantro_return_bufs() in stop streaming), right? > > Hans, Tomasz, any idea how this dynamic resolution change could/should > > be supported? > > As Tomasz also mentioned, supporting this is much more work, and probably > requires new streaming ioctls. > > In the meantime I think this patch is fine (with the typo fixed, I can do > that), so is it OK if I merge this? Sure, go ahead, here's my Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> in case you haven't applied the patch already. Oh, BTW, it wasn't clear in my previous reply, but I didn't intend to block this patch with my VP9 concerns. My only motivation was to start a discussion on how to solve my specific issue ;-).
diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c index 3dae52abb96c..586d243cc3cc 100644 --- a/drivers/staging/media/hantro/hantro_v4l2.c +++ b/drivers/staging/media/hantro/hantro_v4l2.c @@ -367,19 +367,26 @@ 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; unsigned int num_fmts; - struct vb2_queue *vq; int ret; - /* Change not allowed if queue is busy. */ - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type); - if (vb2_is_busy(vq)) - return -EBUSY; + ret = vidioc_try_fmt_out_mplane(file, priv, f); + if (ret) + return ret; if (!hantro_is_encoder_ctx(ctx)) { struct vb2_queue *peer_vq; + /* + * In other to support dynamic resolution change, + * the decoder admits a resolution change, as long + * as the pixelformat remains. Can't be done if streaming. + */ + if (vb2_is_streaming(vq) || (vb2_is_busy(vq) && + pix_mp->pixelformat != ctx->src_fmt.pixelformat)) + return -EBUSY; /* * Since format change on the OUTPUT queue will reset * the CAPTURE queue, we can't allow doing so @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f) V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); if (vb2_is_busy(peer_vq)) return -EBUSY; + } else { + /* + * The encoder doesn't admit a format change if + * there are OUTPUT buffers allocated. + */ + if (vb2_is_busy(vq)) + return -EBUSY; } - ret = vidioc_try_fmt_out_mplane(file, priv, f); - if (ret) - return ret; - formats = hantro_get_formats(ctx, &num_fmts); ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts, pix_mp->pixelformat);
Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") changed the conditions under S_FMT was allowed for OUTPUT CAPTURE buffers. However, and according to the mem-to-mem stateless decoder specification, in order to support dynamic resolution changes, S_FMT should be allowed even if OUTPUT buffers have been allocated. Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution modification, provided the pixel format stays the same. Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests. Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders") Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> -- v2: * Call try_fmt_out before using the format, pointed out by Philipp. drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++------- 1 file changed, 19 insertions(+), 9 deletions(-)