diff mbox series

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

Message ID 20230914133323.198857-46-benjamin.gaignard@collabora.com
State New
Headers show
Series [v7,01/49] media: videobuf2: Rework offset 'cookie' encoding pattern | expand

Commit Message

Benjamin Gaignard Sept. 14, 2023, 1:33 p.m. UTC
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(-)

Comments

Hans Verkuil Sept. 19, 2023, 3 p.m. UTC | #1
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.

>  		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.

>  		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 */

>  	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.

> +
> +	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;

>  }
>  
>  /**

Regards,

	Hans
Benjamin Gaignard Sept. 20, 2023, 2:30 p.m. UTC | #2
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
>
Hans Verkuil Sept. 20, 2023, 2:56 p.m. UTC | #3
On 20/09/2023 16:30, Benjamin Gaignard wrote:
> 

<snip>

>>>       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.

What is the backtrace? How can this happen? It feels wrong that this can be
called with a vb2_queue that apparently is not properly initialized.

>>> +
>>> +    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.

Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.

Regards,

	Hans
Benjamin Gaignard Sept. 20, 2023, 3:17 p.m. UTC | #4
Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
> On 20/09/2023 16:30, Benjamin Gaignard wrote:
> <snip>
>
>>>>        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.
> What is the backtrace? How can this happen? It feels wrong that this can be
> called with a vb2_queue that apparently is not properly initialized.

I will add backtrace when doing test on v8

>
>
>>>> +
>>>> +    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.
> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.

I will change vb2_get_num_buffers() to inline function that solve the problem too.

>
> Regards,
>
> 	Hans
>
Benjamin Gaignard Sept. 21, 2023, 9:28 a.m. UTC | #5
Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
> On 20/09/2023 16:30, Benjamin Gaignard wrote:
> <snip>
>
>>>>        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.
> What is the backtrace? How can this happen? It feels wrong that this can be
> called with a vb2_queue that apparently is not properly initialized.

I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap:

[   18.924627] Call trace:
[   18.927090]  dump_backtrace+0x94/0xec
[   18.930787]  show_stack+0x18/0x24
[   18.934137]  dump_stack_lvl+0x48/0x60
[   18.937833]  dump_stack+0x18/0x24
[   18.941166]  __vb2_queue_cancel+0x23c/0x2f0
[   18.945365]  vb2_core_queue_release+0x24/0x6c
[   18.949740]  vb2_queue_release+0x10/0x1c
[   18.953677]  v4l2_m2m_ctx_release+0x20/0x40
[   18.957892]  hantro_release+0x20/0x54
[   18.961584]  v4l2_release+0x74/0xec
[   18.965110]  __fput+0xb4/0x274
[   18.968205]  __fput_sync+0x50/0x5c
[   18.971626]  __arm64_sys_close+0x38/0x7c
[   18.975562]  invoke_syscall+0x48/0x114
[   18.979329]  el0_svc_common.constprop.0+0xc0/0xe0
[   18.984068]  do_el0_svc+0x1c/0x28
[   18.987402]  el0_svc+0x40/0xe8
[   18.990470]  el0t_64_sync_handler+0x100/0x12c
[   18.994842]  el0t_64_sync+0x190/0x194

This happen at boot time when hantro driver is open and close without other actions.
     

>
>>>> +
>>>> +    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.
> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.
>
> Regards,
>
> 	Hans
>
Hans Verkuil Sept. 21, 2023, 10:24 a.m. UTC | #6
On 21/09/2023 11:28, Benjamin Gaignard wrote:
> 
> Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
>> On 20/09/2023 16:30, Benjamin Gaignard wrote:
>> <snip>
>>
>>>>>        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.
>> What is the backtrace? How can this happen? It feels wrong that this can be
>> called with a vb2_queue that apparently is not properly initialized.
> 
> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap:
> 
> [   18.924627] Call trace:
> [   18.927090]  dump_backtrace+0x94/0xec
> [   18.930787]  show_stack+0x18/0x24
> [   18.934137]  dump_stack_lvl+0x48/0x60
> [   18.937833]  dump_stack+0x18/0x24
> [   18.941166]  __vb2_queue_cancel+0x23c/0x2f0
> [   18.945365]  vb2_core_queue_release+0x24/0x6c
> [   18.949740]  vb2_queue_release+0x10/0x1c
> [   18.953677]  v4l2_m2m_ctx_release+0x20/0x40
> [   18.957892]  hantro_release+0x20/0x54
> [   18.961584]  v4l2_release+0x74/0xec
> [   18.965110]  __fput+0xb4/0x274
> [   18.968205]  __fput_sync+0x50/0x5c
> [   18.971626]  __arm64_sys_close+0x38/0x7c
> [   18.975562]  invoke_syscall+0x48/0x114
> [   18.979329]  el0_svc_common.constprop.0+0xc0/0xe0
> [   18.984068]  do_el0_svc+0x1c/0x28
> [   18.987402]  el0_svc+0x40/0xe8
> [   18.990470]  el0t_64_sync_handler+0x100/0x12c
> [   18.994842]  el0t_64_sync+0x190/0x194
> 
> This happen at boot time when hantro driver is open and close without other actions.

Ah, now I see the problem. q->bufs and q->bufs_map are allocated in
vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated
in vb2_queue_init: that's the counterpart of vb2_core_queue_release.

With that change you shouldn't have to check for q->bufs/bufs_map anymore.

Regards,

	Hans

>    
>>
>>>>> +
>>>>> +    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.
>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.
>>
>> Regards,
>>
>>     Hans
>>
Benjamin Gaignard Sept. 21, 2023, 12:05 p.m. UTC | #7
Le 21/09/2023 à 12:24, Hans Verkuil a écrit :
> On 21/09/2023 11:28, Benjamin Gaignard wrote:
>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
>>> On 20/09/2023 16:30, Benjamin Gaignard wrote:
>>> <snip>
>>>
>>>>>>         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.
>>> What is the backtrace? How can this happen? It feels wrong that this can be
>>> called with a vb2_queue that apparently is not properly initialized.
>> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap:
>>
>> [   18.924627] Call trace:
>> [   18.927090]  dump_backtrace+0x94/0xec
>> [   18.930787]  show_stack+0x18/0x24
>> [   18.934137]  dump_stack_lvl+0x48/0x60
>> [   18.937833]  dump_stack+0x18/0x24
>> [   18.941166]  __vb2_queue_cancel+0x23c/0x2f0
>> [   18.945365]  vb2_core_queue_release+0x24/0x6c
>> [   18.949740]  vb2_queue_release+0x10/0x1c
>> [   18.953677]  v4l2_m2m_ctx_release+0x20/0x40
>> [   18.957892]  hantro_release+0x20/0x54
>> [   18.961584]  v4l2_release+0x74/0xec
>> [   18.965110]  __fput+0xb4/0x274
>> [   18.968205]  __fput_sync+0x50/0x5c
>> [   18.971626]  __arm64_sys_close+0x38/0x7c
>> [   18.975562]  invoke_syscall+0x48/0x114
>> [   18.979329]  el0_svc_common.constprop.0+0xc0/0xe0
>> [   18.984068]  do_el0_svc+0x1c/0x28
>> [   18.987402]  el0_svc+0x40/0xe8
>> [   18.990470]  el0t_64_sync_handler+0x100/0x12c
>> [   18.994842]  el0t_64_sync+0x190/0x194
>>
>> This happen at boot time when hantro driver is open and close without other actions.
> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in
> vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated
> in vb2_queue_init: that's the counterpart of vb2_core_queue_release.
>
> With that change you shouldn't have to check for q->bufs/bufs_map anymore.

It is a better solution but even like this vb2_core_queue_release() is called
at least 2 times on the same vivid queue and without testing q->bufs_bitmap
makes kernel crash.

>
> Regards,
>
> 	Hans
>
>>     
>>>>>> +
>>>>>> +    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.
>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.
>>>
>>> Regards,
>>>
>>>      Hans
>>>
>
Hans Verkuil Sept. 21, 2023, 12:13 p.m. UTC | #8
On 21/09/2023 14:05, Benjamin Gaignard wrote:
> 
> Le 21/09/2023 à 12:24, Hans Verkuil a écrit :
>> On 21/09/2023 11:28, Benjamin Gaignard wrote:
>>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
>>>> On 20/09/2023 16:30, Benjamin Gaignard wrote:
>>>> <snip>
>>>>
>>>>>>>         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.
>>>> What is the backtrace? How can this happen? It feels wrong that this can be
>>>> called with a vb2_queue that apparently is not properly initialized.
>>> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap:
>>>
>>> [   18.924627] Call trace:
>>> [   18.927090]  dump_backtrace+0x94/0xec
>>> [   18.930787]  show_stack+0x18/0x24
>>> [   18.934137]  dump_stack_lvl+0x48/0x60
>>> [   18.937833]  dump_stack+0x18/0x24
>>> [   18.941166]  __vb2_queue_cancel+0x23c/0x2f0
>>> [   18.945365]  vb2_core_queue_release+0x24/0x6c
>>> [   18.949740]  vb2_queue_release+0x10/0x1c
>>> [   18.953677]  v4l2_m2m_ctx_release+0x20/0x40
>>> [   18.957892]  hantro_release+0x20/0x54
>>> [   18.961584]  v4l2_release+0x74/0xec
>>> [   18.965110]  __fput+0xb4/0x274
>>> [   18.968205]  __fput_sync+0x50/0x5c
>>> [   18.971626]  __arm64_sys_close+0x38/0x7c
>>> [   18.975562]  invoke_syscall+0x48/0x114
>>> [   18.979329]  el0_svc_common.constprop.0+0xc0/0xe0
>>> [   18.984068]  do_el0_svc+0x1c/0x28
>>> [   18.987402]  el0_svc+0x40/0xe8
>>> [   18.990470]  el0t_64_sync_handler+0x100/0x12c
>>> [   18.994842]  el0t_64_sync+0x190/0x194
>>>
>>> This happen at boot time when hantro driver is open and close without other actions.
>> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in
>> vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated
>> in vb2_queue_init: that's the counterpart of vb2_core_queue_release.
>>
>> With that change you shouldn't have to check for q->bufs/bufs_map anymore.
> 
> It is a better solution but even like this vb2_core_queue_release() is called
> at least 2 times on the same vivid queue and without testing q->bufs_bitmap
> makes kernel crash.

Do you have a stacktrace for that? Perhaps vb2_core_queue_release should check
for q->bufs/q->bufs_map and return if those are NULL. But it could also be a
bug that it is called twice, it just was never noticed because it was harmless
before.

Regards,

	Hans

> 
>>
>> Regards,
>>
>>     Hans
>>
>>>    
>>>>>>> +
>>>>>>> +    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.
>>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.
>>>>
>>>> Regards,
>>>>
>>>>      Hans
>>>>
>>
Benjamin Gaignard Sept. 21, 2023, 12:46 p.m. UTC | #9
Le 21/09/2023 à 14:13, Hans Verkuil a écrit :
> On 21/09/2023 14:05, Benjamin Gaignard wrote:
>> Le 21/09/2023 à 12:24, Hans Verkuil a écrit :
>>> On 21/09/2023 11:28, Benjamin Gaignard wrote:
>>>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
>>>>> On 20/09/2023 16:30, Benjamin Gaignard wrote:
>>>>> <snip>
>>>>>
>>>>>>>>          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.
>>>>> What is the backtrace? How can this happen? It feels wrong that this can be
>>>>> called with a vb2_queue that apparently is not properly initialized.
>>>> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap:
>>>>
>>>> [   18.924627] Call trace:
>>>> [   18.927090]  dump_backtrace+0x94/0xec
>>>> [   18.930787]  show_stack+0x18/0x24
>>>> [   18.934137]  dump_stack_lvl+0x48/0x60
>>>> [   18.937833]  dump_stack+0x18/0x24
>>>> [   18.941166]  __vb2_queue_cancel+0x23c/0x2f0
>>>> [   18.945365]  vb2_core_queue_release+0x24/0x6c
>>>> [   18.949740]  vb2_queue_release+0x10/0x1c
>>>> [   18.953677]  v4l2_m2m_ctx_release+0x20/0x40
>>>> [   18.957892]  hantro_release+0x20/0x54
>>>> [   18.961584]  v4l2_release+0x74/0xec
>>>> [   18.965110]  __fput+0xb4/0x274
>>>> [   18.968205]  __fput_sync+0x50/0x5c
>>>> [   18.971626]  __arm64_sys_close+0x38/0x7c
>>>> [   18.975562]  invoke_syscall+0x48/0x114
>>>> [   18.979329]  el0_svc_common.constprop.0+0xc0/0xe0
>>>> [   18.984068]  do_el0_svc+0x1c/0x28
>>>> [   18.987402]  el0_svc+0x40/0xe8
>>>> [   18.990470]  el0t_64_sync_handler+0x100/0x12c
>>>> [   18.994842]  el0t_64_sync+0x190/0x194
>>>>
>>>> This happen at boot time when hantro driver is open and close without other actions.
>>> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in
>>> vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated
>>> in vb2_queue_init: that's the counterpart of vb2_core_queue_release.
>>>
>>> With that change you shouldn't have to check for q->bufs/bufs_map anymore.
>> It is a better solution but even like this vb2_core_queue_release() is called
>> at least 2 times on the same vivid queue and without testing q->bufs_bitmap
>> makes kernel crash.
> Do you have a stacktrace for that? Perhaps vb2_core_queue_release should check
> for q->bufs/q->bufs_map and return if those are NULL. But it could also be a
> bug that it is called twice, it just was never noticed because it was harmless
> before.

I have added some printk to log that when running test-media on vivid:

[  130.497426] vb2_core_queue_init queue cap-0000000050d195ab allocate q->bufs 00000000dc2c15ed and q->bufs_bitmap 000000008173fc5a
...
[  130.733967] vb2_core_queue_release queue cap-0000000050d195ab release q->bufs and q->bufs_bitmap
[  133.866345] vb2_get_buffer queue cap-0000000050d195ab q->bufs_bitmap is NULL
[  133.873454] CPU: 1 PID: 321 Comm: v4l2-ctl Not tainted 6.6.0-rc1+ #542
[  133.879997] Hardware name: NXP i.MX8MQ EVK (DT)
[  133.884536] Call trace:
[  133.886988]  dump_backtrace+0x94/0xec
[  133.890673]  show_stack+0x18/0x24
[  133.894002]  dump_stack_lvl+0x48/0x60
[  133.897681]  dump_stack+0x18/0x24
[  133.901009]  __vb2_queue_cancel+0x250/0x31c
[  133.905209]  vb2_core_queue_release+0x24/0x88
[  133.909580]  _vb2_fop_release+0xb0/0xbc
[  133.913428]  vb2_fop_release+0x2c/0x58
[  133.917187]  vivid_fop_release+0x80/0x388 [vivid]
[  133.921948]  v4l2_release+0x74/0xec
[  133.925452]  __fput+0xb4/0x274
[  133.928520]  __fput_sync+0x50/0x5c
[  133.931934]  __arm64_sys_close+0x38/0x7c
[  133.935868]  invoke_syscall+0x48/0x114
[  133.939630]  el0_svc_common.constprop.0+0x40/0xe0
[  133.944349]  do_el0_svc+0x1c/0x28
[  133.947677]  el0_svc+0x40/0xe8
[  133.950741]  el0t_64_sync_handler+0x100/0x12c
[  133.955109]  el0t_64_sync+0x190/0x194

and later I have a call to reqbufs on the same queue without call to vb2_core_queue_init before

[   58.696812] __vb2_queue_alloc queue cap- 0000000050d195abq->bufs_bitmap is NULL
[   58.704148] CPU: 1 PID: 319 Comm: v4l2-compliance Not tainted 6.6.0-rc1+ #544
[   58.711291] Hardware name: NXP i.MX8MQ EVK (DT)
[   58.715826] Call trace:
[   58.718274]  dump_backtrace+0x94/0xec
[   58.721951]  show_stack+0x18/0x24
[   58.725274]  dump_stack_lvl+0x48/0x60
[   58.728946]  dump_stack+0x18/0x24
[   58.732268]  __vb2_queue_alloc+0x4a8/0x50c
[   58.736374]  vb2_core_reqbufs+0x274/0x46c
[   58.740391]  vb2_ioctl_reqbufs+0xb0/0xe8
[   58.744320]  vidioc_reqbufs+0x50/0x64 [vivid]
[   58.748717]  v4l_reqbufs+0x50/0x64
[   58.752125]  __video_do_ioctl+0x164/0x3c8
[   58.756140]  video_usercopy+0x200/0x668
[   58.759982]  video_ioctl2+0x18/0x28
[   58.763475]  v4l2_ioctl+0x40/0x60
[   58.766798]  __arm64_sys_ioctl+0xac/0xf0
[   58.770730]  invoke_syscall+0x48/0x114
[   58.774487]  el0_svc_common.constprop.0+0x40/0xe0
[   58.779199]  do_el0_svc+0x1c/0x28
[   58.782520]  el0_svc+0x40/0xe8
[   58.785580]  el0t_64_sync_handler+0x100/0x12c
[   58.789942]  el0t_64_sync+0x190/0x194

>
> Regards,
>
> 	Hans
>
>>> Regards,
>>>
>>>      Hans
>>>
>>>>     
>>>>>>>> +
>>>>>>>> +    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.
>>>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.
>>>>>
>>>>> Regards,
>>>>>
>>>>>       Hans
>>>>>
>
Benjamin Gaignard Sept. 21, 2023, 1:07 p.m. UTC | #10
Le 21/09/2023 à 14:46, Benjamin Gaignard a écrit :
>
> Le 21/09/2023 à 14:13, Hans Verkuil a écrit :
>> On 21/09/2023 14:05, Benjamin Gaignard wrote:
>>> Le 21/09/2023 à 12:24, Hans Verkuil a écrit :
>>>> On 21/09/2023 11:28, Benjamin Gaignard wrote:
>>>>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
>>>>>> On 20/09/2023 16:30, Benjamin Gaignard wrote:
>>>>>> <snip>
>>>>>>
>>>>>>>>>          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.
>>>>>> What is the backtrace? How can this happen? It feels wrong that 
>>>>>> this can be
>>>>>> called with a vb2_queue that apparently is not properly initialized.
>>>>> I have this log when adding dump_stack() in vb2_get_buffer() if 
>>>>> !q->bufs_bitmap:
>>>>>
>>>>> [   18.924627] Call trace:
>>>>> [   18.927090]  dump_backtrace+0x94/0xec
>>>>> [   18.930787]  show_stack+0x18/0x24
>>>>> [   18.934137]  dump_stack_lvl+0x48/0x60
>>>>> [   18.937833]  dump_stack+0x18/0x24
>>>>> [   18.941166]  __vb2_queue_cancel+0x23c/0x2f0
>>>>> [   18.945365]  vb2_core_queue_release+0x24/0x6c
>>>>> [   18.949740]  vb2_queue_release+0x10/0x1c
>>>>> [   18.953677]  v4l2_m2m_ctx_release+0x20/0x40
>>>>> [   18.957892]  hantro_release+0x20/0x54
>>>>> [   18.961584]  v4l2_release+0x74/0xec
>>>>> [   18.965110]  __fput+0xb4/0x274
>>>>> [   18.968205]  __fput_sync+0x50/0x5c
>>>>> [   18.971626]  __arm64_sys_close+0x38/0x7c
>>>>> [   18.975562]  invoke_syscall+0x48/0x114
>>>>> [   18.979329]  el0_svc_common.constprop.0+0xc0/0xe0
>>>>> [   18.984068]  do_el0_svc+0x1c/0x28
>>>>> [   18.987402]  el0_svc+0x40/0xe8
>>>>> [   18.990470]  el0t_64_sync_handler+0x100/0x12c
>>>>> [   18.994842]  el0t_64_sync+0x190/0x194
>>>>>
>>>>> This happen at boot time when hantro driver is open and close 
>>>>> without other actions.
>>>> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in
>>>> vb2_core_create_bufs and vb2_core_reqbufs, but they should be 
>>>> allocated
>>>> in vb2_queue_init: that's the counterpart of vb2_core_queue_release.

Hans,
I think we are doing loops in your comment :-)
https://patchwork.kernel.org/comment/25496456/

Regards,
Benjamin

>>>>
>>>> With that change you shouldn't have to check for q->bufs/bufs_map 
>>>> anymore.
>>> It is a better solution but even like this vb2_core_queue_release() 
>>> is called
>>> at least 2 times on the same vivid queue and without testing 
>>> q->bufs_bitmap
>>> makes kernel crash.
>> Do you have a stacktrace for that? Perhaps vb2_core_queue_release 
>> should check
>> for q->bufs/q->bufs_map and return if those are NULL. But it could 
>> also be a
>> bug that it is called twice, it just was never noticed because it was 
>> harmless
>> before.
>
> I have added some printk to log that when running test-media on vivid:
>
> [  130.497426] vb2_core_queue_init queue cap-0000000050d195ab allocate 
> q->bufs 00000000dc2c15ed and q->bufs_bitmap 000000008173fc5a
> ...
> [  130.733967] vb2_core_queue_release queue cap-0000000050d195ab 
> release q->bufs and q->bufs_bitmap
> [  133.866345] vb2_get_buffer queue cap-0000000050d195ab 
> q->bufs_bitmap is NULL
> [  133.873454] CPU: 1 PID: 321 Comm: v4l2-ctl Not tainted 6.6.0-rc1+ #542
> [  133.879997] Hardware name: NXP i.MX8MQ EVK (DT)
> [  133.884536] Call trace:
> [  133.886988]  dump_backtrace+0x94/0xec
> [  133.890673]  show_stack+0x18/0x24
> [  133.894002]  dump_stack_lvl+0x48/0x60
> [  133.897681]  dump_stack+0x18/0x24
> [  133.901009]  __vb2_queue_cancel+0x250/0x31c
> [  133.905209]  vb2_core_queue_release+0x24/0x88
> [  133.909580]  _vb2_fop_release+0xb0/0xbc
> [  133.913428]  vb2_fop_release+0x2c/0x58
> [  133.917187]  vivid_fop_release+0x80/0x388 [vivid]
> [  133.921948]  v4l2_release+0x74/0xec
> [  133.925452]  __fput+0xb4/0x274
> [  133.928520]  __fput_sync+0x50/0x5c
> [  133.931934]  __arm64_sys_close+0x38/0x7c
> [  133.935868]  invoke_syscall+0x48/0x114
> [  133.939630]  el0_svc_common.constprop.0+0x40/0xe0
> [  133.944349]  do_el0_svc+0x1c/0x28
> [  133.947677]  el0_svc+0x40/0xe8
> [  133.950741]  el0t_64_sync_handler+0x100/0x12c
> [  133.955109]  el0t_64_sync+0x190/0x194
>
> and later I have a call to reqbufs on the same queue without call to 
> vb2_core_queue_init before
>
> [   58.696812] __vb2_queue_alloc queue cap- 
> 0000000050d195abq->bufs_bitmap is NULL
> [   58.704148] CPU: 1 PID: 319 Comm: v4l2-compliance Not tainted 
> 6.6.0-rc1+ #544
> [   58.711291] Hardware name: NXP i.MX8MQ EVK (DT)
> [   58.715826] Call trace:
> [   58.718274]  dump_backtrace+0x94/0xec
> [   58.721951]  show_stack+0x18/0x24
> [   58.725274]  dump_stack_lvl+0x48/0x60
> [   58.728946]  dump_stack+0x18/0x24
> [   58.732268]  __vb2_queue_alloc+0x4a8/0x50c
> [   58.736374]  vb2_core_reqbufs+0x274/0x46c
> [   58.740391]  vb2_ioctl_reqbufs+0xb0/0xe8
> [   58.744320]  vidioc_reqbufs+0x50/0x64 [vivid]
> [   58.748717]  v4l_reqbufs+0x50/0x64
> [   58.752125]  __video_do_ioctl+0x164/0x3c8
> [   58.756140]  video_usercopy+0x200/0x668
> [   58.759982]  video_ioctl2+0x18/0x28
> [   58.763475]  v4l2_ioctl+0x40/0x60
> [   58.766798]  __arm64_sys_ioctl+0xac/0xf0
> [   58.770730]  invoke_syscall+0x48/0x114
> [   58.774487]  el0_svc_common.constprop.0+0x40/0xe0
> [   58.779199]  do_el0_svc+0x1c/0x28
> [   58.782520]  el0_svc+0x40/0xe8
> [   58.785580]  el0t_64_sync_handler+0x100/0x12c
> [   58.789942]  el0t_64_sync+0x190/0x194
>
>>
>> Regards,
>>
>>     Hans
>>
>>>> Regards,
>>>>
>>>>      Hans
>>>>
>>>>>>>>> +
>>>>>>>>> +    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.
>>>>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>>       Hans
>>>>>>
>>
Hans Verkuil Sept. 21, 2023, 1:48 p.m. UTC | #11
On 21/09/2023 14:46, Benjamin Gaignard wrote:
> 
> Le 21/09/2023 à 14:13, Hans Verkuil a écrit :
>> On 21/09/2023 14:05, Benjamin Gaignard wrote:
>>> Le 21/09/2023 à 12:24, Hans Verkuil a écrit :
>>>> On 21/09/2023 11:28, Benjamin Gaignard wrote:
>>>>> Le 20/09/2023 à 16:56, Hans Verkuil a écrit :
>>>>>> On 20/09/2023 16:30, Benjamin Gaignard wrote:
>>>>>> <snip>
>>>>>>
>>>>>>>>>          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.
>>>>>> What is the backtrace? How can this happen? It feels wrong that this can be
>>>>>> called with a vb2_queue that apparently is not properly initialized.
>>>>> I have this log when adding dump_stack() in vb2_get_buffer() if !q->bufs_bitmap:
>>>>>
>>>>> [   18.924627] Call trace:
>>>>> [   18.927090]  dump_backtrace+0x94/0xec
>>>>> [   18.930787]  show_stack+0x18/0x24
>>>>> [   18.934137]  dump_stack_lvl+0x48/0x60
>>>>> [   18.937833]  dump_stack+0x18/0x24
>>>>> [   18.941166]  __vb2_queue_cancel+0x23c/0x2f0
>>>>> [   18.945365]  vb2_core_queue_release+0x24/0x6c
>>>>> [   18.949740]  vb2_queue_release+0x10/0x1c
>>>>> [   18.953677]  v4l2_m2m_ctx_release+0x20/0x40
>>>>> [   18.957892]  hantro_release+0x20/0x54
>>>>> [   18.961584]  v4l2_release+0x74/0xec
>>>>> [   18.965110]  __fput+0xb4/0x274
>>>>> [   18.968205]  __fput_sync+0x50/0x5c
>>>>> [   18.971626]  __arm64_sys_close+0x38/0x7c
>>>>> [   18.975562]  invoke_syscall+0x48/0x114
>>>>> [   18.979329]  el0_svc_common.constprop.0+0xc0/0xe0
>>>>> [   18.984068]  do_el0_svc+0x1c/0x28
>>>>> [   18.987402]  el0_svc+0x40/0xe8
>>>>> [   18.990470]  el0t_64_sync_handler+0x100/0x12c
>>>>> [   18.994842]  el0t_64_sync+0x190/0x194
>>>>>
>>>>> This happen at boot time when hantro driver is open and close without other actions.
>>>> Ah, now I see the problem. q->bufs and q->bufs_map are allocated in
>>>> vb2_core_create_bufs and vb2_core_reqbufs, but they should be allocated
>>>> in vb2_queue_init: that's the counterpart of vb2_core_queue_release.
>>>>
>>>> With that change you shouldn't have to check for q->bufs/bufs_map anymore.
>>> It is a better solution but even like this vb2_core_queue_release() is called
>>> at least 2 times on the same vivid queue and without testing q->bufs_bitmap
>>> makes kernel crash.
>> Do you have a stacktrace for that? Perhaps vb2_core_queue_release should check
>> for q->bufs/q->bufs_map and return if those are NULL. But it could also be a
>> bug that it is called twice, it just was never noticed because it was harmless
>> before.
> 
> I have added some printk to log that when running test-media on vivid:
> 
> [  130.497426] vb2_core_queue_init queue cap-0000000050d195ab allocate q->bufs 00000000dc2c15ed and q->bufs_bitmap 000000008173fc5a
> ...
> [  130.733967] vb2_core_queue_release queue cap-0000000050d195ab release q->bufs and q->bufs_bitmap
> [  133.866345] vb2_get_buffer queue cap-0000000050d195ab q->bufs_bitmap is NULL
> [  133.873454] CPU: 1 PID: 321 Comm: v4l2-ctl Not tainted 6.6.0-rc1+ #542
> [  133.879997] Hardware name: NXP i.MX8MQ EVK (DT)
> [  133.884536] Call trace:
> [  133.886988]  dump_backtrace+0x94/0xec
> [  133.890673]  show_stack+0x18/0x24
> [  133.894002]  dump_stack_lvl+0x48/0x60
> [  133.897681]  dump_stack+0x18/0x24
> [  133.901009]  __vb2_queue_cancel+0x250/0x31c
> [  133.905209]  vb2_core_queue_release+0x24/0x88
> [  133.909580]  _vb2_fop_release+0xb0/0xbc
> [  133.913428]  vb2_fop_release+0x2c/0x58
> [  133.917187]  vivid_fop_release+0x80/0x388 [vivid]
> [  133.921948]  v4l2_release+0x74/0xec
> [  133.925452]  __fput+0xb4/0x274
> [  133.928520]  __fput_sync+0x50/0x5c
> [  133.931934]  __arm64_sys_close+0x38/0x7c
> [  133.935868]  invoke_syscall+0x48/0x114
> [  133.939630]  el0_svc_common.constprop.0+0x40/0xe0
> [  133.944349]  do_el0_svc+0x1c/0x28
> [  133.947677]  el0_svc+0x40/0xe8
> [  133.950741]  el0t_64_sync_handler+0x100/0x12c
> [  133.955109]  el0t_64_sync+0x190/0x194
> 
> and later I have a call to reqbufs on the same queue without call to vb2_core_queue_init before
> 
> [   58.696812] __vb2_queue_alloc queue cap- 0000000050d195abq->bufs_bitmap is NULL
> [   58.704148] CPU: 1 PID: 319 Comm: v4l2-compliance Not tainted 6.6.0-rc1+ #544
> [   58.711291] Hardware name: NXP i.MX8MQ EVK (DT)
> [   58.715826] Call trace:
> [   58.718274]  dump_backtrace+0x94/0xec
> [   58.721951]  show_stack+0x18/0x24
> [   58.725274]  dump_stack_lvl+0x48/0x60
> [   58.728946]  dump_stack+0x18/0x24
> [   58.732268]  __vb2_queue_alloc+0x4a8/0x50c
> [   58.736374]  vb2_core_reqbufs+0x274/0x46c
> [   58.740391]  vb2_ioctl_reqbufs+0xb0/0xe8
> [   58.744320]  vidioc_reqbufs+0x50/0x64 [vivid]
> [   58.748717]  v4l_reqbufs+0x50/0x64
> [   58.752125]  __video_do_ioctl+0x164/0x3c8
> [   58.756140]  video_usercopy+0x200/0x668
> [   58.759982]  video_ioctl2+0x18/0x28
> [   58.763475]  v4l2_ioctl+0x40/0x60
> [   58.766798]  __arm64_sys_ioctl+0xac/0xf0
> [   58.770730]  invoke_syscall+0x48/0x114
> [   58.774487]  el0_svc_common.constprop.0+0x40/0xe0
> [   58.779199]  do_el0_svc+0x1c/0x28
> [   58.782520]  el0_svc+0x40/0xe8
> [   58.785580]  el0t_64_sync_handler+0x100/0x12c
> [   58.789942]  el0t_64_sync+0x190/0x194

Argh, I see what is happening. The root cause is that vb2_core_queue_release
is actually not a true counterpart to vb2_core_queue_init.

The '_release' part refers to when a file handle is released, and not to
releasing resources allocated in queue_init.

The queue_init function never actually allocated any resources, so there
was never a reason to make a counterpart to that, but now that bites us.

Changing this would be a huge amount of work, and it is not worth the
effort, IMHO.

But at least we shouldn't have to test for both bufs and bufs_map,
they are either both set or both NULL. Just test one of the two.

The vb2_core_queue_init() function documentation in the header
should perhaps be more clear about the fact that this function
does not allocate any resources, and that there is no cleanup
counterpart.

It is what got me confused...

Regards,

	Hans

> 
>>
>> Regards,
>>
>>     Hans
>>
>>>> Regards,
>>>>
>>>>      Hans
>>>>
>>>>>    
>>>>>>>>> +
>>>>>>>>> +    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.
>>>>>> Not a problem. E.g. v4l2-ctrls.h is full of such static inlines.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>>       Hans
>>>>>>
>>
diff mbox series

Patch

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)) {
 		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)) {
 		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 */
 	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;
+
+	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;
+
+	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;
+
+	return (bitmap_weight(q->bufs_map, q->max_allowed_buffers) > 0);
 }
 
 /**