| Message ID | 20191217032034.54897-7-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement V4L2_BUF_FLAG_NO_CACHE_* flags | expand |
On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > This patch lets user-space to request a non-consistent memory > allocation during CREATE_BUFS ioctl call. struct v4l2_create_buffers > has seven 4-byte reserved areas, so reserved[0] is renamed to ->flags. > The struct, thus, now has six reserved 4-byte regions. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/uapi/v4l/vidioc-create-bufs.rst | 8 +++++- > .../media/common/videobuf2/videobuf2-core.c | 27 +++++++++++++++---- > .../media/common/videobuf2/videobuf2-v4l2.c | 7 ++++- > drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- > include/media/videobuf2-core.h | 4 ++- > include/uapi/linux/videodev2.h | 3 ++- > 6 files changed, 41 insertions(+), 10 deletions(-) > > diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst > index bd08e4f77ae4..c56e80659b4a 100644 > --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst > +++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst > @@ -121,7 +121,13 @@ than the number requested. > other changes, then set ``count`` to 0, ``memory`` to > ``V4L2_MEMORY_MMAP`` and ``format.type`` to the buffer type. > * - __u32 > - - ``reserved``\ [7] > + - ``flags`` > + - Specifies additional buffer management attributes. E.g. when > + ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated > + in non-consistent memory. Same comment as for patch 05/15. > + > + * - __u32 > + - ``reserved``\ [6] > - A place holder for future extensions. Drivers and applications > must set the array to zero. > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 668c56df13f6..d1012a24755d 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -812,9 +812,21 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > } > EXPORT_SYMBOL_GPL(vb2_core_reqbufs); > > +static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem) > +{ > + bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT; > + > + if (consistent_mem != queue_attr) { > + dprintk(1, "memory consistency model mismatch\n"); > + return false; > + } > + return true; > +} > + This belongs in patch 04/15. The commit log for that patch makes a lot more sense if this code is moved there. > int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > - unsigned int *count, unsigned requested_planes, > - const unsigned requested_sizes[]) > + bool consistent_mem, unsigned int *count, > + unsigned requested_planes, > + const unsigned requested_sizes[]) > { > unsigned int num_planes = 0, num_buffers, allocated_buffers; > unsigned plane_sizes[VB2_MAX_PLANES] = { }; > @@ -832,10 +844,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > } > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > q->memory = memory; > + __set_queue_consistency(q, consistent_mem); > q->waiting_for_buffers = !q->is_output; > - } else if (q->memory != memory) { > - dprintk(1, "memory model mismatch\n"); > - return -EINVAL; > + } else { > + if (q->memory != memory) { > + dprintk(1, "memory model mismatch\n"); > + return -EINVAL; > + } > + if (!verify_consistency_attr(q, consistent_mem)) > + return -EINVAL; > } Ditto. > > num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index 0eabb589684f..48d123a1ac2a 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -730,6 +730,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) > unsigned requested_sizes[VIDEO_MAX_PLANES]; > struct v4l2_format *f = &create->format; > int ret = vb2_verify_memory_type(q, create->memory, f->type); > + bool consistent = true; > unsigned i; > > fill_buf_caps(q, &create->capabilities); > @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) > for (i = 0; i < requested_planes; i++) > if (requested_sizes[i] == 0) > return -EINVAL; > - return ret ? ret : vb2_core_create_bufs(q, create->memory, > + > + if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT) > + consistent = false; > + > + return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent, > &create->count, requested_planes, requested_sizes); As mentioned before: we need a V4L2_BUF_CAP capability. > } > EXPORT_SYMBOL_GPL(vb2_create_bufs); > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 225d06819bce..793cb6534de4 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -2012,7 +2012,7 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops, > if (ret) > return ret; > > - CLEAR_AFTER_FIELD(create, capabilities); > + CLEAR_AFTER_FIELD(create, flags); > > v4l_sanitize_format(&create->format); > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 810af5cf5742..5e5450bdabbd 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -757,6 +757,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > * vb2_core_create_bufs() - Allocate buffers and any required auxiliary structs > * @q: pointer to &struct vb2_queue with videobuf2 queue. > * @memory: memory type, as defined by &enum vb2_memory. > + * @consistent_mem: memory consistency model. > * @count: requested buffer count. > * @requested_planes: number of planes requested. > * @requested_sizes: array with the size of the planes. > @@ -774,7 +775,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > * Return: returns zero on success; an error code otherwise. > */ > int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > - unsigned int *count, unsigned int requested_planes, > + bool consistent_mem, unsigned int *count, > + unsigned int requested_planes, > const unsigned int requested_sizes[]); > > /** > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 73a4854f71bd..82e2ded5a136 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -2419,7 +2419,8 @@ struct v4l2_create_buffers { > __u32 memory; > struct v4l2_format format; > __u32 capabilities; > - __u32 reserved[7]; > + __u32 flags; > + __u32 reserved[6]; > }; > > /* >
On (20/01/10 10:59), Hans Verkuil wrote: [..] > > > > num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > > index 0eabb589684f..48d123a1ac2a 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > > @@ -730,6 +730,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) > > unsigned requested_sizes[VIDEO_MAX_PLANES]; > > struct v4l2_format *f = &create->format; > > int ret = vb2_verify_memory_type(q, create->memory, f->type); > > + bool consistent = true; > > unsigned i; > > > > fill_buf_caps(q, &create->capabilities); > > @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) > > for (i = 0; i < requested_planes; i++) > > if (requested_sizes[i] == 0) > > return -EINVAL; > > - return ret ? ret : vb2_core_create_bufs(q, create->memory, > > + > > + if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT) > > + consistent = false; > > + > > + return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent, > > &create->count, requested_planes, requested_sizes); > > As mentioned before: we need a V4L2_BUF_CAP capability. I can add V4L2_BUF_CAP for memory consistency. Isn't it just q->memory property though? User space may request MMAP consistent memory or MMAP inconsistent memory. -ss
On 1/23/20 4:41 AM, Sergey Senozhatsky wrote: > On (20/01/10 10:59), Hans Verkuil wrote: > [..] >>> >>> num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); >>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >>> index 0eabb589684f..48d123a1ac2a 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >>> @@ -730,6 +730,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) >>> unsigned requested_sizes[VIDEO_MAX_PLANES]; >>> struct v4l2_format *f = &create->format; >>> int ret = vb2_verify_memory_type(q, create->memory, f->type); >>> + bool consistent = true; >>> unsigned i; >>> >>> fill_buf_caps(q, &create->capabilities); >>> @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) >>> for (i = 0; i < requested_planes; i++) >>> if (requested_sizes[i] == 0) >>> return -EINVAL; >>> - return ret ? ret : vb2_core_create_bufs(q, create->memory, >>> + >>> + if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT) >>> + consistent = false; >>> + >>> + return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent, >>> &create->count, requested_planes, requested_sizes); >> >> As mentioned before: we need a V4L2_BUF_CAP capability. > > I can add V4L2_BUF_CAP for memory consistency. Isn't it just q->memory > property though? User space may request MMAP consistent memory or MMAP > inconsistent memory. So instead of adding a flag we add a V4L2_MEMORY_MMAP_NON_CONSISTENT memory type and add a V4L2_BUF_CAP_SUPPORTS_MMAP_NON_CONSISTENT to signal support for this? I like that better than a flag. It also automatically enforces that all buffers must be of that type. Regards, Hans > > -ss >
On (20/01/23 12:41), Hans Verkuil wrote: [..] > >>> > >>> fill_buf_caps(q, &create->capabilities); > >>> @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) > >>> for (i = 0; i < requested_planes; i++) > >>> if (requested_sizes[i] == 0) > >>> return -EINVAL; > >>> - return ret ? ret : vb2_core_create_bufs(q, create->memory, > >>> + > >>> + if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT) > >>> + consistent = false; > >>> + > >>> + return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent, > >>> &create->count, requested_planes, requested_sizes); > >> > >> As mentioned before: we need a V4L2_BUF_CAP capability. > > > > I can add V4L2_BUF_CAP for memory consistency. Isn't it just q->memory > > property though? User space may request MMAP consistent memory or MMAP > > inconsistent memory. > > So instead of adding a flag we add a V4L2_MEMORY_MMAP_NON_CONSISTENT memory > type and add a V4L2_BUF_CAP_SUPPORTS_MMAP_NON_CONSISTENT to signal support > for this? > > I like that better than a flag. It also automatically enforces that all > buffers must be of that type. Yes, we had this idea as well. The conclusion was it makes the patch set bigger and harder to verify and review. Passing memory consistency attribute via ->flags was the shortest path at the end. Namely due to all those numerous places that test q->memory: 455 if (q->memory == VB2_MEMORY_MMAP) 456 __vb2_buf_mem_free(vb); 457 else if (q->memory == VB2_MEMORY_DMABUF) 458 __vb2_buf_dmabuf_put(vb); 459 else 460 __vb2_buf_userptr_put(vb); [..] 737 mutex_lock(&q->mmap_lock); 738 if (debug && q->memory == VB2_MEMORY_MMAP && 739 __buffers_in_use(q)) 740 dprintk(1, "memory in use, orphaning buffers\n"); [..] etc. As a workaround we looked at the idea that V4L2_MEMORY_MMAP_NON_CONSISTENT flag might make sense only on the very high level - when user space requests V4L2_MEMORY_MMAP_NON_CONSISTENT then we simply set DMA attribute and then "downgrade" requested V4L2_MEMORY_MMAP_NON_CONSISTENT memory type to V4L2_MEMORY_MMAP. Something like this. --- diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 4489744fbbd9..60afbfcca995 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -803,6 +803,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, } EXPORT_SYMBOL_GPL(vb2_core_reqbufs); +static void __set_queue_consistency(struct vb2_queue *q, bool consistent_mem) +{ + if (consistent_mem) + q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT; + else + q->dma_attrs |= DMA_ATTR_NON_CONSISTENT; +} + int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, unsigned int *count, unsigned requested_planes, const unsigned requested_sizes[]) @@ -810,6 +818,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, unsigned int num_planes = 0, num_buffers, allocated_buffers; unsigned plane_sizes[VB2_MAX_PLANES] = { }; int ret; + bool consistent_mem = (memory == V4L2_MEMORY_MMAP_NON_CONSISTENT); + + if (consistent_mem) + memory = V4L2_MEMORY_MMAP; if (q->num_buffers == VB2_MAX_FRAME) { dprintk(1, "maximum number of buffers already allocated\n"); @@ -822,6 +834,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, return -EBUSY; } memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); + __set_queue_consistency(q, consistent_mem); q->memory = memory; q->waiting_for_buffers = !q->is_output; } else if (q->memory != memory) {
diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst index bd08e4f77ae4..c56e80659b4a 100644 --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst +++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst @@ -121,7 +121,13 @@ than the number requested. other changes, then set ``count`` to 0, ``memory`` to ``V4L2_MEMORY_MMAP`` and ``format.type`` to the buffer type. * - __u32 - - ``reserved``\ [7] + - ``flags`` + - Specifies additional buffer management attributes. E.g. when + ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated + in non-consistent memory. + + * - __u32 + - ``reserved``\ [6] - A place holder for future extensions. Drivers and applications must set the array to zero. diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 668c56df13f6..d1012a24755d 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -812,9 +812,21 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, } EXPORT_SYMBOL_GPL(vb2_core_reqbufs); +static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem) +{ + bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT; + + if (consistent_mem != queue_attr) { + dprintk(1, "memory consistency model mismatch\n"); + return false; + } + return true; +} + int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, - unsigned int *count, unsigned requested_planes, - const unsigned requested_sizes[]) + bool consistent_mem, unsigned int *count, + unsigned requested_planes, + const unsigned requested_sizes[]) { unsigned int num_planes = 0, num_buffers, allocated_buffers; unsigned plane_sizes[VB2_MAX_PLANES] = { }; @@ -832,10 +844,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, } memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); q->memory = memory; + __set_queue_consistency(q, consistent_mem); q->waiting_for_buffers = !q->is_output; - } else if (q->memory != memory) { - dprintk(1, "memory model mismatch\n"); - return -EINVAL; + } else { + if (q->memory != memory) { + dprintk(1, "memory model mismatch\n"); + return -EINVAL; + } + if (!verify_consistency_attr(q, consistent_mem)) + return -EINVAL; } num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index 0eabb589684f..48d123a1ac2a 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -730,6 +730,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) unsigned requested_sizes[VIDEO_MAX_PLANES]; struct v4l2_format *f = &create->format; int ret = vb2_verify_memory_type(q, create->memory, f->type); + bool consistent = true; unsigned i; fill_buf_caps(q, &create->capabilities); @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create) for (i = 0; i < requested_planes; i++) if (requested_sizes[i] == 0) return -EINVAL; - return ret ? ret : vb2_core_create_bufs(q, create->memory, + + if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT) + consistent = false; + + return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent, &create->count, requested_planes, requested_sizes); } EXPORT_SYMBOL_GPL(vb2_create_bufs); diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 225d06819bce..793cb6534de4 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -2012,7 +2012,7 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops, if (ret) return ret; - CLEAR_AFTER_FIELD(create, capabilities); + CLEAR_AFTER_FIELD(create, flags); v4l_sanitize_format(&create->format); diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 810af5cf5742..5e5450bdabbd 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -757,6 +757,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, * vb2_core_create_bufs() - Allocate buffers and any required auxiliary structs * @q: pointer to &struct vb2_queue with videobuf2 queue. * @memory: memory type, as defined by &enum vb2_memory. + * @consistent_mem: memory consistency model. * @count: requested buffer count. * @requested_planes: number of planes requested. * @requested_sizes: array with the size of the planes. @@ -774,7 +775,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, * Return: returns zero on success; an error code otherwise. */ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, - unsigned int *count, unsigned int requested_planes, + bool consistent_mem, unsigned int *count, + unsigned int requested_planes, const unsigned int requested_sizes[]); /** diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 73a4854f71bd..82e2ded5a136 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -2419,7 +2419,8 @@ struct v4l2_create_buffers { __u32 memory; struct v4l2_format format; __u32 capabilities; - __u32 reserved[7]; + __u32 flags; + __u32 reserved[6]; }; /*
This patch lets user-space to request a non-consistent memory allocation during CREATE_BUFS ioctl call. struct v4l2_create_buffers has seven 4-byte reserved areas, so reserved[0] is renamed to ->flags. The struct, thus, now has six reserved 4-byte regions. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- .../media/uapi/v4l/vidioc-create-bufs.rst | 8 +++++- .../media/common/videobuf2/videobuf2-core.c | 27 +++++++++++++++---- .../media/common/videobuf2/videobuf2-v4l2.c | 7 ++++- drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- include/media/videobuf2-core.h | 4 ++- include/uapi/linux/videodev2.h | 3 ++- 6 files changed, 41 insertions(+), 10 deletions(-)