| Message ID | 20200302041213.27662-5-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement V4L2_BUF_FLAG_NO_CACHE_* flags | expand |
On 02/03/2020 05:12, Sergey Senozhatsky wrote: > Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. > > Extend vb2_core_reqbufs() with queue memory consistency flag > that is applied to the newly allocated buffers. > > An attempt to allocate a buffer with consistency requirements > which don't match queue's consistency model will fail. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/common/videobuf2/videobuf2-core.c | 47 +++++++++++++++---- > .../media/common/videobuf2/videobuf2-v4l2.c | 6 +-- > drivers/media/dvb-core/dvb_vb2.c | 2 +- > include/media/videobuf2-core.h | 7 ++- > 4 files changed, 47 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 4489744fbbd9..3ca0545db7ee 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -664,8 +664,19 @@ 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 (!vb2_queue_allows_cache_hints(q)) > + return; > + > + 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 +731,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. > @@ -803,9 +815,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) { This is the wrong way around! It's much better to write it like this: bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT); if (consistent_mem != queue_is_consistent) { What concerns me more is that this means that this series has not been tested properly. I found this when testing with v4l2-compliance and vivid. I'll reply to the cover letter with more information about what needs to be done before I merge this. > + 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[]) Use 'unsigned int' in the two lines above, as per checkpatch suggestion. Regards, Hans > { > unsigned int num_planes = 0, num_buffers, allocated_buffers; > unsigned plane_sizes[VB2_MAX_PLANES] = { }; > @@ -823,10 +847,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); > @@ -2498,7 +2527,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 +2584,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 +2604,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 c847bcea6e95..6111d74f68c9 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -724,7 +724,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); > > @@ -798,7 +798,7 @@ 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, > + return ret ? ret : vb2_core_create_bufs(q, create->memory, true, > &create->count, requested_planes, requested_sizes); > } > EXPORT_SYMBOL_GPL(vb2_create_bufs); > @@ -974,7 +974,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 731fd9fbd506..ba83ac754c21 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -737,6 +737,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 > @@ -761,12 +762,13 @@ 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 > * @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. > @@ -784,7 +786,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[]); > > /** >
On (20/03/06 15:04), Hans Verkuil wrote: [..] > > +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) { > > This is the wrong way around! > > It's much better to write it like this: > > bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT); > > if (consistent_mem != queue_is_consistent) { Hmm... That's a great catch. Thanks for spotting this. Puzzled, how come I've never seen problems. > What concerns me more is that this means that this series has not been > tested properly. I found this when testing with v4l2-compliance and vivid. I fully understand your concerns. Give me a moment to figure out what's going on... OK. Apparently, the user-space I'm using for tests, utilizes different call path. vb2_core_create_bufs() is never even invoked. Hence queue consistency vs. request consistency checks are not performed. What happens, instead, is v4l_reqbufs()->vb2_core_reqbufs() path. It orphans existing buffers (if any), sets queue memory model, sets queue consistency model (DMA attr), then allocates buffers. On my test environment, I see that vb2_core_reqbufs() orphans the buffers, but it's always due to "*count == 0 || q->num_buffers != 0" conditions. The user-space I'm using does not twist queue ->memory or consistency attr, so the tests I'm running are limited in scenarios. verify_consistency_attr() is not on the list of reasons to orphan allocated buffer. It probably should be, tho. === diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index afb3c21a5902..d6b1d32bef3f 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -730,7 +730,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, } if (*count == 0 || q->num_buffers != 0 || - (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) { + (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory) || + !verify_consistency_attr(q, consistent_mem)) { /* * We already have buffers allocated, so first check if they * are not in use and can be freed. === > > + 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[]) > > Use 'unsigned int' in the two lines above, as per checkpatch suggestion. OK, will do. This comes from the original code. There are 'unsigned'-s in the existing code, I saw it and didn't want to modify, in order to keep diffstats shorter. -ss
On 07/03/2020 08:50, Sergey Senozhatsky wrote: > On (20/03/06 15:04), Hans Verkuil wrote: > [..] >>> +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) { >> >> This is the wrong way around! >> >> It's much better to write it like this: >> >> bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT); >> >> if (consistent_mem != queue_is_consistent) { > > Hmm... That's a great catch. Thanks for spotting this. > Puzzled, how come I've never seen problems. > >> What concerns me more is that this means that this series has not been >> tested properly. I found this when testing with v4l2-compliance and vivid. > > I fully understand your concerns. Give me a moment to figure > out what's going on... > > > OK. > > Apparently, the user-space I'm using for tests, utilizes different > call path. vb2_core_create_bufs() is never even invoked. Hence queue > consistency vs. request consistency checks are not performed. > > What happens, instead, is v4l_reqbufs()->vb2_core_reqbufs() path. > It orphans existing buffers (if any), sets queue memory model, sets > queue consistency model (DMA attr), then allocates buffers. > > On my test environment, I see that vb2_core_reqbufs() orphans the > buffers, but it's always due to "*count == 0 || q->num_buffers != 0" > conditions. The user-space I'm using does not twist queue ->memory > or consistency attr, so the tests I'm running are limited in scenarios. That's why v4l2-compliance is so important: it tests 'twisty code' for correct handling. > > verify_consistency_attr() is not on the list of reasons to orphan > allocated buffer. It probably should be, tho. > > === > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index afb3c21a5902..d6b1d32bef3f 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -730,7 +730,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > } > > if (*count == 0 || q->num_buffers != 0 || > - (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) { > + (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory) || > + !verify_consistency_attr(q, consistent_mem)) { > /* > * We already have buffers allocated, so first check if they > * are not in use and can be freed. > === > >>> + 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[]) >> >> Use 'unsigned int' in the two lines above, as per checkpatch suggestion. > > OK, will do. > > This comes from the original code. There are 'unsigned'-s in the > existing code, I saw it and didn't want to modify, in order to keep > diffstats shorter. Yeah, but the prototype was already inconsistent (count is an unsigned int *), so it makes sense to fix this. Regards, Hans > > -ss >
Hi On 02.03.20 05:12, Sergey Senozhatsky wrote: > Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. > > Extend vb2_core_reqbufs() with queue memory consistency flag > that is applied to the newly allocated buffers. > > An attempt to allocate a buffer with consistency requirements > which don't match queue's consistency model will fail. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/common/videobuf2/videobuf2-core.c | 47 +++++++++++++++---- > .../media/common/videobuf2/videobuf2-v4l2.c | 6 +-- > drivers/media/dvb-core/dvb_vb2.c | 2 +- > include/media/videobuf2-core.h | 7 ++- > 4 files changed, 47 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 4489744fbbd9..3ca0545db7ee 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -664,8 +664,19 @@ 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 (!vb2_queue_allows_cache_hints(q)) > + return; > + > + 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) You extended the vb2_core_reqbufs accepting a new boolean that is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT but in the future some other flags might be added, and so I think it is better to replace the boolean with a u32 consisting of all the flags. Thanks, Dafna > { > unsigned int num_buffers, allocated_buffers, num_planes = 0; > unsigned plane_sizes[VB2_MAX_PLANES] = { }; > @@ -720,6 +731,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. > @@ -803,9 +815,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] = { }; > @@ -823,10 +847,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); > @@ -2498,7 +2527,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 +2584,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 +2604,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 c847bcea6e95..6111d74f68c9 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -724,7 +724,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); > > @@ -798,7 +798,7 @@ 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, > + return ret ? ret : vb2_core_create_bufs(q, create->memory, true, > &create->count, requested_planes, requested_sizes); > } > EXPORT_SYMBOL_GPL(vb2_create_bufs); > @@ -974,7 +974,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 731fd9fbd506..ba83ac754c21 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -737,6 +737,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 > @@ -761,12 +762,13 @@ 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 > * @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. > @@ -784,7 +786,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[]); > > /** >
On (20/03/20 14:46), Dafna Hirschfeld wrote: [..] > > +static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem) > > +{ > > + if (!vb2_queue_allows_cache_hints(q)) > > + return; > > + > > + 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) > You extended the vb2_core_reqbufs accepting a new boolean that > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT > but in the future some other flags might be added, and so I think it > is better to replace the boolean with a u32 consisting of all the flags. Don't have any objections. Can change the `bool' to `u32'. -ss
On Tue, 2020-03-24 at 11:39 +0900, Sergey Senozhatsky wrote: > On (20/03/20 14:46), Dafna Hirschfeld wrote: > [..] > > > +static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem) > > > +{ > > > + if (!vb2_queue_allows_cache_hints(q)) > > > + return; > > > + > > > + 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) > > You extended the vb2_core_reqbufs accepting a new boolean that > > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT > > but in the future some other flags might be added, and so I think it > > is better to replace the boolean with a u32 consisting of all the flags. > > Don't have any objections. Can change the `bool' to `u32'. > or maybe an enum instead? That would help get a cleaner interface. Thanks, Ezequiel
On (20/03/24 07:17), Ezequiel Garcia wrote: [..] > > > > +static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem) > > > > +{ > > > > + if (!vb2_queue_allows_cache_hints(q)) > > > > + return; > > > > + > > > > + 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) > > > You extended the vb2_core_reqbufs accepting a new boolean that > > > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT > > > but in the future some other flags might be added, and so I think it > > > is better to replace the boolean with a u32 consisting of all the flags. > > > > Don't have any objections. Can change the `bool' to `u32'. > > > > or maybe an enum instead? That would help get a cleaner > interface. Hmm, interesting. The flags in question can be from different, unrelated groups (types), controlling various parts of the stack. Not necessarily all of them are memory_consistency related. We can, for instance, pass some additional flags to underlying memory allocators (contig, sg), etc. E.g. enum MEMORY_ATTR { MEM_NON_CONSISTENT, ... }; enum VMALLOC_ALLOCATOR_ATTR { DO_A_BARREL_ROLL, ... }; enum DMA_SG_ALLOCATOR_ATTR { WRITEBACK_TO_TAPE_DEVICE, ... }; enum DMA_CONTIG_ALLOCATOR_ATTR { PREFER_HTTPS, ... }; and so on. We maybe can keep all of those in one enum (umm, what should be the name?), and then either make sure that all of them are proper powers of two enum AUX_FLAGS { MEM_NON_CONSISTENT = (1 << 0), DO_A_BARREL_ROLL = (1 << 1), WRITEBACK_TO_TAPE_DEVICE = (1 << 2), PREFER_HTTPS = (1 << 3), }; or enum AUX_FLAGS { MEM_NON_CONSISTENT = 0, DO_A_BARREL_ROLL, WRITEBACK_TO_TAPE_DEVICE, PREFER_HTTPS, }; and make sure that those are not flags, but bits. IOW, if (flags & BIT(MEM_NON_CONSISTENT)). What do others think? -ss
On Wed, Mar 25, 2020 at 3:32 AM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (20/03/24 07:17), Ezequiel Garcia wrote: > [..] > > > > > +static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem) > > > > > +{ > > > > > + if (!vb2_queue_allows_cache_hints(q)) > > > > > + return; > > > > > + > > > > > + 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) > > > > You extended the vb2_core_reqbufs accepting a new boolean that > > > > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT > > > > but in the future some other flags might be added, and so I think it > > > > is better to replace the boolean with a u32 consisting of all the flags. > > > > > > Don't have any objections. Can change the `bool' to `u32'. > > > > > > > or maybe an enum instead? That would help get a cleaner > > interface. > > Hmm, interesting. > > The flags in question can be from different, unrelated groups > (types), controlling various parts of the stack. Not necessarily > all of them are memory_consistency related. We can, for instance, > pass some additional flags to underlying memory allocators (contig, > sg), etc. > > E.g. > > enum MEMORY_ATTR { > MEM_NON_CONSISTENT, > ... > }; > > enum VMALLOC_ALLOCATOR_ATTR { > DO_A_BARREL_ROLL, > ... > }; > > enum DMA_SG_ALLOCATOR_ATTR { > WRITEBACK_TO_TAPE_DEVICE, > ... > }; > > enum DMA_CONTIG_ALLOCATOR_ATTR { > PREFER_HTTPS, > ... > }; > > and so on. We maybe can keep all of those in one enum (umm, what should > be the name?), and then either make sure that all of them are proper powers > of two > > enum AUX_FLAGS { > MEM_NON_CONSISTENT = (1 << 0), > DO_A_BARREL_ROLL = (1 << 1), > WRITEBACK_TO_TAPE_DEVICE = (1 << 2), > PREFER_HTTPS = (1 << 3), > }; > > or > enum AUX_FLAGS { > MEM_NON_CONSISTENT = 0, > DO_A_BARREL_ROLL, > WRITEBACK_TO_TAPE_DEVICE, > PREFER_HTTPS, > }; > > and make sure that those are not flags, but bits. > IOW, if (flags & BIT(MEM_NON_CONSISTENT)). > > > What do others think? My feeling is that there it's a bit of an abuse of the language construct. Enum is expected to be an enumeration type, where the value is always one and only one of the defined values at the same time. Making a bitwise OR of several values makes the resulting value outside of the enum specification. AFAICT, the typical approach in the kernel is to just have a block of #define statements to define the flags and have the flag names prefixed with some consistent prefix, e.g. VB2_QUEUE_FLAG_. The flags itself would be defined using BIT() so they can be used directly in the bitwise arithmetics. Best regards, Tomasz
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 4489744fbbd9..3ca0545db7ee 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -664,8 +664,19 @@ 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 (!vb2_queue_allows_cache_hints(q)) + return; + + 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 +731,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. @@ -803,9 +815,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] = { }; @@ -823,10 +847,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); @@ -2498,7 +2527,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 +2584,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 +2604,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 c847bcea6e95..6111d74f68c9 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -724,7 +724,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); @@ -798,7 +798,7 @@ 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, + return ret ? ret : vb2_core_create_bufs(q, create->memory, true, &create->count, requested_planes, requested_sizes); } EXPORT_SYMBOL_GPL(vb2_create_bufs); @@ -974,7 +974,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 731fd9fbd506..ba83ac754c21 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -737,6 +737,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 @@ -761,12 +762,13 @@ 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 * @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. @@ -784,7 +786,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[]); /**
Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support. Extend vb2_core_reqbufs() with queue memory consistency flag that is applied to the newly allocated buffers. An attempt to allocate a buffer with consistency requirements which don't match queue's consistency model will fail. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- .../media/common/videobuf2/videobuf2-core.c | 47 +++++++++++++++---- .../media/common/videobuf2/videobuf2-v4l2.c | 6 +-- drivers/media/dvb-core/dvb_vb2.c | 2 +- include/media/videobuf2-core.h | 7 ++- 4 files changed, 47 insertions(+), 15 deletions(-)