[libcamera-devel,v4,16/37] libcamera: IPAProxy: Add isolate parameter to create()
diff mbox series

Message ID 20201106103707.49660-17-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • IPA isolation implementation
Related show

Commit Message

Paul Elder Nov. 6, 2020, 10:36 a.m. UTC 
Since IPAProxy implementations now always encapsulate IPA modules, add a
parameter to create() to signal if the proxy should isolate the IPA or not.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

---
No change in v4

No change in v3

Changes in v2:
- document isolate argument
---
 include/libcamera/internal/ipa_proxy.h | 22 +++++++++++-----------
 src/libcamera/ipa_proxy.cpp            |  4 +++-
 2 files changed, 14 insertions(+), 12 deletions(-)

Comments

Jacopo Mondi Nov. 18, 2020, 1:11 p.m. UTC | #1 
Hi Paul,

On Fri, Nov 06, 2020 at 07:36:46PM +0900, Paul Elder wrote:
> Since IPAProxy implementations now always encapsulate IPA modules, add a
> parameter to create() to signal if the proxy should isolate the IPA or not.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
> No change in v4
>
> No change in v3
>
> Changes in v2:
> - document isolate argument
> ---
>  include/libcamera/internal/ipa_proxy.h | 22 +++++++++++-----------
>  src/libcamera/ipa_proxy.cpp            |  4 +++-
>  2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index 59a5b841..195d2cb4 100644
> --- a/include/libcamera/internal/ipa_proxy.h
> +++ b/include/libcamera/internal/ipa_proxy.h
> @@ -42,7 +42,7 @@ public:
>  	IPAProxyFactory(const char *name);
>  	virtual ~IPAProxyFactory() = default;
>
> -	virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam) = 0;
> +	virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0;
>
>  	const std::string &name() const { return name_; }
>
> @@ -53,16 +53,16 @@ private:
>  	std::string name_;
>  };
>
> -#define REGISTER_IPA_PROXY(proxy)			\
> -class proxy##Factory final : public IPAProxyFactory	\
> -{							\
> -public:							\
> -	proxy##Factory() : IPAProxyFactory(#proxy) {}	\
> -	std::unique_ptr<IPAProxy> create(IPAModule *ipam)	\
> -	{						\
> -		return std::make_unique<proxy>(ipam);	\
> -	}						\
> -};							\
> +#define REGISTER_IPA_PROXY(proxy)					\
> +class proxy##Factory final : public IPAProxyFactory			\
> +{									\
> +public:									\
> +	proxy##Factory() : IPAProxyFactory(#proxy) {}			\
> +	std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate)	\
> +	{								\
> +		return std::make_unique<proxy>(ipam, isolate);		\
> +	}								\
> +};									\
>  static proxy##Factory global_##proxy##Factory;
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 23be24ad..ee5e6f3e 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -250,10 +250,12 @@ IPAProxyFactory::IPAProxyFactory(const char *name)
>   * \fn IPAProxyFactory::create()
>   * \brief Create an instance of the IPAProxy corresponding to the factory
>   * \param[in] ipam The IPA module
> + * \param[in] isolate Flag to isolate the IPA module
>   *
>   * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.
>   * It creates a IPAProxy instance that isolates an IPA interface designated
> - * by the IPA module \a ipam.
> + * by the IPA module \a ipam. If \a isolate is true, then the IPA module will
> + * be isolated.
>   *
>   * \return A pointer to a newly constructed instance of the IPAProxy subclass
>   * corresponding to the factory
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Nov. 25, 2020, 2:54 p.m. UTC | #2 
Hi Paul,

Thank you for the patch.

On Fri, Nov 06, 2020 at 07:36:46PM +0900, Paul Elder wrote:
> Since IPAProxy implementations now always encapsulate IPA modules, add a
> parameter to create() to signal if the proxy should isolate the IPA or not.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> ---
> No change in v4
> 
> No change in v3
> 
> Changes in v2:
> - document isolate argument
> ---
>  include/libcamera/internal/ipa_proxy.h | 22 +++++++++++-----------
>  src/libcamera/ipa_proxy.cpp            |  4 +++-
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index 59a5b841..195d2cb4 100644
> --- a/include/libcamera/internal/ipa_proxy.h
> +++ b/include/libcamera/internal/ipa_proxy.h
> @@ -42,7 +42,7 @@ public:
>  	IPAProxyFactory(const char *name);
>  	virtual ~IPAProxyFactory() = default;
>  
> -	virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam) = 0;
> +	virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0;
>  
>  	const std::string &name() const { return name_; }
>  
> @@ -53,16 +53,16 @@ private:
>  	std::string name_;
>  };
>  
> -#define REGISTER_IPA_PROXY(proxy)			\
> -class proxy##Factory final : public IPAProxyFactory	\
> -{							\
> -public:							\
> -	proxy##Factory() : IPAProxyFactory(#proxy) {}	\
> -	std::unique_ptr<IPAProxy> create(IPAModule *ipam)	\
> -	{						\
> -		return std::make_unique<proxy>(ipam);	\
> -	}						\
> -};							\
> +#define REGISTER_IPA_PROXY(proxy)					\
> +class proxy##Factory final : public IPAProxyFactory			\
> +{									\
> +public:									\
> +	proxy##Factory() : IPAProxyFactory(#proxy) {}			\
> +	std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate)	\
> +	{								\
> +		return std::make_unique<proxy>(ipam, isolate);		\
> +	}								\
> +};									\
>  static proxy##Factory global_##proxy##Factory;
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 23be24ad..ee5e6f3e 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -250,10 +250,12 @@ IPAProxyFactory::IPAProxyFactory(const char *name)
>   * \fn IPAProxyFactory::create()
>   * \brief Create an instance of the IPAProxy corresponding to the factory
>   * \param[in] ipam The IPA module
> + * \param[in] isolate Flag to isolate the IPA module
>   *
>   * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.
>   * It creates a IPAProxy instance that isolates an IPA interface designated
> - * by the IPA module \a ipam.
> + * by the IPA module \a ipam. If \a isolate is true, then the IPA module will
> + * be isolated.

Reading the whole paragraph, isolation is a bit confusing. How about the following ?

 * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.
 * It creates a IPAProxy instance that encapsulates the IPA interface provided
 * by the IPA module \a ipam. If \a isolate is true, the proxy isolates the IPA
 * interface in a separate process. Otherwise it runs it in a thread.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>   *
>   * \return A pointer to a newly constructed instance of the IPAProxy subclass
>   * corresponding to the factory

Patch
diff mbox series 

diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
index 59a5b841..195d2cb4 100644
--- a/include/libcamera/internal/ipa_proxy.h
+++ b/include/libcamera/internal/ipa_proxy.h
@@ -42,7 +42,7 @@  public:
 	IPAProxyFactory(const char *name);
 	virtual ~IPAProxyFactory() = default;
 
-	virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam) = 0;
+	virtual std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate) = 0;
 
 	const std::string &name() const { return name_; }
 
@@ -53,16 +53,16 @@  private:
 	std::string name_;
 };
 
-#define REGISTER_IPA_PROXY(proxy)			\
-class proxy##Factory final : public IPAProxyFactory	\
-{							\
-public:							\
-	proxy##Factory() : IPAProxyFactory(#proxy) {}	\
-	std::unique_ptr<IPAProxy> create(IPAModule *ipam)	\
-	{						\
-		return std::make_unique<proxy>(ipam);	\
-	}						\
-};							\
+#define REGISTER_IPA_PROXY(proxy)					\
+class proxy##Factory final : public IPAProxyFactory			\
+{									\
+public:									\
+	proxy##Factory() : IPAProxyFactory(#proxy) {}			\
+	std::unique_ptr<IPAProxy> create(IPAModule *ipam, bool isolate)	\
+	{								\
+		return std::make_unique<proxy>(ipam, isolate);		\
+	}								\
+};									\
 static proxy##Factory global_##proxy##Factory;
 
 } /* namespace libcamera */
diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
index 23be24ad..ee5e6f3e 100644
--- a/src/libcamera/ipa_proxy.cpp
+++ b/src/libcamera/ipa_proxy.cpp
@@ -250,10 +250,12 @@  IPAProxyFactory::IPAProxyFactory(const char *name)
  * \fn IPAProxyFactory::create()
  * \brief Create an instance of the IPAProxy corresponding to the factory
  * \param[in] ipam The IPA module
+ * \param[in] isolate Flag to isolate the IPA module
  *
  * This virtual function is implemented by the REGISTER_IPA_PROXY() macro.
  * It creates a IPAProxy instance that isolates an IPA interface designated
- * by the IPA module \a ipam.
+ * by the IPA module \a ipam. If \a isolate is true, then the IPA module will
+ * be isolated.
  *
  * \return A pointer to a newly constructed instance of the IPAProxy subclass
  * corresponding to the factory