libcamera: ipa_manager: Remove singleton requirement
diff mbox series

Message ID 20240803212832.22766-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: ipa_manager: Remove singleton requirement
Related show

Commit Message

Laurent Pinchart Aug. 3, 2024, 9:28 p.m. UTC 
The IPAManager class implements a singleton pattern due to the need of
accessing the instance in a static member function. The function now
takes a pointer to a PipelineHandler, which we can use to access the
CameraManager, and from there, the IPAManager.

Add accessors to the internal API to expose the CameraManager from the
PipelineHandler, and the IPAManager from the CameraManager. This
requires allocating the IPAManager dynamically to avoid a loop in
includes. Use those accessors to replace the IPAManager singleton.

Update the IPA interface unit test to instantiate a CameraManager
instead of an IPAManager and ProcessManager, to reflect the new way that
the IPAManager is accessed.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/camera_manager.h   |  7 +++++--
 include/libcamera/internal/ipa_manager.h      |  9 +++++----
 include/libcamera/internal/pipeline_handler.h |  2 ++
 src/libcamera/camera_manager.cpp              |  9 +++++++++
 src/libcamera/ipa_manager.cpp                 | 10 ----------
 src/libcamera/pipeline_handler.cpp            |  7 +++++++
 test/ipa/ipa_interface_test.cpp               | 12 +++++-------
 7 files changed, 33 insertions(+), 23 deletions(-)


base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5

Comments

Kieran Bingham Aug. 7, 2024, 1:16 p.m. UTC | #1 
Quoting Laurent Pinchart (2024-08-03 22:28:32)
> The IPAManager class implements a singleton pattern due to the need of
> accessing the instance in a static member function. The function now
> takes a pointer to a PipelineHandler, which we can use to access the
> CameraManager, and from there, the IPAManager.
> 
> Add accessors to the internal API to expose the CameraManager from the
> PipelineHandler, and the IPAManager from the CameraManager. This
> requires allocating the IPAManager dynamically to avoid a loop in
> includes. Use those accessors to replace the IPAManager singleton.
> 
> Update the IPA interface unit test to instantiate a CameraManager
> instead of an IPAManager and ProcessManager, to reflect the new way that
> the IPAManager is accessed.

I guess that's a bit more heavyweight for a unit test, but nothing out
of the ordinary given 'CameraManager' is a core component of libcamera.

No objection to this, and removing seems worthwhile - but I'm curious
what the motivation was.

Anyway, I haven't spotted any errors so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_manager.h   |  7 +++++--
>  include/libcamera/internal/ipa_manager.h      |  9 +++++----
>  include/libcamera/internal/pipeline_handler.h |  2 ++
>  src/libcamera/camera_manager.cpp              |  9 +++++++++
>  src/libcamera/ipa_manager.cpp                 | 10 ----------
>  src/libcamera/pipeline_handler.cpp            |  7 +++++++
>  test/ipa/ipa_interface_test.cpp               | 12 +++++-------
>  7 files changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index af9ed60a0353..e098cb69aa43 100644
> --- a/include/libcamera/internal/camera_manager.h
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -19,13 +19,14 @@
>  #include <libcamera/base/thread.h>
>  #include <libcamera/base/thread_annotations.h>
>  
> -#include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/process.h"
>  
>  namespace libcamera {
>  
>  class Camera;
>  class DeviceEnumerator;
> +class IPAManager;
> +class PipelineHandlerFactoryBase;
>  
>  class CameraManager::Private : public Extensible::Private, public Thread
>  {
> @@ -38,6 +39,8 @@ public:
>         void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
>         void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
>  
> +       IPAManager *ipaManager() const { return ipaManager_.get(); }
> +
>  protected:
>         void run() override;
>  
> @@ -62,7 +65,7 @@ private:
>  
>         std::unique_ptr<DeviceEnumerator> enumerator_;
>  
> -       IPAManager ipaManager_;
> +       std::unique_ptr<IPAManager> ipaManager_;
>         ProcessManager processManager_;
>  };
>  
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index c6f74e11c434..16dede0c5a7e 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -15,6 +15,7 @@
>  #include <libcamera/ipa/ipa_interface.h>
>  #include <libcamera/ipa/ipa_module_info.h>
>  
> +#include "libcamera/internal/camera_manager.h"
>  #include "libcamera/internal/ipa_module.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/pub_key.h"
> @@ -34,11 +35,13 @@ public:
>                                             uint32_t minVersion,
>                                             uint32_t maxVersion)
>         {
> -               IPAModule *m = self_->module(pipe, minVersion, maxVersion);
> +               CameraManager *cm = pipe->cameraManager();
> +               IPAManager *self = cm->_d()->ipaManager();
> +               IPAModule *m = self->module(pipe, minVersion, maxVersion);
>                 if (!m)
>                         return nullptr;
>  
> -               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self_->isSignatureValid(m));
> +               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
>                 if (!proxy->isValid()) {
>                         LOG(IPAManager, Error) << "Failed to load proxy";
>                         return nullptr;
> @@ -55,8 +58,6 @@ public:
>  #endif
>  
>  private:
> -       static IPAManager *self_;
> -
>         void parseDir(const char *libDir, unsigned int maxDepth,
>                       std::vector<std::string> &files);
>         unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 746a34f8e7bd..cad5812f640c 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -70,6 +70,8 @@ public:
>  
>         const char *name() const { return name_; }
>  
> +       CameraManager *cameraManager() const { return manager_; }
> +
>  protected:
>         void registerCamera(std::shared_ptr<Camera> camera);
>         void hotplugMediaDevice(MediaDevice *media);
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 95a9e3264526..31760a8680cc 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -15,6 +15,7 @@
>  
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  
>  /**
> @@ -37,6 +38,7 @@ LOG_DEFINE_CATEGORY(Camera)
>  CameraManager::Private::Private()
>         : initialized_(false)
>  {
> +       ipaManager_ = std::make_unique<IPAManager>();
>  }
>  
>  int CameraManager::Private::start()
> @@ -249,6 +251,13 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
>         o->cameraRemoved.emit(camera);
>  }
>  
> +/**
> + * \fn CameraManager::Private::ipaManager() const
> + * \brief Retrieve the IPAManager
> + * \context This function is \threadsafe.
> + * \return The IPAManager for this CameraManager
> + */
> +
>  /**
>   * \class CameraManager
>   * \brief Provide access and manage all cameras in the system
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index f4e0b6339f08..cfc24d389c4f 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -95,8 +95,6 @@ LOG_DEFINE_CATEGORY(IPAManager)
>   * IPC.
>   */
>  
> -IPAManager *IPAManager::self_ = nullptr;
> -
>  /**
>   * \brief Construct an IPAManager instance
>   *
> @@ -105,10 +103,6 @@ IPAManager *IPAManager::self_ = nullptr;
>   */
>  IPAManager::IPAManager()
>  {
> -       if (self_)
> -               LOG(IPAManager, Fatal)
> -                       << "Multiple IPAManager objects are not allowed";
> -
>  #if HAVE_IPA_PUBKEY
>         if (!pubKey_.isValid())
>                 LOG(IPAManager, Warning) << "Public key not valid";
> @@ -153,16 +147,12 @@ IPAManager::IPAManager()
>         if (!ipaCount)
>                 LOG(IPAManager, Warning)
>                         << "No IPA found in '" IPA_MODULE_DIR "'";
> -
> -       self_ = this;
>  }
>  
>  IPAManager::~IPAManager()
>  {
>         for (IPAModule *module : modules_)
>                 delete module;
> -
> -       self_ = nullptr;
>  }
>  
>  /**
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 5ea2ca780b63..5a6de685b292 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -719,6 +719,13 @@ void PipelineHandler::disconnect()
>   * \return The pipeline handler name
>   */
>  
> +/**
> + * \fn PipelineHandler::cameraManager() const
> + * \brief Retrieve the CameraManager that this pipeline handler belongs to
> + * \context This function is \threadsafe.
> + * \return The CameraManager for this pipeline handler
> + */
> +
>  /**
>   * \class PipelineHandlerFactoryBase
>   * \brief Base class for pipeline handler factories
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index e840f6ab17c4..b81783664977 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -20,11 +20,11 @@
>  #include <libcamera/base/thread.h>
>  #include <libcamera/base/timer.h>
>  
> +#include "libcamera/internal/camera_manager.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/ipa_module.h"
>  #include "libcamera/internal/pipeline_handler.h"
> -#include "libcamera/internal/process.h"
>  
>  #include "test.h"
>  
> @@ -44,20 +44,20 @@ public:
>         {
>                 delete notifier_;
>                 ipa_.reset();
> -               ipaManager_.reset();
> +               cameraManager_.reset();
>         }
>  
>  protected:
>         int init() override
>         {
> -               ipaManager_ = make_unique<IPAManager>();
> +               cameraManager_ = make_unique<CameraManager>();
>  
>                 /* Create a pipeline handler for vimc. */
>                 const std::vector<PipelineHandlerFactoryBase *> &factories =
>                         PipelineHandlerFactoryBase::factories();
>                 for (const PipelineHandlerFactoryBase *factory : factories) {
>                         if (factory->name() == "vimc") {
> -                               pipe_ = factory->create(nullptr);
> +                               pipe_ = factory->create(cameraManager_.get());
>                                 break;
>                         }
>                 }
> @@ -171,11 +171,9 @@ private:
>                 }
>         }
>  
> -       ProcessManager processManager_;
> -
>         std::shared_ptr<PipelineHandler> pipe_;
>         std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> -       std::unique_ptr<IPAManager> ipaManager_;
> +       std::unique_ptr<CameraManager> cameraManager_;
>         enum ipa::vimc::IPAOperationCode trace_;
>         EventNotifier *notifier_;
>         int fd_;
> 
> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Aug. 7, 2024, 1:28 p.m. UTC | #2 
On Wed, Aug 07, 2024 at 02:16:39PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-08-03 22:28:32)
> > The IPAManager class implements a singleton pattern due to the need of
> > accessing the instance in a static member function. The function now
> > takes a pointer to a PipelineHandler, which we can use to access the
> > CameraManager, and from there, the IPAManager.
> > 
> > Add accessors to the internal API to expose the CameraManager from the
> > PipelineHandler, and the IPAManager from the CameraManager. This
> > requires allocating the IPAManager dynamically to avoid a loop in
> > includes. Use those accessors to replace the IPAManager singleton.
> > 
> > Update the IPA interface unit test to instantiate a CameraManager
> > instead of an IPAManager and ProcessManager, to reflect the new way that
> > the IPAManager is accessed.
> 
> I guess that's a bit more heavyweight for a unit test, but nothing out
> of the ordinary given 'CameraManager' is a core component of libcamera.
> 
> No objection to this, and removing seems worthwhile - but I'm curious
> what the motivation was.

I was trying to remove all singletons. Some are more difficult, so this
is still work in progress.

> Anyway, I haven't spotted any errors so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_manager.h   |  7 +++++--
> >  include/libcamera/internal/ipa_manager.h      |  9 +++++----
> >  include/libcamera/internal/pipeline_handler.h |  2 ++
> >  src/libcamera/camera_manager.cpp              |  9 +++++++++
> >  src/libcamera/ipa_manager.cpp                 | 10 ----------
> >  src/libcamera/pipeline_handler.cpp            |  7 +++++++
> >  test/ipa/ipa_interface_test.cpp               | 12 +++++-------
> >  7 files changed, 33 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> > index af9ed60a0353..e098cb69aa43 100644
> > --- a/include/libcamera/internal/camera_manager.h
> > +++ b/include/libcamera/internal/camera_manager.h
> > @@ -19,13 +19,14 @@
> >  #include <libcamera/base/thread.h>
> >  #include <libcamera/base/thread_annotations.h>
> >  
> > -#include "libcamera/internal/ipa_manager.h"
> >  #include "libcamera/internal/process.h"
> >  
> >  namespace libcamera {
> >  
> >  class Camera;
> >  class DeviceEnumerator;
> > +class IPAManager;
> > +class PipelineHandlerFactoryBase;
> >  
> >  class CameraManager::Private : public Extensible::Private, public Thread
> >  {
> > @@ -38,6 +39,8 @@ public:
> >         void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >         void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> >  
> > +       IPAManager *ipaManager() const { return ipaManager_.get(); }
> > +
> >  protected:
> >         void run() override;
> >  
> > @@ -62,7 +65,7 @@ private:
> >  
> >         std::unique_ptr<DeviceEnumerator> enumerator_;
> >  
> > -       IPAManager ipaManager_;
> > +       std::unique_ptr<IPAManager> ipaManager_;
> >         ProcessManager processManager_;
> >  };
> >  
> > diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> > index c6f74e11c434..16dede0c5a7e 100644
> > --- a/include/libcamera/internal/ipa_manager.h
> > +++ b/include/libcamera/internal/ipa_manager.h
> > @@ -15,6 +15,7 @@
> >  #include <libcamera/ipa/ipa_interface.h>
> >  #include <libcamera/ipa/ipa_module_info.h>
> >  
> > +#include "libcamera/internal/camera_manager.h"
> >  #include "libcamera/internal/ipa_module.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> >  #include "libcamera/internal/pub_key.h"
> > @@ -34,11 +35,13 @@ public:
> >                                             uint32_t minVersion,
> >                                             uint32_t maxVersion)
> >         {
> > -               IPAModule *m = self_->module(pipe, minVersion, maxVersion);
> > +               CameraManager *cm = pipe->cameraManager();
> > +               IPAManager *self = cm->_d()->ipaManager();
> > +               IPAModule *m = self->module(pipe, minVersion, maxVersion);
> >                 if (!m)
> >                         return nullptr;
> >  
> > -               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self_->isSignatureValid(m));
> > +               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
> >                 if (!proxy->isValid()) {
> >                         LOG(IPAManager, Error) << "Failed to load proxy";
> >                         return nullptr;
> > @@ -55,8 +58,6 @@ public:
> >  #endif
> >  
> >  private:
> > -       static IPAManager *self_;
> > -
> >         void parseDir(const char *libDir, unsigned int maxDepth,
> >                       std::vector<std::string> &files);
> >         unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 746a34f8e7bd..cad5812f640c 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -70,6 +70,8 @@ public:
> >  
> >         const char *name() const { return name_; }
> >  
> > +       CameraManager *cameraManager() const { return manager_; }
> > +
> >  protected:
> >         void registerCamera(std::shared_ptr<Camera> camera);
> >         void hotplugMediaDevice(MediaDevice *media);
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 95a9e3264526..31760a8680cc 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -15,6 +15,7 @@
> >  
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/device_enumerator.h"
> > +#include "libcamera/internal/ipa_manager.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> >  
> >  /**
> > @@ -37,6 +38,7 @@ LOG_DEFINE_CATEGORY(Camera)
> >  CameraManager::Private::Private()
> >         : initialized_(false)
> >  {
> > +       ipaManager_ = std::make_unique<IPAManager>();
> >  }
> >  
> >  int CameraManager::Private::start()
> > @@ -249,6 +251,13 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
> >         o->cameraRemoved.emit(camera);
> >  }
> >  
> > +/**
> > + * \fn CameraManager::Private::ipaManager() const
> > + * \brief Retrieve the IPAManager
> > + * \context This function is \threadsafe.
> > + * \return The IPAManager for this CameraManager
> > + */
> > +
> >  /**
> >   * \class CameraManager
> >   * \brief Provide access and manage all cameras in the system
> > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> > index f4e0b6339f08..cfc24d389c4f 100644
> > --- a/src/libcamera/ipa_manager.cpp
> > +++ b/src/libcamera/ipa_manager.cpp
> > @@ -95,8 +95,6 @@ LOG_DEFINE_CATEGORY(IPAManager)
> >   * IPC.
> >   */
> >  
> > -IPAManager *IPAManager::self_ = nullptr;
> > -
> >  /**
> >   * \brief Construct an IPAManager instance
> >   *
> > @@ -105,10 +103,6 @@ IPAManager *IPAManager::self_ = nullptr;
> >   */
> >  IPAManager::IPAManager()
> >  {
> > -       if (self_)
> > -               LOG(IPAManager, Fatal)
> > -                       << "Multiple IPAManager objects are not allowed";
> > -
> >  #if HAVE_IPA_PUBKEY
> >         if (!pubKey_.isValid())
> >                 LOG(IPAManager, Warning) << "Public key not valid";
> > @@ -153,16 +147,12 @@ IPAManager::IPAManager()
> >         if (!ipaCount)
> >                 LOG(IPAManager, Warning)
> >                         << "No IPA found in '" IPA_MODULE_DIR "'";
> > -
> > -       self_ = this;
> >  }
> >  
> >  IPAManager::~IPAManager()
> >  {
> >         for (IPAModule *module : modules_)
> >                 delete module;
> > -
> > -       self_ = nullptr;
> >  }
> >  
> >  /**
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 5ea2ca780b63..5a6de685b292 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -719,6 +719,13 @@ void PipelineHandler::disconnect()
> >   * \return The pipeline handler name
> >   */
> >  
> > +/**
> > + * \fn PipelineHandler::cameraManager() const
> > + * \brief Retrieve the CameraManager that this pipeline handler belongs to
> > + * \context This function is \threadsafe.
> > + * \return The CameraManager for this pipeline handler
> > + */
> > +
> >  /**
> >   * \class PipelineHandlerFactoryBase
> >   * \brief Base class for pipeline handler factories
> > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> > index e840f6ab17c4..b81783664977 100644
> > --- a/test/ipa/ipa_interface_test.cpp
> > +++ b/test/ipa/ipa_interface_test.cpp
> > @@ -20,11 +20,11 @@
> >  #include <libcamera/base/thread.h>
> >  #include <libcamera/base/timer.h>
> >  
> > +#include "libcamera/internal/camera_manager.h"
> >  #include "libcamera/internal/device_enumerator.h"
> >  #include "libcamera/internal/ipa_manager.h"
> >  #include "libcamera/internal/ipa_module.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> > -#include "libcamera/internal/process.h"
> >  
> >  #include "test.h"
> >  
> > @@ -44,20 +44,20 @@ public:
> >         {
> >                 delete notifier_;
> >                 ipa_.reset();
> > -               ipaManager_.reset();
> > +               cameraManager_.reset();
> >         }
> >  
> >  protected:
> >         int init() override
> >         {
> > -               ipaManager_ = make_unique<IPAManager>();
> > +               cameraManager_ = make_unique<CameraManager>();
> >  
> >                 /* Create a pipeline handler for vimc. */
> >                 const std::vector<PipelineHandlerFactoryBase *> &factories =
> >                         PipelineHandlerFactoryBase::factories();
> >                 for (const PipelineHandlerFactoryBase *factory : factories) {
> >                         if (factory->name() == "vimc") {
> > -                               pipe_ = factory->create(nullptr);
> > +                               pipe_ = factory->create(cameraManager_.get());
> >                                 break;
> >                         }
> >                 }
> > @@ -171,11 +171,9 @@ private:
> >                 }
> >         }
> >  
> > -       ProcessManager processManager_;
> > -
> >         std::shared_ptr<PipelineHandler> pipe_;
> >         std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> > -       std::unique_ptr<IPAManager> ipaManager_;
> > +       std::unique_ptr<CameraManager> cameraManager_;
> >         enum ipa::vimc::IPAOperationCode trace_;
> >         EventNotifier *notifier_;
> >         int fd_;
> > 
> > base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
index af9ed60a0353..e098cb69aa43 100644
--- a/include/libcamera/internal/camera_manager.h
+++ b/include/libcamera/internal/camera_manager.h
@@ -19,13 +19,14 @@ 
 #include <libcamera/base/thread.h>
 #include <libcamera/base/thread_annotations.h>
 
-#include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/process.h"
 
 namespace libcamera {
 
 class Camera;
 class DeviceEnumerator;
+class IPAManager;
+class PipelineHandlerFactoryBase;
 
 class CameraManager::Private : public Extensible::Private, public Thread
 {
@@ -38,6 +39,8 @@  public:
 	void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
 	void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
 
+	IPAManager *ipaManager() const { return ipaManager_.get(); }
+
 protected:
 	void run() override;
 
@@ -62,7 +65,7 @@  private:
 
 	std::unique_ptr<DeviceEnumerator> enumerator_;
 
-	IPAManager ipaManager_;
+	std::unique_ptr<IPAManager> ipaManager_;
 	ProcessManager processManager_;
 };
 
diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index c6f74e11c434..16dede0c5a7e 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -15,6 +15,7 @@ 
 #include <libcamera/ipa/ipa_interface.h>
 #include <libcamera/ipa/ipa_module_info.h>
 
+#include "libcamera/internal/camera_manager.h"
 #include "libcamera/internal/ipa_module.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/pub_key.h"
@@ -34,11 +35,13 @@  public:
 					    uint32_t minVersion,
 					    uint32_t maxVersion)
 	{
-		IPAModule *m = self_->module(pipe, minVersion, maxVersion);
+		CameraManager *cm = pipe->cameraManager();
+		IPAManager *self = cm->_d()->ipaManager();
+		IPAModule *m = self->module(pipe, minVersion, maxVersion);
 		if (!m)
 			return nullptr;
 
-		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self_->isSignatureValid(m));
+		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
 		if (!proxy->isValid()) {
 			LOG(IPAManager, Error) << "Failed to load proxy";
 			return nullptr;
@@ -55,8 +58,6 @@  public:
 #endif
 
 private:
-	static IPAManager *self_;
-
 	void parseDir(const char *libDir, unsigned int maxDepth,
 		      std::vector<std::string> &files);
 	unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
index 746a34f8e7bd..cad5812f640c 100644
--- a/include/libcamera/internal/pipeline_handler.h
+++ b/include/libcamera/internal/pipeline_handler.h
@@ -70,6 +70,8 @@  public:
 
 	const char *name() const { return name_; }
 
+	CameraManager *cameraManager() const { return manager_; }
+
 protected:
 	void registerCamera(std::shared_ptr<Camera> camera);
 	void hotplugMediaDevice(MediaDevice *media);
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 95a9e3264526..31760a8680cc 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -15,6 +15,7 @@ 
 
 #include "libcamera/internal/camera.h"
 #include "libcamera/internal/device_enumerator.h"
+#include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/pipeline_handler.h"
 
 /**
@@ -37,6 +38,7 @@  LOG_DEFINE_CATEGORY(Camera)
 CameraManager::Private::Private()
 	: initialized_(false)
 {
+	ipaManager_ = std::make_unique<IPAManager>();
 }
 
 int CameraManager::Private::start()
@@ -249,6 +251,13 @@  void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
 	o->cameraRemoved.emit(camera);
 }
 
+/**
+ * \fn CameraManager::Private::ipaManager() const
+ * \brief Retrieve the IPAManager
+ * \context This function is \threadsafe.
+ * \return The IPAManager for this CameraManager
+ */
+
 /**
  * \class CameraManager
  * \brief Provide access and manage all cameras in the system
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index f4e0b6339f08..cfc24d389c4f 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -95,8 +95,6 @@  LOG_DEFINE_CATEGORY(IPAManager)
  * IPC.
  */
 
-IPAManager *IPAManager::self_ = nullptr;
-
 /**
  * \brief Construct an IPAManager instance
  *
@@ -105,10 +103,6 @@  IPAManager *IPAManager::self_ = nullptr;
  */
 IPAManager::IPAManager()
 {
-	if (self_)
-		LOG(IPAManager, Fatal)
-			<< "Multiple IPAManager objects are not allowed";
-
 #if HAVE_IPA_PUBKEY
 	if (!pubKey_.isValid())
 		LOG(IPAManager, Warning) << "Public key not valid";
@@ -153,16 +147,12 @@  IPAManager::IPAManager()
 	if (!ipaCount)
 		LOG(IPAManager, Warning)
 			<< "No IPA found in '" IPA_MODULE_DIR "'";
-
-	self_ = this;
 }
 
 IPAManager::~IPAManager()
 {
 	for (IPAModule *module : modules_)
 		delete module;
-
-	self_ = nullptr;
 }
 
 /**
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index 5ea2ca780b63..5a6de685b292 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -719,6 +719,13 @@  void PipelineHandler::disconnect()
  * \return The pipeline handler name
  */
 
+/**
+ * \fn PipelineHandler::cameraManager() const
+ * \brief Retrieve the CameraManager that this pipeline handler belongs to
+ * \context This function is \threadsafe.
+ * \return The CameraManager for this pipeline handler
+ */
+
 /**
  * \class PipelineHandlerFactoryBase
  * \brief Base class for pipeline handler factories
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
index e840f6ab17c4..b81783664977 100644
--- a/test/ipa/ipa_interface_test.cpp
+++ b/test/ipa/ipa_interface_test.cpp
@@ -20,11 +20,11 @@ 
 #include <libcamera/base/thread.h>
 #include <libcamera/base/timer.h>
 
+#include "libcamera/internal/camera_manager.h"
 #include "libcamera/internal/device_enumerator.h"
 #include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/ipa_module.h"
 #include "libcamera/internal/pipeline_handler.h"
-#include "libcamera/internal/process.h"
 
 #include "test.h"
 
@@ -44,20 +44,20 @@  public:
 	{
 		delete notifier_;
 		ipa_.reset();
-		ipaManager_.reset();
+		cameraManager_.reset();
 	}
 
 protected:
 	int init() override
 	{
-		ipaManager_ = make_unique<IPAManager>();
+		cameraManager_ = make_unique<CameraManager>();
 
 		/* Create a pipeline handler for vimc. */
 		const std::vector<PipelineHandlerFactoryBase *> &factories =
 			PipelineHandlerFactoryBase::factories();
 		for (const PipelineHandlerFactoryBase *factory : factories) {
 			if (factory->name() == "vimc") {
-				pipe_ = factory->create(nullptr);
+				pipe_ = factory->create(cameraManager_.get());
 				break;
 			}
 		}
@@ -171,11 +171,9 @@  private:
 		}
 	}
 
-	ProcessManager processManager_;
-
 	std::shared_ptr<PipelineHandler> pipe_;
 	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
-	std::unique_ptr<IPAManager> ipaManager_;
+	std::unique_ptr<CameraManager> cameraManager_;
 	enum ipa::vimc::IPAOperationCode trace_;
 	EventNotifier *notifier_;
 	int fd_;