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]
Message-ID: <CAAFQd5AQTZa2epfkJnfn_6kpnaNRMzjZ=FjYjf1DM+ZxE0YkZQ@mail.gmail.com>
Date: Thu, 22 Aug 2024 10:29:29 +0900
From: Tomasz Figa <tfiga@...omium.org>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, hverkuil-cisco@...all.nl
Cc: mchehab@...nel.org, m.szyprowski@...sung.com, 
	ezequiel@...guardiasur.com.ar, p.zabel@...gutronix.de, nicolas@...fresne.ca, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-rockchip@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v21 2/9] videobuf2: Add min_reqbufs_allocation field to
 vb2_queue structure

On Thu, Aug 22, 2024 at 8:28 AM Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Benjamin,
>
> On Thu, Mar 14, 2024 at 04:32:19PM +0100, Benjamin Gaignard wrote:
> > Add 'min_reqbufs_allocation' field in the vb2_queue structure so drivers
> > can specify the minimum number of buffers to allocate when calling
> > VIDIOC_REQBUFS.
> > When initializing the queue, v4l2 core makes sure that the following
> > constraints are respected:
> > - the minimum number of buffers to allocate must be at least 2 because
> > one buffer is used by the hardware while the other is being processed
> > by userspace.
>
> This breaks userspace for the Renesas vsp1 driver :-( Unit tests fail,
> as some of them rely on operation with a single buffer.
>
> The vsp1 is a memory-to-memory processing engine, so operating with a
> single buffer is fine in some use cases. I would argue that for live
> capture devices there are valid use cases to operate with a single
> buffer too. Changing this by default will break use cases.
>
> How can we relax this check ? Should I simply submit a patch that lowers
> the minimum to one buffer ?
>

Uhm, I didn't notice this when reading this series. (Actually if I
recall correctly, originally it didn't have this restriction.)

Indeed, I don't see what's wrong with just having 1 buffer. If
performance is not a concern, it's okay to just serialize the
operation on 1 buffer.

If you would be so nice to send it, I think a patch that changes
min_reqbufs_allocation to max(max(1, min_reqbufs_allocation),
min_queued_buffers)) would be good.

That said, Benjamin, Hans, are we missing something by any chance? :)

Best,
Tomasz

> > -if the driver needs 'min_queued_buffers' in the queue before calling
> > start_streaming(), then the minimum requirement is 'min_queued_buffers + 1'
> > to keep at least one buffer available for userspace.
> >
> > Simplify __vb2_init_fileio() by using 'min_reqbufs_allocation' directly
> > to avoid duplicating the minimum number of buffers to allocate computation.
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   | 38 +++++++++++--------
> >  include/media/videobuf2-core.h                | 15 +++++++-
> >  2 files changed, 37 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index d8b3c04cb3b5..58c495b253ce 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -866,7 +866,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> >       /*
> >        * Make sure the requested values and current defaults are sane.
> >        */
> > -     num_buffers = max_t(unsigned int, *count, q->min_queued_buffers);
> > +     num_buffers = max_t(unsigned int, *count, q->min_reqbufs_allocation);
> >       num_buffers = min_t(unsigned int, num_buffers, q->max_num_buffers);
> >       memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
> >       /*
> > @@ -918,7 +918,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> >        * There is no point in continuing if we can't allocate the minimum
> >        * number of buffers needed by this vb2_queue.
> >        */
> > -     if (allocated_buffers < q->min_queued_buffers)
> > +     if (allocated_buffers < q->min_reqbufs_allocation)
> >               ret = -ENOMEM;
> >
> >       /*
> > @@ -2524,6 +2524,25 @@ int vb2_core_queue_init(struct vb2_queue *q)
> >       if (WARN_ON(q->supports_requests && q->min_queued_buffers))
> >               return -EINVAL;
> >
> > +     /*
> > +      * The minimum requirement is 2: one buffer is used
> > +      * by the hardware while the other is being processed by userspace.
> > +      */
> > +     if (q->min_reqbufs_allocation < 2)
> > +             q->min_reqbufs_allocation = 2;
> > +
> > +     /*
> > +      * If the driver needs 'min_queued_buffers' in the queue before
> > +      * calling start_streaming() then the minimum requirement is
> > +      * 'min_queued_buffers + 1' to keep at least one buffer available
> > +      * for userspace.
> > +      */
> > +     if (q->min_reqbufs_allocation < q->min_queued_buffers + 1)
> > +             q->min_reqbufs_allocation = q->min_queued_buffers + 1;
> > +
> > +     if (WARN_ON(q->min_reqbufs_allocation > q->max_num_buffers))
> > +             return -EINVAL;
> > +
> >       INIT_LIST_HEAD(&q->queued_list);
> >       INIT_LIST_HEAD(&q->done_list);
> >       spin_lock_init(&q->done_lock);
> > @@ -2717,7 +2736,6 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> >       struct vb2_fileio_data *fileio;
> >       struct vb2_buffer *vb;
> >       int i, ret;
> > -     unsigned int count = 0;
> >
> >       /*
> >        * Sanity check
> > @@ -2738,18 +2756,8 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> >       if (q->streaming || vb2_get_num_buffers(q) > 0)
> >               return -EBUSY;
> >
> > -     /*
> > -      * Start with q->min_queued_buffers + 1, driver can increase it in
> > -      * queue_setup()
> > -      *
> > -      * 'min_queued_buffers' buffers need to be queued up before you
> > -      * can start streaming, plus 1 for userspace (or in this case,
> > -      * kernelspace) processing.
> > -      */
> > -     count = max(2, q->min_queued_buffers + 1);
> > -
> >       dprintk(q, 3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n",
> > -             (read) ? "read" : "write", count, q->fileio_read_once,
> > +             (read) ? "read" : "write", q->min_reqbufs_allocation, q->fileio_read_once,
> >               q->fileio_write_immediately);
> >
> >       fileio = kzalloc(sizeof(*fileio), GFP_KERNEL);
> > @@ -2763,7 +2771,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> >        * Request buffers and use MMAP type to force driver
> >        * to allocate buffers by itself.
> >        */
> > -     fileio->count = count;
> > +     fileio->count = q->min_reqbufs_allocation;
> >       fileio->memory = VB2_MEMORY_MMAP;
> >       fileio->type = q->type;
> >       q->fileio = fileio;
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 667bf9ee1101..4a8b9135cec8 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -549,9 +549,21 @@ struct vb2_buf_ops {
> >   *           @start_streaming can be called. Used when a DMA engine
> >   *           cannot be started unless at least this number of buffers
> >   *           have been queued into the driver.
> > - *           VIDIOC_REQBUFS will ensure at least @min_queued_buffers
> > + *           VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1
> >   *           buffers will be allocated. Note that VIDIOC_CREATE_BUFS will not
> >   *           modify the requested buffer count.
> > + * @min_reqbufs_allocation: the minimum number of buffers to be allocated when
> > + *           calling VIDIOC_REQBUFS. Note that VIDIOC_CREATE_BUFS will *not*
> > + *           modify the requested buffer count and does not use this field.
> > + *           Drivers can set this if there has to be a certain number of
> > + *           buffers available for the hardware to work effectively.
> > + *           This allows calling VIDIOC_REQBUFS with a buffer count of 1 and
> > + *           it will be automatically adjusted to a workable buffer count.
> > + *           If set, then @min_reqbufs_allocation must be larger than
> > + *           @min_queued_buffers + 1.
> > + *           If this field is > 3, then it is highly recommended that the
> > + *           driver implements the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE/OUTPUT
> > + *           control.
> >   * @alloc_devs:      &struct device memory type/allocator-specific per-plane device
> >   */
> >  /*
> > @@ -622,6 +634,7 @@ struct vb2_queue {
> >       u32                             timestamp_flags;
> >       gfp_t                           gfp_flags;
> >       u32                             min_queued_buffers;
> > +     u32                             min_reqbufs_allocation;
> >
> >       struct device                   *alloc_devs[VB2_MAX_PLANES];
> >
>
> --
> Regards,
>
> Laurent Pinchart

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ