[libcamera-devel] [PATCH v3 5/6] v4l2: v4l2_compat: Add V4L2 compatibility layer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 31 13:18:22 CET 2019
Hi Paul,
On Mon, Dec 30, 2019 at 11:40:41PM -0600, Paul Elder wrote:
> On Sat, Dec 28, 2019 at 02:40:07AM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 27, 2019 at 03:50:31PM +0100, Jacopo Mondi wrote:
> > > On Mon, Dec 23, 2019 at 01:26:19AM -0600, Paul Elder wrote:
> > > > Add libcamera V4L2 compatibility layer.
> > > >
> > > > This initial implementation supports the minimal set of V4L2 operations,
> > > > which allows getting, setting, and enumerating formats, and streaming
> > > > frames from a video device. Some data about the wrapped V4L2 video
> > > > device are hardcoded.
> > > >
> > > > Add a build option named 'v4l2' and adjust the build system to
> > > > selectively compile the V4L2 compatibility layer.
> > > >
> > > > For now we match the V4L2 device node to a libcamera camera based on a
> > > > devnum that a device enumerator may optionally map to a libcamera
> >
> > Isn't it the pipeline handler that maps devnums to cameras, not the
> > device enumerator ?
> >
> > > > camera.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > >
> > > > ---
> > > > Major changes in v3:
> > > > - V4L2CompatManager::openat() verify video node and match to camera via
> > > > devnum instead of path
> > > > - add FrameMetadata class to encapsulate portions of the libcamera
> > > > Buffer that needs to be extracted to v4l2_buffer, and to prepare for
> > > > the Buffer rework
> > > > - V4L2CameraProxy refcount for tracking dups and closes (and open)
> > > > - add V4L2CompatManager::initialized_ to deal with race of waiting on
> > > > init cv after the cv is notified
> > > >
> > > > Less major changes in v3:
> > > > - change the list of pending Reqeusts (v4l2 buffers queued before
> > > > streamon) to unique pointers
> > > > - V4L2Camera::updateSizeImage() -> V4L2CameraProxy::calculateSizeImage()
> > > > - change V4L2CompatManager list of V4L2CameraProxy to unique_ptr, with
> > > > maps of fd/mmap addr to V4L2CameraProxy *
> > > > - removed cross-thread validation methods from V4L2CameraProxy
> > > > (validate[Stream|Memory]Type)
> > > > - removed hasPixelFormat() and hasSize() from V4L2CameraProxy
> > > > - moved mmap logic out of V4L2Camera and into V4L2CameraProxy
> > > > - moved nonblock logic out of V4L2Camera and into V4L2CameraProxy
> > > > - add todos
> > > > - oter cosmetic changes
> > > >
> > > > Changes in v2:
> > > > - move all errno acrobatics to V4L2CameraProxy
> > > > - remove all mentions of dmabuf
> > > > - make V4L2CompatManager::getCamera return pointer rather than
> > > > shared_ptr
> > > > - match V4L2 device nodes to libcamera cameras using Camera::name()
> > > > compared to /sys/dev/char/maj:min/name (only works for UVC cameras)
> > > > - in V4L2CompatManager::getCameraIndex()
> > > > - add -fvisibility=hidden to v4l2 compat
> > > > - cache the results of V4L2CompatManager::imageSize() within V4L2Camera
> > > > (where V4L2Camera is interested)
> > > > - remove V4L2CompatManager::valid[fd|mmap], and where those methods were
> > > > used, check V4L2CompatManager::getCamera() != nullptr instead
> > > > - fixed V4L2CompatManager::drmToV4L2() mappings for DRM_FORMAT_BGR888
> > > > and DRM_FORMAT_RGB888
> > > > - other cosmetic changes
> > > > ---
> > > > meson_options.txt | 5 +
> > > > src/meson.build | 4 +
> > > > src/v4l2/meson.build | 31 ++
> > > > src/v4l2/v4l2_camera.cpp | 248 ++++++++++++++++
> > > > src/v4l2/v4l2_camera.h | 84 ++++++
> > > > src/v4l2/v4l2_camera_proxy.cpp | 480 +++++++++++++++++++++++++++++++
> > > > src/v4l2/v4l2_camera_proxy.h | 70 +++++
> > > > src/v4l2/v4l2_compat.cpp | 78 +++++
> > > > src/v4l2/v4l2_compat_manager.cpp | 379 ++++++++++++++++++++++++
> > > > src/v4l2/v4l2_compat_manager.h | 86 ++++++
> > > > 10 files changed, 1465 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')
> >
> > 'Compile the V4L2 compatibility layer'
> >
> > (I incorrectly told you to disregard my whole comment in v2, when I
> > meant disregarding the s/compatibility/adaptation/)
> >
> > > > diff --git a/src/meson.build b/src/meson.build
> > > > index 67ad20aa..5adcd61f 100644
> > > > --- a/src/meson.build
> > > > +++ b/src/meson.build
> > > > @@ -6,3 +6,7 @@ subdir('libcamera')
> > > > subdir('ipa')
> > > > subdir('cam')
> > > > subdir('qcam')
> > > > +
> > > > +if get_option('v4l2')
> > > > + subdir('v4l2')
> > > > +endif
> > > > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> > > > new file mode 100644
> > > > index 00000000..14ee3594
> > > > --- /dev/null
> > > > +++ b/src/v4l2/meson.build
> > > > @@ -0,0 +1,31 @@
> > > > +v4l2_compat_sources = files([
> > > > + 'v4l2_camera.cpp',
> > > > + 'v4l2_camera_proxy.cpp',
> > > > + 'v4l2_compat.cpp',
> > > > + 'v4l2_compat_manager.cpp',
> > > > +])
> > > > +
> > > > +v4l2_compat_includes = [
> > > > + libcamera_includes,
> > > > + libcamera_internal_includes,
> > > > +]
> > > > +
> > > > +v4l2_compat_cpp_args = [
> > > > + # Meson enables large file support unconditionally, which redirect file
> > > > + # operations to 64-bit versions. This results in some symbols being
> > > > + # renamed, for instance open() being renamed to open64(). As the V4L2
> > > > + # adaptation wrapper needs to provide both 32-bit and 64-bit versions of
> > > > + # file operations, disable transparent large file support.
> > > > + '-U_FILE_OFFSET_BITS',
> > > > + '-D_FILE_OFFSET_BITS=32',
> > > > + '-fvisibility=hidden',
> > > > +]
> > > > +
> > > > +v4l2_compat = shared_library('v4l2-compat',
> > > > + v4l2_compat_sources,
> > > > + name_prefix : '',
> > > > + install : true,
> > > > + link_with : libcamera,
> > > > + include_directories : v4l2_compat_includes,
> > > > + dependencies : libcamera_deps,
> > > > + cpp_args : v4l2_compat_cpp_args)
> > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > > new file mode 100644
> > > > index 00000000..2d33be9f
> > > > --- /dev/null
> > > > +++ b/src/v4l2/v4l2_camera.cpp
> > > > @@ -0,0 +1,248 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_camera.cpp - V4L2 compatibility camera
> > > > + */
> > > > +
> > > > +#include "v4l2_camera.h"
> > > > +
> > > > +#include <errno.h>
> > > > +#include <linux/videodev2.h>
> > > > +#include <sys/mman.h>
> > > > +#include <sys/syscall.h>
> > > > +#include <time.h>
> > > > +#include <unistd.h>
> > > > +
> > > > +#include "log.h"
> > > > +#include "utils.h"
> > > > +#include "v4l2_compat_manager.h"
> > >
> > > I think you can now drop this.
> > >
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +LOG_DECLARE_CATEGORY(V4L2Compat);
> > > > +
> > > > +FrameMetadata::FrameMetadata(Buffer *buffer)
> > > > + : index_(buffer->index()),
> > > > + bytesused_(buffer->bytesused()),
> > > > + timestamp_(buffer->timestamp()),
> > > > + sequence_(buffer->sequence()),
> > > > + status_(buffer->status())
> > >
> > > please span 80cols
> > >
> > > > +{
> > > > +}
> > > > +
> > > > +V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> > > > + : camera_(camera), bufferCount_(0), isRunning_(false)
> > > > +{
> > > > + camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> > > > +};
> > >
> > > s/};/}
> > >
> > > > +
> > > > +V4L2Camera::~V4L2Camera()
> > > > +{
> > > > + camera_->release();
> > >
> > > Closing then destroying a V4L2Camera would cause a double release() ?
> >
> > It can, but I think that's fine, Camera::release() is a no-op in that
> > case.
> >
> > > > +}
> > > > +
> > > > +void V4L2Camera::open(int *ret)
> > > > +{
> > > > + if (camera_->acquire() < 0) {
> > > > + LOG(V4L2Compat, Error) << "Failed to acquire camera";
> > > > + *ret = -EINVAL;
> > > > + return;
> > > > + }
> > > > +
> > > > + config_ = camera_->generateConfiguration({ StreamRole::Viewfinder });
> > > > + if (!config_) {
> > > > + camera_->release();
> > > > + *ret = -EINVAL;
> > > > + return;
> > > > + }
> > > > +
> > > > + *ret = 0;
> > > > +}
> > > > +
> > > > +void V4L2Camera::close(int *ret)
> > > > +{
> > > > + *ret = camera_->release();
> > > > +}
> > > > +
> > > > +void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
> > > > +{
> > > > + *streamConfig = config_->at(0);
> > > > +}
> > > > +
> > > > +void V4L2Camera::requestComplete(Request *request)
> > > > +{
> > > > + if (request->status() == Request::RequestCancelled)
> > > > + return;
> > > > +
> > > > + /* We only have one stream at the moment. */
> > > > + bufferLock_.lock();
> > > > + Buffer *buffer = request->buffers().begin()->second;
> > > > + std::unique_ptr<FrameMetadata> fmd =
> >
> > s/fmd/metadata/ ?
> >
> > > > + utils::make_unique<FrameMetadata>(buffer);
> > > > + completedBuffers_.push(std::move(fmd));
> > > > + bufferLock_.unlock();
> > > > +
> > > > + bufferSema_.release();
> > > > +}
> > > > +
> > > > +void V4L2Camera::configure(int *ret, StreamConfiguration *streamConfigOut,
> > > > + Size *size, PixelFormat pixelformat,
> > > > + unsigned int bufferCount)
> > > > +{
> > > > + StreamConfiguration &streamConfig = config_->at(0);
> > > > + streamConfig.size.width = size->width;
> > > > + streamConfig.size.height = size->height;
> > > > + streamConfig.pixelFormat = 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;
> > > > +
> > > > + *streamConfigOut = config_->at(0);
> > > > +}
> > > > +
> > > > +void V4L2Camera::mmap(void **ret, unsigned int index)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing MMAP";
> > > > +
> > > > + Stream *stream = *camera_->streams().begin();
> > > > + *ret = stream->buffers()[index].planes()[0].mem();
> > >
> > > Should we check if index is not out of buffers() array bounds ?
> >
> > This is already checked by the caller.
> >
> > > > +}
> > > > +
> > > > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Allocating libcamera bufs";
> > > > +
> > > > + *ret = camera_->allocateBuffers();
> > > > + if (*ret == -EACCES)
> > > > + *ret = -EBUSY;
> > > > +}
> > > > +
> > > > +void V4L2Camera::freeBuffers()
> > > > +{
> > > > + camera_->freeBuffers();
> > > > + bufferCount_ = 0;
> > >
> > > is bufferCount_ used at all ?
> >
> > Not anymore in this class, it's probably a leftover.
> >
> > > > +}
> > > > +
> > > > +void V4L2Camera::streamOn(int *ret)
> > > > +{
> > > > + *ret = 0;
> > > > +
> > > > + if (isRunning_)
> > > > + return;
> > > > +
> > > > + *ret = camera_->start();
> > > > + if (*ret < 0) {
> > > > + if (*ret == -EACCES)
> > > > + *ret = -EBUSY;
> > > > + return;
> > > > + }
> > > > + isRunning_ = true;
> > > > +
> > > > + for (std::unique_ptr<Request> &req : pendingRequests_) {
> > > > + /* \todo What should we do if this returns -EINVAL? */
> >
> > Good point, we'll have to implement better error handling here. Thanks
> > for capturing it.
> >
> > > > + *ret = camera_->queueRequest(req.release());
> > > > + if (*ret < 0) {
> > > > + *ret = (*ret == -EACCES ? -EAGAIN : *ret);
> > > > + return;
> > > > + }
> > > > + }
> > > > +
> > > > + pendingRequests_.clear();
> > > > +}
> > > > +
> > > > +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, unsigned int index)
> > > > +{
> > > > + Stream *stream = config_->at(0).stream();
> > > > + std::unique_ptr<Buffer> buffer = stream->createBuffer(index);
> > > > + if (!buffer) {
> > > > + LOG(V4L2Compat, Error) << "Can't create buffer";
> > > > + *ret = -ENOMEM;
> > > > + return;
> > > > + }
> > > > +
> > > > + std::unique_ptr<Request> request =
> > > > + std::unique_ptr<Request>(camera_->createRequest());
> > > > + if (!request) {
> > > > + LOG(V4L2Compat, Error) << "Can't create request";
> > > > + *ret = -ENOMEM;
> > > > + return;
> > > > + }
> > > > +
> > > > + *ret = request->addBuffer(std::move(buffer));
> > > > + if (*ret < 0) {
> > > > + LOG(V4L2Compat, Error) << "Can't set buffer for request";
> > > > + *ret = -ENOMEM;
> > > > + return;
> > > > + }
> > > > +
> > > > + if (!isRunning_) {
> > > > + pendingRequests_.push_back(std::move(request));
> > > > + } else {
> > > > + *ret = camera_->queueRequest(request.release());
> > > > + if (*ret < 0) {
> > > > + LOG(V4L2Compat, Error) << "Can't queue request";
> > > > + if (*ret == -EACCES)
> > > > + *ret = -EBUSY;
> > > > + return;
> > > > + }
> > > > + }
> > > > +
> > > > + *ret = 0;
> > > > +}
> > > > +
> > > > +int V4L2Camera::dqbuf(struct v4l2_buffer *arg, bool nonblock)
> > > > +{
> > > > + if (nonblock && !bufferSema_.tryAcquire())
> > > > + return -EAGAIN;
> > > > + else
> > > > + bufferSema_.acquire();
> > > > +
> > > > + bufferLock_.lock();
> > > > + FrameMetadata *fmd = completedBuffers_.front().get();
> > > > + completedBuffers_.pop();
> > > > + bufferLock_.unlock();
> > > > +
> > > > + arg->bytesused = fmd->bytesused();
> > > > + arg->field = V4L2_FIELD_NONE;
> > > > + arg->timestamp.tv_sec = fmd->timestamp() / 1000000000;
> > > > + arg->timestamp.tv_usec = fmd->timestamp() % 1000000;
> > > > + arg->sequence = fmd->sequence();
> > >
> > > What about the buffer index ?
> >
> > It's set by the caller.
> >
> > I'll review the qbuf/dqbuf strategy on patch 6/6.
> >
> > > > +
> > > > + return 0;
> > > > +}
> > > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > > new file mode 100644
> > > > index 00000000..13418b6b
> > > > --- /dev/null
> > > > +++ b/src/v4l2/v4l2_camera.h
> > > > @@ -0,0 +1,84 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_camera.h - V4L2 compatibility camera
> > > > + */
> > > > +
> > > > +#ifndef __V4L2_CAMERA_H__
> > > > +#define __V4L2_CAMERA_H__
> > > > +
> > > > +#include <deque>
> > > > +#include <linux/videodev2.h>
> > > > +#include <mutex>
> > > > +#include <queue>
> > > > +
> > > > +#include <libcamera/buffer.h>
> > > > +#include <libcamera/camera.h>
> > > > +#include "semaphore.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +class FrameMetadata
> > > > +{
> > > > +public:
> > > > + FrameMetadata(Buffer *buffer);
> > > > +
> > > > + int index() const { return index_; }
> > > > +
> > > > + unsigned int bytesused() const { return bytesused_; }
> > > > + uint64_t timestamp() const { return timestamp_; }
> > > > + unsigned int sequence() const { return sequence_; }
> > > > +
> > > > + Buffer::Status status() const { return status_; }
> > > > +
> > > > +private:
> > > > + int index_;
> > > > +
> > > > + unsigned int bytesused_;
> > > > + uint64_t timestamp_;
> > > > + unsigned int sequence_;
> > > > +
> > > > + Buffer::Status status_;
> > > > +};
> > > > +
> > > > +class V4L2Camera : public Object
> > > > +{
> > > > +public:
> > > > + V4L2Camera(std::shared_ptr<Camera> camera);
> > > > + ~V4L2Camera();
> > > > +
> > > > + void open(int *ret);
> > > > + void close(int *ret);
> > > > + void getStreamConfig(StreamConfiguration *streamConfig);
> > > > +
> > > > + void mmap(void **ret, unsigned int index);
> > > > +
> > > > + void configure(int *ret, StreamConfiguration *streamConfigOut, Size *size,
> > > > + PixelFormat pixelformat, unsigned int bufferCount);
> > > > +
> > > > + void allocBuffers(int *ret, unsigned int count);
> > > > + void freeBuffers();
> > > > + void streamOn(int *ret);
> > > > + void streamOff(int *ret);
> > > > +
> > > > + void qbuf(int *ret, unsigned int index);
> > > > + int dqbuf(struct v4l2_buffer *arg, bool nonblock);
> > > > +
> > > > +private:
> > > > + void requestComplete(Request *request);
> > > > +
> > > > + std::shared_ptr<Camera> camera_;
> > > > + std::unique_ptr<CameraConfiguration> config_;
> > > > +
> > > > + unsigned int bufferCount_;
> > > > + bool isRunning_;
> > > > +
> > > > + Semaphore bufferSema_;
> > > > + std::mutex bufferLock_;
> > > > +
> > > > + std::deque<std::unique_ptr<Request>> pendingRequests_;
> > > > + std::queue<std::unique_ptr<FrameMetadata>> completedBuffers_;
> > > > +};
> > > > +
> > > > +#endif /* __V4L2_CAMERA_H__ */
> > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > > new file mode 100644
> > > > index 00000000..b0acd477
> > > > --- /dev/null
> > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > > @@ -0,0 +1,480 @@
> > > > +/* 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))
> >
> > You need to initialize refcount_ to 0.
> >
> > > > +{
> > > > + querycap(camera);
> > > > +}
> > > > +
> > > > +int V4L2CameraProxy::open(bool nonBlocking)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing open";
> > > > +
> > > > + int ret;
> > > > + vcam_->invokeMethod(&V4L2Camera::open, ConnectionTypeBlocking,
> > > > + &ret);
> > > > + if (ret < 0) {
> > > > + errno = -ret;
> > > > + return -1;
> > > > + }
> > > > +
> > > > + nonBlocking_ = nonBlocking;
> > > > +
> > > > + vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> > > > + ConnectionTypeBlocking, &streamConfig_);
> > > > + setFmtFromConfig(streamConfig_);
> > > > + sizeimage_ = calculateSizeImage(streamConfig_);
> > >
> > > sizeimage_ could be 0, which means the default format is not supported
> > > by our compat layer. Can this cause troubles later?
> >
> > Potentially, but that should not happen as the compat layer should
> > support all formats supported by libcamera. I wonder if we could
> > somehow enforce this at compilation time. Moving the DRM <-> V4L2
> > format conversion functions to the libcamera core could help in this
> > area. For now I think we can ignore the issue.
> >
> > > > +
> > > > + refcount_++;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void V4L2CameraProxy::dup()
> > > > +{
> > > > + refcount_++;
> > > > +}
> > > > +
> > > > +int V4L2CameraProxy::close()
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing close";
> > > > +
> > > > + if (refcount_ > 1) {
> > > > + refcount_--;
> >
> > You need to decrement refcount_ even if == 1. I would thus write
> >
> > if (--refcount_ > 0)
> > return 0;
> >
> > > > + return 0;
> > > > + }
> > >
> > > Should this be locked ?
> >
> > Eventually, but it's not the only thing that needs to be locked. It just
> > occurred to me that we are very thread-unsafe if the application using
> > the compat layer is threaded. That's OK, one step at a time :-) Let's
> > just remember about it.
> >
> > > > +
> > > > + int ret;
> > > > + vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking, &ret);
> > > > + if (ret < 0) {
> > > > + errno = EIO;
> > > > + return -1;
> > > > + }
> >
> > This ends up calling Camera::release() and forwarding the error up, and
> > the only possible cause of error is an invalid state of the camera,
> > which would result from a bug in the V4L2 adaptation layer. Let's
> > simplify the code (saving us from handling refcount_ specially in case
> > of close() errors) and avoid dealing with close() errors by making this
> > method and V4L2Camera::close() void.
> >
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > > > + off_t offset)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing mmap";
> > > > +
> > > > + if (prot != (PROT_READ | PROT_WRITE)) {
> > > > + errno = ENOTSUP;
> > > > + return MAP_FAILED;
> > > > + }
> > > > +
> > > > + unsigned int index = offset / sizeimage_;
> > > > + if (index * sizeimage_ != offset || length != sizeimage_) {
> > > > + errno = EINVAL;
> > > > + return MAP_FAILED;
> > > > + }
> > > > +
> > > > + void *val;
> > > > + vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
> > > > + &val, index);
> > > > + return val;
> > > > +}
> > > > +
> > > > +int V4L2CameraProxy::munmap(void *addr, size_t length)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing munmap";
> > > > +
> > > > + if (length != sizeimage_) {
> > > > + errno = EINVAL;
> > > > + return -1;
> > > > + }
> >
> > We should probably also keep a list of mapped addresses and check that
> > addr is in that list. This can be left for later, but please add a \todo
> > item in that case.
> >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/* \todo getDeviceCaps? getMemoryCaps? */
> >
> > What is this ?
> >
> > > > +
> > > > +bool V4L2CameraProxy::validateStreamType(uint32_t type)
> > > > +{
> > > > + return (type == V4L2_BUF_TYPE_VIDEO_CAPTURE);
> >
> > No need for parentheses.
> >
> > > > +}
> > > > +
> > > > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> > > > +{
> > > > + return (memory == V4L2_MEMORY_MMAP);
> >
> > Same here.
> >
> > > > +}
> > > > +
> > > > +void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig)
> > > > +{
> > > > + 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;
> > > > +}
> > > > +
> > > > +unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig)
> > > > +{
> > > > + return V4L2CompatManager::imageSize(
> > > > + V4L2CompatManager::drmToV4L2(streamConfig.pixelFormat),
> > > > + streamConfig.size.width,
> > > > + streamConfig.size.height);
> > > > +}
> > > > +
> > > > +void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
> >
> > This is an instance method, no need to pass camera as an argument, you
> > can use vcam_.
>
> I need access to camera for camera->name()... I could extract it in the
> constructor and pass it in as an argument maybe? (too late, as I just
> sent v4, though I prepared the below reply before)
>
> In any case, I can't get it from vcam_->camera_, since that's private,
> and I was under the impression that we didn't want friend classes :/
>
> Also this entire method looked a bit bulky to put into the constructor.
OK let's leave it as-is for now.
> > > > +{
> > > > + std::string driver = "libcamera";
> > > > + std::string bus_info = driver + ":" + std::to_string(index_);
> > > > +
> > > > + memcpy(capabilities_.driver, driver.c_str(),
> > > > + sizeof(capabilities_.driver));
> > > > + memcpy(capabilities_.card, camera->name().c_str(),
> > > > + sizeof(capabilities_.card));
> > > > + memcpy(capabilities_.bus_info, bus_info.c_str(),
> > > > + sizeof(capabilities_.bus_info));
> > > > + /* \todo Put this is header/config somewhere. */
> >
> > s/is/in/
> >
> > > > + capabilities_.version = KERNEL_VERSION(5, 2, 0);
> > > > + capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;
> > > > + capabilities_.capabilities =
> > > > + capabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;
> > >
> > > Nit:
> > > capabilities_.capabilities = capabilities_.device_caps |
> > > V4L2_CAP_DEVICE_CAPS;
> >
> > 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) ||
> > > > + arg->index > streamConfig_.formats().pixelformats().size())
> > > > + return -EINVAL;
> > > > +
> > > > + /* \todo Add map from format to description. */
> > > > + memcpy(arg->description, "asdf", 5);
> > >
> > > Could you at least provide a temporary name a little more meaningful ?
> >
> > Agreed.
> >
> > > > + arg->pixelformat = V4L2CompatManager::drmToV4L2(
> > > > + streamConfig_.formats().pixelformats()[arg->index]);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt";
> > > > +
> > > > + if (!validateStreamType(arg->type))
> > > > + return -EINVAL;
> > > > +
> > > > + 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;
> > >
> > > Nit:
> > > Couldn't you just assign arg->fmt.pix = curV4L2Format_.fmt.pix ?
> > > It will make it easier to support multiplanar ?
> >
> > v4l2_pix_format doesn't contain pointers, so this should be sage and
> > will simplify the code. Note that we should likely
> >
> > memset(&arg->fmt, 0, sizeof(arg->fmt));
> >
> > before the assignment.
> >
> > It will be interesting to run v4l2-compliance on this :-)
> >
> > > > +
> > > > + 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,
> > > > + &streamConfig_,
> > > > + new Size(arg->fmt.pix.width, arg->fmt.pix.height),
> >
> > You're leaking the newly allocate Size. Why don't you pass a const Size
> > & instead of a Size * ? Were you trying to work around a compilation
> > error ? :-)
> >
> > > > + V4L2CompatManager::v4l2ToDrm(arg->fmt.pix.pixelformat),
> > > > + bufferCount_);
> > > > + if (ret < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + sizeimage_ = calculateSizeImage(streamConfig_);
> > > > + if (sizeimage_ == 0)
> > > > + return -EINVAL;
> >
> > A VIDIOC_S_FMT error shouldn't change the active configuration of the
> > device. You thus need to work on a local copy of sizeimage_.
> >
> > > > +
> > > > + setFmtFromConfig(streamConfig_);
> >
> > You always call calculateSizeImage() and setFmtFromConfig() together,
> > and calculateSizeImage() actually duplicates some code from
> > setFmtFromConfig(). I would thus remove calculateSizeImage() and turn
> > setFmtFromConfig() into an int function that will not perform any change
> > to the active config if an error has to be returned. You could then
> > remove sizeimage_ and replace it with curV4L2Format_.fmt.pix.sizeimage
> > (with appropriate local variables if needed to keep the code readable).
>
> I've decided to keep them separate, because setFmtFromConfig does
> initialization in V4L2CameraProxy::open(), where sizeimage (until we fix
> v4l2 <-> drm conversions) may not be valid until we s_fmt/configure,
> but we still want the curV4L2Format_ to be populated.
curV4L2Format_ contains a fmt.pix.sizeimage field which duplicates
sizeimage_. I don't think we should have curV4L2Format_ only partly
valid. Furthermore I still don't see why we need two separate methods as
they're always called together with the same arguments.
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int V4L2CameraProxy::vidioc_try_fmt(struct v4l2_format *arg)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt";
> > > > + if (!validateStreamType(arg->type))
> > > > + return -EINVAL;
> > > > +
> > > > + unsigned int format = arg->fmt.pix.pixelformat;
> > > > + const std::vector<PixelFormat> &formats =
> > > > + streamConfig_.formats().pixelformats();
> > > > + if (std::find(formats.begin(), formats.end(), format) == formats.end())
> > > > + format = streamConfig_.formats().pixelformats()[0];
> > > > +
> > > > + Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> > > > + const std::vector<Size> &sizes = streamConfig_.formats().sizes(format);
> > > > + if (std::find(sizes.begin(), sizes.end(), size) == sizes.end())
> > > > + size = streamConfig_.formats().sizes(format)[0];
> > > > +
> > > > + arg->fmt.pix.width = size.width;
> > > > + arg->fmt.pix.height = size.height;
> > > > + arg->fmt.pix.pixelformat = format;
> > > > + arg->fmt.pix.field = V4L2_FIELD_NONE;
> > > > + arg->fmt.pix.bytesperline =
> > > > + 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);
> > >
> > > Nit:
> > >
> > > 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);
> >
> > I'm not sure it's much easier to read. The good news is that moving
> > bplMultiplier() and imageSize() to the V4L2CameraProxy class (as
> > explained below) will help.
> >
> > > > + 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) ||
> > > > + !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,
> > > > + &streamConfig_,
> > > > + new Size(curV4L2Format_.fmt.pix.width,
> > > > + curV4L2Format_.fmt.pix.height),
> > > > + V4L2CompatManager::v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),
> > > > + arg->count);
> > > > + if (ret < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + sizeimage_ = calculateSizeImage(streamConfig_);
> > > > + if (sizeimage_ == 0)
> > > > + return -EINVAL;
> > > > +
> > > > + setFmtFromConfig(streamConfig_);
> > >
> > > Do we need to go through configure() again ?
> >
> > I was going to ask the same :-) It is valid for a V4L2 application to
> > open a device and call VIDIOC_REQBUFS, so we need to handle that.
> > Calling configure() here is an option, but maybe setting an initial
> > configuration at open() time would be better.
>
> We need configure() again here because configuring the number of buffers
> in libcamera is done via configure(), while in V4L2 it is done via
> reqbufs, separate from s_fmt. This is also why we have bufferCount_.
>
> > > > +
> > > > + arg->count = streamConfig_.bufferCount;
> > > > + bufferCount_ = arg->count;
> > > > +
> > > > + if (arg->memory != V4L2_MEMORY_MMAP)
> > > > + return -EINVAL;
> > >
> > > I would fail earlier in this case.
> > > Ah wait, you do already, don't you?
> >
> > Good catch :-)
> >
> > > > +
> > > > + 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) ||
> > > > + 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) ||
> > > > + !validateMemoryType(arg->memory) ||
> > > > + arg->index >= stream->buffers().size())
> > > > + return -EINVAL;
> > > > +
> > > > + int ret;
> > > > + vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,
> > > > + &ret, arg->index);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + arg->flags |= V4L2_BUF_FLAG_QUEUED;
> > > > + arg->flags |= V4L2_BUF_FLAG_MAPPED;
> >
> > The V4L2_BUF_FLAG_MAPPED flag should be set at mmap() time and cleared
> > an munmap() time. It shouldn't be touched here.
> >
> > > > + arg->flags &= ~V4L2_BUF_FLAG_DONE;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> > > > +{
> > > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf";
> > > > +
> > > > + if (!validateStreamType(arg->type) ||
> > > > + !validateMemoryType(arg->memory))
> > > > + return -EINVAL;
> > > > +
> > > > + arg->index = currentBuf_;
> > > > + currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> > > > +
> > > > + int ret = vcam_->dqbuf(arg, nonBlocking_);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + arg->flags &= ~V4L2_BUF_FLAG_QUEUED;
> > > > + arg->flags |= V4L2_BUF_FLAG_DONE;
> > > > +
> > > > + arg->length = sizeimage_;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +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;
> > > > + default:
> > > > + ret = -ENOTTY;
> > > > + break;
> > > > + }
> > > > +
> > > > + if (ret < 0) {
> > > > + errno = -ret;
> > > > + return -1;
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
> > > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > > > new file mode 100644
> > > > index 00000000..51fdbe19
> > > > --- /dev/null
> > > > +++ b/src/v4l2/v4l2_camera_proxy.h
> > > > @@ -0,0 +1,70 @@
> > > > +/* 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 <memory>
> > > > +#include <sys/types.h>
> > > > +
> > > > +#include <libcamera/camera.h>
> > > > +
> > > > +#include "v4l2_camera.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +class V4L2CameraProxy
> > > > +{
> > > > +public:
> > > > + V4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera);
> > > > +
> > > > + int open(bool nonBlocking);
> > > > + void dup();
> > > > + 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);
> > > > +
> > > > + unsigned int refcount() { return refcount_; }
> >
> > This method is not used, you can drop it.
> >
> > > > +
> > > > +private:
> > > > + bool validateStreamType(uint32_t type);
> > > > + bool validateMemoryType(uint32_t memory);
> > > > + void setFmtFromConfig(StreamConfiguration &streamConfig);
> > > > + unsigned int calculateSizeImage(StreamConfiguration &streamConfig);
> > > > + void querycap(std::shared_ptr<Camera> camera);
> > > > +
> > > > + int vidioc_querycap(struct v4l2_capability *arg);
> > > > + int vidioc_enum_fmt(struct v4l2_fmtdesc *arg);
> > > > + int vidioc_g_fmt(struct v4l2_format *arg);
> > > > + int vidioc_s_fmt(struct v4l2_format *arg);
> > > > + int vidioc_try_fmt(struct v4l2_format *arg);
> > > > + int vidioc_reqbufs(struct v4l2_requestbuffers *arg);
> > > > + int vidioc_querybuf(struct v4l2_buffer *arg);
> > > > + int vidioc_qbuf(struct v4l2_buffer *arg);
> > > > + int vidioc_dqbuf(struct v4l2_buffer *arg);
> > > > + int vidioc_streamon(int *arg);
> > > > + int vidioc_streamoff(int *arg);
> > > > +
> > > > + unsigned int refcount_;
> > > > + unsigned int index_;
> > > > + bool nonBlocking_;
> > > > +
> > > > + struct v4l2_format curV4L2Format_;
> > > > + StreamConfiguration streamConfig_;
> > > > + struct v4l2_capability capabilities_;
> > > > + unsigned int bufferCount_;
> > > > + unsigned int currentBuf_;
> > > > + unsigned int sizeimage_;
> > > > +
> > > > + std::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..a6a7aed2
> > > > --- /dev/null
> > > > +++ b/src/v4l2/v4l2_compat.cpp
> > > > @@ -0,0 +1,78 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_compat.cpp - V4L2 compatibility layer
> > > > + */
> > > > +
> > > > +#include "v4l2_compat_manager.h"
> > > > +
> > > > +#include <errno.h>
> > > > +#include <fcntl.h>
> > > > +#include <stdarg.h>
> > > > +#include <sys/mman.h>
> > > > +#include <sys/stat.h>
> > > > +#include <sys/types.h>
> > > > +
> > > > +#define LIBCAMERA_PUBLIC __attribute__((visibility("default")))
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +#define extract_va_arg(type, arg, last) \
> > > > +{ \
> > > > + va_list ap; \
> > > > + va_start(ap, last); \
> > > > + arg = va_arg(ap, type); \
> > > > + va_end(ap); \
> > > > +}
> > > > +
> > > > +extern "C" {
> > > > +
> > > > +LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)
> > > > +{
> > > > + mode_t mode = 0;
> > > > + if (oflag & O_CREAT || oflag & O_TMPFILE)
> > > > + extract_va_arg(mode_t, mode, oflag);
> > > > +
> > > > + return V4L2CompatManager::instance()->openat(AT_FDCWD, path, oflag, mode);
> >
> > Could you wrap lines at a 80 columns boundary when feasible ?
> >
> > > > +}
> > > > +
> > > > +LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)
> > > > +{
> > > > + mode_t mode = 0;
> > > > + if (oflag & O_CREAT || oflag & O_TMPFILE)
> > > > + extract_va_arg(mode_t, mode, oflag);
> > > > +
> > > > + return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);
> > > > +}
> > > > +
> > > > +LIBCAMERA_PUBLIC int dup(int oldfd)
> > > > +{
> > > > + return V4L2CompatManager::instance()->dup(oldfd);
> > > > +}
> > > > +
> > > > +LIBCAMERA_PUBLIC int close(int fd)
> > > > +{
> > > > + return V4L2CompatManager::instance()->close(fd);
> > > > +}
> > > > +
> > > > +LIBCAMERA_PUBLIC void *mmap(void *addr, size_t length, int prot, int flags,
> > > > + int fd, off_t offset)
> > > > +{
> > > > + return V4L2CompatManager::instance()->mmap(addr, length, prot, flags, fd, offset);
> >
> > Same comment here.
> >
> > It's annoying that clang-format doesn't implement a soft boundary and a
> > hard boundary.
> >
> > > > +}
> > > > +
> > > > +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..bfe76e82
> > > > --- /dev/null
> > > > +++ b/src/v4l2/v4l2_compat_manager.cpp
> > > > @@ -0,0 +1,379 @@
> > > > +/* 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 <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), initialized_(false)
> > > > +{
> > > > + openat_func_ = (openat_func_t)dlsym(RTLD_NEXT, "openat");
> > > > + dup_func_ = (dup_func_t )dlsym(RTLD_NEXT, "dup");
> > > > + close_func_ = (close_func_t )dlsym(RTLD_NEXT, "close");
> > > > + ioctl_func_ = (ioctl_func_t )dlsym(RTLD_NEXT, "ioctl");
> > > > + mmap_func_ = (mmap_func_t )dlsym(RTLD_NEXT, "mmap");
> > > > + munmap_func_ = (munmap_func_t)dlsym(RTLD_NEXT, "munmap");
> > > > +}
> > > > +
> > > > +V4L2CompatManager::~V4L2CompatManager()
> > > > +{
> > > > + devices_.clear();
> > > > + mmaps_.clear();
> > > > + proxies_.clear();
> >
> > Wouldn't it be better to clear proxies_ after stopping the thread ? The
> > vector stores unique pointers, clearing it will delete all proxies,
> > which in turn deletes the V4L2Camera instances that are accessible from
> > the thread.
> >
> > > > +
> > > > + if (isRunning()) {
> > > > + exit(0);
> > > > + /* \todo Wait with a timeout, just in case. */
> > > > + wait();
> > > > + }
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::init()
> > > > +{
> > > > + start();
> > > > +
> > > > + MutexLocker locker(mutex_);
> > > > + cv_.wait(locker, []{ return V4L2CompatManager::instance()->initialized_; });
> >
> > We're in an instance method, so there's no need to go through
> > V4L2CompatManager::instance(), you can return initialized_; directly.
> > The compiler will then throw an error:
> >
> > ../../src/v4l2/v4l2_compat_manager.cpp: In lambda function:
> > ../../src/v4l2/v4l2_compat_manager.cpp:64:30: error: ‘this’ was not captured for this lambda function
> > cv_.wait(locker, []{ return initialized_; });
> > ^~~~~~~~~~~~
> > ../../src/v4l2/v4l2_compat_manager.cpp:64:30: error: invalid use of non-static data member ‘V4L2CompatManager::initialized_’
> > In file included from ../../src/v4l2/v4l2_compat_manager.cpp:8:
> > ../../src/v4l2/v4l2_compat_manager.h:79:7: note: declared here
> > bool initialized_;
> > ^~~~~~~~~~~~
> > In file included from ../../src/v4l2/v4l2_compat_manager.h:11,
> > from ../../src/v4l2/v4l2_compat_manager.cpp:8:
> > /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/g++-v8/condition_variable: In instantiation of ‘void std::condition_variable::wait(std::unique_lock<std::mutex>&, _Predicate) [with _Predicate = V4L2CompatManager::init()::<lambda()>]’:
> > ../../src/v4l2/v4l2_compat_manager.cpp:64:45: required from here
> > /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/g++-v8/condition_variable:98:13: error: could not convert ‘__p.V4L2CompatManager::init()::<lambda()>()’ from ‘void’ to ‘bool’
> > while (!__p())
> > ~~~^~
> > /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/g++-v8/condition_variable:98:9: error: in argument to unary !
> > while (!__p())
> > ^~~~~~
> >
> > which I assume led you to using V4L2CompatManager::instance(). The right
> > fix is to capture 'this' for the lambda function. You can read mode
> > about lambda functions and captures at
> > https://en.cppreference.com/w/cpp/language/lambda.
> >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void V4L2CompatManager::run()
> > > > +{
> > > > + cm_ = new CameraManager();
> > > > +
> > > > + int ret = cm_->start();
> > > > + if (ret) {
> > > > + LOG(V4L2Compat, Error) << "Failed to start camera manager: "
> > > > + << strerror(-ret);
> > > > + return;
> > > > + }
> > > > +
> > > > + LOG(V4L2Compat, Debug) << "Started camera manager";
> > > > +
> > > > + /*
> > > > + * For each Camera registered in the system, a V4L2CameraProxy gets
> > > > + * created here to wrap a camera device.
> > > > + */
> > > > + unsigned int index = 0;
> > > > + for (auto &camera : cm_->cameras()) {
> > > > + V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera);
> > > > + proxies_.emplace_back(proxy);
> > > > + ++index;
> > > > + }
> > > > +
> > > > + /*
> > > > + * libcamera has been initialized. Unlock the init() caller as we're
> > > > + * now ready to handle calls from the framework.
> > > > + */
> > > > + mutex_.lock();
> > > > + initialized_ = true;
> > > > + mutex_.unlock();
> > > > + cv_.notify_one();
> > > > +
> > > > + /* Now start processing events and messages. */
> > > > + exec();
> > > > +
> > > > + cm_->stop();
> > > > + delete cm_;
> > > > + cm_ = nullptr;
> > > > +}
> > > > +
> > > > +V4L2CompatManager *V4L2CompatManager::instance()
> > > > +{
> > > > + static V4L2CompatManager instance;
> > > > + return &instance;
> > > > +}
> > > > +
> > > > +V4L2CameraProxy *V4L2CompatManager::getCamera(int fd)
> > >
> > > or getProxy() ?
> >
> > Good idea.
> >
> > > > +{
> > > > + auto device = devices_.find(fd);
> > > > + if (device == devices_.end())
> > > > + return nullptr;
> > > > +
> > > > + return device->second;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::getCameraIndex(int fd)
> > > > +{
> > > > + struct stat statbuf;
> > > > + fstat(fd, &statbuf);
> > > > + unsigned int devMajor = major(statbuf.st_rdev);
> > > > + unsigned int devMinor = minor(statbuf.st_rdev);
> > > > +
> > > > + std::shared_ptr<Camera> target = cm_->get(makedev(devMajor, devMinor));
> > >
> > > Shouldn't you check for (target != nullptr)
> >
> > Agreed.
> >
> > > > +
> > > > + unsigned int index = 0;
> > > > + for (auto &camera : cm_->cameras()) {
> > > > + if (camera == target)
> > > > + break;
> > > > + ++index;
> > > > + }
> > > > +
> > > > + if (index >= cm_->cameras().size())
> > > > + return -1;
> > >
> > > This is a bit convoluted...
> > > A V4l2CameraProxy has the index and Camera, so once you got the target
> > > Camera from the CameraManager you could use std::find_if<> on proxies_
> > > with a V4L2CameraProxy::match(Camera *) function.
> >
> > Is it worth the additional complexity though, given that both are O(n)
> > anyway (and with a small n) ? I think we'll have to revisit this to
> > handle hotplug in any case, so I'd keep it as-is.
> >
> > > > +
> > > > + return index;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mode)
> > > > +{
> > > > + int fd = openat_func_(dirfd, path, oflag, mode);
> > > > + if (fd < 0)
> > > > + return fd;
> > > > +
> > > > + struct stat statbuf;
> > > > + fstat(fd, &statbuf);
> > > > + if (major(statbuf.st_rdev) != 81)
> >
> > In the very unlikely case that statbuf.st_rdev is initialized to 81 due
> > to random stack data, an fstat() failure won't be caught here. I woult
> > thus test
> >
> > if (ret < 0 || major(statbuf.st_rdev) != 81)
> >
> > For completeness you should also test that the device is a character
> > device and not a block device, so
> >
> > if (ret < 0 || (statbuf.st_mode & S_IFMT) != S_IFCHR ||
> > major(statbuf.st_rdev) != 81)
> >
> > > > + return fd;
> > > > +
> > > > + if (!isRunning())
> > > > + init();
> > > > +
> > > > + int ret = getCameraIndex(fd);
> > > > + if (ret < 0) {
> > > > + LOG(V4L2Compat, Info) << "No camera found for " << path;
> > > > + return fd;
> > >
> > > fd > 0 here
> > > and fd should be closed before returning (above here too)
> >
> > I think this is actually correct, the device isn't handled by libcamera,
> > so we forward openat().
> >
> > > > + }
> > > > +
> > > > + close_func_(fd);
> > > > +
> > > > + unsigned int camera_index = static_cast<unsigned int>(ret);
> > > > +
> > > > + V4L2CameraProxy *proxy = proxies_[camera_index].get();
> > > > + ret = proxy->open(oflag & O_NONBLOCK);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + int efd = eventfd(0, (oflag & O_CLOEXEC) | (oflag & O_NONBLOCK));
> >
> > Do you prefer this over
> >
> > int efd = eventfd(0, oflag & (O_CLOEXEC | O_NONBLOCK));
> >
> > for a specific reason ? There's no need to change it if you prefer your
> > version, I'm just curious.
> >
> > > > + if (efd < 0)
> > > > + return efd;
> > > > +
> > > > + devices_.emplace(efd, proxy);
> > > > +
> > > > + return efd;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::dup(int oldfd)
> > > > +{
> > > > + int newfd = dup_func_(oldfd);
> > > > + if (getCamera(oldfd)) {
> > > > + devices_[oldfd]->dup();
> > > > + devices_[newfd] = devices_[oldfd];
> > > > + }
> >
> > You would avoid a few lookups with
> >
> > auto device = devices_.find(oldfd);
> > if (device != devices_.end()) {
> > V4L2CameraProxy *proxy = device->second;
> > devices_[newfd] = proxy;
> > proxy->dup();
> > }
> >
> > > > +
> > > > + return newfd;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::close(int fd)
> > > > +{
> > > > + V4L2CameraProxy *proxy = getCamera(fd);
> > > > + if (proxy) {
> > > > + int ret = proxy->close();
> > > > + devices_.erase(fd);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return close_func_(fd);
> > > > +}
> > > > +
> > > > +void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> > > > + int fd, off_t offset)
> > > > +{
> > > > + 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_[map] = proxy;
> > > > + return map;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::munmap(void *addr, size_t length)
> > > > +{
> > > > + auto device = mmaps_.find(addr);
> > > > + if (device == mmaps_.end())
> > > > + return munmap_func_(addr, length);
> > > > +
> > > > + V4L2CameraProxy *proxy = device->second;
> > > > +
> > > > + int ret = proxy->munmap(addr, length);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + mmaps_.erase(device);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)
> > > > +{
> > > > + V4L2CameraProxy *proxy = 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..b8ae6efe
> > > > --- /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 <fcntl.h>
> > > > +#include <map>
> > > > +#include <mutex>
> > > > +#include <sys/types.h>
> > > > +#include <vector>
> > > > +
> > > > +#include <libcamera/camera.h>
> > > > +#include <libcamera/camera_manager.h>
> > > > +#include <libcamera/stream.h>
> > >
> > > You can drop stream.h and camera.h
> > >
> > > > +
> > > > +#include "thread.h"
> > > > +#include "v4l2_camera_proxy.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +class V4L2CompatManager : public Thread
> > > > +{
> > > > +public:
> > > > + static V4L2CompatManager *instance();
> > > > +
> > > > + int init();
> > > > +
> > > > + V4L2CameraProxy *getCamera(int fd);
> > > > + V4L2CameraProxy *getCamera(void *addr);
> > > > +
> > > > + int openat(int dirfd, const char *path, int oflag, mode_t mode);
> > > > +
> > > > + int dup(int oldfd);
> > > > + int close(int fd);
> > > > + void *mmap(void *addr, size_t length, int prot, int flags,
> > > > + int fd, off_t offset);
> > > > + int munmap(void *addr, size_t length);
> > > > + int ioctl(int fd, unsigned long request, void *arg);
> > > > +
> > > > + 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);
> >
> > Those four methods are now used by the V4L2CameraProxy class only, you
> > can move them there and make them private.
> >
> > > > +
> > > > +private:
> > > > + V4L2CompatManager();
> > > > + ~V4L2CompatManager();
> > > > +
> > > > + void run() override;
> > > > + int getCameraIndex(int fd);
> > > > +
> > > > + typedef int (*openat_func_t)(int dirfd, const char *path, int oflag, ...);
> > > > + typedef int (*dup_func_t)(int oldfd);
> > > > + typedef int (*close_func_t)(int fd);
> > > > + typedef int (*ioctl_func_t)(int fd, unsigned long request, ...);
> > > > + typedef void *(*mmap_func_t)(void *addr, size_t length, int prot,
> > > > + int flags, int fd, off_t offset);
> > > > + typedef int (*munmap_func_t)(void *addr, size_t length);
> > > > +
> > > > + openat_func_t openat_func_;
> > > > + dup_func_t dup_func_;
> > > > + close_func_t close_func_;
> > > > + ioctl_func_t ioctl_func_;
> > > > + mmap_func_t mmap_func_;
> > > > + munmap_func_t munmap_func_;
> > > > +
> > > > + CameraManager *cm_;
> > > > +
> > > > + std::mutex mutex_;
> > > > + std::condition_variable cv_;
> > > > + bool initialized_;
> > > > +
> > > > + std::vector<std::unique_ptr<V4L2CameraProxy>> proxies_;
> > > > + std::map<int, V4L2CameraProxy *> devices_;
> > > > + std::map<void *, V4L2CameraProxy *> mmaps_;
> > > > +};
> > > > +
> > > > +#endif /* __V4L2_COMPAT_MANAGER_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list