[libcamera-devel] [PATCH] ipa: Allow short-circuiting the ipa_operations
Jacopo Mondi
jacopo at jmondi.org
Sat Sep 28 16:44:22 CEST 2019
Hi Laurent,
On Sat, Sep 28, 2019 at 07:17:34AM +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>
> ---
> 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_)
I tried thnking of a way to avoid doinig this switch every operation.
For init is easy, but intf_->op() and ctx_->ops->ops() will differ in
the argument they take, as in the first case we can skip
serialization, so I see not many ways around this...
> 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
and back ?
> + * 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
I would mention we save translating to C APIs. Or maybe it's implied.
In any case:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> + * 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190928/4a1b72ab/attachment.sig>
More information about the libcamera-devel
mailing list