Message ID | 20211223023331.13505-2-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Quoting Laurent Pinchart (2021-12-23 02:33:30) > The MediaDevice lock is meant to prevent concurrent usage of multiple > cameras from the same pipeline handlers. As media devices are acquired > by pipeline handlers, we can't have multiple pipeline handlers trying to > lock the same media device. The recursive locking detection can thus be > moved to the pipeline handler. This simplifies the media device > implementation that now implements true lock semantics, and prepares for > support of concurrent camera usage. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/media_device.h | 1 - > include/libcamera/internal/pipeline_handler.h | 2 ++ > src/libcamera/media_device.cpp | 14 +------------- > src/libcamera/pipeline_handler.cpp | 13 ++++++++++++- > 4 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h > index af0b25b731eb..6e2a63f38229 100644 > --- a/include/libcamera/internal/media_device.h > +++ b/include/libcamera/internal/media_device.h > @@ -86,7 +86,6 @@ private: > UniqueFD fd_; > bool valid_; > bool acquired_; > - bool lockOwner_; > > std::map<unsigned int, MediaObject *> objects_; > std::vector<MediaEntity *> entities_; > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index ec986a518b5c..a7331cedfb16 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -88,6 +88,8 @@ private: > > const char *name_; > > + bool lockOwner_; > + > friend class PipelineHandlerFactory; > }; > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index 0b7940182d0c..941f86c25f66 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice) > * populate() before the media graph can be queried. > */ > MediaDevice::MediaDevice(const std::string &deviceNode) > - : deviceNode_(deviceNode), valid_(false), acquired_(false), > - lockOwner_(false) > + : deviceNode_(deviceNode), valid_(false), acquired_(false) > { > } > > @@ -145,15 +144,9 @@ bool MediaDevice::lock() > if (!fd_.isValid()) > return false; > > - /* Do not allow nested locking in the same libcamera instance. */ > - if (lockOwner_) > - return false; > - > if (lockf(fd_.get(), F_TLOCK, 0)) > return false; > > - lockOwner_ = true; > - > return true; > } > > @@ -171,11 +164,6 @@ void MediaDevice::unlock() > if (!fd_.isValid()) > return; > > - if (!lockOwner_) > - return; > - > - lockOwner_ = false; > - > lockf(fd_.get(), F_ULOCK, 0); > } > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 0bc0e341b9e7..b2ee4ec0ce87 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline) > * respective factories. > */ > PipelineHandler::PipelineHandler(CameraManager *manager) > - : manager_(manager) > + : manager_(manager), lockOwner_(false) > { > } > > @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, > */ > bool PipelineHandler::lock() > { > + /* Do not allow nested locking in the same libcamera instance. */ > + if (lockOwner_) > + return false; > + > for (std::shared_ptr<MediaDevice> &media : mediaDevices_) { > if (!media->lock()) { > unlock(); > @@ -162,6 +166,8 @@ bool PipelineHandler::lock() > } > } > > + lockOwner_ = true; > + > return true; > } > > @@ -177,8 +183,13 @@ bool PipelineHandler::lock() > */ > void PipelineHandler::unlock() > { > + if (lockOwner_) if (!lockOwner_) ? > + return; > + > for (std::shared_ptr<MediaDevice> &media : mediaDevices_) > media->unlock(); > + > + lockOwner_ = false; > } > > /** > -- > Regards, > > Laurent Pinchart >
On Thu, Dec 23, 2021 at 01:31:57PM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart (2021-12-23 02:33:30) > > The MediaDevice lock is meant to prevent concurrent usage of multiple > > cameras from the same pipeline handlers. As media devices are acquired > > by pipeline handlers, we can't have multiple pipeline handlers trying to > > lock the same media device. The recursive locking detection can thus be > > moved to the pipeline handler. This simplifies the media device > > implementation that now implements true lock semantics, and prepares for > > support of concurrent camera usage. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/media_device.h | 1 - > > include/libcamera/internal/pipeline_handler.h | 2 ++ > > src/libcamera/media_device.cpp | 14 +------------- > > src/libcamera/pipeline_handler.cpp | 13 ++++++++++++- > > 4 files changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h > > index af0b25b731eb..6e2a63f38229 100644 > > --- a/include/libcamera/internal/media_device.h > > +++ b/include/libcamera/internal/media_device.h > > @@ -86,7 +86,6 @@ private: > > UniqueFD fd_; > > bool valid_; > > bool acquired_; > > - bool lockOwner_; > > > > std::map<unsigned int, MediaObject *> objects_; > > std::vector<MediaEntity *> entities_; > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index ec986a518b5c..a7331cedfb16 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -88,6 +88,8 @@ private: > > > > const char *name_; > > > > + bool lockOwner_; > > + > > friend class PipelineHandlerFactory; > > }; > > > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > > index 0b7940182d0c..941f86c25f66 100644 > > --- a/src/libcamera/media_device.cpp > > +++ b/src/libcamera/media_device.cpp > > @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice) > > * populate() before the media graph can be queried. > > */ > > MediaDevice::MediaDevice(const std::string &deviceNode) > > - : deviceNode_(deviceNode), valid_(false), acquired_(false), > > - lockOwner_(false) > > + : deviceNode_(deviceNode), valid_(false), acquired_(false) > > { > > } > > > > @@ -145,15 +144,9 @@ bool MediaDevice::lock() > > if (!fd_.isValid()) > > return false; > > > > - /* Do not allow nested locking in the same libcamera instance. */ > > - if (lockOwner_) > > - return false; > > - > > if (lockf(fd_.get(), F_TLOCK, 0)) > > return false; > > > > - lockOwner_ = true; > > - > > return true; > > } > > > > @@ -171,11 +164,6 @@ void MediaDevice::unlock() > > if (!fd_.isValid()) > > return; > > > > - if (!lockOwner_) > > - return; > > - > > - lockOwner_ = false; > > - > > lockf(fd_.get(), F_ULOCK, 0); > > } > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 0bc0e341b9e7..b2ee4ec0ce87 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline) > > * respective factories. > > */ > > PipelineHandler::PipelineHandler(CameraManager *manager) > > - : manager_(manager) > > + : manager_(manager), lockOwner_(false) > > { > > } > > > > @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, > > */ > > bool PipelineHandler::lock() > > { > > + /* Do not allow nested locking in the same libcamera instance. */ > > + if (lockOwner_) > > + return false; > > + > > for (std::shared_ptr<MediaDevice> &media : mediaDevices_) { > > if (!media->lock()) { > > unlock(); > > @@ -162,6 +166,8 @@ bool PipelineHandler::lock() > > } > > } > > > > + lockOwner_ = true; > > + > > return true; > > } > > > > @@ -177,8 +183,13 @@ bool PipelineHandler::lock() > > */ > > void PipelineHandler::unlock() > > { > > + if (lockOwner_) > > if (!lockOwner_) ? Oops, indeed. I'll fix it in v2. > > + return; > > + > > for (std::shared_ptr<MediaDevice> &media : mediaDevices_) > > media->unlock(); > > + > > + lockOwner_ = false; > > } > > > > /**
diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h index af0b25b731eb..6e2a63f38229 100644 --- a/include/libcamera/internal/media_device.h +++ b/include/libcamera/internal/media_device.h @@ -86,7 +86,6 @@ private: UniqueFD fd_; bool valid_; bool acquired_; - bool lockOwner_; std::map<unsigned int, MediaObject *> objects_; std::vector<MediaEntity *> entities_; diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index ec986a518b5c..a7331cedfb16 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -88,6 +88,8 @@ private: const char *name_; + bool lockOwner_; + friend class PipelineHandlerFactory; }; diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index 0b7940182d0c..941f86c25f66 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice) * populate() before the media graph can be queried. */ MediaDevice::MediaDevice(const std::string &deviceNode) - : deviceNode_(deviceNode), valid_(false), acquired_(false), - lockOwner_(false) + : deviceNode_(deviceNode), valid_(false), acquired_(false) { } @@ -145,15 +144,9 @@ bool MediaDevice::lock() if (!fd_.isValid()) return false; - /* Do not allow nested locking in the same libcamera instance. */ - if (lockOwner_) - return false; - if (lockf(fd_.get(), F_TLOCK, 0)) return false; - lockOwner_ = true; - return true; } @@ -171,11 +164,6 @@ void MediaDevice::unlock() if (!fd_.isValid()) return; - if (!lockOwner_) - return; - - lockOwner_ = false; - lockf(fd_.get(), F_ULOCK, 0); } diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 0bc0e341b9e7..b2ee4ec0ce87 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline) * respective factories. */ PipelineHandler::PipelineHandler(CameraManager *manager) - : manager_(manager) + : manager_(manager), lockOwner_(false) { } @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, */ bool PipelineHandler::lock() { + /* Do not allow nested locking in the same libcamera instance. */ + if (lockOwner_) + return false; + for (std::shared_ptr<MediaDevice> &media : mediaDevices_) { if (!media->lock()) { unlock(); @@ -162,6 +166,8 @@ bool PipelineHandler::lock() } } + lockOwner_ = true; + return true; } @@ -177,8 +183,13 @@ bool PipelineHandler::lock() */ void PipelineHandler::unlock() { + if (lockOwner_) + return; + for (std::shared_ptr<MediaDevice> &media : mediaDevices_) media->unlock(); + + lockOwner_ = false; } /**
The MediaDevice lock is meant to prevent concurrent usage of multiple cameras from the same pipeline handlers. As media devices are acquired by pipeline handlers, we can't have multiple pipeline handlers trying to lock the same media device. The recursive locking detection can thus be moved to the pipeline handler. This simplifies the media device implementation that now implements true lock semantics, and prepares for support of concurrent camera usage. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/media_device.h | 1 - include/libcamera/internal/pipeline_handler.h | 2 ++ src/libcamera/media_device.cpp | 14 +------------- src/libcamera/pipeline_handler.cpp | 13 ++++++++++++- 4 files changed, 15 insertions(+), 15 deletions(-)