* Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers
2018-11-14 19:59 ` Laurent Pinchart
@ 2018-11-15 3:01 ` Tomasz Figa
2018-11-15 7:35 ` Hans Verkuil
2018-11-15 10:29 ` Hans Verkuil
2 siblings, 0 replies; 7+ messages in thread
From: Tomasz Figa @ 2018-11-15 3:01 UTC (permalink / raw)
To: Laurent Pinchart, Philipp Zabel
Cc: Linux Media Mailing List, Hans Verkuil, Mauro Carvalho Chehab,
nicolas, Sakari Ailus
On Thu, Nov 15, 2018 at 4:59 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Philipp,
>
> Thank you for the patch.
>
> On Wednesday, 14 November 2018 17:04:49 EET Philipp Zabel wrote:
> > From: John Sheu <sheu@chromium.org>
> >
> > 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 ?
>
I'm not very convinced that it's something we need to have, but
possibly one could have it as a settable capability, that's given to
REQBUF/CREATE_BUFS at allocation (count > 0) time?
> > 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 <sheu@chromium.org>
> > Reviewed-by: Pawel Osciak <posciak@chromium.org>
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> > [p.zabel@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
> > and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > 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 ?
Looks like a rebase error. The original patch didn't move the mutex_lock() call.
Anyway, thanks a lot Philipp for reviving it!
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers
2018-11-14 19:59 ` Laurent Pinchart
2018-11-15 3:01 ` Tomasz Figa
@ 2018-11-15 7:35 ` Hans Verkuil
2018-11-15 10:29 ` Hans Verkuil
2 siblings, 0 replies; 7+ messages in thread
From: Hans Verkuil @ 2018-11-15 7:35 UTC (permalink / raw)
To: Laurent Pinchart, Philipp Zabel
Cc: linux-media, Hans Verkuil, Mauro Carvalho Chehab, Tomasz Figa,
Nicolas Dufresne, Sakari Ailus
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 <sheu@chromium.org>
>>
>> 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 <sheu@chromium.org>
>> Reviewed-by: Pawel Osciak <posciak@chromium.org>
>> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> [p.zabel@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>> and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers
2018-11-14 19:59 ` Laurent Pinchart
2018-11-15 3:01 ` Tomasz Figa
2018-11-15 7:35 ` Hans Verkuil
@ 2018-11-15 10:29 ` Hans Verkuil
2018-11-15 11:40 ` Sakari Ailus
2 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2018-11-15 10:29 UTC (permalink / raw)
To: Laurent Pinchart, Philipp Zabel
Cc: linux-media, Hans Verkuil, Mauro Carvalho Chehab, Tomasz Figa,
Nicolas Dufresne, Sakari Ailus
On 11/14/18 20:59, 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 <sheu@chromium.org>
>>
>> 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 ?
Should we just keep the debug message? (rephrased of course)
That way you can enable debugging and see that this happens.
It sounds reasonable to me.
Regards,
Hans
>
>> 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 <sheu@chromium.org>
>> Reviewed-by: Pawel Osciak <posciak@chromium.org>
>> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> [p.zabel@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>> and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>> 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);
>> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers
2018-11-15 10:29 ` Hans Verkuil
@ 2018-11-15 11:40 ` Sakari Ailus
0 siblings, 0 replies; 7+ messages in thread
From: Sakari Ailus @ 2018-11-15 11:40 UTC (permalink / raw)
To: Hans Verkuil
Cc: Laurent Pinchart, Philipp Zabel, linux-media, Hans Verkuil,
Mauro Carvalho Chehab, Tomasz Figa, Nicolas Dufresne
On Thu, Nov 15, 2018 at 11:29:35AM +0100, Hans Verkuil wrote:
> On 11/14/18 20:59, 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 <sheu@chromium.org>
> >>
> >> 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 ?
>
> Should we just keep the debug message? (rephrased of course)
>
> That way you can enable debugging and see that this happens.
>
> It sounds reasonable to me.
Makes sense IMO.
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 7+ messages in thread