Hi Hans,
Thank you for the patch.
On Friday 29 November 2013 10:58:36 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> 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.
This looks better to me, but what about a vb2_reqbufs(0) call being processed
during the time window where we release the queue mutex ?
> Moving it down offers opportunities to simplify the code.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> 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 b19b306..c8a1f22b5 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -462,6 +462,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;
> @@ -1207,6 +1208,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);
> @@ -1216,12 +1218,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);
> @@ -1235,8 +1256,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;
> }
> @@ -1247,80 +1267,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;
> }
>
> @@ -1369,6 +1356,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 bd8218b..4bc4ad2 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,
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
