[libcamera-devel,7/7] ipa: raspberrypi: Remove atomic variable from Algorithm class

11137 diff mbox series

Pause() and Resume() are only called synchronously so paused_ does not
need to be atomic.

With this commit, libatomic can finally be removed as a build
dependency.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/algorithm.hpp | 3 +--
 src/ipa/raspberrypi/meson.build              | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

Hi David,

Thank you for the patch.

On Thu, Feb 04, 2021 at 09:34:57AM +0000, David Plowman wrote:
> Pause() and Resume() are only called synchronously so paused_ does not
> need to be atomic.
> 
> With this commit, libatomic can finally be removed as a build
> dependency.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/algorithm.hpp | 3 +--
>  src/ipa/raspberrypi/meson.build              | 1 -
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp
> index e9b040c7..5123c87b 100644
> --- a/src/ipa/raspberrypi/controller/algorithm.hpp
> +++ b/src/ipa/raspberrypi/controller/algorithm.hpp
> @@ -12,7 +12,6 @@
>  #include <string>
>  #include <memory>
>  #include <map>
> -#include <atomic>
>  
>  #include "controller.hpp"
>  
> @@ -46,7 +45,7 @@ public:
>  
>  private:
>  	Controller *controller_;
> -	std::atomic<bool> paused_;
> +	bool paused_;
>  };
>  
>  // This code is for automatic registration of Front End algorithms with the

This part is fine.

> diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> index 9445cd09..4cdd0434 100644
> --- a/src/ipa/raspberrypi/meson.build
> +++ b/src/ipa/raspberrypi/meson.build
> @@ -5,7 +5,6 @@ ipa_name = 'ipa_rpi'
>  rpi_ipa_deps = [
>      libcamera_dep,
>      dependency('boost'),
> -    libatomic,
>  ]
>  
>  rpi_ipa_includes = [

But here, I wonder if we may need to keep libatomic. It provides C
functions that are used under the hood by the compiler to implement
atomic operations. Those are used by the C++ std::atomic implementation,
but they could also be used internally by std::thread or related locking
classes.

I'm not an expert on this topic, so I don't know what the right solution
is. Should we split this patch in two to merge the removal of
std::atomic from algorithm.hpp while we try to figure out whether we
should still keep linking with libatomic ?

Hi Laurent

Thanks for the comments.

On Sun, 7 Feb 2021 at 14:22, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Feb 04, 2021 at 09:34:57AM +0000, David Plowman wrote:
> > Pause() and Resume() are only called synchronously so paused_ does not
> > need to be atomic.
> >
> > With this commit, libatomic can finally be removed as a build
> > dependency.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/algorithm.hpp | 3 +--
> >  src/ipa/raspberrypi/meson.build              | 1 -
> >  2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp
> > index e9b040c7..5123c87b 100644
> > --- a/src/ipa/raspberrypi/controller/algorithm.hpp
> > +++ b/src/ipa/raspberrypi/controller/algorithm.hpp
> > @@ -12,7 +12,6 @@
> >  #include <string>
> >  #include <memory>
> >  #include <map>
> > -#include <atomic>
> >
> >  #include "controller.hpp"
> >
> > @@ -46,7 +45,7 @@ public:
> >
> >  private:
> >       Controller *controller_;
> > -     std::atomic<bool> paused_;
> > +     bool paused_;
> >  };
> >
> >  // This code is for automatic registration of Front End algorithms with the
>
> This part is fine.
>
> > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build
> > index 9445cd09..4cdd0434 100644
> > --- a/src/ipa/raspberrypi/meson.build
> > +++ b/src/ipa/raspberrypi/meson.build
> > @@ -5,7 +5,6 @@ ipa_name = 'ipa_rpi'
> >  rpi_ipa_deps = [
> >      libcamera_dep,
> >      dependency('boost'),
> > -    libatomic,
> >  ]
> >
> >  rpi_ipa_includes = [
>
> But here, I wonder if we may need to keep libatomic. It provides C
> functions that are used under the hood by the compiler to implement
> atomic operations. Those are used by the C++ std::atomic implementation,
> but they could also be used internally by std::thread or related locking
> classes.
>
> I'm not an expert on this topic, so I don't know what the right solution
> is. Should we split this patch in two to merge the removal of
> std::atomic from algorithm.hpp while we try to figure out whether we
> should still keep linking with libatomic ?

Yes, let me turn that one patch into a pair so that we can consider what's best.

Thanks
David

>
> --
> Regards,
>
> Laurent Pinchart