[{"id":16658,"web_url":"https://patchwork.libcamera.org/comment/16658/","msgid":"<762f677b-7a4a-f817-3d5e-b7d0922d00c7@ideasonboard.com>","date":"2021-04-27T11:53:35","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nThanks for the patch !\n\nOn 27/04/2021 08:35, Umang Jain wrote:\n> IPAConfigInfo is a consolidated data structure passed from IPU3\n> pipeline-handler to IPU3 IPA. This provides a streamlined and\n> extensible way to provide the configuration data to IPA interface.\n> \n> The IPA interface should be common to both closed-source and\n> open-source IPU3 IPAs. Hence, the structure encompasses additional\n> configuration information required to init and configure the\n> closed-source IPA as well.\n> \n> Signed-off-by: Umang Jain \n> ---\n> Changes in v2:\n> - Mark as RFC(do not merge) as this is built upon Paul's\n> [Fix support for core.mojom structs v3] - waiting for merge to master\n> - Drop cropRegion_ from structure - this is provided from CameraSensorInfo\n> itself.\n> - Doxygen documentation of IPAConfigInfo.\n> ---\n> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++--\n> src/ipa/ipu3/ipu3.cpp | 14 ++++-----\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++-\n> 3 files changed, 58 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index a717b1e6..acd5cfa4 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -25,13 +25,55 @@ struct IPU3Action {\n> \tlibcamera.ControlList controls;\n> };\n> \n> +/**\n> + * \\struct IPAConfigInfo\n> + * \\brief Information to be passed from IPU3 pipeline-handler to IPA\n> + *\n> + * The IPAConfigInfo structure stores the data passed from IPU3 pipeline-handler\n> + * to the IPAIPU3Interface::configure(). The structure provides extensibility\n> + * for additional configuration data as required, for closed-source or\n> + * open-source IPAs' configure() phases.\n> + */\n> +\n> +/**\n> + * \\var IPAConfigInfo::sensorInfo\n> + * \\brief Camera sensor information\n> + *\n> + * Provides the camera sensor information such as model name, pixel-rate,\n> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further\n> + * details.\n> + */\n> +\n> + /**\n> + * \\var IPAConfigInfo::entityControls\n> + * \\brief Controls supported by the sensor\n> + *\n> + * A map of V4L2 controls supported by the sensor in order to read or write\n> + * control values to them.\n> + */\n> +\n> + /**\n> + * \\var IPAConfigInfo::bdsOutputSize\n> + * \\brief Size of ImgU BDS rectangle\n> + */\n> +\n> + /**\n> + * \\var IPAConfigInfo::iif\n> + * \\brief Size of ImgU input-feeder rectangle\n> + */\n> +struct IPAConfigInfo {\n> +\tlibcamera.CameraSensorInfo sensorInfo;\n> +\tmap entityControls;\n> +\tlibcamera.Size bdsOutputSize;\n> +\tlibcamera.Size iif;\n> +};\n> +\n\nI think it is interesting, and would like your advice: I would need this\nsame structure for rkisp1 platform, except that the sizes are obviously\nnot the same.\nAnd so, I am wondering if we could imagine a more general structure like:\nstruct IPAConfigInfo {\n\tlibcamera.CameraSensorInfo sensorInfo;\n\tmap entityControls;\n\tlibcamera.Size ispInputSize;\n\tlibcamera.Size ispOutputSize;\n};\n\nnames and all may have to be reworked but that is the base idea.\nThere is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least\nyou have an input an output pad size for the ISP node.\n\n> interface IPAIPU3Interface {\n> \tinit(libcamera.IPASettings settings) => (int32 ret);\n> \tstart() => (int32 ret);\n> \tstop();\n> \n> -\tconfigure(map entityControls,\n> -\t\t libcamera.Size bdsOutputSize) => ();\n> +\tconfigure(IPAConfigInfo configInfo);\n> \n> \tmapBuffers(array buffers);\n> \tunmapBuffers(array ids);\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index f5343547..769c24d3 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -43,8 +43,7 @@ public:\n> \tint start() override;\n> \tvoid stop() override {}\n> \n> -\tvoid configure(const std::map &entityControls,\n> -\t\t const Size &bdsOutputSize) override;\n> +\tvoid configure(const IPAConfigInfo &configInfo) override;\n> \n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \t\t\t << (int)bdsGrid_.height << \" << \" << (int)bdsGrid_.block_height_log2 << \")\";\n> }\n> \n> -void IPAIPU3::configure(const std::map &entityControls,\n> -\t\t\tconst Size &bdsOutputSize)\n> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> {\n> -\tif (entityControls.empty())\n> +\tif (configInfo.entityControls.empty())\n> \t\treturn;\n> \n> -\tctrls_ = entityControls.at(0);\n> +\tctrls_ = configInfo.entityControls.at(0);\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const std::map &entityControls\n> \n> \tparams_ = {};\n> \n> -\tcalculateBdsGrid(bdsOutputSize);\n> +\tcalculateBdsGrid(configInfo.bdsOutputSize);\n> \n> \tawbAlgo_ = std::make_unique();\n> -\tawbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);\n> +\tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> \n> \tagcAlgo_ = std::make_unique();\n> \tagcAlgo_->initialise(bdsGrid_);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 73306cea..b1ff1dbe 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \n> \tstd::map entityControls;\n> \tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> -\tdata->ipa_->configure(entityControls, config->imguConfig().bds);\n> +\n> +\tipa::ipu3::IPAConfigInfo configInfo;\n> +\tconfigInfo.sensorInfo = sensorInfo;\n> +\tconfigInfo.entityControls = entityControls;\n> +\tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> +\tconfigInfo.iif = config->imguConfig().iif;\n> +\n> +\tdata->ipa_->configure(configInfo);\n> \n> \treturn 0;\n> }\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 9310ABDCC3\n\tfor ;\n\tTue, 27 Apr 2021 11:53:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8FA568882;\n\tTue, 27 Apr 2021 13:53: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 453EE602C2\n\tfor ;\n\tTue, 27 Apr 2021 13:53:36 +0200 (CEST)","from [IPv6:2a01:e0a:169:7140:2228:b567:49e4:8de9] (unknown\n\t[IPv6:2a01:e0a:169:7140:2228:b567:49e4:8de9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CBCD9E9;\n\tTue, 27 Apr 2021 13:53:35 +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=\"K+aHyb2a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619524415;\n\tbh=7UflnyQtTPjvG+mtOUHsOVqRhJQVMkfBRTogGTgGivc=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=K+aHyb2az8uZQ0OX488d5BkJbQnK3x7xbBq7qIhl5DPEsuxwXMHzWTSKU9hdkXQ3d\n\tLqimFdTYQNAfTPHMXeOLjgw6UB+o+7lGENWmE312/4nPWxXVeebbra4pUZjPKvqG0d\n\tefiZNeJ+g4Ig5X3VBs9gpjDsf1+AEelisMyFgZWk=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<762f677b-7a4a-f817-3d5e-b7d0922d00c7@ideasonboard.com>","Date":"Tue, 27 Apr 2021 13:53:35 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<20210427063527.219509-1-umang.jain@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16659,"web_url":"https://patchwork.libcamera.org/comment/16659/","msgid":"<4efbaad6-dae7-236c-bc91-49950ccd4fdc@ideasonboard.com>","date":"2021-04-27T12:54:08","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM\n\nOn 4/27/21 5:23 PM, Jean-Michel Hautbois wrote:\n> Hi Umang,\n>\n> Thanks for the patch !\n>\n> On 27/04/2021 08:35, Umang Jain wrote:\n>> IPAConfigInfo is a consolidated data structure passed from IPU3\n>> pipeline-handler to IPU3 IPA. This provides a streamlined and\n>> extensible way to provide the configuration data to IPA interface.\n>>\n>> The IPA interface should be common to both closed-source and\n>> open-source IPU3 IPAs. Hence, the structure encompasses additional\n>> configuration information required to init and configure the\n>> closed-source IPA as well.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> Changes in v2:\n>> - Mark as RFC(do not merge) as this is built upon Paul's\n>> [Fix support for core.mojom structs v3] - waiting for merge to master\n>> - Drop cropRegion_ from structure - this is provided from CameraSensorInfo\n>> itself.\n>> - Doxygen documentation of IPAConfigInfo.\n>> ---\n>> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++--\n>> src/ipa/ipu3/ipu3.cpp | 14 ++++-----\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++-\n>> 3 files changed, 58 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index a717b1e6..acd5cfa4 100644\n>> --- a/include/libcamera/ipa/ipu3.mojom\n>> +++ b/include/libcamera/ipa/ipu3.mojom\n>> @@ -25,13 +25,55 @@ struct IPU3Action {\n>> \tlibcamera.ControlList controls;\n>> };\n>> \n>> +/**\n>> + * \\struct IPAConfigInfo\n>> + * \\brief Information to be passed from IPU3 pipeline-handler to IPA\n>> + *\n>> + * The IPAConfigInfo structure stores the data passed from IPU3 pipeline-handler\n>> + * to the IPAIPU3Interface::configure(). The structure provides extensibility\n>> + * for additional configuration data as required, for closed-source or\n>> + * open-source IPAs' configure() phases.\n>> + */\n>> +\n>> +/**\n>> + * \\var IPAConfigInfo::sensorInfo\n>> + * \\brief Camera sensor information\n>> + *\n>> + * Provides the camera sensor information such as model name, pixel-rate,\n>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further\n>> + * details.\n>> + */\n>> +\n>> + /**\n>> + * \\var IPAConfigInfo::entityControls\n>> + * \\brief Controls supported by the sensor\n>> + *\n>> + * A map of V4L2 controls supported by the sensor in order to read or write\n>> + * control values to them.\n>> + */\n>> +\n>> + /**\n>> + * \\var IPAConfigInfo::bdsOutputSize\n>> + * \\brief Size of ImgU BDS rectangle\n>> + */\n>> +\n>> + /**\n>> + * \\var IPAConfigInfo::iif\n>> + * \\brief Size of ImgU input-feeder rectangle\n>> + */\n>> +struct IPAConfigInfo {\n>> +\tlibcamera.CameraSensorInfo sensorInfo;\n>> +\tmap entityControls;\n>> +\tlibcamera.Size bdsOutputSize;\n>> +\tlibcamera.Size iif;\n>> +};\n>> +\n> I think it is interesting, and would like your advice: I would need this\n> same structure for rkisp1 platform, except that the sizes are obviously\n> not the same.\n> And so, I am wondering if we could imagine a more general structure like:\n> struct IPAConfigInfo {\n> \tlibcamera.CameraSensorInfo sensorInfo;\n> \tmap entityControls;\n> \tlibcamera.Size ispInputSize;\n> \tlibcamera.Size ispOutputSize;\n> };\n>\n> names and all may have to be reworked but that is the base idea.\n> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least\n> you have an input an output pad size for the ISP node.\nCertainly, this might be a good idea. I am not opposed to it.\n\nThe question here is, that is the structure core.mojom worthy, in order \nto be shared between multiple IPAs right? If not, I think we need to \nkeep them in .mojom for now.\n\nAnd if we want to share the data-structures, we need to work out how all \nexisting IPAs (for e.g. Raspberry Pi) can be ported to used that \nIPAConfig structure too. We probably, should address this by looking at \nall the common parameters we need for all the existing IPAs and have a \npatch series that ports every IPA to use that data-structure.\n\nHowever, there are some risky? situations that can happen, for e.g.\n  - An IPA X requiring a parameter Y for configuration and the common \nstruct IPAConfig doesn't have a placeholder\n  - Parameter Y is added to common struct IPAConfig\n  - Parameter Y is unused/nullptr for all IPAs except IPA X\n\nI do not know if adding placeholder for Y is fine in the common struct, \nif fine or not, in the codebase purview.\n\nThinking about it, the chances of various IPA, having exact \nconfiguration data (common IPAConfig) are slim. Hence, the IPA authors \nwill try hacks for other parameters they need for IPA configuration, \neither as function-params or introducing new structures and passing \nalongside with common struct IPAConfig to the IPA. That would not be \nnice, isn't it?\n\nI might be wrong, so I shall keep an open mind about it. Maybe we can \ntrack this as a task somewhere too?\n\n>\n>> interface IPAIPU3Interface {\n>> \tinit(libcamera.IPASettings settings) => (int32 ret);\n>> \tstart() => (int32 ret);\n>> \tstop();\n>> \n>> -\tconfigure(map entityControls,\n>> -\t\t libcamera.Size bdsOutputSize) => ();\n>> +\tconfigure(IPAConfigInfo configInfo);\n>> \n>> \tmapBuffers(array buffers);\n>> \tunmapBuffers(array ids);\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index f5343547..769c24d3 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -43,8 +43,7 @@ public:\n>> \tint start() override;\n>> \tvoid stop() override {}\n>> \n>> -\tvoid configure(const std::map &entityControls,\n>> -\t\t const Size &bdsOutputSize) override;\n>> +\tvoid configure(const IPAConfigInfo &configInfo) override;\n>> \n>> \tvoid mapBuffers(const std::vector &buffers) override;\n>> \tvoid unmapBuffers(const std::vector &ids) override;\n>> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n>> \t\t\t << (int)bdsGrid_.height << \" << \" << (int)bdsGrid_.block_height_log2 << \")\";\n>> }\n>> \n>> -void IPAIPU3::configure(const std::map &entityControls,\n>> -\t\t\tconst Size &bdsOutputSize)\n>> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>> {\n>> -\tif (entityControls.empty())\n>> +\tif (configInfo.entityControls.empty())\n>> \t\treturn;\n>> \n>> -\tctrls_ = entityControls.at(0);\n>> +\tctrls_ = configInfo.entityControls.at(0);\n>> \n>> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>> \tif (itExp == ctrls_.end()) {\n>> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const std::map &entityControls\n>> \n>> \tparams_ = {};\n>> \n>> -\tcalculateBdsGrid(bdsOutputSize);\n>> +\tcalculateBdsGrid(configInfo.bdsOutputSize);\n>> \n>> \tawbAlgo_ = std::make_unique();\n>> -\tawbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);\n>> +\tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n>> \n>> \tagcAlgo_ = std::make_unique();\n>> \tagcAlgo_->initialise(bdsGrid_);\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 73306cea..b1ff1dbe 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>> \n>> \tstd::map entityControls;\n>> \tentityControls.emplace(0, data->cio2_.sensor()->controls());\n>> -\tdata->ipa_->configure(entityControls, config->imguConfig().bds);\n>> +\n>> +\tipa::ipu3::IPAConfigInfo configInfo;\n>> +\tconfigInfo.sensorInfo = sensorInfo;\n>> +\tconfigInfo.entityControls = entityControls;\n>> +\tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n>> +\tconfigInfo.iif = config->imguConfig().iif;\n>> +\n>> +\tdata->ipa_->configure(configInfo);\n>> \n>> \treturn 0;\n>> }\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 C37ACBDCC3\n\tfor ;\n\tTue, 27 Apr 2021 12:54:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F52A68866;\n\tTue, 27 Apr 2021 14:54:15 +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 EDA9C602C2\n\tfor ;\n\tTue, 27 Apr 2021 14:54:13 +0200 (CEST)","from localhost.localdomain (unknown [103.238.109.25])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EBA3EE9;\n\tTue, 27 Apr 2021 14:54:12 +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=\"km+0rSxa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619528053;\n\tbh=DT2zav0gaQW1+cavif7uxVsV2bBbJdUOM2GYC4JcK0U=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=km+0rSxaK0izz2GC8HTaUS6oCSZ5LDWRhooJEo1YT7yT4ftxiDHZLHQ4a/p8R4UNS\n\t55V0wZ9TNIHY0YWqNhmLAdlFVf0EUhcmDYgEDMMGtRDeRF23IsIKZHZAoGzVupSDLL\n\tgkerRnDdRi6W4tHGn0ZOuMtb04iIiNczQ77vJBYw=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>\n\t<762f677b-7a4a-f817-3d5e-b7d0922d00c7@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<4efbaad6-dae7-236c-bc91-49950ccd4fdc@ideasonboard.com>","Date":"Tue, 27 Apr 2021 18:24:08 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"<762f677b-7a4a-f817-3d5e-b7d0922d00c7@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16679,"web_url":"https://patchwork.libcamera.org/comment/16679/","msgid":"","date":"2021-04-29T05:30:47","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 27/04/2021 14:54, Umang Jain wrote:\n> Hi JM\n> \n> On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote:\n>> Hi Umang,\n>>\n>> Thanks for the patch !\n>>\n>> On 27/04/2021 08:35, Umang Jain wrote:\n>>> IPAConfigInfo is a consolidated data structure passed from IPU3\n>>> pipeline-handler to IPU3 IPA. This provides a streamlined and\n>>> extensible way to provide the configuration data to IPA interface.\n>>>\n>>> The IPA interface should be common to both closed-source and\n>>> open-source IPU3 IPAs. Hence, the structure encompasses additional\n>>> configuration information required to init and configure the\n>>> closed-source IPA as well.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> Changes in v2:\n>>> - Mark as RFC(do not merge) as this is built upon Paul's\n>>>    [Fix support for core.mojom structs v3] - waiting for merge to master\n>>> - Drop cropRegion_  from structure - this is provided from\n>>> CameraSensorInfo\n>>>    itself.\n>>> - Doxygen documentation of IPAConfigInfo.\n>>> ---\n>>>   include/libcamera/ipa/ipu3.mojom     | 46 ++++++++++++++++++++++++++--\n>>>   src/ipa/ipu3/ipu3.cpp                | 14 ++++-----\n>>>   src/libcamera/pipeline/ipu3/ipu3.cpp |  9 +++++-\n>>>   3 files changed, 58 insertions(+), 11 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/ipa/ipu3.mojom\n>>> b/include/libcamera/ipa/ipu3.mojom\n>>> index a717b1e6..acd5cfa4 100644\n>>> --- a/include/libcamera/ipa/ipu3.mojom\n>>> +++ b/include/libcamera/ipa/ipu3.mojom\n>>> @@ -25,13 +25,55 @@ struct IPU3Action {\n>>>       libcamera.ControlList controls;\n>>>   };\n>>>   +/**\n>>> + * \\struct IPAConfigInfo\n>>> + * \\brief Information to be passed from IPU3 pipeline-handler to IPA\n>>> + *\n>>> + * The IPAConfigInfo structure stores the data passed from IPU3\n>>> pipeline-handler\n>>> + * to the IPAIPU3Interface::configure(). The structure provides\n>>> extensibility\n>>> + * for additional configuration data as required, for closed-source or\n>>> + * open-source IPAs' configure() phases.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var IPAConfigInfo::sensorInfo\n>>> + * \\brief Camera sensor information\n>>> + *\n>>> + * Provides the camera sensor information such as model name,\n>>> pixel-rate,\n>>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for\n>>> further\n>>> + * details.\n>>> + */\n>>> +\n>>> + /**\n>>> + * \\var IPAConfigInfo::entityControls\n>>> + * \\brief Controls supported by the sensor\n>>> + *\n>>> + * A map of V4L2 controls supported by the sensor in order to read\n>>> or write\n>>> + * control values to them.\n>>> + */\n>>> +\n>>> + /**\n>>> + * \\var IPAConfigInfo::bdsOutputSize\n>>> + * \\brief Size of ImgU BDS rectangle\n>>> + */\n>>> +\n>>> + /**\n>>> + * \\var IPAConfigInfo::iif\n>>> + * \\brief Size of ImgU input-feeder rectangle\n>>> + */\n>>> +struct IPAConfigInfo {\n>>> +    libcamera.CameraSensorInfo sensorInfo;\n>>> +    map entityControls;\n>>> +    libcamera.Size bdsOutputSize;\n>>> +    libcamera.Size iif;\n>>> +};\n>>> +\n>> I think it is interesting, and would like your advice: I would need this\n>> same structure for rkisp1 platform, except that the sizes are obviously\n>> not the same.\n>> And so, I am wondering if we could imagine a more general structure like:\n>> struct IPAConfigInfo {\n>>     libcamera.CameraSensorInfo sensorInfo;\n>>     map entityControls;\n>>     libcamera.Size ispInputSize;\n>>     libcamera.Size ispOutputSize;\n>> };\n>>\n>> names and all may have to be reworked but that is the base idea.\n>> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least\n>> you have an input an output pad size for the ISP node.\n> Certainly, this might be a good idea. I am not opposed to it.\n> \n> The question here is, that is the structure core.mojom worthy, in order\n> to be shared between multiple IPAs right? If not, I think we need to\n> keep them in .mojom for now.\n> \n> And if we want to share the data-structures, we need to work out how all\n> existing IPAs (for e.g. Raspberry Pi) can be ported to used that\n> IPAConfig structure too. We probably, should address this by looking at\n> all the common parameters we need for all the existing IPAs and have a\n> patch series that ports every IPA to use that data-structure.\n> \n> However, there are some risky? situations that can happen, for e.g.\n>  - An IPA X requiring a parameter Y for configuration and the common\n> struct IPAConfig doesn't have a placeholder\n>  - Parameter Y is added to common struct IPAConfig\n>  - Parameter Y is unused/nullptr for all IPAs except IPA X\n> \n> I do not know if adding placeholder for Y is fine in the common struct,\n> if fine or not, in the codebase purview.\n> \n> Thinking about it, the chances of various IPA, having exact\n> configuration data (common IPAConfig) are slim. Hence, the IPA authors\n> will try hacks for other parameters they need for IPA configuration,\n> either as function-params or introducing new structures and passing\n> alongside with common struct IPAConfig to the IPA. That would not be\n> nice, isn't it?\n> \n> I might be wrong, so I shall keep an open mind about it. Maybe we can\n> track this as a task somewhere too?\n> \n\nThe idea I had is to have a common structure, and having the IPA\nspecific ones inherit from this common one.\nYou would have something like\nIPAIPU3ConfigInfo: IPAConfigInfo {\n\t/* myGreatParameters */\n}\n\nThat one may not make sense though :-).\n\n>>\n>>>   interface IPAIPU3Interface {\n>>>       init(libcamera.IPASettings settings) => (int32 ret);\n>>>       start() => (int32 ret);\n>>>       stop();\n>>>   -    configure(map entityControls,\n>>> -          libcamera.Size bdsOutputSize) => ();\n>>> +    configure(IPAConfigInfo configInfo);\n>>>         mapBuffers(array buffers);\n>>>       unmapBuffers(array ids);\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index f5343547..769c24d3 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -43,8 +43,7 @@ public:\n>>>       int start() override;\n>>>       void stop() override {}\n>>>   -    void configure(const std::map\n>>> &entityControls,\n>>> -               const Size &bdsOutputSize) override;\n>>> +    void configure(const IPAConfigInfo &configInfo) override;\n>>>         void mapBuffers(const std::vector &buffers) override;\n>>>       void unmapBuffers(const std::vector &ids) override;\n>>> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size\n>>> &bdsOutputSize)\n>>>                   << (int)bdsGrid_.height << \" << \" <<\n>>> (int)bdsGrid_.block_height_log2 << \")\";\n>>>   }\n>>>   -void IPAIPU3::configure(const std::map\n>>> &entityControls,\n>>> -            const Size &bdsOutputSize)\n>>> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>>>   {\n>>> -    if (entityControls.empty())\n>>> +    if (configInfo.entityControls.empty())\n>>>           return;\n>>>   -    ctrls_ = entityControls.at(0);\n>>> +    ctrls_ = configInfo.entityControls.at(0);\n>>>         const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>>>       if (itExp == ctrls_.end()) {\n>>> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const\n>>> std::map &entityControls\n>>>         params_ = {};\n>>>   -    calculateBdsGrid(bdsOutputSize);\n>>> +    calculateBdsGrid(configInfo.bdsOutputSize);\n>>>         awbAlgo_ = std::make_unique();\n>>> -    awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);\n>>> +    awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n>>>         agcAlgo_ = std::make_unique();\n>>>       agcAlgo_->initialise(bdsGrid_);\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 73306cea..b1ff1dbe 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera\n>>> *camera, CameraConfiguration *c)\n>>>         std::map entityControls;\n>>>       entityControls.emplace(0, data->cio2_.sensor()->controls());\n>>> -    data->ipa_->configure(entityControls, config->imguConfig().bds);\n>>> +\n>>> +    ipa::ipu3::IPAConfigInfo configInfo;\n>>> +    configInfo.sensorInfo = sensorInfo;\n>>> +    configInfo.entityControls = entityControls;\n>>> +    configInfo.bdsOutputSize = config->imguConfig().bds;\n>>> +    configInfo.iif = config->imguConfig().iif;\n>>> +\n>>> +    data->ipa_->configure(configInfo);\n>>>         return 0;\n>>>   }\n>>>\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 32CF7BDE44\n\tfor ;\n\tThu, 29 Apr 2021 05:30:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BA3468909;\n\tThu, 29 Apr 2021 07:30: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 DA2F9688AF\n\tfor ;\n\tThu, 29 Apr 2021 07:30:48 +0200 (CEST)","from [IPv6:2a01:e0a:169:7140:a545:36fd:61e5:5dc8] (unknown\n\t[IPv6:2a01:e0a:169:7140:a545:36fd:61e5:5dc8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B4ECBC0;\n\tThu, 29 Apr 2021 07:30:48 +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=\"aHgRdo7Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619674248;\n\tbh=LtOvB+li9jvE0B/6GpRwaNq3bSp02Eqlt76uw/lCpI0=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=aHgRdo7ZMbTN4KQkOyfM877RhRxLhklo1TYmtcravf8BFQQCqkavQLS8BQc9SCdot\n\t0oz3sgSdyVylK3k/CAwxdDJFk8u13NSmRbfXwwJsjs8HYzezjYpojZHAyY1PJXso3D\n\tWbYkeyRnJ4lAi1NcyAhhGwtSJa4t11LxFcYopkak=","To":"Umang Jain ,\n\tJean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>\n\t<762f677b-7a4a-f817-3d5e-b7d0922d00c7@ideasonboard.com>\n\t<4efbaad6-dae7-236c-bc91-49950ccd4fdc@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Thu, 29 Apr 2021 07:30:47 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<4efbaad6-dae7-236c-bc91-49950ccd4fdc@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16681,"web_url":"https://patchwork.libcamera.org/comment/16681/","msgid":"<20210429074514.svifyxthwj4ep7ts@uno.localdomain>","date":"2021-04-29T07:45:14","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Tue, Apr 27, 2021 at 12:05:27PM +0530, Umang Jain wrote:\n> IPAConfigInfo is a consolidated data structure passed from IPU3\n> pipeline-handler to IPU3 IPA. This provides a streamlined and\n> extensible way to provide the configuration data to IPA interface.\n>\n> The IPA interface should be common to both closed-source and\n> open-source IPU3 IPAs. Hence, the structure encompasses additional\n> configuration information required to init and configure the\n> closed-source IPA as well.\n>\n> Signed-off-by: Umang Jain \n> ---\n> Changes in v2:\n> - Mark as RFC(do not merge) as this is built upon Paul's\n> [Fix support for core.mojom structs v3] - waiting for merge to master\n> - Drop cropRegion_ from structure - this is provided from CameraSensorInfo\n> itself.\n> - Doxygen documentation of IPAConfigInfo.\n> ---\n> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++--\n> src/ipa/ipu3/ipu3.cpp | 14 ++++-----\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++-\n> 3 files changed, 58 insertions(+), 11 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index a717b1e6..acd5cfa4 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -25,13 +25,55 @@ struct IPU3Action {\n> \tlibcamera.ControlList controls;\n> };\n>\n> +/**\n> + * \\struct IPAConfigInfo\n> + * \\brief Information to be passed from IPU3 pipeline-handler to IPA\n\nto 'the' IPA ?\n\n> + *\n> + * The IPAConfigInfo structure stores the data passed from IPU3 pipeline-handler\n> + * to the IPAIPU3Interface::configure(). The structure provides extensibility\n> + * for additional configuration data as required, for closed-source or\n> + * open-source IPAs' configure() phases.\n\nWhat do you mean with extensibility ? That one can add fields to the\nstructure, like to all structures ? :)\nOr are you thinking at some inheritance mechanism ?\n\nIn the former case I would say\n\"The structure can be extended with additional parameters to\naccommodate the requirements of different IPA modules\"\n\n\n> + */\n> +\n> +/**\n> + * \\var IPAConfigInfo::sensorInfo\n> + * \\brief Camera sensor information\n> + *\n> + * Provides the camera sensor information such as model name, pixel-rate,\n> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further\n> + * details.\n\nI would just:\n \\sa CameraSensorInfo\n\nAs duplicating the description has limited value imho\n\n> + */\n> +\n> + /**\n> + * \\var IPAConfigInfo::entityControls\n> + * \\brief Controls supported by the sensor\n> + *\n> + * A map of V4L2 controls supported by the sensor in order to read or write\n> + * control values to them.\n> + */\n\nThe existing entityControls is described as\n\\param[in] entityControls Controls provided by the pipeline entities\n\nand it does not only include the sensor controls (otherwise it would\nhave been just a ControlInfoMap not a map). The idea was that each ph-IPA\ndefines it's own mapping. Ie entityControls[0] = sensor controls,\nentityControls[1] = someother entity controls, etc etc).\n\nSo far I think only this is only been effectively used to transport\nsensor controls, and I would be fine making this a ControlList and\nname it 'sensorControls'. Even if we later need to pass to IPA the\nControlInfoMap of another entity, we can add it to this structure with\na more explicit name (much better than hiding it in a map and\nestablishing what an index maps to like we're doing now)\n\n> +\n> + /**\n> + * \\var IPAConfigInfo::bdsOutputSize\n> + * \\brief Size of ImgU BDS rectangle\n> + */\n> +\n> + /**\n> + * \\var IPAConfigInfo::iif\n> + * \\brief Size of ImgU input-feeder rectangle\n> + */\n\nEmpty line maybe.\n\nI wonder if it wouldn't be better to pass the full\nImgUDevice::PipeConfig. True that the less data we transport over IPC\nthe better, so this makes sense. Is it worth wrapping thise two sizes\nin an 'ImguSize' structure ?\n\nBTW I don't see iif being used currently. Will you need it in future ?\n\n> +struct IPAConfigInfo {\n> +\tlibcamera.CameraSensorInfo sensorInfo;\n> +\tmap entityControls;\n> +\tlibcamera.Size bdsOutputSize;\n> +\tlibcamera.Size iif;\n> +};\n> +\n> interface IPAIPU3Interface {\n> \tinit(libcamera.IPASettings settings) => (int32 ret);\n> \tstart() => (int32 ret);\n> \tstop();\n>\n> -\tconfigure(map entityControls,\n> -\t\t libcamera.Size bdsOutputSize) => ();\n> +\tconfigure(IPAConfigInfo configInfo);\n\nNo idea how that works for IPC, but if this was a regular function I\nwould have made this\n configure(const IPAConfigInfo &configInfo)\n\n>\n> \tmapBuffers(array buffers);\n> \tunmapBuffers(array ids);\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index f5343547..769c24d3 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -43,8 +43,7 @@ public:\n> \tint start() override;\n> \tvoid stop() override {}\n>\n> -\tvoid configure(const std::map &entityControls,\n> -\t\t const Size &bdsOutputSize) override;\n> +\tvoid configure(const IPAConfigInfo &configInfo) override;\n\nAh\n\nWhy the two prototypes are different ?\n\n>\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \t\t\t << (int)bdsGrid_.height << \" << \" << (int)bdsGrid_.block_height_log2 << \")\";\n> }\n>\n> -void IPAIPU3::configure(const std::map &entityControls,\n> -\t\t\tconst Size &bdsOutputSize)\n> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> {\n> -\tif (entityControls.empty())\n> +\tif (configInfo.entityControls.empty())\n> \t\treturn;\n>\n> -\tctrls_ = entityControls.at(0);\n> +\tctrls_ = configInfo.entityControls.at(0);\n>\n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const std::map &entityControls\n>\n> \tparams_ = {};\n>\n> -\tcalculateBdsGrid(bdsOutputSize);\n> +\tcalculateBdsGrid(configInfo.bdsOutputSize);\n>\n> \tawbAlgo_ = std::make_unique();\n> -\tawbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);\n> +\tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n>\n> \tagcAlgo_ = std::make_unique();\n> \tagcAlgo_->initialise(bdsGrid_);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 73306cea..b1ff1dbe 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>\n> \tstd::map entityControls;\n> \tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> -\tdata->ipa_->configure(entityControls, config->imguConfig().bds);\n> +\n> +\tipa::ipu3::IPAConfigInfo configInfo;\n> +\tconfigInfo.sensorInfo = sensorInfo;\n> +\tconfigInfo.entityControls = entityControls;\n\nWhy a copy when you can populate this one directly ?\n\nOverall the idea is good. I would refrain from premature optimizations\nlike the one suggested by Jean-Micheal for the moment though, it's a\nbit too early in my opinion, and the effort for each pipeline handler\nto define its own exchange structures is limited.\n\nThanks\n j\n\n> +\tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> +\tconfigInfo.iif = config->imguConfig().iif;\n> +\n> +\tdata->ipa_->configure(configInfo);\n>\n> \treturn 0;\n> }\n> --\n> 2.26.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 9A7BBBDE44\n\tfor ;\n\tThu, 29 Apr 2021 07:44:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F13926890D;\n\tThu, 29 Apr 2021 09:44:34 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9485668909\n\tfor ;\n\tThu, 29 Apr 2021 09:44:33 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id D454F200005;\n\tThu, 29 Apr 2021 07:44:32 +0000 (UTC)"],"Date":"Thu, 29 Apr 2021 09:45:14 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210429074514.svifyxthwj4ep7ts@uno.localdomain>","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210427063527.219509-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16682,"web_url":"https://patchwork.libcamera.org/comment/16682/","msgid":"<913cf8dd-3eb5-4683-1efd-1b79507770cc@ideasonboard.com>","date":"2021-04-29T08:25:57","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for comments,\n\nOn 4/29/21 1:15 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Tue, Apr 27, 2021 at 12:05:27PM +0530, Umang Jain wrote:\n>> IPAConfigInfo is a consolidated data structure passed from IPU3\n>> pipeline-handler to IPU3 IPA. This provides a streamlined and\n>> extensible way to provide the configuration data to IPA interface.\n>>\n>> The IPA interface should be common to both closed-source and\n>> open-source IPU3 IPAs. Hence, the structure encompasses additional\n>> configuration information required to init and configure the\n>> closed-source IPA as well.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> Changes in v2:\n>> - Mark as RFC(do not merge) as this is built upon Paul's\n>> [Fix support for core.mojom structs v3] - waiting for merge to master\n>> - Drop cropRegion_ from structure - this is provided from CameraSensorInfo\n>> itself.\n>> - Doxygen documentation of IPAConfigInfo.\n>> ---\n>> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++--\n>> src/ipa/ipu3/ipu3.cpp | 14 ++++-----\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++-\n>> 3 files changed, 58 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index a717b1e6..acd5cfa4 100644\n>> --- a/include/libcamera/ipa/ipu3.mojom\n>> +++ b/include/libcamera/ipa/ipu3.mojom\n>> @@ -25,13 +25,55 @@ struct IPU3Action {\n>> \tlibcamera.ControlList controls;\n>> };\n>>\n>> +/**\n>> + * \\struct IPAConfigInfo\n>> + * \\brief Information to be passed from IPU3 pipeline-handler to IPA\n> to 'the' IPA ?\n>\n>> + *\n>> + * The IPAConfigInfo structure stores the data passed from IPU3 pipeline-handler\n>> + * to the IPAIPU3Interface::configure(). The structure provides extensibility\n>> + * for additional configuration data as required, for closed-source or\n>> + * open-source IPAs' configure() phases.\n> What do you mean with extensibility ? That one can add fields to the\n> structure, like to all structures ? :)\n> Or are you thinking at some inheritance mechanism ?\n>\n> In the former case I would say\n> \"The structure can be extended with additional parameters to\n> accommodate the requirements of different IPA modules\"\nOk, I meant the former, I shall re-word in next iteration\n>\n>\n>> + */\n>> +\n>> +/**\n>> + * \\var IPAConfigInfo::sensorInfo\n>> + * \\brief Camera sensor information\n>> + *\n>> + * Provides the camera sensor information such as model name, pixel-rate,\n>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for further\n>> + * details.\n> I would just:\n> \\sa CameraSensorInfo\n>\n> As duplicating the description has limited value imho\nAgreed\n>\n>> + */\n>> +\n>> + /**\n>> + * \\var IPAConfigInfo::entityControls\n>> + * \\brief Controls supported by the sensor\n>> + *\n>> + * A map of V4L2 controls supported by the sensor in order to read or write\n>> + * control values to them.\n>> + */\n> The existing entityControls is described as\n> \\param[in] entityControls Controls provided by the pipeline entities\n>\n> and it does not only include the sensor controls (otherwise it would\n> have been just a ControlInfoMap not a map). The idea was that each ph-IPA\n> defines it's own mapping. Ie entityControls[0] = sensor controls,\n> entityControls[1] = someother entity controls, etc etc).\n>\n> So far I think only this is only been effectively used to transport\n> sensor controls, and I would be fine making this a ControlList and\n> name it 'sensorControls'. Even if we later need to pass to IPA the\n> ControlInfoMap of another entity, we can add it to this structure with\n> a more explicit name (much better than hiding it in a map and\n> establishing what an index maps to like we're doing now)\nOh, thanks for the context. I'll take a note of it and tinker on how we \ncan address this change and adapt the documentation. Might need some \ncycle of discussion in general with Kieran too!\n>\n>> +\n>> + /**\n>> + * \\var IPAConfigInfo::bdsOutputSize\n>> + * \\brief Size of ImgU BDS rectangle\n>> + */\n>> +\n>> + /**\n>> + * \\var IPAConfigInfo::iif\n>> + * \\brief Size of ImgU input-feeder rectangle\n>> + */\n> Empty line maybe.\n>\n> I wonder if it wouldn't be better to pass the full\n> ImgUDevice::PipeConfig. True that the less data we transport over IPC\n> the better, so this makes sense. Is it worth wrapping thise two sizes\n> in an 'ImguSize' structure ?\nMaybe - I see this as an optimization point, so not my highest priority \nas of now.\n>\n> BTW I don't see iif being used currently. Will you need it in future ?\n`iif` is used for closed source IPA. So, the  current issue is we have \ntwo competing IPAs - Open IPU3 IPA (already merged in-tree) and a closed \none(probably will be residing in it's own git repo). But the ph-IPA \ninterface has to be common to both. This is why you don't see `iif` \nusage here(in open IPA), but is need for the closed one.\n>\n>> +struct IPAConfigInfo {\n>> +\tlibcamera.CameraSensorInfo sensorInfo;\n>> +\tmap entityControls;\n>> +\tlibcamera.Size bdsOutputSize;\n>> +\tlibcamera.Size iif;\n>> +};\n>> +\n>> interface IPAIPU3Interface {\n>> \tinit(libcamera.IPASettings settings) => (int32 ret);\n>> \tstart() => (int32 ret);\n>> \tstop();\n>>\n>> -\tconfigure(map entityControls,\n>> -\t\t libcamera.Size bdsOutputSize) => ();\n>> +\tconfigure(IPAConfigInfo configInfo);\n> No idea how that works for IPC, but if this was a regular function I\n> would have made this\n> configure(const IPAConfigInfo &configInfo)\nThese are mojom declarations - hence I believe the framework by-default \nenforces that all function parameters are `const` and passed \nby-reference. These mojom files are used to generated the .cpp interface \ncode (see include/libcamera/ipa/meson.build), which matches the \nprototype below (where you have comment (`Why the two prototypes are \ndifferent`).\n>\n>> \tmapBuffers(array buffers);\n>> \tunmapBuffers(array ids);\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index f5343547..769c24d3 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -43,8 +43,7 @@ public:\n>> \tint start() override;\n>> \tvoid stop() override {}\n>>\n>> -\tvoid configure(const std::map &entityControls,\n>> -\t\t const Size &bdsOutputSize) override;\n>> +\tvoid configure(const IPAConfigInfo &configInfo) override;\n> Ah\n>\n> Why the two prototypes are different ?\n>\n>> \tvoid mapBuffers(const std::vector &buffers) override;\n>> \tvoid unmapBuffers(const std::vector &ids) override;\n>> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n>> \t\t\t << (int)bdsGrid_.height << \" << \" << (int)bdsGrid_.block_height_log2 << \")\";\n>> }\n>>\n>> -void IPAIPU3::configure(const std::map &entityControls,\n>> -\t\t\tconst Size &bdsOutputSize)\n>> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>> {\n>> -\tif (entityControls.empty())\n>> +\tif (configInfo.entityControls.empty())\n>> \t\treturn;\n>>\n>> -\tctrls_ = entityControls.at(0);\n>> +\tctrls_ = configInfo.entityControls.at(0);\n>>\n>> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>> \tif (itExp == ctrls_.end()) {\n>> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const std::map &entityControls\n>>\n>> \tparams_ = {};\n>>\n>> -\tcalculateBdsGrid(bdsOutputSize);\n>> +\tcalculateBdsGrid(configInfo.bdsOutputSize);\n>>\n>> \tawbAlgo_ = std::make_unique();\n>> -\tawbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);\n>> +\tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n>>\n>> \tagcAlgo_ = std::make_unique();\n>> \tagcAlgo_->initialise(bdsGrid_);\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 73306cea..b1ff1dbe 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>>\n>> \tstd::map entityControls;\n>> \tentityControls.emplace(0, data->cio2_.sensor()->controls());\n>> -\tdata->ipa_->configure(entityControls, config->imguConfig().bds);\n>> +\n>> +\tipa::ipu3::IPAConfigInfo configInfo;\n>> +\tconfigInfo.sensorInfo = sensorInfo;\n>> +\tconfigInfo.entityControls = entityControls;\n> Why a copy when you can populate this one directly ?\nOriginally it was your suggested way only, but Laurent wanted to use \nnamed initializers for safety - as there are couple of `Sizes` \nparameters and one can get the ordering wrong in directly populating \nthem. IPAConfigInfo is generated from mojom framework and doesn't yet \nsupport named initiazers hence, this is what I ended up as closest \nimplementation.\n>\n> Overall the idea is good. I would refrain from premature optimizations\n> like the one suggested by Jean-Micheal for the moment though, it's a\n> bit too early in my opinion, and the effort for each pipeline handler\n> to define its own exchange structures is limited.\nYes - overall idea is good, but I would be comfortable doing that when \nwe have got some level of stability for the closed source IPA\n>\n> Thanks\n> j\n>\n>> +\tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n>> +\tconfigInfo.iif = config->imguConfig().iif;\n>> +\n>> +\tdata->ipa_->configure(configInfo);\n>>\n>> \treturn 0;\n>> }\n>> --\n>> 2.26.2\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","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 5874ABDE45\n\tfor ;\n\tThu, 29 Apr 2021 08:26:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E5126890F;\n\tThu, 29 Apr 2021 10:26:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E855C68909\n\tfor ;\n\tThu, 29 Apr 2021 10:26:02 +0200 (CEST)","from localhost.localdomain (unknown [103.238.109.25])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03AF8DA8;\n\tThu, 29 Apr 2021 10:26: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=\"NzcfTvm5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619684762;\n\tbh=HiElktS2VRGee4Wx9/uhUwTbi+pVHbmXdOsdraXcCUQ=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=NzcfTvm5C2Cs7p5eSP6gk8M+nQG4Url+bw+43iPE/ycSsVsg23YnX+lZiks8xzevy\n\tLuJvBPxrZ79M06CIOpa6j3lXSLMFf5Ee+XDBi8T8E/W/D45LGSJ346g6zmPp6IMznb\n\t60f0FUxP23KE56ymsSmbNegJ9rYvg6OW311H7kOc=","To":"Jacopo Mondi ","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>\n\t<20210429074514.svifyxthwj4ep7ts@uno.localdomain>","From":"Umang Jain ","Message-ID":"<913cf8dd-3eb5-4683-1efd-1b79507770cc@ideasonboard.com>","Date":"Thu, 29 Apr 2021 13:55:57 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"<20210429074514.svifyxthwj4ep7ts@uno.localdomain>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16683,"web_url":"https://patchwork.libcamera.org/comment/16683/","msgid":"<20210429083248.7uj6zydz5omriyxa@uno.localdomain>","date":"2021-04-29T08:32:48","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Thu, Apr 29, 2021 at 01:55:57PM +0530, Umang Jain wrote:\n\n[snip]\n\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > >\n> > > \tstd::map entityControls;\n> > > \tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> > > -\tdata->ipa_->configure(entityControls, config->imguConfig().bds);\n> > > +\n> > > +\tipa::ipu3::IPAConfigInfo configInfo;\n> > > +\tconfigInfo.sensorInfo = sensorInfo;\n> > > +\tconfigInfo.entityControls = entityControls;\n> > Why a copy when you can populate this one directly ?\n> Originally it was your suggested way only, but Laurent wanted to use named\n> initializers for safety - as there are couple of `Sizes` parameters and one\n> can get the ordering wrong in directly populating them. IPAConfigInfo is\n> generated from mojom framework and doesn't yet support named initiazers\n> hence, this is what I ended up as closest implementation.\n\nWhat I meant was:\n\nwhy do this\n std::map entityControls;\n\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n ....\n configInfo.entityControls = entityControls;\n\nInstead of\n configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());\n\nand avoid the copy ?\n\nThanks\n j\n\n\n> >\n> > Overall the idea is good. I would refrain from premature optimizations\n> > like the one suggested by Jean-Micheal for the moment though, it's a\n> > bit too early in my opinion, and the effort for each pipeline handler\n> > to define its own exchange structures is limited.\n> Yes - overall idea is good, but I would be comfortable doing that when we\n> have got some level of stability for the closed source IPA\n> >\n> > Thanks\n> > j\n> >\n> > > +\tconfigInfo.bdsOutputSize = config->imguConfig().bds;\n> > > +\tconfigInfo.iif = config->imguConfig().iif;\n> > > +\n> > > +\tdata->ipa_->configure(configInfo);\n> > >\n> > > \treturn 0;\n> > > }\n> > > --\n> > > 2.26.2\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 5B37DBDE44\n\tfor ;\n\tThu, 29 Apr 2021 08:32:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D575E6890B;\n\tThu, 29 Apr 2021 10:32:07 +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 88D5A68909\n\tfor ;\n\tThu, 29 Apr 2021 10:32:06 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 06ED5C0012;\n\tThu, 29 Apr 2021 08:32:05 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 29 Apr 2021 10:32:48 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210429083248.7uj6zydz5omriyxa@uno.localdomain>","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>\n\t<20210429074514.svifyxthwj4ep7ts@uno.localdomain>\n\t<913cf8dd-3eb5-4683-1efd-1b79507770cc@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<913cf8dd-3eb5-4683-1efd-1b79507770cc@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16691,"web_url":"https://patchwork.libcamera.org/comment/16691/","msgid":"","date":"2021-04-30T00:40:13","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Thu, Apr 29, 2021 at 07:30:47AM +0200, Jean-Michel Hautbois wrote:\n> On 27/04/2021 14:54, Umang Jain wrote:\n> > On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote:\n> >> On 27/04/2021 08:35, Umang Jain wrote:\n> >>> IPAConfigInfo is a consolidated data structure passed from IPU3\n> >>> pipeline-handler to IPU3 IPA. This provides a streamlined and\n> >>> extensible way to provide the configuration data to IPA interface.\n> >>>\n> >>> The IPA interface should be common to both closed-source and\n> >>> open-source IPU3 IPAs. Hence, the structure encompasses additional\n> >>> configuration information required to init and configure the\n> >>> closed-source IPA as well.\n> >>>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> Changes in v2:\n> >>> - Mark as RFC(do not merge) as this is built upon Paul's\n> >>>    [Fix support for core.mojom structs v3] - waiting for merge to master\n> >>> - Drop cropRegion_  from structure - this is provided from\n> >>> CameraSensorInfo\n> >>>    itself.\n> >>> - Doxygen documentation of IPAConfigInfo.\n> >>> ---\n> >>>   include/libcamera/ipa/ipu3.mojom     | 46 ++++++++++++++++++++++++++--\n> >>>   src/ipa/ipu3/ipu3.cpp                | 14 ++++-----\n> >>>   src/libcamera/pipeline/ipu3/ipu3.cpp |  9 +++++-\n> >>>   3 files changed, 58 insertions(+), 11 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/ipa/ipu3.mojom\n> >>> b/include/libcamera/ipa/ipu3.mojom\n> >>> index a717b1e6..acd5cfa4 100644\n> >>> --- a/include/libcamera/ipa/ipu3.mojom\n> >>> +++ b/include/libcamera/ipa/ipu3.mojom\n> >>> @@ -25,13 +25,55 @@ struct IPU3Action {\n> >>>       libcamera.ControlList controls;\n> >>>   };\n> >>>   +/**\n> >>> + * \\struct IPAConfigInfo\n> >>> + * \\brief Information to be passed from IPU3 pipeline-handler to IPA\n> >>> + *\n> >>> + * The IPAConfigInfo structure stores the data passed from IPU3\n> >>> pipeline-handler\n> >>> + * to the IPAIPU3Interface::configure(). The structure provides\n> >>> extensibility\n> >>> + * for additional configuration data as required, for closed-source or\n> >>> + * open-source IPAs' configure() phases.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPAConfigInfo::sensorInfo\n> >>> + * \\brief Camera sensor information\n> >>> + *\n> >>> + * Provides the camera sensor information such as model name,\n> >>> pixel-rate,\n> >>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for\n> >>> further\n> >>> + * details.\n> >>> + */\n> >>> +\n> >>> + /**\n> >>> + * \\var IPAConfigInfo::entityControls\n> >>> + * \\brief Controls supported by the sensor\n> >>> + *\n> >>> + * A map of V4L2 controls supported by the sensor in order to read\n> >>> or write\n> >>> + * control values to them.\n> >>> + */\n> >>> +\n> >>> + /**\n> >>> + * \\var IPAConfigInfo::bdsOutputSize\n> >>> + * \\brief Size of ImgU BDS rectangle\n> >>> + */\n> >>> +\n> >>> + /**\n> >>> + * \\var IPAConfigInfo::iif\n> >>> + * \\brief Size of ImgU input-feeder rectangle\n> >>> + */\n> >>> +struct IPAConfigInfo {\n> >>> +    libcamera.CameraSensorInfo sensorInfo;\n> >>> +    map entityControls;\n> >>> +    libcamera.Size bdsOutputSize;\n> >>> +    libcamera.Size iif;\n> >>> +};\n> >>> +\n> >> I think it is interesting, and would like your advice: I would need this\n> >> same structure for rkisp1 platform, except that the sizes are obviously\n> >> not the same.\n> >> And so, I am wondering if we could imagine a more general structure like:\n> >> struct IPAConfigInfo {\n> >>     libcamera.CameraSensorInfo sensorInfo;\n> >>     map entityControls;\n> >>     libcamera.Size ispInputSize;\n> >>     libcamera.Size ispOutputSize;\n> >> };\n> >>\n> >> names and all may have to be reworked but that is the base idea.\n> >> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least\n> >> you have an input an output pad size for the ISP node.\n> > Certainly, this might be a good idea. I am not opposed to it.\n> > \n> > The question here is, that is the structure core.mojom worthy, in order\n> > to be shared between multiple IPAs right? If not, I think we need to\n> > keep them in .mojom for now.\n> > \n> > And if we want to share the data-structures, we need to work out how all\n> > existing IPAs (for e.g. Raspberry Pi) can be ported to used that\n> > IPAConfig structure too. We probably, should address this by looking at\n> > all the common parameters we need for all the existing IPAs and have a\n> > patch series that ports every IPA to use that data-structure.\n> > \n> > However, there are some risky? situations that can happen, for e.g.\n> >  - An IPA X requiring a parameter Y for configuration and the common\n> > struct IPAConfig doesn't have a placeholder\n> >  - Parameter Y is added to common struct IPAConfig\n> >  - Parameter Y is unused/nullptr for all IPAs except IPA X\n> > \n> > I do not know if adding placeholder for Y is fine in the common struct,\n> > if fine or not, in the codebase purview.\n> > \n> > Thinking about it, the chances of various IPA, having exact\n> > configuration data (common IPAConfig) are slim. Hence, the IPA authors\n> > will try hacks for other parameters they need for IPA configuration,\n> > either as function-params or introducing new structures and passing\n> > alongside with common struct IPAConfig to the IPA. That would not be\n> > nice, isn't it?\n> > \n> > I might be wrong, so I shall keep an open mind about it. Maybe we can\n> > track this as a task somewhere too?\n> > \n> \n> The idea I had is to have a common structure, and having the IPA\n> specific ones inherit from this common one.\n> You would have something like\n> IPAIPU3ConfigInfo: IPAConfigInfo {\n> \t/* myGreatParameters */\n> }\n> \n> That one may not make sense though :-).\n\nIPA parameters are very platform-specific, I don't think this would\nbring much benefit.\n\n> >>>   interface IPAIPU3Interface {\n> >>>       init(libcamera.IPASettings settings) => (int32 ret);\n> >>>       start() => (int32 ret);\n> >>>       stop();\n> >>>   -    configure(map entityControls,\n> >>> -          libcamera.Size bdsOutputSize) => ();\n> >>> +    configure(IPAConfigInfo configInfo);\n> >>>         mapBuffers(array buffers);\n> >>>       unmapBuffers(array ids);\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index f5343547..769c24d3 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -43,8 +43,7 @@ public:\n> >>>       int start() override;\n> >>>       void stop() override {}\n> >>>   -    void configure(const std::map\n> >>> &entityControls,\n> >>> -               const Size &bdsOutputSize) override;\n> >>> +    void configure(const IPAConfigInfo &configInfo) override;\n> >>>         void mapBuffers(const std::vector &buffers) override;\n> >>>       void unmapBuffers(const std::vector &ids) override;\n> >>> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size\n> >>> &bdsOutputSize)\n> >>>                   << (int)bdsGrid_.height << \" << \" <<\n> >>> (int)bdsGrid_.block_height_log2 << \")\";\n> >>>   }\n> >>>   -void IPAIPU3::configure(const std::map\n> >>> &entityControls,\n> >>> -            const Size &bdsOutputSize)\n> >>> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> >>>   {\n> >>> -    if (entityControls.empty())\n> >>> +    if (configInfo.entityControls.empty())\n> >>>           return;\n> >>>   -    ctrls_ = entityControls.at(0);\n> >>> +    ctrls_ = configInfo.entityControls.at(0);\n> >>>         const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> >>>       if (itExp == ctrls_.end()) {\n> >>> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const\n> >>> std::map &entityControls\n> >>>         params_ = {};\n> >>>   -    calculateBdsGrid(bdsOutputSize);\n> >>> +    calculateBdsGrid(configInfo.bdsOutputSize);\n> >>>         awbAlgo_ = std::make_unique();\n> >>> -    awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);\n> >>> +    awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> >>>         agcAlgo_ = std::make_unique();\n> >>>       agcAlgo_->initialise(bdsGrid_);\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index 73306cea..b1ff1dbe 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera\n> >>> *camera, CameraConfiguration *c)\n> >>>         std::map entityControls;\n> >>>       entityControls.emplace(0, data->cio2_.sensor()->controls());\n> >>> -    data->ipa_->configure(entityControls, config->imguConfig().bds);\n> >>> +\n> >>> +    ipa::ipu3::IPAConfigInfo configInfo;\n> >>> +    configInfo.sensorInfo = sensorInfo;\n> >>> +    configInfo.entityControls = entityControls;\n> >>> +    configInfo.bdsOutputSize = config->imguConfig().bds;\n> >>> +    configInfo.iif = config->imguConfig().iif;\n> >>> +\n> >>> +    data->ipa_->configure(configInfo);\n> >>>         return 0;\n> >>>   }\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 81887BDE45\n\tfor ;\n\tFri, 30 Apr 2021 00:40:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B84C468909;\n\tFri, 30 Apr 2021 02:40:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28AFA688A5\n\tfor ;\n\tFri, 30 Apr 2021 02:40: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 817E0B8B;\n\tFri, 30 Apr 2021 02:40: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=\"jBuSaLjZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619743214;\n\tbh=54GjznrzzNlG20Sr6CEd/LvzMbL/uFZ3vx6HmsRNdds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jBuSaLjZbCn3ZsLuZVh3y0wq6olajYxIpUvldK1Y5AWJeC4tBrvc6manVFm76d1Gt\n\t36Uxm2/0aB9NuF+2PLyFwp4xL8SXaGXUqi1EHhr5b+ZZOLU7tMldPXVzTw8hTOzx5d\n\tfudiqNDpGT9J4wLda8CpEMFTaGf9+fZh2IK7qN9U=","Date":"Fri, 30 Apr 2021 03:40:13 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>\n\t<762f677b-7a4a-f817-3d5e-b7d0922d00c7@ideasonboard.com>\n\t<4efbaad6-dae7-236c-bc91-49950ccd4fdc@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16692,"web_url":"https://patchwork.libcamera.org/comment/16692/","msgid":"<20210430032240.GN195599@pyrite.rasen.tech>","date":"2021-04-30T03:22:40","subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hello Umang and Jean-Michel,\n\nOn Thu, Apr 29, 2021 at 07:30:47AM +0200, Jean-Michel Hautbois wrote:\n> \n> \n> On 27/04/2021 14:54, Umang Jain wrote:\n> > Hi JM\n> > \n> > On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote:\n> >> Hi Umang,\n> >>\n> >> Thanks for the patch !\n> >>\n> >> On 27/04/2021 08:35, Umang Jain wrote:\n> >>> IPAConfigInfo is a consolidated data structure passed from IPU3\n> >>> pipeline-handler to IPU3 IPA. This provides a streamlined and\n> >>> extensible way to provide the configuration data to IPA interface.\n> >>>\n> >>> The IPA interface should be common to both closed-source and\n> >>> open-source IPU3 IPAs. Hence, the structure encompasses additional\n> >>> configuration information required to init and configure the\n> >>> closed-source IPA as well.\n> >>>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> Changes in v2:\n> >>> - Mark as RFC(do not merge) as this is built upon Paul's\n> >>>    [Fix support for core.mojom structs v3] - waiting for merge to master\n\nThis has been merged already.\n\n> >>> - Drop cropRegion_  from structure - this is provided from\n> >>> CameraSensorInfo\n> >>>    itself.\n> >>> - Doxygen documentation of IPAConfigInfo.\n> >>> ---\n> >>>   include/libcamera/ipa/ipu3.mojom     | 46 ++++++++++++++++++++++++++--\n> >>>   src/ipa/ipu3/ipu3.cpp                | 14 ++++-----\n> >>>   src/libcamera/pipeline/ipu3/ipu3.cpp |  9 +++++-\n> >>>   3 files changed, 58 insertions(+), 11 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/ipa/ipu3.mojom\n> >>> b/include/libcamera/ipa/ipu3.mojom\n> >>> index a717b1e6..acd5cfa4 100644\n> >>> --- a/include/libcamera/ipa/ipu3.mojom\n> >>> +++ b/include/libcamera/ipa/ipu3.mojom\n> >>> @@ -25,13 +25,55 @@ struct IPU3Action {\n> >>>       libcamera.ControlList controls;\n> >>>   };\n> >>>   +/**\n> >>> + * \\struct IPAConfigInfo\n> >>> + * \\brief Information to be passed from IPU3 pipeline-handler to IPA\n> >>> + *\n> >>> + * The IPAConfigInfo structure stores the data passed from IPU3\n> >>> pipeline-handler\n> >>> + * to the IPAIPU3Interface::configure(). The structure provides\n> >>> extensibility\n> >>> + * for additional configuration data as required, for closed-source or\n> >>> + * open-source IPAs' configure() phases.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\var IPAConfigInfo::sensorInfo\n> >>> + * \\brief Camera sensor information\n> >>> + *\n> >>> + * Provides the camera sensor information such as model name,\n> >>> pixel-rate,\n> >>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for\n> >>> further\n> >>> + * details.\n> >>> + */\n> >>> +\n> >>> + /**\n> >>> + * \\var IPAConfigInfo::entityControls\n> >>> + * \\brief Controls supported by the sensor\n> >>> + *\n> >>> + * A map of V4L2 controls supported by the sensor in order to read\n> >>> or write\n> >>> + * control values to them.\n> >>> + */\n> >>> +\n> >>> + /**\n> >>> + * \\var IPAConfigInfo::bdsOutputSize\n> >>> + * \\brief Size of ImgU BDS rectangle\n> >>> + */\n> >>> +\n> >>> + /**\n> >>> + * \\var IPAConfigInfo::iif\n> >>> + * \\brief Size of ImgU input-feeder rectangle\n> >>> + */\n> >>> +struct IPAConfigInfo {\n> >>> +    libcamera.CameraSensorInfo sensorInfo;\n> >>> +    map entityControls;\n> >>> +    libcamera.Size bdsOutputSize;\n> >>> +    libcamera.Size iif;\n> >>> +};\n> >>> +\n> >> I think it is interesting, and would like your advice: I would need this\n> >> same structure for rkisp1 platform, except that the sizes are obviously\n> >> not the same.\n> >> And so, I am wondering if we could imagine a more general structure like:\n> >> struct IPAConfigInfo {\n> >>     libcamera.CameraSensorInfo sensorInfo;\n> >>     map entityControls;\n> >>     libcamera.Size ispInputSize;\n> >>     libcamera.Size ispOutputSize;\n> >> };\n> >>\n> >> names and all may have to be reworked but that is the base idea.\n> >> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least\n> >> you have an input an output pad size for the ISP node.\n> > Certainly, this might be a good idea. I am not opposed to it.\n> > \n> > The question here is, that is the structure core.mojom worthy, in order\n> > to be shared between multiple IPAs right? If not, I think we need to\n> > keep them in .mojom for now.\n> > \n> > And if we want to share the data-structures, we need to work out how all\n> > existing IPAs (for e.g. Raspberry Pi) can be ported to used that\n> > IPAConfig structure too. We probably, should address this by looking at\n> > all the common parameters we need for all the existing IPAs and have a\n> > patch series that ports every IPA to use that data-structure.\n> > \n> > However, there are some risky? situations that can happen, for e.g.\n> >  - An IPA X requiring a parameter Y for configuration and the common\n> > struct IPAConfig doesn't have a placeholder\n> >  - Parameter Y is added to common struct IPAConfig\n> >  - Parameter Y is unused/nullptr for all IPAs except IPA X\n> > \n> > I do not know if adding placeholder for Y is fine in the common struct,\n> > if fine or not, in the codebase purview.\n> > \n> > Thinking about it, the chances of various IPA, having exact\n> > configuration data (common IPAConfig) are slim. Hence, the IPA authors\n> > will try hacks for other parameters they need for IPA configuration,\n> > either as function-params or introducing new structures and passing\n> > alongside with common struct IPAConfig to the IPA. That would not be\n> > nice, isn't it?\n\nWe could have a greatest common divisor for the IPA config info struct\nin core.mojom, and then the IPAs could define their own additional\nstruct.\n\n> > \n> > I might be wrong, so I shall keep an open mind about it. Maybe we can\n> > track this as a task somewhere too?\n> > \n> \n> The idea I had is to have a common structure, and having the IPA\n> specific ones inherit from this common one.\n> You would have something like\n> IPAIPU3ConfigInfo: IPAConfigInfo {\n> \t/* myGreatParameters */\n> }\n\nBecause mojo doesn't support inheritance :/\n\nWell okay, if you *really* wanted to, in theory, we could use an attribute\nto declare inheritance, and then I'd have to expand the code generator\nfor that (just keeping doors and minds open).\n\n\nPaul\n\n> \n> That one may not make sense though :-).\n> \n> >>\n> >>>   interface IPAIPU3Interface {\n> >>>       init(libcamera.IPASettings settings) => (int32 ret);\n> >>>       start() => (int32 ret);\n> >>>       stop();\n> >>>   -    configure(map entityControls,\n> >>> -          libcamera.Size bdsOutputSize) => ();\n> >>> +    configure(IPAConfigInfo configInfo);\n> >>>         mapBuffers(array buffers);\n> >>>       unmapBuffers(array ids);\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index f5343547..769c24d3 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -43,8 +43,7 @@ public:\n> >>>       int start() override;\n> >>>       void stop() override {}\n> >>>   -    void configure(const std::map\n> >>> &entityControls,\n> >>> -               const Size &bdsOutputSize) override;\n> >>> +    void configure(const IPAConfigInfo &configInfo) override;\n> >>>         void mapBuffers(const std::vector &buffers) override;\n> >>>       void unmapBuffers(const std::vector &ids) override;\n> >>> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size\n> >>> &bdsOutputSize)\n> >>>                   << (int)bdsGrid_.height << \" << \" <<\n> >>> (int)bdsGrid_.block_height_log2 << \")\";\n> >>>   }\n> >>>   -void IPAIPU3::configure(const std::map\n> >>> &entityControls,\n> >>> -            const Size &bdsOutputSize)\n> >>> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> >>>   {\n> >>> -    if (entityControls.empty())\n> >>> +    if (configInfo.entityControls.empty())\n> >>>           return;\n> >>>   -    ctrls_ = entityControls.at(0);\n> >>> +    ctrls_ = configInfo.entityControls.at(0);\n> >>>         const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> >>>       if (itExp == ctrls_.end()) {\n> >>> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const\n> >>> std::map &entityControls\n> >>>         params_ = {};\n> >>>   -    calculateBdsGrid(bdsOutputSize);\n> >>> +    calculateBdsGrid(configInfo.bdsOutputSize);\n> >>>         awbAlgo_ = std::make_unique();\n> >>> -    awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);\n> >>> +    awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> >>>         agcAlgo_ = std::make_unique();\n> >>>       agcAlgo_->initialise(bdsGrid_);\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index 73306cea..b1ff1dbe 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera\n> >>> *camera, CameraConfiguration *c)\n> >>>         std::map entityControls;\n> >>>       entityControls.emplace(0, data->cio2_.sensor()->controls());\n> >>> -    data->ipa_->configure(entityControls, config->imguConfig().bds);\n> >>> +\n> >>> +    ipa::ipu3::IPAConfigInfo configInfo;\n> >>> +    configInfo.sensorInfo = sensorInfo;\n> >>> +    configInfo.entityControls = entityControls;\n> >>> +    configInfo.bdsOutputSize = config->imguConfig().bds;\n> >>> +    configInfo.iif = config->imguConfig().iif;\n> >>> +\n> >>> +    data->ipa_->configure(configInfo);\n> >>>         return 0;\n> >>>   }\n> >>>\n> > \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 B4AC4BDE45\n\tfor ;\n\tFri, 30 Apr 2021 03:22:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4F9968911;\n\tFri, 30 Apr 2021 05:22: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 3E997602C0\n\tfor ;\n\tFri, 30 Apr 2021 05:22: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 48671AF0;\n\tFri, 30 Apr 2021 05:22:45 +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=\"kQ+uSxd1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619752967;\n\tbh=qXb4U5GjMwf0hr3qSecebTOF+/nFQeNjl9N6bbLYr08=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kQ+uSxd1t6xqa8n0MtAt/CCz4b7BlihF3p1DRbWEA4VI9oSvmR818R6WYfC5/UVzm\n\tUdfLaY8f4mkDaG4TkgK0bZjeUiJ2Q52pZRoToLBwWPAUFYjHJ5THuxzuwDW85L3enY\n\ttiCzRTj3N108S54n/cGNrM56GjTyqPtBiiNXpfms=","Date":"Fri, 30 Apr 2021 12:22:40 +0900","From":"paul.elder@ideasonboard.com","To":"Jean-Michel Hautbois ","Message-ID":"<20210430032240.GN195599@pyrite.rasen.tech>","References":"<20210427063527.219509-1-umang.jain@ideasonboard.com>\n\t<762f677b-7a4a-f817-3d5e-b7d0922d00c7@ideasonboard.com>\n\t<4efbaad6-dae7-236c-bc91-49950ccd4fdc@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce\n\tIPAConfigInfo in IPC","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]