[{"id":2709,"web_url":"https://patchwork.libcamera.org/comment/2709/","msgid":"<20190928103534.4kitjwaqj6vuxtf2@uno.localdomain>","date":"2019-09-28T10:35:34","subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: ipa: Extend to\n\tsupport IPA interactions","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Sep 27, 2019 at 04:44:12AM +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> Signed-off-by: Niklas Söderlund \n> ---\n> include/ipa/ipa_interface.h | 21 +++++\n> src/ipa/ipa_dummy.cpp | 6 +-\n> src/libcamera/ipa_interface.cpp | 106 ++++++++++++++++++++++++\n> src/libcamera/proxy/ipa_proxy_linux.cpp | 13 ++-\n> 4 files changed, 137 insertions(+), 9 deletions(-)\n>\n> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> index 2c5eb1fd524311cb..feeb00b1b1315ca5 100644\n> --- a/include/ipa/ipa_interface.h\n> +++ b/include/ipa/ipa_interface.h\n> @@ -7,14 +7,35 @@\n> #ifndef __LIBCAMERA_IPA_INTERFACE_H__\n> #define __LIBCAMERA_IPA_INTERFACE_H__\n>\n> +#include \n> +#include \n> +\n> +#include \"v4l2_controls.h\"\n> +\n> namespace libcamera {\n>\n> +class BufferMemory;\n> +class Request;\n> +struct IPAMetaData;\n> +\n> class IPAInterface\n> {\n> public:\n> \tvirtual ~IPAInterface() {}\n>\n> \tvirtual int init() = 0;\n> +\n> +\tvirtual void initSensor(const V4L2ControlInfoMap &controls) = 0;\n\nAre we ok for now to assume all sensor related information could be\nexpressed a V4L2Controls ?\n\n> +\tvirtual void initBuffers(unsigned int type, const std::vector &buffers) = 0;\n\nWhy raw memory buffers and not pools ?\n\nWhen running through the C interface I assume we'll just need to get\ndown to pass file descriptors in a binary buffer, which makes me wonder\nif we assume all memory buffers will have the same number of planes\n(each of them with an associated fd) or do we need to define a\nserialization format to express that.\n\n> +\n> +\tvirtual void signalBuffer(unsigned int type, unsigned int id) = 0;\n> +\n> +\tvirtual void queueRequest(unsigned int frame, const ControlList &controls) = 0;\n> +\n> +\tSignal updateSensor;\n> +\tSignal queueBuffer;\n> +\tSignal setDelay;\n> +\tSignal metaDataReady;\n\nI'm a bit skeptical of IPAMetaData. As we refreined from using C\nstructures to pass parameters from pipeline to IPA to avoid\nversioning and the need to keep in sync several pieces of the\nimplementation, I'm for the same reason oriented to do the same here\nand return a ControlList. Would this be possible?\n\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/ipa/ipa_dummy.cpp b/src/ipa/ipa_dummy.cpp\n> index 9d0cbdc8b1ad5565..4d429f20d7aaa955 100644\n> --- a/src/ipa/ipa_dummy.cpp\n> +++ b/src/ipa/ipa_dummy.cpp\n> @@ -15,7 +15,11 @@ namespace libcamera {\n> class IPADummy : public IPAInterface\n> {\n> public:\n> -\tint init();\n> +\tint init() override;\n> +\tvoid initSensor(const V4L2ControlInfoMap &controls) override {}\n> +\tvoid initBuffers(unsigned int type, const std::vector &buffers) override {}\n> +\tvoid signalBuffer(unsigned int type, unsigned int id) override {}\n> +\tvoid queueRequest(unsigned int frame, const ControlList &controls) override {}\n> };\n>\n> int IPADummy::init()\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index d7d8ca8881efcf66..79853771c70f15d4 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -24,4 +24,110 @@ namespace libcamera {\n> * \\brief Initialise the IPAInterface\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\nmm, more than provided I would say supported. Same question as above,\nthis is the list of supported controls, will they capture the sensor's\ncharacteristics too ?\n\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> +\n> +/**\n> + * \\fn IPAInterface::initBuffers()\n> + * \\brief Initialize the buffers shared between pipeline and IPA\n> + * \\param[in] type Type of buffers described in \\a buffers\n> + * \\param[in] buffers List of buffers of \\a type\n> + *\n> + * This function is called when a pipeline handler wants to inform the IPA of\n> + * which buffers it has mapped for a specific purpose. All buffers shared\n> + * between these two object's needs to be shared using this function prior to\n> + * use.\n> + *\n> + * After the buffers have been initialized they are referenced using an\n> + * numerical \\a id which represents the insertion order in the \\a buffers list\n> + * given as an argument here.\n> + *\n> + * It is possible to call this function multiple times for different kinds of\n> + * buffer types, e.g. once for statistic buffers and once more for parameter\n> + * buffers. It's also possible to call this function multiple times for the\n> + * same buffer type if the pipeline handler wants to update the mappings inside\n> + * the IPA.\n> + *\n> + * The buffer type numerical ids and it's usage are not enforced by the IPA\n> + * interface and is left as pipeline handler specific.\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::signalBuffer()\n> + * \\brief Signal that the pipeline handlers is done processing a buffer\n\ndone processing or the buffer is ready for processing on the IPA side ?\n\n> + * \\param[in] type Type of buffer\n\nAs a general note on types, we could define an empty ActionType enum\nand let timeline subclasses extend it. This would gurantee a better\n(sometimes annoying) type safety when handling types in timelines\nimplementations\n\n> + * \\param[in] id Buffer id in \\a type\n\nI wonder if the bookeeping of buffer id <-> buffers you have\nimplemented in the RkISP IPA is robust. We had a similar translation\nto indexes to buffer for IPU3 and it was a bit of a pain.\n\nCouldn't we just make BufferPools and Buffers serializable and pass\nthe buffer here?\n\n> + *\n> + * A pipeline handler can signal to the IPA that it is done with buffer. This\n> + * may have different meanings for different buffer types. For example signaling\n> + * a parameter buffer may be an indication that it's now free and the IPA can\n> + * update its content and schedule it to be written to the hardware again. While\n> + * signaling a parameters buffer may indicate that it's filled with data from\n> + * the hardware and are ready to be consumed by the IPA.\n> + *\n> + * As all pipeline designs are under the unique it must document what signaling\n> + * a buffer of a specific type means for that particular IPA interface.\n\nIs this a todo ? I didn't get \"under the unique it\"\n\n> + */\n> +\n> +/**\n> + * \\fn IPAInterface::queueRequest()\n> + * \\brief Inform the IPA that a request have been queued to the pipeline\n> + * \\param[in] frame The frame number for the request\n> + * \\param[in] controls List of controls associated with the request\n> + *\n> + * This function is called by a pipeline handler when it has a request to\n> + * process. The pipeline informs the IPA that \\a frame shall be processed\n> + * with the parameters in \\a controls.\n> + *\n> + * The IPA may act on this by emitting different signals in the IPA interface\n> + * to configure the pipeline to act on \\a frame according to the \\a controls.\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 frame number the settings should be\n> + * ready for and a list of controls to modify is passed as parameters.\n> + */\n> +\n> +/**\n> + * \\var IPAInterface::queueBuffer\n> + * \\brief Signal emitted when the IPA wish to queue a buffer to the hardware\n> + *\n> + * This signal is emitted then the IPA wish to queue q buffer to the hardware.\n> + * The frame number and buffer type and id are passed as parameters.\n> + */\n> +\n> +/**\n> + * \\var IPAInterface::setDelay\n> + * \\brief Signal emitted when the IPA wish to set a delay in the pipeline\n> + *\n> + * This signal is emitted when the IPA wish to change a delay inside the\n> + * pipeline timeline. The action type, frame and time offsets are passed as\n> + * parameters.\n> + *\n> + * The unit for the time offset is not fixed in the IPA interface, it is up to\n> + * the pipeline implementation to choose a suitable unit for its use-case.\n> + */\n> +\n> +/**\n> + * \\var IPAInterface::metaDataReady\n> + * \\brief Signal emitted when the IPA is done processing statistics\n> + *\n> + * This signal is emitted when the IPA have finished processing the statistics\n> + * buffer and have created an IPAMetaData object which are ready to be consumed\n\nare/is\n\n> + * by the pipeline handler. The frame number and the meta data is passed as\n\nis/are\n\n> + * parameters.\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 62fcb529e1c7e4ff..17d09fb21582376d 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -26,7 +26,11 @@ public:\n> \tIPAProxyLinux(IPAModule *ipam);\n> \t~IPAProxyLinux();\n>\n> -\tint init();\n> +\tint init() override { return 0; }\n> +\tvoid initSensor(const V4L2ControlInfoMap &controls) override {}\n> +\tvoid initBuffers(unsigned int type, const std::vector &buffers) override {}\n> +\tvoid signalBuffer(unsigned int type, unsigned int id) override {}\n> +\tvoid queueRequest(unsigned int frame, const ControlList &controls) override {}\n>\n> private:\n> \tvoid readyRead(IPCUnixSocket *ipc);\n> @@ -36,13 +40,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> \t: proc_(nullptr), socket_(nullptr)\n> {\n> --\n> 2.23.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 23F4760BBA\n\tfor ;\n\tSat, 28 Sep 2019 12:33:56 +0200 (CEST)","from uno.localdomain\n\t(host71-63-dynamic.19-79-r.retail.telecomitalia.it [79.19.63.71])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 98EC2240004;\n\tSat, 28 Sep 2019 10:33:53 +0000 (UTC)"],"X-Originating-IP":"79.19.63.71","Date":"Sat, 28 Sep 2019 12:35:34 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190928103534.4kitjwaqj6vuxtf2@uno.localdomain>","References":"<20190927024417.725906-1-niklas.soderlund@ragnatech.se>\n\t<20190927024417.725906-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vnwkqof7g22id6wg\"","Content-Disposition":"inline","In-Reply-To":"<20190927024417.725906-9-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 08/13] libcamera: ipa: Extend to\n\tsupport IPA interactions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 28 Sep 2019 10:33:56 -0000"}}]