From: Boris Brezillon <boris.brezillon@collabora.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Ezequiel Garcia <ezequiel@collabora.com>,
Tomasz Figa <tfiga@chromium.org>,
linux-media@vger.kernel.org, kernel@collabora.com,
Nicolas Dufresne <nicolas.dufresne@collabora.com>,
linux-rockchip@lists.infradead.org,
Heiko Stuebner <heiko@sntech.de>, Jonas Karlman <jonas@kwiboo.se>,
Philipp Zabel <p.zabel@pengutronix.de>,
Alexandre Courbot <acourbot@chromium.org>,
fbuergisser@chromium.org, linux-kernel@vger.kernel.org,
Douglas Anderson <dianders@chromium.org>
Subject: Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
Date: Sat, 9 Nov 2019 20:13:33 +0100 [thread overview]
Message-ID: <20191109201333.4dc63e0e@collabora.com> (raw)
In-Reply-To: <92cba217-4f14-a397-2ae5-8797cc931703@xs4all.nl>
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 ;-).
next prev parent reply other threads:[~2019-11-09 19:13 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-07 17:45 [PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4 Ezequiel Garcia
2019-10-07 17:45 ` [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes Ezequiel Garcia
2019-11-08 10:19 ` Boris Brezillon
2019-11-08 11:52 ` Tomasz Figa
2019-11-09 12:17 ` Hans Verkuil
2019-11-09 12:25 ` Hans Verkuil
2019-11-09 16:01 ` Ezequiel Garcia
2019-11-09 19:13 ` Boris Brezillon [this message]
2019-10-07 17:45 ` [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288 Ezequiel Garcia
2019-10-08 5:27 ` Tomasz Figa
2019-10-08 6:31 ` Jonas Karlman
2019-10-08 10:42 ` Tomasz Figa
2019-10-08 13:35 ` Tomasz Figa
2019-10-08 13:53 ` Tomasz Figa
2019-10-08 14:12 ` Jonas Karlman
2019-10-08 20:39 ` Jonas Karlman
2019-10-10 7:27 ` Tomasz Figa
2019-10-10 7:23 ` Tomasz Figa
2019-10-13 22:10 ` Nicolas Dufresne
2019-10-15 3:27 ` Tomasz Figa
2019-10-07 17:45 ` [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition Ezequiel Garcia
2019-10-07 18:33 ` Jonas Karlman
2019-10-08 3:29 ` Tomasz Figa
2019-10-08 6:23 ` Jonas Karlman
2019-10-08 10:26 ` Tomasz Figa
2019-10-08 13:57 ` Jonas Karlman
2019-10-10 7:36 ` Tomasz Figa
2019-10-07 17:45 ` [PATCH v2 for 5.4 4/4] media: hantro: Fix picture order count table enable Ezequiel Garcia
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191109201333.4dc63e0e@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=acourbot@chromium.org \
--cc=dianders@chromium.org \
--cc=ezequiel@collabora.com \
--cc=fbuergisser@chromium.org \
--cc=heiko@sntech.de \
--cc=hverkuil@xs4all.nl \
--cc=jonas@kwiboo.se \
--cc=kernel@collabora.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=nicolas.dufresne@collabora.com \
--cc=p.zabel@pengutronix.de \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).