[{"id":2612,"web_url":"https://patchwork.libcamera.org/comment/2612/","msgid":"<20190904223252.GO5433@pendragon.ideasonboard.com>","date":"2019-09-04T22:32:52","subject":"Re: [libcamera-devel] [PATCH v2 11/14] libcamera: ipa: Extend to\n\tsupport IPA interactions","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\nJust a few comments already, I'll review the API itself after reading\nthe rest of the series.\n\nOn Fri, Aug 30, 2019 at 01:26:50AM +0200, Niklas Söderlund wrote:\n> The IPA interface needs to support interactions with the pipeline, add\n> interfaces to control the sensor and handling of request ISP parameters\n> and statistics.\n> \n> The interface needs to be extended further to support attaching the\n> result of the statistic analyses to the request before the request\n> completes to the user. It also needs to modified to allow proper per\n> frame control of capture parameters.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/ipa/ipa_interface.h | 17 ++++++\n> src/ipa/ipa_dummy.cpp | 5 ++\n> src/ipa/ipa_dummy_isolate.cpp | 5 ++\n> src/libcamera/ipa_interface.cpp | 73 +++++++++++++++++++++++++\n> src/libcamera/proxy/ipa_proxy_linux.cpp | 12 ++--\n> 5 files changed, 104 insertions(+), 8 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h\n> index 9bbc4cf58ec66420..38e16ff37214f7b9 100644\n> --- a/include/libcamera/ipa/ipa_interface.h\n> +++ b/include/libcamera/ipa/ipa_interface.h\n> @@ -7,12 +7,29 @@\n> #ifndef __LIBCAMERA_IPA_INTERFACE_H__\n> #define __LIBCAMERA_IPA_INTERFACE_H__\n> \n> +#include \n> +#include \n> +\n> +#include \"v4l2_controls.h\"\n\nI really don't like including internal headers here, as the IPA API\nneeds to preserve backward compatibility. Any libcamera API used by\nout-of-tree components should be part of the public API.\n\n> +\n> namespace libcamera {\n> \n> +class Buffer;\n> +class Request;\n> +\n> class IPAInterface\n> {\n> public:\n> \tvirtual ~IPAInterface() {}\n> +\n> +\tvirtual int initSensor(const V4L2ControlInfoMap &controls) = 0;\n> +\tvirtual void processRequest(const void *cookie,\n> +\t\t\t\t const ControlList &controls,\n> +\t\t\t\t Buffer ¶meters) = 0;\n> +\tvirtual void updateStatistics(const void *cookie, Buffer &statistics) = 0;\n> +\n> +\tSignal updateSensor;\n> +\tSignal queueRequest;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> index 09f1b96a8f3d5c36..11dcc54ce4ebc6f4 100644\n> --- a/src/ipa/ipa_dummy.cpp\n> +++ b/src/ipa/ipa_dummy.cpp\n> @@ -14,6 +14,11 @@ namespace libcamera {\n> \n> class IPADummy : public IPAInterface\n> {\n> +public:\n> +\tint initSensor(const V4L2ControlInfoMap &controls) { return 0; }\n> +\tvoid processRequest(const void *cookie, const ControlList &controls,\n> +\t\t\t Buffer ¶meters) {}\n> +\tvoid updateStatistics(const void *cookie, Buffer &statistics) {}\n\nPlease mark all these method as override, here and for IPADummyIsolate\nand IPAProxyLinux.\n\n> };\n> \n> /*\n> diff --git a/src/ipa/ipa_dummy_isolate.cpp b/src/ipa/ipa_dummy_isolate.cpp\n> index fa50be5309eba3c4..4d49c0e372466af6 100644\n> --- a/src/ipa/ipa_dummy_isolate.cpp\n> +++ b/src/ipa/ipa_dummy_isolate.cpp\n> @@ -15,6 +15,11 @@ namespace libcamera {\n> \n> class IPADummyIsolate : public IPAInterface\n> {\n> +public:\n> +\tint initSensor(const V4L2ControlInfoMap &controls) { return 0; }\n> +\tvoid processRequest(const void *cookie, const ControlList &controls,\n> +\t\t\t Buffer ¶meters) {}\n> +\tvoid updateStatistics(const void *cookie, Buffer &statistics) {}\n> };\n> \n> /*\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 273477a5272677b7..be9eab3cda32379d 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -19,4 +19,77 @@ namespace libcamera {\n> * \\brief Interface for IPA implementation\n> */\n> \n> +/**\n> + * \\fn IPAInterface::initSensor()\n> + * \\brief Initialize the IPA sensor settings\n> + * \\param[in] controls List of controls provided by the sensor\n> + *\n> + * This function is called when a pipeline attaches to an IPA to inform the IPA\n> + * of the controls and limits the sensor in the video pipeline supports. The IPA\n> + * have the option to set controls of the sensor by emitting the updateSensor\n> + * signal.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::processRequest()\n> + * \\brief Provide IPA with a parameter buffer to fill\n> + * \\param[in] cookie Cookie for the request\n> + * \\param[in] controls List of controls associated with the request\n> + * \\param[in,out] parameters Buffer containing an empty parameters buffer\n> + *\n> + * This function is called by a pipeline handler when it has a request to\n> + * process. The IPA shall fill in the \\a parameters buffer to achieve the\n> + * capture result described in \\a controls. The \\a cookie value identifies\n> + * the request the controls and parameters buffer corresponds to and can be\n> + * matched in updateStatistics() once the request have been processed by the\n> + * hardware. The cookie value is only valid from that processRequest() is\n> + * called and until updateStatistics() return.\n> + *\n> + * When the \\a parameters buffer is filled in and ready to be queued to hardware\n> + * the IPA shall signal pipeline handler using the queueRequest signal with the\n> + * cookie value corresponding to the request the parameters belong to.\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::updateStatistics()\n> + * \\brief Provide IPA with statistic buffer to examine\n> + * \\param[in] cookie Cookie for the request\n> + * \\param[in] statistics Buffer containing a filled in statistic buffer\n> + *\n> + * This function is called once a statistic buffer is completed and have been\n> + * dequeue. The IPA may inspect the buffer and update its internal view of the\n> + * capture conditions. The \\a cookie value can be used to associate the dequeued\n> + * statistics buffer with the parameters buffer filled in by the IPA in\n> + * processRequest().\n> + *\n> + * \\todo Extend this function to return or signal the result of the statistic\n> + * examination by the IPA.\n> + */\n> +\n> +/**\n> + * \\var IPAInterface::updateSensor\n> + * \\brief Signal emitted when the IPA wish to update sensor V4L2 controls\n> + *\n> + * This signal is emitted when the IPA wish to update one or more V4L2 control\n> + * of the sensor in the video pipeline. The list of controls to modify is passed\n> + * as a parameter.\n> + *\n> + * \\todo Extend this function to work with per-frame control setting of\n> + * controls.\n> + */\n> +\n> +/**\n> + * \\var IPAInterface::queueRequest\n> + * \\brief Signal emitted when the IPA is done preparing a request\n> + *\n> + * This signal is emitted then the IPA have finished filling in the parameters\n> + * buffer and is ready for the request to be committed to hardware for capture.\n> + * The request cookie is passed as a parameter.\n> + *\n> + * \\todo Extend this function to work with per-frame control setting of\n> + * controls.\n> + */\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index f881aab5b2e1178a..bb7aeedf12779c66 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -26,7 +26,10 @@ public:\n> \tIPAProxyLinux(IPAModule *ipam);\n> \t~IPAProxyLinux();\n> \n> -\tint init();\n\nThis belongs to the patch that removed the init() method from\nIPAInterface.\n\n> +\tint initSensor(const V4L2ControlInfoMap &controls) { return 0; }\n> +\tvoid processRequest(const void *cookie, const ControlList &controls,\n> +\t\t\t Buffer ¶meters) {}\n> +\tvoid updateStatistics(const void *cookie, Buffer &statistics) {}\n> \n> private:\n> \tvoid readyRead(IPCUnixSocket *ipc);\n> @@ -36,13 +39,6 @@ private:\n> \tIPCUnixSocket *socket_;\n> };\n> \n> -int IPAProxyLinux::init()\n> -{\n> -\tLOG(IPAProxy, Debug) << \"initializing IPA via dummy proxy!\";\n> -\n> -\treturn 0;\n> -}\n> -\n> IPAProxyLinux::IPAProxyLinux(IPAModule *ipam)\n> {\n> \tLOG(IPAProxy, Debug)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D254D60BB2\n\tfor ;\n\tThu, 5 Sep 2019 00:32:58 +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 55CEE440;\n\tThu, 5 Sep 2019 00:32:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567636378;\n\tbh=ww9Bl6rMZbi2EGsnuGenAd7JFaczQnjsO/pe+35ZnzQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YLkZcH67/MPocn1ZbpkDjdnfaAksVHNEO1p468EAahqdoSvwiZLlV+dnIPMaXcc7Y\n\t4XVEVg2pG+uJP6Ublt1+qJs5v8HnWAkRiBPMHf1cD/1khgyIb32mA4HcrCvmiMpIlb\n\tK486ifmj/vvO7ywdjBRAqe3+0yiknQLVkiWvq7e4=","Date":"Thu, 5 Sep 2019 01:32:52 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190904223252.GO5433@pendragon.ideasonboard.com>","References":"<20190829232653.13214-1-niklas.soderlund@ragnatech.se>\n\t<20190829232653.13214-12-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":"<20190829232653.13214-12-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 11/14] libcamera: ipa: Extend to\n\tsupport IPA interactions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 04 Sep 2019 22:32:59 -0000"}}]