| Message ID | 20230321102855.346732-3-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
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); > } > > /*
Le 21/03/2023 à 19:16, Laurent Pinchart a écrit : > 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 ? I wrote it the cover letter, it is because IDA/IDR APIs are marked as deprecated in kernel documentation. https://docs.kernel.org/core-api/idr.html?highlight=idr and Xarray looks to be for large array. Regards, Benjamin > >> 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); >> } >> >> /*
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 }
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 } >
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 } >>
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 }
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 } >
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 } >
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()
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
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
Le 19/05/2023 à 12:04, Tomasz Figa a écrit : > 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. From documentation Xarray seems to useful for "very large array of pointers" which is, I think, not our case. Regards, Benjamin > > 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 --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); } /*
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(-)