[{"id":16599,"web_url":"https://patchwork.libcamera.org/comment/16599/","msgid":"","date":"2021-04-27T02:57:43","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Apr 26, 2021 at 10:46:08PM +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\nThe commit message should mention that you're adding more data to the\nstructure, and why.\n\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/ipa/ipu3.mojom | 11 +++++++++--\n> src/ipa/ipu3/ipu3.cpp | 14 ++++++--------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 ++++++-\n> 3 files changed, 21 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index a717b1e6..88cbb403 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -25,13 +25,20 @@ struct IPU3Action {\n> \tlibcamera.ControlList controls;\n> };\n> \n> +struct IPAConfigInfo {\n> +\tlibcamera.CameraSensorInfo sensorInfo;\n> +\tmap entityControls;\n> +\tlibcamera.Size bdsOutputSize;\n> +\tlibcamera.Size iif;\n> +\tlibcamera.Size cropRegion;\n> +};\n\nCould you document this structure with doxygen comments ? We don't\ngenerate documentation from mojom files yet, but we should still be\nprepared.\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\nWhile at it, I think you can drop the => ().\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..a77afd55 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 struct 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 struct 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..be43d5a1 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -635,7 +635,12 @@ 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> +\tstruct ipa::ipu3::IPAConfigInfo configInfo = {\n\nYou can drop the struct keyword.\n\n> +\t\tsensorInfo, entityControls, config->imguConfig().bds,\n> +\t\tconfig->imguConfig().iif, data->cropRegion_.size()\n> +\t};\n\nGiven that several members of this structures have the same type, it\nwould be safer to use named initializers.\n\n\tipa::ipu3::IPAConfigInfo configInfo = {\n\t\t.sensorInfo = sensorInfo,\n\t\t.entityControls = entityControls,\n\t\t.bdsOutputSize = config->imguConfig().bds,\n\t\t.iif = config->imguConfig().iif,\n\t\t.cropRegion = data->cropRegion_.size(),\n\t};\n\n> +\tdata->ipa_->configure(configInfo);\n> \n> \treturn 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 66D9CBDCA4\n\tfor ;\n\tTue, 27 Apr 2021 02:57:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC5EF68878;\n\tTue, 27 Apr 2021 04:57:50 +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 BF89560512\n\tfor ;\n\tTue, 27 Apr 2021 04:57:49 +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 222AEE9;\n\tTue, 27 Apr 2021 04:57:49 +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=\"beOaUR9R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619492269;\n\tbh=HNuTs9gGeT66ICl/M7P9bkjJ+2C7OaAej4PEKmkumP8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=beOaUR9RPL5MTQt0J1MsTTsxarpNdr6EaEIJfN+tJFVmKKDTTrMOIZ5whfwSi+whY\n\tXSm/oeewwPmmBKpMoYNE8deolwcz5xNt/jDNNf0nPuefaItR9xT/MjwVHcWl37DDdM\n\tt9L7FnklE5yZLZuSCNyQ7Zl7r+KrB+1n3WOyN0fw=","Date":"Tue, 27 Apr 2021 05:57:43 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210426171608.145784-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210426171608.145784-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","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":16622,"web_url":"https://patchwork.libcamera.org/comment/16622/","msgid":"<0e5b310d-6a22-e612-2223-a33e38fda202@ideasonboard.com>","date":"2021-04-27T06:10:25","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThanks for the reviews.\n\nOn 4/27/21 8:27 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 26, 2021 at 10:46:08PM +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> The commit message should mention that you're adding more data to the\n> structure, and why.\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/ipa/ipu3.mojom | 11 +++++++++--\n>> src/ipa/ipu3/ipu3.cpp | 14 ++++++--------\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 ++++++-\n>> 3 files changed, 21 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index a717b1e6..88cbb403 100644\n>> --- a/include/libcamera/ipa/ipu3.mojom\n>> +++ b/include/libcamera/ipa/ipu3.mojom\n>> @@ -25,13 +25,20 @@ struct IPU3Action {\n>> \tlibcamera.ControlList controls;\n>> };\n>> \n>> +struct IPAConfigInfo {\n>> +\tlibcamera.CameraSensorInfo sensorInfo;\n>> +\tmap entityControls;\n>> +\tlibcamera.Size bdsOutputSize;\n>> +\tlibcamera.Size iif;\n>> +\tlibcamera.Size cropRegion;\n>> +};\n> Could you document this structure with doxygen comments ? We don't\n> generate documentation from mojom files yet, but we should still be\n> prepared.\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> While at it, I think you can drop the => ().\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..a77afd55 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 struct 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 struct 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..be43d5a1 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -635,7 +635,12 @@ 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>> +\tstruct ipa::ipu3::IPAConfigInfo configInfo = {\n> You can drop the struct keyword.\n>\n>> +\t\tsensorInfo, entityControls, config->imguConfig().bds,\n>> +\t\tconfig->imguConfig().iif, data->cropRegion_.size()\n>> +\t};\n> Given that several members of this structures have the same type, it\n> would be safer to use named initializers.\n>\n> \tipa::ipu3::IPAConfigInfo configInfo = {\n> \t\t.sensorInfo = sensorInfo,\n> \t\t.entityControls = entityControls,\n> \t\t.bdsOutputSize = config->imguConfig().bds,\n> \t\t.iif = config->imguConfig().iif,\n> \t\t.cropRegion = data->cropRegion_.size(),\n> \t};\nThis throws a compile-time error because the generated \nipu3-ipa-interface.h code doesn't suppport named initializers for \nIPAConfigInfo I think. I think we need to support in mojom first?\n\nFAILED: src/libcamera/4ab8042@@camera@sha/pipeline_ipu3_ipu3.cpp.o\n../../../../../tmp/portage/media-libs/libcamera-9999/work/libcamera-9999/src/libcamera/pipeline/ipu3/ipu3.cpp:639:27: \nerror: no matching constructor for initialization of \n'ipa::ipu3::IPAConfigInfo'\n         ipa::ipu3::IPAConfigInfo configInfo = {\n                                  ^            ~\ninclude/libcamera/ipa/ipu3_ipa_interface.h:96:2: note: candidate \nconstructor not viable: cannot convert argument of incomplete type \n'void' to 'const libcamera::CameraSensorInfo' for 1st argument\n         IPAConfigInfo(const CameraSensorInfo &_sensorInfo, const \nstd::map &_entityControls, const Size \n&_bdsOutputSize, const Size &_iif)\n         ^\ninclude/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate \nconstructor (the implicit copy constructor) not viable: requires 1 \nargument, but 4 were provided\nstruct IPAConfigInfo\n        ^\ninclude/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate \nconstructor (the implicit move constructor) not viable: requires 1 \nargument, but 4 were provided\ninclude/libcamera/ipa/ipu3_ipa_interface.h:92:2: note: candidate \nconstructor not viable: requires 0 arguments, but 4 were provided\n         IPAConfigInfo()\n         ^\n1 error generated.\n\nMeanwhile, in order to address your concerns, maybe this?\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>> +\tdata->ipa_->configure(configInfo);\n>> \n>> \treturn 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 7EEFCBDCA6\n\tfor ;\n\tTue, 27 Apr 2021 06:10:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B907A68880;\n\tTue, 27 Apr 2021 08:10:37 +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 53CDE60512\n\tfor ;\n\tTue, 27 Apr 2021 08:10:36 +0200 (CEST)","from localhost.localdomain (unknown [103.238.109.25])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 632CDE9;\n\tTue, 27 Apr 2021 08:10: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=\"ZJpOwutH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619503836;\n\tbh=9+dcIi6PEYTi+TqtP7dCUGrSJf5v0jC2HRIzPDYJdiw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ZJpOwutHfM5efqREcMjHUCW1a051rtqg6t8djXtOVTslhP/UFXTCRTicFhWyZesXd\n\t7DrWPyU+xk+LBImtoWNqHMmYnFcrs7WZS1u9L5HNGa+T3pezeRxQoIZQQShMtw7xQK\n\tbnaF9yJk4+Nx/6lDAE+yOPOfFOCwlQMKybDq9R30=","To":"Laurent Pinchart ","References":"<20210426171608.145784-1-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<0e5b310d-6a22-e612-2223-a33e38fda202@ideasonboard.com>","Date":"Tue, 27 Apr 2021 11:40:25 +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":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","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":16624,"web_url":"https://patchwork.libcamera.org/comment/16624/","msgid":"","date":"2021-04-27T06:37:24","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Apr 27, 2021 at 11:40:25AM +0530, Umang Jain wrote:\n> On 4/27/21 8:27 AM, Laurent Pinchart wrote:\n> > On Mon, Apr 26, 2021 at 10:46:08PM +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> > The commit message should mention that you're adding more data to the\n> > structure, and why.\n> >\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> include/libcamera/ipa/ipu3.mojom | 11 +++++++++--\n> >> src/ipa/ipu3/ipu3.cpp | 14 ++++++--------\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 ++++++-\n> >> 3 files changed, 21 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> >> index a717b1e6..88cbb403 100644\n> >> --- a/include/libcamera/ipa/ipu3.mojom\n> >> +++ b/include/libcamera/ipa/ipu3.mojom\n> >> @@ -25,13 +25,20 @@ struct IPU3Action {\n> >> \tlibcamera.ControlList controls;\n> >> };\n> >> \n> >> +struct IPAConfigInfo {\n> >> +\tlibcamera.CameraSensorInfo sensorInfo;\n> >> +\tmap entityControls;\n> >> +\tlibcamera.Size bdsOutputSize;\n> >> +\tlibcamera.Size iif;\n> >> +\tlibcamera.Size cropRegion;\n> >> +};\n> > Could you document this structure with doxygen comments ? We don't\n> > generate documentation from mojom files yet, but we should still be\n> > prepared.\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> > While at it, I think you can drop the => ().\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..a77afd55 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 struct 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 struct 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..be43d5a1 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -635,7 +635,12 @@ 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> >> +\tstruct ipa::ipu3::IPAConfigInfo configInfo = {\n> > You can drop the struct keyword.\n> >\n> >> +\t\tsensorInfo, entityControls, config->imguConfig().bds,\n> >> +\t\tconfig->imguConfig().iif, data->cropRegion_.size()\n> >> +\t};\n> > Given that several members of this structures have the same type, it\n> > would be safer to use named initializers.\n> >\n> > \tipa::ipu3::IPAConfigInfo configInfo = {\n> > \t\t.sensorInfo = sensorInfo,\n> > \t\t.entityControls = entityControls,\n> > \t\t.bdsOutputSize = config->imguConfig().bds,\n> > \t\t.iif = config->imguConfig().iif,\n> > \t\t.cropRegion = data->cropRegion_.size(),\n> > \t};\n> This throws a compile-time error because the generated \n> ipu3-ipa-interface.h code doesn't suppport named initializers for \n> IPAConfigInfo I think. I think we need to support in mojom first?\n> \n> FAILED: src/libcamera/4ab8042@@camera@sha/pipeline_ipu3_ipu3.cpp.o\n> ../../../../../tmp/portage/media-libs/libcamera-9999/work/libcamera-9999/src/libcamera/pipeline/ipu3/ipu3.cpp:639:27: \n> error: no matching constructor for initialization of \n> 'ipa::ipu3::IPAConfigInfo'\n>         ipa::ipu3::IPAConfigInfo configInfo = {\n>                                  ^            ~\n> include/libcamera/ipa/ipu3_ipa_interface.h:96:2: note: candidate \n> constructor not viable: cannot convert argument of incomplete type \n> 'void' to 'const libcamera::CameraSensorInfo' for 1st argument\n>         IPAConfigInfo(const CameraSensorInfo &_sensorInfo, const \n> std::map &_entityControls, const Size \n> &_bdsOutputSize, const Size &_iif)\n>         ^\n> include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate \n> constructor (the implicit copy constructor) not viable: requires 1 \n> argument, but 4 were provided\n> struct IPAConfigInfo\n>        ^\n> include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate \n> constructor (the implicit move constructor) not viable: requires 1 \n> argument, but 4 were provided\n> include/libcamera/ipa/ipu3_ipa_interface.h:92:2: note: candidate \n> constructor not viable: requires 0 arguments, but 4 were provided\n>         IPAConfigInfo()\n>         ^\n> 1 error generated.\n\nGood point, we can't use aggregate initialization when user-provided\nconstructors are present. I had overlooked that.\n\n> Meanwhile, in order to address your concerns, maybe this?\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\nUp to you, I'm fine either way.\n\n> >> +\tdata->ipa_->configure(configInfo);\n> >> \n> >> \treturn 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 BDD05BDE19\n\tfor ;\n\tTue, 27 Apr 2021 06:37:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13038688AE;\n\tTue, 27 Apr 2021 08:37: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 549A260512\n\tfor ;\n\tTue, 27 Apr 2021 08:37:30 +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 CB1A6E9;\n\tTue, 27 Apr 2021 08:37:29 +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=\"ANZY+lCl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619505450;\n\tbh=KQPhAV4fjtMS482sYiszi9WW2gJgQzkNjkUuxNGrQc4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ANZY+lClKUOm4GqeVljbJKAxowzsAwxVtTnI159D0wc4C3oHStf56WFzkhyxpMvMc\n\tPQqdpKaHr4mCrhrCRQEh8LASgdEmgFwUkG/3WAqvPP8lHXsJ6T8t72jOSrcVsB8A0D\n\tkXeWEgUeDiP/SEJGXcMXcxwaGUUk0UpcILQQb6rg=","Date":"Tue, 27 Apr 2021 09:37:24 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210426171608.145784-1-umang.jain@ideasonboard.com>\n\t\n\t<0e5b310d-6a22-e612-2223-a33e38fda202@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<0e5b310d-6a22-e612-2223-a33e38fda202@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","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":16644,"web_url":"https://patchwork.libcamera.org/comment/16644/","msgid":"<20210427075428.GH195599@pyrite.rasen.tech>","date":"2021-04-27T07:54:28","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang and Laurent,\n\nOn Tue, Apr 27, 2021 at 09:37:24AM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> On Tue, Apr 27, 2021 at 11:40:25AM +0530, Umang Jain wrote:\n> > On 4/27/21 8:27 AM, Laurent Pinchart wrote:\n> > > On Mon, Apr 26, 2021 at 10:46:08PM +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> > > The commit message should mention that you're adding more data to the\n> > > structure, and why.\n> > >\n> > >> Signed-off-by: Umang Jain \n> > >> ---\n> > >> include/libcamera/ipa/ipu3.mojom | 11 +++++++++--\n> > >> src/ipa/ipu3/ipu3.cpp | 14 ++++++--------\n> > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 ++++++-\n> > >> 3 files changed, 21 insertions(+), 11 deletions(-)\n> > >>\n> > >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > >> index a717b1e6..88cbb403 100644\n> > >> --- a/include/libcamera/ipa/ipu3.mojom\n> > >> +++ b/include/libcamera/ipa/ipu3.mojom\n> > >> @@ -25,13 +25,20 @@ struct IPU3Action {\n> > >> \tlibcamera.ControlList controls;\n> > >> };\n> > >> \n> > >> +struct IPAConfigInfo {\n> > >> +\tlibcamera.CameraSensorInfo sensorInfo;\n> > >> +\tmap entityControls;\n> > >> +\tlibcamera.Size bdsOutputSize;\n> > >> +\tlibcamera.Size iif;\n> > >> +\tlibcamera.Size cropRegion;\n> > >> +};\n> > > Could you document this structure with doxygen comments ? We don't\n> > > generate documentation from mojom files yet, but we should still be\n> > > prepared.\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> > > While at it, I think you can drop the => ().\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..a77afd55 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 struct 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 struct 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..be43d5a1 100644\n> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> @@ -635,7 +635,12 @@ 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> > >> +\tstruct ipa::ipu3::IPAConfigInfo configInfo = {\n> > > You can drop the struct keyword.\n> > >\n> > >> +\t\tsensorInfo, entityControls, config->imguConfig().bds,\n> > >> +\t\tconfig->imguConfig().iif, data->cropRegion_.size()\n> > >> +\t};\n> > > Given that several members of this structures have the same type, it\n> > > would be safer to use named initializers.\n> > >\n> > > \tipa::ipu3::IPAConfigInfo configInfo = {\n> > > \t\t.sensorInfo = sensorInfo,\n> > > \t\t.entityControls = entityControls,\n> > > \t\t.bdsOutputSize = config->imguConfig().bds,\n> > > \t\t.iif = config->imguConfig().iif,\n> > > \t\t.cropRegion = data->cropRegion_.size(),\n> > > \t};\n> > This throws a compile-time error because the generated \n> > ipu3-ipa-interface.h code doesn't suppport named initializers for \n> > IPAConfigInfo I think. I think we need to support in mojom first?\n\nI put in the user-provided constructors to support setting default\nvalues in mojom, so I don't think named initializers will be supported.\n\n\nPaul\n\n> > \n> > FAILED: src/libcamera/4ab8042@@camera@sha/pipeline_ipu3_ipu3.cpp.o\n> > ../../../../../tmp/portage/media-libs/libcamera-9999/work/libcamera-9999/src/libcamera/pipeline/ipu3/ipu3.cpp:639:27: \n> > error: no matching constructor for initialization of \n> > 'ipa::ipu3::IPAConfigInfo'\n> >         ipa::ipu3::IPAConfigInfo configInfo = {\n> >                                  ^            ~\n> > include/libcamera/ipa/ipu3_ipa_interface.h:96:2: note: candidate \n> > constructor not viable: cannot convert argument of incomplete type \n> > 'void' to 'const libcamera::CameraSensorInfo' for 1st argument\n> >         IPAConfigInfo(const CameraSensorInfo &_sensorInfo, const \n> > std::map &_entityControls, const Size \n> > &_bdsOutputSize, const Size &_iif)\n> >         ^\n> > include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate \n> > constructor (the implicit copy constructor) not viable: requires 1 \n> > argument, but 4 were provided\n> > struct IPAConfigInfo\n> >        ^\n> > include/libcamera/ipa/ipu3_ipa_interface.h:89:8: note: candidate \n> > constructor (the implicit move constructor) not viable: requires 1 \n> > argument, but 4 were provided\n> > include/libcamera/ipa/ipu3_ipa_interface.h:92:2: note: candidate \n> > constructor not viable: requires 0 arguments, but 4 were provided\n> >         IPAConfigInfo()\n> >         ^\n> > 1 error generated.\n> \n> Good point, we can't use aggregate initialization when user-provided\n> constructors are present. I had overlooked that.\n> \n> > Meanwhile, in order to address your concerns, maybe this?\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> Up to you, I'm fine either way.\n> \n> > >> +\tdata->ipa_->configure(configInfo);\n> > >> \n> > >> \treturn 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 BC3A3BDE19\n\tfor ;\n\tTue, 27 Apr 2021 07:54:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3726B688C1;\n\tTue, 27 Apr 2021 09:54:37 +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 4202E688B6\n\tfor ;\n\tTue, 27 Apr 2021 09:54: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 7CE99E9;\n\tTue, 27 Apr 2021 09:54: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=\"sjIIGgtN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619510074;\n\tbh=Z+LZJPL3uTh6ZJCDNQT0XiowwRpS+5My6S/zJcjPg08=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sjIIGgtN1foEVg3ZD7zqnVmGLDanPh2X7d3ULLUCor+m99R/frj+n41JZLpA+cjbg\n\tdMAk1MwBuK7B9L6zxS+9egvVSDhA/HqiLmZnMc6GJygQFi6UkKY+1hgM07F8EUDNn5\n\tnMLwYsZy947e+YIeQUeoVy1uIoFxQo6BupyJRSVc=","Date":"Tue, 27 Apr 2021 16:54:28 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210427075428.GH195599@pyrite.rasen.tech>","References":"<20210426171608.145784-1-umang.jain@ideasonboard.com>\n\t\n\t<0e5b310d-6a22-e612-2223-a33e38fda202@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Introduce IPAConfigInfo in\n\tIPC","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\" "}}]