[libcamera-devel,2/2] v4l2: v4l2_compat: Add V4L2 compatibility layer

2407 diff mbox series

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.

Note that until we have a way of mapping V4L2 device nodes to libcamera
cameras, the V4L2 compatibility layer will always selects and use the
first enumerated libcamera camera.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 meson_options.txt                |   5 +
 src/meson.build                  |   4 +
 src/v4l2/meson.build             |  30 +++
 src/v4l2/v4l2_camera.cpp         | 312 ++++++++++++++++++++++
 src/v4l2/v4l2_camera.h           |  68 +++++
 src/v4l2/v4l2_camera_proxy.cpp   | 438 +++++++++++++++++++++++++++++++
 src/v4l2/v4l2_camera_proxy.h     |  63 +++++
 src/v4l2/v4l2_compat.cpp         |  81 ++++++
 src/v4l2/v4l2_compat_manager.cpp | 353 +++++++++++++++++++++++++
 src/v4l2/v4l2_compat_manager.h   |  91 +++++++
 10 files changed, 1445 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

Hi Paul,

On Fri, Dec 06, 2019 at 04:26:54AM -0500, Paul Elder wrote:
> Add libcamera V4L2 compatibility layer.

Maybe just me, but having this patch split in components would have
helped not having to digest 1445 lines of patch in one go

I know Niklas agrees :)

>
> 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.
>
> Note that until we have a way of mapping V4L2 device nodes to libcamera
> cameras, the V4L2 compatibility layer will always selects and use the
> first enumerated libcamera camera.

That's not trivial indeed...

For simple devices, we could easily collect the video nodes a camera
uses and match them with the one the v4l2 application tries to use.

For complex devices, where the DMA output node could be multiplexed by
different cameras depending on the ISP configuration, this is not
easy.

I presume v4l2 application are mostly meant to be used with simple
devices, so adding a list of V4L2Videodevices to the Camera (while a
bit of an hack) is totally doable. What do others think ?

>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  meson_options.txt                |   5 +
>  src/meson.build                  |   4 +
>  src/v4l2/meson.build             |  30 +++
>  src/v4l2/v4l2_camera.cpp         | 312 ++++++++++++++++++++++
>  src/v4l2/v4l2_camera.h           |  68 +++++
>  src/v4l2/v4l2_camera_proxy.cpp   | 438 +++++++++++++++++++++++++++++++
>  src/v4l2/v4l2_camera_proxy.h     |  63 +++++
>  src/v4l2/v4l2_compat.cpp         |  81 ++++++
>  src/v4l2/v4l2_compat_manager.cpp | 353 +++++++++++++++++++++++++
>  src/v4l2/v4l2_compat_manager.h   |  91 +++++++
>  10 files changed, 1445 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..b06dd494 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 libcamera with 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..d96db4ff
> --- /dev/null
> +++ b/src/v4l2/meson.build
> @@ -0,0 +1,30 @@
> +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',

Seems it is enough to undefine the _FILE_OFFSET_BITS macro to use the
32-bit interface ?

https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html
If _FILE_OFFSET_BITS is undefined, or if it is defined to the value
32, nothing changes. The 32 bit interface is used and types like off_t
have a size of 32 bits on 32 bit systems.

> +]
> +
> +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..c6c84ef3
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -0,0 +1,312 @@
> +/* 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 "v4l2_compat_manager.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(V4L2Compat);
> +
> +V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> +	: camera_(camera), bufferCount_(0), isRunning_(false), sequence_(0),
> +	  buffer_sema_(new Semaphore())

Can't this be a class member instead of a pointer ?
Also, I would name it differently, or at least make it cameraCase_

> +{
> +	camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> +};
> +
> +V4L2Camera::~V4L2Camera()
> +{
> +	while (!pendingRequests_.empty()) {
> +		delete pendingRequests_.front();
> +		pendingRequests_.pop();
> +	}
> +
> +	buffer_lock_.lock();
> +	while (!completedBuffers_.empty()) {
> +		delete completedBuffers_.front();
> +		completedBuffers_.pop();
> +	}
> +	buffer_lock_.unlock();
> +
> +	delete buffer_sema_;

you would save deleting it

> +
> +	camera_->release();
> +}
> +
> +void V4L2Camera::open(int *ret, bool nonblock)
> +{
> +	nonblock_ = nonblock;
> +
> +	if (camera_->acquire() < 0) {
> +		LOG(V4L2Compat, Error) << "Failed to acquire camera";
> +		*ret = -EINVAL;
> +		return;
> +	}
> +
> +	config_ = camera_->generateConfiguration({ StreamRole::Viewfinder });
> +	if (config_ == nullptr) {
> +		*ret = -EINVAL;
> +		return;
> +	}
> +
> +	*ret = 0;
> +}
> +
> +void V4L2Camera::close(int *ret)
> +{
> +	*ret = camera_->release();
> +}
> +
> +void V4L2Camera::getStreamConfig(StreamConfiguration *ret)

Maybe *streaConfig ?

> +{
> +	*ret = config_->at(0);
> +}
> +
> +void V4L2Camera::requestComplete(Request *request)
> +{
> +	if (request->status() == Request::RequestCancelled) {
> +		LOG(V4L2Compat, Error) << "Request not succesfully completed: "
> +				       << request->status();
> +		return;
> +	}
> +
> +	/* We only have one stream at the moment. */
> +	buffer_lock_.lock();
> +	Buffer *buffer = request->buffers().begin()->second;
> +	completedBuffers_.push(buffer);
> +	buffer_lock_.unlock();
> +
> +	buffer_sema_->release();
> +}
> +
> +void V4L2Camera::configure(int *ret, struct v4l2_format *arg,
> +			   unsigned int bufferCount)
> +{
> +	config_->at(0).size.width = arg->fmt.pix.width;

Nit: can you use a reference to config_->at(0) ?

> +	config_->at(0).size.height = arg->fmt.pix.height;
> +	config_->at(0).pixelFormat =
> +		V4L2CompatManager::v4l2_to_drm(arg->fmt.pix.pixelformat);
> +	bufferCount_ = bufferCount;
> +	config_->at(0).bufferCount = bufferCount_;
> +	/* \todo memoryType (interval vs external) */
> +
> +	CameraConfiguration::Status validation = config_->validate();
> +	if (validation == CameraConfiguration::Invalid) {
> +		LOG(V4L2Compat, Info) << "Configuration invalid";

that's probably an error, isn't it ?

> +		*ret = -EINVAL;
> +		return;
> +	}
> +	if (validation == CameraConfiguration::Adjusted)
> +		LOG(V4L2Compat, Info) << "Configuration adjusted";
> +
> +	LOG(V4L2Compat, Info) << "Validated configuration is: "
> +			      << config_->at(0).toString();
> +
> +	*ret = camera_->configure(config_.get());
> +	if (*ret < 0)
> +		return;
> +
> +	bufferCount_ = config_->at(0).bufferCount;
> +
> +	*ret = 0;
> +}
> +
> +void V4L2Camera::mmap(void **ret, void *addr, size_t length, int prot,
> +		      int flags, off_t offset)

Do we need to check flags ?

> +{
> +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> +
> +	if (prot != (PROT_READ | PROT_WRITE)) {
> +		errno = EINVAL;

This function (actually all V4L2Camera functions) is called through a method
invocation and has a long call stack before getting back to the
caller. I wonder if errno does not get overwritten along that route.

Also, from man mmap:

ENOTSUP
MAP_FIXED or MAP_PRIVATE was specified in the flags argument and the
implementation does not support this functionality.

The implementation does not support the combination of accesses
requested in the prot argument.

> +		*ret = MAP_FAILED;
> +		return;
> +	}
> +
> +	StreamConfiguration &streamConfig = config_->at(0);
> +	unsigned int sizeimage =
> +		V4L2CompatManager::image_size(
> +			V4L2CompatManager::drm_to_v4l2(streamConfig.pixelFormat),
> +			streamConfig.size.width,
> +			streamConfig.size.height);
> +	if (sizeimage == 0) {
> +		errno = EINVAL;
> +		*ret = MAP_FAILED;
> +		return;
> +	}
> +
> +	unsigned int index = offset / sizeimage;
> +	if (index * sizeimage != offset || length != sizeimage) {
> +		errno = EINVAL;

Is EINVAL the right error here?
from man mmap:

ENOMEM
MAP_FIXED  was  specified, and the range [addr,addr+len)
exceeds that allowed for the address space of a process; or, if
MAP_FIXED was not specified and there is insufficient room in the
address space to effect the map‐ ping

EOVERFLOW
The file is a regular file and the value of off plus len exceeds the
offset maximum established in the open file description associated
with fildes.

?

However I'm not sure we should care about the mmap errors at this
level, as those should be returned when the actual mapping is
performed. What do you think ?

> +		*ret = MAP_FAILED;
> +		return;
> +	}
> +
> +	Stream *stream = *camera_->streams().begin();
> +	*ret = stream->buffers()[index].planes()[0].mem();
> +}
> +
> +void V4L2Camera::munmap(int *ret, void *addr, size_t length)
> +{
> +	StreamConfiguration &streamConfig = config_->at(0);
> +	unsigned int sizeimage =
> +		V4L2CompatManager::image_size(streamConfig.pixelFormat,
> +					      streamConfig.size.width,
> +					      streamConfig.size.height);
> +
> +	if (length != sizeimage) {
> +		errno = EINVAL;

Here EINVAL seems to be appropriate

> +		*ret = -1;
> +		return;
> +	}
> +
> +	*ret = 0;
> +}
> +
> +void V4L2Camera::validStreamType(bool *ret, uint32_t type)
> +{
> +	*ret = (type == V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +}
> +
> +void V4L2Camera::validMemoryType(bool *ret, uint32_t memory)
> +{
> +	*ret = (memory == V4L2_MEMORY_MMAP);
> +}
> +
> +void V4L2Camera::allocBuffers(int *ret, unsigned int count)
> +{
> +	LOG(V4L2Compat, Debug) << "Allocating libcamera bufs";
> +	*ret = camera_->allocateBuffers();

I fear the buffer rework would required a big rebase of this series :(

> +	if (*ret == -EACCES)
> +		*ret = -EBUSY;
> +}
> +
> +/* \todo implement allocDMABuffers */
> +void V4L2Camera::allocDMABuffers(int *ret, unsigned int count)
> +{
> +	*ret = -ENOTTY;
> +}
> +
> +void V4L2Camera::freeBuffers()
> +{
> +	camera_->freeBuffers();
> +	bufferCount_ = 0;
> +}
> +
> +void V4L2Camera::streamOn(int *ret)
> +{
> +	if (isRunning_) {
> +		*ret = 0;
> +		return;
> +	}
> +
> +	sequence_ = 0;
> +
> +	*ret = camera_->start();
> +	if (*ret < 0)
> +		return;

errno ?

> +	isRunning_ = true;
> +
> +	while (!pendingRequests_.empty()) {
> +		*ret = camera_->queueRequest(pendingRequests_.front());
> +		pendingRequests_.pop();
> +		if (*ret < 0)
> +			return;
> +	}
> +
> +	*ret = 0;
> +}
> +
> +void V4L2Camera::streamOff(int *ret)
> +{
> +	/* \todo restore buffers to reqbufs state? */
> +	if (!isRunning_) {
> +		*ret = 0;
> +		return;
> +	}
> +
> +	*ret = camera_->stop();
> +	if (*ret < 0)
> +		return;
> +	isRunning_ = false;
> +
> +	*ret = 0;

Here and in streamOn you could set *ret = 0 at the beginning of the
function.

> +}
> +
> +void V4L2Camera::qbuf(int *ret, struct v4l2_buffer *arg)
> +{
> +	Stream *stream = config_->at(0).stream();
> +	std::unique_ptr<Buffer> buffer = stream->createBuffer(arg->index);
> +	if (!buffer) {
> +		LOG(V4L2Compat, Error) << "Can't create buffer";
> +		*ret = -ENOMEM;
> +		return;
> +	}
> +
> +	Request *request = camera_->createRequest();

paranoid: createRequest() can return nullptr

> +	*ret = request->addBuffer(std::move(buffer));
> +	if (*ret < 0) {
> +		LOG(V4L2Compat, Error) << "Can't set buffer for request";
> +		return;
> +	}
> +
> +	if (!isRunning_) {
> +		pendingRequests_.push(request);
> +	} else {
> +		*ret = camera_->queueRequest(request);
> +		if (*ret < 0) {
> +			LOG(V4L2Compat, Error) << "Can't queue request";
> +			if (*ret == -EACCES)
> +				*ret = -EBUSY;
> +			return;
> +		}
> +	}
> +
> +	arg->flags |= V4L2_BUF_FLAG_QUEUED;
> +	arg->flags |= V4L2_BUF_FLAG_MAPPED;
> +	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> +	*ret = 0;
> +}
> +
> +int V4L2Camera::dqbuf(struct v4l2_buffer *arg)
> +{
> +	if (nonblock_ && !buffer_sema_->tryAcquire())
> +		return -EAGAIN;
> +	else
> +		buffer_sema_->acquire();
> +
> +	buffer_lock_.lock();
> +	Buffer *buffer = completedBuffers_.front();
> +	completedBuffers_.pop();
> +	buffer_lock_.unlock();
> +
> +	arg->bytesused = buffer->bytesused();
> +	arg->field = V4L2_FIELD_NONE;
> +	arg->timestamp.tv_sec = buffer->timestamp() / 1000000000;
> +	arg->timestamp.tv_usec = buffer->timestamp() / 1000;
> +	arg->sequence = sequence_++;

Don't we have sequence in Buffer ?

> +
> +	arg->flags &= ~V4L2_BUF_FLAG_QUEUED;
> +	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> +
> +	StreamConfiguration &streamConfig = config_->at(0);
> +	unsigned int sizeimage =
> +		V4L2CompatManager::image_size(streamConfig.pixelFormat,
> +					      streamConfig.size.width,
> +					      streamConfig.size.height);
> +	arg->length = sizeimage;

You can save the sizeimage variable.
I wonder if this needs to be re-calculated everytime... nothing big
however...

> +
> +	return 0;
> +}
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> new file mode 100644
> index 00000000..3d3cd8ff
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera.h
> @@ -0,0 +1,68 @@
> +/* 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 <linux/videodev2.h>
> +#include <mutex>
> +#include <queue>
> +
> +#include <libcamera/camera.h>
> +#include "semaphore.h"
> +
> +using namespace libcamera;
> +
> +class V4L2Camera : public Object
> +{
> +public:
> +	V4L2Camera(std::shared_ptr<Camera> camera);
> +	~V4L2Camera();
> +
> +	void open(int *ret, bool nonblock);
> +	void close(int *ret);
> +	void getStreamConfig(StreamConfiguration *ret);
> +	void requestComplete(Request *request);
> +
> +	void mmap(void **ret, void *addr, size_t length, int prot, int flags,
> +		  off_t offset);
> +	void munmap(int *ret, void *addr, size_t length);
> +
> +	void configure(int *ret, struct v4l2_format *arg,
> +		       unsigned int bufferCount);
> +
> +	void validStreamType(bool *ret, uint32_t type);
> +	void validMemoryType(bool *ret, uint32_t memory);
> +
> +	void allocBuffers(int *ret, unsigned int count);
> +	void allocDMABuffers(int *ret, unsigned int count);
> +	void freeBuffers();
> +	void streamOn(int *ret);
> +	void streamOff(int *ret);
> +
> +	void qbuf(int *ret, struct v4l2_buffer *arg);
> +	int dqbuf(struct v4l2_buffer *arg);
> +
> +private:
> +	void initDefaultV4L2Format();
> +
> +	std::shared_ptr<Camera> camera_;
> +	std::unique_ptr<CameraConfiguration> config_;
> +
> +	unsigned int bufferCount_;
> +	bool isRunning_;
> +	bool nonblock_;
> +
> +	unsigned int sequence_;
> +
> +	Semaphore *buffer_sema_;
> +	std::mutex buffer_lock_;
> +
> +	std::queue<Request *> pendingRequests_;
> +	std::queue<Buffer *> 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..1dd2c363
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -0,0 +1,438 @@
> +/* 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/videodev2.h>
> +#include <string.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)
> +	: index_(index), bufferCount_(0), currentBuf_(0),
> +	  vcam_(utils::make_unique<V4L2Camera>(camera))
> +{
> +	querycap(camera);
> +}
> +
> +V4L2CameraProxy::~V4L2CameraProxy()
> +{
> +	vcam_.reset();

Not sure it is necessary to reset a unique_ptr<> at destruction time,
the managed pointer will be destroyed anyway.

> +}
> +
> +int V4L2CameraProxy::open(bool nonblock)
> +{
> +	int ret;
> +	vcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,
> +			    &ret, nonblock);
> +	if (ret < 0)
> +		return ret;
> +
> +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> +			    ConnectionTypeBlocking, &streamConfig_);
> +	setFmtFromConfig();
> +
> +	return 0;
> +}
> +
> +int V4L2CameraProxy::close()
> +{
> +	int ret;
> +	vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);
> +	return ret;
> +}
> +
> +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> +			    off_t offset)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> +
> +	void *val;
> +	vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
> +			    &val, addr, length, prot, flags, offset);
> +	return val;
> +}
> +
> +int V4L2CameraProxy::munmap(void *addr, size_t length)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing MUNMAP";
> +
> +	int ret;
> +	vcam_->invokeMethod(&V4L2Camera::munmap, ConnectionTypeBlocking,
> +			    &ret, addr, length);
> +	return ret;
> +}
> +
> +bool V4L2CameraProxy::hasPixelFormat(unsigned int format)
> +{
> +	const std::vector<PixelFormat> &formats =
> +		streamConfig_.formats().pixelformats();
> +	return std::find(formats.begin(), formats.end(), format) != formats.end();
> +}
> +
> +/* \todo getDeviceCaps? getMemoryCaps? */
> +
> +bool V4L2CameraProxy::hasSize(unsigned int format, Size size)
> +{
> +	int len = streamConfig_.formats().sizes(format).size();
> +	for (int i = 0; i < len; i++)
> +		if (streamConfig_.formats().sizes(format)[i] == size)
> +			return true;

Can't you find() on the vector<Size> returned by
streamConfig_.formats().sizes(format) ?

> +
> +	return false;
> +}
> +
> +bool V4L2CameraProxy::validateStreamType(uint32_t type)
> +{
> +	bool valid;
> +	vcam_->invokeMethod(&V4L2Camera::validStreamType,
> +			    ConnectionTypeBlocking, &valid, type);
> +	if (!valid)
> +		return false;
> +
> +	return true;
> +}
> +
> +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> +{
> +	bool valid;
> +	vcam_->invokeMethod(&V4L2Camera::validMemoryType,
> +			    ConnectionTypeBlocking, &valid, memory);
> +	if (!valid)
> +		return false;
> +
> +	return true;

In this two functions you can here just return 'valid'

> +}
> +
> +void V4L2CameraProxy::setFmtFromConfig()
> +{
> +	curV4L2Format_.fmt.pix.width = streamConfig_.size.width;
> +	curV4L2Format_.fmt.pix.height = streamConfig_.size.height;
> +	curV4L2Format_.fmt.pix.pixelformat =
> +		V4L2CompatManager::drm_to_v4l2(streamConfig_.pixelFormat);
> +	curV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;
> +	curV4L2Format_.fmt.pix.bytesperline =
> +		V4L2CompatManager::bpl_multiplier(
> +			curV4L2Format_.fmt.pix.pixelformat) *
> +		curV4L2Format_.fmt.pix.width;
> +	curV4L2Format_.fmt.pix.sizeimage =
> +		V4L2CompatManager::image_size(curV4L2Format_.fmt.pix.pixelformat,
> +					      curV4L2Format_.fmt.pix.width,
> +					      curV4L2Format_.fmt.pix.height);
> +	curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> +}
> +
> +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));
> +	capabilities_.version = KERNEL_VERSION(5, 2, 0);
> +	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;

Are we single planar only ? I guess so, it's fine :)

> +	capabilities_.capabilities =
> +		capabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;
> +	memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
> +}
> +
> +
> +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))
> +		return -EINVAL;
> +	if (arg->index > streamConfig_.formats().pixelformats().size())
> +		return -EINVAL;
> +
> +	memcpy(arg->description, "asdf", 5);

That's a real nice format name! :D
Do we need a map of formats to descriptions ?

> +	arg->pixelformat =
> +		V4L2CompatManager::drm_to_v4l2(
> +			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;
> +
> +	arg->fmt.pix.width        = curV4L2Format_.fmt.pix.width;
> +	arg->fmt.pix.height       = curV4L2Format_.fmt.pix.height;
> +	arg->fmt.pix.pixelformat  = curV4L2Format_.fmt.pix.pixelformat;
> +	arg->fmt.pix.field        = curV4L2Format_.fmt.pix.field;
> +	arg->fmt.pix.bytesperline = curV4L2Format_.fmt.pix.bytesperline;
> +	arg->fmt.pix.sizeimage    = curV4L2Format_.fmt.pix.sizeimage;
> +	arg->fmt.pix.colorspace   = curV4L2Format_.fmt.pix.colorspace;
> +
> +	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;
> +
> +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> +			    &ret, arg, bufferCount_);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> +			    ConnectionTypeBlocking, &streamConfig_);
> +	setFmtFromConfig();
> +
> +	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;
> +	if (!hasPixelFormat(format))
> +		format = streamConfig_.formats().pixelformats()[0];
> +
> +	Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> +	if (!hasSize(format, size))
> +		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 =
> +		V4L2CompatManager::bpl_multiplier(
> +			V4L2CompatManager::drm_to_v4l2(format)) *
> +		arg->fmt.pix.width;
> +	arg->fmt.pix.sizeimage    =
> +		V4L2CompatManager::image_size(
> +			V4L2CompatManager::drm_to_v4l2(format),
> +			arg->fmt.pix.width, arg->fmt.pix.height);
> +	arg->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;
> +
> +	return 0;
> +}
> +
> +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing VIDIOC_REQBUFS";
> +	if (!validateStreamType(arg->type))
> +		return -EINVAL;
> +	if (!validateMemoryType(arg->memory))
> +		return -EINVAL;
> +
> +	LOG(V4L2Compat, Debug) << arg->count << " bufs requested ";
> +
> +	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> +
> +	if (arg->count == 0) {
> +		LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> +		int ret;
> +		vcam_->invokeMethod(&V4L2Camera::streamOff,
> +				    ConnectionTypeBlocking, &ret);
> +		vcam_->invokeMethod(&V4L2Camera::freeBuffers,
> +				    ConnectionTypeBlocking);
> +		bufferCount_ = 0;
> +		return 0;
> +	}
> +
> +	int ret;

as ret is used function-wise, I would its declaration to the beginning

> +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> +			    &ret, &curV4L2Format_, arg->count);
> +	if (ret < 0)
> +		return -EINVAL;
> +	arg->count = streamConfig_.bufferCount;
> +	bufferCount_ = arg->count;
> +
> +	ret = -EINVAL;
> +	if (arg->memory == V4L2_MEMORY_MMAP)
> +		vcam_->invokeMethod(&V4L2Camera::allocBuffers,
> +				    ConnectionTypeBlocking, &ret, arg->count);
> +	else if (arg->memory == V4L2_MEMORY_DMABUF)

Do we even claim support for this ?

> +		vcam_->invokeMethod(&V4L2Camera::allocDMABuffers,
> +				    ConnectionTypeBlocking, &ret, arg->count);
> +
> +	if (ret < 0) {
> +		arg->count = 0;
> +		return ret == -EACCES ? -EBUSY : ret;
> +	}
> +
> +	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))
> +		return -EINVAL;
> +	if (arg->index >= stream->buffers().size())
> +		return -EINVAL;
> +
> +	unsigned int index = arg->index;
> +	memset(arg, 0, sizeof(*arg));
> +	arg->index = index;
> +	arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	arg->length = curV4L2Format_.fmt.pix.sizeimage;
> +	arg->memory = V4L2_MEMORY_MMAP;
> +	arg->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;
> +
> +	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))
> +		return -EINVAL;
> +	if (!validateMemoryType(arg->memory))
> +		return -EINVAL;
> +	if (arg->index >= stream->buffers().size())
> +		return -EINVAL;
> +
> +	int ret;
> +	vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,
> +			    &ret, arg);
> +	return ret;
> +}
> +
> +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing VIDIOC_DQBUF";
> +
> +	if (!validateStreamType(arg->type))
> +		return -EINVAL;
> +	if (!validateMemoryType(arg->memory))
> +		return -EINVAL;
> +
> +	arg->index = currentBuf_;
> +	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> +
> +	return vcam_->dqbuf(arg);

Is dqbuf special ?

I know it could return immediately if nonblock_ is set, but
invokeMethod checks that invoked method is called, if it returns
immediately, that's fine. Or have I missed some other reason why this
is called directly.

> +}
> +
> +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);
> +	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);
> +	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));

camelCase for method names as well ?

> +		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;
> +	case VIDIOC_EXPBUF:
> +	case VIDIOC_ENUM_FRAMESIZES:
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +	if (ret < 0) {
> +		errno = ret;
> +		return -1;
> +	}
> +
> +	errno = 0;
> +	return ret;
> +
> +}
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> new file mode 100644
> index 00000000..64c7aadd
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -0,0 +1,63 @@
> +/* 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 <libcamera/camera.h>
> +
> +#include "v4l2_camera.h"
> +
> +using namespace libcamera;
> +
> +class V4L2CameraProxy
> +{
> +public:
> +	V4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera);
> +	~V4L2CameraProxy();
> +
> +	int open(bool nonblock);
> +	int 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);
> +
> +private:
> +	bool hasPixelFormat(unsigned int format);
> +	bool hasSize(unsigned int format, Size size);
> +	bool validateStreamType(uint32_t type);
> +	bool validateMemoryType(uint32_t memory);
> +	void setFmtFromConfig();
> +	void querycap(std::shared_ptr<Camera> camera);
> +
> +	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 index_;
> +
> +	struct v4l2_format curV4L2Format_;
> +	StreamConfiguration streamConfig_;
> +	struct v4l2_capability capabilities_;
> +	unsigned int bufferCount_;
> +	unsigned int currentBuf_;
> +
> +	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..3330e7bc
> --- /dev/null
> +++ b/src/v4l2/v4l2_compat.cpp
> @@ -0,0 +1,81 @@
> +/* 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 <iostream>
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/videodev2.h>
> +#include <stdarg.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +
> +#define LIBCAMERA_PUBLIC __attribute__((visibility("default")))

Am I wrong or this makes sense only if we compile with
-fvisiblity=hidden ?
https://gcc.gnu.org/wiki/Visibility

I welcome this change, but then probably you should add that
compilation flag to the v4l2 compat library, it I have not
mis-interpreted the above wiki page

> +
> +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 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)
> +{
> +	void *val = V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);
> +	return val;
> +}
> +
> +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..4eeb714f
> --- /dev/null
> +++ b/src/v4l2/v4l2_compat_manager.cpp
> @@ -0,0 +1,353 @@
> +/* 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 <iostream>
> +#include <linux/drm_fourcc.h>
> +#include <linux/videodev2.h>
> +#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()
> +	: cameraManagerRunning_(false), cm_(nullptr)
> +{
> +	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");

You seem to be mixing cameraCase and snake_case here and there in
variable declaration. Was this intentional ?

> +}
> +
> +V4L2CompatManager::~V4L2CompatManager()
> +{
> +	devices_.clear();
> +
> +	if (isRunning()) {
> +		exit(0);
> +		/* \todo Wait with a timeout, just in case. */
> +		wait();
> +	}
> +}
> +
> +int V4L2CompatManager::init()
> +{
> +	start();
> +
> +	MutexLocker locker(mutex_);
> +	cv_.wait(locker);
> +
> +	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 wraps a camera device.
> +	 */
> +	// \todo map v4l2 devices to libcamera cameras; minor device node?
> +	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.
> +	 */
> +	cv_.notify_one();
> +
> +	/* Now start processing events and messages. */
> +	exec();
> +
> +	cm_->stop();
> +	delete cm_;
> +	cm_ = nullptr;
> +}
> +
> +V4L2CompatManager *V4L2CompatManager::instance()
> +{
> +	static V4L2CompatManager v4l2CompatManager;
> +	return &v4l2CompatManager;
> +}
> +
> +bool V4L2CompatManager::validfd(int fd)
> +{
> +	return devices_.find(fd) != devices_.end();
> +}
> +
> +bool V4L2CompatManager::validmmap(void *addr)
> +{
> +	return mmaps_.find(addr) != mmaps_.end();
> +}
> +
> +std::shared_ptr<V4L2CameraProxy> V4L2CompatManager::getCamera(int fd)
> +{
> +	if (!validfd(fd))
> +		return nullptr;
> +
> +	return devices_.at(fd);

This is safe, but it's a double lookup, same as below. This is minor
indeed, but probably using the iterator returned from find() directly
is slightly more efficient.

Then you can inline the valid[fd|mmap}() methods, probably

> +}
> +
> +std::shared_ptr<V4L2CameraProxy> V4L2CompatManager::getCamera(void *addr)
> +{
> +	if (!validmmap(addr))
> +		return nullptr;
> +
> +	return devices_.at(mmaps_.at(addr));
> +}
> +
> +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;
> +
> +	if (std::string(path).find("video") == std::string::npos)
> +		return fd;
> +
> +	if (!isRunning())
> +		init();
> +
> +	/* \todo map v4l2 devnodes to libcamera cameras */
> +	unsigned int camera_index = 0;
> +
> +	std::shared_ptr<V4L2CameraProxy> proxy = proxies_[camera_index];
> +	int ret = proxy->open(mode & O_NONBLOCK);
> +	if (ret < 0)
> +		return ret;
> +
> +	int efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));
> +	if (efd < 0)
> +		return errno;

I'm not sure I get this usage of the file descriptor returned by
eventfd...

It is used as index in the map, possibly replaced by dup(). I would
have expected the fd returned by openat() to be used as index in the
map... What am I missing ?

> +
> +	devices_.emplace(efd, proxy);
> +
> +	return efd;
> +}
> +
> +int V4L2CompatManager::dup(int oldfd)
> +{
> +	int newfd = dup_func_(oldfd);
> +	if (validfd(oldfd))

Shouldn't you return an error if the fd to duplicate is not valid, ad
dup() only if it is ?

> +		devices_[newfd] = devices_[oldfd];
> +
> +	return newfd;
> +}
> +
> +int V4L2CompatManager::close(int fd)
> +{
> +	if (validfd(fd) && devices_[fd]->close() < 0)

I would return -EIO if !validfd() and propagate the error up if
close() fails.

> +		return -EIO;
> +
> +	return close_func_(fd);
> +}
> +
> +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> +			      int fd, off_t offset)
> +{
> +	if (!validfd(fd))
> +		return mmap_func_(addr, length, prot, flags, fd, offset);
> +
> +	void *map = getCamera(fd)->mmap(addr, length, prot, flags, offset);
> +	if (map == MAP_FAILED) {
> +		errno = EINVAL;
> +		return map;
> +	}
> +
> +	mmaps_[addr] = fd;
> +	return map;
> +}
> +
> +int V4L2CompatManager::munmap(void *addr, size_t length)
> +{
> +	if (!validmmap(addr))
> +		return munmap_func_(addr, length);
> +
> +	int ret = getCamera(addr)->munmap(addr, length);
> +	if (ret < 0)
> +		return ret;
> +
> +	mmaps_.erase(addr);
> +	addr = nullptr;
> +
> +	return 0;
> +}
> +
> +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)
> +{
> +	std::shared_ptr<V4L2CameraProxy> proxy = getCamera(fd);
> +	if (!proxy)
> +		return ioctl_func_(fd, request, arg);
> +

What is the use case for bypassing the proxy ? That might be my
limited knowledge of how v4l2 application behave

> +	return proxy->ioctl(request, arg);
> +}
> +
> +/* \todo make libcamera export these */

mmm, V4L2VideoDevice has very similar format conversion routines... we
should really centralize them somehow.. maybe not in scope for this
patch, but replicating cose which is tricky to get right due to the
different format definition between DRM and V4L2 is not a good idea.

> +int V4L2CompatManager::bpl_multiplier(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 V4L2CompatManager::image_size(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 V4L2CompatManager::v4l2_to_drm(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 V4L2CompatManager::drm_to_v4l2(unsigned int pixelformat)
> +{
> +	switch (pixelformat) {
> +	/* RGB formats. */
> +	case DRM_FORMAT_BGR888:
> +		return V4L2_PIX_FMT_BGR24;

This in example does not match the one we have in V4L2Videodevice.
DRM_FORMAT_BGR24 = V4L2_PIX_FMT_RGB24 not BGR24.

I know, it's a pain.

I think the other version is correct, as we validated them using
conversion routines from kernel drivers...

There might be more below (and possibly above)

> +	case DRM_FORMAT_RGB888:
> +		return V4L2_PIX_FMT_RGB24;
> +	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_compat_manager.h b/src/v4l2/v4l2_compat_manager.h
> new file mode 100644
> index 00000000..492f97fd
> --- /dev/null
> +++ b/src/v4l2/v4l2_compat_manager.h
> @@ -0,0 +1,91 @@
> +/* 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 <linux/videodev2.h>
> +#include <map>
> +#include <mutex>
> +#include <queue>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +#include <vector>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/stream.h>
> +
> +#include "thread.h"
> +#include "v4l2_camera_proxy.h"
> +
> +using namespace libcamera;
> +
> +class V4L2CompatManager : public Thread
> +{
> +public:
> +	static V4L2CompatManager *instance();
> +
> +	int init();
> +
> +	int openat(int dirfd, const char *path, int oflag, mode_t mode);
> +
> +	std::shared_ptr<V4L2CameraProxy> getCamera(int fd);
> +	std::shared_ptr<V4L2CameraProxy> getCamera(void *addr);

Every time you return a shared_ptr by value, you increase the
reference count for no good reason. I would either return by reference
or return V4L2CameraProxy and use shared pointers only in the proxies_
array and not in the rest of the code base.

Overall this is a very good first submission and I'm happy to see it
posted to the list \o/

I admit in this first pass I didn't go into extreme detail on the way
the v4l2 operations semantic is handled, but it seems sane to me!

Thanks
   j

> +
> +	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);
> +
> +	static int bpl_multiplier(unsigned int format);
> +	static int image_size(unsigned int format, unsigned int width,
> +			      unsigned int height);
> +
> +	static unsigned int v4l2_to_drm(unsigned int pixelformat);
> +	static unsigned int drm_to_v4l2(unsigned int pixelformat);
> +
> +private:
> +	V4L2CompatManager();
> +	~V4L2CompatManager();
> +
> +	void run() override;
> +
> +	bool validfd(int fd);
> +	bool validmmap(void *addr);
> +
> +	int default_ioctl(int fd, unsigned long request, void *arg);
> +
> +	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_;
> +
> +	bool cameraManagerRunning_;
> +	CameraManager *cm_;
> +
> +	std::mutex mutex_;
> +	std::condition_variable cv_;
> +
> +	std::vector<std::shared_ptr<V4L2CameraProxy>> proxies_;
> +	std::map<int, std::shared_ptr<V4L2CameraProxy>> devices_;
> +	std::map<void *, int> mmaps_;
> +};
> +
> +#endif /* __V4L2_COMPAT_MANAGER_H__ */
> --
> 2.23.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the review.

On Fri, Dec 06, 2019 at 05:12:03PM +0100, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Fri, Dec 06, 2019 at 04:26:54AM -0500, Paul Elder wrote:
> > Add libcamera V4L2 compatibility layer.
> 
> Maybe just me, but having this patch split in components would have
> helped not having to digest 1445 lines of patch in one go

Laurent said it was okay... :)

> I know Niklas agrees :)
> 
> >
> > 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.
> >
> > Note that until we have a way of mapping V4L2 device nodes to libcamera
> > cameras, the V4L2 compatibility layer will always selects and use the
> > first enumerated libcamera camera.
> 
> That's not trivial indeed...
> 
> For simple devices, we could easily collect the video nodes a camera
> uses and match them with the one the v4l2 application tries to use.
> 
> For complex devices, where the DMA output node could be multiplexed by
> different cameras depending on the ISP configuration, this is not
> easy.
> 
> I presume v4l2 application are mostly meant to be used with simple
> devices, so adding a list of V4L2Videodevices to the Camera (while a
> bit of an hack) is totally doable. What do others think ?

I'm thinking that it's not a good idea to expose anything V4L2 from
Camera. If it's just UVC devices, might we match instead by the name
exposed by sysfs? The uvcvideo pipeline handler names the Camera based
on that name anyway.

> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  meson_options.txt                |   5 +
> >  src/meson.build                  |   4 +
> >  src/v4l2/meson.build             |  30 +++
> >  src/v4l2/v4l2_camera.cpp         | 312 ++++++++++++++++++++++
> >  src/v4l2/v4l2_camera.h           |  68 +++++
> >  src/v4l2/v4l2_camera_proxy.cpp   | 438 +++++++++++++++++++++++++++++++
> >  src/v4l2/v4l2_camera_proxy.h     |  63 +++++
> >  src/v4l2/v4l2_compat.cpp         |  81 ++++++
> >  src/v4l2/v4l2_compat_manager.cpp | 353 +++++++++++++++++++++++++
> >  src/v4l2/v4l2_compat_manager.h   |  91 +++++++
> >  10 files changed, 1445 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..b06dd494 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 libcamera with 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..d96db4ff
> > --- /dev/null
> > +++ b/src/v4l2/meson.build
> > @@ -0,0 +1,30 @@
> > +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',
> 
> Seems it is enough to undefine the _FILE_OFFSET_BITS macro to use the
> 32-bit interface ?

Looks like it is.

> https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html
> If _FILE_OFFSET_BITS is undefined, or if it is defined to the value
> 32, nothing changes. The 32 bit interface is used and types like off_t
> have a size of 32 bits on 32 bit systems.
> 
> > +]
> > +
> > +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..c6c84ef3
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -0,0 +1,312 @@
> > +/* 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 "v4l2_compat_manager.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(V4L2Compat);
> > +
> > +V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> > +	: camera_(camera), bufferCount_(0), isRunning_(false), sequence_(0),
> > +	  buffer_sema_(new Semaphore())
> 
> Can't this be a class member instead of a pointer ?
> Also, I would name it differently, or at least make it cameraCase_

ack

> > +{
> > +	camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> > +};
> > +
> > +V4L2Camera::~V4L2Camera()
> > +{
> > +	while (!pendingRequests_.empty()) {
> > +		delete pendingRequests_.front();
> > +		pendingRequests_.pop();
> > +	}
> > +
> > +	buffer_lock_.lock();
> > +	while (!completedBuffers_.empty()) {
> > +		delete completedBuffers_.front();
> > +		completedBuffers_.pop();
> > +	}
> > +	buffer_lock_.unlock();
> > +
> > +	delete buffer_sema_;
> 
> you would save deleting it
> 
> > +
> > +	camera_->release();
> > +}
> > +
> > +void V4L2Camera::open(int *ret, bool nonblock)
> > +{
> > +	nonblock_ = nonblock;
> > +
> > +	if (camera_->acquire() < 0) {
> > +		LOG(V4L2Compat, Error) << "Failed to acquire camera";
> > +		*ret = -EINVAL;
> > +		return;
> > +	}
> > +
> > +	config_ = camera_->generateConfiguration({ StreamRole::Viewfinder });
> > +	if (config_ == nullptr) {
> > +		*ret = -EINVAL;
> > +		return;
> > +	}
> > +
> > +	*ret = 0;
> > +}
> > +
> > +void V4L2Camera::close(int *ret)
> > +{
> > +	*ret = camera_->release();
> > +}
> > +
> > +void V4L2Camera::getStreamConfig(StreamConfiguration *ret)
> 
> Maybe *streaConfig ?

ack

> > +{
> > +	*ret = config_->at(0);
> > +}
> > +
> > +void V4L2Camera::requestComplete(Request *request)
> > +{
> > +	if (request->status() == Request::RequestCancelled) {
> > +		LOG(V4L2Compat, Error) << "Request not succesfully completed: "
> > +				       << request->status();
> > +		return;
> > +	}
> > +
> > +	/* We only have one stream at the moment. */
> > +	buffer_lock_.lock();
> > +	Buffer *buffer = request->buffers().begin()->second;
> > +	completedBuffers_.push(buffer);
> > +	buffer_lock_.unlock();
> > +
> > +	buffer_sema_->release();
> > +}
> > +
> > +void V4L2Camera::configure(int *ret, struct v4l2_format *arg,
> > +			   unsigned int bufferCount)
> > +{
> > +	config_->at(0).size.width = arg->fmt.pix.width;
> 
> Nit: can you use a reference to config_->at(0) ?

ack

> > +	config_->at(0).size.height = arg->fmt.pix.height;
> > +	config_->at(0).pixelFormat =
> > +		V4L2CompatManager::v4l2_to_drm(arg->fmt.pix.pixelformat);
> > +	bufferCount_ = bufferCount;
> > +	config_->at(0).bufferCount = bufferCount_;
> > +	/* \todo memoryType (interval vs external) */
> > +
> > +	CameraConfiguration::Status validation = config_->validate();
> > +	if (validation == CameraConfiguration::Invalid) {
> > +		LOG(V4L2Compat, Info) << "Configuration invalid";
> 
> that's probably an error, isn't it ?

Yep.

> > +		*ret = -EINVAL;
> > +		return;
> > +	}
> > +	if (validation == CameraConfiguration::Adjusted)
> > +		LOG(V4L2Compat, Info) << "Configuration adjusted";
> > +
> > +	LOG(V4L2Compat, Info) << "Validated configuration is: "
> > +			      << config_->at(0).toString();
> > +
> > +	*ret = camera_->configure(config_.get());
> > +	if (*ret < 0)
> > +		return;
> > +
> > +	bufferCount_ = config_->at(0).bufferCount;
> > +
> > +	*ret = 0;
> > +}
> > +
> > +void V4L2Camera::mmap(void **ret, void *addr, size_t length, int prot,
> > +		      int flags, off_t offset)
> 
> Do we need to check flags ?

No.

> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> > +
> > +	if (prot != (PROT_READ | PROT_WRITE)) {
> > +		errno = EINVAL;
> 
> This function (actually all V4L2Camera functions) is called through a method
> invocation and has a long call stack before getting back to the
> caller. I wonder if errno does not get overwritten along that route.

You're right; it doesn't seem to be kept. The caller still receives an
EINVAL though (but only EINVAL).

I looked back at this and I think I messed up some errno passing.

> Also, from man mmap:
> 
> ENOTSUP
> MAP_FIXED or MAP_PRIVATE was specified in the flags argument and the
> implementation does not support this functionality.
>
> The implementation does not support the combination of accesses
> requested in the prot argument.

I don't see this in my man mmap... oh wait I found it in the posix man.

Yeah, this is probably better.

> 
> > +		*ret = MAP_FAILED;
> > +		return;
> > +	}
> > +
> > +	StreamConfiguration &streamConfig = config_->at(0);
> > +	unsigned int sizeimage =
> > +		V4L2CompatManager::image_size(
> > +			V4L2CompatManager::drm_to_v4l2(streamConfig.pixelFormat),
> > +			streamConfig.size.width,
> > +			streamConfig.size.height);
> > +	if (sizeimage == 0) {
> > +		errno = EINVAL;
> > +		*ret = MAP_FAILED;
> > +		return;
> > +	}
> > +
> > +	unsigned int index = offset / sizeimage;
> > +	if (index * sizeimage != offset || length != sizeimage) {
> > +		errno = EINVAL;
> 
> Is EINVAL the right error here?
> from man mmap:
> 
> ENOMEM
> MAP_FIXED  was  specified, and the range [addr,addr+len)
> exceeds that allowed for the address space of a process; or, if
> MAP_FIXED was not specified and there is insufficient room in the
> address space to effect the mapping

I don't think this is right, as ENOMEM indicates some form of
"out of memory", but we are checking for alignment.

> EOVERFLOW
> The file is a regular file and the value of off plus len exceeds the
> offset maximum established in the open file description associated
> with fildes.

Similar issue here.

> However I'm not sure we should care about the mmap errors at this
> level, as those should be returned when the actual mapping is
> performed. What do you think ?

I think we should already return the errors at this point, especially
since these are all input validation for the mmap for V4L2 devices.

> > +		*ret = MAP_FAILED;
> > +		return;
> > +	}
> > +
> > +	Stream *stream = *camera_->streams().begin();
> > +	*ret = stream->buffers()[index].planes()[0].mem();
> > +}
> > +
> > +void V4L2Camera::munmap(int *ret, void *addr, size_t length)
> > +{
> > +	StreamConfiguration &streamConfig = config_->at(0);
> > +	unsigned int sizeimage =
> > +		V4L2CompatManager::image_size(streamConfig.pixelFormat,
> > +					      streamConfig.size.width,
> > +					      streamConfig.size.height);
> > +
> > +	if (length != sizeimage) {
> > +		errno = EINVAL;
> 
> Here EINVAL seems to be appropriate
> 
> > +		*ret = -1;
> > +		return;
> > +	}
> > +
> > +	*ret = 0;
> > +}
> > +
> > +void V4L2Camera::validStreamType(bool *ret, uint32_t type)
> > +{
> > +	*ret = (type == V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > +}
> > +
> > +void V4L2Camera::validMemoryType(bool *ret, uint32_t memory)
> > +{
> > +	*ret = (memory == V4L2_MEMORY_MMAP);
> > +}
> > +
> > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Allocating libcamera bufs";
> > +	*ret = camera_->allocateBuffers();
> 
> I fear the buffer rework would required a big rebase of this series :(

Oh no :(

> > +	if (*ret == -EACCES)
> > +		*ret = -EBUSY;
> > +}
> > +
> > +/* \todo implement allocDMABuffers */
> > +void V4L2Camera::allocDMABuffers(int *ret, unsigned int count)
> > +{
> > +	*ret = -ENOTTY;
> > +}
> > +
> > +void V4L2Camera::freeBuffers()
> > +{
> > +	camera_->freeBuffers();
> > +	bufferCount_ = 0;
> > +}
> > +
> > +void V4L2Camera::streamOn(int *ret)
> > +{
> > +	if (isRunning_) {
> > +		*ret = 0;
> > +		return;
> > +	}
> > +
> > +	sequence_ = 0;
> > +
> > +	*ret = camera_->start();
> > +	if (*ret < 0)
> > +		return;
> 
> errno ?

Yes.

I'm going to shift things around so that V4L2Camera can return the
errnos directly, and ethen either V4L2CameraProxy or V4L2CompatManager
can set the errno - that should fix the errno being lost in the call
stack too.

> > +	isRunning_ = true;
> > +
> > +	while (!pendingRequests_.empty()) {
> > +		*ret = camera_->queueRequest(pendingRequests_.front());
> > +		pendingRequests_.pop();
> > +		if (*ret < 0)
> > +			return;
> > +	}
> > +
> > +	*ret = 0;
> > +}
> > +
> > +void V4L2Camera::streamOff(int *ret)
> > +{
> > +	/* \todo restore buffers to reqbufs state? */
> > +	if (!isRunning_) {
> > +		*ret = 0;
> > +		return;
> > +	}
> > +
> > +	*ret = camera_->stop();
> > +	if (*ret < 0)
> > +		return;
> > +	isRunning_ = false;
> > +
> > +	*ret = 0;
> 
> Here and in streamOn you could set *ret = 0 at the beginning of the
> function.

I originally had to to be analogous to "return 0", but it's duplicated
in if (!isRunning_) so I suppose setting it at the beginning is better.

> > +}
> > +
> > +void V4L2Camera::qbuf(int *ret, struct v4l2_buffer *arg)
> > +{
> > +	Stream *stream = config_->at(0).stream();
> > +	std::unique_ptr<Buffer> buffer = stream->createBuffer(arg->index);
> > +	if (!buffer) {
> > +		LOG(V4L2Compat, Error) << "Can't create buffer";
> > +		*ret = -ENOMEM;
> > +		return;
> > +	}
> > +
> > +	Request *request = camera_->createRequest();
> 
> paranoid: createRequest() can return nullptr

Oh no :o

> > +	*ret = request->addBuffer(std::move(buffer));
> > +	if (*ret < 0) {
> > +		LOG(V4L2Compat, Error) << "Can't set buffer for request";
> > +		return;
> > +	}
> > +
> > +	if (!isRunning_) {
> > +		pendingRequests_.push(request);
> > +	} else {
> > +		*ret = camera_->queueRequest(request);
> > +		if (*ret < 0) {
> > +			LOG(V4L2Compat, Error) << "Can't queue request";
> > +			if (*ret == -EACCES)
> > +				*ret = -EBUSY;
> > +			return;
> > +		}
> > +	}
> > +
> > +	arg->flags |= V4L2_BUF_FLAG_QUEUED;
> > +	arg->flags |= V4L2_BUF_FLAG_MAPPED;
> > +	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> > +	*ret = 0;
> > +}
> > +
> > +int V4L2Camera::dqbuf(struct v4l2_buffer *arg)
> > +{
> > +	if (nonblock_ && !buffer_sema_->tryAcquire())
> > +		return -EAGAIN;
> > +	else
> > +		buffer_sema_->acquire();
> > +
> > +	buffer_lock_.lock();
> > +	Buffer *buffer = completedBuffers_.front();
> > +	completedBuffers_.pop();
> > +	buffer_lock_.unlock();
> > +
> > +	arg->bytesused = buffer->bytesused();
> > +	arg->field = V4L2_FIELD_NONE;
> > +	arg->timestamp.tv_sec = buffer->timestamp() / 1000000000;
> > +	arg->timestamp.tv_usec = buffer->timestamp() / 1000;
> > +	arg->sequence = sequence_++;
> 
> Don't we have sequence in Buffer ?

Oops, yeah we do.

> > +
> > +	arg->flags &= ~V4L2_BUF_FLAG_QUEUED;
> > +	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> > +
> > +	StreamConfiguration &streamConfig = config_->at(0);
> > +	unsigned int sizeimage =
> > +		V4L2CompatManager::image_size(streamConfig.pixelFormat,
> > +					      streamConfig.size.width,
> > +					      streamConfig.size.height);
> > +	arg->length = sizeimage;
> 
> You can save the sizeimage variable.
> I wonder if this needs to be re-calculated everytime... nothing big
> however...

Okay.

> > +
> > +	return 0;
> > +}
> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > new file mode 100644
> > index 00000000..3d3cd8ff
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera.h
> > @@ -0,0 +1,68 @@
> > +/* 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 <linux/videodev2.h>
> > +#include <mutex>
> > +#include <queue>
> > +
> > +#include <libcamera/camera.h>
> > +#include "semaphore.h"
> > +
> > +using namespace libcamera;
> > +
> > +class V4L2Camera : public Object
> > +{
> > +public:
> > +	V4L2Camera(std::shared_ptr<Camera> camera);
> > +	~V4L2Camera();
> > +
> > +	void open(int *ret, bool nonblock);
> > +	void close(int *ret);
> > +	void getStreamConfig(StreamConfiguration *ret);
> > +	void requestComplete(Request *request);
> > +
> > +	void mmap(void **ret, void *addr, size_t length, int prot, int flags,
> > +		  off_t offset);
> > +	void munmap(int *ret, void *addr, size_t length);
> > +
> > +	void configure(int *ret, struct v4l2_format *arg,
> > +		       unsigned int bufferCount);
> > +
> > +	void validStreamType(bool *ret, uint32_t type);
> > +	void validMemoryType(bool *ret, uint32_t memory);
> > +
> > +	void allocBuffers(int *ret, unsigned int count);
> > +	void allocDMABuffers(int *ret, unsigned int count);
> > +	void freeBuffers();
> > +	void streamOn(int *ret);
> > +	void streamOff(int *ret);
> > +
> > +	void qbuf(int *ret, struct v4l2_buffer *arg);
> > +	int dqbuf(struct v4l2_buffer *arg);
> > +
> > +private:
> > +	void initDefaultV4L2Format();
> > +
> > +	std::shared_ptr<Camera> camera_;
> > +	std::unique_ptr<CameraConfiguration> config_;
> > +
> > +	unsigned int bufferCount_;
> > +	bool isRunning_;
> > +	bool nonblock_;
> > +
> > +	unsigned int sequence_;
> > +
> > +	Semaphore *buffer_sema_;
> > +	std::mutex buffer_lock_;
> > +
> > +	std::queue<Request *> pendingRequests_;
> > +	std::queue<Buffer *> 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..1dd2c363
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -0,0 +1,438 @@
> > +/* 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/videodev2.h>
> > +#include <string.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)
> > +	: index_(index), bufferCount_(0), currentBuf_(0),
> > +	  vcam_(utils::make_unique<V4L2Camera>(camera))
> > +{
> > +	querycap(camera);
> > +}
> > +
> > +V4L2CameraProxy::~V4L2CameraProxy()
> > +{
> > +	vcam_.reset();
> 
> Not sure it is necessary to reset a unique_ptr<> at destruction time,
> the managed pointer will be destroyed anyway.

Ah, right.

> > +}
> > +
> > +int V4L2CameraProxy::open(bool nonblock)
> > +{
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,
> > +			    &ret, nonblock);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> > +			    ConnectionTypeBlocking, &streamConfig_);
> > +	setFmtFromConfig();
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::close()
> > +{
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);
> > +	return ret;
> > +}
> > +
> > +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > +			    off_t offset)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> > +
> > +	void *val;
> > +	vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
> > +			    &val, addr, length, prot, flags, offset);
> > +	return val;
> > +}
> > +
> > +int V4L2CameraProxy::munmap(void *addr, size_t length)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing MUNMAP";
> > +
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::munmap, ConnectionTypeBlocking,
> > +			    &ret, addr, length);
> > +	return ret;
> > +}
> > +
> > +bool V4L2CameraProxy::hasPixelFormat(unsigned int format)
> > +{
> > +	const std::vector<PixelFormat> &formats =
> > +		streamConfig_.formats().pixelformats();
> > +	return std::find(formats.begin(), formats.end(), format) != formats.end();
> > +}
> > +
> > +/* \todo getDeviceCaps? getMemoryCaps? */
> > +
> > +bool V4L2CameraProxy::hasSize(unsigned int format, Size size)
> > +{
> > +	int len = streamConfig_.formats().sizes(format).size();
> > +	for (int i = 0; i < len; i++)
> > +		if (streamConfig_.formats().sizes(format)[i] == size)
> > +			return true;
> 
> Can't you find() on the vector<Size> returned by
> streamConfig_.formats().sizes(format) ?

Yes.

> > +
> > +	return false;
> > +}
> > +
> > +bool V4L2CameraProxy::validateStreamType(uint32_t type)
> > +{
> > +	bool valid;
> > +	vcam_->invokeMethod(&V4L2Camera::validStreamType,
> > +			    ConnectionTypeBlocking, &valid, type);
> > +	if (!valid)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> > +{
> > +	bool valid;
> > +	vcam_->invokeMethod(&V4L2Camera::validMemoryType,
> > +			    ConnectionTypeBlocking, &valid, memory);
> > +	if (!valid)
> > +		return false;
> > +
> > +	return true;
> 
> In this two functions you can here just return 'valid'

Why?

> > +}
> > +
> > +void V4L2CameraProxy::setFmtFromConfig()
> > +{
> > +	curV4L2Format_.fmt.pix.width = streamConfig_.size.width;
> > +	curV4L2Format_.fmt.pix.height = streamConfig_.size.height;
> > +	curV4L2Format_.fmt.pix.pixelformat =
> > +		V4L2CompatManager::drm_to_v4l2(streamConfig_.pixelFormat);
> > +	curV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;
> > +	curV4L2Format_.fmt.pix.bytesperline =
> > +		V4L2CompatManager::bpl_multiplier(
> > +			curV4L2Format_.fmt.pix.pixelformat) *
> > +		curV4L2Format_.fmt.pix.width;
> > +	curV4L2Format_.fmt.pix.sizeimage =
> > +		V4L2CompatManager::image_size(curV4L2Format_.fmt.pix.pixelformat,
> > +					      curV4L2Format_.fmt.pix.width,
> > +					      curV4L2Format_.fmt.pix.height);
> > +	curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > +}
> > +
> > +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));
> > +	capabilities_.version = KERNEL_VERSION(5, 2, 0);
> > +	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;
> 
> Are we single planar only ? I guess so, it's fine :)

Yeah :)

> > +	capabilities_.capabilities =
> > +		capabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;
> > +	memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
> > +}
> > +
> > +
> > +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))
> > +		return -EINVAL;
> > +	if (arg->index > streamConfig_.formats().pixelformats().size())
> > +		return -EINVAL;
> > +
> > +	memcpy(arg->description, "asdf", 5);
> 
> That's a real nice format name! :D
> Do we need a map of formats to descriptions ?

Thanks! :D
We might.

> > +	arg->pixelformat =
> > +		V4L2CompatManager::drm_to_v4l2(
> > +			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;
> > +
> > +	arg->fmt.pix.width        = curV4L2Format_.fmt.pix.width;
> > +	arg->fmt.pix.height       = curV4L2Format_.fmt.pix.height;
> > +	arg->fmt.pix.pixelformat  = curV4L2Format_.fmt.pix.pixelformat;
> > +	arg->fmt.pix.field        = curV4L2Format_.fmt.pix.field;
> > +	arg->fmt.pix.bytesperline = curV4L2Format_.fmt.pix.bytesperline;
> > +	arg->fmt.pix.sizeimage    = curV4L2Format_.fmt.pix.sizeimage;
> > +	arg->fmt.pix.colorspace   = curV4L2Format_.fmt.pix.colorspace;
> > +
> > +	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;
> > +
> > +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > +			    &ret, arg, bufferCount_);
> > +	if (ret < 0)
> > +		return -EINVAL;
> > +
> > +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> > +			    ConnectionTypeBlocking, &streamConfig_);
> > +	setFmtFromConfig();
> > +
> > +	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;
> > +	if (!hasPixelFormat(format))
> > +		format = streamConfig_.formats().pixelformats()[0];
> > +
> > +	Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> > +	if (!hasSize(format, size))
> > +		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 =
> > +		V4L2CompatManager::bpl_multiplier(
> > +			V4L2CompatManager::drm_to_v4l2(format)) *
> > +		arg->fmt.pix.width;
> > +	arg->fmt.pix.sizeimage    =
> > +		V4L2CompatManager::image_size(
> > +			V4L2CompatManager::drm_to_v4l2(format),
> > +			arg->fmt.pix.width, arg->fmt.pix.height);
> > +	arg->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing VIDIOC_REQBUFS";
> > +	if (!validateStreamType(arg->type))
> > +		return -EINVAL;
> > +	if (!validateMemoryType(arg->memory))
> > +		return -EINVAL;
> > +
> > +	LOG(V4L2Compat, Debug) << arg->count << " bufs requested ";
> > +
> > +	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > +
> > +	if (arg->count == 0) {
> > +		LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> > +		int ret;
> > +		vcam_->invokeMethod(&V4L2Camera::streamOff,
> > +				    ConnectionTypeBlocking, &ret);
> > +		vcam_->invokeMethod(&V4L2Camera::freeBuffers,
> > +				    ConnectionTypeBlocking);
> > +		bufferCount_ = 0;
> > +		return 0;
> > +	}
> > +
> > +	int ret;
> 
> as ret is used function-wise, I would its declaration to the beginning

ack

> > +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > +			    &ret, &curV4L2Format_, arg->count);
> > +	if (ret < 0)
> > +		return -EINVAL;
> > +	arg->count = streamConfig_.bufferCount;
> > +	bufferCount_ = arg->count;
> > +
> > +	ret = -EINVAL;
> > +	if (arg->memory == V4L2_MEMORY_MMAP)
> > +		vcam_->invokeMethod(&V4L2Camera::allocBuffers,
> > +				    ConnectionTypeBlocking, &ret, arg->count);
> > +	else if (arg->memory == V4L2_MEMORY_DMABUF)
> 
> Do we even claim support for this ?

I was going to but then didn't... and removed it and didn't remove it in
other places.

> > +		vcam_->invokeMethod(&V4L2Camera::allocDMABuffers,
> > +				    ConnectionTypeBlocking, &ret, arg->count);
> > +
> > +	if (ret < 0) {
> > +		arg->count = 0;
> > +		return ret == -EACCES ? -EBUSY : ret;
> > +	}
> > +
> > +	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))
> > +		return -EINVAL;
> > +	if (arg->index >= stream->buffers().size())
> > +		return -EINVAL;
> > +
> > +	unsigned int index = arg->index;
> > +	memset(arg, 0, sizeof(*arg));
> > +	arg->index = index;
> > +	arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +	arg->length = curV4L2Format_.fmt.pix.sizeimage;
> > +	arg->memory = V4L2_MEMORY_MMAP;
> > +	arg->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;
> > +
> > +	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))
> > +		return -EINVAL;
> > +	if (!validateMemoryType(arg->memory))
> > +		return -EINVAL;
> > +	if (arg->index >= stream->buffers().size())
> > +		return -EINVAL;
> > +
> > +	int ret;
> > +	vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,
> > +			    &ret, arg);
> > +	return ret;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing VIDIOC_DQBUF";
> > +
> > +	if (!validateStreamType(arg->type))
> > +		return -EINVAL;
> > +	if (!validateMemoryType(arg->memory))
> > +		return -EINVAL;
> > +
> > +	arg->index = currentBuf_;
> > +	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> > +
> > +	return vcam_->dqbuf(arg);
> 
> Is dqbuf special ?

Yes.

> I know it could return immediately if nonblock_ is set, but
> invokeMethod checks that invoked method is called, if it returns
> immediately, that's fine. Or have I missed some other reason why this
> is called directly.

Here we are waiting for the frames to be produced, which must be in a
different thread from where the frames are produced.

Otherwise (speaking from experience) we either busy wait in V4L2Camera,
or the frame can't become available because we're blocked waiting for it
to become available.

> > +}
> > +
> > +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);
> > +	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);
> > +	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));
> 
> camelCase for method names as well ?

I suppose... I liked that the vidiocs mirrored the ioctl identifiers
though...
Should I do camelCase instead?

vidiocQueryCap
vidiocEnumFmt
vidiocGFmt
vidiocTryFmt
vidiocReqBufs
vidiocStreamOn

What do you think?

> > +		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;
> > +	case VIDIOC_EXPBUF:
> > +	case VIDIOC_ENUM_FRAMESIZES:
> > +	default:
> > +		ret = -ENOTTY;
> > +	}
> > +
> > +	if (ret < 0) {
> > +		errno = ret;
> > +		return -1;
> > +	}
> > +
> > +	errno = 0;
> > +	return ret;
> > +
> > +}
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > new file mode 100644
> > index 00000000..64c7aadd
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -0,0 +1,63 @@
> > +/* 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 <libcamera/camera.h>
> > +
> > +#include "v4l2_camera.h"
> > +
> > +using namespace libcamera;
> > +
> > +class V4L2CameraProxy
> > +{
> > +public:
> > +	V4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera);
> > +	~V4L2CameraProxy();
> > +
> > +	int open(bool nonblock);
> > +	int 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);
> > +
> > +private:
> > +	bool hasPixelFormat(unsigned int format);
> > +	bool hasSize(unsigned int format, Size size);
> > +	bool validateStreamType(uint32_t type);
> > +	bool validateMemoryType(uint32_t memory);
> > +	void setFmtFromConfig();
> > +	void querycap(std::shared_ptr<Camera> camera);
> > +
> > +	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 index_;
> > +
> > +	struct v4l2_format curV4L2Format_;
> > +	StreamConfiguration streamConfig_;
> > +	struct v4l2_capability capabilities_;
> > +	unsigned int bufferCount_;
> > +	unsigned int currentBuf_;
> > +
> > +	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..3330e7bc
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_compat.cpp
> > @@ -0,0 +1,81 @@
> > +/* 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 <iostream>
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <linux/videodev2.h>
> > +#include <stdarg.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +
> > +#define LIBCAMERA_PUBLIC __attribute__((visibility("default")))
> 
> Am I wrong or this makes sense only if we compile with
> -fvisiblity=hidden ?
> https://gcc.gnu.org/wiki/Visibility

It might be. When I tried it in the beginning with the compile options
that we already had, the symbols weren't being exported.

> I welcome this change, but then probably you should add that
> compilation flag to the v4l2 compat library, it I have not
> mis-interpreted the above wiki page

Okay.

> > +
> > +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 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)
> > +{
> > +	void *val = V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);
> > +	return val;
> > +}
> > +
> > +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..4eeb714f
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > @@ -0,0 +1,353 @@
> > +/* 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 <iostream>
> > +#include <linux/drm_fourcc.h>
> > +#include <linux/videodev2.h>
> > +#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()
> > +	: cameraManagerRunning_(false), cm_(nullptr)
> > +{
> > +	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");
> 
> You seem to be mixing cameraCase and snake_case here and there in
> variable declaration. Was this intentional ?

It was intentional, until I found the few that were unintentional.

These ones, plus the vidioc ones are the only ones that I intended to be
snake_case and thought was borderline acceptable.

> > +}
> > +
> > +V4L2CompatManager::~V4L2CompatManager()
> > +{
> > +	devices_.clear();
> > +
> > +	if (isRunning()) {
> > +		exit(0);
> > +		/* \todo Wait with a timeout, just in case. */
> > +		wait();
> > +	}
> > +}
> > +
> > +int V4L2CompatManager::init()
> > +{
> > +	start();
> > +
> > +	MutexLocker locker(mutex_);
> > +	cv_.wait(locker);
> > +
> > +	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 wraps a camera device.
> > +	 */
> > +	// \todo map v4l2 devices to libcamera cameras; minor device node?
> > +	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.
> > +	 */
> > +	cv_.notify_one();
> > +
> > +	/* Now start processing events and messages. */
> > +	exec();
> > +
> > +	cm_->stop();
> > +	delete cm_;
> > +	cm_ = nullptr;
> > +}
> > +
> > +V4L2CompatManager *V4L2CompatManager::instance()
> > +{
> > +	static V4L2CompatManager v4l2CompatManager;
> > +	return &v4l2CompatManager;
> > +}
> > +
> > +bool V4L2CompatManager::validfd(int fd)
> > +{
> > +	return devices_.find(fd) != devices_.end();
> > +}
> > +
> > +bool V4L2CompatManager::validmmap(void *addr)
> > +{
> > +	return mmaps_.find(addr) != mmaps_.end();
> > +}
> > +
> > +std::shared_ptr<V4L2CameraProxy> V4L2CompatManager::getCamera(int fd)
> > +{
> > +	if (!validfd(fd))
> > +		return nullptr;
> > +
> > +	return devices_.at(fd);
> 
> This is safe, but it's a double lookup, same as below. This is minor
> indeed, but probably using the iterator returned from find() directly
> is slightly more efficient.

How can I use the iterator directly?

When we intercept the calls and check if we should simply forward the
call, I think it's simpler to have just a validfd() call... but then
when we do need to process it then it does become a double (triple?)
lookup... or just check getCamera() == nullptr instead of validfd().

> Then you can inline the valid[fd|mmap}() methods, probably
> 
> > +}
> > +
> > +std::shared_ptr<V4L2CameraProxy> V4L2CompatManager::getCamera(void *addr)
> > +{
> > +	if (!validmmap(addr))
> > +		return nullptr;
> > +
> > +	return devices_.at(mmaps_.at(addr));
> > +}
> > +
> > +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;
> > +
> > +	if (std::string(path).find("video") == std::string::npos)
> > +		return fd;
> > +
> > +	if (!isRunning())
> > +		init();
> > +
> > +	/* \todo map v4l2 devnodes to libcamera cameras */
> > +	unsigned int camera_index = 0;
> > +
> > +	std::shared_ptr<V4L2CameraProxy> proxy = proxies_[camera_index];
> > +	int ret = proxy->open(mode & O_NONBLOCK);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	int efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));
> > +	if (efd < 0)
> > +		return errno;
> 
> I'm not sure I get this usage of the file descriptor returned by
> eventfd...
> 
> It is used as index in the map, possibly replaced by dup(). I would
> have expected the fd returned by openat() to be used as index in the
> map... What am I missing ?

The fd returned by eventfd reserves the fd so that any other open()
call doesn't overwrite/conflict with our fd. This fd is indeed used as
an index in the map (see next line), and dup() doesn't replace it.

> > +
> > +	devices_.emplace(efd, proxy);
> > +
> > +	return efd;
> > +}
> > +
> > +int V4L2CompatManager::dup(int oldfd)
> > +{
> > +	int newfd = dup_func_(oldfd);
> > +	if (validfd(oldfd))
> 
> Shouldn't you return an error if the fd to duplicate is not valid, ad
> dup() only if it is ?

If the fd is not valid, then it means the fd is not for us, and we need
to forward the dup() call. So in any case, we need to dup(). The only
difference is if we need to save the new fd in the mapping or not.

> > +		devices_[newfd] = devices_[oldfd];
> > +
> > +	return newfd;
> > +}
> > +
> > +int V4L2CompatManager::close(int fd)
> > +{
> > +	if (validfd(fd) && devices_[fd]->close() < 0)
> 
> I would return -EIO if !validfd() and propagate the error up if
> close() fails.

If !validfd() then the fd isn't for us, and the call needs to be
forwarded. -EIO is only if *our* close() fails.

> > +		return -EIO;
> > +
> > +	return close_func_(fd);
> > +}
> > +
> > +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> > +			      int fd, off_t offset)
> > +{
> > +	if (!validfd(fd))
> > +		return mmap_func_(addr, length, prot, flags, fd, offset);
> > +
> > +	void *map = getCamera(fd)->mmap(addr, length, prot, flags, offset);
> > +	if (map == MAP_FAILED) {
> > +		errno = EINVAL;
> > +		return map;
> > +	}
> > +
> > +	mmaps_[addr] = fd;
> > +	return map;
> > +}
> > +
> > +int V4L2CompatManager::munmap(void *addr, size_t length)
> > +{
> > +	if (!validmmap(addr))
> > +		return munmap_func_(addr, length);
> > +
> > +	int ret = getCamera(addr)->munmap(addr, length);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mmaps_.erase(addr);
> > +	addr = nullptr;
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)
> > +{
> > +	std::shared_ptr<V4L2CameraProxy> proxy = getCamera(fd);
> > +	if (!proxy)
> > +		return ioctl_func_(fd, request, arg);
> > +
> 
> What is the use case for bypassing the proxy ? That might be my
> limited knowledge of how v4l2 application behave

Perhaps. Here I check if proxy == nullptr, but in other places I checked
!validfd(fd) (I'll make these consistent). If the fd is not valid = the
proxy doesn't exist for the fd, then the fd is not for us and we need to
forward it to the original call.

> > +	return proxy->ioctl(request, arg);
> > +}
> > +
> > +/* \todo make libcamera export these */
> 
> mmm, V4L2VideoDevice has very similar format conversion routines... we
> should really centralize them somehow.. maybe not in scope for this
> patch, but replicating cose which is tricky to get right due to the
> different format definition between DRM and V4L2 is not a good idea.

I know...

> > +int V4L2CompatManager::bpl_multiplier(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 V4L2CompatManager::image_size(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 V4L2CompatManager::v4l2_to_drm(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 V4L2CompatManager::drm_to_v4l2(unsigned int pixelformat)
> > +{
> > +	switch (pixelformat) {
> > +	/* RGB formats. */
> > +	case DRM_FORMAT_BGR888:
> > +		return V4L2_PIX_FMT_BGR24;
> 
> This in example does not match the one we have in V4L2Videodevice.
> DRM_FORMAT_BGR24 = V4L2_PIX_FMT_RGB24 not BGR24.

Oh wat :/
Great.

> I know, it's a pain.
> 
> I think the other version is correct, as we validated them using
> conversion routines from kernel drivers...
> 
> There might be more below (and possibly above)
> 
> > +	case DRM_FORMAT_RGB888:
> > +		return V4L2_PIX_FMT_RGB24;
> > +	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_compat_manager.h b/src/v4l2/v4l2_compat_manager.h
> > new file mode 100644
> > index 00000000..492f97fd
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_compat_manager.h
> > @@ -0,0 +1,91 @@
> > +/* 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 <linux/videodev2.h>
> > +#include <map>
> > +#include <mutex>
> > +#include <queue>
> > +#include <sys/syscall.h>
> > +#include <unistd.h>
> > +#include <vector>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/camera_manager.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "thread.h"
> > +#include "v4l2_camera_proxy.h"
> > +
> > +using namespace libcamera;
> > +
> > +class V4L2CompatManager : public Thread
> > +{
> > +public:
> > +	static V4L2CompatManager *instance();
> > +
> > +	int init();
> > +
> > +	int openat(int dirfd, const char *path, int oflag, mode_t mode);
> > +
> > +	std::shared_ptr<V4L2CameraProxy> getCamera(int fd);
> > +	std::shared_ptr<V4L2CameraProxy> getCamera(void *addr);
> 
> Every time you return a shared_ptr by value, you increase the
> reference count for no good reason.

But it goes back down after the caller finishes right? :p

> I would either return by reference
> or return V4L2CameraProxy and use shared pointers only in the proxies_
> array and not in the rest of the code base.

Okay.

> Overall this is a very good first submission and I'm happy to see it
> posted to the list \o/
> 
> I admit in this first pass I didn't go into extreme detail on the way
> the v4l2 operations semantic is handled, but it seems sane to me!

Thank you!


Paul

> 
> > +
> > +	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);
> > +
> > +	static int bpl_multiplier(unsigned int format);
> > +	static int image_size(unsigned int format, unsigned int width,
> > +			      unsigned int height);
> > +
> > +	static unsigned int v4l2_to_drm(unsigned int pixelformat);
> > +	static unsigned int drm_to_v4l2(unsigned int pixelformat);
> > +
> > +private:
> > +	V4L2CompatManager();
> > +	~V4L2CompatManager();
> > +
> > +	void run() override;
> > +
> > +	bool validfd(int fd);
> > +	bool validmmap(void *addr);
> > +
> > +	int default_ioctl(int fd, unsigned long request, void *arg);
> > +
> > +	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_;
> > +
> > +	bool cameraManagerRunning_;
> > +	CameraManager *cm_;
> > +
> > +	std::mutex mutex_;
> > +	std::condition_variable cv_;
> > +
> > +	std::vector<std::shared_ptr<V4L2CameraProxy>> proxies_;
> > +	std::map<int, std::shared_ptr<V4L2CameraProxy>> devices_;
> > +	std::map<void *, int> mmaps_;
> > +};
> > +
> > +#endif /* __V4L2_COMPAT_MANAGER_H__ */
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hello,

On Sun, Dec 08, 2019 at 08:10:31PM -0500, Paul Elder wrote:
> On Fri, Dec 06, 2019 at 05:12:03PM +0100, Jacopo Mondi wrote:
> > On Fri, Dec 06, 2019 at 04:26:54AM -0500, Paul Elder wrote:
> > > Add libcamera V4L2 compatibility layer.
> > 
> > Maybe just me, but having this patch split in components would have
> > helped not having to digest 1445 lines of patch in one go
> 
> Laurent said it was okay... :)

The issue with such large patches is that they are difficult to
meaningfully split. Sure, it could have been split in one patch per
file, but they wouldn't contain changes that can be reviewed in
isolation, so a big drop is often the only meaningful option.

> > I know Niklas agrees :)
> > 
> > > 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.
> > >
> > > Note that until we have a way of mapping V4L2 device nodes to libcamera
> > > cameras, the V4L2 compatibility layer will always selects and use the
> > > first enumerated libcamera camera.
> > 
> > That's not trivial indeed...
> > 
> > For simple devices, we could easily collect the video nodes a camera
> > uses and match them with the one the v4l2 application tries to use.
> > 
> > For complex devices, where the DMA output node could be multiplexed by
> > different cameras depending on the ISP configuration, this is not
> > easy.
> > 
> > I presume v4l2 application are mostly meant to be used with simple
> > devices, so adding a list of V4L2Videodevices to the Camera (while a
> > bit of an hack) is totally doable. What do others think ?
> 
> I'm thinking that it's not a good idea to expose anything V4L2 from
> Camera. If it's just UVC devices, might we match instead by the name
> exposed by sysfs?

Quite the contrary, we want the V4L2 adaptation layer to cover *all*
cameras, not just UVC. 

I agree we don't want to expose V4L2 information in the public API, but
at the same time we need to expose enough information to make the match
possible for the V4L2 adaptation layer.

One option would be to add a method to the camera manager to retrieve a
camera by device node path. This wouldn't expose any V4L2-specific
information towards applications, the map of device nodes to cameras
would be private.

> The uvcvideo pipeline handler names the Camera based on that name
> anyway.

And that's something we have to fix as it leads to cameras with
identical names today.

> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  meson_options.txt                |   5 +
> > >  src/meson.build                  |   4 +
> > >  src/v4l2/meson.build             |  30 +++
> > >  src/v4l2/v4l2_camera.cpp         | 312 ++++++++++++++++++++++
> > >  src/v4l2/v4l2_camera.h           |  68 +++++
> > >  src/v4l2/v4l2_camera_proxy.cpp   | 438 +++++++++++++++++++++++++++++++
> > >  src/v4l2/v4l2_camera_proxy.h     |  63 +++++
> > >  src/v4l2/v4l2_compat.cpp         |  81 ++++++
> > >  src/v4l2/v4l2_compat_manager.cpp | 353 +++++++++++++++++++++++++
> > >  src/v4l2/v4l2_compat_manager.h   |  91 +++++++
> > >  10 files changed, 1445 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..b06dd494 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 libcamera with 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..d96db4ff
> > > --- /dev/null
> > > +++ b/src/v4l2/meson.build
> > > @@ -0,0 +1,30 @@
> > > +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',
> > 
> > Seems it is enough to undefine the _FILE_OFFSET_BITS macro to use the
> > 32-bit interface ?
> 
> Looks like it is.

Defining it explicitly to =32 allows catching conflicting redefinitions.
If we just undefine _FILE_OFFSET_BITS, we may then include a header that
would define it to =64 without being aware of that, and run into weird
issues.

> > https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html
> > If _FILE_OFFSET_BITS is undefined, or if it is defined to the value
> > 32, nothing changes. The 32 bit interface is used and types like off_t
> > have a size of 32 bits on 32 bit systems.
> > 
> > > +]
> > > +
> > > +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..c6c84ef3
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -0,0 +1,312 @@
> > > +/* 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 "v4l2_compat_manager.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DECLARE_CATEGORY(V4L2Compat);
> > > +
> > > +V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> > > +	: camera_(camera), bufferCount_(0), isRunning_(false), sequence_(0),
> > > +	  buffer_sema_(new Semaphore())
> > 
> > Can't this be a class member instead of a pointer ?
> > Also, I would name it differently, or at least make it cameraCase_
> 
> ack
> 
> > > +{
> > > +	camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> > > +};
> > > +
> > > +V4L2Camera::~V4L2Camera()
> > > +{
> > > +	while (!pendingRequests_.empty()) {
> > > +		delete pendingRequests_.front();
> > > +		pendingRequests_.pop();
> > > +	}
> > > +
> > > +	buffer_lock_.lock();
> > > +	while (!completedBuffers_.empty()) {
> > > +		delete completedBuffers_.front();
> > > +		completedBuffers_.pop();
> > > +	}
> > > +	buffer_lock_.unlock();
> > > +
> > > +	delete buffer_sema_;
> > 
> > you would save deleting it
> > 
> > > +
> > > +	camera_->release();
> > > +}
> > > +
> > > +void V4L2Camera::open(int *ret, bool nonblock)
> > > +{
> > > +	nonblock_ = nonblock;
> > > +
> > > +	if (camera_->acquire() < 0) {
> > > +		LOG(V4L2Compat, Error) << "Failed to acquire camera";
> > > +		*ret = -EINVAL;
> > > +		return;
> > > +	}
> > > +
> > > +	config_ = camera_->generateConfiguration({ StreamRole::Viewfinder });
> > > +	if (config_ == nullptr) {
> > > +		*ret = -EINVAL;
> > > +		return;
> > > +	}
> > > +
> > > +	*ret = 0;
> > > +}
> > > +
> > > +void V4L2Camera::close(int *ret)
> > > +{
> > > +	*ret = camera_->release();
> > > +}
> > > +
> > > +void V4L2Camera::getStreamConfig(StreamConfiguration *ret)
> > 
> > Maybe *streaConfig ?
> 
> ack
> 
> > > +{
> > > +	*ret = config_->at(0);
> > > +}
> > > +
> > > +void V4L2Camera::requestComplete(Request *request)
> > > +{
> > > +	if (request->status() == Request::RequestCancelled) {
> > > +		LOG(V4L2Compat, Error) << "Request not succesfully completed: "
> > > +				       << request->status();
> > > +		return;
> > > +	}
> > > +
> > > +	/* We only have one stream at the moment. */
> > > +	buffer_lock_.lock();
> > > +	Buffer *buffer = request->buffers().begin()->second;
> > > +	completedBuffers_.push(buffer);
> > > +	buffer_lock_.unlock();
> > > +
> > > +	buffer_sema_->release();
> > > +}
> > > +
> > > +void V4L2Camera::configure(int *ret, struct v4l2_format *arg,
> > > +			   unsigned int bufferCount)
> > > +{
> > > +	config_->at(0).size.width = arg->fmt.pix.width;
> > 
> > Nit: can you use a reference to config_->at(0) ?
> 
> ack
> 
> > > +	config_->at(0).size.height = arg->fmt.pix.height;
> > > +	config_->at(0).pixelFormat =
> > > +		V4L2CompatManager::v4l2_to_drm(arg->fmt.pix.pixelformat);
> > > +	bufferCount_ = bufferCount;
> > > +	config_->at(0).bufferCount = bufferCount_;
> > > +	/* \todo memoryType (interval vs external) */
> > > +
> > > +	CameraConfiguration::Status validation = config_->validate();
> > > +	if (validation == CameraConfiguration::Invalid) {
> > > +		LOG(V4L2Compat, Info) << "Configuration invalid";
> > 
> > that's probably an error, isn't it ?
> 
> Yep.
> 
> > > +		*ret = -EINVAL;
> > > +		return;
> > > +	}
> > > +	if (validation == CameraConfiguration::Adjusted)
> > > +		LOG(V4L2Compat, Info) << "Configuration adjusted";
> > > +
> > > +	LOG(V4L2Compat, Info) << "Validated configuration is: "
> > > +			      << config_->at(0).toString();
> > > +
> > > +	*ret = camera_->configure(config_.get());
> > > +	if (*ret < 0)
> > > +		return;
> > > +
> > > +	bufferCount_ = config_->at(0).bufferCount;
> > > +
> > > +	*ret = 0;
> > > +}
> > > +
> > > +void V4L2Camera::mmap(void **ret, void *addr, size_t length, int prot,
> > > +		      int flags, off_t offset)
> > 
> > Do we need to check flags ?
> 
> No.
> 
> > > +{
> > > +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> > > +
> > > +	if (prot != (PROT_READ | PROT_WRITE)) {
> > > +		errno = EINVAL;
> > 
> > This function (actually all V4L2Camera functions) is called through a method
> > invocation and has a long call stack before getting back to the
> > caller. I wonder if errno does not get overwritten along that route.
> 
> You're right; it doesn't seem to be kept. The caller still receives an
> EINVAL though (but only EINVAL).
> 
> I looked back at this and I think I messed up some errno passing.
> 
> > Also, from man mmap:
> > 
> > ENOTSUP
> > MAP_FIXED or MAP_PRIVATE was specified in the flags argument and the
> > implementation does not support this functionality.
> >
> > The implementation does not support the combination of accesses
> > requested in the prot argument.
> 
> I don't see this in my man mmap... oh wait I found it in the posix man.
> 
> Yeah, this is probably better.
> 
> > > +		*ret = MAP_FAILED;
> > > +		return;
> > > +	}
> > > +
> > > +	StreamConfiguration &streamConfig = config_->at(0);
> > > +	unsigned int sizeimage =
> > > +		V4L2CompatManager::image_size(
> > > +			V4L2CompatManager::drm_to_v4l2(streamConfig.pixelFormat),
> > > +			streamConfig.size.width,
> > > +			streamConfig.size.height);
> > > +	if (sizeimage == 0) {
> > > +		errno = EINVAL;
> > > +		*ret = MAP_FAILED;
> > > +		return;
> > > +	}
> > > +
> > > +	unsigned int index = offset / sizeimage;
> > > +	if (index * sizeimage != offset || length != sizeimage) {
> > > +		errno = EINVAL;
> > 
> > Is EINVAL the right error here?
> > from man mmap:
> > 
> > ENOMEM
> > MAP_FIXED  was  specified, and the range [addr,addr+len)
> > exceeds that allowed for the address space of a process; or, if
> > MAP_FIXED was not specified and there is insufficient room in the
> > address space to effect the mapping
> 
> I don't think this is right, as ENOMEM indicates some form of
> "out of memory", but we are checking for alignment.

The V4L2 adaptation layer should emulate V4L2, so the authoritative
source of information regarding what error codes to return is the kernel
implementation. I'll let you double-check the different error codes
returned through the adaptation layer :-)

> > EOVERFLOW
> > The file is a regular file and the value of off plus len exceeds the
> > offset maximum established in the open file description associated
> > with fildes.
> 
> Similar issue here.
> 
> > However I'm not sure we should care about the mmap errors at this
> > level, as those should be returned when the actual mapping is
> > performed. What do you think ?
> 
> I think we should already return the errors at this point, especially
> since these are all input validation for the mmap for V4L2 devices.

I agree, we need to validate all parameters the same way V4L2 would in
the kernel. How would we decide what index to pass to buffer()[] below
otherwise ?

> > > +		*ret = MAP_FAILED;
> > > +		return;
> > > +	}
> > > +
> > > +	Stream *stream = *camera_->streams().begin();
> > > +	*ret = stream->buffers()[index].planes()[0].mem();
> > > +}
> > > +
> > > +void V4L2Camera::munmap(int *ret, void *addr, size_t length)
> > > +{
> > > +	StreamConfiguration &streamConfig = config_->at(0);
> > > +	unsigned int sizeimage =
> > > +		V4L2CompatManager::image_size(streamConfig.pixelFormat,
> > > +					      streamConfig.size.width,
> > > +					      streamConfig.size.height);
> > > +
> > > +	if (length != sizeimage) {
> > > +		errno = EINVAL;
> > 
> > Here EINVAL seems to be appropriate
> > 
> > > +		*ret = -1;
> > > +		return;
> > > +	}
> > > +
> > > +	*ret = 0;
> > > +}
> > > +
> > > +void V4L2Camera::validStreamType(bool *ret, uint32_t type)
> > > +{
> > > +	*ret = (type == V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > +}
> > > +
> > > +void V4L2Camera::validMemoryType(bool *ret, uint32_t memory)
> > > +{
> > > +	*ret = (memory == V4L2_MEMORY_MMAP);
> > > +}
> > > +
> > > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)
> > > +{
> > > +	LOG(V4L2Compat, Debug) << "Allocating libcamera bufs";
> > > +	*ret = camera_->allocateBuffers();
> > 
> > I fear the buffer rework would required a big rebase of this series :(
> 
> Oh no :(
> 
> > > +	if (*ret == -EACCES)
> > > +		*ret = -EBUSY;
> > > +}
> > > +
> > > +/* \todo implement allocDMABuffers */
> > > +void V4L2Camera::allocDMABuffers(int *ret, unsigned int count)
> > > +{
> > > +	*ret = -ENOTTY;
> > > +}
> > > +
> > > +void V4L2Camera::freeBuffers()
> > > +{
> > > +	camera_->freeBuffers();
> > > +	bufferCount_ = 0;
> > > +}
> > > +
> > > +void V4L2Camera::streamOn(int *ret)
> > > +{
> > > +	if (isRunning_) {
> > > +		*ret = 0;
> > > +		return;
> > > +	}
> > > +
> > > +	sequence_ = 0;
> > > +
> > > +	*ret = camera_->start();
> > > +	if (*ret < 0)
> > > +		return;
> > 
> > errno ?
> 
> Yes.
> 
> I'm going to shift things around so that V4L2Camera can return the
> errnos directly, and ethen either V4L2CameraProxy or V4L2CompatManager
> can set the errno - that should fix the errno being lost in the call
> stack too.

I think that's a good idea, the global errno variable should be set at
the highest level of the call stack.

> > > +	isRunning_ = true;
> > > +
> > > +	while (!pendingRequests_.empty()) {
> > > +		*ret = camera_->queueRequest(pendingRequests_.front());
> > > +		pendingRequests_.pop();
> > > +		if (*ret < 0)
> > > +			return;
> > > +	}
> > > +
> > > +	*ret = 0;
> > > +}
> > > +
> > > +void V4L2Camera::streamOff(int *ret)
> > > +{
> > > +	/* \todo restore buffers to reqbufs state? */
> > > +	if (!isRunning_) {
> > > +		*ret = 0;
> > > +		return;
> > > +	}
> > > +
> > > +	*ret = camera_->stop();
> > > +	if (*ret < 0)
> > > +		return;
> > > +	isRunning_ = false;
> > > +
> > > +	*ret = 0;
> > 
> > Here and in streamOn you could set *ret = 0 at the beginning of the
> > function.
> 
> I originally had to to be analogous to "return 0", but it's duplicated
> in if (!isRunning_) so I suppose setting it at the beginning is better.

If someone is bored, figuring out how to return a value from a bound
method would be nice. I however fear we may be blocked by the fact that
C++ doesn't consider the return type for overload resolution.

> > > +}
> > > +
> > > +void V4L2Camera::qbuf(int *ret, struct v4l2_buffer *arg)
> > > +{
> > > +	Stream *stream = config_->at(0).stream();
> > > +	std::unique_ptr<Buffer> buffer = stream->createBuffer(arg->index);
> > > +	if (!buffer) {
> > > +		LOG(V4L2Compat, Error) << "Can't create buffer";
> > > +		*ret = -ENOMEM;
> > > +		return;
> > > +	}
> > > +
> > > +	Request *request = camera_->createRequest();
> > 
> > paranoid: createRequest() can return nullptr
> 
> Oh no :o
> 
> > > +	*ret = request->addBuffer(std::move(buffer));
> > > +	if (*ret < 0) {
> > > +		LOG(V4L2Compat, Error) << "Can't set buffer for request";
> > > +		return;
> > > +	}
> > > +
> > > +	if (!isRunning_) {
> > > +		pendingRequests_.push(request);
> > > +	} else {
> > > +		*ret = camera_->queueRequest(request);
> > > +		if (*ret < 0) {
> > > +			LOG(V4L2Compat, Error) << "Can't queue request";
> > > +			if (*ret == -EACCES)
> > > +				*ret = -EBUSY;
> > > +			return;
> > > +		}
> > > +	}
> > > +
> > > +	arg->flags |= V4L2_BUF_FLAG_QUEUED;
> > > +	arg->flags |= V4L2_BUF_FLAG_MAPPED;
> > > +	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> > > +	*ret = 0;
> > > +}
> > > +
> > > +int V4L2Camera::dqbuf(struct v4l2_buffer *arg)
> > > +{
> > > +	if (nonblock_ && !buffer_sema_->tryAcquire())
> > > +		return -EAGAIN;
> > > +	else
> > > +		buffer_sema_->acquire();
> > > +
> > > +	buffer_lock_.lock();
> > > +	Buffer *buffer = completedBuffers_.front();
> > > +	completedBuffers_.pop();
> > > +	buffer_lock_.unlock();
> > > +
> > > +	arg->bytesused = buffer->bytesused();
> > > +	arg->field = V4L2_FIELD_NONE;
> > > +	arg->timestamp.tv_sec = buffer->timestamp() / 1000000000;
> > > +	arg->timestamp.tv_usec = buffer->timestamp() / 1000;
> > > +	arg->sequence = sequence_++;
> > 
> > Don't we have sequence in Buffer ?
> 
> Oops, yeah we do.
> 
> > > +
> > > +	arg->flags &= ~V4L2_BUF_FLAG_QUEUED;
> > > +	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> > > +
> > > +	StreamConfiguration &streamConfig = config_->at(0);
> > > +	unsigned int sizeimage =
> > > +		V4L2CompatManager::image_size(streamConfig.pixelFormat,
> > > +					      streamConfig.size.width,
> > > +					      streamConfig.size.height);
> > > +	arg->length = sizeimage;
> > 
> > You can save the sizeimage variable.
> > I wonder if this needs to be re-calculated everytime... nothing big
> > however...
> 
> Okay.
> 
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > new file mode 100644
> > > index 00000000..3d3cd8ff
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_camera.h
> > > @@ -0,0 +1,68 @@
> > > +/* 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 <linux/videodev2.h>
> > > +#include <mutex>
> > > +#include <queue>
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include "semaphore.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +class V4L2Camera : public Object
> > > +{
> > > +public:
> > > +	V4L2Camera(std::shared_ptr<Camera> camera);
> > > +	~V4L2Camera();
> > > +
> > > +	void open(int *ret, bool nonblock);
> > > +	void close(int *ret);
> > > +	void getStreamConfig(StreamConfiguration *ret);
> > > +	void requestComplete(Request *request);
> > > +
> > > +	void mmap(void **ret, void *addr, size_t length, int prot, int flags,
> > > +		  off_t offset);
> > > +	void munmap(int *ret, void *addr, size_t length);
> > > +
> > > +	void configure(int *ret, struct v4l2_format *arg,
> > > +		       unsigned int bufferCount);
> > > +
> > > +	void validStreamType(bool *ret, uint32_t type);
> > > +	void validMemoryType(bool *ret, uint32_t memory);
> > > +
> > > +	void allocBuffers(int *ret, unsigned int count);
> > > +	void allocDMABuffers(int *ret, unsigned int count);
> > > +	void freeBuffers();
> > > +	void streamOn(int *ret);
> > > +	void streamOff(int *ret);
> > > +
> > > +	void qbuf(int *ret, struct v4l2_buffer *arg);
> > > +	int dqbuf(struct v4l2_buffer *arg);
> > > +
> > > +private:
> > > +	void initDefaultV4L2Format();
> > > +
> > > +	std::shared_ptr<Camera> camera_;
> > > +	std::unique_ptr<CameraConfiguration> config_;
> > > +
> > > +	unsigned int bufferCount_;
> > > +	bool isRunning_;
> > > +	bool nonblock_;
> > > +
> > > +	unsigned int sequence_;
> > > +
> > > +	Semaphore *buffer_sema_;
> > > +	std::mutex buffer_lock_;
> > > +
> > > +	std::queue<Request *> pendingRequests_;
> > > +	std::queue<Buffer *> 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..1dd2c363
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > @@ -0,0 +1,438 @@
> > > +/* 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/videodev2.h>
> > > +#include <string.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)
> > > +	: index_(index), bufferCount_(0), currentBuf_(0),
> > > +	  vcam_(utils::make_unique<V4L2Camera>(camera))
> > > +{
> > > +	querycap(camera);
> > > +}
> > > +
> > > +V4L2CameraProxy::~V4L2CameraProxy()
> > > +{
> > > +	vcam_.reset();
> > 
> > Not sure it is necessary to reset a unique_ptr<> at destruction time,
> > the managed pointer will be destroyed anyway.
> 
> Ah, right.
> 
> > > +}
> > > +
> > > +int V4L2CameraProxy::open(bool nonblock)
> > > +{
> > > +	int ret;
> > > +	vcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,
> > > +			    &ret, nonblock);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> > > +			    ConnectionTypeBlocking, &streamConfig_);
> > > +	setFmtFromConfig();
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int V4L2CameraProxy::close()
> > > +{
> > > +	int ret;
> > > +	vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);
> > > +	return ret;
> > > +}
> > > +
> > > +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > > +			    off_t offset)
> > > +{
> > > +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> > > +
> > > +	void *val;
> > > +	vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
> > > +			    &val, addr, length, prot, flags, offset);
> > > +	return val;
> > > +}
> > > +
> > > +int V4L2CameraProxy::munmap(void *addr, size_t length)
> > > +{
> > > +	LOG(V4L2Compat, Debug) << "Servicing MUNMAP";
> > > +
> > > +	int ret;
> > > +	vcam_->invokeMethod(&V4L2Camera::munmap, ConnectionTypeBlocking,
> > > +			    &ret, addr, length);
> > > +	return ret;
> > > +}
> > > +
> > > +bool V4L2CameraProxy::hasPixelFormat(unsigned int format)
> > > +{
> > > +	const std::vector<PixelFormat> &formats =
> > > +		streamConfig_.formats().pixelformats();
> > > +	return std::find(formats.begin(), formats.end(), format) != formats.end();
> > > +}
> > > +
> > > +/* \todo getDeviceCaps? getMemoryCaps? */
> > > +
> > > +bool V4L2CameraProxy::hasSize(unsigned int format, Size size)
> > > +{
> > > +	int len = streamConfig_.formats().sizes(format).size();
> > > +	for (int i = 0; i < len; i++)
> > > +		if (streamConfig_.formats().sizes(format)[i] == size)
> > > +			return true;
> > 
> > Can't you find() on the vector<Size> returned by
> > streamConfig_.formats().sizes(format) ?
> 
> Yes.
> 
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +bool V4L2CameraProxy::validateStreamType(uint32_t type)
> > > +{
> > > +	bool valid;
> > > +	vcam_->invokeMethod(&V4L2Camera::validStreamType,
> > > +			    ConnectionTypeBlocking, &valid, type);
> > > +	if (!valid)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> > > +{
> > > +	bool valid;
> > > +	vcam_->invokeMethod(&V4L2Camera::validMemoryType,
> > > +			    ConnectionTypeBlocking, &valid, memory);
> > > +	if (!valid)
> > > +		return false;
> > > +
> > > +	return true;
> > 
> > In this two functions you can here just return 'valid'
> 
> Why?
> 
> > > +}
> > > +
> > > +void V4L2CameraProxy::setFmtFromConfig()
> > > +{
> > > +	curV4L2Format_.fmt.pix.width = streamConfig_.size.width;
> > > +	curV4L2Format_.fmt.pix.height = streamConfig_.size.height;
> > > +	curV4L2Format_.fmt.pix.pixelformat =
> > > +		V4L2CompatManager::drm_to_v4l2(streamConfig_.pixelFormat);
> > > +	curV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;
> > > +	curV4L2Format_.fmt.pix.bytesperline =
> > > +		V4L2CompatManager::bpl_multiplier(
> > > +			curV4L2Format_.fmt.pix.pixelformat) *
> > > +		curV4L2Format_.fmt.pix.width;
> > > +	curV4L2Format_.fmt.pix.sizeimage =
> > > +		V4L2CompatManager::image_size(curV4L2Format_.fmt.pix.pixelformat,
> > > +					      curV4L2Format_.fmt.pix.width,
> > > +					      curV4L2Format_.fmt.pix.height);
> > > +	curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > > +}
> > > +
> > > +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));
> > > +	capabilities_.version = KERNEL_VERSION(5, 2, 0);
> > > +	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;
> > 
> > Are we single planar only ? I guess so, it's fine :)
> 
> Yeah :)
> 
> > > +	capabilities_.capabilities =
> > > +		capabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;
> > > +	memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
> > > +}
> > > +
> > > +
> > > +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))
> > > +		return -EINVAL;
> > > +	if (arg->index > streamConfig_.formats().pixelformats().size())
> > > +		return -EINVAL;
> > > +
> > > +	memcpy(arg->description, "asdf", 5);
> > 
> > That's a real nice format name! :D
> > Do we need a map of formats to descriptions ?
> 
> Thanks! :D
> We might.

I'm afraid we do. For now you can replace "asdf" with "<format name>"
and add a \todo item.

> > > +	arg->pixelformat =
> > > +		V4L2CompatManager::drm_to_v4l2(
> > > +			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;
> > > +
> > > +	arg->fmt.pix.width        = curV4L2Format_.fmt.pix.width;
> > > +	arg->fmt.pix.height       = curV4L2Format_.fmt.pix.height;
> > > +	arg->fmt.pix.pixelformat  = curV4L2Format_.fmt.pix.pixelformat;
> > > +	arg->fmt.pix.field        = curV4L2Format_.fmt.pix.field;
> > > +	arg->fmt.pix.bytesperline = curV4L2Format_.fmt.pix.bytesperline;
> > > +	arg->fmt.pix.sizeimage    = curV4L2Format_.fmt.pix.sizeimage;
> > > +	arg->fmt.pix.colorspace   = curV4L2Format_.fmt.pix.colorspace;
> > > +
> > > +	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;
> > > +
> > > +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > > +			    &ret, arg, bufferCount_);
> > > +	if (ret < 0)
> > > +		return -EINVAL;
> > > +
> > > +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> > > +			    ConnectionTypeBlocking, &streamConfig_);
> > > +	setFmtFromConfig();
> > > +
> > > +	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;
> > > +	if (!hasPixelFormat(format))
> > > +		format = streamConfig_.formats().pixelformats()[0];
> > > +
> > > +	Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> > > +	if (!hasSize(format, size))
> > > +		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 =
> > > +		V4L2CompatManager::bpl_multiplier(
> > > +			V4L2CompatManager::drm_to_v4l2(format)) *
> > > +		arg->fmt.pix.width;
> > > +	arg->fmt.pix.sizeimage    =
> > > +		V4L2CompatManager::image_size(
> > > +			V4L2CompatManager::drm_to_v4l2(format),
> > > +			arg->fmt.pix.width, arg->fmt.pix.height);
> > > +	arg->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> > > +{
> > > +	LOG(V4L2Compat, Debug) << "Servicing VIDIOC_REQBUFS";
> > > +	if (!validateStreamType(arg->type))
> > > +		return -EINVAL;
> > > +	if (!validateMemoryType(arg->memory))
> > > +		return -EINVAL;
> > > +
> > > +	LOG(V4L2Compat, Debug) << arg->count << " bufs requested ";
> > > +
> > > +	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > > +
> > > +	if (arg->count == 0) {
> > > +		LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> > > +		int ret;
> > > +		vcam_->invokeMethod(&V4L2Camera::streamOff,
> > > +				    ConnectionTypeBlocking, &ret);
> > > +		vcam_->invokeMethod(&V4L2Camera::freeBuffers,
> > > +				    ConnectionTypeBlocking);
> > > +		bufferCount_ = 0;
> > > +		return 0;
> > > +	}
> > > +
> > > +	int ret;
> > 
> > as ret is used function-wise, I would its declaration to the beginning
> 
> ack
> 
> > > +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > > +			    &ret, &curV4L2Format_, arg->count);
> > > +	if (ret < 0)
> > > +		return -EINVAL;
> > > +	arg->count = streamConfig_.bufferCount;
> > > +	bufferCount_ = arg->count;
> > > +
> > > +	ret = -EINVAL;
> > > +	if (arg->memory == V4L2_MEMORY_MMAP)
> > > +		vcam_->invokeMethod(&V4L2Camera::allocBuffers,
> > > +				    ConnectionTypeBlocking, &ret, arg->count);
> > > +	else if (arg->memory == V4L2_MEMORY_DMABUF)
> > 
> > Do we even claim support for this ?
> 
> I was going to but then didn't... and removed it and didn't remove it in
> other places.

We will have to, but it can be left unimplemented in the first version.
Please add a \todo item.

> > > +		vcam_->invokeMethod(&V4L2Camera::allocDMABuffers,
> > > +				    ConnectionTypeBlocking, &ret, arg->count);
> > > +
> > > +	if (ret < 0) {
> > > +		arg->count = 0;
> > > +		return ret == -EACCES ? -EBUSY : ret;
> > > +	}
> > > +
> > > +	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))
> > > +		return -EINVAL;
> > > +	if (arg->index >= stream->buffers().size())
> > > +		return -EINVAL;
> > > +
> > > +	unsigned int index = arg->index;
> > > +	memset(arg, 0, sizeof(*arg));
> > > +	arg->index = index;
> > > +	arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > +	arg->length = curV4L2Format_.fmt.pix.sizeimage;
> > > +	arg->memory = V4L2_MEMORY_MMAP;
> > > +	arg->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;
> > > +
> > > +	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))
> > > +		return -EINVAL;
> > > +	if (!validateMemoryType(arg->memory))
> > > +		return -EINVAL;
> > > +	if (arg->index >= stream->buffers().size())
> > > +		return -EINVAL;
> > > +
> > > +	int ret;
> > > +	vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,
> > > +			    &ret, arg);
> > > +	return ret;
> > > +}
> > > +
> > > +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> > > +{
> > > +	LOG(V4L2Compat, Debug) << "Servicing VIDIOC_DQBUF";
> > > +
> > > +	if (!validateStreamType(arg->type))
> > > +		return -EINVAL;
> > > +	if (!validateMemoryType(arg->memory))
> > > +		return -EINVAL;
> > > +
> > > +	arg->index = currentBuf_;
> > > +	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> > > +
> > > +	return vcam_->dqbuf(arg);
> > 
> > Is dqbuf special ?
> 
> Yes.
> 
> > I know it could return immediately if nonblock_ is set, but
> > invokeMethod checks that invoked method is called, if it returns
> > immediately, that's fine. Or have I missed some other reason why this
> > is called directly.
> 
> Here we are waiting for the frames to be produced, which must be in a
> different thread from where the frames are produced.
> 
> Otherwise (speaking from experience) we either busy wait in V4L2Camera,
> or the frame can't become available because we're blocked waiting for it
> to become available.
> 
> > > +}
> > > +
> > > +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);
> > > +	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);
> > > +	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));
> > 
> > camelCase for method names as well ?
> 
> I suppose... I liked that the vidiocs mirrored the ioctl identifiers
> though...
> Should I do camelCase instead?
> 
> vidiocQueryCap
> vidiocEnumFmt
> vidiocGFmt
> vidiocTryFmt
> vidiocReqBufs
> vidiocStreamOn
> 
> What do you think?

I'm fine with either. The coding style mandates camelCase, but
exceptions are allowed when mapping to snake_case symbols whose name is
standardized outside of libcamera.

> > > +		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;
> > > +	case VIDIOC_EXPBUF:
> > > +	case VIDIOC_ENUM_FRAMESIZES:
> > > +	default:
> > > +		ret = -ENOTTY;
> > > +	}
> > > +
> > > +	if (ret < 0) {
> > > +		errno = ret;
> > > +		return -1;
> > > +	}
> > > +
> > > +	errno = 0;
> > > +	return ret;
> > > +
> > > +}
> > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > > new file mode 100644
> > > index 00000000..64c7aadd
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_camera_proxy.h
> > > @@ -0,0 +1,63 @@
> > > +/* 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 <libcamera/camera.h>
> > > +
> > > +#include "v4l2_camera.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +class V4L2CameraProxy
> > > +{
> > > +public:
> > > +	V4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera);
> > > +	~V4L2CameraProxy();
> > > +
> > > +	int open(bool nonblock);
> > > +	int 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);
> > > +
> > > +private:
> > > +	bool hasPixelFormat(unsigned int format);
> > > +	bool hasSize(unsigned int format, Size size);
> > > +	bool validateStreamType(uint32_t type);
> > > +	bool validateMemoryType(uint32_t memory);
> > > +	void setFmtFromConfig();
> > > +	void querycap(std::shared_ptr<Camera> camera);
> > > +
> > > +	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 index_;
> > > +
> > > +	struct v4l2_format curV4L2Format_;
> > > +	StreamConfiguration streamConfig_;
> > > +	struct v4l2_capability capabilities_;
> > > +	unsigned int bufferCount_;
> > > +	unsigned int currentBuf_;
> > > +
> > > +	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..3330e7bc
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_compat.cpp
> > > @@ -0,0 +1,81 @@
> > > +/* 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 <iostream>
> > > +
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <linux/videodev2.h>
> > > +#include <stdarg.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/types.h>
> > > +
> > > +#define LIBCAMERA_PUBLIC __attribute__((visibility("default")))
> > 
> > Am I wrong or this makes sense only if we compile with
> > -fvisiblity=hidden ?
> > https://gcc.gnu.org/wiki/Visibility
> 
> It might be. When I tried it in the beginning with the compile options
> that we already had, the symbols weren't being exported.
> 
> > I welcome this change, but then probably you should add that
> > compilation flag to the v4l2 compat library, it I have not
> > mis-interpreted the above wiki page
> 
> Okay.

Could you please compare the objdump of the adaptation layer with and
without the visibility option, to see what difference it makes ?

> > > +
> > > +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 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)
> > > +{
> > > +	void *val = V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);
> > > +	return val;
> > > +}
> > > +
> > > +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..4eeb714f
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > > @@ -0,0 +1,353 @@
> > > +/* 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 <iostream>
> > > +#include <linux/drm_fourcc.h>
> > > +#include <linux/videodev2.h>
> > > +#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()
> > > +	: cameraManagerRunning_(false), cm_(nullptr)
> > > +{
> > > +	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");
> > 
> > You seem to be mixing cameraCase and snake_case here and there in

Is cameraCase a libcamera-specific naming scheme ? :-)

> > variable declaration. Was this intentional ?
> 
> It was intentional, until I found the few that were unintentional.
> 
> These ones, plus the vidioc ones are the only ones that I intended to be
> snake_case and thought was borderline acceptable.
> 
> > > +}
> > > +
> > > +V4L2CompatManager::~V4L2CompatManager()
> > > +{
> > > +	devices_.clear();
> > > +
> > > +	if (isRunning()) {
> > > +		exit(0);
> > > +		/* \todo Wait with a timeout, just in case. */
> > > +		wait();
> > > +	}
> > > +}
> > > +
> > > +int V4L2CompatManager::init()
> > > +{
> > > +	start();
> > > +
> > > +	MutexLocker locker(mutex_);
> > > +	cv_.wait(locker);
> > > +
> > > +	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 wraps a camera device.
> > > +	 */
> > > +	// \todo map v4l2 devices to libcamera cameras; minor device node?
> > > +	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.
> > > +	 */
> > > +	cv_.notify_one();
> > > +
> > > +	/* Now start processing events and messages. */
> > > +	exec();
> > > +
> > > +	cm_->stop();
> > > +	delete cm_;
> > > +	cm_ = nullptr;
> > > +}
> > > +
> > > +V4L2CompatManager *V4L2CompatManager::instance()
> > > +{
> > > +	static V4L2CompatManager v4l2CompatManager;
> > > +	return &v4l2CompatManager;
> > > +}
> > > +
> > > +bool V4L2CompatManager::validfd(int fd)
> > > +{
> > > +	return devices_.find(fd) != devices_.end();
> > > +}
> > > +
> > > +bool V4L2CompatManager::validmmap(void *addr)
> > > +{
> > > +	return mmaps_.find(addr) != mmaps_.end();
> > > +}
> > > +
> > > +std::shared_ptr<V4L2CameraProxy> V4L2CompatManager::getCamera(int fd)
> > > +{
> > > +	if (!validfd(fd))
> > > +		return nullptr;
> > > +
> > > +	return devices_.at(fd);
> > 
> > This is safe, but it's a double lookup, same as below. This is minor
> > indeed, but probably using the iterator returned from find() directly
> > is slightly more efficient.
> 
> How can I use the iterator directly?
> 
> When we intercept the calls and check if we should simply forward the
> call, I think it's simpler to have just a validfd() call... but then
> when we do need to process it then it does become a double (triple?)
> lookup... or just check getCamera() == nullptr instead of validfd().
> 
> > Then you can inline the valid[fd|mmap}() methods, probably
> > 
> > > +}
> > > +
> > > +std::shared_ptr<V4L2CameraProxy> V4L2CompatManager::getCamera(void *addr)
> > > +{
> > > +	if (!validmmap(addr))
> > > +		return nullptr;
> > > +
> > > +	return devices_.at(mmaps_.at(addr));
> > > +}
> > > +
> > > +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;
> > > +
> > > +	if (std::string(path).find("video") == std::string::npos)
> > > +		return fd;
> > > +
> > > +	if (!isRunning())
> > > +		init();
> > > +
> > > +	/* \todo map v4l2 devnodes to libcamera cameras */
> > > +	unsigned int camera_index = 0;
> > > +
> > > +	std::shared_ptr<V4L2CameraProxy> proxy = proxies_[camera_index];
> > > +	int ret = proxy->open(mode & O_NONBLOCK);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	int efd = eventfd(0, (mode & O_CLOEXEC) | (mode & O_NONBLOCK));
> > > +	if (efd < 0)
> > > +		return errno;
> > 
> > I'm not sure I get this usage of the file descriptor returned by
> > eventfd...
> > 
> > It is used as index in the map, possibly replaced by dup(). I would
> > have expected the fd returned by openat() to be used as index in the
> > map... What am I missing ?
> 
> The fd returned by eventfd reserves the fd so that any other open()
> call doesn't overwrite/conflict with our fd. This fd is indeed used as
> an index in the map (see next line), and dup() doesn't replace it.

The fd returned by openat_func_ at the top is leaked by the way. It
should be closed before calling isRunning().

> > > +
> > > +	devices_.emplace(efd, proxy);
> > > +
> > > +	return efd;
> > > +}
> > > +
> > > +int V4L2CompatManager::dup(int oldfd)
> > > +{
> > > +	int newfd = dup_func_(oldfd);
> > > +	if (validfd(oldfd))
> > 
> > Shouldn't you return an error if the fd to duplicate is not valid, ad
> > dup() only if it is ?
> 
> If the fd is not valid, then it means the fd is not for us, and we need
> to forward the dup() call. So in any case, we need to dup(). The only
> difference is if we need to save the new fd in the mapping or not.
> 
> > > +		devices_[newfd] = devices_[oldfd];
> > > +
> > > +	return newfd;
> > > +}
> > > +
> > > +int V4L2CompatManager::close(int fd)
> > > +{
> > > +	if (validfd(fd) && devices_[fd]->close() < 0)
> > 
> > I would return -EIO if !validfd() and propagate the error up if
> > close() fails.
> 
> If !validfd() then the fd isn't for us, and the call needs to be
> forwarded. -EIO is only if *our* close() fails.
> 
> > > +		return -EIO;
> > > +
> > > +	return close_func_(fd);
> > > +}
> > > +
> > > +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> > > +			      int fd, off_t offset)
> > > +{
> > > +	if (!validfd(fd))
> > > +		return mmap_func_(addr, length, prot, flags, fd, offset);
> > > +
> > > +	void *map = getCamera(fd)->mmap(addr, length, prot, flags, offset);
> > > +	if (map == MAP_FAILED) {
> > > +		errno = EINVAL;
> > > +		return map;
> > > +	}
> > > +
> > > +	mmaps_[addr] = fd;
> > > +	return map;
> > > +}
> > > +
> > > +int V4L2CompatManager::munmap(void *addr, size_t length)
> > > +{
> > > +	if (!validmmap(addr))
> > > +		return munmap_func_(addr, length);
> > > +
> > > +	int ret = getCamera(addr)->munmap(addr, length);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	mmaps_.erase(addr);
> > > +	addr = nullptr;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)
> > > +{
> > > +	std::shared_ptr<V4L2CameraProxy> proxy = getCamera(fd);
> > > +	if (!proxy)
> > > +		return ioctl_func_(fd, request, arg);
> > > +
> > 
> > What is the use case for bypassing the proxy ? That might be my
> > limited knowledge of how v4l2 application behave
> 
> Perhaps. Here I check if proxy == nullptr, but in other places I checked
> !validfd(fd) (I'll make these consistent). If the fd is not valid = the
> proxy doesn't exist for the fd, then the fd is not for us and we need to
> forward it to the original call.
> 
> > > +	return proxy->ioctl(request, arg);
> > > +}
> > > +
> > > +/* \todo make libcamera export these */
> > 
> > mmm, V4L2VideoDevice has very similar format conversion routines... we
> > should really centralize them somehow.. maybe not in scope for this
> > patch, but replicating cose which is tricky to get right due to the
> > different format definition between DRM and V4L2 is not a good idea.
> 
> I know...
> 
> > > +int V4L2CompatManager::bpl_multiplier(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 V4L2CompatManager::image_size(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 V4L2CompatManager::v4l2_to_drm(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 V4L2CompatManager::drm_to_v4l2(unsigned int pixelformat)
> > > +{
> > > +	switch (pixelformat) {
> > > +	/* RGB formats. */
> > > +	case DRM_FORMAT_BGR888:
> > > +		return V4L2_PIX_FMT_BGR24;
> > 
> > This in example does not match the one we have in V4L2Videodevice.
> > DRM_FORMAT_BGR24 = V4L2_PIX_FMT_RGB24 not BGR24.
> 
> Oh wat :/
> Great.
> 
> > I know, it's a pain.
> > 
> > I think the other version is correct, as we validated them using
> > conversion routines from kernel drivers...
> > 
> > There might be more below (and possibly above)
> > 
> > > +	case DRM_FORMAT_RGB888:
> > > +		return V4L2_PIX_FMT_RGB24;
> > > +	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_compat_manager.h b/src/v4l2/v4l2_compat_manager.h
> > > new file mode 100644
> > > index 00000000..492f97fd
> > > --- /dev/null
> > > +++ b/src/v4l2/v4l2_compat_manager.h
> > > @@ -0,0 +1,91 @@
> > > +/* 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 <linux/videodev2.h>
> > > +#include <map>
> > > +#include <mutex>
> > > +#include <queue>
> > > +#include <sys/syscall.h>
> > > +#include <unistd.h>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/camera_manager.h>
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include "thread.h"
> > > +#include "v4l2_camera_proxy.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +class V4L2CompatManager : public Thread
> > > +{
> > > +public:
> > > +	static V4L2CompatManager *instance();
> > > +
> > > +	int init();
> > > +
> > > +	int openat(int dirfd, const char *path, int oflag, mode_t mode);
> > > +
> > > +	std::shared_ptr<V4L2CameraProxy> getCamera(int fd);
> > > +	std::shared_ptr<V4L2CameraProxy> getCamera(void *addr);
> > 
> > Every time you return a shared_ptr by value, you increase the
> > reference count for no good reason.
> 
> But it goes back down after the caller finishes right? :p
> 
> > I would either return by reference
> > or return V4L2CameraProxy and use shared pointers only in the proxies_
> > array and not in the rest of the code base.
> 
> Okay.

If you return a reference, make it a const reference, otherwise the
caller would be able to mess up with the shared pointer stored
internally in the compat manager.

I've only looked at Jacopo's comments in this review, I'll have a look
at your code when reviewing v2. As I disagreed with a few points above,
some of the changes in v2 may need to be reverted (or modified), could
you please already have a look at those ?

> > Overall this is a very good first submission and I'm happy to see it
> > posted to the list \o/
> > 
> > I admit in this first pass I didn't go into extreme detail on the way
> > the v4l2 operations semantic is handled, but it seems sane to me!
> 
> Thank you!
> 
> > > +
> > > +	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);
> > > +
> > > +	static int bpl_multiplier(unsigned int format);
> > > +	static int image_size(unsigned int format, unsigned int width,
> > > +			      unsigned int height);
> > > +
> > > +	static unsigned int v4l2_to_drm(unsigned int pixelformat);
> > > +	static unsigned int drm_to_v4l2(unsigned int pixelformat);
> > > +
> > > +private:
> > > +	V4L2CompatManager();
> > > +	~V4L2CompatManager();
> > > +
> > > +	void run() override;
> > > +
> > > +	bool validfd(int fd);
> > > +	bool validmmap(void *addr);
> > > +
> > > +	int default_ioctl(int fd, unsigned long request, void *arg);
> > > +
> > > +	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_;
> > > +
> > > +	bool cameraManagerRunning_;
> > > +	CameraManager *cm_;
> > > +
> > > +	std::mutex mutex_;
> > > +	std::condition_variable cv_;
> > > +
> > > +	std::vector<std::shared_ptr<V4L2CameraProxy>> proxies_;
> > > +	std::map<int, std::shared_ptr<V4L2CameraProxy>> devices_;
> > > +	std::map<void *, int> mmaps_;
> > > +};
> > > +
> > > +#endif /* __V4L2_COMPAT_MANAGER_H__ */