| Message ID | 20181113150621.22276-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vb2: Allow reqbufs(0) with "in use" MMAP buffers | expand |
Sorry, that should have said [PATCH v2]. regards Philipp
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 >
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
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
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 >
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
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
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 >
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
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
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.
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
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