[libcamera-devel] [PATCH 02/10] ipa: add missing thread-safety annotations

[Kieran Bingham]

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 at rothemail.net --reply-to=
> > libcamera-devel at lists.libcamera.org --to=libcamera-devel at 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 at 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 at 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 at 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 at 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
> > > >
> > >