| 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
| ||
|
Message-ID: <7bbb62e8-8df8-4167-bb00-a0deedf3eedd@xs4all.nl>
Date: Thu, 22 Aug 2024 08:11:17 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Tomasz Figa <tfiga@...omium.org>,
Benjamin Gaignard <benjamin.gaignard@...labora.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
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 22/08/2024 03:29, Tomasz Figa wrote:
> 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? :)
See my proposal below:
>
> 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)
This should be:
if (!q->min_reqbufs_allocation)
>>> + q->min_reqbufs_allocation = 2;
and vsp1 should set q->min_reqbufs_allocation to 1.
>>> +
>>> + /*
>>> + * 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;
The reasoning behind all this is that VIDIOC_REBUFS is expected to allocate
a sane (i.e. workable and efficient) number of buffers.
So if the DMA engine requires at least X buffers queued, then the minimum
reqbufs allocation is at least q->min_queued_buffers + 1, otherwise you
would never be able to get a buffer back.
That's also why q->min_reqbufs_allocation was set to a minimum of 2: one
buffer in flight, one buffer processed by userspace. That made the queue_setup
call simpler for quite a few drivers that manually set the buffer count to 2.
For most drivers, allocating 2 buffers makes perfect sense.
But if a driver sets q->min_reqbufs_allocation explicitly to 1, then that
should be honored, and my proposed change above will do that.
Laurent, if you agree with this, just post patches for this.
Regards,
Hans
>>> +
>>> + 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