[libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2 compatibility layer

Laurent Pinchart laurent.pinchart at ideasonboard.com
Thu Jan 2 04:02:14 CET 2020


Hi Paul,

Thank you for the patch. Nearly there, there remaining comments are
small.

On Tue, Dec 31, 2019 at 11:22:09AM +0100, Jacopo Mondi wrote:
> On Mon, Dec 30, 2019 at 11:33:14PM -0600, Paul Elder wrote:
> > Add libcamera V4L2 compatibility layer.
> >
> > This initial implementation supports the minimal set of V4L2 operations,
> > which allows getting, setting, and enumerating formats, and streaming
> > frames from a video device. Some data about the wrapped V4L2 video
> > device are hardcoded.
> >
> > Add a build option named 'v4l2' and adjust the build system to
> > selectively compile the V4L2 compatibility layer.
> >
> > For now we match the V4L2 device node to a libcamera camera based on a
> > devnum that a pipeline handler may optionally map to a libcamera
> > camera.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Major changes in v4:
> > - squash buffer state tracking in V4L2CameraProxy (from 6/6 in v3)
> > - separated out V4L2CameraProxy::freeBuffers() (since it was cluttering
> >   up V4L2CameraProxy::vidioc_reqbufs)
> >
> > Less major changes in v4:
> > - check that addresses passed to V4L2CameraProxy::munmap() have been
> >   mmaped
> > - other cosmetic changes
> >
> > Major changes in v3:
> > - V4L2CompatManager::openat() verify video node and match to camera via
> >   devnum instead of path
> > - add FrameMetadata class to encapsulate portions of the libcamera
> >   Buffer that needs to be extracted to v4l2_buffer, and to prepare for
> >   the Buffer rework
> > - V4L2CameraProxy refcount for tracking dups and closes (and open)
> > - add V4L2CompatManager::initialized_ to deal with race of waiting on
> >   init cv after the cv is notified
> >
> > Less major changes in v3:
> > - change the list of pending Reqeusts (v4l2 buffers queued before
> >   streamon) to unique pointers
> > - V4L2Camera::updateSizeImage() -> V4L2CameraProxy::calculateSizeImage()
> > - change V4L2CompatManager list of V4L2CameraProxy to unique_ptr, with
> >   maps of fd/mmap addr to V4L2CameraProxy *
> > - removed cross-thread validation methods from V4L2CameraProxy
> >   (validate[Stream|Memory]Type)
> > - removed hasPixelFormat() and hasSize() from V4L2CameraProxy
> > - moved mmap logic out of V4L2Camera and into V4L2CameraProxy
> > - moved nonblock logic out of V4L2Camera and into V4L2CameraProxy
> > - add todos
> > - oter cosmetic changes
> >
> > Changes in v2:
> > - move all errno acrobatics to V4L2CameraProxy
> > - remove all mentions of dmabuf
> > - make V4L2CompatManager::getCamera return pointer rather than
> >   shared_ptr
> > - match V4L2 device nodes to libcamera cameras using Camera::name()
> >   compared to /sys/dev/char/maj:min/name (only works for UVC cameras)
> >   - in V4L2CompatManager::getCameraIndex()
> > - add -fvisibility=hidden to v4l2 compat
> > - cache the results of V4L2CompatManager::imageSize() within V4L2Camera
> >   (where V4L2Camera is interested)
> > - remove V4L2CompatManager::valid[fd|mmap], and where those methods were
> >   used, check V4L2CompatManager::getCamera() != nullptr instead
> > - fixed V4L2CompatManager::drmToV4L2() mappings for DRM_FORMAT_BGR888
> >   and DRM_FORMAT_RGB888
> > - other cosmetic changes
> > ---
> >  meson_options.txt                |   5 +
> >  src/meson.build                  |   4 +
> >  src/v4l2/meson.build             |  31 ++
> >  src/v4l2/v4l2_camera.cpp         | 233 +++++++++++
> >  src/v4l2/v4l2_camera.h           |  83 ++++
> >  src/v4l2/v4l2_camera_proxy.cpp   | 649 +++++++++++++++++++++++++++++++
> >  src/v4l2/v4l2_camera_proxy.h     |  82 ++++
> >  src/v4l2/v4l2_compat.cpp         |  80 ++++
> >  src/v4l2/v4l2_compat_manager.cpp | 255 ++++++++++++
> >  src/v4l2/v4l2_compat_manager.h   |  77 ++++
> >  10 files changed, 1499 insertions(+)
> >  create mode 100644 src/v4l2/meson.build
> >  create mode 100644 src/v4l2/v4l2_camera.cpp
> >  create mode 100644 src/v4l2/v4l2_camera.h
> >  create mode 100644 src/v4l2/v4l2_camera_proxy.cpp
> >  create mode 100644 src/v4l2/v4l2_camera_proxy.h
> >  create mode 100644 src/v4l2/v4l2_compat.cpp
> >  create mode 100644 src/v4l2/v4l2_compat_manager.cpp
> >  create mode 100644 src/v4l2/v4l2_compat_manager.h
> >
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 1a328045..79ee4de6 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -10,3 +10,8 @@ option('documentation',
> >  option('test',
> >          type : 'boolean',
> >          description: 'Compile and include the tests')
> > +
> > +option('v4l2',
> > +        type : 'boolean',
> > +        value : false,
> > +        description : 'Compile the V4L2 compatibility layer')
> > diff --git a/src/meson.build b/src/meson.build
> > index 67ad20aa..5adcd61f 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -6,3 +6,7 @@ subdir('libcamera')
> >  subdir('ipa')
> >  subdir('cam')
> >  subdir('qcam')
> > +
> > +if get_option('v4l2')
> > +    subdir('v4l2')
> > +endif
> > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> > new file mode 100644
> > index 00000000..14ee3594
> > --- /dev/null
> > +++ b/src/v4l2/meson.build
> > @@ -0,0 +1,31 @@
> > +v4l2_compat_sources = files([
> > +    'v4l2_camera.cpp',
> > +    'v4l2_camera_proxy.cpp',
> > +    'v4l2_compat.cpp',
> > +    'v4l2_compat_manager.cpp',
> > +])
> > +
> > +v4l2_compat_includes = [
> > +    libcamera_includes,
> > +    libcamera_internal_includes,
> > +]
> > +
> > +v4l2_compat_cpp_args = [
> > +    # Meson enables large file support unconditionally, which redirect file
> > +    # operations to 64-bit versions. This results in some symbols being
> > +    # renamed, for instance open() being renamed to open64(). As the V4L2
> > +    # adaptation wrapper needs to provide both 32-bit and 64-bit versions of
> > +    # file operations, disable transparent large file support.
> > +    '-U_FILE_OFFSET_BITS',
> > +    '-D_FILE_OFFSET_BITS=32',
> > +    '-fvisibility=hidden',
> > +]
> > +
> > +v4l2_compat = shared_library('v4l2-compat',
> > +                             v4l2_compat_sources,
> > +                             name_prefix : '',
> > +                             install : true,
> > +                             link_with : libcamera,
> > +                             include_directories : v4l2_compat_includes,
> > +                             dependencies : libcamera_deps,
> > +                             cpp_args : v4l2_compat_cpp_args)
> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > new file mode 100644
> > index 00000000..82ada8d4
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -0,0 +1,233 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_camera.cpp - V4L2 compatibility camera
> > + */
> > +
> > +#include "v4l2_camera.h"
> > +
> > +#include <errno.h>
> > +#include <linux/videodev2.h>
> > +#include <sys/mman.h>
> > +#include <sys/syscall.h>
> > +#include <time.h>
> > +#include <unistd.h>
> > +
> > +#include "log.h"
> > +#include "utils.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(V4L2Compat);
> > +
> > +FrameMetadata::FrameMetadata(Buffer *buffer)
> > +	: index_(buffer->index()), bytesused_(buffer->bytesused()),
> > +	  timestamp_(buffer->timestamp()), sequence_(buffer->sequence()),
> > +	  status_(buffer->status())
> > +{
> > +}
> > +
> > +V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> > +	: camera_(camera), isRunning_(false)
> > +{
> > +	camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> > +}
> > +
> > +V4L2Camera::~V4L2Camera()
> > +{
> > +	camera_->release();
> > +}
> > +
> > +void V4L2Camera::open(int *ret)
> > +{
> > +	if (camera_->acquire() < 0) {
> > +		LOG(V4L2Compat, Error) << "Failed to acquire camera";
> > +		*ret = -EINVAL;
> > +		return;
> > +	}
> > +
> > +	config_ = camera_->generateConfiguration({ StreamRole::Viewfinder });
> > +	if (!config_) {
> > +		camera_->release();
> > +		*ret = -EINVAL;
> > +		return;
> > +	}
> > +
> > +	*ret = 0;
> > +}
> > +
> > +void V4L2Camera::close()
> > +{
> > +	camera_->release();
> > +}
> > +
> > +void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
> > +{
> > +	*streamConfig = config_->at(0);
> > +}
> > +
> > +std::vector<FrameMetadata> V4L2Camera::completedBuffers()
> > +{
> > +	std::vector<FrameMetadata> v;
> > +
> > +	bufferLock_.lock();
> > +	for (std::unique_ptr<FrameMetadata> &metadata : completedBuffers_)
> > +		v.push_back(*metadata.get());
> > +	completedBuffers_.clear();
> > +	bufferLock_.unlock();
> > +
> > +	return v;
> > +}
> > +
> > +void V4L2Camera::requestComplete(Request *request)
> > +{
> > +	if (request->status() == Request::RequestCancelled)
> > +		return;
> > +
> > +	/* We only have one stream at the moment. */
> > +	bufferLock_.lock();
> > +	Buffer *buffer = request->buffers().begin()->second;
> > +	std::unique_ptr<FrameMetadata> metadata =
> > +		utils::make_unique<FrameMetadata>(buffer);
> > +	completedBuffers_.push_back(std::move(metadata));
> > +	bufferLock_.unlock();
> > +
> > +	bufferSema_.release();
> 
> The usage of this semaphore from two different classes looks weird :/

We could encapsulate it, but given that V4L2Camera and V4L2CameraProxy
are internal to the V4L2 compatibility layer, I think it's overkill.

> > +}
> > +
> > +void V4L2Camera::configure(int *ret, StreamConfiguration *streamConfigOut,
> > +			   Size size, PixelFormat pixelformat,

No need to copy the size, you can make it a const Size &.

> > +			   unsigned int bufferCount)
> > +{
> > +	StreamConfiguration &streamConfig = config_->at(0);
> > +	streamConfig.size.width = size.width;
> > +	streamConfig.size.height = size.height;
> > +	streamConfig.pixelFormat = pixelformat;
> > +	streamConfig.bufferCount = bufferCount;
> > +	/* \todo memoryType (interval vs external) */
> > +
> > +	CameraConfiguration::Status validation = config_->validate();
> > +	if (validation == CameraConfiguration::Invalid) {
> > +		LOG(V4L2Compat, Error) << "Configuration invalid";
> > +		*ret = -EINVAL;
> > +		return;
> > +	}
> > +	if (validation == CameraConfiguration::Adjusted)
> > +		LOG(V4L2Compat, Info) << "Configuration adjusted";
> > +
> > +	LOG(V4L2Compat, Info) << "Validated configuration is: "
> > +			      << streamConfig.toString();
> > +
> > +	*ret = camera_->configure(config_.get());
> > +	if (*ret < 0)
> > +		return;
> > +
> > +	*streamConfigOut = config_->at(0);
> > +}
> > +
> > +void V4L2Camera::mmap(void **ret, unsigned int index)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> 
> You have removed most of these, but some are still here, is this
> intentional ?
> 
> > +
> > +	Stream *stream = *camera_->streams().begin();
> > +	*ret = stream->buffers()[index].planes()[0].mem();
> > +}
> > +
> > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Allocating libcamera bufs";
> > +
> > +	*ret = camera_->allocateBuffers();
> > +	if (*ret == -EACCES)
> > +		*ret = -EBUSY;
> > +}
> > +
> > +void V4L2Camera::freeBuffers()
> > +{
> > +	camera_->freeBuffers();
> > +}
> > +
> > +void V4L2Camera::streamOn(int *ret)
> > +{
> > +	*ret = 0;
> 
> Do you need this ?

Without this the next return will result in ret being uninitialized.

I'll try to implement native support for return values in
invokeMethod().

> > +
> > +	if (isRunning_)
> > +		return;
> > +
> > +	*ret = camera_->start();
> > +	if (*ret < 0) {
> > +		if (*ret == -EACCES)
> > +			*ret = -EBUSY;
> > +		return;
> > +	}
> > +	isRunning_ = true;
> > +
> > +	for (std::unique_ptr<Request> &req : pendingRequests_) {
> > +		/* \todo What should we do if this returns -EINVAL? */
> > +		*ret = camera_->queueRequest(req.release());
> > +		if (*ret < 0) {
> > +			*ret = (*ret == -EACCES ? -EAGAIN : *ret);
> 
> A few lines below you have the same construct in the form of
> 
> 	if (*ret < 0) {
> 		if (*ret == -EACCES)
> 			*ret = -EBUSY;
> 
> please unify on one of the two
> 
> > +			return;
> > +		}
> > +	}
> > +
> > +	pendingRequests_.clear();
> > +}
> > +
> > +void V4L2Camera::streamOff(int *ret)
> > +{
> > +	*ret = 0;
> 
> do you need this ?
> 
> > +
> > +	/* \todo restore buffers to reqbufs state? */
> > +	if (!isRunning_)
> > +		return;
> > +
> > +	*ret = camera_->stop();
> > +	if (*ret < 0) {
> > +		if (*ret == -EACCES)
> > +			*ret = -EBUSY;
> > +		return;
> > +	}
> > +	isRunning_ = false;
> > +}
> > +
> > +void V4L2Camera::qbuf(int *ret, unsigned int index)
> > +{
> > +	Stream *stream = config_->at(0).stream();
> > +	std::unique_ptr<Buffer> buffer = stream->createBuffer(index);
> > +	if (!buffer) {
> > +		LOG(V4L2Compat, Error) << "Can't create buffer";
> > +		*ret = -ENOMEM;
> > +		return;
> > +	}
> > +
> > +	std::unique_ptr<Request> request =
> > +		std::unique_ptr<Request>(camera_->createRequest());
> > +	if (!request) {
> > +		LOG(V4L2Compat, Error) << "Can't create request";
> > +		*ret = -ENOMEM;
> > +		return;
> > +	}
> > +
> > +	*ret = request->addBuffer(std::move(buffer));
> > +	if (*ret < 0) {
> > +		LOG(V4L2Compat, Error) << "Can't set buffer for request";
> > +		*ret = -ENOMEM;
> > +		return;
> > +	}
> > +
> > +	if (!isRunning_) {
> > +		pendingRequests_.push_back(std::move(request));
> 
> You can return here and remove the else branch below
> ret should be set to 0 already by addBuffer()
> 
> > +	} else {
> > +		*ret = camera_->queueRequest(request.release());
> > +		if (*ret < 0) {
> > +			LOG(V4L2Compat, Error) << "Can't queue request";
> > +			if (*ret == -EACCES)
> > +				*ret = -EBUSY;
> > +			return;
> > +		}
> > +	}
> > +
> > +	*ret = 0;
> > +}
> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > new file mode 100644
> > index 00000000..a48a2d59
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_camera.h - V4L2 compatibility camera
> > + */
> > +
> > +#ifndef __V4L2_CAMERA_H__
> > +#define __V4L2_CAMERA_H__
> > +
> > +#include <deque>
> > +#include <linux/videodev2.h>
> > +#include <mutex>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/camera.h>
> 
> Do we separate publinc and internal inclusion directives with an empty line ?
> 
> > +#include "semaphore.h"
> > +
> > +using namespace libcamera;
> > +
> > +class FrameMetadata
> > +{
> > +public:
> > +	FrameMetadata(Buffer *buffer);
> > +
> > +	int index() const { return index_; }
> > +
> > +	unsigned int bytesused() const { return bytesused_; }
> > +	uint64_t timestamp() const { return timestamp_; }
> > +	unsigned int sequence() const { return sequence_; }
> > +
> > +	Buffer::Status status() const { return status_; }
> > +
> > +private:
> > +	int index_;
> > +
> > +	unsigned int bytesused_;
> > +	uint64_t timestamp_;
> > +	unsigned int sequence_;
> > +
> > +	Buffer::Status status_;
> > +};
> > +
> > +class V4L2Camera : public Object
> > +{
> > +public:
> > +	V4L2Camera(std::shared_ptr<Camera> camera);
> > +	~V4L2Camera();
> > +
> > +	void open(int *ret);
> > +	void close();
> > +	void getStreamConfig(StreamConfiguration *streamConfig);
> > +	std::vector<FrameMetadata> completedBuffers();
> > +
> > +	void mmap(void **ret, unsigned int index);
> > +
> > +	void configure(int *ret, StreamConfiguration *streamConfigOut,
> > +		       Size size, PixelFormat pixelformat,
> > +		       unsigned int bufferCount);
> > +
> > +	void allocBuffers(int *ret, unsigned int count);
> > +	void freeBuffers();
> > +	void streamOn(int *ret);
> > +	void streamOff(int *ret);
> > +
> > +	void qbuf(int *ret, unsigned int index);
> > +
> > +	Semaphore bufferSema_;

You need an empty line here.

> > +private:
> > +	void requestComplete(Request *request);
> > +
> > +	std::shared_ptr<Camera> camera_;
> > +	std::unique_ptr<CameraConfiguration> config_;
> > +
> > +	bool isRunning_;
> > +
> > +	std::mutex bufferLock_;
> > +
> > +	std::deque<std::unique_ptr<Request>> pendingRequests_;
> > +	std::deque<std::unique_ptr<FrameMetadata>> completedBuffers_;
> > +};
> > +
> > +#endif /* __V4L2_CAMERA_H__ */
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > new file mode 100644
> > index 00000000..ae27ea1b
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -0,0 +1,649 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_camera_proxy.cpp - Proxy to V4L2 compatibility camera
> > + */
> > +
> > +#include "v4l2_camera_proxy.h"
> > +
> > +#include <algorithm>
> > +#include <linux/drm_fourcc.h>
> > +#include <linux/videodev2.h>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/object.h>
> > +
> > +#include "log.h"
> > +#include "utils.h"
> > +#include "v4l2_camera.h"
> > +#include "v4l2_compat_manager.h"
> > +
> > +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(V4L2Compat);
> > +
> > +V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
> > +				 std::shared_ptr<Camera> camera)
> > +	: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),
> > +	  vcam_(utils::make_unique<V4L2Camera>(camera))
> > +{
> > +	querycap(camera);
> > +}
> > +
> > +int V4L2CameraProxy::open(bool nonBlocking)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing open";
> > +
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,
> > +			    &ret);
> > +	if (ret < 0) {
> > +		errno = -ret;
> > +		return -1;
> > +	}
> > +
> > +	nonBlocking_ = nonBlocking;
> > +
> > +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> > +			    ConnectionTypeBlocking, &streamConfig_);
> > +	setFmtFromConfig(streamConfig_);
> > +	sizeimage_ = calculateSizeImage(streamConfig_);
> > +
> > +	refcount_++;
> > +
> > +	return 0;
> > +}
> > +
> > +void V4L2CameraProxy::dup()
> > +{
> > +	refcount_++;
> > +}
> > +
> > +void V4L2CameraProxy::close()
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing close";
> > +
> > +	if (--refcount_ > 0)
> > +		return;
> > +
> > +	vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking);
> > +
> > +	return;

This return isn't needed.

> > +}
> > +
> > +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > +			    off_t offset)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing mmap";
> 
> This seems to be duplicated in V4L2Camera
> 
> > +
> > +	if (prot != (PROT_READ | PROT_WRITE)) {
> > +		errno = ENOTSUP;
> > +		return MAP_FAILED;
> > +	}
> > +
> > +	unsigned int index = offset / sizeimage_;
> > +	if (index * sizeimage_ != offset || length != sizeimage_) {
> > +		errno = EINVAL;
> > +		return MAP_FAILED;
> > +	}
> > +
> > +	void *val;
> > +	vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
> > +			    &val, index);
> > +
> > +	buffers_[index].flags |= V4L2_BUF_FLAG_MAPPED;
> > +	mmaps_[val] = index;
> > +
> > +	return val;
> > +}
> > +
> > +int V4L2CameraProxy::munmap(void *addr, size_t length)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing munmap";
> > +
> > +	auto iter = mmaps_.find(addr);
> > +	if (iter == mmaps_.end() || length != sizeimage_) {
> > +		errno = EAGAIN;

Shouldn't this be EINVAL ?

> > +		return -1;
> > +	}
> > +
> > +	buffers_[iter->second].flags &= ~V4L2_BUF_FLAG_MAPPED;
> > +	mmaps_.erase(iter);
> > +
> > +	return 0;
> > +}
> > +
> > +bool V4L2CameraProxy::validateStreamType(uint32_t type)
> > +{
> > +	return type == V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +}
> > +
> > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> > +{
> > +	return memory == V4L2_MEMORY_MMAP;
> > +}
> > +
> > +void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig)
> > +{
> > +	curV4L2Format_.fmt.pix.width = streamConfig.size.width;
> > +	curV4L2Format_.fmt.pix.height = streamConfig.size.height;
> > +	curV4L2Format_.fmt.pix.pixelformat = drmToV4L2(streamConfig.pixelFormat);
> > +	curV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;
> > +	curV4L2Format_.fmt.pix.bytesperline =
> > +		bplMultiplier(curV4L2Format_.fmt.pix.pixelformat) *
> > +		curV4L2Format_.fmt.pix.width;
> > +	curV4L2Format_.fmt.pix.sizeimage =
> > +		imageSize(curV4L2Format_.fmt.pix.pixelformat,
> > +			  curV4L2Format_.fmt.pix.width,
> > +			  curV4L2Format_.fmt.pix.height);
> > +	curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > +}
> > +
> > +unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig)
> > +{
> > +	return imageSize(drmToV4L2(streamConfig.pixelFormat),
> > +			 streamConfig.size.width,
> > +			 streamConfig.size.height);
> > +}
> > +
> > +void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
> > +{
> > +	std::string driver = "libcamera";
> > +	std::string bus_info = driver + ":" + std::to_string(index_);
> > +
> > +	memcpy(capabilities_.driver, driver.c_str(),
> > +	       sizeof(capabilities_.driver));
> > +	memcpy(capabilities_.card, camera->name().c_str(),
> > +	       sizeof(capabilities_.card));
> > +	memcpy(capabilities_.bus_info, bus_info.c_str(),
> > +	       sizeof(capabilities_.bus_info));
> > +	/* \todo Put this in a header/config somewhere. */
> > +	capabilities_.version = KERNEL_VERSION(5, 2, 0);
> > +	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;
> > +	capabilities_.capabilities = capabilities_.device_caps
> > +				   | V4L2_CAP_DEVICE_CAPS;
> > +	memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
> > +}
> > +
> > +void V4L2CameraProxy::updateBuffers()
> > +{
> > +	std::vector<FrameMetadata> completedBuffers = vcam_->completedBuffers();
> 
> This should be a reference otherwise you copy the whole vector
> everytime you dequeue or query a buffer

completedBuffers() returns a value, not a reference, so this is fine.

> > +	for (FrameMetadata &fmd : completedBuffers) {
> > +		/* \todo is this index valid if the buffer status != success? */
> > +		struct v4l2_buffer &buf = buffers_[fmd.index()];
> > +
> > +		switch (fmd.status()) {
> > +		case Buffer::Status::BufferSuccess:
> > +			buf.index = fmd.index();
> > +			buf.bytesused = fmd.bytesused();
> > +			buf.field = V4L2_FIELD_NONE;
> > +			buf.timestamp.tv_sec = fmd.timestamp() / 1000000000;
> > +			buf.timestamp.tv_usec = fmd.timestamp() % 1000000;
> > +			buf.sequence = fmd.sequence();
> > +
> > +			buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +			buf.length = curV4L2Format_.fmt.pix.sizeimage;
> > +			buf.memory = V4L2_MEMORY_MMAP;
> > +			buf.m.offset = buf.index * curV4L2Format_.fmt.pix.sizeimage;
> > +
> > +			buf.flags |= V4L2_BUF_FLAG_DONE;
> > +			break;
> > +		case Buffer::Status::BufferError:
> > +			buf.flags |= V4L2_BUF_FLAG_ERROR;
> 
> not strictly necessary but break here please, it's safer if we later
> do something in default:

Agreed. Furthermore, with a compiler that enables the implicit
fall-through warning it would result in a warning.

> > +		default:
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap";
> > +
> > +	memcpy(arg, &capabilities_, sizeof(*arg));
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt";
> > +
> > +	if (!validateStreamType(arg->type) ||
> > +	    arg->index > streamConfig_.formats().pixelformats().size())
> > +		return -EINVAL;
> > +
> > +	/* \todo Add map from format to description. */
> > +	memcpy(arg->description, "Video Format Description", 5);

5 bytes may be too little :-)

	strlcpy(arg->description, "Video Format Description",
		sizeof(arg->description));

> > +	arg->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]);
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt";
> > +
> > +	if (!validateStreamType(arg->type))
> > +		return -EINVAL;
> > +
> > +	memset(&arg->fmt, 0, sizeof(arg->fmt));
> > +	arg->fmt.pix = curV4L2Format_.fmt.pix;
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt";
> > +
> > +	int ret = vidioc_try_fmt(arg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> > +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > +			    &ret, &streamConfig_, size,
> > +			    v4l2ToDrm(arg->fmt.pix.pixelformat), bufferCount_);
> > +	if (ret < 0)
> > +		return -EINVAL;
> 
> Should you set errno like you do in other functions ?

For ioctls errno is set by V4L2CameraProxy::ioctl().

> > +
> > +	unsigned int sizeimage = calculateSizeImage(streamConfig_);
> > +	if (sizeimage == 0)
> > +		return -EINVAL;
> > +
> > +	sizeimage_ = sizeimage;
> > +
> > +	setFmtFromConfig(streamConfig_);
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_try_fmt(struct v4l2_format *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt";
> > +	if (!validateStreamType(arg->type))
> > +		return -EINVAL;
> > +
> > +	unsigned int format = arg->fmt.pix.pixelformat;
> > +	const std::vector<PixelFormat> &formats =
> > +		streamConfig_.formats().pixelformats();
> > +	if (std::find(formats.begin(), formats.end(), format) == formats.end())
> > +		format = streamConfig_.formats().pixelformats()[0];
> > +
> > +	Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> > +	const std::vector<Size> &sizes = streamConfig_.formats().sizes(format);
> > +	if (std::find(sizes.begin(), sizes.end(), size) == sizes.end())
> > +		size = streamConfig_.formats().sizes(format)[0];
> > +
> > +	arg->fmt.pix.width        = size.width;
> > +	arg->fmt.pix.height       = size.height;
> > +	arg->fmt.pix.pixelformat  = format;
> > +	arg->fmt.pix.field        = V4L2_FIELD_NONE;
> > +	arg->fmt.pix.bytesperline = bplMultiplier(drmToV4L2(format)) *
> > +				    arg->fmt.pix.width;
> > +	arg->fmt.pix.sizeimage    = imageSize(drmToV4L2(format),
> > +					      arg->fmt.pix.width,
> > +					      arg->fmt.pix.height);
> > +	arg->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::freeBuffers()
> > +{
> > +	LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> > +
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::streamOff,
> > +			    ConnectionTypeBlocking, &ret);
> > +	if (ret < 0) {
> > +		LOG(V4L2Compat, Error) << "Failed to stop stream";
> > +		return ret;
> > +	}
> > +	vcam_->invokeMethod(&V4L2Camera::freeBuffers,
> > +			    ConnectionTypeBlocking);
> > +	bufferCount_ = 0;
> 
> Empty line before return
> 
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> > +{
> > +	int ret;
> > +
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs";
> > +	if (!validateStreamType(arg->type) ||
> > +	    !validateMemoryType(arg->memory))
> > +		return -EINVAL;
> > +
> > +	LOG(V4L2Compat, Debug) << arg->count << " bufs requested ";
> > +
> > +	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > +
> > +	if (arg->count == 0)
> > +		return freeBuffers();
> > +
> > +	Size size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);
> > +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > +			    &ret, &streamConfig_, size,
> > +			    v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),
> > +			    arg->count);
> > +	if (ret < 0)
> > +		return -EINVAL;
> > +
> > +	sizeimage_ = calculateSizeImage(streamConfig_);
> > +	if (sizeimage_ == 0)
> > +		return -EINVAL;
> > +
> > +	setFmtFromConfig(streamConfig_);
> > +
> > +	arg->count = streamConfig_.bufferCount;
> > +	bufferCount_ = arg->count;
> > +
> > +	vcam_->invokeMethod(&V4L2Camera::allocBuffers,
> > +			    ConnectionTypeBlocking, &ret, arg->count);
> > +	if (ret < 0) {
> > +		arg->count = 0;
> > +		return ret == -EACCES ? -EBUSY : ret;
> 
> This is rightfully handled by the V4L2Camera already.
> 
> > +	}
> > +
> > +	for (unsigned int i = 0; i < arg->count; i++) {
> > +		struct v4l2_buffer buf;

	= { };

in order to clear all fields that you don't set explicitly.

> > +		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +		buf.length = curV4L2Format_.fmt.pix.sizeimage;
> > +		buf.memory = V4L2_MEMORY_MMAP;
> > +		buf.m.offset = i * curV4L2Format_.fmt.pix.sizeimage;

You should set buf.index = i too.

> > +
> > +		if (buffers_.size() <= i)
> > +			buffers_.resize(i + 1);

You can move this before the loop.

	buffers_.resize(arg->count);

> > +		buffers_[i] = buf;
> > +	}
> > +
> > +	LOG(V4L2Compat, Debug) << "Allocated " << arg->count << " buffers";
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf";
> > +	Stream *stream = streamConfig_.stream();
> > +
> > +	if (!validateStreamType(arg->type) ||
> > +	    arg->index >= stream->buffers().size())
> > +		return -EINVAL;
> > +
> > +	updateBuffers();
> 
> updateBuffers() retreives information on completed buffers, while
> querybuf can be called anytime after buffers have been requested.
> 
> From V4L2 specs:
> This ioctl is part of the streaming I/O method. It can be used to
> query the status of a buffer at any time after buffers have been
> allocated with the ioctl VIDIOC_REQBUFS ioctl.
> 
> It should thus depend on static information of allocated buffers not
> just for completed ones...
> 
> I suspect I am missing something, as this updateBuffers() thing
> looks weird to me both here and in dqbuf.

The idea is that the buffer status is kept in V4L2CameraProxy, and
updated with updateBuffers(). The method is called both at querybuf and
dqbuf time to ensure we have the latest status.

> > +
> > +	memcpy(arg, &buffers_[arg->index], sizeof(*arg));
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = "
> > +			       << arg->index;
> > +
> > +	Stream *stream = streamConfig_.stream();
> > +
> > +	if (!validateStreamType(arg->type) ||
> > +	    !validateMemoryType(arg->memory) ||
> > +	    arg->index >= stream->buffers().size())
> > +		return -EINVAL;
> > +
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,
> > +			    &ret, arg->index);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	arg->flags |= V4L2_BUF_FLAG_QUEUED;
> > +	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> > +
> > +	return ret;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf";
> > +
> > +	if (!validateStreamType(arg->type) ||
> > +	    !validateMemoryType(arg->memory))
> > +		return -EINVAL;
> > +
> > +	if (nonBlocking_ && !vcam_->bufferSema_.tryAcquire())
> > +		return -EAGAIN;
> > +	else
> > +		vcam_->bufferSema_.acquire();
> > +
> > +	updateBuffers();
> > +
> > +	struct v4l2_buffer &buf = buffers_[arg->index];
> 
> I'm not sure  this is right, index should be set by us, not received
> from the application, there is no guarantee it is valid at all, am I wrong ?

That's correct. This should be

	struct v4l2_buffer &buf = buffers_[currentBuf_];

> From V4L2 specs:
> Applications call the VIDIOC_DQBUF ioctl to dequeue a filled
> (capturing) or displayed (output) buffer from the driver’s outgoing
> queue. They just set the type, memory and reserved fields of a struct
> v4l2_buffer as above, when VIDIOC_DQBUF is called with a pointer to
> this structure the driver fills the remaining fields or returns an
> error code.
> 
> I'm not a huge fan of this whole updateBuffers() mechanism, I agree
> the V4L2Camera should keep a list of completed buffers, but why
> would we want to copy here information for all of them when we dequeue
> one only ?

For performance reasons, and because we need to do so for querybuf too
anyway.

> I would simply keep a list of completed buffers in V4L2Camera, and get
> the older on when dequeuing a buffer here.
> 
> > +
> > +	buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> > +	buf.length = sizeimage_;
> > +	memcpy(arg, &buf, sizeof(*arg));
> > +
> > +	arg->index = currentBuf_;

This isn't needed if you initialize the buffers_ entries with an index
in reqbufs.

> > +	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_streamon(int *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon";
> > +
> > +	if (!validateStreamType(*arg))
> > +		return -EINVAL;
> > +
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::streamOn,
> > +			    ConnectionTypeBlocking, &ret);
> 
> Also here, empty line before return like you do in most functions.
> 
> > +	return ret;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_streamoff(int *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff";
> > +
> > +	if (!validateStreamType(*arg))
> > +		return -EINVAL;
> > +
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::streamOff,
> > +			    ConnectionTypeBlocking, &ret);
> > +
> > +	for (struct v4l2_buffer &buf : buffers_)
> > +		buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> > +
> > +	return ret;
> > +}
> > +
> > +int V4L2CameraProxy::ioctl(unsigned long request, void *arg)
> > +{
> > +	int ret;
> > +	switch (request) {
> > +	case VIDIOC_QUERYCAP:
> > +		ret = vidioc_querycap(static_cast<struct v4l2_capability *>(arg));
> > +		break;
> > +	case VIDIOC_ENUM_FMT:
> > +		ret = vidioc_enum_fmt(static_cast<struct v4l2_fmtdesc *>(arg));
> > +		break;
> > +	case VIDIOC_G_FMT:
> > +		ret = vidioc_g_fmt(static_cast<struct v4l2_format *>(arg));
> > +		break;
> > +	case VIDIOC_S_FMT:
> > +		ret = vidioc_s_fmt(static_cast<struct v4l2_format *>(arg));
> > +		break;
> > +	case VIDIOC_TRY_FMT:
> > +		ret = vidioc_try_fmt(static_cast<struct v4l2_format *>(arg));
> > +		break;
> > +	case VIDIOC_REQBUFS:
> > +		ret = vidioc_reqbufs(static_cast<struct v4l2_requestbuffers *>(arg));
> > +		break;
> > +	case VIDIOC_QUERYBUF:
> > +		ret = vidioc_querybuf(static_cast<struct v4l2_buffer *>(arg));
> > +		break;
> > +	case VIDIOC_QBUF:
> > +		ret = vidioc_qbuf(static_cast<struct v4l2_buffer *>(arg));
> > +		break;
> > +	case VIDIOC_DQBUF:
> > +		ret = vidioc_dqbuf(static_cast<struct v4l2_buffer *>(arg));
> > +		break;
> > +	case VIDIOC_STREAMON:
> > +		ret = vidioc_streamon(static_cast<int *>(arg));
> > +		break;
> > +	case VIDIOC_STREAMOFF:
> > +		ret = vidioc_streamoff(static_cast<int *>(arg));
> > +		break;
> > +	default:
> > +		ret = -ENOTTY;
> > +		break;
> > +	}
> > +
> > +	if (ret < 0) {
> > +		errno = -ret;
> > +		return -1;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/* \todo make libcamera export these */
> > +int V4L2CameraProxy::bplMultiplier(unsigned int format)
> > +{
> > +	switch (format) {
> > +	case V4L2_PIX_FMT_NV12:
> > +	case V4L2_PIX_FMT_NV21:
> > +	case V4L2_PIX_FMT_NV16:
> > +	case V4L2_PIX_FMT_NV61:
> > +	case V4L2_PIX_FMT_NV24:
> > +	case V4L2_PIX_FMT_NV42:
> > +		return 1;
> > +	case V4L2_PIX_FMT_BGR24:
> > +	case V4L2_PIX_FMT_RGB24:
> > +		return 3;
> > +	case V4L2_PIX_FMT_ARGB32:
> > +		return 4;
> > +	case V4L2_PIX_FMT_VYUY:
> > +	case V4L2_PIX_FMT_YVYU:
> > +	case V4L2_PIX_FMT_UYVY:
> > +	case V4L2_PIX_FMT_YUYV:
> > +		return 2;
> > +	default:
> > +		return 0;
> > +	};
> > +}
> > +
> > +int V4L2CameraProxy::imageSize(unsigned int format,
> > +			       unsigned int width, unsigned int height)
> > +{
> > +	switch (format) {
> > +	case V4L2_PIX_FMT_NV12:
> > +	case V4L2_PIX_FMT_NV21:
> > +		return width * height + width * height / 2;
> > +	case V4L2_PIX_FMT_NV16:
> > +	case V4L2_PIX_FMT_NV61:
> > +		return width * height * 2;
> > +	case V4L2_PIX_FMT_NV24:
> > +	case V4L2_PIX_FMT_NV42:
> > +		return width * height * 3;
> > +	case V4L2_PIX_FMT_BGR24:
> > +	case V4L2_PIX_FMT_RGB24:
> > +		return width * height * 3;
> > +	case V4L2_PIX_FMT_ARGB32:
> > +		return width * height * 4;
> > +	case V4L2_PIX_FMT_VYUY:
> > +	case V4L2_PIX_FMT_YVYU:
> > +	case V4L2_PIX_FMT_UYVY:
> > +	case V4L2_PIX_FMT_YUYV:
> > +		return width * height * 2;
> > +	default:
> > +		return 0;
> > +	};
> > +}
> > +
> > +unsigned int V4L2CameraProxy::v4l2ToDrm(unsigned int pixelformat)
> > +{
> > +	switch (pixelformat) {
> > +	/* RGB formats. */
> > +	case V4L2_PIX_FMT_RGB24:
> > +		return DRM_FORMAT_BGR888;
> > +	case V4L2_PIX_FMT_BGR24:
> > +		return DRM_FORMAT_RGB888;
> > +	case V4L2_PIX_FMT_ARGB32:
> > +		return DRM_FORMAT_BGRA8888;
> > +
> > +	/* YUV packed formats. */
> > +	case V4L2_PIX_FMT_YUYV:
> > +		return DRM_FORMAT_YUYV;
> > +	case V4L2_PIX_FMT_YVYU:
> > +		return DRM_FORMAT_YVYU;
> > +	case V4L2_PIX_FMT_UYVY:
> > +		return DRM_FORMAT_UYVY;
> > +	case V4L2_PIX_FMT_VYUY:
> > +		return DRM_FORMAT_VYUY;
> > +
> > +	/* YUY planar formats. */
> > +	case V4L2_PIX_FMT_NV16:
> > +		return DRM_FORMAT_NV16;
> > +	case V4L2_PIX_FMT_NV61:
> > +		return DRM_FORMAT_NV61;
> > +	case V4L2_PIX_FMT_NV12:
> > +		return DRM_FORMAT_NV12;
> > +	case V4L2_PIX_FMT_NV21:
> > +		return DRM_FORMAT_NV21;
> > +	case V4L2_PIX_FMT_NV24:
> > +		return DRM_FORMAT_NV24;
> > +	case V4L2_PIX_FMT_NV42:
> > +		return DRM_FORMAT_NV42;
> > +	default:
> > +		return pixelformat;
> > +	};
> > +}
> > +
> > +unsigned int V4L2CameraProxy::drmToV4L2(unsigned int pixelformat)
> > +{
> > +	switch (pixelformat) {
> > +	/* RGB formats. */
> > +	case DRM_FORMAT_BGR888:
> > +		return V4L2_PIX_FMT_RGB24;
> > +	case DRM_FORMAT_RGB888:
> > +		return V4L2_PIX_FMT_BGR24;
> > +	case DRM_FORMAT_BGRA8888:
> > +		return V4L2_PIX_FMT_ARGB32;
> > +
> > +	/* YUV packed formats. */
> > +	case DRM_FORMAT_YUYV:
> > +		return V4L2_PIX_FMT_YUYV;
> > +	case DRM_FORMAT_YVYU:
> > +		return V4L2_PIX_FMT_YVYU;
> > +	case DRM_FORMAT_UYVY:
> > +		return V4L2_PIX_FMT_UYVY;
> > +	case DRM_FORMAT_VYUY:
> > +		return V4L2_PIX_FMT_VYUY;
> > +
> > +	/* YUY planar formats. */
> > +	case DRM_FORMAT_NV16:
> > +		return V4L2_PIX_FMT_NV16;
> > +	case DRM_FORMAT_NV61:
> > +		return V4L2_PIX_FMT_NV61;
> > +	case DRM_FORMAT_NV12:
> > +		return V4L2_PIX_FMT_NV12;
> > +	case DRM_FORMAT_NV21:
> > +		return V4L2_PIX_FMT_NV21;
> > +	case DRM_FORMAT_NV24:
> > +		return V4L2_PIX_FMT_NV24;
> > +	case DRM_FORMAT_NV42:
> > +		return V4L2_PIX_FMT_NV42;
> > +	default:
> > +		return pixelformat;
> > +	}
> > +}
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > new file mode 100644
> > index 00000000..b167c3f4
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -0,0 +1,82 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_camera_proxy.h - Proxy to V4L2 compatibility camera
> > + */
> > +
> > +#ifndef __V4L2_CAMERA_PROXY_H__
> > +#define __V4L2_CAMERA_PROXY_H__
> > +
> > +#include <linux/videodev2.h>
> > +#include <map>
> > +#include <memory>
> > +#include <sys/types.h>
> > +#include <vector>
> > +
> > +#include <libcamera/camera.h>
> > +
> > +#include "v4l2_camera.h"
> > +
> > +using namespace libcamera;
> > +
> > +class V4L2CameraProxy
> > +{
> > +public:
> > +	V4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera);
> > +
> > +	int open(bool nonBlocking);
> > +	void dup();
> > +	void close();
> > +	void *mmap(void *addr, size_t length, int prot, int flags,
> > +		   off_t offset);
> > +	int munmap(void *addr, size_t length);
> > +
> > +	int ioctl(unsigned long request, void *arg);
> > +
> > +	static int bplMultiplier(unsigned int format);
> > +	static int imageSize(unsigned int format, unsigned int width,
> > +			     unsigned int height);
> > +
> > +	static unsigned int v4l2ToDrm(unsigned int pixelformat);
> > +	static unsigned int drmToV4L2(unsigned int pixelformat);
> > +
> > +private:
> > +	bool validateStreamType(uint32_t type);
> > +	bool validateMemoryType(uint32_t memory);
> > +	void setFmtFromConfig(StreamConfiguration &streamConfig);
> > +	unsigned int calculateSizeImage(StreamConfiguration &streamConfig);
> > +	void querycap(std::shared_ptr<Camera> camera);
> > +	void updateBuffers();
> > +	int freeBuffers();
> > +
> > +	int vidioc_querycap(struct v4l2_capability *arg);
> > +	int vidioc_enum_fmt(struct v4l2_fmtdesc *arg);
> > +	int vidioc_g_fmt(struct v4l2_format *arg);
> > +	int vidioc_s_fmt(struct v4l2_format *arg);
> > +	int vidioc_try_fmt(struct v4l2_format *arg);
> > +	int vidioc_reqbufs(struct v4l2_requestbuffers *arg);
> > +	int vidioc_querybuf(struct v4l2_buffer *arg);
> > +	int vidioc_qbuf(struct v4l2_buffer *arg);
> > +	int vidioc_dqbuf(struct v4l2_buffer *arg);
> > +	int vidioc_streamon(int *arg);
> > +	int vidioc_streamoff(int *arg);
> > +
> > +	unsigned int refcount_;
> > +	unsigned int index_;
> > +	bool nonBlocking_;
> > +
> > +	struct v4l2_format curV4L2Format_;
> > +	StreamConfiguration streamConfig_;
> > +	struct v4l2_capability capabilities_;
> > +	unsigned int bufferCount_;
> > +	unsigned int currentBuf_;
> > +	unsigned int sizeimage_;
> > +
> > +	std::vector<struct v4l2_buffer> buffers_;
> > +	std::map<void *, unsigned int> mmaps_;
> > +
> > +	std::unique_ptr<V4L2Camera> vcam_;
> > +};
> > +
> > +#endif /* __V4L2_CAMERA_PROXY_H__ */
> > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp
> > new file mode 100644
> > index 00000000..7a184488
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_compat.cpp
> > @@ -0,0 +1,80 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_compat.cpp - V4L2 compatibility layer
> > + */
> > +
> > +#include "v4l2_compat_manager.h"
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <stdarg.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +
> > +#define LIBCAMERA_PUBLIC __attribute__((visibility("default")))
> > +
> > +using namespace libcamera;
> > +
> > +#define extract_va_arg(type, arg, last)	\
> > +{					\
> > +	va_list ap;			\
> > +	va_start(ap, last);		\
> > +	arg = va_arg(ap, type);		\
> > +	va_end(ap);			\
> > +}
> > +
> > +extern "C" {
> > +
> > +LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)
> > +{
> > +	mode_t mode = 0;
> > +	if (oflag & O_CREAT || oflag & O_TMPFILE)
> > +		extract_va_arg(mode_t, mode, oflag);
> > +
> > +	return V4L2CompatManager::instance()->openat(AT_FDCWD, path,
> > +						     oflag, mode);
> > +}
> > +
> > +LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)
> > +{
> > +	mode_t mode = 0;
> > +	if (oflag & O_CREAT || oflag & O_TMPFILE)
> > +		extract_va_arg(mode_t, mode, oflag);
> > +
> > +	return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);
> > +}
> > +
> > +LIBCAMERA_PUBLIC int dup(int oldfd)
> > +{
> > +	return V4L2CompatManager::instance()->dup(oldfd);
> > +}
> > +
> > +LIBCAMERA_PUBLIC int close(int fd)
> > +{
> > +	return V4L2CompatManager::instance()->close(fd);
> > +}
> > +
> > +LIBCAMERA_PUBLIC void *mmap(void *addr, size_t length, int prot, int flags,
> > +			    int fd, off_t offset)
> > +{
> > +	return V4L2CompatManager::instance()->mmap(addr, length, prot, flags,
> > +						   fd, offset);
> > +}
> > +
> > +LIBCAMERA_PUBLIC int munmap(void *addr, size_t length)
> > +{
> > +	return V4L2CompatManager::instance()->munmap(addr, length);
> > +}
> > +
> > +LIBCAMERA_PUBLIC int ioctl(int fd, unsigned long request, ...)
> > +{
> > +	void *arg;
> > +	extract_va_arg(void *, arg, request);
> > +
> > +	return V4L2CompatManager::instance()->ioctl(fd, request, arg);
> > +}
> > +
> > +}
> > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> > new file mode 100644
> > index 00000000..cb224ba6
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > @@ -0,0 +1,255 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_compat_manager.cpp - V4L2 compatibility manager
> > + */
> > +
> > +#include "v4l2_compat_manager.h"
> > +
> > +#include <dlfcn.h>
> > +#include <fcntl.h>
> > +#include <fstream>
> > +#include <map>
> > +#include <stdarg.h>
> > +#include <string.h>
> > +#include <sys/eventfd.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <sys/sysmacros.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/camera_manager.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "log.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DEFINE_CATEGORY(V4L2Compat)
> > +
> > +V4L2CompatManager::V4L2CompatManager()
> > +	: cm_(nullptr), initialized_(false)
> > +{
> > +	openat_func_ = (openat_func_t)dlsym(RTLD_NEXT, "openat");
> > +	dup_func_    = (dup_func_t   )dlsym(RTLD_NEXT, "dup");
> > +	close_func_  = (close_func_t )dlsym(RTLD_NEXT, "close");
> > +	ioctl_func_  = (ioctl_func_t )dlsym(RTLD_NEXT, "ioctl");
> > +	mmap_func_   = (mmap_func_t  )dlsym(RTLD_NEXT, "mmap");
> > +	munmap_func_ = (munmap_func_t)dlsym(RTLD_NEXT, "munmap");
> > +}
> > +
> > +V4L2CompatManager::~V4L2CompatManager()
> > +{
> > +	devices_.clear();
> > +	mmaps_.clear();
> > +
> > +	if (isRunning()) {
> > +		exit(0);
> > +		/* \todo Wait with a timeout, just in case. */
> > +		wait();
> > +	}
> > +}
> > +
> > +int V4L2CompatManager::init()
> > +{
> > +	start();
> > +
> > +	MutexLocker locker(mutex_);
> > +	cv_.wait(locker, [this] { return initialized_; });

I think the usual way to capture 'this' is

	cv_.wait(locker, [&] { return initialized_; });

> > +
> > +	return 0;
> > +}
> > +
> > +void V4L2CompatManager::run()
> > +{
> > +	cm_ = new CameraManager();
> > +
> > +	int ret = cm_->start();
> > +	if (ret) {
> > +		LOG(V4L2Compat, Error) << "Failed to start camera manager: "
> > +				       << strerror(-ret);
> > +		return;
> > +	}
> > +
> > +	LOG(V4L2Compat, Debug) << "Started camera manager";
> > +
> > +	/*
> > +	 * For each Camera registered in the system, a V4L2CameraProxy gets
> > +	 * created here to wrap a camera device.
> > +	 */
> > +	unsigned int index = 0;
> > +	for (auto &camera : cm_->cameras()) {
> > +		V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera);
> > +		proxies_.emplace_back(proxy);
> > +		++index;
> > +	}
> > +
> > +	/*
> > +	 * libcamera has been initialized. Unlock the init() caller as we're
> > +	 * now ready to handle calls from the framework.
> > +	 */
> > +	mutex_.lock();
> > +	initialized_ = true;
> > +	mutex_.unlock();
> > +	cv_.notify_one();
> > +
> > +	/* Now start processing events and messages. */
> > +	exec();
> > +
> > +	proxies_.clear();
> > +	cm_->stop();
> > +	delete cm_;
> > +	cm_ = nullptr;
> > +}
> > +
> > +V4L2CompatManager *V4L2CompatManager::instance()
> > +{
> > +	static V4L2CompatManager instance;
> > +	return &instance;
> > +}
> > +
> > +V4L2CameraProxy *V4L2CompatManager::getProxy(int fd)
> > +{
> > +	auto device = devices_.find(fd);
> > +	if (device == devices_.end())
> > +		return nullptr;
> > +
> > +	return device->second;
> > +}
> > +
> > +int V4L2CompatManager::getCameraIndex(int fd)
> > +{
> > +	struct stat statbuf;
> > +	int ret = fstat(fd, &statbuf);
> > +	if (ret < 0)
> > +		return -1;
> > +
> > +	unsigned int devMajor = major(statbuf.st_rdev);
> > +	unsigned int devMinor = minor(statbuf.st_rdev);
> > +
> > +	dev_t devnum = makedev(devMajor, devMinor);
> > +	std::shared_ptr<Camera> target = cm_->get(devnum);
> > +	if (!target)
> > +		return -1;
> > +
> > +	unsigned int index = 0;
> > +	for (auto &camera : cm_->cameras()) {
> > +		if (camera == target)
> > +			break;
> > +		++index;
> > +	}
> > +
> > +	if (index >= cm_->cameras().size())
> > +		return -1;
> > +
> > +	return index;
> > +}
> > +
> > +int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mode)
> > +{
> > +	int fd = openat_func_(dirfd, path, oflag, mode);
> > +	if (fd < 0)
> > +		return fd;
> > +
> > +	struct stat statbuf;
> > +	int ret = fstat(fd, &statbuf);
> > +	if (ret < 0 || (statbuf.st_mode & S_IFMT) != S_IFCHR ||
> > +	    major(statbuf.st_rdev) != 81)
> > +		return fd;
> > +
> > +	if (!isRunning())
> > +		init();
> > +
> > +	ret = getCameraIndex(fd);
> > +	if (ret < 0) {
> > +		LOG(V4L2Compat, Info) << "No camera found for " << path;
> > +		return fd;
> > +	}
> > +
> > +	close_func_(fd);
> > +
> > +	unsigned int camera_index = static_cast<unsigned int>(ret);
> > +
> > +	V4L2CameraProxy *proxy = proxies_[camera_index].get();
> > +	ret = proxy->open(oflag & O_NONBLOCK);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	int efd = eventfd(0, oflag & (O_CLOEXEC | O_NONBLOCK));
> > +	if (efd < 0)
> > +		return efd;
> > +
> > +	devices_.emplace(efd, proxy);
> > +
> > +	return efd;
> > +}
> > +
> > +int V4L2CompatManager::dup(int oldfd)
> > +{
> > +	int newfd = dup_func_(oldfd);

	if (newfd < 0)
		return newfd;

> > +
> > +	auto device = devices_.find(oldfd);
> > +	if (device != devices_.end()) {
> > +		V4L2CameraProxy *proxy = device->second;
> > +		devices_[newfd] = proxy;
> > +		proxy->dup();
> > +	}
> > +
> > +	return newfd;
> > +}
> > +
> > +int V4L2CompatManager::close(int fd)
> > +{
> > +	V4L2CameraProxy *proxy = getProxy(fd);
> > +	if (proxy) {
> > +		proxy->close();
> > +		devices_.erase(fd);
> > +		return 0;
> > +	}
> > +
> > +	return close_func_(fd);
> > +}
> > +
> > +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> > +			      int fd, off_t offset)
> > +{
> > +	V4L2CameraProxy *proxy = getProxy(fd);
> > +	if (!proxy)
> > +		return mmap_func_(addr, length, prot, flags, fd, offset);
> > +
> > +	void *map = proxy->mmap(addr, length, prot, flags, offset);
> > +	if (map == MAP_FAILED)
> > +		return map;
> > +
> > +	mmaps_[map] = proxy;
> > +	return map;
> > +}
> > +
> > +int V4L2CompatManager::munmap(void *addr, size_t length)
> > +{
> > +	auto device = mmaps_.find(addr);
> > +	if (device == mmaps_.end())
> > +		return munmap_func_(addr, length);
> > +
> > +	V4L2CameraProxy *proxy = device->second;
> > +
> > +	int ret = proxy->munmap(addr, length);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mmaps_.erase(device);
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)
> > +{
> > +	V4L2CameraProxy *proxy = getProxy(fd);
> > +	if (!proxy)
> > +		return ioctl_func_(fd, request, arg);
> > +
> > +	return proxy->ioctl(request, arg);
> > +}
> > diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h
> > new file mode 100644
> > index 00000000..13a240c4
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_compat_manager.h
> > @@ -0,0 +1,77 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_compat_manager.h - V4L2 compatibility manager
> > + */
> > +
> > +#ifndef __V4L2_COMPAT_MANAGER_H__
> > +#define __V4L2_COMPAT_MANAGER_H__
> > +
> > +#include <condition_variable>
> > +#include <fcntl.h>
> > +#include <map>
> > +#include <mutex>
> > +#include <sys/types.h>
> > +#include <vector>
> > +
> > +#include <libcamera/camera_manager.h>
> > +
> > +#include "thread.h"
> > +#include "v4l2_camera_proxy.h"
> > +
> > +using namespace libcamera;
> > +
> > +class V4L2CompatManager : public Thread
> > +{
> > +public:
> > +	static V4L2CompatManager *instance();
> > +
> > +	int init();
> > +
> > +	V4L2CameraProxy *getProxy(int fd);
> > +	V4L2CameraProxy *getCamera(void *addr);
> > +
> > +	int openat(int dirfd, const char *path, int oflag, mode_t mode);
> > +
> > +	int dup(int oldfd);
> > +	int close(int fd);
> > +	void *mmap(void *addr, size_t length, int prot, int flags,
> > +		   int fd, off_t offset);
> > +	int munmap(void *addr, size_t length);
> > +	int ioctl(int fd, unsigned long request, void *arg);
> > +
> > +private:
> > +	V4L2CompatManager();
> > +	~V4L2CompatManager();
> > +
> > +	void run() override;
> > +	int getCameraIndex(int fd);
> > +
> > +	typedef int (*openat_func_t)(int dirfd, const char *path, int oflag, ...);
> > +	typedef int (*dup_func_t)(int oldfd);
> > +	typedef int (*close_func_t)(int fd);
> > +	typedef int (*ioctl_func_t)(int fd, unsigned long request, ...);
> > +	typedef void *(*mmap_func_t)(void *addr, size_t length, int prot,
> > +				     int flags, int fd, off_t offset);
> > +	typedef int (*munmap_func_t)(void *addr, size_t length);
> > +
> > +	openat_func_t openat_func_;
> > +	dup_func_t    dup_func_;
> > +	close_func_t  close_func_;
> > +	ioctl_func_t  ioctl_func_;
> > +	mmap_func_t   mmap_func_;
> > +	munmap_func_t munmap_func_;
> > +
> > +	CameraManager *cm_;
> > +
> > +	std::mutex mutex_;
> > +	std::condition_variable cv_;
> > +	bool initialized_;
> > +
> > +	std::vector<std::unique_ptr<V4L2CameraProxy>> proxies_;
> > +	std::map<int, V4L2CameraProxy *> devices_;
> > +	std::map<void *, V4L2CameraProxy *> mmaps_;
> > +};
> > +
> > +#endif /* __V4L2_COMPAT_MANAGER_H__ */

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list