diff mbox series

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

Message ID 20230321102855.346732-3-benjamin.gaignard@collabora.com
State New
Headers
Series Add DELETE_BUF ioctl |

Commit Message

Benjamin Gaignard March 21, 2023, 10:28 a.m. UTC 
  Instead of a static array change bufs to a dynamically allocated array.
This will allow to store more video buffer if needed.
Protect the array with a spinlock.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 .../media/common/videobuf2/videobuf2-core.c   |  8 +++
 include/media/videobuf2-core.h                | 49 ++++++++++++++++---
 2 files changed, 49 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart March 21, 2023, 6:16 p.m. UTC | #1 
Hi Benjamin,

Thank you for the patch.

On Tue, Mar 21, 2023 at 11:28:49AM +0100, Benjamin Gaignard wrote:
> Instead of a static array change bufs to a dynamically allocated array.
> This will allow to store more video buffer if needed.
> Protect the array with a spinlock.

I think I asked in the review of v1 why we couldn't use the kernel
IDA/IDR APIs to allocate buffer IDs and register buffers, but I don't
think I've received a reply. Wouldn't it be better than rolling out our
own mechanism ?

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   |  8 +++
>  include/media/videobuf2-core.h                | 49 ++++++++++++++++---
>  2 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 79e90e338846..ae9d72f4d181 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2452,6 +2452,13 @@ int vb2_core_queue_init(struct vb2_queue *q)
>  	mutex_init(&q->mmap_lock);
>  	init_waitqueue_head(&q->done_wq);
>  
> +	q->max_num_bufs = 32;
> +	q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
> +	if (!q->bufs)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&q->bufs_lock);
> +
>  	q->memory = VB2_MEMORY_UNKNOWN;
>  
>  	if (q->buf_struct_size == 0)
> @@ -2479,6 +2486,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
>  	mutex_lock(&q->mmap_lock);
>  	__vb2_queue_free(q, q->num_buffers);
>  	mutex_unlock(&q->mmap_lock);
> +	kfree(q->bufs);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_queue_release);
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 5b1e3d801546..397dbf6e61e1 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -558,6 +558,8 @@ struct vb2_buf_ops {
>   * @dma_dir:	DMA mapping direction.
>   * @bufs:	videobuf2 buffer structures
>   * @num_buffers: number of allocated/used buffers
> + * @bufs_lock: lock to protect bufs access
> + * @max_num_bufs: max number of buffers storable in bufs
>   * @queued_list: list of buffers currently queued from userspace
>   * @queued_count: number of buffers queued and ready for streaming.
>   * @owned_by_drv_count: number of buffers owned by the driver
> @@ -619,8 +621,10 @@ struct vb2_queue {
>  	struct mutex			mmap_lock;
>  	unsigned int			memory;
>  	enum dma_data_direction		dma_dir;
> -	struct vb2_buffer		*bufs[VB2_MAX_FRAME];
> +	struct vb2_buffer		**bufs;
>  	unsigned int			num_buffers;
> +	spinlock_t			bufs_lock;
> +	size_t				max_num_bufs;
>  
>  	struct list_head		queued_list;
>  	unsigned int			queued_count;
> @@ -1239,9 +1243,16 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
>  static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>  						unsigned int index)
>  {
> -	if (index < q->num_buffers)
> -		return q->bufs[index];
> -	return NULL;
> +	struct vb2_buffer *vb = NULL;
> +
> +	spin_lock(&q->bufs_lock);
> +
> +	if (index < q->max_num_bufs)
> +		vb = q->bufs[index];
> +
> +	spin_unlock(&q->bufs_lock);
> +
> +	return vb;
>  }
>  
>  /**
> @@ -1251,12 +1262,30 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>   */
>  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>  {
> -	if (vb->index < VB2_MAX_FRAME) {
> +	bool ret = false;
> +
> +	spin_lock(&q->bufs_lock);
> +
> +	if (vb->index >= q->max_num_bufs) {
> +		struct vb2_buffer **tmp;
> +
> +		tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> +		if (!tmp)
> +			goto realloc_failed;
> +
> +		q->max_num_bufs *= 2;
> +		q->bufs = tmp;
> +	}
> +
> +	if (vb->index < q->max_num_bufs) {
>  		q->bufs[vb->index] = vb;
> -		return true;
> +		ret = true;
>  	}
>  
> -	return false;
> +realloc_failed:
> +	spin_unlock(&q->bufs_lock);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -1266,8 +1295,12 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
>   */
>  static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>  {
> -	if (vb->index < VB2_MAX_FRAME)
> +	spin_lock(&q->bufs_lock);
> +
> +	if (vb->index < q->max_num_bufs)
>  		q->bufs[vb->index] = NULL;
> +
> +	spin_unlock(&q->bufs_lock);
>  }
>  
>  /*
Dan Carpenter March 24, 2023, 5:01 a.m. UTC | #2 
Hi Benjamin,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/

smatch warnings:
include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array

vim +1272 include/media/videobuf2-core.h

625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1263  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1264  {
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1265  	bool ret = false;
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1266  
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1267  	spin_lock(&q->bufs_lock);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^
Holding a spin lock.

487d3f14d12ecf Benjamin Gaignard 2023-03-21  1268  
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1269  	if (vb->index >= q->max_num_bufs) {
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1270  		struct vb2_buffer **tmp;
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1271  
487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272  		tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
                                                                                                                                     ^^^^^^^^^^
Sleeping allocation.  GFP_ATOMIC?  Or is there a way to move the
allocation outside the lock?

487d3f14d12ecf Benjamin Gaignard 2023-03-21  1273  		if (!tmp)
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1274  			goto realloc_failed;
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1275  
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1276  		q->max_num_bufs *= 2;
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1277  		q->bufs = tmp;
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1278  	}
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1279  
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1280  	if (vb->index < q->max_num_bufs) {
625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1281  		q->bufs[vb->index] = vb;
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1282  		ret = true;
625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1283  	}
625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1284  
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1285  realloc_failed:
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1286  	spin_unlock(&q->bufs_lock);
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1287  
487d3f14d12ecf Benjamin Gaignard 2023-03-21  1288  	return ret;
625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1289  }
Benjamin Gaignard March 24, 2023, 8:11 a.m. UTC | #3 
Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
> Hi Benjamin,
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
> base:   git://linuxtv.org/media_tree.git master
> patch link:    https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
> compiler: aarch64-linux-gcc (GCC) 12.1.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>
> smatch warnings:
> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>
> vim +1272 include/media/videobuf2-core.h
>
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1263  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1264  {
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1265  	bool ret = false;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1266
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1267  	spin_lock(&q->bufs_lock);
>                                                          ^^^^^^^^^^^^^^^^^^^^^^^
> Holding a spin lock.
>
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1268
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1269  	if (vb->index >= q->max_num_bufs) {
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1270  		struct vb2_buffer **tmp;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1271
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272  		tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>                                                                                                                                       ^^^^^^^^^^
> Sleeping allocation.  GFP_ATOMIC?  Or is there a way to move the
> allocation outside the lock?

I will add GFP_ATOMIC flag in next version.

Thanks for your help,
Benjamin

>
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1273  		if (!tmp)
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1274  			goto realloc_failed;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1275
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1276  		q->max_num_bufs *= 2;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1277  		q->bufs = tmp;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1278  	}
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1279
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1280  	if (vb->index < q->max_num_bufs) {
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1281  		q->bufs[vb->index] = vb;
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1282  		ret = true;
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1283  	}
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1284
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1285  realloc_failed:
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1286  	spin_unlock(&q->bufs_lock);
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1287
> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1288  	return ret;
> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1289  }
>
Hans Verkuil March 24, 2023, 8:31 a.m. UTC | #4 
On 24/03/2023 09:11, Benjamin Gaignard wrote:
> 
> Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
>> Hi Benjamin,
>>
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
>> base:   git://linuxtv.org/media_tree.git master
>> patch link:    https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
>> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
>> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
>> compiler: aarch64-linux-gcc (GCC) 12.1.0
>>
>> If you fix the issue, kindly add following tag where applicable
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Reported-by: Dan Carpenter <error27@gmail.com>
>> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>>
>> smatch warnings:
>> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
>> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>>
>> vim +1272 include/media/videobuf2-core.h
>>
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1263  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1264  {
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1265      bool ret = false;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1266
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1267      spin_lock(&q->bufs_lock);
>>                                                          ^^^^^^^^^^^^^^^^^^^^^^^
>> Holding a spin lock.
>>
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1268
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1269      if (vb->index >= q->max_num_bufs) {
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1270          struct vb2_buffer **tmp;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1271
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272          tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>>                                                                                                                                       ^^^^^^^^^^
>> Sleeping allocation.  GFP_ATOMIC?  Or is there a way to move the
>> allocation outside the lock?
> 
> I will add GFP_ATOMIC flag in next version.

No need. Instead, don't use realloc here, just allocate a new array, copy over all
the data from the old, and then switch q->bufs with the spinlock held. Then you
can free the old one.

It's only when you update q->bufs that you need the lock.

Regards,

	Hans

> 
> Thanks for your help,
> Benjamin
> 
>>
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1273          if (!tmp)
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1274              goto realloc_failed;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1275
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1276          q->max_num_bufs *= 2;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1277          q->bufs = tmp;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1278      }
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1279
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1280      if (vb->index < q->max_num_bufs) {
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1281          q->bufs[vb->index] = vb;
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1282          ret = true;
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1283      }
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1284
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1285  realloc_failed:
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1286      spin_unlock(&q->bufs_lock);
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1287
>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1288      return ret;
>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1289  }
>>
Laurent Pinchart March 24, 2023, 8:48 a.m. UTC | #5 
On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
> On 24/03/2023 09:11, Benjamin Gaignard wrote:
> > 
> > Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
> >> Hi Benjamin,
> >>
> >> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>
> >> url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
> >> base:   git://linuxtv.org/media_tree.git master
> >> patch link:    https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
> >> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
> >> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
> >> compiler: aarch64-linux-gcc (GCC) 12.1.0
> >>
> >> If you fix the issue, kindly add following tag where applicable
> >> | Reported-by: kernel test robot <lkp@intel.com>
> >> | Reported-by: Dan Carpenter <error27@gmail.com>
> >> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
> >>
> >> smatch warnings:
> >> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
> >> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
> >>
> >> vim +1272 include/media/videobuf2-core.h
> >>
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1263  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1264  {
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1265      bool ret = false;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1266
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1267      spin_lock(&q->bufs_lock);
> >>                                                          ^^^^^^^^^^^^^^^^^^^^^^^
> >> Holding a spin lock.
> >>
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1268
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1269      if (vb->index >= q->max_num_bufs) {
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1270          struct vb2_buffer **tmp;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1271
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272          tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> >>                                                                                                                                       ^^^^^^^^^^
> >> Sleeping allocation.  GFP_ATOMIC?  Or is there a way to move the
> >> allocation outside the lock?
> > 
> > I will add GFP_ATOMIC flag in next version.
> 
> No need. Instead, don't use realloc here, just allocate a new array, copy over all
> the data from the old, and then switch q->bufs with the spinlock held. Then you
> can free the old one.
> 
> It's only when you update q->bufs that you need the lock.

The copy also needs to be protected by the lock.

> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1273          if (!tmp)
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1274              goto realloc_failed;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1275
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1276          q->max_num_bufs *= 2;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1277          q->bufs = tmp;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1278      }
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1279
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1280      if (vb->index < q->max_num_bufs) {
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1281          q->bufs[vb->index] = vb;
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1282          ret = true;
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1283      }
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1284
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1285  realloc_failed:
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1286      spin_unlock(&q->bufs_lock);
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1287
> >> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1288      return ret;
> >> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1289  }
Hans Verkuil March 24, 2023, 8:52 a.m. UTC | #6 
On 24/03/2023 09:48, Laurent Pinchart wrote:
> On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
>> On 24/03/2023 09:11, Benjamin Gaignard wrote:
>>>
>>> Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
>>>> Hi Benjamin,
>>>>
>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>
>>>> url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
>>>> base:   git://linuxtv.org/media_tree.git master
>>>> patch link:    https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
>>>> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
>>>> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
>>>> compiler: aarch64-linux-gcc (GCC) 12.1.0
>>>>
>>>> If you fix the issue, kindly add following tag where applicable
>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>> | Reported-by: Dan Carpenter <error27@gmail.com>
>>>> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>>>>
>>>> smatch warnings:
>>>> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
>>>> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>>>>
>>>> vim +1272 include/media/videobuf2-core.h
>>>>
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1263  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1264  {
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1265      bool ret = false;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1266
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1267      spin_lock(&q->bufs_lock);
>>>>                                                          ^^^^^^^^^^^^^^^^^^^^^^^
>>>> Holding a spin lock.
>>>>
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1268
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1269      if (vb->index >= q->max_num_bufs) {
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1270          struct vb2_buffer **tmp;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1271
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272          tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>>>>                                                                                                                                       ^^^^^^^^^^
>>>> Sleeping allocation.  GFP_ATOMIC?  Or is there a way to move the
>>>> allocation outside the lock?
>>>
>>> I will add GFP_ATOMIC flag in next version.
>>
>> No need. Instead, don't use realloc here, just allocate a new array, copy over all
>> the data from the old, and then switch q->bufs with the spinlock held. Then you
>> can free the old one.
>>
>> It's only when you update q->bufs that you need the lock.
> 
> The copy also needs to be protected by the lock.

I suspect that that is not needed, since you shouldn't be able to add buffers here
since a mutex should be held at this time.

That said, it's something that Benjamin needs to analyze.

Regards,

	Hans

> 
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1273          if (!tmp)
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1274              goto realloc_failed;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1275
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1276          q->max_num_bufs *= 2;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1277          q->bufs = tmp;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1278      }
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1279
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1280      if (vb->index < q->max_num_bufs) {
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1281          q->bufs[vb->index] = vb;
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1282          ret = true;
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1283      }
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1284
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1285  realloc_failed:
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1286      spin_unlock(&q->bufs_lock);
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1287
>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1288      return ret;
>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1289  }
>
Benjamin Gaignard March 24, 2023, 8:56 a.m. UTC | #7 
Le 24/03/2023 à 09:52, Hans Verkuil a écrit :
> On 24/03/2023 09:48, Laurent Pinchart wrote:
>> On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
>>> On 24/03/2023 09:11, Benjamin Gaignard wrote:
>>>> Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
>>>>> Hi Benjamin,
>>>>>
>>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>>
>>>>> url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
>>>>> base:   git://linuxtv.org/media_tree.git master
>>>>> patch link:    https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
>>>>> patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
>>>>> config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
>>>>> compiler: aarch64-linux-gcc (GCC) 12.1.0
>>>>>
>>>>> If you fix the issue, kindly add following tag where applicable
>>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>>> | Reported-by: Dan Carpenter <error27@gmail.com>
>>>>> | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
>>>>>
>>>>> smatch warnings:
>>>>> include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
>>>>> drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
>>>>>
>>>>> vim +1272 include/media/videobuf2-core.h
>>>>>
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1263  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1264  {
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1265      bool ret = false;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1266
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1267      spin_lock(&q->bufs_lock);
>>>>>                                                           ^^^^^^^^^^^^^^^^^^^^^^^
>>>>> Holding a spin lock.
>>>>>
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1268
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1269      if (vb->index >= q->max_num_bufs) {
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1270          struct vb2_buffer **tmp;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1271
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272          tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
>>>>>                                                                                                                                        ^^^^^^^^^^
>>>>> Sleeping allocation.  GFP_ATOMIC?  Or is there a way to move the
>>>>> allocation outside the lock?
>>>> I will add GFP_ATOMIC flag in next version.
>>> No need. Instead, don't use realloc here, just allocate a new array, copy over all
>>> the data from the old, and then switch q->bufs with the spinlock held. Then you
>>> can free the old one.
>>>
>>> It's only when you update q->bufs that you need the lock.
>> The copy also needs to be protected by the lock.
> I suspect that that is not needed, since you shouldn't be able to add buffers here
> since a mutex should be held at this time.
>
> That said, it's something that Benjamin needs to analyze.

Does using GFP_ATOMIC is problematic ?

>
> Regards,
>
> 	Hans
>
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1273          if (!tmp)
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1274              goto realloc_failed;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1275
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1276          q->max_num_bufs *= 2;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1277          q->bufs = tmp;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1278      }
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1279
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1280      if (vb->index < q->max_num_bufs) {
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1281          q->bufs[vb->index] = vb;
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1282          ret = true;
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1283      }
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1284
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1285  realloc_failed:
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1286      spin_unlock(&q->bufs_lock);
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1287
>>>>> 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1288      return ret;
>>>>> 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1289  }
>
Dmitry Osipenko March 24, 2023, 1:02 p.m. UTC | #8 
On 3/21/23 13:28, Benjamin Gaignard wrote:
> +	q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
> +	if (!q->bufs)
> +		return -ENOMEM;
> +

Use kcalloc()
Tomasz Figa May 19, 2023, 10 a.m. UTC | #9 
On Fri, Mar 24, 2023 at 09:56:34AM +0100, Benjamin Gaignard wrote:
> 
> Le 24/03/2023 à 09:52, Hans Verkuil a écrit :
> > On 24/03/2023 09:48, Laurent Pinchart wrote:
> > > On Fri, Mar 24, 2023 at 09:31:35AM +0100, Hans Verkuil wrote:
> > > > On 24/03/2023 09:11, Benjamin Gaignard wrote:
> > > > > Le 24/03/2023 à 06:01, Dan Carpenter a écrit :
> > > > > > Hi Benjamin,
> > > > > > 
> > > > > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > > > > > 
> > > > > > url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
> > > > > > base:   git://linuxtv.org/media_tree.git master
> > > > > > patch link:    https://lore.kernel.org/r/20230321102855.346732-3-benjamin.gaignard%40collabora.com
> > > > > > patch subject: [PATCH v2 2/8] media: videobuf2: Make bufs array dynamic allocated
> > > > > > config: arm64-randconfig-m041-20230319 (https://download.01.org/0day-ci/archive/20230324/202303240148.lKRnUqW9-lkp@intel.com/config)
> > > > > > compiler: aarch64-linux-gcc (GCC) 12.1.0
> > > > > > 
> > > > > > If you fix the issue, kindly add following tag where applicable
> > > > > > | Reported-by: kernel test robot <lkp@intel.com>
> > > > > > | Reported-by: Dan Carpenter <error27@gmail.com>
> > > > > > | Link: https://lore.kernel.org/r/202303240148.lKRnUqW9-lkp@intel.com/
> > > > > > 
> > > > > > smatch warnings:
> > > > > > include/media/videobuf2-core.h:1272 vb2_queue_add_buffer() warn: sleeping in atomic context
> > > > > > drivers/media/common/videobuf2/videobuf2-core.c:2456 vb2_core_queue_init() warn: Please consider using kcalloc instead of kmalloc_array
> > > > > > 
> > > > > > vim +1272 include/media/videobuf2-core.h
> > > > > > 
> > > > > > 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1263  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> > > > > > 625d46c1c1fe8e Benjamin Gaignard 2023-03-21  1264  {
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1265      bool ret = false;
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1266
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1267      spin_lock(&q->bufs_lock);
> > > > > >                                                           ^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > Holding a spin lock.
> > > > > > 
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1268
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1269      if (vb->index >= q->max_num_bufs) {
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1270          struct vb2_buffer **tmp;
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21  1271
> > > > > > 487d3f14d12ecf Benjamin Gaignard 2023-03-21 @1272          tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> > > > > >                                                                                                                                        ^^^^^^^^^^
> > > > > > Sleeping allocation.  GFP_ATOMIC?  Or is there a way to move the
> > > > > > allocation outside the lock?
> > > > > I will add GFP_ATOMIC flag in next version.
> > > > No need. Instead, don't use realloc here, just allocate a new array, copy over all
> > > > the data from the old, and then switch q->bufs with the spinlock held. Then you
> > > > can free the old one.
> > > > 
> > > > It's only when you update q->bufs that you need the lock.
> > > The copy also needs to be protected by the lock.
> > I suspect that that is not needed, since you shouldn't be able to add buffers here
> > since a mutex should be held at this time.
> > 
> > That said, it's something that Benjamin needs to analyze.

I spent some time looking through the call sites of vb2_get_buffer() and
how those can be called and it turned out to be a massive list of code
paths, including a lot of calls originating from codec drivers calling
vb2_find_buffer() in random contexts (possibly even interrupt). So a
spinlock protecting the array pointer makes sense indeed.

> 
> Does using GFP_ATOMIC is problematic ?
> 

Yes, because the ability to reclaim memory is drastically limited and
the allocation is more likely to fail (as in: it's actually possible).
(And generally the time with interrupts disabled should be minimized to
keep system latency reasonable.)

Best regards,
Tomasz
Tomasz Figa May 19, 2023, 10:04 a.m. UTC | #10 
On Tue, Mar 21, 2023 at 08:16:10PM +0200, Laurent Pinchart wrote:
> Hi Benjamin,
> 
> Thank you for the patch.
> 
> On Tue, Mar 21, 2023 at 11:28:49AM +0100, Benjamin Gaignard wrote:
> > Instead of a static array change bufs to a dynamically allocated array.
> > This will allow to store more video buffer if needed.
> > Protect the array with a spinlock.
> 
> I think I asked in the review of v1 why we couldn't use the kernel
> IDA/IDR APIs to allocate buffer IDs and register buffers, but I don't
> think I've received a reply. Wouldn't it be better than rolling out our
> own mechanism ?
> 

+1, with a note that [1] suggests that IDR is deprecated and XArray[2]
should be used instead.

[1] https://docs.kernel.org/core-api/idr.html
[2] https://docs.kernel.org/core-api/xarray.html

Also from my quick look, XArray may be solving the locking concerns for us
automatically.

Best regards,
Tomasz

> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   |  8 +++
> >  include/media/videobuf2-core.h                | 49 ++++++++++++++++---
> >  2 files changed, 49 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 79e90e338846..ae9d72f4d181 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -2452,6 +2452,13 @@ int vb2_core_queue_init(struct vb2_queue *q)
> >  	mutex_init(&q->mmap_lock);
> >  	init_waitqueue_head(&q->done_wq);
> >  
> > +	q->max_num_bufs = 32;
> > +	q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
> > +	if (!q->bufs)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_init(&q->bufs_lock);
> > +
> >  	q->memory = VB2_MEMORY_UNKNOWN;
> >  
> >  	if (q->buf_struct_size == 0)
> > @@ -2479,6 +2486,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
> >  	mutex_lock(&q->mmap_lock);
> >  	__vb2_queue_free(q, q->num_buffers);
> >  	mutex_unlock(&q->mmap_lock);
> > +	kfree(q->bufs);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_core_queue_release);
> >  
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 5b1e3d801546..397dbf6e61e1 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -558,6 +558,8 @@ struct vb2_buf_ops {
> >   * @dma_dir:	DMA mapping direction.
> >   * @bufs:	videobuf2 buffer structures
> >   * @num_buffers: number of allocated/used buffers
> > + * @bufs_lock: lock to protect bufs access
> > + * @max_num_bufs: max number of buffers storable in bufs
> >   * @queued_list: list of buffers currently queued from userspace
> >   * @queued_count: number of buffers queued and ready for streaming.
> >   * @owned_by_drv_count: number of buffers owned by the driver
> > @@ -619,8 +621,10 @@ struct vb2_queue {
> >  	struct mutex			mmap_lock;
> >  	unsigned int			memory;
> >  	enum dma_data_direction		dma_dir;
> > -	struct vb2_buffer		*bufs[VB2_MAX_FRAME];
> > +	struct vb2_buffer		**bufs;
> >  	unsigned int			num_buffers;
> > +	spinlock_t			bufs_lock;
> > +	size_t				max_num_bufs;
> >  
> >  	struct list_head		queued_list;
> >  	unsigned int			queued_count;
> > @@ -1239,9 +1243,16 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> >  static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> >  						unsigned int index)
> >  {
> > -	if (index < q->num_buffers)
> > -		return q->bufs[index];
> > -	return NULL;
> > +	struct vb2_buffer *vb = NULL;
> > +
> > +	spin_lock(&q->bufs_lock);
> > +
> > +	if (index < q->max_num_bufs)
> > +		vb = q->bufs[index];
> > +
> > +	spin_unlock(&q->bufs_lock);
> > +
> > +	return vb;
> >  }
> >  
> >  /**
> > @@ -1251,12 +1262,30 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> >   */
> >  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> >  {
> > -	if (vb->index < VB2_MAX_FRAME) {
> > +	bool ret = false;
> > +
> > +	spin_lock(&q->bufs_lock);
> > +
> > +	if (vb->index >= q->max_num_bufs) {
> > +		struct vb2_buffer **tmp;
> > +
> > +		tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
> > +		if (!tmp)
> > +			goto realloc_failed;
> > +
> > +		q->max_num_bufs *= 2;
> > +		q->bufs = tmp;
> > +	}
> > +
> > +	if (vb->index < q->max_num_bufs) {
> >  		q->bufs[vb->index] = vb;
> > -		return true;
> > +		ret = true;
> >  	}
> >  
> > -	return false;
> > +realloc_failed:
> > +	spin_unlock(&q->bufs_lock);
> > +
> > +	return ret;
> >  }
> >  
> >  /**
> > @@ -1266,8 +1295,12 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
> >   */
> >  static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
> >  {
> > -	if (vb->index < VB2_MAX_FRAME)
> > +	spin_lock(&q->bufs_lock);
> > +
> > +	if (vb->index < q->max_num_bufs)
> >  		q->bufs[vb->index] = NULL;
> > +
> > +	spin_unlock(&q->bufs_lock);
> >  }
> >  
> >  /*
> 
> -- 
> Regards,
> 
> Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 79e90e338846..ae9d72f4d181 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2452,6 +2452,13 @@  int vb2_core_queue_init(struct vb2_queue *q)
 	mutex_init(&q->mmap_lock);
 	init_waitqueue_head(&q->done_wq);
 
+	q->max_num_bufs = 32;
+	q->bufs = kmalloc_array(q->max_num_bufs, sizeof(*q->bufs), GFP_KERNEL | __GFP_ZERO);
+	if (!q->bufs)
+		return -ENOMEM;
+
+	spin_lock_init(&q->bufs_lock);
+
 	q->memory = VB2_MEMORY_UNKNOWN;
 
 	if (q->buf_struct_size == 0)
@@ -2479,6 +2486,7 @@  void vb2_core_queue_release(struct vb2_queue *q)
 	mutex_lock(&q->mmap_lock);
 	__vb2_queue_free(q, q->num_buffers);
 	mutex_unlock(&q->mmap_lock);
+	kfree(q->bufs);
 }
 EXPORT_SYMBOL_GPL(vb2_core_queue_release);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5b1e3d801546..397dbf6e61e1 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -558,6 +558,8 @@  struct vb2_buf_ops {
  * @dma_dir:	DMA mapping direction.
  * @bufs:	videobuf2 buffer structures
  * @num_buffers: number of allocated/used buffers
+ * @bufs_lock: lock to protect bufs access
+ * @max_num_bufs: max number of buffers storable in bufs
  * @queued_list: list of buffers currently queued from userspace
  * @queued_count: number of buffers queued and ready for streaming.
  * @owned_by_drv_count: number of buffers owned by the driver
@@ -619,8 +621,10 @@  struct vb2_queue {
 	struct mutex			mmap_lock;
 	unsigned int			memory;
 	enum dma_data_direction		dma_dir;
-	struct vb2_buffer		*bufs[VB2_MAX_FRAME];
+	struct vb2_buffer		**bufs;
 	unsigned int			num_buffers;
+	spinlock_t			bufs_lock;
+	size_t				max_num_bufs;
 
 	struct list_head		queued_list;
 	unsigned int			queued_count;
@@ -1239,9 +1243,16 @@  static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
 static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
 						unsigned int index)
 {
-	if (index < q->num_buffers)
-		return q->bufs[index];
-	return NULL;
+	struct vb2_buffer *vb = NULL;
+
+	spin_lock(&q->bufs_lock);
+
+	if (index < q->max_num_bufs)
+		vb = q->bufs[index];
+
+	spin_unlock(&q->bufs_lock);
+
+	return vb;
 }
 
 /**
@@ -1251,12 +1262,30 @@  static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
  */
 static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
 {
-	if (vb->index < VB2_MAX_FRAME) {
+	bool ret = false;
+
+	spin_lock(&q->bufs_lock);
+
+	if (vb->index >= q->max_num_bufs) {
+		struct vb2_buffer **tmp;
+
+		tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL);
+		if (!tmp)
+			goto realloc_failed;
+
+		q->max_num_bufs *= 2;
+		q->bufs = tmp;
+	}
+
+	if (vb->index < q->max_num_bufs) {
 		q->bufs[vb->index] = vb;
-		return true;
+		ret = true;
 	}
 
-	return false;
+realloc_failed:
+	spin_unlock(&q->bufs_lock);
+
+	return ret;
 }
 
 /**
@@ -1266,8 +1295,12 @@  static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *
  */
 static inline void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
 {
-	if (vb->index < VB2_MAX_FRAME)
+	spin_lock(&q->bufs_lock);
+
+	if (vb->index < q->max_num_bufs)
 		q->bufs[vb->index] = NULL;
+
+	spin_unlock(&q->bufs_lock);
 }
 
 /*