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

[Laurent Pinchart]

Hi Jacopo,

On Sat, Sep 28, 2019 at 04:44:22PM +0200, Jacopo Mondi wrote:
> 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 ?

"Back" is for the IPA module side, going from the C API back to the C++ API.

> > + * 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.

I think that's implied by "bypass the ipa_operations API".

> In any case:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > + * ipa_operations API.
> >   */
> >
> >  namespace libcamera {

-- 
Regards,

Laurent Pinchart