[libcamera-devel] [PATCH v3 03/22] v4l2: v4l2_compat: Support multiple open
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 24 00:23:32 CEST 2020
Hi Paul,
Thank you for the patch.
On Wed, Jun 24, 2020 at 04:08:17AM +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 ownership by a V4L2CameraFile of a V4L2Camera via the
> V4L2CameraProxy. All vidioc ioctls prior to reqbufs > 0 (except for
> s_fmt) are able to access the camera without ownership. A call to
> reqbufs > 0 (and s_fmt) will take ownership, and the ownership will be
> released at reqbufs = 0. While ownership is assigned, the eventfd that
> should be signaled (and cleared) by V4L2Camera and V4L2CameraProxy is
> set to the V4L2CameraFile that has ownership, and is cleared when the
> ownership is released. In case close() is called without a
> reqbufs = 0 first, the ownership is also released on close().
>
> 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.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v3:
> - split V4L2CompatManager stuff into separate patch
> - split changing the fd argument to camera file argument in all proxy
> methods into the separate patch
> - remove V4L2CameraProxy::efd_ (it's duplicated from
> V4L2CameraFile::efd())
> - rename cf in all the V4L2CameraProxy methods to file
> - rename V4L2CameraProxy::acquiredCf_ to V4L2CameraProxy::owner_
> - remove V4L2CameraProxy::initialized_; just use V4L2CameraProxy::refcount_
> - move bufferCount_ > 0 in reqbufs to different patch
> - move TIMESTAMP_MONOTONIC to different patch
> - don't take the lock for querybuf
> - add V4L2CameraProxy::hasOwnership
> - rename lock() to acquire() and unlock() to release()
> - only lock on s_fmt and reqbufs > 0, in other ioctls only check for
> ownership
> - add explanation about not supporting polling for events from an fd
> different than the one that owns the device for polling for bufs
>
> 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_proxy.cpp | 99 ++++++++++++++++++++++++++++------
> src/v4l2/v4l2_camera_proxy.h | 16 +++++-
> 4 files changed, 106 insertions(+), 19 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_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index c1ee1be..eb59d49 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -35,7 +35,7 @@ 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)), owner_(nullptr)
> {
> querycap(camera);
> }
> @@ -44,18 +44,29 @@ int V4L2CameraProxy::open(V4L2CameraFile *file)
> {
> LOG(V4L2Compat, Debug) << "Servicing open fd = " << file->efd();
>
> + if (refcount_++)
> + return 0;
> +
> + /*
> + * We open the camera here, once, and keep it open until the last
> + * V4L2CameraFile is closed. The proxy is initially not owned by any
> + * file. The first file that calls reqbufs with count > 0 will become
... with count > 0 or s_fmt will ...
> + * the owner, and no other file will be allowed to call buffer-related
> + * ioctls (except querycap, querybuf, try_fmt, g/s_priority,
> + * enum/g/s_input), set the format or start or stop the stream until
In that list only querybuf is a buffer-related ioctl. I'd just write
"(except querybuf), set the format ...".
> + * ownership is released with a call to reqbufs with count = 0.
> + */
> +
> int ret = vcam_->open();
> if (ret < 0) {
> - errno = -ret;
> - return -1;
> + refcount_--;
> + return ret;
> }
>
> vcam_->getStreamConfig(&streamConfig_);
> setFmtFromConfig(streamConfig_);
> sizeimage_ = calculateSizeImage(streamConfig_);
>
> - refcount_++;
> -
> return 0;
> }
>
> @@ -63,6 +74,7 @@ void V4L2CameraProxy::close(V4L2CameraFile *file)
> {
> LOG(V4L2Compat, Debug) << "Servicing close fd = " << file->efd();
>
> + release(file);
>
> if (--refcount_ > 0)
> return;
> @@ -277,12 +289,16 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *file, struct v4l2_format *arg)
> if (!validateBufferType(arg->type))
> return -EINVAL;
>
> + int ret = acquire(file);
> + if (ret < 0)
> + return ret;
> +
> 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;
>
> @@ -334,15 +350,21 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
>
> LOG(V4L2Compat, Debug) << arg->count << " buffers requested ";
>
> + int ret = acquire(file);
> + if (ret < 0)
> + return ret;
> +
> arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
>
> - if (arg->count == 0)
> + if (arg->count == 0) {
> + release(file);
> return freeBuffers();
> + }
>
> Size size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);
> - int ret = vcam_->configure(&streamConfig_, size,
> - v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),
> - arg->count);
> + ret = vcam_->configure(&streamConfig_, size,
> + v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),
> + arg->count);
> if (ret < 0)
Don't you need to call release(file) here too ?
> return -EINVAL;
>
> @@ -409,6 +431,9 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
> LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = "
> << arg->index << " fd = " << file->efd();
>
> + if (!hasOwnership(file))
> + return -EBUSY;
> +
> if (!validateBufferType(arg->type) ||
> !validateMemoryType(arg->memory) ||
> arg->index >= bufferCount_)
> @@ -428,6 +453,9 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
> {
> LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << file->efd();
>
> + if (!hasOwnership(file))
> + return -EBUSY;
> +
> if (!validateBufferType(arg->type) ||
> !validateMemoryType(arg->memory))
> return -EINVAL;
> @@ -462,6 +490,9 @@ int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
> if (!validateBufferType(*arg))
> return -EINVAL;
>
> + if (!hasOwnership(file))
> + return -EBUSY;
> +
> currentBuf_ = 0;
>
> return vcam_->streamOn();
> @@ -474,6 +505,9 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *file, int *arg)
> if (!validateBufferType(*arg))
> return -EINVAL;
>
> + if (!hasOwnership(file) && owner_)
> + return -EBUSY;
> +
> int ret = vcam_->streamOff();
>
> for (struct v4l2_buffer &buf : buffers_)
> @@ -532,10 +566,45 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
> return ret;
> }
>
> -void V4L2CameraProxy::bind(int fd)
> +bool V4L2CameraProxy::hasOwnership(V4L2CameraFile *file)
> +{
> + return owner_ == file;
> +}
> +
> +/**
> + * \brief Acquire exclusive ownership of the V4L2Camera
> + *
> + * \return Zero on success or if already acquired, and negative error on
> + * failure.
> + *
> + * This is sufficient for poll()ing for buffers. Events, however, are signaled
> + * on the file level, so all fds must be signaled. poll()ing from a different
> + * fd than the one that locks the device is a corner case, and is currently not
> + * supported.
> + */
> +int V4L2CameraProxy::acquire(V4L2CameraFile *file)
> {
> - efd_ = fd;
> - vcam_->bind(fd);
> + if (owner_ == file)
> + return 0;
> +
> + if (owner_)
> + return -EBUSY;
> +
> + vcam_->bind(file->efd());
> +
> + owner_ = file;
> +
> + return 0;
> +}
> +
> +void V4L2CameraProxy::release(V4L2CameraFile *file)
> +{
> + if (owner_ != file)
> + return;
> +
> + vcam_->unbind();
> +
> + owner_ = nullptr;
> }
>
> struct PixelFormatPlaneInfo {
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index b2197ef..2be46e6 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -33,7 +33,6 @@ public:
> void *mmap(void *addr, size_t length, int prot, int flags, off64_t offset);
> int munmap(void *addr, size_t length);
>
> - void bind(int fd);
> int ioctl(V4L2CameraFile *file, unsigned long request, void *arg);
>
> private:
> @@ -58,6 +57,10 @@ private:
> int vidioc_streamon(V4L2CameraFile *file, int *arg);
> int vidioc_streamoff(V4L2CameraFile *file, int *arg);
>
> + bool hasOwnership(V4L2CameraFile *file);
> + int acquire(V4L2CameraFile *file);
> + void release(V4L2CameraFile *file);
> +
> static unsigned int bplMultiplier(uint32_t format);
> static unsigned int imageSize(uint32_t format, unsigned int width,
> unsigned int height);
> @@ -80,7 +83,16 @@ private:
>
> std::unique_ptr<V4L2Camera> vcam_;
>
> - int efd_;
> + /*
> + * This is the exclusive owner of this V4L2CameraProxy instance.
> + * When there is no owner, anybody can call any ioctl before reqbufs.
> + * The first file to call reqbufs with count > 0 will become the owner,
... count > 0 or s_fmt will ...
> + * and when the owner calls reqbufs with count = 0 it will release
> + * ownership. Any ioctl that is call by a non-owner while there exists
> + * and owner will return -EBUSY (except querycap, querybuf, try_fmt,
> + * g/s_priority, enum/g/s_input
And to match the previous comment, "Any buffer-related ioctl (except
querybuf) or s_fmt call by a non-owner ... will return -EBUSY." ?
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> + V4L2CameraFile *owner_;
> };
>
> #endif /* __V4L2_CAMERA_PROXY_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list