[libcamera-devel] [PATCH v2 2/2] v4l2: v4l2_compat: Add V4L2 compatibility layer
Paul Elder
paul.elder at ideasonboard.com
Mon Dec 16 05:49:07 CET 2019
Hi Jacopo,
Thanks for the review.
On Mon, Dec 09, 2019 at 11:30:13AM +0100, Jacopo Mondi wrote:
> 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...
Without it it'll become libv4l2-compat.so
> > + 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 ?
I'm not sure. I put it in just in case. Is it ever possible that
V4L2CompatManager will be destroyed after, say, a qbuf but before a
corresponding dqbuf? Like if the V4L2 application exits non-gracefully?
> > +
> > + 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 ?
I thought viewfinder was always supported...?
The only way to query is via V4L2... querycap? It doesn't look like we
can use it for roles?
> > + 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
ack
> > + 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.
True...
> 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.
ack
> > + *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...
I could remove it before pushing it. It's really useful to see if it's
actually libcamera doing the work or if the V4L2 calls are going
directly to V4L2.
> > + *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 ?
pendingRequests_ is a queue of V4L2 qbuf requests that were enqueued
before V4L2 streamon, because in libcamera the order is opposite. So
when the V4L2 -> libcamera streamon is called, then v4l2-compat enqueues
those V4L2 qbuf bufs that were queued before the streamon.
Camera::queueRequest() returns 0 on success, so if they all succeed then
the final return value is 0. If any returns error, then we don't enqueue
any further and return the error. The error message would just be the
one from Camera::queueRequest... which now that you mention it might
look weird coming from a VIDIOC_STREAMON... The caller would think that
the streamon failed even though it didn't, because what actually failed
was enqueueing the prequeued buffers...
I'm not sure how best to resolve this... of the possible errors returned
by Camera::queueRequest(), ENODEV, EACCES (which would be translated to
EAGAIN), and ENOMEM are fine I think, but EINVAL would be really
confusing... :/
> > +}
> > +
> > +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
ack
> > + 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 ?
Uh yeah probably.
> > +
> > + 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 ?
I guess not.
> > +
> > +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
Ah I see.
> > +}
> > +
> > +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
I'll put it as a todo.
> > + 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
ack
> > + arg->pixelformat =
> > + V4L2CompatManager::drmToV4L2(
>
> Nit: fits on the previous line
ack
> > + 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
I used this weird indentation to show what is being grouped. So the one
that's indented is inside the parens of the parent indent, and the
arg->fmt.pix.width is outside of the parens, and is one of the arguments
to the multiplication operator.
> 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
Hmmmm I suppose...
> > +
> > + 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)
I was thinking the same thing...
I added the validation calls so we could scale (?) to multiple types
later on, if we decide to... That's why I had the DMABUF in there too
until I got rid of it last/this time.
> > + 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 ?
They're not special :)
> > + 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.
Hmm... I'm going to go with Laurent's suggestion...
tbh I'm not sure why I used shared pointers...
> > +
> > + 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
It is aligned :)
> Mostly stylistic comments, so I think we're almost there, at least
> from my side!
Thanks!
Paul
>
> > +{
> > + 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
More information about the libcamera-devel
mailing list