| Message ID | 20240221155551.100156-3-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l2-compliance: Add tests for REMOVE_BUFS | expand |
On 21/02/2024 4:55 pm, Benjamin Gaignard wrote: > Add new test for REMOVE_BUFS ioctl. > It create buffers and check if they could be removed from queue. > It also check that removing non existing buffer or a queued > buffer failed. > Since using REMOVE_BUFS can create "holes" v4l_queue_querybufs() > function needs to be modify to do a range check between [from..from+count-1]. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > utils/common/cv4l-helpers.h | 4 + > utils/common/v4l-helpers.h | 27 ++++-- > utils/v4l2-compliance/v4l2-compliance.cpp | 1 + > utils/v4l2-compliance/v4l2-compliance.h | 1 + > utils/v4l2-compliance/v4l2-test-buffers.cpp | 99 +++++++++++++++++++++ > 5 files changed, 127 insertions(+), 5 deletions(-) > > diff --git a/utils/common/cv4l-helpers.h b/utils/common/cv4l-helpers.h > index 77c6517a..afe8d469 100644 > --- a/utils/common/cv4l-helpers.h > +++ b/utils/common/cv4l-helpers.h > @@ -764,6 +764,10 @@ public: > { > return v4l_queue_reqbufs(fd->g_v4l_fd(), this, count, flags); > } > + int remove_bufs(cv4l_fd *fd, unsigned index = 0, unsigned count = 0) > + { > + return v4l_queue_remove_bufs(fd->g_v4l_fd(), this, index, count); > + } > bool has_create_bufs(cv4l_fd *fd) const > { > return v4l_queue_has_create_bufs(fd->g_v4l_fd(), this); > diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h > index 7387b621..1240c23f 100644 > --- a/utils/common/v4l-helpers.h > +++ b/utils/common/v4l-helpers.h > @@ -1513,12 +1513,29 @@ static inline void *v4l_queue_g_dataptr(const struct v4l_queue *q, unsigned inde > return v4l_queue_g_mmapping(q, index, plane); > } > > -static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from) > +static inline int v4l_queue_remove_bufs(struct v4l_fd *f, struct v4l_queue *q, unsigned index, unsigned count) > { > - unsigned b, p; > + struct v4l2_remove_buffers removebufs; > int ret; > > - for (b = from; b < v4l_queue_g_buffers(q); b++) { > + memset(&removebufs, 0, sizeof(removebufs)); > + removebufs.type = q->type; > + removebufs.index = index; > + removebufs.count = count; > + > + ret = v4l_ioctl(f, VIDIOC_REMOVE_BUFS, &removebufs); > + if (!ret) > + q->buffers -= removebufs.count; > + > + return ret; > +} > + > +static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from, unsigned count) > +{ > + unsigned b, p, max = from + count; > + int ret; > + > + for (b = from; b < max; b++) { > struct v4l_buffer buf; > > v4l_buffer_init(&buf, v4l_queue_g_type(q), v4l_queue_g_memory(q), b); > @@ -1556,7 +1573,7 @@ static inline int v4l_queue_reqbufs(struct v4l_fd *f, > return ret; > q->buffers = reqbufs.count; > q->capabilities = reqbufs.capabilities; > - return v4l_queue_querybufs(f, q, 0); > + return v4l_queue_querybufs(f, q, 0, reqbufs.count); > } > > static inline bool v4l_queue_has_create_bufs(struct v4l_fd *f, const struct v4l_queue *q) > @@ -1596,7 +1613,7 @@ static inline int v4l_queue_create_bufs(struct v4l_fd *f, > if (q->capabilities & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS) > q->max_num_buffers = createbufs.max_num_buffers; > q->buffers += createbufs.count; > - return v4l_queue_querybufs(f, q, q->buffers - createbufs.count); > + return v4l_queue_querybufs(f, q, createbufs.index, createbufs.count); > } > > static inline int v4l_queue_mmap_bufs(struct v4l_fd *f, > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp > index 2f7a5058..c6a685eb 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.cpp > +++ b/utils/v4l2-compliance/v4l2-compliance.cpp > @@ -1466,6 +1466,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ > printf("Buffer ioctls%s:\n", suffix); > printf("\ttest VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: %s\n", ok(testReqBufs(&node))); > printf("\ttest CREATE_BUFS maximum buffers: %s\n", ok(testCreateBufsMax(&node))); > + printf("\ttest VIDIOC_REMOVE_BUFS: %s\n", ok(testRemoveBufs(&node))); > // Reopen after each streaming test to reset the streaming state > // in case of any errors in the preceeding test. > node.reopen(); > diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h > index 0cfc9a37..b6e342f3 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.h > +++ b/utils/v4l2-compliance/v4l2-compliance.h > @@ -385,6 +385,7 @@ int testReadWrite(struct node *node); > int testExpBuf(struct node *node); > int testBlockingWait(struct node *node); > int testCreateBufsMax(struct node *node); > +int testRemoveBufs(struct node *node); > > // 32-bit architecture, 32/64-bit time_t tests > int testTime32_64(struct node *node); > diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp > index 922b99b5..f71a3c0e 100644 > --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp > +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp > @@ -529,6 +529,105 @@ static int testCanSetSameTimings(struct node *node) > return 0; > } > > +int testRemoveBufs(struct node *node) > +{ > + int ret; > + unsigned i; > + > + node->reopen(); > + > + for (i = 1; i <= V4L2_BUF_TYPE_LAST; i++) { > + struct v4l2_remove_buffers removebufs = { }; > + > + if (!(node->valid_buftypes & (1 << i))) > + continue; > + > + cv4l_queue q(i, V4L2_MEMORY_MMAP); > + > + if (testSetupVbi(node, i)) > + continue; > + ret = q.remove_bufs(node, 0, 0); > + if (ret == ENOTTY) { > + warn("VIDIOC_REMOVE_BUFS not supported\n"); > + continue; > + } > + > + q.init(i, V4L2_MEMORY_MMAP); > + ret = q.create_bufs(node, 0); > + fail_on_test_val(ret && ret != EINVAL, ret); Why the EINVAL check? If q.remove_bufs is present, then q.create_bufs must also be present. And creating '0' buffers should always work. > + > + memset(&removebufs, 0xff, sizeof(removebufs)); > + removebufs.index = 0; > + removebufs.count = 0; > + removebufs.type = q.g_type(); > + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs)); > + fail_on_test(check_0(removebufs.reserved, sizeof(removebufs.reserved))); > + > + if (!ret) { With the above in mind, this 'if' can just be dropped and everything below has one less TAB indentation. > + unsigned buffers; > + buffer buf(i); > + > + /* Create only 1 buffer */ > + fail_on_test(q.create_bufs(node, 1)); > + buffers = q.g_buffers(); > + fail_on_test(buffers == 0); Why not 'fail_on_test(buffers != 1);'? > + /* Removing buffer index 1 must fail */ > + fail_on_test(q.remove_bufs(node, 1, buffers) != EINVAL); > + /* Removing buffer index 0 is valid */ > + fail_on_test(q.remove_bufs(node, 0, buffers)); > + /* Removing buffer index 0 again must fail */ > + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); > + /* Create 3 buffers indexes 0 to 2 */ > + fail_on_test(q.create_bufs(node, 3)); > + /* Remove them one by one */ > + fail_on_test(q.remove_bufs(node, 2, 1)); > + fail_on_test(q.remove_bufs(node, 0, 1)); > + fail_on_test(q.remove_bufs(node, 1, 1)); > + /* Removing buffer index 0 again must fail */ > + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); > + > + /* for the next test the queue needs to be able to allocate 7 buffers */ > + if (q.g_max_num_buffers() < 7) Can g_max_num_buffers ever be < 32? Do we allow that? If so, does that work? I think we shouldn't allow that as it can potentially cause problems with existing applications that expect up to 32 buffers. Looking at the current vb2 code I think it is allowed, but I think we should forbid it, at least for now. So an attempt by a driver to set q->max_num_buffers to a non-zero value < 32 should be rejected with a WARN_ON(). > + continue; > + > + /* Create 4 buffers indexes 0 to 3 */ > + fail_on_test(q.create_bufs(node, 4)); > + /* Remove buffers index 1 and 2 */ > + fail_on_test(q.remove_bufs(node, 1, 2)); > + /* Add 3 more buffers should be indexes 4 to 6 */ > + fail_on_test(q.create_bufs(node, 3)); > + /* Query buffers: > + * 1 and 2 have been removed they must fail > + * 0 and 4 to 6 must exist*/ Shouldn't that be '3 to 6' instead of '4 to 6'? > + fail_on_test(buf.querybuf(node, 0)); > + fail_on_test(buf.querybuf(node, 1) != EINVAL); > + fail_on_test(buf.querybuf(node, 2) != EINVAL); Missing test for buffer index 3. > + fail_on_test(buf.querybuf(node, 4)); > + fail_on_test(buf.querybuf(node, 5)); > + fail_on_test(buf.querybuf(node, 6)); Add checks to verify that remove_bufs works if count == 0 and index is 0, 1, 6, 7 or 0xffffffff. I.e. regardless of the buffer index, if count == 0 remove_bufs should just return 0. > + /* Remove existing buffer index 6 with bad type must fail */ > + memset(&removebufs, 0xff, sizeof(removebufs)); > + removebufs.index = 6; > + removebufs.count = 1; > + removebufs.type = 0; > + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs) != EINVAL); If count == 0, should this also fail or not? We certainly need a test for that, but I'm not sure what it should do. I'll reply to patch v20 7/9 as well about this. > + > + /* Remove crossing max allowed buffers boundary must fail */ > + fail_on_test(q.remove_bufs(node, q.g_max_num_buffers() - 2, 7) != EINVAL); > + > + /* Remove overflow must fail */ > + fail_on_test(q.remove_bufs(node, 3, 0xfffffff) != EINVAL); I'd like to see a test removing 2 buffers from index 2: that should fail. Ditto for removing 2 buffers from index 0. > + > + /* Create 2 buffers, that must fill the hole */ > + fail_on_test(q.create_bufs(node, 2)); > + /* Remove all buffers */ > + fail_on_test(q.remove_bufs(node, 0, 7)); > + } > + } > + > + return 0; > +} > + > int testReqBufs(struct node *node) > { > struct v4l2_create_buffers crbufs = { }; Regards, Hans
On 14/03/2024 12:38 pm, Hans Verkuil wrote: > On 21/02/2024 4:55 pm, Benjamin Gaignard wrote: >> Add new test for REMOVE_BUFS ioctl. >> It create buffers and check if they could be removed from queue. >> It also check that removing non existing buffer or a queued >> buffer failed. >> Since using REMOVE_BUFS can create "holes" v4l_queue_querybufs() >> function needs to be modify to do a range check between [from..from+count-1]. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> utils/common/cv4l-helpers.h | 4 + >> utils/common/v4l-helpers.h | 27 ++++-- >> utils/v4l2-compliance/v4l2-compliance.cpp | 1 + >> utils/v4l2-compliance/v4l2-compliance.h | 1 + >> utils/v4l2-compliance/v4l2-test-buffers.cpp | 99 +++++++++++++++++++++ >> 5 files changed, 127 insertions(+), 5 deletions(-) >> >> diff --git a/utils/common/cv4l-helpers.h b/utils/common/cv4l-helpers.h >> index 77c6517a..afe8d469 100644 >> --- a/utils/common/cv4l-helpers.h >> +++ b/utils/common/cv4l-helpers.h >> @@ -764,6 +764,10 @@ public: >> { >> return v4l_queue_reqbufs(fd->g_v4l_fd(), this, count, flags); >> } >> + int remove_bufs(cv4l_fd *fd, unsigned index = 0, unsigned count = 0) >> + { >> + return v4l_queue_remove_bufs(fd->g_v4l_fd(), this, index, count); >> + } >> bool has_create_bufs(cv4l_fd *fd) const >> { >> return v4l_queue_has_create_bufs(fd->g_v4l_fd(), this); >> diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h >> index 7387b621..1240c23f 100644 >> --- a/utils/common/v4l-helpers.h >> +++ b/utils/common/v4l-helpers.h >> @@ -1513,12 +1513,29 @@ static inline void *v4l_queue_g_dataptr(const struct v4l_queue *q, unsigned inde >> return v4l_queue_g_mmapping(q, index, plane); >> } >> >> -static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from) >> +static inline int v4l_queue_remove_bufs(struct v4l_fd *f, struct v4l_queue *q, unsigned index, unsigned count) >> { >> - unsigned b, p; >> + struct v4l2_remove_buffers removebufs; >> int ret; >> >> - for (b = from; b < v4l_queue_g_buffers(q); b++) { >> + memset(&removebufs, 0, sizeof(removebufs)); >> + removebufs.type = q->type; >> + removebufs.index = index; >> + removebufs.count = count; >> + >> + ret = v4l_ioctl(f, VIDIOC_REMOVE_BUFS, &removebufs); >> + if (!ret) >> + q->buffers -= removebufs.count; >> + >> + return ret; >> +} >> + >> +static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from, unsigned count) >> +{ >> + unsigned b, p, max = from + count; >> + int ret; >> + >> + for (b = from; b < max; b++) { >> struct v4l_buffer buf; >> >> v4l_buffer_init(&buf, v4l_queue_g_type(q), v4l_queue_g_memory(q), b); >> @@ -1556,7 +1573,7 @@ static inline int v4l_queue_reqbufs(struct v4l_fd *f, >> return ret; >> q->buffers = reqbufs.count; >> q->capabilities = reqbufs.capabilities; >> - return v4l_queue_querybufs(f, q, 0); >> + return v4l_queue_querybufs(f, q, 0, reqbufs.count); >> } >> >> static inline bool v4l_queue_has_create_bufs(struct v4l_fd *f, const struct v4l_queue *q) >> @@ -1596,7 +1613,7 @@ static inline int v4l_queue_create_bufs(struct v4l_fd *f, >> if (q->capabilities & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS) >> q->max_num_buffers = createbufs.max_num_buffers; >> q->buffers += createbufs.count; >> - return v4l_queue_querybufs(f, q, q->buffers - createbufs.count); >> + return v4l_queue_querybufs(f, q, createbufs.index, createbufs.count); >> } >> >> static inline int v4l_queue_mmap_bufs(struct v4l_fd *f, >> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >> index 2f7a5058..c6a685eb 100644 >> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >> @@ -1466,6 +1466,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >> printf("Buffer ioctls%s:\n", suffix); >> printf("\ttest VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: %s\n", ok(testReqBufs(&node))); >> printf("\ttest CREATE_BUFS maximum buffers: %s\n", ok(testCreateBufsMax(&node))); >> + printf("\ttest VIDIOC_REMOVE_BUFS: %s\n", ok(testRemoveBufs(&node))); >> // Reopen after each streaming test to reset the streaming state >> // in case of any errors in the preceeding test. >> node.reopen(); >> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >> index 0cfc9a37..b6e342f3 100644 >> --- a/utils/v4l2-compliance/v4l2-compliance.h >> +++ b/utils/v4l2-compliance/v4l2-compliance.h >> @@ -385,6 +385,7 @@ int testReadWrite(struct node *node); >> int testExpBuf(struct node *node); >> int testBlockingWait(struct node *node); >> int testCreateBufsMax(struct node *node); >> +int testRemoveBufs(struct node *node); >> >> // 32-bit architecture, 32/64-bit time_t tests >> int testTime32_64(struct node *node); >> diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp >> index 922b99b5..f71a3c0e 100644 >> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp >> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp >> @@ -529,6 +529,105 @@ static int testCanSetSameTimings(struct node *node) >> return 0; >> } >> >> +int testRemoveBufs(struct node *node) >> +{ >> + int ret; >> + unsigned i; >> + >> + node->reopen(); >> + >> + for (i = 1; i <= V4L2_BUF_TYPE_LAST; i++) { >> + struct v4l2_remove_buffers removebufs = { }; >> + >> + if (!(node->valid_buftypes & (1 << i))) >> + continue; >> + >> + cv4l_queue q(i, V4L2_MEMORY_MMAP); >> + >> + if (testSetupVbi(node, i)) >> + continue; >> + ret = q.remove_bufs(node, 0, 0); >> + if (ret == ENOTTY) { >> + warn("VIDIOC_REMOVE_BUFS not supported\n"); >> + continue; >> + } >> + >> + q.init(i, V4L2_MEMORY_MMAP); >> + ret = q.create_bufs(node, 0); >> + fail_on_test_val(ret && ret != EINVAL, ret); > > Why the EINVAL check? If q.remove_bufs is present, then q.create_bufs must > also be present. And creating '0' buffers should always work. > >> + >> + memset(&removebufs, 0xff, sizeof(removebufs)); >> + removebufs.index = 0; >> + removebufs.count = 0; >> + removebufs.type = q.g_type(); >> + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs)); >> + fail_on_test(check_0(removebufs.reserved, sizeof(removebufs.reserved))); >> + >> + if (!ret) { > > With the above in mind, this 'if' can just be dropped and everything below > has one less TAB indentation. > >> + unsigned buffers; >> + buffer buf(i); >> + >> + /* Create only 1 buffer */ >> + fail_on_test(q.create_bufs(node, 1)); >> + buffers = q.g_buffers(); >> + fail_on_test(buffers == 0); > > Why not 'fail_on_test(buffers != 1);'? > >> + /* Removing buffer index 1 must fail */ >> + fail_on_test(q.remove_bufs(node, 1, buffers) != EINVAL); >> + /* Removing buffer index 0 is valid */ >> + fail_on_test(q.remove_bufs(node, 0, buffers)); >> + /* Removing buffer index 0 again must fail */ >> + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); >> + /* Create 3 buffers indexes 0 to 2 */ >> + fail_on_test(q.create_bufs(node, 3)); >> + /* Remove them one by one */ >> + fail_on_test(q.remove_bufs(node, 2, 1)); >> + fail_on_test(q.remove_bufs(node, 0, 1)); >> + fail_on_test(q.remove_bufs(node, 1, 1)); >> + /* Removing buffer index 0 again must fail */ >> + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); >> + >> + /* for the next test the queue needs to be able to allocate 7 buffers */ >> + if (q.g_max_num_buffers() < 7) > > Can g_max_num_buffers ever be < 32? Do we allow that? If so, does that work? > I think we shouldn't allow that as it can potentially cause problems with > existing applications that expect up to 32 buffers. > > Looking at the current vb2 code I think it is allowed, but I think we > should forbid it, at least for now. So an attempt by a driver to set > q->max_num_buffers to a non-zero value < 32 should be rejected with > a WARN_ON(). > >> + continue; >> + >> + /* Create 4 buffers indexes 0 to 3 */ >> + fail_on_test(q.create_bufs(node, 4)); >> + /* Remove buffers index 1 and 2 */ >> + fail_on_test(q.remove_bufs(node, 1, 2)); >> + /* Add 3 more buffers should be indexes 4 to 6 */ >> + fail_on_test(q.create_bufs(node, 3)); >> + /* Query buffers: >> + * 1 and 2 have been removed they must fail >> + * 0 and 4 to 6 must exist*/ > > Shouldn't that be '3 to 6' instead of '4 to 6'? > >> + fail_on_test(buf.querybuf(node, 0)); >> + fail_on_test(buf.querybuf(node, 1) != EINVAL); >> + fail_on_test(buf.querybuf(node, 2) != EINVAL); > > Missing test for buffer index 3. > >> + fail_on_test(buf.querybuf(node, 4)); >> + fail_on_test(buf.querybuf(node, 5)); >> + fail_on_test(buf.querybuf(node, 6)); > > Add checks to verify that remove_bufs works if count == 0 and index is > 0, 1, 6, 7 or 0xffffffff. I.e. regardless of the buffer index, if count == 0 > remove_bufs should just return 0. > >> + /* Remove existing buffer index 6 with bad type must fail */ >> + memset(&removebufs, 0xff, sizeof(removebufs)); >> + removebufs.index = 6; >> + removebufs.count = 1; >> + removebufs.type = 0; >> + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs) != EINVAL); > > If count == 0, should this also fail or not? We certainly need a test for that, > but I'm not sure what it should do. > > I'll reply to patch v20 7/9 as well about this. > >> + >> + /* Remove crossing max allowed buffers boundary must fail */ >> + fail_on_test(q.remove_bufs(node, q.g_max_num_buffers() - 2, 7) != EINVAL); >> + >> + /* Remove overflow must fail */ >> + fail_on_test(q.remove_bufs(node, 3, 0xfffffff) != EINVAL); > > I'd like to see a test removing 2 buffers from index 2: that should fail. > Ditto for removing 2 buffers from index 0. > >> + >> + /* Create 2 buffers, that must fill the hole */ >> + fail_on_test(q.create_bufs(node, 2)); >> + /* Remove all buffers */ >> + fail_on_test(q.remove_bufs(node, 0, 7)); You need to end with this: fail_on_test(q.reqbufs(node, 0)); Otherwise the queue will remain marked as busy and v4l2-compliance will fail on /dev/vbiX devices. The reason it fails on vbi devices is that those can be opened with two different buffer types: VBI_CAPTURE and SLICED_VBI_CAPTURE. The vivid driver supports both types, and running this test for VBI_CAPTURE is fine, but running it for the next type fails because the queue is still in use for type VBI_CAPTURE. Regards, Hans >> + } >> + } >> + >> + return 0; >> +} >> + >> int testReqBufs(struct node *node) >> { >> struct v4l2_create_buffers crbufs = { }; > > Regards, > > Hans >
On 21/02/2024 4:55 pm, Benjamin Gaignard wrote: > Add new test for REMOVE_BUFS ioctl. > It create buffers and check if they could be removed from queue. > It also check that removing non existing buffer or a queued > buffer failed. > Since using REMOVE_BUFS can create "holes" v4l_queue_querybufs() > function needs to be modify to do a range check between [from..from+count-1]. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > utils/common/cv4l-helpers.h | 4 + > utils/common/v4l-helpers.h | 27 ++++-- > utils/v4l2-compliance/v4l2-compliance.cpp | 1 + > utils/v4l2-compliance/v4l2-compliance.h | 1 + > utils/v4l2-compliance/v4l2-test-buffers.cpp | 99 +++++++++++++++++++++ > 5 files changed, 127 insertions(+), 5 deletions(-) > > diff --git a/utils/common/cv4l-helpers.h b/utils/common/cv4l-helpers.h > index 77c6517a..afe8d469 100644 > --- a/utils/common/cv4l-helpers.h > +++ b/utils/common/cv4l-helpers.h > @@ -764,6 +764,10 @@ public: > { > return v4l_queue_reqbufs(fd->g_v4l_fd(), this, count, flags); > } > + int remove_bufs(cv4l_fd *fd, unsigned index = 0, unsigned count = 0) > + { > + return v4l_queue_remove_bufs(fd->g_v4l_fd(), this, index, count); > + } > bool has_create_bufs(cv4l_fd *fd) const > { > return v4l_queue_has_create_bufs(fd->g_v4l_fd(), this); > diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h > index 7387b621..1240c23f 100644 > --- a/utils/common/v4l-helpers.h > +++ b/utils/common/v4l-helpers.h > @@ -1513,12 +1513,29 @@ static inline void *v4l_queue_g_dataptr(const struct v4l_queue *q, unsigned inde > return v4l_queue_g_mmapping(q, index, plane); > } > > -static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from) > +static inline int v4l_queue_remove_bufs(struct v4l_fd *f, struct v4l_queue *q, unsigned index, unsigned count) > { > - unsigned b, p; > + struct v4l2_remove_buffers removebufs; > int ret; > > - for (b = from; b < v4l_queue_g_buffers(q); b++) { > + memset(&removebufs, 0, sizeof(removebufs)); > + removebufs.type = q->type; > + removebufs.index = index; > + removebufs.count = count; > + > + ret = v4l_ioctl(f, VIDIOC_REMOVE_BUFS, &removebufs); > + if (!ret) > + q->buffers -= removebufs.count; > + > + return ret; > +} > + > +static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from, unsigned count) > +{ > + unsigned b, p, max = from + count; > + int ret; > + > + for (b = from; b < max; b++) { > struct v4l_buffer buf; > > v4l_buffer_init(&buf, v4l_queue_g_type(q), v4l_queue_g_memory(q), b); > @@ -1556,7 +1573,7 @@ static inline int v4l_queue_reqbufs(struct v4l_fd *f, > return ret; > q->buffers = reqbufs.count; > q->capabilities = reqbufs.capabilities; > - return v4l_queue_querybufs(f, q, 0); > + return v4l_queue_querybufs(f, q, 0, reqbufs.count); > } > > static inline bool v4l_queue_has_create_bufs(struct v4l_fd *f, const struct v4l_queue *q) > @@ -1596,7 +1613,7 @@ static inline int v4l_queue_create_bufs(struct v4l_fd *f, > if (q->capabilities & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS) > q->max_num_buffers = createbufs.max_num_buffers; > q->buffers += createbufs.count; > - return v4l_queue_querybufs(f, q, q->buffers - createbufs.count); > + return v4l_queue_querybufs(f, q, createbufs.index, createbufs.count); > } > > static inline int v4l_queue_mmap_bufs(struct v4l_fd *f, > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp > index 2f7a5058..c6a685eb 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.cpp > +++ b/utils/v4l2-compliance/v4l2-compliance.cpp > @@ -1466,6 +1466,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ > printf("Buffer ioctls%s:\n", suffix); > printf("\ttest VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: %s\n", ok(testReqBufs(&node))); > printf("\ttest CREATE_BUFS maximum buffers: %s\n", ok(testCreateBufsMax(&node))); > + printf("\ttest VIDIOC_REMOVE_BUFS: %s\n", ok(testRemoveBufs(&node))); > // Reopen after each streaming test to reset the streaming state > // in case of any errors in the preceeding test. > node.reopen(); > diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h > index 0cfc9a37..b6e342f3 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.h > +++ b/utils/v4l2-compliance/v4l2-compliance.h > @@ -385,6 +385,7 @@ int testReadWrite(struct node *node); > int testExpBuf(struct node *node); > int testBlockingWait(struct node *node); > int testCreateBufsMax(struct node *node); > +int testRemoveBufs(struct node *node); > > // 32-bit architecture, 32/64-bit time_t tests > int testTime32_64(struct node *node); > diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp > index 922b99b5..f71a3c0e 100644 > --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp > +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp > @@ -529,6 +529,105 @@ static int testCanSetSameTimings(struct node *node) > return 0; > } > > +int testRemoveBufs(struct node *node) > +{ > + int ret; > + unsigned i; > + > + node->reopen(); > + > + for (i = 1; i <= V4L2_BUF_TYPE_LAST; i++) { > + struct v4l2_remove_buffers removebufs = { }; > + > + if (!(node->valid_buftypes & (1 << i))) > + continue; > + > + cv4l_queue q(i, V4L2_MEMORY_MMAP); > + > + if (testSetupVbi(node, i)) > + continue; > + ret = q.remove_bufs(node, 0, 0); > + if (ret == ENOTTY) { > + warn("VIDIOC_REMOVE_BUFS not supported\n"); Don't warn here. If ENOTTY is returned, then just return that. In the v4l2-compliance output it will appear as: test VIDIOC_REMOVE_BUFS: OK (Not Supported) Regards, Hans > + continue; > + } > + > + q.init(i, V4L2_MEMORY_MMAP); > + ret = q.create_bufs(node, 0); > + fail_on_test_val(ret && ret != EINVAL, ret); > + > + memset(&removebufs, 0xff, sizeof(removebufs)); > + removebufs.index = 0; > + removebufs.count = 0; > + removebufs.type = q.g_type(); > + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs)); > + fail_on_test(check_0(removebufs.reserved, sizeof(removebufs.reserved))); > + > + if (!ret) { > + unsigned buffers; > + buffer buf(i); > + > + /* Create only 1 buffer */ > + fail_on_test(q.create_bufs(node, 1)); > + buffers = q.g_buffers(); > + fail_on_test(buffers == 0); > + /* Removing buffer index 1 must fail */ > + fail_on_test(q.remove_bufs(node, 1, buffers) != EINVAL); > + /* Removing buffer index 0 is valid */ > + fail_on_test(q.remove_bufs(node, 0, buffers)); > + /* Removing buffer index 0 again must fail */ > + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); > + /* Create 3 buffers indexes 0 to 2 */ > + fail_on_test(q.create_bufs(node, 3)); > + /* Remove them one by one */ > + fail_on_test(q.remove_bufs(node, 2, 1)); > + fail_on_test(q.remove_bufs(node, 0, 1)); > + fail_on_test(q.remove_bufs(node, 1, 1)); > + /* Removing buffer index 0 again must fail */ > + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); > + > + /* for the next test the queue needs to be able to allocate 7 buffers */ > + if (q.g_max_num_buffers() < 7) > + continue; > + > + /* Create 4 buffers indexes 0 to 3 */ > + fail_on_test(q.create_bufs(node, 4)); > + /* Remove buffers index 1 and 2 */ > + fail_on_test(q.remove_bufs(node, 1, 2)); > + /* Add 3 more buffers should be indexes 4 to 6 */ > + fail_on_test(q.create_bufs(node, 3)); > + /* Query buffers: > + * 1 and 2 have been removed they must fail > + * 0 and 4 to 6 must exist*/ > + fail_on_test(buf.querybuf(node, 0)); > + fail_on_test(buf.querybuf(node, 1) != EINVAL); > + fail_on_test(buf.querybuf(node, 2) != EINVAL); > + fail_on_test(buf.querybuf(node, 4)); > + fail_on_test(buf.querybuf(node, 5)); > + fail_on_test(buf.querybuf(node, 6)); > + /* Remove existing buffer index 6 with bad type must fail */ > + memset(&removebufs, 0xff, sizeof(removebufs)); > + removebufs.index = 6; > + removebufs.count = 1; > + removebufs.type = 0; > + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs) != EINVAL); > + > + /* Remove crossing max allowed buffers boundary must fail */ > + fail_on_test(q.remove_bufs(node, q.g_max_num_buffers() - 2, 7) != EINVAL); > + > + /* Remove overflow must fail */ > + fail_on_test(q.remove_bufs(node, 3, 0xfffffff) != EINVAL); > + > + /* Create 2 buffers, that must fill the hole */ > + fail_on_test(q.create_bufs(node, 2)); > + /* Remove all buffers */ > + fail_on_test(q.remove_bufs(node, 0, 7)); > + } > + } > + > + return 0; > +} > + > int testReqBufs(struct node *node) > { > struct v4l2_create_buffers crbufs = { };
diff --git a/utils/common/cv4l-helpers.h b/utils/common/cv4l-helpers.h index 77c6517a..afe8d469 100644 --- a/utils/common/cv4l-helpers.h +++ b/utils/common/cv4l-helpers.h @@ -764,6 +764,10 @@ public: { return v4l_queue_reqbufs(fd->g_v4l_fd(), this, count, flags); } + int remove_bufs(cv4l_fd *fd, unsigned index = 0, unsigned count = 0) + { + return v4l_queue_remove_bufs(fd->g_v4l_fd(), this, index, count); + } bool has_create_bufs(cv4l_fd *fd) const { return v4l_queue_has_create_bufs(fd->g_v4l_fd(), this); diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h index 7387b621..1240c23f 100644 --- a/utils/common/v4l-helpers.h +++ b/utils/common/v4l-helpers.h @@ -1513,12 +1513,29 @@ static inline void *v4l_queue_g_dataptr(const struct v4l_queue *q, unsigned inde return v4l_queue_g_mmapping(q, index, plane); } -static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from) +static inline int v4l_queue_remove_bufs(struct v4l_fd *f, struct v4l_queue *q, unsigned index, unsigned count) { - unsigned b, p; + struct v4l2_remove_buffers removebufs; int ret; - for (b = from; b < v4l_queue_g_buffers(q); b++) { + memset(&removebufs, 0, sizeof(removebufs)); + removebufs.type = q->type; + removebufs.index = index; + removebufs.count = count; + + ret = v4l_ioctl(f, VIDIOC_REMOVE_BUFS, &removebufs); + if (!ret) + q->buffers -= removebufs.count; + + return ret; +} + +static inline int v4l_queue_querybufs(struct v4l_fd *f, struct v4l_queue *q, unsigned from, unsigned count) +{ + unsigned b, p, max = from + count; + int ret; + + for (b = from; b < max; b++) { struct v4l_buffer buf; v4l_buffer_init(&buf, v4l_queue_g_type(q), v4l_queue_g_memory(q), b); @@ -1556,7 +1573,7 @@ static inline int v4l_queue_reqbufs(struct v4l_fd *f, return ret; q->buffers = reqbufs.count; q->capabilities = reqbufs.capabilities; - return v4l_queue_querybufs(f, q, 0); + return v4l_queue_querybufs(f, q, 0, reqbufs.count); } static inline bool v4l_queue_has_create_bufs(struct v4l_fd *f, const struct v4l_queue *q) @@ -1596,7 +1613,7 @@ static inline int v4l_queue_create_bufs(struct v4l_fd *f, if (q->capabilities & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS) q->max_num_buffers = createbufs.max_num_buffers; q->buffers += createbufs.count; - return v4l_queue_querybufs(f, q, q->buffers - createbufs.count); + return v4l_queue_querybufs(f, q, createbufs.index, createbufs.count); } static inline int v4l_queue_mmap_bufs(struct v4l_fd *f, diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp index 2f7a5058..c6a685eb 100644 --- a/utils/v4l2-compliance/v4l2-compliance.cpp +++ b/utils/v4l2-compliance/v4l2-compliance.cpp @@ -1466,6 +1466,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ printf("Buffer ioctls%s:\n", suffix); printf("\ttest VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: %s\n", ok(testReqBufs(&node))); printf("\ttest CREATE_BUFS maximum buffers: %s\n", ok(testCreateBufsMax(&node))); + printf("\ttest VIDIOC_REMOVE_BUFS: %s\n", ok(testRemoveBufs(&node))); // Reopen after each streaming test to reset the streaming state // in case of any errors in the preceeding test. node.reopen(); diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h index 0cfc9a37..b6e342f3 100644 --- a/utils/v4l2-compliance/v4l2-compliance.h +++ b/utils/v4l2-compliance/v4l2-compliance.h @@ -385,6 +385,7 @@ int testReadWrite(struct node *node); int testExpBuf(struct node *node); int testBlockingWait(struct node *node); int testCreateBufsMax(struct node *node); +int testRemoveBufs(struct node *node); // 32-bit architecture, 32/64-bit time_t tests int testTime32_64(struct node *node); diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp index 922b99b5..f71a3c0e 100644 --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp @@ -529,6 +529,105 @@ static int testCanSetSameTimings(struct node *node) return 0; } +int testRemoveBufs(struct node *node) +{ + int ret; + unsigned i; + + node->reopen(); + + for (i = 1; i <= V4L2_BUF_TYPE_LAST; i++) { + struct v4l2_remove_buffers removebufs = { }; + + if (!(node->valid_buftypes & (1 << i))) + continue; + + cv4l_queue q(i, V4L2_MEMORY_MMAP); + + if (testSetupVbi(node, i)) + continue; + ret = q.remove_bufs(node, 0, 0); + if (ret == ENOTTY) { + warn("VIDIOC_REMOVE_BUFS not supported\n"); + continue; + } + + q.init(i, V4L2_MEMORY_MMAP); + ret = q.create_bufs(node, 0); + fail_on_test_val(ret && ret != EINVAL, ret); + + memset(&removebufs, 0xff, sizeof(removebufs)); + removebufs.index = 0; + removebufs.count = 0; + removebufs.type = q.g_type(); + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs)); + fail_on_test(check_0(removebufs.reserved, sizeof(removebufs.reserved))); + + if (!ret) { + unsigned buffers; + buffer buf(i); + + /* Create only 1 buffer */ + fail_on_test(q.create_bufs(node, 1)); + buffers = q.g_buffers(); + fail_on_test(buffers == 0); + /* Removing buffer index 1 must fail */ + fail_on_test(q.remove_bufs(node, 1, buffers) != EINVAL); + /* Removing buffer index 0 is valid */ + fail_on_test(q.remove_bufs(node, 0, buffers)); + /* Removing buffer index 0 again must fail */ + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); + /* Create 3 buffers indexes 0 to 2 */ + fail_on_test(q.create_bufs(node, 3)); + /* Remove them one by one */ + fail_on_test(q.remove_bufs(node, 2, 1)); + fail_on_test(q.remove_bufs(node, 0, 1)); + fail_on_test(q.remove_bufs(node, 1, 1)); + /* Removing buffer index 0 again must fail */ + fail_on_test(q.remove_bufs(node, 0, 1) != EINVAL); + + /* for the next test the queue needs to be able to allocate 7 buffers */ + if (q.g_max_num_buffers() < 7) + continue; + + /* Create 4 buffers indexes 0 to 3 */ + fail_on_test(q.create_bufs(node, 4)); + /* Remove buffers index 1 and 2 */ + fail_on_test(q.remove_bufs(node, 1, 2)); + /* Add 3 more buffers should be indexes 4 to 6 */ + fail_on_test(q.create_bufs(node, 3)); + /* Query buffers: + * 1 and 2 have been removed they must fail + * 0 and 4 to 6 must exist*/ + fail_on_test(buf.querybuf(node, 0)); + fail_on_test(buf.querybuf(node, 1) != EINVAL); + fail_on_test(buf.querybuf(node, 2) != EINVAL); + fail_on_test(buf.querybuf(node, 4)); + fail_on_test(buf.querybuf(node, 5)); + fail_on_test(buf.querybuf(node, 6)); + /* Remove existing buffer index 6 with bad type must fail */ + memset(&removebufs, 0xff, sizeof(removebufs)); + removebufs.index = 6; + removebufs.count = 1; + removebufs.type = 0; + fail_on_test(doioctl(node, VIDIOC_REMOVE_BUFS, &removebufs) != EINVAL); + + /* Remove crossing max allowed buffers boundary must fail */ + fail_on_test(q.remove_bufs(node, q.g_max_num_buffers() - 2, 7) != EINVAL); + + /* Remove overflow must fail */ + fail_on_test(q.remove_bufs(node, 3, 0xfffffff) != EINVAL); + + /* Create 2 buffers, that must fill the hole */ + fail_on_test(q.create_bufs(node, 2)); + /* Remove all buffers */ + fail_on_test(q.remove_bufs(node, 0, 7)); + } + } + + return 0; +} + int testReqBufs(struct node *node) { struct v4l2_create_buffers crbufs = { };
Add new test for REMOVE_BUFS ioctl. It create buffers and check if they could be removed from queue. It also check that removing non existing buffer or a queued buffer failed. Since using REMOVE_BUFS can create "holes" v4l_queue_querybufs() function needs to be modify to do a range check between [from..from+count-1]. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- utils/common/cv4l-helpers.h | 4 + utils/common/v4l-helpers.h | 27 ++++-- utils/v4l2-compliance/v4l2-compliance.cpp | 1 + utils/v4l2-compliance/v4l2-compliance.h | 1 + utils/v4l2-compliance/v4l2-test-buffers.cpp | 99 +++++++++++++++++++++ 5 files changed, 127 insertions(+), 5 deletions(-)