Re: [PATCH v14 52/56] media: core: Add bitmap manage bufs array entries

[Tomasz Figa]

On Tue, Oct 31, 2023 at 05:31:00PM +0100, 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@xxxxxxxxxxxxx>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 42 +++++++++++++++----
>  include/media/videobuf2-core.h                | 15 ++++---
>  2 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 2c8cf479a962..6e88406fcae9 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -416,11 +416,12 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
>   */
>  static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
>  {
> -	WARN_ON(index >= q->max_num_buffers || q->bufs[index]);
> +	WARN_ON(index >= q->max_num_buffers || test_bit(index, q->bufs_bitmap));
>  
>  	q->bufs[index] = vb;
>  	vb->index = index;
>  	vb->vb2_queue = q;
> +	set_bit(index, q->bufs_bitmap);
>  }
>  
>  /**
> @@ -429,6 +430,7 @@ static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
>   */
>  static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>  {
> +	clear_bit(vb->index, vb->vb2_queue->bufs_bitmap);
>  	vb->vb2_queue->bufs[vb->index] = NULL;
>  	vb->vb2_queue = NULL;
>  }
> @@ -450,11 +452,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>  	unsigned long index;
>  	int ret;
>  
> -	/* Ensure that the number of already queue + num_buffers is below q->max_num_buffers */
> +	/* Ensure that vb2_get_num_buffers(q) + num_buffers is no more than q->max_num_buffers */
>  	num_buffers = min_t(unsigned int, num_buffers,
>  			    q->max_num_buffers - vb2_get_num_buffers(q));
>  
> -	index = vb2_get_num_buffers(q);
> +	index = bitmap_find_next_zero_area(q->bufs_bitmap, q->max_num_buffers,
> +					   0, num_buffers, 0);
>  
>  	*first_index = index;
>  
> @@ -656,7 +659,6 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>  		kfree(vb);
>  	}
>  
> -	q->num_buffers -= buffers;
>  	if (!vb2_get_num_buffers(q)) {
>  		q->memory = VB2_MEMORY_UNKNOWN;
>  		INIT_LIST_HEAD(&q->queued_list);
> @@ -874,6 +876,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  		q->bufs = kcalloc(q->max_num_buffers, sizeof(*q->bufs), GFP_KERNEL);
>  	if (!q->bufs)
>  		ret = -ENOMEM;
> +
> +	if (!q->bufs_bitmap)
> +		q->bufs_bitmap = bitmap_zalloc(q->max_num_buffers, GFP_KERNEL);
> +	if (!q->bufs_bitmap) {
> +		ret = -ENOMEM;
> +		kfree(q->bufs);
> +		q->bufs = NULL;
> +	}
>  	q->memory = memory;
>  	mutex_unlock(&q->mmap_lock);
>  	if (ret)
> @@ -943,7 +953,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) {
>  		/*
> @@ -970,6 +979,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_bitmap);
> +	q->bufs_bitmap = NULL;
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
> @@ -1006,9 +1019,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_num_buffers, sizeof(*q->bufs), GFP_KERNEL);
> -		if (!q->bufs)
> +		if (!q->bufs) {
>  			ret = -ENOMEM;
> +			goto unlock;
> +		}
> +		if (!q->bufs_bitmap)
> +			q->bufs_bitmap = bitmap_zalloc(q->max_num_buffers, GFP_KERNEL);

Same as with the kcalloc(). Why not just allocate this in the core code,
e.g. vb2_core_queue_init()?

Actually, is it because we want to avoid allocating
resources early, before the need to actually use the vb2 queue?
If so, could this go to some other core function that runs later, e.g. __vb2_queue_alloc()?

> +		if (!q->bufs_bitmap) {
> +			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;
> @@ -1070,7 +1093,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) {
>  		/*
> @@ -2549,7 +2571,9 @@ void vb2_core_queue_release(struct vb2_queue *q)
>  	__vb2_queue_free(q, q->max_num_buffers);
>  	kfree(q->bufs);
>  	q->bufs = NULL;
> -	q->num_buffers = 0;
> +	bitmap_free(q->bufs_bitmap);
> +	q->bufs_bitmap = NULL;
> +
>  	mutex_unlock(&q->mmap_lock);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_queue_release);
> @@ -2904,7 +2928,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_bitmap)) {

I don't like this low level manipulation of queue internals here (after all
the work other patches did to use helpers). Why not just keep
vb2_get_num_buffers() here?

>  		struct vb2_buffer *b;
>  
>  		/*
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 6986ff4b77cd..288477075a0e 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -346,7 +346,7 @@ struct vb2_buffer {
>   *			describes the requested number of planes and sizes\[\]
>   *			contains the requested plane sizes. In this case
>   *			\*num_buffers are being allocated additionally to
> - *			q->num_buffers. If either \*num_planes or the requested
> + *			queue buffers. If either \*num_planes or the requested

Perhaps "the buffers already in the queue"?

>   *			sizes are invalid callback must return %-EINVAL.
>   * @wait_prepare:	release any locks taken while calling vb2 functions;
>   *			it is called before an ioctl needs to wait for a new
> @@ -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_bitmap: bitmap to manage bufs entries.

Perhaps "bitmap tracking whether each bufs[] entry is used"?

>   * @max_num_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_bitmap;
>  	unsigned int			max_num_buffers;
>  
>  	struct list_head		queued_list;
> @@ -1150,7 +1150,10 @@ static inline bool vb2_fileio_is_active(struct vb2_queue *q)
>   */
>  static inline unsigned int vb2_get_num_buffers(struct vb2_queue *q)
>  {
> -	return q->num_buffers;
> +	if (!q->bufs_bitmap)
> +		return 0;
> +
> +	return bitmap_weight(q->bufs_bitmap, q->max_num_buffers);

Hmm, could we just cache the number of buffers we have, so that we don't
have to go over the entire bitmap every time? (Basically just keep the
code that we had for handling q->num_buffers before this patch.)

>  }
>  
>  /**
> @@ -1253,13 +1256,13 @@ 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 (!q->bufs)
> +	if (!q->bufs_bitmap)
>  		return NULL;
>  
>  	if (index >= q->max_num_buffers)
>  		return NULL;
>  
> -	if (index < q->num_buffers)
> +	if (test_bit(index, q->bufs_bitmap))

Aha, I see why we need the extra condition above now. Perhaps it should've
been added in this patch instead?

>  		return q->bufs[index];
>  	return NULL;
>  }
> -- 
> 2.39.2
> 

Best regards,
Tomasz