[libcamera-devel] [PATCH v1.1 07/11] ipa: Pass IPA initialization settings to IPAInterface::init()
Jacopo Mondi
jacopo at jmondi.org
Mon Apr 27 21:25:22 CEST 2020
Hi Laurent,
On Mon, Apr 27, 2020 at 02:12:25PM +0300, Laurent Pinchart wrote:
> Add a new IPASettings class to pass IPA initialization settings through
> the IPAInterface::init() method. The settings currently only contain the
> name of a configuration file, and are expected to be extended later.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v1:
>
> - Drop explicit std::string constructor
> - Expand IPASettings documentation
> ---
> include/ipa/ipa_interface.h | 13 ++++++--
> src/ipa/libipa/ipa_interface_wrapper.cpp | 8 +++--
> src/ipa/libipa/ipa_interface_wrapper.h | 3 +-
> src/ipa/rkisp1/rkisp1.cpp | 2 +-
> src/ipa/vimc/vimc.cpp | 4 +--
> src/libcamera/include/ipa_context_wrapper.h | 2 +-
> src/libcamera/ipa_context_wrapper.cpp | 9 ++++--
> src/libcamera/ipa_interface.cpp | 34 +++++++++++++++++++++
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
> src/libcamera/proxy/ipa_proxy_linux.cpp | 2 +-
> src/libcamera/proxy/ipa_proxy_thread.cpp | 6 ++--
> test/ipa/ipa_interface_test.cpp | 3 +-
> test/ipa/ipa_wrappers_test.cpp | 13 ++++++--
> 14 files changed, 82 insertions(+), 21 deletions(-)
>
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index e65844bc7b34..ef3d6507b692 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -18,6 +18,10 @@ struct ipa_context {
> const struct ipa_context_ops *ops;
> };
>
> +struct ipa_settings {
> + const char *configuration_file;
> +};
> +
> struct ipa_stream {
> unsigned int id;
> unsigned int pixel_format;
> @@ -63,7 +67,8 @@ 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 (*init)(struct ipa_context *ctx,
> + const struct ipa_settings *settings);
> int (*start)(struct ipa_context *ctx);
> void (*stop)(struct ipa_context *ctx);
> void (*register_callbacks)(struct ipa_context *ctx,
> @@ -100,6 +105,10 @@ struct ipa_context *ipaCreate();
>
> namespace libcamera {
>
> +struct IPASettings {
> + std::string configurationFile;
> +};
> +
> struct IPAStream {
> unsigned int pixelFormat;
> Size size;
> @@ -121,7 +130,7 @@ class IPAInterface
> public:
> virtual ~IPAInterface() {}
>
> - virtual int init() = 0;
> + virtual int init(const IPASettings &settings) = 0;
> virtual int start() = 0;
> virtual void stop() = 0;
>
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index f50f93a0185a..9596cb20cd66 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -79,11 +79,15 @@ void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)
> return ctx->ipa_.get();
> }
>
> -void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
> +void IPAInterfaceWrapper::init(struct ipa_context *_ctx,
> + const struct ipa_settings *settings)
> {
> IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
>
> - ctx->ipa_->init();
> + IPASettings ipaSettings{
> + .configurationFile = settings->configuration_file
> + };
> + ctx->ipa_->init(ipaSettings);
> }
>
> int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> index e4bc6dd4535d..78ccf0f59d42 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -23,7 +23,8 @@ 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 init(struct ipa_context *ctx,
> + const struct ipa_settings *settings);
> static int start(struct ipa_context *ctx);
> static void stop(struct ipa_context *ctx);
> static void register_callbacks(struct ipa_context *ctx,
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 7f0ffb0a791f..9a347e527cd2 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -32,7 +32,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)
> class IPARkISP1 : public IPAInterface
> {
> public:
> - int init() override { return 0; }
> + int init(const IPASettings &settings) override { return 0; }
> int start() override { return 0; }
> void stop() override {}
>
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cfdbd6f99155..e6bda8ec58b0 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -31,7 +31,7 @@ public:
> IPAVimc();
> ~IPAVimc();
>
> - int init() override;
> + int init(const IPASettings &settings) override;
>
> int start() override;
> void stop() override;
> @@ -61,7 +61,7 @@ IPAVimc::~IPAVimc()
> ::close(fd_);
> }
>
> -int IPAVimc::init()
> +int IPAVimc::init(const IPASettings &settings)
> {
> trace(IPAOperationInit);
>
> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> index 0a48bfe732c8..64395b4a450b 100644
> --- a/src/libcamera/include/ipa_context_wrapper.h
> +++ b/src/libcamera/include/ipa_context_wrapper.h
> @@ -19,7 +19,7 @@ public:
> IPAContextWrapper(struct ipa_context *context);
> ~IPAContextWrapper();
>
> - int init() override;
> + int init(const IPASettings &settings) override;
> int start() override;
> void stop() override;
> void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index ab6ce396b88a..5bd5d916ccc0 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -69,15 +69,18 @@ IPAContextWrapper::~IPAContextWrapper()
> ctx_->ops->destroy(ctx_);
> }
>
> -int IPAContextWrapper::init()
> +int IPAContextWrapper::init(const IPASettings &settings)
> {
> if (intf_)
> - return intf_->init();
> + return intf_->init(settings);
>
> if (!ctx_)
> return 0;
>
> - ctx_->ops->init(ctx_);
> + struct ipa_settings c_settings;
> + c_settings.configuration_file = settings.configurationFile.c_str();
> +
> + ctx_->ops->init(ctx_, &c_settings);
>
> return 0;
> }
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 890d4340e4f3..55ea6140220a 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -92,6 +92,16 @@
> * \brief The IPA context operations
> */
>
> +/**
> + * \struct ipa_settings
> + * \brief IPA initialization settings for the IPA context operations
> + * \sa IPASettings
> + *
> + * \var ipa_settings::configuration_file
> + * \brief The name of the IPA configuration file (may be null or point to an
> + * empty string)
> + */
> +
> /**
> * \struct ipa_stream
> * \brief Stream information for the IPA context operations
> @@ -231,6 +241,7 @@
> * \var ipa_context_ops::init
> * \brief Initialise the IPA context
> * \param[in] ctx The IPA context
> + * \param[in] settings The IPA initialization settings
> *
> * \sa libcamera::IPAInterface::init()
> */
> @@ -310,6 +321,24 @@
>
> namespace libcamera {
>
> +/**
> + * \struct IPASettings
> + * \brief IPA interface initialization settings
> + *
> + * The IPASettings structure stores data passed to the IPAInterface::init()
> + * function. The data contain settings that don't depend on a particular camera
data contains ? or contain ?
> + * or pipeline configuration and are valid for the whole life time of the IPA
> + * interface.
> + */
> +
> +/**
> + * \var IPASettings::configurationFile
> + * \brief The name of the IPA configuration file
> + *
> + * This field may be an empty string if the IPA doesn't require a configuration
> + * file.
> + */
> +
> /**
> * \struct IPAStream
> * \brief Stream configuration for the IPA interface
> @@ -424,6 +453,11 @@ namespace libcamera {
> /**
> * \fn IPAInterface::init()
> * \brief Initialise the IPAInterface
> + * \param[in] settings The IPA initialization settings
> + *
> + * This function initializes the IPA interface. It shall be called before any
> + * other function of the IPAInterface. The \a settings carry initialization
> + * parameters that will remain valid for the whole life time of the interface.
s/wil remain/are/
s/interface/IPA interface
to match the above documentation of IPASettings
>
> /**
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index f42264361785..fde445b99a46 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -401,7 +401,7 @@ int RkISP1CameraData::loadIPA()
> ipa_->queueFrameAction.connect(this,
> &RkISP1CameraData::queueFrameAction);
>
> - ipa_->init();
> + ipa_->init(IPASettings{});
>
> return 0;
> }
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index c5eea3a01b0e..699f788aa1b8 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -374,7 +374,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> if (data->ipa_ == nullptr)
> LOG(VIMC, Warning) << "no matching IPA found";
> else
> - data->ipa_->init();
> + data->ipa_->init(IPASettings{});
>
> /* Locate and open the capture video node. */
> if (data->init(media))
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index 89f5cb54687b..cd8ac824679d 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -26,7 +26,7 @@ public:
> IPAProxyLinux(IPAModule *ipam);
> ~IPAProxyLinux();
>
> - int init() override { return 0; }
> + int init(const IPASettings &settings) override { return 0; }
> int start() override { return 0; }
> void stop() override {}
> void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index 1ebb9b6a6c9d..be82fde34bd9 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -25,7 +25,7 @@ class IPAProxyThread : public IPAProxy, public Object
> public:
> IPAProxyThread(IPAModule *ipam);
>
> - int init() override;
> + int init(const IPASettings &settings) override;
> int start() override;
> void stop() override;
>
> @@ -97,9 +97,9 @@ IPAProxyThread::IPAProxyThread(IPAModule *ipam)
> valid_ = true;
> }
>
> -int IPAProxyThread::init()
> +int IPAProxyThread::init(const IPASettings &settings)
> {
> - int ret = ipa_->init();
> + int ret = ipa_->init(settings);
> if (ret)
> return ret;
>
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 22f9ca41ef37..2e2dfb8d1ebd 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -98,7 +98,8 @@ protected:
> }
>
> /* Test initialization of IPA module. */
> - ipa_->init();
> + IPASettings settings;
> + ipa_->init(settings);
> timer.start(1000);
> while (timer.isRunning() && trace_ != IPAOperationInit)
> dispatcher->processEvents();
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index fae1d56b67c7..21bf51a2e046 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -43,8 +43,14 @@ public:
> {
> }
>
> - int init() override
> + int init(const IPASettings &settings) override
> {
> + if (settings.configurationFile != "/ipa/configuration/file") {
> + cerr << "init(): Invalid configuration file" << endl;
> + report(Op_init, TestFail);
> + return 0;
While, as you explained, this is correct, it doesn't match the style
of all other failure conditions in the test which 'return report(...)'
Up to you.
> + }
> +
> report(Op_init, TestPass);
> return 0;
> }
> @@ -339,7 +345,10 @@ protected:
> * Test init(), start() and stop() last to ensure nothing in the
> * wrappers or serializer depends on them being called first.
> */
> - ret = INVOKE(init);
> + IPASettings settings{
> + .configurationFile = "/ipa/configuration/file"
> + };
> + ret = INVOKE(init, settings);
> if (ret == TestFail) {
> cerr << "Failed to run init()";
> return TestFail;
Nits apart
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list