| Message ID | 20181114143833.19267-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4l-utils] v4l2-compliance: test orphaned buffer support | expand |
On 11/14/18 15:38, Philipp Zabel wrote: > Test that V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS is reported equally for > both MMAP and DMABUF memory types. If supported, try to orphan buffers > by calling reqbufs(0) before unmapping or closing DMABUF fds. > > Also close exported DMABUF fds and free buffers in testDmaBuf if > orphaned buffers are not supported. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > contrib/freebsd/include/linux/videodev2.h | 1 + > include/linux/videodev2.h | 1 + > utils/common/v4l2-info.cpp | 1 + > utils/v4l2-compliance/v4l2-compliance.h | 1 + > utils/v4l2-compliance/v4l2-test-buffers.cpp | 35 +++++++++++++++++---- > 5 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/contrib/freebsd/include/linux/videodev2.h b/contrib/freebsd/include/linux/videodev2.h > index 9928c00e4b68..33153b53c175 100644 > --- a/contrib/freebsd/include/linux/videodev2.h > +++ b/contrib/freebsd/include/linux/videodev2.h > @@ -907,6 +907,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 > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h > index 79418cd39480..a39300cacb6a 100644 > --- a/include/linux/videodev2.h > +++ b/include/linux/videodev2.h > @@ -873,6 +873,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 > diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp > index 258e5446f030..3699c35cb9d6 100644 > --- a/utils/common/v4l2-info.cpp > +++ b/utils/common/v4l2-info.cpp > @@ -200,6 +200,7 @@ static const flag_def bufcap_def[] = { > { V4L2_BUF_CAP_SUPPORTS_USERPTR, "userptr" }, > { V4L2_BUF_CAP_SUPPORTS_DMABUF, "dmabuf" }, > { V4L2_BUF_CAP_SUPPORTS_REQUESTS, "requests" }, > + { V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS, "orphaned-bufs" }, > { 0, NULL } > }; > > diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h > index def185f17261..88ec260a9bcc 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.h > +++ b/utils/v4l2-compliance/v4l2-compliance.h > @@ -119,6 +119,7 @@ struct base_node { > __u32 valid_buftypes; > __u32 valid_buftype; > __u32 valid_memorytype; > + bool has_orphaned_bufs; I'd rename that to supports_orphaned_bufs. > }; > > struct node : public base_node, public cv4l_fd { > diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp > index c59a56d9ced7..6174015cb4e7 100644 > --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp > +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp > @@ -400,8 +400,11 @@ int testReqBufs(struct node *node) > mmap_valid = !ret; > if (mmap_valid) > caps = q.g_capabilities(); > - if (caps) > + if (caps) { > fail_on_test(mmap_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_MMAP)); > + if (caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS) > + node->has_orphaned_bufs = true; > + } > > q.init(i, V4L2_MEMORY_USERPTR); > ret = q.reqbufs(node, 0); > @@ -418,8 +421,11 @@ int testReqBufs(struct node *node) > fail_on_test(!mmap_valid && dmabuf_valid); > // Note: dmabuf is only supported with vb2, so we can assume a > // non-0 caps value if dmabuf is supported. > - if (caps || dmabuf_valid) > + if (caps || dmabuf_valid) { > fail_on_test(dmabuf_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_DMABUF)); > + if (node->has_orphaned_bufs) > + fail_on_test(userptr_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS)); Huh? I'm not sure what you are testing here. > + } > > fail_on_test((can_stream && !is_overlay) && !mmap_valid && !userptr_valid && !dmabuf_valid); > fail_on_test((!can_stream || is_overlay) && (mmap_valid || userptr_valid || dmabuf_valid)); > @@ -967,12 +973,22 @@ int testMmap(struct node *node, unsigned frame_count) The setupM2M function should check if m2m_q also sets the V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS. I.e. this capability for m2m_q must match node->has_orphaned_bufs. It makes no sense if it is set for the capture queue but not the output queue for m2m devices. And since this has to be set manually in the drivers (at least for now), this needs to be checked by v4l2-compliance. > 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())); > - q.munmap_bufs(node); > - fail_on_test(q.reqbufs(node, 0)); > + if (node->has_orphaned_bufs) { > + fail_on_test(q.reqbufs(node, 0)); > + q.munmap_bufs(node); > + } else { > + q.munmap_bufs(node); > + fail_on_test(q.reqbufs(node, 0)); This 'else' can be improved: } else if (!q.reqbufs(node, 0)) { // It's either a bug or this driver should set // V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS warn("Can free buffers even if still mmap()ed\n"); q.munmap_bufs(node); } else { q.munmap_bufs(node); fail_on_test(q.reqbufs(node, 0)); > + } > if (node->is_m2m) { > fail_on_test(node->streamoff(m2m_q.g_type())); > - m2m_q.munmap_bufs(node); > - fail_on_test(m2m_q.reqbufs(node, 0)); > + if (node->has_orphaned_bufs) { > + fail_on_test(m2m_q.reqbufs(node, 0)); > + m2m_q.munmap_bufs(node); > + } else { > + m2m_q.munmap_bufs(node); > + fail_on_test(m2m_q.reqbufs(node, 0)); > + } > } > } > return 0; > @@ -1201,6 +1217,13 @@ 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())); > + if (node->has_orphaned_bufs) { > + fail_on_test(q.reqbufs(node, 0)); > + exp_q.close_exported_fds(); > + } else { Something similar to the MMAP case should be done here as well. If nothing else, that checks that if V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS is *not* set, then q.reqbufs(node, 0) should fail. > + exp_q.close_exported_fds(); > + fail_on_test(q.reqbufs(node, 0)); > + } > } > return 0; > } > Regards, Hans
On Thu, 2018-11-15 at 11:21 +0100, Hans Verkuil wrote: > On 11/14/18 15:38, Philipp Zabel wrote: > > Test that V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS is reported equally for > > both MMAP and DMABUF memory types. If supported, try to orphan buffers > > by calling reqbufs(0) before unmapping or closing DMABUF fds. > > > > Also close exported DMABUF fds and free buffers in testDmaBuf if > > orphaned buffers are not supported. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > contrib/freebsd/include/linux/videodev2.h | 1 + > > include/linux/videodev2.h | 1 + > > utils/common/v4l2-info.cpp | 1 + > > utils/v4l2-compliance/v4l2-compliance.h | 1 + > > utils/v4l2-compliance/v4l2-test-buffers.cpp | 35 +++++++++++++++++---- > > 5 files changed, 33 insertions(+), 6 deletions(-) > > > > diff --git a/contrib/freebsd/include/linux/videodev2.h b/contrib/freebsd/include/linux/videodev2.h > > index 9928c00e4b68..33153b53c175 100644 > > --- a/contrib/freebsd/include/linux/videodev2.h > > +++ b/contrib/freebsd/include/linux/videodev2.h > > @@ -907,6 +907,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 > > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h > > index 79418cd39480..a39300cacb6a 100644 > > --- a/include/linux/videodev2.h > > +++ b/include/linux/videodev2.h > > @@ -873,6 +873,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 > > diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp > > index 258e5446f030..3699c35cb9d6 100644 > > --- a/utils/common/v4l2-info.cpp > > +++ b/utils/common/v4l2-info.cpp > > @@ -200,6 +200,7 @@ static const flag_def bufcap_def[] = { > > { V4L2_BUF_CAP_SUPPORTS_USERPTR, "userptr" }, > > { V4L2_BUF_CAP_SUPPORTS_DMABUF, "dmabuf" }, > > { V4L2_BUF_CAP_SUPPORTS_REQUESTS, "requests" }, > > + { V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS, "orphaned-bufs" }, > > { 0, NULL } > > }; > > > > diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h > > index def185f17261..88ec260a9bcc 100644 > > --- a/utils/v4l2-compliance/v4l2-compliance.h > > +++ b/utils/v4l2-compliance/v4l2-compliance.h > > @@ -119,6 +119,7 @@ struct base_node { > > __u32 valid_buftypes; > > __u32 valid_buftype; > > __u32 valid_memorytype; > > + bool has_orphaned_bufs; > > I'd rename that to supports_orphaned_bufs. Ok. > > }; > > > > struct node : public base_node, public cv4l_fd { > > diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp > > index c59a56d9ced7..6174015cb4e7 100644 > > --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp > > +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp > > @@ -400,8 +400,11 @@ int testReqBufs(struct node *node) > > mmap_valid = !ret; > > if (mmap_valid) > > caps = q.g_capabilities(); > > - if (caps) > > + if (caps) { > > fail_on_test(mmap_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_MMAP)); > > + if (caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS) > > + node->has_orphaned_bufs = true; > > + } > > > > q.init(i, V4L2_MEMORY_USERPTR); > > ret = q.reqbufs(node, 0); > > @@ -418,8 +421,11 @@ int testReqBufs(struct node *node) > > fail_on_test(!mmap_valid && dmabuf_valid); > > // Note: dmabuf is only supported with vb2, so we can assume a > > // non-0 caps value if dmabuf is supported. > > - if (caps || dmabuf_valid) > > + if (caps || dmabuf_valid) { > > fail_on_test(dmabuf_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_DMABUF)); > > + if (node->has_orphaned_bufs) > > + fail_on_test(userptr_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS)); > > Huh? I'm not sure what you are testing here. I intended to test that if MMAP supports orphaned buffers, DMABUF should as well, but stopped halfway. Otherwise we'd have to keep separate flags for MMAP and DMABUF. > > + } > > > > fail_on_test((can_stream && !is_overlay) && !mmap_valid && !userptr_valid && !dmabuf_valid); > > fail_on_test((!can_stream || is_overlay) && (mmap_valid || userptr_valid || dmabuf_valid)); > > @@ -967,12 +973,22 @@ int testMmap(struct node *node, unsigned frame_count) > > The setupM2M function should check if m2m_q also sets the V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS. > I.e. this capability for m2m_q must match node->has_orphaned_bufs. > > It makes no sense if it is set for the capture queue but not the output queue > for m2m devices. And since this has to be set manually in the drivers (at least > for now), this needs to be checked by v4l2-compliance. I'll add a check for this. > > 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())); > > - q.munmap_bufs(node); > > - fail_on_test(q.reqbufs(node, 0)); > > + if (node->has_orphaned_bufs) { > > + fail_on_test(q.reqbufs(node, 0)); > > + q.munmap_bufs(node); > > + } else { > > + q.munmap_bufs(node); > > + fail_on_test(q.reqbufs(node, 0)); > > This 'else' can be improved: > > } else if (!q.reqbufs(node, 0)) { > // It's either a bug or this driver should set > // V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS > warn("Can free buffers even if still mmap()ed\n"); > q.munmap_bufs(node); > } else { > q.munmap_bufs(node); > fail_on_test(q.reqbufs(node, 0)); Ok. > > + } > > if (node->is_m2m) { > > fail_on_test(node->streamoff(m2m_q.g_type())); > > - m2m_q.munmap_bufs(node); > > - fail_on_test(m2m_q.reqbufs(node, 0)); > > + if (node->has_orphaned_bufs) { > > + fail_on_test(m2m_q.reqbufs(node, 0)); > > + m2m_q.munmap_bufs(node); > > + } else { > > + m2m_q.munmap_bufs(node); > > + fail_on_test(m2m_q.reqbufs(node, 0)); > > + } > > } > > } > > return 0; > > @@ -1201,6 +1217,13 @@ 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())); > > + if (node->has_orphaned_bufs) { > > + fail_on_test(q.reqbufs(node, 0)); > > + exp_q.close_exported_fds(); > > + } else { > > Something similar to the MMAP case should be done here as well. > > If nothing else, that checks that if V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS > is *not* set, then q.reqbufs(node, 0) should fail. Will do. Thank you for the comments! regards Philipp
On 11/15/18 13:52, Philipp Zabel wrote: > On Thu, 2018-11-15 at 11:21 +0100, Hans Verkuil wrote: >> On 11/14/18 15:38, Philipp Zabel wrote: >>> Test that V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS is reported equally for >>> both MMAP and DMABUF memory types. If supported, try to orphan buffers >>> by calling reqbufs(0) before unmapping or closing DMABUF fds. >>> >>> Also close exported DMABUF fds and free buffers in testDmaBuf if >>> orphaned buffers are not supported. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> contrib/freebsd/include/linux/videodev2.h | 1 + >>> include/linux/videodev2.h | 1 + >>> utils/common/v4l2-info.cpp | 1 + >>> utils/v4l2-compliance/v4l2-compliance.h | 1 + >>> utils/v4l2-compliance/v4l2-test-buffers.cpp | 35 +++++++++++++++++---- >>> 5 files changed, 33 insertions(+), 6 deletions(-) >>> >>> diff --git a/contrib/freebsd/include/linux/videodev2.h b/contrib/freebsd/include/linux/videodev2.h >>> index 9928c00e4b68..33153b53c175 100644 >>> --- a/contrib/freebsd/include/linux/videodev2.h >>> +++ b/contrib/freebsd/include/linux/videodev2.h >>> @@ -907,6 +907,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 >>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h >>> index 79418cd39480..a39300cacb6a 100644 >>> --- a/include/linux/videodev2.h >>> +++ b/include/linux/videodev2.h >>> @@ -873,6 +873,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 >>> diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp >>> index 258e5446f030..3699c35cb9d6 100644 >>> --- a/utils/common/v4l2-info.cpp >>> +++ b/utils/common/v4l2-info.cpp >>> @@ -200,6 +200,7 @@ static const flag_def bufcap_def[] = { >>> { V4L2_BUF_CAP_SUPPORTS_USERPTR, "userptr" }, >>> { V4L2_BUF_CAP_SUPPORTS_DMABUF, "dmabuf" }, >>> { V4L2_BUF_CAP_SUPPORTS_REQUESTS, "requests" }, >>> + { V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS, "orphaned-bufs" }, >>> { 0, NULL } >>> }; >>> >>> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >>> index def185f17261..88ec260a9bcc 100644 >>> --- a/utils/v4l2-compliance/v4l2-compliance.h >>> +++ b/utils/v4l2-compliance/v4l2-compliance.h >>> @@ -119,6 +119,7 @@ struct base_node { >>> __u32 valid_buftypes; >>> __u32 valid_buftype; >>> __u32 valid_memorytype; >>> + bool has_orphaned_bufs; >> >> I'd rename that to supports_orphaned_bufs. > > Ok. > >>> }; >>> >>> struct node : public base_node, public cv4l_fd { >>> diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp >>> index c59a56d9ced7..6174015cb4e7 100644 >>> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp >>> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp >>> @@ -400,8 +400,11 @@ int testReqBufs(struct node *node) >>> mmap_valid = !ret; >>> if (mmap_valid) >>> caps = q.g_capabilities(); >>> - if (caps) >>> + if (caps) { >>> fail_on_test(mmap_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_MMAP)); >>> + if (caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS) >>> + node->has_orphaned_bufs = true; >>> + } >>> >>> q.init(i, V4L2_MEMORY_USERPTR); >>> ret = q.reqbufs(node, 0); >>> @@ -418,8 +421,11 @@ int testReqBufs(struct node *node) >>> fail_on_test(!mmap_valid && dmabuf_valid); >>> // Note: dmabuf is only supported with vb2, so we can assume a >>> // non-0 caps value if dmabuf is supported. >>> - if (caps || dmabuf_valid) >>> + if (caps || dmabuf_valid) { >>> fail_on_test(dmabuf_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_DMABUF)); >>> + if (node->has_orphaned_bufs) >>> + fail_on_test(userptr_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS)); >> >> Huh? I'm not sure what you are testing here. > > I intended to test that if MMAP supports orphaned buffers, DMABUF should > as well, but stopped halfway. Otherwise we'd have to keep separate flags > for MMAP and DMABUF. It's shared. So the capability field as returned by REQBUFS and CREATE_BUFS is valid for both MMAP and DMABUF. Or to put it in another way: the returned capabilities are independent of the memory field. What would be a good test is to check if the capabilities returned when testing USERPTR and DMABUF are equal to that returned by MMAP. Hmm, it should also check that the capabilities returned by CREATE_BUFS are identical to those returned by REQBUFS. > >>> + } >>> >>> fail_on_test((can_stream && !is_overlay) && !mmap_valid && !userptr_valid && !dmabuf_valid); >>> fail_on_test((!can_stream || is_overlay) && (mmap_valid || userptr_valid || dmabuf_valid)); >>> @@ -967,12 +973,22 @@ int testMmap(struct node *node, unsigned frame_count) >> >> The setupM2M function should check if m2m_q also sets the V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS. >> I.e. this capability for m2m_q must match node->has_orphaned_bufs. >> >> It makes no sense if it is set for the capture queue but not the output queue >> for m2m devices. And since this has to be set manually in the drivers (at least >> for now), this needs to be checked by v4l2-compliance. > > I'll add a check for this. > >>> 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())); >>> - q.munmap_bufs(node); >>> - fail_on_test(q.reqbufs(node, 0)); >>> + if (node->has_orphaned_bufs) { >>> + fail_on_test(q.reqbufs(node, 0)); >>> + q.munmap_bufs(node); >>> + } else { >>> + q.munmap_bufs(node); >>> + fail_on_test(q.reqbufs(node, 0)); >> >> This 'else' can be improved: >> >> } else if (!q.reqbufs(node, 0)) { >> // It's either a bug or this driver should set >> // V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS >> warn("Can free buffers even if still mmap()ed\n"); >> q.munmap_bufs(node); >> } else { >> q.munmap_bufs(node); >> fail_on_test(q.reqbufs(node, 0)); > > Ok. > >>> + } >>> if (node->is_m2m) { >>> fail_on_test(node->streamoff(m2m_q.g_type())); >>> - m2m_q.munmap_bufs(node); >>> - fail_on_test(m2m_q.reqbufs(node, 0)); >>> + if (node->has_orphaned_bufs) { >>> + fail_on_test(m2m_q.reqbufs(node, 0)); >>> + m2m_q.munmap_bufs(node); >>> + } else { >>> + m2m_q.munmap_bufs(node); >>> + fail_on_test(m2m_q.reqbufs(node, 0)); >>> + } >>> } >>> } >>> return 0; >>> @@ -1201,6 +1217,13 @@ 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())); >>> + if (node->has_orphaned_bufs) { >>> + fail_on_test(q.reqbufs(node, 0)); >>> + exp_q.close_exported_fds(); >>> + } else { >> >> Something similar to the MMAP case should be done here as well. >> >> If nothing else, that checks that if V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS >> is *not* set, then q.reqbufs(node, 0) should fail. > > Will do. Thank you for the comments! > > regards > Philipp > Regards, Hans
diff --git a/contrib/freebsd/include/linux/videodev2.h b/contrib/freebsd/include/linux/videodev2.h index 9928c00e4b68..33153b53c175 100644 --- a/contrib/freebsd/include/linux/videodev2.h +++ b/contrib/freebsd/include/linux/videodev2.h @@ -907,6 +907,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 diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 79418cd39480..a39300cacb6a 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -873,6 +873,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 diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp index 258e5446f030..3699c35cb9d6 100644 --- a/utils/common/v4l2-info.cpp +++ b/utils/common/v4l2-info.cpp @@ -200,6 +200,7 @@ static const flag_def bufcap_def[] = { { V4L2_BUF_CAP_SUPPORTS_USERPTR, "userptr" }, { V4L2_BUF_CAP_SUPPORTS_DMABUF, "dmabuf" }, { V4L2_BUF_CAP_SUPPORTS_REQUESTS, "requests" }, + { V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS, "orphaned-bufs" }, { 0, NULL } }; diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h index def185f17261..88ec260a9bcc 100644 --- a/utils/v4l2-compliance/v4l2-compliance.h +++ b/utils/v4l2-compliance/v4l2-compliance.h @@ -119,6 +119,7 @@ struct base_node { __u32 valid_buftypes; __u32 valid_buftype; __u32 valid_memorytype; + bool has_orphaned_bufs; }; struct node : public base_node, public cv4l_fd { diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp index c59a56d9ced7..6174015cb4e7 100644 --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp @@ -400,8 +400,11 @@ int testReqBufs(struct node *node) mmap_valid = !ret; if (mmap_valid) caps = q.g_capabilities(); - if (caps) + if (caps) { fail_on_test(mmap_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_MMAP)); + if (caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS) + node->has_orphaned_bufs = true; + } q.init(i, V4L2_MEMORY_USERPTR); ret = q.reqbufs(node, 0); @@ -418,8 +421,11 @@ int testReqBufs(struct node *node) fail_on_test(!mmap_valid && dmabuf_valid); // Note: dmabuf is only supported with vb2, so we can assume a // non-0 caps value if dmabuf is supported. - if (caps || dmabuf_valid) + if (caps || dmabuf_valid) { fail_on_test(dmabuf_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_DMABUF)); + if (node->has_orphaned_bufs) + fail_on_test(userptr_valid ^ !!(caps & V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS)); + } fail_on_test((can_stream && !is_overlay) && !mmap_valid && !userptr_valid && !dmabuf_valid); fail_on_test((!can_stream || is_overlay) && (mmap_valid || userptr_valid || dmabuf_valid)); @@ -967,12 +973,22 @@ int testMmap(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())); - q.munmap_bufs(node); - fail_on_test(q.reqbufs(node, 0)); + if (node->has_orphaned_bufs) { + fail_on_test(q.reqbufs(node, 0)); + q.munmap_bufs(node); + } else { + q.munmap_bufs(node); + fail_on_test(q.reqbufs(node, 0)); + } if (node->is_m2m) { fail_on_test(node->streamoff(m2m_q.g_type())); - m2m_q.munmap_bufs(node); - fail_on_test(m2m_q.reqbufs(node, 0)); + if (node->has_orphaned_bufs) { + fail_on_test(m2m_q.reqbufs(node, 0)); + m2m_q.munmap_bufs(node); + } else { + m2m_q.munmap_bufs(node); + fail_on_test(m2m_q.reqbufs(node, 0)); + } } } return 0; @@ -1201,6 +1217,13 @@ 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())); + if (node->has_orphaned_bufs) { + fail_on_test(q.reqbufs(node, 0)); + exp_q.close_exported_fds(); + } else { + exp_q.close_exported_fds(); + fail_on_test(q.reqbufs(node, 0)); + } } return 0; }
Test that V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS is reported equally for both MMAP and DMABUF memory types. If supported, try to orphan buffers by calling reqbufs(0) before unmapping or closing DMABUF fds. Also close exported DMABUF fds and free buffers in testDmaBuf if orphaned buffers are not supported. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- contrib/freebsd/include/linux/videodev2.h | 1 + include/linux/videodev2.h | 1 + utils/common/v4l2-info.cpp | 1 + utils/v4l2-compliance/v4l2-compliance.h | 1 + utils/v4l2-compliance/v4l2-test-buffers.cpp | 35 +++++++++++++++++---- 5 files changed, 33 insertions(+), 6 deletions(-)