[libcamera-devel] [PATCH v2 22/24] ipa: Allow short-circuiting the ipa_context_ops
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 19 23:42:50 CET 2019
Hi Kieran,
On Tue, Nov 19, 2019 at 05:00:26PM +0000, Kieran Bingham wrote:
> On 08/11/2019 20:54, Laurent Pinchart wrote:
> > When an IPA module is loaded without isolation and implements the
> > IPAInterface internally, going through ipa_context_ops is a waste of
> > time. Add an operation to retrieve the IPAInterface, and use it directly
> > in the IPAContextWrapper.
> >
> > For debugging purpose, make it possible to forcing usage of the C API by
> > defining the LIBCAMERA_IPA_FORCE_C_API environment variable.
>
> Shouldn't the LIBCAMERA_IPA_FORCE_C_API be documented somewhere, as it
> is effectively exposed as a user-space interface to libcamera?
>
> We're also missing documentation for the LIBCAMERA_LOG_LEVELS etc, so
> perhaps we need some dedicated section documenting the available
> environment variables that may have an effect on the operation of libcamera.
I agree with you. To avoid delaying this series too much due to yack
shaving, I've added a task to document all the environment variables. Is
that OK with you ?
Ideally I'd like to document the environment variables in the source
file where they're used, but I think they should also be centralized on
a single page. I'm not sure if Doxygen can support this, I'll try to
have a look. This also brings up the question of higher-level libcamera
usage documentation, we need to figure out where to put it and how to
expose it.
> Otherwise, this is certainly useful.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > 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 | 4 +++
> > src/libcamera/ipa_context_wrapper.cpp | 34 +++++++++++++++++++--
> > src/libcamera/ipa_interface.cpp | 18 +++++++++++
> > 6 files changed, 64 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index 92f1aac50b85..0ea53d120fe1 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -62,6 +62,7 @@ struct ipa_callback_ops {
> >
> > struct ipa_context_ops {
> > void (*destroy)(struct ipa_context *ctx);
> > + void *(*get_interface)(struct ipa_context *ctx);
> > void (*init)(struct ipa_context *ctx);
> > void (*register_callbacks)(struct ipa_context *ctx,
> > const struct ipa_callback_ops *callbacks,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index 80c5648ffed6..6a389dfa714a 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -72,6 +72,13 @@ void IPAInterfaceWrapper::destroy(struct ipa_context *_ctx)
> > delete ctx;
> > }
> >
> > +void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)
> > +{
> > + IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> > +
> > + return ctx->ipa_.get();
> > +}
> > +
> > void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
> > {
> > IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> > @@ -228,6 +235,7 @@ void IPAInterfaceWrapper::queueFrameAction(unsigned int frame,
> > */
> > const struct ipa_context_ops IPAInterfaceWrapper::operations_ = {
> > .destroy = &IPAInterfaceWrapper::destroy,
> > + .get_interface = &IPAInterfaceWrapper::get_interface,
> > .init = &IPAInterfaceWrapper::init,
> > .register_callbacks = &IPAInterfaceWrapper::register_callbacks,
> > .configure = &IPAInterfaceWrapper::configure,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> > index 17be2062b6c7..3fb7b4476439 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > @@ -22,6 +22,7 @@ public:
> >
> > private:
> > static void destroy(struct ipa_context *ctx);
> > + static void *get_interface(struct ipa_context *ctx);
> > static void init(struct ipa_context *ctx);
> > static void register_callbacks(struct ipa_context *ctx,
> > const struct ipa_callback_ops *callbacks,
> > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> > index 060888218838..c9e194120de6 100644
> > --- a/src/libcamera/include/ipa_context_wrapper.h
> > +++ b/src/libcamera/include/ipa_context_wrapper.h
> > @@ -33,7 +33,11 @@ private:
> > struct ipa_operation_data &data);
> > static const struct ipa_callback_ops callbacks_;
> >
> > + void doQueueFrameAction(unsigned int frame,
> > + const IPAOperationData &data);
> > +
> > struct ipa_context *ctx_;
> > + IPAInterface *intf_;
> >
> > ControlSerializer serializer_;
> > };
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > index 66fc59b82373..0efbae0f675f 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -12,6 +12,7 @@
> > #include <libcamera/controls.h>
> >
> > #include "byte_stream_buffer.h"
> > +#include "utils.h"
> >
> > /**
> > * \file ipa_context_wrapper.h
> > @@ -43,11 +44,19 @@ namespace libcamera {
> > * with it.
> > */
> > IPAContextWrapper::IPAContextWrapper(struct ipa_context *context)
> > - : ctx_(context)
> > + : ctx_(context), intf_(nullptr)
> > {
> > if (!ctx_)
> > return;
> >
> > + bool forceCApi = !!utils::secure_getenv("LIBCAMERA_IPA_FORCE_C_API");
> > +
> > + if (!forceCApi && ctx_ && ctx_->ops->get_interface) {
> > + intf_ = reinterpret_cast<IPAInterface *>(ctx_->ops->get_interface(ctx_));
> > + intf_->queueFrameAction.connect(this, &IPAContextWrapper::doQueueFrameAction);
> > + return;
> > + }
> > +
> > ctx_->ops->register_callbacks(ctx_, &IPAContextWrapper::callbacks_,
> > this);
> > }
> > @@ -60,6 +69,9 @@ IPAContextWrapper::~IPAContextWrapper()
> >
> > int IPAContextWrapper::init()
> > {
> > + if (intf_)
> > + return intf_->init();
> > +
> > if (!ctx_)
> > return 0;
> >
> > @@ -71,6 +83,9 @@ int IPAContextWrapper::init()
> > void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > {
> > + if (intf_)
> > + return intf_->configure(streamConfig, entityControls);
> > +
> > if (!ctx_)
> > return;
> >
> > @@ -121,6 +136,9 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea
> >
> > void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
> > {
> > + if (intf_)
> > + return intf_->mapBuffers(buffers);
> > +
> > if (!ctx_)
> > return;
> >
> > @@ -146,6 +164,9 @@ void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
> >
> > void IPAContextWrapper::unmapBuffers(const std::vector<unsigned int> &ids)
> > {
> > + if (intf_)
> > + return intf_->unmapBuffers(ids);
> > +
> > if (!ctx_)
> > return;
> >
> > @@ -154,6 +175,9 @@ void IPAContextWrapper::unmapBuffers(const std::vector<unsigned int> &ids)
> >
> > void IPAContextWrapper::processEvent(const IPAOperationData &data)
> > {
> > + if (intf_)
> > + return intf_->processEvent(data);
> > +
> > if (!ctx_)
> > return;
> >
> > @@ -187,6 +211,12 @@ void IPAContextWrapper::processEvent(const IPAOperationData &data)
> > ctx_->ops->process_event(ctx_, &c_data);
> > }
> >
> > +void IPAContextWrapper::doQueueFrameAction(unsigned int frame,
> > + const IPAOperationData &data)
> > +{
> > + IPAInterface::queueFrameAction.emit(frame, data);
> > +}
> > +
> > void IPAContextWrapper::queue_frame_action(void *ctx, unsigned int frame,
> > struct ipa_operation_data &data)
> > {
> > @@ -203,7 +233,7 @@ void IPAContextWrapper::queue_frame_action(void *ctx, unsigned int frame,
> > opData.controls.push_back(_this->serializer_.deserialize<ControlList>(b));
> > }
> >
> > - _this->queueFrameAction.emit(frame, opData);
> > + _this->doQueueFrameAction(frame, opData);
> > }
> >
> > #ifndef __DOXYGEN__
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index cb2767a04711..715e7972271b 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -62,6 +62,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_context_ops and back to IPAInterface is desirable. To support this, IPA
> > + * modules may implement the ipa_context_ops::get_interface function to return a
> > + * pointer to their internal IPAInterface.
> > */
> >
> > /**
> > @@ -209,6 +215,18 @@
> > * \param[in] ctx The IPA context
> > */
> >
> > +/**
> > + * \var ipa_context_ops::get_interface
> > + * \brief Retrieve the IPAInterface implemented by the ipa_context (optional)
> > + * \param[in] ctx The IPA context
> > + *
> > + * IPA modules may implement this function to expose their internal
> > + * IPAInterface, if any. When implemented, libcamera may at its sole discretion
> > + * call it and then bypass the ipa_context_ops API by calling the IPAInterface
> > + * methods directly. IPA modules shall still implement and support the full
> > + * ipa_context_ops API.
> > + */
> > +
> > /**
> > * \var ipa_context_ops::init
> > * \brief Initialise the IPA context
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list