[{"id":15720,"web_url":"https://patchwork.libcamera.org/comment/15720/","msgid":"<20210316111310.3i5apq2p7ksikzlr@uno.localdomain>","date":"2021-03-16T11:13:10","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Jean-Micheal,\n\nOn Tue, Mar 16, 2021 at 11:18:38AM +0100, Jean-Michel Hautbois wrote:\n> The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to\n> calculate the statistics grids.\n> This patch makes it possible for PipelineHandlerIPU3::start() to access\n> the BDS configuration and later pass the rectangle to the IPU3 IPA\n> configure method.\n>\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> v2:\n> - move pipe configuration calculation from validate to configure\n> - move ipa configuration from start to configure\n\nThis are really 2 (or maybe even 3) separate patches.\n\nAlso, when applied on master, this patch does not compile here\n\n../src/libcamera/pipeline/ipu3/ipu3.cpp:633:40: error: too many arguments to function call, expected single argument 'entityControls', have 2 arguments\n data->ipa_->configure(entityControls, config->pipeConfig_.bds);\n ~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~\ninclude/libcamera/ipa/ipu3_ipa_proxy.h:44:14: note: 'configure' declared here\n void configure(\n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 45 +++++++++++++---------------\n> 1 file changed, 20 insertions(+), 25 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2ea13ec9..3f49ded3 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -97,21 +97,23 @@ public:\n> \tStatus validate() override;\n>\n> \tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; }\n> -\tconst ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }\n>\n> \t/* Cache the combinedTransform_ that will be applied to the sensor */\n> \tTransform combinedTransform_;\n>\n> +\t/* Store the pipe configuration */\n> +\tImgUDevice::PipeConfig pipeConfig_;\n> +\tImgUDevice::Pipe pipe_{};\n\nIs the initialization needed ?\nI would rather zero-initialize pipe_ at every validate() call.\n\n> +\n> private:\n> \t/*\n> \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> \t * corresponding Camera instance is valid. In order to borrow a\n> \t * reference to the camera data, store a new reference to the camera.\n> \t */\n> -\tconst IPU3CameraData *data_;\n> +\tIPU3CameraData *data_;\n\nYou don't need this change anymore\n\n>\n> \tStreamConfiguration cio2Configuration_;\n> -\tImgUDevice::PipeConfig pipeConfig_;\n> };\n>\n> class PipelineHandlerIPU3 : public PipelineHandler\n> @@ -272,8 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>\n> \tLOG(IPU3, Debug) << \"CIO2 configuration: \" << cio2Configuration_.toString();\n>\n> -\tImgUDevice::Pipe pipe{};\n> -\tpipe.input = cio2Configuration_.size;\n> +\tpipe_.input = cio2Configuration_.size;\n>\n> \t/*\n> \t * Adjust the configurations if needed and assign streams while\n> @@ -349,15 +350,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\t\t\tcfg->setStream(const_cast(&data_->outStream_));\n> \t\t\t\tmainOutputAvailable = false;\n>\n> -\t\t\t\tpipe.main = cfg->size;\n> +\t\t\t\tpipe_.main = cfg->size;\n> \t\t\t\tif (yuvCount == 1)\n> -\t\t\t\t\tpipe.viewfinder = pipe.main;\n> +\t\t\t\t\tpipe_.viewfinder = pipe_.main;\n>\n> \t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> \t\t\t\t\t\t << \" to the main output\";\n> \t\t\t} else {\n> \t\t\t\tcfg->setStream(const_cast(&data_->vfStream_));\n> -\t\t\t\tpipe.viewfinder = cfg->size;\n> +\t\t\t\tpipe_.viewfinder = cfg->size;\n>\n> \t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> \t\t\t\t\t\t << \" to the viewfinder output\";\n> @@ -373,16 +374,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\t}\n> \t}\n>\n> -\t/* Only compute the ImgU configuration if a YUV stream has been requested. */\n> -\tif (yuvCount) {\n> -\t\tpipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe);\n> -\t\tif (pipeConfig_.isNull()) {\n> -\t\t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration: \"\n> -\t\t\t\t\t << \"unsupported resolutions.\";\n> -\t\t\treturn Invalid;\n> -\t\t}\n> -\t}\n> -\n> \treturn status;\n> }\n>\n> @@ -576,11 +567,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t * stream has been requested: return here to skip the ImgU configuration\n> \t * part.\n> \t */\n> -\tImgUDevice::PipeConfig imguConfig = config->imguConfig();\n> -\tif (imguConfig.isNull())\n\nYou can't drop this, as you the comment explain if not imguConfig was\ncalculated in validate() it's because !yuvCount, and we should stop\nhere in configure.\n\nYou can keep collecting the pipe_ sizes in validate() and replace this\ncheck with\n if (pipe_.main.isNull() && pipe_.viewfinder.isNull())\n return 0;\n\n> +\n> +\tconfig->pipeConfig_ = imgu->calculatePipeConfig(&config->pipe_);\n> +\tif (config->pipeConfig_.isNull()) {\n> +\t\tLOG(IPU3, Error) << \"Failed to calculate pipe configuration: \"\n> +\t\t\t\t << \"unsupported resolutions.\";\n\nOtherwise this will always fail in case of RAW-only capture.\n\n> \t\treturn 0;\n> +\t}\n>\n> -\tret = imgu->configure(imguConfig, &cio2Format);\n> +\tret = imgu->configure(config->pipeConfig_, &cio2Format);\n\nThen you can move the ImgU pipe configuration to\nImgUDevice::configure()\n\n> \tif (ret)\n> \t\treturn ret;\n>\n> @@ -633,6 +628,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\treturn ret;\n> \t}\n>\n> +\tstd::map entityControls;\n> +\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> +\tdata->ipa_->configure(entityControls, config->pipeConfig_.bds);\n> +\n\nMoving ipa_->configure() here is worth a separate patch\nAdding the BDS rectange to IPA::configure() is worth a separate patch\n\nThanks\n j\n\n> \treturn 0;\n> }\n>\n> @@ -717,7 +716,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>\n> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> -\tstd::map entityControls;\n> \tIPU3CameraData *data = cameraData(camera);\n> \tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> @@ -744,9 +742,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \tif (ret)\n> \t\tgoto error;\n>\n> -\tentityControls.emplace(0, data->cio2_.sensor()->controls());\n> -\tdata->ipa_->configure(entityControls);\n> -\n> \treturn 0;\n>\n> error:\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","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 2EAB4C32E1\n\tfor ;\n\tTue, 16 Mar 2021 11:12:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82A2868D48;\n\tTue, 16 Mar 2021 12:12:41 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 010F46084F\n\tfor ;\n\tTue, 16 Mar 2021 12:12:39 +0100 (CET)","from uno.localdomain (mob-37-118-70-207.net.vodafone.it\n\t[37.118.70.207]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 166CEE0018;\n\tTue, 16 Mar 2021 11:12:38 +0000 (UTC)"],"X-Originating-IP":"37.118.70.207","Date":"Tue, 16 Mar 2021 12:13:10 +0100","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20210316111310.3i5apq2p7ksikzlr@uno.localdomain>","References":"<20210316101842.18674-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210316101842.18674-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210316101842.18674-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: ipu3: Move Imgu\n\tconfiguration to IPU3CameraData","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]