[{"id":4550,"web_url":"https://patchwork.libcamera.org/comment/4550/","msgid":"<20200427091303.qazewpeugyxbplol@uno.localdomain>","date":"2020-04-27T09:13:03","subject":"Re: [libcamera-devel] [PATCH 07/11] ipa: Pass IPA initialization\n\tsettings to IPAInterface::init()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Apr 27, 2020 at 06:17:09AM +0300, Laurent Pinchart wrote:\n> Add a new IPASettings class to pass IPA initialization settings through\n> the IPAInterface::init() method. The settings currently only contain the\n> name of a configuration file, and are expected to be extended later.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/ipa/ipa_interface.h                 | 13 +++++++--\n>  src/ipa/libipa/ipa_interface_wrapper.cpp    |  8 ++++--\n>  src/ipa/libipa/ipa_interface_wrapper.h      |  3 +-\n>  src/ipa/rkisp1/rkisp1.cpp                   |  2 +-\n>  src/ipa/vimc/vimc.cpp                       |  4 +--\n>  src/libcamera/include/ipa_context_wrapper.h |  2 +-\n>  src/libcamera/ipa_context_wrapper.cpp       |  9 ++++--\n>  src/libcamera/ipa_interface.cpp             | 32 +++++++++++++++++++++\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  2 +-\n>  src/libcamera/pipeline/vimc/vimc.cpp        |  2 +-\n>  src/libcamera/proxy/ipa_proxy_linux.cpp     |  2 +-\n>  src/libcamera/proxy/ipa_proxy_thread.cpp    |  6 ++--\n>  test/ipa/ipa_interface_test.cpp             |  3 +-\n>  test/ipa/ipa_wrappers_test.cpp              | 13 +++++++--\n>  14 files changed, 80 insertions(+), 21 deletions(-)\n>\n> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> index e65844bc7b34..ef3d6507b692 100644\n> --- a/include/ipa/ipa_interface.h\n> +++ b/include/ipa/ipa_interface.h\n> @@ -18,6 +18,10 @@ struct ipa_context {\n>  \tconst struct ipa_context_ops *ops;\n>  };\n>\n> +struct ipa_settings {\n> +\tconst char *configuration_file;\n> +};\n> +\n\nI've seen you suggesting multiple times to use a char * to transport\nstrings, here in the IPA operations and the kernel ioctls in example. I\nwonder, isn't this dangeours ? One can easily point this to some\nmemory that could vanish, and even if I recognize reserving a buffer\nwith a fixed length is limiting, it feels more safer to me...\n\n>  struct ipa_stream {\n>  \tunsigned int id;\n>  \tunsigned int pixel_format;\n> @@ -63,7 +67,8 @@ struct ipa_callback_ops {\n>  struct ipa_context_ops {\n>  \tvoid (*destroy)(struct ipa_context *ctx);\n>  \tvoid *(*get_interface)(struct ipa_context *ctx);\n> -\tvoid (*init)(struct ipa_context *ctx);\n> +\tvoid (*init)(struct ipa_context *ctx,\n> +\t\t     const struct ipa_settings *settings);\n>  \tint (*start)(struct ipa_context *ctx);\n>  \tvoid (*stop)(struct ipa_context *ctx);\n>  \tvoid (*register_callbacks)(struct ipa_context *ctx,\n> @@ -100,6 +105,10 @@ struct ipa_context *ipaCreate();\n>\n>  namespace libcamera {\n>\n> +struct IPASettings {\n> +\tstd::string configurationFile;\n> +};\n> +\n>  struct IPAStream {\n>  \tunsigned int pixelFormat;\n>  \tSize size;\n> @@ -121,7 +130,7 @@ class IPAInterface\n>  public:\n>  \tvirtual ~IPAInterface() {}\n>\n> -\tvirtual int init() = 0;\n> +\tvirtual int init(const IPASettings &settings) = 0;\n>  \tvirtual int start() = 0;\n>  \tvirtual void stop() = 0;\n>\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> index f50f93a0185a..7776d9881575 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -79,11 +79,15 @@ void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)\n>  \treturn ctx->ipa_.get();\n>  }\n>\n> -void IPAInterfaceWrapper::init(struct ipa_context *_ctx)\n> +void IPAInterfaceWrapper::init(struct ipa_context *_ctx,\n> +\t\t\t       const struct ipa_settings *settings)\n>  {\n>  \tIPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);\n>\n> -\tctx->ipa_->init();\n> +\tIPASettings ipaSettings{\n> +\t\t.configurationFile = std::string{ settings->configuration_file }\n\nCan't you just assign a char * to an std::string ?\n\n> +\t};\n> +\tctx->ipa_->init(ipaSettings);\n>  }\n>\n>  int IPAInterfaceWrapper::start(struct ipa_context *_ctx)\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> index e4bc6dd4535d..78ccf0f59d42 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> @@ -23,7 +23,8 @@ public:\n>  private:\n>  \tstatic void destroy(struct ipa_context *ctx);\n>  \tstatic void *get_interface(struct ipa_context *ctx);\n> -\tstatic void init(struct ipa_context *ctx);\n> +\tstatic void init(struct ipa_context *ctx,\n> +\t\t\t const struct ipa_settings *settings);\n>  \tstatic int start(struct ipa_context *ctx);\n>  \tstatic void stop(struct ipa_context *ctx);\n>  \tstatic void register_callbacks(struct ipa_context *ctx,\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 7f0ffb0a791f..9a347e527cd2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -32,7 +32,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)\n>  class IPARkISP1 : public IPAInterface\n>  {\n>  public:\n> -\tint init() override { return 0; }\n> +\tint init(const IPASettings &settings) override { return 0; }\n>  \tint start() override { return 0; }\n>  \tvoid stop() override {}\n>\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index cfdbd6f99155..e6bda8ec58b0 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -31,7 +31,7 @@ public:\n>  \tIPAVimc();\n>  \t~IPAVimc();\n>\n> -\tint init() override;\n> +\tint init(const IPASettings &settings) override;\n>\n>  \tint start() override;\n>  \tvoid stop() override;\n> @@ -61,7 +61,7 @@ IPAVimc::~IPAVimc()\n>  \t\t::close(fd_);\n>  }\n>\n> -int IPAVimc::init()\n> +int IPAVimc::init(const IPASettings &settings)\n>  {\n>  \ttrace(IPAOperationInit);\n>\n> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h\n> index 0a48bfe732c8..64395b4a450b 100644\n> --- a/src/libcamera/include/ipa_context_wrapper.h\n> +++ b/src/libcamera/include/ipa_context_wrapper.h\n> @@ -19,7 +19,7 @@ public:\n>  \tIPAContextWrapper(struct ipa_context *context);\n>  \t~IPAContextWrapper();\n>\n> -\tint init() override;\n> +\tint init(const IPASettings &settings) override;\n>  \tint start() override;\n>  \tvoid stop() override;\n>  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> index ab6ce396b88a..5bd5d916ccc0 100644\n> --- a/src/libcamera/ipa_context_wrapper.cpp\n> +++ b/src/libcamera/ipa_context_wrapper.cpp\n> @@ -69,15 +69,18 @@ IPAContextWrapper::~IPAContextWrapper()\n>  \tctx_->ops->destroy(ctx_);\n>  }\n>\n> -int IPAContextWrapper::init()\n> +int IPAContextWrapper::init(const IPASettings &settings)\n>  {\n>  \tif (intf_)\n> -\t\treturn intf_->init();\n> +\t\treturn intf_->init(settings);\n>\n>  \tif (!ctx_)\n>  \t\treturn 0;\n>\n> -\tctx_->ops->init(ctx_);\n> +\tstruct ipa_settings c_settings;\n> +\tc_settings.configuration_file = settings.configurationFile.c_str();\n> +\n> +\tctx_->ops->init(ctx_, &c_settings);\n>\n>  \treturn 0;\n>  }\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 890d4340e4f3..3949b7d6ea5d 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -92,6 +92,16 @@\n>   * \\brief The IPA context operations\n>   */\n>\n> +/**\n> + * \\struct ipa_settings\n> + * \\brief IPA initialization settings for the IPA context operations\n> + * \\sa IPASettings\n> + *\n> + * \\var ipa_settings::configuration_file\n> + * \\brief The name of the IPA configuration file (may be null or point to an\n> + * empty string)\n> + */\n> +\n>  /**\n>   * \\struct ipa_stream\n>   * \\brief Stream information for the IPA context operations\n> @@ -231,6 +241,7 @@\n>   * \\var ipa_context_ops::init\n>   * \\brief Initialise the IPA context\n>   * \\param[in] ctx The IPA context\n> + * \\param[in] settings The IPA initialization settings\n>   *\n>   * \\sa libcamera::IPAInterface::init()\n>   */\n> @@ -310,6 +321,22 @@\n>\n>  namespace libcamera {\n>\n> +/**\n> + * \\struct IPASettings\n> + * \\brief IPA interface initialization settings\n> + *\n> + * The IPASettings structure stores data passed to the IPAInterface::init()\n> + * function.\n\nIs it worth mentioning these data are meant not to depend upon run\ntime operations ?\n\n> + */\n> +\n> +/**\n> + * \\var IPASettings::configurationFile\n> + * \\brief The name of the IPA configuration file\n> + *\n> + * This field may be an empty string if the IPA doesn't require a configuration\n> + * file.\n> + */\n> +\n>  /**\n>   * \\struct IPAStream\n>   * \\brief Stream configuration for the IPA interface\n> @@ -424,6 +451,11 @@ namespace libcamera {\n>  /**\n>   * \\fn IPAInterface::init()\n>   * \\brief Initialise the IPAInterface\n> + * \\param[in] settings The IPA initialization settings\n> + *\n> + * This function initializes the IPA interface. It shall be called before any\n> + * other function of the IPAInterface. The \\a settings carry initialization\n> + * parameters that will remain valid for the whole life time of the interface.\n\nIt's here, but maybe it's worth up there too...\n\n>   */\n>\n>  /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index f42264361785..fde445b99a46 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -401,7 +401,7 @@ int RkISP1CameraData::loadIPA()\n>  \tipa_->queueFrameAction.connect(this,\n>  \t\t\t\t       &RkISP1CameraData::queueFrameAction);\n>\n> -\tipa_->init();\n> +\tipa_->init(IPASettings{});\n>\n>  \treturn 0;\n>  }\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index c5eea3a01b0e..699f788aa1b8 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -374,7 +374,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \tif (data->ipa_ == nullptr)\n>  \t\tLOG(VIMC, Warning) << \"no matching IPA found\";\n>  \telse\n> -\t\tdata->ipa_->init();\n> +\t\tdata->ipa_->init(IPASettings{});\n>\n>  \t/* Locate and open the capture video node. */\n>  \tif (data->init(media))\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index 89f5cb54687b..cd8ac824679d 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -26,7 +26,7 @@ public:\n>  \tIPAProxyLinux(IPAModule *ipam);\n>  \t~IPAProxyLinux();\n>\n> -\tint init() override { return 0; }\n> +\tint init(const IPASettings &settings) override { return 0; }\n>  \tint start() override { return 0; }\n>  \tvoid stop() override {}\n>  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> index 1ebb9b6a6c9d..be82fde34bd9 100644\n> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> @@ -25,7 +25,7 @@ class IPAProxyThread : public IPAProxy, public Object\n>  public:\n>  \tIPAProxyThread(IPAModule *ipam);\n>\n> -\tint init() override;\n> +\tint init(const IPASettings &settings) override;\n>  \tint start() override;\n>  \tvoid stop() override;\n>\n> @@ -97,9 +97,9 @@ IPAProxyThread::IPAProxyThread(IPAModule *ipam)\n>  \tvalid_ = true;\n>  }\n>\n> -int IPAProxyThread::init()\n> +int IPAProxyThread::init(const IPASettings &settings)\n>  {\n> -\tint ret = ipa_->init();\n> +\tint ret = ipa_->init(settings);\n>  \tif (ret)\n>  \t\treturn ret;\n>\n> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> index 22f9ca41ef37..2e2dfb8d1ebd 100644\n> --- a/test/ipa/ipa_interface_test.cpp\n> +++ b/test/ipa/ipa_interface_test.cpp\n> @@ -98,7 +98,8 @@ protected:\n>  \t\t}\n>\n>  \t\t/* Test initialization of IPA module. */\n> -\t\tipa_->init();\n> +\t\tIPASettings settings;\n> +\t\tipa_->init(settings);\n>  \t\ttimer.start(1000);\n>  \t\twhile (timer.isRunning() && trace_ != IPAOperationInit)\n>  \t\t\tdispatcher->processEvents();\n> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> index fae1d56b67c7..21bf51a2e046 100644\n> --- a/test/ipa/ipa_wrappers_test.cpp\n> +++ b/test/ipa/ipa_wrappers_test.cpp\n> @@ -43,8 +43,14 @@ public:\n>  \t{\n>  \t}\n>\n> -\tint init() override\n> +\tint init(const IPASettings &settings) override\n>  \t{\n> +\t\tif (settings.configurationFile != \"/ipa/configuration/file\") {\n> +\t\t\tcerr << \"init(): Invalid configuration file\" << endl;\n> +\t\t\treport(Op_init, TestFail);\n> +\t\t\treturn 0;\n\nShouldn't you 'return report(Op_init.. ' ?\n\n> +\t\t}\n> +\n>  \t\treport(Op_init, TestPass);\n>  \t\treturn 0;\n>  \t}\n> @@ -339,7 +345,10 @@ protected:\n>  \t\t * Test init(), start() and stop() last to ensure nothing in the\n>  \t\t * wrappers or serializer depends on them being called first.\n>  \t\t */\n> -\t\tret = INVOKE(init);\n> +\t\tIPASettings settings{\n> +\t\t\t.configurationFile = \"/ipa/configuration/file\"\n> +\t\t};\n> +\t\tret = INVOKE(init, settings);\n>  \t\tif (ret == TestFail) {\n>  \t\t\tcerr << \"Failed to run init()\";\n>  \t\t\treturn TestFail;\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8FF24603F8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Apr 2020 11:09:54 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 5336C100003;\n\tMon, 27 Apr 2020 09:09:53 +0000 (UTC)"],"Date":"Mon, 27 Apr 2020 11:13:03 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427091303.qazewpeugyxbplol@uno.localdomain>","References":"<20200427031713.14013-1-laurent.pinchart@ideasonboard.com>\n\t<20200427031713.14013-8-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200427031713.14013-8-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 07/11] ipa: Pass IPA initialization\n\tsettings to IPAInterface::init()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 27 Apr 2020 09:09:54 -0000"}},{"id":4565,"web_url":"https://patchwork.libcamera.org/comment/4565/","msgid":"<20200427111109.GA10040@pendragon.ideasonboard.com>","date":"2020-04-27T11:11:09","subject":"Re: [libcamera-devel] [PATCH 07/11] ipa: Pass IPA initialization\n\tsettings to IPAInterface::init()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 27, 2020 at 11:13:03AM +0200, Jacopo Mondi wrote:\n> On Mon, Apr 27, 2020 at 06:17:09AM +0300, Laurent Pinchart wrote:\n> > Add a new IPASettings class to pass IPA initialization settings through\n> > the IPAInterface::init() method. The settings currently only contain the\n> > name of a configuration file, and are expected to be extended later.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/ipa/ipa_interface.h                 | 13 +++++++--\n> >  src/ipa/libipa/ipa_interface_wrapper.cpp    |  8 ++++--\n> >  src/ipa/libipa/ipa_interface_wrapper.h      |  3 +-\n> >  src/ipa/rkisp1/rkisp1.cpp                   |  2 +-\n> >  src/ipa/vimc/vimc.cpp                       |  4 +--\n> >  src/libcamera/include/ipa_context_wrapper.h |  2 +-\n> >  src/libcamera/ipa_context_wrapper.cpp       |  9 ++++--\n> >  src/libcamera/ipa_interface.cpp             | 32 +++++++++++++++++++++\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  2 +-\n> >  src/libcamera/pipeline/vimc/vimc.cpp        |  2 +-\n> >  src/libcamera/proxy/ipa_proxy_linux.cpp     |  2 +-\n> >  src/libcamera/proxy/ipa_proxy_thread.cpp    |  6 ++--\n> >  test/ipa/ipa_interface_test.cpp             |  3 +-\n> >  test/ipa/ipa_wrappers_test.cpp              | 13 +++++++--\n> >  14 files changed, 80 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> > index e65844bc7b34..ef3d6507b692 100644\n> > --- a/include/ipa/ipa_interface.h\n> > +++ b/include/ipa/ipa_interface.h\n> > @@ -18,6 +18,10 @@ struct ipa_context {\n> >  \tconst struct ipa_context_ops *ops;\n> >  };\n> >\n> > +struct ipa_settings {\n> > +\tconst char *configuration_file;\n> > +};\n> > +\n> \n> I've seen you suggesting multiple times to use a char * to transport\n> strings, here in the IPA operations and the kernel ioctls in example. I\n> wonder, isn't this dangeours ? One can easily point this to some\n> memory that could vanish, and even if I recognize reserving a buffer\n> with a fixed length is limiting, it feels more safer to me...\n\nOne could indeed, but given that the C API structures are filled and\nconsumed by the wrappers only, there's a single place where we have to\nbe careful, so I think it's fine.\n\n> >  struct ipa_stream {\n> >  \tunsigned int id;\n> >  \tunsigned int pixel_format;\n> > @@ -63,7 +67,8 @@ struct ipa_callback_ops {\n> >  struct ipa_context_ops {\n> >  \tvoid (*destroy)(struct ipa_context *ctx);\n> >  \tvoid *(*get_interface)(struct ipa_context *ctx);\n> > -\tvoid (*init)(struct ipa_context *ctx);\n> > +\tvoid (*init)(struct ipa_context *ctx,\n> > +\t\t     const struct ipa_settings *settings);\n> >  \tint (*start)(struct ipa_context *ctx);\n> >  \tvoid (*stop)(struct ipa_context *ctx);\n> >  \tvoid (*register_callbacks)(struct ipa_context *ctx,\n> > @@ -100,6 +105,10 @@ struct ipa_context *ipaCreate();\n> >\n> >  namespace libcamera {\n> >\n> > +struct IPASettings {\n> > +\tstd::string configurationFile;\n> > +};\n> > +\n> >  struct IPAStream {\n> >  \tunsigned int pixelFormat;\n> >  \tSize size;\n> > @@ -121,7 +130,7 @@ class IPAInterface\n> >  public:\n> >  \tvirtual ~IPAInterface() {}\n> >\n> > -\tvirtual int init() = 0;\n> > +\tvirtual int init(const IPASettings &settings) = 0;\n> >  \tvirtual int start() = 0;\n> >  \tvirtual void stop() = 0;\n> >\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > index f50f93a0185a..7776d9881575 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > @@ -79,11 +79,15 @@ void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)\n> >  \treturn ctx->ipa_.get();\n> >  }\n> >\n> > -void IPAInterfaceWrapper::init(struct ipa_context *_ctx)\n> > +void IPAInterfaceWrapper::init(struct ipa_context *_ctx,\n> > +\t\t\t       const struct ipa_settings *settings)\n> >  {\n> >  \tIPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);\n> >\n> > -\tctx->ipa_->init();\n> > +\tIPASettings ipaSettings{\n> > +\t\t.configurationFile = std::string{ settings->configuration_file }\n> \n> Can't you just assign a char * to an std::string ?\n\nWe actually can, it's not just that the constructor isn't marked as\nexplicit, there's an overload of the assignment operator that takes a\nconst char *. I'll fix this.\n\n> > +\t};\n> > +\tctx->ipa_->init(ipaSettings);\n> >  }\n> >\n> >  int IPAInterfaceWrapper::start(struct ipa_context *_ctx)\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> > index e4bc6dd4535d..78ccf0f59d42 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> > @@ -23,7 +23,8 @@ public:\n> >  private:\n> >  \tstatic void destroy(struct ipa_context *ctx);\n> >  \tstatic void *get_interface(struct ipa_context *ctx);\n> > -\tstatic void init(struct ipa_context *ctx);\n> > +\tstatic void init(struct ipa_context *ctx,\n> > +\t\t\t const struct ipa_settings *settings);\n> >  \tstatic int start(struct ipa_context *ctx);\n> >  \tstatic void stop(struct ipa_context *ctx);\n> >  \tstatic void register_callbacks(struct ipa_context *ctx,\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 7f0ffb0a791f..9a347e527cd2 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -32,7 +32,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)\n> >  class IPARkISP1 : public IPAInterface\n> >  {\n> >  public:\n> > -\tint init() override { return 0; }\n> > +\tint init(const IPASettings &settings) override { return 0; }\n> >  \tint start() override { return 0; }\n> >  \tvoid stop() override {}\n> >\n> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> > index cfdbd6f99155..e6bda8ec58b0 100644\n> > --- a/src/ipa/vimc/vimc.cpp\n> > +++ b/src/ipa/vimc/vimc.cpp\n> > @@ -31,7 +31,7 @@ public:\n> >  \tIPAVimc();\n> >  \t~IPAVimc();\n> >\n> > -\tint init() override;\n> > +\tint init(const IPASettings &settings) override;\n> >\n> >  \tint start() override;\n> >  \tvoid stop() override;\n> > @@ -61,7 +61,7 @@ IPAVimc::~IPAVimc()\n> >  \t\t::close(fd_);\n> >  }\n> >\n> > -int IPAVimc::init()\n> > +int IPAVimc::init(const IPASettings &settings)\n> >  {\n> >  \ttrace(IPAOperationInit);\n> >\n> > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h\n> > index 0a48bfe732c8..64395b4a450b 100644\n> > --- a/src/libcamera/include/ipa_context_wrapper.h\n> > +++ b/src/libcamera/include/ipa_context_wrapper.h\n> > @@ -19,7 +19,7 @@ public:\n> >  \tIPAContextWrapper(struct ipa_context *context);\n> >  \t~IPAContextWrapper();\n> >\n> > -\tint init() override;\n> > +\tint init(const IPASettings &settings) override;\n> >  \tint start() override;\n> >  \tvoid stop() override;\n> >  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> > index ab6ce396b88a..5bd5d916ccc0 100644\n> > --- a/src/libcamera/ipa_context_wrapper.cpp\n> > +++ b/src/libcamera/ipa_context_wrapper.cpp\n> > @@ -69,15 +69,18 @@ IPAContextWrapper::~IPAContextWrapper()\n> >  \tctx_->ops->destroy(ctx_);\n> >  }\n> >\n> > -int IPAContextWrapper::init()\n> > +int IPAContextWrapper::init(const IPASettings &settings)\n> >  {\n> >  \tif (intf_)\n> > -\t\treturn intf_->init();\n> > +\t\treturn intf_->init(settings);\n> >\n> >  \tif (!ctx_)\n> >  \t\treturn 0;\n> >\n> > -\tctx_->ops->init(ctx_);\n> > +\tstruct ipa_settings c_settings;\n> > +\tc_settings.configuration_file = settings.configurationFile.c_str();\n> > +\n> > +\tctx_->ops->init(ctx_, &c_settings);\n> >\n> >  \treturn 0;\n> >  }\n> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> > index 890d4340e4f3..3949b7d6ea5d 100644\n> > --- a/src/libcamera/ipa_interface.cpp\n> > +++ b/src/libcamera/ipa_interface.cpp\n> > @@ -92,6 +92,16 @@\n> >   * \\brief The IPA context operations\n> >   */\n> >\n> > +/**\n> > + * \\struct ipa_settings\n> > + * \\brief IPA initialization settings for the IPA context operations\n> > + * \\sa IPASettings\n> > + *\n> > + * \\var ipa_settings::configuration_file\n> > + * \\brief The name of the IPA configuration file (may be null or point to an\n> > + * empty string)\n> > + */\n> > +\n> >  /**\n> >   * \\struct ipa_stream\n> >   * \\brief Stream information for the IPA context operations\n> > @@ -231,6 +241,7 @@\n> >   * \\var ipa_context_ops::init\n> >   * \\brief Initialise the IPA context\n> >   * \\param[in] ctx The IPA context\n> > + * \\param[in] settings The IPA initialization settings\n> >   *\n> >   * \\sa libcamera::IPAInterface::init()\n> >   */\n> > @@ -310,6 +321,22 @@\n> >\n> >  namespace libcamera {\n> >\n> > +/**\n> > + * \\struct IPASettings\n> > + * \\brief IPA interface initialization settings\n> > + *\n> > + * The IPASettings structure stores data passed to the IPAInterface::init()\n> > + * function.\n> \n> Is it worth mentioning these data are meant not to depend upon run\n> time operations ?\n\nI'll write\n\n * The IPASettings structure stores data passed to the IPAInterface::init()\n * function. The data contain settings that don't depend on a particular camera\n * or pipeline configuration and are valid for the whole life time of the IPA\n * interface.\n\n> > + */\n> > +\n> > +/**\n> > + * \\var IPASettings::configurationFile\n> > + * \\brief The name of the IPA configuration file\n> > + *\n> > + * This field may be an empty string if the IPA doesn't require a configuration\n> > + * file.\n> > + */\n> > +\n> >  /**\n> >   * \\struct IPAStream\n> >   * \\brief Stream configuration for the IPA interface\n> > @@ -424,6 +451,11 @@ namespace libcamera {\n> >  /**\n> >   * \\fn IPAInterface::init()\n> >   * \\brief Initialise the IPAInterface\n> > + * \\param[in] settings The IPA initialization settings\n> > + *\n> > + * This function initializes the IPA interface. It shall be called before any\n> > + * other function of the IPAInterface. The \\a settings carry initialization\n> > + * parameters that will remain valid for the whole life time of the interface.\n> \n> It's here, but maybe it's worth up there too...\n> \n> >   */\n> >\n> >  /**\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index f42264361785..fde445b99a46 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -401,7 +401,7 @@ int RkISP1CameraData::loadIPA()\n> >  \tipa_->queueFrameAction.connect(this,\n> >  \t\t\t\t       &RkISP1CameraData::queueFrameAction);\n> >\n> > -\tipa_->init();\n> > +\tipa_->init(IPASettings{});\n> >\n> >  \treturn 0;\n> >  }\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index c5eea3a01b0e..699f788aa1b8 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -374,7 +374,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> >  \tif (data->ipa_ == nullptr)\n> >  \t\tLOG(VIMC, Warning) << \"no matching IPA found\";\n> >  \telse\n> > -\t\tdata->ipa_->init();\n> > +\t\tdata->ipa_->init(IPASettings{});\n> >\n> >  \t/* Locate and open the capture video node. */\n> >  \tif (data->init(media))\n> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > index 89f5cb54687b..cd8ac824679d 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > @@ -26,7 +26,7 @@ public:\n> >  \tIPAProxyLinux(IPAModule *ipam);\n> >  \t~IPAProxyLinux();\n> >\n> > -\tint init() override { return 0; }\n> > +\tint init(const IPASettings &settings) override { return 0; }\n> >  \tint start() override { return 0; }\n> >  \tvoid stop() override {}\n> >  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > index 1ebb9b6a6c9d..be82fde34bd9 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > @@ -25,7 +25,7 @@ class IPAProxyThread : public IPAProxy, public Object\n> >  public:\n> >  \tIPAProxyThread(IPAModule *ipam);\n> >\n> > -\tint init() override;\n> > +\tint init(const IPASettings &settings) override;\n> >  \tint start() override;\n> >  \tvoid stop() override;\n> >\n> > @@ -97,9 +97,9 @@ IPAProxyThread::IPAProxyThread(IPAModule *ipam)\n> >  \tvalid_ = true;\n> >  }\n> >\n> > -int IPAProxyThread::init()\n> > +int IPAProxyThread::init(const IPASettings &settings)\n> >  {\n> > -\tint ret = ipa_->init();\n> > +\tint ret = ipa_->init(settings);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >\n> > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> > index 22f9ca41ef37..2e2dfb8d1ebd 100644\n> > --- a/test/ipa/ipa_interface_test.cpp\n> > +++ b/test/ipa/ipa_interface_test.cpp\n> > @@ -98,7 +98,8 @@ protected:\n> >  \t\t}\n> >\n> >  \t\t/* Test initialization of IPA module. */\n> > -\t\tipa_->init();\n> > +\t\tIPASettings settings;\n> > +\t\tipa_->init(settings);\n> >  \t\ttimer.start(1000);\n> >  \t\twhile (timer.isRunning() && trace_ != IPAOperationInit)\n> >  \t\t\tdispatcher->processEvents();\n> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> > index fae1d56b67c7..21bf51a2e046 100644\n> > --- a/test/ipa/ipa_wrappers_test.cpp\n> > +++ b/test/ipa/ipa_wrappers_test.cpp\n> > @@ -43,8 +43,14 @@ public:\n> >  \t{\n> >  \t}\n> >\n> > -\tint init() override\n> > +\tint init(const IPASettings &settings) override\n> >  \t{\n> > +\t\tif (settings.configurationFile != \"/ipa/configuration/file\") {\n> > +\t\t\tcerr << \"init(): Invalid configuration file\" << endl;\n> > +\t\t\treport(Op_init, TestFail);\n> > +\t\t\treturn 0;\n> \n> Shouldn't you 'return report(Op_init.. ' ?\n\nreport returns void, while this function returns an int. The INVOKE()\nmacro used below ignores the return value of the function. Testing the\nreturn code of the init() is already done by another test, so I'm not\ntoo concerned.\n\n> > +\t\t}\n> > +\n> >  \t\treport(Op_init, TestPass);\n> >  \t\treturn 0;\n> >  \t}\n> > @@ -339,7 +345,10 @@ protected:\n> >  \t\t * Test init(), start() and stop() last to ensure nothing in the\n> >  \t\t * wrappers or serializer depends on them being called first.\n> >  \t\t */\n> > -\t\tret = INVOKE(init);\n> > +\t\tIPASettings settings{\n> > +\t\t\t.configurationFile = \"/ipa/configuration/file\"\n> > +\t\t};\n> > +\t\tret = INVOKE(init, settings);\n> >  \t\tif (ret == TestFail) {\n> >  \t\t\tcerr << \"Failed to run init()\";\n> >  \t\t\treturn TestFail;","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16991603F9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Apr 2020 13:11:25 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69B5D72C;\n\tMon, 27 Apr 2020 13:11:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Nep8BJWd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587985884;\n\tbh=gEoHRx7WRLUK4dWH4XOkR3BYt+xP3xvvCZml2e0gpbA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Nep8BJWdJBwWCwPXW2v0d3nKusJH/anEdEsXUgE2cSFJdxBjwQmO6tAc2eOd0pTlj\n\t9mBZnxfT0RuqCsHa2Ct6p2nWsza0cHzoxVA0TienAKz5u/3wWd4Iu0rIkRIMLX4BaG\n\tjI2KjWy5r0zvEHQpswGkJsFeGebVSP1vaWdkhflc=","Date":"Mon, 27 Apr 2020 14:11:09 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200427111109.GA10040@pendragon.ideasonboard.com>","References":"<20200427031713.14013-1-laurent.pinchart@ideasonboard.com>\n\t<20200427031713.14013-8-laurent.pinchart@ideasonboard.com>\n\t<20200427091303.qazewpeugyxbplol@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200427091303.qazewpeugyxbplol@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 07/11] ipa: Pass IPA initialization\n\tsettings to IPAInterface::init()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 27 Apr 2020 11:11:25 -0000"}}]