Message-ID: <8476356b-9452-40d6-8eba-a2155135cd7c@xs4all.nl>
Date: Fri, 23 Aug 2024 10:08:00 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Tomasz Figa <tfiga@...omium.org>,
 Benjamin Gaignard <benjamin.gaignard@...labora.com>, 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 14:20, Laurent Pinchart wrote:
> Hello,
> 
> On Thu, Aug 22, 2024 at 08:11:17AM +0200, Hans Verkuil wrote:
>> On 22/08/2024 03:29, Tomasz Figa wrote:
>>> On Thu, Aug 22, 2024 at 8:28 AM Laurent Pinchart wrote:
>>>> 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:
>>
>>>>> -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.
> 
> That would work to fix the issue with vsp1, but I don't think it's
> enough. See below.
> 
>>>>> +
>>>>> +     /*
>>>>> +      * 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.
> 
> For devices that can absolutely not work with less than two buffers,
> setting the minimum to 2 is fine. That is however not the majority use
> case, and that's why setting the default in the vb2 core, overriding all
> the drivers that haven't been patched, doesn't sound like a good idea to
> me.
> 
> Generally speaking, 2 is in many cases too low. In the common use case
> of capture and display, you will need a minimum of 3 buffers. Use cases
> are the responsibility of userspace, we shouldn't try to be too smart
> here.
> 
> I actually want to lower the number of buffers and require camera
> drivers to support operating with a single buffer as a general rule.
> Drivers that hold on the last buffer until a new one is provided are
> very painful to use, they're causing issues with libcamera.

I thought about it some more, and I agree with you. So just post a patch
to drop that 'if (q->min_reqbufs_allocation < 2)' bit.

Regards,

	Hans

> 
>> 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.
>>
>>>>> +
>>>>> +     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];
>>>>>
> 

Powered by blists - more mailing lists