[EXT] [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage

[Benjamin Gaignard]

Le 22/03/2023 à 07:22, Ming Qian a écrit :
>
> Hi Benjamin,
>
>> Add module parameter "max_vb_buffer_per_queue" to be able to limit the
>> number of vb2 buffers store in queue.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard at collabora.com>
>> ---
>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------
>> include/media/videobuf2-core.h                  | 11 +++++++++--
>> 2 files changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index ae9d72f4d181..f4da917ccf3f 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -34,6 +34,8 @@
>> static int debug;
>> module_param(debug, int, 0644);
>>
>> +module_param(max_vb_buffer_per_queue, ulong, 0644);
>> +
>> #define dprintk(q, level, fmt, arg...)                                 \
>>         do {                                                            \
>>                 if (debug >= level)                                     \
>> @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q,
>> enum vb2_memory memory,
>>         struct vb2_buffer *vb;
>>         int ret;
>>
>> -       /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME
>> */
>> -       num_buffers = min_t(unsigned int, num_buffers,
>> -                           VB2_MAX_FRAME - q->num_buffers);
>> -
>>         for (buffer = 0; buffer < num_buffers; ++buffer) {
>>                 /* Allocate vb2 buffer structures */
>>                 vb = kzalloc(q->buf_struct_size, GFP_KERNEL); @@ -801,9 +799,7
>> @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>         /*
>>          * Make sure the requested values and current defaults are sane.
>>          */
>> -       WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
>>         num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
>> -       num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
>>         memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
>>         /*
>>          * Set this now to ensure that drivers see the correct q->memory value
>> @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q,
>> enum vb2_memory memory,
>>         bool no_previous_buffers = !q->num_buffers;
>>         int ret;
>>
>> -       if (q->num_buffers == VB2_MAX_FRAME) {
>> -               dprintk(q, 1, "maximum number of buffers already allocated\n");
>> -               return -ENOBUFS;
>> -       }
>> -
>>         if (no_previous_buffers) {
>>                 if (q->waiting_in_dqbuf && *count) {
>>                         dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n");
>> @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum
>> vb2_memory memory,
>>                         return -EINVAL;
>>         }
>>
>> -       num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
>> +       num_buffers = *count;
>>
>>         if (requested_planes && requested_sizes) {
>>                 num_planes = requested_planes; diff --git
>> a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index
>> 397dbf6e61e1..b8b34a993e04 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -12,6 +12,7 @@
>> #ifndef _MEDIA_VIDEOBUF2_CORE_H
>> #define _MEDIA_VIDEOBUF2_CORE_H
>>
>> +#include <linux/minmax.h>
>> #include <linux/mm_types.h>
>> #include <linux/mutex.h>
>> #include <linux/poll.h>
>> @@ -48,6 +49,8 @@ struct vb2_fileio_data;  struct vb2_threadio_data;  struct
>> vb2_buffer;
>>
>> +static size_t max_vb_buffer_per_queue = 1024;
>> +
>> /**
>>   * struct vb2_mem_ops - memory handling/memory allocator operations.
>>   * @alloc:     allocate video memory and, optionally, allocator private data,
>> @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct
>> vb2_queue *q, struct vb2_buffer *
>>
>>         if (vb->index >= q->max_num_bufs) {
>>                 struct vb2_buffer **tmp;
>> +               int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs *
>> + 2);
>> +
>> +               if (cnt >= q->max_num_bufs)
>> +                       goto realloc_failed;
>>
> Is it likely that goto realloc_failed directly?
> The cnt is likely equal to q->max_num_bufs * 2, and it will greater than q->max_num_bufs.
> For example, the default value of q->max_num_bufs is 32, when vb->index comes to 32, cnt is 64,  it's greater than 32, then goto recalloc_failed?

I had in mind to check that we don't realloc more than
max_vb_buffer_per_queue buffer so I will remove this test and check
if vb->index < max_vb_buffer_per_queue instead.

Thanks for your review.

Benjamin

>
>> -               tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q-
>>> bufs), GFP_KERNEL);
>> +               tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs),
>> + GFP_KERNEL);
>>                 if (!tmp)
>>                         goto realloc_failed;
>>
>> -               q->max_num_bufs *= 2;
>> +               q->max_num_bufs = cnt;
>>                 q->bufs = tmp;
>>         }
>>
>> --
>> 2.34.1
>