Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide
how many buffers could be stored in a queue.
This request 'bufs' array to be allocated at queue init time and freed
when releasing the queue.
By default VB2_MAX_FRAME remains the limit.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------
include/media/videobuf2-core.h | 4 ++-
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 15b583ce0c69..dc7f6b59d237 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
*/
static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
{
- if (index < VB2_MAX_FRAME && !q->bufs[index]) {
+ if (index < q->max_allowed_buffers && !q->bufs[index]) {
q->bufs[index] = vb;
vb->index = index;
vb->vb2_queue = q;
@@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
*/
static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb)
{
- if (vb->index < VB2_MAX_FRAME) {
+ if (vb->index < q->max_allowed_buffers) {
q->bufs[vb->index] = NULL;
vb->vb2_queue = NULL;
}
@@ -449,9 +449,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
struct vb2_buffer *vb;
int ret;
- /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */
+ /* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */
num_buffers = min_t(unsigned int, num_buffers,
- VB2_MAX_FRAME - q->num_buffers);
+ q->max_allowed_buffers - q->num_buffers);
for (buffer = 0; buffer < num_buffers; ++buffer) {
/* Allocate vb2 buffer structures */
@@ -862,9 +862,9 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
/*
* Make sure the requested values and current defaults are sane.
*/
- WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
+ WARN_ON(q->min_buffers_needed > q->max_allowed_buffers);
num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
- num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
+ num_buffers = min_t(unsigned int, num_buffers, q->max_allowed_buffers);
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
/*
* Set this now to ensure that drivers see the correct q->memory value
@@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
bool no_previous_buffers = !q->num_buffers;
int ret;
- if (q->num_buffers == VB2_MAX_FRAME) {
+ if (q->num_buffers == q->max_allowed_buffers) {
dprintk(q, 1, "maximum number of buffers already allocated\n");
return -ENOBUFS;
}
@@ -1009,7 +1009,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
return -EINVAL;
}
- num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
+ num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers);
if (requested_planes && requested_sizes) {
num_planes = requested_planes;
@@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q)
q->memory = VB2_MEMORY_UNKNOWN;
+ if (!q->max_allowed_buffers)
+ q->max_allowed_buffers = VB2_MAX_FRAME;
+
+ /* The maximum is limited by offset cookie encoding pattern */
+ q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK);
+
+ q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL);
+
if (q->buf_struct_size == 0)
q->buf_struct_size = sizeof(struct vb2_buffer);
@@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q)
__vb2_queue_cancel(q);
mutex_lock(&q->mmap_lock);
__vb2_queue_free(q, q->num_buffers);
+ kfree(q->bufs);
mutex_unlock(&q->mmap_lock);
}
EXPORT_SYMBOL_GPL(vb2_core_queue_release);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index cd3ff1cd759d..97153c69583f 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -558,6 +558,7 @@ struct vb2_buf_ops {
* @dma_dir: DMA mapping direction.
* @bufs: videobuf2 buffer structures
* @num_buffers: number of allocated/used buffers
+ * @max_allowed_buffers: upper limit of number of allocated/used buffers
* @queued_list: list of buffers currently queued from userspace
* @queued_count: number of buffers queued and ready for streaming.
* @owned_by_drv_count: number of buffers owned by the driver
@@ -619,8 +620,9 @@ 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;
+ unsigned int max_allowed_buffers;
struct list_head queued_list;
unsigned int queued_count;
--
2.39.2
On 01/09/2023 14:44, Benjamin Gaignard wrote: > Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide > how many buffers could be stored in a queue. > This request 'bufs' array to be allocated at queue init time and freed > when releasing the queue. > By default VB2_MAX_FRAME remains the limit. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > .../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------ > include/media/videobuf2-core.h | 4 ++- > 2 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 15b583ce0c69..dc7f6b59d237 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) > */ > static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) > { > - if (index < VB2_MAX_FRAME && !q->bufs[index]) { > + if (index < q->max_allowed_buffers && !q->bufs[index]) { > q->bufs[index] = vb; > vb->index = index; > vb->vb2_queue = q; > @@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns > */ > static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb) > { > - if (vb->index < VB2_MAX_FRAME) { > + if (vb->index < q->max_allowed_buffers) { > q->bufs[vb->index] = NULL; > vb->vb2_queue = NULL; > } > @@ -449,9 +449,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > struct vb2_buffer *vb; > int ret; > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > + /* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */ > num_buffers = min_t(unsigned int, num_buffers, > - VB2_MAX_FRAME - q->num_buffers); > + q->max_allowed_buffers - q->num_buffers); > > for (buffer = 0; buffer < num_buffers; ++buffer) { > /* Allocate vb2 buffer structures */ > @@ -862,9 +862,9 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > + WARN_ON(q->min_buffers_needed > q->max_allowed_buffers); > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > + num_buffers = min_t(unsigned int, num_buffers, q->max_allowed_buffers); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > * Set this now to ensure that drivers see the correct q->memory value > @@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > bool no_previous_buffers = !q->num_buffers; > int ret; > > - if (q->num_buffers == VB2_MAX_FRAME) { > + if (q->num_buffers == q->max_allowed_buffers) { > dprintk(q, 1, "maximum number of buffers already allocated\n"); > return -ENOBUFS; > } > @@ -1009,7 +1009,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > return -EINVAL; > } > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > + num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers); > > if (requested_planes && requested_sizes) { > num_planes = requested_planes; > @@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q) > > q->memory = VB2_MEMORY_UNKNOWN; > > + if (!q->max_allowed_buffers) > + q->max_allowed_buffers = VB2_MAX_FRAME; > + > + /* The maximum is limited by offset cookie encoding pattern */ > + q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK); I think this should be 'BUFFER_INDEX_MASK + 1'. > + > + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); > + > if (q->buf_struct_size == 0) > q->buf_struct_size = sizeof(struct vb2_buffer); > > @@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q) > __vb2_queue_cancel(q); > mutex_lock(&q->mmap_lock); > __vb2_queue_free(q, q->num_buffers); > + kfree(q->bufs); > mutex_unlock(&q->mmap_lock); > } > EXPORT_SYMBOL_GPL(vb2_core_queue_release); > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index cd3ff1cd759d..97153c69583f 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -558,6 +558,7 @@ struct vb2_buf_ops { > * @dma_dir: DMA mapping direction. > * @bufs: videobuf2 buffer structures > * @num_buffers: number of allocated/used buffers > + * @max_allowed_buffers: upper limit of number of allocated/used buffers > * @queued_list: list of buffers currently queued from userspace > * @queued_count: number of buffers queued and ready for streaming. > * @owned_by_drv_count: number of buffers owned by the driver > @@ -619,8 +620,9 @@ 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; > + unsigned int max_allowed_buffers; > > struct list_head queued_list; > unsigned int queued_count; Regards, Hans
Hi Benjamin, On 01/09/2023 14:44, Benjamin Gaignard wrote: > Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide > how many buffers could be stored in a queue. > This request 'bufs' array to be allocated at queue init time and freed > when releasing the queue. > By default VB2_MAX_FRAME remains the limit. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > .../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------ > include/media/videobuf2-core.h | 4 ++- > 2 files changed, 20 insertions(+), 9 deletions(-) > This patch breaks v4l2-compliance. I see lots of issues when running the test-media script in v4l-utils, contrib/test, among them memory leaks and use-after-free. I actually tested using virtme with the build scripts, but that in turn calls the test-media script in a qemu environment, and it is at the moment a bit tricky to set up, unless you run a debian 12 distro. I will email the test logs directly to you since they are a bit large (>5000 lines). Regards, Hans > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 15b583ce0c69..dc7f6b59d237 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) > */ > static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) > { > - if (index < VB2_MAX_FRAME && !q->bufs[index]) { > + if (index < q->max_allowed_buffers && !q->bufs[index]) { > q->bufs[index] = vb; > vb->index = index; > vb->vb2_queue = q; > @@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns > */ > static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb) > { > - if (vb->index < VB2_MAX_FRAME) { > + if (vb->index < q->max_allowed_buffers) { > q->bufs[vb->index] = NULL; > vb->vb2_queue = NULL; > } > @@ -449,9 +449,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > struct vb2_buffer *vb; > int ret; > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > + /* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */ > num_buffers = min_t(unsigned int, num_buffers, > - VB2_MAX_FRAME - q->num_buffers); > + q->max_allowed_buffers - q->num_buffers); > > for (buffer = 0; buffer < num_buffers; ++buffer) { > /* Allocate vb2 buffer structures */ > @@ -862,9 +862,9 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > + WARN_ON(q->min_buffers_needed > q->max_allowed_buffers); > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > + num_buffers = min_t(unsigned int, num_buffers, q->max_allowed_buffers); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > * Set this now to ensure that drivers see the correct q->memory value > @@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > bool no_previous_buffers = !q->num_buffers; > int ret; > > - if (q->num_buffers == VB2_MAX_FRAME) { > + if (q->num_buffers == q->max_allowed_buffers) { > dprintk(q, 1, "maximum number of buffers already allocated\n"); > return -ENOBUFS; > } > @@ -1009,7 +1009,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > return -EINVAL; > } > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > + num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers); > > if (requested_planes && requested_sizes) { > num_planes = requested_planes; > @@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q) > > q->memory = VB2_MEMORY_UNKNOWN; > > + if (!q->max_allowed_buffers) > + q->max_allowed_buffers = VB2_MAX_FRAME; > + > + /* The maximum is limited by offset cookie encoding pattern */ > + q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK); > + > + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); > + > if (q->buf_struct_size == 0) > q->buf_struct_size = sizeof(struct vb2_buffer); > > @@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q) > __vb2_queue_cancel(q); > mutex_lock(&q->mmap_lock); > __vb2_queue_free(q, q->num_buffers); > + kfree(q->bufs); > mutex_unlock(&q->mmap_lock); > } > EXPORT_SYMBOL_GPL(vb2_core_queue_release); > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index cd3ff1cd759d..97153c69583f 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -558,6 +558,7 @@ struct vb2_buf_ops { > * @dma_dir: DMA mapping direction. > * @bufs: videobuf2 buffer structures > * @num_buffers: number of allocated/used buffers > + * @max_allowed_buffers: upper limit of number of allocated/used buffers > * @queued_list: list of buffers currently queued from userspace > * @queued_count: number of buffers queued and ready for streaming. > * @owned_by_drv_count: number of buffers owned by the driver > @@ -619,8 +620,9 @@ 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; > + unsigned int max_allowed_buffers; > > struct list_head queued_list; > unsigned int queued_count;
Le 04/09/2023 à 13:24, Hans Verkuil a écrit : > Hi Benjamin, > > On 01/09/2023 14:44, Benjamin Gaignard wrote: >> Add 'max_allowed_buffers' field in vb2_queue struct to let drivers decide >> how many buffers could be stored in a queue. >> This request 'bufs' array to be allocated at queue init time and freed >> when releasing the queue. >> By default VB2_MAX_FRAME remains the limit. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> .../media/common/videobuf2/videobuf2-core.c | 25 +++++++++++++------ >> include/media/videobuf2-core.h | 4 ++- >> 2 files changed, 20 insertions(+), 9 deletions(-) >> > This patch breaks v4l2-compliance. I see lots of issues when running the > test-media script in v4l-utils, contrib/test, among them memory leaks > and use-after-free. > > I actually tested using virtme with the build scripts, but that in turn > calls the test-media script in a qemu environment, and it is at the moment > a bit tricky to set up, unless you run a debian 12 distro. > > I will email the test logs directly to you since they are a bit large (>5000 lines). I will try to reproduce this error on my side to fix it. Regards, Benjamin > > Regards, > > Hans > > > >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >> index 15b583ce0c69..dc7f6b59d237 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-core.c >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >> @@ -411,7 +411,7 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb) >> */ >> static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index) >> { >> - if (index < VB2_MAX_FRAME && !q->bufs[index]) { >> + if (index < q->max_allowed_buffers && !q->bufs[index]) { >> q->bufs[index] = vb; >> vb->index = index; >> vb->vb2_queue = q; >> @@ -428,7 +428,7 @@ static bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns >> */ >> static void vb2_queue_remove_buffer(struct vb2_queue *q, struct vb2_buffer *vb) >> { >> - if (vb->index < VB2_MAX_FRAME) { >> + if (vb->index < q->max_allowed_buffers) { >> q->bufs[vb->index] = NULL; >> vb->vb2_queue = NULL; >> } >> @@ -449,9 +449,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, >> struct vb2_buffer *vb; >> int ret; >> >> - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ >> + /* Ensure that q->num_buffers+num_buffers is below q->max_allowed_buffers */ >> num_buffers = min_t(unsigned int, num_buffers, >> - VB2_MAX_FRAME - q->num_buffers); >> + q->max_allowed_buffers - q->num_buffers); >> >> for (buffer = 0; buffer < num_buffers; ++buffer) { >> /* Allocate vb2 buffer structures */ >> @@ -862,9 +862,9 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, >> /* >> * Make sure the requested values and current defaults are sane. >> */ >> - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); >> + WARN_ON(q->min_buffers_needed > q->max_allowed_buffers); >> num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); >> - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); >> + num_buffers = min_t(unsigned int, num_buffers, q->max_allowed_buffers); >> memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); >> /* >> * Set this now to ensure that drivers see the correct q->memory value >> @@ -980,7 +980,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> bool no_previous_buffers = !q->num_buffers; >> int ret; >> >> - if (q->num_buffers == VB2_MAX_FRAME) { >> + if (q->num_buffers == q->max_allowed_buffers) { >> dprintk(q, 1, "maximum number of buffers already allocated\n"); >> return -ENOBUFS; >> } >> @@ -1009,7 +1009,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, >> return -EINVAL; >> } >> >> - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); >> + num_buffers = min(*count, q->max_allowed_buffers - q->num_buffers); >> >> if (requested_planes && requested_sizes) { >> num_planes = requested_planes; >> @@ -2519,6 +2519,14 @@ int vb2_core_queue_init(struct vb2_queue *q) >> >> q->memory = VB2_MEMORY_UNKNOWN; >> >> + if (!q->max_allowed_buffers) >> + q->max_allowed_buffers = VB2_MAX_FRAME; >> + >> + /* The maximum is limited by offset cookie encoding pattern */ >> + q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK); >> + >> + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); >> + >> if (q->buf_struct_size == 0) >> q->buf_struct_size = sizeof(struct vb2_buffer); >> >> @@ -2543,6 +2551,7 @@ void vb2_core_queue_release(struct vb2_queue *q) >> __vb2_queue_cancel(q); >> mutex_lock(&q->mmap_lock); >> __vb2_queue_free(q, q->num_buffers); >> + kfree(q->bufs); >> mutex_unlock(&q->mmap_lock); >> } >> EXPORT_SYMBOL_GPL(vb2_core_queue_release); >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index cd3ff1cd759d..97153c69583f 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -558,6 +558,7 @@ struct vb2_buf_ops { >> * @dma_dir: DMA mapping direction. >> * @bufs: videobuf2 buffer structures >> * @num_buffers: number of allocated/used buffers >> + * @max_allowed_buffers: upper limit of number of allocated/used buffers >> * @queued_list: list of buffers currently queued from userspace >> * @queued_count: number of buffers queued and ready for streaming. >> * @owned_by_drv_count: number of buffers owned by the driver >> @@ -619,8 +620,9 @@ 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; >> + unsigned int max_allowed_buffers; >> >> struct list_head queued_list; >> unsigned int queued_count; >
Hi Benjamin, This patch can be folded into 11/18 to make it work properly. vb2_core_queue_release() is also called when the file handle of the video device is closed and it is also the owner of the currently allocated buffers. This will free q->bufs, but queue_init isn't called a second time for non-m2m devices. So move the allocation of q->bufs from queue_init to reqbufs/create_buffers. And when releasing the file handle we also check if there is no owner at all: in that case vb2_queue_release() must still be called to free q->bufs. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/common/videobuf2/videobuf2-core.c | 15 +++++++++++++-- drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index dc7f6b59d237..202d7c80ffd2 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -859,6 +859,12 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, return 0; } + if (!q->bufs) { + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); + if (!q->bufs) + return -ENOMEM; + } + /* * Make sure the requested values and current defaults are sane. */ @@ -985,6 +991,12 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, return -ENOBUFS; } + if (!q->bufs) { + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); + if (!q->bufs) + return -ENOMEM; + } + if (no_previous_buffers) { if (q->waiting_in_dqbuf && *count) { dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); @@ -2525,8 +2537,6 @@ int vb2_core_queue_init(struct vb2_queue *q) /* The maximum is limited by offset cookie encoding pattern */ q->max_allowed_buffers = min_t(unsigned int, q->max_allowed_buffers, BUFFER_INDEX_MASK); - q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); - if (q->buf_struct_size == 0) q->buf_struct_size = sizeof(struct vb2_buffer); @@ -2552,6 +2562,7 @@ void vb2_core_queue_release(struct vb2_queue *q) mutex_lock(&q->mmap_lock); __vb2_queue_free(q, q->num_buffers); kfree(q->bufs); + q->bufs = NULL; mutex_unlock(&q->mmap_lock); } EXPORT_SYMBOL_GPL(vb2_core_queue_release); diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index 8ba658ad9891..104fc5c4f574 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -1148,7 +1148,7 @@ int _vb2_fop_release(struct file *file, struct mutex *lock) if (lock) mutex_lock(lock); - if (file->private_data == vdev->queue->owner) { + if (!vdev->queue->owner || file->private_data == vdev->queue->owner) { vb2_queue_release(vdev->queue); vdev->queue->owner = NULL; } @@ -1276,7 +1276,7 @@ void vb2_video_unregister_device(struct video_device *vdev) */ get_device(&vdev->dev); video_unregister_device(vdev); - if (vdev->queue && vdev->queue->owner) { + if (vdev->queue) { struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; -- 2.40.1
A follow-up to my follow-up... I realized that it is wise to do the allocation with mmap_lock held, since that is also held when freeing q->bufs. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- .../media/common/videobuf2/videobuf2-core.c | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index fe15e583b52a..dd25937c6dc8 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -818,7 +818,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, unsigned plane_sizes[VB2_MAX_PLANES] = { }; bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT; unsigned int i; - int ret; + int ret = 0; if (q->streaming) { dprintk(q, 1, "streaming active\n"); @@ -859,12 +859,6 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, return 0; } - if (!q->bufs) { - q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); - if (!q->bufs) - return -ENOMEM; - } - /* * Make sure the requested values and current defaults are sane. */ @@ -877,8 +871,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, * in the queue_setup op. */ mutex_lock(&q->mmap_lock); + if (!q->bufs) + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); + if (!q->bufs) + ret = -ENOMEM; q->memory = memory; mutex_unlock(&q->mmap_lock); + if (ret) + return ret; set_queue_coherency(q, non_coherent_mem); /* @@ -984,19 +984,13 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, unsigned plane_sizes[VB2_MAX_PLANES] = { }; bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT; bool no_previous_buffers = !q->num_buffers; - int ret; + int ret = 0; if (q->num_buffers == q->max_allowed_buffers) { dprintk(q, 1, "maximum number of buffers already allocated\n"); return -ENOBUFS; } - if (!q->bufs) { - q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); - if (!q->bufs) - return -ENOMEM; - } - if (no_previous_buffers) { if (q->waiting_in_dqbuf && *count) { dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); @@ -1009,7 +1003,13 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, */ mutex_lock(&q->mmap_lock); q->memory = memory; + if (!q->bufs) + q->bufs = kcalloc(q->max_allowed_buffers, sizeof(*q->bufs), GFP_KERNEL); + if (!q->bufs) + ret = -ENOMEM; mutex_unlock(&q->mmap_lock); + if (ret) + return ret; q->waiting_for_buffers = !q->is_output; set_queue_coherency(q, non_coherent_mem); } else { -- 2.40.1
© 2016 - 2024 Red Hat, Inc.