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
Series Add DELETE_BUF ioctl |

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

kernel test robot Sept. 15, 2023, 12:47 a.m. UTC | #1 
Hi Benjamin,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc1]
[cannot apply to next-20230914]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Rework-offset-cookie-encoding-pattern/20230914-221757
base:   linus/master
patch link:    https://lore.kernel.org/r/20230914133323.198857-46-benjamin.gaignard%40collabora.com
patch subject: [PATCH v7 45/49] media: core: Add bitmap manage bufs array entries
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309150835.kxjWQyEU-lkp@intel.com/

All errors (new ones prefixed by >>):

   samples/v4l/v4l2-pci-skeleton.c: In function 'queue_setup':
>> samples/v4l/v4l2-pci-skeleton.c:170:15: error: 'struct vb2_queue' has no member named 'num_buffers'
     170 |         if (vq->num_buffers + *nbuffers < 3)
         |               ^~
   samples/v4l/v4l2-pci-skeleton.c:171:35: error: 'struct vb2_queue' has no member named 'num_buffers'
     171 |                 *nbuffers = 3 - vq->num_buffers;
         |                                   ^~
--
   drivers/input/touchscreen/sur40.c: In function 'sur40_queue_setup':
>> drivers/input/touchscreen/sur40.c:851:14: error: 'struct vb2_queue' has no member named 'num_buffers'
     851 |         if (q->num_buffers + *nbuffers < 3)
         |              ^~
   drivers/input/touchscreen/sur40.c:852:34: error: 'struct vb2_queue' has no member named 'num_buffers'
     852 |                 *nbuffers = 3 - q->num_buffers;
         |                                  ^~


vim +170 samples/v4l/v4l2-pci-skeleton.c

926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  145  
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  146  /*
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  147   * Setup the constraints of the queue: besides setting the number of planes
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  148   * per buffer and the size and allocation context of each plane, it also
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  149   * checks if sufficient buffers have been allocated. Usually 3 is a good
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  150   * minimum number: many DMA engines need a minimum of 2 buffers in the
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  151   * queue and you need to have another available for userspace processing.
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  152   */
df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  153  static int queue_setup(struct vb2_queue *vq,
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  154  		       unsigned int *nbuffers, unsigned int *nplanes,
36c0f8b32c4bd4 samples/v4l/v4l2-pci-skeleton.c               Hans Verkuil 2016-04-15  155  		       unsigned int sizes[], struct device *alloc_devs[])
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  156  {
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  157  	struct skeleton *skel = vb2_get_drv_priv(vq);
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  158  
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  159  	skel->field = skel->format.field;
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  160  	if (skel->field == V4L2_FIELD_ALTERNATE) {
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  161  		/*
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  162  		 * You cannot use read() with FIELD_ALTERNATE since the field
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  163  		 * information (TOP/BOTTOM) cannot be passed back to the user.
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  164  		 */
3130a28a1568b1 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-23  165  		if (vb2_fileio_is_active(vq))
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  166  			return -EINVAL;
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  167  		skel->field = V4L2_FIELD_TOP;
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  168  	}
5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  169  
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 @170  	if (vq->num_buffers + *nbuffers < 3)
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  171  		*nbuffers = 3 - vq->num_buffers;
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  172  
df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  173  	if (*nplanes)
df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  174  		return sizes[0] < skel->format.sizeimage ? -EINVAL : 0;
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  175  	*nplanes = 1;
df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  176  	sizes[0] = skel->format.sizeimage;
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  177  	return 0;
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  178  }
926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  179
Benjamin Gaignard Sept. 15, 2023, 1:02 p.m. UTC | #2 
Le 15/09/2023 à 02:47, kernel test robot a écrit :
> Hi Benjamin,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.6-rc1]
> [cannot apply to next-20230914]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Rework-offset-cookie-encoding-pattern/20230914-221757
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20230914133323.198857-46-benjamin.gaignard%40collabora.com
> patch subject: [PATCH v7 45/49] media: core: Add bitmap manage bufs array entries
> config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/config)
> compiler: sparc64-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230915/202309150835.kxjWQyEU-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202309150835.kxjWQyEU-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>     samples/v4l/v4l2-pci-skeleton.c: In function 'queue_setup':
>>> samples/v4l/v4l2-pci-skeleton.c:170:15: error: 'struct vb2_queue' has no member named 'num_buffers'
>       170 |         if (vq->num_buffers + *nbuffers < 3)
>           |               ^~
>     samples/v4l/v4l2-pci-skeleton.c:171:35: error: 'struct vb2_queue' has no member named 'num_buffers'
>       171 |                 *nbuffers = 3 - vq->num_buffers;
>           |                                   ^~
> --
>     drivers/input/touchscreen/sur40.c: In function 'sur40_queue_setup':
>>> drivers/input/touchscreen/sur40.c:851:14: error: 'struct vb2_queue' has no member named 'num_buffers'
>       851 |         if (q->num_buffers + *nbuffers < 3)
>           |              ^~
>     drivers/input/touchscreen/sur40.c:852:34: error: 'struct vb2_queue' has no member named 'num_buffers'
>       852 |                 *nbuffers = 3 - q->num_buffers;
>           |                                  ^~
>
>
> vim +170 samples/v4l/v4l2-pci-skeleton.c

v4l2 drivers outside drivers/media or drivers/staging/media direct directories.
I have miss them. I will fix that in v8.

Regards,
Benjamin

>
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  145
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  146  /*
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  147   * Setup the constraints of the queue: besides setting the number of planes
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  148   * per buffer and the size and allocation context of each plane, it also
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  149   * checks if sufficient buffers have been allocated. Usually 3 is a good
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  150   * minimum number: many DMA engines need a minimum of 2 buffers in the
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  151   * queue and you need to have another available for userspace processing.
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  152   */
> df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  153  static int queue_setup(struct vb2_queue *vq,
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  154  		       unsigned int *nbuffers, unsigned int *nplanes,
> 36c0f8b32c4bd4 samples/v4l/v4l2-pci-skeleton.c               Hans Verkuil 2016-04-15  155  		       unsigned int sizes[], struct device *alloc_devs[])
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  156  {
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  157  	struct skeleton *skel = vb2_get_drv_priv(vq);
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  158
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  159  	skel->field = skel->format.field;
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  160  	if (skel->field == V4L2_FIELD_ALTERNATE) {
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  161  		/*
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  162  		 * You cannot use read() with FIELD_ALTERNATE since the field
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  163  		 * information (TOP/BOTTOM) cannot be passed back to the user.
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  164  		 */
> 3130a28a1568b1 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-23  165  		if (vb2_fileio_is_active(vq))
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  166  			return -EINVAL;
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  167  		skel->field = V4L2_FIELD_TOP;
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  168  	}
> 5f26f2501b8119 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-04-11  169
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14 @170  	if (vq->num_buffers + *nbuffers < 3)
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  171  		*nbuffers = 3 - vq->num_buffers;
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  172
> df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  173  	if (*nplanes)
> df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  174  		return sizes[0] < skel->format.sizeimage ? -EINVAL : 0;
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  175  	*nplanes = 1;
> df9ecb0cad14b9 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2015-10-28  176  	sizes[0] = skel->format.sizeimage;
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  177  	return 0;
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  178  }
> 926977e0ae7556 Documentation/video4linux/v4l2-pci-skeleton.c Hans Verkuil 2014-03-14  179
>
Hans Verkuil Sept. 19, 2023, 3 p.m. UTC | #3 
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 | #4 
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 | #5 
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 | #6 
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 | #7 
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 | #8 
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 | #9 
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 | #10 
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 | #11 
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 | #12 
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 | #13 
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);
 }
 
 /**