[{"id":3193,"web_url":"https://patchwork.libcamera.org/comment/3193/","msgid":"<20191206161203.gpbtfub44oarpzic@uno.localdomain>","date":"2019-12-06T16:12:03","subject":"Re: [libcamera-devel] [PATCH 2/2] 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\nOn Fri, Dec 06, 2019 at 04:26:54AM -0500, Paul Elder wrote:\n> Add libcamera V4L2 compatibility layer.\n\nMaybe just me, but having this patch split in components would have\nhelped not having to digest 1445 lines of patch in one go\n\nI know Niklas agrees :)\n\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> Note that until we have a way of mapping V4L2 device nodes to libcamera\n> cameras, the V4L2 compatibility layer will always selects and use the\n> first enumerated libcamera camera.\n\nThat's not trivial indeed...\n\nFor simple devices, we could easily collect the video nodes a camera\nuses and match them with the one the v4l2 application tries to use.\n\nFor complex devices, where the DMA output node could be multiplexed by\ndifferent cameras depending on the ISP configuration, this is not\neasy.\n\nI presume v4l2 application are mostly meant to be used with simple\ndevices, so adding a list of V4L2Videodevices to the Camera (while a\nbit of an hack) is totally doable. What do others think ?\n\n>\n> Signed-off-by: Paul Elder \n> ---\n> meson_options.txt | 5 +\n> src/meson.build | 4 +\n> src/v4l2/meson.build | 30 +++\n> src/v4l2/v4l2_camera.cpp | 312 ++++++++++++++++++++++\n> src/v4l2/v4l2_camera.h | 68 +++++\n> src/v4l2/v4l2_camera_proxy.cpp | 438 +++++++++++++++++++++++++++++++\n> src/v4l2/v4l2_camera_proxy.h | 63 +++++\n> src/v4l2/v4l2_compat.cpp | 81 ++++++\n> src/v4l2/v4l2_compat_manager.cpp | 353 +++++++++++++++++++++++++\n> src/v4l2/v4l2_compat_manager.h | 91 +++++++\n> 10 files changed, 1445 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..b06dd494 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 libcamera with 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..d96db4ff\n> --- /dev/null\n> +++ b/src/v4l2/meson.build\n> @@ -0,0 +1,30 @@\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\nSeems it is enough to undefine the _FILE_OFFSET_BITS macro to use the\n32-bit interface ?\n\nhttps://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html\nIf _FILE_OFFSET_BITS is undefined, or if it is defined to the value\n32, nothing changes. The 32 bit interface is used and types like off_t\nhave a size of 32 bits on 32 bit systems.\n\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..c6c84ef3\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -0,0 +1,312 @@\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> +#include \"v4l2_camera.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"v4l2_compat_manager.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(V4L2Compat);\n> +\n> +V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> +\t: camera_(camera), bufferCount_(0), isRunning_(false), sequence_(0),\n> +\t buffer_sema_(new Semaphore())\n\nCan't this be a class member instead of a pointer ?\nAlso, I would name it differently, or at least make it cameraCase_\n\n> +{\n> +\tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> +};\n> +\n> +V4L2Camera::~V4L2Camera()\n> +{\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tdelete pendingRequests_.front();\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +\n> +\tbuffer_lock_.lock();\n> +\twhile (!completedBuffers_.empty()) {\n> +\t\tdelete completedBuffers_.front();\n> +\t\tcompletedBuffers_.pop();\n> +\t}\n> +\tbuffer_lock_.unlock();\n> +\n> +\tdelete buffer_sema_;\n\nyou would save deleting it\n\n> +\n> +\tcamera_->release();\n> +}\n> +\n> +void V4L2Camera::open(int *ret, bool nonblock)\n> +{\n> +\tnonblock_ = nonblock;\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_ == nullptr) {\n> +\t\t*ret = -EINVAL;\n> +\t\treturn;\n> +\t}\n> +\n> +\t*ret = 0;\n> +}\n> +\n> +void V4L2Camera::close(int *ret)\n> +{\n> +\t*ret = camera_->release();\n> +}\n> +\n> +void V4L2Camera::getStreamConfig(StreamConfiguration *ret)\n\nMaybe *streaConfig ?\n\n> +{\n> +\t*ret = config_->at(0);\n> +}\n> +\n> +void V4L2Camera::requestComplete(Request *request)\n> +{\n> +\tif (request->status() == Request::RequestCancelled) {\n> +\t\tLOG(V4L2Compat, Error) << \"Request not succesfully completed: \"\n> +\t\t\t\t << request->status();\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* We only have one stream at the moment. */\n> +\tbuffer_lock_.lock();\n> +\tBuffer *buffer = request->buffers().begin()->second;\n> +\tcompletedBuffers_.push(buffer);\n> +\tbuffer_lock_.unlock();\n> +\n> +\tbuffer_sema_->release();\n> +}\n> +\n> +void V4L2Camera::configure(int *ret, struct v4l2_format *arg,\n> +\t\t\t unsigned int bufferCount)\n> +{\n> +\tconfig_->at(0).size.width = arg->fmt.pix.width;\n\nNit: can you use a reference to config_->at(0) ?\n\n> +\tconfig_->at(0).size.height = arg->fmt.pix.height;\n> +\tconfig_->at(0).pixelFormat =\n> +\t\tV4L2CompatManager::v4l2_to_drm(arg->fmt.pix.pixelformat);\n> +\tbufferCount_ = bufferCount;\n> +\tconfig_->at(0).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, Info) << \"Configuration invalid\";\n\nthat's probably an error, isn't it ?\n\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 << config_->at(0).toString();\n> +\n> +\t*ret = camera_->configure(config_.get());\n> +\tif (*ret < 0)\n> +\t\treturn;\n> +\n> +\tbufferCount_ = config_->at(0).bufferCount;\n> +\n> +\t*ret = 0;\n> +}\n> +\n> +void V4L2Camera::mmap(void **ret, void *addr, size_t length, int prot,\n> +\t\t int flags, off_t offset)\n\nDo we need to check flags ?\n\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing MMAP\";\n> +\n> +\tif (prot != (PROT_READ | PROT_WRITE)) {\n> +\t\terrno = EINVAL;\n\nThis function (actually all V4L2Camera functions) is called through a method\ninvocation and has a long call stack before getting back to the\ncaller. I wonder if errno does not get overwritten along that route.\n\nAlso, from man mmap:\n\nENOTSUP\nMAP_FIXED or MAP_PRIVATE was specified in the flags argument and the\nimplementation does not support this functionality.\n\nThe implementation does not support the combination of accesses\nrequested in the prot argument.\n\n> +\t\t*ret = MAP_FAILED;\n> +\t\treturn;\n> +\t}\n> +\n> +\tStreamConfiguration &streamConfig = config_->at(0);\n> +\tunsigned int sizeimage =\n> +\t\tV4L2CompatManager::image_size(\n> +\t\t\tV4L2CompatManager::drm_to_v4l2(streamConfig.pixelFormat),\n> +\t\t\tstreamConfig.size.width,\n> +\t\t\tstreamConfig.size.height);\n> +\tif (sizeimage == 0) {\n> +\t\terrno = EINVAL;\n> +\t\t*ret = MAP_FAILED;\n> +\t\treturn;\n> +\t}\n> +\n> +\tunsigned int index = offset / sizeimage;\n> +\tif (index * sizeimage != offset || length != sizeimage) {\n> +\t\terrno = EINVAL;\n\nIs EINVAL the right error here?\nfrom man mmap:\n\nENOMEM\nMAP_FIXED was specified, and the range [addr,addr+len)\nexceeds that allowed for the address space of a process; or, if\nMAP_FIXED was not specified and there is insufficient room in the\naddress space to effect the map‐ ping\n\nEOVERFLOW\nThe file is a regular file and the value of off plus len exceeds the\noffset maximum established in the open file description associated\nwith fildes.\n\n?\n\nHowever I'm not sure we should care about the mmap errors at this\nlevel, as those should be returned when the actual mapping is\nperformed. What do you think ?\n\n> +\t\t*ret = MAP_FAILED;\n> +\t\treturn;\n> +\t}\n> +\n> +\tStream *stream = *camera_->streams().begin();\n> +\t*ret = stream->buffers()[index].planes()[0].mem();\n> +}\n> +\n> +void V4L2Camera::munmap(int *ret, void *addr, size_t length)\n> +{\n> +\tStreamConfiguration &streamConfig = config_->at(0);\n> +\tunsigned int sizeimage =\n> +\t\tV4L2CompatManager::image_size(streamConfig.pixelFormat,\n> +\t\t\t\t\t streamConfig.size.width,\n> +\t\t\t\t\t streamConfig.size.height);\n> +\n> +\tif (length != sizeimage) {\n> +\t\terrno = EINVAL;\n\nHere EINVAL seems to be appropriate\n\n> +\t\t*ret = -1;\n> +\t\treturn;\n> +\t}\n> +\n> +\t*ret = 0;\n> +}\n> +\n> +void V4L2Camera::validStreamType(bool *ret, uint32_t type)\n> +{\n> +\t*ret = (type == V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> +}\n> +\n> +void V4L2Camera::validMemoryType(bool *ret, uint32_t memory)\n> +{\n> +\t*ret = (memory == V4L2_MEMORY_MMAP);\n> +}\n> +\n> +void V4L2Camera::allocBuffers(int *ret, unsigned int count)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Allocating libcamera bufs\";\n> +\t*ret = camera_->allocateBuffers();\n\nI fear the buffer rework would required a big rebase of this series :(\n\n> +\tif (*ret == -EACCES)\n> +\t\t*ret = -EBUSY;\n> +}\n> +\n> +/* \\todo implement allocDMABuffers */\n> +void V4L2Camera::allocDMABuffers(int *ret, unsigned int count)\n> +{\n> +\t*ret = -ENOTTY;\n> +}\n> +\n> +void V4L2Camera::freeBuffers()\n> +{\n> +\tcamera_->freeBuffers();\n> +\tbufferCount_ = 0;\n> +}\n> +\n> +void V4L2Camera::streamOn(int *ret)\n> +{\n> +\tif (isRunning_) {\n> +\t\t*ret = 0;\n> +\t\treturn;\n> +\t}\n> +\n> +\tsequence_ = 0;\n> +\n> +\t*ret = camera_->start();\n> +\tif (*ret < 0)\n> +\t\treturn;\n\nerrno ?\n\n> +\tisRunning_ = true;\n> +\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\t*ret = camera_->queueRequest(pendingRequests_.front());\n> +\t\tpendingRequests_.pop();\n> +\t\tif (*ret < 0)\n> +\t\t\treturn;\n> +\t}\n> +\n> +\t*ret = 0;\n> +}\n> +\n> +void V4L2Camera::streamOff(int *ret)\n> +{\n> +\t/* \\todo restore buffers to reqbufs state? */\n> +\tif (!isRunning_) {\n> +\t\t*ret = 0;\n> +\t\treturn;\n> +\t}\n> +\n> +\t*ret = camera_->stop();\n> +\tif (*ret < 0)\n> +\t\treturn;\n> +\tisRunning_ = false;\n> +\n> +\t*ret = 0;\n\nHere and in streamOn you could set *ret = 0 at the beginning of the\nfunction.\n\n> +}\n> +\n> +void V4L2Camera::qbuf(int *ret, struct v4l2_buffer *arg)\n> +{\n> +\tStream *stream = config_->at(0).stream();\n> +\tstd::unique_ptr buffer = stream->createBuffer(arg->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> +\tRequest *request = camera_->createRequest();\n\nparanoid: createRequest() can return nullptr\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\treturn;\n> +\t}\n> +\n> +\tif (!isRunning_) {\n> +\t\tpendingRequests_.push(request);\n> +\t} else {\n> +\t\t*ret = camera_->queueRequest(request);\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> +\targ->flags |= V4L2_BUF_FLAG_QUEUED;\n> +\targ->flags |= V4L2_BUF_FLAG_MAPPED;\n> +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> +\t*ret = 0;\n> +}\n> +\n> +int V4L2Camera::dqbuf(struct v4l2_buffer *arg)\n> +{\n> +\tif (nonblock_ && !buffer_sema_->tryAcquire())\n> +\t\treturn -EAGAIN;\n> +\telse\n> +\t\tbuffer_sema_->acquire();\n> +\n> +\tbuffer_lock_.lock();\n> +\tBuffer *buffer = completedBuffers_.front();\n> +\tcompletedBuffers_.pop();\n> +\tbuffer_lock_.unlock();\n> +\n> +\targ->bytesused = buffer->bytesused();\n> +\targ->field = V4L2_FIELD_NONE;\n> +\targ->timestamp.tv_sec = buffer->timestamp() / 1000000000;\n> +\targ->timestamp.tv_usec = buffer->timestamp() / 1000;\n> +\targ->sequence = sequence_++;\n\nDon't we have sequence in Buffer ?\n\n> +\n> +\targ->flags &= ~V4L2_BUF_FLAG_QUEUED;\n> +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> +\n> +\tStreamConfiguration &streamConfig = config_->at(0);\n> +\tunsigned int sizeimage =\n> +\t\tV4L2CompatManager::image_size(streamConfig.pixelFormat,\n> +\t\t\t\t\t streamConfig.size.width,\n> +\t\t\t\t\t streamConfig.size.height);\n> +\targ->length = sizeimage;\n\nYou can save the sizeimage variable.\nI wonder if this needs to be re-calculated everytime... nothing big\nhowever...\n\n> +\n> +\treturn 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..3d3cd8ff\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -0,0 +1,68 @@\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> +#ifndef __V4L2_CAMERA_H__\n> +#define __V4L2_CAMERA_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \"semaphore.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class V4L2Camera : public Object\n> +{\n> +public:\n> +\tV4L2Camera(std::shared_ptr camera);\n> +\t~V4L2Camera();\n> +\n> +\tvoid open(int *ret, bool nonblock);\n> +\tvoid close(int *ret);\n> +\tvoid getStreamConfig(StreamConfiguration *ret);\n> +\tvoid requestComplete(Request *request);\n> +\n> +\tvoid mmap(void **ret, void *addr, size_t length, int prot, int flags,\n> +\t\t off_t offset);\n> +\tvoid munmap(int *ret, void *addr, size_t length);\n> +\n> +\tvoid configure(int *ret, struct v4l2_format *arg,\n> +\t\t unsigned int bufferCount);\n> +\n> +\tvoid validStreamType(bool *ret, uint32_t type);\n> +\tvoid validMemoryType(bool *ret, uint32_t memory);\n> +\n> +\tvoid allocBuffers(int *ret, unsigned int count);\n> +\tvoid allocDMABuffers(int *ret, unsigned int count);\n> +\tvoid freeBuffers();\n> +\tvoid streamOn(int *ret);\n> +\tvoid streamOff(int *ret);\n> +\n> +\tvoid qbuf(int *ret, struct v4l2_buffer *arg);\n> +\tint dqbuf(struct v4l2_buffer *arg);\n> +\n> +private:\n> +\tvoid initDefaultV4L2Format();\n> +\n> +\tstd::shared_ptr camera_;\n> +\tstd::unique_ptr config_;\n> +\n> +\tunsigned int bufferCount_;\n> +\tbool isRunning_;\n> +\tbool nonblock_;\n> +\n> +\tunsigned int sequence_;\n> +\n> +\tSemaphore *buffer_sema_;\n> +\tstd::mutex buffer_lock_;\n> +\n> +\tstd::queue pendingRequests_;\n> +\tstd::queue 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..1dd2c363\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -0,0 +1,438 @@\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> +#include \"v4l2_camera_proxy.h\"\n> +\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: index_(index), bufferCount_(0), currentBuf_(0),\n> +\t vcam_(utils::make_unique(camera))\n> +{\n> +\tquerycap(camera);\n> +}\n> +\n> +V4L2CameraProxy::~V4L2CameraProxy()\n> +{\n> +\tvcam_.reset();\n\nNot sure it is necessary to reset a unique_ptr<> at destruction time,\nthe managed pointer will be destroyed anyway.\n\n> +}\n> +\n> +int V4L2CameraProxy::open(bool nonblock)\n> +{\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,\n> +\t\t\t &ret, nonblock);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> +\tsetFmtFromConfig();\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::close()\n> +{\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);\n> +\treturn ret;\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> +\tvoid *val;\n> +\tvcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,\n> +\t\t\t &val, addr, length, prot, flags, offset);\n> +\treturn val;\n> +}\n> +\n> +int V4L2CameraProxy::munmap(void *addr, size_t length)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing MUNMAP\";\n> +\n> +\tint ret;\n> +\tvcam_->invokeMethod(&V4L2Camera::munmap, ConnectionTypeBlocking,\n> +\t\t\t &ret, addr, length);\n> +\treturn ret;\n> +}\n> +\n> +bool V4L2CameraProxy::hasPixelFormat(unsigned int format)\n> +{\n> +\tconst std::vector &formats =\n> +\t\tstreamConfig_.formats().pixelformats();\n> +\treturn std::find(formats.begin(), formats.end(), format) != formats.end();\n> +}\n> +\n> +/* \\todo getDeviceCaps? getMemoryCaps? */\n> +\n> +bool V4L2CameraProxy::hasSize(unsigned int format, Size size)\n> +{\n> +\tint len = streamConfig_.formats().sizes(format).size();\n> +\tfor (int i = 0; i < len; i++)\n> +\t\tif (streamConfig_.formats().sizes(format)[i] == size)\n> +\t\t\treturn true;\n\nCan't you find() on the vector returned by\nstreamConfig_.formats().sizes(format) ?\n\n> +\n> +\treturn false;\n> +}\n> +\n> +bool V4L2CameraProxy::validateStreamType(uint32_t type)\n> +{\n> +\tbool valid;\n> +\tvcam_->invokeMethod(&V4L2Camera::validStreamType,\n> +\t\t\t ConnectionTypeBlocking, &valid, type);\n> +\tif (!valid)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)\n> +{\n> +\tbool valid;\n> +\tvcam_->invokeMethod(&V4L2Camera::validMemoryType,\n> +\t\t\t ConnectionTypeBlocking, &valid, memory);\n> +\tif (!valid)\n> +\t\treturn false;\n> +\n> +\treturn true;\n\nIn this two functions you can here just return 'valid'\n\n> +}\n> +\n> +void V4L2CameraProxy::setFmtFromConfig()\n> +{\n> +\tcurV4L2Format_.fmt.pix.width = streamConfig_.size.width;\n> +\tcurV4L2Format_.fmt.pix.height = streamConfig_.size.height;\n> +\tcurV4L2Format_.fmt.pix.pixelformat =\n> +\t\tV4L2CompatManager::drm_to_v4l2(streamConfig_.pixelFormat);\n> +\tcurV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;\n> +\tcurV4L2Format_.fmt.pix.bytesperline =\n> +\t\tV4L2CompatManager::bpl_multiplier(\n> +\t\t\tcurV4L2Format_.fmt.pix.pixelformat) *\n> +\t\tcurV4L2Format_.fmt.pix.width;\n> +\tcurV4L2Format_.fmt.pix.sizeimage =\n> +\t\tV4L2CompatManager::image_size(curV4L2Format_.fmt.pix.pixelformat,\n> +\t\t\t\t\t curV4L2Format_.fmt.pix.width,\n> +\t\t\t\t\t curV4L2Format_.fmt.pix.height);\n> +\tcurV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\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> +\tcapabilities_.version = KERNEL_VERSION(5, 2, 0);\n> +\tcapabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;\n\nAre we single planar only ? I guess so, it's fine :)\n\n> +\tcapabilities_.capabilities =\n> +\t\tcapabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;\n> +\tmemset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));\n> +}\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\treturn -EINVAL;\n> +\tif (arg->index > streamConfig_.formats().pixelformats().size())\n> +\t\treturn -EINVAL;\n> +\n> +\tmemcpy(arg->description, \"asdf\", 5);\n\nThat's a real nice format name! :D\nDo we need a map of formats to descriptions ?\n\n> +\targ->pixelformat =\n> +\t\tV4L2CompatManager::drm_to_v4l2(\n> +\t\t\tstreamConfig_.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> +\targ->fmt.pix.width = curV4L2Format_.fmt.pix.width;\n> +\targ->fmt.pix.height = curV4L2Format_.fmt.pix.height;\n> +\targ->fmt.pix.pixelformat = curV4L2Format_.fmt.pix.pixelformat;\n> +\targ->fmt.pix.field = curV4L2Format_.fmt.pix.field;\n> +\targ->fmt.pix.bytesperline = curV4L2Format_.fmt.pix.bytesperline;\n> +\targ->fmt.pix.sizeimage = curV4L2Format_.fmt.pix.sizeimage;\n> +\targ->fmt.pix.colorspace = curV4L2Format_.fmt.pix.colorspace;\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> +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> +\t\t\t &ret, arg, bufferCount_);\n> +\tif (ret < 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> +\tsetFmtFromConfig();\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> +\tif (!hasPixelFormat(format))\n> +\t\tformat = streamConfig_.formats().pixelformats()[0];\n> +\n> +\tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> +\tif (!hasSize(format, size))\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 =\n> +\t\tV4L2CompatManager::bpl_multiplier(\n> +\t\t\tV4L2CompatManager::drm_to_v4l2(format)) *\n> +\t\targ->fmt.pix.width;\n> +\targ->fmt.pix.sizeimage =\n> +\t\tV4L2CompatManager::image_size(\n> +\t\t\tV4L2CompatManager::drm_to_v4l2(format),\n> +\t\t\targ->fmt.pix.width, arg->fmt.pix.height);\n> +\targ->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing VIDIOC_REQBUFS\";\n> +\tif (!validateStreamType(arg->type))\n> +\t\treturn -EINVAL;\n> +\tif (!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\tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> +\t\tint ret;\n> +\t\tvcam_->invokeMethod(&V4L2Camera::streamOff,\n> +\t\t\t\t ConnectionTypeBlocking, &ret);\n> +\t\tvcam_->invokeMethod(&V4L2Camera::freeBuffers,\n> +\t\t\t\t ConnectionTypeBlocking);\n> +\t\tbufferCount_ = 0;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint ret;\n\nas ret is used function-wise, I would its declaration to the beginning\n\n> +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> +\t\t\t &ret, &curV4L2Format_, arg->count);\n> +\tif (ret < 0)\n> +\t\treturn -EINVAL;\n> +\targ->count = streamConfig_.bufferCount;\n> +\tbufferCount_ = arg->count;\n> +\n> +\tret = -EINVAL;\n> +\tif (arg->memory == V4L2_MEMORY_MMAP)\n> +\t\tvcam_->invokeMethod(&V4L2Camera::allocBuffers,\n> +\t\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> +\telse if (arg->memory == V4L2_MEMORY_DMABUF)\n\nDo we even claim support for this ?\n\n> +\t\tvcam_->invokeMethod(&V4L2Camera::allocDMABuffers,\n> +\t\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> +\n> +\tif (ret < 0) {\n> +\t\targ->count = 0;\n> +\t\treturn ret == -EACCES ? -EBUSY : ret;\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\treturn -EINVAL;\n> +\tif (arg->index >= stream->buffers().size())\n> +\t\treturn -EINVAL;\n> +\n> +\tunsigned int index = arg->index;\n> +\tmemset(arg, 0, sizeof(*arg));\n> +\targ->index = index;\n> +\targ->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> +\targ->length = curV4L2Format_.fmt.pix.sizeimage;\n> +\targ->memory = V4L2_MEMORY_MMAP;\n> +\targ->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;\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\treturn -EINVAL;\n> +\tif (!validateMemoryType(arg->memory))\n> +\t\treturn -EINVAL;\n> +\tif (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);\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\treturn -EINVAL;\n> +\tif (!validateMemoryType(arg->memory))\n> +\t\treturn -EINVAL;\n> +\n> +\targ->index = currentBuf_;\n> +\tcurrentBuf_ = (currentBuf_ + 1) % bufferCount_;\n> +\n> +\treturn vcam_->dqbuf(arg);\n\nIs dqbuf special ?\n\nI know it could return immediately if nonblock_ is set, but\ninvokeMethod checks that invoked method is called, if it returns\nimmediately, that's fine. Or have I missed some other reason why this\nis called directly.\n\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> +\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> +\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\ncamelCase for method names as well ?\n\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> +\tcase VIDIOC_EXPBUF:\n> +\tcase VIDIOC_ENUM_FRAMESIZES:\n> +\tdefault:\n> +\t\tret = -ENOTTY;\n> +\t}\n> +\n> +\tif (ret < 0) {\n> +\t\terrno = ret;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\terrno = 0;\n> +\treturn ret;\n> +\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..64c7aadd\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -0,0 +1,63 @@\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> +#ifndef __V4L2_CAMERA_PROXY_H__\n> +#define __V4L2_CAMERA_PROXY_H__\n> +\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> +\t~V4L2CameraProxy();\n> +\n> +\tint open(bool nonblock);\n> +\tint 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> +private:\n> +\tbool hasPixelFormat(unsigned int format);\n> +\tbool hasSize(unsigned int format, Size size);\n> +\tbool validateStreamType(uint32_t type);\n> +\tbool validateMemoryType(uint32_t memory);\n> +\tvoid setFmtFromConfig();\n> +\tvoid querycap(std::shared_ptr camera);\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 index_;\n> +\n> +\tstruct v4l2_format curV4L2Format_;\n> +\tStreamConfiguration streamConfig_;\n> +\tstruct v4l2_capability capabilities_;\n> +\tunsigned int bufferCount_;\n> +\tunsigned int currentBuf_;\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..3330e7bc\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_compat.cpp\n> @@ -0,0 +1,81 @@\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> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#define LIBCAMERA_PUBLIC __attribute__((visibility(\"default\")))\n\nAm I wrong or this makes sense only if we compile with\n-fvisiblity=hidden ?\nhttps://gcc.gnu.org/wiki/Visibility\n\nI welcome this change, but then probably you should add that\ncompilation flag to the v4l2 compat library, it I have not\nmis-interpreted the above wiki page\n\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> +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 openat(AT_FDCWD, path, 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> +\tvoid *val = V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);\n> +\treturn val;\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..4eeb714f\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_compat_manager.cpp\n> @@ -0,0 +1,353 @@\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> +#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> +#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: cameraManagerRunning_(false), cm_(nullptr)\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\nYou seem to be mixing cameraCase and snake_case here and there in\nvariable declaration. Was this intentional ?\n\n> +}\n> +\n> +V4L2CompatManager::~V4L2CompatManager()\n> +{\n> +\tdevices_.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);\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\n> +\t * gets created here to wraps a camera device.\n> +\t */\n> +\t// \\todo map v4l2 devices to libcamera cameras; minor device node?\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\n> +\t * as we're now ready to handle calls from the framework.\n> +\t */\n> +\tcv_.notify_one();\n> +\n> +\t/* Now start processing events and messages. */\n> +\texec();\n> +\n> +\tcm_->stop();\n> +\tdelete cm_;\n> +\tcm_ = nullptr;\n> +}\n> +\n> +V4L2CompatManager *V4L2CompatManager::instance()\n> +{\n> +\tstatic V4L2CompatManager v4l2CompatManager;\n> +\treturn &v4l2CompatManager;\n> +}\n> +\n> +bool V4L2CompatManager::validfd(int fd)\n> +{\n> +\treturn devices_.find(fd) != devices_.end();\n> +}\n> +\n> +bool V4L2CompatManager::validmmap(void *addr)\n> +{\n> +\treturn mmaps_.find(addr) != mmaps_.end();\n> +}\n> +\n> +std::shared_ptr V4L2CompatManager::getCamera(int fd)\n> +{\n> +\tif (!validfd(fd))\n> +\t\treturn nullptr;\n> +\n> +\treturn devices_.at(fd);\n\nThis is safe, but it's a double lookup, same as below. This is minor\nindeed, but probably using the iterator returned from find() directly\nis slightly more efficient.\n\nThen you can inline the valid[fd|mmap}() methods, probably\n\n> +}\n> +\n> +std::shared_ptr V4L2CompatManager::getCamera(void *addr)\n> +{\n> +\tif (!validmmap(addr))\n> +\t\treturn nullptr;\n> +\n> +\treturn devices_.at(mmaps_.at(addr));\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> +\tif (std::string(path).find(\"video\") == std::string::npos)\n> +\t\treturn fd;\n> +\n> +\tif (!isRunning())\n> +\t\tinit();\n> +\n> +\t/* \\todo map v4l2 devnodes to libcamera cameras */\n> +\tunsigned int camera_index = 0;\n> +\n> +\tstd::shared_ptr proxy = proxies_[camera_index];\n> +\tint ret = proxy->open(mode & O_NONBLOCK);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tint efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));\n> +\tif (efd < 0)\n> +\t\treturn errno;\n\nI'm not sure I get this usage of the file descriptor returned by\neventfd...\n\nIt is used as index in the map, possibly replaced by dup(). I would\nhave expected the fd returned by openat() to be used as index in the\nmap... What am I missing ?\n\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> +\tif (validfd(oldfd))\n\nShouldn't you return an error if the fd to duplicate is not valid, ad\ndup() only if it is ?\n\n> +\t\tdevices_[newfd] = devices_[oldfd];\n> +\n> +\treturn newfd;\n> +}\n> +\n> +int V4L2CompatManager::close(int fd)\n> +{\n> +\tif (validfd(fd) && devices_[fd]->close() < 0)\n\nI would return -EIO if !validfd() and propagate the error up if\nclose() fails.\n\n> +\t\treturn -EIO;\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> +\tif (!validfd(fd))\n> +\t\treturn mmap_func_(addr, length, prot, flags, fd, offset);\n> +\n> +\tvoid *map = getCamera(fd)->mmap(addr, length, prot, flags, offset);\n> +\tif (map == MAP_FAILED) {\n> +\t\terrno = EINVAL;\n> +\t\treturn map;\n> +\t}\n> +\n> +\tmmaps_[addr] = fd;\n> +\treturn map;\n> +}\n> +\n> +int V4L2CompatManager::munmap(void *addr, size_t length)\n> +{\n> +\tif (!validmmap(addr))\n> +\t\treturn munmap_func_(addr, length);\n> +\n> +\tint ret = getCamera(addr)->munmap(addr, length);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tmmaps_.erase(addr);\n> +\taddr = nullptr;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)\n> +{\n> +\tstd::shared_ptr proxy = getCamera(fd);\n> +\tif (!proxy)\n> +\t\treturn ioctl_func_(fd, request, arg);\n> +\n\nWhat is the use case for bypassing the proxy ? That might be my\nlimited knowledge of how v4l2 application behave\n\n> +\treturn proxy->ioctl(request, arg);\n> +}\n> +\n> +/* \\todo make libcamera export these */\n\nmmm, V4L2VideoDevice has very similar format conversion routines... we\nshould really centralize them somehow.. maybe not in scope for this\npatch, but replicating cose which is tricky to get right due to the\ndifferent format definition between DRM and V4L2 is not a good idea.\n\n> +int V4L2CompatManager::bpl_multiplier(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 V4L2CompatManager::image_size(unsigned int format,\n> +\t\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 V4L2CompatManager::v4l2_to_drm(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 V4L2CompatManager::drm_to_v4l2(unsigned int pixelformat)\n> +{\n> +\tswitch (pixelformat) {\n> +\t/* RGB formats. */\n> +\tcase DRM_FORMAT_BGR888:\n> +\t\treturn V4L2_PIX_FMT_BGR24;\n\nThis in example does not match the one we have in V4L2Videodevice.\nDRM_FORMAT_BGR24 = V4L2_PIX_FMT_RGB24 not BGR24.\n\nI know, it's a pain.\n\nI think the other version is correct, as we validated them using\nconversion routines from kernel drivers...\n\nThere might be more below (and possibly above)\n\n> +\tcase DRM_FORMAT_RGB888:\n> +\t\treturn V4L2_PIX_FMT_RGB24;\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_compat_manager.h b/src/v4l2/v4l2_compat_manager.h\n> new file mode 100644\n> index 00000000..492f97fd\n> --- /dev/null\n> +++ b/src/v4l2/v4l2_compat_manager.h\n> @@ -0,0 +1,91 @@\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> +#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> +#include \n> +#include \n> +\n> +#include \n> +#include \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> +\tint openat(int dirfd, const char *path, int oflag, mode_t mode);\n> +\n> +\tstd::shared_ptr getCamera(int fd);\n> +\tstd::shared_ptr getCamera(void *addr);\n\nEvery time you return a shared_ptr by value, you increase the\nreference count for no good reason. I would either return by reference\nor return V4L2CameraProxy and use shared pointers only in the proxies_\narray and not in the rest of the code base.\n\nOverall this is a very good first submission and I'm happy to see it\nposted to the list \\o/\n\nI admit in this first pass I didn't go into extreme detail on the way\nthe v4l2 operations semantic is handled, but it seems sane to me!\n\nThanks\n j\n\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> +\tstatic int bpl_multiplier(unsigned int format);\n> +\tstatic int image_size(unsigned int format, unsigned int width,\n> +\t\t\t unsigned int height);\n> +\n> +\tstatic unsigned int v4l2_to_drm(unsigned int pixelformat);\n> +\tstatic unsigned int drm_to_v4l2(unsigned int pixelformat);\n> +\n> +private:\n> +\tV4L2CompatManager();\n> +\t~V4L2CompatManager();\n> +\n> +\tvoid run() override;\n> +\n> +\tbool validfd(int fd);\n> +\tbool validmmap(void *addr);\n> +\n> +\tint default_ioctl(int fd, unsigned long request, void *arg);\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> +\tbool cameraManagerRunning_;\n> +\tCameraManager *cm_;\n> +\n> +\tstd::mutex mutex_;\n> +\tstd::condition_variable cv_;\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 325AD60BBC\n\tfor ;\n\tFri, 6 Dec 2019 17:09:58 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 62ADC1C0002;\n\tFri, 6 Dec 2019 16:09:57 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Fri, 6 Dec 2019 17:12:03 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191206161203.gpbtfub44oarpzic@uno.localdomain>","References":"<20191206092654.24340-1-paul.elder@ideasonboard.com>\n\t<20191206092654.24340-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ulswirfikdzfshpl\"","Content-Disposition":"inline","In-Reply-To":"<20191206092654.24340-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 06 Dec 2019 16:09:58 -0000"}},{"id":3212,"web_url":"https://patchwork.libcamera.org/comment/3212/","msgid":"<20191209011031.GA12803@localhost.localdomain>","date":"2019-12-09T01:10:31","subject":"Re: [libcamera-devel] [PATCH 2/2] v4l2: v4l2_compat: Add V4L2\n\tcompatibility layer","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the review.\n\nOn Fri, Dec 06, 2019 at 05:12:03PM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Dec 06, 2019 at 04:26:54AM -0500, Paul Elder wrote:\n> > Add libcamera V4L2 compatibility layer.\n> \n> Maybe just me, but having this patch split in components would have\n> helped not having to digest 1445 lines of patch in one go\n\nLaurent said it was okay... :)\n\n> I know Niklas agrees :)\n> \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> > Note that until we have a way of mapping V4L2 device nodes to libcamera\n> > cameras, the V4L2 compatibility layer will always selects and use the\n> > first enumerated libcamera camera.\n> \n> That's not trivial indeed...\n> \n> For simple devices, we could easily collect the video nodes a camera\n> uses and match them with the one the v4l2 application tries to use.\n> \n> For complex devices, where the DMA output node could be multiplexed by\n> different cameras depending on the ISP configuration, this is not\n> easy.\n> \n> I presume v4l2 application are mostly meant to be used with simple\n> devices, so adding a list of V4L2Videodevices to the Camera (while a\n> bit of an hack) is totally doable. What do others think ?\n\nI'm thinking that it's not a good idea to expose anything V4L2 from\nCamera. If it's just UVC devices, might we match instead by the name\nexposed by sysfs? The uvcvideo pipeline handler names the Camera based\non that name anyway.\n\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > meson_options.txt | 5 +\n> > src/meson.build | 4 +\n> > src/v4l2/meson.build | 30 +++\n> > src/v4l2/v4l2_camera.cpp | 312 ++++++++++++++++++++++\n> > src/v4l2/v4l2_camera.h | 68 +++++\n> > src/v4l2/v4l2_camera_proxy.cpp | 438 +++++++++++++++++++++++++++++++\n> > src/v4l2/v4l2_camera_proxy.h | 63 +++++\n> > src/v4l2/v4l2_compat.cpp | 81 ++++++\n> > src/v4l2/v4l2_compat_manager.cpp | 353 +++++++++++++++++++++++++\n> > src/v4l2/v4l2_compat_manager.h | 91 +++++++\n> > 10 files changed, 1445 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..b06dd494 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 libcamera with 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..d96db4ff\n> > --- /dev/null\n> > +++ b/src/v4l2/meson.build\n> > @@ -0,0 +1,30 @@\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> \n> Seems it is enough to undefine the _FILE_OFFSET_BITS macro to use the\n> 32-bit interface ?\n\nLooks like it is.\n\n> https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html\n> If _FILE_OFFSET_BITS is undefined, or if it is defined to the value\n> 32, nothing changes. The 32 bit interface is used and types like off_t\n> have a size of 32 bits on 32 bit systems.\n> \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..c6c84ef3\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -0,0 +1,312 @@\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> > +#include \"v4l2_camera.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"v4l2_compat_manager.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DECLARE_CATEGORY(V4L2Compat);\n> > +\n> > +V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> > +\t: camera_(camera), bufferCount_(0), isRunning_(false), sequence_(0),\n> > +\t buffer_sema_(new Semaphore())\n> \n> Can't this be a class member instead of a pointer ?\n> Also, I would name it differently, or at least make it cameraCase_\n\nack\n\n> > +{\n> > +\tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> > +};\n> > +\n> > +V4L2Camera::~V4L2Camera()\n> > +{\n> > +\twhile (!pendingRequests_.empty()) {\n> > +\t\tdelete pendingRequests_.front();\n> > +\t\tpendingRequests_.pop();\n> > +\t}\n> > +\n> > +\tbuffer_lock_.lock();\n> > +\twhile (!completedBuffers_.empty()) {\n> > +\t\tdelete completedBuffers_.front();\n> > +\t\tcompletedBuffers_.pop();\n> > +\t}\n> > +\tbuffer_lock_.unlock();\n> > +\n> > +\tdelete buffer_sema_;\n> \n> you would save deleting it\n> \n> > +\n> > +\tcamera_->release();\n> > +}\n> > +\n> > +void V4L2Camera::open(int *ret, bool nonblock)\n> > +{\n> > +\tnonblock_ = nonblock;\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_ == nullptr) {\n> > +\t\t*ret = -EINVAL;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t*ret = 0;\n> > +}\n> > +\n> > +void V4L2Camera::close(int *ret)\n> > +{\n> > +\t*ret = camera_->release();\n> > +}\n> > +\n> > +void V4L2Camera::getStreamConfig(StreamConfiguration *ret)\n> \n> Maybe *streaConfig ?\n\nack\n\n> > +{\n> > +\t*ret = config_->at(0);\n> > +}\n> > +\n> > +void V4L2Camera::requestComplete(Request *request)\n> > +{\n> > +\tif (request->status() == Request::RequestCancelled) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Request not succesfully completed: \"\n> > +\t\t\t\t << request->status();\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t/* We only have one stream at the moment. */\n> > +\tbuffer_lock_.lock();\n> > +\tBuffer *buffer = request->buffers().begin()->second;\n> > +\tcompletedBuffers_.push(buffer);\n> > +\tbuffer_lock_.unlock();\n> > +\n> > +\tbuffer_sema_->release();\n> > +}\n> > +\n> > +void V4L2Camera::configure(int *ret, struct v4l2_format *arg,\n> > +\t\t\t unsigned int bufferCount)\n> > +{\n> > +\tconfig_->at(0).size.width = arg->fmt.pix.width;\n> \n> Nit: can you use a reference to config_->at(0) ?\n\nack\n\n> > +\tconfig_->at(0).size.height = arg->fmt.pix.height;\n> > +\tconfig_->at(0).pixelFormat =\n> > +\t\tV4L2CompatManager::v4l2_to_drm(arg->fmt.pix.pixelformat);\n> > +\tbufferCount_ = bufferCount;\n> > +\tconfig_->at(0).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, Info) << \"Configuration invalid\";\n> \n> that's probably an error, isn't it ?\n\nYep.\n\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 << config_->at(0).toString();\n> > +\n> > +\t*ret = camera_->configure(config_.get());\n> > +\tif (*ret < 0)\n> > +\t\treturn;\n> > +\n> > +\tbufferCount_ = config_->at(0).bufferCount;\n> > +\n> > +\t*ret = 0;\n> > +}\n> > +\n> > +void V4L2Camera::mmap(void **ret, void *addr, size_t length, int prot,\n> > +\t\t int flags, off_t offset)\n> \n> Do we need to check flags ?\n\nNo.\n\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing MMAP\";\n> > +\n> > +\tif (prot != (PROT_READ | PROT_WRITE)) {\n> > +\t\terrno = EINVAL;\n> \n> This function (actually all V4L2Camera functions) is called through a method\n> invocation and has a long call stack before getting back to the\n> caller. I wonder if errno does not get overwritten along that route.\n\nYou're right; it doesn't seem to be kept. The caller still receives an\nEINVAL though (but only EINVAL).\n\nI looked back at this and I think I messed up some errno passing.\n\n> Also, from man mmap:\n> \n> ENOTSUP\n> MAP_FIXED or MAP_PRIVATE was specified in the flags argument and the\n> implementation does not support this functionality.\n>\n> The implementation does not support the combination of accesses\n> requested in the prot argument.\n\nI don't see this in my man mmap... oh wait I found it in the posix man.\n\nYeah, this is probably better.\n\n> \n> > +\t\t*ret = MAP_FAILED;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tStreamConfiguration &streamConfig = config_->at(0);\n> > +\tunsigned int sizeimage =\n> > +\t\tV4L2CompatManager::image_size(\n> > +\t\t\tV4L2CompatManager::drm_to_v4l2(streamConfig.pixelFormat),\n> > +\t\t\tstreamConfig.size.width,\n> > +\t\t\tstreamConfig.size.height);\n> > +\tif (sizeimage == 0) {\n> > +\t\terrno = EINVAL;\n> > +\t\t*ret = MAP_FAILED;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tunsigned int index = offset / sizeimage;\n> > +\tif (index * sizeimage != offset || length != sizeimage) {\n> > +\t\terrno = EINVAL;\n> \n> Is EINVAL the right error here?\n> from man mmap:\n> \n> ENOMEM\n> MAP_FIXED was specified, and the range [addr,addr+len)\n> exceeds that allowed for the address space of a process; or, if\n> MAP_FIXED was not specified and there is insufficient room in the\n> address space to effect the mapping\n\nI don't think this is right, as ENOMEM indicates some form of\n\"out of memory\", but we are checking for alignment.\n\n> EOVERFLOW\n> The file is a regular file and the value of off plus len exceeds the\n> offset maximum established in the open file description associated\n> with fildes.\n\nSimilar issue here.\n\n> However I'm not sure we should care about the mmap errors at this\n> level, as those should be returned when the actual mapping is\n> performed. What do you think ?\n\nI think we should already return the errors at this point, especially\nsince these are all input validation for the mmap for V4L2 devices.\n\n> > +\t\t*ret = MAP_FAILED;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tStream *stream = *camera_->streams().begin();\n> > +\t*ret = stream->buffers()[index].planes()[0].mem();\n> > +}\n> > +\n> > +void V4L2Camera::munmap(int *ret, void *addr, size_t length)\n> > +{\n> > +\tStreamConfiguration &streamConfig = config_->at(0);\n> > +\tunsigned int sizeimage =\n> > +\t\tV4L2CompatManager::image_size(streamConfig.pixelFormat,\n> > +\t\t\t\t\t streamConfig.size.width,\n> > +\t\t\t\t\t streamConfig.size.height);\n> > +\n> > +\tif (length != sizeimage) {\n> > +\t\terrno = EINVAL;\n> \n> Here EINVAL seems to be appropriate\n> \n> > +\t\t*ret = -1;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t*ret = 0;\n> > +}\n> > +\n> > +void V4L2Camera::validStreamType(bool *ret, uint32_t type)\n> > +{\n> > +\t*ret = (type == V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> > +}\n> > +\n> > +void V4L2Camera::validMemoryType(bool *ret, uint32_t memory)\n> > +{\n> > +\t*ret = (memory == V4L2_MEMORY_MMAP);\n> > +}\n> > +\n> > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Allocating libcamera bufs\";\n> > +\t*ret = camera_->allocateBuffers();\n> \n> I fear the buffer rework would required a big rebase of this series :(\n\nOh no :(\n\n> > +\tif (*ret == -EACCES)\n> > +\t\t*ret = -EBUSY;\n> > +}\n> > +\n> > +/* \\todo implement allocDMABuffers */\n> > +void V4L2Camera::allocDMABuffers(int *ret, unsigned int count)\n> > +{\n> > +\t*ret = -ENOTTY;\n> > +}\n> > +\n> > +void V4L2Camera::freeBuffers()\n> > +{\n> > +\tcamera_->freeBuffers();\n> > +\tbufferCount_ = 0;\n> > +}\n> > +\n> > +void V4L2Camera::streamOn(int *ret)\n> > +{\n> > +\tif (isRunning_) {\n> > +\t\t*ret = 0;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tsequence_ = 0;\n> > +\n> > +\t*ret = camera_->start();\n> > +\tif (*ret < 0)\n> > +\t\treturn;\n> \n> errno ?\n\nYes.\n\nI'm going to shift things around so that V4L2Camera can return the\nerrnos directly, and ethen either V4L2CameraProxy or V4L2CompatManager\ncan set the errno - that should fix the errno being lost in the call\nstack too.\n\n> > +\tisRunning_ = true;\n> > +\n> > +\twhile (!pendingRequests_.empty()) {\n> > +\t\t*ret = camera_->queueRequest(pendingRequests_.front());\n> > +\t\tpendingRequests_.pop();\n> > +\t\tif (*ret < 0)\n> > +\t\t\treturn;\n> > +\t}\n> > +\n> > +\t*ret = 0;\n> > +}\n> > +\n> > +void V4L2Camera::streamOff(int *ret)\n> > +{\n> > +\t/* \\todo restore buffers to reqbufs state? */\n> > +\tif (!isRunning_) {\n> > +\t\t*ret = 0;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t*ret = camera_->stop();\n> > +\tif (*ret < 0)\n> > +\t\treturn;\n> > +\tisRunning_ = false;\n> > +\n> > +\t*ret = 0;\n> \n> Here and in streamOn you could set *ret = 0 at the beginning of the\n> function.\n\nI originally had to to be analogous to \"return 0\", but it's duplicated\nin if (!isRunning_) so I suppose setting it at the beginning is better.\n\n> > +}\n> > +\n> > +void V4L2Camera::qbuf(int *ret, struct v4l2_buffer *arg)\n> > +{\n> > +\tStream *stream = config_->at(0).stream();\n> > +\tstd::unique_ptr buffer = stream->createBuffer(arg->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> > +\tRequest *request = camera_->createRequest();\n> \n> paranoid: createRequest() can return nullptr\n\nOh no :o\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\treturn;\n> > +\t}\n> > +\n> > +\tif (!isRunning_) {\n> > +\t\tpendingRequests_.push(request);\n> > +\t} else {\n> > +\t\t*ret = camera_->queueRequest(request);\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> > +\targ->flags |= V4L2_BUF_FLAG_QUEUED;\n> > +\targ->flags |= V4L2_BUF_FLAG_MAPPED;\n> > +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> > +\t*ret = 0;\n> > +}\n> > +\n> > +int V4L2Camera::dqbuf(struct v4l2_buffer *arg)\n> > +{\n> > +\tif (nonblock_ && !buffer_sema_->tryAcquire())\n> > +\t\treturn -EAGAIN;\n> > +\telse\n> > +\t\tbuffer_sema_->acquire();\n> > +\n> > +\tbuffer_lock_.lock();\n> > +\tBuffer *buffer = completedBuffers_.front();\n> > +\tcompletedBuffers_.pop();\n> > +\tbuffer_lock_.unlock();\n> > +\n> > +\targ->bytesused = buffer->bytesused();\n> > +\targ->field = V4L2_FIELD_NONE;\n> > +\targ->timestamp.tv_sec = buffer->timestamp() / 1000000000;\n> > +\targ->timestamp.tv_usec = buffer->timestamp() / 1000;\n> > +\targ->sequence = sequence_++;\n> \n> Don't we have sequence in Buffer ?\n\nOops, yeah we do.\n\n> > +\n> > +\targ->flags &= ~V4L2_BUF_FLAG_QUEUED;\n> > +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> > +\n> > +\tStreamConfiguration &streamConfig = config_->at(0);\n> > +\tunsigned int sizeimage =\n> > +\t\tV4L2CompatManager::image_size(streamConfig.pixelFormat,\n> > +\t\t\t\t\t streamConfig.size.width,\n> > +\t\t\t\t\t streamConfig.size.height);\n> > +\targ->length = sizeimage;\n> \n> You can save the sizeimage variable.\n> I wonder if this needs to be re-calculated everytime... nothing big\n> however...\n\nOkay.\n\n> > +\n> > +\treturn 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..3d3cd8ff\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera.h\n> > @@ -0,0 +1,68 @@\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> > +#ifndef __V4L2_CAMERA_H__\n> > +#define __V4L2_CAMERA_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \"semaphore.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +class V4L2Camera : public Object\n> > +{\n> > +public:\n> > +\tV4L2Camera(std::shared_ptr camera);\n> > +\t~V4L2Camera();\n> > +\n> > +\tvoid open(int *ret, bool nonblock);\n> > +\tvoid close(int *ret);\n> > +\tvoid getStreamConfig(StreamConfiguration *ret);\n> > +\tvoid requestComplete(Request *request);\n> > +\n> > +\tvoid mmap(void **ret, void *addr, size_t length, int prot, int flags,\n> > +\t\t off_t offset);\n> > +\tvoid munmap(int *ret, void *addr, size_t length);\n> > +\n> > +\tvoid configure(int *ret, struct v4l2_format *arg,\n> > +\t\t unsigned int bufferCount);\n> > +\n> > +\tvoid validStreamType(bool *ret, uint32_t type);\n> > +\tvoid validMemoryType(bool *ret, uint32_t memory);\n> > +\n> > +\tvoid allocBuffers(int *ret, unsigned int count);\n> > +\tvoid allocDMABuffers(int *ret, unsigned int count);\n> > +\tvoid freeBuffers();\n> > +\tvoid streamOn(int *ret);\n> > +\tvoid streamOff(int *ret);\n> > +\n> > +\tvoid qbuf(int *ret, struct v4l2_buffer *arg);\n> > +\tint dqbuf(struct v4l2_buffer *arg);\n> > +\n> > +private:\n> > +\tvoid initDefaultV4L2Format();\n> > +\n> > +\tstd::shared_ptr camera_;\n> > +\tstd::unique_ptr config_;\n> > +\n> > +\tunsigned int bufferCount_;\n> > +\tbool isRunning_;\n> > +\tbool nonblock_;\n> > +\n> > +\tunsigned int sequence_;\n> > +\n> > +\tSemaphore *buffer_sema_;\n> > +\tstd::mutex buffer_lock_;\n> > +\n> > +\tstd::queue pendingRequests_;\n> > +\tstd::queue 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..1dd2c363\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -0,0 +1,438 @@\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> > +#include \"v4l2_camera_proxy.h\"\n> > +\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: index_(index), bufferCount_(0), currentBuf_(0),\n> > +\t vcam_(utils::make_unique(camera))\n> > +{\n> > +\tquerycap(camera);\n> > +}\n> > +\n> > +V4L2CameraProxy::~V4L2CameraProxy()\n> > +{\n> > +\tvcam_.reset();\n> \n> Not sure it is necessary to reset a unique_ptr<> at destruction time,\n> the managed pointer will be destroyed anyway.\n\nAh, right.\n\n> > +}\n> > +\n> > +int V4L2CameraProxy::open(bool nonblock)\n> > +{\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,\n> > +\t\t\t &ret, nonblock);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> > +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> > +\tsetFmtFromConfig();\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::close()\n> > +{\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);\n> > +\treturn ret;\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> > +\tvoid *val;\n> > +\tvcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,\n> > +\t\t\t &val, addr, length, prot, flags, offset);\n> > +\treturn val;\n> > +}\n> > +\n> > +int V4L2CameraProxy::munmap(void *addr, size_t length)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing MUNMAP\";\n> > +\n> > +\tint ret;\n> > +\tvcam_->invokeMethod(&V4L2Camera::munmap, ConnectionTypeBlocking,\n> > +\t\t\t &ret, addr, length);\n> > +\treturn ret;\n> > +}\n> > +\n> > +bool V4L2CameraProxy::hasPixelFormat(unsigned int format)\n> > +{\n> > +\tconst std::vector &formats =\n> > +\t\tstreamConfig_.formats().pixelformats();\n> > +\treturn std::find(formats.begin(), formats.end(), format) != formats.end();\n> > +}\n> > +\n> > +/* \\todo getDeviceCaps? getMemoryCaps? */\n> > +\n> > +bool V4L2CameraProxy::hasSize(unsigned int format, Size size)\n> > +{\n> > +\tint len = streamConfig_.formats().sizes(format).size();\n> > +\tfor (int i = 0; i < len; i++)\n> > +\t\tif (streamConfig_.formats().sizes(format)[i] == size)\n> > +\t\t\treturn true;\n> \n> Can't you find() on the vector returned by\n> streamConfig_.formats().sizes(format) ?\n\nYes.\n\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > +bool V4L2CameraProxy::validateStreamType(uint32_t type)\n> > +{\n> > +\tbool valid;\n> > +\tvcam_->invokeMethod(&V4L2Camera::validStreamType,\n> > +\t\t\t ConnectionTypeBlocking, &valid, type);\n> > +\tif (!valid)\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)\n> > +{\n> > +\tbool valid;\n> > +\tvcam_->invokeMethod(&V4L2Camera::validMemoryType,\n> > +\t\t\t ConnectionTypeBlocking, &valid, memory);\n> > +\tif (!valid)\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> \n> In this two functions you can here just return 'valid'\n\nWhy?\n\n> > +}\n> > +\n> > +void V4L2CameraProxy::setFmtFromConfig()\n> > +{\n> > +\tcurV4L2Format_.fmt.pix.width = streamConfig_.size.width;\n> > +\tcurV4L2Format_.fmt.pix.height = streamConfig_.size.height;\n> > +\tcurV4L2Format_.fmt.pix.pixelformat =\n> > +\t\tV4L2CompatManager::drm_to_v4l2(streamConfig_.pixelFormat);\n> > +\tcurV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;\n> > +\tcurV4L2Format_.fmt.pix.bytesperline =\n> > +\t\tV4L2CompatManager::bpl_multiplier(\n> > +\t\t\tcurV4L2Format_.fmt.pix.pixelformat) *\n> > +\t\tcurV4L2Format_.fmt.pix.width;\n> > +\tcurV4L2Format_.fmt.pix.sizeimage =\n> > +\t\tV4L2CompatManager::image_size(curV4L2Format_.fmt.pix.pixelformat,\n> > +\t\t\t\t\t curV4L2Format_.fmt.pix.width,\n> > +\t\t\t\t\t curV4L2Format_.fmt.pix.height);\n> > +\tcurV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\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> > +\tcapabilities_.version = KERNEL_VERSION(5, 2, 0);\n> > +\tcapabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;\n> \n> Are we single planar only ? I guess so, it's fine :)\n\nYeah :)\n\n> > +\tcapabilities_.capabilities =\n> > +\t\tcapabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;\n> > +\tmemset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));\n> > +}\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\treturn -EINVAL;\n> > +\tif (arg->index > streamConfig_.formats().pixelformats().size())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tmemcpy(arg->description, \"asdf\", 5);\n> \n> That's a real nice format name! :D\n> Do we need a map of formats to descriptions ?\n\nThanks! :D\nWe might.\n\n> > +\targ->pixelformat =\n> > +\t\tV4L2CompatManager::drm_to_v4l2(\n> > +\t\t\tstreamConfig_.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> > +\targ->fmt.pix.width = curV4L2Format_.fmt.pix.width;\n> > +\targ->fmt.pix.height = curV4L2Format_.fmt.pix.height;\n> > +\targ->fmt.pix.pixelformat = curV4L2Format_.fmt.pix.pixelformat;\n> > +\targ->fmt.pix.field = curV4L2Format_.fmt.pix.field;\n> > +\targ->fmt.pix.bytesperline = curV4L2Format_.fmt.pix.bytesperline;\n> > +\targ->fmt.pix.sizeimage = curV4L2Format_.fmt.pix.sizeimage;\n> > +\targ->fmt.pix.colorspace = curV4L2Format_.fmt.pix.colorspace;\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> > +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> > +\t\t\t &ret, arg, bufferCount_);\n> > +\tif (ret < 0)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> > +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> > +\tsetFmtFromConfig();\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> > +\tif (!hasPixelFormat(format))\n> > +\t\tformat = streamConfig_.formats().pixelformats()[0];\n> > +\n> > +\tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> > +\tif (!hasSize(format, size))\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 =\n> > +\t\tV4L2CompatManager::bpl_multiplier(\n> > +\t\t\tV4L2CompatManager::drm_to_v4l2(format)) *\n> > +\t\targ->fmt.pix.width;\n> > +\targ->fmt.pix.sizeimage =\n> > +\t\tV4L2CompatManager::image_size(\n> > +\t\t\tV4L2CompatManager::drm_to_v4l2(format),\n> > +\t\t\targ->fmt.pix.width, arg->fmt.pix.height);\n> > +\targ->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing VIDIOC_REQBUFS\";\n> > +\tif (!validateStreamType(arg->type))\n> > +\t\treturn -EINVAL;\n> > +\tif (!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\tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> > +\t\tint ret;\n> > +\t\tvcam_->invokeMethod(&V4L2Camera::streamOff,\n> > +\t\t\t\t ConnectionTypeBlocking, &ret);\n> > +\t\tvcam_->invokeMethod(&V4L2Camera::freeBuffers,\n> > +\t\t\t\t ConnectionTypeBlocking);\n> > +\t\tbufferCount_ = 0;\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint ret;\n> \n> as ret is used function-wise, I would its declaration to the beginning\n\nack\n\n> > +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> > +\t\t\t &ret, &curV4L2Format_, arg->count);\n> > +\tif (ret < 0)\n> > +\t\treturn -EINVAL;\n> > +\targ->count = streamConfig_.bufferCount;\n> > +\tbufferCount_ = arg->count;\n> > +\n> > +\tret = -EINVAL;\n> > +\tif (arg->memory == V4L2_MEMORY_MMAP)\n> > +\t\tvcam_->invokeMethod(&V4L2Camera::allocBuffers,\n> > +\t\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> > +\telse if (arg->memory == V4L2_MEMORY_DMABUF)\n> \n> Do we even claim support for this ?\n\nI was going to but then didn't... and removed it and didn't remove it in\nother places.\n\n> > +\t\tvcam_->invokeMethod(&V4L2Camera::allocDMABuffers,\n> > +\t\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> > +\n> > +\tif (ret < 0) {\n> > +\t\targ->count = 0;\n> > +\t\treturn ret == -EACCES ? -EBUSY : ret;\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\treturn -EINVAL;\n> > +\tif (arg->index >= stream->buffers().size())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tunsigned int index = arg->index;\n> > +\tmemset(arg, 0, sizeof(*arg));\n> > +\targ->index = index;\n> > +\targ->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> > +\targ->length = curV4L2Format_.fmt.pix.sizeimage;\n> > +\targ->memory = V4L2_MEMORY_MMAP;\n> > +\targ->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;\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\treturn -EINVAL;\n> > +\tif (!validateMemoryType(arg->memory))\n> > +\t\treturn -EINVAL;\n> > +\tif (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);\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\treturn -EINVAL;\n> > +\tif (!validateMemoryType(arg->memory))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\targ->index = currentBuf_;\n> > +\tcurrentBuf_ = (currentBuf_ + 1) % bufferCount_;\n> > +\n> > +\treturn vcam_->dqbuf(arg);\n> \n> Is dqbuf special ?\n\nYes.\n\n> I know it could return immediately if nonblock_ is set, but\n> invokeMethod checks that invoked method is called, if it returns\n> immediately, that's fine. Or have I missed some other reason why this\n> is called directly.\n\nHere we are waiting for the frames to be produced, which must be in a\ndifferent thread from where the frames are produced.\n\nOtherwise (speaking from experience) we either busy wait in V4L2Camera,\nor the frame can't become available because we're blocked waiting for it\nto become available.\n\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> > +\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> > +\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> \n> camelCase for method names as well ?\n\nI suppose... I liked that the vidiocs mirrored the ioctl identifiers\nthough...\nShould I do camelCase instead?\n\nvidiocQueryCap\nvidiocEnumFmt\nvidiocGFmt\nvidiocTryFmt\nvidiocReqBufs\nvidiocStreamOn\n\nWhat do you think?\n\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> > +\tcase VIDIOC_EXPBUF:\n> > +\tcase VIDIOC_ENUM_FRAMESIZES:\n> > +\tdefault:\n> > +\t\tret = -ENOTTY;\n> > +\t}\n> > +\n> > +\tif (ret < 0) {\n> > +\t\terrno = ret;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\terrno = 0;\n> > +\treturn ret;\n> > +\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..64c7aadd\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -0,0 +1,63 @@\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> > +#ifndef __V4L2_CAMERA_PROXY_H__\n> > +#define __V4L2_CAMERA_PROXY_H__\n> > +\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> > +\t~V4L2CameraProxy();\n> > +\n> > +\tint open(bool nonblock);\n> > +\tint 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> > +private:\n> > +\tbool hasPixelFormat(unsigned int format);\n> > +\tbool hasSize(unsigned int format, Size size);\n> > +\tbool validateStreamType(uint32_t type);\n> > +\tbool validateMemoryType(uint32_t memory);\n> > +\tvoid setFmtFromConfig();\n> > +\tvoid querycap(std::shared_ptr camera);\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 index_;\n> > +\n> > +\tstruct v4l2_format curV4L2Format_;\n> > +\tStreamConfiguration streamConfig_;\n> > +\tstruct v4l2_capability capabilities_;\n> > +\tunsigned int bufferCount_;\n> > +\tunsigned int currentBuf_;\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..3330e7bc\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_compat.cpp\n> > @@ -0,0 +1,81 @@\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> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#define LIBCAMERA_PUBLIC __attribute__((visibility(\"default\")))\n> \n> Am I wrong or this makes sense only if we compile with\n> -fvisiblity=hidden ?\n> https://gcc.gnu.org/wiki/Visibility\n\nIt might be. When I tried it in the beginning with the compile options\nthat we already had, the symbols weren't being exported.\n\n> I welcome this change, but then probably you should add that\n> compilation flag to the v4l2 compat library, it I have not\n> mis-interpreted the above wiki page\n\nOkay.\n\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> > +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 openat(AT_FDCWD, path, 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> > +\tvoid *val = V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);\n> > +\treturn val;\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..4eeb714f\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_compat_manager.cpp\n> > @@ -0,0 +1,353 @@\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> > +#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> > +#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: cameraManagerRunning_(false), cm_(nullptr)\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> You seem to be mixing cameraCase and snake_case here and there in\n> variable declaration. Was this intentional ?\n\nIt was intentional, until I found the few that were unintentional.\n\nThese ones, plus the vidioc ones are the only ones that I intended to be\nsnake_case and thought was borderline acceptable.\n\n> > +}\n> > +\n> > +V4L2CompatManager::~V4L2CompatManager()\n> > +{\n> > +\tdevices_.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);\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\n> > +\t * gets created here to wraps a camera device.\n> > +\t */\n> > +\t// \\todo map v4l2 devices to libcamera cameras; minor device node?\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\n> > +\t * as we're now ready to handle calls from the framework.\n> > +\t */\n> > +\tcv_.notify_one();\n> > +\n> > +\t/* Now start processing events and messages. */\n> > +\texec();\n> > +\n> > +\tcm_->stop();\n> > +\tdelete cm_;\n> > +\tcm_ = nullptr;\n> > +}\n> > +\n> > +V4L2CompatManager *V4L2CompatManager::instance()\n> > +{\n> > +\tstatic V4L2CompatManager v4l2CompatManager;\n> > +\treturn &v4l2CompatManager;\n> > +}\n> > +\n> > +bool V4L2CompatManager::validfd(int fd)\n> > +{\n> > +\treturn devices_.find(fd) != devices_.end();\n> > +}\n> > +\n> > +bool V4L2CompatManager::validmmap(void *addr)\n> > +{\n> > +\treturn mmaps_.find(addr) != mmaps_.end();\n> > +}\n> > +\n> > +std::shared_ptr V4L2CompatManager::getCamera(int fd)\n> > +{\n> > +\tif (!validfd(fd))\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn devices_.at(fd);\n> \n> This is safe, but it's a double lookup, same as below. This is minor\n> indeed, but probably using the iterator returned from find() directly\n> is slightly more efficient.\n\nHow can I use the iterator directly?\n\nWhen we intercept the calls and check if we should simply forward the\ncall, I think it's simpler to have just a validfd() call... but then\nwhen we do need to process it then it does become a double (triple?)\nlookup... or just check getCamera() == nullptr instead of validfd().\n\n> Then you can inline the valid[fd|mmap}() methods, probably\n> \n> > +}\n> > +\n> > +std::shared_ptr V4L2CompatManager::getCamera(void *addr)\n> > +{\n> > +\tif (!validmmap(addr))\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn devices_.at(mmaps_.at(addr));\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> > +\tif (std::string(path).find(\"video\") == std::string::npos)\n> > +\t\treturn fd;\n> > +\n> > +\tif (!isRunning())\n> > +\t\tinit();\n> > +\n> > +\t/* \\todo map v4l2 devnodes to libcamera cameras */\n> > +\tunsigned int camera_index = 0;\n> > +\n> > +\tstd::shared_ptr proxy = proxies_[camera_index];\n> > +\tint ret = proxy->open(mode & O_NONBLOCK);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tint efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));\n> > +\tif (efd < 0)\n> > +\t\treturn errno;\n> \n> I'm not sure I get this usage of the file descriptor returned by\n> eventfd...\n> \n> It is used as index in the map, possibly replaced by dup(). I would\n> have expected the fd returned by openat() to be used as index in the\n> map... What am I missing ?\n\nThe fd returned by eventfd reserves the fd so that any other open()\ncall doesn't overwrite/conflict with our fd. This fd is indeed used as\nan index in the map (see next line), and dup() doesn't replace it.\n\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> > +\tif (validfd(oldfd))\n> \n> Shouldn't you return an error if the fd to duplicate is not valid, ad\n> dup() only if it is ?\n\nIf the fd is not valid, then it means the fd is not for us, and we need\nto forward the dup() call. So in any case, we need to dup(). The only\ndifference is if we need to save the new fd in the mapping or not.\n\n> > +\t\tdevices_[newfd] = devices_[oldfd];\n> > +\n> > +\treturn newfd;\n> > +}\n> > +\n> > +int V4L2CompatManager::close(int fd)\n> > +{\n> > +\tif (validfd(fd) && devices_[fd]->close() < 0)\n> \n> I would return -EIO if !validfd() and propagate the error up if\n> close() fails.\n\nIf !validfd() then the fd isn't for us, and the call needs to be\nforwarded. -EIO is only if *our* close() fails.\n\n> > +\t\treturn -EIO;\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> > +\tif (!validfd(fd))\n> > +\t\treturn mmap_func_(addr, length, prot, flags, fd, offset);\n> > +\n> > +\tvoid *map = getCamera(fd)->mmap(addr, length, prot, flags, offset);\n> > +\tif (map == MAP_FAILED) {\n> > +\t\terrno = EINVAL;\n> > +\t\treturn map;\n> > +\t}\n> > +\n> > +\tmmaps_[addr] = fd;\n> > +\treturn map;\n> > +}\n> > +\n> > +int V4L2CompatManager::munmap(void *addr, size_t length)\n> > +{\n> > +\tif (!validmmap(addr))\n> > +\t\treturn munmap_func_(addr, length);\n> > +\n> > +\tint ret = getCamera(addr)->munmap(addr, length);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tmmaps_.erase(addr);\n> > +\taddr = nullptr;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)\n> > +{\n> > +\tstd::shared_ptr proxy = getCamera(fd);\n> > +\tif (!proxy)\n> > +\t\treturn ioctl_func_(fd, request, arg);\n> > +\n> \n> What is the use case for bypassing the proxy ? That might be my\n> limited knowledge of how v4l2 application behave\n\nPerhaps. Here I check if proxy == nullptr, but in other places I checked\n!validfd(fd) (I'll make these consistent). If the fd is not valid = the\nproxy doesn't exist for the fd, then the fd is not for us and we need to\nforward it to the original call.\n\n> > +\treturn proxy->ioctl(request, arg);\n> > +}\n> > +\n> > +/* \\todo make libcamera export these */\n> \n> mmm, V4L2VideoDevice has very similar format conversion routines... we\n> should really centralize them somehow.. maybe not in scope for this\n> patch, but replicating cose which is tricky to get right due to the\n> different format definition between DRM and V4L2 is not a good idea.\n\nI know...\n\n> > +int V4L2CompatManager::bpl_multiplier(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 V4L2CompatManager::image_size(unsigned int format,\n> > +\t\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 V4L2CompatManager::v4l2_to_drm(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 V4L2CompatManager::drm_to_v4l2(unsigned int pixelformat)\n> > +{\n> > +\tswitch (pixelformat) {\n> > +\t/* RGB formats. */\n> > +\tcase DRM_FORMAT_BGR888:\n> > +\t\treturn V4L2_PIX_FMT_BGR24;\n> \n> This in example does not match the one we have in V4L2Videodevice.\n> DRM_FORMAT_BGR24 = V4L2_PIX_FMT_RGB24 not BGR24.\n\nOh wat :/\nGreat.\n\n> I know, it's a pain.\n> \n> I think the other version is correct, as we validated them using\n> conversion routines from kernel drivers...\n> \n> There might be more below (and possibly above)\n> \n> > +\tcase DRM_FORMAT_RGB888:\n> > +\t\treturn V4L2_PIX_FMT_RGB24;\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_compat_manager.h b/src/v4l2/v4l2_compat_manager.h\n> > new file mode 100644\n> > index 00000000..492f97fd\n> > --- /dev/null\n> > +++ b/src/v4l2/v4l2_compat_manager.h\n> > @@ -0,0 +1,91 @@\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> > +#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> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \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> > +\tint openat(int dirfd, const char *path, int oflag, mode_t mode);\n> > +\n> > +\tstd::shared_ptr getCamera(int fd);\n> > +\tstd::shared_ptr getCamera(void *addr);\n> \n> Every time you return a shared_ptr by value, you increase the\n> reference count for no good reason.\n\nBut it goes back down after the caller finishes right? :p\n\n> I would either return by reference\n> or return V4L2CameraProxy and use shared pointers only in the proxies_\n> array and not in the rest of the code base.\n\nOkay.\n\n> Overall this is a very good first submission and I'm happy to see it\n> posted to the list \\o/\n> \n> I admit in this first pass I didn't go into extreme detail on the way\n> the v4l2 operations semantic is handled, but it seems sane to me!\n\nThank you!\n\n\nPaul\n\n> \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> > +\tstatic int bpl_multiplier(unsigned int format);\n> > +\tstatic int image_size(unsigned int format, unsigned int width,\n> > +\t\t\t unsigned int height);\n> > +\n> > +\tstatic unsigned int v4l2_to_drm(unsigned int pixelformat);\n> > +\tstatic unsigned int drm_to_v4l2(unsigned int pixelformat);\n> > +\n> > +private:\n> > +\tV4L2CompatManager();\n> > +\t~V4L2CompatManager();\n> > +\n> > +\tvoid run() override;\n> > +\n> > +\tbool validfd(int fd);\n> > +\tbool validmmap(void *addr);\n> > +\n> > +\tint default_ioctl(int fd, unsigned long request, void *arg);\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> > +\tbool cameraManagerRunning_;\n> > +\tCameraManager *cm_;\n> > +\n> > +\tstd::mutex mutex_;\n> > +\tstd::condition_variable cv_;\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 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 BC7B860BD1\n\tfor ;\n\tMon, 9 Dec 2019 02:10:36 +0100 (CET)","from localhost.localdomain (unknown [96.44.9.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BABC552B;\n\tMon, 9 Dec 2019 02:10:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575853836;\n\tbh=91xqjn03vnUy6M5AO7FodkdllBz0jB0e5bVGYNHd5/c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j9yTziRQ3mTFv+aVZ5KBg2GRk3LYfgjQRk8Y9eu/C7e/+5yojhhaffVVcgvLoc+5Z\n\t4T/GEa1qOoH/4JfRjjxKdzQzsywhwR36oOJvvLigIsfiIbJ2t1CuIQkJnVDf3HWmhx\n\t3hVketumfTLaDa5o2LcigVJifNPlLZeMrFjKWBb4=","Date":"Sun, 8 Dec 2019 20:10:31 -0500","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191209011031.GA12803@localhost.localdomain>","References":"<20191206092654.24340-1-paul.elder@ideasonboard.com>\n\t<20191206092654.24340-2-paul.elder@ideasonboard.com>\n\t<20191206161203.gpbtfub44oarpzic@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20191206161203.gpbtfub44oarpzic@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Mon, 09 Dec 2019 01:10:37 -0000"}},{"id":3215,"web_url":"https://patchwork.libcamera.org/comment/3215/","msgid":"<20191209110116.GA4853@pendragon.ideasonboard.com>","date":"2019-12-09T11:01:16","subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Hello,\n\nOn Sun, Dec 08, 2019 at 08:10:31PM -0500, Paul Elder wrote:\n> On Fri, Dec 06, 2019 at 05:12:03PM +0100, Jacopo Mondi wrote:\n> > On Fri, Dec 06, 2019 at 04:26:54AM -0500, Paul Elder wrote:\n> > > Add libcamera V4L2 compatibility layer.\n> > \n> > Maybe just me, but having this patch split in components would have\n> > helped not having to digest 1445 lines of patch in one go\n> \n> Laurent said it was okay... :)\n\nThe issue with such large patches is that they are difficult to\nmeaningfully split. Sure, it could have been split in one patch per\nfile, but they wouldn't contain changes that can be reviewed in\nisolation, so a big drop is often the only meaningful option.\n\n> > I know Niklas agrees :)\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> > > Note that until we have a way of mapping V4L2 device nodes to libcamera\n> > > cameras, the V4L2 compatibility layer will always selects and use the\n> > > first enumerated libcamera camera.\n> > \n> > That's not trivial indeed...\n> > \n> > For simple devices, we could easily collect the video nodes a camera\n> > uses and match them with the one the v4l2 application tries to use.\n> > \n> > For complex devices, where the DMA output node could be multiplexed by\n> > different cameras depending on the ISP configuration, this is not\n> > easy.\n> > \n> > I presume v4l2 application are mostly meant to be used with simple\n> > devices, so adding a list of V4L2Videodevices to the Camera (while a\n> > bit of an hack) is totally doable. What do others think ?\n> \n> I'm thinking that it's not a good idea to expose anything V4L2 from\n> Camera. If it's just UVC devices, might we match instead by the name\n> exposed by sysfs?\n\nQuite the contrary, we want the V4L2 adaptation layer to cover *all*\ncameras, not just UVC. \n\nI agree we don't want to expose V4L2 information in the public API, but\nat the same time we need to expose enough information to make the match\npossible for the V4L2 adaptation layer.\n\nOne option would be to add a method to the camera manager to retrieve a\ncamera by device node path. This wouldn't expose any V4L2-specific\ninformation towards applications, the map of device nodes to cameras\nwould be private.\n\n> The uvcvideo pipeline handler names the Camera based on that name\n> anyway.\n\nAnd that's something we have to fix as it leads to cameras with\nidentical names today.\n\n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > meson_options.txt | 5 +\n> > > src/meson.build | 4 +\n> > > src/v4l2/meson.build | 30 +++\n> > > src/v4l2/v4l2_camera.cpp | 312 ++++++++++++++++++++++\n> > > src/v4l2/v4l2_camera.h | 68 +++++\n> > > src/v4l2/v4l2_camera_proxy.cpp | 438 +++++++++++++++++++++++++++++++\n> > > src/v4l2/v4l2_camera_proxy.h | 63 +++++\n> > > src/v4l2/v4l2_compat.cpp | 81 ++++++\n> > > src/v4l2/v4l2_compat_manager.cpp | 353 +++++++++++++++++++++++++\n> > > src/v4l2/v4l2_compat_manager.h | 91 +++++++\n> > > 10 files changed, 1445 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..b06dd494 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 libcamera with 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..d96db4ff\n> > > --- /dev/null\n> > > +++ b/src/v4l2/meson.build\n> > > @@ -0,0 +1,30 @@\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> > \n> > Seems it is enough to undefine the _FILE_OFFSET_BITS macro to use the\n> > 32-bit interface ?\n> \n> Looks like it is.\n\nDefining it explicitly to =32 allows catching conflicting redefinitions.\nIf we just undefine _FILE_OFFSET_BITS, we may then include a header that\nwould define it to =64 without being aware of that, and run into weird\nissues.\n\n> > https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html\n> > If _FILE_OFFSET_BITS is undefined, or if it is defined to the value\n> > 32, nothing changes. The 32 bit interface is used and types like off_t\n> > have a size of 32 bits on 32 bit systems.\n> > \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..c6c84ef3\n> > > --- /dev/null\n> > > +++ b/src/v4l2/v4l2_camera.cpp\n> > > @@ -0,0 +1,312 @@\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> > > +#include \"v4l2_camera.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +#include \"v4l2_compat_manager.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DECLARE_CATEGORY(V4L2Compat);\n> > > +\n> > > +V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> > > +\t: camera_(camera), bufferCount_(0), isRunning_(false), sequence_(0),\n> > > +\t buffer_sema_(new Semaphore())\n> > \n> > Can't this be a class member instead of a pointer ?\n> > Also, I would name it differently, or at least make it cameraCase_\n> \n> ack\n> \n> > > +{\n> > > +\tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> > > +};\n> > > +\n> > > +V4L2Camera::~V4L2Camera()\n> > > +{\n> > > +\twhile (!pendingRequests_.empty()) {\n> > > +\t\tdelete pendingRequests_.front();\n> > > +\t\tpendingRequests_.pop();\n> > > +\t}\n> > > +\n> > > +\tbuffer_lock_.lock();\n> > > +\twhile (!completedBuffers_.empty()) {\n> > > +\t\tdelete completedBuffers_.front();\n> > > +\t\tcompletedBuffers_.pop();\n> > > +\t}\n> > > +\tbuffer_lock_.unlock();\n> > > +\n> > > +\tdelete buffer_sema_;\n> > \n> > you would save deleting it\n> > \n> > > +\n> > > +\tcamera_->release();\n> > > +}\n> > > +\n> > > +void V4L2Camera::open(int *ret, bool nonblock)\n> > > +{\n> > > +\tnonblock_ = nonblock;\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_ == nullptr) {\n> > > +\t\t*ret = -EINVAL;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\t*ret = 0;\n> > > +}\n> > > +\n> > > +void V4L2Camera::close(int *ret)\n> > > +{\n> > > +\t*ret = camera_->release();\n> > > +}\n> > > +\n> > > +void V4L2Camera::getStreamConfig(StreamConfiguration *ret)\n> > \n> > Maybe *streaConfig ?\n> \n> ack\n> \n> > > +{\n> > > +\t*ret = config_->at(0);\n> > > +}\n> > > +\n> > > +void V4L2Camera::requestComplete(Request *request)\n> > > +{\n> > > +\tif (request->status() == Request::RequestCancelled) {\n> > > +\t\tLOG(V4L2Compat, Error) << \"Request not succesfully completed: \"\n> > > +\t\t\t\t << request->status();\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\t/* We only have one stream at the moment. */\n> > > +\tbuffer_lock_.lock();\n> > > +\tBuffer *buffer = request->buffers().begin()->second;\n> > > +\tcompletedBuffers_.push(buffer);\n> > > +\tbuffer_lock_.unlock();\n> > > +\n> > > +\tbuffer_sema_->release();\n> > > +}\n> > > +\n> > > +void V4L2Camera::configure(int *ret, struct v4l2_format *arg,\n> > > +\t\t\t unsigned int bufferCount)\n> > > +{\n> > > +\tconfig_->at(0).size.width = arg->fmt.pix.width;\n> > \n> > Nit: can you use a reference to config_->at(0) ?\n> \n> ack\n> \n> > > +\tconfig_->at(0).size.height = arg->fmt.pix.height;\n> > > +\tconfig_->at(0).pixelFormat =\n> > > +\t\tV4L2CompatManager::v4l2_to_drm(arg->fmt.pix.pixelformat);\n> > > +\tbufferCount_ = bufferCount;\n> > > +\tconfig_->at(0).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, Info) << \"Configuration invalid\";\n> > \n> > that's probably an error, isn't it ?\n> \n> Yep.\n> \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 << config_->at(0).toString();\n> > > +\n> > > +\t*ret = camera_->configure(config_.get());\n> > > +\tif (*ret < 0)\n> > > +\t\treturn;\n> > > +\n> > > +\tbufferCount_ = config_->at(0).bufferCount;\n> > > +\n> > > +\t*ret = 0;\n> > > +}\n> > > +\n> > > +void V4L2Camera::mmap(void **ret, void *addr, size_t length, int prot,\n> > > +\t\t int flags, off_t offset)\n> > \n> > Do we need to check flags ?\n> \n> No.\n> \n> > > +{\n> > > +\tLOG(V4L2Compat, Debug) << \"Servicing MMAP\";\n> > > +\n> > > +\tif (prot != (PROT_READ | PROT_WRITE)) {\n> > > +\t\terrno = EINVAL;\n> > \n> > This function (actually all V4L2Camera functions) is called through a method\n> > invocation and has a long call stack before getting back to the\n> > caller. I wonder if errno does not get overwritten along that route.\n> \n> You're right; it doesn't seem to be kept. The caller still receives an\n> EINVAL though (but only EINVAL).\n> \n> I looked back at this and I think I messed up some errno passing.\n> \n> > Also, from man mmap:\n> > \n> > ENOTSUP\n> > MAP_FIXED or MAP_PRIVATE was specified in the flags argument and the\n> > implementation does not support this functionality.\n> >\n> > The implementation does not support the combination of accesses\n> > requested in the prot argument.\n> \n> I don't see this in my man mmap... oh wait I found it in the posix man.\n> \n> Yeah, this is probably better.\n> \n> > > +\t\t*ret = MAP_FAILED;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tStreamConfiguration &streamConfig = config_->at(0);\n> > > +\tunsigned int sizeimage =\n> > > +\t\tV4L2CompatManager::image_size(\n> > > +\t\t\tV4L2CompatManager::drm_to_v4l2(streamConfig.pixelFormat),\n> > > +\t\t\tstreamConfig.size.width,\n> > > +\t\t\tstreamConfig.size.height);\n> > > +\tif (sizeimage == 0) {\n> > > +\t\terrno = EINVAL;\n> > > +\t\t*ret = MAP_FAILED;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tunsigned int index = offset / sizeimage;\n> > > +\tif (index * sizeimage != offset || length != sizeimage) {\n> > > +\t\terrno = EINVAL;\n> > \n> > Is EINVAL the right error here?\n> > from man mmap:\n> > \n> > ENOMEM\n> > MAP_FIXED was specified, and the range [addr,addr+len)\n> > exceeds that allowed for the address space of a process; or, if\n> > MAP_FIXED was not specified and there is insufficient room in the\n> > address space to effect the mapping\n> \n> I don't think this is right, as ENOMEM indicates some form of\n> \"out of memory\", but we are checking for alignment.\n\nThe V4L2 adaptation layer should emulate V4L2, so the authoritative\nsource of information regarding what error codes to return is the kernel\nimplementation. I'll let you double-check the different error codes\nreturned through the adaptation layer :-)\n\n> > EOVERFLOW\n> > The file is a regular file and the value of off plus len exceeds the\n> > offset maximum established in the open file description associated\n> > with fildes.\n> \n> Similar issue here.\n> \n> > However I'm not sure we should care about the mmap errors at this\n> > level, as those should be returned when the actual mapping is\n> > performed. What do you think ?\n> \n> I think we should already return the errors at this point, especially\n> since these are all input validation for the mmap for V4L2 devices.\n\nI agree, we need to validate all parameters the same way V4L2 would in\nthe kernel. How would we decide what index to pass to buffer()[] below\notherwise ?\n\n> > > +\t\t*ret = MAP_FAILED;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tStream *stream = *camera_->streams().begin();\n> > > +\t*ret = stream->buffers()[index].planes()[0].mem();\n> > > +}\n> > > +\n> > > +void V4L2Camera::munmap(int *ret, void *addr, size_t length)\n> > > +{\n> > > +\tStreamConfiguration &streamConfig = config_->at(0);\n> > > +\tunsigned int sizeimage =\n> > > +\t\tV4L2CompatManager::image_size(streamConfig.pixelFormat,\n> > > +\t\t\t\t\t streamConfig.size.width,\n> > > +\t\t\t\t\t streamConfig.size.height);\n> > > +\n> > > +\tif (length != sizeimage) {\n> > > +\t\terrno = EINVAL;\n> > \n> > Here EINVAL seems to be appropriate\n> > \n> > > +\t\t*ret = -1;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\t*ret = 0;\n> > > +}\n> > > +\n> > > +void V4L2Camera::validStreamType(bool *ret, uint32_t type)\n> > > +{\n> > > +\t*ret = (type == V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> > > +}\n> > > +\n> > > +void V4L2Camera::validMemoryType(bool *ret, uint32_t memory)\n> > > +{\n> > > +\t*ret = (memory == V4L2_MEMORY_MMAP);\n> > > +}\n> > > +\n> > > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)\n> > > +{\n> > > +\tLOG(V4L2Compat, Debug) << \"Allocating libcamera bufs\";\n> > > +\t*ret = camera_->allocateBuffers();\n> > \n> > I fear the buffer rework would required a big rebase of this series :(\n> \n> Oh no :(\n> \n> > > +\tif (*ret == -EACCES)\n> > > +\t\t*ret = -EBUSY;\n> > > +}\n> > > +\n> > > +/* \\todo implement allocDMABuffers */\n> > > +void V4L2Camera::allocDMABuffers(int *ret, unsigned int count)\n> > > +{\n> > > +\t*ret = -ENOTTY;\n> > > +}\n> > > +\n> > > +void V4L2Camera::freeBuffers()\n> > > +{\n> > > +\tcamera_->freeBuffers();\n> > > +\tbufferCount_ = 0;\n> > > +}\n> > > +\n> > > +void V4L2Camera::streamOn(int *ret)\n> > > +{\n> > > +\tif (isRunning_) {\n> > > +\t\t*ret = 0;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tsequence_ = 0;\n> > > +\n> > > +\t*ret = camera_->start();\n> > > +\tif (*ret < 0)\n> > > +\t\treturn;\n> > \n> > errno ?\n> \n> Yes.\n> \n> I'm going to shift things around so that V4L2Camera can return the\n> errnos directly, and ethen either V4L2CameraProxy or V4L2CompatManager\n> can set the errno - that should fix the errno being lost in the call\n> stack too.\n\nI think that's a good idea, the global errno variable should be set at\nthe highest level of the call stack.\n\n> > > +\tisRunning_ = true;\n> > > +\n> > > +\twhile (!pendingRequests_.empty()) {\n> > > +\t\t*ret = camera_->queueRequest(pendingRequests_.front());\n> > > +\t\tpendingRequests_.pop();\n> > > +\t\tif (*ret < 0)\n> > > +\t\t\treturn;\n> > > +\t}\n> > > +\n> > > +\t*ret = 0;\n> > > +}\n> > > +\n> > > +void V4L2Camera::streamOff(int *ret)\n> > > +{\n> > > +\t/* \\todo restore buffers to reqbufs state? */\n> > > +\tif (!isRunning_) {\n> > > +\t\t*ret = 0;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\t*ret = camera_->stop();\n> > > +\tif (*ret < 0)\n> > > +\t\treturn;\n> > > +\tisRunning_ = false;\n> > > +\n> > > +\t*ret = 0;\n> > \n> > Here and in streamOn you could set *ret = 0 at the beginning of the\n> > function.\n> \n> I originally had to to be analogous to \"return 0\", but it's duplicated\n> in if (!isRunning_) so I suppose setting it at the beginning is better.\n\nIf someone is bored, figuring out how to return a value from a bound\nmethod would be nice. I however fear we may be blocked by the fact that\nC++ doesn't consider the return type for overload resolution.\n\n> > > +}\n> > > +\n> > > +void V4L2Camera::qbuf(int *ret, struct v4l2_buffer *arg)\n> > > +{\n> > > +\tStream *stream = config_->at(0).stream();\n> > > +\tstd::unique_ptr buffer = stream->createBuffer(arg->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> > > +\tRequest *request = camera_->createRequest();\n> > \n> > paranoid: createRequest() can return nullptr\n> \n> Oh no :o\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\treturn;\n> > > +\t}\n> > > +\n> > > +\tif (!isRunning_) {\n> > > +\t\tpendingRequests_.push(request);\n> > > +\t} else {\n> > > +\t\t*ret = camera_->queueRequest(request);\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> > > +\targ->flags |= V4L2_BUF_FLAG_QUEUED;\n> > > +\targ->flags |= V4L2_BUF_FLAG_MAPPED;\n> > > +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> > > +\t*ret = 0;\n> > > +}\n> > > +\n> > > +int V4L2Camera::dqbuf(struct v4l2_buffer *arg)\n> > > +{\n> > > +\tif (nonblock_ && !buffer_sema_->tryAcquire())\n> > > +\t\treturn -EAGAIN;\n> > > +\telse\n> > > +\t\tbuffer_sema_->acquire();\n> > > +\n> > > +\tbuffer_lock_.lock();\n> > > +\tBuffer *buffer = completedBuffers_.front();\n> > > +\tcompletedBuffers_.pop();\n> > > +\tbuffer_lock_.unlock();\n> > > +\n> > > +\targ->bytesused = buffer->bytesused();\n> > > +\targ->field = V4L2_FIELD_NONE;\n> > > +\targ->timestamp.tv_sec = buffer->timestamp() / 1000000000;\n> > > +\targ->timestamp.tv_usec = buffer->timestamp() / 1000;\n> > > +\targ->sequence = sequence_++;\n> > \n> > Don't we have sequence in Buffer ?\n> \n> Oops, yeah we do.\n> \n> > > +\n> > > +\targ->flags &= ~V4L2_BUF_FLAG_QUEUED;\n> > > +\targ->flags &= ~V4L2_BUF_FLAG_DONE;\n> > > +\n> > > +\tStreamConfiguration &streamConfig = config_->at(0);\n> > > +\tunsigned int sizeimage =\n> > > +\t\tV4L2CompatManager::image_size(streamConfig.pixelFormat,\n> > > +\t\t\t\t\t streamConfig.size.width,\n> > > +\t\t\t\t\t streamConfig.size.height);\n> > > +\targ->length = sizeimage;\n> > \n> > You can save the sizeimage variable.\n> > I wonder if this needs to be re-calculated everytime... nothing big\n> > however...\n> \n> Okay.\n> \n> > > +\n> > > +\treturn 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..3d3cd8ff\n> > > --- /dev/null\n> > > +++ b/src/v4l2/v4l2_camera.h\n> > > @@ -0,0 +1,68 @@\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> > > +#ifndef __V4L2_CAMERA_H__\n> > > +#define __V4L2_CAMERA_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \"semaphore.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +class V4L2Camera : public Object\n> > > +{\n> > > +public:\n> > > +\tV4L2Camera(std::shared_ptr camera);\n> > > +\t~V4L2Camera();\n> > > +\n> > > +\tvoid open(int *ret, bool nonblock);\n> > > +\tvoid close(int *ret);\n> > > +\tvoid getStreamConfig(StreamConfiguration *ret);\n> > > +\tvoid requestComplete(Request *request);\n> > > +\n> > > +\tvoid mmap(void **ret, void *addr, size_t length, int prot, int flags,\n> > > +\t\t off_t offset);\n> > > +\tvoid munmap(int *ret, void *addr, size_t length);\n> > > +\n> > > +\tvoid configure(int *ret, struct v4l2_format *arg,\n> > > +\t\t unsigned int bufferCount);\n> > > +\n> > > +\tvoid validStreamType(bool *ret, uint32_t type);\n> > > +\tvoid validMemoryType(bool *ret, uint32_t memory);\n> > > +\n> > > +\tvoid allocBuffers(int *ret, unsigned int count);\n> > > +\tvoid allocDMABuffers(int *ret, unsigned int count);\n> > > +\tvoid freeBuffers();\n> > > +\tvoid streamOn(int *ret);\n> > > +\tvoid streamOff(int *ret);\n> > > +\n> > > +\tvoid qbuf(int *ret, struct v4l2_buffer *arg);\n> > > +\tint dqbuf(struct v4l2_buffer *arg);\n> > > +\n> > > +private:\n> > > +\tvoid initDefaultV4L2Format();\n> > > +\n> > > +\tstd::shared_ptr camera_;\n> > > +\tstd::unique_ptr config_;\n> > > +\n> > > +\tunsigned int bufferCount_;\n> > > +\tbool isRunning_;\n> > > +\tbool nonblock_;\n> > > +\n> > > +\tunsigned int sequence_;\n> > > +\n> > > +\tSemaphore *buffer_sema_;\n> > > +\tstd::mutex buffer_lock_;\n> > > +\n> > > +\tstd::queue pendingRequests_;\n> > > +\tstd::queue 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..1dd2c363\n> > > --- /dev/null\n> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > > @@ -0,0 +1,438 @@\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> > > +#include \"v4l2_camera_proxy.h\"\n> > > +\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: index_(index), bufferCount_(0), currentBuf_(0),\n> > > +\t vcam_(utils::make_unique(camera))\n> > > +{\n> > > +\tquerycap(camera);\n> > > +}\n> > > +\n> > > +V4L2CameraProxy::~V4L2CameraProxy()\n> > > +{\n> > > +\tvcam_.reset();\n> > \n> > Not sure it is necessary to reset a unique_ptr<> at destruction time,\n> > the managed pointer will be destroyed anyway.\n> \n> Ah, right.\n> \n> > > +}\n> > > +\n> > > +int V4L2CameraProxy::open(bool nonblock)\n> > > +{\n> > > +\tint ret;\n> > > +\tvcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,\n> > > +\t\t\t &ret, nonblock);\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> > > +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> > > +\tsetFmtFromConfig();\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int V4L2CameraProxy::close()\n> > > +{\n> > > +\tint ret;\n> > > +\tvcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);\n> > > +\treturn ret;\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> > > +\tvoid *val;\n> > > +\tvcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,\n> > > +\t\t\t &val, addr, length, prot, flags, offset);\n> > > +\treturn val;\n> > > +}\n> > > +\n> > > +int V4L2CameraProxy::munmap(void *addr, size_t length)\n> > > +{\n> > > +\tLOG(V4L2Compat, Debug) << \"Servicing MUNMAP\";\n> > > +\n> > > +\tint ret;\n> > > +\tvcam_->invokeMethod(&V4L2Camera::munmap, ConnectionTypeBlocking,\n> > > +\t\t\t &ret, addr, length);\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +bool V4L2CameraProxy::hasPixelFormat(unsigned int format)\n> > > +{\n> > > +\tconst std::vector &formats =\n> > > +\t\tstreamConfig_.formats().pixelformats();\n> > > +\treturn std::find(formats.begin(), formats.end(), format) != formats.end();\n> > > +}\n> > > +\n> > > +/* \\todo getDeviceCaps? getMemoryCaps? */\n> > > +\n> > > +bool V4L2CameraProxy::hasSize(unsigned int format, Size size)\n> > > +{\n> > > +\tint len = streamConfig_.formats().sizes(format).size();\n> > > +\tfor (int i = 0; i < len; i++)\n> > > +\t\tif (streamConfig_.formats().sizes(format)[i] == size)\n> > > +\t\t\treturn true;\n> > \n> > Can't you find() on the vector returned by\n> > streamConfig_.formats().sizes(format) ?\n> \n> Yes.\n> \n> > > +\n> > > +\treturn false;\n> > > +}\n> > > +\n> > > +bool V4L2CameraProxy::validateStreamType(uint32_t type)\n> > > +{\n> > > +\tbool valid;\n> > > +\tvcam_->invokeMethod(&V4L2Camera::validStreamType,\n> > > +\t\t\t ConnectionTypeBlocking, &valid, type);\n> > > +\tif (!valid)\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)\n> > > +{\n> > > +\tbool valid;\n> > > +\tvcam_->invokeMethod(&V4L2Camera::validMemoryType,\n> > > +\t\t\t ConnectionTypeBlocking, &valid, memory);\n> > > +\tif (!valid)\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> > \n> > In this two functions you can here just return 'valid'\n> \n> Why?\n> \n> > > +}\n> > > +\n> > > +void V4L2CameraProxy::setFmtFromConfig()\n> > > +{\n> > > +\tcurV4L2Format_.fmt.pix.width = streamConfig_.size.width;\n> > > +\tcurV4L2Format_.fmt.pix.height = streamConfig_.size.height;\n> > > +\tcurV4L2Format_.fmt.pix.pixelformat =\n> > > +\t\tV4L2CompatManager::drm_to_v4l2(streamConfig_.pixelFormat);\n> > > +\tcurV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;\n> > > +\tcurV4L2Format_.fmt.pix.bytesperline =\n> > > +\t\tV4L2CompatManager::bpl_multiplier(\n> > > +\t\t\tcurV4L2Format_.fmt.pix.pixelformat) *\n> > > +\t\tcurV4L2Format_.fmt.pix.width;\n> > > +\tcurV4L2Format_.fmt.pix.sizeimage =\n> > > +\t\tV4L2CompatManager::image_size(curV4L2Format_.fmt.pix.pixelformat,\n> > > +\t\t\t\t\t curV4L2Format_.fmt.pix.width,\n> > > +\t\t\t\t\t curV4L2Format_.fmt.pix.height);\n> > > +\tcurV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\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> > > +\tcapabilities_.version = KERNEL_VERSION(5, 2, 0);\n> > > +\tcapabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;\n> > \n> > Are we single planar only ? I guess so, it's fine :)\n> \n> Yeah :)\n> \n> > > +\tcapabilities_.capabilities =\n> > > +\t\tcapabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;\n> > > +\tmemset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));\n> > > +}\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\treturn -EINVAL;\n> > > +\tif (arg->index > streamConfig_.formats().pixelformats().size())\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tmemcpy(arg->description, \"asdf\", 5);\n> > \n> > That's a real nice format name! :D\n> > Do we need a map of formats to descriptions ?\n> \n> Thanks! :D\n> We might.\n\nI'm afraid we do. For now you can replace \"asdf\" with \"\"\nand add a \\todo item.\n\n> > > +\targ->pixelformat =\n> > > +\t\tV4L2CompatManager::drm_to_v4l2(\n> > > +\t\t\tstreamConfig_.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> > > +\targ->fmt.pix.width = curV4L2Format_.fmt.pix.width;\n> > > +\targ->fmt.pix.height = curV4L2Format_.fmt.pix.height;\n> > > +\targ->fmt.pix.pixelformat = curV4L2Format_.fmt.pix.pixelformat;\n> > > +\targ->fmt.pix.field = curV4L2Format_.fmt.pix.field;\n> > > +\targ->fmt.pix.bytesperline = curV4L2Format_.fmt.pix.bytesperline;\n> > > +\targ->fmt.pix.sizeimage = curV4L2Format_.fmt.pix.sizeimage;\n> > > +\targ->fmt.pix.colorspace = curV4L2Format_.fmt.pix.colorspace;\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> > > +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> > > +\t\t\t &ret, arg, bufferCount_);\n> > > +\tif (ret < 0)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tvcam_->invokeMethod(&V4L2Camera::getStreamConfig,\n> > > +\t\t\t ConnectionTypeBlocking, &streamConfig_);\n> > > +\tsetFmtFromConfig();\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> > > +\tif (!hasPixelFormat(format))\n> > > +\t\tformat = streamConfig_.formats().pixelformats()[0];\n> > > +\n> > > +\tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> > > +\tif (!hasSize(format, size))\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 =\n> > > +\t\tV4L2CompatManager::bpl_multiplier(\n> > > +\t\t\tV4L2CompatManager::drm_to_v4l2(format)) *\n> > > +\t\targ->fmt.pix.width;\n> > > +\targ->fmt.pix.sizeimage =\n> > > +\t\tV4L2CompatManager::image_size(\n> > > +\t\t\tV4L2CompatManager::drm_to_v4l2(format),\n> > > +\t\t\targ->fmt.pix.width, arg->fmt.pix.height);\n> > > +\targ->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n> > > +{\n> > > +\tLOG(V4L2Compat, Debug) << \"Servicing VIDIOC_REQBUFS\";\n> > > +\tif (!validateStreamType(arg->type))\n> > > +\t\treturn -EINVAL;\n> > > +\tif (!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\tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> > > +\t\tint ret;\n> > > +\t\tvcam_->invokeMethod(&V4L2Camera::streamOff,\n> > > +\t\t\t\t ConnectionTypeBlocking, &ret);\n> > > +\t\tvcam_->invokeMethod(&V4L2Camera::freeBuffers,\n> > > +\t\t\t\t ConnectionTypeBlocking);\n> > > +\t\tbufferCount_ = 0;\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\tint ret;\n> > \n> > as ret is used function-wise, I would its declaration to the beginning\n> \n> ack\n> \n> > > +\tvcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,\n> > > +\t\t\t &ret, &curV4L2Format_, arg->count);\n> > > +\tif (ret < 0)\n> > > +\t\treturn -EINVAL;\n> > > +\targ->count = streamConfig_.bufferCount;\n> > > +\tbufferCount_ = arg->count;\n> > > +\n> > > +\tret = -EINVAL;\n> > > +\tif (arg->memory == V4L2_MEMORY_MMAP)\n> > > +\t\tvcam_->invokeMethod(&V4L2Camera::allocBuffers,\n> > > +\t\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> > > +\telse if (arg->memory == V4L2_MEMORY_DMABUF)\n> > \n> > Do we even claim support for this ?\n> \n> I was going to but then didn't... and removed it and didn't remove it in\n> other places.\n\nWe will have to, but it can be left unimplemented in the first version.\nPlease add a \\todo item.\n\n> > > +\t\tvcam_->invokeMethod(&V4L2Camera::allocDMABuffers,\n> > > +\t\t\t\t ConnectionTypeBlocking, &ret, arg->count);\n> > > +\n> > > +\tif (ret < 0) {\n> > > +\t\targ->count = 0;\n> > > +\t\treturn ret == -EACCES ? -EBUSY : ret;\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\treturn -EINVAL;\n> > > +\tif (arg->index >= stream->buffers().size())\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tunsigned int index = arg->index;\n> > > +\tmemset(arg, 0, sizeof(*arg));\n> > > +\targ->index = index;\n> > > +\targ->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;\n> > > +\targ->length = curV4L2Format_.fmt.pix.sizeimage;\n> > > +\targ->memory = V4L2_MEMORY_MMAP;\n> > > +\targ->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;\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\treturn -EINVAL;\n> > > +\tif (!validateMemoryType(arg->memory))\n> > > +\t\treturn -EINVAL;\n> > > +\tif (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);\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\treturn -EINVAL;\n> > > +\tif (!validateMemoryType(arg->memory))\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\targ->index = currentBuf_;\n> > > +\tcurrentBuf_ = (currentBuf_ + 1) % bufferCount_;\n> > > +\n> > > +\treturn vcam_->dqbuf(arg);\n> > \n> > Is dqbuf special ?\n> \n> Yes.\n> \n> > I know it could return immediately if nonblock_ is set, but\n> > invokeMethod checks that invoked method is called, if it returns\n> > immediately, that's fine. Or have I missed some other reason why this\n> > is called directly.\n> \n> Here we are waiting for the frames to be produced, which must be in a\n> different thread from where the frames are produced.\n> \n> Otherwise (speaking from experience) we either busy wait in V4L2Camera,\n> or the frame can't become available because we're blocked waiting for it\n> to become available.\n> \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> > > +\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> > > +\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> > \n> > camelCase for method names as well ?\n> \n> I suppose... I liked that the vidiocs mirrored the ioctl identifiers\n> though...\n> Should I do camelCase instead?\n> \n> vidiocQueryCap\n> vidiocEnumFmt\n> vidiocGFmt\n> vidiocTryFmt\n> vidiocReqBufs\n> vidiocStreamOn\n> \n> What do you think?\n\nI'm fine with either. The coding style mandates camelCase, but\nexceptions are allowed when mapping to snake_case symbols whose name is\nstandardized outside of libcamera.\n\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> > > +\tcase VIDIOC_EXPBUF:\n> > > +\tcase VIDIOC_ENUM_FRAMESIZES:\n> > > +\tdefault:\n> > > +\t\tret = -ENOTTY;\n> > > +\t}\n> > > +\n> > > +\tif (ret < 0) {\n> > > +\t\terrno = ret;\n> > > +\t\treturn -1;\n> > > +\t}\n> > > +\n> > > +\terrno = 0;\n> > > +\treturn ret;\n> > > +\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..64c7aadd\n> > > --- /dev/null\n> > > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > > @@ -0,0 +1,63 @@\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> > > +#ifndef __V4L2_CAMERA_PROXY_H__\n> > > +#define __V4L2_CAMERA_PROXY_H__\n> > > +\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> > > +\t~V4L2CameraProxy();\n> > > +\n> > > +\tint open(bool nonblock);\n> > > +\tint 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> > > +private:\n> > > +\tbool hasPixelFormat(unsigned int format);\n> > > +\tbool hasSize(unsigned int format, Size size);\n> > > +\tbool validateStreamType(uint32_t type);\n> > > +\tbool validateMemoryType(uint32_t memory);\n> > > +\tvoid setFmtFromConfig();\n> > > +\tvoid querycap(std::shared_ptr camera);\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 index_;\n> > > +\n> > > +\tstruct v4l2_format curV4L2Format_;\n> > > +\tStreamConfiguration streamConfig_;\n> > > +\tstruct v4l2_capability capabilities_;\n> > > +\tunsigned int bufferCount_;\n> > > +\tunsigned int currentBuf_;\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..3330e7bc\n> > > --- /dev/null\n> > > +++ b/src/v4l2/v4l2_compat.cpp\n> > > @@ -0,0 +1,81 @@\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> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#define LIBCAMERA_PUBLIC __attribute__((visibility(\"default\")))\n> > \n> > Am I wrong or this makes sense only if we compile with\n> > -fvisiblity=hidden ?\n> > https://gcc.gnu.org/wiki/Visibility\n> \n> It might be. When I tried it in the beginning with the compile options\n> that we already had, the symbols weren't being exported.\n> \n> > I welcome this change, but then probably you should add that\n> > compilation flag to the v4l2 compat library, it I have not\n> > mis-interpreted the above wiki page\n> \n> Okay.\n\nCould you please compare the objdump of the adaptation layer with and\nwithout the visibility option, to see what difference it makes ?\n\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> > > +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 openat(AT_FDCWD, path, 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> > > +\tvoid *val = V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);\n> > > +\treturn val;\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..4eeb714f\n> > > --- /dev/null\n> > > +++ b/src/v4l2/v4l2_compat_manager.cpp\n> > > @@ -0,0 +1,353 @@\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> > > +#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> > > +#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: cameraManagerRunning_(false), cm_(nullptr)\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> > You seem to be mixing cameraCase and snake_case here and there in\n\nIs cameraCase a libcamera-specific naming scheme ? :-)\n\n> > variable declaration. Was this intentional ?\n> \n> It was intentional, until I found the few that were unintentional.\n> \n> These ones, plus the vidioc ones are the only ones that I intended to be\n> snake_case and thought was borderline acceptable.\n> \n> > > +}\n> > > +\n> > > +V4L2CompatManager::~V4L2CompatManager()\n> > > +{\n> > > +\tdevices_.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);\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\n> > > +\t * gets created here to wraps a camera device.\n> > > +\t */\n> > > +\t// \\todo map v4l2 devices to libcamera cameras; minor device node?\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\n> > > +\t * as we're now ready to handle calls from the framework.\n> > > +\t */\n> > > +\tcv_.notify_one();\n> > > +\n> > > +\t/* Now start processing events and messages. */\n> > > +\texec();\n> > > +\n> > > +\tcm_->stop();\n> > > +\tdelete cm_;\n> > > +\tcm_ = nullptr;\n> > > +}\n> > > +\n> > > +V4L2CompatManager *V4L2CompatManager::instance()\n> > > +{\n> > > +\tstatic V4L2CompatManager v4l2CompatManager;\n> > > +\treturn &v4l2CompatManager;\n> > > +}\n> > > +\n> > > +bool V4L2CompatManager::validfd(int fd)\n> > > +{\n> > > +\treturn devices_.find(fd) != devices_.end();\n> > > +}\n> > > +\n> > > +bool V4L2CompatManager::validmmap(void *addr)\n> > > +{\n> > > +\treturn mmaps_.find(addr) != mmaps_.end();\n> > > +}\n> > > +\n> > > +std::shared_ptr V4L2CompatManager::getCamera(int fd)\n> > > +{\n> > > +\tif (!validfd(fd))\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn devices_.at(fd);\n> > \n> > This is safe, but it's a double lookup, same as below. This is minor\n> > indeed, but probably using the iterator returned from find() directly\n> > is slightly more efficient.\n> \n> How can I use the iterator directly?\n> \n> When we intercept the calls and check if we should simply forward the\n> call, I think it's simpler to have just a validfd() call... but then\n> when we do need to process it then it does become a double (triple?)\n> lookup... or just check getCamera() == nullptr instead of validfd().\n> \n> > Then you can inline the valid[fd|mmap}() methods, probably\n> > \n> > > +}\n> > > +\n> > > +std::shared_ptr V4L2CompatManager::getCamera(void *addr)\n> > > +{\n> > > +\tif (!validmmap(addr))\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn devices_.at(mmaps_.at(addr));\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> > > +\tif (std::string(path).find(\"video\") == std::string::npos)\n> > > +\t\treturn fd;\n> > > +\n> > > +\tif (!isRunning())\n> > > +\t\tinit();\n> > > +\n> > > +\t/* \\todo map v4l2 devnodes to libcamera cameras */\n> > > +\tunsigned int camera_index = 0;\n> > > +\n> > > +\tstd::shared_ptr proxy = proxies_[camera_index];\n> > > +\tint ret = proxy->open(mode & O_NONBLOCK);\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tint efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));\n> > > +\tif (efd < 0)\n> > > +\t\treturn errno;\n> > \n> > I'm not sure I get this usage of the file descriptor returned by\n> > eventfd...\n> > \n> > It is used as index in the map, possibly replaced by dup(). I would\n> > have expected the fd returned by openat() to be used as index in the\n> > map... What am I missing ?\n> \n> The fd returned by eventfd reserves the fd so that any other open()\n> call doesn't overwrite/conflict with our fd. This fd is indeed used as\n> an index in the map (see next line), and dup() doesn't replace it.\n\nThe fd returned by openat_func_ at the top is leaked by the way. It\nshould be closed before calling isRunning().\n\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> > > +\tif (validfd(oldfd))\n> > \n> > Shouldn't you return an error if the fd to duplicate is not valid, ad\n> > dup() only if it is ?\n> \n> If the fd is not valid, then it means the fd is not for us, and we need\n> to forward the dup() call. So in any case, we need to dup(). The only\n> difference is if we need to save the new fd in the mapping or not.\n> \n> > > +\t\tdevices_[newfd] = devices_[oldfd];\n> > > +\n> > > +\treturn newfd;\n> > > +}\n> > > +\n> > > +int V4L2CompatManager::close(int fd)\n> > > +{\n> > > +\tif (validfd(fd) && devices_[fd]->close() < 0)\n> > \n> > I would return -EIO if !validfd() and propagate the error up if\n> > close() fails.\n> \n> If !validfd() then the fd isn't for us, and the call needs to be\n> forwarded. -EIO is only if *our* close() fails.\n> \n> > > +\t\treturn -EIO;\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> > > +\tif (!validfd(fd))\n> > > +\t\treturn mmap_func_(addr, length, prot, flags, fd, offset);\n> > > +\n> > > +\tvoid *map = getCamera(fd)->mmap(addr, length, prot, flags, offset);\n> > > +\tif (map == MAP_FAILED) {\n> > > +\t\terrno = EINVAL;\n> > > +\t\treturn map;\n> > > +\t}\n> > > +\n> > > +\tmmaps_[addr] = fd;\n> > > +\treturn map;\n> > > +}\n> > > +\n> > > +int V4L2CompatManager::munmap(void *addr, size_t length)\n> > > +{\n> > > +\tif (!validmmap(addr))\n> > > +\t\treturn munmap_func_(addr, length);\n> > > +\n> > > +\tint ret = getCamera(addr)->munmap(addr, length);\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tmmaps_.erase(addr);\n> > > +\taddr = nullptr;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)\n> > > +{\n> > > +\tstd::shared_ptr proxy = getCamera(fd);\n> > > +\tif (!proxy)\n> > > +\t\treturn ioctl_func_(fd, request, arg);\n> > > +\n> > \n> > What is the use case for bypassing the proxy ? That might be my\n> > limited knowledge of how v4l2 application behave\n> \n> Perhaps. Here I check if proxy == nullptr, but in other places I checked\n> !validfd(fd) (I'll make these consistent). If the fd is not valid = the\n> proxy doesn't exist for the fd, then the fd is not for us and we need to\n> forward it to the original call.\n> \n> > > +\treturn proxy->ioctl(request, arg);\n> > > +}\n> > > +\n> > > +/* \\todo make libcamera export these */\n> > \n> > mmm, V4L2VideoDevice has very similar format conversion routines... we\n> > should really centralize them somehow.. maybe not in scope for this\n> > patch, but replicating cose which is tricky to get right due to the\n> > different format definition between DRM and V4L2 is not a good idea.\n> \n> I know...\n> \n> > > +int V4L2CompatManager::bpl_multiplier(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 V4L2CompatManager::image_size(unsigned int format,\n> > > +\t\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 V4L2CompatManager::v4l2_to_drm(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 V4L2CompatManager::drm_to_v4l2(unsigned int pixelformat)\n> > > +{\n> > > +\tswitch (pixelformat) {\n> > > +\t/* RGB formats. */\n> > > +\tcase DRM_FORMAT_BGR888:\n> > > +\t\treturn V4L2_PIX_FMT_BGR24;\n> > \n> > This in example does not match the one we have in V4L2Videodevice.\n> > DRM_FORMAT_BGR24 = V4L2_PIX_FMT_RGB24 not BGR24.\n> \n> Oh wat :/\n> Great.\n> \n> > I know, it's a pain.\n> > \n> > I think the other version is correct, as we validated them using\n> > conversion routines from kernel drivers...\n> > \n> > There might be more below (and possibly above)\n> > \n> > > +\tcase DRM_FORMAT_RGB888:\n> > > +\t\treturn V4L2_PIX_FMT_RGB24;\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_compat_manager.h b/src/v4l2/v4l2_compat_manager.h\n> > > new file mode 100644\n> > > index 00000000..492f97fd\n> > > --- /dev/null\n> > > +++ b/src/v4l2/v4l2_compat_manager.h\n> > > @@ -0,0 +1,91 @@\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> > > +#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> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \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> > > +\tint openat(int dirfd, const char *path, int oflag, mode_t mode);\n> > > +\n> > > +\tstd::shared_ptr getCamera(int fd);\n> > > +\tstd::shared_ptr getCamera(void *addr);\n> > \n> > Every time you return a shared_ptr by value, you increase the\n> > reference count for no good reason.\n> \n> But it goes back down after the caller finishes right? :p\n> \n> > I would either return by reference\n> > or return V4L2CameraProxy and use shared pointers only in the proxies_\n> > array and not in the rest of the code base.\n> \n> Okay.\n\nIf you return a reference, make it a const reference, otherwise the\ncaller would be able to mess up with the shared pointer stored\ninternally in the compat manager.\n\nI've only looked at Jacopo's comments in this review, I'll have a look\nat your code when reviewing v2. As I disagreed with a few points above,\nsome of the changes in v2 may need to be reverted (or modified), could\nyou please already have a look at those ?\n\n> > Overall this is a very good first submission and I'm happy to see it\n> > posted to the list \\o/\n> > \n> > I admit in this first pass I didn't go into extreme detail on the way\n> > the v4l2 operations semantic is handled, but it seems sane to me!\n> \n> Thank you!\n> \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> > > +\tstatic int bpl_multiplier(unsigned int format);\n> > > +\tstatic int image_size(unsigned int format, unsigned int width,\n> > > +\t\t\t unsigned int height);\n> > > +\n> > > +\tstatic unsigned int v4l2_to_drm(unsigned int pixelformat);\n> > > +\tstatic unsigned int drm_to_v4l2(unsigned int pixelformat);\n> > > +\n> > > +private:\n> > > +\tV4L2CompatManager();\n> > > +\t~V4L2CompatManager();\n> > > +\n> > > +\tvoid run() override;\n> > > +\n> > > +\tbool validfd(int fd);\n> > > +\tbool validmmap(void *addr);\n> > > +\n> > > +\tint default_ioctl(int fd, unsigned long request, void *arg);\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> > > +\tbool cameraManagerRunning_;\n> > > +\tCameraManager *cm_;\n> > > +\n> > > +\tstd::mutex mutex_;\n> > > +\tstd::condition_variable cv_;\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 449D661378\n\tfor ;\n\tMon, 9 Dec 2019 12:01:24 +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 7627999A;\n\tMon, 9 Dec 2019 12:01:23 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575889283;\n\tbh=4ZrHbAfv/ziv6c44sHzzYUHtlKlH/rz0tJ8IPLhwf2E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X+UvfyEAd5ej3F8AXltQY9EGiDxdFMq0SOeJYGDM5aE3+fjRIVrUR//Zoq8uEllJw\n\tsxF+l0VWavUUcXvQ6qjJIWS4ac4ViW+pDcJyyfX6AR1Ijw51/gSaIPKz6/RtmHg/pP\n\tTTPXvg4jwgcAuXPp0DoA5kVNBaN9zIZCwtsaybGc=","Date":"Mon, 9 Dec 2019 13:01:16 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191209110116.GA4853@pendragon.ideasonboard.com>","References":"<20191206092654.24340-1-paul.elder@ideasonboard.com>\n\t<20191206092654.24340-2-paul.elder@ideasonboard.com>\n\t<20191206161203.gpbtfub44oarpzic@uno.localdomain>\n\t<20191209011031.GA12803@localhost.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191209011031.GA12803@localhost.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Mon, 09 Dec 2019 11:01:24 -0000"}}]