| Message ID | 20230321102855.346732-1-benjamin.gaignard@collabora.com |
|---|---|
| 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); > } > > /*
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 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
VP9 dynamic resolution change use case requires to change resolution without doing stream off/on to keep references frames. In consequence the numbers of buffers increase until all 'old' reference frames are deprecated. To make it possible this series remove the 32 buffers limit per queue and introduce DELETE_BUF ioctl to delete buffers from a queue without doing stream off/on sequence. change in version 2: - Use a dynamic array and not a list to keep trace of allocated buffers. Not use IDR interface because it is marked as deprecated in kernel documentation. - Add a module parameter to limit the number of buffer per queue. - Add DELETE_BUF ioctl and m2m helpers. Benjamin Gaignard (8): media: videobuf2: Access vb2_queue bufs array through helper functions media: videobuf2: Make bufs array dynamic allocated media: videobuf2: Add a module param to limit vb2 queue buffer storage media: videobuf2: Stop define VB2_MAX_FRAME as global media: v4l2: Add DELETE_BUF ioctl media: v4l2: Add mem2mem helpers for DELETE_BUF ioctl media: vim2m: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl media: verisilicon: Use v4l2-mem2mem helpers for VIDIOC_DELETE_BUF ioctl .../userspace-api/media/v4l/user-func.rst | 1 + .../media/v4l/vidioc-delete-buf.rst | 51 ++++++ .../media/common/videobuf2/videobuf2-core.c | 168 +++++++++++++----- .../media/common/videobuf2/videobuf2-v4l2.c | 23 ++- drivers/media/platform/amphion/vdec.c | 1 + drivers/media/platform/amphion/vpu_dbg.c | 4 +- .../platform/mediatek/jpeg/mtk_jpeg_core.c | 2 +- .../vcodec/vdec/vdec_vp9_req_lat_if.c | 4 +- drivers/media/platform/qcom/venus/hfi.h | 2 + .../media/platform/verisilicon/hantro_hw.h | 2 + .../media/platform/verisilicon/hantro_v4l2.c | 1 + drivers/media/test-drivers/vim2m.c | 1 + drivers/media/test-drivers/visl/visl-dec.c | 16 +- drivers/media/v4l2-core/v4l2-dev.c | 1 + drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++ drivers/media/v4l2-core/v4l2-mem2mem.c | 20 +++ .../staging/media/atomisp/pci/atomisp_ioctl.c | 2 +- drivers/staging/media/ipu3/ipu3-v4l2.c | 2 + include/media/v4l2-ioctl.h | 4 + include/media/v4l2-mem2mem.h | 12 ++ include/media/videobuf2-core.h | 84 ++++++++- include/media/videobuf2-v4l2.h | 15 +- include/uapi/linux/videodev2.h | 1 + 23 files changed, 353 insertions(+), 74 deletions(-) create mode 100644 Documentation/userspace-api/media/v4l/vidioc-delete-buf.rst