| Message ID | 20191217032034.54897-5-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: > Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. > > Extend vb2_core_reqbufs() with queue memory consistency flag. > API permits queue's consistency attribute adjustment only if > the queue has no allocated buffers, not busy, and does not have > buffers waiting to be de-queued. Actually, you can call vb2_core_reqbufs() when buffers are allocated: it will free the old buffers, then allocate the new ones. So drop the 'has no allocated buffers' bit. > > If user-space attempts to allocate a buffer with consistency > requirements which don't match queue's consistency model such > allocation requests will be failed. Is this last paragraph right? I don't see any code for that. BTW, a general comment about patches 4-6: I prefer if you changes this to two patches: one that adds videobuf2-core.c support for this for reqbufs and create_bufs, then another that wires up the new V4L2 flag in videobuf2-v4l2.c. It's easier to review that way. Regards, Hans > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 17 +++++++++++++---- > drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++-- > drivers/media/dvb-core/dvb_vb2.c | 2 +- > include/media/videobuf2-core.h | 3 ++- > 4 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 4489744fbbd9..668c56df13f6 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -664,8 +664,16 @@ int vb2_verify_memory_type(struct vb2_queue *q, > } > EXPORT_SYMBOL(vb2_verify_memory_type); > > +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_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > - unsigned int *count) > + bool consistent_mem, unsigned int *count) > { > unsigned int num_buffers, allocated_buffers, num_planes = 0; > unsigned plane_sizes[VB2_MAX_PLANES] = { }; > @@ -720,6 +728,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > q->memory = memory; > + __set_queue_consistency(q, consistent_mem); > > /* > * Ask the driver how many buffers and planes per buffer it requires. > @@ -2498,7 +2507,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > fileio->memory = VB2_MEMORY_MMAP; > fileio->type = q->type; > q->fileio = fileio; > - ret = vb2_core_reqbufs(q, fileio->memory, &fileio->count); > + ret = vb2_core_reqbufs(q, fileio->memory, true, &fileio->count); > if (ret) > goto err_kfree; > > @@ -2555,7 +2564,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > > err_reqbufs: > fileio->count = 0; > - vb2_core_reqbufs(q, fileio->memory, &fileio->count); > + vb2_core_reqbufs(q, fileio->memory, true, &fileio->count); > > err_kfree: > q->fileio = NULL; > @@ -2575,7 +2584,7 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q) > vb2_core_streamoff(q, q->type); > q->fileio = NULL; > fileio->count = 0; > - vb2_core_reqbufs(q, fileio->memory, &fileio->count); > + vb2_core_reqbufs(q, fileio->memory, true, &fileio->count); > kfree(fileio); > dprintk(3, "file io emulator closed\n"); > } > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index 2fccfe2a57f8..f1e88c9398c7 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -695,7 +695,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > int ret = vb2_verify_memory_type(q, req->memory, req->type); > > fill_buf_caps(q, &req->capabilities); > - return ret ? ret : vb2_core_reqbufs(q, req->memory, &req->count); > + return ret ? ret : vb2_core_reqbufs(q, req->memory, true, &req->count); > } > EXPORT_SYMBOL_GPL(vb2_reqbufs); > > @@ -945,7 +945,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv, > return res; > if (vb2_queue_is_busy(vdev, file)) > return -EBUSY; > - res = vb2_core_reqbufs(vdev->queue, p->memory, &p->count); > + res = vb2_core_reqbufs(vdev->queue, p->memory, true, &p->count); > /* If count == 0, then the owner has released all buffers and he > is no longer owner of the queue. Otherwise we have a new owner. */ > if (res == 0) > diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c > index 6974f1731529..e60063652164 100644 > --- a/drivers/media/dvb-core/dvb_vb2.c > +++ b/drivers/media/dvb-core/dvb_vb2.c > @@ -342,7 +342,7 @@ int dvb_vb2_reqbufs(struct dvb_vb2_ctx *ctx, struct dmx_requestbuffers *req) > > ctx->buf_siz = req->size; > ctx->buf_cnt = req->count; > - ret = vb2_core_reqbufs(&ctx->vb_q, VB2_MEMORY_MMAP, &req->count); > + ret = vb2_core_reqbufs(&ctx->vb_q, VB2_MEMORY_MMAP, true, &req->count); > if (ret) { > ctx->state = DVB_VB2_STATE_NONE; > dprintk(1, "[%s] count=%d size=%d errno=%d\n", ctx->name, > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 026004180440..810af5cf5742 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -726,6 +726,7 @@ void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb); > * vb2_core_reqbufs() - Initiate streaming. > * @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. > * > * Videobuf2 core helper to implement VIDIOC_REQBUF() operation. It is called > @@ -750,7 +751,7 @@ void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb); > * Return: returns zero on success; an error code otherwise. > */ > int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > - unsigned int *count); > + bool consistent_mem, unsigned int *count); > > /** > * vb2_core_create_bufs() - Allocate buffers and any required auxiliary structs >
On (20/01/10 10:47), Hans Verkuil wrote: > On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > > Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. > > > > Extend vb2_core_reqbufs() with queue memory consistency flag. > > API permits queue's consistency attribute adjustment only if > > the queue has no allocated buffers, not busy, and does not have > > buffers waiting to be de-queued. > > Actually, you can call vb2_core_reqbufs() when buffers are allocated: > it will free the old buffers, then allocate the new ones. > So drop the 'has no allocated buffers' bit. Well, the wording, basically, follows the existing vb2_core_reqbufs() behavior "queue memory type"-wise. What I'm trying to say: [..] int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, bool consistent_mem, unsigned int *count) { unsigned int num_buffers, allocated_buffers, num_planes = 0; unsigned plane_sizes[VB2_MAX_PLANES] = { }; unsigned int i; int ret; if (q->streaming) { dprintk(1, "streaming active\n"); return -EBUSY; } if (q->waiting_in_dqbuf && *count) { dprintk(1, "another dup()ped fd is waiting for a buffer\n"); return -EBUSY; } if (*count == 0 || q->num_buffers != 0 || (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) { /* * We already have buffers allocated, so first check if they * are not in use and can be freed. */ mutex_lock(&q->mmap_lock); if (debug && q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) dprintk(1, "memory in use, orphaning buffers\n"); /* * Call queue_cancel to clean up any buffers in the * QUEUED state which is possible if buffers were prepared or * queued without ever calling STREAMON. */ __vb2_queue_cancel(q); ret = __vb2_queue_free(q, q->num_buffers); mutex_unlock(&q->mmap_lock); if (ret) return ret; /* * In case of REQBUFS(0) return immediately without calling * driver's queue_setup() callback and allocating resources. */ if (*count == 0) return 0; } /* * Make sure the requested values and current defaults are sane. */ WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); q->memory = memory; + __set_queue_consistency(q, consistent_mem); [..] So we set/change queue consistency attribute when we set/change queue memory type. Is there a use case for more flexibility when it comes to queue consistency? > > If user-space attempts to allocate a buffer with consistency > > requirements which don't match queue's consistency model such > > allocation requests will be failed. > > Is this last paragraph right? I don't see any code for that. Yeah, this was more about the general direction. The actual code was added later in the series. > BTW, a general comment about patches 4-6: I prefer if you changes > this to two patches: one that adds videobuf2-core.c support for > this for reqbufs and create_bufs, then another that wires up the > new V4L2 flag in videobuf2-v4l2.c. I'll take a look. -ss
On 1/22/20 3:05 AM, Sergey Senozhatsky wrote: > On (20/01/10 10:47), Hans Verkuil wrote: >> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: >>> Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. >>> >>> Extend vb2_core_reqbufs() with queue memory consistency flag. >>> API permits queue's consistency attribute adjustment only if >>> the queue has no allocated buffers, not busy, and does not have >>> buffers waiting to be de-queued. >> >> Actually, you can call vb2_core_reqbufs() when buffers are allocated: >> it will free the old buffers, then allocate the new ones. >> So drop the 'has no allocated buffers' bit. > > Well, the wording, basically, follows the existing vb2_core_reqbufs() > behavior "queue memory type"-wise. What I'm trying to say: How about this commit log replacement of the first paragraph: "Extend vb2_core_reqbufs() with queue memory consistency flag that is applied to the newly allocated buffers." The bits about 'only if the queue has no allocated buffers, not busy, and does not have buffers waiting to be de-queued.' is really irrelevant and confusing (at least to me!). Regards, Hans > > [..] > int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > bool consistent_mem, unsigned int *count) > { > unsigned int num_buffers, allocated_buffers, num_planes = 0; > unsigned plane_sizes[VB2_MAX_PLANES] = { }; > unsigned int i; > int ret; > > if (q->streaming) { > dprintk(1, "streaming active\n"); > return -EBUSY; > } > > if (q->waiting_in_dqbuf && *count) { > dprintk(1, "another dup()ped fd is waiting for a buffer\n"); > return -EBUSY; > } > > if (*count == 0 || q->num_buffers != 0 || > (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) { > /* > * We already have buffers allocated, so first check if they > * are not in use and can be freed. > */ > mutex_lock(&q->mmap_lock); > if (debug && q->memory == VB2_MEMORY_MMAP && > __buffers_in_use(q)) > dprintk(1, "memory in use, orphaning buffers\n"); > > /* > * Call queue_cancel to clean up any buffers in the > * QUEUED state which is possible if buffers were prepared or > * queued without ever calling STREAMON. > */ > __vb2_queue_cancel(q); > ret = __vb2_queue_free(q, q->num_buffers); > mutex_unlock(&q->mmap_lock); > if (ret) > return ret; > > /* > * In case of REQBUFS(0) return immediately without calling > * driver's queue_setup() callback and allocating resources. > */ > if (*count == 0) > return 0; > } > > /* > * Make sure the requested values and current defaults are sane. > */ > WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > q->memory = memory; > + __set_queue_consistency(q, consistent_mem); > > [..] > > So we set/change queue consistency attribute when we set/change > queue memory type. Is there a use case for more flexibility when > it comes to queue consistency? > >>> If user-space attempts to allocate a buffer with consistency >>> requirements which don't match queue's consistency model such >>> allocation requests will be failed. >> >> Is this last paragraph right? I don't see any code for that. > > Yeah, this was more about the general direction. The actual code > was added later in the series. > >> BTW, a general comment about patches 4-6: I prefer if you changes >> this to two patches: one that adds videobuf2-core.c support for >> this for reqbufs and create_bufs, then another that wires up the >> new V4L2 flag in videobuf2-v4l2.c. > > I'll take a look. > > -ss >
On (20/01/23 12:02), Hans Verkuil wrote: > On 1/22/20 3:05 AM, Sergey Senozhatsky wrote: > > On (20/01/10 10:47), Hans Verkuil wrote: > >> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > >>> Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. > >>> > >>> Extend vb2_core_reqbufs() with queue memory consistency flag. > >>> API permits queue's consistency attribute adjustment only if > >>> the queue has no allocated buffers, not busy, and does not have > >>> buffers waiting to be de-queued. > >> > >> Actually, you can call vb2_core_reqbufs() when buffers are allocated: > >> it will free the old buffers, then allocate the new ones. > >> So drop the 'has no allocated buffers' bit. > > > > Well, the wording, basically, follows the existing vb2_core_reqbufs() > > behavior "queue memory type"-wise. What I'm trying to say: > > How about this commit log replacement of the first paragraph: > > "Extend vb2_core_reqbufs() with queue memory consistency flag that is > applied to the newly allocated buffers." Looks good. > The bits about 'only if the queue has no allocated buffers, not busy, and does > not have buffers waiting to be de-queued.' is really irrelevant and confusing > (at least to me!). Agreed, those bits describe implementation details which can change. Better get rid of them. -ss
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 4489744fbbd9..668c56df13f6 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -664,8 +664,16 @@ int vb2_verify_memory_type(struct vb2_queue *q, } EXPORT_SYMBOL(vb2_verify_memory_type); +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_reqbufs(struct vb2_queue *q, enum vb2_memory memory, - unsigned int *count) + bool consistent_mem, unsigned int *count) { unsigned int num_buffers, allocated_buffers, num_planes = 0; unsigned plane_sizes[VB2_MAX_PLANES] = { }; @@ -720,6 +728,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); q->memory = memory; + __set_queue_consistency(q, consistent_mem); /* * Ask the driver how many buffers and planes per buffer it requires. @@ -2498,7 +2507,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) fileio->memory = VB2_MEMORY_MMAP; fileio->type = q->type; q->fileio = fileio; - ret = vb2_core_reqbufs(q, fileio->memory, &fileio->count); + ret = vb2_core_reqbufs(q, fileio->memory, true, &fileio->count); if (ret) goto err_kfree; @@ -2555,7 +2564,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) err_reqbufs: fileio->count = 0; - vb2_core_reqbufs(q, fileio->memory, &fileio->count); + vb2_core_reqbufs(q, fileio->memory, true, &fileio->count); err_kfree: q->fileio = NULL; @@ -2575,7 +2584,7 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q) vb2_core_streamoff(q, q->type); q->fileio = NULL; fileio->count = 0; - vb2_core_reqbufs(q, fileio->memory, &fileio->count); + vb2_core_reqbufs(q, fileio->memory, true, &fileio->count); kfree(fileio); dprintk(3, "file io emulator closed\n"); } diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index 2fccfe2a57f8..f1e88c9398c7 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -695,7 +695,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) int ret = vb2_verify_memory_type(q, req->memory, req->type); fill_buf_caps(q, &req->capabilities); - return ret ? ret : vb2_core_reqbufs(q, req->memory, &req->count); + return ret ? ret : vb2_core_reqbufs(q, req->memory, true, &req->count); } EXPORT_SYMBOL_GPL(vb2_reqbufs); @@ -945,7 +945,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv, return res; if (vb2_queue_is_busy(vdev, file)) return -EBUSY; - res = vb2_core_reqbufs(vdev->queue, p->memory, &p->count); + res = vb2_core_reqbufs(vdev->queue, p->memory, true, &p->count); /* If count == 0, then the owner has released all buffers and he is no longer owner of the queue. Otherwise we have a new owner. */ if (res == 0) diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c index 6974f1731529..e60063652164 100644 --- a/drivers/media/dvb-core/dvb_vb2.c +++ b/drivers/media/dvb-core/dvb_vb2.c @@ -342,7 +342,7 @@ int dvb_vb2_reqbufs(struct dvb_vb2_ctx *ctx, struct dmx_requestbuffers *req) ctx->buf_siz = req->size; ctx->buf_cnt = req->count; - ret = vb2_core_reqbufs(&ctx->vb_q, VB2_MEMORY_MMAP, &req->count); + ret = vb2_core_reqbufs(&ctx->vb_q, VB2_MEMORY_MMAP, true, &req->count); if (ret) { ctx->state = DVB_VB2_STATE_NONE; dprintk(1, "[%s] count=%d size=%d errno=%d\n", ctx->name, diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 026004180440..810af5cf5742 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -726,6 +726,7 @@ void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb); * vb2_core_reqbufs() - Initiate streaming. * @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. * * Videobuf2 core helper to implement VIDIOC_REQBUF() operation. It is called @@ -750,7 +751,7 @@ void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb); * Return: returns zero on success; an error code otherwise. */ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, - unsigned int *count); + bool consistent_mem, unsigned int *count); /** * vb2_core_create_bufs() - Allocate buffers and any required auxiliary structs
Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. Extend vb2_core_reqbufs() with queue memory consistency flag. API permits queue's consistency attribute adjustment only if the queue has no allocated buffers, not busy, and does not have buffers waiting to be de-queued. If user-space attempts to allocate a buffer with consistency requirements which don't match queue's consistency model such allocation requests will be failed. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- drivers/media/common/videobuf2/videobuf2-core.c | 17 +++++++++++++---- drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++-- drivers/media/dvb-core/dvb_vb2.c | 2 +- include/media/videobuf2-core.h | 3 ++- 4 files changed, 18 insertions(+), 8 deletions(-)