[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 <hans.verkuil@cisco.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 12df9fd..d3f7e8a 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -481,6 +481,7 @@
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 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 @@
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 @@
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 @@
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;
+ if (!ret) {
+ /* Fill buffer information for the userspace */
+ __fill_v4l2_buffer(vb, b);
- /* 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 @@
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 0ae974e..ea76652 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -142,6 +142,7 @@
/**
* 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_buffer_state {
VB2_BUF_STATE_DEQUEUED,
+ VB2_BUF_STATE_PREPARING,
VB2_BUF_STATE_PREPARED,
VB2_BUF_STATE_QUEUED,
VB2_BUF_STATE_ACTIVE,