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

Jacopo Mondi jacopo at jmondi.org
Mon Dec 9 11:30:13 CET 2019


Hi Paul, thanks for the quick follow up

On Sun, Dec 08, 2019 at 11:56:03PM -0500, Paul Elder wrote:
> Add libcamera V4L2 compatibility layer.
>
> This initial implementation supports the minimal set of V4L2 operations,
> which allows getting, setting, and enumerating formats, and streaming
> frames from a video device. Some data about the wrapped V4L2 video
> device are hardcoded.
>
> Add a build option named 'v4l2' and adjust the build system to
> selectively compile the V4L2 compatibility layer.
>
> 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 at ideasonboard.com>
>
> ---
> Changes in v2:
> - move all errno acrobatics to V4L2CameraProxy
> - remove all mentions of dmabuf
> - make V4L2CompatManager::getCamera return pointer rather than
>   shared_ptr
> - match V4L2 device nodes to libcamera cameras using Camera::name()
>   compared to /sys/dev/char/maj:min/name (only works for UVC cameras)
>   - in V4L2CompatManager::getCameraIndex()
> - add -fvisibility=hidden to v4l2 compat
> - cache the results of V4L2CompatManager::imageSize() within V4L2Camera
>   (where V4L2Camera is interested)
> - remove V4L2CompatManager::valid[fd|mmap], and where those methods were
>   used, check V4L2CompatManager::getCamera() != nullptr instead
> - fixed V4L2CompatManager::drmToV4L2() mappings for DRM_FORMAT_BGR888
>   and DRM_FORMAT_RGB888
> - other cosmetic changes
> ---
>  meson_options.txt                |   5 +
>  src/meson.build                  |   4 +
>  src/v4l2/meson.build             |  30 ++
>  src/v4l2/v4l2_camera.cpp         | 299 ++++++++++++++++++++
>  src/v4l2/v4l2_camera.h           |  67 +++++
>  src/v4l2/v4l2_camera_proxy.cpp   | 452 +++++++++++++++++++++++++++++++
>  src/v4l2/v4l2_camera_proxy.h     |  63 +++++
>  src/v4l2/v4l2_compat.cpp         |  81 ++++++
>  src/v4l2/v4l2_compat_manager.cpp | 382 ++++++++++++++++++++++++++
>  src/v4l2/v4l2_compat_manager.h   |  86 ++++++
>  10 files changed, 1469 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..1650048e
> --- /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',
> +    '-fvisibility=hidden',
> +]
> +
> +v4l2_compat = shared_library('v4l2-compat',
> +                             v4l2_compat_sources,
> +                             name_prefix : '',

Do you need this ? I tried removing it and it succesfully compiled...

> +                             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..f944c577
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -0,0 +1,299 @@
> +/* 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)
> +{
> +	camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> +};
> +
> +V4L2Camera::~V4L2Camera()
> +{
> +	while (!pendingRequests_.empty()) {
> +		delete pendingRequests_.front();
> +		pendingRequests_.pop();
> +	}
> +
> +	bufferLock_.lock();
> +	while (!completedBuffers_.empty()) {
> +		delete completedBuffers_.front();
> +		completedBuffers_.pop();
> +	}
> +	bufferLock_.unlock();

Is locking required here ? The V4L2Camera is destroyed when the proxy
is destroyed, no other calls should be in-flight. Are all buffers
dequeued at this point ?

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

Probably roles should be handled differently, but I'm not sure how.
Does our API fall short here ? What if vewifinder is not supported,
should we be able to query what roles are supported from a camera ?

> +	if (config_ == nullptr) {
> +		*ret = -EINVAL;
> +		return;
> +	}
> +
> +	updateSizeImage();
> +
> +	*ret = 0;
> +}
> +
> +void V4L2Camera::close(int *ret)
> +{
> +	*ret = camera_->release();
> +}
> +
> +void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
> +{
> +	*streamConfig = config_->at(0);
> +}
> +
> +void V4L2Camera::updateSizeImage()
> +{
> +	StreamConfiguration &streamConfig = config_->at(0);
> +	sizeimage_ =
> +		V4L2CompatManager::imageSize(

nit: this could fit on the previous line

> +			V4L2CompatManager::drmToV4L2(streamConfig.pixelFormat),
> +			streamConfig.size.width,
> +			streamConfig.size.height);
> +}
> +
> +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. */
> +	bufferLock_.lock();
> +	Buffer *buffer = request->buffers().begin()->second;
> +	completedBuffers_.push(buffer);
> +	bufferLock_.unlock();
> +
> +	bufferSema_.release();
> +}
> +
> +void V4L2Camera::configure(int *ret, struct v4l2_format *arg,
> +			   unsigned int bufferCount)
> +{
> +	StreamConfiguration &streamConfig = config_->at(0);
> +	streamConfig.size.width = arg->fmt.pix.width;
> +	streamConfig.size.height = arg->fmt.pix.height;
> +	streamConfig.pixelFormat =
> +		V4L2CompatManager::v4l2ToDrm(arg->fmt.pix.pixelformat);
> +	bufferCount_ = bufferCount;
> +	streamConfig.bufferCount = bufferCount_;
> +	/* \todo memoryType (interval vs external) */
> +
> +	CameraConfiguration::Status validation = config_->validate();
> +	if (validation == CameraConfiguration::Invalid) {
> +		LOG(V4L2Compat, Error) << "Configuration invalid";
> +		*ret = -EINVAL;
> +		return;
> +	}
> +	if (validation == CameraConfiguration::Adjusted)
> +		LOG(V4L2Compat, Info) << "Configuration adjusted";
> +
> +	LOG(V4L2Compat, Info) << "Validated configuration is: "
> +			      << streamConfig.toString();
> +
> +	*ret = camera_->configure(config_.get());
> +	if (*ret < 0)
> +		return;
> +
> +	bufferCount_ = streamConfig.bufferCount;
> +
> +	updateSizeImage();
> +	if (sizeimage_ == 0)

I'm always a bit scared by functions that updates a global variable as
side effect and let the caller responsability of making sure the
intended variable is set to the desired value.

I would rather make updateSizeImage() a calculateSizeImage(config) and
assign it to the the global sizeimage_ in the caller, or make it
return an error code.

> +		*ret = -EINVAL;
> +}
> +
> +void V4L2Camera::mmap(void **ret, int *err, void *addr, size_t length, int prot, off_t offset)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> +
> +	if (prot != (PROT_READ | PROT_WRITE)) {
> +		*ret = MAP_FAILED;
> +		*err = ENOTSUP;
> +		return;
> +	}
> +
> +	unsigned int index = offset / sizeimage_;
> +	if (index * sizeimage_ != offset || length != sizeimage_) {
> +		*ret = MAP_FAILED;
> +		*err = EINVAL;
> +		return;
> +	}
> +
> +	Stream *stream = *camera_->streams().begin();
> +	*ret = stream->buffers()[index].planes()[0].mem();
> +	*err = 0;
> +}
> +
> +void V4L2Camera::munmap(int *ret, void *addr, size_t length)
> +{
> +	*ret = 0;
> +
> +	if (length != sizeimage_)
> +		*ret = -EINVAL;
> +}
> +
> +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";

Empty line, to be consistent with other functions.

But I actually wonder if we need this. i had the same in the android
HAL and it's a useful tracing tool, so I understand you might want to
keep them. After all, debug can be filtered out...

> +	*ret = camera_->allocateBuffers();
> +	if (*ret == -EACCES)
> +		*ret = -EBUSY;
> +}
> +
> +void V4L2Camera::freeBuffers()
> +{
> +	camera_->freeBuffers();
> +	bufferCount_ = 0;
> +}
> +
> +void V4L2Camera::streamOn(int *ret)
> +{
> +	*ret = 0;
> +
> +	if (isRunning_)
> +		return;
> +
> +	*ret = camera_->start();
> +	if (*ret < 0) {
> +		if (*ret == -EACCES)
> +			*ret = -EBUSY;
> +		return;
> +	}
> +	isRunning_ = true;
> +
> +	while (!pendingRequests_.empty()) {
> +		*ret = camera_->queueRequest(pendingRequests_.front());
> +		pendingRequests_.pop();
> +		if (*ret < 0)
> +			return;
> +	}

No error messages ? How does the error log looks like here ?

> +}
> +
> +void V4L2Camera::streamOff(int *ret)
> +{
> +	*ret = 0;
> +
> +	/* \todo restore buffers to reqbufs state? */
> +	if (!isRunning_)
> +		return;
> +
> +	*ret = camera_->stop();
> +	if (*ret < 0) {
> +		if (*ret == -EACCES)
> +			*ret = -EBUSY;
> +		return;
> +	}
> +	isRunning_ = false;
> +}
> +
> +void V4L2Camera::qbuf(int *ret, 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();
> +	if (request == nullptr) {

Nit: could you unify on a single pattern ? My preference would be for
        if (!request)

and I think the style guied suggests it as well

> +		LOG(V4L2Compat, Error) << "Can't create request";
> +		*ret = -ENOMEM;
> +		return;
> +	}
> +
> +	*ret = request->addBuffer(std::move(buffer));
> +	if (*ret < 0) {
> +		LOG(V4L2Compat, Error) << "Can't set buffer for request";
> +		*ret = -ENOMEM;
> +		return;
> +	}
> +
> +	if (!isRunning_) {
> +		pendingRequests_.push(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_ && !bufferSema_.tryAcquire())
> +		return -EAGAIN;
> +	else
> +		bufferSema_.acquire();
> +
> +	bufferLock_.lock();
> +	Buffer *buffer = completedBuffers_.front();
> +	completedBuffers_.pop();
> +	bufferLock_.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 = buffer->sequence();
> +
> +	arg->flags &= ~V4L2_BUF_FLAG_QUEUED;
> +	arg->flags &= ~V4L2_BUF_FLAG_DONE;

Shouldn't the FLAG_DONE bit be set ?

> +
> +	arg->length = sizeimage_;
> +
> +	return 0;
> +}
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> new file mode 100644
> index 00000000..73a427c4
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera.h
> @@ -0,0 +1,67 @@
> +/* 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 *streamConfig);
> +	void requestComplete(Request *request);
> +
> +	void mmap(void **ret, int *err, void *addr, size_t length,
> +		  int prot, 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 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 updateSizeImage();
> +
> +	std::shared_ptr<Camera> camera_;
> +	std::unique_ptr<CameraConfiguration> config_;
> +
> +	unsigned int bufferCount_;
> +	bool isRunning_;
> +	bool nonblock_;
> +
> +	unsigned int sizeimage_;
> +
> +	Semaphore bufferSema_;
> +	std::mutex bufferLock_;
> +
> +	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..66d558e4
> --- /dev/null
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -0,0 +1,452 @@
> +/* 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 <sys/mman.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/object.h>
> +
> +#include "log.h"
> +#include "utils.h"
> +#include "v4l2_camera.h"
> +#include "v4l2_compat_manager.h"
> +
> +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(V4L2Compat);
> +
> +V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
> +				 std::shared_ptr<Camera> camera)
> +	: index_(index), bufferCount_(0), currentBuf_(0),
> +	  vcam_(utils::make_unique<V4L2Camera>(camera))
> +{
> +	querycap(camera);
> +}
> +
> +V4L2CameraProxy::~V4L2CameraProxy()
> +{
> +}

Do you need to declare and empty destructor ?

> +
> +int V4L2CameraProxy::open(bool nonblock)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing OPEN";
> +
> +	int ret;
> +	vcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,
> +			    &ret, nonblock);
> +	if (ret < 0) {
> +		errno = -ret;
> +		return -1;
> +	}
> +
> +	vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> +			    ConnectionTypeBlocking, &streamConfig_);
> +	setFmtFromConfig();
> +
> +	return 0;
> +}
> +
> +int V4L2CameraProxy::close()
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing CLOSE";
> +
> +	int ret;
> +	vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);
> +	if (ret < 0) {
> +		errno = EIO;
> +		return -1;
> +	}
> +
> +	return ret;
> +}
> +
> +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> +			    off_t offset)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing MMAP";
> +
> +	void *val;
> +	int err;
> +	vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
> +			    &val, &err, addr, length, prot, offset);
> +	if (val == MAP_FAILED)
> +		errno = err;
> +	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);
> +	if (ret < 0) {
> +		errno = -ret;
> +		return -1;
> +	}
> +
> +	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)
> +{
> +	const std::vector<Size> &sizes = streamConfig_.formats().sizes(format);
> +	return std::find(sizes.begin(), sizes.end(), size) != sizes.end();
> +}
> +
> +bool V4L2CameraProxy::validateStreamType(uint32_t type)
> +{
> +	bool valid;
> +	vcam_->invokeMethod(&V4L2Camera::validStreamType,
> +			    ConnectionTypeBlocking, &valid, type);
> +	if (!valid)
> +		return false;
> +
> +	return true;

To answer you question on the reply to v1: if you prefer this stlye,
then it's fine with me. I would have
        return valid;
and that's it

> +}
> +
> +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> +{
> +	bool valid;
> +	vcam_->invokeMethod(&V4L2Camera::validMemoryType,
> +			    ConnectionTypeBlocking, &valid, memory);
> +	if (!valid)
> +		return false;
> +
> +	return true;
> +}
> +
> +void V4L2CameraProxy::setFmtFromConfig()
> +{
> +	curV4L2Format_.fmt.pix.width = streamConfig_.size.width;
> +	curV4L2Format_.fmt.pix.height = streamConfig_.size.height;
> +	curV4L2Format_.fmt.pix.pixelformat =
> +		V4L2CompatManager::drmToV4L2(streamConfig_.pixelFormat);
> +	curV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;
> +	curV4L2Format_.fmt.pix.bytesperline =
> +		V4L2CompatManager::bplMultiplier(
> +			curV4L2Format_.fmt.pix.pixelformat) *
> +		curV4L2Format_.fmt.pix.width;
> +	curV4L2Format_.fmt.pix.sizeimage =
> +		V4L2CompatManager::imageSize(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);

This should come from some library wide header I guess. Having it
hard-coded here won't scale. Not for this series probably, but is
worth a todo I guess

> +	capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;
> +	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);

If we don't want to introduce the format->description map now, add a
todo please

> +	arg->pixelformat =
> +		V4L2CompatManager::drmToV4L2(

Nit: fits on the previous line

> +			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::bplMultiplier(
> +			V4L2CompatManager::drmToV4L2(format)) *
> +		arg->fmt.pix.width;

Nit: hard to parse

	arg->fmt.pix.bytesperline = V4L2CompatManager::bplMultiplier(
			                V4L2CompatManager::drmToV4L2(format)) *
                                        arg->fmt.pix.width;

 ?

> +	arg->fmt.pix.sizeimage    =
> +		V4L2CompatManager::imageSize(
> +			V4L2CompatManager::drmToV4L2(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)
> +{
> +	int ret;
> +
> +	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";
> +		vcam_->invokeMethod(&V4L2Camera::streamOff,
> +				    ConnectionTypeBlocking, &ret);
> +		if (ret < 0) {
> +			LOG(V4L2Compat, Error) << "Failed to stop stream";
> +			return ret;
> +		}
> +		vcam_->invokeMethod(&V4L2Camera::freeBuffers,
> +				    ConnectionTypeBlocking);
> +		bufferCount_ = 0;
> +		return 0;
> +	}
> +
> +	vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> +			    &ret, &curV4L2Format_, arg->count);
> +	if (ret < 0)
> +		return -EINVAL;
> +	arg->count = streamConfig_.bufferCount;
> +	bufferCount_ = arg->count;
> +
> +	if (arg->memory != V4L2_MEMORY_MMAP)
> +		return -EINVAL;
> +
> +	vcam_->invokeMethod(&V4L2Camera::allocBuffers,
> +			    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;

Nit: could be made a single condition, up to you

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

So, the validation of the buffer and memory type is performed by
invoking a method on the V4L2Camera, while we here hardcode them in
the proxy... Not big, but I would rather keep the information here or
there (I would drop the method call for the validation, as so far we
only support one type)

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

Aren't there more ioctl calls ? Why are these two special ? Can't they
be catched by default like the others ?

> +	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")))
> +
> +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..90416b35
> --- /dev/null
> +++ b/src/v4l2/v4l2_compat_manager.cpp
> @@ -0,0 +1,382 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_compat_manager.cpp - V4L2 compatibility manager
> + */
> +#include "v4l2_compat_manager.h"
> +
> +#include <dlfcn.h>
> +#include <fcntl.h>
> +#include <fstream>
> +#include <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()
> +	: 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");
> +}
> +
> +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.
> +	 */
> +	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;
> +}
> +
> +V4L2CameraProxy *V4L2CompatManager::getCamera(int fd)
> +{
> +	auto device = devices_.find(fd);
> +	if (device == devices_.end())
> +		return nullptr;
> +
> +	return device->second.get();
> +}
> +
> +V4L2CameraProxy *V4L2CompatManager::getCamera(void *addr)
> +{
> +	auto map = mmaps_.find(addr);
> +	if (map == mmaps_.end())
> +		return nullptr;
> +
> +	return devices_.at(map->second).get();
> +}
> +
> +int V4L2CompatManager::getCameraIndex(int fd)
> +{
> +	struct stat statbuf;
> +	fstat(fd, &statbuf);
> +	unsigned int dev_major = major(statbuf.st_rdev);
> +	unsigned int dev_minor = minor(statbuf.st_rdev);
> +
> +	std::string name;
> +	std::ifstream nameFile;
> +	nameFile.open("/sys/dev/char/" + std::to_string(dev_major) + ":" +
> +		      std::to_string(dev_minor) + "/name");
> +	if (!nameFile)
> +		return -1;
> +
> +	std::getline(nameFile, name);
> +	nameFile.close();
> +
> +	unsigned int index = 0;
> +	for (auto &camera : cm_->cameras()) {
> +		if (!camera->name().compare(name))
> +			break;
> +		++index;
> +	}
> +
> +	if (index >= cm_->cameras().size())
> +		return -1;
> +
> +	return index;
> +}
> +
> +int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mode)
> +{
> +	int fd = openat_func_(dirfd, path, oflag, mode);
> +	if (fd < 0)
> +		return fd;
> +
> +	if (std::string(path).find("video") == std::string::npos)
> +		return fd;
> +
> +	if (!isRunning())
> +		init();
> +
> +	int ret = getCameraIndex(fd);
> +	if (ret < 0) {
> +		LOG(V4L2Compat, Error) << "No camera found for " << path;
> +		return fd;
> +	}
> +
> +	unsigned int camera_index = static_cast<unsigned int>(ret);
> +
> +	std::shared_ptr<V4L2CameraProxy> proxy = proxies_[camera_index];

You here share the ownership of the managed object owned by
proxies_[camera_index] with a local variable, increasing its
reference count

> +	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 efd;

I'm still not convinced about this, I'll go read your reply to v1
again.

> +
> +	devices_.emplace(efd, proxy);

And here you store it permanently in a map, making its reference count
permanently = 2 (it goes up to 3, then the local variable goes out of
scope and gest back to 2).
> +
> +	return efd;
> +}
> +
> +int V4L2CompatManager::dup(int oldfd)
> +{
> +	int newfd = dup_func_(oldfd);
> +	if (getCamera(oldfd))
> +		devices_[newfd] = devices_[oldfd];

Then here you create a new copy, with reference count = 3

At delete time you clear all the entries in devices_, causing the
shared pointer stored in proxies_ to get back to 1, then proxies_ goes
out of scope, and the managed object gets deleted.

Why all this dance ? Is the ownership of the CameraProxy shared
between different componentes? Is it created or destoryed outside of
this class ? I would save all this complexity and store
V4L2CameraProxy instances in a vector, and point to them in the map at
openat() time.

The only thing you care about is to increase the reference count to
the Camera, but you have a shared_ptr<> class instance in V4L2Camera,
so that's fine.

> +
> +	return newfd;
> +}
> +
> +int V4L2CompatManager::close(int fd)
> +{
> +	V4L2CameraProxy *proxy = getCamera(fd);
> +	if (proxy)
> +		return proxy->close();
> +
> +	int ret = close_func_(fd);
> +	return ret;
> +}
> +
> +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> +			      int fd, off_t offset)

Align to open ( please

Mostly stylistic comments, so I think we're almost there, at least
from my side!

Thanks
   j

> +{
> +	V4L2CameraProxy *proxy = getCamera(fd);
> +	if (!proxy)
> +		return mmap_func_(addr, length, prot, flags, fd, offset);
> +
> +	void *map = proxy->mmap(addr, length, prot, flags, offset);
> +	if (map == MAP_FAILED)
> +		return map;
> +
> +	mmaps_[addr] = fd;
> +	return map;
> +}
> +
> +int V4L2CompatManager::munmap(void *addr, size_t length)
> +{
> +	V4L2CameraProxy *proxy = getCamera(addr);
> +	if (!proxy)
> +		return munmap_func_(addr, length);
> +
> +	int ret = proxy->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)
> +{
> +	V4L2CameraProxy *proxy = getCamera(fd);
> +	if (!proxy)
> +		return ioctl_func_(fd, request, arg);
> +
> +	return proxy->ioctl(request, arg);
> +}
> +
> +/* \todo make libcamera export these */
> +int V4L2CompatManager::bplMultiplier(unsigned int format)
> +{
> +	switch (format) {
> +	case V4L2_PIX_FMT_NV12:
> +	case V4L2_PIX_FMT_NV21:
> +	case V4L2_PIX_FMT_NV16:
> +	case V4L2_PIX_FMT_NV61:
> +	case V4L2_PIX_FMT_NV24:
> +	case V4L2_PIX_FMT_NV42:
> +		return 1;
> +	case V4L2_PIX_FMT_BGR24:
> +	case V4L2_PIX_FMT_RGB24:
> +		return 3;
> +	case V4L2_PIX_FMT_ARGB32:
> +		return 4;
> +	case V4L2_PIX_FMT_VYUY:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_YUYV:
> +		return 2;
> +	default:
> +		return 0;
> +	};
> +}
> +
> +int V4L2CompatManager::imageSize(unsigned int format,
> +				 unsigned int width, unsigned int height)
> +{
> +	switch (format) {
> +	case V4L2_PIX_FMT_NV12:
> +	case V4L2_PIX_FMT_NV21:
> +		return width * height + width * height / 2;
> +	case V4L2_PIX_FMT_NV16:
> +	case V4L2_PIX_FMT_NV61:
> +		return width * height * 2;
> +	case V4L2_PIX_FMT_NV24:
> +	case V4L2_PIX_FMT_NV42:
> +		return width * height * 3;
> +	case V4L2_PIX_FMT_BGR24:
> +	case V4L2_PIX_FMT_RGB24:
> +		return width * height * 3;
> +	case V4L2_PIX_FMT_ARGB32:
> +		return width * height * 4;
> +	case V4L2_PIX_FMT_VYUY:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_YUYV:
> +		return width * height * 2;
> +	default:
> +		return 0;
> +	};
> +}
> +
> +unsigned int V4L2CompatManager::v4l2ToDrm(unsigned int pixelformat)
> +{
> +	switch (pixelformat) {
> +	/* RGB formats. */
> +	case V4L2_PIX_FMT_RGB24:
> +		return DRM_FORMAT_BGR888;
> +	case V4L2_PIX_FMT_BGR24:
> +		return DRM_FORMAT_RGB888;
> +	case V4L2_PIX_FMT_ARGB32:
> +		return DRM_FORMAT_BGRA8888;
> +
> +	/* YUV packed formats. */
> +	case V4L2_PIX_FMT_YUYV:
> +		return DRM_FORMAT_YUYV;
> +	case V4L2_PIX_FMT_YVYU:
> +		return DRM_FORMAT_YVYU;
> +	case V4L2_PIX_FMT_UYVY:
> +		return DRM_FORMAT_UYVY;
> +	case V4L2_PIX_FMT_VYUY:
> +		return DRM_FORMAT_VYUY;
> +
> +	/* YUY planar formats. */
> +	case V4L2_PIX_FMT_NV16:
> +		return DRM_FORMAT_NV16;
> +	case V4L2_PIX_FMT_NV61:
> +		return DRM_FORMAT_NV61;
> +	case V4L2_PIX_FMT_NV12:
> +		return DRM_FORMAT_NV12;
> +	case V4L2_PIX_FMT_NV21:
> +		return DRM_FORMAT_NV21;
> +	case V4L2_PIX_FMT_NV24:
> +		return DRM_FORMAT_NV24;
> +	case V4L2_PIX_FMT_NV42:
> +		return DRM_FORMAT_NV42;
> +	default:
> +		return pixelformat;
> +	};
> +}
> +
> +unsigned int V4L2CompatManager::drmToV4L2(unsigned int pixelformat)
> +{
> +	switch (pixelformat) {
> +	/* RGB formats. */
> +	case DRM_FORMAT_BGR888:
> +		return V4L2_PIX_FMT_RGB24;
> +	case DRM_FORMAT_RGB888:
> +		return V4L2_PIX_FMT_BGR24;
> +	case DRM_FORMAT_BGRA8888:
> +		return V4L2_PIX_FMT_ARGB32;
> +
> +	/* YUV packed formats. */
> +	case DRM_FORMAT_YUYV:
> +		return V4L2_PIX_FMT_YUYV;
> +	case DRM_FORMAT_YVYU:
> +		return V4L2_PIX_FMT_YVYU;
> +	case DRM_FORMAT_UYVY:
> +		return V4L2_PIX_FMT_UYVY;
> +	case DRM_FORMAT_VYUY:
> +		return V4L2_PIX_FMT_VYUY;
> +
> +	/* YUY planar formats. */
> +	case DRM_FORMAT_NV16:
> +		return V4L2_PIX_FMT_NV16;
> +	case DRM_FORMAT_NV61:
> +		return V4L2_PIX_FMT_NV61;
> +	case DRM_FORMAT_NV12:
> +		return V4L2_PIX_FMT_NV12;
> +	case DRM_FORMAT_NV21:
> +		return V4L2_PIX_FMT_NV21;
> +	case DRM_FORMAT_NV24:
> +		return V4L2_PIX_FMT_NV24;
> +	case DRM_FORMAT_NV42:
> +		return V4L2_PIX_FMT_NV42;
> +	default:
> +		return pixelformat;
> +	}
> +}
> diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h
> new file mode 100644
> index 00000000..d5ae7810
> --- /dev/null
> +++ b/src/v4l2/v4l2_compat_manager.h
> @@ -0,0 +1,86 @@
> +/* 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);
> +
> +	V4L2CameraProxy *getCamera(int fd);
> +	V4L2CameraProxy *getCamera(void *addr);
> +
> +	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 bplMultiplier(unsigned int format);
> +	static int imageSize(unsigned int format, unsigned int width,
> +			     unsigned int height);
> +
> +	static unsigned int v4l2ToDrm(unsigned int pixelformat);
> +	static unsigned int drmToV4L2(unsigned int pixelformat);
> +
> +private:
> +	V4L2CompatManager();
> +	~V4L2CompatManager();
> +
> +	void run() override;
> +	int getCameraIndex(int fd);
> +
> +	typedef int (*openat_func_t)(int dirfd, const char *path, int oflag, ...);
> +	typedef int (*dup_func_t)(int oldfd);
> +	typedef int (*close_func_t)(int fd);
> +	typedef int (*ioctl_func_t)(int fd, unsigned long request, ...);
> +	typedef void *(*mmap_func_t)(void *addr, size_t length, int prot,
> +				     int flags, int fd, off_t offset);
> +	typedef int (*munmap_func_t)(void *addr, size_t length);
> +
> +	openat_func_t openat_func_;
> +	dup_func_t    dup_func_;
> +	close_func_t  close_func_;
> +	ioctl_func_t  ioctl_func_;
> +	mmap_func_t   mmap_func_;
> +	munmap_func_t munmap_func_;
> +
> +	CameraManager *cm_;
> +
> +	std::mutex mutex_;
> +	std::condition_variable cv_;
> +
> +	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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191209/d65719d1/attachment-0001.sig>


More information about the libcamera-devel mailing list