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