On 11/14/2018 08:59 PM, Laurent Pinchart wrote:
> Hi Philipp,
>
> Thank you for the patch.
>
> On Wednesday, 14 November 2018 17:04:49 EET Philipp Zabel wrote:
>> From: John Sheu <[email protected]>
>>
>> Videobuf2 presently does not allow VIDIOC_REQBUFS to destroy outstanding
>> buffers if the queue is of type V4L2_MEMORY_MMAP, and if the buffers are
>> considered "in use". This is different behavior than for other memory
>> types and prevents us from deallocating buffers in following two cases:
>>
>> 1) There are outstanding mmap()ed views on the buffer. However even if
>> we put the buffer in reqbufs(0), there will be remaining references,
>> due to vma .open/close() adjusting vb2 buffer refcount appropriately.
>> This means that the buffer will be in fact freed only when the last
>> mmap()ed view is unmapped.
>
> While I agree that we should remove this restriction, it has helped me in the
> past to find missing munmap() in userspace. This patch thus has the potential
> of causing memory leaks in userspace. Is there a way we could assist
> application developers with this ?
>
>> 2) Buffer has been exported as a DMABUF. Refcount of the vb2 buffer
>> is managed properly by VB2 DMABUF ops, i.e. incremented on DMABUF
>> get and decremented on DMABUF release. This means that the buffer
>> will be alive until all importers release it.
>>
>> Considering both cases above, there does not seem to be any need to
>> prevent reqbufs(0) operation, because buffer lifetime is already
>> properly managed by both mmap() and DMABUF code paths. Let's remove it
>> and allow userspace freeing the queue (and potentially allocating a new
>> one) even though old buffers might be still in processing.
>>
>> To let userspace know that the kernel now supports orphaning buffers
>> that are still in use, add a new V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
>> to be set by reqbufs and create_bufs.
>>
>> Signed-off-by: John Sheu <[email protected]>
>> Reviewed-by: Pawel Osciak <[email protected]>
>> Reviewed-by: Tomasz Figa <[email protected]>
>> Signed-off-by: Tomasz Figa <[email protected]>
>> [[email protected]: moved __vb2_queue_cancel out of the mmap_lock
>> and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
>> Signed-off-by: Philipp Zabel <[email protected]>
>> Acked-by: Sakari Ailus <[email protected]>
>> ---
>> Changes since v2:
>> - Added documentation for V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS
>> ---
>> .../media/uapi/v4l/vidioc-reqbufs.rst | 15 ++++++++---
>> .../media/common/videobuf2/videobuf2-core.c | 26 +------------------
>> .../media/common/videobuf2/videobuf2-v4l2.c | 2 +-
>> include/uapi/linux/videodev2.h | 1 +
>> 4 files changed, 15 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>> b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst index
>> d4bbbb0c60e8..d53006b938ac 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>> @@ -59,9 +59,12 @@ When the I/O method is not supported the ioctl returns an
>> ``EINVAL`` error code.
>>
>> Applications can call :ref:`VIDIOC_REQBUFS` again to change the number of
>> -buffers, however this cannot succeed when any buffers are still mapped.
>> -A ``count`` value of zero frees all buffers, after aborting or finishing
>> -any DMA in progress, an implicit
>> +buffers. Note that if any buffers are still mapped or exported via DMABUF,
>> +this can only succeed if the ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS`` flag
>> +is set. In that case these buffers are orphaned and will be freed when they
>> +are unmapped or when the exported DMABUF fds are closed.
>> +A ``count`` value of zero frees or orphans all buffers, after aborting or
>> +finishing any DMA in progress, an implicit
>>
>> :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>`.
>>
>> @@ -132,6 +135,12 @@ any DMA in progress, an implicit
>> * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
>> - 0x00000008
>> - This buffer type supports :ref:`requests <media-request-api>`.
>> + * - ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
>> + - 0x00000010
>> + - The kernel allows calling :ref:`VIDIOC_REQBUFS` with a ``count``
>> value + of zero while buffers are still mapped or exported via
>> DMABUF. These + orphaned buffers will be freed when they are
>> unmapped or when the + exported DMABUF fds are closed.
>>
>> Return Value
>> ============
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c index
>> 975ff5669f72..608459450c1e 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -553,20 +553,6 @@ bool vb2_buffer_in_use(struct vb2_queue *q, struct
>> vb2_buffer *vb) }
>> EXPORT_SYMBOL(vb2_buffer_in_use);
>>
>> -/*
>> - * __buffers_in_use() - return true if any buffers on the queue are in use
>> and - * the queue cannot be freed (by the means of REQBUFS(0)) call
>> - */
>> -static bool __buffers_in_use(struct vb2_queue *q)
>> -{
>> - unsigned int buffer;
>> - for (buffer = 0; buffer < q->num_buffers; ++buffer) {
>> - if (vb2_buffer_in_use(q, q->bufs[buffer]))
>> - return true;
>> - }
>> - return false;
>> -}
>> -
>> void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
>> {
>> call_void_bufop(q, fill_user_buffer, q->bufs[index], pb);
>> @@ -674,23 +660,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
>> vb2_memory memory,
>>
>> if (*count == 0 || q->num_buffers != 0 ||
>> (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
>> - /*
>> - * We already have buffers allocated, so first check if they
>> - * are not in use and can be freed.
>> - */
>> - mutex_lock(&q->mmap_lock);
>> - if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
>> - mutex_unlock(&q->mmap_lock);
>> - dprintk(1, "memory in use, cannot free\n");
>> - return -EBUSY;
>> - }
>> -
>> /*
>> * Call queue_cancel to clean up any buffers in the
>> * QUEUED state which is possible if buffers were prepared or
>> * queued without ever calling STREAMON.
>> */
>> __vb2_queue_cancel(q);
>> + mutex_lock(&q->mmap_lock);
>
> This results in __vb2_queue_cancel() called without the mmap_lock held. Is
> that OK ?
>
>> ret = __vb2_queue_free(q, q->num_buffers);
It is OK, it is __vb2_queue_free that needs the lock, not __vb2_queue_cancel.
The reason the lock was originally before the __vb2_queue_cancel call was
due to the __buffers_in_use() call.
Regards,
Hans
>> mutex_unlock(&q->mmap_lock);
>> if (ret)
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> b/drivers/media/common/videobuf2/videobuf2-v4l2.c index
>> a17033ab2c22..f02d452ceeb9 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -624,7 +624,7 @@ EXPORT_SYMBOL(vb2_querybuf);
>>
>> static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
>> {
>> - *caps = 0;
>> + *caps = V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS;
>> if (q->io_modes & VB2_MMAP)
>> *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
>> if (q->io_modes & VB2_USERPTR)
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index c8e8ff810190..2a223835214c 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -879,6 +879,7 @@ struct v4l2_requestbuffers {
>> #define V4L2_BUF_CAP_SUPPORTS_USERPTR (1 << 1)
>> #define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
>> #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
>> +#define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
>>
>> /**
>> * struct v4l2_plane - plane info for multi-planar buffers
>
