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] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 07 Nov 2023 21:39:27 -0500
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Jonas Karlman <jonas@...boo.se>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Alex Bee <knaerzche@...il.com>,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        Christopher Obbard <chris.obbard@...labora.com>,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at
 streaming start

Le mardi 07 novembre 2023 à 23:56 +0100, Jonas Karlman a écrit :
> On 2023-11-07 23:01, Nicolas Dufresne wrote:
> > Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> > > SPS parameters is validated in try_ctrl() ops so there is no need to
> > 
> >                  are
> > 
> > > re-validate when streaming starts.
> > > 
> > > Remove the unnecessary call to validate sps at streaming start.
> > 
> > This patch is not working since user may change the format after the
> > control have been set. The proper fix should actually reset the SPS
> > (well all header controls) to match the the newly set format. Then this
> > validation won't be needed anymore.
> > 
> > The sequence that is problematic after this patch is:
> > 
> > S_FMT (OUTPUT, width, height);
> > S_CTRL (SPS) // valid
> > S_FMT(OUTPUT, width', height'); // SPS is no longer valid
> > 
> > One suggestion I may make is to add a ops so that each codec
> > implementation can reset their header controls to make it valid against
> > the new resolution. With that in place you'll be able drop the check.
> 
> According to the Initialization section of the V4L2 stateless
> documentation a client is supposed to S_CTRL(SPS) after S_FMT(OUTPUT).

Yes, but other part of the spec prevents us from failing if the
userspace restart in the middle of the process.

> 
> https://docs.kernel.org/userspace-api/media/v4l/dev-stateless-decoder.html#initialization
> 
> I guess that all stateless decoders probably should reset all ctrls to
> default value on S_FMT(OUTPUT) or decoders may end up in an unexpected
> state?
> 
> Is resetting a ctrl value back to default something that is supported by
> v4l2 ctrl core? Did not find any obvious way to reset a ctrl value.

In order to avoid having to do this, Hantro driver just ignores these
values from SPS control and do the following:

	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) |
	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->src_fmt.height)) |
	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);

Then all they do is reset the CAPTURE format whenever needed, matching
the bit depth that was previously set. The SPS is unfortunatly not
guarantied to be valid, but at first sight its safe in regard to
picture dimensions.

> 
> Will probably just drop this patch in v5.

That or do like in Hantro driver. What is scary though is that some of
the feature enabled by SPS may requires an auxiliary chunk of memory to
be allocated, and then this method falls appart. I think it would be
nice to fix that properly in all drivers in a future patchset.

> 
> Regards,
> Jonas
> 
> > 
> > Nicolas
> > 
> > p.s. you can also just drop this patch from the series and revisit it
> > later, though I think its worth fixing.
> > 
> > > 
> > > Suggested-by: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
> > > Signed-off-by: Jonas Karlman <jonas@...boo.se>
> > > ---
> > > v4:
> > > - No change
> > > 
> > > v3:
> > > - New patch
> > > 
> > >  drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
> > >  1 file changed, 2 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > index 8bce8902b8dd..815d5359ddd5 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > @@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
> > >  	struct rkvdec_dev *rkvdec = ctx->dev;
> > >  	struct rkvdec_h264_priv_tbl *priv_tbl;
> > >  	struct rkvdec_h264_ctx *h264_ctx;
> > > -	struct v4l2_ctrl *ctrl;
> > > -	int ret;
> > > -
> > > -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
> > > -			      V4L2_CID_STATELESS_H264_SPS);
> > > -	if (!ctrl)
> > > -		return -EINVAL;
> > > -
> > > -	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
> > > -	if (ret)
> > > -		return ret;
> > >  
> > >  	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
> > >  	if (!h264_ctx)
> > > @@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
> > >  	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
> > >  				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
> > >  	if (!priv_tbl) {
> > > -		ret = -ENOMEM;
> > > -		goto err_free_ctx;
> > > +		kfree(h264_ctx);
> > > +		return -ENOMEM;
> > >  	}
> > >  
> > >  	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
> > > @@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
> > >  
> > >  	ctx->priv = h264_ctx;
> > >  	return 0;
> > > -
> > > -err_free_ctx:
> > > -	kfree(h264_ctx);
> > > -	return ret;
> > >  }
> > >  
> > >  static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
> > 
> 

Powered by blists - more mailing lists