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

[Benjamin Gaignard]

Le 19/05/2023 à 12:04, Tomasz Figa a écrit :
> On Tue, Mar 21, 2023 at 08:16:10PM +0200, Laurent Pinchart wrote:
>> 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 ?
>>
> +1, with a note that [1] suggests that IDR is deprecated and XArray[2]
> should be used instead.
>
> [1] https://docs.kernel.org/core-api/idr.html
> [2] https://docs.kernel.org/core-api/xarray.html
>
> Also from my quick look, XArray may be solving the locking concerns for us
> automatically.

 From documentation Xarray seems to useful for "very large array of pointers"
which is, I think, not our case.

Regards,
Benjamin

>
> Best regards,
> Tomasz
>
>>> 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);
>>>   }
>>>   
>>>   /*
>> -- 
>> Regards,
>>
>> Laurent Pinchart