[libcamera-devel] [PATCH v2 02/17] v4l2: v4l2_compat: Support multiple open
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 20 03:02:24 CEST 2020
Hi Paul,
Thank you for the patch.
On Fri, Jun 19, 2020 at 02:41:08PM +0900, Paul Elder wrote:
> Previously, since we acquired the libcamera camera upon open(), it was
> impossible to support multiple open, as any subsequent opens would
> return error because the camera would already be acquired.
>
> To fix this, we first initialize the camera in the first call to
> V4L2CameraProxy::open(), just to heat up the stream format cache. We
> then add an exclusive lock on the V4L2Camera. All vidioc ioctls prior to
> reqbufs > 0 (except for s_fmt) are able to access the camera without
> this exclusive lock. A call to reqbufs > 0 (and s_fmt) will take the
> lock, and the lock will be released at reqbufs = 0.
So far so good.
> While the lock is
> taken, the eventfd that should be signaled (and cleared) by V4L2Camera
> and V4L2CameraProxy is set to the fd that has taken the lock, and is
> cleared when the lock is released. In case close() is called without a
> reqbufs = 0 first, the lock is also released on close().
In V4L2 poll() signalling is performed at the video device (== inode)
level for buffers, and at the file level for V4L2 events. This would be
tricky to emulate, as we would have to signal all fds, but there would
be a single DQBUF call that would acquire the semaphore. All the other
fds would then return immediately from a poll() call, without a wait to
reset the semaphore.
Given that poll()ing from a different fd (pointing to the same file or
even a different file) and the one that locks the device is really a
corner case, I don't think we need to address this issue until an
application sadly forces us to, but recording the explanation somewhere
would be useful, either in a comment in the code, or in the commit
message.
> We also use the V4L2CameraFile to contain all the information specific
> to an open instance of the file. This removes the need to keep track of
> such information within V4L2CameraProxy via multiple maps from int fd to
> info. We also handle dup in V4L2CompatManager instead, since that is
> just another int fd pointing to the same V4L2CameraFile instance.
> V4L2CompatManager is also modified to access the V4L2CameraProxy through
> the V4L2CameraFile.
The patch is fairly large, and contains multiple changes, I think I
would have split it (for instance passing V4L2CameraFile to the various
V4L2CameraProxy functions could have gone to a separate preparatory
patch). It would make review a bit easier. There's no need to resubmit
just for this.
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v2:
> - use V4L2CameraFile in V4L2CompatManager to deal with dup, and in
> V4L2CameraProxy to deal with information specific to the open file
> (nonblock and priority)
> - unlock exclusive camera lock on close()
> ---
> src/v4l2/v4l2_camera.cpp | 9 +-
> src/v4l2/v4l2_camera.h | 1 +
> src/v4l2/v4l2_camera_file.cpp | 4 +-
> src/v4l2/v4l2_camera_proxy.cpp | 201 ++++++++++++++++++++++---------
> src/v4l2/v4l2_camera_proxy.h | 47 +++++---
> src/v4l2/v4l2_compat_manager.cpp | 71 +++++------
> src/v4l2/v4l2_compat_manager.h | 4 +-
> 7 files changed, 215 insertions(+), 122 deletions(-)
>
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 9a1ebc8..2557320 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -17,7 +17,8 @@ using namespace libcamera;
> LOG_DECLARE_CATEGORY(V4L2Compat);
>
> V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
> - : camera_(camera), isRunning_(false), bufferAllocator_(nullptr)
> + : camera_(camera), isRunning_(false), bufferAllocator_(nullptr),
> + efd_(-1)
> {
> camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
> }
> @@ -29,7 +30,6 @@ V4L2Camera::~V4L2Camera()
>
> int V4L2Camera::open()
> {
> - /* \todo Support multiple open. */
> if (camera_->acquire() < 0) {
> LOG(V4L2Compat, Error) << "Failed to acquire camera";
> return -EINVAL;
> @@ -59,6 +59,11 @@ void V4L2Camera::bind(int efd)
> efd_ = efd;
> }
>
> +void V4L2Camera::unbind()
> +{
> + efd_ = -1;
> +}
> +
> void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
> {
> *streamConfig = config_->at(0);
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 33f5eb0..30114ed 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -40,6 +40,7 @@ public:
> int open();
> void close();
> void bind(int efd);
> + void unbind();
> void getStreamConfig(StreamConfiguration *streamConfig);
> std::vector<Buffer> completedBuffers();
>
> diff --git a/src/v4l2/v4l2_camera_file.cpp b/src/v4l2/v4l2_camera_file.cpp
> index 8916729..d07b936 100644
> --- a/src/v4l2/v4l2_camera_file.cpp
> +++ b/src/v4l2/v4l2_camera_file.cpp
> @@ -21,12 +21,12 @@ V4L2CameraFile::V4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy
> : priority_(V4L2_PRIORITY_DEFAULT), proxy_(proxy),
> nonBlocking_(nonBlocking), efd_(efd)
> {
> - proxy_->open(nonBlocking);
> + proxy_->open(this);
> }
>
> V4L2CameraFile::~V4L2CameraFile()
> {
> - proxy_->close();
> + proxy_->close(this);
> }
>
> V4L2CameraProxy *V4L2CameraFile::proxy()
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index bf47aa7..f06f58d 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -23,6 +23,7 @@
> #include "libcamera/internal/utils.h"
>
> #include "v4l2_camera.h"
> +#include "v4l2_camera_file.h"
> #include "v4l2_compat_manager.h"
>
> #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> @@ -34,45 +35,57 @@ LOG_DECLARE_CATEGORY(V4L2Compat);
> V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
> std::shared_ptr<Camera> camera)
> : refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),
> - vcam_(std::make_unique<V4L2Camera>(camera))
> + vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1),
> + acquiredCf_(nullptr), initialized_(false)
How about s/acquiredCf_/owner_/ ?
> {
> querycap(camera);
> }
>
> -int V4L2CameraProxy::open(bool nonBlocking)
> +int V4L2CameraProxy::open(V4L2CameraFile *cf)
s/cf/file/
and below too. cf isn't very explicit.
> {
> - LOG(V4L2Compat, Debug) << "Servicing open";
> + LOG(V4L2Compat, Debug) << "Servicing open fd = " << cf->efd();
>
> - int ret = vcam_->open();
> - if (ret < 0) {
> - errno = -ret;
> - return -1;
> - }
> + refcount_++;
> +
> + if (initialized_)
> + return 0;
>
> - nonBlocking_ = nonBlocking;
> + /*
> + * We will open the camera here, once, and keep it open until the last
> + * V4L2CameraFile is closed. Before any reqbufs with count > 0 is
> + * called, anybody can call any ioctl. Once reqbufs is called with
> + * count > 0, the * exclusive lock will be assigned to that
s/the \*/the/
> + * V4L2CameraFile, in acquiredCf_. At this point, no other fd can call
s/fd/file/
> + * any ioctl (except for querycap, try_fmt, g/s_priority, and
> + * enum/g/s_input), and they will return -EBUSY. After reqbufs is
> + * called with count = 0, the exclusive lock will be released.
> + */
I think it would be better to talk about owner than lock. That's the
concept used in V4L2.
/*
* We open the camera here, once, and keep it open until the last
* V4L2CameraFile is closed. The proxy is initially not owner by any
* file. The first file that calls reqbufs with count > 0 will become
* the owner, and no other file will be allowed to call buffer-related
* ioctls (except querybuf), set the format or start or stop the
* stream until ownership is released with a call to reqbufs with
* count = 0.
*/
One reason is that I believe we'll have to add locking in the future to
make the V4L2 compat layer thread-safe, and confusion between the owner
lock and the thread lock would then be likely.
> +
> + initialized_ = true;
> +
Should you increase refcount_ and set initialized_ after calling
vcam_->open() ?
> + int ret = vcam_->open();
> + if (ret < 0)
> + return ret;
>
> vcam_->getStreamConfig(&streamConfig_);
> setFmtFromConfig(streamConfig_);
> sizeimage_ = calculateSizeImage(streamConfig_);
>
> - refcount_++;
> -
> return 0;
> }
>
> -void V4L2CameraProxy::dup()
> +void V4L2CameraProxy::close(V4L2CameraFile *cf)
> {
> - refcount_++;
> -}
> + LOG(V4L2Compat, Debug) << "Servicing close fd = " << cf->efd();
>
> -void V4L2CameraProxy::close()
> -{
> - LOG(V4L2Compat, Debug) << "Servicing close";
> + unlock(cf);
>
> if (--refcount_ > 0)
> return;
>
> vcam_->close();
> +
> + initialized_ = false;
initialized_ seems to duplicated refcount_. How about dropping it, and
changing open() as follows ? You can then ignore my comment about
increasing refcount_ after vcam_->open(), but you will need to decrease
it in the error path.
- refcount_++;
-
- if (initialized_)
- return 0;
+ if (refcount_++)
+ return 0;
> }
>
> void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> @@ -221,9 +234,10 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)
> return 0;
> }
>
> -int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)
> +int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << cf->efd();
> +
>
> if (!validateBufferType(arg->type) ||
> arg->index >= streamConfig_.formats().pixelformats().size())
> @@ -237,9 +251,10 @@ int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)
> return 0;
> }
>
> -int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg)
> +int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << cf->efd();
> +
>
> if (!validateBufferType(arg->type))
> return -EINVAL;
> @@ -275,9 +290,13 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg)
> arg->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
> }
>
> -int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)
> +int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_s_fmt fd = " << cf->efd();
> +
> + int ret = lock(cf);
> + if (ret < 0)
> + return ret;
Should this be moved after validateBufferType() ?
>
> if (!validateBufferType(arg->type))
> return -EINVAL;
> @@ -285,9 +304,9 @@ int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)
> tryFormat(arg);
>
> Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> - int ret = vcam_->configure(&streamConfig_, size,
> - v4l2ToDrm(arg->fmt.pix.pixelformat),
> - bufferCount_);
> + ret = vcam_->configure(&streamConfig_, size,
> + v4l2ToDrm(arg->fmt.pix.pixelformat),
> + bufferCount_);
> if (ret < 0)
> return -EINVAL;
>
> @@ -302,9 +321,9 @@ int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)
> return 0;
> }
>
> -int V4L2CameraProxy::vidioc_try_fmt(struct v4l2_format *arg)
> +int V4L2CameraProxy::vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_try_fmt fd = " << cf->efd();
>
> if (!validateBufferType(arg->type))
> return -EINVAL;
> @@ -329,11 +348,13 @@ int V4L2CameraProxy::freeBuffers()
> return 0;
> }
>
> -int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> +int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffers *arg)
> {
> - int ret;
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs fd = " << cf->efd();
>
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_reqbufs";
> + int ret = lock(cf);
> + if (ret < 0)
> + return ret;
>
> if (!validateBufferType(arg->type) ||
> !validateMemoryType(arg->memory))
> @@ -342,9 +363,21 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> LOG(V4L2Compat, Debug) << arg->count << " buffers requested ";
>
> arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> + memset(arg->reserved, 0, sizeof(arg->reserved));
>
> - if (arg->count == 0)
> + if (arg->count == 0) {
> + unlock(cf);
> return freeBuffers();
> + }
> +
> + if (bufferCount_ > 0) {
> + ret = freeBuffers();
> + if (ret < 0) {
> + LOG(V4L2Compat, Error)
> + << "Failed to free libcamera buffers for re-reqbufs";
> + return ret;
> + }
> + }
The bufferCount_ > 0 part seems unrelated to $subject, should it be
moved to a different patch ?
>
> Size size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);
> ret = vcam_->configure(&streamConfig_, size,
> @@ -387,6 +420,7 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> buf.memory = V4L2_MEMORY_MMAP;
> buf.m.offset = i * curV4L2Format_.fmt.pix.sizeimage;
> buf.index = i;
> + buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
Same here.
>
> buffers_[i] = buf;
> }
> @@ -396,9 +430,13 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)
> return 0;
> }
>
> -int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)
> +int V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf fd = " << cf->efd();
> +
> + int ret = lock(cf);
> + if (ret < 0)
> + return ret;
querybuf should be callable from any file, not just the owner. Looks
like a missing test in v4l2-compliance ? :-)
>
> if (!validateBufferType(arg->type) ||
> arg->index >= bufferCount_)
> @@ -411,17 +449,21 @@ int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)
> return 0;
> }
>
> -int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)
> +int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)
> {
> LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = "
> - << arg->index;
> + << arg->index << " fd = " << cf->efd();
> +
> + int ret = lock(cf);
> + if (ret < 0)
> + return ret;
I wonder if a hasOwnership() function would make sense, to write
if (!hasOwnership(cf))
return -EBUSY;
lock()/acquire() would then only be called in reqbufs and s_fmt, and we
could call them at the end of the function, to avoid taking ownership in
case of failure. The function could actually be called setOwner(), and
cover both lock() and unlock() (when called with a null owner). Maybe
I'm too influenced by the implementation of vb2, but keeping a similar
scheme could help achieving more similar behaviours. What do you think ?
>
> if (!validateBufferType(arg->type) ||
> !validateMemoryType(arg->memory) ||
> arg->index >= bufferCount_)
> return -EINVAL;
>
> - int ret = vcam_->qbuf(arg->index);
> + ret = vcam_->qbuf(arg->index);
> if (ret < 0)
> return ret;
>
> @@ -431,15 +473,19 @@ int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)
> return ret;
> }
>
> -int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> +int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << cf->efd();
> +
> + int ret = lock(cf);
> + if (ret < 0)
> + return ret;
>
> if (!validateBufferType(arg->type) ||
> !validateMemoryType(arg->memory))
> return -EINVAL;
>
> - if (!nonBlocking_)
> + if (!(cf->nonBlocking()))
> vcam_->bufferSema_.acquire();
> else if (!vcam_->bufferSema_.tryAcquire())
> return -EAGAIN;
> @@ -455,16 +501,20 @@ int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
> currentBuf_ = (currentBuf_ + 1) % bufferCount_;
>
> uint64_t data;
> - int ret = ::read(efd_, &data, sizeof(data));
> + ret = ::read(efd_, &data, sizeof(data));
> if (ret != sizeof(data))
> LOG(V4L2Compat, Error) << "Failed to clear eventfd POLLIN";
>
> return 0;
> }
>
> -int V4L2CameraProxy::vidioc_streamon(int *arg)
> +int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *cf, int *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << cf->efd();
> +
> + int ret = lock(cf);
> + if (ret < 0)
> + return ret;
>
> if (!validateBufferType(*arg))
> return -EINVAL;
> @@ -474,14 +524,18 @@ int V4L2CameraProxy::vidioc_streamon(int *arg)
> return vcam_->streamOn();
> }
>
> -int V4L2CameraProxy::vidioc_streamoff(int *arg)
> +int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg)
> {
> - LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff";
> + LOG(V4L2Compat, Debug) << "Servicing vidioc_streamoff fd = " << cf->efd();
> +
> + int ret = lock(cf);
> + if (ret < 0)
> + return ret;
>
> if (!validateBufferType(*arg))
> return -EINVAL;
>
> - int ret = vcam_->streamOff();
> + ret = vcam_->streamOff();
>
> for (struct v4l2_buffer &buf : buffers_)
> buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> @@ -489,7 +543,7 @@ int V4L2CameraProxy::vidioc_streamoff(int *arg)
> return ret;
> }
>
> -int V4L2CameraProxy::ioctl(unsigned long request, void *arg)
> +int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg)
> {
> int ret;
> switch (request) {
> @@ -497,34 +551,34 @@ int V4L2CameraProxy::ioctl(unsigned long request, void *arg)
> ret = vidioc_querycap(static_cast<struct v4l2_capability *>(arg));
> break;
> case VIDIOC_ENUM_FMT:
> - ret = vidioc_enum_fmt(static_cast<struct v4l2_fmtdesc *>(arg));
> + ret = vidioc_enum_fmt(cf, static_cast<struct v4l2_fmtdesc *>(arg));
> break;
> case VIDIOC_G_FMT:
> - ret = vidioc_g_fmt(static_cast<struct v4l2_format *>(arg));
> + ret = vidioc_g_fmt(cf, static_cast<struct v4l2_format *>(arg));
> break;
> case VIDIOC_S_FMT:
> - ret = vidioc_s_fmt(static_cast<struct v4l2_format *>(arg));
> + ret = vidioc_s_fmt(cf, static_cast<struct v4l2_format *>(arg));
> break;
> case VIDIOC_TRY_FMT:
> - ret = vidioc_try_fmt(static_cast<struct v4l2_format *>(arg));
> + ret = vidioc_try_fmt(cf, static_cast<struct v4l2_format *>(arg));
> break;
> case VIDIOC_REQBUFS:
> - ret = vidioc_reqbufs(static_cast<struct v4l2_requestbuffers *>(arg));
> + ret = vidioc_reqbufs(cf, static_cast<struct v4l2_requestbuffers *>(arg));
> break;
> case VIDIOC_QUERYBUF:
> - ret = vidioc_querybuf(static_cast<struct v4l2_buffer *>(arg));
> + ret = vidioc_querybuf(cf, static_cast<struct v4l2_buffer *>(arg));
> break;
> case VIDIOC_QBUF:
> - ret = vidioc_qbuf(static_cast<struct v4l2_buffer *>(arg));
> + ret = vidioc_qbuf(cf, static_cast<struct v4l2_buffer *>(arg));
> break;
> case VIDIOC_DQBUF:
> - ret = vidioc_dqbuf(static_cast<struct v4l2_buffer *>(arg));
> + ret = vidioc_dqbuf(cf, static_cast<struct v4l2_buffer *>(arg));
> break;
> case VIDIOC_STREAMON:
> - ret = vidioc_streamon(static_cast<int *>(arg));
> + ret = vidioc_streamon(cf, static_cast<int *>(arg));
> break;
> case VIDIOC_STREAMOFF:
> - ret = vidioc_streamoff(static_cast<int *>(arg));
> + ret = vidioc_streamoff(cf, static_cast<int *>(arg));
> break;
> default:
> ret = -ENOTTY;
> @@ -539,10 +593,37 @@ int V4L2CameraProxy::ioctl(unsigned long request, void *arg)
> return ret;
> }
>
> -void V4L2CameraProxy::bind(int fd)
> +/**
> + * \brief Acquire an exclusive lock on the V4L2Camera
> + *
> + * \return Zero on success or if already acquired, and negative error on
> + * failure.
> + */
> +int V4L2CameraProxy::lock(V4L2CameraFile *cf)
If we go for the concept of owner, I would call this acquire() and the
next function release(). The comment above should then state "Acquire
exclusive ownership of the V4L2Camera" or something similar.
> +{
> + if (acquiredCf_ == cf)
> + return 0;
> +
> + if (acquiredCf_)
> + return -EBUSY;
> +
> + efd_ = cf->efd();
Do we still need to store the fd in efd_ ? It duplicates acquiredCf_,
can't we access the fd from acquiredCf_ when needed ?
> + vcam_->bind(efd_);
> +
> + acquiredCf_ = cf;
> +
> + return 0;
> +}
> +
> +void V4L2CameraProxy::unlock(V4L2CameraFile *cf)
> {
> - efd_ = fd;
> - vcam_->bind(fd);
> + if (acquiredCf_ != cf)
> + return;
> +
> + efd_ = -1;
> + vcam_->unbind();
> +
> + acquiredCf_ = nullptr;
> }
>
> struct PixelFormatPlaneInfo {
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 27d3e50..43290ca 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -21,20 +21,19 @@
>
> using namespace libcamera;
>
> +class V4L2CameraFile;
> +
> class V4L2CameraProxy
> {
> public:
> V4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera);
>
> - int open(bool nonBlocking);
> - void dup();
> - void close();
> + int open(V4L2CameraFile *cfile);
> + void close(V4L2CameraFile *cf);
> void *mmap(void *addr, size_t length, int prot, int flags, off64_t offset);
> int munmap(void *addr, size_t length);
>
> - int ioctl(unsigned long request, void *arg);
> -
> - void bind(int fd);
> + int ioctl(V4L2CameraFile *cf, unsigned long request, void *arg);
>
> private:
> bool validateBufferType(uint32_t type);
> @@ -47,16 +46,19 @@ private:
> int freeBuffers();
>
> 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);
> + int vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *arg);
> + int vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);
> + int vidioc_s_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);
> + int vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);
> + int vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffers *arg);
> + int vidioc_querybuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);
> + int vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);
> + int vidioc_dqbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);
> + int vidioc_streamon(V4L2CameraFile *cf, int *arg);
> + int vidioc_streamoff(V4L2CameraFile *cf, int *arg);
> +
> + int lock(V4L2CameraFile *cf);
> + void unlock(V4L2CameraFile *cf);
>
> static unsigned int bplMultiplier(uint32_t format);
> static unsigned int imageSize(uint32_t format, unsigned int width,
> @@ -67,7 +69,6 @@ private:
>
> unsigned int refcount_;
> unsigned int index_;
> - bool nonBlocking_;
>
> struct v4l2_format curV4L2Format_;
> StreamConfiguration streamConfig_;
> @@ -82,6 +83,18 @@ private:
> std::unique_ptr<V4L2Camera> vcam_;
>
> int efd_;
> +
> + /*
> + * This is an exclusive lock on the camera. When this is not taken,
> + * anybody can call any ioctl before reqbufs. reqbufs with count > 0
> + * will claim this lock, and reqbufs with count = 0 will release it.
> + * Any ioctl is called while the lock is taken will return -EBUSY
> + * (if applicable, eg. try_fmt, querycap, g/s_priority, and
> + * enum/g/s_input are exempt).
This could would also need to be updated if we use the concept of
ownership.
> + */
> + V4L2CameraFile *acquiredCf_;
> +
> + bool initialized_;
> };
>
> #endif /* __V4L2_CAMERA_PROXY_H__ */
> diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> index 8da3316..9cede76 100644
> --- a/src/v4l2/v4l2_compat_manager.cpp
> +++ b/src/v4l2/v4l2_compat_manager.cpp
> @@ -24,6 +24,8 @@
>
> #include "libcamera/internal/log.h"
>
> +#include "v4l2_camera_file.h"
> +
> using namespace libcamera;
>
> LOG_DEFINE_CATEGORY(V4L2Compat)
> @@ -49,7 +51,7 @@ V4L2CompatManager::V4L2CompatManager()
>
> V4L2CompatManager::~V4L2CompatManager()
> {
> - devices_.clear();
> + cameraFiles_.clear();
How about naming this just files_, and the various cameraFile variables
file ? All the files are instances of V4L2CameraFile, I don't think a
camera prefix clears up any possible confusion.
> mmaps_.clear();
>
> if (cm_) {
> @@ -95,13 +97,13 @@ V4L2CompatManager *V4L2CompatManager::instance()
> return &instance;
> }
>
> -V4L2CameraProxy *V4L2CompatManager::getProxy(int fd)
> +std::shared_ptr<V4L2CameraFile> V4L2CompatManager::getCameraFile(int fd)
> {
> - auto device = devices_.find(fd);
> - if (device == devices_.end())
> - return nullptr;
> + auto cameraFile = cameraFiles_.find(fd);
> + if (cameraFile == cameraFiles_.end())
> + return std::shared_ptr<V4L2CameraFile>();
>
> - return device->second;
> + return cameraFile->second;
> }
>
> int V4L2CompatManager::getCameraIndex(int fd)
> @@ -148,25 +150,17 @@ int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mod
>
> fops_.close(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, EFD_SEMAPHORE |
> ((oflag & O_CLOEXEC) ? EFD_CLOEXEC : 0) |
> ((oflag & O_NONBLOCK) ? EFD_NONBLOCK : 0));
> - if (efd < 0) {
> - int err = errno;
> - proxy->close();
> - errno = err;
> + if (efd < 0)
> return efd;
> - }
>
> - proxy->bind(efd);
> - devices_.emplace(efd, proxy);
> + unsigned int camera_index = static_cast<unsigned int>(ret);
> +
> + V4L2CameraProxy *proxy = proxies_[camera_index].get();
I think you can skip the local camera_index variable and use ret
directly. If you prefer keeping it, I'd name it index, and remove the
blank line between the two.
> +
And this blank line too :-)
> + cameraFiles_.emplace(efd, std::make_shared<V4L2CameraFile>(efd, oflag & O_NONBLOCK, proxy));
I wonder if V4L2CameraFile could later need to support more flags, in
which case passing it oflag may be useful already. the nonBlocking()
method would return flags_ & O_NONBLOCK. It would also shorten this line
(OK, it's cheating ;-)).
>
> return efd;
> }
> @@ -177,40 +171,39 @@ int V4L2CompatManager::dup(int oldfd)
> if (newfd < 0)
> return newfd;
>
> - auto device = devices_.find(oldfd);
> - if (device != devices_.end()) {
> - V4L2CameraProxy *proxy = device->second;
> - devices_[newfd] = proxy;
> - proxy->dup();
> - }
> + auto cameraFile = cameraFiles_.find(oldfd);
> + if (cameraFile != cameraFiles_.end())
> + cameraFiles_[newfd] = cameraFile->second;
>
> return newfd;
> }
>
> int V4L2CompatManager::close(int fd)
> {
> - V4L2CameraProxy *proxy = getProxy(fd);
> - if (proxy) {
> - proxy->close();
> - devices_.erase(fd);
> - return 0;
> - }
> + std::shared_ptr<V4L2CameraFile> cameraFile = getCameraFile(fd);
> + if (cameraFile)
> + cameraFiles_.erase(fd);
auto file = files_.find(fd);
if (file != files_.end())
files_.erase(file);
That will avoid a double lookup.
>
> + /* We still need to close the eventfd. */
> return fops_.close(fd);
> }
>
> void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> int fd, off64_t offset)
> {
> - V4L2CameraProxy *proxy = getProxy(fd);
> - if (!proxy)
> + std::shared_ptr<V4L2CameraFile> cameraFile = getCameraFile(fd);
> + if (!cameraFile)
> return fops_.mmap(addr, length, prot, flags, fd, offset);
>
> - void *map = proxy->mmap(addr, length, prot, flags, offset);
> + void *map = cameraFile->proxy()->mmap(addr, length, prot, flags, offset);
> if (map == MAP_FAILED)
> return map;
>
> - mmaps_[map] = proxy;
> + /*
> + * Map to V4L2CameraProxy directly to prevent adding more references
> + * to V4L2CameraFile.
> + */
> + mmaps_[map] = cameraFile->proxy();
> return map;
> }
>
> @@ -233,9 +226,9 @@ int V4L2CompatManager::munmap(void *addr, size_t length)
>
> int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)
> {
> - V4L2CameraProxy *proxy = getProxy(fd);
> - if (!proxy)
> + std::shared_ptr<V4L2CameraFile> cameraFile = getCameraFile(fd);
> + if (!cameraFile)
> return fops_.ioctl(fd, request, arg);
>
> - return proxy->ioctl(request, arg);
> + return cameraFile->proxy()->ioctl(cameraFile.get(), request, arg);
> }
> diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h
> index 3d4e512..b92f147 100644
> --- a/src/v4l2/v4l2_compat_manager.h
> +++ b/src/v4l2/v4l2_compat_manager.h
> @@ -44,7 +44,7 @@ public:
>
> static V4L2CompatManager *instance();
>
> - V4L2CameraProxy *getProxy(int fd);
> + std::shared_ptr<V4L2CameraFile> getCameraFile(int fd);
While at it, I would rename this function to cameraFile() as we don't
prefix getters with get. It could even become just file().
> const FileOperations &fops() const { return fops_; }
>
> int openat(int dirfd, const char *path, int oflag, mode_t mode);
> @@ -68,7 +68,7 @@ private:
> CameraManager *cm_;
>
> std::vector<std::unique_ptr<V4L2CameraProxy>> proxies_;
> - std::map<int, V4L2CameraProxy *> devices_;
> + std::map<int, std::shared_ptr<V4L2CameraFile>> cameraFiles_;
> std::map<void *, V4L2CameraProxy *> mmaps_;
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list