On 27/10/2023 10:47, Benjamin Gaignard wrote:
> If V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS is set v4l2_create_buffers
> 'max_num_buffers' field reports the maximum number of buffers supported
> by the queue.
> Add a test to allocate this maximum value and make sure that one
> more allocation is failing.
> Display the flag in v4l2-ctl.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> ---
> utils/common/cv4l-helpers.h | 1 +
> utils/common/v4l-helpers.h | 5 +++
> utils/common/v4l2-info.cpp | 1 +
> utils/v4l2-compliance/v4l2-compliance.cpp | 1 +
> utils/v4l2-compliance/v4l2-compliance.h | 1 +
> utils/v4l2-compliance/v4l2-test-buffers.cpp | 42 ++++++++++++++++++++-
> 6 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/utils/common/cv4l-helpers.h b/utils/common/cv4l-helpers.h
> index 91a04146..5a515a13 100644
> --- a/utils/common/cv4l-helpers.h
> +++ b/utils/common/cv4l-helpers.h
> @@ -743,6 +743,7 @@ public:
> unsigned g_type() const { return v4l_queue_g_type(this); }
> unsigned g_memory() const { return v4l_queue_g_memory(this); }
> unsigned g_buffers() const { return v4l_queue_g_buffers(this); }
> + unsigned g_max_buffers() const { return v4l_queue_g_max_buffers(this); }
Let's call this g_max_num_buffers...
> unsigned g_num_planes() const { return v4l_queue_g_num_planes(this); }
> unsigned g_capabilities() const { return v4l_queue_g_capabilities(this); }
> unsigned g_length(unsigned plane) const { return v4l_queue_g_length(this, plane); }
> diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h
> index f8e96d58..550ec377 100644
> --- a/utils/common/v4l-helpers.h
> +++ b/utils/common/v4l-helpers.h
> @@ -1429,6 +1429,7 @@ struct v4l_queue {
> unsigned mappings;
> unsigned num_planes;
> unsigned capabilities;
> + unsigned max_buffers;
...and this max_num_buffers.
No need to rename this from what videodev2.h uses.
Same elsewhere in this patch.
>
> __u32 lengths[VIDEO_MAX_PLANES];
> __u32 mem_offsets[VIDEO_MAX_FRAME][VIDEO_MAX_PLANES];
> @@ -1453,6 +1454,7 @@ static inline void v4l_queue_init(struct v4l_queue *q,
> static inline unsigned v4l_queue_g_type(const struct v4l_queue *q) { return q->type; }
> static inline unsigned v4l_queue_g_memory(const struct v4l_queue *q) { return q->memory; }
> static inline unsigned v4l_queue_g_buffers(const struct v4l_queue *q) { return q->buffers; }
> +static inline unsigned v4l_queue_g_max_buffers(const struct v4l_queue *q) { return q->max_buffers; }
> static inline unsigned v4l_queue_g_mappings(const struct v4l_queue *q) { return q->mappings; }
> static inline unsigned v4l_queue_g_num_planes(const struct v4l_queue *q) { return q->num_planes; }
> static inline unsigned v4l_queue_g_capabilities(const struct v4l_queue *q) { return q->capabilities; }
> @@ -1587,6 +1589,9 @@ static inline int v4l_queue_create_bufs(struct v4l_fd *f,
> if (ret)
> return ret;
> q->capabilities = createbufs.capabilities;
> + q->max_buffers = 32;
> + if (q->capabilities & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS)
> + q->max_buffers = createbufs.max_num_buffers;
> q->buffers += createbufs.count;
> return v4l_queue_querybufs(f, q, q->buffers - createbufs.count);
> }
> diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp
> index 4f8c2aa7..b11f2b77 100644
> --- a/utils/common/v4l2-info.cpp
> +++ b/utils/common/v4l2-info.cpp
> @@ -206,6 +206,7 @@ static constexpr flag_def bufcap_def[] = {
> { V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS, "orphaned-bufs" },
> { V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF, "m2m-hold-capture-buf" },
> { V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS, "mmap-cache-hints" },
> + { V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS, "set-max-num-bufs" },
bufs -> buffers
> { 0, nullptr }
> };
>
> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
> index ac6a8f80..a165ef5a 100644
> --- a/utils/v4l2-compliance/v4l2-compliance.cpp
> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp
> @@ -1453,6 +1453,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)));
> // 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 7e3860c8..4f9aa17e 100644
> --- a/utils/v4l2-compliance/v4l2-compliance.h
> +++ b/utils/v4l2-compliance/v4l2-compliance.h
> @@ -383,6 +383,7 @@ int testReqBufs(struct node *node);
> int testReadWrite(struct node *node);
> int testExpBuf(struct node *node);
> int testBlockingWait(struct node *node);
> +int testCreateBufsMax(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 6d592c9b..02ea498f 100644
> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -634,7 +634,6 @@ int testReqBufs(struct node *node)
> fail_on_test(testQueryBuf(node, i, q.g_buffers()));
> node->valid_memorytype |= 1 << V4L2_MEMORY_DMABUF;
> }
> -
> /*
> * It should be possible to set the same std, timings or
> * native size even after reqbufs was called.
Spurious deleted newline.
> @@ -761,6 +760,47 @@ int testReqBufs(struct node *node)
> return 0;
> }
>
> +int testCreateBufsMax(struct node *node)
> +{
> + unsigned int i;
> + int ret;
> +
> + node->reopen();
> +
> + cv4l_queue q(0, 0);
> +
> + for (i = 1; i <= V4L2_BUF_TYPE_LAST; i++) {
> + if (!(node->valid_buftypes & (1 << i)))
> + continue;
> +
> + q.init(i, V4L2_MEMORY_USERPTR);
There is no need to test this for all streaming modes. MMAP will suffice.
> + ret = q.create_bufs(node, 1);
> + if (!ret && (q.g_capabilities() & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS)) {
> + fail_on_test(q.create_bufs(node, q.g_max_buffers() - q.g_buffers()));
> + ret = q.create_bufs(node, 1);
> + fail_on_test(ret != ENOBUFS && (q.g_max_buffers() == q.g_buffers()));
> + }
> +
> + q.init(i, V4L2_MEMORY_MMAP);
> + ret = q.create_bufs(node, 1);
Use 0 instead of 1: CREATE_BUFS will in that case only return the capabilities
without attempting to allocate anything. It is really what you want here.
> + if (!ret && (q.g_capabilities() & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS)) {
> + fail_on_test(q.create_bufs(node, q.g_max_buffers() - q.g_buffers()));
And in that case you can drop the '- q.g_buffers()' part as well, since no buffers
will be allocated yet.
I think you want to get the return code of q.create_bufs: if it is ENOMEM you
want to report a different fail message, ideally something that indicated the
amount of memory that is needed for this. That's based on the current format
image size and the g_max_num_buffers value.
> + ret = q.create_bufs(node, 1);
> + fail_on_test(ret != ENOBUFS && (q.g_max_buffers() == q.g_buffers()));
Wouldn't it be enough to test 'ret' here? Did you need the second part of this check?
> + }
> +
> + q.init(i, V4L2_MEMORY_DMABUF);
> + ret = q.create_bufs(node, 1);
> + if (!ret && (q.g_capabilities() & V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS)) {
> + fail_on_test(q.create_bufs(node, q.g_max_buffers() - q.g_buffers()));
> + ret = q.create_bufs(node, 1);
> + fail_on_test(ret != ENOBUFS && (q.g_max_buffers() == q.g_buffers()));
> + }
> + }
> +
> + return 0;
> +}
> +
> int testExpBuf(struct node *node)
> {
> bool have_expbuf = false;
Regards,
Hans
