Hi Hans,
Thank you for the patch.
On Thursday 21 November 2013 16:21:59 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, however, I see no way that any race can happen.
What about the following scenario ? Both QBUF calls are performed on the same
buffer.
CPU 0 CPU 1
-------------------------------------------------------------------------
QBUF QBUF
locks the queue mutex waits for the queue mutex
vb2_qbuf
vb2_queue_or_prepare_buf
__vb2_qbuf
checks vb->state, calls
__buf_prepare
call_qop(q, wait_prepare, q);
unlocks the queue mutex
locks the queue mutex
vb2_qbuf
vb2_queue_or_prepare_buf
__vb2_qbuf
checks vb->state, calls
__buf_prepare
call_qop(q, wait_prepare, q);
unlocks the queue mutex
queue the buffer, set buffer
state to queue
queue the buffer, set buffer
state to queue
We would thus end up queueing the buffer twice. The vb->state check needs to
be performed after the brief release of 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 | 74 +++++++++++-----------------
> 1 file changed, 30 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 91412d4..d2b2efb 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1205,6 +1205,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);
> @@ -1219,7 +1220,25 @@ static int __buf_prepare(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) 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);
> @@ -1245,80 +1264,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;
> }
--
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
