[PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated

[Benjamin Gaignard]

Le 21/03/2023 à 19:16, Laurent Pinchart a écrit :
> Hi Benjamin,
>
> Thank you for the patch.
>
> On Tue, Mar 21, 2023 at 11:28:49AM +0100, Benjamin Gaignard wrote:
>> Instead of a static array change bufs to a dynamically allocated array.
>> This will allow to store more video buffer if needed.
>> Protect the array with a spinlock.
> I think I asked in the review of v1 why we couldn't use the kernel
> IDA/IDR APIs to allocate buffer IDs and register buffers, but I don't
> think I've received a reply. Wouldn't it be better than rolling out our
> own mechanism ?

I wrote it the cover letter, it is because IDA/IDR APIs are marked as deprecated
in kernel documentation.
https://docs.kernel.org/core-api/idr.html?highlight=idr
and Xarray looks to be for large array.

Regards,
Benjamin

>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard at collabora.com>
>> ---
>>   .../media/common/videobuf2/videobuf2-core.c   |  8 +++
>>   include/media/videobuf2-core.h                | 49 ++++++++++++++++---
>>   2 files changed, 49 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 79e90e338846..ae9d72f4d181 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -2452,6 +2452,13 @@ int vb2_core_queue_init(struct vb2_queue *q)
>>   	mutex_init(&q->mmap_lock);
>>   	init_waitqueue_head(&q->done_wq);
>>   
>> +	q->max_num_bufs = 32;
>> +	q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
>> +	if (!q->bufs)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&q->bufs_lock);
>> +
>>   	q->memory = VB2_MEMORY_UNKNOWN;
>>   
>>   	if (q->buf_struct_size == 0)
>> @@ -2479,6 +2486,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
>>   	mutex_lock(&q->mmap_lock);
>>   	__vb2_queue_free(q, q->num_buffers);
>>   	mutex_unlock(&q->mmap_lock);
>> +	kfree(q->bufs);
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>>   
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 5b1e3d801546..397dbf6e61e1 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -558,6 +558,8 @@ struct vb2_buf_ops {
>>    * @dma_dir:	DMA mapping direction.
>>    * @bufs:	videobuf2 buffer structures
>>    * @num_buffers: number of allocated/used buffers
>> + * @bufs_lock: lock to protect bufs access
>> + * @max_num_bufs: max number of buffers storable in bufs
>>    * @queued_list: list of buffers currently queued from userspace
>>    * @queued_count: number of buffers queued and ready for streaming.
>>    * @owned_by_drv_count: number of buffers owned by the driver
>> @@ -619,8 +621,10 @@ struct vb2_queue {
>>   	struct mutex			mmap_lock;
>>   	unsigned int			memory;
>>   	enum dma_data_direction		dma_dir;
>> -	struct vb2_buffer		*bufs[VB2_MAX_FRAME];
>> +	struct vb2_buffer		**bufs;
>>   	unsigned int			num_buffers;
>> +	spinlock_t			bufs_lock;
>> +	size_t				max_num_bufs;
>>   
>>   	struct list_head		queued_list;
>>   	unsigned int			queued_count;
>> @@ -1239,9 +1243,16 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
>>   static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>>   						unsigned int index)
>>   {
>> -	if (index < q->num_buffers)
>> -		return q->bufs[index];
>> -	return NULL;
>> +	struct vb2_buffer *vb = NULL;
>> +
>> +	spin_lock(&q->bufs_lock);
>> +
>> +	if (index < q->max_num_bufs)
>> +		vb = q->bufs[index];
>> +
>> +	spin_unlock(&q->bufs_lock);
>> +
>> +	return vb;
>>   }
>>   
>>   /**
>> @@ -1251,12 +1262,30 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>>    */
>>   static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>>   {
>> -	if (vb->index < VB2_MAX_FRAME) {
>> +	bool ret = false;
>> +
>> +	spin_lock(&q->bufs_lock);
>> +
>> +	if (vb->index >= q->max_num_bufs) {
>> +		struct vb2_buffer **tmp;
>> +
>> +		tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>> +		if (!tmp)
>> +			goto realloc_failed;
>> +
>> +		q->max_num_bufs *= 2;
>> +		q->bufs = tmp;
>> +	}
>> +
>> +	if (vb->index < q->max_num_bufs) {
>>   		q->bufs[vb->index] = vb;
>> -		return true;
>> +		ret = true;
>>   	}
>>   
>> -	return false;
>> +realloc_failed:
>> +	spin_unlock(&q->bufs_lock);
>> +
>> +	return ret;
>>   }
>>   
>>   /**
>> @@ -1266,8 +1295,12 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
>>    */
>>   static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>>   {
>> -	if (vb->index < VB2_MAX_FRAME)
>> +	spin_lock(&q->bufs_lock);
>> +
>> +	if (vb->index < q->max_num_bufs)
>>   		q->bufs[vb->index] = NULL;
>> +
>> +	spin_unlock(&q->bufs_lock);
>>   }
>>   
>>   /*