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@xxxxxxxxxxxxx>
> ---
> 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
