[v3,2/3] camera: Use invokeMethod() for pipe_->acquire() and pipe_->release()
diff mbox series

Message ID 20240830111207.46455-3-hdegoede@redhat.com
State Accepted
Headers show
Series
  • Fix uvcvideo pipelinehandler keeping /dev/video# open
Related show

Commit Message

Hans de Goede Aug. 30, 2024, 11:12 a.m. UTC 
The uvcvideo driver needs to open / close its /dev/video# node from
pipe_->acquireDevices() / pipe_->releaseDevices().

V4L2VideoDevice::open() creates an EventNotifier and this notifier needs
to be created from the CameraManager thread.

Use invokeMethod() for pipe_->acquire() and pipe_->release() so that
the EventNotifiers are created from the CameraManager thread context.

Running pipe_->acquire() and pipe_->release() from the CameraManager
thread context serializes all calls to them. Drop PipelineHandler::lock_
this now is no longer necessary and update the "\context" part of
the documentation for acquire[Device]() and release[Device]() to match.

Note the delayed opening of /dev/video# is a special case because
the kernel uvcvideo driver powers on the USB device as soon as /dev/video#
is opened. This behavior should *not* be copied by other pipeline handlers.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add a note to the commit message that opening/closing /dev/video# from
  acquire()/release() as the uvcvideo pipeline handler does is an exception
  and that this behavior should not be copied by other pipeline handlers
- Drop lock_, update "\context" in doxygen docs
---
 include/libcamera/internal/pipeline_handler.h |  5 +----
 src/libcamera/camera.cpp                      |  6 ++++--
 src/libcamera/pipeline_handler.cpp            | 12 ++++++------
 3 files changed, 11 insertions(+), 12 deletions(-)

Patch
diff mbox series 

diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
index 597f7c3e..c33cf715 100644
--- a/include/libcamera/internal/pipeline_handler.h
+++ b/include/libcamera/internal/pipeline_handler.h
@@ -14,7 +14,6 @@ 
 #include <sys/types.h>
 #include <vector>
 
-#include <libcamera/base/mutex.h>
 #include <libcamera/base/object.h>
 
 #include <libcamera/controls.h>
@@ -99,9 +98,7 @@  private:
 	std::queue<Request *> waitingRequests_;
 
 	const char *name_;
-
-	Mutex lock_;
-	unsigned int useCount_ LIBCAMERA_TSA_GUARDED_BY(lock_);
+	unsigned int useCount_;
 
 	friend class PipelineHandlerFactoryBase;
 };
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 4e393f89..61925e83 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -995,7 +995,8 @@  int Camera::acquire()
 	if (ret < 0)
 		return ret == -EACCES ? -EBUSY : ret;
 
-	if (!d->pipe_->acquire(this)) {
+	if (!d->pipe_->invokeMethod(&PipelineHandler::acquire,
+				    ConnectionTypeBlocking, this)) {
 		LOG(Camera, Info)
 			<< "Pipeline handler in use by another process";
 		return -EBUSY;
@@ -1030,7 +1031,8 @@  int Camera::release()
 		return ret == -EACCES ? -EBUSY : ret;
 
 	if (d->isAcquired())
-		d->pipe_->release(this);
+		d->pipe_->invokeMethod(&PipelineHandler::release,
+				       ConnectionTypeBlocking, this);
 
 	d->setState(Private::CameraAvailable);
 
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index 861815cb..b18b6d0b 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -157,7 +157,7 @@  MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
  * Pipeline handlers shall not call this function directly as the Camera class
  * handles access internally.
  *
- * \context This function is \threadsafe.
+ * \context This function is called from the CameraManager thread.
  *
  * \return True if the pipeline handler was acquired, false if another process
  * has already acquired it
@@ -165,8 +165,6 @@  MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
  */
 bool PipelineHandler::acquire(Camera *camera)
 {
-	MutexLocker locker(lock_);
-
 	if (useCount_ == 0) {
 		for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
 			if (!media->lock()) {
@@ -199,14 +197,12 @@  bool PipelineHandler::acquire(Camera *camera)
  * Pipeline handlers shall not call this function directly as the Camera class
  * handles access internally.
  *
- * \context This function is \threadsafe.
+ * \context This function is called from the CameraManager thread.
  *
  * \sa acquire()
  */
 void PipelineHandler::release(Camera *camera)
 {
-	MutexLocker locker(lock_);
-
 	ASSERT(useCount_);
 
 	releaseDevice(camera);
@@ -230,6 +226,8 @@  void PipelineHandler::release(Camera *camera)
  * powers on the USB device as soon as /dev/video# is opened. This behavior
  * should *not* be copied by other pipeline handlers.
  *
+ * \context This function is called from the CameraManager thread.
+ *
  * \return True on success, false on failure
  * \sa releaseDevice()
  */
@@ -250,6 +248,8 @@  bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)
  * release them until releaseDevice() has been called for all previously
  * acquired cameras.
  *
+ * \context This function is called from the CameraManager thread.
+ *
  * \sa acquireDevice()
  */
 void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)