[{"id":4321,"web_url":"https://patchwork.libcamera.org/comment/4321/","msgid":"<20200326212512.GB20581@pendragon.ideasonboard.com>","date":"2020-03-26T21:25:12","subject":"Re: [libcamera-devel] [RFCv2 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 Thu, Mar 26, 2020 at 05:08:13PM +0100, 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> in actions in conjunction with the camera start and stop.\n\n\"in actions in conjunction\" ?\n\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\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/proxy/ipa_proxy_linux.cpp     |  2 ++\n>  test/ipa/ipa_interface_test.cpp             | 24 +++++++++++++++++++\n>  test/ipa/ipa_wrappers_test.cpp              | 25 ++++++++++++++++++--\n>  11 files changed, 123 insertions(+), 2 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> +\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> +}\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> +}\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> +}\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/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> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Test start of IPA module. */\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> +\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..ac56498c40e597c9 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,13 +336,21 @@ 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\t\treturn TestFail;\n>  \n> +\t\tret = INVOKE(start);\n> +\t\tif (ret == TestFail)\n> +\t\t\treturn TestFail;\n> +\n> +\t\tret = INVOKE(stop);\n> +\t\tif (ret == TestFail)\n> +\t\t\treturn TestFail;\n\nWhile at it, cerr messages for failures ?\n\nApart from the lack of documentation (which I understand is due to RFC\nstatus), this looks good to me.\n\n> +\n>  \t\treturn TestPass;\n>  \t}\n>","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 7CB8260412\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 26 Mar 2020 22:25:16 +0100 (CET)","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 DEF152DC;\n\tThu, 26 Mar 2020 22:25:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SNsirTbG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585257916;\n\tbh=ytiHQZpd87fqaIgLWs0uz9+SECNos2xRB6lp7kAdQ5c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SNsirTbG2qqj0A/ru6H94NQFI7AJsr0ifSljjNjfazKQifA21KEfvBn7wrOKieCnt\n\t85htET0J7y1MyRZ+8hCOT0oXuLI6EQw9SMreLoO/HAuhoBroTZmUbTG1gDdDU24XNt\n\tL9kxl3zg9YeN/zBAYWj0Qs9eYrSt0469C/yvVShE=","Date":"Thu, 26 Mar 2020 23:25:12 +0200","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":"<20200326212512.GB20581@pendragon.ideasonboard.com>","References":"<20200326160819.4088361-1-niklas.soderlund@ragnatech.se>\n\t<20200326160819.4088361-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":"<20200326160819.4088361-2-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFCv2 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":"Thu, 26 Mar 2020 21:25:16 -0000"}}]