linux-kernel - Re: [PATCH 8/9] media: cedrus: Add start and stop decoder operations

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:   Mon, 25 Jun 2018 15:32:33 +0200
From:   Maxime Ripard <maxime.ripard@...tlin.com>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     hans.verkuil@...co.com, acourbot@...omium.org,
        sakari.ailus@...ux.intel.com,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        tfiga@...omium.org, posciak@...omium.org,
        Chen-Yu Tsai <wens@...e.org>, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
        nicolas.dufresne@...labora.com, jenskuske@...il.com,
        linux-sunxi@...glegroups.com,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 8/9] media: cedrus: Add start and stop decoder operations

On Thu, Jun 21, 2018 at 05:38:23PM +0200, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed, 2018-06-13 at 16:07 +0200, Maxime Ripard wrote:
> > Some codec needs to perform some additional task when a decoding is started
> > and stopped, and not only at every frame.
> > 
> > For example, the H264 decoding support needs to allocate buffers that will
> > be used in the decoding process, but do not need to change over time, or at
> > each frame.
> > 
> > In order to allow that for codecs, introduce a start and stop hook that
> > will be called if present at start_streaming and stop_streaming time.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@...tlin.com>
> > ---
> >  .../platform/sunxi/cedrus/sunxi_cedrus_common.h    |  2 ++
> >  .../platform/sunxi/cedrus/sunxi_cedrus_video.c     | 14 +++++++++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h
> > index a2a507eb9fc9..20c78ec1f037 100644
> > --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h
> > +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_common.h
> > @@ -120,6 +120,8 @@ struct sunxi_cedrus_dec_ops {
> >  	enum sunxi_cedrus_irq_status (*irq_status)(struct sunxi_cedrus_ctx *ctx);
> >  	void (*setup)(struct sunxi_cedrus_ctx *ctx,
> >  		      struct sunxi_cedrus_run *run);
> > +	int (*start)(struct sunxi_cedrus_ctx *ctx);
> > +	void (*stop)(struct sunxi_cedrus_ctx *ctx);
> >  	void (*trigger)(struct sunxi_cedrus_ctx *ctx);
> >  };
> >  
> > diff --git a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c
> > index fb7b081a5bb7..d93461178857 100644
> > --- a/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c
> > +++ b/drivers/media/platform/sunxi/cedrus/sunxi_cedrus_video.c
> > @@ -416,6 +416,8 @@ static int sunxi_cedrus_buf_prepare(struct vb2_buffer *vb)
> >  static int sunxi_cedrus_start_streaming(struct vb2_queue *q, unsigned int count)
> >  {
> >  	struct sunxi_cedrus_ctx *ctx = vb2_get_drv_priv(q);
> > +	struct sunxi_cedrus_dev *dev = ctx->dev;
> > +	int ret = 0;
> >  
> >  	switch (ctx->vpu_src_fmt->fourcc) {
> >  	case V4L2_PIX_FMT_MPEG2_FRAME:
> > @@ -425,16 +427,26 @@ static int sunxi_cedrus_start_streaming(struct vb2_queue *q, unsigned int count)
> >  		return -EINVAL;
> >  	}
> >  
> > -	return 0;
> > +	if (V4L2_TYPE_IS_OUTPUT(q->type) &&
> 
> I suppose this check was put in place to ensure that ->start is only
> called once, but what if start_streaming is called multiple times on
> output? Am I totally unsure about whether the API guarantees that we
> only get one start_streaming call per buffer queue, regardless of how
> many userspace issues.
> 
> If we don't have such a guarantee, we probably need an internal
> mechanism to avoid having ->start called more than once.

As far as I understand it, start_streaming can only be called once:
https://elixir.bootlin.com/linux/latest/source/include/media/videobuf2-core.h#L357

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)