From patchwork Wed Jan 22 20:57:11 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2717 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 28CF160871 for ; Wed, 22 Jan 2020 21:57:43 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id CF2DE2F9 for ; Wed, 22 Jan 2020 21:57:42 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726663; bh=RYzAJ/Do9yiXZrKTgiF64fSQHcD03ntuRoEUIMr0iWU=; h=From:To:Subject:Date:In-Reply-To:References:From; b=XO4dYHlMK0EBXDC+SKfoZ4m4uYMa2mbEz8NrtiiXepzj56K+UQ63pyqoGOtuU+slK UPxCbZuRYuvZEOW8agQEleTJD98Ks4SFnXcMizR0CpTDv5knkwtEVvUM1ojgx6Er3C 4tI5fErAjPxLmGuyYjarVR47adN+GDdsTrXWPbHg= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:11 +0200 Message-Id: <20200122205723.8865-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 01/13] libcamera: Fix documentation of buffer allocation/export functions X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:43 -0000 The V4L2VideoDevice::exportBuffers(), PipelineHandler::exportFrameBuffers() and FrameBufferAllocator::allocate() functions all return the number of allocated buffers on success, but are documented as returning 0 in that case. Fix their documentation. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/framebuffer_allocator.cpp | 11 ++++------- src/libcamera/pipeline_handler.cpp | 3 ++- src/libcamera/v4l2_videodevice.cpp | 3 ++- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index a7588c7fe4c2..c772b5165bbf 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -108,7 +108,8 @@ FrameBufferAllocator::~FrameBufferAllocator() * Upon successful allocation, the allocated buffers can be retrieved with the * buffers() method. * - * \return 0 on success or a negative error code otherwise + * \return The number of allocated buffers on success or a negative error code + * otherwise * \retval -EACCES The camera is not in a state where buffers can be allocated * \retval -EINVAL The \a stream does not belong to the camera or the stream is * not part of the active camera configuration @@ -140,12 +141,8 @@ int FrameBufferAllocator::allocate(Stream *stream) return -EBUSY; } - int ret = camera_->pipe_->exportFrameBuffers(camera_.get(), stream, - &buffers_[stream]); - if (ret) - return ret; - - return 0; + return camera_->pipe_->exportFrameBuffers(camera_.get(), stream, + &buffers_[stream]); } /** diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 669097f609ab..01b9ede34b53 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -316,7 +316,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * * The only intended caller is the FrameBufferAllocator helper. * - * \return 0 on success or a negative error code otherwise + * \return The number of allocated buffers on success or a negative error code + * otherwise */ /** diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 18220b81af21..82267730289d 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -973,7 +973,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count) * \brief Allocate buffers from the video device * \param[in] count Number of buffers to allocate * \param[out] buffers Vector to store allocated buffers - * \return 0 on success or a negative error code otherwise + * \return The number of allocated buffers on success or a negative error code + * otherwise */ int V4L2VideoDevice::exportBuffers(unsigned int count, std::vector> *buffers) From patchwork Wed Jan 22 20:57:12 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2718 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C2BF60871 for ; Wed, 22 Jan 2020 21:57:43 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 29A032E5 for ; Wed, 22 Jan 2020 21:57:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726663; bh=7mNSt+sbbHv/Og5UrBBudy/mTdyR8YfL+pVx4+/3P6Y=; h=From:To:Subject:Date:In-Reply-To:References:From; b=efMRXlbb4Y45pWhtuJdqE54zvk6Z+X4lCWZpw3O7PKlQ8TbtKxk0tUg7aj2bsbcMi NEI2h7/BkvJIezjJ1sjVOaFIfNZUyeAX7Fp2Ne6Va4MrnPHSukTcAYqm/2yLMLh9J4 +w4/QzJbYmiyHtJ06iT92+BcRnL/o6wPf26g61M8= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:12 +0200 Message-Id: <20200122205723.8865-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager: Move private data members to private implementation X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:43 -0000 Use the d-pointer idiom ([1], [2]) to hide the private data members from the CameraManager class interface. This will ease maintaining ABI compatibility, and prepares for the implementation of the CameraManager class threading model. [1] https://wiki.qt.io/D-Pointer [2] https://en.cppreference.com/w/cpp/language/pimpl libcamera: camera_manager: Run the camera manager in a thread:q Signed-off-by: Laurent Pinchart Reviewed-by: Kieran Bingham Reviewed-by: Niklas Söderlund --- Documentation/Doxyfile.in | 1 + include/libcamera/camera_manager.h | 13 +- src/libcamera/camera_manager.cpp | 218 ++++++++++++++++++----------- 3 files changed, 143 insertions(+), 89 deletions(-) diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in index 8e6fbdbb92b6..1f746393568a 100644 --- a/Documentation/Doxyfile.in +++ b/Documentation/Doxyfile.in @@ -877,6 +877,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \ libcamera::BoundMethodPackBase \ libcamera::BoundMethodStatic \ libcamera::SignalBase \ + libcamera::*::Private \ std::* # The EXAMPLE_PATH tag can be used to specify one or more files or directories diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 094197668698..068afd58762f 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -7,7 +7,6 @@ #ifndef __LIBCAMERA_CAMERA_MANAGER_H__ #define __LIBCAMERA_CAMERA_MANAGER_H__ -#include #include #include #include @@ -18,9 +17,7 @@ namespace libcamera { class Camera; -class DeviceEnumerator; class EventDispatcher; -class PipelineHandler; class CameraManager : public Object { @@ -33,7 +30,7 @@ public: int start(); void stop(); - const std::vector> &cameras() const { return cameras_; } + const std::vector> &cameras() const; std::shared_ptr get(const std::string &name); std::shared_ptr get(dev_t devnum); @@ -46,13 +43,11 @@ public: EventDispatcher *eventDispatcher(); private: - std::unique_ptr enumerator_; - std::vector> pipes_; - std::vector> cameras_; - std::map> camerasByDevnum_; - static const std::string version_; static CameraManager *self_; + + class Private; + std::unique_ptr p_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index a71caf8536bb..e0a07ec557d3 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -7,6 +7,8 @@ #include +#include + #include #include @@ -26,6 +28,124 @@ namespace libcamera { LOG_DEFINE_CATEGORY(Camera) +class CameraManager::Private +{ +public: + Private(CameraManager *cm); + + int start(); + void stop(); + + void addCamera(std::shared_ptr &camera, dev_t devnum); + void removeCamera(Camera *camera); + + std::vector> cameras_; + std::map> camerasByDevnum_; + +private: + CameraManager *cm_; + + std::vector> pipes_; + std::unique_ptr enumerator_; +}; + +CameraManager::Private::Private(CameraManager *cm) + : cm_(cm) +{ +} + +int CameraManager::Private::start() +{ + enumerator_ = DeviceEnumerator::create(); + if (!enumerator_ || enumerator_->enumerate()) + return -ENODEV; + + /* + * TODO: Try to read handlers and order from configuration + * file and only fallback on all handlers if there is no + * configuration file. + */ + std::vector &factories = PipelineHandlerFactory::factories(); + + for (PipelineHandlerFactory *factory : factories) { + /* + * Try each pipeline handler until it exhaust + * all pipelines it can provide. + */ + while (1) { + std::shared_ptr pipe = factory->create(cm_); + if (!pipe->match(enumerator_.get())) + break; + + LOG(Camera, Debug) + << "Pipeline handler \"" << factory->name() + << "\" matched"; + pipes_.push_back(std::move(pipe)); + } + } + + /* TODO: register hot-plug callback here */ + + return 0; +} + +void CameraManager::Private::stop() +{ + /* TODO: unregister hot-plug callback here */ + + /* + * Release all references to cameras and pipeline handlers to ensure + * they all get destroyed before the device enumerator deletes the + * media devices. + */ + pipes_.clear(); + cameras_.clear(); + + enumerator_.reset(nullptr); +} + +void CameraManager::Private::addCamera(std::shared_ptr &camera, + dev_t devnum) +{ + for (std::shared_ptr c : cameras_) { + if (c->name() == camera->name()) { + LOG(Camera, Warning) + << "Registering camera with duplicate name '" + << camera->name() << "'"; + break; + } + } + + cameras_.push_back(std::move(camera)); + + if (devnum) { + unsigned int index = cameras_.size() - 1; + camerasByDevnum_[devnum] = cameras_[index]; + } +} + +void CameraManager::Private::removeCamera(Camera *camera) +{ + auto iter = std::find_if(cameras_.begin(), cameras_.end(), + [camera](std::shared_ptr &c) { + return c.get() == camera; + }); + if (iter == cameras_.end()) + return; + + LOG(Camera, Debug) + << "Unregistering camera '" << camera->name() << "'"; + + auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(), + [camera](const std::pair> &p) { + return p.second.lock().get() == camera; + }); + if (iter_d != camerasByDevnum_.end()) + camerasByDevnum_.erase(iter_d); + + cameras_.erase(iter); +} + /** * \class CameraManager * \brief Provide access and manage all cameras in the system @@ -62,7 +182,7 @@ LOG_DEFINE_CATEGORY(Camera) CameraManager *CameraManager::self_ = nullptr; CameraManager::CameraManager() - : enumerator_(nullptr) + : p_(new CameraManager::Private(this)) { if (self_) LOG(Camera, Fatal) @@ -73,6 +193,8 @@ CameraManager::CameraManager() CameraManager::~CameraManager() { + stop(); + self_ = nullptr; } @@ -88,42 +210,14 @@ CameraManager::~CameraManager() */ int CameraManager::start() { - if (enumerator_) - return -EBUSY; - LOG(Camera, Info) << "libcamera " << version_; - enumerator_ = DeviceEnumerator::create(); - if (!enumerator_ || enumerator_->enumerate()) - return -ENODEV; - - /* - * TODO: Try to read handlers and order from configuration - * file and only fallback on all handlers if there is no - * configuration file. - */ - std::vector &factories = PipelineHandlerFactory::factories(); + int ret = p_->start(); + if (ret) + LOG(Camera, Error) << "Failed to start camera manager: " + << strerror(-ret); - for (PipelineHandlerFactory *factory : factories) { - /* - * Try each pipeline handler until it exhaust - * all pipelines it can provide. - */ - while (1) { - std::shared_ptr pipe = factory->create(this); - if (!pipe->match(enumerator_.get())) - break; - - LOG(Camera, Debug) - << "Pipeline handler \"" << factory->name() - << "\" matched"; - pipes_.push_back(std::move(pipe)); - } - } - - /* TODO: register hot-plug callback here */ - - return 0; + return ret; } /** @@ -138,17 +232,7 @@ int CameraManager::start() */ void CameraManager::stop() { - /* TODO: unregister hot-plug callback here */ - - /* - * Release all references to cameras and pipeline handlers to ensure - * they all get destroyed before the device enumerator deletes the - * media devices. - */ - pipes_.clear(); - cameras_.clear(); - - enumerator_.reset(nullptr); + p_->stop(); } /** @@ -160,6 +244,10 @@ void CameraManager::stop() * * \return List of all available cameras */ +const std::vector> &CameraManager::cameras() const +{ + return p_->cameras_; +} /** * \brief Get a camera based on name @@ -172,7 +260,7 @@ void CameraManager::stop() */ std::shared_ptr CameraManager::get(const std::string &name) { - for (std::shared_ptr camera : cameras_) { + for (std::shared_ptr camera : p_->cameras_) { if (camera->name() == name) return camera; } @@ -196,8 +284,8 @@ std::shared_ptr CameraManager::get(const std::string &name) */ std::shared_ptr CameraManager::get(dev_t devnum) { - auto iter = camerasByDevnum_.find(devnum); - if (iter == camerasByDevnum_.end()) + auto iter = p_->camerasByDevnum_.find(devnum); + if (iter == p_->camerasByDevnum_.end()) return nullptr; return iter->second.lock(); @@ -217,21 +305,8 @@ std::shared_ptr CameraManager::get(dev_t devnum) */ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum) { - for (std::shared_ptr c : cameras_) { - if (c->name() == camera->name()) { - LOG(Camera, Warning) - << "Registering camera with duplicate name '" - << camera->name() << "'"; - break; - } - } - cameras_.push_back(std::move(camera)); - - if (devnum) { - unsigned int index = cameras_.size() - 1; - camerasByDevnum_[devnum] = cameras_[index]; - } + p_->addCamera(camera, devnum); } /** @@ -244,24 +319,7 @@ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum) */ void CameraManager::removeCamera(Camera *camera) { - auto iter = std::find_if(cameras_.begin(), cameras_.end(), - [camera](std::shared_ptr &c) { - return c.get() == camera; - }); - if (iter == cameras_.end()) - return; - - LOG(Camera, Debug) - << "Unregistering camera '" << camera->name() << "'"; - - auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(), - [camera](const std::pair> &p) { - return p.second.lock().get() == camera; - }); - if (iter_d != camerasByDevnum_.end()) - camerasByDevnum_.erase(iter_d); - - cameras_.erase(iter); + p_->removeCamera(camera); } /** From patchwork Wed Jan 22 20:57:13 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2719 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id CD37C60871 for ; Wed, 22 Jan 2020 21:57:43 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 77BC72F9 for ; Wed, 22 Jan 2020 21:57:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726663; bh=yyjEI7pQLc+6xITewJUfAh0GuwQzH3yxmc44qDBtIc8=; h=From:To:Subject:Date:In-Reply-To:References:From; b=vGVXP2VkSYmyUKGfVLOeU1+hhFh9jS7u8S381/CSQs72HYca8wDFdYhFAr5BbipEY 3x4+KYtUEoI3MafGjLY9r2NXobTtmpOF7mj+w84n1WPYP1TMyGpKyfZiJ26chKdjMj Ur/pKZx68QOLj6z5zBvGl6ChvS6pofrcMX0QdSZM= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:13 +0200 Message-Id: <20200122205723.8865-4-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 03/13] libcamera: camera_manager: Return a copy of the vector from cameras() X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:44 -0000 Making CameraManager::cameras() thread-safe requires returning a copy of the cameras vector instead of a reference. This is also required for hot-plugging support and is thus desirable. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- include/libcamera/camera_manager.h | 2 +- src/libcamera/camera_manager.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 068afd58762f..079f848aec79 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -30,7 +30,7 @@ public: int start(); void stop(); - const std::vector> &cameras() const; + std::vector> cameras() const; std::shared_ptr get(const std::string &name); std::shared_ptr get(dev_t devnum); diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index e0a07ec557d3..5fc1bba974c6 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -244,7 +244,7 @@ void CameraManager::stop() * * \return List of all available cameras */ -const std::vector> &CameraManager::cameras() const +std::vector> CameraManager::cameras() const { return p_->cameras_; } From patchwork Wed Jan 22 20:57:14 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2720 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 274DF60883 for ; Wed, 22 Jan 2020 21:57:44 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C4F3C2E5 for ; Wed, 22 Jan 2020 21:57:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726663; bh=LglUdNDFcVxyqCcNxpUpAsRVPyNTV5VJW+5TRef3+Xw=; h=From:To:Subject:Date:In-Reply-To:References:From; b=siZGhcwJepcPfPOZ++AGH6htwCtrzlPEjY8a3qcmYb8bgO2X3MwWJtZh1N/lMiErm mS1q2yjl8PBklpkV1oukb85DDMIWDv1WUOmwjmQnsLK7GSR1x989Rn78L2yzDnFD5X qGyD6ajU+62DuahCdQYWL2i6kfej/+5d7lkkUpYw= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:14 +0200 Message-Id: <20200122205723.8865-5-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 04/13] libcamera: camera: Move private data members to private implementation X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:45 -0000 Use the d-pointer idiom ([1], [2]) to hide the private data members from the Camera class interface. This will ease maintaining ABI compatibility, and prepares for the implementation of the Camera class threading model. The FrameBufferAllocator class accesses the Camera private data members directly. In order to hide them, this pattern is replaced with new private member functions in the Camera class, and the FrameBufferAllocator is updated accordingly. [1] https://wiki.qt.io/D-Pointer [2] https://en.cppreference.com/w/cpp/language/pimpl Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- include/libcamera/camera.h | 28 +-- src/libcamera/camera.cpp | 224 +++++++++++++++--------- src/libcamera/framebuffer_allocator.cpp | 43 ++--- 3 files changed, 161 insertions(+), 134 deletions(-) diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 6597ade83288..c37319eda2dc 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -98,34 +98,22 @@ public: int stop(); private: - enum State { - CameraAvailable, - CameraAcquired, - CameraConfigured, - CameraRunning, - }; - - Camera(PipelineHandler *pipe, const std::string &name); + Camera(PipelineHandler *pipe, const std::string &name, + const std::set &streams); ~Camera(); - bool stateBetween(State low, State high) const; - bool stateIs(State state) const; + class Private; + std::unique_ptr p_; friend class PipelineHandler; void disconnect(); - void requestComplete(Request *request); - std::shared_ptr pipe_; - std::string name_; - std::set streams_; - std::set activeStreams_; - - bool disconnected_; - State state_; - - /* Needed to update allocator_ and to read state_ and activeStreams_. */ friend class FrameBufferAllocator; + int exportFrameBuffers(Stream *stream, + std::vector> *buffers); + int freeFrameBuffers(Stream *stream); + /* \todo Remove allocator_ from the exposed API */ FrameBufferAllocator *allocator_; }; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 79a5f994f9bb..d567148c36b1 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -254,6 +254,75 @@ std::size_t CameraConfiguration::size() const * \brief The vector of stream configurations */ +class Camera::Private +{ +public: + enum State { + CameraAvailable, + CameraAcquired, + CameraConfigured, + CameraRunning, + }; + + Private(PipelineHandler *pipe, const std::string &name, + const std::set &streams); + + bool stateBetween(State low, State high) const; + bool stateIs(State state) const; + + std::shared_ptr pipe_; + std::string name_; + std::set streams_; + std::set activeStreams_; + + bool disconnected_; + State state_; +}; + +Camera::Private::Private(PipelineHandler *pipe, const std::string &name, + const std::set &streams) + : pipe_(pipe->shared_from_this()), name_(name), streams_(streams), + disconnected_(false), state_(CameraAvailable) +{ +} + +static const char *const camera_state_names[] = { + "Available", + "Acquired", + "Configured", + "Running", +}; + +bool Camera::Private::stateBetween(State low, State high) const +{ + if (state_ >= low && state_ <= high) + return true; + + ASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) && + static_cast(high) < ARRAY_SIZE(camera_state_names)); + + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + << " state trying operation requiring state between " + << camera_state_names[low] << " and " + << camera_state_names[high]; + + return false; +} + +bool Camera::Private::stateIs(State state) const +{ + if (state_ == state) + return true; + + ASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names)); + + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + << " state trying operation requiring state " + << camera_state_names[state]; + + return false; +} + /** * \class Camera * \brief Camera device @@ -354,8 +423,7 @@ std::shared_ptr Camera::create(PipelineHandler *pipe, } }; - Camera *camera = new Camera(pipe, name); - camera->streams_ = streams; + Camera *camera = new Camera(pipe, name, streams); return std::shared_ptr(camera, Deleter()); } @@ -366,7 +434,7 @@ std::shared_ptr Camera::create(PipelineHandler *pipe, */ const std::string &Camera::name() const { - return name_; + return p_->name_; } /** @@ -392,55 +460,18 @@ const std::string &Camera::name() const * application API calls by returning errors immediately. */ -Camera::Camera(PipelineHandler *pipe, const std::string &name) - : pipe_(pipe->shared_from_this()), name_(name), disconnected_(false), - state_(CameraAvailable), allocator_(nullptr) +Camera::Camera(PipelineHandler *pipe, const std::string &name, + const std::set &streams) + : p_(new Private(pipe, name, streams)), allocator_(nullptr) { } Camera::~Camera() { - if (!stateIs(CameraAvailable)) + if (!p_->stateIs(Private::CameraAvailable)) LOG(Camera, Error) << "Removing camera while still in use"; } -static const char *const camera_state_names[] = { - "Available", - "Acquired", - "Configured", - "Running", -}; - -bool Camera::stateBetween(State low, State high) const -{ - if (state_ >= low && state_ <= high) - return true; - - ASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) && - static_cast(high) < ARRAY_SIZE(camera_state_names)); - - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] - << " state trying operation requiring state between " - << camera_state_names[low] << " and " - << camera_state_names[high]; - - return false; -} - -bool Camera::stateIs(State state) const -{ - if (state_ == state) - return true; - - ASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names)); - - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] - << " state trying operation requiring state " - << camera_state_names[state]; - - return false; -} - /** * \brief Notify camera disconnection * @@ -455,20 +486,45 @@ bool Camera::stateIs(State state) const */ void Camera::disconnect() { - LOG(Camera, Debug) << "Disconnecting camera " << name_; + LOG(Camera, Debug) << "Disconnecting camera " << name(); /* * If the camera was running when the hardware was removed force the * state to Configured state to allow applications to free resources * and call release() before deleting the camera. */ - if (state_ == CameraRunning) - state_ = CameraConfigured; + if (p_->state_ == Private::CameraRunning) + p_->state_ = Private::CameraConfigured; - disconnected_ = true; + p_->disconnected_ = true; disconnected.emit(this); } +int Camera::exportFrameBuffers(Stream *stream, + std::vector> *buffers) +{ + if (!p_->stateIs(Private::CameraConfigured)) + return -EACCES; + + if (streams().find(stream) == streams().end()) + return -EINVAL; + + if (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) + return -EINVAL; + + return p_->pipe_->exportFrameBuffers(this, stream, buffers); +} + +int Camera::freeFrameBuffers(Stream *stream) +{ + if (!p_->stateIs(Private::CameraConfigured)) + return -EACCES; + + p_->pipe_->freeFrameBuffers(this, stream); + + return 0; +} + /** * \brief Acquire the camera device for exclusive access * @@ -494,19 +550,19 @@ void Camera::disconnect() */ int Camera::acquire() { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraAvailable)) + if (!p_->stateIs(Private::CameraAvailable)) return -EBUSY; - if (!pipe_->lock()) { + if (!p_->pipe_->lock()) { LOG(Camera, Info) << "Pipeline handler in use by another process"; return -EBUSY; } - state_ = CameraAcquired; + p_->state_ = Private::CameraAcquired; return 0; } @@ -524,7 +580,8 @@ int Camera::acquire() */ int Camera::release() { - if (!stateBetween(CameraAvailable, CameraConfigured)) + if (!p_->stateBetween(Private::CameraAvailable, + Private::CameraConfigured)) return -EBUSY; if (allocator_) { @@ -537,9 +594,9 @@ int Camera::release() return -EBUSY; } - pipe_->unlock(); + p_->pipe_->unlock(); - state_ = CameraAvailable; + p_->state_ = Private::CameraAvailable; return 0; } @@ -553,7 +610,7 @@ int Camera::release() */ const ControlInfoMap &Camera::controls() { - return pipe_->controls(this); + return p_->pipe_->controls(this); } /** @@ -567,7 +624,7 @@ const ControlInfoMap &Camera::controls() */ const std::set &Camera::streams() const { - return streams_; + return p_->streams_; } /** @@ -586,10 +643,10 @@ const std::set &Camera::streams() const */ std::unique_ptr Camera::generateConfiguration(const StreamRoles &roles) { - if (disconnected_ || roles.size() > streams_.size()) + if (p_->disconnected_ || roles.size() > streams().size()) return nullptr; - CameraConfiguration *config = pipe_->generateConfiguration(this, roles); + CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles); if (!config) { LOG(Camera, Debug) << "Pipeline handler failed to generate configuration"; @@ -639,10 +696,11 @@ int Camera::configure(CameraConfiguration *config) { int ret; - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateBetween(CameraAcquired, CameraConfigured)) + if (!p_->stateBetween(Private::CameraAcquired, + Private::CameraConfigured)) return -EACCES; if (allocator_ && allocator_->allocated()) { @@ -667,11 +725,11 @@ int Camera::configure(CameraConfiguration *config) LOG(Camera, Info) << msg.str(); - ret = pipe_->configure(this, config); + ret = p_->pipe_->configure(this, config); if (ret) return ret; - activeStreams_.clear(); + p_->activeStreams_.clear(); for (const StreamConfiguration &cfg : *config) { Stream *stream = cfg.stream(); if (!stream) @@ -679,10 +737,10 @@ int Camera::configure(CameraConfiguration *config) << "Pipeline handler failed to update stream configuration"; stream->configuration_ = cfg; - activeStreams_.insert(stream); + p_->activeStreams_.insert(stream); } - state_ = CameraConfigured; + p_->state_ = Private::CameraConfigured; return 0; } @@ -709,7 +767,9 @@ int Camera::configure(CameraConfiguration *config) */ Request *Camera::createRequest(uint64_t cookie) { - if (disconnected_ || !stateBetween(CameraConfigured, CameraRunning)) + if (p_->disconnected_ || + !p_->stateBetween(Private::CameraConfigured, + Private::CameraRunning)) return nullptr; return new Request(this, cookie); @@ -739,10 +799,10 @@ Request *Camera::createRequest(uint64_t cookie) */ int Camera::queueRequest(Request *request) { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraRunning)) + if (!p_->stateIs(Private::CameraRunning)) return -EACCES; if (request->buffers().empty()) { @@ -753,13 +813,13 @@ int Camera::queueRequest(Request *request) for (auto const &it : request->buffers()) { Stream *stream = it.first; - if (activeStreams_.find(stream) == activeStreams_.end()) { + if (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) { LOG(Camera, Error) << "Invalid request"; return -EINVAL; } } - return pipe_->queueRequest(this, request); + return p_->pipe_->queueRequest(this, request); } /** @@ -777,26 +837,26 @@ int Camera::queueRequest(Request *request) */ int Camera::start() { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraConfigured)) + if (!p_->stateIs(Private::CameraConfigured)) return -EACCES; LOG(Camera, Debug) << "Starting capture"; - for (Stream *stream : activeStreams_) { + for (Stream *stream : p_->activeStreams_) { if (allocator_ && !allocator_->buffers(stream).empty()) continue; - pipe_->importFrameBuffers(this, stream); + p_->pipe_->importFrameBuffers(this, stream); } - int ret = pipe_->start(this); + int ret = p_->pipe_->start(this); if (ret) return ret; - state_ = CameraRunning; + p_->state_ = Private::CameraRunning; return 0; } @@ -815,23 +875,23 @@ int Camera::start() */ int Camera::stop() { - if (disconnected_) + if (p_->disconnected_) return -ENODEV; - if (!stateIs(CameraRunning)) + if (!p_->stateIs(Private::CameraRunning)) return -EACCES; LOG(Camera, Debug) << "Stopping capture"; - state_ = CameraConfigured; + p_->state_ = Private::CameraConfigured; - pipe_->stop(this); + p_->pipe_->stop(this); - for (Stream *stream : activeStreams_) { + for (Stream *stream : p_->activeStreams_) { if (allocator_ && !allocator_->buffers(stream).empty()) continue; - pipe_->freeFrameBuffers(this, stream); + p_->pipe_->freeFrameBuffers(this, stream); } return 0; diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index c772b5165bbf..4ced10cda2e3 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -89,7 +89,7 @@ FrameBufferAllocator::~FrameBufferAllocator() { for (auto &value : buffers_) { Stream *stream = value.first; - camera_->pipe_->freeFrameBuffers(camera_.get(), stream); + camera_->freeFrameBuffers(stream); } buffers_.clear(); @@ -117,32 +117,17 @@ FrameBufferAllocator::~FrameBufferAllocator() */ int FrameBufferAllocator::allocate(Stream *stream) { - if (camera_->state_ != Camera::CameraConfigured) { - LOG(Allocator, Error) - << "Camera must be in the configured state to allocate buffers"; - return -EACCES; - } - - if (camera_->streams().find(stream) == camera_->streams().end()) { - LOG(Allocator, Error) - << "Stream does not belong to " << camera_->name(); - return -EINVAL; - } - - if (camera_->activeStreams_.find(stream) == camera_->activeStreams_.end()) { - LOG(Allocator, Error) - << "Stream is not part of " << camera_->name() - << " active configuration"; - return -EINVAL; - } - if (buffers_.count(stream)) { LOG(Allocator, Error) << "Buffers already allocated for stream"; return -EBUSY; } - return camera_->pipe_->exportFrameBuffers(camera_.get(), stream, - &buffers_[stream]); + int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]); + if (ret == -EINVAL) + LOG(Allocator, Error) + << "Stream is not part of " << camera_->name() + << " active configuration"; + return ret; } /** @@ -159,22 +144,16 @@ int FrameBufferAllocator::allocate(Stream *stream) */ int FrameBufferAllocator::free(Stream *stream) { - if (camera_->state_ != Camera::CameraConfigured) { - LOG(Allocator, Error) - << "Camera must be in the configured state to free buffers"; - return -EACCES; - } - auto iter = buffers_.find(stream); if (iter == buffers_.end()) return -EINVAL; - std::vector> &buffers = iter->second; + int ret = camera_->freeFrameBuffers(stream); + if (ret < 0) + return ret; + std::vector> &buffers = iter->second; buffers.clear(); - - camera_->pipe_->freeFrameBuffers(camera_.get(), stream); - buffers_.erase(iter); return 0; From patchwork Wed Jan 22 20:57:15 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2721 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F0E260877 for ; Wed, 22 Jan 2020 21:57:44 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 205BA2F9 for ; Wed, 22 Jan 2020 21:57:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726664; bh=+UVWjheWtVGjdlQ6DFm06uYCb9pKg14xuBRDDbB/PzA=; h=From:To:Subject:Date:In-Reply-To:References:From; b=ZaFGfo8iGWesHwOXSoBx1Sx/PDh9fWfoiSfU5zACKQH99lxKGNeNaVwN/56nZoJcJ vRggHQCrt7AoBnsDAYXpXUFJP90z/nOKbdBVHdQqUD6kfhGMypKDmvQkQ82X0vDfUF K+vaCV3xZgV4r2LGtFm82b+j1iyg+d8jTdFCmp/w= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:15 +0200 Message-Id: <20200122205723.8865-6-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 05/13] libcamera: camera: Centralize state checks in Private class X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:46 -0000 Move all accesses to the state_ and disconnected_ members to functions of the Private class. This will make it easier to implement synchronization, and simplifies the Camera class implementation. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/camera.cpp | 158 ++++++++++++++++------------- src/libcamera/pipeline_handler.cpp | 5 +- 2 files changed, 90 insertions(+), 73 deletions(-) diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index d567148c36b1..802e7fd0d12f 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -266,15 +266,21 @@ public: Private(PipelineHandler *pipe, const std::string &name, const std::set &streams); + ~Private(); - bool stateBetween(State low, State high) const; - bool stateIs(State state) const; + int isAccessAllowed(State state, bool allowDisconnected = false) const; + int isAccessAllowed(State low, State high, + bool allowDisconnected = false) const; + + void disconnect(); + void setState(State state); std::shared_ptr pipe_; std::string name_; std::set streams_; std::set activeStreams_; +private: bool disconnected_; State state_; }; @@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name, { } +Camera::Private::~Private() +{ + if (state_ != Private::CameraAvailable) + LOG(Camera, Error) << "Removing camera while still in use"; +} + static const char *const camera_state_names[] = { "Available", "Acquired", @@ -293,10 +305,31 @@ static const char *const camera_state_names[] = { "Running", }; -bool Camera::Private::stateBetween(State low, State high) const +int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const +{ + if (!allowDisconnected && disconnected_) + return -ENODEV; + + if (state_ == state) + return 0; + + ASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names)); + + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + << " state trying operation requiring state " + << camera_state_names[state]; + + return -EACCES; +} + +int Camera::Private::isAccessAllowed(State low, State high, + bool allowDisconnected) const { + if (!allowDisconnected && disconnected_) + return -ENODEV; + if (state_ >= low && state_ <= high) - return true; + return 0; ASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) && static_cast(high) < ARRAY_SIZE(camera_state_names)); @@ -306,21 +339,20 @@ bool Camera::Private::stateBetween(State low, State high) const << camera_state_names[low] << " and " << camera_state_names[high]; - return false; + return -EACCES; } -bool Camera::Private::stateIs(State state) const +void Camera::Private::disconnect() { - if (state_ == state) - return true; - - ASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names)); + if (state_ == Private::CameraRunning) + state_ = Private::CameraConfigured; - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] - << " state trying operation requiring state " - << camera_state_names[state]; + disconnected_ = true; +} - return false; +void Camera::Private::setState(State state) +{ + state_ = state; } /** @@ -468,8 +500,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name, Camera::~Camera() { - if (!p_->stateIs(Private::CameraAvailable)) - LOG(Camera, Error) << "Removing camera while still in use"; } /** @@ -488,23 +518,16 @@ void Camera::disconnect() { LOG(Camera, Debug) << "Disconnecting camera " << name(); - /* - * If the camera was running when the hardware was removed force the - * state to Configured state to allow applications to free resources - * and call release() before deleting the camera. - */ - if (p_->state_ == Private::CameraRunning) - p_->state_ = Private::CameraConfigured; - - p_->disconnected_ = true; + p_->disconnect(); disconnected.emit(this); } int Camera::exportFrameBuffers(Stream *stream, std::vector> *buffers) { - if (!p_->stateIs(Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraConfigured); + if (ret < 0) + return ret; if (streams().find(stream) == streams().end()) return -EINVAL; @@ -517,8 +540,9 @@ int Camera::exportFrameBuffers(Stream *stream, int Camera::freeFrameBuffers(Stream *stream) { - if (!p_->stateIs(Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraConfigured, true); + if (ret < 0) + return ret; p_->pipe_->freeFrameBuffers(this, stream); @@ -550,11 +574,9 @@ int Camera::freeFrameBuffers(Stream *stream) */ int Camera::acquire() { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraAvailable)) - return -EBUSY; + int ret = p_->isAccessAllowed(Private::CameraAvailable); + if (ret < 0) + return ret == -EACCES ? -EBUSY : ret; if (!p_->pipe_->lock()) { LOG(Camera, Info) @@ -562,7 +584,7 @@ int Camera::acquire() return -EBUSY; } - p_->state_ = Private::CameraAcquired; + p_->setState(Private::CameraAcquired); return 0; } @@ -580,9 +602,10 @@ int Camera::acquire() */ int Camera::release() { - if (!p_->stateBetween(Private::CameraAvailable, - Private::CameraConfigured)) - return -EBUSY; + int ret = p_->isAccessAllowed(Private::CameraAvailable, + Private::CameraConfigured, true); + if (ret < 0) + return ret == -EACCES ? -EBUSY : ret; if (allocator_) { /* @@ -596,7 +619,7 @@ int Camera::release() p_->pipe_->unlock(); - p_->state_ = Private::CameraAvailable; + p_->setState(Private::CameraAvailable); return 0; } @@ -643,7 +666,12 @@ const std::set &Camera::streams() const */ std::unique_ptr Camera::generateConfiguration(const StreamRoles &roles) { - if (p_->disconnected_ || roles.size() > streams().size()) + int ret = p_->isAccessAllowed(Private::CameraAvailable, + Private::CameraRunning); + if (ret < 0) + return nullptr; + + if (roles.size() > streams().size()) return nullptr; CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles); @@ -694,14 +722,10 @@ std::unique_ptr Camera::generateConfiguration(const StreamR */ int Camera::configure(CameraConfiguration *config) { - int ret; - - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateBetween(Private::CameraAcquired, - Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraAcquired, + Private::CameraConfigured); + if (ret < 0) + return ret; if (allocator_ && allocator_->allocated()) { LOG(Camera, Error) @@ -740,7 +764,7 @@ int Camera::configure(CameraConfiguration *config) p_->activeStreams_.insert(stream); } - p_->state_ = Private::CameraConfigured; + p_->setState(Private::CameraConfigured); return 0; } @@ -767,9 +791,9 @@ int Camera::configure(CameraConfiguration *config) */ Request *Camera::createRequest(uint64_t cookie) { - if (p_->disconnected_ || - !p_->stateBetween(Private::CameraConfigured, - Private::CameraRunning)) + int ret = p_->isAccessAllowed(Private::CameraConfigured, + Private::CameraRunning); + if (ret < 0) return nullptr; return new Request(this, cookie); @@ -799,11 +823,9 @@ Request *Camera::createRequest(uint64_t cookie) */ int Camera::queueRequest(Request *request) { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraRunning)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraRunning); + if (ret < 0) + return ret; if (request->buffers().empty()) { LOG(Camera, Error) << "Request contains no buffers"; @@ -837,11 +859,9 @@ int Camera::queueRequest(Request *request) */ int Camera::start() { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraConfigured)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraConfigured); + if (ret < 0) + return ret; LOG(Camera, Debug) << "Starting capture"; @@ -852,11 +872,11 @@ int Camera::start() p_->pipe_->importFrameBuffers(this, stream); } - int ret = p_->pipe_->start(this); + ret = p_->pipe_->start(this); if (ret) return ret; - p_->state_ = Private::CameraRunning; + p_->setState(Private::CameraRunning); return 0; } @@ -875,15 +895,13 @@ int Camera::start() */ int Camera::stop() { - if (p_->disconnected_) - return -ENODEV; - - if (!p_->stateIs(Private::CameraRunning)) - return -EACCES; + int ret = p_->isAccessAllowed(Private::CameraRunning); + if (ret < 0) + return ret; LOG(Camera, Debug) << "Stopping capture"; - p_->state_ = Private::CameraConfigured; + p_->setState(Private::CameraConfigured); p_->pipe_->stop(this); diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 01b9ede34b53..5476dbab74d5 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * exportFrameBuffers() and importFrameBuffers() for the streams contained in * any camera configuration. * - * The only intended caller is the FrameBufferAllocator helper. + * The only intended caller is Camera::exportFrameBuffers(). * * \return The number of allocated buffers on success or a negative error code * otherwise @@ -358,8 +358,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * called only after a successful call to either of these two methods, and only * once per stream. * - * The only intended callers are Camera::stop() and the FrameBufferAllocator - * helper. + * The only intended callers are Camera::stop() and Camera::freeFrameBuffers(). */ /** From patchwork Wed Jan 22 20:57:16 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2722 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id C56CC60885 for ; Wed, 22 Jan 2020 21:57:44 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 6FF802E5 for ; Wed, 22 Jan 2020 21:57:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726664; bh=2fgBu15L+B46kfEzGWuKnK8L+KrMOBDQS8P6z3Zp6c4=; h=From:To:Subject:Date:In-Reply-To:References:From; b=I0Z5X8odMymlTmFl7+nccu8P/1DzMWveUe2UNZoKkZeH9yzdabGW2adZQ6ZjrpM2A z1NqImNvJUVdsrykX5CGamvIhAS+GPaukwgMQpvGo2pevXTDP8pfsooiXj6YZ/k6GK MBLS7sSKJkkDdrqGgpH8y1Ldg7Hk6e7kXaXPK9so= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:16 +0200 Message-Id: <20200122205723.8865-7-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 06/13] libcamera: Define the threading model X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:47 -0000 Document the design of libcamera's threading support, and prepare to document thread-safety of classes and functions with a doxygen alias command. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Kieran Bingham Reviewed-by: Niklas Söderlund --- Changes since v1: - Move the thread-reentrancy after thread-objects and thread-signals - Add a sentence that was mistakenly part of another patch - Fix typos --- Documentation/Doxyfile.in | 4 +- src/libcamera/thread.cpp | 84 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in index 1f746393568a..1c46b04b3f7e 100644 --- a/Documentation/Doxyfile.in +++ b/Documentation/Doxyfile.in @@ -239,7 +239,9 @@ TAB_SIZE = 4 # newlines (in the resulting output). You can put ^^ in the value part of an # alias to insert a newline as if a physical newline was in the original file. -ALIASES = +ALIASES = "context=\xrefitem context \"Thread Safety\" \"Thread Safety\"" +ALIASES += "threadbound=\ref thread-bound \"thread-bound\"" +ALIASES += "threadsafe=\ref thread-safe \"thread-safe\"" # This tag can be used to specify a number of word-keyword mappings (TCL only). # A mapping has the form "name=value". For example adding "class=itcl::class" diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index fe32cd677596..38e01771111d 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -19,6 +19,90 @@ #include "log.h" #include "message.h" +/** + * \page thread Thread Support + * + * libcamera supports multi-threaded applications through a threading model that + * sets precise rules to guarantee thread-safe usage of the API. Additionally, + * libcamera makes internal use of threads, and offers APIs that simplify + * interactions with application threads. Careful compliance with the threading + * model will ensure avoidance of race conditions. + * + * \section thread-objects Threads and Objects + * + * Instances of the Object class and all its derived classes are thread-aware + * and are bound to the thread they are created in. They are said to *live* in + * a thread, and they interact with the event loop of their thread for the + * purpose of message passing and signal delivery. Messages posted to the + * object with Object::postMessage() will be delivered from the event loop of + * the thread that the object lives in. Signals delivered to the object, unless + * explicitly connected with ConnectionTypeDirect, will also be delivered from + * the object thread's event loop. + * + * All Object instances created by libcamera are bound to an internal thread, + * and applications don't need to provide an event loop to support them. Object + * instances created by applications require an event loop. It is the + * responsibility of applications to provide that event loop, either explicitly + * through CameraManager::setEventDispatcher(), or by running the default event + * loop provided by CameraManager::eventDispatcher() in their main thread. The + * main thread of an application is the one that calls CameraManager::start(). + * + * \section thread-signals Threads and Signals + * + * When sent to a receiver that does not inherit from the Object class, signals + * are delivered synchronously in the thread of the sender. When the receiver + * inherits from the Object class, delivery is by default asynchronous if the + * sender and receiver live in different threads. In that case, the signal is + * posted to the receiver's message queue and will be delivered from the + * receiver's event loop, running in the receiver's thread. This mechanism can + * be overridden by selecting a different connection type when calling + * Signal::connect(). + * + * Asynchronous signal delivery is used internally in libcamera, but is also + * available to applications if desired. To use this feature, applications + * shall create receiver classes that inherit from the Object class, and + * provide an event loop to the CameraManager as explained above. Note that + * Object instances created by the application are limited to living in the + * application's main thread. Creating Object instances from another thread of + * an application causes undefined behaviour. + * + * \section thread-reentrancy Reentrancy and Thread-Safety + * + * Through the documentation, several terms are used to define how classes and + * their member functions can be used from multiple threads. + * + * - A **reentrant** function may be called simultaneously from multiple + * threads if and only if each invocation uses a different instance of the + * class. This is the default for all member functions not explictly marked + * otherwise. + * + * - \anchor thread-safe A **thread-safe** function may be called + * simultaneously from multiple threads on the same instance of a class. A + * thread-safe function is thus reentrant. Thread-safe functions may also be + * called simultaneously with any other reentrant function of the same class + * on the same instance. + * + * - \anchor thread-bound A **thread-bound** function may be called only from + * the thread that the class instances lives in (see section \ref + * thread-objects). For instances of classes that do not derive from the + * Object class, this is the thread in which the instance was created. A + * thread-bound function is not thread-safe, and may or may not be reentrant. + * + * Neither reentrancy nor thread-safety, in this context, mean that a function + * may be called simultaneously from the same thread, for instance from a + * callback invoked by the function. This may deadlock and isn't allowed unless + * separately documented. + * + * A class is defined as reentrant, thread-safe or thread-bound if all its + * member functions are reentrant, thread-safe or thread-bound respectively. + * Some member functions may additionally be documented as having additional + * thread-related attributes. + * + * Most classes are reentrant but not thread-safe, as making them fully + * thread-safe would incur locking costs considered prohibitive for the + * expected use cases. + */ + /** * \file thread.h * \brief Thread support From patchwork Wed Jan 22 20:57:17 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2723 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B04B60890 for ; Wed, 22 Jan 2020 21:57:45 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id BE15B2F9 for ; Wed, 22 Jan 2020 21:57:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726664; bh=XEGGuOsYttLglPAufH/U5dDguG8C6OG9/6uQKqC3CmM=; h=From:To:Subject:Date:In-Reply-To:References:From; b=HpqkYBd+XKDKR0fvohjsaGZDVA68e4ZU09teLY5xW99IK9aq1gQ8UCy3giwH8Q1YO gAetIj2GsdG2KF99X9pCJ0vL96tbBh24YVUNOW8drKSrS85GVGXaxGk61JhkCaC1hb 6vDWDBq+2EoINhODy9V+ECooB21Th56ldME3jdEQ= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:17 +0200 Message-Id: <20200122205723.8865-8-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 07/13] libcamera: Document thread-safety attributes of core classes X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:47 -0000 Define the thread-safety attributes of the classes and methods that are either thread-safe or thread-bound. The CameraManager, Camera and PipelineHandler will be addressed separately. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/device_enumerator.cpp | 2 ++ src/libcamera/event_notifier.cpp | 2 ++ src/libcamera/ipc_unixsocket.cpp | 2 ++ src/libcamera/object.cpp | 10 ++++++++-- src/libcamera/thread.cpp | 2 ++ src/libcamera/timer.cpp | 20 ++++++++++++-------- src/libcamera/v4l2_videodevice.cpp | 2 ++ 7 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp index a8b5c90f5a5d..64cdc132308a 100644 --- a/src/libcamera/device_enumerator.cpp +++ b/src/libcamera/device_enumerator.cpp @@ -190,6 +190,8 @@ DeviceEnumerator::~DeviceEnumerator() * with a warning message logged, without returning an error. Only errors that * prevent enumeration altogether shall be fatal. * + * \context This function is \threadbound. + * * \return 0 on success or a negative error code otherwise */ diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp index 4326b0b413e2..a9be686f79ae 100644 --- a/src/libcamera/event_notifier.cpp +++ b/src/libcamera/event_notifier.cpp @@ -99,6 +99,8 @@ EventNotifier::~EventNotifier() * * This function enables or disables the notifier. A disabled notifier ignores * events and does not emit the \ref activated signal. + * + * \context This function is \threadbound. */ void EventNotifier::setEnabled(bool enable) { diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp index eb1a50239188..6e5cab894a93 100644 --- a/src/libcamera/ipc_unixsocket.cpp +++ b/src/libcamera/ipc_unixsocket.cpp @@ -62,6 +62,8 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket) * communication method. The remote side then instantiates a socket, and binds * it to the other side by passing the file descriptor to bind(). At that point * the channel is operation and communication is bidirectional and symmmetrical. + * + * \context This class is \threadbound. */ IPCUnixSocket::IPCUnixSocket() diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp index f2a8be172547..99c3bf9a709b 100644 --- a/src/libcamera/object.cpp +++ b/src/libcamera/object.cpp @@ -110,6 +110,8 @@ Object::~Object() * Messages are delivered through the thread's event loop. If the thread is not * running its event loop the message will not be delivered until the event * loop gets started. + * + * \context This function is \threadsafe. */ void Object::postMessage(std::unique_ptr msg) { @@ -162,6 +164,8 @@ void Object::message(Message *msg) * are passed untouched. The caller shall ensure that any pointer argument * remains valid until the method is invoked. * + * \context This function is \threadsafe. + * * \return For connection types ConnectionTypeDirect and * ConnectionTypeBlocking, return the return value of the invoked method. For * connection type ConnectionTypeQueued, return a default-constructed R value. @@ -170,6 +174,7 @@ void Object::message(Message *msg) /** * \fn Object::thread() * \brief Retrieve the thread the object is bound to + * \context This function is \threadsafe. * \return The thread the object is bound to */ @@ -178,8 +183,7 @@ void Object::message(Message *msg) * \param[in] thread The target thread * * This method moves the object and all its children from the current thread to - * the new \a thread. It shall be called from the thread in which the object - * currently lives, otherwise the behaviour is undefined. + * the new \a thread. * * Before the object is moved, a Message::ThreadMoveMessage message is sent to * it. The message() method can be reimplement in derived classes to be notified @@ -187,6 +191,8 @@ void Object::message(Message *msg) * * Moving an object that has a parent is not allowed, and causes undefined * behaviour. + * + * \context This function is thread-bound. */ void Object::moveToThread(Thread *thread) { diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index 38e01771111d..1d0a600ab5cc 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -222,6 +222,8 @@ ThreadData *ThreadData::current() * called. A custom event dispatcher may be installed with * setEventDispatcher(), otherwise a poll-based event dispatcher is used. This * behaviour can be overriden by overloading the run() method. + * + * \context This class is \threadsafe. */ /** diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp index 4c68883204e8..24da51524efb 100644 --- a/src/libcamera/timer.cpp +++ b/src/libcamera/timer.cpp @@ -67,16 +67,18 @@ Timer::~Timer() * \brief Start or restart the timer with a timeout of \a msec * \param[in] msec The timer duration in milliseconds * - * This method shall be called from the thread the timer is associated with. If - * the timer is already running it will be stopped and restarted. + * If the timer is already running it will be stopped and restarted. + * + * \context This function is \threadbound. */ /** * \brief Start or restart the timer with a timeout of \a duration * \param[in] duration The timer duration in milliseconds * - * This method shall be called from the thread the timer is associated with. If - * the timer is already running it will be stopped and restarted. + * If the timer is already running it will be stopped and restarted. + * + * \context This function is \threadbound. */ void Timer::start(std::chrono::milliseconds duration) { @@ -87,8 +89,9 @@ void Timer::start(std::chrono::milliseconds duration) * \brief Start or restart the timer with a \a deadline * \param[in] deadline The timer deadline * - * This method shall be called from the thread the timer is associated with. If - * the timer is already running it will be stopped and restarted. + * If the timer is already running it will be stopped and restarted. + * + * \context This function is \threadbound. */ void Timer::start(std::chrono::steady_clock::time_point deadline) { @@ -115,8 +118,9 @@ void Timer::start(std::chrono::steady_clock::time_point deadline) * After this function returns the timer is guaranteed not to emit the * \ref timeout signal. * - * This method shall be called from the thread the timer is associated with. If - * the timer is not running this function performs no operation. + * If the timer is not running this function performs no operation. + * + * \context This function is \threadbound. */ void Timer::stop() { diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 82267730289d..b874f2384a53 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -403,6 +403,8 @@ const std::string V4L2DeviceFormat::toString() const * * Upon destruction any device left open will be closed, and any resources * released. + * + * \context This class is \threadbound. */ /** From patchwork Wed Jan 22 20:57:18 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2724 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D4AC60881 for ; Wed, 22 Jan 2020 21:57:45 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 181E82E5 for ; Wed, 22 Jan 2020 21:57:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726665; bh=vebNeGtWI0w6k+eCpMICX8kMcmih7ByGwkSV+rMLlPw=; h=From:To:Subject:Date:In-Reply-To:References:From; b=tDaL/PdQ9r64ykxIFMh5oDKcALJmz0N8xEGgbOPmh0sQMlIrzZmOKz6In20Bvjhb/ 3+E16tU2ZskyJfD7wl2aUJDqv7Il7o52fsoas6azpRfAj0HwLdsqnkeDUMYmbWjyh1 ynvuCOBY428erkSTkQ8Y/uxgeRPktoJD1AY8Jjxo= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:18 +0200 Message-Id: <20200122205723.8865-9-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 08/13] libcamera: signal: Make connection and disconnection thread-safe X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:48 -0000 Make the signal connection and disconnection thread-safe, and document them as such. This is required to make objects connectable from different threads. The connect(), disconnect() and emit() methods are now all protected by a global mutex, which may generate a high lock contention. This could be improved with finer-grained locks or with a pool of mutexes. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- src/libcamera/signal.cpp | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp index 9bb7eca8ed6e..6eab1fa74d42 100644 --- a/src/libcamera/signal.cpp +++ b/src/libcamera/signal.cpp @@ -7,6 +7,8 @@ #include +#include "thread.h" + /** * \file signal.h * \brief Signal & slot implementation @@ -14,8 +16,21 @@ namespace libcamera { +namespace { + +/* + * Mutex to protect the SignalBase::slots_ and Object::signals_ lists. If lock + * contention needs to be decreased, this could be replaced with locks in + * Object and SignalBase, or with a mutex pool. + */ +Mutex signalsLock; + +} /* namespace */ + void SignalBase::connect(BoundMethodBase *slot) { + MutexLocker locker(signalsLock); + Object *object = slot->object(); if (object) object->connect(this); @@ -31,6 +46,8 @@ void SignalBase::disconnect(Object *object) void SignalBase::disconnect(std::function match) { + MutexLocker locker(signalsLock); + for (auto iter = slots_.begin(); iter != slots_.end(); ) { if (match(iter)) { Object *object = (*iter)->object(); @@ -47,6 +64,7 @@ void SignalBase::disconnect(std::function match) SignalBase::SlotList SignalBase::slots() { + MutexLocker locker(signalsLock); return slots_; } @@ -99,23 +117,31 @@ SignalBase::SlotList SignalBase::slots() * disconnected from the \a func slot of \a object when \a object is destroyed. * Otherwise the caller shall disconnect signals manually before destroying \a * object. + * + * \context This function is \threadsafe. */ /** * \fn Signal::connect(R (*func)(Args...)) * \brief Connect the signal to a static function slot * \param[in] func The slot static function + * + * \context This function is \threadsafe. */ /** * \fn Signal::disconnect() * \brief Disconnect the signal from all slots + * + * \context This function is \threadsafe. */ /** * \fn Signal::disconnect(T *object) * \brief Disconnect the signal from all slots of the \a object * \param[in] object The object pointer whose slots to disconnect + * + * \context This function is \threadsafe. */ /** @@ -123,12 +149,16 @@ SignalBase::SlotList SignalBase::slots() * \brief Disconnect the signal from the \a object slot member function \a func * \param[in] object The object pointer whose slots to disconnect * \param[in] func The slot member function to disconnect + * + * \context This function is \threadsafe. */ /** * \fn Signal::disconnect(R (*func)(Args...)) * \brief Disconnect the signal from the slot static function \a func * \param[in] func The slot static function to disconnect + * + * \context This function is \threadsafe. */ /** @@ -141,6 +171,9 @@ SignalBase::SlotList SignalBase::slots() * function are passed to the slot functions unchanged. If a slot modifies one * of the arguments (when passed by pointer or reference), the modification is * thus visible to all subsequently called slots. + * + * This function is not \threadsafe, but thread-safety is guaranteed against + * concurrent connect() and disconnect() calls. */ } /* namespace libcamera */ From patchwork Wed Jan 22 20:57:19 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2725 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id BB9DF60883 for ; Wed, 22 Jan 2020 21:57:45 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 6535D2F9 for ; Wed, 22 Jan 2020 21:57:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726665; bh=8M1B515kSSEKe77KbFMO71pqYBDFHYuOtjx8kJJarMg=; h=From:To:Subject:Date:In-Reply-To:References:From; b=rbcAhGtL01Uy1x2z5q2m/O8iOCQ00ceJhLitPK2q3r/1jcF+TECuKT8n+3i0+rrc8 3Tk8dcwGyL+4UaiV85UnamomubMEHFRIvmsuWB4IMgi+N4xw+E+uOLlz5LbtOG37J6 jY5ZG0gfW6CZScyuo3c6XujKdGffy05r2qW05Qrg= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:19 +0200 Message-Id: <20200122205723.8865-10-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 09/13] libcamera: camera_manager: Run the camera manager in a thread X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:48 -0000 Relying on the application event loop to process all our internal events is a bad idea for multiple reasons. In many cases the user of libcamera can't provide an event loop, for instance when running through one of the adaptation layers. The Android camera HAL and V4L2 compatibility layer create a thread for this reason, and the GStreamer element would need to do so as well. Furthermore, relying on the application event loop pushes libcamera's realtime constraints to the application, which isn't manageable. For these reasons it's desirable to always run the camera manager, the pipeline handlers and the cameras in a separate thread. Doing so isn't too complicated, it only involves creating the thread internally when starting the camera manager, and synchronizing a few methods of the Camera class. Do so as a first step towards defining the threading model of libcamera. The event dispatcher interface is still exposed to applications, to enable cross-thread signal delivery if desired. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- Changes since v1: - Move "libcamera: camera_manager: Move private data members to private implementation" and "libcamera: camera_manager: Return a copy of the vector from cameras()" to separate patches --- src/libcamera/camera_manager.cpp | 94 ++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 6 deletions(-) diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 5fc1bba974c6..11c333ccb4e0 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -7,6 +7,7 @@ #include +#include #include #include @@ -28,33 +29,89 @@ namespace libcamera { LOG_DEFINE_CATEGORY(Camera) -class CameraManager::Private +class CameraManager::Private : public Thread { public: Private(CameraManager *cm); int start(); - void stop(); - void addCamera(std::shared_ptr &camera, dev_t devnum); void removeCamera(Camera *camera); + /* + * This mutex protects + * + * - initialized_ and status_ during initialization + * - cameras_ and camerasByDevnum_ after initialization + */ + Mutex mutex_; std::vector> cameras_; std::map> camerasByDevnum_; +protected: + void run() override; + private: + int init(); + void cleanup(); + CameraManager *cm_; + std::condition_variable cv_; + bool initialized_; + int status_; + std::vector> pipes_; std::unique_ptr enumerator_; }; CameraManager::Private::Private(CameraManager *cm) - : cm_(cm) + : cm_(cm), initialized_(false) { } int CameraManager::Private::start() +{ + /* Start the thread and wait for initialization to complete. */ + Thread::start(); + + { + MutexLocker locker(mutex_); + cv_.wait(locker, [&] { return initialized_; }); + } + + /* If a failure happened during initialization, stop the thread. */ + if (status_ < 0) { + exit(); + wait(); + return status_; + } + + return 0; +} + +void CameraManager::Private::run() +{ + LOG(Camera, Debug) << "Starting camera manager"; + + int ret = init(); + + mutex_.lock(); + status_ = ret; + initialized_ = true; + mutex_.unlock(); + cv_.notify_one(); + + if (ret < 0) + return; + + /* Now start processing events and messages. */ + exec(); + + cleanup(); +} + +int CameraManager::Private::init() { enumerator_ = DeviceEnumerator::create(); if (!enumerator_ || enumerator_->enumerate()) @@ -89,7 +146,7 @@ int CameraManager::Private::start() return 0; } -void CameraManager::Private::stop() +void CameraManager::Private::cleanup() { /* TODO: unregister hot-plug callback here */ @@ -107,6 +164,8 @@ void CameraManager::Private::stop() void CameraManager::Private::addCamera(std::shared_ptr &camera, dev_t devnum) { + MutexLocker locker(mutex_); + for (std::shared_ptr c : cameras_) { if (c->name() == camera->name()) { LOG(Camera, Warning) @@ -126,6 +185,8 @@ void CameraManager::Private::addCamera(std::shared_ptr &camera, void CameraManager::Private::removeCamera(Camera *camera) { + MutexLocker locker(mutex_); + auto iter = std::find_if(cameras_.begin(), cameras_.end(), [camera](std::shared_ptr &c) { return c.get() == camera; @@ -232,7 +293,8 @@ int CameraManager::start() */ void CameraManager::stop() { - p_->stop(); + p_->exit(); + p_->wait(); } /** @@ -242,10 +304,14 @@ void CameraManager::stop() * Before calling this function the caller is responsible for ensuring that * the camera manager is running. * + * \context This function is \threadsafe. + * * \return List of all available cameras */ std::vector> CameraManager::cameras() const { + MutexLocker locker(p_->mutex_); + return p_->cameras_; } @@ -256,10 +322,14 @@ std::vector> CameraManager::cameras() const * Before calling this function the caller is responsible for ensuring that * the camera manager is running. * + * \context This function is \threadsafe. + * * \return Shared pointer to Camera object or nullptr if camera not found */ std::shared_ptr CameraManager::get(const std::string &name) { + MutexLocker locker(p_->mutex_); + for (std::shared_ptr camera : p_->cameras_) { if (camera->name() == name) return camera; @@ -279,11 +349,15 @@ std::shared_ptr CameraManager::get(const std::string &name) * Before calling this function the caller is responsible for ensuring that * the camera manager is running. * + * \context This function is \threadsafe. + * * \return Shared pointer to Camera object, which is empty if the camera is * not found */ std::shared_ptr CameraManager::get(dev_t devnum) { + MutexLocker locker(p_->mutex_); + auto iter = p_->camerasByDevnum_.find(devnum); if (iter == p_->camerasByDevnum_.end()) return nullptr; @@ -302,9 +376,12 @@ std::shared_ptr CameraManager::get(dev_t devnum) * * \a devnum is used by the V4L2 compatibility layer to map V4L2 device nodes * to Camera instances. + * + * \context This function shall be called from the CameraManager thread. */ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum) { + ASSERT(Thread::current() == p_.get()); p_->addCamera(camera, devnum); } @@ -316,15 +393,20 @@ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum) * This function is called by pipeline handlers to unregister cameras from the * camera manager. Unregistered cameras won't be reported anymore by the * cameras() and get() calls, but references may still exist in applications. + * + * \context This function shall be called from the CameraManager thread. */ void CameraManager::removeCamera(Camera *camera) { + ASSERT(Thread::current() == p_.get()); + p_->removeCamera(camera); } /** * \fn const std::string &CameraManager::version() * \brief Retrieve the libcamera version string + * \context This function is \a threadsafe. * \return The libcamera version string */ From patchwork Wed Jan 22 20:57:20 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2726 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A1FF60894 for ; Wed, 22 Jan 2020 21:57:46 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id B7FE8595 for ; Wed, 22 Jan 2020 21:57:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726665; bh=xqTEHQhbL8QzqgB7RTQsssPL05nEOSLfWEawC/QGuXU=; h=From:To:Subject:Date:In-Reply-To:References:From; b=S83rmv79NPwLpO81I+6CmhcklQli022R5OVW4qpqlGwyhRVJyMC44oRJqMHoKWuQD aRLUwhFAtpkYmIwEjYwT25BxIuizcNuKDIhwI2H2dUOGnn6t1yvWKejPjN7aGfhpaP UU4rHemceWPmL5z5iw95/WCtsZHD+LPEKtbmpSz4= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:20 +0200 Message-Id: <20200122205723.8865-11-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 10/13] libcamera: camera: Implement the threading model X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:48 -0000 Document the threading model of the Camera class and implement it. Selected functions become thread-safe, and require a few functions of the PipelineHandler class to be called through cross-thread invocation as the pipeline handlers live in the camera manager thread, while the Camera class is mostly accessed from the application thread. The PipelineHandler is made to inherit from the Object class to support this. Disconnection is currently left out as it is not implemented in pipeline handlers, and isn't fully supported in the Camera class either. This will be revisited when implementing proper hotplug support. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/camera.cpp | 106 +++++++++++++++++------ src/libcamera/include/pipeline_handler.h | 4 +- 2 files changed, 83 insertions(+), 27 deletions(-) diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 802e7fd0d12f..44f2d71b4959 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -7,6 +7,7 @@ #include +#include #include #include @@ -282,7 +283,7 @@ public: private: bool disconnected_; - State state_; + std::atomic state_; }; Camera::Private::Private(PipelineHandler *pipe, const std::string &name, @@ -294,7 +295,7 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name, Camera::Private::~Private() { - if (state_ != Private::CameraAvailable) + if (state_.load(std::memory_order_acquire) != Private::CameraAvailable) LOG(Camera, Error) << "Removing camera while still in use"; } @@ -310,12 +311,13 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const if (!allowDisconnected && disconnected_) return -ENODEV; - if (state_ == state) + State currentState = state_.load(std::memory_order_acquire); + if (currentState == state) return 0; ASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names)); - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] << " state trying operation requiring state " << camera_state_names[state]; @@ -328,13 +330,14 @@ int Camera::Private::isAccessAllowed(State low, State high, if (!allowDisconnected && disconnected_) return -ENODEV; - if (state_ >= low && state_ <= high) + State currentState = state_.load(std::memory_order_acquire); + if (currentState >= low && currentState <= high) return 0; ASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) && static_cast(high) < ARRAY_SIZE(camera_state_names)); - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_] + LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState] << " state trying operation requiring state between " << camera_state_names[low] << " and " << camera_state_names[high]; @@ -344,15 +347,20 @@ int Camera::Private::isAccessAllowed(State low, State high, void Camera::Private::disconnect() { - if (state_ == Private::CameraRunning) - state_ = Private::CameraConfigured; + /* + * If the camera was running when the hardware was removed force the + * state to Configured state to allow applications to free resources + * and call release() before deleting the camera. + */ + if (state_.load(std::memory_order_acquire) == Private::CameraRunning) + state_.store(Private::CameraConfigured, std::memory_order_release); disconnected_ = true; } void Camera::Private::setState(State state) { - state_ = state; + state_.store(state, std::memory_order_release); } /** @@ -384,12 +392,17 @@ void Camera::Private::setState(State state) * An application may start and stop a camera multiple times as long as it is * not released. The camera may also be reconfigured. * + * Functions that affect the camera state as defined below are generally not + * synchronized with each other by the Camera class. The caller is responsible + * for ensuring their synchronization if necessary. + * * \subsection Camera States * * To help manage the sequence of operations needed to control the camera a set * of states are defined. Each state describes which operations may be performed - * on the camera. Operations not listed in the state diagram are allowed in all - * states. + * on the camera. Performing an operation not allowed in the camera state + * results in undefined behaviour. Operations not listed at all in the state + * diagram are allowed in all states. * * \dot * digraph camera_state_machine { @@ -462,6 +475,7 @@ std::shared_ptr Camera::create(PipelineHandler *pipe, /** * \brief Retrieve the name of the camera + * \context This function is \threadsafe. * \return Name of the camera device */ const std::string &Camera::name() const @@ -535,7 +549,9 @@ int Camera::exportFrameBuffers(Stream *stream, if (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) return -EINVAL; - return p_->pipe_->exportFrameBuffers(this, stream, buffers); + return p_->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers, + ConnectionTypeBlocking, this, stream, + buffers); } int Camera::freeFrameBuffers(Stream *stream) @@ -544,7 +560,8 @@ int Camera::freeFrameBuffers(Stream *stream) if (ret < 0) return ret; - p_->pipe_->freeFrameBuffers(this, stream); + p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, + ConnectionTypeBlocking, this, stream); return 0; } @@ -566,7 +583,8 @@ int Camera::freeFrameBuffers(Stream *stream) * Once exclusive access isn't needed anymore, the device should be released * with a call to the release() function. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function is \threadsafe. It may only be called when the camera + * is in the Available state as defined in \ref camera_operation. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -574,6 +592,10 @@ int Camera::freeFrameBuffers(Stream *stream) */ int Camera::acquire() { + /* + * No manual locking is required as PipelineHandler::lock() is + * thread-safe. + */ int ret = p_->isAccessAllowed(Private::CameraAvailable); if (ret < 0) return ret == -EACCES ? -EBUSY : ret; @@ -595,7 +617,10 @@ int Camera::acquire() * Releasing the camera device allows other users to acquire exclusive access * with the acquire() function. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the + * Available or Configured state as defined in \ref camera_operation, and shall + * be synchronized by the caller with other functions that affect the camera + * state. * * \return 0 on success or a negative error code otherwise * \retval -EBUSY The camera is running and can't be released @@ -629,6 +654,8 @@ int Camera::release() * * Camera controls remain constant through the lifetime of the camera. * + * \context This function is \threadsafe. + * * \return A ControlInfoMap listing the controls supported by the camera */ const ControlInfoMap &Camera::controls() @@ -643,6 +670,8 @@ const ControlInfoMap &Camera::controls() * information describes among other things how many streams the camera * supports and the capabilities of each stream. * + * \context This function is \threadsafe. + * * \return An array of all the camera's streams. */ const std::set &Camera::streams() const @@ -660,6 +689,8 @@ const std::set &Camera::streams() const * empty list of roles is valid, and will generate an empty configuration that * can be filled by the caller. * + * \context This function is \threadsafe. + * * \return A CameraConfiguration if the requested roles can be satisfied, or a * null pointer otherwise. The ownership of the returned configuration is * passed to the caller. @@ -710,7 +741,10 @@ std::unique_ptr Camera::generateConfiguration(const StreamR * Exclusive access to the camera shall be ensured by a call to acquire() prior * to calling this function, otherwise an -EACCES error will be returned. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the Acquired + * or Configured state as defined in \ref camera_operation, and shall be + * synchronized by the caller with other functions that affect the camera + * state. * * Upon return the StreamConfiguration entries in \a config are associated with * Stream instances which can be retrieved with StreamConfiguration::stream(). @@ -749,7 +783,8 @@ int Camera::configure(CameraConfiguration *config) LOG(Camera, Info) << msg.str(); - ret = p_->pipe_->configure(this, config); + ret = p_->pipe_->invokeMethod(&PipelineHandler::configure, + ConnectionTypeBlocking, this, config); if (ret) return ret; @@ -784,8 +819,8 @@ int Camera::configure(CameraConfiguration *config) * The ownership of the returned request is passed to the caller, which is * responsible for either queueing the request or deleting it. * - * This function shall only be called when the camera is in the Configured - * or Running state, see \ref camera_operation. + * \context This function is \threadsafe. It may only be called when the camera + * is in the Configured or Running state as defined in \ref camera_operation. * * \return A pointer to the newly created request, or nullptr on error */ @@ -815,6 +850,9 @@ Request *Camera::createRequest(uint64_t cookie) * Ownership of the request is transferred to the camera. It will be deleted * automatically after it completes. * + * \context This function is \threadsafe. It may only be called when the camera + * is in the Running state as defined in \ref camera_operation. + * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system * \retval -EACCES The camera is not running so requests can't be queued @@ -827,6 +865,12 @@ int Camera::queueRequest(Request *request) if (ret < 0) return ret; + /* + * The camera state may chance until the end of the function. No locking + * is however needed as PipelineHandler::queueRequest() will handle + * this. + */ + if (request->buffers().empty()) { LOG(Camera, Error) << "Request contains no buffers"; return -EINVAL; @@ -841,7 +885,8 @@ int Camera::queueRequest(Request *request) } } - return p_->pipe_->queueRequest(this, request); + return p_->pipe_->invokeMethod(&PipelineHandler::queueRequest, + ConnectionTypeQueued, this, request); } /** @@ -851,7 +896,10 @@ int Camera::queueRequest(Request *request) * can queue requests to the camera to process and return to the application * until the capture session is terminated with \a stop(). * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the + * Configured state as defined in \ref camera_operation, and shall be + * synchronized by the caller with other functions that affect the camera + * state. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -869,10 +917,12 @@ int Camera::start() if (allocator_ && !allocator_->buffers(stream).empty()) continue; - p_->pipe_->importFrameBuffers(this, stream); + p_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers, + ConnectionTypeDirect, this, stream); } - ret = p_->pipe_->start(this); + ret = p_->pipe_->invokeMethod(&PipelineHandler::start, + ConnectionTypeBlocking, this); if (ret) return ret; @@ -887,7 +937,9 @@ int Camera::start() * This method stops capturing and processing requests immediately. All pending * requests are cancelled and complete synchronously in an error state. * - * This function affects the state of the camera, see \ref camera_operation. + * \context This function may only be called when the camera is in the Running + * state as defined in \ref camera_operation, and shall be synchronized by the + * caller with other functions that affect the camera state. * * \return 0 on success or a negative error code otherwise * \retval -ENODEV The camera has been disconnected from the system @@ -903,13 +955,15 @@ int Camera::stop() p_->setState(Private::CameraConfigured); - p_->pipe_->stop(this); + p_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, + this); for (Stream *stream : p_->activeStreams_) { if (allocator_ && !allocator_->buffers(stream).empty()) continue; - p_->pipe_->freeFrameBuffers(this, stream); + p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, + ConnectionTypeBlocking, this, stream); } return 0; diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index a6c1e1fbae38..db0ec6ac00d7 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -17,6 +17,7 @@ #include #include +#include #include namespace libcamera { @@ -51,7 +52,8 @@ private: CameraData &operator=(const CameraData &) = delete; }; -class PipelineHandler : public std::enable_shared_from_this +class PipelineHandler : public std::enable_shared_from_this, + public Object { public: PipelineHandler(CameraManager *manager); From patchwork Wed Jan 22 20:57:21 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2727 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 6680E60898 for ; Wed, 22 Jan 2020 21:57:46 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 10C042F9 for ; Wed, 22 Jan 2020 21:57:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726666; bh=bAXB8AyBsIGz/tQW5GiZEnVhwnLjyVw2idQhmizf2Cs=; h=From:To:Subject:Date:In-Reply-To:References:From; b=mESRwBhcvMF5NkqyWnk5uRDw5IAyjCvkvzx3CYalCDIt1tldKbzd2KL8gjL+7LTFR gNuGGZVB4kgWatgY4nPATOqudqL8JUi5EKLIToOljaqL6qR2Gu7DsI1YLQt0jsxaMr o/jlXytgLtxYdF0kpMul8V+1W56cBf8eIk7Sl3zs= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:21 +0200 Message-Id: <20200122205723.8865-12-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 11/13] libcamera: pipeline_handler: Document the threading model X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:48 -0000 Document the threading model of the PipelineHandler class (and all its derived classes). The model is already enforced by the Camera class, so no change in the implementation is required. As for the Camera class, disconnection is currently left out. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/pipeline_handler.cpp | 36 ++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 5476dbab74d5..22889be43398 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -166,6 +166,8 @@ PipelineHandler::~PipelineHandler() * If this function returns true, a new instance of the pipeline handler will * be created and its match() function called. * + * \context This function is called from the CameraManager thread. + * * \return true if media devices have been acquired and camera instances * created, or false otherwise */ @@ -182,6 +184,8 @@ PipelineHandler::~PipelineHandler() * device explicitly, it will be automatically released when the pipeline * handler is destroyed. * + * \context This function shall be called from the CameraManager thread. + * * \return A pointer to the matching MediaDevice, or nullptr if no match is found */ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, @@ -205,6 +209,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, * This method shall not be called from pipeline handler implementation, as the * Camera class handles locking directly. * + * \context This function is \threadsafe. + * * \return True if the devices could be locked, false otherwise * \sa unlock() * \sa MediaDevice::lock() @@ -227,6 +233,8 @@ bool PipelineHandler::lock() * This method shall not be called from pipeline handler implementation, as the * Camera class handles locking directly. * + * \context This function is \threadsafe. + * * \sa lock() */ void PipelineHandler::unlock() @@ -238,6 +246,7 @@ void PipelineHandler::unlock() /** * \brief Retrieve the list of controls for a camera * \param[in] camera The camera + * \context This function is \threadsafe. * \return A ControlInfoMap listing the controls support by \a camera */ const ControlInfoMap &PipelineHandler::controls(Camera *camera) @@ -261,6 +270,10 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * The intended companion to this is \a configure() which can be used to change * the group of streams parameters. * + * \context This function may be called from any thread and shall be + * \threadsafe. It shall not modify the state of the \a camera in the pipeline + * handler. + * * \return A valid CameraConfiguration if the requested roles can be satisfied, * or a null pointer otherwise. The ownership of the returned configuration is * passed to the caller. @@ -286,6 +299,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * instance to each StreamConfiguration entry in the CameraConfiguration using * the StreamConfiguration::setStream() method. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -316,6 +331,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * * The only intended caller is Camera::exportFrameBuffers(). * + * \context This function is called from the CameraManager thread. + * * \return The number of allocated buffers on success or a negative error code * otherwise */ @@ -344,6 +361,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * * The only intended caller is Camera::start(). * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -359,6 +378,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * once per stream. * * The only intended callers are Camera::stop() and Camera::freeFrameBuffers(). + * + * \context This function is called from the CameraManager thread. */ /** @@ -371,6 +392,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * will in turn be called from the application to indicate that it has * configured the streams and is ready to capture. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -381,6 +404,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * * This method stops capturing and processing requests immediately. All pending * requests are cancelled and complete immediately in an error state. + * + * \context This function is called from the CameraManager thread. */ /** @@ -397,6 +422,8 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * when the pipeline handler is stopped with stop(). Request completion shall be * signalled by the pipeline handler using the completeRequest() method. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ int PipelineHandler::queueRequest(Camera *camera, Request *request) @@ -423,6 +450,8 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request) * parameters will be applied to the frames captured in the buffers provided in * the request. * + * \context This function is called from the CameraManager thread. + * * \return 0 on success or a negative error code otherwise */ @@ -439,6 +468,8 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request) * pipeline handlers a chance to perform any operation that may still be * needed. They shall complete requests explicitly with completeRequest(). * + * \context This function shall be called from the CameraManager thread. + * * \return True if all buffers contained in the request have completed, false * otherwise */ @@ -461,6 +492,8 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request, * This method ensures that requests will be returned to the application in * submission order, the pipeline handler may call it on any complete request * without any ordering constraint. + * + * \context This function shall be called from the CameraManager thread. */ void PipelineHandler::completeRequest(Camera *camera, Request *request) { @@ -494,6 +527,8 @@ void PipelineHandler::completeRequest(Camera *camera, Request *request) * is to be associated with. This is for the V4L2 compatibility layer to map * device nodes to Camera instances based on the device number * registered by this method in \a devnum. + * + * \context This function shall be called from the CameraManager thread. */ void PipelineHandler::registerCamera(std::shared_ptr camera, std::unique_ptr data, @@ -587,6 +622,7 @@ CameraData *PipelineHandler::cameraData(const Camera *camera) /** * \fn PipelineHandler::name() * \brief Retrieve the pipeline handler name + * \context This function shall be \threadsafe. * \return The pipeline handler name */ From patchwork Wed Jan 22 20:57:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2728 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id B306660893 for ; Wed, 22 Jan 2020 21:57:46 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 5DF112E5 for ; Wed, 22 Jan 2020 21:57:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726666; bh=WfJvYv+m/UaA5uvzwJC01dn3Xw6cGJUCVJKyRUB+0hc=; h=From:To:Subject:Date:In-Reply-To:References:From; b=crQ2EdAllS0rIteht4RVYaJ8uWvjdAOd+NDKpMEo557OoVA4q57z0lxFCyNMo0VZt BDPEp6hJJRBQzqv/PVDKbSHO+md5NdEv9pyhArtNEDPD+cA9f0aNj+Aj6WRv4b3xuc cbBOkqQ5npOPtgX+6P9fp5dPToZreSkE+PVPicWs= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:22 +0200 Message-Id: <20200122205723.8865-13-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 12/13] v4l2: Remove internal thread X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:49 -0000 Now that libcamera creates threads internally and doesn't rely on an application-provided event loop, remove the thread from the V4L2 compatibility layer. The split between the V4L2CameraProxy and V4L2Camera classes is still kept to separate the V4L2 adaptation from camera operation. This may be further refactored later. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/v4l2/v4l2_camera.h | 2 +- src/v4l2/v4l2_camera_proxy.cpp | 43 ++++++++++------------------ src/v4l2/v4l2_compat_manager.cpp | 48 +++++++++----------------------- src/v4l2/v4l2_compat_manager.h | 13 ++------- 4 files changed, 31 insertions(+), 75 deletions(-) diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index f1f04d9ef6ed..37bd358462db 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -21,7 +21,7 @@ using namespace libcamera; -class V4L2Camera : public Object +class V4L2Camera { public: struct Buffer { diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index e58fd6a0d8b5..622520479be0 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -41,8 +41,7 @@ int V4L2CameraProxy::open(bool nonBlocking) { LOG(V4L2Compat, Debug) << "Servicing open"; - int ret = vcam_->invokeMethod(&V4L2Camera::open, - ConnectionTypeBlocking); + int ret = vcam_->open(); if (ret < 0) { errno = -ret; return -1; @@ -50,8 +49,7 @@ int V4L2CameraProxy::open(bool nonBlocking) nonBlocking_ = nonBlocking; - vcam_->invokeMethod(&V4L2Camera::getStreamConfig, - ConnectionTypeBlocking, &streamConfig_); + vcam_->getStreamConfig(&streamConfig_); setFmtFromConfig(streamConfig_); sizeimage_ = calculateSizeImage(streamConfig_); @@ -72,7 +70,7 @@ void V4L2CameraProxy::close() if (--refcount_ > 0) return; - vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking); + vcam_->close(); } void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags, @@ -284,11 +282,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_->invokeMethod(&V4L2Camera::configure, - ConnectionTypeBlocking, - &streamConfig_, size, - v4l2ToDrm(arg->fmt.pix.pixelformat), - bufferCount_); + int ret = vcam_->configure(&streamConfig_, size, + v4l2ToDrm(arg->fmt.pix.pixelformat), + bufferCount_); if (ret < 0) return -EINVAL; @@ -319,13 +315,12 @@ int V4L2CameraProxy::freeBuffers() { LOG(V4L2Compat, Debug) << "Freeing libcamera bufs"; - int ret = vcam_->invokeMethod(&V4L2Camera::streamOff, - ConnectionTypeBlocking); + int ret = vcam_->streamOff(); if (ret < 0) { LOG(V4L2Compat, Error) << "Failed to stop stream"; return ret; } - vcam_->invokeMethod(&V4L2Camera::freeBuffers, ConnectionTypeBlocking); + vcam_->freeBuffers(); bufferCount_ = 0; return 0; @@ -349,11 +344,9 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg) return freeBuffers(); Size size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height); - ret = vcam_->invokeMethod(&V4L2Camera::configure, - ConnectionTypeBlocking, - &streamConfig_, size, - v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat), - arg->count); + ret = vcam_->configure(&streamConfig_, size, + v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat), + arg->count); if (ret < 0) return -EINVAL; @@ -366,8 +359,7 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg) arg->count = streamConfig_.bufferCount; bufferCount_ = arg->count; - ret = vcam_->invokeMethod(&V4L2Camera::allocBuffers, - ConnectionTypeBlocking, arg->count); + ret = vcam_->allocBuffers(arg->count); if (ret < 0) { arg->count = 0; return ret; @@ -415,8 +407,7 @@ int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg) arg->index >= bufferCount_) return -EINVAL; - int ret = vcam_->invokeMethod(&V4L2Camera::qbuf, ConnectionTypeBlocking, - arg->index); + int ret = vcam_->qbuf(arg->index); if (ret < 0) return ret; @@ -459,10 +450,7 @@ int V4L2CameraProxy::vidioc_streamon(int *arg) if (!validateBufferType(*arg)) return -EINVAL; - int ret = vcam_->invokeMethod(&V4L2Camera::streamOn, - ConnectionTypeBlocking); - - return ret; + return vcam_->streamOn(); } int V4L2CameraProxy::vidioc_streamoff(int *arg) @@ -472,8 +460,7 @@ int V4L2CameraProxy::vidioc_streamoff(int *arg) if (!validateBufferType(*arg)) return -EINVAL; - int ret = vcam_->invokeMethod(&V4L2Camera::streamOff, - ConnectionTypeBlocking); + int ret = vcam_->streamOff(); for (struct v4l2_buffer &buf : buffers_) buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE); diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index f5a7b2ac4229..961d06b3e39a 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -37,7 +37,7 @@ void get_symbol(T &func, const char *name) } /* namespace */ V4L2CompatManager::V4L2CompatManager() - : cm_(nullptr), initialized_(false) + : cm_(nullptr) { get_symbol(fops_.openat, "openat"); get_symbol(fops_.dup, "dup"); @@ -52,24 +52,15 @@ V4L2CompatManager::~V4L2CompatManager() devices_.clear(); mmaps_.clear(); - if (isRunning()) { - exit(0); - /* \todo Wait with a timeout, just in case. */ - wait(); + if (cm_) { + proxies_.clear(); + cm_->stop(); + delete cm_; + cm_ = nullptr; } } -int V4L2CompatManager::init() -{ - start(); - - MutexLocker locker(mutex_); - cv_.wait(locker, [&] { return initialized_; }); - - return 0; -} - -void V4L2CompatManager::run() +int V4L2CompatManager::start() { cm_ = new CameraManager(); @@ -77,7 +68,9 @@ void V4L2CompatManager::run() if (ret) { LOG(V4L2Compat, Error) << "Failed to start camera manager: " << strerror(-ret); - return; + delete cm_; + cm_ = nullptr; + return ret; } LOG(V4L2Compat, Debug) << "Started camera manager"; @@ -93,22 +86,7 @@ void V4L2CompatManager::run() ++index; } - /* - * libcamera has been initialized. Unlock the init() caller as we're - * now ready to handle calls from the framework. - */ - mutex_.lock(); - initialized_ = true; - mutex_.unlock(); - cv_.notify_one(); - - /* Now start processing events and messages. */ - exec(); - - proxies_.clear(); - cm_->stop(); - delete cm_; - cm_ = nullptr; + return 0; } V4L2CompatManager *V4L2CompatManager::instance() @@ -159,8 +137,8 @@ int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mod major(statbuf.st_rdev) != 81) return fd; - if (!isRunning()) - init(); + if (!cm_) + start(); ret = getCameraIndex(fd); if (ret < 0) { diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h index d51b5953d930..872c7c3b10e8 100644 --- a/src/v4l2/v4l2_compat_manager.h +++ b/src/v4l2/v4l2_compat_manager.h @@ -8,22 +8,19 @@ #ifndef __V4L2_COMPAT_MANAGER_H__ #define __V4L2_COMPAT_MANAGER_H__ -#include #include #include #include -#include #include #include #include -#include "thread.h" #include "v4l2_camera_proxy.h" using namespace libcamera; -class V4L2CompatManager : public Thread +class V4L2CompatManager { public: struct FileOperations { @@ -46,8 +43,6 @@ public: static V4L2CompatManager *instance(); - int init(); - V4L2CameraProxy *getProxy(int fd); const FileOperations &fops() const { return fops_; } @@ -64,17 +59,13 @@ private: V4L2CompatManager(); ~V4L2CompatManager(); - void run() override; + int start(); int getCameraIndex(int fd); FileOperations fops_; CameraManager *cm_; - std::mutex mutex_; - std::condition_variable cv_; - bool initialized_; - std::vector> proxies_; std::map devices_; std::map mmaps_; From patchwork Wed Jan 22 20:57:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2729 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 118CE6089D for ; Wed, 22 Jan 2020 21:57:47 +0100 (CET) Received: from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi [81.175.216.236]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id AC165595 for ; Wed, 22 Jan 2020 21:57:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1579726666; bh=peFN2Ay2QuFU4xqB+UxkwIydOLtT275sffzIlsacPjc=; h=From:To:Subject:Date:In-Reply-To:References:From; b=T9DkJffivbCGT25Wk7j31EXVF/igvG5aJMzUUTpUq7ZmmhGhs9WITHiAGctfwkfPx koS+Zbik/HX9r/ea7dHYdMreLZPP2+gBa1Zoyb2ZCU0Ij7Y6/4jTsyOeNMxaUeDopi rwewZ+6gEggfe7l8OhKIWs9AyjofGSD0aO2pPv2Y= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 22 Jan 2020 22:57:23 +0200 Message-Id: <20200122205723.8865-14-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> References: <20200122205723.8865-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 13/13] android: Remove internal thread X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jan 2020 20:57:49 -0000 Now that libcamera creates threads internally and doesn't rely on an application-provided event loop, remove the thread from the Android Camera HAL layer. The CameraProxy class becomes meaningless, remove it and communicate directly from the CameraHalManager to the CameraDevice. Signed-off-by: Laurent Pinchart Acked-by: Jacopo Mondi --- Changes since v1: - Remove unused CameraDevice::cameraDevice_ - Include stddef.h in camera_hal_manager.h to fix compilation of Android's hardware/hardware.h header --- src/android/camera3_hal.cpp | 8 +- src/android/camera_device.cpp | 48 +++++--- src/android/camera_device.h | 16 ++- src/android/camera_hal_manager.cpp | 78 +++++-------- src/android/camera_hal_manager.h | 17 +-- src/android/camera_ops.cpp | 96 +++++++++++++++ src/android/camera_ops.h | 15 +++ src/android/camera_proxy.cpp | 180 ----------------------------- src/android/camera_proxy.h | 42 ------- src/android/meson.build | 2 +- 10 files changed, 188 insertions(+), 314 deletions(-) create mode 100644 src/android/camera_ops.cpp create mode 100644 src/android/camera_ops.h delete mode 100644 src/android/camera_proxy.cpp delete mode 100644 src/android/camera_proxy.h diff --git a/src/android/camera3_hal.cpp b/src/android/camera3_hal.cpp index 8d2629ca356c..d6fc1ecc01bf 100644 --- a/src/android/camera3_hal.cpp +++ b/src/android/camera3_hal.cpp @@ -7,8 +7,8 @@ #include +#include "camera_device.h" #include "camera_hal_manager.h" -#include "camera_proxy.h" #include "log.h" using namespace libcamera; @@ -71,14 +71,14 @@ static int hal_dev_open(const hw_module_t *module, const char *name, LOG(HAL, Debug) << "Open camera " << name; int id = atoi(name); - CameraProxy *proxy = cameraManager.open(id, module); - if (!proxy) { + CameraDevice *camera = cameraManager.open(id, module); + if (!camera) { LOG(HAL, Error) << "Failed to open camera module '" << id << "'"; return -ENODEV; } - *device = &proxy->camera3Device()->common; + *device = &camera->camera3Device()->common; return 0; } diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 67c1d47e67ed..b460d499375b 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -6,6 +6,7 @@ */ #include "camera_device.h" +#include "camera_ops.h" #include "log.h" #include "utils.h" @@ -39,13 +40,13 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() * \class CameraDevice * * The CameraDevice class wraps a libcamera::Camera instance, and implements - * the camera_device_t interface by handling RPC requests received from its - * associated CameraProxy. + * the camera3_device_t interface, bridging calls received from the Android + * camera service to the CameraDevice. * - * It translate parameters and operations from Camera HALv3 API to the libcamera - * ones to provide static information for a Camera, create request templates - * for it, process capture requests and then deliver capture results back - * to the framework using the designated callbacks. + * The class translates parameters and operations from the Camera HALv3 API to + * the libcamera API to provide static information for a Camera, create request + * templates for it, process capture requests and then deliver capture results + * back to the framework using the designated callbacks. */ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera) @@ -63,7 +64,7 @@ CameraDevice::~CameraDevice() delete it.second; } -int CameraDevice::open() +int CameraDevice::open(const hw_module_t *hardwareModule) { int ret = camera_->acquire(); if (ret) { @@ -71,6 +72,19 @@ int CameraDevice::open() return ret; } + /* Initialize the hw_device_t in the instance camera3_module_t. */ + camera3Device_.common.tag = HARDWARE_DEVICE_TAG; + camera3Device_.common.version = CAMERA_DEVICE_API_VERSION_3_3; + camera3Device_.common.module = (hw_module_t *)hardwareModule; + camera3Device_.common.close = hal_dev_close; + + /* + * The camera device operations. These actually implement + * the Android Camera HALv3 interface. + */ + camera3Device_.ops = &hal_dev_ops; + camera3Device_.priv = this; + return 0; } @@ -90,7 +104,7 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks) /* * Return static information for the camera. */ -camera_metadata_t *CameraDevice::getStaticMetadata() +const camera_metadata_t *CameraDevice::getStaticMetadata() { if (staticMetadata_) return staticMetadata_->get(); @@ -675,7 +689,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) return 0; } -void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) { StreamConfiguration *streamConfiguration = &config_->at(0); Stream *stream = streamConfiguration->stream(); @@ -683,7 +697,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque if (camera3Request->num_output_buffers != 1) { LOG(HAL, Error) << "Invalid number of output buffers: " << camera3Request->num_output_buffers; - return; + return -EINVAL; } /* Start the camera if that's the first request we handle. */ @@ -691,7 +705,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque int ret = camera_->start(); if (ret) { LOG(HAL, Error) << "Failed to start camera"; - return; + return ret; } running_ = true; @@ -747,7 +761,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque if (!buffer) { LOG(HAL, Error) << "Failed to create buffer"; delete descriptor; - return; + return -ENOMEM; } Request *request = @@ -757,14 +771,12 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque int ret = camera_->queueRequest(request); if (ret) { LOG(HAL, Error) << "Failed to queue request"; - goto error; + delete request; + delete descriptor; + return ret; } - return; - -error: - delete request; - delete descriptor; + return 0; } void CameraDevice::requestComplete(Request *request) diff --git a/src/android/camera_device.h b/src/android/camera_device.h index caa617dcbfe1..55eac317e0e4 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -13,7 +13,6 @@ #include #include -#include #include #include @@ -21,19 +20,23 @@ class CameraMetadata; -class CameraDevice : public libcamera::Object +class CameraDevice { public: CameraDevice(unsigned int id, const std::shared_ptr &camera); ~CameraDevice(); - int open(); + int open(const hw_module_t *hardwareModule); void close(); + + unsigned int id() const { return id_; } + camera3_device_t *camera3Device() { return &camera3Device_; } + void setCallbacks(const camera3_callback_ops_t *callbacks); - camera_metadata_t *getStaticMetadata(); + const camera_metadata_t *getStaticMetadata(); const camera_metadata_t *constructDefaultRequestSettings(int type); int configureStreams(camera3_stream_configuration_t *stream_list); - void processCaptureRequest(camera3_capture_request_t *request); + int processCaptureRequest(camera3_capture_request_t *request); void requestComplete(libcamera::Request *request); private: @@ -52,6 +55,9 @@ private: std::unique_ptr getResultMetadata(int frame_number, int64_t timestamp); + unsigned int id_; + camera3_device_t camera3Device_; + bool running_; std::shared_ptr camera_; std::unique_ptr config_; diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index 22f0323b3ff0..5bd3bdba8a55 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -12,7 +12,6 @@ #include "log.h" #include "camera_device.h" -#include "camera_proxy.h" using namespace libcamera; @@ -28,92 +27,67 @@ LOG_DECLARE_CATEGORY(HAL); * their static information and to open camera devices. */ -CameraHalManager::~CameraHalManager() +CameraHalManager::CameraHalManager() + : cameraManager_(nullptr) { - if (isRunning()) { - exit(0); - /* \todo Wait with a timeout, just in case. */ - wait(); - } } -int CameraHalManager::init() +CameraHalManager::~CameraHalManager() { - /* - * Start the camera HAL manager thread and wait until its - * initialisation completes to be fully operational before - * receiving calls from the camera stack. - */ - start(); - - MutexLocker locker(mutex_); - cv_.wait(locker); + cameras_.clear(); - return 0; + if (cameraManager_) { + cameraManager_->stop(); + delete cameraManager_; + cameraManager_ = nullptr; + } } -void CameraHalManager::run() +int CameraHalManager::init() { - /* - * All the libcamera components must be initialised here, in - * order to bind them to the camera HAL manager thread that - * executes the event dispatcher. - */ cameraManager_ = new CameraManager(); int ret = cameraManager_->start(); if (ret) { LOG(HAL, Error) << "Failed to start camera manager: " << strerror(-ret); - return; + delete cameraManager_; + cameraManager_ = nullptr; + return ret; } /* - * For each Camera registered in the system, a CameraProxy - * gets created here to wraps a camera device. + * For each Camera registered in the system, a CameraDevice + * gets created here to wraps a libcamera Camera instance. * * \todo Support camera hotplug. */ unsigned int index = 0; - for (auto &camera : cameraManager_->cameras()) { - CameraProxy *proxy = new CameraProxy(index, camera); - proxies_.emplace_back(proxy); + for (auto &cam : cameraManager_->cameras()) { + CameraDevice *camera = new CameraDevice(index, cam); + cameras_.emplace_back(camera); ++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(); - - /* Clean up the resources we have allocated. */ - proxies_.clear(); - - cameraManager_->stop(); - delete cameraManager_; - cameraManager_ = nullptr; + return 0; } -CameraProxy *CameraHalManager::open(unsigned int id, - const hw_module_t *hardwareModule) +CameraDevice *CameraHalManager::open(unsigned int id, + const hw_module_t *hardwareModule) { if (id >= numCameras()) { LOG(HAL, Error) << "Invalid camera id '" << id << "'"; return nullptr; } - CameraProxy *proxy = proxies_[id].get(); - if (proxy->open(hardwareModule)) + CameraDevice *camera = cameras_[id].get(); + if (camera->open(hardwareModule)) return nullptr; LOG(HAL, Info) << "Open camera '" << id << "'"; - return proxy; + return camera; } unsigned int CameraHalManager::numCameras() const @@ -131,14 +105,14 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info) return -EINVAL; } - CameraProxy *proxy = proxies_[id].get(); + CameraDevice *camera = cameras_[id].get(); /* \todo Get these info dynamically inspecting the camera module. */ info->facing = id ? CAMERA_FACING_FRONT : CAMERA_FACING_BACK; info->orientation = 0; info->device_version = 0; info->resource_cost = 0; - info->static_camera_characteristics = proxy->getStaticMetadata(); + info->static_camera_characteristics = camera->getStaticMetadata(); info->conflicting_devices = nullptr; info->conflicting_devices_length = 0; diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h index c23abd1c00af..94d8f005444d 100644 --- a/src/android/camera_hal_manager.h +++ b/src/android/camera_hal_manager.h @@ -7,8 +7,7 @@ #ifndef __ANDROID_CAMERA_MANAGER_H__ #define __ANDROID_CAMERA_MANAGER_H__ -#include -#include +#include #include #include @@ -16,33 +15,27 @@ #include -#include "thread.h" - class CameraDevice; -class CameraProxy; -class CameraHalManager : public libcamera::Thread +class CameraHalManager { public: + CameraHalManager(); ~CameraHalManager(); int init(); - CameraProxy *open(unsigned int id, const hw_module_t *module); + CameraDevice *open(unsigned int id, const hw_module_t *module); unsigned int numCameras() const; int getCameraInfo(unsigned int id, struct camera_info *info); private: - void run() override; camera_metadata_t *getStaticMetadata(unsigned int id); libcamera::CameraManager *cameraManager_; - std::mutex mutex_; - std::condition_variable cv_; - - std::vector> proxies_; + std::vector> cameras_; }; #endif /* __ANDROID_CAMERA_MANAGER_H__ */ diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp new file mode 100644 index 000000000000..9dfc2e655e83 --- /dev/null +++ b/src/android/camera_ops.cpp @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * camera_ops.h - Android Camera HAL Operations + */ + +#include "camera_ops.h" + +#include + +#include "camera_device.h" + +using namespace libcamera; + +/* + * Translatation layer between the Android Camera HAL device operations and the + * CameraDevice. + */ + +static int hal_dev_initialize(const struct camera3_device *dev, + const camera3_callback_ops_t *callback_ops) +{ + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast(dev->priv); + camera->setCallbacks(callback_ops); + + return 0; +} + +static int hal_dev_configure_streams(const struct camera3_device *dev, + camera3_stream_configuration_t *stream_list) +{ + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast(dev->priv); + return camera->configureStreams(stream_list); +} + +static const camera_metadata_t * +hal_dev_construct_default_request_settings(const struct camera3_device *dev, + int type) +{ + if (!dev) + return nullptr; + + CameraDevice *camera = reinterpret_cast(dev->priv); + return camera->constructDefaultRequestSettings(type); +} + +static int hal_dev_process_capture_request(const struct camera3_device *dev, + camera3_capture_request_t *request) +{ + if (!dev) + return -EINVAL; + + CameraDevice *camera = reinterpret_cast(dev->priv); + return camera->processCaptureRequest(request); +} + +static void hal_dev_dump(const struct camera3_device *dev, int fd) +{ +} + +static int hal_dev_flush(const struct camera3_device *dev) +{ + return 0; +} + +int hal_dev_close(hw_device_t *hw_device) +{ + if (!hw_device) + return -EINVAL; + + camera3_device_t *dev = reinterpret_cast(hw_device); + CameraDevice *camera = reinterpret_cast(dev->priv); + + camera->close(); + + return 0; +} + +camera3_device_ops hal_dev_ops = { + .initialize = hal_dev_initialize, + .configure_streams = hal_dev_configure_streams, + .register_stream_buffers = nullptr, + .construct_default_request_settings = hal_dev_construct_default_request_settings, + .process_capture_request = hal_dev_process_capture_request, + .get_metadata_vendor_tag_ops = nullptr, + .dump = hal_dev_dump, + .flush = hal_dev_flush, + .reserved = { nullptr }, +}; diff --git a/src/android/camera_ops.h b/src/android/camera_ops.h new file mode 100644 index 000000000000..304e7b856e81 --- /dev/null +++ b/src/android/camera_ops.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * camera_ops.h - Android Camera HAL Operations + */ +#ifndef __ANDROID_CAMERA_OPS_H__ +#define __ANDROID_CAMERA_OPS_H__ + +#include + +int hal_dev_close(hw_device_t *hw_device); +extern camera3_device_ops hal_dev_ops; + +#endif /* __ANDROID_CAMERA_OPS_H__ */ diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp deleted file mode 100644 index 3964b5665e2f..000000000000 --- a/src/android/camera_proxy.cpp +++ /dev/null @@ -1,180 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Copyright (C) 2019, Google Inc. - * - * camera_proxy.cpp - Proxy to camera devices - */ - -#include "camera_proxy.h" - -#include - -#include - -#include "log.h" -#include "message.h" -#include "utils.h" - -#include "camera_device.h" - -using namespace libcamera; - -LOG_DECLARE_CATEGORY(HAL); - -/* - * \class CameraProxy - * - * The CameraProxy wraps a CameraDevice and implements the camera3_device_t - * API, bridging calls received from the camera framework to the CameraDevice. - * - * Bridging operation calls between the framework and the CameraDevice is - * required as the two run in two different threads and certain operations, - * such as queueing a new capture request to the camera, shall be called in - * the thread that dispatches events. Other operations do not require any - * bridging and resolve to direct function calls on the CameraDevice instance - * instead. - */ - -static int hal_dev_initialize(const struct camera3_device *dev, - const camera3_callback_ops_t *callback_ops) -{ - if (!dev) - return -EINVAL; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - proxy->initialize(callback_ops); - - return 0; -} - -static int hal_dev_configure_streams(const struct camera3_device *dev, - camera3_stream_configuration_t *stream_list) -{ - if (!dev) - return -EINVAL; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - return proxy->configureStreams(stream_list); -} - -static const camera_metadata_t * -hal_dev_construct_default_request_settings(const struct camera3_device *dev, - int type) -{ - if (!dev) - return nullptr; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - return proxy->constructDefaultRequestSettings(type); -} - -static int hal_dev_process_capture_request(const struct camera3_device *dev, - camera3_capture_request_t *request) -{ - if (!dev) - return -EINVAL; - - CameraProxy *proxy = reinterpret_cast(dev->priv); - return proxy->processCaptureRequest(request); -} - -static void hal_dev_dump(const struct camera3_device *dev, int fd) -{ -} - -static int hal_dev_flush(const struct camera3_device *dev) -{ - return 0; -} - -static int hal_dev_close(hw_device_t *hw_device) -{ - if (!hw_device) - return -EINVAL; - - camera3_device_t *dev = reinterpret_cast(hw_device); - CameraProxy *proxy = reinterpret_cast(dev->priv); - - proxy->close(); - - return 0; -} - -static camera3_device_ops hal_dev_ops = { - .initialize = hal_dev_initialize, - .configure_streams = hal_dev_configure_streams, - .register_stream_buffers = nullptr, - .construct_default_request_settings = hal_dev_construct_default_request_settings, - .process_capture_request = hal_dev_process_capture_request, - .get_metadata_vendor_tag_ops = nullptr, - .dump = hal_dev_dump, - .flush = hal_dev_flush, - .reserved = { nullptr }, -}; - -CameraProxy::CameraProxy(unsigned int id, const std::shared_ptr &camera) - : id_(id) -{ - cameraDevice_ = new CameraDevice(id, camera); -} - -CameraProxy::~CameraProxy() -{ - delete cameraDevice_; -} - -int CameraProxy::open(const hw_module_t *hardwareModule) -{ - int ret = cameraDevice_->open(); - if (ret) - return ret; - - /* Initialize the hw_device_t in the instance camera3_module_t. */ - camera3Device_.common.tag = HARDWARE_DEVICE_TAG; - camera3Device_.common.version = CAMERA_DEVICE_API_VERSION_3_3; - camera3Device_.common.module = (hw_module_t *)hardwareModule; - camera3Device_.common.close = hal_dev_close; - - /* - * The camera device operations. These actually implement - * the Android Camera HALv3 interface. - */ - camera3Device_.ops = &hal_dev_ops; - camera3Device_.priv = this; - - return 0; -} - -void CameraProxy::close() -{ - cameraDevice_->invokeMethod(&CameraDevice::close, - ConnectionTypeBlocking); -} - -void CameraProxy::initialize(const camera3_callback_ops_t *callbacks) -{ - cameraDevice_->setCallbacks(callbacks); -} - -const camera_metadata_t *CameraProxy::getStaticMetadata() -{ - return cameraDevice_->getStaticMetadata(); -} - -const camera_metadata_t *CameraProxy::constructDefaultRequestSettings(int type) -{ - return cameraDevice_->constructDefaultRequestSettings(type); -} - -int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list) -{ - return cameraDevice_->configureStreams(stream_list); -} - -int CameraProxy::processCaptureRequest(camera3_capture_request_t *request) -{ - cameraDevice_->invokeMethod(&CameraDevice::processCaptureRequest, - ConnectionTypeBlocking, request); - - return 0; -} diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h deleted file mode 100644 index e8cfbc9dd526..000000000000 --- a/src/android/camera_proxy.h +++ /dev/null @@ -1,42 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Copyright (C) 2019, Google Inc. - * - * camera_proxy.h - Proxy to camera devices - */ -#ifndef __ANDROID_CAMERA_PROXY_H__ -#define __ANDROID_CAMERA_PROXY_H__ - -#include - -#include - -#include - -class CameraDevice; - -class CameraProxy -{ -public: - CameraProxy(unsigned int id, const std::shared_ptr &camera); - ~CameraProxy(); - - int open(const hw_module_t *hardwareModule); - void close(); - - void initialize(const camera3_callback_ops_t *callbacks); - const camera_metadata_t *getStaticMetadata(); - const camera_metadata_t *constructDefaultRequestSettings(int type); - int configureStreams(camera3_stream_configuration_t *stream_list); - int processCaptureRequest(camera3_capture_request_t *request); - - unsigned int id() const { return id_; } - camera3_device_t *camera3Device() { return &camera3Device_; } - -private: - unsigned int id_; - CameraDevice *cameraDevice_; - camera3_device_t camera3Device_; -}; - -#endif /* __ANDROID_CAMERA_PROXY_H__ */ diff --git a/src/android/meson.build b/src/android/meson.build index 70dfcc1df27a..5a5a332e6a6f 100644 --- a/src/android/meson.build +++ b/src/android/meson.build @@ -3,7 +3,7 @@ android_hal_sources = files([ 'camera_hal_manager.cpp', 'camera_device.cpp', 'camera_metadata.cpp', - 'camera_proxy.cpp', + 'camera_ops.cpp', ]) android_camera_metadata_sources = files([