From b18a8ff29d80b132018d33479e86ab8ecaee6b46 Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Fri, 13 Dec 2013 13:13:38 -0300 Subject: [PATCH] [media] vb2: push the mmap semaphore down to __buf_prepare() Rather than taking the mmap semaphore at a relatively high-level function, push it down to the place where it is really needed. It was placed in vb2_queue_or_prepare_buf() to prevent racing with other vb2 calls. The only way I can see that a race can happen is when two threads queue the same buffer. The solution for that it to introduce a PREPARING state. Moving it down offers opportunities to simplify the code. Signed-off-by: Hans Verkuil Acked-by: Laurent Pinchart Signed-off-by: Mauro Carvalho Chehab --- drivers/media/v4l2-core/videobuf2-core.c | 82 +++++++++++------------- include/media/videobuf2-core.h | 2 + 2 files changed, 38 insertions(+), 46 deletions(-) diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 12df9fda2924..d3f7e8abe212 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -481,6 +481,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) case VB2_BUF_STATE_PREPARED: b->flags |= V4L2_BUF_FLAG_PREPARED; break; + case VB2_BUF_STATE_PREPARING: case VB2_BUF_STATE_DEQUEUED: /* nothing */ break; @@ -1228,6 +1229,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) { struct vb2_queue *q = vb->vb2_queue; + struct rw_semaphore *mmap_sem; int ret; ret = __verify_length(vb, b); @@ -1237,12 +1239,31 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) return ret; } + vb->state = VB2_BUF_STATE_PREPARING; switch (q->memory) { case V4L2_MEMORY_MMAP: ret = __qbuf_mmap(vb, b); break; case V4L2_MEMORY_USERPTR: + /* + * In case of user pointer buffers vb2 allocators need to get direct + * access to userspace pages. This requires getting the mmap semaphore + * for read access in the current process structure. The same semaphore + * is taken before calling mmap operation, while both qbuf/prepare_buf + * and mmap are called by the driver or v4l2 core with the driver's lock + * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in mmap + * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2 + * core releases the driver's lock, takes mmap_sem and then takes the + * driver's lock again. + */ + mmap_sem = ¤t->mm->mmap_sem; + call_qop(q, wait_prepare, q); + down_read(mmap_sem); + call_qop(q, wait_finish, q); + ret = __qbuf_userptr(vb, b); + + up_read(mmap_sem); break; case V4L2_MEMORY_DMABUF: ret = __qbuf_dmabuf(vb, b); @@ -1256,8 +1277,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) ret = call_qop(q, buf_prepare, vb); if (ret) dprintk(1, "qbuf: buffer preparation failed: %d\n", ret); - else - vb->state = VB2_BUF_STATE_PREPARED; + vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED; return ret; } @@ -1268,80 +1288,47 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b, struct v4l2_buffer *, struct vb2_buffer *)) { - struct rw_semaphore *mmap_sem = NULL; struct vb2_buffer *vb; int ret; - /* - * In case of user pointer buffers vb2 allocators need to get direct - * access to userspace pages. This requires getting the mmap semaphore - * for read access in the current process structure. The same semaphore - * is taken before calling mmap operation, while both qbuf/prepare_buf - * and mmap are called by the driver or v4l2 core with the driver's lock - * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in mmap - * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2 - * core releases the driver's lock, takes mmap_sem and then takes the - * driver's lock again. - * - * To avoid racing with other vb2 calls, which might be called after - * releasing the driver's lock, this operation is performed at the - * beginning of qbuf/prepare_buf processing. This way the queue status - * is consistent after getting the driver's lock back. - */ - if (q->memory == V4L2_MEMORY_USERPTR) { - mmap_sem = ¤t->mm->mmap_sem; - call_qop(q, wait_prepare, q); - down_read(mmap_sem); - call_qop(q, wait_finish, q); - } - if (q->fileio) { dprintk(1, "%s(): file io in progress\n", opname); - ret = -EBUSY; - goto unlock; + return -EBUSY; } if (b->type != q->type) { dprintk(1, "%s(): invalid buffer type\n", opname); - ret = -EINVAL; - goto unlock; + return -EINVAL; } if (b->index >= q->num_buffers) { dprintk(1, "%s(): buffer index out of range\n", opname); - ret = -EINVAL; - goto unlock; + return -EINVAL; } vb = q->bufs[b->index]; if (NULL == vb) { /* Should never happen */ dprintk(1, "%s(): buffer is NULL\n", opname); - ret = -EINVAL; - goto unlock; + return -EINVAL; } if (b->memory != q->memory) { dprintk(1, "%s(): invalid memory type\n", opname); - ret = -EINVAL; - goto unlock; + return -EINVAL; } ret = __verify_planes_array(vb, b); if (ret) - goto unlock; + return ret; ret = handler(q, b, vb); - if (ret) - goto unlock; - - /* Fill buffer information for the userspace */ - __fill_v4l2_buffer(vb, b); + if (!ret) { + /* Fill buffer information for the userspace */ + __fill_v4l2_buffer(vb, b); - dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index); -unlock: - if (mmap_sem) - up_read(mmap_sem); + dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index); + } return ret; } @@ -1390,6 +1377,9 @@ static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b, return ret; case VB2_BUF_STATE_PREPARED: break; + case VB2_BUF_STATE_PREPARING: + dprintk(1, "qbuf: buffer still being prepared\n"); + return -EINVAL; default: dprintk(1, "qbuf: buffer already in use\n"); return -EINVAL; diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 0ae974ee8894..ea766525a268 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -142,6 +142,7 @@ enum vb2_fileio_flags { /** * enum vb2_buffer_state - current video buffer state * @VB2_BUF_STATE_DEQUEUED: buffer under userspace control + * @VB2_BUF_STATE_PREPARING: buffer is being prepared in videobuf * @VB2_BUF_STATE_PREPARED: buffer prepared in videobuf and by the driver * @VB2_BUF_STATE_QUEUED: buffer queued in videobuf, but not in driver * @VB2_BUF_STATE_ACTIVE: buffer queued in driver and possibly used @@ -154,6 +155,7 @@ enum vb2_fileio_flags { */ enum vb2_buffer_state { VB2_BUF_STATE_DEQUEUED, + VB2_BUF_STATE_PREPARING, VB2_BUF_STATE_PREPARED, VB2_BUF_STATE_QUEUED, VB2_BUF_STATE_ACTIVE, -- GitLab