linux-kernel - Re: [PATCH for 5.4] media: hantro: Fix s

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 06 Sep 2019 11:26:16 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Ezequiel Garcia <ezequiel@...labora.com>,
        linux-media@...r.kernel.org
Cc:     kernel@...labora.com,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Tomasz Figa <tfiga@...omium.org>,
        linux-rockchip@...ts.infradead.org,
        Heiko Stuebner <heiko@...ech.de>,
        Jonas Karlman <jonas@...boo.se>,
        Boris Brezillon <boris.brezillon@...labora.com>,
        Alexandre Courbot <acourbot@...omium.org>,
        fbuergisser@...omium.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH for 5.4] media: hantro: Fix s_fmt for dynamic resolution
 changes

On Thu, 2019-09-05 at 15:00 -0300, Ezequiel Garcia wrote:
> On Wed, 2019-09-04 at 15:28 +0200, Philipp Zabel wrote:
> > On Wed, 2019-09-04 at 10:01 -0300, Ezequiel Garcia wrote:
> > > On Wed, 2019-09-04 at 12:13 +0200, Philipp Zabel wrote:
> > > > Hi Ezequiel,
> > > > 
> > > > On Tue, 2019-09-03 at 14:12 -0300, Ezequiel Garcia 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@...labora.com>
> > > > > ---
> > > > >  drivers/staging/media/hantro/hantro_v4l2.c | 22 ++++++++++++++++------
> > > > >  1 file changed, 16 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> > > > > index 3dae52abb96c..d48b548842cf 100644
> > > > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > > > @@ -367,19 +367,22 @@ 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;
> > > > > -
> > > > >  	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))
> > > > 
> > > > Before using contents of the v4l2_format f for comparison, we should run
> > > > vidioc_try_fmt_out_mplane over it.
> > > 
> > > Right, good catch.
> > > 
> > > >  Also, besides pixelformat, sizeimage
> > > > shouldn't change either, at least if this is a VB2_MMAP queue.
> > > > 
> > > 
> > > This is the OUTPUT queue, so I don't see why the sizeimage
> > > of the coded buffers should stay the same. Maybe I'm missing
> > > something? 
> > 
> > If the OUTPUT vb2_queue is busy, we already have some buffers of the old
> > size allocated. We can't change their size dynamically with just
> > VIDIOC_S_FMT.
> > 
> > Maybe this should correct sizeimage to the old size instead of returning
> > -EBUSY? Either way, if the old buffer size is too small to reasonably
> > decode the new resolution, the OUTPUT buffers have to be reallocated.
> > 
> 
> Note that for a decoder, the OUTPUT side buffers are coded. Is there any
> straightforward correlation between the buffer size and the resolution?
>
> In other words, how does the driver figure out if the size is
> resonably large?

The decoded frame size seems like a reasonable upper bound at first, but
especially for formats like H.264 it is trivial to construct slices that
are larger than that with I_PCM macroblocks.

I think for H.264 some limit depending on profile and level can be
constructed from spec chapter A.3. "Levels", by looking at the rules for
the sum of NumBytesInNALunit, though not quite straightforward.

regards
Philipp