[libcamera-devel] [PATCH v2 2/2] v4l2: v4l2_compat: Add V4L2 compatibility layer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 17 01:31:35 CET 2019
Hi Paul,
Here's the second part of the review.
On Mon, Dec 09, 2019 at 11:30:13AM +0100, Jacopo Mondi wrote:
> 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
[snip]
> > 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();
> > + }
You can turn pendingRequests_ into a
std::queue<std::unique_ptr<Request>> and remove this.
> > +
> > + bufferLock_.lock();
> > + while (!completedBuffers_.empty()) {
> > + delete completedBuffers_.front();
> > + completedBuffers_.pop();
> > + }
Same for the completed buffers. Using std::unique_ptr, beside handling
automatic deletion, brings the nice benefit of making the object
ownership rules explicit.
> > + bufferLock_.unlock();
>
> Is locking required here ? The V4L2Camera is destroyed when the proxy
> is destroyed, no other calls should be in-flight. Are all buffers
> dequeued at this point ?
>
> > +
> > + camera_->release();
> > +}
> > +
> > +void V4L2Camera::open(int *ret, bool nonblock)
> > +{
> > + nonblock_ = nonblock;
s/nonblock/nonBlocking/g
But even better, I think, would be to pass the bool nonBlocking to
dqbuf().
> > +
> > + 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 think the camera is then allowed to pick a different role. But in any
case I think it's an issue to solve in libcamera if needed, for now this
should be fine.
> > + if (config_ == nullptr) {
if (!config_) {
> > + *ret = -EINVAL;
Shouldn't you release the camera here as open() fails ?
> > + return;
> > + }
> > +
> > + updateSizeImage();
> > +
> > + *ret = 0;
> > +}
> > +
> > +void V4L2Camera::close(int *ret)
> > +{
> > + *ret = camera_->release();
> > +}
> > +
> > +void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
> > +{
> > + *streamConfig = config_->at(0);
> > +}
> > +
> > +void V4L2Camera::updateSizeImage()
> > +{
> > + StreamConfiguration &streamConfig = config_->at(0);
> > + sizeimage_ =
> > + V4L2CompatManager::imageSize(
>
> nit: this could fit on the previous line
>
> > + V4L2CompatManager::drmToV4L2(streamConfig.pixelFormat),
> > + streamConfig.size.width,
> > + streamConfig.size.height);
> > +}
> > +
> > +void V4L2Camera::requestComplete(Request *request)
> > +{
> > + if (request->status() == Request::RequestCancelled) {
> > + LOG(V4L2Compat, Error) << "Request not succesfully completed: "
> > + << request->status();
This isn't really an error, it can happen when the camera is stopped
while requests are queued. I would drop the error message.
> > + return;
> > + }
> > +
> > + /* We only have one stream at the moment. */
> > + bufferLock_.lock();
> > + Buffer *buffer = request->buffers().begin()->second;
> > + completedBuffers_.push(buffer);
Quoting the documentation of Request::addBuffer(),
* Ownership of the buffer is passed to the request. It will be deleted when
* the request is destroyed after completing.
The buffer will be deleted once this method returns, and there's no copy
constructor for the Buffer class, so you will need to extract the
information you need from the buffer right here, and store them as a
custom class in the completedBuffers_ queue. That's just bytesused,
timestamp and sequence for now, but you also need to store the buffer
state (to handle BufferError correctly in dqbuf) and the index (for a
reason explained below). Once the buffer rework from Niklas lands, we'll
have a FrameMetadata class to store these, and it will be copyable. You
can actually already name your custom class FrameMetadata to prepare for
that.
> > + 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);
I think you should pass a Size and a PixelFormat to this function
instead of a v4l2_format, converting from V4L2 to DRM in the caller.
V4L2Camera would then deal with libcamera structures only, the
conversion to V4L2 structures would happen in V4L2CameraProxy.
> > + bufferCount_ = bufferCount;
> > + streamConfig.bufferCount = bufferCount_;
> > + /* \todo memoryType (interval vs external) */
> > +
> > + CameraConfiguration::Status validation = config_->validate();
> > + if (validation == CameraConfiguration::Invalid) {
> > + LOG(V4L2Compat, Error) << "Configuration invalid";
> > + *ret = -EINVAL;
> > + return;
> > + }
> > + if (validation == CameraConfiguration::Adjusted)
> > + LOG(V4L2Compat, Info) << "Configuration adjusted";
> > +
> > + LOG(V4L2Compat, Info) << "Validated configuration is: "
> > + << streamConfig.toString();
> > +
> > + *ret = camera_->configure(config_.get());
> > + if (*ret < 0)
> > + return;
> > +
> > + bufferCount_ = streamConfig.bufferCount;
> > +
> > + updateSizeImage();
> > + if (sizeimage_ == 0)
>
> I'm always a bit scared by functions that updates a global variable as
> side effect and let the caller responsability of making sure the
> intended variable is set to the desired value.
>
> I would rather make updateSizeImage() a calculateSizeImage(config) and
> assign it to the the global sizeimage_ in the caller, or make it
> return an error code.
>
> > + *ret = -EINVAL;
> > +}
> > +
> > +void V4L2Camera::mmap(void **ret, int *err, void *addr, size_t length, int prot, off_t offset)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing MMAP";
> > +
> > + if (prot != (PROT_READ | PROT_WRITE)) {
> > + *ret = MAP_FAILED;
> > + *err = ENOTSUP;
> > + return;
> > + }
You can move this check to the caller and remove the addr and prot
arguments from this function.
> > +
> > + unsigned int index = offset / sizeimage_;
> > + if (index * sizeimage_ != offset || length != sizeimage_) {
> > + *ret = MAP_FAILED;
> > + *err = EINVAL;
> > + return;
> > + }
I think sizeimage_ should be cached in V4L2CameraProxy, and this check
moved there. You can then replace the offset parameter with an index
parameter.
To cache sizeimage_ in the caller, I think you should make configure()
return the stream configuration (through an argument of course, not a
return value as you need to use invokeMethod()), and remove the
getStreamConfig() method.
> > +
> > + Stream *stream = *camera_->streams().begin();
> > + *ret = stream->buffers()[index].planes()[0].mem();
> > + *err = 0;
And you can also remove the err parameter as no error can be returned
anymore.
> > +}
> > +
> > +void V4L2Camera::munmap(int *ret, void *addr, size_t length)
> > +{
> > + *ret = 0;
> > +
> > + if (length != sizeimage_)
> > + *ret = -EINVAL;
> > +}
If sizeimage_ is cached in the V4L2CameraProxy you can move this check
to the caller and remove this method.
> > +
> > +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);
> > +}
Calling this through invokeMethod is a bit expensive for no real gain,
as the check is completely static. You can just inline these checks in
validateStreamType() and validateMemoryType().
The rationale for cross-thread calls with invokeMethod() is to avoid
races when a non thread-safe method of the Camera object has to be
called from the application thread, while the camera lives in the compat
manager thread. For everything else, and especially static validation,
you can perform direct calls, or even move some logic to
V4L2CameraProxy.
> > +
> > +void V4L2Camera::allocBuffers(int *ret, unsigned int count)
> > +{
> > + LOG(V4L2Compat, Debug) << "Allocating libcamera bufs";
>
> Empty line, to be consistent with other functions.
>
> But I actually wonder if we need this. i had the same in the android
> HAL and it's a useful tracing tool, so I understand you might want to
> keep them. After all, debug can be filtered out...
>
> > + *ret = camera_->allocateBuffers();
> > + if (*ret == -EACCES)
> > + *ret = -EBUSY;
> > +}
> > +
> > +void V4L2Camera::freeBuffers()
> > +{
> > + camera_->freeBuffers();
> > + bufferCount_ = 0;
> > +}
> > +
> > +void V4L2Camera::streamOn(int *ret)
> > +{
> > + *ret = 0;
> > +
> > + if (isRunning_)
> > + return;
> > +
> > + *ret = camera_->start();
> > + if (*ret < 0) {
> > + if (*ret == -EACCES)
> > + *ret = -EBUSY;
> > + return;
> > + }
> > + isRunning_ = true;
> > +
> > + while (!pendingRequests_.empty()) {
> > + *ret = camera_->queueRequest(pendingRequests_.front());
> > + pendingRequests_.pop();
> > + if (*ret < 0)
> > + return;
> > + }
>
> No error messages ? How does the error log looks like here ?
>
> > +}
> > +
> > +void V4L2Camera::streamOff(int *ret)
> > +{
> > + *ret = 0;
> > +
> > + /* \todo restore buffers to reqbufs state? */
This will need to be done, yes. I think you should track buffer states
in V4L2CameraProxy, likely with a vector of struct v4l2_buffer (let's
name it V4L2CameraProxy::buffers_). That way you can udpate the state of
each buffer when needed, and VIDIOC_QUERYBUF will return up-to-date
information with a minimum number of cross-thread calls.
Speaking of cross-thread calls, I think we should change the strategy a
little bit. What you're doing here is probably fine as a first step, and
what I'm proposing could be implemented on top, but you may want to
already go for it.
I think we need to introduce an updateBuffers() method in
V4L2CameraProxy and a completedBuffers() method in V4L2Camera.
The latter will simply move the contents of the completedBuffers_ queue
to a buffer and return it. You should do so protected by the
bufferLock_, so you can call it directly from V4L2CameraProxy without
going through invokeMethod(), as the method will just access a class
member variable without any risk of race thanks to the lock.
In V4L2CameraProxy::updateBuffers(), you should update each of the
buffers in buffers_ from the vector you receive from
V4L2Camera::completedBuffers(). That way buffers_ will be up-to-date
with the current state of the camera.
In V4L2CameraProxy::dqbuf(), you should first call
bufferSema_.tryAcquire() or bufferSema_.acquire() based on the
non-blocking mode (that should be stored in V4L2CameraProxy and not
V4L2Camera). There's no need for invokeMethod() there either, the
Semaphore class is thread-safe. dqbuf() then call updateBuffers(), and
finally dequeue the buffer. V4L2Camera::dqbuf() won't be needed anymore,
all the code that fills v4l2_buffer moves to updateBuffers() or
V4L2CameraProxy::vidioc_dqbuf().
Finally, V4L2CameraProxy::vidioc_querybuf() calls updateBuffers() and
then just copies the info from the requested entry in the buffers_
vector. V4L2CameraProxy::vidioc_streamoff also updates buffers_ to mark
all buffers as dequeued. And same for mmap and munmap, they should
update the buffer V4L2_BUF_FLAG_MAPPED flag.
Hopefully all this makes sense :-) The number of cross-thread calls
should be minimized, and you will be able to remove some state
information from V4L2Camera (such as bufferCount_, nonblock_ and
sizeimage_).
> > + 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)
Let's pass the buffer index only instead of struct v4l2_buffer.
arg->flags should then be updated in the caller of this method.
> > +{
> > + 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
Correct.
> > + 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";
You need to delete request here, or make request a
std::unique_ptr<Request>. I think that would be best as pendingRequests_
should be a queue of unique pointers.
> > + *ret = -ENOMEM;
> > + return;
> > + }
> > +
> > + if (!isRunning_) {
> > + pendingRequests_.push(request);
> > + } else {
> > + *ret = camera_->queueRequest(request);
This will become
*ret = camera_->queueRequest(request.get());
> > + if (*ret < 0) {
> > + LOG(V4L2Compat, Error) << "Can't queue request";
> > + if (*ret == -EACCES)
> > + *ret = -EBUSY;
> > + return;
> > + }
And here you will need a
request.release();
> > + }
> > +
> > + 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;
% 1000000
> > + arg->sequence = buffer->sequence();
> > +
> > + arg->flags &= ~V4L2_BUF_FLAG_QUEUED;
> > + arg->flags &= ~V4L2_BUF_FLAG_DONE;
>
> Shouldn't the FLAG_DONE bit be set ?
>
> > +
> > + arg->length = sizeimage_;
> > +
> > + return 0;
> > +}
> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > new file mode 100644
> > index 00000000..73a427c4
> > --- /dev/null
> > +++ b/src/v4l2/v4l2_camera.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_camera.h - V4L2 compatibility camera
> > + */
> > +#ifndef __V4L2_CAMERA_H__
> > +#define __V4L2_CAMERA_H__
> > +
> > +#include <linux/videodev2.h>
> > +#include <mutex>
> > +#include <queue>
> > +
> > +#include <libcamera/camera.h>
> > +#include "semaphore.h"
> > +
> > +using namespace libcamera;
> > +
> > +class V4L2Camera : public Object
> > +{
> > +public:
> > + V4L2Camera(std::shared_ptr<Camera> camera);
> > + ~V4L2Camera();
> > +
> > + void open(int *ret, bool nonblock);
> > + void close(int *ret);
> > + void getStreamConfig(StreamConfiguration *streamConfig);
> > + void requestComplete(Request *request);
This can be private.
> > +
> > + 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
> > + */
Missing blank line.
> > +#include "v4l2_camera_proxy.h"
> > +
> > +#include <algorithm>
> > +#include <linux/videodev2.h>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/object.h>
> > +
> > +#include "log.h"
> > +#include "utils.h"
> > +#include "v4l2_camera.h"
> > +#include "v4l2_compat_manager.h"
> > +
> > +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DECLARE_CATEGORY(V4L2Compat);
> > +
> > +V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
> > + std::shared_ptr<Camera> camera)
> > + : index_(index), bufferCount_(0), currentBuf_(0),
> > + vcam_(utils::make_unique<V4L2Camera>(camera))
> > +{
> > + querycap(camera);
> > +}
> > +
> > +V4L2CameraProxy::~V4L2CameraProxy()
> > +{
> > +}
>
> Do you need to declare and empty destructor ?
>
> > +
> > +int V4L2CameraProxy::open(bool nonblock)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing OPEN";
If you want to keep these debugging messages (we really need a tracing
infrastructure...) I would s/OPEN/open()/ and similarly below.
> > +
> > + 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;
Why not -ret ?
> > + return -1;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > + off_t offset)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing MMAP";
> > +
> > + void *val;
> > + int err;
> > + vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
> > + &val, &err, addr, length, prot, offset);
> > + if (val == MAP_FAILED)
> > + errno = err;
> > + return val;
> > +}
> > +
> > +int V4L2CameraProxy::munmap(void *addr, size_t length)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing MUNMAP";
> > +
> > + int ret;
> > + vcam_->invokeMethod(&V4L2Camera::munmap, ConnectionTypeBlocking,
> > + &ret, addr, length);
> > + if (ret < 0) {
> > + errno = -ret;
> > + return -1;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +bool V4L2CameraProxy::hasPixelFormat(unsigned int format)
> > +{
> > + const std::vector<PixelFormat> &formats =
> > + streamConfig_.formats().pixelformats();
> > + return std::find(formats.begin(), formats.end(), format) != formats.end();
> > +}
> > +
> > +/* \todo getDeviceCaps? getMemoryCaps? */
> > +
> > +bool V4L2CameraProxy::hasSize(unsigned int format, Size size)
> > +{
> > + const std::vector<Size> &sizes = streamConfig_.formats().sizes(format);
> > + return std::find(sizes.begin(), sizes.end(), size) != sizes.end();
> > +}
> > +
> > +bool V4L2CameraProxy::validateStreamType(uint32_t type)
> > +{
> > + bool valid;
> > + vcam_->invokeMethod(&V4L2Camera::validStreamType,
> > + ConnectionTypeBlocking, &valid, type);
> > + if (!valid)
> > + return false;
> > +
> > + return true;
>
> To answer you question on the reply to v1: if you prefer this stlye,
> then it's fine with me. I would have
> return valid;
> and that's it
>
> > +}
> > +
> > +bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> > +{
> > + bool valid;
> > + vcam_->invokeMethod(&V4L2Camera::validMemoryType,
> > + ConnectionTypeBlocking, &valid, memory);
> > + if (!valid)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +void V4L2CameraProxy::setFmtFromConfig()
> > +{
> > + curV4L2Format_.fmt.pix.width = streamConfig_.size.width;
> > + curV4L2Format_.fmt.pix.height = streamConfig_.size.height;
> > + curV4L2Format_.fmt.pix.pixelformat =
> > + V4L2CompatManager::drmToV4L2(streamConfig_.pixelFormat);
> > + curV4L2Format_.fmt.pix.field = V4L2_FIELD_NONE;
> > + curV4L2Format_.fmt.pix.bytesperline =
> > + V4L2CompatManager::bplMultiplier(
> > + curV4L2Format_.fmt.pix.pixelformat) *
> > + curV4L2Format_.fmt.pix.width;
> > + curV4L2Format_.fmt.pix.sizeimage =
> > + V4L2CompatManager::imageSize(curV4L2Format_.fmt.pix.pixelformat,
> > + curV4L2Format_.fmt.pix.width,
> > + curV4L2Format_.fmt.pix.height);
> > + curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > +}
> > +
> > +void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
> > +{
> > + std::string driver = "libcamera";
> > + std::string bus_info = driver + ":" + std::to_string(index_);
> > +
> > + memcpy(capabilities_.driver, driver.c_str(),
> > + sizeof(capabilities_.driver));
> > + memcpy(capabilities_.card, camera->name().c_str(),
> > + sizeof(capabilities_.card));
> > + memcpy(capabilities_.bus_info, bus_info.c_str(),
> > + sizeof(capabilities_.bus_info));
> > + capabilities_.version = KERNEL_VERSION(5, 2, 0);
>
> This should come from some library wide header I guess. Having it
> hard-coded here won't scale. Not for this series probably, but is
> worth a todo I guess
>
> > + capabilities_.device_caps = V4L2_CAP_VIDEO_CAPTURE;
> > + capabilities_.capabilities =
> > + capabilities_.device_caps | V4L2_CAP_DEVICE_CAPS;
> > + memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_QUERYCAP";
> > +
> > + memcpy(arg, &capabilities_, sizeof(*arg));
> > +
> > + return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_ENUM_FMT";
> > +
> > + if (!validateStreamType(arg->type))
> > + return -EINVAL;
> > + if (arg->index > streamConfig_.formats().pixelformats().size())
> > + return -EINVAL;
> > +
> > + memcpy(arg->description, "asdf", 5);
>
> If we don't want to introduce the format->description map now, add a
> todo please
>
> > + arg->pixelformat =
> > + V4L2CompatManager::drmToV4L2(
>
> Nit: fits on the previous line
>
> > + streamConfig_.formats().pixelformats()[arg->index]);
> > +
> > + return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_G_FMT";
> > +
> > + if (!validateStreamType(arg->type))
> > + return -EINVAL;
> > +
> > + arg->fmt.pix.width = curV4L2Format_.fmt.pix.width;
> > + arg->fmt.pix.height = curV4L2Format_.fmt.pix.height;
> > + arg->fmt.pix.pixelformat = curV4L2Format_.fmt.pix.pixelformat;
> > + arg->fmt.pix.field = curV4L2Format_.fmt.pix.field;
> > + arg->fmt.pix.bytesperline = curV4L2Format_.fmt.pix.bytesperline;
> > + arg->fmt.pix.sizeimage = curV4L2Format_.fmt.pix.sizeimage;
> > + arg->fmt.pix.colorspace = curV4L2Format_.fmt.pix.colorspace;
> > +
> > + return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_S_FMT";
> > +
> > + int ret = vidioc_try_fmt(arg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > + &ret, arg, bufferCount_);
> > + if (ret < 0)
> > + return -EINVAL;
> > +
> > + vcam_->invokeMethod(&V4L2Camera::getStreamConfig,
> > + ConnectionTypeBlocking, &streamConfig_);
> > + setFmtFromConfig();
> > +
> > + return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_try_fmt(struct v4l2_format *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_TRY_FMT";
> > + if (!validateStreamType(arg->type))
> > + return -EINVAL;
> > +
> > + unsigned int format = arg->fmt.pix.pixelformat;
> > + if (!hasPixelFormat(format))
> > + format = streamConfig_.formats().pixelformats()[0];
> > +
> > + Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> > + if (!hasSize(format, size))
> > + size = streamConfig_.formats().sizes(format)[0];
As hasPixelFormat() and hasSize() are only used here, I would inline
them.
> > +
> > + arg->fmt.pix.width = size.width;
> > + arg->fmt.pix.height = size.height;
> > + arg->fmt.pix.pixelformat = format;
> > + arg->fmt.pix.field = V4L2_FIELD_NONE;
> > + arg->fmt.pix.bytesperline =
> > + V4L2CompatManager::bplMultiplier(
> > + V4L2CompatManager::drmToV4L2(format)) *
> > + arg->fmt.pix.width;
>
> Nit: hard to parse
>
> arg->fmt.pix.bytesperline = V4L2CompatManager::bplMultiplier(
> V4L2CompatManager::drmToV4L2(format)) *
> arg->fmt.pix.width;
>
> ?
>
> > + arg->fmt.pix.sizeimage =
> > + V4L2CompatManager::imageSize(
> > + V4L2CompatManager::drmToV4L2(format),
> > + arg->fmt.pix.width, arg->fmt.pix.height);
> > + arg->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> > +
> > + return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> > +{
> > + int ret;
> > +
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_REQBUFS";
> > + if (!validateStreamType(arg->type))
> > + return -EINVAL;
> > + if (!validateMemoryType(arg->memory))
> > + return -EINVAL;
> > +
> > + LOG(V4L2Compat, Debug) << arg->count << " bufs requested ";
> > +
> > + arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > +
> > + if (arg->count == 0) {
> > + LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> > + vcam_->invokeMethod(&V4L2Camera::streamOff,
> > + ConnectionTypeBlocking, &ret);
> > + if (ret < 0) {
> > + LOG(V4L2Compat, Error) << "Failed to stop stream";
> > + return ret;
> > + }
> > + vcam_->invokeMethod(&V4L2Camera::freeBuffers,
> > + ConnectionTypeBlocking);
> > + bufferCount_ = 0;
> > + return 0;
> > + }
> > +
> > + vcam_->invokeMethod(&V4L2Camera::configure, ConnectionTypeBlocking,
> > + &ret, &curV4L2Format_, arg->count);
> > + if (ret < 0)
> > + return -EINVAL;
> > + arg->count = streamConfig_.bufferCount;
> > + bufferCount_ = arg->count;
> > +
> > + if (arg->memory != V4L2_MEMORY_MMAP)
> > + return -EINVAL;
> > +
> > + vcam_->invokeMethod(&V4L2Camera::allocBuffers,
> > + ConnectionTypeBlocking, &ret, arg->count);
> > + if (ret < 0) {
> > + arg->count = 0;
> > + return ret == -EACCES ? -EBUSY : ret;
> > + }
> > +
> > + LOG(V4L2Compat, Debug) << "Allocated " << arg->count << " buffers";
> > +
> > + return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_QUERYBUF";
> > + Stream *stream = streamConfig_.stream();
> > +
> > + if (!validateStreamType(arg->type))
> > + return -EINVAL;
> > + if (arg->index >= stream->buffers().size())
> > + return -EINVAL;
>
> Nit: could be made a single condition, up to you
>
> > +
> > + unsigned int index = arg->index;
> > + memset(arg, 0, sizeof(*arg));
> > + arg->index = index;
> > + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > + arg->length = curV4L2Format_.fmt.pix.sizeimage;
> > + arg->memory = V4L2_MEMORY_MMAP;
>
> So, the validation of the buffer and memory type is performed by
> invoking a method on the V4L2Camera, while we here hardcode them in
> the proxy... Not big, but I would rather keep the information here or
> there (I would drop the method call for the validation, as so far we
> only support one type)
>
> > + arg->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;
> > +
> > + return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_QBUF, index = "
> > + << arg->index;
> > +
> > + Stream *stream = streamConfig_.stream();
> > +
> > + if (!validateStreamType(arg->type))
> > + return -EINVAL;
> > + if (!validateMemoryType(arg->memory))
> > + return -EINVAL;
> > + if (arg->index >= stream->buffers().size())
> > + return -EINVAL;
> > +
> > + int ret;
> > + vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking,
> > + &ret, arg);
> > + return ret;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_DQBUF";
> > +
> > + if (!validateStreamType(arg->type))
> > + return -EINVAL;
> > + if (!validateMemoryType(arg->memory))
> > + return -EINVAL;
> > +
> > + arg->index = currentBuf_;
> > + currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> > +
> > + return vcam_->dqbuf(arg);
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_streamon(int *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_STREAMON";
> > +
> > + if (!validateStreamType(*arg))
> > + return -EINVAL;
> > +
> > + int ret;
> > + vcam_->invokeMethod(&V4L2Camera::streamOn,
> > + ConnectionTypeBlocking, &ret);
> > + return ret;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_streamoff(int *arg)
> > +{
> > + LOG(V4L2Compat, Debug) << "Servicing VIDIOC_STREAMOFF";
> > +
> > + if (!validateStreamType(*arg))
> > + return -EINVAL;
> > +
> > + int ret;
> > + vcam_->invokeMethod(&V4L2Camera::streamOff,
> > + ConnectionTypeBlocking, &ret);
> > + return ret;
> > +}
> > +
> > +int V4L2CameraProxy::ioctl(unsigned long request, void *arg)
> > +{
> > + int ret;
> > + switch (request) {
> > + case VIDIOC_QUERYCAP:
> > + ret = vidioc_querycap(static_cast<struct v4l2_capability *>(arg));
> > + break;
> > + case VIDIOC_ENUM_FMT:
> > + ret = vidioc_enum_fmt(static_cast<struct v4l2_fmtdesc *>(arg));
> > + break;
> > + case VIDIOC_G_FMT:
> > + ret = vidioc_g_fmt(static_cast<struct v4l2_format *>(arg));
> > + break;
> > + case VIDIOC_S_FMT:
> > + ret = vidioc_s_fmt(static_cast<struct v4l2_format *>(arg));
> > + break;
> > + case VIDIOC_TRY_FMT:
> > + ret = vidioc_try_fmt(static_cast<struct v4l2_format *>(arg));
> > + break;
> > + case VIDIOC_REQBUFS:
> > + ret = vidioc_reqbufs(static_cast<struct v4l2_requestbuffers *>(arg));
> > + break;
> > + case VIDIOC_QUERYBUF:
> > + ret = vidioc_querybuf(static_cast<struct v4l2_buffer *>(arg));
> > + break;
> > + case VIDIOC_QBUF:
> > + ret = vidioc_qbuf(static_cast<struct v4l2_buffer *>(arg));
> > + break;
> > + case VIDIOC_DQBUF:
> > + ret = vidioc_dqbuf(static_cast<struct v4l2_buffer *>(arg));
> > + break;
> > + case VIDIOC_STREAMON:
> > + ret = vidioc_streamon(static_cast<int *>(arg));
> > + break;
> > + case VIDIOC_STREAMOFF:
> > + ret = vidioc_streamoff(static_cast<int *>(arg));
> > + break;
> > + case VIDIOC_EXPBUF:
> > + case VIDIOC_ENUM_FRAMESIZES:
>
> Aren't there more ioctl calls ? Why are these two special ? Can't they
> be catched by default like the others ?
It's not strictly needed indeed, but I don't mind them as reminders that
those two ioctls are on our roadmap. A "\todo Implement the ioctls
below" would be even better.
> > + default:
> > + ret = -ENOTTY;
While not strictly necessary, I would add a break here for consistency.
> > + }
> > +
> > + if (ret < 0) {
> > + errno = -ret;
> > + return -1;
> > + }
> > +
> > + errno = 0;
Quoting the errno man page,
"The value of errno is never set to zero by any system call or library
function.".
> > + 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>
You should also include <memory> for std::shared_ptr, and <sys/types.h>
for off_t and size_t.
> > +
> > +#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__ */
[snip]
> > 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 @@
[snip]
> > +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);
O_NONBLOCK, as well as O_CLOEXEC below, are part of the flags, not of the
mode.
I think you should also check other flags and reject the ones that we
don't support (or maybe all flags but the ones we support).
> > + 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;
> > +}
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list