[{"id":4432,"web_url":"https://patchwork.libcamera.org/comment/4432/","msgid":"<20200411114832.GC4871@pendragon.ideasonboard.com>","date":"2020-04-11T11:48:32","subject":"Re: [libcamera-devel] [PATCH v3 1/7] ipa: Add start() and stop()\n\toperations","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sat, Apr 11, 2020 at 02:19:05AM +0200, Niklas Söderlund wrote:\n> Add two new operations to the IPA interface to start and stop it. The\n> intention is that these functions shall be used by the IPA to perform\n> actions when the camera is started and stopped.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n> * Changes since v2\n> - Update commit message\n> - Add cerr to test case\n> - Add documentation\n> ---\n>  include/ipa/ipa_interface.h                 |  4 +++\n>  include/ipa/ipa_vimc.h                      |  2 ++\n>  src/ipa/libipa/ipa_interface_wrapper.cpp    | 16 ++++++++++\n>  src/ipa/libipa/ipa_interface_wrapper.h      |  2 ++\n>  src/ipa/rkisp1/rkisp1.cpp                   |  2 ++\n>  src/ipa/vimc/vimc.cpp                       | 20 +++++++++++++\n>  src/libcamera/include/ipa_context_wrapper.h |  2 ++\n>  src/libcamera/ipa_context_wrapper.cpp       | 26 ++++++++++++++++\n>  src/libcamera/ipa_interface.cpp             | 32 ++++++++++++++++++++\n>  src/libcamera/proxy/ipa_proxy_linux.cpp     |  2 ++\n>  test/ipa/ipa_interface_test.cpp             | 24 +++++++++++++++\n>  test/ipa/ipa_wrappers_test.cpp              | 33 +++++++++++++++++++--\n>  12 files changed, 162 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> index 229d1124b19ec1c9..9a62208cc80084c8 100644\n> --- a/include/ipa/ipa_interface.h\n> +++ b/include/ipa/ipa_interface.h\n> @@ -64,6 +64,8 @@ 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 (*start)(struct ipa_context *ctx);\n\nShould this return an int ?\n\n> +\tvoid (*stop)(struct ipa_context *ctx);\n>  \tvoid (*register_callbacks)(struct ipa_context *ctx,\n>  \t\t\t\t   const struct ipa_callback_ops *callbacks,\n>  \t\t\t\t   void *cb_ctx);\n> @@ -120,6 +122,8 @@ public:\n>  \tvirtual ~IPAInterface() {}\n>  \n>  \tvirtual int init() = 0;\n> +\tvirtual int start() = 0;\n> +\tvirtual void stop() = 0;\n>  \n>  \tvirtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;\n> diff --git a/include/ipa/ipa_vimc.h b/include/ipa/ipa_vimc.h\n> index 9add122cf598aa6f..8e82dd94bf479e35 100644\n> --- a/include/ipa/ipa_vimc.h\n> +++ b/include/ipa/ipa_vimc.h\n> @@ -15,6 +15,8 @@ namespace libcamera {\n>  enum IPAOperationCode {\n>  \tIPAOperationNone,\n>  \tIPAOperationInit,\n> +\tIPAOperationStart,\n> +\tIPAOperationStop,\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> index b93c1c1f1c1a2018..ef074ba17316ed97 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -86,6 +86,20 @@ void IPAInterfaceWrapper::init(struct ipa_context *_ctx)\n>  \tctx->ipa_->init();\n>  }\n>  \n> +void IPAInterfaceWrapper::start(struct ipa_context *_ctx)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);\n> +\n> +\tctx->ipa_->start();\n\nThis would become\n\n\treturn ctx->ipa_->start();\n\n> +}\n> +\n> +void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)\n> +{\n> +\tIPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);\n> +\n> +\tctx->ipa_->stop();\n> +}\n> +\n>  void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n>  \t\t\t\t\t     const struct ipa_callback_ops *callbacks,\n>  \t\t\t\t\t     void *cb_ctx)\n> @@ -234,6 +248,8 @@ const struct ipa_context_ops IPAInterfaceWrapper::operations_ = {\n>  \t.destroy = &IPAInterfaceWrapper::destroy,\n>  \t.get_interface = &IPAInterfaceWrapper::get_interface,\n>  \t.init = &IPAInterfaceWrapper::init,\n> +\t.start = &IPAInterfaceWrapper::start,\n> +\t.stop = &IPAInterfaceWrapper::stop,\n>  \t.register_callbacks = &IPAInterfaceWrapper::register_callbacks,\n>  \t.configure = &IPAInterfaceWrapper::configure,\n>  \t.map_buffers = &IPAInterfaceWrapper::map_buffers,\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> index 3fb7b447643953ff..9ee4fe15b2738cf4 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> @@ -24,6 +24,8 @@ 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 start(struct ipa_context *ctx);\n> +\tstatic void stop(struct ipa_context *ctx);\n>  \tstatic void register_callbacks(struct ipa_context *ctx,\n>  \t\t\t\t       const struct ipa_callback_ops *callbacks,\n>  \t\t\t\t       void *cb_ctx);\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 438b3c66f77a1e57..af38e3299f2ff045 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -33,6 +33,8 @@ class IPARkISP1 : public IPAInterface\n>  {\n>  public:\n>  \tint init() override { return 0; }\n> +\tint start() override { return 0; }\n> +\tvoid stop() override {}\n>  \n>  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index 6e2095b56bbcdaa3..566a66e404affbd5 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -32,6 +32,10 @@ public:\n>  \t~IPAVimc();\n>  \n>  \tint init() override;\n> +\n> +\tint start() override;\n> +\tvoid stop() override;\n> +\n>  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> @@ -66,6 +70,22 @@ int IPAVimc::init()\n>  \treturn 0;\n>  }\n>  \n> +int IPAVimc::start()\n> +{\n> +\ttrace(IPAOperationStart);\n> +\n> +\tLOG(IPAVimc, Debug) << \"start vimc IPA!\";\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IPAVimc::stop()\n> +{\n> +\ttrace(IPAOperationStop);\n> +\n> +\tLOG(IPAVimc, Debug) << \"stop vimc IPA!\";\n> +}\n> +\n>  void IPAVimc::initTrace()\n>  {\n>  \tstruct stat fifoStat;\n> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h\n> index c9e194120de6b69c..0a48bfe732c831d7 100644\n> --- a/src/libcamera/include/ipa_context_wrapper.h\n> +++ b/src/libcamera/include/ipa_context_wrapper.h\n> @@ -20,6 +20,8 @@ public:\n>  \t~IPAContextWrapper();\n>  \n>  \tint init() override;\n> +\tint start() override;\n> +\tvoid stop() override;\n>  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;\n>  \n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> index 946a2fd8cab1782a..c40e56169512a1ba 100644\n> --- a/src/libcamera/ipa_context_wrapper.cpp\n> +++ b/src/libcamera/ipa_context_wrapper.cpp\n> @@ -82,6 +82,32 @@ int IPAContextWrapper::init()\n>  \treturn 0;\n>  }\n>  \n> +int IPAContextWrapper::start()\n> +{\n> +\tif (intf_)\n> +\t\treturn intf_->start();\n> +\n> +\tif (!ctx_)\n> +\t\treturn 0;\n> +\n> +\tctx_->ops->start(ctx_);\n> +\n> +\treturn 0;\n\nAnd here,\n\n\treturn ctx_->ops_->start(ctx_);\n\n> +}\n> +\n> +void IPAContextWrapper::stop()\n> +{\n> +\tif (intf_)\n> +\t\treturn intf_->stop();\n> +\n> +\tif (!ctx_)\n> +\t\treturn;\n> +\n> +\tctx_->ops->stop(ctx_);\n> +\n> +\treturn;\n\nNo need for a return here.\n\n> +}\n> +\n>  void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t\t\t  const std::map<unsigned int, const ControlInfoMap &> &entityControls)\n>  {\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 5959e7deda6c5172..d8d57fdb2ae59d2e 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -235,6 +235,20 @@\n>   * \\sa libcamera::IPAInterface::init()\n>   */\n>  \n> +/**\n> + * \\var ipa_context_ops::start\n> + * \\brief Starts the IPA context\n\ns/Starts/Start/\n\n> + *\n> + * \\sa libcamera::IPAInterface::start()\n> + */\n> +\n> +/**\n> + * \\var ipa_context_ops::stop\n> + * \\brief Stops the IPA context\n\ns/Stops/Stop/\n\n> + *\n> + * \\sa libcamera::IPAInterface::stop()\n> + */\n> +\n>  /**\n>   * \\var ipa_context_ops::register_callbacks\n>   * \\brief Register callback operation from the IPA to the pipeline handler\n> @@ -412,6 +426,24 @@ namespace libcamera {\n>   * \\brief Initialise the IPAInterface\n>   */\n>  \n> +/**\n> + * \\fn IPAInterface::start()\n> + * \\brief Starts the IPA\n\ns/Starts/Start/\n\n> + *\n> + * This method informs the IPA module that the camera is about to be started\n> + * and it shall prepare any resources it needs to be operate.\n\ns/and it shall/and that it shall/\ns/to be operate/to operate/\n\nOr maybe\n\n * This method informs the IPA module that the camera is about to be started.\n * The IPA module shall prepare any resources it needs to operate.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::stop()\n> + * \\brief Stops the IPA\n\ns/Stops/Stop/\n\n> + *\n> + * This method informs the IPA module that the camera is stopped and resources\n> + * prepared in start() can be freed.\n\nIf you prefer the second option above, this would become\n\n * This method informs the IPA module that the camera is stopped. The IPA module\n * shall release resources prepared in start().\n\n> + */\n> +\n>  /**\n>   * \\fn IPAInterface::configure()\n>   * \\brief Configure the IPA stream and sensor settings\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index c7218fb472490cc8..2aa80b9467044fbf 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -27,6 +27,8 @@ public:\n>  \t~IPAProxyLinux();\n>  \n>  \tint init() override { return 0; }\n> +\tint start() override { return 0; }\n> +\tvoid stop() override {}\n>  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}\n>  \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override {}\n> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> index cafc249bbbc96cf8..f861c10f16925d78 100644\n> --- a/test/ipa/ipa_interface_test.cpp\n> +++ b/test/ipa/ipa_interface_test.cpp\n> @@ -109,6 +109,30 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> +\t\t/* Test start of IPA module. */\n> +\t\tipa_->start();\n> +\t\ttimer.start(1000);\n> +\t\twhile (timer.isRunning() && trace_ != IPAOperationStart)\n> +\t\t\tdispatcher->processEvents();\n> +\n> +\t\tif (trace_ != IPAOperationStart) {\n> +\t\t\tcerr << \"Failed to test IPA start sequence\"\n> +\t\t\t     << endl;\n\nThis holds on a single line.\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test start of IPA module. */\n\ns/start/stop/ ?\n\n> +\t\tipa_->stop();\n> +\t\ttimer.start(1000);\n> +\t\twhile (timer.isRunning() && trace_ != IPAOperationStop)\n> +\t\t\tdispatcher->processEvents();\n> +\n> +\t\tif (trace_ != IPAOperationStop) {\n> +\t\t\tcerr << \"Failed to test IPA stop sequence\"\n> +\t\t\t     << endl;\n\nThis holds on a single line too.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n>  \t\treturn TestPass;\n>  \t}\n>  \n> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> index 1ae1781169c85e6c..fae1d56b67c7933c 100644\n> --- a/test/ipa/ipa_wrappers_test.cpp\n> +++ b/test/ipa/ipa_wrappers_test.cpp\n> @@ -27,6 +27,8 @@ using namespace std;\n>  \n>  enum Operation {\n>  \tOp_init,\n> +\tOp_start,\n> +\tOp_stop,\n>  \tOp_configure,\n>  \tOp_mapBuffers,\n>  \tOp_unmapBuffers,\n> @@ -47,6 +49,17 @@ public:\n>  \t\treturn 0;\n>  \t}\n>  \n> +\tint start() override\n> +\t{\n> +\t\treport(Op_start, TestPass);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tvoid stop() override\n> +\t{\n> +\t\treport(Op_stop, TestPass);\n> +\t}\n> +\n>  \tvoid configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  \t\t       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override\n>  \t{\n> @@ -323,12 +336,26 @@ protected:\n>  \t\t\treturn TestFail;\n>  \n>  \t\t/*\n> -\t\t * Test init() last to ensure nothing in the wrappers or\n> -\t\t * serializer depends on init() being called first.\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\tif (ret == TestFail)\n> +\t\tif (ret == TestFail) {\n> +\t\t\tcerr << \"Failed to run init()\";\n>  \t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tret = INVOKE(start);\n> +\t\tif (ret == TestFail) {\n> +\t\t\tcerr << \"Failed to run start()\";\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tret = INVOKE(stop);\n> +\t\tif (ret == TestFail) {\n> +\t\t\tcerr << \"Failed to run stop()\";\n> +\t\t\treturn TestFail;\n> +\t\t}\n>  \n>  \t\treturn TestPass;\n>  \t}","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 58FCA600EC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 11 Apr 2020 13:48:43 +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 BD86124B;\n\tSat, 11 Apr 2020 13:48:42 +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=\"jX096Iwe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1586605722;\n\tbh=jXnuDBjQId0YWWUfT7Vkvhc20yGou7StqfkD430kKds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jX096IwekeyUroLj4Txv9Epx3zDPi71352tAGG/16vx0OE8C28APiILK7xdhhjIOe\n\toZkG8t9ZnzP6WFVG3PAQjg0o7h1y4Bisnv0IOA1UNfM4g2Ifn6UOY7h24pv2ZMWTvz\n\tjP8jREj69lgyMQbWhTTGJMApXKkl4sNeN/K1Q4O4=","Date":"Sat, 11 Apr 2020 14:48:32 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200411114832.GC4871@pendragon.ideasonboard.com>","References":"<20200411001911.1143155-1-niklas.soderlund@ragnatech.se>\n\t<20200411001911.1143155-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200411001911.1143155-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 1/7] ipa: Add start() and stop()\n\toperations","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":"Sat, 11 Apr 2020 11:48:43 -0000"}}]