[{"id":37051,"web_url":"https://patchwork.libcamera.org/comment/37051/","msgid":"<176409018066.567526.14226923225807685719@ping.linuxembedded.co.uk>","date":"2025-11-25T17:03:00","subject":"Re: [PATCH v3 03/29] libcamera: Add support for V4L2 requests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-11-25 16:28:15)\n> The V4L2 requests API provides support to atomically tie controls to a\n> set of buffers. This is especially common for m2m devices. Such a\n> request is represented by an fd that is allocated via\n> MEDIA_IOC_REQUEST_ALLOC and then passed to the various V4L2 functions.\n> \n> Implement a V4L2Request class to wrap such an fd and add the\n> corresponding utility functions.\n> \n> Signed-off-by: Stefan Klug \n> \n\nMostly minor comments below.\n\nOnly my question on safety of usage of the vector on allocate stops me\nputting a tag in... if someone confirms that is safe then I could add\none ;-)\n\n\n\n> ---\n> \n> Changes in v3:\n> - Replaced int by UniqueFD\n> - Fixed lots if typos from review\n> \n> Changes in v2:\n> - Added documentation\n> ---\n> include/libcamera/internal/media_device.h | 8 ++\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/internal/v4l2_device.h | 5 +-\n> include/libcamera/internal/v4l2_request.h | 44 ++++++\n> include/libcamera/internal/v4l2_videodevice.h | 3 +-\n> src/libcamera/media_device.cpp | 47 +++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/v4l2_device.cpp | 30 +++-\n> src/libcamera/v4l2_request.cpp | 128 ++++++++++++++++++\n> src/libcamera/v4l2_videodevice.cpp | 10 +-\n> 10 files changed, 268 insertions(+), 9 deletions(-)\n> create mode 100644 include/libcamera/internal/v4l2_request.h\n> create mode 100644 src/libcamera/v4l2_request.cpp\n> \n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index b3a48b98d64b..2eb3ad988b09 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -8,6 +8,7 @@\n> #pragma once\n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -18,6 +19,7 @@\n> #include \n> \n> #include \"libcamera/internal/media_object.h\"\n> +#include \"libcamera/internal/v4l2_request.h\"\n> \n> namespace libcamera {\n> \n> @@ -57,6 +59,11 @@ public:\n> \n> std::vector locateEntities(unsigned int function);\n> \n> + int allocateRequests(unsigned int count,\n> + std::vector> *requests);\n> +\n> + bool supportsRequests();\n> +\n> protected:\n> std::string logPrefix() const override;\n> \n> @@ -87,6 +94,7 @@ private:\n> UniqueFD fd_;\n> bool valid_;\n> bool acquired_;\n> + std::optional supportsRequests_;\n\nstd::optional is intriguing here... I'd default false until known\notherwise...\n\n> std::map objects_;\n> std::vector entities_;\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 45c299f6a332..e9540a2f734f 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -44,6 +44,7 @@ libcamera_internal_headers = files([\n> 'sysfs.h',\n> 'v4l2_device.h',\n> 'v4l2_pixelformat.h',\n> + 'v4l2_request.h',\n> 'v4l2_subdevice.h',\n> 'v4l2_videodevice.h',\n> 'vector.h',\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 5bc9da96677d..dbbd118abd00 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -24,6 +24,7 @@\n> #include \n> \n> #include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/v4l2_request.h\"\n> \n> namespace libcamera {\n> \n> @@ -37,8 +38,8 @@ public:\n> \n> const ControlInfoMap &controls() const { return controls_; }\n> \n> - ControlList getControls(Span ids);\n> - int setControls(ControlList *ctrls);\n> + ControlList getControls(Span ids, const V4L2Request *request = nullptr);\n> + int setControls(ControlList *ctrls, const V4L2Request *request = nullptr);\n> \n> const struct v4l2_query_ext_ctrl *controlInfo(uint32_t id) const;\n> \n> diff --git a/include/libcamera/internal/v4l2_request.h b/include/libcamera/internal/v4l2_request.h\n> new file mode 100644\n> index 000000000000..376c79ceedba\n> --- /dev/null\n> +++ b/include/libcamera/internal/v4l2_request.h\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board\n> + *\n> + * V4L2 requests\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Request : protected Loggable\n> +{\n> +public:\n> + bool isValid() const { return fd_.isValid(); }\n> + int fd() const { return fd_.get(); }\n> +\n> + int reinit();\n> + int queue();\n> +\n> + V4L2Request(UniqueFD &&fd);\n> +\n> + Signal requestDone;\n> +\n> +private:\n> + LIBCAMERA_DISABLE_COPY_AND_MOVE(V4L2Request)\n> +\n> + void requestReady();\n> + std::string logPrefix() const override;\n> +\n> + UniqueFD fd_;\n> + EventNotifier fdNotifier_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 6caafc4dcf08..57db0036db6b 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -33,6 +33,7 @@\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/v4l2_device.h\"\n> #include \"libcamera/internal/v4l2_pixelformat.h\"\n> +#include \"libcamera/internal/v4l2_request.h\"\n> \n> namespace libcamera {\n> \n> @@ -217,7 +218,7 @@ public:\n> int importBuffers(unsigned int count);\n> int releaseBuffers();\n> \n> - int queueBuffer(FrameBuffer *buffer);\n> + int queueBuffer(FrameBuffer *buffer, const V4L2Request *request = nullptr);\n> Signal bufferReady;\n> \n> int streamOn();\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 353f34a81eca..2a848ebed998 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -20,6 +20,7 @@\n> #include \n> \n> #include \n> +#include \"libcamera/internal/v4l2_request.h\"\n> \n> /**\n> * \\file media_device.h\n> @@ -851,4 +852,50 @@ std::vector MediaDevice::locateEntities(unsigned int function)\n> return found;\n> }\n> \n> +/**\n> + * \\brief Allocate requests\n> + * \\param[in] count Number of requests to allocate\n> + * \\param[out] requests Vector to store allocated requests\n> + *\n> + * Allocates and stores \\a count requests in \\a requests. If allocation fails,\n> + * an error is returned and \\a requests is cleared.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int MediaDevice::allocateRequests(unsigned int count,\n> + std::vector> *requests)\n> +{\n> + requests->resize(count);\n> + for (unsigned int i = 0; i < count; i++) {\n> + int requestFd;\n> + int ret = ::ioctl(fd_.get(), MEDIA_IOC_REQUEST_ALLOC, &requestFd);\n> + if (ret < 0) {\n> + requests->clear();\n> + return -errno;\n> + }\n> + (*requests)[i] = std::make_unique(UniqueFD(requestFd));\n\nIs this line safe on a vector? Is the size pre-determined before coming\nin here? Or should this be something like:\n\n\trequests->push_back(std::make_unique(UniqueFD(requestFd));\n\nI'm sure that's how we use vectors elsewhere...\n\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +/**\n> + * \\brief Check if requests are supported\n> + *\n> + * Checks if the device supports V4L2 requests by trying to allocate a single\n> + * request. The result is cached, so the allocation is only tried once.\n> + *\n> + * \\return True if the device supports requests, false otherwise\n> + */\n> +bool MediaDevice::supportsRequests()\n> +{\n> + if (supportsRequests_.has_value())\n\nOhhh now I see why it's an optional. Indeed.\n\n> + return supportsRequests_.value();\n> +\n> + std::vector> requests;\n> + supportsRequests_ = (allocateRequests(1, &requests) == 0);\n> +\n> + return supportsRequests_.value();\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 5b9b86f211f1..34e20f557514 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -54,6 +54,7 @@ libcamera_internal_sources = files([\n> 'sysfs.cpp',\n> 'v4l2_device.cpp',\n> 'v4l2_pixelformat.cpp',\n> + 'v4l2_request.cpp',\n> 'v4l2_subdevice.cpp',\n> 'v4l2_videodevice.cpp',\n> 'vector.cpp',\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 8c78b8c424e5..8dcd5e618938 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -162,6 +162,7 @@ void V4L2Device::close()\n> /**\n> * \\brief Read controls from the device\n> * \\param[in] ids The list of controls to read, specified by their ID\n> + * \\param[in] request An optional request\n> *\n> * This function reads the value of all controls contained in \\a ids, and\n> * returns their values as a ControlList.\n> @@ -171,10 +172,12 @@ void V4L2Device::close()\n> * during validation of the requested controls, no control is read and this\n> * function returns an empty control list.\n> *\n> + * If \\a request is specified the controls tied to that request are read.\n> + *\n> * \\return The control values in a ControlList on success, or an empty list on\n> * error\n> */\n> -ControlList V4L2Device::getControls(Span ids)\n> +ControlList V4L2Device::getControls(Span ids, const V4L2Request *request)\n> {\n> if (ids.empty())\n> return {};\n> @@ -242,10 +245,16 @@ ControlList V4L2Device::getControls(Span ids)\n> }\n> \n> struct v4l2_ext_controls v4l2ExtCtrls = {};\n> - v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> v4l2ExtCtrls.controls = v4l2Ctrls.data();\n> v4l2ExtCtrls.count = v4l2Ctrls.size();\n> \n> + if (request) {\n> + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_REQUEST_VAL;\n> + v4l2ExtCtrls.request_fd = request->fd();\n> + } else {\n> + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> + }\n> +\n> int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> if (ret) {\n> unsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> @@ -273,6 +282,7 @@ ControlList V4L2Device::getControls(Span ids)\n> /**\n> * \\brief Write controls to the device\n> * \\param[in] ctrls The list of controls to write\n> + * \\param[in] request An optional request\n> *\n> * This function writes the value of all controls contained in \\a ctrls, and\n> * stores the values actually applied to the device in the corresponding\n> @@ -288,11 +298,15 @@ ControlList V4L2Device::getControls(Span ids)\n> * are written and their values are updated in \\a ctrls, while all other\n> * controls are not written and their values are not changed.\n> *\n> + * If \\a request is set, the controls will be applied to that request. If the\n> + * device doesn't support requests, -EACCESS will be returned. If \\a request is\n> + * invalid, -EINVAL will be returned.\n> + *\n> * \\return 0 on success or an error code otherwise\n> - * \\retval -EINVAL One of the control is not supported or not accessible\n> + * \\retval -EINVAL One of the controls is not supported or not accessible\n\nMissing adding\n\n \\retval -EACCESS the device does not support requests\n\n> * \\retval i The index of the control that failed\n> */\n> -int V4L2Device::setControls(ControlList *ctrls)\n> +int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request)\n> {\n> if (ctrls->empty())\n> return 0;\n> @@ -377,10 +391,16 @@ int V4L2Device::setControls(ControlList *ctrls)\n> }\n> \n> struct v4l2_ext_controls v4l2ExtCtrls = {};\n> - v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> v4l2ExtCtrls.controls = v4l2Ctrls.data();\n> v4l2ExtCtrls.count = v4l2Ctrls.size();\n> \n> + if (request) {\n> + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_REQUEST_VAL;\n> + v4l2ExtCtrls.request_fd = request->fd();\n> + } else {\n> + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> + }\n> +\n> int ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);\n> if (ret) {\n> unsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> diff --git a/src/libcamera/v4l2_request.cpp b/src/libcamera/v4l2_request.cpp\n> new file mode 100644\n> index 000000000000..9441d7c4017b\n> --- /dev/null\n> +++ b/src/libcamera/v4l2_request.cpp\n> @@ -0,0 +1,128 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board\n> + *\n> + * V4L2 Request API\n> + */\n> +\n> +#include \"libcamera/internal/v4l2_request.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +/**\n> + * \\file v4l2_request.h\n> + * \\brief V4L2 Request\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(V4L2)\n> +\n> +/**\n> + * \\class V4L2Request\n> + * \\brief V4L2Request object and API\n> + *\n> + * The V4L2Request class wraps a V4L2 request fd and provides some convenience\n> + * functions to handle request.\n> + *\n> + * It is usually constructed by calling \\a MediaDevice::allocateRequests().\n> + *\n> + * A request can then be passed to the V4L2Device::setControls(),\n> + * V4L2Device::getControls() and V4L2VideoDevice::queueBuffer().\n> + */\n> +\n> +/**\n> + * \\brief Construct a V4L2Request\n> + * \\param[in] fd The request fd\n> + */\n> +V4L2Request::V4L2Request(UniqueFD &&fd)\n> + : fd_(std::move(fd)), fdNotifier_(fd_.get(), EventNotifier::Exception)\n> +{\n> + if (!fd_.isValid())\n> + return;\n> +\n> + fdNotifier_.activated.connect(this, &V4L2Request::requestReady);\n> + fdNotifier_.setEnabled(false);\n> +}\n> +\n> +/**\n> + * \\fn V4L2Request::isValid()\n> + * \\brief Check if the request is valid\n> + *\n> + * Checks if the underlying fd is valid.\n> + *\n> + * \\return True if the request is valid, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn V4L2Request::fd()\n> + * \\brief Get the file descriptor\n> + *\n> + * \\return The file descriptor wrapped by this V4L2Request\n> + */\n> +\n> +/**\n> + * \\var V4L2Request::requestDone\n> + * \\brief Signal that is emitted when the request is done\n> + */\n> +\n> +/**\n> + * \\brief Reinit the request\n> + *\n> + * Calls MEDIA_REQUEST_IOC_REINIT on the request fd.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Request::reinit()\n> +{\n> + fdNotifier_.setEnabled(false);\n> +\n> + if (::ioctl(fd_.get(), MEDIA_REQUEST_IOC_REINIT) < 0)\n> + return -errno;\n> +\n> + return 0;\n> +}\n> +\n> +/**\n> + * \\brief Queue the request\n> + *\n> + * Calls MEDIA_REQUEST_IOC_QUEUE on the request fd.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Request::queue()\n> +{\n> + if (::ioctl(fd_.get(), MEDIA_REQUEST_IOC_QUEUE) < 0)\n> + return -errno;\n> +\n> + fdNotifier_.setEnabled(true);\n> +\n> + return 0;\n> +}\n> +\n> +std::string V4L2Request::logPrefix() const\n> +{\n> + return \"Request [\" + std::to_string(fd()) + \"]\";\n> +}\n> +\n> +/**\n> + * \\brief Slot to handle request done events\n> + *\n> + * When this slot is called, the request is done and the requestDone will be\n> + * emitted.\n> + */\n> +void V4L2Request::requestReady()\n> +{\n> + requestDone.emit(this);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 7b48d911db73..25b61d049a0e 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -30,6 +30,7 @@\n> #include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/media_object.h\"\n> +#include \"libcamera/internal/v4l2_request.h\"\n> \n> /**\n> * \\file v4l2_videodevice.h\n> @@ -1629,6 +1630,7 @@ int V4L2VideoDevice::releaseBuffers()\n> /**\n> * \\brief Queue a buffer to the video device\n> * \\param[in] buffer The buffer to be queued\n> + * \\param[in] request An optional request\n> *\n> * For capture video devices the \\a buffer will be filled with data by the\n> * device. For output video devices the \\a buffer shall contain valid data and\n> @@ -1641,9 +1643,11 @@ int V4L2VideoDevice::releaseBuffers()\n> * Note that queueBuffer() will fail if the device is in the process of being\n> * stopped from a streaming state through streamOff().\n> *\n> + * If \\a request is specified, the buffer will be tied to that request.\n> + *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, const V4L2Request *request)\n> {\n> struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};\n> struct v4l2_buffer buf = {};\n> @@ -1674,6 +1678,10 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> buf.type = bufferType_;\n> buf.memory = memoryType_;\n> buf.field = V4L2_FIELD_NONE;\n> + if (request) {\n> + buf.flags = V4L2_BUF_FLAG_REQUEST_FD;\n> + buf.request_fd = request->fd();\n> + }\n> \n> bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> Span planes = buffer->planes();\n> -- \n> 2.51.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8B08EC3257\n\tfor ;\n\tTue, 25 Nov 2025 17:03:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43BF360A9E;\n\tTue, 25 Nov 2025 18:03:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43C7F609D8\n\tfor ;\n\tTue, 25 Nov 2025 18:03:03 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32987524;\n\tTue, 25 Nov 2025 18:00:54 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nUQAfXq8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764090054;\n\tbh=MpcJvPaC+d9hgFka0zChbu5+9c/1phszurEm5SZHAoo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nUQAfXq8FlaR+QwE4LBH9OMUwJigINVPB5yeaWeF4kIPEDjmeIk7jVZrrrSI2IShQ\n\t9rBqYLqqTqt8+x+EB7NBWeMIBHFHx37yZgBC/IdhyxtiXUhkSqVeKtslcyuF0C7WNs\n\t0Xgxx4zx8Kaei4S/hc8gc4EDnIQ8OkrOL8ApngZM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251125162851.2301793-4-stefan.klug@ideasonboard.com>","References":"<20251125162851.2301793-1-stefan.klug@ideasonboard.com>\n\t<20251125162851.2301793-4-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v3 03/29] libcamera: Add support for V4L2 requests","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 25 Nov 2025 17:03:00 +0000","Message-ID":"<176409018066.567526.14226923225807685719@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37066,"web_url":"https://patchwork.libcamera.org/comment/37066/","msgid":"<176409475004.567526.14606863082713701147@ping.linuxembedded.co.uk>","date":"2025-11-25T18:19:10","subject":"Re: [PATCH v3 03/29] libcamera: Add support for V4L2 requests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-11-25 17:03:00)\n> Quoting Stefan Klug (2025-11-25 16:28:15)\n> > The V4L2 requests API provides support to atomically tie controls to a\n> > set of buffers. This is especially common for m2m devices. Such a\n> > request is represented by an fd that is allocated via\n> > MEDIA_IOC_REQUEST_ALLOC and then passed to the various V4L2 functions.\n> > \n> > Implement a V4L2Request class to wrap such an fd and add the\n> > corresponding utility functions.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> \n> Mostly minor comments below.\n> \n> Only my question on safety of usage of the vector on allocate stops me\n> putting a tag in... if someone confirms that is safe then I could add\n> one ;-)\n> \n> \n> \n> > ---\n> > \n> > Changes in v3:\n> > - Replaced int by UniqueFD\n> > - Fixed lots if typos from review\n> > \n> > Changes in v2:\n> > - Added documentation\n> > ---\n> > include/libcamera/internal/media_device.h | 8 ++\n> > include/libcamera/internal/meson.build | 1 +\n> > include/libcamera/internal/v4l2_device.h | 5 +-\n> > include/libcamera/internal/v4l2_request.h | 44 ++++++\n> > include/libcamera/internal/v4l2_videodevice.h | 3 +-\n> > src/libcamera/media_device.cpp | 47 +++++++\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/v4l2_device.cpp | 30 +++-\n> > src/libcamera/v4l2_request.cpp | 128 ++++++++++++++++++\n> > src/libcamera/v4l2_videodevice.cpp | 10 +-\n> > 10 files changed, 268 insertions(+), 9 deletions(-)\n> > create mode 100644 include/libcamera/internal/v4l2_request.h\n> > create mode 100644 src/libcamera/v4l2_request.cpp\n> > \n> > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> > index b3a48b98d64b..2eb3ad988b09 100644\n> > --- a/include/libcamera/internal/media_device.h\n> > +++ b/include/libcamera/internal/media_device.h\n> > @@ -8,6 +8,7 @@\n> > #pragma once\n> > \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -18,6 +19,7 @@\n> > #include \n> > \n> > #include \"libcamera/internal/media_object.h\"\n> > +#include \"libcamera/internal/v4l2_request.h\"\n> > \n> > namespace libcamera {\n> > \n> > @@ -57,6 +59,11 @@ public:\n> > \n> > std::vector locateEntities(unsigned int function);\n> > \n> > + int allocateRequests(unsigned int count,\n> > + std::vector> *requests);\n> > +\n> > + bool supportsRequests();\n> > +\n> > protected:\n> > std::string logPrefix() const override;\n> > \n> > @@ -87,6 +94,7 @@ private:\n> > UniqueFD fd_;\n> > bool valid_;\n> > bool acquired_;\n> > + std::optional supportsRequests_;\n> \n> std::optional is intriguing here... I'd default false until known\n> otherwise...\n> \n> > std::map objects_;\n> > std::vector entities_;\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 45c299f6a332..e9540a2f734f 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -44,6 +44,7 @@ libcamera_internal_headers = files([\n> > 'sysfs.h',\n> > 'v4l2_device.h',\n> > 'v4l2_pixelformat.h',\n> > + 'v4l2_request.h',\n> > 'v4l2_subdevice.h',\n> > 'v4l2_videodevice.h',\n> > 'vector.h',\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index 5bc9da96677d..dbbd118abd00 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -24,6 +24,7 @@\n> > #include \n> > \n> > #include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/v4l2_request.h\"\n> > \n> > namespace libcamera {\n> > \n> > @@ -37,8 +38,8 @@ public:\n> > \n> > const ControlInfoMap &controls() const { return controls_; }\n> > \n> > - ControlList getControls(Span ids);\n> > - int setControls(ControlList *ctrls);\n> > + ControlList getControls(Span ids, const V4L2Request *request = nullptr);\n> > + int setControls(ControlList *ctrls, const V4L2Request *request = nullptr);\n> > \n> > const struct v4l2_query_ext_ctrl *controlInfo(uint32_t id) const;\n> > \n> > diff --git a/include/libcamera/internal/v4l2_request.h b/include/libcamera/internal/v4l2_request.h\n> > new file mode 100644\n> > index 000000000000..376c79ceedba\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/v4l2_request.h\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2025, Ideas On Board\n> > + *\n> > + * V4L2 requests\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class V4L2Request : protected Loggable\n> > +{\n> > +public:\n> > + bool isValid() const { return fd_.isValid(); }\n> > + int fd() const { return fd_.get(); }\n> > +\n> > + int reinit();\n> > + int queue();\n> > +\n> > + V4L2Request(UniqueFD &&fd);\n> > +\n> > + Signal requestDone;\n> > +\n> > +private:\n> > + LIBCAMERA_DISABLE_COPY_AND_MOVE(V4L2Request)\n> > +\n> > + void requestReady();\n> > + std::string logPrefix() const override;\n> > +\n> > + UniqueFD fd_;\n> > + EventNotifier fdNotifier_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index 6caafc4dcf08..57db0036db6b 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -33,6 +33,7 @@\n> > #include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/v4l2_device.h\"\n> > #include \"libcamera/internal/v4l2_pixelformat.h\"\n> > +#include \"libcamera/internal/v4l2_request.h\"\n> > \n> > namespace libcamera {\n> > \n> > @@ -217,7 +218,7 @@ public:\n> > int importBuffers(unsigned int count);\n> > int releaseBuffers();\n> > \n> > - int queueBuffer(FrameBuffer *buffer);\n> > + int queueBuffer(FrameBuffer *buffer, const V4L2Request *request = nullptr);\n> > Signal bufferReady;\n> > \n> > int streamOn();\n> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > index 353f34a81eca..2a848ebed998 100644\n> > --- a/src/libcamera/media_device.cpp\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -20,6 +20,7 @@\n> > #include \n> > \n> > #include \n> > +#include \"libcamera/internal/v4l2_request.h\"\n> > \n> > /**\n> > * \\file media_device.h\n> > @@ -851,4 +852,50 @@ std::vector MediaDevice::locateEntities(unsigned int function)\n> > return found;\n> > }\n> > \n> > +/**\n> > + * \\brief Allocate requests\n> > + * \\param[in] count Number of requests to allocate\n> > + * \\param[out] requests Vector to store allocated requests\n> > + *\n> > + * Allocates and stores \\a count requests in \\a requests. If allocation fails,\n> > + * an error is returned and \\a requests is cleared.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int MediaDevice::allocateRequests(unsigned int count,\n> > + std::vector> *requests)\n> > +{\n> > + requests->resize(count);\n\nThere it is - it was hiding in plain sight. I missed it.\n\nReviewed-by: Kieran Bingham \n\n> > + for (unsigned int i = 0; i < count; i++) {\n> > + int requestFd;\n> > + int ret = ::ioctl(fd_.get(), MEDIA_IOC_REQUEST_ALLOC, &requestFd);\n> > + if (ret < 0) {\n> > + requests->clear();\n> > + return -errno;\n> > + }\n> > + (*requests)[i] = std::make_unique(UniqueFD(requestFd));\n> \n> Is this line safe on a vector? Is the size pre-determined before coming\n> in here? Or should this be something like:\n> \n> requests->push_back(std::make_unique(UniqueFD(requestFd));\n> \n> I'm sure that's how we use vectors elsewhere...\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5AB80C333E\n\tfor ;\n\tTue, 25 Nov 2025 18:19:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 84276609E0;\n\tTue, 25 Nov 2025 19:19:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB11B609D8\n\tfor ;\n\tTue, 25 Nov 2025 19:19:13 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9EB456AF;\n\tTue, 25 Nov 2025 19:17:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PluXxgyq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764094624;\n\tbh=QTUKDAAblQu2/3Eur9VxSAYkOyHRpoeerHzL7sc0iAY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=PluXxgyq7d5znXB1HO4JVQZMGEUuPq6kF3FAEjve13Eqna6+FApbdpMLwbFhS+3t3\n\tZDDruqpugPchT/QnIVhfLkr1n0/VYa3F0JhIC2rv6z8PZMozC7gssblKcty0XY7ot8\n\tJYwew3nNFq2VM9Irz3zLyuyL2Q5exiaJh38lelwo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176409018066.567526.14226923225807685719@ping.linuxembedded.co.uk>","References":"<20251125162851.2301793-1-stefan.klug@ideasonboard.com>\n\t<20251125162851.2301793-4-stefan.klug@ideasonboard.com>\n\t<176409018066.567526.14226923225807685719@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v3 03/29] libcamera: Add support for V4L2 requests","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 25 Nov 2025 18:19:10 +0000","Message-ID":"<176409475004.567526.14606863082713701147@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]