| Message ID | 20211223023331.13505-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart (2021-12-23 02:33:31) > The PipelineHandler lock() and unlock() functions are documented as > thread-safe, but they're not. Fix them using a mutex. A lock for a lock ... Seems fine though, but a comment below Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/pipeline_handler.h | 2 ++ > src/libcamera/pipeline_handler.cpp | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index a7331cedfb16..3768bb8f6cb6 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -14,6 +14,7 @@ > #include <sys/types.h> > #include <vector> > > +#include <libcamera/base/mutex.h> > #include <libcamera/base/object.h> > > #include <libcamera/controls.h> > @@ -88,6 +89,7 @@ private: > > const char *name_; > > + Mutex lock_; > bool lockOwner_; Do we need to comment these at all? lock_ and lockOwner_ while closely linked, could be confusing. The lockOwner_ afterall does /not/ have any correspondance to the owner of lock_. A comment above each would probably suffice. > > friend class PipelineHandlerFactory; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index b2ee4ec0ce87..07d9d387daff 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -11,6 +11,7 @@ > #include <sys/sysmacros.h> > > #include <libcamera/base/log.h> > +#include <libcamera/base/mutex.h> > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, > */ > bool PipelineHandler::lock() > { > + MutexLocker locker(lock_); > + > /* Do not allow nested locking in the same libcamera instance. */ > if (lockOwner_) > return false; > @@ -183,6 +186,8 @@ bool PipelineHandler::lock() > */ > void PipelineHandler::unlock() > { > + MutexLocker locker(lock_); > + > if (lockOwner_) > return; > > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Thu, Dec 23, 2021 at 01:35:46PM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart (2021-12-23 02:33:31) > > The PipelineHandler lock() and unlock() functions are documented as > > thread-safe, but they're not. Fix them using a mutex. > > A lock for a lock ... > > Seems fine though, but a comment below > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/pipeline_handler.h | 2 ++ > > src/libcamera/pipeline_handler.cpp | 5 +++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index a7331cedfb16..3768bb8f6cb6 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -14,6 +14,7 @@ > > #include <sys/types.h> > > #include <vector> > > > > +#include <libcamera/base/mutex.h> > > #include <libcamera/base/object.h> > > > > #include <libcamera/controls.h> > > @@ -88,6 +89,7 @@ private: > > > > const char *name_; > > > > + Mutex lock_; > > bool lockOwner_; > > Do we need to comment these at all? lock_ and lockOwner_ while closely > linked, could be confusing. > > The lockOwner_ afterall does /not/ have any correspondance to the owner > of lock_. > > A comment above each would probably suffice. I'll add a comment to lockOwner_ and annotate it with LIBCAMERA_TSA_GUARDED_BY(lock_). > > > > friend class PipelineHandlerFactory; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index b2ee4ec0ce87..07d9d387daff 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -11,6 +11,7 @@ > > #include <sys/sysmacros.h> > > > > #include <libcamera/base/log.h> > > +#include <libcamera/base/mutex.h> > > #include <libcamera/base/utils.h> > > > > #include <libcamera/camera.h> > > @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, > > */ > > bool PipelineHandler::lock() > > { > > + MutexLocker locker(lock_); > > + > > /* Do not allow nested locking in the same libcamera instance. */ > > if (lockOwner_) > > return false; > > @@ -183,6 +186,8 @@ bool PipelineHandler::lock() > > */ > > void PipelineHandler::unlock() > > { > > + MutexLocker locker(lock_); > > + > > if (lockOwner_) > > return; > >
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index a7331cedfb16..3768bb8f6cb6 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -14,6 +14,7 @@ #include <sys/types.h> #include <vector> +#include <libcamera/base/mutex.h> #include <libcamera/base/object.h> #include <libcamera/controls.h> @@ -88,6 +89,7 @@ private: const char *name_; + Mutex lock_; bool lockOwner_; friend class PipelineHandlerFactory; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index b2ee4ec0ce87..07d9d387daff 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -11,6 +11,7 @@ #include <sys/sysmacros.h> #include <libcamera/base/log.h> +#include <libcamera/base/mutex.h> #include <libcamera/base/utils.h> #include <libcamera/camera.h> @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator, */ bool PipelineHandler::lock() { + MutexLocker locker(lock_); + /* Do not allow nested locking in the same libcamera instance. */ if (lockOwner_) return false; @@ -183,6 +186,8 @@ bool PipelineHandler::lock() */ void PipelineHandler::unlock() { + MutexLocker locker(lock_); + if (lockOwner_) return;
The PipelineHandler lock() and unlock() functions are documented as thread-safe, but they're not. Fix them using a mutex. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/pipeline_handler.h | 2 ++ src/libcamera/pipeline_handler.cpp | 5 +++++ 2 files changed, 7 insertions(+)