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@xxxxxxxxxxxxx> > > --- > > .../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
