| Message ID | 20211226231255.18653-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | ec6921d7f7ef70cd7c5ea3d7a20082d66b8aa6e5 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Dec 27, 2021 at 01:12:54AM +0200, Laurent Pinchart wrote: > 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. Out of curiosity: what do we talk about recursive locking ? Where is the recursive part ? I though this is just about preventing to lock the same media device twice. How do you envision per-camera locking going forward ? Currently Camera::acquire() locks the pipeline handlers, if the locking should be moved to be per-camera, shouldn't the whole pipeline handler locking mechanism be reworked ? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Fix lock owner check un PipelineHandler::unlock() > --- > 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..a9241ac62979 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; > } > > /** > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Dec 27, 2021 at 10:03:43AM +0100, Jacopo Mondi wrote: > On Mon, Dec 27, 2021 at 01:12:54AM +0200, Laurent Pinchart wrote: > > 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. > > Out of curiosity: what do we talk about recursive locking ? > Where is the recursive part ? I though this is just about preventing > to lock the same media device twice. It's recursive in the sense that the lock is taken again (with a call to PipelineHandler::lock()) for a second camera while already held for a first camera. That corresponds to the definition of std::recursive_mutex, but it's not recursive in the sense of calling lock() from within a lock() call. > How do you envision per-camera locking going forward ? Currently > Camera::acquire() locks the pipeline handlers, if the locking should > be moved to be per-camera, shouldn't the whole pipeline handler > locking mechanism be reworked ? Yes, it will need to be reworked. This patch only prepares for that by giving true lock semantics to MediaDevice::lock(). The rest will need to be modified, with an API for pipeline handlers to expose mutual exclusion constraints between different cameras to applications. I'm not sure yet how that will look like. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Fix lock owner check un PipelineHandler::unlock() > > --- > > 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..a9241ac62979 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; > > } > > > > /**
Quoting Laurent Pinchart (2021-12-26 23:12:54) > 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> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Changes since v1: > > - Fix lock owner check un PipelineHandler::unlock() > --- > 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..a9241ac62979 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; > } > > /** > -- > Regards, > > Laurent Pinchart >
Hi Laurent, Thank you for the patch On 12/27/21 4:42 AM, Laurent Pinchart wrote: > 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> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Changes since v1: > > - Fix lock owner check un PipelineHandler::unlock() > --- > 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..a9241ac62979 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; > } > > /**
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..a9241ac62979 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> --- Changes since v1: - Fix lock owner check un PipelineHandler::unlock() --- 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(-)