[libcamera-devel] [PATCH] ipa: Allow short-circuiting the ipa_operations

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2019-09-28 07:17:34 +0300, Laurent Pinchart wrote:
> When an IPA module is loaded without isolation and implements the
> IPAInterface internally, going through ipa_operations is a waste of
> time. Add an operation to retrieve the IPAInterface, and use it directly
> in the IPAContextWrapper.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  include/ipa/ipa_interface.h                 |  1 +
>  src/ipa/libipa/ipa_interface_wrapper.cpp    |  8 ++++++++
>  src/ipa/libipa/ipa_interface_wrapper.h      |  1 +
>  src/libcamera/include/ipa_context_wrapper.h |  1 +
>  src/libcamera/ipa_context_wrapper.cpp       |  8 +++++++-
>  src/libcamera/ipa_interface.cpp             | 15 +++++++++++++++
>  6 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index a0dfce96fd1c..7c7771b97e30 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -18,6 +18,7 @@ struct ipa_context {
>  struct ipa_operations {
>  	void (*destroy)(struct ipa_context *ctx);
>  	void (*init)(struct ipa_context *ctx);
> +	void *(*get_interface)(struct ipa_context *ctx);
>  };
>  
>  #ifdef __cplusplus
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index 0a58b5e8f7e1..a39b699ad600 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -68,6 +68,13 @@ void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
>  	ctx->ipa->init();
>  }
>  
> +void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)
> +{
> +	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +
> +	return ctx->ipa;
> +}
> +
>  #ifndef __DOXYGEN__
>  /*
>   * This construct confuses Doygen and makes it believe that all members of the
> @@ -76,6 +83,7 @@ void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
>  const struct ipa_operations IPAInterfaceWrapper::operations = {
>  	.destroy = &IPAInterfaceWrapper::destroy,
>  	.init = &IPAInterfaceWrapper::init,
> +	.get_interface = &IPAInterfaceWrapper::get_interface,
>  };
>  #endif
>  
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> index 582d787d27ed..66aa387ac191 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -19,6 +19,7 @@ public:
>  private:
>  	static void destroy(struct ipa_context *ctx);
>  	static void init(struct ipa_context *ctx);
> +	static void *get_interface(struct ipa_context *ctx);
>  
>  	static const struct ipa_operations operations;
>  
> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> index 879911e8a566..928781429845 100644
> --- a/src/libcamera/include/ipa_context_wrapper.h
> +++ b/src/libcamera/include/ipa_context_wrapper.h
> @@ -21,6 +21,7 @@ public:
>  
>  private:
>  	struct ipa_context *ctx_;
> +	IPAInterface *intf_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index bdd3857731ef..edbfea612515 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -41,6 +41,10 @@ namespace libcamera {
>  IPAContextWrapper::IPAContextWrapper(struct ipa_context *context)
>  	: ctx_(context)
>  {
> +	if (ctx_ && ctx_->ops->get_interface)
> +		intf_ = reinterpret_cast<IPAInterface *>(ctx_->ops->get_interface(ctx_));
> +	else
> +		intf_ = nullptr;
>  }
>  
>  IPAContextWrapper::~IPAContextWrapper()
> @@ -51,7 +55,9 @@ IPAContextWrapper::~IPAContextWrapper()
>  
>  int IPAContextWrapper::init()
>  {
> -	if (ctx_)
> +	if (intf_)
> +		intf_->init();
> +	else if (ctx_)
>  		ctx_->ops->init(ctx_);
>  
>  	return 0;
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index e95ccecda019..38d284cea114 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -40,6 +40,12 @@
>   * handlers to communicate with IPA modules. IPA modules may use the
>   * IPAInterface API internally if they want to benefit from the data and helper
>   * classes offered by libcamera.
> + *
> + * When an IPA module is loaded directly into the libcamera process and uses
> + * the IPAInterface API internally, short-circuiting the path to the
> + * ipa_operations and back to IPAInterface is desirable. To support this, IPA
> + * modules may implement the ipa_operations::get_interface function to return a
> + * pointer to their internal IPAInterface.
>   */
>  
>  /**
> @@ -80,6 +86,15 @@
>   *
>   * \var ipa_operations::init
>   * \brief Initialise the ipa_context
> + *
> + * \var ipa_operations::get_interface
> + * \brief Retrieve the IPAInterface implemented by the ipa_context (optional)
> + *
> + * IPA modules may implement this operation to expose their internal
> + * IPAInterface, if any. When implemented, libcamera may at its sole discretion
> + * call it and then bypass the ipa_operations API by calling the IPAInterface
> + * methods directly. IPA modules shall still implement and support the full
> + * ipa_operations API.
>   */
>  
>  namespace libcamera {
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund