| Message ID | 20221027055515.321791-3-nicholas@rothemail.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicholas On Thu, Oct 27, 2022 at 12:55:07AM -0500, Nicholas Roth via libcamera-devel wrote: > From: Nicholas Roth <nicholas@rothemail.net> How come this gets inserted here by git when the commit author and the sender have the same identity ? > > The raspberrypi IPA is missing thread-safety annotations, which breaks > the build. > > Add required thread-safety annotations. > > ../src/ipa/raspberrypi/controller/metadata.h:108:31: error: mutex > 'mutex_' is still held at the end of function [-Werror,-Wthread-safety-analysis] > void lock() { mutex_.lock(); } > ^ > ../src/ipa/raspberrypi/controller/metadata.h:108:23: note: mutex > acquired here > void lock() { mutex_.lock(); } > ^ > ../src/ipa/raspberrypi/controller/metadata.h:109:25: error: releasing > mutex 'mutex_' that was not held [-Werror,-Wthread-safety-analysis] > void unlock() { mutex_.unlock(); } > ^ nit: double empty line > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> I've not followed much the thread annotation thing, but this seems correct to me, even more so if it silences a compilation error. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/ipa/raspberrypi/controller/metadata.h | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/raspberrypi/controller/metadata.h b/src/ipa/raspberrypi/controller/metadata.h > index 0f7ebfaf..870b6e26 100644 > --- a/src/ipa/raspberrypi/controller/metadata.h > +++ b/src/ipa/raspberrypi/controller/metadata.h > @@ -13,9 +13,11 @@ > #include <mutex> > #include <string> > > +#include <libcamera/base/thread_annotations.h> > + > namespace RPiController { > > -class Metadata > +class LIBCAMERA_TSA_CAPABILITY("mutex") Metadata > { > public: > Metadata() = default; > @@ -103,8 +105,8 @@ public: > * locks with the standard lock classes. > * e.g. std::lock_guard<RPiController::Metadata> lock(metadata) > */ > - void lock() { mutex_.lock(); } > - void unlock() { mutex_.unlock(); } > + void lock() LIBCAMERA_TSA_ACQUIRE() { mutex_.lock(); } > + void unlock() LIBCAMERA_TSA_RELEASE() { mutex_.unlock(); } > > private: > mutable std::mutex mutex_; > -- > 2.34.1 >
> How come this gets inserted here by git when the commit author and the sender have the same identity ? I'm really not sure. Maybe because I specify "--from" in git-send-email? Is this not a standard thing to do? git send-email --compose --from=nicholas@rothemail.net --reply-to= libcamera-devel@lists.libcamera.org --to=libcamera-devel@lists.libcamera.org --subject="libcamera Android Enhancements" --thread --no-chain-reply-to --annotate -v2 main > nit: double empty line If you're referring to the commit message, the line below the error is for the carat that Clang uses to indicate the position of the error. Happy to not skip a line before "Signed-off-by" if it would help. > this seems correct to me, even more so if it silences a compilation error. Do I need an LGTM from anyone else? What would the next step be here? On Thu, Oct 27, 2022 at 11:08 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Nicholas > > On Thu, Oct 27, 2022 at 12:55:07AM -0500, Nicholas Roth via > libcamera-devel wrote: > > From: Nicholas Roth <nicholas@rothemail.net> > > How come this gets inserted here by git when the commit author and the > sender have the same identity ? > > > > > The raspberrypi IPA is missing thread-safety annotations, which breaks > > the build. > > > > Add required thread-safety annotations. > > > > ../src/ipa/raspberrypi/controller/metadata.h:108:31: error: mutex > > 'mutex_' is still held at the end of function > [-Werror,-Wthread-safety-analysis] > > void lock() { mutex_.lock(); } > > ^ > > ../src/ipa/raspberrypi/controller/metadata.h:108:23: note: mutex > > acquired here > > void lock() { mutex_.lock(); } > > ^ > > ../src/ipa/raspberrypi/controller/metadata.h:109:25: error: releasing > > mutex 'mutex_' that was not held [-Werror,-Wthread-safety-analysis] > > void unlock() { mutex_.unlock(); } > > ^ > > nit: double empty line > > > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > > I've not followed much the thread annotation thing, but this seems > correct to me, even more so if it silences a compilation error. > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > src/ipa/raspberrypi/controller/metadata.h | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/controller/metadata.h > b/src/ipa/raspberrypi/controller/metadata.h > > index 0f7ebfaf..870b6e26 100644 > > --- a/src/ipa/raspberrypi/controller/metadata.h > > +++ b/src/ipa/raspberrypi/controller/metadata.h > > @@ -13,9 +13,11 @@ > > #include <mutex> > > #include <string> > > > > +#include <libcamera/base/thread_annotations.h> > > + > > namespace RPiController { > > > > -class Metadata > > +class LIBCAMERA_TSA_CAPABILITY("mutex") Metadata > > { > > public: > > Metadata() = default; > > @@ -103,8 +105,8 @@ public: > > * locks with the standard lock classes. > > * e.g. std::lock_guard<RPiController::Metadata> lock(metadata) > > */ > > - void lock() { mutex_.lock(); } > > - void unlock() { mutex_.unlock(); } > > + void lock() LIBCAMERA_TSA_ACQUIRE() { mutex_.lock(); } > > + void unlock() LIBCAMERA_TSA_RELEASE() { mutex_.unlock(); } > > > > private: > > mutable std::mutex mutex_; > > -- > > 2.34.1 > > >
Hi Nicholas On Thu, Oct 27, 2022 at 05:04:31PM -0500, Nicholas Roth wrote: > > How come this gets inserted here by git when the commit author and the > sender have the same identity ? > > I'm really not sure. Maybe because I specify "--from" in git-send-email? Is > this not a standard thing to do? > git send-email --compose --from=nicholas@rothemail.net --reply-to= > libcamera-devel@lists.libcamera.org --to=libcamera-devel@lists.libcamera.org > --subject="libcamera Android Enhancements" --thread --no-chain-reply-to > --annotate -v2 main Oh, so many options :) I'm not sure if --from is the culprit, it might be, but be aware the sender can be picked up automatically by git global config variables and you most probably don't need this option. I would discourage --reply-to, it mangles the "Reply-To" field of your patches, something that might even break someone's scripts. Just spcify the mailing list in the receiver or cc list and people will reply to it. --thread should be on by default if I get it right, same for --no-chain-reply-to Try to drop the --from and --reply-to options and see how it goes > > > nit: double empty line > If you're referring to the commit message, the line below the error is for > the carat that Clang uses to indicate the position of the error. Happy to > not skip a line before "Signed-off-by" if it would help. Ah sorry, missed it > > > this seems correct to me, even more so if it silences a compilation error. > Do I need an LGTM from anyone else? What would the next step be here? The policy is usually to get at least two reviewed-by tags. Once the series is fully reviewed it will be picked up, run through a few compiler matrices and if it touches the Android HAL through CTS, then merged. The process is still a bit fuzzy, we don't have per-component maintainers nor someone designed specifically for the 'collect-test-merge' part, but it's usually Laurent and Kieran taking care of that. > > On Thu, Oct 27, 2022 at 11:08 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hi Nicholas > > > > On Thu, Oct 27, 2022 at 12:55:07AM -0500, Nicholas Roth via > > libcamera-devel wrote: > > > From: Nicholas Roth <nicholas@rothemail.net> > > > > How come this gets inserted here by git when the commit author and the > > sender have the same identity ? > > > > > > > > The raspberrypi IPA is missing thread-safety annotations, which breaks > > > the build. > > > > > > Add required thread-safety annotations. > > > > > > ../src/ipa/raspberrypi/controller/metadata.h:108:31: error: mutex > > > 'mutex_' is still held at the end of function > > [-Werror,-Wthread-safety-analysis] > > > void lock() { mutex_.lock(); } > > > ^ > > > ../src/ipa/raspberrypi/controller/metadata.h:108:23: note: mutex > > > acquired here > > > void lock() { mutex_.lock(); } > > > ^ > > > ../src/ipa/raspberrypi/controller/metadata.h:109:25: error: releasing > > > mutex 'mutex_' that was not held [-Werror,-Wthread-safety-analysis] > > > void unlock() { mutex_.unlock(); } > > > ^ > > > > nit: double empty line > > > > > > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > > > > I've not followed much the thread annotation thing, but this seems > > correct to me, even more so if it silences a compilation error. > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > src/ipa/raspberrypi/controller/metadata.h | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/ipa/raspberrypi/controller/metadata.h > > b/src/ipa/raspberrypi/controller/metadata.h > > > index 0f7ebfaf..870b6e26 100644 > > > --- a/src/ipa/raspberrypi/controller/metadata.h > > > +++ b/src/ipa/raspberrypi/controller/metadata.h > > > @@ -13,9 +13,11 @@ > > > #include <mutex> > > > #include <string> > > > > > > +#include <libcamera/base/thread_annotations.h> > > > + > > > namespace RPiController { > > > > > > -class Metadata > > > +class LIBCAMERA_TSA_CAPABILITY("mutex") Metadata > > > { > > > public: > > > Metadata() = default; > > > @@ -103,8 +105,8 @@ public: > > > * locks with the standard lock classes. > > > * e.g. std::lock_guard<RPiController::Metadata> lock(metadata) > > > */ > > > - void lock() { mutex_.lock(); } > > > - void unlock() { mutex_.unlock(); } > > > + void lock() LIBCAMERA_TSA_ACQUIRE() { mutex_.lock(); } > > > + void unlock() LIBCAMERA_TSA_RELEASE() { mutex_.unlock(); } > > > > > > private: > > > mutable std::mutex mutex_; > > > -- > > > 2.34.1 > > > > >
Hi Nicholas, Quoting Jacopo Mondi via libcamera-devel (2022-10-27 17:08:33) > Hi Nicholas > > On Thu, Oct 27, 2022 at 12:55:07AM -0500, Nicholas Roth via libcamera-devel wrote: > > From: Nicholas Roth <nicholas@rothemail.net> > > How come this gets inserted here by git when the commit author and the > sender have the same identity ? > > > > > The raspberrypi IPA is missing thread-safety annotations, which breaks > > the build. > > > > Add required thread-safety annotations. > > > > ../src/ipa/raspberrypi/controller/metadata.h:108:31: error: mutex > > 'mutex_' is still held at the end of function [-Werror,-Wthread-safety-analysis] > > void lock() { mutex_.lock(); } > > ^ > > ../src/ipa/raspberrypi/controller/metadata.h:108:23: note: mutex > > acquired here > > void lock() { mutex_.lock(); } > > ^ > > ../src/ipa/raspberrypi/controller/metadata.h:109:25: error: releasing > > mutex 'mutex_' that was not held [-Werror,-Wthread-safety-analysis] > > void unlock() { mutex_.unlock(); } > > ^ > > nit: double empty line > > > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > > I've not followed much the thread annotation thing, but this seems > correct to me, even more so if it silences a compilation error. > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> These tags are valuable to you. We desire two Reviewed-by (or Acked-by:) tags to get a patch merged. So when you're given one, please add it to your patch so that it gets included when you repost a later version. I also think this patch is good, but I'll add my tag to the latest version, however in this instance - because it's under src/ipa/raspberrypi/ - we'll want an ack from RPi too. -- Kieran > > > --- > > src/ipa/raspberrypi/controller/metadata.h | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/controller/metadata.h b/src/ipa/raspberrypi/controller/metadata.h > > index 0f7ebfaf..870b6e26 100644 > > --- a/src/ipa/raspberrypi/controller/metadata.h > > +++ b/src/ipa/raspberrypi/controller/metadata.h > > @@ -13,9 +13,11 @@ > > #include <mutex> > > #include <string> > > > > +#include <libcamera/base/thread_annotations.h> > > + > > namespace RPiController { > > > > -class Metadata > > +class LIBCAMERA_TSA_CAPABILITY("mutex") Metadata > > { > > public: > > Metadata() = default; > > @@ -103,8 +105,8 @@ public: > > * locks with the standard lock classes. > > * e.g. std::lock_guard<RPiController::Metadata> lock(metadata) > > */ > > - void lock() { mutex_.lock(); } > > - void unlock() { mutex_.unlock(); } > > + void lock() LIBCAMERA_TSA_ACQUIRE() { mutex_.lock(); } > > + void unlock() LIBCAMERA_TSA_RELEASE() { mutex_.unlock(); } > > > > private: > > mutable std::mutex mutex_; > > -- > > 2.34.1 > >
Quoting Jacopo Mondi via libcamera-devel (2022-10-28 08:32:31) > Hi Nicholas > > On Thu, Oct 27, 2022 at 05:04:31PM -0500, Nicholas Roth wrote: > > > How come this gets inserted here by git when the commit author and the > > sender have the same identity ? > > > > I'm really not sure. Maybe because I specify "--from" in git-send-email? Is > > this not a standard thing to do? > > git send-email --compose --from=nicholas@rothemail.net --reply-to= > > libcamera-devel@lists.libcamera.org --to=libcamera-devel@lists.libcamera.org > > --subject="libcamera Android Enhancements" --thread --no-chain-reply-to > > --annotate -v2 main > > Oh, so many options :) > > I'm not sure if --from is the culprit, it might be, but be aware the > sender can be picked up automatically by git global config variables > and you most probably don't need this option. > > I would discourage --reply-to, it mangles the "Reply-To" field of your > patches, something that might even break someone's scripts. Just > spcify the mailing list in the receiver or cc list and people will > reply to it. > > --thread should be on by default if I get it right, same for > --no-chain-reply-to > > Try to drop the --from and --reply-to options and see how it goes > > > > > > > nit: double empty line > > If you're referring to the commit message, the line below the error is for > > the carat that Clang uses to indicate the position of the error. Happy to > > not skip a line before "Signed-off-by" if it would help. > > Ah sorry, missed it > > > > > > this seems correct to me, even more so if it silences a compilation error. > > Do I need an LGTM from anyone else? What would the next step be here? > > The policy is usually to get at least two reviewed-by tags. Once the > series is fully reviewed it will be picked up, run through a few > compiler matrices and if it touches the Android HAL through CTS, then > merged. Yes, we need to write this down clearly somewhere. > The process is still a bit fuzzy, we don't have per-component > maintainers nor someone designed specifically for the > 'collect-test-merge' part, but it's usually Laurent and Kieran taking > care of that. I suspect that's mostly because I have infrastructure to run all the tests in the lab here. But if we could get that more automated, with CTS and testing on RPi/ other boards - we could hook up better means for integration. Just seems to take so much time to do CI development etc. The only other missing piece here, is that patches touching code supporting the RPi should be either reviewed or Acked by a developer at RPi. (The same would be said later of other platforms supported by their vendors, but we cover the rest at the moment). -- Kieran > > On Thu, Oct 27, 2022 at 11:08 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > Hi Nicholas > > > > > > On Thu, Oct 27, 2022 at 12:55:07AM -0500, Nicholas Roth via > > > libcamera-devel wrote: > > > > From: Nicholas Roth <nicholas@rothemail.net> > > > > > > How come this gets inserted here by git when the commit author and the > > > sender have the same identity ? > > > > > > > > > > > The raspberrypi IPA is missing thread-safety annotations, which breaks > > > > the build. > > > > > > > > Add required thread-safety annotations. > > > > > > > > ../src/ipa/raspberrypi/controller/metadata.h:108:31: error: mutex > > > > 'mutex_' is still held at the end of function > > > [-Werror,-Wthread-safety-analysis] > > > > void lock() { mutex_.lock(); } > > > > ^ > > > > ../src/ipa/raspberrypi/controller/metadata.h:108:23: note: mutex > > > > acquired here > > > > void lock() { mutex_.lock(); } > > > > ^ > > > > ../src/ipa/raspberrypi/controller/metadata.h:109:25: error: releasing > > > > mutex 'mutex_' that was not held [-Werror,-Wthread-safety-analysis] > > > > void unlock() { mutex_.unlock(); } > > > > ^ > > > > > > nit: double empty line > > > > > > > > > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > > > > > > I've not followed much the thread annotation thing, but this seems > > > correct to me, even more so if it silences a compilation error. > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > --- > > > > src/ipa/raspberrypi/controller/metadata.h | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/metadata.h > > > b/src/ipa/raspberrypi/controller/metadata.h > > > > index 0f7ebfaf..870b6e26 100644 > > > > --- a/src/ipa/raspberrypi/controller/metadata.h > > > > +++ b/src/ipa/raspberrypi/controller/metadata.h > > > > @@ -13,9 +13,11 @@ > > > > #include <mutex> > > > > #include <string> > > > > > > > > +#include <libcamera/base/thread_annotations.h> > > > > + > > > > namespace RPiController { > > > > > > > > -class Metadata > > > > +class LIBCAMERA_TSA_CAPABILITY("mutex") Metadata > > > > { > > > > public: > > > > Metadata() = default; > > > > @@ -103,8 +105,8 @@ public: > > > > * locks with the standard lock classes. > > > > * e.g. std::lock_guard<RPiController::Metadata> lock(metadata) > > > > */ > > > > - void lock() { mutex_.lock(); } > > > > - void unlock() { mutex_.unlock(); } > > > > + void lock() LIBCAMERA_TSA_ACQUIRE() { mutex_.lock(); } > > > > + void unlock() LIBCAMERA_TSA_RELEASE() { mutex_.unlock(); } > > > > > > > > private: > > > > mutable std::mutex mutex_; > > > > -- > > > > 2.34.1 > > > > > > >
diff --git a/src/ipa/raspberrypi/controller/metadata.h b/src/ipa/raspberrypi/controller/metadata.h index 0f7ebfaf..870b6e26 100644 --- a/src/ipa/raspberrypi/controller/metadata.h +++ b/src/ipa/raspberrypi/controller/metadata.h @@ -13,9 +13,11 @@ #include <mutex> #include <string> +#include <libcamera/base/thread_annotations.h> + namespace RPiController { -class Metadata +class LIBCAMERA_TSA_CAPABILITY("mutex") Metadata { public: Metadata() = default; @@ -103,8 +105,8 @@ public: * locks with the standard lock classes. * e.g. std::lock_guard<RPiController::Metadata> lock(metadata) */ - void lock() { mutex_.lock(); } - void unlock() { mutex_.unlock(); } + void lock() LIBCAMERA_TSA_ACQUIRE() { mutex_.lock(); } + void unlock() LIBCAMERA_TSA_RELEASE() { mutex_.unlock(); } private: mutable std::mutex mutex_;