[{"id":19292,"web_url":"https://patchwork.libcamera.org/comment/19292/","msgid":"<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>","date":"2021-09-02T13:16:27","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 01/09/2021 15:37, Jacopo Mondi wrote:\n> The IPA::configure() function has an IPAConfigInfo parameters which\n> contains a map of numerical indexes to ControlInfoMap instances.\n> \n> This is a leftover of the old IPA protocol, where it was not possible to\n> specify a rich interface as it is possible today and each entity\n> ControlInfoMap was indexed by a numerical id and stored in a map.\n> \n> Now that the IPA interface allows to specify parameters by name, drop the\n> map and send the sensor's control info map only.\n> \n> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> can be added to IPAConfigInfo.\n\n\nIt looks like this will need a patch for\n\n https://git.libcamera.org/libcamera/ipu3-ipa.git/\n\nas well. But given that it requires manually installed AIQ libraries on\nnon CrOS build environments, perhaps it's something myself or Umang\nshould handle when this series merges.\n\nI wonder if we should be increasing the IPA ABI / version numbers too\nwhenever we make a change.\n\nBut alas - it doesn't look like that's fully implemented, The mojo files\nshould have a version number in there (or even better, could mojo create\na hash of the interface to catch when it changes I wonder)?\n\nSo, given that you can't update the other IPA, and you can't update the\nIPU3 IPA Version (without further patches) ... I guess this is it ...\n\nReviewed-by: Kieran Bingham \n\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/ipu3.mojom | 2 +-\n> src/ipa/ipu3/ipu3.cpp | 4 ++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> 3 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index d561c2244907..2045ce909a88 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -32,7 +32,7 @@ struct IPU3Action {\n> \n> struct IPAConfigInfo {\n> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> -\tmap entityControls;\n> +\tlibcamera.ControlInfoMap sensorControls;\n> \tlibcamera.Size bdsOutputSize;\n> \tlibcamera.Size iif;\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c925cf642611..6a74f7826332 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \n> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> {\n> -\tif (configInfo.entityControls.empty()) {\n> +\tif (configInfo.sensorControls.empty()) {\n> \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> \t\treturn -ENODATA;\n> \t}\n> \n> \tsensorInfo_ = configInfo.sensorInfo;\n> \n> -\tctrls_ = configInfo.entityControls.at(0);\n> +\tctrls_ = configInfo.sensorControls;\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..92e869257e53 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t}\n> \n> \tipa::ipu3::IPAConfigInfo configInfo;\n> -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> \tconfigInfo.sensorInfo = sensorInfo;\n> \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> \tconfigInfo.iif = config->imguConfig().iif;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6AE05BDC71\n\tfor ;\n\tThu, 2 Sep 2021 13:16:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1FEE6916A;\n\tThu, 2 Sep 2021 15:16:31 +0200 (CEST)","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 4B5A160255\n\tfor ;\n\tThu, 2 Sep 2021 15:16:30 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C925845E;\n\tThu, 2 Sep 2021 15:16:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kOxZdE8e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630588589;\n\tbh=HlVThzAuj3Hie5QxvN6r1XSU4NBU1qaPu/Xf7VKen2M=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=kOxZdE8eblSwezujrAQ8ezpcvIF0KOh4ga/snAG3F6oJLyB2asXJw29DsVk3kPWYa\n\tZeQtgxVB5IjaJM3vD5BVJXLgSaIdPQctKGfea12ETEtOJlPKmMXaanjvO1racj1T/G\n\tLVG5gLxHF4jXs3ELosHSd0gD//OQkTiUr9R/cf7s=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>","From":"Kieran Bingham ","Message-ID":"<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>","Date":"Thu, 2 Sep 2021 14:16:27 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19294,"web_url":"https://patchwork.libcamera.org/comment/19294/","msgid":"","date":"2021-09-02T14:05:23","subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 01/09/2021 15:37, Jacopo Mondi wrote:\n> When running the IPA in isolated mode, each side of the IPC boundary\n> has an instance of the ControlSerializer class which is used to\n> serializer/deserialize controls before transmitting them on the wire.\n> \n> The IPAProxyWorker, which creates and manages the process the IPA runs into,\n> does not reset its ControlSerializer upon an IPA::configure() call, while\n> the IPAProxy does so, effectively creating a misalignment between the\n> two sides of the fence.\n> \n> This obviously creates issues as one side of the IPC runs with a\n> populated and possibly stale cache of ControlInfoMap references, while the\n> other side gets reset every time a new configuration is applied to the\n> Camera.\n> \n> Fix that by resetting the IPAProxyWorker ControlSerializer on an\n> IPA configure() call.\n> \n> This change fixes an issue which is easily triggered by running two\n> consecutive capture sessions with the IPA running in isolated mode:\n> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap\n> \n> Fixes: 7832e19a599e (\"utils: ipc: add templates for code generation for IPC mechanism\")\n\nIs it also technically fixing the patch that added the reset on the\nother side? as that's when the misalignment begins?\n\n\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> index 8a88bd467da7..22cbc15c5eff 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -79,6 +79,10 @@ public:\n> \n> {% for method in interface_main.methods %}\n> \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n> +\n> +{%- if method.mojom_name == \"configure\" %}\n> +\t\tcontrolSerializer_.reset();\n\none more level indentation >> ?\n\nIt looks like we're \"inside\" this case statement, so it should be one\nlevel deeper.\n\n\n> +{%- endif %}\n\n\nWow, these templates are hard to parse :-)\n\nBut ... it looks like this is right ;-)\n\nReviewed-by: Kieran Bingham \n\n> \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n> {% for param in method|method_param_outputs %}\n> \t\t\t{{param|name}} {{param.mojom_name}};\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A803FBD87D\n\tfor ;\n\tThu, 2 Sep 2021 14:05:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BB6B6916A;\n\tThu, 2 Sep 2021 16:05:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E8F560255\n\tfor ;\n\tThu, 2 Sep 2021 16:05:26 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 85F928F;\n\tThu, 2 Sep 2021 16:05:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WGqJzrSh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630591525;\n\tbh=8J9qtGlbPyn4n4xOWFMk2bQQFGjl4l2PNWTufz4huF4=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=WGqJzrShip4tqoomgw2o8+DnJWsY8onyC+3UieY/jwGewpLJzE0ZO9prV8GCFd/Z7\n\tnhBBWNqKKm9y1qyj4P/jxuxZKQ6AYOBfm5fSYN4QSvaUWdqOfshwaY25Sq4MV9Tx4j\n\tLCqUF5e2KjysxwKQzdiOhxP2K1mSW9CQm494aHm0=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-3-jacopo@jmondi.org>","From":"Kieran Bingham ","Message-ID":"","Date":"Thu, 2 Sep 2021 15:05:23 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-3-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19295,"web_url":"https://patchwork.libcamera.org/comment/19295/","msgid":"<90438e00-aa4f-8413-bae9-808d7ff9e46c@ideasonboard.com>","date":"2021-09-02T14:22:39","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 01/09/2021 15:37, Jacopo Mondi wrote:\n> When running the IPA in isolated mode, each side of the IPC boundary\n> has an instance of the ControlSerializer class.\n> \n> Each serializer instance tags with a numerical id (handle) each\n> ControlInfoMap instance it serializes, to be capable of associating\n> ControlList with it at serialization and deserialization time, and\n> increments the numerical counter for each newly serialized control info\n> map.\n> \n> Having two instances of the ControlSerializer class running in two\n> different process spaces, each instance increments its own counter,\n> preventing from maintaining a globally shared state where each\n> ControlInfoMap instance is reference by a unique identifier.\n> \n> Fix this by advancing the serial_ counter at ControlInfoMap\n> de-serialization time, so that each newly serialized map will have a\n> globally unique identifier.\n> \n> Fixes: 2c5f0ad23aa4 (\"libcamera: Add controls serializer\")\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/control_serializer.cpp | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 27360526f9eb..08cfecca3078 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\treturn {};\n> \t}\n> \n> +\t/* Keep the info map handles in sync between the two sides of IPC. */\n> +\tserial_ = std::max(serial_, hdr->handle);\n> +\n\nOk, I can see how this works now. Every serialize gets deserialized, and\nat that point you know both sides have the same serial_.\n\nSo whichever side next calls serialize() will be the one that\nincrements, and the other side will then update accordingly when it has\nit's deser...\n\nTook me a longer time to realise than I liked - but it's actually simple\nenough ;-)\n\nReviewed-by: Kieran Bingham \n\n> \t/*\n> \t * Use the ControlIdMap corresponding to the id map type. If the type\n> \t * references a globally defined id map (such as controls::controls\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B7025BDC71\n\tfor ;\n\tThu, 2 Sep 2021 14:22:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0348469166;\n\tThu, 2 Sep 2021 16:22:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8128D60255\n\tfor ;\n\tThu, 2 Sep 2021 16:22:42 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09BED317;\n\tThu, 2 Sep 2021 16:22:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Jq4avge0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630592562;\n\tbh=1oxFZrIy81pRT5my3x0jnDUtJisWM0luhxQhY6z8QjM=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=Jq4avge0MSYIHAS59k25Pi9dLhrOX1D3Pl22SoTIVogGTXcpZR51nKuTAD+u9XqwA\n\tz0yHdDITJM+RL3l3R9F8h7Ds46oEC04WszpMF4OSiBde/CoHYF0i3B0jYr+mAqPx1v\n\tXFQHrio85+oon7c2TwditjiHZaXuhypwilcC/svI=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-4-jacopo@jmondi.org>","From":"Kieran Bingham ","Message-ID":"<90438e00-aa4f-8413-bae9-808d7ff9e46c@ideasonboard.com>","Date":"Thu, 2 Sep 2021 15:22:39 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-4-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19296,"web_url":"https://patchwork.libcamera.org/comment/19296/","msgid":"<0432e00a-aa7a-bbe1-7d43-2c757b643168@ideasonboard.com>","date":"2021-09-02T14:34:01","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 01/09/2021 15:37, Jacopo Mondi wrote:\n> When a ControlList is deserialized, the code searches for a valid\n> ControlInfoMap in the local cache and use its id map to initialize the\n> list. If no valid ControlInfoMap is found, as it's usually the case\n> for lists transporting libcamera controls and properties, the globally\n> defined controls::controls id map is used unconditionally.\n> \n> This breaks the deserialization of libcamera properties, for which a\n> wrong idmap is used at construction time.\n> \n> As the serialization header now transports an id_map_type field, store\n> the idmap type at serialization time, and re-use it at\n> deserialization time to identify the correct id map.\n> \n> Also make the validation stricter by imposing to list of V4L2 controls to\n> have an associated ControlInfoMap available, as there is no globally\n> defined idmap for such controls.\n> \n> To be able to retrieve the idmap associated with a ControlList, add an\n> accessor function to the ControlList class.\n> \n> It might be worth in future using a ControlInfoMap to initialize the\n> deserialized ControlList to implement controls validation against their\n> limit. As such validation is not implemented at the moment, maintain the\n> current behaviour and initialize the control list with an idmap.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/controls.h | 1 +\n> src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++-----\n> src/libcamera/controls.cpp | 8 +++++\n> 3 files changed, 49 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 6668e4bb1053..8e5bc8b70b94 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -407,6 +407,7 @@ public:\n> \tvoid set(unsigned int id, const ControlValue &value);\n> \n> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> +\tconst ControlIdMap *idMap() const { return idmap_; }\n> \n> private:\n> \tconst ControlValue *find(unsigned int id) const;\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 08cfecca3078..d4377c024079 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> \t\tinfoMapHandle = 0;\n> \t}\n> \n> +\tconst ControlIdMap *idmap = list.idMap();\n> +\tenum ipa_controls_id_map_type idMapType;\n> +\tif (idmap == &controls::controls)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> +\telse if (idmap == &properties::properties)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> +\telse\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> +\n> \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> \tsize_t valuesSize = 0;\n> \tfor (const auto &ctrl : list)\n> @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> \thdr.entries = list.size();\n> \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> \thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\thdr.id_map_type = idMapType;\n> \n> \tbuffer.write(&hdr);\n> \n> @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t}\n> \n> \t/*\n> -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> -\t * currently the case for ControlList related to libcamera controls),\n> -\t * use the global control::control idmap.\n> +\t * Retrieve the ControlIdMap associated with the ControlList.\n> +\t *\n> +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> +\t * a valid handle has been initialized at serialization time, or by\n> +\t * using the header's id_map_type field for lists that refer to the\n> +\t * globally defined libcamera controls and properties, for which no\n> +\t * ControlInfoMap is available.\n> \t */\n> -\tconst ControlInfoMap *infoMap;\n> +\tconst ControlIdMap *idMap;\n> \tif (hdr->handle) {\n> \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\tinfoMap = iter->first;\n> +\t\tconst ControlInfoMap *infoMap = iter->first;\n> +\t\tidMap = &infoMap->idmap();\n> \t} else {\n> -\t\tinfoMap = nullptr;\n> +\t\tswitch (hdr->id_map_type) {\n> +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> +\t\t\tidMap = &controls::controls;\n> +\t\t\tbreak;\n> +\n> +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> +\t\t\tidMap = &properties::properties;\n> +\t\t\tbreak;\n> +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> +\t\t\tLOG(Serializer, Fatal)\n> +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n\nCan we have a return statement after the Fatal?\n\n(Presuming that there is no safe path forwards if Fatal is lowered to\nError on Release builds).\n\n\n> +\t\t}\n> \t}\n> \n> -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> +\t/*\n> +\t * \\todo If possible, initialize the list with the ControlInfoMap\n> +\t * so that controls can be validated against their limits.\n> +\t * Currently no validation is performed, so it's fine relying on the\n> +\t * idmap only.\n> +\t */\n> +\tASSERT(idMap);\n\nI presume this assert can only be hit through the\nIPA_CONTROL_ID_MAP_V4L2 path above?\n\nAlthough actually - if there's ever a new IPA_CONTROL_ID_MAP type added,\nit guards against that too - so the assert is good to have.\n\nOnly minors there:\n\nReviewed-by: Kieran Bingham \n\n\n> +\tControlList ctrls(*idMap);\n> \n> \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> \t\tconst struct ipa_control_value_entry *entry =\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 5c05ba4a7cd0..96625edb1380 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> * associated ControlInfoMap, nullptr is returned in that case.\n> */\n> \n> +/**\n> + * \\fn ControlList::idMap()\n> + * \\brief Retrieve the ControlId map used to construct the ControlList\n> + * \\return The ControlId map used to construct the ControlList. ControlsList\n> + * instances constructed with ControlList() have no associated idmap, nullptr is\n> + * returned in that case.\n> + */\n> +\n> const ControlValue *ControlList::find(unsigned int id) const\n> {\n> \tconst auto iter = controls_.find(id);\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CB8E4BD87D\n\tfor ;\n\tThu, 2 Sep 2021 14:34:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36F2169166;\n\tThu, 2 Sep 2021 16:34:06 +0200 (CEST)","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 4806960255\n\tfor ;\n\tThu, 2 Sep 2021 16:34:04 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C5ACC317;\n\tThu, 2 Sep 2021 16:34:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Yi19LnGY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630593243;\n\tbh=oBknbRKvUqtJ3jLZJAvtLgn2DXg6Na5kbmR4+5PQBM0=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=Yi19LnGYI9qjMfow1jEjYg8tCz2QZVlj+ATsZ6I7YkRAjjUJunKzSh3Sgz1AYFz5o\n\tZbO/MozN7hK1EbDRyIVIMy5lbIDSGd0dl2R4QKkrTefCznTWx1PIJHkfDReWxUzt1t\n\tHYjzihg6VQC5pothgMvBms96WCnMBu8EpEWRzZi8=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-5-jacopo@jmondi.org>","From":"Kieran Bingham ","Message-ID":"<0432e00a-aa7a-bbe1-7d43-2c757b643168@ideasonboard.com>","Date":"Thu, 2 Sep 2021 15:34:01 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-5-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19298,"web_url":"https://patchwork.libcamera.org/comment/19298/","msgid":"<7c04a523-513d-343a-6100-beadf603aa4e@ideasonboard.com>","date":"2021-09-02T15:03:23","subject":"Re: [libcamera-devel] [PATCH 6/6] libcamera: control_serializer:\n\tSerialize info::def()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 01/09/2021 15:38, Jacopo Mondi wrote:\n> The ControlInfo class was originally designed to only transport\n> the control's minimum and maximum values which represent the control's\n> valid limits.\n> \n> Later the default value of the control has been added to the ControlInfo\n> class, but the control serializer implementation has not been updated\n> accordingly.\n> \n> This causes issues to IPA modules making use of ControlInfo::def() as,\n> when running in isolation, they would receive a 0 value as default.\n> \n> Fix that by serializing and deserializing the additional ControlValue\n> and update the protocol description accordingly.\n\nReviewed-by: Kieran Bingham \n\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/control_serializer.cpp | 6 ++++--\n> src/libcamera/ipa_controls.cpp | 14 ++++++++------\n> 2 files changed, 12 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 7eef041a16ee..4e8045251938 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value)\n> \n> size_t ControlSerializer::binarySize(const ControlInfo &info)\n> {\n> -\treturn binarySize(info.min()) + binarySize(info.max());\n> +\treturn binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def());\n> }\n> \n> /**\n> @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer)\n> {\n> \tstore(info.min(), buffer);\n> \tstore(info.max(), buffer);\n> +\tstore(info.def(), buffer);\n> }\n> \n> /**\n> @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type,\n> \n> \tControlValue min = loadControlValue(type, b);\n> \tControlValue max = loadControlValue(type, b);\n> +\tControlValue def = loadControlValue(type, b);\n> \n> -\treturn ControlInfo(min, max);\n> +\treturn ControlInfo(min, max, def);\n> }\n> \n> /**\n> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> index fb98cda30ede..3d172d66b30f 100644\n> --- a/src/libcamera/ipa_controls.cpp\n> +++ b/src/libcamera/ipa_controls.cpp\n> @@ -108,17 +108,19 @@\n> * +-------------------------+ .\n> * / | ... | | entry[n].offset\n> * | +-------------------------+ <-----´\n> - * Data | | minimum value (#n) | \\\n> - * section | +-------------------------+ | Entry #n\n> - * | | maximum value (#n) | /\n> + * | | minimum value (#n) | \\\n> + * Data | +-------------------------+ |\n> + * section | | maximum value (#n) | | Entry #n\n> + * | +-------------------------+ |\n> + * | | default value (#n) | /\n> * | +-------------------------+\n> * \\ | ... |\n> * +-------------------------+\n> * ~~~~\n> *\n> - * The minimum and maximum value are stored in the platform's native data\n> - * format. The ipa_control_info_entry::offset field stores the offset from the\n> - * beginning of the data section to the info data.\n> + * The minimum, maximum and default value are stored in the platform's native\n> + * data format. The ipa_control_info_entry::offset field stores the offset from\n> + * the beginning of the data section to the info data.\n> *\n> * Info data in the data section shall be stored in the same order as the\n> * entries array, shall be aligned to a multiple of 8 bytes, and shall be\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 87595BD87D\n\tfor ;\n\tThu, 2 Sep 2021 15:03:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F18486916A;\n\tThu, 2 Sep 2021 17:03:27 +0200 (CEST)","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 AC11C60255\n\tfor ;\n\tThu, 2 Sep 2021 17:03:26 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2CC9418F9;\n\tThu, 2 Sep 2021 17:03:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ppyju8+k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630595006;\n\tbh=1IVpjbFoaycaT/O8iL2uH8sciAzWaFctmxyDqlFJan0=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Ppyju8+kiOaHtXWQy9o2x+7K9pW/3o51rJ5HTCr0ZcHiCMpUEuhh/L48bjK77F34O\n\t7YTiIaszLOxQgpEpKFSovE27dUWOUVWnyV0MC+k0ZWPXlwkfaGEXOpetWHY9qUJGcQ\n\tJ+htdbLdQy9Z2jKezKrzAtFDJcl0d9cRfh4/KlYY=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-7-jacopo@jmondi.org>","From":"Kieran Bingham ","Message-ID":"<7c04a523-513d-343a-6100-beadf603aa4e@ideasonboard.com>","Date":"Thu, 2 Sep 2021 16:03:23 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-7-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 6/6] libcamera: control_serializer:\n\tSerialize info::def()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19311,"web_url":"https://patchwork.libcamera.org/comment/19311/","msgid":"<20210903054228.GG968527@pyrite.rasen.tech>","date":"2021-09-03T05:42:28","subject":"Re: [libcamera-devel] [PATCH 6/6] libcamera: control_serializer:\n\tSerialize info::def()","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 01, 2021 at 04:38:00PM +0200, Jacopo Mondi wrote:\n> The ControlInfo class was originally designed to only transport\n> the control's minimum and maximum values which represent the control's\n> valid limits.\n> \n> Later the default value of the control has been added to the ControlInfo\n> class, but the control serializer implementation has not been updated\n> accordingly.\n> \n> This causes issues to IPA modules making use of ControlInfo::def() as,\n\ns/to/in/\n\n> when running in isolation, they would receive a 0 value as default.\n\ns/ as default//\n\nThere's no way to turn off receiving the \"default\" :D\n\n> \n> Fix that by serializing and deserializing the additional ControlValue\n> and update the protocol description accordingly.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/control_serializer.cpp | 6 ++++--\n> src/libcamera/ipa_controls.cpp | 14 ++++++++------\n> 2 files changed, 12 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 7eef041a16ee..4e8045251938 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value)\n> \n> size_t ControlSerializer::binarySize(const ControlInfo &info)\n> {\n> -\treturn binarySize(info.min()) + binarySize(info.max());\n> +\treturn binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def());\n> }\n> \n> /**\n> @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer)\n> {\n> \tstore(info.min(), buffer);\n> \tstore(info.max(), buffer);\n> +\tstore(info.def(), buffer);\n> }\n> \n> /**\n> @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type,\n> \n> \tControlValue min = loadControlValue(type, b);\n> \tControlValue max = loadControlValue(type, b);\n> +\tControlValue def = loadControlValue(type, b);\n> \n> -\treturn ControlInfo(min, max);\n> +\treturn ControlInfo(min, max, def);\n> }\n> \n> /**\n> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> index fb98cda30ede..3d172d66b30f 100644\n> --- a/src/libcamera/ipa_controls.cpp\n> +++ b/src/libcamera/ipa_controls.cpp\n> @@ -108,17 +108,19 @@\n> * +-------------------------+ .\n> * / | ... | | entry[n].offset\n> * | +-------------------------+ <-----´\n> - * Data | | minimum value (#n) | \\\n> - * section | +-------------------------+ | Entry #n\n> - * | | maximum value (#n) | /\n> + * | | minimum value (#n) | \\\n> + * Data | +-------------------------+ |\n> + * section | | maximum value (#n) | | Entry #n\n> + * | +-------------------------+ |\n> + * | | default value (#n) | /\n> * | +-------------------------+\n> * \\ | ... |\n> * +-------------------------+\n> * ~~~~\n> *\n> - * The minimum and maximum value are stored in the platform's native data\n> - * format. The ipa_control_info_entry::offset field stores the offset from the\n> - * beginning of the data section to the info data.\n> + * The minimum, maximum and default value are stored in the platform's native\n\ns/maximum/maximum,/\n\ns/value/values/\n\n\nReviewed-by: Paul Elder \n\n> + * data format. The ipa_control_info_entry::offset field stores the offset from\n> + * the beginning of the data section to the info data.\n> *\n> * Info data in the data section shall be stored in the same order as the\n> * entries array, shall be aligned to a multiple of 8 bytes, and shall be\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8212ABD87D\n\tfor ;\n\tFri, 3 Sep 2021 05:42:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D60916916B;\n\tFri, 3 Sep 2021 07:42:38 +0200 (CEST)","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 C172869166\n\tfor ;\n\tFri, 3 Sep 2021 07:42:36 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3BBEEBBE;\n\tFri, 3 Sep 2021 07:42:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HeV1dTum\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630647756;\n\tbh=LkV/I7+Fyc5w3ZO+VErNqaSUqrkoyHH5//G46NVtqxo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HeV1dTum1WaThgWc90C7ym6q5jiIppCOBfgw8hT3rSI2xBzw8Q1Z31OlL8vwFiBGT\n\tgW4amkZoqEpWup6Xc3jgkO9kRkLRpx0Cf8mAh+OWb2ew/+ZezjnMjUkQzGfF9xjprC\n\txZ/PJo2ypzBnJiTHgSvgU8/+B+0oA4Km1h7ruKjg=","Date":"Fri, 3 Sep 2021 14:42:28 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210903054228.GG968527@pyrite.rasen.tech>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210901143800.119118-7-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 6/6] libcamera: control_serializer:\n\tSerialize info::def()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19317,"web_url":"https://patchwork.libcamera.org/comment/19317/","msgid":"<21a7a1f9-b1d4-5156-b1d2-ff7bde4b7489@ideasonboard.com>","date":"2021-09-03T07:09:32","subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 9/1/21 8:07 PM, Jacopo Mondi wrote:\n> When running the IPA in isolated mode, each side of the IPC boundary\n> has an instance of the ControlSerializer class which is used to\n> serializer/deserialize controls before transmitting them on the wire.\n>\n> The IPAProxyWorker, which creates and manages the process the IPA runs into,\n> does not reset its ControlSerializer upon an IPA::configure() call, while\n> the IPAProxy does so, effectively creating a misalignment between the\n> two sides of the fence.\n>\n> This obviously creates issues as one side of the IPC runs with a\n> populated and possibly stale cache of ControlInfoMap references, while the\n> other side gets reset every time a new configuration is applied to the\n> Camera.\n>\n> Fix that by resetting the IPAProxyWorker ControlSerializer on an\n> IPA configure() call.\n>\n> This change fixes an issue which is easily triggered by running two\n> consecutive capture sessions with the IPA running in isolated mode:\n> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap\n>\n> Fixes: 7832e19a599e (\"utils: ipc: add templates for code generation for IPC mechanism\")\n> Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Umang Jain \n\n> ---\n> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> index 8a88bd467da7..22cbc15c5eff 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -79,6 +79,10 @@ public:\n> \n> {% for method in interface_main.methods %}\n> \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n> +\n> +{%- if method.mojom_name == \"configure\" %}\n> +\t\tcontrolSerializer_.reset();\n> +{%- endif %}\n> \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n> {% for param in method|method_param_outputs %}\n> \t\t\t{{param|name}} {{param.mojom_name}};","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 70587BDC71\n\tfor ;\n\tFri, 3 Sep 2021 07:09:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECA6C69165;\n\tFri, 3 Sep 2021 09:09:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F045469165\n\tfor ;\n\tFri, 3 Sep 2021 09:09:37 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B489BBE;\n\tFri, 3 Sep 2021 09:09:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qyTXgl4v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630652977;\n\tbh=lmk7SJSMcWi6D+n1/rHwrE/wLk4qh2TYURhs9QP01S4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=qyTXgl4vhz/hCgHbP2AZP9nW7nXWiv8usjDfoFUnxxGrPQs7diLOLR+LFAYJCI1ap\n\tH5dj4SAg6Sd5V15SY8vAyEyNwXUfe3XxMRZmZmtzPmY7+HKVJmmFYv1D+usGVRUQvp\n\tkteRCjpNze5BJ2kC9bJ8BQTFbPZdNGMj343URqVk=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-3-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"<21a7a1f9-b1d4-5156-b1d2-ff7bde4b7489@ideasonboard.com>","Date":"Fri, 3 Sep 2021 12:39:32 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-3-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19318,"web_url":"https://patchwork.libcamera.org/comment/19318/","msgid":"<1e860bfb-b5c9-a059-e085-3bfc2e85c105@ideasonboard.com>","date":"2021-09-03T07:12:40","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 9/1/21 8:07 PM, Jacopo Mondi wrote:\n> The IPA::configure() function has an IPAConfigInfo parameters which\n> contains a map of numerical indexes to ControlInfoMap instances.\n>\n> This is a leftover of the old IPA protocol, where it was not possible to\n> specify a rich interface as it is possible today and each entity\n> ControlInfoMap was indexed by a numerical id and stored in a map.\n>\n> Now that the IPA interface allows to specify parameters by name, drop the\n> map and send the sensor's control info map only.\n\nack\n\n>\n> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> can be added to IPAConfigInfo.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/ipu3.mojom | 2 +-\n> src/ipa/ipu3/ipu3.cpp | 4 ++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> 3 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index d561c2244907..2045ce909a88 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -32,7 +32,7 @@ struct IPU3Action {\n> \n> struct IPAConfigInfo {\n> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> -\tmap entityControls;\n> +\tlibcamera.ControlInfoMap sensorControls;\n> \tlibcamera.Size bdsOutputSize;\n> \tlibcamera.Size iif;\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c925cf642611..6a74f7826332 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \n> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> {\n> -\tif (configInfo.entityControls.empty()) {\n> +\tif (configInfo.sensorControls.empty()) {\n> \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n\nMaybe also change the error string to be more specific now:\n\n\tLOG(IPAIPU3, Error) << \"No sensor controls provided\";\n\nReviewed-by: Umang Jain \n\n> \t\treturn -ENODATA;\n> \t}\n> \n> \tsensorInfo_ = configInfo.sensorInfo;\n> \n> -\tctrls_ = configInfo.entityControls.at(0);\n> +\tctrls_ = configInfo.sensorControls;\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..92e869257e53 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t}\n> \n> \tipa::ipu3::IPAConfigInfo configInfo;\n> -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> \tconfigInfo.sensorInfo = sensorInfo;\n> \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> \tconfigInfo.iif = config->imguConfig().iif;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 353B7BDC71\n\tfor ;\n\tFri, 3 Sep 2021 07:12:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E39669169;\n\tFri, 3 Sep 2021 09:12:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC08869165\n\tfor ;\n\tFri, 3 Sep 2021 09:12:45 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1EA9BBE;\n\tFri, 3 Sep 2021 09:12:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"T4C44Lkp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630653165;\n\tbh=FhVnabBMoSyLaT6GuOXp3MaataY56KPTnbcWAwxTo/s=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=T4C44Lkpnx/5sqBFSign5NAqS2FtUYkGwuy4f4zyuWyJcpOYUAr7NEj1HZDeEF1qw\n\torLgjPS3AhNtmkMQcYOXSG3cUA34+s+ixMU8eQMtHZf1jAbOQTWMMffpLx6G0UAwbH\n\txUZF6RCPBbyaJ6FKU0lc9yEjIcqRwjMQ/ggQNn3k=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"<1e860bfb-b5c9-a059-e085-3bfc2e85c105@ideasonboard.com>","Date":"Fri, 3 Sep 2021 12:42:40 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19332,"web_url":"https://patchwork.libcamera.org/comment/19332/","msgid":"<20210903085732.ua7q3nrok5pmbo7a@uno.localdomain>","date":"2021-09-03T08:57:32","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote:\n> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > The IPA::configure() function has an IPAConfigInfo parameters which\n> > contains a map of numerical indexes to ControlInfoMap instances.\n> >\n> > This is a leftover of the old IPA protocol, where it was not possible to\n> > specify a rich interface as it is possible today and each entity\n> > ControlInfoMap was indexed by a numerical id and stored in a map.\n> >\n> > Now that the IPA interface allows to specify parameters by name, drop the\n> > map and send the sensor's control info map only.\n> >\n> > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> > can be added to IPAConfigInfo.\n>\n>\n> It looks like this will need a patch for\n>\n> https://git.libcamera.org/libcamera/ipu3-ipa.git/\n>\n> as well. But given that it requires manually installed AIQ libraries on\n> non CrOS build environments, perhaps it's something myself or Umang\n> should handle when this series merges.\n\nI would ofc be happy if you and Umang could take care of that, but as\nwe evolve the IPA interface on the open source IPA module, the burden\nof keeping the two in sync might get considerable on your side.\n\nIf I'm not mistaken most the changes to the IPA interface will reflect\nonly in ipu3.cpp from the above repository.\n\nIs there plan to merge the glue code between the PH and the closed\nsource IPA module in libcamera ? In this way we could keep\nat least the interface in sync when we modify the protocol.\n\nThanks\n j\n\n>\n> I wonder if we should be increasing the IPA ABI / version numbers too\n> whenever we make a change.\n>\n> But alas - it doesn't look like that's fully implemented, The mojo files\n> should have a version number in there (or even better, could mojo create\n> a hash of the interface to catch when it changes I wonder)?\n>\n> So, given that you can't update the other IPA, and you can't update the\n> IPU3 IPA Version (without further patches) ... I guess this is it ...\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 2 +-\n> > src/ipa/ipu3/ipu3.cpp | 4 ++--\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > 3 files changed, 4 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > index d561c2244907..2045ce909a88 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -32,7 +32,7 @@ struct IPU3Action {\n> >\n> > struct IPAConfigInfo {\n> > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > -\tmap entityControls;\n> > +\tlibcamera.ControlInfoMap sensorControls;\n> > \tlibcamera.Size bdsOutputSize;\n> > \tlibcamera.Size iif;\n> > };\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index c925cf642611..6a74f7826332 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> >\n> > int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> > {\n> > -\tif (configInfo.entityControls.empty()) {\n> > +\tif (configInfo.sensorControls.empty()) {\n> > \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> > \t\treturn -ENODATA;\n> > \t}\n> >\n> > \tsensorInfo_ = configInfo.sensorInfo;\n> >\n> > -\tctrls_ = configInfo.entityControls.at(0);\n> > +\tctrls_ = configInfo.sensorControls;\n> >\n> > \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> > \tif (itExp == ctrls_.end()) {\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c287bf86e79a..92e869257e53 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t}\n> >\n> > \tipa::ipu3::IPAConfigInfo configInfo;\n> > -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> > +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> > \tconfigInfo.sensorInfo = sensorInfo;\n> > \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> > \tconfigInfo.iif = config->imguConfig().iif;\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1451CBDC71\n\tfor ;\n\tFri, 3 Sep 2021 08:56:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E24D6916A;\n\tFri, 3 Sep 2021 10:56:44 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E58E960251\n\tfor ;\n\tFri, 3 Sep 2021 10:56:43 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 60FA2C0009;\n\tFri, 3 Sep 2021 08:56:43 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 10:57:32 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210903085732.ua7q3nrok5pmbo7a@uno.localdomain>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>\n\t<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19335,"web_url":"https://patchwork.libcamera.org/comment/19335/","msgid":"<20210903091653.5s2bgggba2d2yv72@uno.localdomain>","date":"2021-09-03T09:16:53","subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote:\n> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > When running the IPA in isolated mode, each side of the IPC boundary\n> > has an instance of the ControlSerializer class which is used to\n> > serializer/deserialize controls before transmitting them on the wire.\n> >\n> > The IPAProxyWorker, which creates and manages the process the IPA runs into,\n> > does not reset its ControlSerializer upon an IPA::configure() call, while\n> > the IPAProxy does so, effectively creating a misalignment between the\n> > two sides of the fence.\n> >\n> > This obviously creates issues as one side of the IPC runs with a\n> > populated and possibly stale cache of ControlInfoMap references, while the\n> > other side gets reset every time a new configuration is applied to the\n> > Camera.\n> >\n> > Fix that by resetting the IPAProxyWorker ControlSerializer on an\n> > IPA configure() call.\n> >\n> > This change fixes an issue which is easily triggered by running two\n> > consecutive capture sessions with the IPA running in isolated mode:\n> > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap\n> >\n> > Fixes: 7832e19a599e (\"utils: ipc: add templates for code generation for IPC mechanism\")\n>\n> Is it also technically fixing the patch that added the reset on the\n> other side? as that's when the misalignment begins?\n>\n\nThe 'other side' was reset from the very beginning.\n\nTo be honest I would question the usage of Fixes in libcamera, as we\ndon't have versions to backport to, but I added this mosotly for cargo\ncult.\n\nIff we want to keep using Fixes I think that's the opportune commit to\npoint to, as this patch should be ideally fixed-up in that one (if we\ncould travel back in time)\n\n>\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > index 8a88bd467da7..22cbc15c5eff 100644\n> > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > @@ -79,6 +79,10 @@ public:\n> >\n> > {% for method in interface_main.methods %}\n> > \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n> > +\n> > +{%- if method.mojom_name == \"configure\" %}\n> > +\t\tcontrolSerializer_.reset();\n>\n> one more level indentation >> ?\n>\n> It looks like we're \"inside\" this case statement, so it should be one\n> level deeper.\n>\n\nLooking at the generated code\n\n\t\tcase _IPU3Cmd::Configure: {\n\t\tcontrolSerializer_.reset();\n\n\n \tconst size_t configInfoStart = 0;\n\n\n \tIPAConfigInfo configInfo = IPADataSerializer::deserialize(\n\nYou are correct the indentation is wrong, but it does anyway matches\nthe rest of the file :)\n\n\n>\n> > +{%- endif %}\n>\n>\n> Wow, these templates are hard to parse :-)\n>\n> But ... it looks like this is right ;-)\n>\n> Reviewed-by: Kieran Bingham \n>\n> > \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n> > {% for param in method|method_param_outputs %}\n> > \t\t\t{{param|name}} {{param.mojom_name}};\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B071DBD87D\n\tfor ;\n\tFri, 3 Sep 2021 09:16:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE5DD6916D;\n\tFri, 3 Sep 2021 11:16:06 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD94060251\n\tfor ;\n\tFri, 3 Sep 2021 11:16:04 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 65BDA40006;\n\tFri, 3 Sep 2021 09:16:04 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 11:16:53 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210903091653.5s2bgggba2d2yv72@uno.localdomain>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19336,"web_url":"https://patchwork.libcamera.org/comment/19336/","msgid":"<20210903092146.pz4lfzj2xkwcuwat@uno.localdomain>","date":"2021-09-03T09:21:46","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote:\n> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > When a ControlList is deserialized, the code searches for a valid\n> > ControlInfoMap in the local cache and use its id map to initialize the\n> > list. If no valid ControlInfoMap is found, as it's usually the case\n> > for lists transporting libcamera controls and properties, the globally\n> > defined controls::controls id map is used unconditionally.\n> >\n> > This breaks the deserialization of libcamera properties, for which a\n> > wrong idmap is used at construction time.\n> >\n> > As the serialization header now transports an id_map_type field, store\n> > the idmap type at serialization time, and re-use it at\n> > deserialization time to identify the correct id map.\n> >\n> > Also make the validation stricter by imposing to list of V4L2 controls to\n> > have an associated ControlInfoMap available, as there is no globally\n> > defined idmap for such controls.\n> >\n> > To be able to retrieve the idmap associated with a ControlList, add an\n> > accessor function to the ControlList class.\n> >\n> > It might be worth in future using a ControlInfoMap to initialize the\n> > deserialized ControlList to implement controls validation against their\n> > limit. As such validation is not implemented at the moment, maintain the\n> > current behaviour and initialize the control list with an idmap.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/controls.h | 1 +\n> > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++-----\n> > src/libcamera/controls.cpp | 8 +++++\n> > 3 files changed, 49 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 6668e4bb1053..8e5bc8b70b94 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -407,6 +407,7 @@ public:\n> > \tvoid set(unsigned int id, const ControlValue &value);\n> >\n> > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > +\tconst ControlIdMap *idMap() const { return idmap_; }\n> >\n> > private:\n> > \tconst ControlValue *find(unsigned int id) const;\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 08cfecca3078..d4377c024079 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> > \t\tinfoMapHandle = 0;\n> > \t}\n> >\n> > +\tconst ControlIdMap *idmap = list.idMap();\n> > +\tenum ipa_controls_id_map_type idMapType;\n> > +\tif (idmap == &controls::controls)\n> > +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > +\telse if (idmap == &properties::properties)\n> > +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > +\telse\n> > +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> > +\n> > \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> > \tsize_t valuesSize = 0;\n> > \tfor (const auto &ctrl : list)\n> > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > \thdr.entries = list.size();\n> > \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > \thdr.data_offset = sizeof(hdr) + entriesSize;\n> > +\thdr.id_map_type = idMapType;\n> >\n> > \tbuffer.write(&hdr);\n> >\n> > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > \t}\n> >\n> > \t/*\n> > -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> > -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> > -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> > -\t * currently the case for ControlList related to libcamera controls),\n> > -\t * use the global control::control idmap.\n> > +\t * Retrieve the ControlIdMap associated with the ControlList.\n> > +\t *\n> > +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> > +\t * a valid handle has been initialized at serialization time, or by\n> > +\t * using the header's id_map_type field for lists that refer to the\n> > +\t * globally defined libcamera controls and properties, for which no\n> > +\t * ControlInfoMap is available.\n> > \t */\n> > -\tconst ControlInfoMap *infoMap;\n> > +\tconst ControlIdMap *idMap;\n> > \tif (hdr->handle) {\n> > \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> > \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > \t\t\treturn {};\n> > \t\t}\n> >\n> > -\t\tinfoMap = iter->first;\n> > +\t\tconst ControlInfoMap *infoMap = iter->first;\n> > +\t\tidMap = &infoMap->idmap();\n> > \t} else {\n> > -\t\tinfoMap = nullptr;\n> > +\t\tswitch (hdr->id_map_type) {\n> > +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> > +\t\t\tidMap = &controls::controls;\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> > +\t\t\tidMap = &properties::properties;\n> > +\t\t\tbreak;\n\nUps, missing a line break here!\n\n> > +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> > +\t\t\tLOG(Serializer, Fatal)\n> > +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n>\n> Can we have a return statement after the Fatal?\n>\n> (Presuming that there is no safe path forwards if Fatal is lowered to\n> Error on Release builds).\n>\n\nRight! In production builds this won't exit. I'll add a return\n\n>\n> > +\t\t}\n> > \t}\n> >\n> > -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> > +\t/*\n> > +\t * \\todo If possible, initialize the list with the ControlInfoMap\n> > +\t * so that controls can be validated against their limits.\n> > +\t * Currently no validation is performed, so it's fine relying on the\n> > +\t * idmap only.\n> > +\t */\n> > +\tASSERT(idMap);\n>\n> I presume this assert can only be hit through the\n> IPA_CONTROL_ID_MAP_V4L2 path above?\n\nIt could actually be hit through the if (hdr->handle) branch\nwhere the idMap is retrieved from the infoMap\n\n>\n> Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added,\n> it guards against that too - so the assert is good to have.\n\nIf we add a new IPA_CONTROL_ID_MAP type the compiler will warn that\nnot all the cases are handled in the switch statment, as I didn't add\na default: case precisely for that reason.\n\nSo yes, I could move the ASSERT() in the if() {} branch actually.\n\n>\n> Only minors there:\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > +\tControlList ctrls(*idMap);\n> >\n> > \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> > \t\tconst struct ipa_control_value_entry *entry =\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 5c05ba4a7cd0..96625edb1380 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> > * associated ControlInfoMap, nullptr is returned in that case.\n> > */\n> >\n> > +/**\n> > + * \\fn ControlList::idMap()\n> > + * \\brief Retrieve the ControlId map used to construct the ControlList\n> > + * \\return The ControlId map used to construct the ControlList. ControlsList\n> > + * instances constructed with ControlList() have no associated idmap, nullptr is\n> > + * returned in that case.\n> > + */\n> > +\n> > const ControlValue *ControlList::find(unsigned int id) const\n> > {\n> > \tconst auto iter = controls_.find(id);\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A9480BDC71\n\tfor ;\n\tFri, 3 Sep 2021 09:20:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3797C69165;\n\tFri, 3 Sep 2021 11:20:59 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E083660251\n\tfor ;\n\tFri, 3 Sep 2021 11:20:57 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 5DF27C0003;\n\tFri, 3 Sep 2021 09:20:57 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 11:21:46 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210903092146.pz4lfzj2xkwcuwat@uno.localdomain>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-5-jacopo@jmondi.org>\n\t<0432e00a-aa7a-bbe1-7d43-2c757b643168@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<0432e00a-aa7a-bbe1-7d43-2c757b643168@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19337,"web_url":"https://patchwork.libcamera.org/comment/19337/","msgid":"<20210903093235.GH968527@pyrite.rasen.tech>","date":"2021-09-03T09:32:35","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 01, 2021 at 04:37:55PM +0200, Jacopo Mondi wrote:\n> The IPA::configure() function has an IPAConfigInfo parameters which\n> contains a map of numerical indexes to ControlInfoMap instances.\n> \n> This is a leftover of the old IPA protocol, where it was not possible to\n> specify a rich interface as it is possible today and each entity\n> ControlInfoMap was indexed by a numerical id and stored in a map.\n> \n> Now that the IPA interface allows to specify parameters by name, drop the\n> map and send the sensor's control info map only.\n> \n> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> can be added to IPAConfigInfo.\n> \n> Signed-off-by: Jacopo Mondi \n\nLooks good.\n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/ipa/ipu3.mojom | 2 +-\n> src/ipa/ipu3/ipu3.cpp | 4 ++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> 3 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index d561c2244907..2045ce909a88 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -32,7 +32,7 @@ struct IPU3Action {\n> \n> struct IPAConfigInfo {\n> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> -\tmap entityControls;\n> +\tlibcamera.ControlInfoMap sensorControls;\n> \tlibcamera.Size bdsOutputSize;\n> \tlibcamera.Size iif;\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c925cf642611..6a74f7826332 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \n> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> {\n> -\tif (configInfo.entityControls.empty()) {\n> +\tif (configInfo.sensorControls.empty()) {\n> \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> \t\treturn -ENODATA;\n> \t}\n> \n> \tsensorInfo_ = configInfo.sensorInfo;\n> \n> -\tctrls_ = configInfo.entityControls.at(0);\n> +\tctrls_ = configInfo.sensorControls;\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..92e869257e53 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t}\n> \n> \tipa::ipu3::IPAConfigInfo configInfo;\n> -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> \tconfigInfo.sensorInfo = sensorInfo;\n> \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> \tconfigInfo.iif = config->imguConfig().iif;\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D5A46BD87D\n\tfor ;\n\tFri, 3 Sep 2021 09:32:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47EDE60251;\n\tFri, 3 Sep 2021 11:32:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C799A60251\n\tfor ;\n\tFri, 3 Sep 2021 11:32:43 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4155BBBE;\n\tFri, 3 Sep 2021 11:32:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mH4GJ7TT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630661563;\n\tbh=3IzesvGN1FgLKA9eBkuvzyKab3z+lI7haU3yEG/39u8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mH4GJ7TTOgIUkrDWUK8OQNaN+d6OIYM2y2+KR+mwzILqANsTDQXZnKCH/yxtqNObs\n\tHFNAE62JbX0IsvvFkyHq47/mbnCJvphqkpmLGYMyy9vm/LeVQ7gMQVDZPLXAx7Kqul\n\tudfq1Eev8shFZuc9/0UYg/QzMZhm2rOMQcEYxodM=","Date":"Fri, 3 Sep 2021 18:32:35 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210903093235.GH968527@pyrite.rasen.tech>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210901143800.119118-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19338,"web_url":"https://patchwork.libcamera.org/comment/19338/","msgid":"<20210903093330.GI968527@pyrite.rasen.tech>","date":"2021-09-03T09:33:30","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote:\n> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > The IPA::configure() function has an IPAConfigInfo parameters which\n> > contains a map of numerical indexes to ControlInfoMap instances.\n> > \n> > This is a leftover of the old IPA protocol, where it was not possible to\n> > specify a rich interface as it is possible today and each entity\n> > ControlInfoMap was indexed by a numerical id and stored in a map.\n> > \n> > Now that the IPA interface allows to specify parameters by name, drop the\n> > map and send the sensor's control info map only.\n> > \n> > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> > can be added to IPAConfigInfo.\n> \n> \n> It looks like this will need a patch for\n> \n> https://git.libcamera.org/libcamera/ipu3-ipa.git/\n> \n> as well. But given that it requires manually installed AIQ libraries on\n> non CrOS build environments, perhaps it's something myself or Umang\n> should handle when this series merges.\n> \n> I wonder if we should be increasing the IPA ABI / version numbers too\n> whenever we make a change.\n\nWe don't have a release yet so it should be fine, right? :p\n\nI guess we gotta start working on this for real... I'll prepare\na summary + discussion points.\n\n\nPaul\n\n> \n> But alas - it doesn't look like that's fully implemented, The mojo files\n> should have a version number in there (or even better, could mojo create\n> a hash of the interface to catch when it changes I wonder)?\n> \n> So, given that you can't update the other IPA, and you can't update the\n> IPU3 IPA Version (without further patches) ... I guess this is it ...\n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 2 +-\n> > src/ipa/ipu3/ipu3.cpp | 4 ++--\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > 3 files changed, 4 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > index d561c2244907..2045ce909a88 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -32,7 +32,7 @@ struct IPU3Action {\n> > \n> > struct IPAConfigInfo {\n> > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > -\tmap entityControls;\n> > +\tlibcamera.ControlInfoMap sensorControls;\n> > \tlibcamera.Size bdsOutputSize;\n> > \tlibcamera.Size iif;\n> > };\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index c925cf642611..6a74f7826332 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> > \n> > int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> > {\n> > -\tif (configInfo.entityControls.empty()) {\n> > +\tif (configInfo.sensorControls.empty()) {\n> > \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> > \t\treturn -ENODATA;\n> > \t}\n> > \n> > \tsensorInfo_ = configInfo.sensorInfo;\n> > \n> > -\tctrls_ = configInfo.entityControls.at(0);\n> > +\tctrls_ = configInfo.sensorControls;\n> > \n> > \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> > \tif (itExp == ctrls_.end()) {\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c287bf86e79a..92e869257e53 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t}\n> > \n> > \tipa::ipu3::IPAConfigInfo configInfo;\n> > -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> > +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> > \tconfigInfo.sensorInfo = sensorInfo;\n> > \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> > \tconfigInfo.iif = config->imguConfig().iif;\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EB84DBDC71\n\tfor ;\n\tFri, 3 Sep 2021 09:33:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC53D6916A;\n\tFri, 3 Sep 2021 11:33:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F92560251\n\tfor ;\n\tFri, 3 Sep 2021 11:33:39 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7187FBBE;\n\tFri, 3 Sep 2021 11:33:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lPkUIex0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630661618;\n\tbh=wRlSk8YFnyzHn2+uugb5Zbcoy9BlY6rlbyUljE2xDhA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lPkUIex0DweY5xHSk/qd1bivYgNLsovveKPe65tAn0sdg/n97r5URQGuc58O6X51n\n\tIJTFhCdmW3wCNqpCoc0NoUeziFOLI4t3c5aPQ6I5WJhMpq7xnRtxezI0yKJUO8i9q4\n\tDEMcpngID78gW83WE6P1K3IXyMwejy2uWnbJvfvo=","Date":"Fri, 3 Sep 2021 18:33:30 +0900","From":"paul.elder@ideasonboard.com","To":"Kieran Bingham ","Message-ID":"<20210903093330.GI968527@pyrite.rasen.tech>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>\n\t<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19340,"web_url":"https://patchwork.libcamera.org/comment/19340/","msgid":"<20210903100251.GK968527@pyrite.rasen.tech>","date":"2021-09-03T10:02:51","subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 01, 2021 at 04:37:56PM +0200, Jacopo Mondi wrote:\n> When running the IPA in isolated mode, each side of the IPC boundary\n> has an instance of the ControlSerializer class which is used to\n> serializer/deserialize controls before transmitting them on the wire.\n> \n> The IPAProxyWorker, which creates and manages the process the IPA runs into,\n\ns/into/in/\n\n> does not reset its ControlSerializer upon an IPA::configure() call, while\n> the IPAProxy does so, effectively creating a misalignment between the\n\ns/ so//\n\n> two sides of the fence.\n> \n> This obviously creates issues as one side of the IPC runs with a\n> populated and possibly stale cache of ControlInfoMap references, while the\n> other side gets reset every time a new configuration is applied to the\n> Camera.\n> \n> Fix that by resetting the IPAProxyWorker ControlSerializer on an\n> IPA configure() call.\n> \n> This change fixes an issue which is easily triggered by running two\n\ns/ / /\n\n> consecutive capture sessions with the IPA running in isolated mode:\n> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap\n> \n> Fixes: 7832e19a599e (\"utils: ipc: add templates for code generation for IPC mechanism\")\n> Signed-off-by: Jacopo Mondi \n\nLooks good.\n\nReviewed-by: Paul Elder \n\n> ---\n> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> index 8a88bd467da7..22cbc15c5eff 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -79,6 +79,10 @@ public:\n> \n> {% for method in interface_main.methods %}\n> \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n> +\n> +{%- if method.mojom_name == \"configure\" %}\n> +\t\tcontrolSerializer_.reset();\n> +{%- endif %}\n> \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n> {% for param in method|method_param_outputs %}\n> \t\t\t{{param|name}} {{param.mojom_name}};\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 391CFBD87D\n\tfor ;\n\tFri, 3 Sep 2021 10:03:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E75F69168;\n\tFri, 3 Sep 2021 12:03:00 +0200 (CEST)","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 52CE769167\n\tfor ;\n\tFri, 3 Sep 2021 12:02:59 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B60CFBBE;\n\tFri, 3 Sep 2021 12:02:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DZp8dUwz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630663378;\n\tbh=mPRSz/5hTVc/60uPCOul3BH2jS7uaQYPt2fMZrhj+3c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DZp8dUwzNkkUpcNY//bc+DQnAlzW4EsoS7jlMa7HoiIqKwUiq7bamJEz1vXap3XDp\n\tJ0BAkUiX89ZHxlPA6K5YQC2yPcXUoUtCZcq9OsfDY4/uu0OD5qwwe3xOJoDs/ujMRW\n\tY26Lv2w5X4Ay4Y597vfMCPSwvnJcM++I4PPfndqw=","Date":"Fri, 3 Sep 2021 19:02:51 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210903100251.GK968527@pyrite.rasen.tech>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210901143800.119118-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19341,"web_url":"https://patchwork.libcamera.org/comment/19341/","msgid":"<20210903100528.GL968527@pyrite.rasen.tech>","date":"2021-09-03T10:05:28","subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote:\n> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > When running the IPA in isolated mode, each side of the IPC boundary\n> > has an instance of the ControlSerializer class which is used to\n> > serializer/deserialize controls before transmitting them on the wire.\n> > \n> > The IPAProxyWorker, which creates and manages the process the IPA runs into,\n> > does not reset its ControlSerializer upon an IPA::configure() call, while\n> > the IPAProxy does so, effectively creating a misalignment between the\n> > two sides of the fence.\n> > \n> > This obviously creates issues as one side of the IPC runs with a\n> > populated and possibly stale cache of ControlInfoMap references, while the\n> > other side gets reset every time a new configuration is applied to the\n> > Camera.\n> > \n> > Fix that by resetting the IPAProxyWorker ControlSerializer on an\n> > IPA configure() call.\n> > \n> > This change fixes an issue which is easily triggered by running two\n> > consecutive capture sessions with the IPA running in isolated mode:\n> > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap\n> > \n> > Fixes: 7832e19a599e (\"utils: ipc: add templates for code generation for IPC mechanism\")\n> \n> Is it also technically fixing the patch that added the reset on the\n> other side? as that's when the misalignment begins?\n> \n> \n> \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> > \n> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > index 8a88bd467da7..22cbc15c5eff 100644\n> > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > @@ -79,6 +79,10 @@ public:\n> > \n> > {% for method in interface_main.methods %}\n> > \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n> > +\n> > +{%- if method.mojom_name == \"configure\" %}\n> > +\t\tcontrolSerializer_.reset();\n> \n> one more level indentation >> ?\n\nYeah, that one line should be indented one more in to align in the case\nstatement.\n\n\nPaul\n\n> \n> It looks like we're \"inside\" this case statement, so it should be one\n> level deeper.\n> \n> \n> > +{%- endif %}\n> \n> \n> Wow, these templates are hard to parse :-)\n> \n> But ... it looks like this is right ;-)\n> \n> Reviewed-by: Kieran Bingham \n> \n> > \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n> > {% for param in method|method_param_outputs %}\n> > \t\t\t{{param|name}} {{param.mojom_name}};\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B0117BDC71\n\tfor ;\n\tFri, 3 Sep 2021 10:05:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34EB06916A;\n\tFri, 3 Sep 2021 12:05:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E2F5B69167\n\tfor ;\n\tFri, 3 Sep 2021 12:05:35 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F3FE9BBE;\n\tFri, 3 Sep 2021 12:05:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hO6zL/tf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630663535;\n\tbh=m+5ItdFsKoqnq9NQqAuPyaYa0qKIT1xbrWn0HYabQdc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hO6zL/tfez1ZLsng6uKn1zGAT51nK8Xrlcv0b83/ZmiPcXSbYIlIVgbjwuDyicKWJ\n\tCPbMBAl2qpdqOkbYUNkxo/tcIZU+nTwlvaGCNKQlDFT3Ek9DOPpCAjeZJo6TIAZyHl\n\tkWjPtcw6ekcDyy++eNGdqe4oll0gwMRy45B5HI4Q=","Date":"Fri, 3 Sep 2021 19:05:28 +0900","From":"paul.elder@ideasonboard.com","To":"Kieran Bingham ","Message-ID":"<20210903100528.GL968527@pyrite.rasen.tech>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19350,"web_url":"https://patchwork.libcamera.org/comment/19350/","msgid":"<20210903110440.GM968527@pyrite.rasen.tech>","date":"2021-09-03T11:04:40","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote:\n> When running the IPA in isolated mode, each side of the IPC boundary\n> has an instance of the ControlSerializer class.\n> \n> Each serializer instance tags with a numerical id (handle) each\n> ControlInfoMap instance it serializes, to be capable of associating\n> ControlList with it at serialization and deserialization time, and\n> increments the numerical counter for each newly serialized control info\n> map.\n> \n> Having two instances of the ControlSerializer class running in two\n> different process spaces, each instance increments its own counter,\n> preventing from maintaining a globally shared state where each\n> ControlInfoMap instance is reference by a unique identifier.\n> \n> Fix this by advancing the serial_ counter at ControlInfoMap\n> de-serialization time, so that each newly serialized map will have a\n> globally unique identifier.\n> \n> Fixes: 2c5f0ad23aa4 (\"libcamera: Add controls serializer\")\n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/control_serializer.cpp | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 27360526f9eb..08cfecca3078 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\treturn {};\n> \t}\n> \n> +\t/* Keep the info map handles in sync between the two sides of IPC. */\n> +\tserial_ = std::max(serial_, hdr->handle);\n> +\n> \t/*\n> \t * Use the ControlIdMap corresponding to the id map type. If the type\n> \t * references a globally defined id map (such as controls::controls\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0CA5EBD87D\n\tfor ;\n\tFri, 3 Sep 2021 11:04:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E29769169;\n\tFri, 3 Sep 2021 13:04:49 +0200 (CEST)","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 B50E369168\n\tfor ;\n\tFri, 3 Sep 2021 13:04:47 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 39CD2BBE;\n\tFri, 3 Sep 2021 13:04:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aczQegPt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630667087;\n\tbh=NCZhON5dGZMI7YAcB6zdoSAppgbMquZyqPgiCThHs4A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aczQegPtLLivN3vei9MdZJTIUeEl39zCV2xkXydnxX6jBoNHTwz55YQqftu95civT\n\tlSyUqEftOYhxJg+uHPWXlUvsXGWApPes9Lc8G00YYveM2+71EN1qRfBk1+duq8LmIo\n\tpKHkgdPZ99ZeScHBXAdexBk7pE0YiyspDkZUIWcY=","Date":"Fri, 3 Sep 2021 20:04:40 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210903110440.GM968527@pyrite.rasen.tech>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210901143800.119118-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19351,"web_url":"https://patchwork.libcamera.org/comment/19351/","msgid":"<20210903110740.GN968527@pyrite.rasen.tech>","date":"2021-09-03T11:07:40","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 01, 2021 at 04:37:58PM +0200, Jacopo Mondi wrote:\n> When a ControlList is deserialized, the code searches for a valid\n> ControlInfoMap in the local cache and use its id map to initialize the\n> list. If no valid ControlInfoMap is found, as it's usually the case\n> for lists transporting libcamera controls and properties, the globally\n> defined controls::controls id map is used unconditionally.\n> \n> This breaks the deserialization of libcamera properties, for which a\n> wrong idmap is used at construction time.\n> \n> As the serialization header now transports an id_map_type field, store\n> the idmap type at serialization time, and re-use it at\n> deserialization time to identify the correct id map.\n> \n> Also make the validation stricter by imposing to list of V4L2 controls to\n> have an associated ControlInfoMap available, as there is no globally\n> defined idmap for such controls.\n> \n> To be able to retrieve the idmap associated with a ControlList, add an\n> accessor function to the ControlList class.\n> \n> It might be worth in future using a ControlInfoMap to initialize the\n> deserialized ControlList to implement controls validation against their\n> limit. As such validation is not implemented at the moment, maintain the\n> current behaviour and initialize the control list with an idmap.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/controls.h | 1 +\n> src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++-----\n> src/libcamera/controls.cpp | 8 +++++\n> 3 files changed, 49 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 6668e4bb1053..8e5bc8b70b94 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -407,6 +407,7 @@ public:\n> \tvoid set(unsigned int id, const ControlValue &value);\n> \n> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> +\tconst ControlIdMap *idMap() const { return idmap_; }\n> \n> private:\n> \tconst ControlValue *find(unsigned int id) const;\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 08cfecca3078..d4377c024079 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> \t\tinfoMapHandle = 0;\n> \t}\n> \n> +\tconst ControlIdMap *idmap = list.idMap();\n> +\tenum ipa_controls_id_map_type idMapType;\n> +\tif (idmap == &controls::controls)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> +\telse if (idmap == &properties::properties)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> +\telse\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> +\n> \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> \tsize_t valuesSize = 0;\n> \tfor (const auto &ctrl : list)\n> @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> \thdr.entries = list.size();\n> \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> \thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\thdr.id_map_type = idMapType;\n> \n> \tbuffer.write(&hdr);\n> \n> @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t}\n> \n> \t/*\n> -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> -\t * currently the case for ControlList related to libcamera controls),\n> -\t * use the global control::control idmap.\n> +\t * Retrieve the ControlIdMap associated with the ControlList.\n> +\t *\n> +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> +\t * a valid handle has been initialized at serialization time, or by\n> +\t * using the header's id_map_type field for lists that refer to the\n> +\t * globally defined libcamera controls and properties, for which no\n> +\t * ControlInfoMap is available.\n> \t */\n> -\tconst ControlInfoMap *infoMap;\n> +\tconst ControlIdMap *idMap;\n> \tif (hdr->handle) {\n> \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\tinfoMap = iter->first;\n> +\t\tconst ControlInfoMap *infoMap = iter->first;\n> +\t\tidMap = &infoMap->idmap();\n> \t} else {\n> -\t\tinfoMap = nullptr;\n> +\t\tswitch (hdr->id_map_type) {\n> +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> +\t\t\tidMap = &controls::controls;\n> +\t\t\tbreak;\n> +\n> +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> +\t\t\tidMap = &properties::properties;\n> +\t\t\tbreak;\n> +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> +\t\t\tLOG(Serializer, Fatal)\n> +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n> +\t\t}\n> \t}\n> \n> -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> +\t/*\n> +\t * \\todo If possible, initialize the list with the ControlInfoMap\n> +\t * so that controls can be validated against their limits.\n> +\t * Currently no validation is performed, so it's fine relying on the\n> +\t * idmap only.\n> +\t */\n> +\tASSERT(idMap);\n> +\tControlList ctrls(*idMap);\n> \n> \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> \t\tconst struct ipa_control_value_entry *entry =\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 5c05ba4a7cd0..96625edb1380 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> * associated ControlInfoMap, nullptr is returned in that case.\n> */\n> \n> +/**\n> + * \\fn ControlList::idMap()\n> + * \\brief Retrieve the ControlId map used to construct the ControlList\n> + * \\return The ControlId map used to construct the ControlList. ControlsList\n> + * instances constructed with ControlList() have no associated idmap, nullptr is\n> + * returned in that case.\n> + */\n> +\n> const ControlValue *ControlList::find(unsigned int id) const\n> {\n> \tconst auto iter = controls_.find(id);\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A99A9BD87D\n\tfor ;\n\tFri, 3 Sep 2021 11:07:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CEFE6916B;\n\tFri, 3 Sep 2021 13:07:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80D1A69168\n\tfor ;\n\tFri, 3 Sep 2021 13:07:48 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F0F38BBE;\n\tFri, 3 Sep 2021 13:07:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D8wgTCRM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630667268;\n\tbh=aziGwFE8WfUcr/VA5RE4S2oUufdtsG0hPEVLsGt17Nw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D8wgTCRM85qE1SOkBjbjY4tjVIj9gkWmkp0b54g42+/f8DAo7hveVmzEO2Y9nMjc4\n\tBy1JyvBjP5ZtXWGXSehFBqCfy2us8MRGfXVneo30YV5Avk+swV0edT36mb7FCMq/jX\n\tUmqSjf9jBOaXWHs5Ry3Ho6gWBgV6lul6E5gODBnE=","Date":"Fri, 3 Sep 2021 20:07:40 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210903110740.GN968527@pyrite.rasen.tech>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210901143800.119118-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19354,"web_url":"https://patchwork.libcamera.org/comment/19354/","msgid":"","date":"2021-09-03T11:12:01","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 03, 2021 at 06:33:30PM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote:\n> > On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > > The IPA::configure() function has an IPAConfigInfo parameters which\n> > > contains a map of numerical indexes to ControlInfoMap instances.\n> > > \n> > > This is a leftover of the old IPA protocol, where it was not possible to\n> > > specify a rich interface as it is possible today and each entity\n> > > ControlInfoMap was indexed by a numerical id and stored in a map.\n> > > \n> > > Now that the IPA interface allows to specify parameters by name, drop the\n> > > map and send the sensor's control info map only.\n> > > \n> > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> > > can be added to IPAConfigInfo.\n> > \n> > \n> > It looks like this will need a patch for\n> > \n> > https://git.libcamera.org/libcamera/ipu3-ipa.git/\n> > \n> > as well. But given that it requires manually installed AIQ libraries on\n> > non CrOS build environments, perhaps it's something myself or Umang\n> > should handle when this series merges.\n> > \n> > I wonder if we should be increasing the IPA ABI / version numbers too\n> > whenever we make a change.\n> \n> We don't have a release yet so it should be fine, right? :p\n> \n> I guess we gotta start working on this for real... I'll prepare\n> a summary + discussion points.\n\nAnother can of worms :-) It certainly needs to be addressed, but will\nneed to include .mojom versioning, and protocol stability. We can start\ndiscussions, but I don't think the implementation is the priority at\nthis point.\n\n> > But alas - it doesn't look like that's fully implemented, The mojo files\n> > should have a version number in there (or even better, could mojo create\n> > a hash of the interface to catch when it changes I wonder)?\n> > \n> > So, given that you can't update the other IPA, and you can't update the\n> > IPU3 IPA Version (without further patches) ... I guess this is it ...\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 2 +-\n> > > src/ipa/ipu3/ipu3.cpp | 4 ++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > 3 files changed, 4 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > index d561c2244907..2045ce909a88 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -32,7 +32,7 @@ struct IPU3Action {\n> > > \n> > > struct IPAConfigInfo {\n> > > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > -\tmap entityControls;\n> > > +\tlibcamera.ControlInfoMap sensorControls;\n> > > \tlibcamera.Size bdsOutputSize;\n> > > \tlibcamera.Size iif;\n> > > };\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index c925cf642611..6a74f7826332 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> > > \n> > > int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> > > {\n> > > -\tif (configInfo.entityControls.empty()) {\n> > > +\tif (configInfo.sensorControls.empty()) {\n> > > \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> > > \t\treturn -ENODATA;\n> > > \t}\n> > > \n> > > \tsensorInfo_ = configInfo.sensorInfo;\n> > > \n> > > -\tctrls_ = configInfo.entityControls.at(0);\n> > > +\tctrls_ = configInfo.sensorControls;\n> > > \n> > > \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> > > \tif (itExp == ctrls_.end()) {\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index c287bf86e79a..92e869257e53 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \t}\n> > > \n> > > \tipa::ipu3::IPAConfigInfo configInfo;\n> > > -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> > > +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> > > \tconfigInfo.sensorInfo = sensorInfo;\n> > > \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> > > \tconfigInfo.iif = config->imguConfig().iif;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9F381BDC71\n\tfor ;\n\tFri, 3 Sep 2021 11:12:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 548F269169;\n\tFri, 3 Sep 2021 13:12:19 +0200 (CEST)","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 CF39069168\n\tfor ;\n\tFri, 3 Sep 2021 13:12:17 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55C6CBBE;\n\tFri, 3 Sep 2021 13:12:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZLoZjB2v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630667537;\n\tbh=agsUs62xoJThelKCponMnLR2Vz+HP0lugP58EbnplEY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZLoZjB2vEBXlDHCMG8mT4SK7zBZlCnkozi2f6B1Lu60Uf/ZR5ZGITbJ2OJBc+7z25\n\ti2QrDjdyo/2qocdxo48HIqWErlyVh3xww1ufuV+uFLEtbhac95O+oHrU8ZpmXIoCIg\n\t0t5hzNkvv8U6Yn8q4j2vEx4dWB3WxSjJ82ldnNFY=","Date":"Fri, 3 Sep 2021 14:12:01 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>\n\t<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>\n\t<20210903093330.GI968527@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210903093330.GI968527@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19356,"web_url":"https://patchwork.libcamera.org/comment/19356/","msgid":"","date":"2021-09-03T11:15:45","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Sep 03, 2021 at 10:57:32AM +0200, Jacopo Mondi wrote:\n> On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote:\n> > On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > > The IPA::configure() function has an IPAConfigInfo parameters which\n> > > contains a map of numerical indexes to ControlInfoMap instances.\n> > >\n> > > This is a leftover of the old IPA protocol, where it was not possible to\n> > > specify a rich interface as it is possible today and each entity\n> > > ControlInfoMap was indexed by a numerical id and stored in a map.\n> > >\n> > > Now that the IPA interface allows to specify parameters by name, drop the\n> > > map and send the sensor's control info map only.\n> > >\n> > > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> > > can be added to IPAConfigInfo.\n> >\n> > It looks like this will need a patch for\n> >\n> > https://git.libcamera.org/libcamera/ipu3-ipa.git/\n> >\n> > as well. But given that it requires manually installed AIQ libraries on\n> > non CrOS build environments, perhaps it's something myself or Umang\n> > should handle when this series merges.\n> \n> I would ofc be happy if you and Umang could take care of that, but as\n> we evolve the IPA interface on the open source IPA module, the burden\n> of keeping the two in sync might get considerable on your side.\n> \n> If I'm not mistaken most the changes to the IPA interface will reflect\n> only in ipu3.cpp from the above repository.\n> \n> Is there plan to merge the glue code between the PH and the closed\n> source IPA module in libcamera ? In this way we could keep\n> at least the interface in sync when we modify the protocol.\n\nNo, closed-source implementations won't be merged in libcamera.\n\nThis is a temporary issue until we stabilize the ABI, after that we'll\nneed versioning (and of course we'll also need to carefully design v1 to\nmake sure a v2 will never be needed ;-)).\n\n> > I wonder if we should be increasing the IPA ABI / version numbers too\n> > whenever we make a change.\n> >\n> > But alas - it doesn't look like that's fully implemented, The mojo files\n> > should have a version number in there (or even better, could mojo create\n> > a hash of the interface to catch when it changes I wonder)?\n> >\n> > So, given that you can't update the other IPA, and you can't update the\n> > IPU3 IPA Version (without further patches) ... I guess this is it ...\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 2 +-\n> > > src/ipa/ipu3/ipu3.cpp | 4 ++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > 3 files changed, 4 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > index d561c2244907..2045ce909a88 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -32,7 +32,7 @@ struct IPU3Action {\n> > >\n> > > struct IPAConfigInfo {\n> > > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > -\tmap entityControls;\n> > > +\tlibcamera.ControlInfoMap sensorControls;\n> > > \tlibcamera.Size bdsOutputSize;\n> > > \tlibcamera.Size iif;\n> > > };\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index c925cf642611..6a74f7826332 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> > >\n> > > int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> > > {\n> > > -\tif (configInfo.entityControls.empty()) {\n> > > +\tif (configInfo.sensorControls.empty()) {\n> > > \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> > > \t\treturn -ENODATA;\n> > > \t}\n> > >\n> > > \tsensorInfo_ = configInfo.sensorInfo;\n> > >\n> > > -\tctrls_ = configInfo.entityControls.at(0);\n> > > +\tctrls_ = configInfo.sensorControls;\n> > >\n> > > \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> > > \tif (itExp == ctrls_.end()) {\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index c287bf86e79a..92e869257e53 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \t}\n> > >\n> > > \tipa::ipu3::IPAConfigInfo configInfo;\n> > > -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> > > +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> > > \tconfigInfo.sensorInfo = sensorInfo;\n> > > \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> > > \tconfigInfo.iif = config->imguConfig().iif;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D1469BD87D\n\tfor ;\n\tFri, 3 Sep 2021 11:16:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E0D469168;\n\tFri, 3 Sep 2021 13:16:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BABDF69167\n\tfor ;\n\tFri, 3 Sep 2021 13:16:01 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4CBBCBBE;\n\tFri, 3 Sep 2021 13:16:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LOiYuDD+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630667761;\n\tbh=23ey19dC2RRkkFrYZLahWV3uUUalPoavfW5T/eIeXL8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LOiYuDD+BSYyOhJ25hXHxUkKqlsNOLaSyjWTgDOShaDrwJ2Qowo2aR4rjoTADYFer\n\tfqzl6QDaKVMK+yQxGKHy1mWCESHCI1k++mfghETD1O9C79B1lvA3AmTarhtsI19FTv\n\t+07s8cH1qAapxkkXOg2hJQkaALbqrRuvC23VlNGA=","Date":"Fri, 3 Sep 2021 14:15:45 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>\n\t<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>\n\t<20210903085732.ua7q3nrok5pmbo7a@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210903085732.ua7q3nrok5pmbo7a@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19357,"web_url":"https://patchwork.libcamera.org/comment/19357/","msgid":"","date":"2021-09-03T11:18:14","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Sep 01, 2021 at 04:37:55PM +0200, Jacopo Mondi wrote:\n> The IPA::configure() function has an IPAConfigInfo parameters which\n> contains a map of numerical indexes to ControlInfoMap instances.\n> \n> This is a leftover of the old IPA protocol, where it was not possible to\n> specify a rich interface as it is possible today and each entity\n> ControlInfoMap was indexed by a numerical id and stored in a map.\n> \n> Now that the IPA interface allows to specify parameters by name, drop the\n> map and send the sensor's control info map only.\n> \n> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> can be added to IPAConfigInfo.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> include/libcamera/ipa/ipu3.mojom | 2 +-\n> src/ipa/ipu3/ipu3.cpp | 4 ++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> 3 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index d561c2244907..2045ce909a88 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -32,7 +32,7 @@ struct IPU3Action {\n> \n> struct IPAConfigInfo {\n> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> -\tmap entityControls;\n> +\tlibcamera.ControlInfoMap sensorControls;\n> \tlibcamera.Size bdsOutputSize;\n> \tlibcamera.Size iif;\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c925cf642611..6a74f7826332 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \n> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> {\n> -\tif (configInfo.entityControls.empty()) {\n> +\tif (configInfo.sensorControls.empty()) {\n> \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> \t\treturn -ENODATA;\n> \t}\n> \n> \tsensorInfo_ = configInfo.sensorInfo;\n> \n> -\tctrls_ = configInfo.entityControls.at(0);\n> +\tctrls_ = configInfo.sensorControls;\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c287bf86e79a..92e869257e53 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t}\n> \n> \tipa::ipu3::IPAConfigInfo configInfo;\n> -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> \tconfigInfo.sensorInfo = sensorInfo;\n> \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> \tconfigInfo.iif = config->imguConfig().iif;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7AEDEBDC71\n\tfor ;\n\tFri, 3 Sep 2021 11:18:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AAE96916D;\n\tFri, 3 Sep 2021 13:18:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80B0369167\n\tfor ;\n\tFri, 3 Sep 2021 13:18:31 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BB3EBBE;\n\tFri, 3 Sep 2021 13:18:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BPJbBvob\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630667911;\n\tbh=E1bczQavAsHm/5lerf+KGTIjWuFZIlUPfoavCenOU5s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BPJbBvobmx4BNDnSkppqOgqj6/BlxAGL+AVMrjPohHYTu/rLQb0fzpIk+RZ1kFA1X\n\tvSyEpKZjQChIblxgiDesVILWuJJUlsjzV6DyR2/C4n2Z5v1oWdSM8lJp7r7YZulLhZ\n\ty3KXhveeByWjSq8CVbnpwJhb01gAlkPMmED+lpBs=","Date":"Fri, 3 Sep 2021 14:18:14 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210901143800.119118-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19360,"web_url":"https://patchwork.libcamera.org/comment/19360/","msgid":"","date":"2021-09-03T11:29:55","subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Sep 01, 2021 at 04:37:56PM +0200, Jacopo Mondi wrote:\n> When running the IPA in isolated mode, each side of the IPC boundary\n> has an instance of the ControlSerializer class which is used to\n> serializer/deserialize controls before transmitting them on the wire.\n> \n> The IPAProxyWorker, which creates and manages the process the IPA runs into,\n> does not reset its ControlSerializer upon an IPA::configure() call, while\n> the IPAProxy does so, effectively creating a misalignment between the\n> two sides of the fence.\n> \n> This obviously creates issues as one side of the IPC runs with a\n> populated and possibly stale cache of ControlInfoMap references, while the\n> other side gets reset every time a new configuration is applied to the\n> Camera.\n> \n> Fix that by resetting the IPAProxyWorker ControlSerializer on an\n> IPA configure() call.\n> \n> This change fixes an issue which is easily triggered by running two\n> consecutive capture sessions with the IPA running in isolated mode:\n> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap\n> \n> Fixes: 7832e19a599e (\"utils: ipc: add templates for code generation for IPC mechanism\")\n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> index 8a88bd467da7..22cbc15c5eff 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -79,6 +79,10 @@ public:\n> \n> {% for method in interface_main.methods %}\n> \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n> +\n> +{%- if method.mojom_name == \"configure\" %}\n> +\t\tcontrolSerializer_.reset();\n> +{%- endif %}\n> \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n> {% for param in method|method_param_outputs %}\n> \t\t\t{{param|name}} {{param.mojom_name}};","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 97700BDC71\n\tfor ;\n\tFri, 3 Sep 2021 11:30:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50FBE6916A;\n\tFri, 3 Sep 2021 13:30:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07ACC69167\n\tfor ;\n\tFri, 3 Sep 2021 13:30:12 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 92809BBE;\n\tFri, 3 Sep 2021 13:30:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YSK6Uz1t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630668611;\n\tbh=OQy0E47jwTmvL/c7mviXSWWyR8/OWVoqEFFZPqtG2V0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YSK6Uz1t3c/Hd3wPGicE+9+pyVjKw2dIVNVHcT5+GRnyTsjwcFESVNhpO65K9iNK0\n\tc3p0TvVMg5V4oxD7zG+CKuAG1VlggK5cadgg5wcwOdITm/reZOe1LkWkpCvrALwimP\n\t1UeSqC7KWD6Z19nkjkyYH7WiAp8qHB5HVlsH7cF0=","Date":"Fri, 3 Sep 2021 14:29:55 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210901143800.119118-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19361,"web_url":"https://patchwork.libcamera.org/comment/19361/","msgid":"<04b6de78-f08f-717e-69e9-be675f56a1b2@ideasonboard.com>","date":"2021-09-03T11:30:17","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 03/09/2021 12:12, Laurent Pinchart wrote:\n> On Fri, Sep 03, 2021 at 06:33:30PM +0900, paul.elder@ideasonboard.com wrote:\n>> On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote:\n>>> On 01/09/2021 15:37, Jacopo Mondi wrote:\n>>>> The IPA::configure() function has an IPAConfigInfo parameters which\n>>>> contains a map of numerical indexes to ControlInfoMap instances.\n>>>>\n>>>> This is a leftover of the old IPA protocol, where it was not possible to\n>>>> specify a rich interface as it is possible today and each entity\n>>>> ControlInfoMap was indexed by a numerical id and stored in a map.\n>>>>\n>>>> Now that the IPA interface allows to specify parameters by name, drop the\n>>>> map and send the sensor's control info map only.\n>>>>\n>>>> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n>>>> can be added to IPAConfigInfo.\n>>>\n>>>\n>>> It looks like this will need a patch for\n>>>\n>>> https://git.libcamera.org/libcamera/ipu3-ipa.git/\n>>>\n>>> as well. But given that it requires manually installed AIQ libraries on\n>>> non CrOS build environments, perhaps it's something myself or Umang\n>>> should handle when this series merges.\n>>>\n>>> I wonder if we should be increasing the IPA ABI / version numbers too\n>>> whenever we make a change.\n>>\n>> We don't have a release yet so it should be fine, right? :p\n>>\n>> I guess we gotta start working on this for real... I'll prepare\n>> a summary + discussion points.\n> \n> Another can of worms :-) It certainly needs to be addressed, but will\n> need to include .mojom versioning, and protocol stability. We can start\n> discussions, but I don't think the implementation is the priority at\n> this point.\n\n\nBut we /do/ have externally built IPAs already which can get out of sync\non this - and therefore crash. (I know, it's crashed on me already :D)\n\nHiding behind \"We don't have a release\" only gets us to the point that\nwe release. If that is going to be in 5 years, sure we can keep hiding.\n\nBut I think we're doing ourselves a dis-service by not actually putting\nthe process in place to use (and therefore test) this before we get to 1.0\n\n\n\n>>> But alas - it doesn't look like that's fully implemented, The mojo files\n>>> should have a version number in there (or even better, could mojo create\n>>> a hash of the interface to catch when it changes I wonder)?\n>>>\n>>> So, given that you can't update the other IPA, and you can't update the\n>>> IPU3 IPA Version (without further patches) ... I guess this is it ...\n>>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>>> Signed-off-by: Jacopo Mondi \n>>>> ---\n>>>> include/libcamera/ipa/ipu3.mojom | 2 +-\n>>>> src/ipa/ipu3/ipu3.cpp | 4 ++--\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n>>>> 3 files changed, 4 insertions(+), 4 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>>>> index d561c2244907..2045ce909a88 100644\n>>>> --- a/include/libcamera/ipa/ipu3.mojom\n>>>> +++ b/include/libcamera/ipa/ipu3.mojom\n>>>> @@ -32,7 +32,7 @@ struct IPU3Action {\n>>>> \n>>>> struct IPAConfigInfo {\n>>>> \tlibcamera.IPACameraSensorInfo sensorInfo;\n>>>> -\tmap entityControls;\n>>>> +\tlibcamera.ControlInfoMap sensorControls;\n>>>> \tlibcamera.Size bdsOutputSize;\n>>>> \tlibcamera.Size iif;\n>>>> };\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index c925cf642611..6a74f7826332 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n>>>> \n>>>> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>>>> {\n>>>> -\tif (configInfo.entityControls.empty()) {\n>>>> +\tif (configInfo.sensorControls.empty()) {\n>>>> \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n>>>> \t\treturn -ENODATA;\n>>>> \t}\n>>>> \n>>>> \tsensorInfo_ = configInfo.sensorInfo;\n>>>> \n>>>> -\tctrls_ = configInfo.entityControls.at(0);\n>>>> +\tctrls_ = configInfo.sensorControls;\n>>>> \n>>>> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>>>> \tif (itExp == ctrls_.end()) {\n>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> index c287bf86e79a..92e869257e53 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>>>> \t}\n>>>> \n>>>> \tipa::ipu3::IPAConfigInfo configInfo;\n>>>> -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n>>>> +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n>>>> \tconfigInfo.sensorInfo = sensorInfo;\n>>>> \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n>>>> \tconfigInfo.iif = config->imguConfig().iif;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 11453BDC71\n\tfor ;\n\tFri, 3 Sep 2021 11:30:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE8FD6916A;\n\tFri, 3 Sep 2021 13:30:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5720569167\n\tfor ;\n\tFri, 3 Sep 2021 13:30:21 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E836FBBE;\n\tFri, 3 Sep 2021 13:30:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"P7e7vh+E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630668621;\n\tbh=nRgQvikeLLhL3G4NaPrgCfCXaEaty65UeHJn4fo9GF4=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=P7e7vh+E3txlsw7y0Z/adyTcQl0Uow3wQ5tYRwJ6ByGch80OpV8V3IthU+zA2Yoyv\n\tFqv0O0s8P2LaGD/SvjjKhBcqwvN9tRbBOsQyuYaAMfvAPkwxQMdbjneL4z2FyLWJ76\n\tYyT6Pjq06bIfeeOl+kegWsYDsal9TkvKNlQTiO3Q=","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tpaul.elder@ideasonboard.com","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>\n\t<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>\n\t<20210903093330.GI968527@pyrite.rasen.tech>\n\t","Message-ID":"<04b6de78-f08f-717e-69e9-be675f56a1b2@ideasonboard.com>","Date":"Fri, 3 Sep 2021 12:30:17 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19363,"web_url":"https://patchwork.libcamera.org/comment/19363/","msgid":"","date":"2021-09-03T11:35:20","subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Sep 03, 2021 at 12:30:17PM +0100, Kieran Bingham wrote:\n> On 03/09/2021 12:12, Laurent Pinchart wrote:\n> > On Fri, Sep 03, 2021 at 06:33:30PM +0900, paul.elder@ideasonboard.com wrote:\n> >> On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote:\n> >>> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> >>>> The IPA::configure() function has an IPAConfigInfo parameters which\n> >>>> contains a map of numerical indexes to ControlInfoMap instances.\n> >>>>\n> >>>> This is a leftover of the old IPA protocol, where it was not possible to\n> >>>> specify a rich interface as it is possible today and each entity\n> >>>> ControlInfoMap was indexed by a numerical id and stored in a map.\n> >>>>\n> >>>> Now that the IPA interface allows to specify parameters by name, drop the\n> >>>> map and send the sensor's control info map only.\n> >>>>\n> >>>> If we'll need more ControlInfoMap to be shared with the IPA, a new parameter\n> >>>> can be added to IPAConfigInfo.\n> >>>\n> >>>\n> >>> It looks like this will need a patch for\n> >>>\n> >>> https://git.libcamera.org/libcamera/ipu3-ipa.git/\n> >>>\n> >>> as well. But given that it requires manually installed AIQ libraries on\n> >>> non CrOS build environments, perhaps it's something myself or Umang\n> >>> should handle when this series merges.\n> >>>\n> >>> I wonder if we should be increasing the IPA ABI / version numbers too\n> >>> whenever we make a change.\n> >>\n> >> We don't have a release yet so it should be fine, right? :p\n> >>\n> >> I guess we gotta start working on this for real... I'll prepare\n> >> a summary + discussion points.\n> > \n> > Another can of worms :-) It certainly needs to be addressed, but will\n> > need to include .mojom versioning, and protocol stability. We can start\n> > discussions, but I don't think the implementation is the priority at\n> > this point.\n> \n> But we /do/ have externally built IPAs already which can get out of sync\n> on this - and therefore crash. (I know, it's crashed on me already :D)\n> \n> Hiding behind \"We don't have a release\" only gets us to the point that\n> we release. If that is going to be in 5 years, sure we can keep hiding.\n> \n> But I think we're doing ourselves a dis-service by not actually putting\n> the process in place to use (and therefore test) this before we get to 1.0\n\nThe process has to be put in place, it will involve lots of work, which\nmeans that starting the discussion early is good. The implementation,\nhowever, isn't the highest priority.\n\n> >>> But alas - it doesn't look like that's fully implemented, The mojo files\n> >>> should have a version number in there (or even better, could mojo create\n> >>> a hash of the interface to catch when it changes I wonder)?\n> >>>\n> >>> So, given that you can't update the other IPA, and you can't update the\n> >>> IPU3 IPA Version (without further patches) ... I guess this is it ...\n> >>>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> include/libcamera/ipa/ipu3.mojom | 2 +-\n> >>>> src/ipa/ipu3/ipu3.cpp | 4 ++--\n> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> >>>> 3 files changed, 4 insertions(+), 4 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> >>>> index d561c2244907..2045ce909a88 100644\n> >>>> --- a/include/libcamera/ipa/ipu3.mojom\n> >>>> +++ b/include/libcamera/ipa/ipu3.mojom\n> >>>> @@ -32,7 +32,7 @@ struct IPU3Action {\n> >>>> \n> >>>> struct IPAConfigInfo {\n> >>>> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> >>>> -\tmap entityControls;\n> >>>> +\tlibcamera.ControlInfoMap sensorControls;\n> >>>> \tlibcamera.Size bdsOutputSize;\n> >>>> \tlibcamera.Size iif;\n> >>>> };\n> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>>> index c925cf642611..6a74f7826332 100644\n> >>>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>>> @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> >>>> \n> >>>> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> >>>> {\n> >>>> -\tif (configInfo.entityControls.empty()) {\n> >>>> +\tif (configInfo.sensorControls.empty()) {\n> >>>> \t\tLOG(IPAIPU3, Error) << \"No controls provided\";\n> >>>> \t\treturn -ENODATA;\n> >>>> \t}\n> >>>> \n> >>>> \tsensorInfo_ = configInfo.sensorInfo;\n> >>>> \n> >>>> -\tctrls_ = configInfo.entityControls.at(0);\n> >>>> +\tctrls_ = configInfo.sensorControls;\n> >>>> \n> >>>> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> >>>> \tif (itExp == ctrls_.end()) {\n> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> index c287bf86e79a..92e869257e53 100644\n> >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> >>>> \t}\n> >>>> \n> >>>> \tipa::ipu3::IPAConfigInfo configInfo;\n> >>>> -\tconfigInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n> >>>> +\tconfigInfo.sensorControls = data->cio2_.sensor()->controls();\n> >>>> \tconfigInfo.sensorInfo = sensorInfo;\n> >>>> \tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> >>>> \tconfigInfo.iif = config->imguConfig().iif;\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1CC2ABD87D\n\tfor ;\n\tFri, 3 Sep 2021 11:35:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 903FC69169;\n\tFri, 3 Sep 2021 13:35:39 +0200 (CEST)","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 E01E969168\n\tfor ;\n\tFri, 3 Sep 2021 13:35:37 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 568DBBBE;\n\tFri, 3 Sep 2021 13:35:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"u8LImYxC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630668937;\n\tbh=aAv5OQgc2Wty1gg8cBI9UTPug2p5Pq2hqB3j2IOL3Ns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u8LImYxCR2b2sP/F6cP7LP2zzTvGq+piszqLKZjoJiDqu5RhX9qlL2c46c54EkfE8\n\t/b2Y8MH5wltLVGcNGQjHF492VbnflJT/Lon4AUkpCi5cGj4u4EV6e50tmZyGXzyIV+\n\tYicZoHa1UrAg+N0eqwAzNUQJBwGg+1ZsKCaCc+Pg=","Date":"Fri, 3 Sep 2021 14:35:20 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-2-jacopo@jmondi.org>\n\t<2887a189-48c5-73c9-3138-475a456462d2@ideasonboard.com>\n\t<20210903093330.GI968527@pyrite.rasen.tech>\n\t\n\t<04b6de78-f08f-717e-69e9-be675f56a1b2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<04b6de78-f08f-717e-69e9-be675f56a1b2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop\n\tentityControls map","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19365,"web_url":"https://patchwork.libcamera.org/comment/19365/","msgid":"","date":"2021-09-03T11:46:35","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote:\n> When running the IPA in isolated mode, each side of the IPC boundary\n> has an instance of the ControlSerializer class.\n> \n> Each serializer instance tags with a numerical id (handle) each\n> ControlInfoMap instance it serializes, to be capable of associating\n> ControlList with it at serialization and deserialization time, and\n> increments the numerical counter for each newly serialized control info\n> map.\n> \n> Having two instances of the ControlSerializer class running in two\n> different process spaces, each instance increments its own counter,\n> preventing from maintaining a globally shared state where each\n> ControlInfoMap instance is reference by a unique identifier.\n> \n> Fix this by advancing the serial_ counter at ControlInfoMap\n> de-serialization time, so that each newly serialized map will have a\n> globally unique identifier.\n\nThat's an interesting one. The control serializer was developed with the\nassumption that a ControlInfoMap would only be sent from the pipeline\nhandler to the IPA, not the other way around.\n\n> Fixes: 2c5f0ad23aa4 (\"libcamera: Add controls serializer\")\n\nTechnically, the breakage was introduced by the first change in the IPA\nprotocol that sent a ControlInfoMap in the IPA to pipeline handler\ndirection, but I don't mind keeping the Fixes tag as-is.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/control_serializer.cpp | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 27360526f9eb..08cfecca3078 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\treturn {};\n> \t}\n> \n> +\t/* Keep the info map handles in sync between the two sides of IPC. */\n> +\tserial_ = std::max(serial_, hdr->handle);\n\nCan it be this simple ? What if the pipeline handler sends a\nControlInfoMap to the IPA at the same time as the IPA sends a\nControlInfoMap to the pipeline handler ? Each side will increment their\nserial number, but the same number will refer to two different maps.\n\n> +\n> \t/*\n> \t * Use the ControlIdMap corresponding to the id map type. If the type\n> \t * references a globally defined id map (such as controls::controls","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AF325BD87D\n\tfor ;\n\tFri, 3 Sep 2021 11:46:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 348EF69168;\n\tFri, 3 Sep 2021 13:46:54 +0200 (CEST)","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 A356E69167\n\tfor ;\n\tFri, 3 Sep 2021 13:46:52 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 139B1BBE;\n\tFri, 3 Sep 2021 13:46:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"g8CLhwUG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630669612;\n\tbh=MwCt06nJ371uEUS3cKJ7GpNNsqH9rdmPGAqKaf7OYIk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g8CLhwUG3XWkac9WneyVeAA5ZTVqIf9sQUXtMEfTkOMBlC+7M++JIMftmJQE4eq1/\n\t8Y3VGWjDTmEPFDpU21D41ytESBjgJCfFWZGUvq3CYO09djSgN0H1160GOLVyLGlsNd\n\tWarpNYLLdQGMnuyVzpkwNrJWgqiOB/4abVamLhF8=","Date":"Fri, 3 Sep 2021 14:46:35 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210901143800.119118-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19367,"web_url":"https://patchwork.libcamera.org/comment/19367/","msgid":"","date":"2021-09-03T12:00:22","subject":"Re: [libcamera-devel] [PATCH 6/6] libcamera: control_serializer:\n\tSerialize info::def()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 03, 2021 at 02:42:28PM +0900, paul.elder@ideasonboard.com wrote:\n> On Wed, Sep 01, 2021 at 04:38:00PM +0200, Jacopo Mondi wrote:\n> > The ControlInfo class was originally designed to only transport\n> > the control's minimum and maximum values which represent the control's\n> > valid limits.\n> > \n> > Later the default value of the control has been added to the ControlInfo\n> > class, but the control serializer implementation has not been updated\n> > accordingly.\n> > \n> > This causes issues to IPA modules making use of ControlInfo::def() as,\n> \n> s/to/in/\n> \n> > when running in isolation, they would receive a 0 value as default.\n> \n> s/ as default//\n> \n> There's no way to turn off receiving the \"default\" :D\n> \n> > Fix that by serializing and deserializing the additional ControlValue\n> > and update the protocol description accordingly.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/control_serializer.cpp | 6 ++++--\n> > src/libcamera/ipa_controls.cpp | 14 ++++++++------\n> > 2 files changed, 12 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 7eef041a16ee..4e8045251938 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value)\n> > \n> > size_t ControlSerializer::binarySize(const ControlInfo &info)\n> > {\n> > -\treturn binarySize(info.min()) + binarySize(info.max());\n> > +\treturn binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def());\n> > }\n> > \n> > /**\n> > @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer)\n> > {\n> > \tstore(info.min(), buffer);\n> > \tstore(info.max(), buffer);\n> > +\tstore(info.def(), buffer);\n> > }\n> > \n> > /**\n> > @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type,\n> > \n> > \tControlValue min = loadControlValue(type, b);\n> > \tControlValue max = loadControlValue(type, b);\n> > +\tControlValue def = loadControlValue(type, b);\n> > \n> > -\treturn ControlInfo(min, max);\n> > +\treturn ControlInfo(min, max, def);\n> > }\n> > \n> > /**\n> > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> > index fb98cda30ede..3d172d66b30f 100644\n> > --- a/src/libcamera/ipa_controls.cpp\n> > +++ b/src/libcamera/ipa_controls.cpp\n> > @@ -108,17 +108,19 @@\n> > * +-------------------------+ .\n> > * / | ... | | entry[n].offset\n> > * | +-------------------------+ <-----´\n> > - * Data | | minimum value (#n) | \\\n> > - * section | +-------------------------+ | Entry #n\n> > - * | | maximum value (#n) | /\n> > + * | | minimum value (#n) | \\\n> > + * Data | +-------------------------+ |\n> > + * section | | maximum value (#n) | | Entry #n\n> > + * | +-------------------------+ |\n> > + * | | default value (#n) | /\n> > * | +-------------------------+\n> > * \\ | ... |\n> > * +-------------------------+\n> > * ~~~~\n> > *\n> > - * The minimum and maximum value are stored in the platform's native data\n> > - * format. The ipa_control_info_entry::offset field stores the offset from the\n> > - * beginning of the data section to the info data.\n> > + * The minimum, maximum and default value are stored in the platform's native\n> \n> s/maximum/maximum,/\n\nOxford commas https://en.wikipedia.org/wiki/Serial_comma :-) (I would\nhave gone without it here, or have kept value as a singular to only\nrefer to default.)\n\n> s/value/values/\n\nAgreed.\n\n> Reviewed-by: Paul Elder \n\nReviewed-by: Laurent Pinchart \n\n> > + * data format. The ipa_control_info_entry::offset field stores the offset from\n> > + * the beginning of the data section to the info data.\n> > *\n> > * Info data in the data section shall be stored in the same order as the\n> > * entries array, shall be aligned to a multiple of 8 bytes, and shall be","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D4F8ABD87D\n\tfor ;\n\tFri, 3 Sep 2021 12:00:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AEF96916A;\n\tFri, 3 Sep 2021 14:00:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E11869167\n\tfor ;\n\tFri, 3 Sep 2021 14:00:39 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55563BBE;\n\tFri, 3 Sep 2021 14:00:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S9Mp8rHR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630670438;\n\tbh=I60MteGUVrCb5KFHBq1Q+SUZUaOSsJgwoanEr4VcNCk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S9Mp8rHRkbtxm1zqw+FijhURoEhJlAPa5mdHy1u5+8VDPCHtTNPGn0hUwlBCYwpBP\n\tpuDfVx1XTvYOVt1BUUcWfVASrolZCiHz79ITun46YuCRINXhZcS5pbmVLkTJcxzTtn\n\trv1WbIJtQ6gh0jAC9xwRnZFn2lTZiiaqD/fzh2RQ=","Date":"Fri, 3 Sep 2021 15:00:22 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-7-jacopo@jmondi.org>\n\t<20210903054228.GG968527@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210903054228.GG968527@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 6/6] libcamera: control_serializer:\n\tSerialize info::def()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19372,"web_url":"https://patchwork.libcamera.org/comment/19372/","msgid":"","date":"2021-09-03T12:58:30","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Sep 03, 2021 at 11:21:46AM +0200, Jacopo Mondi wrote:\n> On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote:\n> > On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > > When a ControlList is deserialized, the code searches for a valid\n> > > ControlInfoMap in the local cache and use its id map to initialize the\n> > > list. If no valid ControlInfoMap is found, as it's usually the case\n> > > for lists transporting libcamera controls and properties, the globally\n> > > defined controls::controls id map is used unconditionally.\n> > >\n> > > This breaks the deserialization of libcamera properties, for which a\n> > > wrong idmap is used at construction time.\n\nWhen does this happen ? Shouldn't we ensure that we first serialize a\nControInfoMap for the properties ? Or do we not have that ? I'm OK with\nthis patch as a short-term fix, but I'd like to understand the big\npicture.\n\n> > > As the serialization header now transports an id_map_type field, store\n> > > the idmap type at serialization time, and re-use it at\n> > > deserialization time to identify the correct id map.\n> > >\n> > > Also make the validation stricter by imposing to list of V4L2 controls to\n> > > have an associated ControlInfoMap available, as there is no globally\n> > > defined idmap for such controls.\n> > >\n> > > To be able to retrieve the idmap associated with a ControlList, add an\n> > > accessor function to the ControlList class.\n> > >\n> > > It might be worth in future using a ControlInfoMap to initialize the\n> > > deserialized ControlList to implement controls validation against their\n> > > limit. As such validation is not implemented at the moment, maintain the\n> > > current behaviour and initialize the control list with an idmap.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/controls.h | 1 +\n> > > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++-----\n> > > src/libcamera/controls.cpp | 8 +++++\n> > > 3 files changed, 49 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 6668e4bb1053..8e5bc8b70b94 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -407,6 +407,7 @@ public:\n> > > \tvoid set(unsigned int id, const ControlValue &value);\n> > >\n> > > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > > +\tconst ControlIdMap *idMap() const { return idmap_; }\n> > >\n> > > private:\n> > > \tconst ControlValue *find(unsigned int id) const;\n> > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > > index 08cfecca3078..d4377c024079 100644\n> > > --- a/src/libcamera/control_serializer.cpp\n> > > +++ b/src/libcamera/control_serializer.cpp\n> > > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > \t\tinfoMapHandle = 0;\n> > > \t}\n> > >\n> > > +\tconst ControlIdMap *idmap = list.idMap();\n> > > +\tenum ipa_controls_id_map_type idMapType;\n> > > +\tif (idmap == &controls::controls)\n> > > +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > > +\telse if (idmap == &properties::properties)\n> > > +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > > +\telse\n> > > +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> > > +\n> > > \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> > > \tsize_t valuesSize = 0;\n> > > \tfor (const auto &ctrl : list)\n> > > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > \thdr.entries = list.size();\n> > > \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > > \thdr.data_offset = sizeof(hdr) + entriesSize;\n> > > +\thdr.id_map_type = idMapType;\n> > >\n> > > \tbuffer.write(&hdr);\n> > >\n> > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > > \t}\n> > >\n> > > \t/*\n> > > -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> > > -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> > > -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> > > -\t * currently the case for ControlList related to libcamera controls),\n> > > -\t * use the global control::control idmap.\n> > > +\t * Retrieve the ControlIdMap associated with the ControlList.\n> > > +\t *\n> > > +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> > > +\t * a valid handle has been initialized at serialization time, or by\n> > > +\t * using the header's id_map_type field for lists that refer to the\n> > > +\t * globally defined libcamera controls and properties, for which no\n> > > +\t * ControlInfoMap is available.\n> > > \t */\n> > > -\tconst ControlInfoMap *infoMap;\n> > > +\tconst ControlIdMap *idMap;\n> > > \tif (hdr->handle) {\n> > > \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> > > \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> > > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > > \t\t\treturn {};\n> > > \t\t}\n> > >\n> > > -\t\tinfoMap = iter->first;\n> > > +\t\tconst ControlInfoMap *infoMap = iter->first;\n> > > +\t\tidMap = &infoMap->idmap();\n> > > \t} else {\n> > > -\t\tinfoMap = nullptr;\n> > > +\t\tswitch (hdr->id_map_type) {\n> > > +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> > > +\t\t\tidMap = &controls::controls;\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> > > +\t\t\tidMap = &properties::properties;\n> > > +\t\t\tbreak;\n> \n> Ups, missing a line break here!\n> \n> > > +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> > > +\t\t\tLOG(Serializer, Fatal)\n> > > +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n> >\n> > Can we have a return statement after the Fatal?\n> >\n> > (Presuming that there is no safe path forwards if Fatal is lowered to\n> > Error on Release builds).\n> \n> Right! In production builds this won't exit. I'll add a return\n> \n> > > +\t\t}\n> > > \t}\n> > >\n> > > -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> > > +\t/*\n> > > +\t * \\todo If possible, initialize the list with the ControlInfoMap\n> > > +\t * so that controls can be validated against their limits.\n> > > +\t * Currently no validation is performed, so it's fine relying on the\n> > > +\t * idmap only.\n> > > +\t */\n> > > +\tASSERT(idMap);\n> >\n> > I presume this assert can only be hit through the\n> > IPA_CONTROL_ID_MAP_V4L2 path above?\n> \n> It could actually be hit through the if (hdr->handle) branch\n> where the idMap is retrieved from the infoMap\n\nCan this happen in practice, or would it be a bug somewhere ?\n\n> > Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added,\n> > it guards against that too - so the assert is good to have.\n> \n> If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that\n> not all the cases are handled in the switch statment, as I didn't add\n> a default: case precisely for that reason.\n> \n> So yes, I could move the ASSERT() in the if() {} branch actually.\n\nSounds good to me.\n\n> >\n> > Only minors there:\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > +\tControlList ctrls(*idMap);\n> > >\n> > > \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> > > \t\tconst struct ipa_control_value_entry *entry =\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 5c05ba4a7cd0..96625edb1380 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> > > * associated ControlInfoMap, nullptr is returned in that case.\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn ControlList::idMap()\n> > > + * \\brief Retrieve the ControlId map used to construct the ControlList\n> > > + * \\return The ControlId map used to construct the ControlList. ControlsList\n> > > + * instances constructed with ControlList() have no associated idmap, nullptr is\n> > > + * returned in that case.\n> > > + */\n> > > +\n> > > const ControlValue *ControlList::find(unsigned int id) const\n> > > {\n> > > \tconst auto iter = controls_.find(id);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D2963BD87D\n\tfor ;\n\tFri, 3 Sep 2021 12:58:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45A4A6916A;\n\tFri, 3 Sep 2021 14:58:49 +0200 (CEST)","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 BE11669167\n\tfor ;\n\tFri, 3 Sep 2021 14:58:47 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31FB7BBE;\n\tFri, 3 Sep 2021 14:58:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lxsfSYz/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630673927;\n\tbh=z+wfJlMzzw5LIbQAdlD8EREh21bbPim4C/zQfBIOCrQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lxsfSYz/NHz1tkOhuhINpvxVtivIi/p+HC/3NsD8HGMku9k9V9atMGxxeRUkE2ogN\n\tnzIIZY20cstFJH0oR5a7rsB5Az9WLWBoOP4HzbWTkGjpNCb9gmOi0KFHsFpedhVNEz\n\tKvbto2/FDwUVFDYWDXjpKcOcdDoXo8xXF0IjiERM=","Date":"Fri, 3 Sep 2021 15:58:30 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-5-jacopo@jmondi.org>\n\t<0432e00a-aa7a-bbe1-7d43-2c757b643168@ideasonboard.com>\n\t<20210903092146.pz4lfzj2xkwcuwat@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210903092146.pz4lfzj2xkwcuwat@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19373,"web_url":"https://patchwork.libcamera.org/comment/19373/","msgid":"<20210903132841.hquluqftj3twvhbq@uno.localdomain>","date":"2021-09-03T13:28:41","subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Sep 03, 2021 at 02:46:35PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote:\n> > When running the IPA in isolated mode, each side of the IPC boundary\n> > has an instance of the ControlSerializer class.\n> >\n> > Each serializer instance tags with a numerical id (handle) each\n> > ControlInfoMap instance it serializes, to be capable of associating\n> > ControlList with it at serialization and deserialization time, and\n> > increments the numerical counter for each newly serialized control info\n> > map.\n> >\n> > Having two instances of the ControlSerializer class running in two\n> > different process spaces, each instance increments its own counter,\n> > preventing from maintaining a globally shared state where each\n> > ControlInfoMap instance is reference by a unique identifier.\n> >\n> > Fix this by advancing the serial_ counter at ControlInfoMap\n> > de-serialization time, so that each newly serialized map will have a\n> > globally unique identifier.\n>\n> That's an interesting one. The control serializer was developed with the\n> assumption that a ControlInfoMap would only be sent from the pipeline\n> handler to the IPA, not the other way around.\n>\n> > Fixes: 2c5f0ad23aa4 (\"libcamera: Add controls serializer\")\n>\n> Technically, the breakage was introduced by the first change in the IPA\n> protocol that sent a ControlInfoMap in the IPA to pipeline handler\n> direction, but I don't mind keeping the Fixes tag as-is.\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/control_serializer.cpp | 3 +++\n> > 1 file changed, 3 insertions(+)\n> >\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 27360526f9eb..08cfecca3078 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > \t\treturn {};\n> > \t}\n> >\n> > +\t/* Keep the info map handles in sync between the two sides of IPC. */\n> > +\tserial_ = std::max(serial_, hdr->handle);\n>\n> Can it be this simple ? What if the pipeline handler sends a\n> ControlInfoMap to the IPA at the same time as the IPA sends a\n> ControlInfoMap to the pipeline handler ? Each side will increment their\n> serial number, but the same number will refer to two different maps.\n>\n\nYes, this might happen during a capture session, where the PH/IPA\ninteractions are aysnchronous. It can't happen before, as there won't\nbe concurrect calls between the two sides of the IPC\n\nA regular run looks like\n\n-----------------------------------------------------------------------------\nPH handle# IPC IPA handl#\n-----------------------------------------------------------------------------\nser(map) 1 --->\n deser(map)\n max(0,1)=1\n\n <--- ser(map) 2\n max(1,2)=2\ndeser(map)\n\nser(map) 3\n ...\n-----------------------------------------------------------------------------\n\nThis could break only if a ser/deser sequence is interleaved with\nanother serialization event\n\n-----------------------------------------------------------------------------\nPH handle# IPC IPA handl#\n-----------------------------------------------------------------------------\nser(map) 1 ---->\n\n <---- ser(map) 1\n\n deser(map)\n max(1,1)=1\n max(1,1)=1\ndeser(map)\n\nser(map) 2\n ...\n-----------------------------------------------------------------------------\n\nWhich is very unfortunate but possible.\n\nWe have no centralized state between the two processes and we have no\nway to assign handles in the proxies, as they have the same\ninformation as the control serializer has.\n\nThis might seem rather stupid, but we have two sides of the IPC, can't\nwe use even handles on one side and odd ones on the other ? The\nproxies initialize the controls serializer with a seed (0 or 1) and we\nsimply serial_ += 2 ?\n\n\n> > +\n> > \t/*\n> > \t * Use the ControlIdMap corresponding to the id map type. If the type\n> > \t * references a globally defined id map (such as controls::controls\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3E124BDC71\n\tfor ;\n\tFri, 3 Sep 2021 13:27:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A468F6916A;\n\tFri, 3 Sep 2021 15:27:54 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B67569167\n\tfor ;\n\tFri, 3 Sep 2021 15:27:53 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 7BF2B240002;\n\tFri, 3 Sep 2021 13:27:52 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 15:28:41 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210903132841.hquluqftj3twvhbq@uno.localdomain>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/6] libcamera: control_serializer:\n\tKeep handles in sync","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19374,"web_url":"https://patchwork.libcamera.org/comment/19374/","msgid":"<20210903133615.voh3yunyhntbjfpx@uno.localdomain>","date":"2021-09-03T13:36:15","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Sep 03, 2021 at 03:58:30PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 03, 2021 at 11:21:46AM +0200, Jacopo Mondi wrote:\n> > On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote:\n> > > On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > > > When a ControlList is deserialized, the code searches for a valid\n> > > > ControlInfoMap in the local cache and use its id map to initialize the\n> > > > list. If no valid ControlInfoMap is found, as it's usually the case\n> > > > for lists transporting libcamera controls and properties, the globally\n> > > > defined controls::controls id map is used unconditionally.\n> > > >\n> > > > This breaks the deserialization of libcamera properties, for which a\n> > > > wrong idmap is used at construction time.\n>\n> When does this happen ? Shouldn't we ensure that we first serialize a\n> ControInfoMap for the properties ? Or do we not have that ? I'm OK with\n> this patch as a short-term fix, but I'd like to understand the big\n> picture.\n>\n\nProperties are intialized using the properties::properties idmap, as\nhaving a ControlInfoMap for unmutable controls like properties are\ndoesn't make much sense. Hence they won't even have an handle\nassociated, and the current implementation uses controls::controls as\nidmap if not handle is available.\n\nWe don't triggere the issue as we don't serialize properties, but as\nsoon as it happens, we're screwed.\n\nAnd yes, we can catch it during review but we'll forget or we carry\naround the issue untile someone gets fed up and addresses it (see\nControlInfoMap::def() which is a small change that was due since a\nyear or so).\n\nGiven that the patch is not super complex I would rather fix it now :)\n\n> > > > As the serialization header now transports an id_map_type field, store\n> > > > the idmap type at serialization time, and re-use it at\n> > > > deserialization time to identify the correct id map.\n> > > >\n> > > > Also make the validation stricter by imposing to list of V4L2 controls to\n> > > > have an associated ControlInfoMap available, as there is no globally\n> > > > defined idmap for such controls.\n> > > >\n> > > > To be able to retrieve the idmap associated with a ControlList, add an\n> > > > accessor function to the ControlList class.\n> > > >\n> > > > It might be worth in future using a ControlInfoMap to initialize the\n> > > > deserialized ControlList to implement controls validation against their\n> > > > limit. As such validation is not implemented at the moment, maintain the\n> > > > current behaviour and initialize the control list with an idmap.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/controls.h | 1 +\n> > > > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++-----\n> > > > src/libcamera/controls.cpp | 8 +++++\n> > > > 3 files changed, 49 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > index 6668e4bb1053..8e5bc8b70b94 100644\n> > > > --- a/include/libcamera/controls.h\n> > > > +++ b/include/libcamera/controls.h\n> > > > @@ -407,6 +407,7 @@ public:\n> > > > \tvoid set(unsigned int id, const ControlValue &value);\n> > > >\n> > > > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > > > +\tconst ControlIdMap *idMap() const { return idmap_; }\n> > > >\n> > > > private:\n> > > > \tconst ControlValue *find(unsigned int id) const;\n> > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > > > index 08cfecca3078..d4377c024079 100644\n> > > > --- a/src/libcamera/control_serializer.cpp\n> > > > +++ b/src/libcamera/control_serializer.cpp\n> > > > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > > \t\tinfoMapHandle = 0;\n> > > > \t}\n> > > >\n> > > > +\tconst ControlIdMap *idmap = list.idMap();\n> > > > +\tenum ipa_controls_id_map_type idMapType;\n> > > > +\tif (idmap == &controls::controls)\n> > > > +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > > > +\telse if (idmap == &properties::properties)\n> > > > +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > > > +\telse\n> > > > +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> > > > +\n> > > > \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> > > > \tsize_t valuesSize = 0;\n> > > > \tfor (const auto &ctrl : list)\n> > > > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > > \thdr.entries = list.size();\n> > > > \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > > > \thdr.data_offset = sizeof(hdr) + entriesSize;\n> > > > +\thdr.id_map_type = idMapType;\n> > > >\n> > > > \tbuffer.write(&hdr);\n> > > >\n> > > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > > > \t}\n> > > >\n> > > > \t/*\n> > > > -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> > > > -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> > > > -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> > > > -\t * currently the case for ControlList related to libcamera controls),\n> > > > -\t * use the global control::control idmap.\n> > > > +\t * Retrieve the ControlIdMap associated with the ControlList.\n> > > > +\t *\n> > > > +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> > > > +\t * a valid handle has been initialized at serialization time, or by\n> > > > +\t * using the header's id_map_type field for lists that refer to the\n> > > > +\t * globally defined libcamera controls and properties, for which no\n> > > > +\t * ControlInfoMap is available.\n> > > > \t */\n> > > > -\tconst ControlInfoMap *infoMap;\n> > > > +\tconst ControlIdMap *idMap;\n> > > > \tif (hdr->handle) {\n> > > > \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> > > > \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> > > > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > > > \t\t\treturn {};\n> > > > \t\t}\n> > > >\n> > > > -\t\tinfoMap = iter->first;\n> > > > +\t\tconst ControlInfoMap *infoMap = iter->first;\n> > > > +\t\tidMap = &infoMap->idmap();\n> > > > \t} else {\n> > > > -\t\tinfoMap = nullptr;\n> > > > +\t\tswitch (hdr->id_map_type) {\n> > > > +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> > > > +\t\t\tidMap = &controls::controls;\n> > > > +\t\t\tbreak;\n> > > > +\n> > > > +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> > > > +\t\t\tidMap = &properties::properties;\n> > > > +\t\t\tbreak;\n> >\n> > Ups, missing a line break here!\n> >\n> > > > +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> > > > +\t\t\tLOG(Serializer, Fatal)\n> > > > +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n> > >\n> > > Can we have a return statement after the Fatal?\n> > >\n> > > (Presuming that there is no safe path forwards if Fatal is lowered to\n> > > Error on Release builds).\n> >\n> > Right! In production builds this won't exit. I'll add a return\n> >\n> > > > +\t\t}\n> > > > \t}\n> > > >\n> > > > -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> > > > +\t/*\n> > > > +\t * \\todo If possible, initialize the list with the ControlInfoMap\n> > > > +\t * so that controls can be validated against their limits.\n> > > > +\t * Currently no validation is performed, so it's fine relying on the\n> > > > +\t * idmap only.\n> > > > +\t */\n> > > > +\tASSERT(idMap);\n> > >\n> > > I presume this assert can only be hit through the\n> > > IPA_CONTROL_ID_MAP_V4L2 path above?\n> >\n> > It could actually be hit through the if (hdr->handle) branch\n> > where the idMap is retrieved from the infoMap\n>\n> Can this happen in practice, or would it be a bug somewhere ?\n\nOnly if someone tries to serialize a ControlList constructed through\nthe default constructor, which is unlikely, but possible.\n\n>\n> > > Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added,\n> > > it guards against that too - so the assert is good to have.\n> >\n> > If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that\n> > not all the cases are handled in the switch statment, as I didn't add\n> > a default: case precisely for that reason.\n> >\n> > So yes, I could move the ASSERT() in the if() {} branch actually.\n>\n> Sounds good to me.\n>\n\nThanks\n j\n\n> > >\n> > > Only minors there:\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > +\tControlList ctrls(*idMap);\n> > > >\n> > > > \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> > > > \t\tconst struct ipa_control_value_entry *entry =\n> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > index 5c05ba4a7cd0..96625edb1380 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> > > > * associated ControlInfoMap, nullptr is returned in that case.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn ControlList::idMap()\n> > > > + * \\brief Retrieve the ControlId map used to construct the ControlList\n> > > > + * \\return The ControlId map used to construct the ControlList. ControlsList\n> > > > + * instances constructed with ControlList() have no associated idmap, nullptr is\n> > > > + * returned in that case.\n> > > > + */\n> > > > +\n> > > > const ControlValue *ControlList::find(unsigned int id) const\n> > > > {\n> > > > \tconst auto iter = controls_.find(id);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5AFC5BD87D\n\tfor ;\n\tFri, 3 Sep 2021 13:35:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A9A96916A;\n\tFri, 3 Sep 2021 15:35:29 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CF9869167\n\tfor ;\n\tFri, 3 Sep 2021 15:35:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 219CBC000D;\n\tFri, 3 Sep 2021 13:35:26 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 15:36:15 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210903133615.voh3yunyhntbjfpx@uno.localdomain>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-5-jacopo@jmondi.org>\n\t<0432e00a-aa7a-bbe1-7d43-2c757b643168@ideasonboard.com>\n\t<20210903092146.pz4lfzj2xkwcuwat@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19378,"web_url":"https://patchwork.libcamera.org/comment/19378/","msgid":"","date":"2021-09-03T14:40:58","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Sep 03, 2021 at 03:36:15PM +0200, Jacopo Mondi wrote:\n> On Fri, Sep 03, 2021 at 03:58:30PM +0300, Laurent Pinchart wrote:\n> > On Fri, Sep 03, 2021 at 11:21:46AM +0200, Jacopo Mondi wrote:\n> > > On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote:\n> > > > On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > > > > When a ControlList is deserialized, the code searches for a valid\n> > > > > ControlInfoMap in the local cache and use its id map to initialize the\n> > > > > list. If no valid ControlInfoMap is found, as it's usually the case\n> > > > > for lists transporting libcamera controls and properties, the globally\n> > > > > defined controls::controls id map is used unconditionally.\n> > > > >\n> > > > > This breaks the deserialization of libcamera properties, for which a\n> > > > > wrong idmap is used at construction time.\n> >\n> > When does this happen ? Shouldn't we ensure that we first serialize a\n> > ControInfoMap for the properties ? Or do we not have that ? I'm OK with\n> > this patch as a short-term fix, but I'd like to understand the big\n> > picture.\n> \n> Properties are intialized using the properties::properties idmap, as\n> having a ControlInfoMap for unmutable controls like properties are\n> doesn't make much sense. Hence they won't even have an handle\n> associated, and the current implementation uses controls::controls as\n> idmap if not handle is available.\n> \n> We don't triggere the issue as we don't serialize properties, but as\n> soon as it happens, we're screwed.\n> \n> And yes, we can catch it during review but we'll forget or we carry\n> around the issue untile someone gets fed up and addresses it (see\n> ControlInfoMap::def() which is a small change that was due since a\n> year or so).\n> \n> Given that the patch is not super complex I would rather fix it now :)\n\nFine with me. I think sending properties from the IPA to the pipeline\nhandler at init time is probably a valid use case.\n\n> > > > > As the serialization header now transports an id_map_type field, store\n> > > > > the idmap type at serialization time, and re-use it at\n> > > > > deserialization time to identify the correct id map.\n> > > > >\n> > > > > Also make the validation stricter by imposing to list of V4L2 controls to\n> > > > > have an associated ControlInfoMap available, as there is no globally\n> > > > > defined idmap for such controls.\n> > > > >\n> > > > > To be able to retrieve the idmap associated with a ControlList, add an\n> > > > > accessor function to the ControlList class.\n> > > > >\n> > > > > It might be worth in future using a ControlInfoMap to initialize the\n> > > > > deserialized ControlList to implement controls validation against their\n> > > > > limit. As such validation is not implemented at the moment, maintain the\n> > > > > current behaviour and initialize the control list with an idmap.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/controls.h | 1 +\n> > > > > src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++-----\n> > > > > src/libcamera/controls.cpp | 8 +++++\n> > > > > 3 files changed, 49 insertions(+), 9 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > > index 6668e4bb1053..8e5bc8b70b94 100644\n> > > > > --- a/include/libcamera/controls.h\n> > > > > +++ b/include/libcamera/controls.h\n> > > > > @@ -407,6 +407,7 @@ public:\n> > > > > \tvoid set(unsigned int id, const ControlValue &value);\n> > > > >\n> > > > > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > > > > +\tconst ControlIdMap *idMap() const { return idmap_; }\n> > > > >\n> > > > > private:\n> > > > > \tconst ControlValue *find(unsigned int id) const;\n> > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > > > > index 08cfecca3078..d4377c024079 100644\n> > > > > --- a/src/libcamera/control_serializer.cpp\n> > > > > +++ b/src/libcamera/control_serializer.cpp\n> > > > > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > > > \t\tinfoMapHandle = 0;\n> > > > > \t}\n> > > > >\n> > > > > +\tconst ControlIdMap *idmap = list.idMap();\n> > > > > +\tenum ipa_controls_id_map_type idMapType;\n> > > > > +\tif (idmap == &controls::controls)\n> > > > > +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > > > > +\telse if (idmap == &properties::properties)\n> > > > > +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > > > > +\telse\n> > > > > +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> > > > > +\n> > > > > \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> > > > > \tsize_t valuesSize = 0;\n> > > > > \tfor (const auto &ctrl : list)\n> > > > > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > > > \thdr.entries = list.size();\n> > > > > \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > > > > \thdr.data_offset = sizeof(hdr) + entriesSize;\n> > > > > +\thdr.id_map_type = idMapType;\n> > > > >\n> > > > > \tbuffer.write(&hdr);\n> > > > >\n> > > > > @@ -499,13 +509,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > > > > \t}\n> > > > >\n> > > > > \t/*\n> > > > > -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> > > > > -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> > > > > -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> > > > > -\t * currently the case for ControlList related to libcamera controls),\n> > > > > -\t * use the global control::control idmap.\n> > > > > +\t * Retrieve the ControlIdMap associated with the ControlList.\n> > > > > +\t *\n> > > > > +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> > > > > +\t * a valid handle has been initialized at serialization time, or by\n> > > > > +\t * using the header's id_map_type field for lists that refer to the\n> > > > > +\t * globally defined libcamera controls and properties, for which no\n> > > > > +\t * ControlInfoMap is available.\n> > > > > \t */\n> > > > > -\tconst ControlInfoMap *infoMap;\n> > > > > +\tconst ControlIdMap *idMap;\n> > > > > \tif (hdr->handle) {\n> > > > > \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> > > > > \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> > > > > @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > > > > \t\t\treturn {};\n> > > > > \t\t}\n> > > > >\n> > > > > -\t\tinfoMap = iter->first;\n> > > > > +\t\tconst ControlInfoMap *infoMap = iter->first;\n> > > > > +\t\tidMap = &infoMap->idmap();\n> > > > > \t} else {\n> > > > > -\t\tinfoMap = nullptr;\n> > > > > +\t\tswitch (hdr->id_map_type) {\n> > > > > +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> > > > > +\t\t\tidMap = &controls::controls;\n> > > > > +\t\t\tbreak;\n> > > > > +\n> > > > > +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> > > > > +\t\t\tidMap = &properties::properties;\n> > > > > +\t\t\tbreak;\n> > >\n> > > Ups, missing a line break here!\n> > >\n> > > > > +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> > > > > +\t\t\tLOG(Serializer, Fatal)\n> > > > > +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n> > > >\n> > > > Can we have a return statement after the Fatal?\n> > > >\n> > > > (Presuming that there is no safe path forwards if Fatal is lowered to\n> > > > Error on Release builds).\n> > >\n> > > Right! In production builds this won't exit. I'll add a return\n> > >\n> > > > > +\t\t}\n> > > > > \t}\n> > > > >\n> > > > > -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> > > > > +\t/*\n> > > > > +\t * \\todo If possible, initialize the list with the ControlInfoMap\n> > > > > +\t * so that controls can be validated against their limits.\n> > > > > +\t * Currently no validation is performed, so it's fine relying on the\n> > > > > +\t * idmap only.\n> > > > > +\t */\n> > > > > +\tASSERT(idMap);\n> > > >\n> > > > I presume this assert can only be hit through the\n> > > > IPA_CONTROL_ID_MAP_V4L2 path above?\n> > >\n> > > It could actually be hit through the if (hdr->handle) branch\n> > > where the idMap is retrieved from the infoMap\n> >\n> > Can this happen in practice, or would it be a bug somewhere ?\n> \n> Only if someone tries to serialize a ControlList constructed through\n> the default constructor, which is unlikely, but possible.\n\nI'll consider that as a bug, so ASSERT() is good enough.\n\n> > > > Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added,\n> > > > it guards against that too - so the assert is good to have.\n> > >\n> > > If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that\n> > > not all the cases are handled in the switch statment, as I didn't add\n> > > a default: case precisely for that reason.\n> > >\n> > > So yes, I could move the ASSERT() in the if() {} branch actually.\n> >\n> > Sounds good to me.\n> \n> Thanks\n> \n> > > > Only minors there:\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > > +\tControlList ctrls(*idMap);\n> > > > >\n> > > > > \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> > > > > \t\tconst struct ipa_control_value_entry *entry =\n> > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > > index 5c05ba4a7cd0..96625edb1380 100644\n> > > > > --- a/src/libcamera/controls.cpp\n> > > > > +++ b/src/libcamera/controls.cpp\n> > > > > @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> > > > > * associated ControlInfoMap, nullptr is returned in that case.\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn ControlList::idMap()\n> > > > > + * \\brief Retrieve the ControlId map used to construct the ControlList\n> > > > > + * \\return The ControlId map used to construct the ControlList. ControlsList\n> > > > > + * instances constructed with ControlList() have no associated idmap, nullptr is\n> > > > > + * returned in that case.\n> > > > > + */\n> > > > > +\n> > > > > const ControlValue *ControlList::find(unsigned int id) const\n> > > > > {\n> > > > > \tconst auto iter = controls_.find(id);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 657F5BD87D\n\tfor ;\n\tFri, 3 Sep 2021 14:41:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD8186916A;\n\tFri, 3 Sep 2021 16:41:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2488369167\n\tfor ;\n\tFri, 3 Sep 2021 16:41:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7CD7424F;\n\tFri, 3 Sep 2021 16:41:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aA6HaA8L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630680074;\n\tbh=V8F0kMRT4QT6lRUzsOHlIS5eHUItZ6XU1Vy9OgKrUcQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aA6HaA8LeiaVzlVeiEPGGr0qtOA2pRjxNewlq4xKI8eUi39soCIRfPS2nw9eNXtbG\n\telIkVbe/iTnGJxuceFg+uelKckwx/1Iu2V6HeqNDaxVC2APxDMjYpVLvlj0IcwSzPk\n\t1FIjmYTtjGdLe+ZXIDz9PDyj03XVueM0TI7Fpl7g=","Date":"Fri, 3 Sep 2021 17:40:58 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-5-jacopo@jmondi.org>\n\t<0432e00a-aa7a-bbe1-7d43-2c757b643168@ideasonboard.com>\n\t<20210903092146.pz4lfzj2xkwcuwat@uno.localdomain>\n\t\n\t<20210903133615.voh3yunyhntbjfpx@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210903133615.voh3yunyhntbjfpx@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: control_serializer:\n\tUse the right idmap","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19383,"web_url":"https://patchwork.libcamera.org/comment/19383/","msgid":"<184ffa13-f108-0dc9-ba38-a8571f7c3479@ideasonboard.com>","date":"2021-09-03T20:11:30","subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 03/09/2021 10:16, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote:\n>> On 01/09/2021 15:37, Jacopo Mondi wrote:\n>>> When running the IPA in isolated mode, each side of the IPC boundary\n>>> has an instance of the ControlSerializer class which is used to\n>>> serializer/deserialize controls before transmitting them on the wire.\n>>>\n>>> The IPAProxyWorker, which creates and manages the process the IPA runs into,\n>>> does not reset its ControlSerializer upon an IPA::configure() call, while\n>>> the IPAProxy does so, effectively creating a misalignment between the\n>>> two sides of the fence.\n>>>\n>>> This obviously creates issues as one side of the IPC runs with a\n>>> populated and possibly stale cache of ControlInfoMap references, while the\n>>> other side gets reset every time a new configuration is applied to the\n>>> Camera.\n>>>\n>>> Fix that by resetting the IPAProxyWorker ControlSerializer on an\n>>> IPA configure() call.\n>>>\n>>> This change fixes an issue which is easily triggered by running two\n>>> consecutive capture sessions with the IPA running in isolated mode:\n>>> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap\n>>>\n>>> Fixes: 7832e19a599e (\"utils: ipc: add templates for code generation for IPC mechanism\")\n>>\n>> Is it also technically fixing the patch that added the reset on the\n>> other side? as that's when the misalignment begins?\n>>\n> \n> The 'other side' was reset from the very beginning.\n> \n> To be honest I would question the usage of Fixes in libcamera, as we\n> don't have versions to backport to, but I added this mosotly for cargo\n> cult.\n> \n> Iff we want to keep using Fixes I think that's the opportune commit to\n> point to, as this patch should be ideally fixed-up in that one (if we\n> could travel back in time)\n\nI don't think we have any requirement to add a Fixes tag, except it\nhelps promote awareness of /why/ or /what/ is being fixed.\n\nIt also helps provide context in some occasions in case the fix turns\nout to be wrong, so it's partly documentation. Imagine reading back\nthrough the history trying to find out why something changed that\ndoesn't seem to make sense. Seeing a Fixes tag helps you see 'Oh, ok -\nso this change was to fix that patch, so between there and here, the\nthing isn't going to be right'.\n\n\nAnd that it's likely a good habit to keep from Kernel side?\n\n\nBut no, I don't think we're backporting to a non-existent version.\n\n\n\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++\n>>> 1 file changed, 4 insertions(+)\n>>>\n>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n>>> index 8a88bd467da7..22cbc15c5eff 100644\n>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n>>> @@ -79,6 +79,10 @@ public:\n>>>\n>>> {% for method in interface_main.methods %}\n>>> \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n>>> +\n>>> +{%- if method.mojom_name == \"configure\" %}\n>>> +\t\tcontrolSerializer_.reset();\n>>\n>> one more level indentation >> ?\n>>\n>> It looks like we're \"inside\" this case statement, so it should be one\n>> level deeper.\n>>\n> \n> Looking at the generated code\n> \n> \t\tcase _IPU3Cmd::Configure: {\n> \t\tcontrolSerializer_.reset();\n> \n> \n> \tconst size_t configInfoStart = 0;\n> \n> \n> \tIPAConfigInfo configInfo = IPADataSerializer::deserialize(\n> \n> You are correct the indentation is wrong, but it does anyway matches\n> the rest of the file :)\n\nIndeed, it matches the next two lines (A,B) - but the rest of the case\nstatement is otherwise correct.\n\nIf I could figure out how to correct those two lines I would - but I\nrabbit-holed too long in this template yesterday.\n\nI think it's still reasonable to put the controlSerializer_.reset(); at\nthe correct indentation.\n\nThe args are incorrectly indented, but would be solved by fixing that\n(waves hand magically) \"somewhere else\" ;-) :\n\n> case _IPU3Cmd::Configure: {\n> \n> \n> A const size_t configInfoStart = 0;\n> \n> \n> B IPAConfigInfo configInfo = IPADataSerializer::deserialize(\n> B _ipcMessage.data().cbegin() + configInfoStart,\n> B _ipcMessage.data().cend(),\n> B &controlSerializer_);\n> \n> \n> int32_t _callRet =ipa_->configure(configInfo);\n> \n> IPCMessage::Header header = { _ipcMessage.header().cmd, _ipcMessage.header().cookie };\n> IPCMessage _response(header);\n> std::vector _callRetBuf;\n> std::tie(_callRetBuf, std::ignore) =\n> IPADataSerializer::serialize(_callRet);\n> _response.data().insert(_response.data().end(), _callRetBuf.cbegin(), _callRetBuf.cend());\n> \n> int _ret = socket_.send(_response.payload());\n> if (_ret < 0) {\n> LOG(IPAProxyIPU3Worker, Error)\n> << \"Reply to configure() failed: \" << _ret;\n> }\n> LOG(IPAProxyIPU3Worker, Debug) << \"Done replying to configure()\";\n> break;\n> }\n\nPaul, Are A and B easy fixes? They appear to be automagically generated\nto do with the IPADataSerializer for every call?\n\n>>> +{%- endif %}\n>>\n>>\n>> Wow, these templates are hard to parse :-)\n>>\n>> But ... it looks like this is right ;-)\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n>>> {% for param in method|method_param_outputs %}\n>>> \t\t\t{{param|name}} {{param.mojom_name}};\n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8934DBDC71\n\tfor ;\n\tFri, 3 Sep 2021 20:11:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2C746916A;\n\tFri, 3 Sep 2021 22:11:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10C3669167\n\tfor ;\n\tFri, 3 Sep 2021 22:11:34 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 75939BBE;\n\tFri, 3 Sep 2021 22:11:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EEH0b/zu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630699893;\n\tbh=WeBcqdAKtFoFKzsb0yFlNPENVDp9Up2H8+dQd1tCmZc=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=EEH0b/zuUuptqmhWXpzci8gLO1xRipqOd+7GskVeEbnJlh/QCWBp96lKccetc10kx\n\tun07ok1kjWUMcflJ4gStxf9g6tW94NmRRs822KFB9b9YVcF8wFK+RujyHBGhdNF/q5\n\tZWDDV8c19cbwYJcjRHQ938l9Ea/Ryy7zi/Sftei8=","From":"Kieran Bingham ","To":"Jacopo Mondi ","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-3-jacopo@jmondi.org>\n\t\n\t<20210903091653.5s2bgggba2d2yv72@uno.localdomain>","Message-ID":"<184ffa13-f108-0dc9-ba38-a8571f7c3479@ideasonboard.com>","Date":"Fri, 3 Sep 2021 21:11:30 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210903091653.5s2bgggba2d2yv72@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset\n\tControlSerializer on worker","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]