[{"id":18250,"web_url":"https://patchwork.libcamera.org/comment/18250/","msgid":"<38d04552-e6ea-6aee-e521-8a7e87b9cb40@ideasonboard.com>","date":"2021-07-22T06:28:02","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn 7/16/21 8:02 PM, Jacopo Mondi wrote:\n> All the IPU3 Camera controls are currently initialized by the pipeline\n> handler which initializes them using the camera sensor configuration and\n> platform specific requirements.\n>\n> However, some controls are better initialized by the IPA, which might,\n> in example, cap the exposure times and frame duration to the constraints\n> of its algorithms implementation.\nMakes sense to me.\n>\n> Also, moving forward, the IPA should register controls to report its\n> capabilities, in example the ability to enable/disable 3A algorithms on\n> request.\n>\n> Move the existing controls initialization to the IPA, by providing\n> the sensor configuration and its controls to the IPU3IPA::init()\n> function, which initializes controls and returns them to the pipeline\n> through an output parameter.\n>\n> The existing controls initialization has been copied verbatim from the\n> pipeline handler to the IPA, if not a for few line breaks adjustments\ns/if not a for few/if not, for a few   maybe?\n> and the resulting Camera controls values are not changed .\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/ipu3.mojom | 8 ++-\n> src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------\n> 3 files changed, 116 insertions(+), 59 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 911a3a072464..eafefa8b7fe3 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -30,6 +30,11 @@ struct IPU3Action {\n> \tlibcamera.ControlList controls;\n> };\n> \n> +struct IPAInitInfo {\n> +\tlibcamera.IPACameraSensorInfo sensorInfo;\n> +\tlibcamera.ControlInfoMap sensorControls;\n> +};\n> +\n> struct IPAConfigInfo {\n> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> \tmap entityControls;\n> @@ -38,7 +43,8 @@ struct IPAConfigInfo {\n> };\n> \n> interface IPAIPU3Interface {\n> -\tinit(libcamera.IPASettings settings) => (int32 ret);\n> +\tinit(IPAInitInfo initInfo)\n> +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n\nAh, this is interesting:\n\nAt first I had a knee jerk reaction that this is wrong and the correct \nway is:\n\n\t+\tinit(IPAInitInfo initInfo, libcamera.ControlInfoMap ipaControls)\n\t+\t\t\t\t\t\t=> (int32 ret);\n\nBut then I see the code generation and saw it was on the correct lines\n\n         virtual int32_t init(\n                 const IPAInitInfo &initInfo,\n                 ControlInfoMap *ipaControls) = 0;\n\nThen, I also read ipaControls is a output parameters, so this is the way \nto let mojom know about out parameter use-cases I assume.\n\nHave you tested CTS with this change with IPA running in isolated mode? \nIt should be easier with LIBCAMERA_IPA_FORCE_ISOLATION env variable now. \nI am a bit concerned about hitting any serialize/de-serialize issue \npopping up with this change, in respect to out parameter i.e. \nipaControls. Maybe Paul can educate me on this aspect.\n\nI finally gave a thought of handling the Intel IPA with this change. I \ndon't see any major issues, more or less, we'll need to replicate this \nchange there as well (Pass controls to IPA and init them). I just want \nto make sure of the timing of merge (ofcourse after attaining R-b tags \non this series). Would it be acceptable as a  general rule that, merge \nof this series (and such series likewise in future) should be done after \n/someone/ prepares changes for Intel IPA and post them on the list as \nwell? This will help minimize the lag-time of Intel IPA catching up to \nthe libcamera master.\n\nRest looks good to me so,\n\nReviewed-by: Umang Jain \n\n\n> \tstart() => (int32 ret);\n> \tstop();\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36e50f..d3c69bc07bd0 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -5,8 +5,10 @@\n> * ipu3.cpp - IPU3 Image Processing Algorithms\n> */\n> \n> +#include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -38,7 +40,8 @@ namespace ipa::ipu3 {\n> class IPAIPU3 : public IPAIPU3Interface\n> {\n> public:\n> -\tint init(const IPASettings &settings) override;\n> +\tint init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;\n> +\n> \tint start() override;\n> \tvoid stop() override {}\n> \n> @@ -86,14 +89,74 @@ private:\n> \tstruct ipu3_uapi_grid_config bdsGrid_;\n> };\n> \n> -int IPAIPU3::init(const IPASettings &settings)\n> +/**\n> + * Initialize the IPA module and its controls.\n> + *\n> + * This function receives the camera sensor information from the pipeline\n> + * handler, computes the limits of the controls it handles and returns\n> + * them in the \\a ipaControls output parameter.\n> + */\n> +int IPAIPU3::init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls)\n> {\n> -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> +\tconst IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;\n> +\n> +\t/* Initialize the camera sensor helper. */\n> +\tcamHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);\n> \tif (camHelper_ == nullptr) {\n> -\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \" << settings.sensorModel;\n> +\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \"\n> +\t\t\t\t << sensorInfo.model;\n> \t\treturn -ENODEV;\n> \t}\n> \n> +\t/* Initialize Controls. */\n> +\tconst ControlInfoMap &sensorControls = initInfo.sensorControls;\n> +\tControlInfoMap::Map controls{};\n> +\n> +\t/*\n> +\t * Compute exposure time limits.\n> +\t *\n> +\t * Initialize the control using the line length and pixel rate of the\n> +\t * current configuration converted to microseconds. Use the\n> +\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> +\t * convert it from lines to microseconds.\n> +\t */\n> +\tdouble lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> +\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> +\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> +\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> +\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> +\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> +\t\t\t\t\t\t\tdefExposure);\n> +\n> +\t/*\n> +\t * Compute the frame duration limits.\n> +\t *\n> +\t * The frame length is computed assuming a fixed line length combined\n> +\t * with the vertical frame sizes.\n> +\t */\n> +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> +\tuint32_t hblank = v4l2HBlank.def().get();\n> +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> +\n> +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> +\tstd::array frameHeights{\n> +\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> +\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> +\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> +\t};\n> +\n> +\tstd::array frameDurations;\n> +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> +\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> +\t}\n> +\n> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> +\t\t\t\t\t\t\t frameDurations[1],\n> +\t\t\t\t\t\t\t frameDurations[2]);\n> +\n> +\t*ipaControls = std::move(controls);\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 76c3bb3d8aa9..22df9c3650af 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -88,6 +88,8 @@ public:\n> \n> \tstd::queue pendingRequests_;\n> \n> +\tControlInfoMap ipaControls_;\n> +\n> private:\n> \tvoid queueFrameAction(unsigned int id,\n> \t\t\t const ipa::ipu3::IPU3Action &action);\n> @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t\treturn ret;\n> \n> \tControlInfoMap::Map controls = IPU3Controls;\n> -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> \tconst std::vector &testPatternModes = sensor->testPatternModes();\n> \tif (!testPatternModes.empty()) {\n> \t\tstd::vector values;\n> @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> \t}\n> \n> -\t/*\n> -\t * Compute exposure time limits.\n> -\t *\n> -\t * Initialize the control using the line length and pixel rate of the\n> -\t * current configuration converted to microseconds. Use the\n> -\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> -\t * convert it from lines to microseconds.\n> -\t */\n> -\tdouble lineDuration = sensorInfo.lineLength\n> -\t\t\t / (sensorInfo.pixelRate / 1e6);\n> -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> -\n> -\t/*\n> -\t * \\todo Report the actual exposure time, use the default for the\n> -\t * moment.\n> -\t */\n> -\tdata->exposureTime_ = defExposure;\n> -\n> -\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> -\t\t\t\t\t\t\tdefExposure);\n> -\n> -\t/*\n> -\t * Compute the frame duration limits.\n> -\t *\n> -\t * The frame length is computed assuming a fixed line length combined\n> -\t * with the vertical frame sizes.\n> -\t */\n> -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> -\tuint32_t hblank = v4l2HBlank.def().get();\n> -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> -\n> -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> -\tstd::array frameHeights{\n> -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> -\t};\n> -\n> -\tstd::array frameDurations;\n> -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> -\t}\n> -\n> -\tcontrols[&controls::FrameDurationLimits] =\n> -\t\tControlInfo(frameDurations[0],\n> -\t\t\t frameDurations[1],\n> -\t\t\t frameDurations[2]);\n> -\n> \t/*\n> \t * Compute the scaler crop limits.\n> \t *\n> @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \n> \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> \n> +\t/* Add the IPA registered controls to list of camera controls. */\n> +\tfor (const auto &ipaControl : data->ipaControls_)\n> +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> +\n> \tdata->controlInfo_ = std::move(controls);\n> \n> \treturn 0;\n> @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()\n> \n> \tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> \n> +\t/*\n> +\t * Pass the sensor info the the IPA to initialize controls.\n> +\t *\n> +\t * \\todo The limits of the registered controls depend on the current\n> +\t * sensor configuration. Initialize the sensor using its resolution as\n> +\t * its initial configuration and use it to compute the controls limits\n> +\t * in the IPA.\n> +\t */\n> \tCameraSensor *sensor = cio2_.sensor();\n> -\tint ret = ipa_->init(IPASettings{ \"\", sensor->model() });\n> +\tV4L2SubdeviceFormat sensorFormat = {};\n> +\tsensorFormat.size = sensor->resolution();\n> +\tint ret = sensor->setFormat(&sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tIPACameraSensorInfo sensorInfo{};\n> +\tret = sensor->sensorInfo(&sensorInfo);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tipa::ipu3::IPAInitInfo initInfo{\n> +\t\tsensorInfo,\n> +\t\tsensor->controls(),\n> +\t};\n> +\tipaControls_ = {};\n> +\tret = ipa_->init(initInfo, &ipaControls_);\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Failed to initialise the IPU3 IPA\";\n> \t\treturn ret;\n> \t}\n> \n> +\t/*\n> +\t * \\todo Report the actual exposure time, use the default for the\n> +\t * moment.\n> +\t */\n> +\tconst auto exposureInfo = ipaControls_.find(&controls::ExposureTime);\n> +\tif (exposureInfo == ipaControls_.end()) {\n> +\t\tLOG(IPU3, Error) << \"Exposure control not initializaed by the IPA\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\texposureTime_ = exposureInfo->second.def().get();\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 65FD0C322B\n\tfor ;\n\tThu, 22 Jul 2021 06:28:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EAEB68545;\n\tThu, 22 Jul 2021 08:28:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0543460275\n\tfor ;\n\tThu, 22 Jul 2021 08:28:08 +0200 (CEST)","from [192.168.0.107] (unknown [103.238.109.21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD6FD465;\n\tThu, 22 Jul 2021 08:28:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AKlR6iQA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626935287;\n\tbh=ZBWFgq8srLLR3MgcLirkhSEFi0TdCowMo/7YNh5SrEA=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=AKlR6iQAryUCC1KuBwfDmbnJXXYqOjpykKWdMUuzK0/9lcCBxd6UiTVEh9yU3d+6P\n\t45PpAiNpA+WLZNNxPf4kwiqebVz2c2cL/wHUqB14sEbYfChUE0T19UtKTfxmOA0208\n\tlCBGSKGbQDPs7zceoBspZvTTkBALQqGADgUfN2qE=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210716143215.67454-1-jacopo@jmondi.org>\n\t<20210716143215.67454-2-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"<38d04552-e6ea-6aee-e521-8a7e87b9cb40@ideasonboard.com>","Date":"Thu, 22 Jul 2021 11:58:02 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210716143215.67454-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18271,"web_url":"https://patchwork.libcamera.org/comment/18271/","msgid":"<20210722150109.bncnczoqfn7nzr7m@uno.localdomain>","date":"2021-07-22T15:01:09","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Thu, Jul 22, 2021 at 11:58:02AM +0530, Umang Jain wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On 7/16/21 8:02 PM, Jacopo Mondi wrote:\n> > All the IPU3 Camera controls are currently initialized by the pipeline\n> > handler which initializes them using the camera sensor configuration and\n> > platform specific requirements.\n> >\n> > However, some controls are better initialized by the IPA, which might,\n> > in example, cap the exposure times and frame duration to the constraints\n> > of its algorithms implementation.\n> Makes sense to me.\n> >\n> > Also, moving forward, the IPA should register controls to report its\n> > capabilities, in example the ability to enable/disable 3A algorithms on\n> > request.\n> >\n> > Move the existing controls initialization to the IPA, by providing\n> > the sensor configuration and its controls to the IPU3IPA::init()\n> > function, which initializes controls and returns them to the pipeline\n> > through an output parameter.\n> >\n> > The existing controls initialization has been copied verbatim from the\n> > pipeline handler to the IPA, if not a for few line breaks adjustments\n> s/if not a for few/if not, for a few   maybe?\n\nSo:\npipeline handler to the IPA, if not, for a few line ?\n\nNot a native speaker but the double comma doesn't really feel right\nhere.\n\n> > and the resulting Camera controls values are not changed .\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 8 ++-\n> > src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------\n> > 3 files changed, 116 insertions(+), 59 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > index 911a3a072464..eafefa8b7fe3 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -30,6 +30,11 @@ struct IPU3Action {\n> > \tlibcamera.ControlList controls;\n> > };\n> > +struct IPAInitInfo {\n> > +\tlibcamera.IPACameraSensorInfo sensorInfo;\n> > +\tlibcamera.ControlInfoMap sensorControls;\n> > +};\n> > +\n> > struct IPAConfigInfo {\n> > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > \tmap entityControls;\n> > @@ -38,7 +43,8 @@ struct IPAConfigInfo {\n> > };\n> > interface IPAIPU3Interface {\n> > -\tinit(libcamera.IPASettings settings) => (int32 ret);\n> > +\tinit(IPAInitInfo initInfo)\n> > +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n>\n> Ah, this is interesting:\n>\n> At first I had a knee jerk reaction that this is wrong and the correct way\n> is:\n>\n> \t+\tinit(IPAInitInfo initInfo, libcamera.ControlInfoMap ipaControls)\n> \t+\t\t\t\t\t\t=> (int32 ret);\n>\n> But then I see the code generation and saw it was on the correct lines\n>\n>         virtual int32_t init(\n>                 const IPAInitInfo &initInfo,\n>                 ControlInfoMap *ipaControls) = 0;\n>\n> Then, I also read ipaControls is a output parameters, so this is the way to\n> let mojom know about out parameter use-cases I assume.\n\nYes, took me a while to figure it out\n\n>\n> Have you tested CTS with this change with IPA running in isolated mode? It\n\nGood catch!!\n\nIt's enough to run\n\n# LIBCAMERA_IPA_FORCE_ISOLATION=1 cam -c1 --list-controls\n[0:10:59.880617099] [11495] ERROR IPU3 ipu3.cpp:1201 Exposure control not initializaed by the IPA\nCamera 1 not found\n\nSo we cannot use output parameters when running isolated IPA ?\nThat's quite a showstopper :(\n\n\n> should be easier with LIBCAMERA_IPA_FORCE_ISOLATION env variable now. I am a\n> bit concerned about hitting any serialize/de-serialize issue popping up with\n> this change, in respect to out parameter i.e. ipaControls. Maybe Paul can\n> educate me on this aspect.\n\nAnd me as well :)\n\n>\n> I finally gave a thought of handling the Intel IPA with this change. I don't\n> see any major issues, more or less, we'll need to replicate this change\n> there as well (Pass controls to IPA and init them). I just want to make sure\n> of the timing of merge (ofcourse after attaining R-b tags on this series).\n> Would it be acceptable as a  general rule that, merge of this series (and\n> such series likewise in future) should be done after /someone/ prepares\n> changes for Intel IPA and post them on the list as well? This will help\n> minimize the lag-time of Intel IPA catching up to the libcamera master.\n>\n> Rest looks good to me so,\n>\n> Reviewed-by: Umang Jain \n\nThanks, let's see about the output parameter thing.\n>\n>\n> > \tstart() => (int32 ret);\n> > \tstop();\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 71698d36e50f..d3c69bc07bd0 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -5,8 +5,10 @@\n> > * ipu3.cpp - IPU3 Image Processing Algorithms\n> > */\n> > +#include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > @@ -38,7 +40,8 @@ namespace ipa::ipu3 {\n> > class IPAIPU3 : public IPAIPU3Interface\n> > {\n> > public:\n> > -\tint init(const IPASettings &settings) override;\n> > +\tint init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;\n> > +\n> > \tint start() override;\n> > \tvoid stop() override {}\n> > @@ -86,14 +89,74 @@ private:\n> > \tstruct ipu3_uapi_grid_config bdsGrid_;\n> > };\n> > -int IPAIPU3::init(const IPASettings &settings)\n> > +/**\n> > + * Initialize the IPA module and its controls.\n> > + *\n> > + * This function receives the camera sensor information from the pipeline\n> > + * handler, computes the limits of the controls it handles and returns\n> > + * them in the \\a ipaControls output parameter.\n> > + */\n> > +int IPAIPU3::init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls)\n> > {\n> > -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > +\tconst IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;\n> > +\n> > +\t/* Initialize the camera sensor helper. */\n> > +\tcamHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);\n> > \tif (camHelper_ == nullptr) {\n> > -\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \" << settings.sensorModel;\n> > +\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \"\n> > +\t\t\t\t << sensorInfo.model;\n> > \t\treturn -ENODEV;\n> > \t}\n> > +\t/* Initialize Controls. */\n> > +\tconst ControlInfoMap &sensorControls = initInfo.sensorControls;\n> > +\tControlInfoMap::Map controls{};\n> > +\n> > +\t/*\n> > +\t * Compute exposure time limits.\n> > +\t *\n> > +\t * Initialize the control using the line length and pixel rate of the\n> > +\t * current configuration converted to microseconds. Use the\n> > +\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > +\t * convert it from lines to microseconds.\n> > +\t */\n> > +\tdouble lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> > +\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > +\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > +\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > +\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > +\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > +\t\t\t\t\t\t\tdefExposure);\n> > +\n> > +\t/*\n> > +\t * Compute the frame duration limits.\n> > +\t *\n> > +\t * The frame length is computed assuming a fixed line length combined\n> > +\t * with the vertical frame sizes.\n> > +\t */\n> > +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > +\tuint32_t hblank = v4l2HBlank.def().get();\n> > +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > +\n> > +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > +\tstd::array frameHeights{\n> > +\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > +\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > +\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > +\t};\n> > +\n> > +\tstd::array frameDurations;\n> > +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > +\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > +\t}\n> > +\n> > +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> > +\t\t\t\t\t\t\t frameDurations[1],\n> > +\t\t\t\t\t\t\t frameDurations[2]);\n> > +\n> > +\t*ipaControls = std::move(controls);\n> > +\n> > \treturn 0;\n> > }\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 76c3bb3d8aa9..22df9c3650af 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -88,6 +88,8 @@ public:\n> > \tstd::queue pendingRequests_;\n> > +\tControlInfoMap ipaControls_;\n> > +\n> > private:\n> > \tvoid queueFrameAction(unsigned int id,\n> > \t\t\t const ipa::ipu3::IPU3Action &action);\n> > @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t\treturn ret;\n> > \tControlInfoMap::Map controls = IPU3Controls;\n> > -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> > \tconst std::vector &testPatternModes = sensor->testPatternModes();\n> > \tif (!testPatternModes.empty()) {\n> > \t\tstd::vector values;\n> > @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > \t}\n> > -\t/*\n> > -\t * Compute exposure time limits.\n> > -\t *\n> > -\t * Initialize the control using the line length and pixel rate of the\n> > -\t * current configuration converted to microseconds. Use the\n> > -\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > -\t * convert it from lines to microseconds.\n> > -\t */\n> > -\tdouble lineDuration = sensorInfo.lineLength\n> > -\t\t\t / (sensorInfo.pixelRate / 1e6);\n> > -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > -\n> > -\t/*\n> > -\t * \\todo Report the actual exposure time, use the default for the\n> > -\t * moment.\n> > -\t */\n> > -\tdata->exposureTime_ = defExposure;\n> > -\n> > -\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > -\t\t\t\t\t\t\tdefExposure);\n> > -\n> > -\t/*\n> > -\t * Compute the frame duration limits.\n> > -\t *\n> > -\t * The frame length is computed assuming a fixed line length combined\n> > -\t * with the vertical frame sizes.\n> > -\t */\n> > -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > -\tuint32_t hblank = v4l2HBlank.def().get();\n> > -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > -\n> > -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > -\tstd::array frameHeights{\n> > -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > -\t};\n> > -\n> > -\tstd::array frameDurations;\n> > -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > -\t}\n> > -\n> > -\tcontrols[&controls::FrameDurationLimits] =\n> > -\t\tControlInfo(frameDurations[0],\n> > -\t\t\t frameDurations[1],\n> > -\t\t\t frameDurations[2]);\n> > -\n> > \t/*\n> > \t * Compute the scaler crop limits.\n> > \t *\n> > @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > +\t/* Add the IPA registered controls to list of camera controls. */\n> > +\tfor (const auto &ipaControl : data->ipaControls_)\n> > +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> > +\n> > \tdata->controlInfo_ = std::move(controls);\n> > \treturn 0;\n> > @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()\n> > \tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> > +\t/*\n> > +\t * Pass the sensor info the the IPA to initialize controls.\n> > +\t *\n> > +\t * \\todo The limits of the registered controls depend on the current\n> > +\t * sensor configuration. Initialize the sensor using its resolution as\n> > +\t * its initial configuration and use it to compute the controls limits\n> > +\t * in the IPA.\n> > +\t */\n> > \tCameraSensor *sensor = cio2_.sensor();\n> > -\tint ret = ipa_->init(IPASettings{ \"\", sensor->model() });\n> > +\tV4L2SubdeviceFormat sensorFormat = {};\n> > +\tsensorFormat.size = sensor->resolution();\n> > +\tint ret = sensor->setFormat(&sensorFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tIPACameraSensorInfo sensorInfo{};\n> > +\tret = sensor->sensorInfo(&sensorInfo);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tipa::ipu3::IPAInitInfo initInfo{\n> > +\t\tsensorInfo,\n> > +\t\tsensor->controls(),\n> > +\t};\n> > +\tipaControls_ = {};\n> > +\tret = ipa_->init(initInfo, &ipaControls_);\n> > \tif (ret) {\n> > \t\tLOG(IPU3, Error) << \"Failed to initialise the IPU3 IPA\";\n> > \t\treturn ret;\n> > \t}\n> > +\t/*\n> > +\t * \\todo Report the actual exposure time, use the default for the\n> > +\t * moment.\n> > +\t */\n> > +\tconst auto exposureInfo = ipaControls_.find(&controls::ExposureTime);\n> > +\tif (exposureInfo == ipaControls_.end()) {\n> > +\t\tLOG(IPU3, Error) << \"Exposure control not initializaed by the IPA\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\texposureTime_ = exposureInfo->second.def().get();\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 65C2DC322B\n\tfor ;\n\tThu, 22 Jul 2021 15:00:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5E56687A3;\n\tThu, 22 Jul 2021 17:00:23 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A5EC468779\n\tfor ;\n\tThu, 22 Jul 2021 17:00:22 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 26866100006;\n\tThu, 22 Jul 2021 15:00:21 +0000 (UTC)"],"Date":"Thu, 22 Jul 2021 17:01:09 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210722150109.bncnczoqfn7nzr7m@uno.localdomain>","References":"<20210716143215.67454-1-jacopo@jmondi.org>\n\t<20210716143215.67454-2-jacopo@jmondi.org>\n\t<38d04552-e6ea-6aee-e521-8a7e87b9cb40@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<38d04552-e6ea-6aee-e521-8a7e87b9cb40@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18273,"web_url":"https://patchwork.libcamera.org/comment/18273/","msgid":"<20210723042711.GA59446@pyrite.rasen.tech>","date":"2021-07-23T04:27:11","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo and Umang,\n\nOn Thu, Jul 22, 2021 at 05:01:09PM +0200, Jacopo Mondi wrote:\n> Hi Umang,\n> \n> On Thu, Jul 22, 2021 at 11:58:02AM +0530, Umang Jain wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On 7/16/21 8:02 PM, Jacopo Mondi wrote:\n> > > All the IPU3 Camera controls are currently initialized by the pipeline\n> > > handler which initializes them using the camera sensor configuration and\n> > > platform specific requirements.\n> > >\n> > > However, some controls are better initialized by the IPA, which might,\n> > > in example, cap the exposure times and frame duration to the constraints\n> > > of its algorithms implementation.\n> > Makes sense to me.\n> > >\n> > > Also, moving forward, the IPA should register controls to report its\n> > > capabilities, in example the ability to enable/disable 3A algorithms on\n> > > request.\n> > >\n> > > Move the existing controls initialization to the IPA, by providing\n> > > the sensor configuration and its controls to the IPU3IPA::init()\n> > > function, which initializes controls and returns them to the pipeline\n> > > through an output parameter.\n\nI was actually going to say that this means that all IPA implementations\nmust also implement this API, but I guess that is indeed the case.\n\n> > >\n> > > The existing controls initialization has been copied verbatim from the\n> > > pipeline handler to the IPA, if not a for few line breaks adjustments\n> > s/if not a for few/if not, for a few   maybe?\n> \n> So:\n> pipeline handler to the IPA, if not, for a few line ?\n> \n> Not a native speaker but the double comma doesn't really feel right\n> here.\n\nI think no extra comma should be fine.\n\n> \n> > > and the resulting Camera controls values are not changed .\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 8 ++-\n> > > src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------\n> > > 3 files changed, 116 insertions(+), 59 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > index 911a3a072464..eafefa8b7fe3 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -30,6 +30,11 @@ struct IPU3Action {\n> > > \tlibcamera.ControlList controls;\n> > > };\n> > > +struct IPAInitInfo {\n> > > +\tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > +\tlibcamera.ControlInfoMap sensorControls;\n> > > +};\n> > > +\n> > > struct IPAConfigInfo {\n> > > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > \tmap entityControls;\n> > > @@ -38,7 +43,8 @@ struct IPAConfigInfo {\n> > > };\n> > > interface IPAIPU3Interface {\n> > > -\tinit(libcamera.IPASettings settings) => (int32 ret);\n> > > +\tinit(IPAInitInfo initInfo)\n> > > +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> >\n> > Ah, this is interesting:\n> >\n> > At first I had a knee jerk reaction that this is wrong and the correct way\n> > is:\n> >\n> > \t+\tinit(IPAInitInfo initInfo, libcamera.ControlInfoMap ipaControls)\n> > \t+\t\t\t\t\t\t=> (int32 ret);\n> >\n> > But then I see the code generation and saw it was on the correct lines\n> >\n> >         virtual int32_t init(\n> >                 const IPAInitInfo &initInfo,\n> >                 ControlInfoMap *ipaControls) = 0;\n> >\n> > Then, I also read ipaControls is a output parameters, so this is the way to\n> > let mojom know about out parameter use-cases I assume.\n> \n> Yes, took me a while to figure it out\n\nIt's magical isn't it :D\n\n> \n> >\n> > Have you tested CTS with this change with IPA running in isolated mode? It\n> \n> Good catch!!\n> \n> It's enough to run\n> \n> # LIBCAMERA_IPA_FORCE_ISOLATION=1 cam -c1 --list-controls\n> [0:10:59.880617099] [11495] ERROR IPU3 ipu3.cpp:1201 Exposure control not initializaed by the IPA\n> Camera 1 not found\n> \n> So we cannot use output parameters when running isolated IPA ?\n> That's quite a showstopper :(\n\nThat's not supposed to happen :/\n\nI'll have to fix that :(\n\n> \n> \n> > should be easier with LIBCAMERA_IPA_FORCE_ISOLATION env variable now. I am a\n> > bit concerned about hitting any serialize/de-serialize issue popping up with\n> > this change, in respect to out parameter i.e. ipaControls. Maybe Paul can\n> > educate me on this aspect.\n> \n> And me as well :)\n\nIt's supposed to work as expected...\n\n\nPaul\n\n> \n> >\n> > I finally gave a thought of handling the Intel IPA with this change. I don't\n> > see any major issues, more or less, we'll need to replicate this change\n> > there as well (Pass controls to IPA and init them). I just want to make sure\n> > of the timing of merge (ofcourse after attaining R-b tags on this series).\n> > Would it be acceptable as a  general rule that, merge of this series (and\n> > such series likewise in future) should be done after /someone/ prepares\n> > changes for Intel IPA and post them on the list as well? This will help\n> > minimize the lag-time of Intel IPA catching up to the libcamera master.\n> >\n> > Rest looks good to me so,\n> >\n> > Reviewed-by: Umang Jain \n> \n> Thanks, let's see about the output parameter thing.\n> >\n> >\n> > > \tstart() => (int32 ret);\n> > > \tstop();\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 71698d36e50f..d3c69bc07bd0 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -5,8 +5,10 @@\n> > > * ipu3.cpp - IPU3 Image Processing Algorithms\n> > > */\n> > > +#include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > @@ -38,7 +40,8 @@ namespace ipa::ipu3 {\n> > > class IPAIPU3 : public IPAIPU3Interface\n> > > {\n> > > public:\n> > > -\tint init(const IPASettings &settings) override;\n> > > +\tint init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;\n> > > +\n> > > \tint start() override;\n> > > \tvoid stop() override {}\n> > > @@ -86,14 +89,74 @@ private:\n> > > \tstruct ipu3_uapi_grid_config bdsGrid_;\n> > > };\n> > > -int IPAIPU3::init(const IPASettings &settings)\n> > > +/**\n> > > + * Initialize the IPA module and its controls.\n> > > + *\n> > > + * This function receives the camera sensor information from the pipeline\n> > > + * handler, computes the limits of the controls it handles and returns\n> > > + * them in the \\a ipaControls output parameter.\n> > > + */\n> > > +int IPAIPU3::init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls)\n> > > {\n> > > -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > > +\tconst IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;\n> > > +\n> > > +\t/* Initialize the camera sensor helper. */\n> > > +\tcamHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);\n> > > \tif (camHelper_ == nullptr) {\n> > > -\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \" << settings.sensorModel;\n> > > +\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \"\n> > > +\t\t\t\t << sensorInfo.model;\n> > > \t\treturn -ENODEV;\n> > > \t}\n> > > +\t/* Initialize Controls. */\n> > > +\tconst ControlInfoMap &sensorControls = initInfo.sensorControls;\n> > > +\tControlInfoMap::Map controls{};\n> > > +\n> > > +\t/*\n> > > +\t * Compute exposure time limits.\n> > > +\t *\n> > > +\t * Initialize the control using the line length and pixel rate of the\n> > > +\t * current configuration converted to microseconds. Use the\n> > > +\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > > +\t * convert it from lines to microseconds.\n> > > +\t */\n> > > +\tdouble lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> > > +\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > > +\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > > +\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > > +\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > > +\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > +\t\t\t\t\t\t\tdefExposure);\n> > > +\n> > > +\t/*\n> > > +\t * Compute the frame duration limits.\n> > > +\t *\n> > > +\t * The frame length is computed assuming a fixed line length combined\n> > > +\t * with the vertical frame sizes.\n> > > +\t */\n> > > +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > > +\tuint32_t hblank = v4l2HBlank.def().get();\n> > > +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > > +\n> > > +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > > +\tstd::array frameHeights{\n> > > +\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > > +\t};\n> > > +\n> > > +\tstd::array frameDurations;\n> > > +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > > +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > > +\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > > +\t}\n> > > +\n> > > +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> > > +\t\t\t\t\t\t\t frameDurations[1],\n> > > +\t\t\t\t\t\t\t frameDurations[2]);\n> > > +\n> > > +\t*ipaControls = std::move(controls);\n> > > +\n> > > \treturn 0;\n> > > }\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 76c3bb3d8aa9..22df9c3650af 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -88,6 +88,8 @@ public:\n> > > \tstd::queue pendingRequests_;\n> > > +\tControlInfoMap ipaControls_;\n> > > +\n> > > private:\n> > > \tvoid queueFrameAction(unsigned int id,\n> > > \t\t\t const ipa::ipu3::IPU3Action &action);\n> > > @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t\treturn ret;\n> > > \tControlInfoMap::Map controls = IPU3Controls;\n> > > -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> > > \tconst std::vector &testPatternModes = sensor->testPatternModes();\n> > > \tif (!testPatternModes.empty()) {\n> > > \t\tstd::vector values;\n> > > @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > > \t}\n> > > -\t/*\n> > > -\t * Compute exposure time limits.\n> > > -\t *\n> > > -\t * Initialize the control using the line length and pixel rate of the\n> > > -\t * current configuration converted to microseconds. Use the\n> > > -\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > > -\t * convert it from lines to microseconds.\n> > > -\t */\n> > > -\tdouble lineDuration = sensorInfo.lineLength\n> > > -\t\t\t / (sensorInfo.pixelRate / 1e6);\n> > > -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > > -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > > -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > > -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > > -\n> > > -\t/*\n> > > -\t * \\todo Report the actual exposure time, use the default for the\n> > > -\t * moment.\n> > > -\t */\n> > > -\tdata->exposureTime_ = defExposure;\n> > > -\n> > > -\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > -\t\t\t\t\t\t\tdefExposure);\n> > > -\n> > > -\t/*\n> > > -\t * Compute the frame duration limits.\n> > > -\t *\n> > > -\t * The frame length is computed assuming a fixed line length combined\n> > > -\t * with the vertical frame sizes.\n> > > -\t */\n> > > -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > > -\tuint32_t hblank = v4l2HBlank.def().get();\n> > > -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > > -\n> > > -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > > -\tstd::array frameHeights{\n> > > -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > > -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > > -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > > -\t};\n> > > -\n> > > -\tstd::array frameDurations;\n> > > -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > > -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > > -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > > -\t}\n> > > -\n> > > -\tcontrols[&controls::FrameDurationLimits] =\n> > > -\t\tControlInfo(frameDurations[0],\n> > > -\t\t\t frameDurations[1],\n> > > -\t\t\t frameDurations[2]);\n> > > -\n> > > \t/*\n> > > \t * Compute the scaler crop limits.\n> > > \t *\n> > > @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > > +\t/* Add the IPA registered controls to list of camera controls. */\n> > > +\tfor (const auto &ipaControl : data->ipaControls_)\n> > > +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> > > +\n> > > \tdata->controlInfo_ = std::move(controls);\n> > > \treturn 0;\n> > > @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()\n> > > \tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> > > +\t/*\n> > > +\t * Pass the sensor info the the IPA to initialize controls.\n> > > +\t *\n> > > +\t * \\todo The limits of the registered controls depend on the current\n> > > +\t * sensor configuration. Initialize the sensor using its resolution as\n> > > +\t * its initial configuration and use it to compute the controls limits\n> > > +\t * in the IPA.\n> > > +\t */\n> > > \tCameraSensor *sensor = cio2_.sensor();\n> > > -\tint ret = ipa_->init(IPASettings{ \"\", sensor->model() });\n> > > +\tV4L2SubdeviceFormat sensorFormat = {};\n> > > +\tsensorFormat.size = sensor->resolution();\n> > > +\tint ret = sensor->setFormat(&sensorFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tIPACameraSensorInfo sensorInfo{};\n> > > +\tret = sensor->sensorInfo(&sensorInfo);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tipa::ipu3::IPAInitInfo initInfo{\n> > > +\t\tsensorInfo,\n> > > +\t\tsensor->controls(),\n> > > +\t};\n> > > +\tipaControls_ = {};\n> > > +\tret = ipa_->init(initInfo, &ipaControls_);\n> > > \tif (ret) {\n> > > \t\tLOG(IPU3, Error) << \"Failed to initialise the IPU3 IPA\";\n> > > \t\treturn ret;\n> > > \t}\n> > > +\t/*\n> > > +\t * \\todo Report the actual exposure time, use the default for the\n> > > +\t * moment.\n> > > +\t */\n> > > +\tconst auto exposureInfo = ipaControls_.find(&controls::ExposureTime);\n> > > +\tif (exposureInfo == ipaControls_.end()) {\n> > > +\t\tLOG(IPU3, Error) << \"Exposure control not initializaed by the IPA\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\texposureTime_ = exposureInfo->second.def().get();\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 2DE2CC322B\n\tfor ;\n\tFri, 23 Jul 2021 04:27:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8975D687B2;\n\tFri, 23 Jul 2021 06:27:21 +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 4EF6B687A6\n\tfor ;\n\tFri, 23 Jul 2021 06:27:19 +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 69AC6255;\n\tFri, 23 Jul 2021 06:27:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qTMZY81d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627014438;\n\tbh=AvL4XxCeJrT8QyjoYE9maFviLdx+hzAr+LZvhy1ES2I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qTMZY81duzCT9uE/EQXtCwh9W9TWUSbcJIxl9N3ocWA0QcVail+uHIbOE8oycUAgl\n\tLNm9CPyH2Kc+TxsyeSyXoKpAu8W0Fcwv0VXb8NQfSN30veiNGnUjUzRyc4Dlkmlt1h\n\tedScaNhRrbf0Yg7u9LHJt6gT4mmIFafsiWNDmveU=","Date":"Fri, 23 Jul 2021 13:27:11 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210723042711.GA59446@pyrite.rasen.tech>","References":"<20210716143215.67454-1-jacopo@jmondi.org>\n\t<20210716143215.67454-2-jacopo@jmondi.org>\n\t<38d04552-e6ea-6aee-e521-8a7e87b9cb40@ideasonboard.com>\n\t<20210722150109.bncnczoqfn7nzr7m@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210722150109.bncnczoqfn7nzr7m@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18332,"web_url":"https://patchwork.libcamera.org/comment/18332/","msgid":"","date":"2021-07-25T01:35:03","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Jul 22, 2021 at 05:01:09PM +0200, Jacopo Mondi wrote:\n> On Thu, Jul 22, 2021 at 11:58:02AM +0530, Umang Jain wrote:\n> > On 7/16/21 8:02 PM, Jacopo Mondi wrote:\n> > > All the IPU3 Camera controls are currently initialized by the pipeline\n> > > handler which initializes them using the camera sensor configuration and\n> > > platform specific requirements.\n> > >\n> > > However, some controls are better initialized by the IPA, which might,\n> > > in example, cap the exposure times and frame duration to the constraints\n> > > of its algorithms implementation.\n> >\n> > Makes sense to me.\n> >\n> > > Also, moving forward, the IPA should register controls to report its\n> > > capabilities, in example the ability to enable/disable 3A algorithms on\n> > > request.\n> > >\n> > > Move the existing controls initialization to the IPA, by providing\n> > > the sensor configuration and its controls to the IPU3IPA::init()\n> > > function, which initializes controls and returns them to the pipeline\n> > > through an output parameter.\n> > >\n> > > The existing controls initialization has been copied verbatim from the\n> > > pipeline handler to the IPA, if not a for few line breaks adjustments\n> >\n> > s/if not a for few/if not, for a few   maybe?\n> \n> So:\n> pipeline handler to the IPA, if not, for a few line ?\n> \n> Not a native speaker but the double comma doesn't really feel right\n> here.\n\nIf the intent was to say that the only difference is a few line breaks,\nno extra comma is needed after \"not\". A comma (or even a full stop)\nwould however be useful after \"adjustments\".\n\n> > > and the resulting Camera controls values are not changed .\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 8 ++-\n> > > src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------\n> > > 3 files changed, 116 insertions(+), 59 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > index 911a3a072464..eafefa8b7fe3 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -30,6 +30,11 @@ struct IPU3Action {\n> > > \tlibcamera.ControlList controls;\n> > > };\n> > > +struct IPAInitInfo {\n> > > +\tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > +\tlibcamera.ControlInfoMap sensorControls;\n> > > +};\n> > > +\n> > > struct IPAConfigInfo {\n> > > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > \tmap entityControls;\n> > > @@ -38,7 +43,8 @@ struct IPAConfigInfo {\n> > > };\n> > > interface IPAIPU3Interface {\n> > > -\tinit(libcamera.IPASettings settings) => (int32 ret);\n> > > +\tinit(IPAInitInfo initInfo)\n> > > +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> >\n> > Ah, this is interesting:\n> >\n> > At first I had a knee jerk reaction that this is wrong and the correct way\n> > is:\n> >\n> > \t+\tinit(IPAInitInfo initInfo, libcamera.ControlInfoMap ipaControls)\n> > \t+\t\t\t\t\t\t=> (int32 ret);\n> >\n> > But then I see the code generation and saw it was on the correct lines\n> >\n> >         virtual int32_t init(\n> >                 const IPAInitInfo &initInfo,\n> >                 ControlInfoMap *ipaControls) = 0;\n> >\n> > Then, I also read ipaControls is a output parameters, so this is the way to\n> > let mojom know about out parameter use-cases I assume.\n> \n> Yes, took me a while to figure it out\n> \n> >\n> > Have you tested CTS with this change with IPA running in isolated mode? It\n> \n> Good catch!!\n> \n> It's enough to run\n> \n> # LIBCAMERA_IPA_FORCE_ISOLATION=1 cam -c1 --list-controls\n> [0:10:59.880617099] [11495] ERROR IPU3 ipu3.cpp:1201 Exposure control not initializaed by the IPA\n> Camera 1 not found\n> \n> So we cannot use output parameters when running isolated IPA ?\n> That's quite a showstopper :(\n> \n> \n> > should be easier with LIBCAMERA_IPA_FORCE_ISOLATION env variable now. I am a\n> > bit concerned about hitting any serialize/de-serialize issue popping up with\n> > this change, in respect to out parameter i.e. ipaControls. Maybe Paul can\n> > educate me on this aspect.\n> \n> And me as well :)\n> \n> >\n> > I finally gave a thought of handling the Intel IPA with this change. I don't\n> > see any major issues, more or less, we'll need to replicate this change\n> > there as well (Pass controls to IPA and init them). I just want to make sure\n> > of the timing of merge (ofcourse after attaining R-b tags on this series).\n> > Would it be acceptable as a  general rule that, merge of this series (and\n> > such series likewise in future) should be done after /someone/ prepares\n> > changes for Intel IPA and post them on the list as well? This will help\n> > minimize the lag-time of Intel IPA catching up to the libcamera master.\n> >\n> > Rest looks good to me so,\n> >\n> > Reviewed-by: Umang Jain \n> \n> Thanks, let's see about the output parameter thing.\n> >\n> >\n> > > \tstart() => (int32 ret);\n> > > \tstop();\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 71698d36e50f..d3c69bc07bd0 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -5,8 +5,10 @@\n> > > * ipu3.cpp - IPU3 Image Processing Algorithms\n> > > */\n> > > +#include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > @@ -38,7 +40,8 @@ namespace ipa::ipu3 {\n> > > class IPAIPU3 : public IPAIPU3Interface\n> > > {\n> > > public:\n> > > -\tint init(const IPASettings &settings) override;\n> > > +\tint init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;\n> > > +\n> > > \tint start() override;\n> > > \tvoid stop() override {}\n> > > @@ -86,14 +89,74 @@ private:\n> > > \tstruct ipu3_uapi_grid_config bdsGrid_;\n> > > };\n> > > -int IPAIPU3::init(const IPASettings &settings)\n> > > +/**\n> > > + * Initialize the IPA module and its controls.\n> > > + *\n> > > + * This function receives the camera sensor information from the pipeline\n> > > + * handler, computes the limits of the controls it handles and returns\n> > > + * them in the \\a ipaControls output parameter.\n> > > + */\n> > > +int IPAIPU3::init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls)\n> > > {\n> > > -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > > +\tconst IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;\n> > > +\n> > > +\t/* Initialize the camera sensor helper. */\n> > > +\tcamHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);\n> > > \tif (camHelper_ == nullptr) {\n> > > -\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \" << settings.sensorModel;\n> > > +\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \"\n> > > +\t\t\t\t << sensorInfo.model;\n> > > \t\treturn -ENODEV;\n> > > \t}\n> > > +\t/* Initialize Controls. */\n> > > +\tconst ControlInfoMap &sensorControls = initInfo.sensorControls;\n> > > +\tControlInfoMap::Map controls{};\n> > > +\n> > > +\t/*\n> > > +\t * Compute exposure time limits.\n> > > +\t *\n> > > +\t * Initialize the control using the line length and pixel rate of the\n> > > +\t * current configuration converted to microseconds. Use the\n> > > +\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > > +\t * convert it from lines to microseconds.\n> > > +\t */\n> > > +\tdouble lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> > > +\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > > +\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > > +\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > > +\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > > +\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > +\t\t\t\t\t\t\tdefExposure);\n> > > +\n> > > +\t/*\n> > > +\t * Compute the frame duration limits.\n> > > +\t *\n> > > +\t * The frame length is computed assuming a fixed line length combined\n> > > +\t * with the vertical frame sizes.\n> > > +\t */\n> > > +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > > +\tuint32_t hblank = v4l2HBlank.def().get();\n> > > +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > > +\n> > > +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > > +\tstd::array frameHeights{\n> > > +\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > > +\t};\n> > > +\n> > > +\tstd::array frameDurations;\n> > > +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > > +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > > +\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > > +\t}\n> > > +\n> > > +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> > > +\t\t\t\t\t\t\t frameDurations[1],\n> > > +\t\t\t\t\t\t\t frameDurations[2]);\n> > > +\n> > > +\t*ipaControls = std::move(controls);\n> > > +\n> > > \treturn 0;\n> > > }\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 76c3bb3d8aa9..22df9c3650af 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -88,6 +88,8 @@ public:\n> > > \tstd::queue pendingRequests_;\n> > > +\tControlInfoMap ipaControls_;\n> > > +\n> > > private:\n> > > \tvoid queueFrameAction(unsigned int id,\n> > > \t\t\t const ipa::ipu3::IPU3Action &action);\n> > > @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t\treturn ret;\n> > > \tControlInfoMap::Map controls = IPU3Controls;\n> > > -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> > > \tconst std::vector &testPatternModes = sensor->testPatternModes();\n> > > \tif (!testPatternModes.empty()) {\n> > > \t\tstd::vector values;\n> > > @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > > \t}\n> > > -\t/*\n> > > -\t * Compute exposure time limits.\n> > > -\t *\n> > > -\t * Initialize the control using the line length and pixel rate of the\n> > > -\t * current configuration converted to microseconds. Use the\n> > > -\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > > -\t * convert it from lines to microseconds.\n> > > -\t */\n> > > -\tdouble lineDuration = sensorInfo.lineLength\n> > > -\t\t\t / (sensorInfo.pixelRate / 1e6);\n> > > -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > > -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > > -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > > -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > > -\n> > > -\t/*\n> > > -\t * \\todo Report the actual exposure time, use the default for the\n> > > -\t * moment.\n> > > -\t */\n> > > -\tdata->exposureTime_ = defExposure;\n> > > -\n> > > -\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > -\t\t\t\t\t\t\tdefExposure);\n> > > -\n> > > -\t/*\n> > > -\t * Compute the frame duration limits.\n> > > -\t *\n> > > -\t * The frame length is computed assuming a fixed line length combined\n> > > -\t * with the vertical frame sizes.\n> > > -\t */\n> > > -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > > -\tuint32_t hblank = v4l2HBlank.def().get();\n> > > -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > > -\n> > > -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > > -\tstd::array frameHeights{\n> > > -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > > -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > > -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > > -\t};\n> > > -\n> > > -\tstd::array frameDurations;\n> > > -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > > -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > > -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > > -\t}\n> > > -\n> > > -\tcontrols[&controls::FrameDurationLimits] =\n> > > -\t\tControlInfo(frameDurations[0],\n> > > -\t\t\t frameDurations[1],\n> > > -\t\t\t frameDurations[2]);\n> > > -\n> > > \t/*\n> > > \t * Compute the scaler crop limits.\n> > > \t *\n> > > @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > > +\t/* Add the IPA registered controls to list of camera controls. */\n> > > +\tfor (const auto &ipaControl : data->ipaControls_)\n> > > +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> > > +\n> > > \tdata->controlInfo_ = std::move(controls);\n> > > \treturn 0;\n> > > @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()\n> > > \tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> > > +\t/*\n> > > +\t * Pass the sensor info the the IPA to initialize controls.\n> > > +\t *\n> > > +\t * \\todo The limits of the registered controls depend on the current\n> > > +\t * sensor configuration. Initialize the sensor using its resolution as\n> > > +\t * its initial configuration and use it to compute the controls limits\n> > > +\t * in the IPA.\n> > > +\t */\n> > > \tCameraSensor *sensor = cio2_.sensor();\n> > > -\tint ret = ipa_->init(IPASettings{ \"\", sensor->model() });\n> > > +\tV4L2SubdeviceFormat sensorFormat = {};\n> > > +\tsensorFormat.size = sensor->resolution();\n> > > +\tint ret = sensor->setFormat(&sensorFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tIPACameraSensorInfo sensorInfo{};\n> > > +\tret = sensor->sensorInfo(&sensorInfo);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tipa::ipu3::IPAInitInfo initInfo{\n> > > +\t\tsensorInfo,\n> > > +\t\tsensor->controls(),\n> > > +\t};\n> > > +\tipaControls_ = {};\n> > > +\tret = ipa_->init(initInfo, &ipaControls_);\n> > > \tif (ret) {\n> > > \t\tLOG(IPU3, Error) << \"Failed to initialise the IPU3 IPA\";\n> > > \t\treturn ret;\n> > > \t}\n> > > +\t/*\n> > > +\t * \\todo Report the actual exposure time, use the default for the\n> > > +\t * moment.\n> > > +\t */\n> > > +\tconst auto exposureInfo = ipaControls_.find(&controls::ExposureTime);\n> > > +\tif (exposureInfo == ipaControls_.end()) {\n> > > +\t\tLOG(IPU3, Error) << \"Exposure control not initializaed by the IPA\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\texposureTime_ = exposureInfo->second.def().get();\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 7891AC0109\n\tfor ;\n\tSun, 25 Jul 2021 01:35:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1E36687B7;\n\tSun, 25 Jul 2021 03:35:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98FBE60274\n\tfor ;\n\tSun, 25 Jul 2021 03:35:07 +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 F326E255;\n\tSun, 25 Jul 2021 03:35:06 +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=\"HjN5V0Ly\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627176907;\n\tbh=780ooHSRuqT85J52d+xEVyjRxRbz62sPHxtraPlQCFo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HjN5V0LyKiEf/pq4sM83S90GsKdGZ60cWHWVMI0m6l/D8GLxlL0jsGYAtEFZMZtUx\n\tP/cTqZKQnAY4+IcVqcDfarZohqN9xm6VsVP556BaHSDt0RxGE47dEZRn+dUVoAkMGe\n\tUFWuObjZ+TYvE7evUuOZE33b0tV3hlYpA6fQ0KT0=","Date":"Sun, 25 Jul 2021 04:35:03 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210716143215.67454-1-jacopo@jmondi.org>\n\t<20210716143215.67454-2-jacopo@jmondi.org>\n\t<38d04552-e6ea-6aee-e521-8a7e87b9cb40@ideasonboard.com>\n\t<20210722150109.bncnczoqfn7nzr7m@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210722150109.bncnczoqfn7nzr7m@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18333,"web_url":"https://patchwork.libcamera.org/comment/18333/","msgid":"","date":"2021-07-25T01:57:15","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Jul 16, 2021 at 04:32:14PM +0200, Jacopo Mondi wrote:\n> All the IPU3 Camera controls are currently initialized by the pipeline\n> handler which initializes them using the camera sensor configuration and\n> platform specific requirements.\n> \n> However, some controls are better initialized by the IPA, which might,\n> in example, cap the exposure times and frame duration to the constraints\n> of its algorithms implementation.\n> \n> Also, moving forward, the IPA should register controls to report its\n> capabilities, in example the ability to enable/disable 3A algorithms on\n> request.\n> \n> Move the existing controls initialization to the IPA, by providing\n> the sensor configuration and its controls to the IPU3IPA::init()\n> function, which initializes controls and returns them to the pipeline\n> through an output parameter.\n> \n> The existing controls initialization has been copied verbatim from the\n> pipeline handler to the IPA, if not a for few line breaks adjustments\n> and the resulting Camera controls values are not changed .\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/ipu3.mojom | 8 ++-\n> src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------\n> 3 files changed, 116 insertions(+), 59 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 911a3a072464..eafefa8b7fe3 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -30,6 +30,11 @@ struct IPU3Action {\n> \tlibcamera.ControlList controls;\n> };\n> \n> +struct IPAInitInfo {\n> +\tlibcamera.IPACameraSensorInfo sensorInfo;\n> +\tlibcamera.ControlInfoMap sensorControls;\n> +};\n> +\n> struct IPAConfigInfo {\n> \tlibcamera.IPACameraSensorInfo sensorInfo;\n> \tmap entityControls;\n> @@ -38,7 +43,8 @@ struct IPAConfigInfo {\n> };\n> \n> interface IPAIPU3Interface {\n> -\tinit(libcamera.IPASettings settings) => (int32 ret);\n> +\tinit(IPAInitInfo initInfo)\n\nBy dropping IPASettings, you drop the IPA configuration file. This isn't\nused by the IPA module yet, but it should be in the future, so it would\nbe best to keep it. One option is to add libcamera.IPASettings as a\nmember of IPAInitInfo.\n\n> +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \tstart() => (int32 ret);\n> \tstop();\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36e50f..d3c69bc07bd0 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -5,8 +5,10 @@\n> * ipu3.cpp - IPU3 Image Processing Algorithms\n> */\n> \n> +#include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -38,7 +40,8 @@ namespace ipa::ipu3 {\n> class IPAIPU3 : public IPAIPU3Interface\n> {\n> public:\n> -\tint init(const IPASettings &settings) override;\n> +\tint init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;\n> +\n> \tint start() override;\n> \tvoid stop() override {}\n> \n> @@ -86,14 +89,74 @@ private:\n> \tstruct ipu3_uapi_grid_config bdsGrid_;\n> };\n> \n> -int IPAIPU3::init(const IPASettings &settings)\n> +/**\n> + * Initialize the IPA module and its controls.\n> + *\n> + * This function receives the camera sensor information from the pipeline\n> + * handler, computes the limits of the controls it handles and returns\n> + * them in the \\a ipaControls output parameter.\n> + */\n> +int IPAIPU3::init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls)\n> {\n> -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> +\tconst IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;\n> +\n> +\t/* Initialize the camera sensor helper. */\n> +\tcamHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);\n> \tif (camHelper_ == nullptr) {\n> -\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \" << settings.sensorModel;\n> +\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \"\n> +\t\t\t\t << sensorInfo.model;\n> \t\treturn -ENODEV;\n> \t}\n> \n> +\t/* Initialize Controls. */\n> +\tconst ControlInfoMap &sensorControls = initInfo.sensorControls;\n> +\tControlInfoMap::Map controls{};\n> +\n> +\t/*\n> +\t * Compute exposure time limits.\n> +\t *\n> +\t * Initialize the control using the line length and pixel rate of the\n> +\t * current configuration converted to microseconds. Use the\n> +\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> +\t * convert it from lines to microseconds.\n> +\t */\n> +\tdouble lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> +\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> +\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> +\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> +\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> +\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> +\t\t\t\t\t\t\tdefExposure);\n> +\n> +\t/*\n> +\t * Compute the frame duration limits.\n> +\t *\n> +\t * The frame length is computed assuming a fixed line length combined\n> +\t * with the vertical frame sizes.\n> +\t */\n> +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> +\tuint32_t hblank = v4l2HBlank.def().get();\n> +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> +\n> +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> +\tstd::array frameHeights{\n> +\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> +\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> +\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> +\t};\n> +\n> +\tstd::array frameDurations;\n> +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> +\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> +\t}\n> +\n> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> +\t\t\t\t\t\t\t frameDurations[1],\n> +\t\t\t\t\t\t\t frameDurations[2]);\n> +\n> +\t*ipaControls = std::move(controls);\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 76c3bb3d8aa9..22df9c3650af 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -88,6 +88,8 @@ public:\n> \n> \tstd::queue pendingRequests_;\n> \n> +\tControlInfoMap ipaControls_;\n> +\n> private:\n> \tvoid queueFrameAction(unsigned int id,\n> \t\t\t const ipa::ipu3::IPU3Action &action);\n> @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t\treturn ret;\n> \n> \tControlInfoMap::Map controls = IPU3Controls;\n> -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> \tconst std::vector &testPatternModes = sensor->testPatternModes();\n> \tif (!testPatternModes.empty()) {\n> \t\tstd::vector values;\n> @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> \t}\n> \n> -\t/*\n> -\t * Compute exposure time limits.\n> -\t *\n> -\t * Initialize the control using the line length and pixel rate of the\n> -\t * current configuration converted to microseconds. Use the\n> -\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> -\t * convert it from lines to microseconds.\n> -\t */\n> -\tdouble lineDuration = sensorInfo.lineLength\n> -\t\t\t / (sensorInfo.pixelRate / 1e6);\n> -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> -\n> -\t/*\n> -\t * \\todo Report the actual exposure time, use the default for the\n> -\t * moment.\n> -\t */\n> -\tdata->exposureTime_ = defExposure;\n> -\n> -\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> -\t\t\t\t\t\t\tdefExposure);\n> -\n> -\t/*\n> -\t * Compute the frame duration limits.\n> -\t *\n> -\t * The frame length is computed assuming a fixed line length combined\n> -\t * with the vertical frame sizes.\n> -\t */\n> -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> -\tuint32_t hblank = v4l2HBlank.def().get();\n> -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> -\n> -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> -\tstd::array frameHeights{\n> -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> -\t};\n> -\n> -\tstd::array frameDurations;\n> -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> -\t}\n> -\n> -\tcontrols[&controls::FrameDurationLimits] =\n> -\t\tControlInfo(frameDurations[0],\n> -\t\t\t frameDurations[1],\n> -\t\t\t frameDurations[2]);\n> -\n> \t/*\n> \t * Compute the scaler crop limits.\n> \t *\n> @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \n> \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> \n> +\t/* Add the IPA registered controls to list of camera controls. */\n> +\tfor (const auto &ipaControl : data->ipaControls_)\n> +\t\tcontrols[ipaControl.first] = ipaControl.second;\n\nIt would be nice to use merge(), but ControlInfoMap inherits from\nstd::unordered_map<> with a private access specifier :-S Yet another\nopportunity to improve the controls API.\n\n> +\n> \tdata->controlInfo_ = std::move(controls);\n> \n> \treturn 0;\n> @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()\n> \n> \tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> \n> +\t/*\n> +\t * Pass the sensor info the the IPA to initialize controls.\n\ns/the the/to the/\n\n> +\t *\n> +\t * \\todo The limits of the registered controls depend on the current\n> +\t * sensor configuration. Initialize the sensor using its resolution as\n> +\t * its initial configuration and use it to compute the controls limits\n> +\t * in the IPA.\n\nCould you rewrite this to explain what would need to be done to fix the\nproblem, instead of only describing the current implementation ? I'm\nsure we understand it correctly now, but that may not be the case when\nwe'll tackle this todo item, or if someone less familiar with the issue\nreads the comment.\n\n> +\t */\n> \tCameraSensor *sensor = cio2_.sensor();\n> -\tint ret = ipa_->init(IPASettings{ \"\", sensor->model() });\n> +\tV4L2SubdeviceFormat sensorFormat = {};\n> +\tsensorFormat.size = sensor->resolution();\n> +\tint ret = sensor->setFormat(&sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tIPACameraSensorInfo sensorInfo{};\n> +\tret = sensor->sensorInfo(&sensorInfo);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tipa::ipu3::IPAInitInfo initInfo{\n> +\t\tsensorInfo,\n> +\t\tsensor->controls(),\n> +\t};\n> +\tipaControls_ = {};\n\nI think this line could be dropped, ipaControls_ is default-initialized\nwhen IPU3CameraData is constructed, and loadIPA() is called once only.\n\nReviewed-by: Laurent Pinchart \n\n> +\tret = ipa_->init(initInfo, &ipaControls_);\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Failed to initialise the IPU3 IPA\";\n> \t\treturn ret;\n> \t}\n> \n> +\t/*\n> +\t * \\todo Report the actual exposure time, use the default for the\n> +\t * moment.\n> +\t */\n> +\tconst auto exposureInfo = ipaControls_.find(&controls::ExposureTime);\n> +\tif (exposureInfo == ipaControls_.end()) {\n> +\t\tLOG(IPU3, Error) << \"Exposure control not initializaed by the IPA\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\texposureTime_ = exposureInfo->second.def().get();\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 75F14C322C\n\tfor ;\n\tSun, 25 Jul 2021 01:57:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FC95687B5;\n\tSun, 25 Jul 2021 03:57:20 +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 BFA2860274\n\tfor ;\n\tSun, 25 Jul 2021 03:57:19 +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 33AE4255;\n\tSun, 25 Jul 2021 03:57:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NGu2RAmy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627178239;\n\tbh=vvfXhZFTr85MaoWfjZPBxiG9w8xkI8cF1siOTeG6hMI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NGu2RAmyDvlHJqDzvMLN+EIP66FK7yAQBCzuWk1Kmi38GxYXf8Q2TSnYrrvHgJt8n\n\tfPyV5xGpO8mHVBobJxJHSWHpFlu0RlkvCmZtQiG7HDCKFPMqhn4hr8ZIeVm+nrTCHP\n\t1sCNYtc4a6npEL+iNIxp8mGJde1gpdxrhe2BWV8Q=","Date":"Sun, 25 Jul 2021 04:57:15 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210716143215.67454-1-jacopo@jmondi.org>\n\t<20210716143215.67454-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210716143215.67454-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18340,"web_url":"https://patchwork.libcamera.org/comment/18340/","msgid":"<20210726082641.3akgxuh4aoirpmfn@uno.localdomain>","date":"2021-07-26T08:26:41","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Sun, Jul 25, 2021 at 04:57:15AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 16, 2021 at 04:32:14PM +0200, Jacopo Mondi wrote:\n> > All the IPU3 Camera controls are currently initialized by the pipeline\n> > handler which initializes them using the camera sensor configuration and\n> > platform specific requirements.\n> >\n> > However, some controls are better initialized by the IPA, which might,\n> > in example, cap the exposure times and frame duration to the constraints\n> > of its algorithms implementation.\n> >\n> > Also, moving forward, the IPA should register controls to report its\n> > capabilities, in example the ability to enable/disable 3A algorithms on\n> > request.\n> >\n> > Move the existing controls initialization to the IPA, by providing\n> > the sensor configuration and its controls to the IPU3IPA::init()\n> > function, which initializes controls and returns them to the pipeline\n> > through an output parameter.\n> >\n> > The existing controls initialization has been copied verbatim from the\n> > pipeline handler to the IPA, if not a for few line breaks adjustments\n> > and the resulting Camera controls values are not changed .\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 8 ++-\n> > src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------\n> > 3 files changed, 116 insertions(+), 59 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > index 911a3a072464..eafefa8b7fe3 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -30,6 +30,11 @@ struct IPU3Action {\n> > \tlibcamera.ControlList controls;\n> > };\n> >\n> > +struct IPAInitInfo {\n> > +\tlibcamera.IPACameraSensorInfo sensorInfo;\n> > +\tlibcamera.ControlInfoMap sensorControls;\n> > +};\n> > +\n> > struct IPAConfigInfo {\n> > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > \tmap entityControls;\n> > @@ -38,7 +43,8 @@ struct IPAConfigInfo {\n> > };\n> >\n> > interface IPAIPU3Interface {\n> > -\tinit(libcamera.IPASettings settings) => (int32 ret);\n> > +\tinit(IPAInitInfo initInfo)\n>\n> By dropping IPASettings, you drop the IPA configuration file. This isn't\n> used by the IPA module yet, but it should be in the future, so it would\n> be best to keep it. One option is to add libcamera.IPASettings as a\n> member of IPAInitInfo.\n\nShouldn't this be done once anyone actually uses the configuration\nfile ? Otherwise we should keep passing \"\" as part of the init()\nfunction argument list, which is cumbersome ?\n\n>\n> > +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > \tstart() => (int32 ret);\n> > \tstop();\n> >\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 71698d36e50f..d3c69bc07bd0 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -5,8 +5,10 @@\n> > * ipu3.cpp - IPU3 Image Processing Algorithms\n> > */\n> >\n> > +#include \n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> > #include \n> > @@ -38,7 +40,8 @@ namespace ipa::ipu3 {\n> > class IPAIPU3 : public IPAIPU3Interface\n> > {\n> > public:\n> > -\tint init(const IPASettings &settings) override;\n> > +\tint init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;\n> > +\n> > \tint start() override;\n> > \tvoid stop() override {}\n> >\n> > @@ -86,14 +89,74 @@ private:\n> > \tstruct ipu3_uapi_grid_config bdsGrid_;\n> > };\n> >\n> > -int IPAIPU3::init(const IPASettings &settings)\n> > +/**\n> > + * Initialize the IPA module and its controls.\n> > + *\n> > + * This function receives the camera sensor information from the pipeline\n> > + * handler, computes the limits of the controls it handles and returns\n> > + * them in the \\a ipaControls output parameter.\n> > + */\n> > +int IPAIPU3::init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls)\n> > {\n> > -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > +\tconst IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;\n> > +\n> > +\t/* Initialize the camera sensor helper. */\n> > +\tcamHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);\n> > \tif (camHelper_ == nullptr) {\n> > -\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \" << settings.sensorModel;\n> > +\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \"\n> > +\t\t\t\t << sensorInfo.model;\n> > \t\treturn -ENODEV;\n> > \t}\n> >\n> > +\t/* Initialize Controls. */\n> > +\tconst ControlInfoMap &sensorControls = initInfo.sensorControls;\n> > +\tControlInfoMap::Map controls{};\n> > +\n> > +\t/*\n> > +\t * Compute exposure time limits.\n> > +\t *\n> > +\t * Initialize the control using the line length and pixel rate of the\n> > +\t * current configuration converted to microseconds. Use the\n> > +\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > +\t * convert it from lines to microseconds.\n> > +\t */\n> > +\tdouble lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> > +\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > +\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > +\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > +\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > +\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > +\t\t\t\t\t\t\tdefExposure);\n> > +\n> > +\t/*\n> > +\t * Compute the frame duration limits.\n> > +\t *\n> > +\t * The frame length is computed assuming a fixed line length combined\n> > +\t * with the vertical frame sizes.\n> > +\t */\n> > +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > +\tuint32_t hblank = v4l2HBlank.def().get();\n> > +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > +\n> > +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > +\tstd::array frameHeights{\n> > +\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > +\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > +\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > +\t};\n> > +\n> > +\tstd::array frameDurations;\n> > +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > +\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > +\t}\n> > +\n> > +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> > +\t\t\t\t\t\t\t frameDurations[1],\n> > +\t\t\t\t\t\t\t frameDurations[2]);\n> > +\n> > +\t*ipaControls = std::move(controls);\n> > +\n> > \treturn 0;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 76c3bb3d8aa9..22df9c3650af 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -88,6 +88,8 @@ public:\n> >\n> > \tstd::queue pendingRequests_;\n> >\n> > +\tControlInfoMap ipaControls_;\n> > +\n> > private:\n> > \tvoid queueFrameAction(unsigned int id,\n> > \t\t\t const ipa::ipu3::IPU3Action &action);\n> > @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t\treturn ret;\n> >\n> > \tControlInfoMap::Map controls = IPU3Controls;\n> > -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> > \tconst std::vector &testPatternModes = sensor->testPatternModes();\n> > \tif (!testPatternModes.empty()) {\n> > \t\tstd::vector values;\n> > @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > \t}\n> >\n> > -\t/*\n> > -\t * Compute exposure time limits.\n> > -\t *\n> > -\t * Initialize the control using the line length and pixel rate of the\n> > -\t * current configuration converted to microseconds. Use the\n> > -\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > -\t * convert it from lines to microseconds.\n> > -\t */\n> > -\tdouble lineDuration = sensorInfo.lineLength\n> > -\t\t\t / (sensorInfo.pixelRate / 1e6);\n> > -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > -\n> > -\t/*\n> > -\t * \\todo Report the actual exposure time, use the default for the\n> > -\t * moment.\n> > -\t */\n> > -\tdata->exposureTime_ = defExposure;\n> > -\n> > -\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > -\t\t\t\t\t\t\tdefExposure);\n> > -\n> > -\t/*\n> > -\t * Compute the frame duration limits.\n> > -\t *\n> > -\t * The frame length is computed assuming a fixed line length combined\n> > -\t * with the vertical frame sizes.\n> > -\t */\n> > -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > -\tuint32_t hblank = v4l2HBlank.def().get();\n> > -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > -\n> > -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > -\tstd::array frameHeights{\n> > -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > -\t};\n> > -\n> > -\tstd::array frameDurations;\n> > -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > -\t}\n> > -\n> > -\tcontrols[&controls::FrameDurationLimits] =\n> > -\t\tControlInfo(frameDurations[0],\n> > -\t\t\t frameDurations[1],\n> > -\t\t\t frameDurations[2]);\n> > -\n> > \t/*\n> > \t * Compute the scaler crop limits.\n> > \t *\n> > @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> >\n> > \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> >\n> > +\t/* Add the IPA registered controls to list of camera controls. */\n> > +\tfor (const auto &ipaControl : data->ipaControls_)\n> > +\t\tcontrols[ipaControl.first] = ipaControl.second;\n>\n> It would be nice to use merge(), but ControlInfoMap inherits from\n> std::unordered_map<> with a private access specifier :-S Yet another\n> opportunity to improve the controls API.\n>\n> > +\n> > \tdata->controlInfo_ = std::move(controls);\n> >\n> > \treturn 0;\n> > @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()\n> >\n> > \tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> >\n> > +\t/*\n> > +\t * Pass the sensor info the the IPA to initialize controls.\n>\n> s/the the/to the/\n>\n> > +\t *\n> > +\t * \\todo The limits of the registered controls depend on the current\n> > +\t * sensor configuration. Initialize the sensor using its resolution as\n> > +\t * its initial configuration and use it to compute the controls limits\n> > +\t * in the IPA.\n>\n> Could you rewrite this to explain what would need to be done to fix the\n> problem, instead of only describing the current implementation ? I'm\n> sure we understand it correctly now, but that may not be the case when\n> we'll tackle this todo item, or if someone less familiar with the issue\n> reads the comment.\n\nNot sure I know exactly what needs to be done, but I presume it boils\ndown to find some sort of heuristic to identify a sensor default\nconfiguration to initialize controls with, like the Viewfinder\nconfiguration. Something like:\n\n\t *\n\t * \\todo The limits of the registered controls depend on the current\n\t * sensor configuration. The sensor is currently initialized\n * using its resolution but a better heuristic should be\n * defined to identify the most opportune configuration to\n * initialize the Camera controls with. A possibility is to\n * use the configuration associated with one of the supported\n * Roles, such as the Viewfinder configuration.\n */\n\n>\n> > +\t */\n> > \tCameraSensor *sensor = cio2_.sensor();\n> > -\tint ret = ipa_->init(IPASettings{ \"\", sensor->model() });\n> > +\tV4L2SubdeviceFormat sensorFormat = {};\n> > +\tsensorFormat.size = sensor->resolution();\n> > +\tint ret = sensor->setFormat(&sensorFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tIPACameraSensorInfo sensorInfo{};\n> > +\tret = sensor->sensorInfo(&sensorInfo);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tipa::ipu3::IPAInitInfo initInfo{\n> > +\t\tsensorInfo,\n> > +\t\tsensor->controls(),\n> > +\t};\n> > +\tipaControls_ = {};\n>\n> I think this line could be dropped, ipaControls_ is default-initialized\n> when IPU3CameraData is constructed, and loadIPA() is called once only.\n\nCorrect!\n\n>\n> Reviewed-by: Laurent Pinchart \n\nThanks\n j\n\n>\n> > +\tret = ipa_->init(initInfo, &ipaControls_);\n> > \tif (ret) {\n> > \t\tLOG(IPU3, Error) << \"Failed to initialise the IPU3 IPA\";\n> > \t\treturn ret;\n> > \t}\n> >\n> > +\t/*\n> > +\t * \\todo Report the actual exposure time, use the default for the\n> > +\t * moment.\n> > +\t */\n> > +\tconst auto exposureInfo = ipaControls_.find(&controls::ExposureTime);\n> > +\tif (exposureInfo == ipaControls_.end()) {\n> > +\t\tLOG(IPU3, Error) << \"Exposure control not initializaed by the IPA\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\texposureTime_ = exposureInfo->second.def().get();\n> > +\n> > \treturn 0;\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0A0F1C0109\n\tfor ;\n\tMon, 26 Jul 2021 08:25:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82016687B8;\n\tMon, 26 Jul 2021 10:25:55 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DE01687B3\n\tfor ;\n\tMon, 26 Jul 2021 10:25:54 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id C5A4540012;\n\tMon, 26 Jul 2021 08:25:53 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 10:26:41 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210726082641.3akgxuh4aoirpmfn@uno.localdomain>","References":"<20210716143215.67454-1-jacopo@jmondi.org>\n\t<20210716143215.67454-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18372,"web_url":"https://patchwork.libcamera.org/comment/18372/","msgid":"","date":"2021-07-27T03:17:59","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 26, 2021 at 10:26:41AM +0200, Jacopo Mondi wrote:\n> On Sun, Jul 25, 2021 at 04:57:15AM +0300, Laurent Pinchart wrote:\n> > On Fri, Jul 16, 2021 at 04:32:14PM +0200, Jacopo Mondi wrote:\n> > > All the IPU3 Camera controls are currently initialized by the pipeline\n> > > handler which initializes them using the camera sensor configuration and\n> > > platform specific requirements.\n> > >\n> > > However, some controls are better initialized by the IPA, which might,\n> > > in example, cap the exposure times and frame duration to the constraints\n> > > of its algorithms implementation.\n> > >\n> > > Also, moving forward, the IPA should register controls to report its\n> > > capabilities, in example the ability to enable/disable 3A algorithms on\n> > > request.\n> > >\n> > > Move the existing controls initialization to the IPA, by providing\n> > > the sensor configuration and its controls to the IPU3IPA::init()\n> > > function, which initializes controls and returns them to the pipeline\n> > > through an output parameter.\n> > >\n> > > The existing controls initialization has been copied verbatim from the\n> > > pipeline handler to the IPA, if not a for few line breaks adjustments\n> > > and the resulting Camera controls values are not changed .\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 8 ++-\n> > > src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------\n> > > 3 files changed, 116 insertions(+), 59 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > index 911a3a072464..eafefa8b7fe3 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -30,6 +30,11 @@ struct IPU3Action {\n> > > \tlibcamera.ControlList controls;\n> > > };\n> > >\n> > > +struct IPAInitInfo {\n> > > +\tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > +\tlibcamera.ControlInfoMap sensorControls;\n> > > +};\n> > > +\n> > > struct IPAConfigInfo {\n> > > \tlibcamera.IPACameraSensorInfo sensorInfo;\n> > > \tmap entityControls;\n> > > @@ -38,7 +43,8 @@ struct IPAConfigInfo {\n> > > };\n> > >\n> > > interface IPAIPU3Interface {\n> > > -\tinit(libcamera.IPASettings settings) => (int32 ret);\n> > > +\tinit(IPAInitInfo initInfo)\n> >\n> > By dropping IPASettings, you drop the IPA configuration file. This isn't\n> > used by the IPA module yet, but it should be in the future, so it would\n> > be best to keep it. One option is to add libcamera.IPASettings as a\n> > member of IPAInitInfo.\n> \n> Shouldn't this be done once anyone actually uses the configuration\n> file ? Otherwise we should keep passing \"\" as part of the init()\n> function argument list, which is cumbersome ?\n\nWe can do so as well, but as we know the need is there already... I'd\nactually keep the settings parameter and add initInfo, instead of moving\nsettings in initInfo.\n\n> > > +\t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > \tstart() => (int32 ret);\n> > > \tstop();\n> > >\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 71698d36e50f..d3c69bc07bd0 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -5,8 +5,10 @@\n> > > * ipu3.cpp - IPU3 Image Processing Algorithms\n> > > */\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > > #include \n> > > @@ -38,7 +40,8 @@ namespace ipa::ipu3 {\n> > > class IPAIPU3 : public IPAIPU3Interface\n> > > {\n> > > public:\n> > > -\tint init(const IPASettings &settings) override;\n> > > +\tint init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;\n> > > +\n> > > \tint start() override;\n> > > \tvoid stop() override {}\n> > >\n> > > @@ -86,14 +89,74 @@ private:\n> > > \tstruct ipu3_uapi_grid_config bdsGrid_;\n> > > };\n> > >\n> > > -int IPAIPU3::init(const IPASettings &settings)\n> > > +/**\n> > > + * Initialize the IPA module and its controls.\n> > > + *\n> > > + * This function receives the camera sensor information from the pipeline\n> > > + * handler, computes the limits of the controls it handles and returns\n> > > + * them in the \\a ipaControls output parameter.\n> > > + */\n> > > +int IPAIPU3::init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls)\n> > > {\n> > > -\tcamHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);\n> > > +\tconst IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;\n> > > +\n> > > +\t/* Initialize the camera sensor helper. */\n> > > +\tcamHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);\n> > > \tif (camHelper_ == nullptr) {\n> > > -\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \" << settings.sensorModel;\n> > > +\t\tLOG(IPAIPU3, Error) << \"Failed to create camera sensor helper for \"\n> > > +\t\t\t\t << sensorInfo.model;\n> > > \t\treturn -ENODEV;\n> > > \t}\n> > >\n> > > +\t/* Initialize Controls. */\n> > > +\tconst ControlInfoMap &sensorControls = initInfo.sensorControls;\n> > > +\tControlInfoMap::Map controls{};\n> > > +\n> > > +\t/*\n> > > +\t * Compute exposure time limits.\n> > > +\t *\n> > > +\t * Initialize the control using the line length and pixel rate of the\n> > > +\t * current configuration converted to microseconds. Use the\n> > > +\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > > +\t * convert it from lines to microseconds.\n> > > +\t */\n> > > +\tdouble lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> > > +\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > > +\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > > +\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > > +\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > > +\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > +\t\t\t\t\t\t\tdefExposure);\n> > > +\n> > > +\t/*\n> > > +\t * Compute the frame duration limits.\n> > > +\t *\n> > > +\t * The frame length is computed assuming a fixed line length combined\n> > > +\t * with the vertical frame sizes.\n> > > +\t */\n> > > +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > > +\tuint32_t hblank = v4l2HBlank.def().get();\n> > > +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > > +\n> > > +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > > +\tstd::array frameHeights{\n> > > +\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > > +\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > > +\t};\n> > > +\n> > > +\tstd::array frameDurations;\n> > > +\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > > +\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > > +\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > > +\t}\n> > > +\n> > > +\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> > > +\t\t\t\t\t\t\t frameDurations[1],\n> > > +\t\t\t\t\t\t\t frameDurations[2]);\n> > > +\n> > > +\t*ipaControls = std::move(controls);\n> > > +\n> > > \treturn 0;\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 76c3bb3d8aa9..22df9c3650af 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -88,6 +88,8 @@ public:\n> > >\n> > > \tstd::queue pendingRequests_;\n> > >\n> > > +\tControlInfoMap ipaControls_;\n> > > +\n> > > private:\n> > > \tvoid queueFrameAction(unsigned int id,\n> > > \t\t\t const ipa::ipu3::IPU3Action &action);\n> > > @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t\treturn ret;\n> > >\n> > > \tControlInfoMap::Map controls = IPU3Controls;\n> > > -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> > > \tconst std::vector &testPatternModes = sensor->testPatternModes();\n> > > \tif (!testPatternModes.empty()) {\n> > > \t\tstd::vector values;\n> > > @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > > \t}\n> > >\n> > > -\t/*\n> > > -\t * Compute exposure time limits.\n> > > -\t *\n> > > -\t * Initialize the control using the line length and pixel rate of the\n> > > -\t * current configuration converted to microseconds. Use the\n> > > -\t * V4L2_CID_EXPOSURE control to get exposure min, max and default and\n> > > -\t * convert it from lines to microseconds.\n> > > -\t */\n> > > -\tdouble lineDuration = sensorInfo.lineLength\n> > > -\t\t\t / (sensorInfo.pixelRate / 1e6);\n> > > -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > > -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > > -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > > -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > > -\n> > > -\t/*\n> > > -\t * \\todo Report the actual exposure time, use the default for the\n> > > -\t * moment.\n> > > -\t */\n> > > -\tdata->exposureTime_ = defExposure;\n> > > -\n> > > -\tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > -\t\t\t\t\t\t\tdefExposure);\n> > > -\n> > > -\t/*\n> > > -\t * Compute the frame duration limits.\n> > > -\t *\n> > > -\t * The frame length is computed assuming a fixed line length combined\n> > > -\t * with the vertical frame sizes.\n> > > -\t */\n> > > -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > > -\tuint32_t hblank = v4l2HBlank.def().get();\n> > > -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > > -\n> > > -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > > -\tstd::array frameHeights{\n> > > -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> > > -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> > > -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> > > -\t};\n> > > -\n> > > -\tstd::array frameDurations;\n> > > -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> > > -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> > > -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> > > -\t}\n> > > -\n> > > -\tcontrols[&controls::FrameDurationLimits] =\n> > > -\t\tControlInfo(frameDurations[0],\n> > > -\t\t\t frameDurations[1],\n> > > -\t\t\t frameDurations[2]);\n> > > -\n> > > \t/*\n> > > \t * Compute the scaler crop limits.\n> > > \t *\n> > > @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > >\n> > > \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > >\n> > > +\t/* Add the IPA registered controls to list of camera controls. */\n> > > +\tfor (const auto &ipaControl : data->ipaControls_)\n> > > +\t\tcontrols[ipaControl.first] = ipaControl.second;\n> >\n> > It would be nice to use merge(), but ControlInfoMap inherits from\n> > std::unordered_map<> with a private access specifier :-S Yet another\n> > opportunity to improve the controls API.\n> >\n> > > +\n> > > \tdata->controlInfo_ = std::move(controls);\n> > >\n> > > \treturn 0;\n> > > @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()\n> > >\n> > > \tipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);\n> > >\n> > > +\t/*\n> > > +\t * Pass the sensor info the the IPA to initialize controls.\n> >\n> > s/the the/to the/\n> >\n> > > +\t *\n> > > +\t * \\todo The limits of the registered controls depend on the current\n> > > +\t * sensor configuration. Initialize the sensor using its resolution as\n> > > +\t * its initial configuration and use it to compute the controls limits\n> > > +\t * in the IPA.\n> >\n> > Could you rewrite this to explain what would need to be done to fix the\n> > problem, instead of only describing the current implementation ? I'm\n> > sure we understand it correctly now, but that may not be the case when\n> > we'll tackle this todo item, or if someone less familiar with the issue\n> > reads the comment.\n> \n> Not sure I know exactly what needs to be done, but I presume it boils\n> down to find some sort of heuristic to identify a sensor default\n> configuration to initialize controls with, like the Viewfinder\n> configuration. Something like:\n> \n> \t *\n> \t * \\todo The limits of the registered controls depend on the current\n> \t * sensor configuration. The sensor is currently initialized\n> * using its resolution but a better heuristic should be\n> * defined to identify the most opportune configuration to\n> * initialize the Camera controls with. A possibility is to\n> * use the configuration associated with one of the supported\n> * Roles, such as the Viewfinder configuration.\n> */\n\nI'm not sure what the right solution is either (otherwise we would\nimplement it already :-)). I meant something like\n\n\t/**\n\t * \\todo Find a way to initialize IPA controls without basing their\n\t * limits on a particular sensor mode. We currently pass sensor\n\t * information corresponding to the largest sensor resolution, and the\n\t * IPA uses this to compute limits for supported controls. There's a\n\t * discrepancy between the need to compute IPA control limits at init\n\t * time, and the fact that those limits may depend on the sensor mode.\n\t * Research is required to find out to handle this issue.\n\t */\n\n> > > +\t */\n> > > \tCameraSensor *sensor = cio2_.sensor();\n> > > -\tint ret = ipa_->init(IPASettings{ \"\", sensor->model() });\n> > > +\tV4L2SubdeviceFormat sensorFormat = {};\n> > > +\tsensorFormat.size = sensor->resolution();\n> > > +\tint ret = sensor->setFormat(&sensorFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tIPACameraSensorInfo sensorInfo{};\n> > > +\tret = sensor->sensorInfo(&sensorInfo);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tipa::ipu3::IPAInitInfo initInfo{\n> > > +\t\tsensorInfo,\n> > > +\t\tsensor->controls(),\n> > > +\t};\n> > > +\tipaControls_ = {};\n> >\n> > I think this line could be dropped, ipaControls_ is default-initialized\n> > when IPU3CameraData is constructed, and loadIPA() is called once only.\n> \n> Correct!\n> \n> > Reviewed-by: Laurent Pinchart \n> >\n> > > +\tret = ipa_->init(initInfo, &ipaControls_);\n> > > \tif (ret) {\n> > > \t\tLOG(IPU3, Error) << \"Failed to initialise the IPU3 IPA\";\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > +\t/*\n> > > +\t * \\todo Report the actual exposure time, use the default for the\n> > > +\t * moment.\n> > > +\t */\n> > > +\tconst auto exposureInfo = ipaControls_.find(&controls::ExposureTime);\n> > > +\tif (exposureInfo == ipaControls_.end()) {\n> > > +\t\tLOG(IPU3, Error) << \"Exposure control not initializaed by the IPA\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\texposureTime_ = exposureInfo->second.def().get();\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 6015EC0109\n\tfor ;\n\tTue, 27 Jul 2021 03:18:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B95E687C1;\n\tTue, 27 Jul 2021 05:18:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 880DC60506\n\tfor ;\n\tTue, 27 Jul 2021 05:18:04 +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 F0B37EE;\n\tTue, 27 Jul 2021 05:18:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BllMk1d9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627355884;\n\tbh=Ams4KUF5l/h++q5AoX0mUuSyR4/jwqDsYitmXTpcdRM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BllMk1d9QiE7wiV4o+l9TdhJCX3MtGNNhHEFmB7qRDpgRamVUrfJkJdED4PNAV18+\n\tjlQPcjwxdGoaZENtyhlYjTfjzk7ka0OePzXTWwo3DeICDLPgzIMHvYg57GKF0cRMaY\n\t/vb+NURCGiUSugL98RZuwxen3hLIIE/AfD2MSA94=","Date":"Tue, 27 Jul 2021 06:17:59 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210716143215.67454-1-jacopo@jmondi.org>\n\t<20210716143215.67454-2-jacopo@jmondi.org>\n\t\n\t<20210726082641.3akgxuh4aoirpmfn@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210726082641.3akgxuh4aoirpmfn@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize\n\tcontrols in the IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]