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

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

[Philipp Zabel]

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.

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>
---
 .../media/common/videobuf2/videobuf2-core.c   | 26 +------------------
 .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
 include/uapi/linux/videodev2.h                |  1 +
 3 files changed, 3 insertions(+), 26 deletions(-)

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);
 		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
-- 
2.19.1

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

[Philipp Zabel]

Sorry, that should have said [PATCH v2].

regards
Philipp

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

[Hans Verkuil]

Hi Philipp,

On 11/13/18 16:06, 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.
> 
> 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.

Looks good, but I have some questions:

1) does v4l2-compliance together with vivid (easiest to test) still work?
   I don't think I have a proper test for this in v4l2-compliance, but
   I'm not 100% certain. If it fails with this patch, then please provide
   a fix for v4l2-compliance as well.

2) I would like to see a new test in v4l2-compliance for this: i.e. if
   the capability is set, then check that you can call REQBUFS(0) before
   unmapping all buffers. Ditto with dmabuffers.

I said during the media summit that I wanted to be more strict about
requiring compliance tests before adding new features, so you're the
unlucky victim of that :-)

Look for munmap_bufs in v4l2-test-buffers.cpp (the MMAP case). The dmabuf
tests are a bit trickier since I noticed that I never actually close
the dmabuf fds in v4l2-compliance. This will fix that:

diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index c59a56d9..03639301 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1201,6 +1201,7 @@ int testDmaBuf(struct node *expbuf_node, struct node *node, unsigned frame_count
 		fail_on_test(captureBufs(node, q, m2m_q, frame_count, true));
 		fail_on_test(node->streamoff(q.g_type()));
 		fail_on_test(node->streamoff(q.g_type()));
+		exp_q.close_exported_fds();
 	}
 	return 0;
 }

What is also missing in testDmaBuf is calling q.reqbufs(node, 0) to free all
buffers, and I never munmap the buffers by calling q.munmap_bufs(node);

In other words, clearly I never wrote proper tests for the behavior of mmap()/dmabuf
and REQBUFS(0).

Regards,

	Hans

> 
> 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>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 26 +------------------
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
>  include/uapi/linux/videodev2.h                |  1 +
>  3 files changed, 3 insertions(+), 26 deletions(-)
> 
> 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);
>  		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
> 

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

[Philipp Zabel]

Hi Hans,

On Tue, 2018-11-13 at 16:43 +0100, Hans Verkuil wrote:
> Hi Philipp,
> 
> On 11/13/18 16:06, 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.
> > 
> > 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.
> 
> Looks good, but I have some questions:
> 
> 1) does v4l2-compliance together with vivid (easiest to test) still work?
>    I don't think I have a proper test for this in v4l2-compliance, but
>    I'm not 100% certain. If it fails with this patch, then please provide
>    a fix for v4l2-compliance as well.

I have tested on v4.20-rc2 with 92539d3eda2c ("media: v4l: event: Add
subscription to list before calling "add" operation") and this patch
applied:

$ modprobe vivid no_error_inj=1
vivid-000: V4L2 capture device registered as video15
vivid-000: V4L2 output device registered as video16

$ v4l2-compliance -d 15 -s 1 --expbuf-device 16
v4l2-compliance SHA: 98b4c9f276a18535b5691e5f350f59ffbf5a9aa5, 32 bits
...
Total: 112, Succeeded: 112, Failed: 0, Warnings: 4

The warnings are:
		warn: v4l2-test-formats.cpp(1426): doioctl(node, VIDIOC_CROPCAP, &cap)
	test Cropping: OK
(one per input) and:
		warn: v4l2-test-controls.cpp(845): V4L2_CID_DV_RX_POWER_PRESENT not found for input 3
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK

> 2) I would like to see a new test in v4l2-compliance for this: i.e. if
>    the capability is set, then check that you can call REQBUFS(0) before
>    unmapping all buffers. Ditto with dmabuffers.
>
> I said during the media summit that I wanted to be more strict about
> requiring compliance tests before adding new features, so you're the
> unlucky victim of that :-)

That's fair. The SHA above is actually a lie, I had one patch applied.

regards
Philipp

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

[Hans Verkuil]

On 11/14/18 15:43, Philipp Zabel wrote:
> Hi Hans,
> 
> On Tue, 2018-11-13 at 16:43 +0100, Hans Verkuil wrote:
>> Hi Philipp,
>>
>> On 11/13/18 16:06, 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.
>>>
>>> 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.
>>
>> Looks good, but I have some questions:
>>
>> 1) does v4l2-compliance together with vivid (easiest to test) still work?
>>    I don't think I have a proper test for this in v4l2-compliance, but
>>    I'm not 100% certain. If it fails with this patch, then please provide
>>    a fix for v4l2-compliance as well.
> 
> I have tested on v4.20-rc2 with 92539d3eda2c ("media: v4l: event: Add
> subscription to list before calling "add" operation") and this patch
> applied:
> 
> $ modprobe vivid no_error_inj=1
> vivid-000: V4L2 capture device registered as video15
> vivid-000: V4L2 output device registered as video16
> 
> $ v4l2-compliance -d 15 -s 1 --expbuf-device 16
> v4l2-compliance SHA: 98b4c9f276a18535b5691e5f350f59ffbf5a9aa5, 32 bits
> ...
> Total: 112, Succeeded: 112, Failed: 0, Warnings: 4
> 
> The warnings are:
> 		warn: v4l2-test-formats.cpp(1426): doioctl(node, VIDIOC_CROPCAP, &cap)

Will be fixed when https://patchwork.linuxtv.org/patch/52811/ is merged.

> 	test Cropping: OK
> (one per input) and:
> 		warn: v4l2-test-controls.cpp(845): V4L2_CID_DV_RX_POWER_PRESENT not found for input 3

Waiting for a volunteer to teach vivid to properly emulate an HDMI connector.

So this looks good.

Will look at the v4l2-compliance patch tomorrow.

Regards,

	Hans

> 	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> 
>> 2) I would like to see a new test in v4l2-compliance for this: i.e. if
>>    the capability is set, then check that you can call REQBUFS(0) before
>>    unmapping all buffers. Ditto with dmabuffers.
>>
>> I said during the media summit that I wanted to be more strict about
>> requiring compliance tests before adding new features, so you're the
>> unlucky victim of that :-)
> 
> That's fair. The SHA above is actually a lie, I had one patch applied.
> 
> regards
> Philipp
> 

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

[Laurent Pinchart]

Hi Hans,

On Tuesday, 13 November 2018 17:43:48 EET Hans Verkuil wrote:
> On 11/13/18 16:06, 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.
> > 
> > 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.
> 
> Looks good, but I have some questions:
> 
> 1) does v4l2-compliance together with vivid (easiest to test) still work?
>    I don't think I have a proper test for this in v4l2-compliance, but
>    I'm not 100% certain. If it fails with this patch, then please provide
>    a fix for v4l2-compliance as well.
> 
> 2) I would like to see a new test in v4l2-compliance for this: i.e. if
>    the capability is set, then check that you can call REQBUFS(0) before
>    unmapping all buffers. Ditto with dmabuffers.
> 
> I said during the media summit that I wanted to be more strict about
> requiring compliance tests before adding new features, so you're the
> unlucky victim of that :-)

Do you have plans to refactor and document the v4l2-compliance internals to 
make it easier ?

> Look for munmap_bufs in v4l2-test-buffers.cpp (the MMAP case). The dmabuf
> tests are a bit trickier since I noticed that I never actually close
> the dmabuf fds in v4l2-compliance. This will fix that:
> 
> diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> b/utils/v4l2-compliance/v4l2-test-buffers.cpp index c59a56d9..03639301
> 100644
> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -1201,6 +1201,7 @@ int testDmaBuf(struct node *expbuf_node, struct node
> *node, unsigned frame_count fail_on_test(captureBufs(node, q, m2m_q,
> frame_count, true));
>  		fail_on_test(node->streamoff(q.g_type()));
>  		fail_on_test(node->streamoff(q.g_type()));
> +		exp_q.close_exported_fds();
>  	}
>  	return 0;
>  }
> 
> What is also missing in testDmaBuf is calling q.reqbufs(node, 0) to free all
> buffers, and I never munmap the buffers by calling q.munmap_bufs(node);
> 
> In other words, clearly I never wrote proper tests for the behavior of
> mmap()/dmabuf and REQBUFS(0).
> 
> Regards,
> 
> 	Hans
> 
> > 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>
> > ---
> > 
> >  .../media/common/videobuf2/videobuf2-core.c   | 26 +------------------
> >  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
> >  include/uapi/linux/videodev2.h                |  1 +
> >  3 files changed, 3 insertions(+), 26 deletions(-)
> > 
> > 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);
> > 
> >  		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


-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

On 11/14/2018 08:52 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday, 13 November 2018 17:43:48 EET Hans Verkuil wrote:
>> On 11/13/18 16:06, 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.
>>>
>>> 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.
>>
>> Looks good, but I have some questions:
>>
>> 1) does v4l2-compliance together with vivid (easiest to test) still work?
>>    I don't think I have a proper test for this in v4l2-compliance, but
>>    I'm not 100% certain. If it fails with this patch, then please provide
>>    a fix for v4l2-compliance as well.
>>
>> 2) I would like to see a new test in v4l2-compliance for this: i.e. if
>>    the capability is set, then check that you can call REQBUFS(0) before
>>    unmapping all buffers. Ditto with dmabuffers.
>>
>> I said during the media summit that I wanted to be more strict about
>> requiring compliance tests before adding new features, so you're the
>> unlucky victim of that :-)
> 
> Do you have plans to refactor and document the v4l2-compliance internals to 
> make it easier ?

Yes. I hope to be able to set aside one or two days for that in the next two
weeks.

Regards,

	Hans

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

[Laurent Pinchart]

Hi Hans,


On Thursday, 15 November 2018 09:30:55 EET Hans Verkuil wrote:
> On 11/14/2018 08:52 PM, Laurent Pinchart wrote:
> > On Tuesday, 13 November 2018 17:43:48 EET Hans Verkuil wrote:
> >> On 11/13/18 16:06, 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.
> >>> 
> >>> 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.
> >> 
> >> Looks good, but I have some questions:
> >> 
> >> 1) does v4l2-compliance together with vivid (easiest to test) still work?
> >>    I don't think I have a proper test for this in v4l2-compliance, but
> >>    I'm not 100% certain. If it fails with this patch, then please provide
> >>    a fix for v4l2-compliance as well.
> >> 
> >> 2) I would like to see a new test in v4l2-compliance for this: i.e. if
> >>    the capability is set, then check that you can call REQBUFS(0) before
> >>    unmapping all buffers. Ditto with dmabuffers.
> >> 
> >> I said during the media summit that I wanted to be more strict about
> >> requiring compliance tests before adding new features, so you're the
> >> unlucky victim of that :-)
> > 
> > Do you have plans to refactor and document the v4l2-compliance internals
> > to make it easier ?
> 
> Yes. I hope to be able to set aside one or two days for that in the next two
> weeks.

That would be great ! Let me know if you would like to discuss how the code 
base could be refactored.

-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

On 11/15/2018 08:49 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> 
> On Thursday, 15 November 2018 09:30:55 EET Hans Verkuil wrote:
>> On 11/14/2018 08:52 PM, Laurent Pinchart wrote:
>>> On Tuesday, 13 November 2018 17:43:48 EET Hans Verkuil wrote:
>>>> On 11/13/18 16:06, 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.
>>>>>
>>>>> 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.
>>>>
>>>> Looks good, but I have some questions:
>>>>
>>>> 1) does v4l2-compliance together with vivid (easiest to test) still work?
>>>>    I don't think I have a proper test for this in v4l2-compliance, but
>>>>    I'm not 100% certain. If it fails with this patch, then please provide
>>>>    a fix for v4l2-compliance as well.
>>>>
>>>> 2) I would like to see a new test in v4l2-compliance for this: i.e. if
>>>>    the capability is set, then check that you can call REQBUFS(0) before
>>>>    unmapping all buffers. Ditto with dmabuffers.
>>>>
>>>> I said during the media summit that I wanted to be more strict about
>>>> requiring compliance tests before adding new features, so you're the
>>>> unlucky victim of that :-)
>>>
>>> Do you have plans to refactor and document the v4l2-compliance internals
>>> to make it easier ?
>>
>> Yes. I hope to be able to set aside one or two days for that in the next two
>> weeks.
> 
> That would be great ! Let me know if you would like to discuss how the code 
> base could be refactored.
> 

I'm happy to hear your ideas, but I only have one day for refactoring, so it is
very limited what can be done.

Frankly, it is mainly v4l2-test-buffers.cpp that I would like to split up in
two parts: one for the tests that do not require actual streaming and one part
for all the streaming tests.

Documentation (esp. cv4l-helpers.h which is used a lot) is much more important
and I wanted to spend the second day on that.

Regards,

	Hans

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

[Sakari Ailus]

Hi Philipp,

On Tue, Nov 13, 2018 at 04:06:21PM +0100, 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.
> 
> 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>

This lets the user to allocate lots of mmap'ed buffers that are pinned in
physical memory. Considering that we don't really have a proper mechanism
to limit that anyway,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

That said, the patch must be accompanied by the documentation change in
Documentation/media/uapi/v4l/vidioc-reqbufs.rst .

I wonder what Hans thinks.

> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 26 +------------------
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
>  include/uapi/linux/videodev2.h                |  1 +
>  3 files changed, 3 insertions(+), 26 deletions(-)
> 
> 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);
>  		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

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Nicolas Dufresne]

Le mercredi 14 novembre 2018 à 00:27 +0200, Sakari Ailus a écrit :
> Hi Philipp,
> 
> On Tue, Nov 13, 2018 at 04:06:21PM +0100, 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.
> > 
> > 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>
> 
> This lets the user to allocate lots of mmap'ed buffers that are pinned in
> physical memory. Considering that we don't really have a proper mechanism
> to limit that anyway,

It's currently limited to 32 buffers. It's not worst then DRM dumb
buffers which will let you allocate as much as you want.

> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> That said, the patch must be accompanied by the documentation change in
> Documentation/media/uapi/v4l/vidioc-reqbufs.rst .
> 
> I wonder what Hans thinks.
> 
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   | 26 +------------------
> >  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
> >  include/uapi/linux/videodev2.h                |  1 +
> >  3 files changed, 3 insertions(+), 26 deletions(-)
> > 
> > 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);
> >  		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

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

[Tomasz Figa]

On Wed, Nov 14, 2018 at 8:51 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le mercredi 14 novembre 2018 à 00:27 +0200, Sakari Ailus a écrit :
> > Hi Philipp,
> >
> > On Tue, Nov 13, 2018 at 04:06:21PM +0100, 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.
> > >
> > > 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>
> >
> > This lets the user to allocate lots of mmap'ed buffers that are pinned in
> > physical memory. Considering that we don't really have a proper mechanism
> > to limit that anyway,
>
> It's currently limited to 32 buffers. It's not worst then DRM dumb
> buffers which will let you allocate as much as you want.
>

32 buffers for one mem2mem instance. One can open many of those and
allocate more anyway.

I think it's a part of the global problem of DMA memory not being
accounted to the process allocating...

Best regards,
Tomasz

> >
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > That said, the patch must be accompanied by the documentation change in
> > Documentation/media/uapi/v4l/vidioc-reqbufs.rst .
> >
> > I wonder what Hans thinks.
> >
> > > ---
> > >  .../media/common/videobuf2/videobuf2-core.c   | 26 +------------------
> > >  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 +-
> > >  include/uapi/linux/videodev2.h                |  1 +
> > >  3 files changed, 3 insertions(+), 26 deletions(-)
> > >
> > > 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);
> > >             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
>

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

[Philipp Zabel]

Hi Sakari,

On Wed, 2018-11-14 at 00:27 +0200, Sakari Ailus wrote:
[...]
> This lets the user to allocate lots of mmap'ed buffers that are pinned in
> physical memory.

This is already possible without this patch, by closing the fd instead
of calling reqbufs(0).

> Considering that we don't really have a proper mechanism
> to limit that anyway,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> That said, the patch must be accompanied by the documentation change in
> Documentation/media/uapi/v4l/vidioc-reqbufs.rst .

Oh right, thanks. I'll add V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS to
_v4l2-buf-capabilities in v2.

regards
Philipp

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

[Tomasz Figa]

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.

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.

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>
---
 drivers/media/v4l2-core/videobuf2-core.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 4f59b7e..931c5de 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -636,19 +636,6 @@ static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
 	return false;
 }
 
-/**
- * __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 (__buffer_in_use(q, q->bufs[buffer]))
-			return true;
-	}
-	return false;
-}
 
 /**
  * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
@@ -884,16 +871,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	}
 
 	if (req->count == 0 || q->num_buffers != 0 || q->memory != req->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 == V4L2_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 PREPARED or
-- 
2.6.0.rc2.230.g3dd15c0

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

[Hans Verkuil]

Hi Tomasz,

I think this makes sense. After all, just closing the filehandle has the
same effect. However, I would like to see some documentation improvements
to go along with this vb2 patch.

1) The VIDIOC_REQBUFS ioctl documentation needs to be extended so it
describes that it is OK to do this (release all buffers even if still mapped),
and mention that the memory remains in use unless userspace unmaps. This to
prevent users from thinking REQBUFS(X) will somehow unmap automatically.

2) Here http://hverkuil.home.xs4all.nl/spec/media.html#mmap the spec mentions
that "This ioctl can also be used to change the number of buffers or to free
the allocated memory, provided none of the buffers are still mapped.". This
needs to be updated.

After making this change, does v4l2-compliance still work? I don't think I
have a test there that checks if reqbufs(0) fails if buffers are still
mmapped, but I'm not 100% certain either.

Regards,

	Hans

On 10/15/15 11:05, Tomasz Figa 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.
> 
> 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.
> 
> 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>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 22 ----------------------
>  1 file changed, 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 4f59b7e..931c5de 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -636,19 +636,6 @@ static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
>  	return false;
>  }
>  
> -/**
> - * __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 (__buffer_in_use(q, q->bufs[buffer]))
> -			return true;
> -	}
> -	return false;
> -}
>  
>  /**
>   * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
> @@ -884,16 +871,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
>  	}
>  
>  	if (req->count == 0 || q->num_buffers != 0 || q->memory != req->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 == V4L2_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 PREPARED or
>