Hi Hans,
On Wednesday 06 August 2014 08:44:27 Hans Verkuil wrote:
> On 08/06/2014 12:15 AM, Laurent Pinchart wrote:
> > On Tuesday 05 August 2014 12:56:14 Hans Verkuil wrote:
> >> Changes since v2: use a mutex instead of a spinlock due to possible
> >> sleeps inside the mmap memop. Use the mutex when buffers are
> >> allocated/freed, so it's a much coarser lock.
> >>
> >> The following lockdep warning has been there ever since commit
> >
> >> a517cca6b24fc54ac209e44118ec8962051662e3 one year ago:
> > [snip]
> >
> >> The reason is that vb2_fop_mmap and vb2_fop_get_unmapped_area take the
> >> core lock while they are called with the mmap_sem semaphore held. But
> >> elsewhere in the code the core lock is taken first but calls to
> >> copy_to/from_user() can take the mmap_sem semaphore as well, potentially
> >> causing a classical A-B/B-A deadlock.
> >>
> >> However, the mmap/get_unmapped_area calls really shouldn't take the core
> >> lock at all. So what would happen if they don't take the core lock
> >> anymore?
> >>
> >> There are two situations that need to be taken into account: calling mmap
> >> while new buffers are being added and calling mmap while buffers are
> >> being deleted.
> >>
> >> The first case works almost fine without a lock: in all cases mmap relies
> >> on correctly filled-in q->num_buffers/q->num_planes values and those are
> >> only updated by reqbufs and create_buffers *after* any new buffers have
> >> been initialized completely. Except in one case: if an error occurred
> >> while allocating the buffers it will increase num_buffers and rely on
> >> __vb2_queue_free to decrease it again. So there is a short period where
> >> the buffer information may be wrong.
> >>
> >> The second case definitely does pose a problem: buffers may be in the
> >> process of being deleted, without the internal structure being updated.
> >>
> >> In order to fix this a new mutex is added to vb2_queue that is taken when
> >> buffers are allocated or deleted, and in vb2_mmap. That way vb2_mmap
> >> won't get stale buffer data. Note that this is a problem only for
> >> MEMORY_MMAP, so even though __qbuf_userptr and __qbuf_dmabuf also mess
> >> around with buffers (mem_priv in particular), this doesn't clash with
> >> vb2_mmap or vb2_get_unmapped_area since those are MMAP specific.
> >>
> >> As an additional bonus the hack in __buf_prepare, the USERPTR case, can
> >> be removed as well since mmap() no longer takes the core lock.
> >>
> >> All in all a much cleaner solution.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >
> > Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >
> > with one small comment, please see below.
> >
> >> ---
> >>
> >> drivers/media/v4l2-core/videobuf2-core.c | 56 +++++++++-----------------
> >> include/media/videobuf2-core.h | 2 ++
> >> 2 files changed, 21 insertions(+), 37 deletions(-)
> >
> > [snip]
> >
> >> diff --git a/include/media/videobuf2-core.h
> >> b/include/media/videobuf2-core.h index fc910a6..ae1289b 100644
> >> --- a/include/media/videobuf2-core.h
> >> +++ b/include/media/videobuf2-core.h
> >> @@ -366,6 +366,7 @@ struct v4l2_fh;
> >> * cannot be started unless at least this number of buffers
> >> * have been queued into the driver.
> >> *
> >> + * @queue_lock: private mutex used when buffers are
> >> allocated/freed/mmapped
> >
> > queue_lock sounds a bit too generic to me, it might confuse readers into
> > thinking the lock protects the whole queue.
>
> How about mmap_lock?
That sounds better.
> >> * @memory: current memory type used
> >> * @bufs: videobuf buffer structures
> >> * @num_buffers: number of allocated/used buffers
> >> @@ -399,6 +400,7 @@ struct vb2_queue {
> >> u32 min_buffers_needed;
> >>
> >> /* private: internal use only */
> >> + struct mutex queue_lock;
> >> enum v4l2_memory memory;
> >> struct vb2_buffer *bufs[VIDEO_MAX_FRAME];
> >> unsigned int num_buffers;
--
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
