[{"id":3289,"web_url":"https://patchwork.libcamera.org/comment/3289/","msgid":"<20191231102209.jaqlwpjtcf6zs6uk@uno.localdomain>","date":"2019-12-31T10:22:09","subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n thanks for v4\n\nOn Mon, Dec 30, 2019 at 11:33:14PM -0600, Paul Elder wrote:\n> Add libcamera V4L2 compatibility layer.\n>\n> This initial implementation supports the minimal set of V4L2 operations,\n> which allows getting, setting, and enumerating formats, and streaming\n> frames from a video device. Some data about the wrapped V4L2 video\n> device are hardcoded.\n>\n> Add a build option named 'v4l2' and adjust the build system to\n> selectively compile the V4L2 compatibility layer.\n>\n> For now we match the V4L2 device node to a libcamera camera based on a\n> devnum that a pipeline handler may optionally map to a libcamera\n> camera.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Major changes in v4:\n> - squash buffer state tracking in V4L2CameraProxy (from 6/6 in v3)\n> - separated out V4L2CameraProxy::freeBuffers() (since it was cluttering\n> up V4L2CameraProxy::vidioc_reqbufs)\n>\n> Less major changes in v4:\n> - check that addresses passed to V4L2CameraProxy::munmap() have been\n> mmaped\n> - other cosmetic changes\n>\n> Major changes in v3:\n> - V4L2CompatManager::openat() verify video node and match to camera via\n> devnum instead of path\n> - add FrameMetadata class to encapsulate portions of the libcamera\n> Buffer that needs to be extracted to v4l2_buffer, and to prepare for\n> the Buffer rework\n> - V4L2CameraProxy refcount for tracking dups and closes (and open)\n> - add V4L2CompatManager::initialized_ to deal with race of waiting on\n> init cv after the cv is notified\n>\n> Less major changes in v3:\n> - change the list of pending Reqeusts (v4l2 buffers queued before\n> streamon) to unique pointers\n> - V4L2Camera::updateSizeImage() -> V4L2CameraProxy::calculateSizeImage()\n> - change V4L2CompatManager list of V4L2CameraProxy to unique_ptr, with\n> maps of fd/mmap addr to V4L2CameraProxy *\n> - removed cross-thread validation methods from V4L2CameraProxy\n> (validate[Stream|Memory]Type)\n> - removed hasPixelFormat() and hasSize() from V4L2CameraProxy\n> - moved mmap logic out of V4L2Camera and into V4L2CameraProxy\n> - moved nonblock logic out of V4L2Camera and into V4L2CameraProxy\n> - add todos\n> - oter cosmetic changes\n>\n> Changes in v2:\n> - move all errno acrobatics to V4L2CameraProxy\n> - remove all mentions of dmabuf\n> - make V4L2CompatManager::getCamera return pointer rather than\n> shared_ptr\n> - match V4L2 device nodes to libcamera cameras using Camera::name()\n> compared to /sys/dev/char/maj:min/name (only works for UVC cameras)\n> - in V4L2CompatManager::getCameraIndex()\n> - add -fvisibility=hidden to v4l2 compat\n> - cache the results of V4L2CompatManager::imageSize() within V4L2Camera\n> (where V4L2Camera is interested)\n> - remove V4L2CompatManager::valid[fd|mmap], and where those methods were\n> used, check V4L2CompatManager::getCamera() != nullptr instead\n> - fixed V4L2CompatManager::drmToV4L2() mappings for DRM_FORMAT_BGR888\n> and DRM_FORMAT_RGB888\n> - other cosmetic changes\n> ---\n> meson_options.txt | 5 +\n> src/meson.build | 4 +\n> src/v4l2/meson.build | 31 ++\n> src/v4l2/v4l2_camera.cpp | 233 +++++++++++\n> src/v4l2/v4l2_camera.h | 83 ++++\n> src/v4l2/v4l2_camera_proxy.cpp | 649 +++++++++++++++++++++++++++++++\n> src/v4l2/v4l2_camera_proxy.h | 82 ++++\n> src/v4l2/v4l2_compat.cpp | 80 ++++\n> src/v4l2/v4l2_compat_manager.cpp | 255 ++++++++++++\n> src/v4l2/v4l2_compat_manager.h | 77 ++++\n> 10 files changed, 1499 insertions(+)\n> create mode 100644 src/v4l2/meson.build\n> create mode 100644 src/v4l2/v4l2_camera.cpp\n> create mode 100644 src/v4l2/v4l2_camera.h\n> create mode 100644 src/v4l2/v4l2_camera_proxy.cpp\n> create mode 100644 src/v4l2/v4l2_camera_proxy.h\n> create mode 100644 src/v4l2/v4l2_compat.cpp\n> create mode 100644 src/v4l2/v4l2_compat_manager.cpp\n> create mode 100644 src/v4l2/v4l2_compat_manager.h\n>\n> diff --git a/meson_options.txt b/meson_options.txt\n> index 1a328045..79ee4de6 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -10,3 +10,8 @@ option('documentation',\n> option('test',\n> type : 'boolean',\n> description: 'Compile and include the tests')\n> +\n> +option('v4l2',\n> + type : 'boolean',\n> + value : false,\n> + description : 'Compile the V4L2 compatibility layer')\n> diff --git a/src/meson.build b/src/meson.build\n> index 67ad20aa..5adcd61f 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -6,3 +6,7 @@ subdir('libcamera')\n> subdir('ipa')\n> subdir('cam')\n> subdir('qcam')\n> +\n> +if get_option('v4l2')\n> + subdir('v4l2')\n> +endif\n> diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build\n> new file mode 100644\n> index 00000000..14ee3594\n> --- /dev/null\n> +++ b/src/v4l2/meson.build\n> @@ -0,0 +1,31 @@\n> +v4l2_compat_sources = files([\n> + 'v4l2_camera.cpp',\n> + 'v4l2_camera_proxy.cpp',\n> + 'v4l2_compat.cpp',\n> + 'v4l2_compat_manager.cpp',\n> +])\n> +\n> +v4l2_compat_includes = [\n> + libcamera_includes,\n> + libcamera_internal_includes,\n> +]\n> +\n> +v4l2_compat_cpp_args = [\n> + # Meson enables large file support unconditionally, which redirect file\n> + # operations to 64-bit versions. This results in some symbols being\n> + # renamed, for instance open() being renamed to open64(). As the V4L2\n> + # adaptation wrapper needs to provide both 32-bit and 64-bit versions of\n> + # file operations, disable transparent large file support.\n> + '-U_FILE_OFFSET_BITS',\n> + '-D_FILE_OFFSET_BITS=32',\n> + '-fvisibility=hidden',\n> +]\n> +\n> +v4l2_compat = shared_library('v4l2-compat',\n> + v4l2_compat_sources,\n> + name_prefix : '',\n> + install : true,\n> + link_with : libcamera,\n> + include_directories : v4l2_compat_includes,\n> + dependencies : libcamera_deps,\n> + cpp_args : v4l2_compat_cpp_args)\n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> new file mode 100644\n> index 00000000..82ada8d4\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -0,0 +1,233 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_camera.cpp - V4L2 compatibility camera\n> + */\n> +\n> +#include \"v4l2_camera.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(V4L2Compat);\n> +\n> +FrameMetadata::FrameMetadata(Buffer *buffer)\n> +\t: index_(buffer->index()), bytesused_(buffer->bytesused()),\n> +\t timestamp_(buffer->timestamp()), sequence_(buffer->sequence()),\n> +\t status_(buffer->status())\n> +{\n> +}\n> +\n> +V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> +\t: camera_(camera), isRunning_(false)\n> +{\n> +\tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> +}\n> +\n> +V4L2Camera::~V4L2Camera()\n> +{\n> +\tcamera_->release();\n> +}\n> +\n> +void V4L2Camera::open(int *ret)\n> +{\n> +\tif (camera_->acquire() < 0) {\n> +\t\tLOG(V4L2Compat, Error) << \"Failed to acquire camera\";\n> +\t\t*ret = -EINVAL;\n> +\t\treturn;\n> +\t}\n> +\n> +\tconfig_ = camera_->generateConfiguration({ StreamRole::Viewfinder });\n> +\tif (!config_) {\n> +\t\tcamera_->release();\n> +\t\t*ret = -EINVAL;\n> +\t\treturn;\n> +\t}\n> +\n> +\t*ret = 0;\n> +}\n> +\n> +void V4L2Camera::close()\n> +{\n> +\tcamera_->release();\n> +}\n> +\n> +void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)\n> +{\n> +\t*streamConfig = config_->at(0);\n> +}\n> +\n> +std::vector V4L2Camera::completedBuffers()\n> +{\n> +\tstd::vector v;\n> +\n> +\tbufferLock_.lock();\n> +\tfor (std::unique_ptr &metadata : completedBuffers_)\n> +\t\tv.push_back(*metadata.get());\n> +\tcompletedBuffers_.clear();\n> +\tbufferLock_.unlock();\n> +\n> +\treturn v;\n> +}\n> +\n> +void V4L2Camera::requestComplete(Request *request)\n> +{\n> +\tif (request->status() == Request::RequestCancelled)\n> +\t\treturn;\n> +\n> +\t/* We only have one stream at the moment. */\n> +\tbufferLock_.lock();\n> +\tBuffer *buffer = request->buffers().begin()->second;\n> +\tstd::unique_ptr metadata =\n> +\t\tutils::make_unique(buffer);\n> +\tcompletedBuffers_.push_back(std::move(metadata));\n> +\tbufferLock_.unlock();\n> +\n> +\tbufferSema_.release();\n\nThe usage of this semaphore from two different classes looks weird :/\n\n> +}\n> +\n> +void V4L2Camera::configure(int *ret, StreamConfiguration *streamConfigOut,\n> +\t\t\t Size size, PixelFormat pixelformat,\n> +\t\t\t unsigned int bufferCount)\n> +{\n> +\tStreamConfiguration &streamConfig = config_->at(0);\n> +\tstreamConfig.size.width = size.width;\n> +\tstreamConfig.size.height = size.height;\n> +\tstreamConfig.pixelFormat = pixelformat;\n> +\tstreamConfig.bufferCount = bufferCount;\n> +\t/* \\todo memoryType (interval vs external) */\n> +\n> +\tCameraConfiguration::Status validation = config_->validate();\n> +\tif (validation == CameraConfiguration::Invalid) {\n> +\t\tLOG(V4L2Compat, Error) << \"Configuration invalid\";\n> +\t\t*ret = -EINVAL;\n> +\t\treturn;\n> +\t}\n> +\tif (validation == CameraConfiguration::Adjusted)\n> +\t\tLOG(V4L2Compat, Info) << \"Configuration adjusted\";\n> +\n> +\tLOG(V4L2Compat, Info) << \"Validated configuration is: \"\n> +\t\t\t << streamConfig.toString();\n> +\n> +\t*ret = camera_->configure(config_.get());\n> +\tif (*ret < 0)\n> +\t\treturn;\n> +\n> +\t*streamConfigOut = config_->at(0);\n> +}\n> +\n> +void V4L2Camera::mmap(void **ret, unsigned int index)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing MMAP\";\n\nYou have removed most of these, but some are still here, is this\nintentional ?\n\n> +\n> +\tStream *stream = *camera_->streams().begin();\n> +\t*ret = stream->buffers()[index].planes()[0].mem();\n> +}\n> +\n> +void V4L2Camera::allocBuffers(int *ret, unsigned int count)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Allocating libcamera bufs\";\n> +\n> +\t*ret = camera_->allocateBuffers();\n> +\tif (*ret == -EACCES)\n> +\t\t*ret = -EBUSY;\n> +}\n> +\n> +void V4L2Camera::freeBuffers()\n> +{\n> +\tcamera_->freeBuffers();\n> +}\n> +\n> +void V4L2Camera::streamOn(int *ret)\n> +{\n> +\t*ret = 0;\n\nDo you need this ?\n\n> +\n> +\tif (isRunning_)\n> +\t\treturn;\n> +\n> +\t*ret = camera_->start();\n> +\tif (*ret < 0) {\n> +\t\tif (*ret == -EACCES)\n> +\t\t\t*ret = -EBUSY;\n> +\t\treturn;\n> +\t}\n> +\tisRunning_ = true;\n> +\n> +\tfor (std::unique_ptr &req : pendingRequests_) {\n> +\t\t/* \\todo What should we do if this returns -EINVAL? */\n> +\t\t*ret = camera_->queueRequest(req.release());\n> +\t\tif (*ret < 0) {\n> +\t\t\t*ret = (*ret == -EACCES ? -EAGAIN : *ret);\n\nA few lines below you have the same construct in the form of\n\n\tif (*ret < 0) {\n\t\tif (*ret == -EACCES)\n\t\t\t*ret = -EBUSY;\n\nplease unify on one of the two\n\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n> +\tpendingRequests_.clear();\n> +}\n> +\n> +void V4L2Camera::streamOff(int *ret)\n> +{\n> +\t*ret = 0;\n\ndo you need this ?\n\n> +\n> +\t/* \\todo restore buffers to reqbufs state? */\n> +\tif (!isRunning_)\n> +\t\treturn;\n> +\n> +\t*ret = camera_->stop();\n> +\tif (*ret < 0) {\n> +\t\tif (*ret == -EACCES)\n> +\t\t\t*ret = -EBUSY;\n> +\t\treturn;\n> +\t}\n> +\tisRunning_ = false;\n> +}\n> +\n> +void V4L2Camera::qbuf(int *ret, unsigned int index)\n> +{\n> +\tStream *stream = config_->at(0).stream();\n> +\tstd::unique_ptr buffer = stream->createBuffer(index);\n> +\tif (!buffer) {\n> +\t\tLOG(V4L2Compat, Error) << \"Can't create buffer\";\n> +\t\t*ret = -ENOMEM;\n> +\t\treturn;\n> +\t}\n> +\n> +\tstd::unique_ptr request =\n> +\t\tstd::unique_ptr(camera_->createRequest());\n> +\tif (!request) {\n> +\t\tLOG(V4L2Compat, Error) << \"Can't create request\";\n> +\t\t*ret = -ENOMEM;\n> +\t\treturn;\n> +\t}\n> +\n> +\t*ret = request->addBuffer(std::move(buffer));\n> +\tif (*ret < 0) {\n> +\t\tLOG(V4L2Compat, Error) << \"Can't set buffer for request\";\n> +\t\t*ret = -ENOMEM;\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!isRunning_) {\n> +\t\tpendingRequests_.push_back(std::move(request));\n\nYou can return here and remove the else branch below\nret should be set to 0 already by addBuffer()\n\n> +\t} else {\n> +\t\t*ret = camera_->queueRequest(request.release());\n> +\t\tif (*ret < 0) {\n> +\t\t\tLOG(V4L2Compat, Error) << \"Can't queue request\";\n> +\t\t\tif (*ret == -EACCES)\n> +\t\t\t\t*ret = -EBUSY;\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n> +\t*ret = 0;\n> +}\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> new file mode 100644\n> index 00000000..a48a2d59\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -0,0 +1,83 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_camera.h - V4L2 compatibility camera\n> + */\n> +\n> +#ifndef __V4L2_CAMERA_H__\n> +#define __V4L2_CAMERA_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n\nDo we separate publinc and internal inclusion directives with an empty line ?\n\n> +#include \"semaphore.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class FrameMetadata\n> +{\n> +public:\n> +\tFrameMetadata(Buffer *buffer);\n> +\n> +\tint index() const { return index_; }\n> +\n> +\tunsigned int bytesused() const { return bytesused_; }\n> +\tuint64_t timestamp() const { return timestamp_; }\n> +\tunsigned int sequence() const { return sequence_; }\n> +\n> +\tBuffer::Status status() const { return status_; }\n> +\n> +private:\n> +\tint index_;\n> +\n> +\tunsigned int bytesused_;\n> +\tuint64_t timestamp_;\n> +\tunsigned int sequence_;\n> +\n> +\tBuffer::Status status_;\n> +};\n> +\n> +class V4L2Camera : public Object\n> +{\n> +public:\n> +\tV4L2Camera(std::shared_ptr camera);\n> +\t~V4L2Camera();\n> +\n> +\tvoid open(int *ret);\n> +\tvoid close();\n> +\tvoid getStreamConfig(StreamConfiguration *streamConfig);\n> +\tstd::vector completedBuffers();\n> +\n> +\tvoid mmap(void **ret, unsigned int index);\n> +\n> +\tvoid configure(int *ret, StreamConfiguration *streamConfigOut,\n> +\t\t Size size, PixelFormat pixelformat,\n> +\t\t unsigned int bufferCount);\n> +\n> +\tvoid allocBuffers(int *ret, unsigned int count);\n> +\tvoid freeBuffers();\n> +\tvoid streamOn(int *ret);\n> +\tvoid streamOff(int *ret);\n> +\n> +\tvoid qbuf(int *ret, unsigned int index);\n> +\n> +\tSemaphore bufferSema_;\n> +private:\n> +\tvoid requestComplete(Request *request);\n> +\n> +\tstd::shared_ptr camera_;\n> +\tstd::unique_ptr config_;\n> +\n> +\tbool isRunning_;\n> +\n> +\tstd::mutex bufferLock_;\n> +\n> +\tstd::deque> pendingRequests_;\n> +\tstd::deque> completedBuffers_;\n> +};\n> +\n> +#endif /* __V4L2_CAMERA_H__ */\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> new file mode 100644\n> index 00000000..ae27ea1b\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -0,0 +1,649 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_camera_proxy.cpp - Proxy to V4L2 compatibility camera\n> + */\n> +\n> +#include \"v4l2_camera_proxy.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +#include \"v4l2_camera.h\"\n> +#include \"v4l2_compat_manager.h\"\n> +\n> +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(V4L2Compat);\n> +\n> +V4L2CameraProxy::V4L2CameraProxy(unsigned int index,\n> +\t\t\t\t std::shared_ptr camera)\n> +\t: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),\n> +\t vcam_(utils::make_unique(camera))\n> +{\n> +\tquerycap(camera);\n> +}\n> +\n> +int V4L2CameraProxy::open(bool nonBlocking)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing open\";\n> +\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,\n> +\t\t\t &ret);\n> +\tif (ret < 0) {\n> +\t\terrno = -ret;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tnonBlocking_ = nonBlocking;\n> +\n> +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> +\tsetFmtFromConfig(streamConfig_);\n> +\tsizeimage_ = calculateSizeImage(streamConfig_);\n> +\n> +\trefcount_++;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void V4L2CameraProxy::dup()\n> +{\n> +\trefcount_++;\n> +}\n> +\n> +void V4L2CameraProxy::close()\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing close\";\n> +\n> +\tif (--refcount_ > 0)\n> +\t\treturn;\n> +\n> +\tvcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking);\n> +\n> +\treturn;\n> +}\n> +\n> +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,\n> +\t\t\t off_t offset)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing mmap\";\n\nThis seems to be duplicated in V4L2Camera\n\n> +\n> +\tif (prot != (PROT_READ | PROT_WRITE)) {\n> +\t\terrno = ENOTSUP;\n> +\t\treturn MAP_FAILED;\n> +\t}\n> +\n> +\tunsigned int index = offset / sizeimage_;\n> +\tif (index * sizeimage_ != offset || length != sizeimage_) {\n> +\t\terrno = EINVAL;\n> +\t\treturn MAP_FAILED;\n> +\t}\n> +\n> +\tvoid *val;\n> +\tvcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,\n> +\t\t\t &val, index);\n> +\n> +\tbuffers_[index].flags |= V4L2_BUF_FLAG_MAPPED;\n> +\tmmaps_[val] = index;\n> +\n> +\treturn val;\n> +}\n> +\n> +int V4L2CameraProxy::munmap(void *addr, size_t length)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing munmap\";\n> +\n> +\tauto iter = mmaps_.find(addr);\n> +\tif (iter == mmaps_.end() || length != sizeimage_) {\n> +\t\terrno = EAGAIN;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tbuffers_[iter->second].flags &= ~V4L2_BUF_FLAG_MAPPED;\n> +\tmmaps_.erase(iter);\n> +\n> +\treturn 0;\n> +}\n> +\n> +bool V4L2CameraProxy::validateStreamType(uint32_t type)\n> +{\n> +\treturn type == V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +}\n> +\n> +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)\n> +{\n> +\treturn memory == V4L2_MEMORY_MMAP;\n> +}\n> +\n> +void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig)\n> +{\n> +\tcurV4L2Format_.fmt.pix.width = streamConfig.size.width;\n> +\tcurV4L2Format_.fmt.pix.height = streamConfig.size.height;\n> +\tcurV4L2Format_.fmt.pix.pixelformat = drmToV4L2(streamConfig.pixelFormat);\n> +\tcurV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;\n> +\tcurV4L2Format_.fmt.pix.bytesperline =\n> +\t\tbplMultiplier(curV4L2Format_.fmt.pix.pixelformat) *\n> +\t\tcurV4L2Format_.fmt.pix.width;\n> +\tcurV4L2Format_.fmt.pix.sizeimage =\n> +\t\timageSize(curV4L2Format_.fmt.pix.pixelformat,\n> +\t\t\t curV4L2Format_.fmt.pix.width,\n> +\t\t\t curV4L2Format_.fmt.pix.height);\n> +\tcurV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> +}\n> +\n> +unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig)\n> +{\n> +\treturn imageSize(drmToV4L2(streamConfig.pixelFormat),\n> +\t\t\t streamConfig.size.width,\n> +\t\t\t streamConfig.size.height);\n> +}\n> +\n> +void V4L2CameraProxy::querycap(std::shared_ptr camera)\n> +{\n> +\tstd::string driver = \"libcamera\";\n> +\tstd::string bus_info = driver + \":\" + std::to_string(index_);\n> +\n> +\tmemcpy(capabilities_.driver, driver.c_str(),\n> +\t sizeof(capabilities_.driver));\n> +\tmemcpy(capabilities_.card, camera->name().c_str(),\n> +\t sizeof(capabilities_.card));\n> +\tmemcpy(capabilities_.bus_info, bus_info.c_str(),\n> +\t sizeof(capabilities_.bus_info));\n> +\t/* \\todo Put this in a header/config somewhere. */\n> +\tcapabilities_.version = KERNEL_VERSION(5, 2, 0);\n> +\tcapabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;\n> +\tcapabilities_.capabilities = capabilities_.device_caps\n> +\t\t\t\t | V4L2_CAP_DEVICE_CAPS;\n> +\tmemset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));\n> +}\n> +\n> +void V4L2CameraProxy::updateBuffers()\n> +{\n> +\tstd::vector completedBuffers = vcam_->completedBuffers();\n\nThis should be a reference otherwise you copy the whole vector\neverytime you dequeue or query a buffer\n\n> +\tfor (FrameMetadata &fmd : completedBuffers) {\n> +\t\t/* \\todo is this index valid if the buffer status != success? */\n> +\t\tstruct v4l2_buffer &buf = buffers_[fmd.index()];\n> +\n> +\t\tswitch (fmd.status()) {\n> +\t\tcase Buffer::Status::BufferSuccess:\n> +\t\t\tbuf.index = fmd.index();\n> +\t\t\tbuf.bytesused = fmd.bytesused();\n> +\t\t\tbuf.field = V4L2_FIELD_NONE;\n> +\t\t\tbuf.timestamp.tv_sec = fmd.timestamp() / 1000000000;\n> +\t\t\tbuf.timestamp.tv_usec = fmd.timestamp() % 1000000;\n> +\t\t\tbuf.sequence = fmd.sequence();\n> +\n> +\t\t\tbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +\t\t\tbuf.length = curV4L2Format_.fmt.pix.sizeimage;\n> +\t\t\tbuf.memory = V4L2_MEMORY_MMAP;\n> +\t\t\tbuf.m.offset = buf.index * curV4L2Format_.fmt.pix.sizeimage;\n> +\n> +\t\t\tbuf.flags |= V4L2_BUF_FLAG_DONE;\n> +\t\t\tbreak;\n> +\t\tcase Buffer::Status::BufferError:\n> +\t\t\tbuf.flags |= V4L2_BUF_FLAG_ERROR;\n\nnot strictly necessary but break here please, it's safer if we later\ndo something in default:\n\n\n> +\t\tdefault:\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querycap\";\n> +\n> +\tmemcpy(arg, &capabilities_, sizeof(*arg));\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt\";\n> +\n> +\tif (!validateStreamType(arg->type) ||\n> +\t arg->index > streamConfig_.formats().pixelformats().size())\n> +\t\treturn -EINVAL;\n> +\n> +\t/* \\todo Add map from format to description. */\n> +\tmemcpy(arg->description, \"Video Format Description\", 5);\n> +\targ->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_fmt\";\n> +\n> +\tif (!validateStreamType(arg->type))\n> +\t\treturn -EINVAL;\n> +\n> +\tmemset(&arg->fmt, 0, sizeof(arg->fmt));\n> +\targ->fmt.pix = curV4L2Format_.fmt.pix;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_s_fmt\";\n> +\n> +\tint ret = vidioc_try_fmt(arg);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> +\t\t\t &ret, &streamConfig_, size,\n> +\t\t\t v4l2ToDrm(arg->fmt.pix.pixelformat), bufferCount_);\n> +\tif (ret < 0)\n> +\t\treturn -EINVAL;\n\nShould you set errno like you do in other functions ?\n\n> +\n> +\tunsigned int sizeimage = calculateSizeImage(streamConfig_);\n> +\tif (sizeimage == 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tsizeimage_ = sizeimage;\n> +\n> +\tsetFmtFromConfig(streamConfig_);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_try_fmt(struct v4l2_format *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_try_fmt\";\n> +\tif (!validateStreamType(arg->type))\n> +\t\treturn -EINVAL;\n> +\n> +\tunsigned int format = arg->fmt.pix.pixelformat;\n> +\tconst std::vector &formats =\n> +\t\tstreamConfig_.formats().pixelformats();\n> +\tif (std::find(formats.begin(), formats.end(), format) == formats.end())\n> +\t\tformat = streamConfig_.formats().pixelformats()[0];\n> +\n> +\tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> +\tconst std::vector &sizes = streamConfig_.formats().sizes(format);\n> +\tif (std::find(sizes.begin(), sizes.end(), size) == sizes.end())\n> +\t\tsize = streamConfig_.formats().sizes(format)[0];\n> +\n> +\targ->fmt.pix.width = size.width;\n> +\targ->fmt.pix.height = size.height;\n> +\targ->fmt.pix.pixelformat = format;\n> +\targ->fmt.pix.field = V4L2_FIELD_NONE;\n> +\targ->fmt.pix.bytesperline = bplMultiplier(drmToV4L2(format)) *\n> +\t\t\t\t arg->fmt.pix.width;\n> +\targ->fmt.pix.sizeimage = imageSize(drmToV4L2(format),\n> +\t\t\t\t\t arg->fmt.pix.width,\n> +\t\t\t\t\t arg->fmt.pix.height);\n> +\targ->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::freeBuffers()\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> +\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::streamOff,\n> +\t\t\t ConnectionTypeBlocking, &ret);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2Compat, Error) << \"Failed to stop stream\";\n> +\t\treturn ret;\n> +\t}\n> +\tvcam_->invokeMethod(&V4L2Camera::freeBuffers,\n> +\t\t\t ConnectionTypeBlocking);\n> +\tbufferCount_ = 0;\n\nEmpty line before return\n\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n> +{\n> +\tint ret;\n> +\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_reqbufs\";\n> +\tif (!validateStreamType(arg->type) ||\n> +\t !validateMemoryType(arg->memory))\n> +\t\treturn -EINVAL;\n> +\n> +\tLOG(V4L2Compat, Debug) << arg->count << \" bufs requested \";\n> +\n> +\targ->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;\n> +\n> +\tif (arg->count == 0)\n> +\t\treturn freeBuffers();\n> +\n> +\tSize size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);\n> +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> +\t\t\t &ret, &streamConfig_, size,\n> +\t\t\t v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),\n> +\t\t\t arg->count);\n> +\tif (ret < 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tsizeimage_ = calculateSizeImage(streamConfig_);\n> +\tif (sizeimage_ == 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tsetFmtFromConfig(streamConfig_);\n> +\n> +\targ->count = streamConfig_.bufferCount;\n> +\tbufferCount_ = arg->count;\n> +\n> +\tvcam_->invokeMethod(&V4L2Camera::allocBuffers,\n> +\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> +\tif (ret < 0) {\n> +\t\targ->count = 0;\n> +\t\treturn ret == -EACCES ? -EBUSY : ret;\n\nThis is rightfully handled by the V4L2Camera already.\n\n> +\t}\n> +\n> +\tfor (unsigned int i = 0; i < arg->count; i++) {\n> +\t\tstruct v4l2_buffer buf;\n> +\t\tbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +\t\tbuf.length = curV4L2Format_.fmt.pix.sizeimage;\n> +\t\tbuf.memory = V4L2_MEMORY_MMAP;\n> +\t\tbuf.m.offset = i * curV4L2Format_.fmt.pix.sizeimage;\n> +\n> +\t\tif (buffers_.size() <= i)\n> +\t\t\tbuffers_.resize(i + 1);\n> +\t\tbuffers_[i] = buf;\n> +\t}\n> +\n> +\tLOG(V4L2Compat, Debug) << \"Allocated \" << arg->count << \" buffers\";\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querybuf\";\n> +\tStream *stream = streamConfig_.stream();\n> +\n> +\tif (!validateStreamType(arg->type) ||\n> +\t arg->index >= stream->buffers().size())\n> +\t\treturn -EINVAL;\n> +\n> +\tupdateBuffers();\n\nupdateBuffers() retreives information on completed buffers, while\nquerybuf can be called anytime after buffers have been requested.\n\nFrom V4L2 specs:\nThis ioctl is part of the streaming I/O method. It can be used to\nquery the status of a buffer at any time after buffers have been\nallocated with the ioctl VIDIOC_REQBUFS ioctl.\n\nIt should thus depend on static information of allocated buffers not\njust for completed ones...\n\nI suspect I am missing something, as this updateBuffers() thing\nlooks weird to me both here and in dqbuf.\n\n> +\n> +\tmemcpy(arg, &buffers_[arg->index], sizeof(*arg));\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_qbuf, index = \"\n> +\t\t\t << arg->index;\n> +\n> +\tStream *stream = streamConfig_.stream();\n> +\n> +\tif (!validateStreamType(arg->type) ||\n> +\t !validateMemoryType(arg->memory) ||\n> +\t arg->index >= stream->buffers().size())\n> +\t\treturn -EINVAL;\n> +\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,\n> +\t\t\t &ret, arg->index);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\targ->flags |= V4L2_BUF_FLAG_QUEUED;\n> +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> +\n> +\treturn ret;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_dqbuf\";\n> +\n> +\tif (!validateStreamType(arg->type) ||\n> +\t !validateMemoryType(arg->memory))\n> +\t\treturn -EINVAL;\n> +\n> +\tif (nonBlocking_ && !vcam_->bufferSema_.tryAcquire())\n> +\t\treturn -EAGAIN;\n> +\telse\n> +\t\tvcam_->bufferSema_.acquire();\n> +\n> +\tupdateBuffers();\n> +\n> +\tstruct v4l2_buffer &buf = buffers_[arg->index];\n\nI'm not sure this is right, index should be set by us, not received\nfrom the application, there is no guarantee it is valid at all, am I wrong ?\n\nFrom V4L2 specs:\nApplications call the VIDIOC_DQBUF ioctl to dequeue a filled\n(capturing) or displayed (output) buffer from the driver’s outgoing\nqueue. They just set the type, memory and reserved fields of a struct\nv4l2_buffer as above, when VIDIOC_DQBUF is called with a pointer to\nthis structure the driver fills the remaining fields or returns an\nerror code.\n\nI'm not a huge fan of this whole updateBuffers() mechanism, I agree\nthe V4L2Camera should keep a list of completed buffers, but why\nwould we want to copy here information for all of them when we dequeue\none only ?\n\nI would simply keep a list of completed buffers in V4L2Camera, and get\nthe older on when dequeuing a buffer here.\n\n> +\n> +\tbuf.flags &= ~V4L2_BUF_FLAG_QUEUED;\n> +\tbuf.length = sizeimage_;\n> +\tmemcpy(arg, &buf, sizeof(*arg));\n> +\n> +\targ->index = currentBuf_;\n> +\tcurrentBuf_ = (currentBuf_ + 1) % bufferCount_;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_streamon(int *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamon\";\n> +\n> +\tif (!validateStreamType(*arg))\n> +\t\treturn -EINVAL;\n> +\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::streamOn,\n> +\t\t\t ConnectionTypeBlocking, &ret);\n\nAlso here, empty line before return like you do in most functions.\n\nThanks\n j\n\n> +\treturn ret;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_streamoff(int *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamoff\";\n> +\n> +\tif (!validateStreamType(*arg))\n> +\t\treturn -EINVAL;\n> +\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::streamOff,\n> +\t\t\t ConnectionTypeBlocking, &ret);\n> +\n> +\tfor (struct v4l2_buffer &buf : buffers_)\n> +\t\tbuf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);\n> +\n> +\treturn ret;\n> +}\n> +\n> +int V4L2CameraProxy::ioctl(unsigned long request, void *arg)\n> +{\n> +\tint ret;\n> +\tswitch (request) {\n> +\tcase VIDIOC_QUERYCAP:\n> +\t\tret = vidioc_querycap(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_ENUM_FMT:\n> +\t\tret = vidioc_enum_fmt(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_G_FMT:\n> +\t\tret = vidioc_g_fmt(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_S_FMT:\n> +\t\tret = vidioc_s_fmt(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_TRY_FMT:\n> +\t\tret = vidioc_try_fmt(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_REQBUFS:\n> +\t\tret = vidioc_reqbufs(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_QUERYBUF:\n> +\t\tret = vidioc_querybuf(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_QBUF:\n> +\t\tret = vidioc_qbuf(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_DQBUF:\n> +\t\tret = vidioc_dqbuf(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_STREAMON:\n> +\t\tret = vidioc_streamon(static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_STREAMOFF:\n> +\t\tret = vidioc_streamoff(static_cast(arg));\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tret = -ENOTTY;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (ret < 0) {\n> +\t\terrno = -ret;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +/* \\todo make libcamera export these */\n> +int V4L2CameraProxy::bplMultiplier(unsigned int format)\n> +{\n> +\tswitch (format) {\n> +\tcase V4L2_PIX_FMT_NV12:\n> +\tcase V4L2_PIX_FMT_NV21:\n> +\tcase V4L2_PIX_FMT_NV16:\n> +\tcase V4L2_PIX_FMT_NV61:\n> +\tcase V4L2_PIX_FMT_NV24:\n> +\tcase V4L2_PIX_FMT_NV42:\n> +\t\treturn 1;\n> +\tcase V4L2_PIX_FMT_BGR24:\n> +\tcase V4L2_PIX_FMT_RGB24:\n> +\t\treturn 3;\n> +\tcase V4L2_PIX_FMT_ARGB32:\n> +\t\treturn 4;\n> +\tcase V4L2_PIX_FMT_VYUY:\n> +\tcase V4L2_PIX_FMT_YVYU:\n> +\tcase V4L2_PIX_FMT_UYVY:\n> +\tcase V4L2_PIX_FMT_YUYV:\n> +\t\treturn 2;\n> +\tdefault:\n> +\t\treturn 0;\n> +\t};\n> +}\n> +\n> +int V4L2CameraProxy::imageSize(unsigned int format,\n> +\t\t\t unsigned int width, unsigned int height)\n> +{\n> +\tswitch (format) {\n> +\tcase V4L2_PIX_FMT_NV12:\n> +\tcase V4L2_PIX_FMT_NV21:\n> +\t\treturn width * height + width * height / 2;\n> +\tcase V4L2_PIX_FMT_NV16:\n> +\tcase V4L2_PIX_FMT_NV61:\n> +\t\treturn width * height * 2;\n> +\tcase V4L2_PIX_FMT_NV24:\n> +\tcase V4L2_PIX_FMT_NV42:\n> +\t\treturn width * height * 3;\n> +\tcase V4L2_PIX_FMT_BGR24:\n> +\tcase V4L2_PIX_FMT_RGB24:\n> +\t\treturn width * height * 3;\n> +\tcase V4L2_PIX_FMT_ARGB32:\n> +\t\treturn width * height * 4;\n> +\tcase V4L2_PIX_FMT_VYUY:\n> +\tcase V4L2_PIX_FMT_YVYU:\n> +\tcase V4L2_PIX_FMT_UYVY:\n> +\tcase V4L2_PIX_FMT_YUYV:\n> +\t\treturn width * height * 2;\n> +\tdefault:\n> +\t\treturn 0;\n> +\t};\n> +}\n> +\n> +unsigned int V4L2CameraProxy::v4l2ToDrm(unsigned int pixelformat)\n> +{\n> +\tswitch (pixelformat) {\n> +\t/* RGB formats. */\n> +\tcase V4L2_PIX_FMT_RGB24:\n> +\t\treturn DRM_FORMAT_BGR888;\n> +\tcase V4L2_PIX_FMT_BGR24:\n> +\t\treturn DRM_FORMAT_RGB888;\n> +\tcase V4L2_PIX_FMT_ARGB32:\n> +\t\treturn DRM_FORMAT_BGRA8888;\n> +\n> +\t/* YUV packed formats. */\n> +\tcase V4L2_PIX_FMT_YUYV:\n> +\t\treturn DRM_FORMAT_YUYV;\n> +\tcase V4L2_PIX_FMT_YVYU:\n> +\t\treturn DRM_FORMAT_YVYU;\n> +\tcase V4L2_PIX_FMT_UYVY:\n> +\t\treturn DRM_FORMAT_UYVY;\n> +\tcase V4L2_PIX_FMT_VYUY:\n> +\t\treturn DRM_FORMAT_VYUY;\n> +\n> +\t/* YUY planar formats. */\n> +\tcase V4L2_PIX_FMT_NV16:\n> +\t\treturn DRM_FORMAT_NV16;\n> +\tcase V4L2_PIX_FMT_NV61:\n> +\t\treturn DRM_FORMAT_NV61;\n> +\tcase V4L2_PIX_FMT_NV12:\n> +\t\treturn DRM_FORMAT_NV12;\n> +\tcase V4L2_PIX_FMT_NV21:\n> +\t\treturn DRM_FORMAT_NV21;\n> +\tcase V4L2_PIX_FMT_NV24:\n> +\t\treturn DRM_FORMAT_NV24;\n> +\tcase V4L2_PIX_FMT_NV42:\n> +\t\treturn DRM_FORMAT_NV42;\n> +\tdefault:\n> +\t\treturn pixelformat;\n> +\t};\n> +}\n> +\n> +unsigned int V4L2CameraProxy::drmToV4L2(unsigned int pixelformat)\n> +{\n> +\tswitch (pixelformat) {\n> +\t/* RGB formats. */\n> +\tcase DRM_FORMAT_BGR888:\n> +\t\treturn V4L2_PIX_FMT_RGB24;\n> +\tcase DRM_FORMAT_RGB888:\n> +\t\treturn V4L2_PIX_FMT_BGR24;\n> +\tcase DRM_FORMAT_BGRA8888:\n> +\t\treturn V4L2_PIX_FMT_ARGB32;\n> +\n> +\t/* YUV packed formats. */\n> +\tcase DRM_FORMAT_YUYV:\n> +\t\treturn V4L2_PIX_FMT_YUYV;\n> +\tcase DRM_FORMAT_YVYU:\n> +\t\treturn V4L2_PIX_FMT_YVYU;\n> +\tcase DRM_FORMAT_UYVY:\n> +\t\treturn V4L2_PIX_FMT_UYVY;\n> +\tcase DRM_FORMAT_VYUY:\n> +\t\treturn V4L2_PIX_FMT_VYUY;\n> +\n> +\t/* YUY planar formats. */\n> +\tcase DRM_FORMAT_NV16:\n> +\t\treturn V4L2_PIX_FMT_NV16;\n> +\tcase DRM_FORMAT_NV61:\n> +\t\treturn V4L2_PIX_FMT_NV61;\n> +\tcase DRM_FORMAT_NV12:\n> +\t\treturn V4L2_PIX_FMT_NV12;\n> +\tcase DRM_FORMAT_NV21:\n> +\t\treturn V4L2_PIX_FMT_NV21;\n> +\tcase DRM_FORMAT_NV24:\n> +\t\treturn V4L2_PIX_FMT_NV24;\n> +\tcase DRM_FORMAT_NV42:\n> +\t\treturn V4L2_PIX_FMT_NV42;\n> +\tdefault:\n> +\t\treturn pixelformat;\n> +\t}\n> +}\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> new file mode 100644\n> index 00000000..b167c3f4\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -0,0 +1,82 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_camera_proxy.h - Proxy to V4L2 compatibility camera\n> + */\n> +\n> +#ifndef __V4L2_CAMERA_PROXY_H__\n> +#define __V4L2_CAMERA_PROXY_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"v4l2_camera.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class V4L2CameraProxy\n> +{\n> +public:\n> +\tV4L2CameraProxy(unsigned int index, std::shared_ptr camera);\n> +\n> +\tint open(bool nonBlocking);\n> +\tvoid dup();\n> +\tvoid close();\n> +\tvoid *mmap(void *addr, size_t length, int prot, int flags,\n> +\t\t off_t offset);\n> +\tint munmap(void *addr, size_t length);\n> +\n> +\tint ioctl(unsigned long request, void *arg);\n> +\n> +\tstatic int bplMultiplier(unsigned int format);\n> +\tstatic int imageSize(unsigned int format, unsigned int width,\n> +\t\t\t unsigned int height);\n> +\n> +\tstatic unsigned int v4l2ToDrm(unsigned int pixelformat);\n> +\tstatic unsigned int drmToV4L2(unsigned int pixelformat);\n> +\n> +private:\n> +\tbool validateStreamType(uint32_t type);\n> +\tbool validateMemoryType(uint32_t memory);\n> +\tvoid setFmtFromConfig(StreamConfiguration &streamConfig);\n> +\tunsigned int calculateSizeImage(StreamConfiguration &streamConfig);\n> +\tvoid querycap(std::shared_ptr camera);\n> +\tvoid updateBuffers();\n> +\tint freeBuffers();\n> +\n> +\tint vidioc_querycap(struct v4l2_capability *arg);\n> +\tint vidioc_enum_fmt(struct v4l2_fmtdesc *arg);\n> +\tint vidioc_g_fmt(struct v4l2_format *arg);\n> +\tint vidioc_s_fmt(struct v4l2_format *arg);\n> +\tint vidioc_try_fmt(struct v4l2_format *arg);\n> +\tint vidioc_reqbufs(struct v4l2_requestbuffers *arg);\n> +\tint vidioc_querybuf(struct v4l2_buffer *arg);\n> +\tint vidioc_qbuf(struct v4l2_buffer *arg);\n> +\tint vidioc_dqbuf(struct v4l2_buffer *arg);\n> +\tint vidioc_streamon(int *arg);\n> +\tint vidioc_streamoff(int *arg);\n> +\n> +\tunsigned int refcount_;\n> +\tunsigned int index_;\n> +\tbool nonBlocking_;\n> +\n> +\tstruct v4l2_format curV4L2Format_;\n> +\tStreamConfiguration streamConfig_;\n> +\tstruct v4l2_capability capabilities_;\n> +\tunsigned int bufferCount_;\n> +\tunsigned int currentBuf_;\n> +\tunsigned int sizeimage_;\n> +\n> +\tstd::vector buffers_;\n> +\tstd::map mmaps_;\n> +\n> +\tstd::unique_ptr vcam_;\n> +};\n> +\n> +#endif /* __V4L2_CAMERA_PROXY_H__ */\n> diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp\n> new file mode 100644\n> index 00000000..7a184488\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_compat.cpp\n> @@ -0,0 +1,80 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_compat.cpp - V4L2 compatibility layer\n> + */\n> +\n> +#include \"v4l2_compat_manager.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#define LIBCAMERA_PUBLIC __attribute__((visibility(\"default\")))\n> +\n> +using namespace libcamera;\n> +\n> +#define extract_va_arg(type, arg, last)\t\\\n> +{\t\t\t\t\t\\\n> +\tva_list ap;\t\t\t\\\n> +\tva_start(ap, last);\t\t\\\n> +\targ = va_arg(ap, type);\t\t\\\n> +\tva_end(ap);\t\t\t\\\n> +}\n> +\n> +extern \"C\" {\n> +\n> +LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> +{\n> +\tmode_t mode = 0;\n> +\tif (oflag & O_CREAT || oflag & O_TMPFILE)\n> +\t\textract_va_arg(mode_t, mode, oflag);\n> +\n> +\treturn V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> +\t\t\t\t\t\t oflag, mode);\n> +}\n> +\n> +LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> +{\n> +\tmode_t mode = 0;\n> +\tif (oflag & O_CREAT || oflag & O_TMPFILE)\n> +\t\textract_va_arg(mode_t, mode, oflag);\n> +\n> +\treturn V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);\n> +}\n> +\n> +LIBCAMERA_PUBLIC int dup(int oldfd)\n> +{\n> +\treturn V4L2CompatManager::instance()->dup(oldfd);\n> +}\n> +\n> +LIBCAMERA_PUBLIC int close(int fd)\n> +{\n> +\treturn V4L2CompatManager::instance()->close(fd);\n> +}\n> +\n> +LIBCAMERA_PUBLIC void *mmap(void *addr, size_t length, int prot, int flags,\n> +\t\t\t int fd, off_t offset)\n> +{\n> +\treturn V4L2CompatManager::instance()->mmap(addr, length, prot, flags,\n> +\t\t\t\t\t\t fd, offset);\n> +}\n> +\n> +LIBCAMERA_PUBLIC int munmap(void *addr, size_t length)\n> +{\n> +\treturn V4L2CompatManager::instance()->munmap(addr, length);\n> +}\n> +\n> +LIBCAMERA_PUBLIC int ioctl(int fd, unsigned long request, ...)\n> +{\n> +\tvoid *arg;\n> +\textract_va_arg(void *, arg, request);\n> +\n> +\treturn V4L2CompatManager::instance()->ioctl(fd, request, arg);\n> +}\n> +\n> +}\n> diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp\n> new file mode 100644\n> index 00000000..cb224ba6\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_compat_manager.cpp\n> @@ -0,0 +1,255 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_compat_manager.cpp - V4L2 compatibility manager\n> + */\n> +\n> +#include \"v4l2_compat_manager.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(V4L2Compat)\n> +\n> +V4L2CompatManager::V4L2CompatManager()\n> +\t: cm_(nullptr), initialized_(false)\n> +{\n> +\topenat_func_ = (openat_func_t)dlsym(RTLD_NEXT, \"openat\");\n> +\tdup_func_ = (dup_func_t )dlsym(RTLD_NEXT, \"dup\");\n> +\tclose_func_ = (close_func_t )dlsym(RTLD_NEXT, \"close\");\n> +\tioctl_func_ = (ioctl_func_t )dlsym(RTLD_NEXT, \"ioctl\");\n> +\tmmap_func_ = (mmap_func_t )dlsym(RTLD_NEXT, \"mmap\");\n> +\tmunmap_func_ = (munmap_func_t)dlsym(RTLD_NEXT, \"munmap\");\n> +}\n> +\n> +V4L2CompatManager::~V4L2CompatManager()\n> +{\n> +\tdevices_.clear();\n> +\tmmaps_.clear();\n> +\n> +\tif (isRunning()) {\n> +\t\texit(0);\n> +\t\t/* \\todo Wait with a timeout, just in case. */\n> +\t\twait();\n> +\t}\n> +}\n> +\n> +int V4L2CompatManager::init()\n> +{\n> +\tstart();\n> +\n> +\tMutexLocker locker(mutex_);\n> +\tcv_.wait(locker, [this] { return initialized_; });\n> +\n> +\treturn 0;\n> +}\n> +\n> +void V4L2CompatManager::run()\n> +{\n> +\tcm_ = new CameraManager();\n> +\n> +\tint ret = cm_->start();\n> +\tif (ret) {\n> +\t\tLOG(V4L2Compat, Error) << \"Failed to start camera manager: \"\n> +\t\t\t\t << strerror(-ret);\n> +\t\treturn;\n> +\t}\n> +\n> +\tLOG(V4L2Compat, Debug) << \"Started camera manager\";\n> +\n> +\t/*\n> +\t * For each Camera registered in the system, a V4L2CameraProxy gets\n> +\t * created here to wrap a camera device.\n> +\t */\n> +\tunsigned int index = 0;\n> +\tfor (auto &camera : cm_->cameras()) {\n> +\t\tV4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera);\n> +\t\tproxies_.emplace_back(proxy);\n> +\t\t++index;\n> +\t}\n> +\n> +\t/*\n> +\t * libcamera has been initialized. Unlock the init() caller as we're\n> +\t * now ready to handle calls from the framework.\n> +\t */\n> +\tmutex_.lock();\n> +\tinitialized_ = true;\n> +\tmutex_.unlock();\n> +\tcv_.notify_one();\n> +\n> +\t/* Now start processing events and messages. */\n> +\texec();\n> +\n> +\tproxies_.clear();\n> +\tcm_->stop();\n> +\tdelete cm_;\n> +\tcm_ = nullptr;\n> +}\n> +\n> +V4L2CompatManager *V4L2CompatManager::instance()\n> +{\n> +\tstatic V4L2CompatManager instance;\n> +\treturn &instance;\n> +}\n> +\n> +V4L2CameraProxy *V4L2CompatManager::getProxy(int fd)\n> +{\n> +\tauto device = devices_.find(fd);\n> +\tif (device == devices_.end())\n> +\t\treturn nullptr;\n> +\n> +\treturn device->second;\n> +}\n> +\n> +int V4L2CompatManager::getCameraIndex(int fd)\n> +{\n> +\tstruct stat statbuf;\n> +\tint ret = fstat(fd, &statbuf);\n> +\tif (ret < 0)\n> +\t\treturn -1;\n> +\n> +\tunsigned int devMajor = major(statbuf.st_rdev);\n> +\tunsigned int devMinor = minor(statbuf.st_rdev);\n> +\n> +\tdev_t devnum = makedev(devMajor, devMinor);\n> +\tstd::shared_ptr target = cm_->get(devnum);\n> +\tif (!target)\n> +\t\treturn -1;\n> +\n> +\tunsigned int index = 0;\n> +\tfor (auto &camera : cm_->cameras()) {\n> +\t\tif (camera == target)\n> +\t\t\tbreak;\n> +\t\t++index;\n> +\t}\n> +\n> +\tif (index >= cm_->cameras().size())\n> +\t\treturn -1;\n> +\n> +\treturn index;\n> +}\n> +\n> +int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mode)\n> +{\n> +\tint fd = openat_func_(dirfd, path, oflag, mode);\n> +\tif (fd < 0)\n> +\t\treturn fd;\n> +\n> +\tstruct stat statbuf;\n> +\tint ret = fstat(fd, &statbuf);\n> +\tif (ret < 0 || (statbuf.st_mode & S_IFMT) != S_IFCHR ||\n> +\t major(statbuf.st_rdev) != 81)\n> +\t\treturn fd;\n> +\n> +\tif (!isRunning())\n> +\t\tinit();\n> +\n> +\tret = getCameraIndex(fd);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2Compat, Info) << \"No camera found for \" << path;\n> +\t\treturn fd;\n> +\t}\n> +\n> +\tclose_func_(fd);\n> +\n> +\tunsigned int camera_index = static_cast(ret);\n> +\n> +\tV4L2CameraProxy *proxy = proxies_[camera_index].get();\n> +\tret = proxy->open(oflag & O_NONBLOCK);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tint efd = eventfd(0, oflag & (O_CLOEXEC | O_NONBLOCK));\n> +\tif (efd < 0)\n> +\t\treturn efd;\n> +\n> +\tdevices_.emplace(efd, proxy);\n> +\n> +\treturn efd;\n> +}\n> +\n> +int V4L2CompatManager::dup(int oldfd)\n> +{\n> +\tint newfd = dup_func_(oldfd);\n> +\n> +\tauto device = devices_.find(oldfd);\n> +\tif (device != devices_.end()) {\n> +\t\tV4L2CameraProxy *proxy = device->second;\n> +\t\tdevices_[newfd] = proxy;\n> +\t\tproxy->dup();\n> +\t}\n> +\n> +\treturn newfd;\n> +}\n> +\n> +int V4L2CompatManager::close(int fd)\n> +{\n> +\tV4L2CameraProxy *proxy = getProxy(fd);\n> +\tif (proxy) {\n> +\t\tproxy->close();\n> +\t\tdevices_.erase(fd);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn close_func_(fd);\n> +}\n> +\n> +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,\n> +\t\t\t int fd, off_t offset)\n> +{\n> +\tV4L2CameraProxy *proxy = getProxy(fd);\n> +\tif (!proxy)\n> +\t\treturn mmap_func_(addr, length, prot, flags, fd, offset);\n> +\n> +\tvoid *map = proxy->mmap(addr, length, prot, flags, offset);\n> +\tif (map == MAP_FAILED)\n> +\t\treturn map;\n> +\n> +\tmmaps_[map] = proxy;\n> +\treturn map;\n> +}\n> +\n> +int V4L2CompatManager::munmap(void *addr, size_t length)\n> +{\n> +\tauto device = mmaps_.find(addr);\n> +\tif (device == mmaps_.end())\n> +\t\treturn munmap_func_(addr, length);\n> +\n> +\tV4L2CameraProxy *proxy = device->second;\n> +\n> +\tint ret = proxy->munmap(addr, length);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tmmaps_.erase(device);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)\n> +{\n> +\tV4L2CameraProxy *proxy = getProxy(fd);\n> +\tif (!proxy)\n> +\t\treturn ioctl_func_(fd, request, arg);\n> +\n> +\treturn proxy->ioctl(request, arg);\n> +}\n> diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h\n> new file mode 100644\n> index 00000000..13a240c4\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_compat_manager.h\n> @@ -0,0 +1,77 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_compat_manager.h - V4L2 compatibility manager\n> + */\n> +\n> +#ifndef __V4L2_COMPAT_MANAGER_H__\n> +#define __V4L2_COMPAT_MANAGER_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"thread.h\"\n> +#include \"v4l2_camera_proxy.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class V4L2CompatManager : public Thread\n> +{\n> +public:\n> +\tstatic V4L2CompatManager *instance();\n> +\n> +\tint init();\n> +\n> +\tV4L2CameraProxy *getProxy(int fd);\n> +\tV4L2CameraProxy *getCamera(void *addr);\n> +\n> +\tint openat(int dirfd, const char *path, int oflag, mode_t mode);\n> +\n> +\tint dup(int oldfd);\n> +\tint close(int fd);\n> +\tvoid *mmap(void *addr, size_t length, int prot, int flags,\n> +\t\t int fd, off_t offset);\n> +\tint munmap(void *addr, size_t length);\n> +\tint ioctl(int fd, unsigned long request, void *arg);\n> +\n> +private:\n> +\tV4L2CompatManager();\n> +\t~V4L2CompatManager();\n> +\n> +\tvoid run() override;\n> +\tint getCameraIndex(int fd);\n> +\n> +\ttypedef int (*openat_func_t)(int dirfd, const char *path, int oflag, ...);\n> +\ttypedef int (*dup_func_t)(int oldfd);\n> +\ttypedef int (*close_func_t)(int fd);\n> +\ttypedef int (*ioctl_func_t)(int fd, unsigned long request, ...);\n> +\ttypedef void *(*mmap_func_t)(void *addr, size_t length, int prot,\n> +\t\t\t\t int flags, int fd, off_t offset);\n> +\ttypedef int (*munmap_func_t)(void *addr, size_t length);\n> +\n> +\topenat_func_t openat_func_;\n> +\tdup_func_t dup_func_;\n> +\tclose_func_t close_func_;\n> +\tioctl_func_t ioctl_func_;\n> +\tmmap_func_t mmap_func_;\n> +\tmunmap_func_t munmap_func_;\n> +\n> +\tCameraManager *cm_;\n> +\n> +\tstd::mutex mutex_;\n> +\tstd::condition_variable cv_;\n> +\tbool initialized_;\n> +\n> +\tstd::vector> proxies_;\n> +\tstd::map devices_;\n> +\tstd::map mmaps_;\n> +};\n> +\n> +#endif /* __V4L2_COMPAT_MANAGER_H__ */\n> --\n> 2.23.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1E2060467\n\tfor ;\n\tTue, 31 Dec 2019 11:19:54 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 5A17A1C0005;\n\tTue, 31 Dec 2019 10:19:54 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 31 Dec 2019 11:22:09 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191231102209.jaqlwpjtcf6zs6uk@uno.localdomain>","References":"<20191231053314.20063-1-paul.elder@ideasonboard.com>\n\t<20191231053314.20063-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"hwzq4rsylyk25p3f\"","Content-Disposition":"inline","In-Reply-To":"<20191231053314.20063-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","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","X-List-Received-Date":"Tue, 31 Dec 2019 10:19:55 -0000"}},{"id":3294,"web_url":"https://patchwork.libcamera.org/comment/3294/","msgid":"<20200102030214.GD15498@pendragon.ideasonboard.com>","date":"2020-01-02T03:02:14","subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch. Nearly there, there remaining comments are\nsmall.\n\nOn Tue, Dec 31, 2019 at 11:22:09AM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 30, 2019 at 11:33:14PM -0600, Paul Elder wrote:\n> > Add libcamera V4L2 compatibility layer.\n> >\n> > This initial implementation supports the minimal set of V4L2 operations,\n> > which allows getting, setting, and enumerating formats, and streaming\n> > frames from a video device. Some data about the wrapped V4L2 video\n> > device are hardcoded.\n> >\n> > Add a build option named 'v4l2' and adjust the build system to\n> > selectively compile the V4L2 compatibility layer.\n> >\n> > For now we match the V4L2 device node to a libcamera camera based on a\n> > devnum that a pipeline handler may optionally map to a libcamera\n> > camera.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Major changes in v4:\n> > - squash buffer state tracking in V4L2CameraProxy (from 6/6 in v3)\n> > - separated out V4L2CameraProxy::freeBuffers() (since it was cluttering\n> > up V4L2CameraProxy::vidioc_reqbufs)\n> >\n> > Less major changes in v4:\n> > - check that addresses passed to V4L2CameraProxy::munmap() have been\n> > mmaped\n> > - other cosmetic changes\n> >\n> > Major changes in v3:\n> > - V4L2CompatManager::openat() verify video node and match to camera via\n> > devnum instead of path\n> > - add FrameMetadata class to encapsulate portions of the libcamera\n> > Buffer that needs to be extracted to v4l2_buffer, and to prepare for\n> > the Buffer rework\n> > - V4L2CameraProxy refcount for tracking dups and closes (and open)\n> > - add V4L2CompatManager::initialized_ to deal with race of waiting on\n> > init cv after the cv is notified\n> >\n> > Less major changes in v3:\n> > - change the list of pending Reqeusts (v4l2 buffers queued before\n> > streamon) to unique pointers\n> > - V4L2Camera::updateSizeImage() -> V4L2CameraProxy::calculateSizeImage()\n> > - change V4L2CompatManager list of V4L2CameraProxy to unique_ptr, with\n> > maps of fd/mmap addr to V4L2CameraProxy *\n> > - removed cross-thread validation methods from V4L2CameraProxy\n> > (validate[Stream|Memory]Type)\n> > - removed hasPixelFormat() and hasSize() from V4L2CameraProxy\n> > - moved mmap logic out of V4L2Camera and into V4L2CameraProxy\n> > - moved nonblock logic out of V4L2Camera and into V4L2CameraProxy\n> > - add todos\n> > - oter cosmetic changes\n> >\n> > Changes in v2:\n> > - move all errno acrobatics to V4L2CameraProxy\n> > - remove all mentions of dmabuf\n> > - make V4L2CompatManager::getCamera return pointer rather than\n> > shared_ptr\n> > - match V4L2 device nodes to libcamera cameras using Camera::name()\n> > compared to /sys/dev/char/maj:min/name (only works for UVC cameras)\n> > - in V4L2CompatManager::getCameraIndex()\n> > - add -fvisibility=hidden to v4l2 compat\n> > - cache the results of V4L2CompatManager::imageSize() within V4L2Camera\n> > (where V4L2Camera is interested)\n> > - remove V4L2CompatManager::valid[fd|mmap], and where those methods were\n> > used, check V4L2CompatManager::getCamera() != nullptr instead\n> > - fixed V4L2CompatManager::drmToV4L2() mappings for DRM_FORMAT_BGR888\n> > and DRM_FORMAT_RGB888\n> > - other cosmetic changes\n> > ---\n> > meson_options.txt | 5 +\n> > src/meson.build | 4 +\n> > src/v4l2/meson.build | 31 ++\n> > src/v4l2/v4l2_camera.cpp | 233 +++++++++++\n> > src/v4l2/v4l2_camera.h | 83 ++++\n> > src/v4l2/v4l2_camera_proxy.cpp | 649 +++++++++++++++++++++++++++++++\n> > src/v4l2/v4l2_camera_proxy.h | 82 ++++\n> > src/v4l2/v4l2_compat.cpp | 80 ++++\n> > src/v4l2/v4l2_compat_manager.cpp | 255 ++++++++++++\n> > src/v4l2/v4l2_compat_manager.h | 77 ++++\n> > 10 files changed, 1499 insertions(+)\n> > create mode 100644 src/v4l2/meson.build\n> > create mode 100644 src/v4l2/v4l2_camera.cpp\n> > create mode 100644 src/v4l2/v4l2_camera.h\n> > create mode 100644 src/v4l2/v4l2_camera_proxy.cpp\n> > create mode 100644 src/v4l2/v4l2_camera_proxy.h\n> > create mode 100644 src/v4l2/v4l2_compat.cpp\n> > create mode 100644 src/v4l2/v4l2_compat_manager.cpp\n> > create mode 100644 src/v4l2/v4l2_compat_manager.h\n> >\n> > diff --git a/meson_options.txt b/meson_options.txt\n> > index 1a328045..79ee4de6 100644\n> > --- a/meson_options.txt\n> > +++ b/meson_options.txt\n> > @@ -10,3 +10,8 @@ option('documentation',\n> > option('test',\n> > type : 'boolean',\n> > description: 'Compile and include the tests')\n> > +\n> > +option('v4l2',\n> > + type : 'boolean',\n> > + value : false,\n> > + description : 'Compile the V4L2 compatibility layer')\n> > diff --git a/src/meson.build b/src/meson.build\n> > index 67ad20aa..5adcd61f 100644\n> > --- a/src/meson.build\n> > +++ b/src/meson.build\n> > @@ -6,3 +6,7 @@ subdir('libcamera')\n> > subdir('ipa')\n> > subdir('cam')\n> > subdir('qcam')\n> > +\n> > +if get_option('v4l2')\n> > + subdir('v4l2')\n> > +endif\n> > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build\n> > new file mode 100644\n> > index 00000000..14ee3594\n> > --- /dev/null\n> > +++ b/src/v4l2/meson.build\n> > @@ -0,0 +1,31 @@\n> > +v4l2_compat_sources = files([\n> > + 'v4l2_camera.cpp',\n> > + 'v4l2_camera_proxy.cpp',\n> > + 'v4l2_compat.cpp',\n> > + 'v4l2_compat_manager.cpp',\n> > +])\n> > +\n> > +v4l2_compat_includes = [\n> > + libcamera_includes,\n> > + libcamera_internal_includes,\n> > +]\n> > +\n> > +v4l2_compat_cpp_args = [\n> > + # Meson enables large file support unconditionally, which redirect file\n> > + # operations to 64-bit versions. This results in some symbols being\n> > + # renamed, for instance open() being renamed to open64(). As the V4L2\n> > + # adaptation wrapper needs to provide both 32-bit and 64-bit versions of\n> > + # file operations, disable transparent large file support.\n> > + '-U_FILE_OFFSET_BITS',\n> > + '-D_FILE_OFFSET_BITS=32',\n> > + '-fvisibility=hidden',\n> > +]\n> > +\n> > +v4l2_compat = shared_library('v4l2-compat',\n> > + v4l2_compat_sources,\n> > + name_prefix : '',\n> > + install : true,\n> > + link_with : libcamera,\n> > + include_directories : v4l2_compat_includes,\n> > + dependencies : libcamera_deps,\n> > + cpp_args : v4l2_compat_cpp_args)\n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > new file mode 100644\n> > index 00000000..82ada8d4\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -0,0 +1,233 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_camera.cpp - V4L2 compatibility camera\n> > + */\n> > +\n> > +#include \"v4l2_camera.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"utils.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(V4L2Compat);\n> > +\n> > +FrameMetadata::FrameMetadata(Buffer *buffer)\n> > +\t: index_(buffer->index()), bytesused_(buffer->bytesused()),\n> > +\t timestamp_(buffer->timestamp()), sequence_(buffer->sequence()),\n> > +\t status_(buffer->status())\n> > +{\n> > +}\n> > +\n> > +V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> > +\t: camera_(camera), isRunning_(false)\n> > +{\n> > +\tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> > +}\n> > +\n> > +V4L2Camera::~V4L2Camera()\n> > +{\n> > +\tcamera_->release();\n> > +}\n> > +\n> > +void V4L2Camera::open(int *ret)\n> > +{\n> > +\tif (camera_->acquire() < 0) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Failed to acquire camera\";\n> > +\t\t*ret = -EINVAL;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tconfig_ = camera_->generateConfiguration({ StreamRole::Viewfinder });\n> > +\tif (!config_) {\n> > +\t\tcamera_->release();\n> > +\t\t*ret = -EINVAL;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t*ret = 0;\n> > +}\n> > +\n> > +void V4L2Camera::close()\n> > +{\n> > +\tcamera_->release();\n> > +}\n> > +\n> > +void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)\n> > +{\n> > +\t*streamConfig = config_->at(0);\n> > +}\n> > +\n> > +std::vector V4L2Camera::completedBuffers()\n> > +{\n> > +\tstd::vector v;\n> > +\n> > +\tbufferLock_.lock();\n> > +\tfor (std::unique_ptr &metadata : completedBuffers_)\n> > +\t\tv.push_back(*metadata.get());\n> > +\tcompletedBuffers_.clear();\n> > +\tbufferLock_.unlock();\n> > +\n> > +\treturn v;\n> > +}\n> > +\n> > +void V4L2Camera::requestComplete(Request *request)\n> > +{\n> > +\tif (request->status() == Request::RequestCancelled)\n> > +\t\treturn;\n> > +\n> > +\t/* We only have one stream at the moment. */\n> > +\tbufferLock_.lock();\n> > +\tBuffer *buffer = request->buffers().begin()->second;\n> > +\tstd::unique_ptr metadata =\n> > +\t\tutils::make_unique(buffer);\n> > +\tcompletedBuffers_.push_back(std::move(metadata));\n> > +\tbufferLock_.unlock();\n> > +\n> > +\tbufferSema_.release();\n> \n> The usage of this semaphore from two different classes looks weird :/\n\nWe could encapsulate it, but given that V4L2Camera and V4L2CameraProxy\nare internal to the V4L2 compatibility layer, I think it's overkill.\n\n> > +}\n> > +\n> > +void V4L2Camera::configure(int *ret, StreamConfiguration *streamConfigOut,\n> > +\t\t\t Size size, PixelFormat pixelformat,\n\nNo need to copy the size, you can make it a const Size &.\n\n> > +\t\t\t unsigned int bufferCount)\n> > +{\n> > +\tStreamConfiguration &streamConfig = config_->at(0);\n> > +\tstreamConfig.size.width = size.width;\n> > +\tstreamConfig.size.height = size.height;\n> > +\tstreamConfig.pixelFormat = pixelformat;\n> > +\tstreamConfig.bufferCount = bufferCount;\n> > +\t/* \\todo memoryType (interval vs external) */\n> > +\n> > +\tCameraConfiguration::Status validation = config_->validate();\n> > +\tif (validation == CameraConfiguration::Invalid) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Configuration invalid\";\n> > +\t\t*ret = -EINVAL;\n> > +\t\treturn;\n> > +\t}\n> > +\tif (validation == CameraConfiguration::Adjusted)\n> > +\t\tLOG(V4L2Compat, Info) << \"Configuration adjusted\";\n> > +\n> > +\tLOG(V4L2Compat, Info) << \"Validated configuration is: \"\n> > +\t\t\t << streamConfig.toString();\n> > +\n> > +\t*ret = camera_->configure(config_.get());\n> > +\tif (*ret < 0)\n> > +\t\treturn;\n> > +\n> > +\t*streamConfigOut = config_->at(0);\n> > +}\n> > +\n> > +void V4L2Camera::mmap(void **ret, unsigned int index)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing MMAP\";\n> \n> You have removed most of these, but some are still here, is this\n> intentional ?\n> \n> > +\n> > +\tStream *stream = *camera_->streams().begin();\n> > +\t*ret = stream->buffers()[index].planes()[0].mem();\n> > +}\n> > +\n> > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Allocating libcamera bufs\";\n> > +\n> > +\t*ret = camera_->allocateBuffers();\n> > +\tif (*ret == -EACCES)\n> > +\t\t*ret = -EBUSY;\n> > +}\n> > +\n> > +void V4L2Camera::freeBuffers()\n> > +{\n> > +\tcamera_->freeBuffers();\n> > +}\n> > +\n> > +void V4L2Camera::streamOn(int *ret)\n> > +{\n> > +\t*ret = 0;\n> \n> Do you need this ?\n\nWithout this the next return will result in ret being uninitialized.\n\nI'll try to implement native support for return values in\ninvokeMethod().\n\n> > +\n> > +\tif (isRunning_)\n> > +\t\treturn;\n> > +\n> > +\t*ret = camera_->start();\n> > +\tif (*ret < 0) {\n> > +\t\tif (*ret == -EACCES)\n> > +\t\t\t*ret = -EBUSY;\n> > +\t\treturn;\n> > +\t}\n> > +\tisRunning_ = true;\n> > +\n> > +\tfor (std::unique_ptr &req : pendingRequests_) {\n> > +\t\t/* \\todo What should we do if this returns -EINVAL? */\n> > +\t\t*ret = camera_->queueRequest(req.release());\n> > +\t\tif (*ret < 0) {\n> > +\t\t\t*ret = (*ret == -EACCES ? -EAGAIN : *ret);\n> \n> A few lines below you have the same construct in the form of\n> \n> \tif (*ret < 0) {\n> \t\tif (*ret == -EACCES)\n> \t\t\t*ret = -EBUSY;\n> \n> please unify on one of the two\n> \n> > +\t\t\treturn;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tpendingRequests_.clear();\n> > +}\n> > +\n> > +void V4L2Camera::streamOff(int *ret)\n> > +{\n> > +\t*ret = 0;\n> \n> do you need this ?\n> \n> > +\n> > +\t/* \\todo restore buffers to reqbufs state? */\n> > +\tif (!isRunning_)\n> > +\t\treturn;\n> > +\n> > +\t*ret = camera_->stop();\n> > +\tif (*ret < 0) {\n> > +\t\tif (*ret == -EACCES)\n> > +\t\t\t*ret = -EBUSY;\n> > +\t\treturn;\n> > +\t}\n> > +\tisRunning_ = false;\n> > +}\n> > +\n> > +void V4L2Camera::qbuf(int *ret, unsigned int index)\n> > +{\n> > +\tStream *stream = config_->at(0).stream();\n> > +\tstd::unique_ptr buffer = stream->createBuffer(index);\n> > +\tif (!buffer) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Can't create buffer\";\n> > +\t\t*ret = -ENOMEM;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tstd::unique_ptr request =\n> > +\t\tstd::unique_ptr(camera_->createRequest());\n> > +\tif (!request) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Can't create request\";\n> > +\t\t*ret = -ENOMEM;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t*ret = request->addBuffer(std::move(buffer));\n> > +\tif (*ret < 0) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Can't set buffer for request\";\n> > +\t\t*ret = -ENOMEM;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (!isRunning_) {\n> > +\t\tpendingRequests_.push_back(std::move(request));\n> \n> You can return here and remove the else branch below\n> ret should be set to 0 already by addBuffer()\n> \n> > +\t} else {\n> > +\t\t*ret = camera_->queueRequest(request.release());\n> > +\t\tif (*ret < 0) {\n> > +\t\t\tLOG(V4L2Compat, Error) << \"Can't queue request\";\n> > +\t\t\tif (*ret == -EACCES)\n> > +\t\t\t\t*ret = -EBUSY;\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t*ret = 0;\n> > +}\n> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> > new file mode 100644\n> > index 00000000..a48a2d59\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera.h\n> > @@ -0,0 +1,83 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_camera.h - V4L2 compatibility camera\n> > + */\n> > +\n> > +#ifndef __V4L2_CAMERA_H__\n> > +#define __V4L2_CAMERA_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> \n> Do we separate publinc and internal inclusion directives with an empty line ?\n> \n> > +#include \"semaphore.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +class FrameMetadata\n> > +{\n> > +public:\n> > +\tFrameMetadata(Buffer *buffer);\n> > +\n> > +\tint index() const { return index_; }\n> > +\n> > +\tunsigned int bytesused() const { return bytesused_; }\n> > +\tuint64_t timestamp() const { return timestamp_; }\n> > +\tunsigned int sequence() const { return sequence_; }\n> > +\n> > +\tBuffer::Status status() const { return status_; }\n> > +\n> > +private:\n> > +\tint index_;\n> > +\n> > +\tunsigned int bytesused_;\n> > +\tuint64_t timestamp_;\n> > +\tunsigned int sequence_;\n> > +\n> > +\tBuffer::Status status_;\n> > +};\n> > +\n> > +class V4L2Camera : public Object\n> > +{\n> > +public:\n> > +\tV4L2Camera(std::shared_ptr camera);\n> > +\t~V4L2Camera();\n> > +\n> > +\tvoid open(int *ret);\n> > +\tvoid close();\n> > +\tvoid getStreamConfig(StreamConfiguration *streamConfig);\n> > +\tstd::vector completedBuffers();\n> > +\n> > +\tvoid mmap(void **ret, unsigned int index);\n> > +\n> > +\tvoid configure(int *ret, StreamConfiguration *streamConfigOut,\n> > +\t\t Size size, PixelFormat pixelformat,\n> > +\t\t unsigned int bufferCount);\n> > +\n> > +\tvoid allocBuffers(int *ret, unsigned int count);\n> > +\tvoid freeBuffers();\n> > +\tvoid streamOn(int *ret);\n> > +\tvoid streamOff(int *ret);\n> > +\n> > +\tvoid qbuf(int *ret, unsigned int index);\n> > +\n> > +\tSemaphore bufferSema_;\n\nYou need an empty line here.\n\n> > +private:\n> > +\tvoid requestComplete(Request *request);\n> > +\n> > +\tstd::shared_ptr camera_;\n> > +\tstd::unique_ptr config_;\n> > +\n> > +\tbool isRunning_;\n> > +\n> > +\tstd::mutex bufferLock_;\n> > +\n> > +\tstd::deque> pendingRequests_;\n> > +\tstd::deque> completedBuffers_;\n> > +};\n> > +\n> > +#endif /* __V4L2_CAMERA_H__ */\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > new file mode 100644\n> > index 00000000..ae27ea1b\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -0,0 +1,649 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_camera_proxy.cpp - Proxy to V4L2 compatibility camera\n> > + */\n> > +\n> > +#include \"v4l2_camera_proxy.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"utils.h\"\n> > +#include \"v4l2_camera.h\"\n> > +#include \"v4l2_compat_manager.h\"\n> > +\n> > +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(V4L2Compat);\n> > +\n> > +V4L2CameraProxy::V4L2CameraProxy(unsigned int index,\n> > +\t\t\t\t std::shared_ptr camera)\n> > +\t: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),\n> > +\t vcam_(utils::make_unique(camera))\n> > +{\n> > +\tquerycap(camera);\n> > +}\n> > +\n> > +int V4L2CameraProxy::open(bool nonBlocking)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing open\";\n> > +\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,\n> > +\t\t\t &ret);\n> > +\tif (ret < 0) {\n> > +\t\terrno = -ret;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\tnonBlocking_ = nonBlocking;\n> > +\n> > +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> > +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> > +\tsetFmtFromConfig(streamConfig_);\n> > +\tsizeimage_ = calculateSizeImage(streamConfig_);\n> > +\n> > +\trefcount_++;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void V4L2CameraProxy::dup()\n> > +{\n> > +\trefcount_++;\n> > +}\n> > +\n> > +void V4L2CameraProxy::close()\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing close\";\n> > +\n> > +\tif (--refcount_ > 0)\n> > +\t\treturn;\n> > +\n> > +\tvcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking);\n> > +\n> > +\treturn;\n\nThis return isn't needed.\n\n> > +}\n> > +\n> > +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,\n> > +\t\t\t off_t offset)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing mmap\";\n> \n> This seems to be duplicated in V4L2Camera\n> \n> > +\n> > +\tif (prot != (PROT_READ | PROT_WRITE)) {\n> > +\t\terrno = ENOTSUP;\n> > +\t\treturn MAP_FAILED;\n> > +\t}\n> > +\n> > +\tunsigned int index = offset / sizeimage_;\n> > +\tif (index * sizeimage_ != offset || length != sizeimage_) {\n> > +\t\terrno = EINVAL;\n> > +\t\treturn MAP_FAILED;\n> > +\t}\n> > +\n> > +\tvoid *val;\n> > +\tvcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,\n> > +\t\t\t &val, index);\n> > +\n> > +\tbuffers_[index].flags |= V4L2_BUF_FLAG_MAPPED;\n> > +\tmmaps_[val] = index;\n> > +\n> > +\treturn val;\n> > +}\n> > +\n> > +int V4L2CameraProxy::munmap(void *addr, size_t length)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing munmap\";\n> > +\n> > +\tauto iter = mmaps_.find(addr);\n> > +\tif (iter == mmaps_.end() || length != sizeimage_) {\n> > +\t\terrno = EAGAIN;\n\nShouldn't this be EINVAL ?\n\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\tbuffers_[iter->second].flags &= ~V4L2_BUF_FLAG_MAPPED;\n> > +\tmmaps_.erase(iter);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +bool V4L2CameraProxy::validateStreamType(uint32_t type)\n> > +{\n> > +\treturn type == V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> > +}\n> > +\n> > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)\n> > +{\n> > +\treturn memory == V4L2_MEMORY_MMAP;\n> > +}\n> > +\n> > +void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig)\n> > +{\n> > +\tcurV4L2Format_.fmt.pix.width = streamConfig.size.width;\n> > +\tcurV4L2Format_.fmt.pix.height = streamConfig.size.height;\n> > +\tcurV4L2Format_.fmt.pix.pixelformat = drmToV4L2(streamConfig.pixelFormat);\n> > +\tcurV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;\n> > +\tcurV4L2Format_.fmt.pix.bytesperline =\n> > +\t\tbplMultiplier(curV4L2Format_.fmt.pix.pixelformat) *\n> > +\t\tcurV4L2Format_.fmt.pix.width;\n> > +\tcurV4L2Format_.fmt.pix.sizeimage =\n> > +\t\timageSize(curV4L2Format_.fmt.pix.pixelformat,\n> > +\t\t\t curV4L2Format_.fmt.pix.width,\n> > +\t\t\t curV4L2Format_.fmt.pix.height);\n> > +\tcurV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> > +}\n> > +\n> > +unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig)\n> > +{\n> > +\treturn imageSize(drmToV4L2(streamConfig.pixelFormat),\n> > +\t\t\t streamConfig.size.width,\n> > +\t\t\t streamConfig.size.height);\n> > +}\n> > +\n> > +void V4L2CameraProxy::querycap(std::shared_ptr camera)\n> > +{\n> > +\tstd::string driver = \"libcamera\";\n> > +\tstd::string bus_info = driver + \":\" + std::to_string(index_);\n> > +\n> > +\tmemcpy(capabilities_.driver, driver.c_str(),\n> > +\t sizeof(capabilities_.driver));\n> > +\tmemcpy(capabilities_.card, camera->name().c_str(),\n> > +\t sizeof(capabilities_.card));\n> > +\tmemcpy(capabilities_.bus_info, bus_info.c_str(),\n> > +\t sizeof(capabilities_.bus_info));\n> > +\t/* \\todo Put this in a header/config somewhere. */\n> > +\tcapabilities_.version = KERNEL_VERSION(5, 2, 0);\n> > +\tcapabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;\n> > +\tcapabilities_.capabilities = capabilities_.device_caps\n> > +\t\t\t\t | V4L2_CAP_DEVICE_CAPS;\n> > +\tmemset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));\n> > +}\n> > +\n> > +void V4L2CameraProxy::updateBuffers()\n> > +{\n> > +\tstd::vector completedBuffers = vcam_->completedBuffers();\n> \n> This should be a reference otherwise you copy the whole vector\n> everytime you dequeue or query a buffer\n\ncompletedBuffers() returns a value, not a reference, so this is fine.\n\n> > +\tfor (FrameMetadata &fmd : completedBuffers) {\n> > +\t\t/* \\todo is this index valid if the buffer status != success? */\n> > +\t\tstruct v4l2_buffer &buf = buffers_[fmd.index()];\n> > +\n> > +\t\tswitch (fmd.status()) {\n> > +\t\tcase Buffer::Status::BufferSuccess:\n> > +\t\t\tbuf.index = fmd.index();\n> > +\t\t\tbuf.bytesused = fmd.bytesused();\n> > +\t\t\tbuf.field = V4L2_FIELD_NONE;\n> > +\t\t\tbuf.timestamp.tv_sec = fmd.timestamp() / 1000000000;\n> > +\t\t\tbuf.timestamp.tv_usec = fmd.timestamp() % 1000000;\n> > +\t\t\tbuf.sequence = fmd.sequence();\n> > +\n> > +\t\t\tbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> > +\t\t\tbuf.length = curV4L2Format_.fmt.pix.sizeimage;\n> > +\t\t\tbuf.memory = V4L2_MEMORY_MMAP;\n> > +\t\t\tbuf.m.offset = buf.index * curV4L2Format_.fmt.pix.sizeimage;\n> > +\n> > +\t\t\tbuf.flags |= V4L2_BUF_FLAG_DONE;\n> > +\t\t\tbreak;\n> > +\t\tcase Buffer::Status::BufferError:\n> > +\t\t\tbuf.flags |= V4L2_BUF_FLAG_ERROR;\n> \n> not strictly necessary but break here please, it's safer if we later\n> do something in default:\n\nAgreed. Furthermore, with a compiler that enables the implicit\nfall-through warning it would result in a warning.\n\n> > +\t\tdefault:\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querycap\";\n> > +\n> > +\tmemcpy(arg, &capabilities_, sizeof(*arg));\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt\";\n> > +\n> > +\tif (!validateStreamType(arg->type) ||\n> > +\t arg->index > streamConfig_.formats().pixelformats().size())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\t/* \\todo Add map from format to description. */\n> > +\tmemcpy(arg->description, \"Video Format Description\", 5);\n\n5 bytes may be too little :-)\n\n\tstrlcpy(arg->description, \"Video Format Description\",\n\t\tsizeof(arg->description));\n\n> > +\targ->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_fmt\";\n> > +\n> > +\tif (!validateStreamType(arg->type))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tmemset(&arg->fmt, 0, sizeof(arg->fmt));\n> > +\targ->fmt.pix = curV4L2Format_.fmt.pix;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_s_fmt\";\n> > +\n> > +\tint ret = vidioc_try_fmt(arg);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> > +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> > +\t\t\t &ret, &streamConfig_, size,\n> > +\t\t\t v4l2ToDrm(arg->fmt.pix.pixelformat), bufferCount_);\n> > +\tif (ret < 0)\n> > +\t\treturn -EINVAL;\n> \n> Should you set errno like you do in other functions ?\n\nFor ioctls errno is set by V4L2CameraProxy::ioctl().\n\n> > +\n> > +\tunsigned int sizeimage = calculateSizeImage(streamConfig_);\n> > +\tif (sizeimage == 0)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tsizeimage_ = sizeimage;\n> > +\n> > +\tsetFmtFromConfig(streamConfig_);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_try_fmt(struct v4l2_format *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_try_fmt\";\n> > +\tif (!validateStreamType(arg->type))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tunsigned int format = arg->fmt.pix.pixelformat;\n> > +\tconst std::vector &formats =\n> > +\t\tstreamConfig_.formats().pixelformats();\n> > +\tif (std::find(formats.begin(), formats.end(), format) == formats.end())\n> > +\t\tformat = streamConfig_.formats().pixelformats()[0];\n> > +\n> > +\tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> > +\tconst std::vector &sizes = streamConfig_.formats().sizes(format);\n> > +\tif (std::find(sizes.begin(), sizes.end(), size) == sizes.end())\n> > +\t\tsize = streamConfig_.formats().sizes(format)[0];\n> > +\n> > +\targ->fmt.pix.width = size.width;\n> > +\targ->fmt.pix.height = size.height;\n> > +\targ->fmt.pix.pixelformat = format;\n> > +\targ->fmt.pix.field = V4L2_FIELD_NONE;\n> > +\targ->fmt.pix.bytesperline = bplMultiplier(drmToV4L2(format)) *\n> > +\t\t\t\t arg->fmt.pix.width;\n> > +\targ->fmt.pix.sizeimage = imageSize(drmToV4L2(format),\n> > +\t\t\t\t\t arg->fmt.pix.width,\n> > +\t\t\t\t\t arg->fmt.pix.height);\n> > +\targ->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::freeBuffers()\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> > +\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::streamOff,\n> > +\t\t\t ConnectionTypeBlocking, &ret);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Failed to stop stream\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\tvcam_->invokeMethod(&V4L2Camera::freeBuffers,\n> > +\t\t\t ConnectionTypeBlocking);\n> > +\tbufferCount_ = 0;\n> \n> Empty line before return\n> \n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n> > +{\n> > +\tint ret;\n> > +\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_reqbufs\";\n> > +\tif (!validateStreamType(arg->type) ||\n> > +\t !validateMemoryType(arg->memory))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tLOG(V4L2Compat, Debug) << arg->count << \" bufs requested \";\n> > +\n> > +\targ->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;\n> > +\n> > +\tif (arg->count == 0)\n> > +\t\treturn freeBuffers();\n> > +\n> > +\tSize size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);\n> > +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> > +\t\t\t &ret, &streamConfig_, size,\n> > +\t\t\t v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),\n> > +\t\t\t arg->count);\n> > +\tif (ret < 0)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tsizeimage_ = calculateSizeImage(streamConfig_);\n> > +\tif (sizeimage_ == 0)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tsetFmtFromConfig(streamConfig_);\n> > +\n> > +\targ->count = streamConfig_.bufferCount;\n> > +\tbufferCount_ = arg->count;\n> > +\n> > +\tvcam_->invokeMethod(&V4L2Camera::allocBuffers,\n> > +\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> > +\tif (ret < 0) {\n> > +\t\targ->count = 0;\n> > +\t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> This is rightfully handled by the V4L2Camera already.\n> \n> > +\t}\n> > +\n> > +\tfor (unsigned int i = 0; i < arg->count; i++) {\n> > +\t\tstruct v4l2_buffer buf;\n\n\t= { };\n\nin order to clear all fields that you don't set explicitly.\n\n> > +\t\tbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> > +\t\tbuf.length = curV4L2Format_.fmt.pix.sizeimage;\n> > +\t\tbuf.memory = V4L2_MEMORY_MMAP;\n> > +\t\tbuf.m.offset = i * curV4L2Format_.fmt.pix.sizeimage;\n\nYou should set buf.index = i too.\n\n> > +\n> > +\t\tif (buffers_.size() <= i)\n> > +\t\t\tbuffers_.resize(i + 1);\n\nYou can move this before the loop.\n\n\tbuffers_.resize(arg->count);\n\n> > +\t\tbuffers_[i] = buf;\n> > +\t}\n> > +\n> > +\tLOG(V4L2Compat, Debug) << \"Allocated \" << arg->count << \" buffers\";\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querybuf\";\n> > +\tStream *stream = streamConfig_.stream();\n> > +\n> > +\tif (!validateStreamType(arg->type) ||\n> > +\t arg->index >= stream->buffers().size())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tupdateBuffers();\n> \n> updateBuffers() retreives information on completed buffers, while\n> querybuf can be called anytime after buffers have been requested.\n> \n> From V4L2 specs:\n> This ioctl is part of the streaming I/O method. It can be used to\n> query the status of a buffer at any time after buffers have been\n> allocated with the ioctl VIDIOC_REQBUFS ioctl.\n> \n> It should thus depend on static information of allocated buffers not\n> just for completed ones...\n> \n> I suspect I am missing something, as this updateBuffers() thing\n> looks weird to me both here and in dqbuf.\n\nThe idea is that the buffer status is kept in V4L2CameraProxy, and\nupdated with updateBuffers(). The method is called both at querybuf and\ndqbuf time to ensure we have the latest status.\n\n> > +\n> > +\tmemcpy(arg, &buffers_[arg->index], sizeof(*arg));\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_qbuf, index = \"\n> > +\t\t\t << arg->index;\n> > +\n> > +\tStream *stream = streamConfig_.stream();\n> > +\n> > +\tif (!validateStreamType(arg->type) ||\n> > +\t !validateMemoryType(arg->memory) ||\n> > +\t arg->index >= stream->buffers().size())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,\n> > +\t\t\t &ret, arg->index);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\targ->flags |= V4L2_BUF_FLAG_QUEUED;\n> > +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_dqbuf\";\n> > +\n> > +\tif (!validateStreamType(arg->type) ||\n> > +\t !validateMemoryType(arg->memory))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tif (nonBlocking_ && !vcam_->bufferSema_.tryAcquire())\n> > +\t\treturn -EAGAIN;\n> > +\telse\n> > +\t\tvcam_->bufferSema_.acquire();\n> > +\n> > +\tupdateBuffers();\n> > +\n> > +\tstruct v4l2_buffer &buf = buffers_[arg->index];\n> \n> I'm not sure this is right, index should be set by us, not received\n> from the application, there is no guarantee it is valid at all, am I wrong ?\n\nThat's correct. This should be\n\n\tstruct v4l2_buffer &buf = buffers_[currentBuf_];\n\n> From V4L2 specs:\n> Applications call the VIDIOC_DQBUF ioctl to dequeue a filled\n> (capturing) or displayed (output) buffer from the driver’s outgoing\n> queue. They just set the type, memory and reserved fields of a struct\n> v4l2_buffer as above, when VIDIOC_DQBUF is called with a pointer to\n> this structure the driver fills the remaining fields or returns an\n> error code.\n> \n> I'm not a huge fan of this whole updateBuffers() mechanism, I agree\n> the V4L2Camera should keep a list of completed buffers, but why\n> would we want to copy here information for all of them when we dequeue\n> one only ?\n\nFor performance reasons, and because we need to do so for querybuf too\nanyway.\n\n> I would simply keep a list of completed buffers in V4L2Camera, and get\n> the older on when dequeuing a buffer here.\n> \n> > +\n> > +\tbuf.flags &= ~V4L2_BUF_FLAG_QUEUED;\n> > +\tbuf.length = sizeimage_;\n> > +\tmemcpy(arg, &buf, sizeof(*arg));\n> > +\n> > +\targ->index = currentBuf_;\n\nThis isn't needed if you initialize the buffers_ entries with an index\nin reqbufs.\n\n> > +\tcurrentBuf_ = (currentBuf_ + 1) % bufferCount_;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_streamon(int *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamon\";\n> > +\n> > +\tif (!validateStreamType(*arg))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::streamOn,\n> > +\t\t\t ConnectionTypeBlocking, &ret);\n> \n> Also here, empty line before return like you do in most functions.\n> \n> > +\treturn ret;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_streamoff(int *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamoff\";\n> > +\n> > +\tif (!validateStreamType(*arg))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::streamOff,\n> > +\t\t\t ConnectionTypeBlocking, &ret);\n> > +\n> > +\tfor (struct v4l2_buffer &buf : buffers_)\n> > +\t\tbuf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +int V4L2CameraProxy::ioctl(unsigned long request, void *arg)\n> > +{\n> > +\tint ret;\n> > +\tswitch (request) {\n> > +\tcase VIDIOC_QUERYCAP:\n> > +\t\tret = vidioc_querycap(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_ENUM_FMT:\n> > +\t\tret = vidioc_enum_fmt(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_G_FMT:\n> > +\t\tret = vidioc_g_fmt(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_S_FMT:\n> > +\t\tret = vidioc_s_fmt(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_TRY_FMT:\n> > +\t\tret = vidioc_try_fmt(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_REQBUFS:\n> > +\t\tret = vidioc_reqbufs(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_QUERYBUF:\n> > +\t\tret = vidioc_querybuf(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_QBUF:\n> > +\t\tret = vidioc_qbuf(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_DQBUF:\n> > +\t\tret = vidioc_dqbuf(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_STREAMON:\n> > +\t\tret = vidioc_streamon(static_cast(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_STREAMOFF:\n> > +\t\tret = vidioc_streamoff(static_cast(arg));\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tret = -ENOTTY;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tif (ret < 0) {\n> > +\t\terrno = -ret;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +/* \\todo make libcamera export these */\n> > +int V4L2CameraProxy::bplMultiplier(unsigned int format)\n> > +{\n> > +\tswitch (format) {\n> > +\tcase V4L2_PIX_FMT_NV12:\n> > +\tcase V4L2_PIX_FMT_NV21:\n> > +\tcase V4L2_PIX_FMT_NV16:\n> > +\tcase V4L2_PIX_FMT_NV61:\n> > +\tcase V4L2_PIX_FMT_NV24:\n> > +\tcase V4L2_PIX_FMT_NV42:\n> > +\t\treturn 1;\n> > +\tcase V4L2_PIX_FMT_BGR24:\n> > +\tcase V4L2_PIX_FMT_RGB24:\n> > +\t\treturn 3;\n> > +\tcase V4L2_PIX_FMT_ARGB32:\n> > +\t\treturn 4;\n> > +\tcase V4L2_PIX_FMT_VYUY:\n> > +\tcase V4L2_PIX_FMT_YVYU:\n> > +\tcase V4L2_PIX_FMT_UYVY:\n> > +\tcase V4L2_PIX_FMT_YUYV:\n> > +\t\treturn 2;\n> > +\tdefault:\n> > +\t\treturn 0;\n> > +\t};\n> > +}\n> > +\n> > +int V4L2CameraProxy::imageSize(unsigned int format,\n> > +\t\t\t unsigned int width, unsigned int height)\n> > +{\n> > +\tswitch (format) {\n> > +\tcase V4L2_PIX_FMT_NV12:\n> > +\tcase V4L2_PIX_FMT_NV21:\n> > +\t\treturn width * height + width * height / 2;\n> > +\tcase V4L2_PIX_FMT_NV16:\n> > +\tcase V4L2_PIX_FMT_NV61:\n> > +\t\treturn width * height * 2;\n> > +\tcase V4L2_PIX_FMT_NV24:\n> > +\tcase V4L2_PIX_FMT_NV42:\n> > +\t\treturn width * height * 3;\n> > +\tcase V4L2_PIX_FMT_BGR24:\n> > +\tcase V4L2_PIX_FMT_RGB24:\n> > +\t\treturn width * height * 3;\n> > +\tcase V4L2_PIX_FMT_ARGB32:\n> > +\t\treturn width * height * 4;\n> > +\tcase V4L2_PIX_FMT_VYUY:\n> > +\tcase V4L2_PIX_FMT_YVYU:\n> > +\tcase V4L2_PIX_FMT_UYVY:\n> > +\tcase V4L2_PIX_FMT_YUYV:\n> > +\t\treturn width * height * 2;\n> > +\tdefault:\n> > +\t\treturn 0;\n> > +\t};\n> > +}\n> > +\n> > +unsigned int V4L2CameraProxy::v4l2ToDrm(unsigned int pixelformat)\n> > +{\n> > +\tswitch (pixelformat) {\n> > +\t/* RGB formats. */\n> > +\tcase V4L2_PIX_FMT_RGB24:\n> > +\t\treturn DRM_FORMAT_BGR888;\n> > +\tcase V4L2_PIX_FMT_BGR24:\n> > +\t\treturn DRM_FORMAT_RGB888;\n> > +\tcase V4L2_PIX_FMT_ARGB32:\n> > +\t\treturn DRM_FORMAT_BGRA8888;\n> > +\n> > +\t/* YUV packed formats. */\n> > +\tcase V4L2_PIX_FMT_YUYV:\n> > +\t\treturn DRM_FORMAT_YUYV;\n> > +\tcase V4L2_PIX_FMT_YVYU:\n> > +\t\treturn DRM_FORMAT_YVYU;\n> > +\tcase V4L2_PIX_FMT_UYVY:\n> > +\t\treturn DRM_FORMAT_UYVY;\n> > +\tcase V4L2_PIX_FMT_VYUY:\n> > +\t\treturn DRM_FORMAT_VYUY;\n> > +\n> > +\t/* YUY planar formats. */\n> > +\tcase V4L2_PIX_FMT_NV16:\n> > +\t\treturn DRM_FORMAT_NV16;\n> > +\tcase V4L2_PIX_FMT_NV61:\n> > +\t\treturn DRM_FORMAT_NV61;\n> > +\tcase V4L2_PIX_FMT_NV12:\n> > +\t\treturn DRM_FORMAT_NV12;\n> > +\tcase V4L2_PIX_FMT_NV21:\n> > +\t\treturn DRM_FORMAT_NV21;\n> > +\tcase V4L2_PIX_FMT_NV24:\n> > +\t\treturn DRM_FORMAT_NV24;\n> > +\tcase V4L2_PIX_FMT_NV42:\n> > +\t\treturn DRM_FORMAT_NV42;\n> > +\tdefault:\n> > +\t\treturn pixelformat;\n> > +\t};\n> > +}\n> > +\n> > +unsigned int V4L2CameraProxy::drmToV4L2(unsigned int pixelformat)\n> > +{\n> > +\tswitch (pixelformat) {\n> > +\t/* RGB formats. */\n> > +\tcase DRM_FORMAT_BGR888:\n> > +\t\treturn V4L2_PIX_FMT_RGB24;\n> > +\tcase DRM_FORMAT_RGB888:\n> > +\t\treturn V4L2_PIX_FMT_BGR24;\n> > +\tcase DRM_FORMAT_BGRA8888:\n> > +\t\treturn V4L2_PIX_FMT_ARGB32;\n> > +\n> > +\t/* YUV packed formats. */\n> > +\tcase DRM_FORMAT_YUYV:\n> > +\t\treturn V4L2_PIX_FMT_YUYV;\n> > +\tcase DRM_FORMAT_YVYU:\n> > +\t\treturn V4L2_PIX_FMT_YVYU;\n> > +\tcase DRM_FORMAT_UYVY:\n> > +\t\treturn V4L2_PIX_FMT_UYVY;\n> > +\tcase DRM_FORMAT_VYUY:\n> > +\t\treturn V4L2_PIX_FMT_VYUY;\n> > +\n> > +\t/* YUY planar formats. */\n> > +\tcase DRM_FORMAT_NV16:\n> > +\t\treturn V4L2_PIX_FMT_NV16;\n> > +\tcase DRM_FORMAT_NV61:\n> > +\t\treturn V4L2_PIX_FMT_NV61;\n> > +\tcase DRM_FORMAT_NV12:\n> > +\t\treturn V4L2_PIX_FMT_NV12;\n> > +\tcase DRM_FORMAT_NV21:\n> > +\t\treturn V4L2_PIX_FMT_NV21;\n> > +\tcase DRM_FORMAT_NV24:\n> > +\t\treturn V4L2_PIX_FMT_NV24;\n> > +\tcase DRM_FORMAT_NV42:\n> > +\t\treturn V4L2_PIX_FMT_NV42;\n> > +\tdefault:\n> > +\t\treturn pixelformat;\n> > +\t}\n> > +}\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > new file mode 100644\n> > index 00000000..b167c3f4\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -0,0 +1,82 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_camera_proxy.h - Proxy to V4L2 compatibility camera\n> > + */\n> > +\n> > +#ifndef __V4L2_CAMERA_PROXY_H__\n> > +#define __V4L2_CAMERA_PROXY_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"v4l2_camera.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +class V4L2CameraProxy\n> > +{\n> > +public:\n> > +\tV4L2CameraProxy(unsigned int index, std::shared_ptr camera);\n> > +\n> > +\tint open(bool nonBlocking);\n> > +\tvoid dup();\n> > +\tvoid close();\n> > +\tvoid *mmap(void *addr, size_t length, int prot, int flags,\n> > +\t\t off_t offset);\n> > +\tint munmap(void *addr, size_t length);\n> > +\n> > +\tint ioctl(unsigned long request, void *arg);\n> > +\n> > +\tstatic int bplMultiplier(unsigned int format);\n> > +\tstatic int imageSize(unsigned int format, unsigned int width,\n> > +\t\t\t unsigned int height);\n> > +\n> > +\tstatic unsigned int v4l2ToDrm(unsigned int pixelformat);\n> > +\tstatic unsigned int drmToV4L2(unsigned int pixelformat);\n> > +\n> > +private:\n> > +\tbool validateStreamType(uint32_t type);\n> > +\tbool validateMemoryType(uint32_t memory);\n> > +\tvoid setFmtFromConfig(StreamConfiguration &streamConfig);\n> > +\tunsigned int calculateSizeImage(StreamConfiguration &streamConfig);\n> > +\tvoid querycap(std::shared_ptr camera);\n> > +\tvoid updateBuffers();\n> > +\tint freeBuffers();\n> > +\n> > +\tint vidioc_querycap(struct v4l2_capability *arg);\n> > +\tint vidioc_enum_fmt(struct v4l2_fmtdesc *arg);\n> > +\tint vidioc_g_fmt(struct v4l2_format *arg);\n> > +\tint vidioc_s_fmt(struct v4l2_format *arg);\n> > +\tint vidioc_try_fmt(struct v4l2_format *arg);\n> > +\tint vidioc_reqbufs(struct v4l2_requestbuffers *arg);\n> > +\tint vidioc_querybuf(struct v4l2_buffer *arg);\n> > +\tint vidioc_qbuf(struct v4l2_buffer *arg);\n> > +\tint vidioc_dqbuf(struct v4l2_buffer *arg);\n> > +\tint vidioc_streamon(int *arg);\n> > +\tint vidioc_streamoff(int *arg);\n> > +\n> > +\tunsigned int refcount_;\n> > +\tunsigned int index_;\n> > +\tbool nonBlocking_;\n> > +\n> > +\tstruct v4l2_format curV4L2Format_;\n> > +\tStreamConfiguration streamConfig_;\n> > +\tstruct v4l2_capability capabilities_;\n> > +\tunsigned int bufferCount_;\n> > +\tunsigned int currentBuf_;\n> > +\tunsigned int sizeimage_;\n> > +\n> > +\tstd::vector buffers_;\n> > +\tstd::map mmaps_;\n> > +\n> > +\tstd::unique_ptr vcam_;\n> > +};\n> > +\n> > +#endif /* __V4L2_CAMERA_PROXY_H__ */\n> > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp\n> > new file mode 100644\n> > index 00000000..7a184488\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_compat.cpp\n> > @@ -0,0 +1,80 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_compat.cpp - V4L2 compatibility layer\n> > + */\n> > +\n> > +#include \"v4l2_compat_manager.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#define LIBCAMERA_PUBLIC __attribute__((visibility(\"default\")))\n> > +\n> > +using namespace libcamera;\n> > +\n> > +#define extract_va_arg(type, arg, last)\t\\\n> > +{\t\t\t\t\t\\\n> > +\tva_list ap;\t\t\t\\\n> > +\tva_start(ap, last);\t\t\\\n> > +\targ = va_arg(ap, type);\t\t\\\n> > +\tva_end(ap);\t\t\t\\\n> > +}\n> > +\n> > +extern \"C\" {\n> > +\n> > +LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> > +{\n> > +\tmode_t mode = 0;\n> > +\tif (oflag & O_CREAT || oflag & O_TMPFILE)\n> > +\t\textract_va_arg(mode_t, mode, oflag);\n> > +\n> > +\treturn V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> > +\t\t\t\t\t\t oflag, mode);\n> > +}\n> > +\n> > +LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> > +{\n> > +\tmode_t mode = 0;\n> > +\tif (oflag & O_CREAT || oflag & O_TMPFILE)\n> > +\t\textract_va_arg(mode_t, mode, oflag);\n> > +\n> > +\treturn V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);\n> > +}\n> > +\n> > +LIBCAMERA_PUBLIC int dup(int oldfd)\n> > +{\n> > +\treturn V4L2CompatManager::instance()->dup(oldfd);\n> > +}\n> > +\n> > +LIBCAMERA_PUBLIC int close(int fd)\n> > +{\n> > +\treturn V4L2CompatManager::instance()->close(fd);\n> > +}\n> > +\n> > +LIBCAMERA_PUBLIC void *mmap(void *addr, size_t length, int prot, int flags,\n> > +\t\t\t int fd, off_t offset)\n> > +{\n> > +\treturn V4L2CompatManager::instance()->mmap(addr, length, prot, flags,\n> > +\t\t\t\t\t\t fd, offset);\n> > +}\n> > +\n> > +LIBCAMERA_PUBLIC int munmap(void *addr, size_t length)\n> > +{\n> > +\treturn V4L2CompatManager::instance()->munmap(addr, length);\n> > +}\n> > +\n> > +LIBCAMERA_PUBLIC int ioctl(int fd, unsigned long request, ...)\n> > +{\n> > +\tvoid *arg;\n> > +\textract_va_arg(void *, arg, request);\n> > +\n> > +\treturn V4L2CompatManager::instance()->ioctl(fd, request, arg);\n> > +}\n> > +\n> > +}\n> > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp\n> > new file mode 100644\n> > index 00000000..cb224ba6\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_compat_manager.cpp\n> > @@ -0,0 +1,255 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_compat_manager.cpp - V4L2 compatibility manager\n> > + */\n> > +\n> > +#include \"v4l2_compat_manager.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(V4L2Compat)\n> > +\n> > +V4L2CompatManager::V4L2CompatManager()\n> > +\t: cm_(nullptr), initialized_(false)\n> > +{\n> > +\topenat_func_ = (openat_func_t)dlsym(RTLD_NEXT, \"openat\");\n> > +\tdup_func_ = (dup_func_t )dlsym(RTLD_NEXT, \"dup\");\n> > +\tclose_func_ = (close_func_t )dlsym(RTLD_NEXT, \"close\");\n> > +\tioctl_func_ = (ioctl_func_t )dlsym(RTLD_NEXT, \"ioctl\");\n> > +\tmmap_func_ = (mmap_func_t )dlsym(RTLD_NEXT, \"mmap\");\n> > +\tmunmap_func_ = (munmap_func_t)dlsym(RTLD_NEXT, \"munmap\");\n> > +}\n> > +\n> > +V4L2CompatManager::~V4L2CompatManager()\n> > +{\n> > +\tdevices_.clear();\n> > +\tmmaps_.clear();\n> > +\n> > +\tif (isRunning()) {\n> > +\t\texit(0);\n> > +\t\t/* \\todo Wait with a timeout, just in case. */\n> > +\t\twait();\n> > +\t}\n> > +}\n> > +\n> > +int V4L2CompatManager::init()\n> > +{\n> > +\tstart();\n> > +\n> > +\tMutexLocker locker(mutex_);\n> > +\tcv_.wait(locker, [this] { return initialized_; });\n\nI think the usual way to capture 'this' is\n\n\tcv_.wait(locker, [&] { return initialized_; });\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void V4L2CompatManager::run()\n> > +{\n> > +\tcm_ = new CameraManager();\n> > +\n> > +\tint ret = cm_->start();\n> > +\tif (ret) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Failed to start camera manager: \"\n> > +\t\t\t\t << strerror(-ret);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tLOG(V4L2Compat, Debug) << \"Started camera manager\";\n> > +\n> > +\t/*\n> > +\t * For each Camera registered in the system, a V4L2CameraProxy gets\n> > +\t * created here to wrap a camera device.\n> > +\t */\n> > +\tunsigned int index = 0;\n> > +\tfor (auto &camera : cm_->cameras()) {\n> > +\t\tV4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera);\n> > +\t\tproxies_.emplace_back(proxy);\n> > +\t\t++index;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * libcamera has been initialized. Unlock the init() caller as we're\n> > +\t * now ready to handle calls from the framework.\n> > +\t */\n> > +\tmutex_.lock();\n> > +\tinitialized_ = true;\n> > +\tmutex_.unlock();\n> > +\tcv_.notify_one();\n> > +\n> > +\t/* Now start processing events and messages. */\n> > +\texec();\n> > +\n> > +\tproxies_.clear();\n> > +\tcm_->stop();\n> > +\tdelete cm_;\n> > +\tcm_ = nullptr;\n> > +}\n> > +\n> > +V4L2CompatManager *V4L2CompatManager::instance()\n> > +{\n> > +\tstatic V4L2CompatManager instance;\n> > +\treturn &instance;\n> > +}\n> > +\n> > +V4L2CameraProxy *V4L2CompatManager::getProxy(int fd)\n> > +{\n> > +\tauto device = devices_.find(fd);\n> > +\tif (device == devices_.end())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn device->second;\n> > +}\n> > +\n> > +int V4L2CompatManager::getCameraIndex(int fd)\n> > +{\n> > +\tstruct stat statbuf;\n> > +\tint ret = fstat(fd, &statbuf);\n> > +\tif (ret < 0)\n> > +\t\treturn -1;\n> > +\n> > +\tunsigned int devMajor = major(statbuf.st_rdev);\n> > +\tunsigned int devMinor = minor(statbuf.st_rdev);\n> > +\n> > +\tdev_t devnum = makedev(devMajor, devMinor);\n> > +\tstd::shared_ptr target = cm_->get(devnum);\n> > +\tif (!target)\n> > +\t\treturn -1;\n> > +\n> > +\tunsigned int index = 0;\n> > +\tfor (auto &camera : cm_->cameras()) {\n> > +\t\tif (camera == target)\n> > +\t\t\tbreak;\n> > +\t\t++index;\n> > +\t}\n> > +\n> > +\tif (index >= cm_->cameras().size())\n> > +\t\treturn -1;\n> > +\n> > +\treturn index;\n> > +}\n> > +\n> > +int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mode)\n> > +{\n> > +\tint fd = openat_func_(dirfd, path, oflag, mode);\n> > +\tif (fd < 0)\n> > +\t\treturn fd;\n> > +\n> > +\tstruct stat statbuf;\n> > +\tint ret = fstat(fd, &statbuf);\n> > +\tif (ret < 0 || (statbuf.st_mode & S_IFMT) != S_IFCHR ||\n> > +\t major(statbuf.st_rdev) != 81)\n> > +\t\treturn fd;\n> > +\n> > +\tif (!isRunning())\n> > +\t\tinit();\n> > +\n> > +\tret = getCameraIndex(fd);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(V4L2Compat, Info) << \"No camera found for \" << path;\n> > +\t\treturn fd;\n> > +\t}\n> > +\n> > +\tclose_func_(fd);\n> > +\n> > +\tunsigned int camera_index = static_cast(ret);\n> > +\n> > +\tV4L2CameraProxy *proxy = proxies_[camera_index].get();\n> > +\tret = proxy->open(oflag & O_NONBLOCK);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tint efd = eventfd(0, oflag & (O_CLOEXEC | O_NONBLOCK));\n> > +\tif (efd < 0)\n> > +\t\treturn efd;\n> > +\n> > +\tdevices_.emplace(efd, proxy);\n> > +\n> > +\treturn efd;\n> > +}\n> > +\n> > +int V4L2CompatManager::dup(int oldfd)\n> > +{\n> > +\tint newfd = dup_func_(oldfd);\n\n\tif (newfd < 0)\n\t\treturn newfd;\n\n> > +\n> > +\tauto device = devices_.find(oldfd);\n> > +\tif (device != devices_.end()) {\n> > +\t\tV4L2CameraProxy *proxy = device->second;\n> > +\t\tdevices_[newfd] = proxy;\n> > +\t\tproxy->dup();\n> > +\t}\n> > +\n> > +\treturn newfd;\n> > +}\n> > +\n> > +int V4L2CompatManager::close(int fd)\n> > +{\n> > +\tV4L2CameraProxy *proxy = getProxy(fd);\n> > +\tif (proxy) {\n> > +\t\tproxy->close();\n> > +\t\tdevices_.erase(fd);\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\treturn close_func_(fd);\n> > +}\n> > +\n> > +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,\n> > +\t\t\t int fd, off_t offset)\n> > +{\n> > +\tV4L2CameraProxy *proxy = getProxy(fd);\n> > +\tif (!proxy)\n> > +\t\treturn mmap_func_(addr, length, prot, flags, fd, offset);\n> > +\n> > +\tvoid *map = proxy->mmap(addr, length, prot, flags, offset);\n> > +\tif (map == MAP_FAILED)\n> > +\t\treturn map;\n> > +\n> > +\tmmaps_[map] = proxy;\n> > +\treturn map;\n> > +}\n> > +\n> > +int V4L2CompatManager::munmap(void *addr, size_t length)\n> > +{\n> > +\tauto device = mmaps_.find(addr);\n> > +\tif (device == mmaps_.end())\n> > +\t\treturn munmap_func_(addr, length);\n> > +\n> > +\tV4L2CameraProxy *proxy = device->second;\n> > +\n> > +\tint ret = proxy->munmap(addr, length);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tmmaps_.erase(device);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)\n> > +{\n> > +\tV4L2CameraProxy *proxy = getProxy(fd);\n> > +\tif (!proxy)\n> > +\t\treturn ioctl_func_(fd, request, arg);\n> > +\n> > +\treturn proxy->ioctl(request, arg);\n> > +}\n> > diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h\n> > new file mode 100644\n> > index 00000000..13a240c4\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_compat_manager.h\n> > @@ -0,0 +1,77 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_compat_manager.h - V4L2 compatibility manager\n> > + */\n> > +\n> > +#ifndef __V4L2_COMPAT_MANAGER_H__\n> > +#define __V4L2_COMPAT_MANAGER_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"thread.h\"\n> > +#include \"v4l2_camera_proxy.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +class V4L2CompatManager : public Thread\n> > +{\n> > +public:\n> > +\tstatic V4L2CompatManager *instance();\n> > +\n> > +\tint init();\n> > +\n> > +\tV4L2CameraProxy *getProxy(int fd);\n> > +\tV4L2CameraProxy *getCamera(void *addr);\n> > +\n> > +\tint openat(int dirfd, const char *path, int oflag, mode_t mode);\n> > +\n> > +\tint dup(int oldfd);\n> > +\tint close(int fd);\n> > +\tvoid *mmap(void *addr, size_t length, int prot, int flags,\n> > +\t\t int fd, off_t offset);\n> > +\tint munmap(void *addr, size_t length);\n> > +\tint ioctl(int fd, unsigned long request, void *arg);\n> > +\n> > +private:\n> > +\tV4L2CompatManager();\n> > +\t~V4L2CompatManager();\n> > +\n> > +\tvoid run() override;\n> > +\tint getCameraIndex(int fd);\n> > +\n> > +\ttypedef int (*openat_func_t)(int dirfd, const char *path, int oflag, ...);\n> > +\ttypedef int (*dup_func_t)(int oldfd);\n> > +\ttypedef int (*close_func_t)(int fd);\n> > +\ttypedef int (*ioctl_func_t)(int fd, unsigned long request, ...);\n> > +\ttypedef void *(*mmap_func_t)(void *addr, size_t length, int prot,\n> > +\t\t\t\t int flags, int fd, off_t offset);\n> > +\ttypedef int (*munmap_func_t)(void *addr, size_t length);\n> > +\n> > +\topenat_func_t openat_func_;\n> > +\tdup_func_t dup_func_;\n> > +\tclose_func_t close_func_;\n> > +\tioctl_func_t ioctl_func_;\n> > +\tmmap_func_t mmap_func_;\n> > +\tmunmap_func_t munmap_func_;\n> > +\n> > +\tCameraManager *cm_;\n> > +\n> > +\tstd::mutex mutex_;\n> > +\tstd::condition_variable cv_;\n> > +\tbool initialized_;\n> > +\n> > +\tstd::vector> proxies_;\n> > +\tstd::map devices_;\n> > +\tstd::map mmaps_;\n> > +};\n> > +\n> > +#endif /* __V4L2_COMPAT_MANAGER_H__ */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CACD60463\n\tfor ;\n\tThu, 2 Jan 2020 04:02:25 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79F54516;\n\tThu, 2 Jan 2020 04:02:24 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1577934144;\n\tbh=pIeLzXMqltntLsAmVeaQsYmYvhCu2e4ViHzc+qRNsCw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FAMZ8+fqZeK2gM4srm269cfLR2s8sAWPCK3sVYnVhigKRO7nAWjO3fliSZfNCKBJs\n\tjjfnNRL90gP/skOZA4In9+rjMtlC0uplOXA61SVBr265y2pGOxRmKUDPUqzUFprm2Y\n\tts7uYUMgg8gaYQoDDqbtpc73x5fa8T9z1c9dMGv8=","Date":"Thu, 2 Jan 2020 05:02:14 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200102030214.GD15498@pendragon.ideasonboard.com>","References":"<20191231053314.20063-1-paul.elder@ideasonboard.com>\n\t<20191231053314.20063-5-paul.elder@ideasonboard.com>\n\t<20191231102209.jaqlwpjtcf6zs6uk@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191231102209.jaqlwpjtcf6zs6uk@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","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","X-List-Received-Date":"Thu, 02 Jan 2020 03:02:25 -0000"}},{"id":3296,"web_url":"https://patchwork.libcamera.org/comment/3296/","msgid":"<20200102082034.lnddftpkd3qxytwo@uno.localdomain>","date":"2020-01-02T08:20:34","subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Jan 02, 2020 at 05:02:14AM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch. Nearly there, there remaining comments are\n> small.\n>\n\n[snip]\n\n> > > +void V4L2Camera::requestComplete(Request *request)\n> > > +{\n> > > +\tif (request->status() == Request::RequestCancelled)\n> > > +\t\treturn;\n> > > +\n> > > +\t/* We only have one stream at the moment. */\n> > > +\tbufferLock_.lock();\n> > > +\tBuffer *buffer = request->buffers().begin()->second;\n> > > +\tstd::unique_ptr metadata =\n> > > +\t\tutils::make_unique(buffer);\n> > > +\tcompletedBuffers_.push_back(std::move(metadata));\n> > > +\tbufferLock_.unlock();\n> > > +\n> > > +\tbufferSema_.release();\n> >\n> > The usage of this semaphore from two different classes looks weird :/\n>\n> We could encapsulate it, but given that V4L2Camera and V4L2CameraProxy\n> are internal to the V4L2 compatibility layer, I think it's overkill.\n>\n\nEncapsulate would have been syntactic sugar. I would have had the\nV4L2Camera handle locking internally, but that's fine.\n\n> > > +}\n\n[snip]\n\n> > > +\n> > > +int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)\n> > > +{\n> > > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querybuf\";\n> > > +\tStream *stream = streamConfig_.stream();\n> > > +\n> > > +\tif (!validateStreamType(arg->type) ||\n> > > +\t arg->index >= stream->buffers().size())\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tupdateBuffers();\n> >\n> > updateBuffers() retreives information on completed buffers, while\n> > querybuf can be called anytime after buffers have been requested.\n\nYou below answer does not address this issue. What if querybuf is\ncalled and not buffer completed yet (because say, streaming has not\nstarted) ?\n\n> >\n> > From V4L2 specs:\n> > This ioctl is part of the streaming I/O method. It can be used to\n> > query the status of a buffer at any time after buffers have been\n> > allocated with the ioctl VIDIOC_REQBUFS ioctl.\n> >\n> > It should thus depend on static information of allocated buffers not\n> > just for completed ones...\n> >\n> > I suspect I am missing something, as this updateBuffers() thing\n> > looks weird to me both here and in dqbuf.\n>\n> The idea is that the buffer status is kept in V4L2CameraProxy, and\n> updated with updateBuffers(). The method is called both at querybuf and\n> dqbuf time to ensure we have the latest status.\n>\n> > > +\n> > > +\tmemcpy(arg, &buffers_[arg->index], sizeof(*arg));\n> > > +\n> > > +\tstruct v4l2_buffer &buf = buffers_[arg->index];\n> >\n> > I'm not sure this is right, index should be set by us, not received\n> > from the application, there is no guarantee it is valid at all, am I wrong ?\n>\n> That's correct. This should be\n>\n> \tstruct v4l2_buffer &buf = buffers_[currentBuf_];\n>\n> > From V4L2 specs:\n> > Applications call the VIDIOC_DQBUF ioctl to dequeue a filled\n> > (capturing) or displayed (output) buffer from the driver’s outgoing\n> > queue. They just set the type, memory and reserved fields of a struct\n> > v4l2_buffer as above, when VIDIOC_DQBUF is called with a pointer to\n> > this structure the driver fills the remaining fields or returns an\n> > error code.\n> >\n> > I'm not a huge fan of this whole updateBuffers() mechanism, I agree\n> > the V4L2Camera should keep a list of completed buffers, but why\n> > would we want to copy here information for all of them when we dequeue\n> > one only ?\n>\n> For performance reasons, and because we need to do so for querybuf too\n> anyway.\n>\n> > I would simply keep a list of completed buffers in V4L2Camera, and get\n> > the older on when dequeuing a buffer here.\n> >\n\nIf the two of you prefer this I'm fine. I still think it's unnecessary\nclunky and we could have lived with V4L2CameraProxy picking the latest\ncompleted buffer here at the expense of one inter-thread call per\nrequest. We should probably profile this to decide what is more\nefficient between an inter-thread call and a copy of a vector of\nbuffers.\n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2C3B60465\n\tfor ;\n\tThu, 2 Jan 2020 09:18:17 +0100 (CET)","from uno.localdomain\n\t(host109-57-dynamic.61-82-r.retail.telecomitalia.it [82.61.57.109])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 7265B20005;\n\tThu, 2 Jan 2020 08:18:16 +0000 (UTC)"],"X-Originating-IP":"82.61.57.109","Date":"Thu, 2 Jan 2020 09:20:34 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200102082034.lnddftpkd3qxytwo@uno.localdomain>","References":"<20191231053314.20063-1-paul.elder@ideasonboard.com>\n\t<20191231053314.20063-5-paul.elder@ideasonboard.com>\n\t<20191231102209.jaqlwpjtcf6zs6uk@uno.localdomain>\n\t<20200102030214.GD15498@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"3hhalbus5qmnqdy4\"","Content-Disposition":"inline","In-Reply-To":"<20200102030214.GD15498@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","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","X-List-Received-Date":"Thu, 02 Jan 2020 08:18:17 -0000"}},{"id":3297,"web_url":"https://patchwork.libcamera.org/comment/3297/","msgid":"<20200102103605.GA4843@pendragon.ideasonboard.com>","date":"2020-01-02T10:36:05","subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Jan 02, 2020 at 09:20:34AM +0100, Jacopo Mondi wrote:\n> On Thu, Jan 02, 2020 at 05:02:14AM +0200, Laurent Pinchart wrote:\n> > Hi Paul,\n> >\n> > Thank you for the patch. Nearly there, there remaining comments are\n> > small.\n> \n> [snip]\n> \n> >>> +void V4L2Camera::requestComplete(Request *request)\n> >>> +{\n> >>> +\tif (request->status() == Request::RequestCancelled)\n> >>> +\t\treturn;\n> >>> +\n> >>> +\t/* We only have one stream at the moment. */\n> >>> +\tbufferLock_.lock();\n> >>> +\tBuffer *buffer = request->buffers().begin()->second;\n> >>> +\tstd::unique_ptr metadata =\n> >>> +\t\tutils::make_unique(buffer);\n> >>> +\tcompletedBuffers_.push_back(std::move(metadata));\n> >>> +\tbufferLock_.unlock();\n> >>> +\n> >>> +\tbufferSema_.release();\n> >>\n> >> The usage of this semaphore from two different classes looks weird :/\n> >\n> > We could encapsulate it, but given that V4L2Camera and V4L2CameraProxy\n> > are internal to the V4L2 compatibility layer, I think it's overkill.\n> \n> Encapsulate would have been syntactic sugar. I would have had the\n> V4L2Camera handle locking internally, but that's fine.\n\nThe whole point of a semaphore is to handle consumers and producers\nliving in different threads, in this case in different objects. We could\nencapsulate it in a V4L2Camera method (something like\nV4L2Camera::waitForBuffer() for instance), that may be oevrkill.\n\n> >>> +}\n> \n> [snip]\n> \n> >>> +\n> >>> +int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)\n> >>> +{\n> >>> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querybuf\";\n> >>> +\tStream *stream = streamConfig_.stream();\n> >>> +\n> >>> +\tif (!validateStreamType(arg->type) ||\n> >>> +\t arg->index >= stream->buffers().size())\n> >>> +\t\treturn -EINVAL;\n> >>> +\n> >>> +\tupdateBuffers();\n> >>\n> >> updateBuffers() retreives information on completed buffers, while\n> >> querybuf can be called anytime after buffers have been requested.\n> \n> You below answer does not address this issue. What if querybuf is\n> called and not buffer completed yet (because say, streaming has not\n> started) ?\n\nThat's not a problem. buffers_ contains the most up-to-date information,\nand that's what querybuf will return.\n\n> >> From V4L2 specs:\n> >> This ioctl is part of the streaming I/O method. It can be used to\n> >> query the status of a buffer at any time after buffers have been\n> >> allocated with the ioctl VIDIOC_REQBUFS ioctl.\n> >>\n> >> It should thus depend on static information of allocated buffers not\n> >> just for completed ones...\n> >>\n> >> I suspect I am missing something, as this updateBuffers() thing\n> >> looks weird to me both here and in dqbuf.\n> >\n> > The idea is that the buffer status is kept in V4L2CameraProxy, and\n> > updated with updateBuffers(). The method is called both at querybuf and\n> > dqbuf time to ensure we have the latest status.\n> >\n> >>> +\n> >>> +\tmemcpy(arg, &buffers_[arg->index], sizeof(*arg));\n> >>> +\n> >>> +\tstruct v4l2_buffer &buf = buffers_[arg->index];\n> >>\n> >> I'm not sure this is right, index should be set by us, not received\n> >> from the application, there is no guarantee it is valid at all, am I wrong ?\n> >\n> > That's correct. This should be\n> >\n> > \tstruct v4l2_buffer &buf = buffers_[currentBuf_];\n> >\n> >> From V4L2 specs:\n> >> Applications call the VIDIOC_DQBUF ioctl to dequeue a filled\n> >> (capturing) or displayed (output) buffer from the driver’s outgoing\n> >> queue. They just set the type, memory and reserved fields of a struct\n> >> v4l2_buffer as above, when VIDIOC_DQBUF is called with a pointer to\n> >> this structure the driver fills the remaining fields or returns an\n> >> error code.\n> >>\n> >> I'm not a huge fan of this whole updateBuffers() mechanism, I agree\n> >> the V4L2Camera should keep a list of completed buffers, but why\n> >> would we want to copy here information for all of them when we dequeue\n> >> one only ?\n> >\n> > For performance reasons, and because we need to do so for querybuf too\n> > anyway.\n> >\n> >> I would simply keep a list of completed buffers in V4L2Camera, and get\n> >> the older on when dequeuing a buffer here.\n> >>\n> \n> If the two of you prefer this I'm fine. I still think it's unnecessary\n> clunky and we could have lived with V4L2CameraProxy picking the latest\n> completed buffer here at the expense of one inter-thread call per\n> request. We should probably profile this to decide what is more\n> efficient between an inter-thread call and a copy of a vector of\n> buffers.\n\nI'll bet on the copy of the vector :-)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4904760465\n\tfor ;\n\tThu, 2 Jan 2020 11:36:15 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F57E516;\n\tThu, 2 Jan 2020 11:36:14 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1577961374;\n\tbh=cdT/G50A5RDaZK0PmGjpAnOU1sIViQwWtPzGlGCE5r0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UUntbLUR6MpsNcU6ejKRtsiaWLJTUDhUwPb4iRNuf1CpiUpmoUNJWm4rU7tyS9VMy\n\t2WX+W49S2EJxQakCiwIKh9XZsm6akb9C8ENrvR5XRyhfcwObg7Wq9MiLl621vR1kOQ\n\tgCQ7HvwhpaBqPcx+ErSGxqOPAuy4vgenTGXoxW60=","Date":"Thu, 2 Jan 2020 12:36:05 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200102103605.GA4843@pendragon.ideasonboard.com>","References":"<20191231053314.20063-1-paul.elder@ideasonboard.com>\n\t<20191231053314.20063-5-paul.elder@ideasonboard.com>\n\t<20191231102209.jaqlwpjtcf6zs6uk@uno.localdomain>\n\t<20200102030214.GD15498@pendragon.ideasonboard.com>\n\t<20200102082034.lnddftpkd3qxytwo@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200102082034.lnddftpkd3qxytwo@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","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","X-List-Received-Date":"Thu, 02 Jan 2020 10:36:15 -0000"}}]