Re: [PATCH v7 45/49] media: core: Add bitmap manage bufs array entries

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 19/09/2023 à 17:00, Hans Verkuil a écrit :
> On 14/09/2023 15:33, Benjamin Gaignard wrote:
>> Add a bitmap field to know which of bufs array entries are
>> used or not.
>> Remove no more used num_buffers field from queue structure.
>> Use bitmap_find_next_zero_area() to find the first possible
>> range when creating new buffers to fill the gaps.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   .../media/common/videobuf2/videobuf2-core.c   | 55 +++++++++++++++----
>>   include/media/videobuf2-core.h                |  9 ++-
>>   2 files changed, 51 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index a4c2fae8705d..c5d4a388331b 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -411,10 +411,11 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
>>    */
>>   static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
>>   {
>> -	if (index < q->max_allowed_buffers && !q->bufs[index]) {
>> +	if (index < q->max_allowed_buffers && !test_bit(index, q->bufs_map)) {
> I think bufs_bitmap would be a better name.

Ok I will change it

>
>>   		q->bufs[index] = vb;
>>   		vb->index = index;
>>   		vb->vb2_queue = q;
>> +		set_bit(index, q->bufs_map);
>>   		return true;
>>   	}
>>   
>> @@ -428,9 +429,10 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
>>    */
>>   static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>>   {
>> -	if (vb->index < q->max_allowed_buffers) {
>> +	if (vb->index < q->max_allowed_buffers && test_bit(vb->index, q->bufs_map)) {
> As mentioned in past reviews, I think these tests can be dropped, it makes no
> sense that these ever fail.

I will drop them.

>
>>   		q->bufs[vb->index] = NULL;
>>   		vb->vb2_queue = NULL;
>> +		clear_bit(vb->index, q->bufs_map);
>>   	}
>>   }
>>   
>> @@ -451,11 +453,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>>   	unsigned long first_index;
>>   	int ret;
>>   
>> -	/* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */
>> +	/* Ensure that the number of already queue + num_buffers is below q->max_allowed_buffers */
> Hmm, how about:
>
> 	/* Ensure that vb2_get_num_buffers(q) + num_buffers is no more than q->max_allowed_buffers */

sure

>
>>   	num_buffers = min_t(unsigned int, num_buffers,
>>   			    q->max_allowed_buffers - vb2_get_num_buffers(q));
>>   
>> -	first_index = vb2_get_num_buffers(q);
>> +	first_index = bitmap_find_next_zero_area(q->bufs_map, q->max_allowed_buffers,
>> +						 0, num_buffers, 0);
>>   
>>   	if (first_index >= q->max_allowed_buffers)
>>   		return 0;
>> @@ -675,7 +678,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>>   
>>   struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q, unsigned int index)
>>   {
>> -	if (index < q->num_buffers)
>> +	if (!q->bufs_map || !q->bufs)
>> +		return NULL;
> I don't think this can ever happen.

I got kernel crash without them.
I will keep them.

>
>> +
>> +	if (index >= q->max_allowed_buffers)
>> +		return NULL;
>> +
>> +	if (test_bit(index, q->bufs_map))
>>   		return q->bufs[index];
>>   	return NULL;
>>   }
>> @@ -683,7 +692,10 @@ EXPORT_SYMBOL_GPL(vb2_get_buffer);
>>   
>>   unsigned int vb2_get_num_buffers(struct vb2_queue *q)
>>   {
>> -	return q->num_buffers;
>> +	if (!q->bufs_map)
>> +		return 0;
> Ditto.
>
>> +
>> +	return bitmap_weight(q->bufs_map, q->max_allowed_buffers);
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_get_num_buffers);
>>   
>> @@ -899,6 +911,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>   		q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
>>   	if (!q->bufs)
>>   		ret = -ENOMEM;
>> +
>> +	if (!q->bufs_map)
>> +		q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL);
>> +	if (!q->bufs_map) {
>> +		ret = -ENOMEM;
>> +		kfree(q->bufs);
>> +		q->bufs = NULL;
>> +	}
>>   	q->memory = memory;
>>   	mutex_unlock(&q->mmap_lock);
>>   	if (ret)
>> @@ -968,7 +988,6 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>   	}
>>   
>>   	mutex_lock(&q->mmap_lock);
>> -	q->num_buffers = allocated_buffers;
>>   
>>   	if (ret < 0) {
>>   		/*
>> @@ -995,6 +1014,10 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>>   	mutex_lock(&q->mmap_lock);
>>   	q->memory = VB2_MEMORY_UNKNOWN;
>>   	mutex_unlock(&q->mmap_lock);
>> +	kfree(q->bufs);
>> +	q->bufs = NULL;
>> +	bitmap_free(q->bufs_map);
>> +	q->bufs_map = NULL;
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>> @@ -1031,9 +1054,19 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>   		q->memory = memory;
>>   		if (!q->bufs)
>>   			q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
>> -		if (!q->bufs)
>> +		if (!q->bufs) {
>> +			ret = -ENOMEM;
>> +			goto unlock;
>> +		}
>> +		if (!q->bufs_map)
>> +			q->bufs_map = bitmap_zalloc(q->max_allowed_buffers, GFP_KERNEL);
>> +		if (!q->bufs_map) {
>>   			ret = -ENOMEM;
>> +			kfree(q->bufs);
>> +			q->bufs = NULL;
>> +		}
>>   		mutex_unlock(&q->mmap_lock);
>> +unlock:
>>   		if (ret)
>>   			return ret;
>>   		q->waiting_for_buffers = !q->is_output;
>> @@ -1095,7 +1128,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>   	}
>>   
>>   	mutex_lock(&q->mmap_lock);
>> -	q->num_buffers += allocated_buffers;
>>   
>>   	if (ret < 0) {
>>   		/*
>> @@ -2588,6 +2620,9 @@ void vb2_core_queue_release(struct vb2_queue *q)
>>   	__vb2_queue_free(q, q->max_allowed_buffers);
>>   	kfree(q->bufs);
>>   	q->bufs = NULL;
>> +	bitmap_free(q->bufs_map);
>> +	q->bufs_map = NULL;
>> +
>>   	mutex_unlock(&q->mmap_lock);
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>> @@ -2944,7 +2979,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>>   	 * Check if we need to dequeue the buffer.
>>   	 */
>>   	index = fileio->cur_index;
>> -	if (index >= q->num_buffers) {
>> +	if (!test_bit(index, q->bufs_map)) {
>>   		struct vb2_buffer *b;
>>   
>>   		/*
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 19c93d8eb7c8..734437236cc4 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -557,7 +557,7 @@ struct vb2_buf_ops {
>>    * @memory:	current memory type used
>>    * @dma_dir:	DMA mapping direction.
>>    * @bufs:	videobuf2 buffer structures
>> - * @num_buffers: number of allocated/used buffers
>> + * @bufs_map:	bitmap to manage bufs entries.
>>    * @max_allowed_buffers: upper limit of number of allocated/used buffers
>>    * @queued_list: list of buffers currently queued from userspace
>>    * @queued_count: number of buffers queued and ready for streaming.
>> @@ -621,7 +621,7 @@ struct vb2_queue {
>>   	unsigned int			memory;
>>   	enum dma_data_direction		dma_dir;
>>   	struct vb2_buffer		**bufs;
>> -	unsigned int			num_buffers;
>> +	unsigned long			*bufs_map;
>>   	unsigned int			max_allowed_buffers;
>>   
>>   	struct list_head		queued_list;
>> @@ -1151,7 +1151,10 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q)
>>    */
>>   static inline bool vb2_is_busy(struct vb2_queue *q)
>>   {
>> -	return (q->num_buffers > 0);
>> +	if (!q->bufs_map)
>> +		return false;
> I don't think this can happen.
>
>> +
>> +	return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0);
> How about:
>
> 	return vb2_get_num_buffers(q) > 0;

vb2_get_num_buffers is defined in videobuf2-core.c, I'm not sure that
an inline function could depend of a module function.

Regards,
Benjamin

>
>>   }
>>   
>>   /**
> Regards,
>
> 	Hans
>