Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-15 Thread Sakari Ailus
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 
> >>
> >> 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.ai...@iki.fi


Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-15 Thread Hans Verkuil
On 11/14/18 16:04, Philipp Zabel wrote:
> From: John Sheu 
> 
> 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.
> 
> 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 
> Reviewed-by: Pawel Osciak 
> Reviewed-by: Tomasz Figa 
> Signed-off-by: Tomasz Figa 
> [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> Signed-off-by: Philipp Zabel 
> Acked-by: Sakari Ailus 
> ---
> 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 d40c60e8..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.

I'd rephrase this:

Note that if any buffers are still mapped or exported via DMABUF, then
:ref:`VIDIOC_REQBUFS` can only succeed if the 
``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
capability is set. Otherwise :ref:`VIDIOC_REQBUFS` will return the ``EBUSY`` 
error code.

If ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS`` is set, then 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 `.
>  
>  
> @@ -132,6 +135,12 @@ any DMA in progress, an implicit
>  * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
>- 0x0008
>- This buffer type supports :ref:`requests `.
> +* - ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
> +  - 0x0010
> +  - The kernel allows calling :ref:`VIDIOC_REQBUFS` with a ``count`` 
> value
> +of zero while buffers are still mapped or exported via DMABUF. These

Not quite true. This isn't related to the count value, so just drop the
'with a ``count`` value of zero' part of the sentence.

Regards,

Hans

> +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 

Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-15 Thread Hans Verkuil
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 
>>
>> 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 
>> Reviewed-by: Pawel Osciak 
>> Reviewed-by: Tomasz Figa 
>> Signed-off-by: Tomasz Figa 
>> [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>>  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
>> Signed-off-by: Philipp Zabel 
>> Acked-by: Sakari Ailus 
>> ---
>> 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
>> d40c60e8..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 `.
>>
>> @@ -132,6 +135,12 @@ any DMA in progress, an implicit
>>  * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
>>- 0x0008
>>- This buffer type supports :ref:`requests `.
>> +* - ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
>> +  - 0x0010
>> +  - 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 - 

Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-14 Thread Hans Verkuil
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 
>>
>> 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 
>> Reviewed-by: Pawel Osciak 
>> Reviewed-by: Tomasz Figa 
>> Signed-off-by: Tomasz Figa 
>> [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>>  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
>> Signed-off-by: Philipp Zabel 
>> Acked-by: Sakari Ailus 
>> ---
>> 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
>> d40c60e8..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 `.
>>
>> @@ -132,6 +135,12 @@ any DMA in progress, an implicit
>>  * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
>>- 0x0008
>>- This buffer type supports :ref:`requests `.
>> +* - ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
>> +  - 0x0010
>> +  - 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 

Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-14 Thread Tomasz Figa
On Thu, Nov 15, 2018 at 4:59 AM 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 
> >
> > 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 
> > Reviewed-by: Pawel Osciak 
> > Reviewed-by: Tomasz Figa 
> > Signed-off-by: Tomasz Figa 
> > [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
> >  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> > Signed-off-by: Philipp Zabel 
> > Acked-by: Sakari Ailus 
> > ---
> > 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
> > d40c60e8..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 `.
> >
> > @@ -132,6 +135,12 @@ any DMA in progress, an implicit
> >  * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
> >- 0x0008
> >- This buffer type supports :ref:`requests `.
> > +* - ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
> > +  - 0x0010
> > +  - 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) }
> >  

Re: [PATCH v3] media: vb2: Allow reqbufs(0) with "in use" MMAP buffers

2018-11-14 Thread Laurent Pinchart
Hi Philipp,

Thank you for the patch.

On Wednesday, 14 November 2018 17:04:49 EET Philipp Zabel wrote:
> From: John Sheu 
> 
> 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 
> Reviewed-by: Pawel Osciak 
> Reviewed-by: Tomasz Figa 
> Signed-off-by: Tomasz Figa 
> [p.za...@pengutronix.de: moved __vb2_queue_cancel out of the mmap_lock
>  and added V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS]
> Signed-off-by: Philipp Zabel 
> Acked-by: Sakari Ailus 
> ---
> 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
> d40c60e8..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 `.
> 
> @@ -132,6 +135,12 @@ any DMA in progress, an implicit
>  * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
>- 0x0008
>- This buffer type supports :ref:`requests `.
> +* - ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS``
> +  - 0x0010
> +  - 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;
> -