Message ID | 20211226231255.18653-3-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Commit | 152adad97a95c15ae9ac2e1889f94a3dc6412165 |
Headers | show |
Series |
|
Related | show |
Hi Laurent, On Mon, Dec 27, 2021 at 01:12:55AM +0200, Laurent Pinchart wrote: > 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> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Changes since v1: > > - Add thread safety annotation > - Comment that lockOwner_ does not indicate ownership of lock_ > --- > include/libcamera/internal/pipeline_handler.h | 4 +++- > src/libcamera/pipeline_handler.cpp | 5 +++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index a7331cedfb16..e5b8ffb4db3d 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,7 +89,8 @@ private: > > const char *name_; > > - bool lockOwner_; > + Mutex lock_; > + bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */ Took me a while to find out what the comment means.. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > friend class PipelineHandlerFactory; > }; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index a9241ac62979..03e4b9e66aa6 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 Jacopo, On Mon, Dec 27, 2021 at 09:56:46AM +0100, Jacopo Mondi wrote: > On Mon, Dec 27, 2021 at 01:12:55AM +0200, Laurent Pinchart wrote: > > 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> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > Changes since v1: > > > > - Add thread safety annotation > > - Comment that lockOwner_ does not indicate ownership of lock_ > > --- > > include/libcamera/internal/pipeline_handler.h | 4 +++- > > src/libcamera/pipeline_handler.cpp | 5 +++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index a7331cedfb16..e5b8ffb4db3d 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,7 +89,8 @@ private: > > > > const char *name_; > > > > - bool lockOwner_; > > + Mutex lock_; > > + bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */ > > Took me a while to find out what the comment means.. I'm sure it could be improved, but as this will be reworked soon, I decided not to spend too much time on it. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > friend class PipelineHandlerFactory; > > }; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index a9241ac62979..03e4b9e66aa6 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; > >
Hi, Thank you for the patch On 12/27/21 4:42 AM, Laurent Pinchart wrote: > 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> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Changes since v1: > > - Add thread safety annotation > - Comment that lockOwner_ does not indicate ownership of lock_ > --- > include/libcamera/internal/pipeline_handler.h | 4 +++- > src/libcamera/pipeline_handler.cpp | 5 +++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index a7331cedfb16..e5b8ffb4db3d 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,7 +89,8 @@ private: > > const char *name_; > > - bool lockOwner_; > + Mutex lock_; > + bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */ > > friend class PipelineHandlerFactory; > }; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index a9241ac62979..03e4b9e66aa6 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..e5b8ffb4db3d 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,7 +89,8 @@ private: const char *name_; - bool lockOwner_; + Mutex lock_; + bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */ friend class PipelineHandlerFactory; }; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index a9241ac62979..03e4b9e66aa6 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;