| Message ID | 20210510105235.28319-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote: > The CameraDevice class uses std::scoped_lock<> to guard access to the > class' descriptors_ member. > > std::scoped_lock<> provides a set of features that guarantees safety > when locking multiple mutexes in a critical section, while for single > locks happening in a scoped block it does not provides benefits compared > to the simplest std::unique_lock<> which libcamera provides the > MutexLocker type for. > > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make > the implementation consistent with the rest of the code base. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/android/camera_device.cpp | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index bdb5c8ed52aa..ff965a1c8c86 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -22,6 +22,7 @@ > > #include "libcamera/internal/formats.h" > #include "libcamera/internal/log.h" > +#include "libcamera/internal/thread.h" > #include "libcamera/internal/utils.h" > > #include "system/graphics.h" > @@ -2016,7 +2017,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > worker_.queueRequest(descriptor.request_.get()); > > { > - std::scoped_lock<std::mutex> lock(mutex_); > + MutexLocker lock(mutex_); > descriptors_[descriptor.request_->cookie()] = std::move(descriptor); > } > > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request *request) > { > decltype(descriptors_)::node_type node; > { > - std::scoped_lock<std::mutex> lock(mutex_); > + MutexLocker lock(mutex_); > auto it = descriptors_.find(request->cookie()); > if (it == descriptors_.end()) { > /* > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, thank you for the patch. On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund < niklas.soderlund@ragnatech.se> wrote: > Hi Jacopo, > > Thanks for your patch. > > On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote: > > The CameraDevice class uses std::scoped_lock<> to guard access to the > > class' descriptors_ member. > > > > std::scoped_lock<> provides a set of features that guarantees safety > > when locking multiple mutexes in a critical section, while for single > > locks happening in a scoped block it does not provides benefits compared > > to the simplest std::unique_lock<> which libcamera provides the > > MutexLocker type for. > > > > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make > > the implementation consistent with the rest of the code base. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > I hear since unique_lock has more functionalities than scoped_lock, the performance of unique_lock is better than scoped_lock. So I would love to use scoped_lock if it is only used like std::lock_guard. On the other hand, unique_lock is needed for std::condition_variable. IMO, we should subtilize std::unique_lock and std::scoped_lock. But ideally, we should declare our own mutex class to utilize clang thread annotation. https://bugs.libcamera.org/show_bug.cgi?id=23 I am not strongly against this patch though as unique_lock covers scoped_lock and the performance difference should be fraction. -Hiro Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > src/android/camera_device.cpp | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp > b/src/android/camera_device.cpp > > index bdb5c8ed52aa..ff965a1c8c86 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -22,6 +22,7 @@ > > > > #include "libcamera/internal/formats.h" > > #include "libcamera/internal/log.h" > > +#include "libcamera/internal/thread.h" > > #include "libcamera/internal/utils.h" > > > > #include "system/graphics.h" > > @@ -2016,7 +2017,7 @@ int > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > worker_.queueRequest(descriptor.request_.get()); > > > > { > > - std::scoped_lock<std::mutex> lock(mutex_); > > + MutexLocker lock(mutex_); > > descriptors_[descriptor.request_->cookie()] = > std::move(descriptor); > > } > > > > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request > *request) > > { > > decltype(descriptors_)::node_type node; > > { > > - std::scoped_lock<std::mutex> lock(mutex_); > > + MutexLocker lock(mutex_); > > auto it = descriptors_.find(request->cookie()); > > if (it == descriptors_.end()) { > > /* > > -- > > 2.31.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Hiro, On Tue, May 11, 2021 at 02:27:29PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for the patch. > > On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund < > niklas.soderlund@ragnatech.se> wrote: > > > Hi Jacopo, > > > > Thanks for your patch. > > > > On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote: > > > The CameraDevice class uses std::scoped_lock<> to guard access to the > > > class' descriptors_ member. > > > > > > std::scoped_lock<> provides a set of features that guarantees safety > > > when locking multiple mutexes in a critical section, while for single > > > locks happening in a scoped block it does not provides benefits compared > > > to the simplest std::unique_lock<> which libcamera provides the > > > MutexLocker type for. > > > > > > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make > > > the implementation consistent with the rest of the code base. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > I hear since unique_lock has more functionalities than scoped_lock, the > performance of unique_lock is better than scoped_lock. My understanding was that unique_lock being simpler than scoped_lock :) > So I would love to use scoped_lock if it is only used like std::lock_guard. > On the other hand, unique_lock is needed for std::condition_variable. > IMO, we should subtilize std::unique_lock and std::scoped_lock. > But ideally, we should declare our own mutex class to utilize clang thread > annotation. https://bugs.libcamera.org/show_bug.cgi?id=23 Oh nice you have created a bug already. > > I am not strongly against this patch though as unique_lock covers > scoped_lock and the performance difference should be fraction. That was my thinking and also that the less we mix different types of mutexes the easier will be later to rework them by using a unified construct... > > -Hiro > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Thanks! > > > > > --- > > > src/android/camera_device.cpp | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp > > b/src/android/camera_device.cpp > > > index bdb5c8ed52aa..ff965a1c8c86 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -22,6 +22,7 @@ > > > > > > #include "libcamera/internal/formats.h" > > > #include "libcamera/internal/log.h" > > > +#include "libcamera/internal/thread.h" > > > #include "libcamera/internal/utils.h" > > > > > > #include "system/graphics.h" > > > @@ -2016,7 +2017,7 @@ int > > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > worker_.queueRequest(descriptor.request_.get()); > > > > > > { > > > - std::scoped_lock<std::mutex> lock(mutex_); > > > + MutexLocker lock(mutex_); > > > descriptors_[descriptor.request_->cookie()] = > > std::move(descriptor); > > > } > > > > > > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request > > *request) > > > { > > > decltype(descriptors_)::node_type node; > > > { > > > - std::scoped_lock<std::mutex> lock(mutex_); > > > + MutexLocker lock(mutex_); > > > auto it = descriptors_.find(request->cookie()); > > > if (it == descriptors_.end()) { > > > /* > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, On Tue, May 11, 2021 at 4:52 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Hiro, > > On Tue, May 11, 2021 at 02:27:29PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, thank you for the patch. > > > > On Tue, May 11, 2021 at 5:29 AM Niklas Söderlund < > > niklas.soderlund@ragnatech.se> wrote: > > > > > Hi Jacopo, > > > > > > Thanks for your patch. > > > > > > On 2021-05-10 12:52:32 +0200, Jacopo Mondi wrote: > > > > The CameraDevice class uses std::scoped_lock<> to guard access to the > > > > class' descriptors_ member. > > > > > > > > std::scoped_lock<> provides a set of features that guarantees safety > > > > when locking multiple mutexes in a critical section, while for single > > > > locks happening in a scoped block it does not provides benefits > compared > > > > to the simplest std::unique_lock<> which libcamera provides the > > > > MutexLocker type for. > > > > > > > > Replace usage of std::scoped_lock<> with libcamera::MutexLocker to > make > > > > the implementation consistent with the rest of the code base. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > I hear since unique_lock has more functionalities than scoped_lock, the > > performance of unique_lock is better than scoped_lock. > > My understanding was that unique_lock being simpler than scoped_lock > :) > > > So I would love to use scoped_lock if it is only used like > std::lock_guard. > > On the other hand, unique_lock is needed for std::condition_variable. > > IMO, we should subtilize std::unique_lock and std::scoped_lock. > > But ideally, we should declare our own mutex class to utilize clang > thread > > annotation. https://bugs.libcamera.org/show_bug.cgi?id=23 > > Oh nice you have created a bug already. > > > > > I am not strongly against this patch though as unique_lock covers > > scoped_lock and the performance difference should be fraction. > > That was my thinking and also that the less we mix different types of > mutexes the easier will be later to rework them by using a unified > construct... > > I agree. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > -Hiro > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Thanks! > > > > > > > > --- > > > > src/android/camera_device.cpp | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp > > > b/src/android/camera_device.cpp > > > > index bdb5c8ed52aa..ff965a1c8c86 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -22,6 +22,7 @@ > > > > > > > > #include "libcamera/internal/formats.h" > > > > #include "libcamera/internal/log.h" > > > > +#include "libcamera/internal/thread.h" > > > > #include "libcamera/internal/utils.h" > > > > > > > > #include "system/graphics.h" > > > > @@ -2016,7 +2017,7 @@ int > > > CameraDevice::processCaptureRequest(camera3_capture_request_t > *camera3Reques > > > > worker_.queueRequest(descriptor.request_.get()); > > > > > > > > { > > > > - std::scoped_lock<std::mutex> lock(mutex_); > > > > + MutexLocker lock(mutex_); > > > > descriptors_[descriptor.request_->cookie()] = > > > std::move(descriptor); > > > > } > > > > > > > > @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request > > > *request) > > > > { > > > > decltype(descriptors_)::node_type node; > > > > { > > > > - std::scoped_lock<std::mutex> lock(mutex_); > > > > + MutexLocker lock(mutex_); > > > > auto it = descriptors_.find(request->cookie()); > > > > if (it == descriptors_.end()) { > > > > /* > > > > -- > > > > 2.31.1 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index bdb5c8ed52aa..ff965a1c8c86 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -22,6 +22,7 @@ #include "libcamera/internal/formats.h" #include "libcamera/internal/log.h" +#include "libcamera/internal/thread.h" #include "libcamera/internal/utils.h" #include "system/graphics.h" @@ -2016,7 +2017,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques worker_.queueRequest(descriptor.request_.get()); { - std::scoped_lock<std::mutex> lock(mutex_); + MutexLocker lock(mutex_); descriptors_[descriptor.request_->cookie()] = std::move(descriptor); } @@ -2027,7 +2028,7 @@ void CameraDevice::requestComplete(Request *request) { decltype(descriptors_)::node_type node; { - std::scoped_lock<std::mutex> lock(mutex_); + MutexLocker lock(mutex_); auto it = descriptors_.find(request->cookie()); if (it == descriptors_.end()) { /*
The CameraDevice class uses std::scoped_lock<> to guard access to the class' descriptors_ member. std::scoped_lock<> provides a set of features that guarantees safety when locking multiple mutexes in a critical section, while for single locks happening in a scoped block it does not provides benefits compared to the simplest std::unique_lock<> which libcamera provides the MutexLocker type for. Replace usage of std::scoped_lock<> with libcamera::MutexLocker to make the implementation consistent with the rest of the code base. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)