[{"id":26939,"web_url":"https://patchwork.libcamera.org/comment/26939/","msgid":"","date":"2023-04-26T16:18:44","subject":"Re: [libcamera-devel] [PATCH 07/13] pipeline: ipa: raspberrypi:\n\tReplace IPA metadataReady() call","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Wed, Apr 26, 2023 at 02:10:51PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Remove the IPA -> pipeline metadataReady() call to signify the return\n> Request metdata is ready to be consumed. Instead, replace this with a\n> new pipeline -> IPA reportMetadata() call that explicitly ask the IPA\n> to fill in the Request metdata at an appropriate time.\n>\n> This change allows the pipeline handler to dictate when it receives the\n> metadata from the IPA if the structure of the pipeline were to ever\n> change.\n>\n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 2 ++\n> src/ipa/rpi/vc4/raspberrypi.cpp | 9 ++++-----\n> src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp | 18 +++++++-----------\n> 3 files changed, 13 insertions(+), 16 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 7d56248f4ab3..620f13ba9717 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -82,6 +82,8 @@ interface IPARPiInterface {\n>\n> \t[async] prepareIsp(PrepareParams params);\n> \t[async] processStats(ProcessParams params);\n> +\n> +\treportMetadata(uint32 ipaContext) => (libcamera.ControlList controls);\n> };\n>\n> interface IPARPiEventInterface {\n> diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> index d737f1d662a0..e3ca8e2f7cbe 100644\n> --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> @@ -154,7 +154,7 @@ private:\n> \tbool validateLensControls();\n> \tvoid applyControls(const ControlList &controls);\n> \tvoid prepare(const PrepareParams ¶ms);\n> -\tvoid reportMetadata(unsigned int ipaContext);\n> +\tvoid reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> \tvoid fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> \tRPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> \tvoid process(unsigned int bufferId, unsigned int ipaContext);\n> @@ -565,7 +565,6 @@ void IPARPi::processStats(const ProcessParams ¶ms)\n> \tif (processPending_ && frameCount_ > mistrustCount_)\n> \t\tprocess(params.buffers.stats, context);\n>\n> -\treportMetadata(context);\n> \tprocessStatsComplete.emit(params.buffers);\n> }\n>\n> @@ -586,9 +585,9 @@ void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> \tprepareIspComplete.emit(params.buffers);\n> }\n>\n> -void IPARPi::reportMetadata(unsigned int ipaContext)\n> +void IPARPi::reportMetadata(unsigned int ipaContext, ControlList *controls)\n> {\n> -\tRPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> +\tRPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext % rpiMetadata_.size()];\n> \tstd::unique_lock lock(rpiMetadata);\n>\n> \t/*\n> @@ -697,7 +696,7 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> \t\tlibcameraMetadata_.set(controls::AfPauseState, p);\n> \t}\n>\n> -\tmetadataReady.emit(libcameraMetadata_);\n> +\t*controls = std::move(libcameraMetadata_);\n> }\n>\n> bool IPARPi::validateSensorControls()\n> diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> index 30ce6feab0d1..bd66468683df 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> @@ -207,7 +207,6 @@ public:\n> \tvoid enumerateVideoDevices(MediaLink *link);\n>\n> \tvoid processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> -\tvoid metadataReady(const ControlList &metadata);\n> \tvoid prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> \tvoid setIspControls(const ControlList &controls);\n> \tvoid setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> @@ -1652,7 +1651,6 @@ int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n>\n> \tipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> \tipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> -\tipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> \tipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> \tipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> \tipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> @@ -1892,17 +1890,12 @@ void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> \tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n>\n> \thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> -\tstate_ = State::IpaComplete;\n> -\thandleState();\n> -}\n>\n> -void RPiCameraData::metadataReady(const ControlList &metadata)\n> -{\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> -\t/* Add to the Request metadata buffer what the IPA has provided. */\n> +\t/* Last thing to do is to fill up the request metadata. */\n> \tRequest *request = requestQueue_.front();\n> +\tControlList metadata;\n> +\n> +\tipa_->reportMetadata(request->sequence(), &metadata);\n> \trequest->metadata().merge(metadata);\n>\n> \t/*\n> @@ -1923,6 +1916,9 @@ void RPiCameraData::metadataReady(const ControlList &metadata)\n>\n> \t\tsensor_->setControls(&ctrls);\n> \t}\n> +\n> +\tstate_ = State::IpaComplete;\n> +\thandleState();\n> }\n>\n> void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> --\n> 2.34.1\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 37622BDCBD\n\tfor ;\n\tWed, 26 Apr 2023 16:18:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68201627D4;\n\tWed, 26 Apr 2023 18:18:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7454C627D4\n\tfor ;\n\tWed, 26 Apr 2023 18:18:47 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE94EC7E;\n\tWed, 26 Apr 2023 18:18:35 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682525928;\n\tbh=dkeJDLkjVU5apcTWbi93pqWT2wiGgBSIHIuq60rg26c=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=quRwbH0kNRSA0o8oPfJ+//3Apx0cmcrDk7hVZrY7DSqWzc5fi6OPRA3T7Z4ULZTTR\n\tHwLu37AmdXAEkmrxVKMLDA67kVdfTS9ASHExgj/PSmru1RKEJ8DTBRVVtTCVcOv7bw\n\tysmTo0r3aCjBPT8sAfYF3QBsZjbvgZ0ccII7YNVLrCxOAFa/nf7cl0MBTedlI+/Fwc\n\tj016Trz2WxmpHtDLmetQiUcoTcvElrjdtxWopnxXEySIyL2Rq+ZcPez4WwlOxbN2Hf\n\tKOGynNA739y0KDzxjcyq78S15REybNHHEQplRvrJoEBBsUcQv+qo9noz+bRwFt8t1y\n\tmZlCEBSwWM2Xw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682525916;\n\tbh=dkeJDLkjVU5apcTWbi93pqWT2wiGgBSIHIuq60rg26c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n7ErcrAW1z3mHvlMbw+8hXaWT+jUL1kPAmpv/W8kbYao8Wdhi7me846E9KTcdcnrx\n\t1o47UeajVK539TD+lyMpHMGi+UZNjVQcK3i6eoD/a0273NBfwuxJClfQlJX4cov9yp\n\tVk7YIfA1yus0gr64leL4t/kIqPvhjd0X5bM3WIMY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"n7ErcrAW\"; dkim-atps=neutral","Date":"Wed, 26 Apr 2023 18:18:44 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-8-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-8-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 07/13] pipeline: ipa: raspberrypi:\n\tReplace IPA metadataReady() call","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26974,"web_url":"https://patchwork.libcamera.org/comment/26974/","msgid":"<20230427150224.GH28489@pendragon.ideasonboard.com>","date":"2023-04-27T15:02:24","subject":"Re: [libcamera-devel] [PATCH 07/13] pipeline: ipa: raspberrypi:\n\tReplace IPA metadataReady() call","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Wed, Apr 26, 2023 at 02:10:51PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Remove the IPA -> pipeline metadataReady() call to signify the return\n> Request metdata is ready to be consumed. Instead, replace this with a\n> new pipeline -> IPA reportMetadata() call that explicitly ask the IPA\n> to fill in the Request metdata at an appropriate time.\n> \n> This change allows the pipeline handler to dictate when it receives the\n> metadata from the IPA if the structure of the pipeline were to ever\n> change.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 2 ++\n> src/ipa/rpi/vc4/raspberrypi.cpp | 9 ++++-----\n> src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp | 18 +++++++-----------\n> 3 files changed, 13 insertions(+), 16 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 7d56248f4ab3..620f13ba9717 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -82,6 +82,8 @@ interface IPARPiInterface {\n> \n> \t[async] prepareIsp(PrepareParams params);\n> \t[async] processStats(ProcessParams params);\n> +\n> +\treportMetadata(uint32 ipaContext) => (libcamera.ControlList controls);\n\nI'm afraid this can't be done. While streaming, only asynchronous calls\nare allowed, synchronous calls may block for too long and have adverse\neffects on the real time requirements on both sides. This is documented\nin Documentation/guides/ipa.rst:\n\n----\nIn addition, any call made after start() and before stop() must be\nasynchronous. The motivation for this is to avoid damaging real-time\nperformance of the pipeline handler. If the pipeline handler wants some data\nfrom the IPA, the IPA should return the data asynchronously via an event\n(see \"The Event IPA interface\").\n----\n\n> };\n> \n> interface IPARPiEventInterface {\n> diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> index d737f1d662a0..e3ca8e2f7cbe 100644\n> --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> @@ -154,7 +154,7 @@ private:\n> \tbool validateLensControls();\n> \tvoid applyControls(const ControlList &controls);\n> \tvoid prepare(const PrepareParams ¶ms);\n> -\tvoid reportMetadata(unsigned int ipaContext);\n> +\tvoid reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> \tvoid fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> \tRPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> \tvoid process(unsigned int bufferId, unsigned int ipaContext);\n> @@ -565,7 +565,6 @@ void IPARPi::processStats(const ProcessParams ¶ms)\n> \tif (processPending_ && frameCount_ > mistrustCount_)\n> \t\tprocess(params.buffers.stats, context);\n> \n> -\treportMetadata(context);\n> \tprocessStatsComplete.emit(params.buffers);\n> }\n> \n> @@ -586,9 +585,9 @@ void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> \tprepareIspComplete.emit(params.buffers);\n> }\n> \n> -void IPARPi::reportMetadata(unsigned int ipaContext)\n> +void IPARPi::reportMetadata(unsigned int ipaContext, ControlList *controls)\n> {\n> -\tRPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> +\tRPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext % rpiMetadata_.size()];\n> \tstd::unique_lock lock(rpiMetadata);\n> \n> \t/*\n> @@ -697,7 +696,7 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> \t\tlibcameraMetadata_.set(controls::AfPauseState, p);\n> \t}\n> \n> -\tmetadataReady.emit(libcameraMetadata_);\n> +\t*controls = std::move(libcameraMetadata_);\n> }\n> \n> bool IPARPi::validateSensorControls()\n> diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> index 30ce6feab0d1..bd66468683df 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> @@ -207,7 +207,6 @@ public:\n> \tvoid enumerateVideoDevices(MediaLink *link);\n> \n> \tvoid processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> -\tvoid metadataReady(const ControlList &metadata);\n> \tvoid prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> \tvoid setIspControls(const ControlList &controls);\n> \tvoid setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> @@ -1652,7 +1651,6 @@ int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n> \n> \tipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> \tipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> -\tipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> \tipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> \tipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> \tipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> @@ -1892,17 +1890,12 @@ void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> \tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> \n> \thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> -\tstate_ = State::IpaComplete;\n> -\thandleState();\n> -}\n> \n> -void RPiCameraData::metadataReady(const ControlList &metadata)\n> -{\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> -\t/* Add to the Request metadata buffer what the IPA has provided. */\n> +\t/* Last thing to do is to fill up the request metadata. */\n> \tRequest *request = requestQueue_.front();\n> +\tControlList metadata;\n> +\n> +\tipa_->reportMetadata(request->sequence(), &metadata);\n> \trequest->metadata().merge(metadata);\n> \n> \t/*\n> @@ -1923,6 +1916,9 @@ void RPiCameraData::metadataReady(const ControlList &metadata)\n> \n> \t\tsensor_->setControls(&ctrls);\n> \t}\n> +\n> +\tstate_ = State::IpaComplete;\n> +\thandleState();\n> }\n> \n> void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)","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 59183C0DA4\n\tfor ;\n\tThu, 27 Apr 2023 15:02:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B72F6627E0;\n\tThu, 27 Apr 2023 17:02:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D79B3627B7\n\tfor ;\n\tThu, 27 Apr 2023 17:02:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E812D802;\n\tThu, 27 Apr 2023 17:02:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682607734;\n\tbh=rszbuvjbWdQNwotmFW2vqofe5smpz9VtGjzCxG7KdQQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=WdFFYMgdWuLZJC7G7uSE6jmtK/b6HoIxzidMBLtMM3lOtzgT5nG8Fx+vo9yz5EJBi\n\twrBLSZJ8dGrFwbRkpJ8Jp7o6me6uEN7IkK0ihyCANk2Jn4Um69q6cMW/59heNK5bjI\n\tCqQm5TK95Yj4MSGDKbHL7D0wFnmSw6sQbuKfLmJSP75vk4bECMGk85kqwijxLPwAuo\n\tw7ycQRlsqiYQHlxdQfSGMedn/yBs5I2XxGdgkMQWbc2fBaAzddwgKCx+ZoMkUGeyZz\n\t3FeigkJga7FlGUVuvPnSQ0yjM5s5h1sUmZ2lA/hBQB6oxLcYLYbQHnGMbzlOIk6uin\n\tGBSxJpSsKQC1Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682607721;\n\tbh=rszbuvjbWdQNwotmFW2vqofe5smpz9VtGjzCxG7KdQQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wLcpUGXLPqNKCp9tDop26DL0wQHulowBAnOSiaj0ri6NmbrasGFMKK+EpyXdl9JOS\n\t+C28pzXKtWpCrXcqVYpSp+gBZHXEzaDhws0sl1uADnquUF/1MLuX5R5A3Mrqb+4MOr\n\tGR8u4Yp+4f9A+vXvLbrqvgmwhra9dkTV7nRkDr8Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wLcpUGXL\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 18:02:24 +0300","To":"Naushir Patuck ","Message-ID":"<20230427150224.GH28489@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-8-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-8-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 07/13] pipeline: ipa: raspberrypi:\n\tReplace IPA metadataReady() call","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26975,"web_url":"https://patchwork.libcamera.org/comment/26975/","msgid":"","date":"2023-04-27T15:21:15","subject":"Re: [libcamera-devel] [PATCH 07/13] pipeline: ipa: raspberrypi:\n\tReplace IPA metadataReady() call","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 27 Apr 2023 at 16:02, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 26, 2023 at 02:10:51PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Remove the IPA -> pipeline metadataReady() call to signify the return\n> > Request metdata is ready to be consumed. Instead, replace this with a\n> > new pipeline -> IPA reportMetadata() call that explicitly ask the IPA\n> > to fill in the Request metdata at an appropriate time.\n> >\n> > This change allows the pipeline handler to dictate when it receives the\n> > metadata from the IPA if the structure of the pipeline were to ever\n> > change.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 2 ++\n> > src/ipa/rpi/vc4/raspberrypi.cpp | 9 ++++-----\n> > src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp | 18 +++++++-----------\n> > 3 files changed, 13 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > index 7d56248f4ab3..620f13ba9717 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -82,6 +82,8 @@ interface IPARPiInterface {\n> >\n> > [async] prepareIsp(PrepareParams params);\n> > [async] processStats(ProcessParams params);\n> > +\n> > + reportMetadata(uint32 ipaContext) => (libcamera.ControlList controls);\n>\n> I'm afraid this can't be done. While streaming, only asynchronous calls\n> are allowed, synchronous calls may block for too long and have adverse\n> effects on the real time requirements on both sides.\n\nOops, that's not great. I'm going to have to think of how to get around this!\n\nNaush\n\n\n> This is documented in Documentation/guides/ipa.rst:\n>\n> ----\n> In addition, any call made after start() and before stop() must be\n> asynchronous. The motivation for this is to avoid damaging real-time\n> performance of the pipeline handler. If the pipeline handler wants some data\n> from the IPA, the IPA should return the data asynchronously via an event\n> (see \"The Event IPA interface\").\n> ----\n>\n> > };\n> >\n> > interface IPARPiEventInterface {\n> > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > index d737f1d662a0..e3ca8e2f7cbe 100644\n> > --- a/src/ipa/rpi/vc4/raspberrypi.cpp\n> > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp\n> > @@ -154,7 +154,7 @@ private:\n> > bool validateLensControls();\n> > void applyControls(const ControlList &controls);\n> > void prepare(const PrepareParams ¶ms);\n> > - void reportMetadata(unsigned int ipaContext);\n> > + void reportMetadata(unsigned int ipaContext, ControlList *controls) override;\n> > void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> > void process(unsigned int bufferId, unsigned int ipaContext);\n> > @@ -565,7 +565,6 @@ void IPARPi::processStats(const ProcessParams ¶ms)\n> > if (processPending_ && frameCount_ > mistrustCount_)\n> > process(params.buffers.stats, context);\n> >\n> > - reportMetadata(context);\n> > processStatsComplete.emit(params.buffers);\n> > }\n> >\n> > @@ -586,9 +585,9 @@ void IPARPi::prepareIsp(const PrepareParams ¶ms)\n> > prepareIspComplete.emit(params.buffers);\n> > }\n> >\n> > -void IPARPi::reportMetadata(unsigned int ipaContext)\n> > +void IPARPi::reportMetadata(unsigned int ipaContext, ControlList *controls)\n> > {\n> > - RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];\n> > + RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext % rpiMetadata_.size()];\n> > std::unique_lock lock(rpiMetadata);\n> >\n> > /*\n> > @@ -697,7 +696,7 @@ void IPARPi::reportMetadata(unsigned int ipaContext)\n> > libcameraMetadata_.set(controls::AfPauseState, p);\n> > }\n> >\n> > - metadataReady.emit(libcameraMetadata_);\n> > + *controls = std::move(libcameraMetadata_);\n> > }\n> >\n> > bool IPARPi::validateSensorControls()\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > index 30ce6feab0d1..bd66468683df 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp\n> > @@ -207,7 +207,6 @@ public:\n> > void enumerateVideoDevices(MediaLink *link);\n> >\n> > void processStatsComplete(const ipa::RPi::BufferIds &buffers);\n> > - void metadataReady(const ControlList &metadata);\n> > void prepareIspComplete(const ipa::RPi::BufferIds &buffers);\n> > void setIspControls(const ControlList &controls);\n> > void setDelayedControls(const ControlList &controls, uint32_t delayContext);\n> > @@ -1652,7 +1651,6 @@ int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)\n> >\n> > ipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);\n> > ipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);\n> > - ipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);\n> > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> > ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);\n> > @@ -1892,17 +1890,12 @@ void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> >\n> > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > - state_ = State::IpaComplete;\n> > - handleState();\n> > -}\n> >\n> > -void RPiCameraData::metadataReady(const ControlList &metadata)\n> > -{\n> > - if (!isRunning())\n> > - return;\n> > -\n> > - /* Add to the Request metadata buffer what the IPA has provided. */\n> > + /* Last thing to do is to fill up the request metadata. */\n> > Request *request = requestQueue_.front();\n> > + ControlList metadata;\n> > +\n> > + ipa_->reportMetadata(request->sequence(), &metadata);\n> > request->metadata().merge(metadata);\n> >\n> > /*\n> > @@ -1923,6 +1916,9 @@ void RPiCameraData::metadataReady(const ControlList &metadata)\n> >\n> > sensor_->setControls(&ctrls);\n> > }\n> > +\n> > + state_ = State::IpaComplete;\n> > + handleState();\n> > }\n> >\n> > void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\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 73CB5BDCBD\n\tfor ;\n\tThu, 27 Apr 2023 15:21:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8954627DF;\n\tThu, 27 Apr 2023 17:21:33 +0200 (CEST)","from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com\n\t[IPv6:2607:f8b0:4864:20::b30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 386D7627B7\n\tfor ;\n\tThu, 27 Apr 2023 17:21:31 +0200 (CEST)","by mail-yb1-xb30.google.com with SMTP id\n\t3f1490d57ef6-b8f32969ab0so15255796276.2\n\tfor ;\n\tThu, 27 Apr 2023 08:21:31 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682608893;\n\tbh=+QdJ95FWG7NRZ3/DBj1ibGfsXZgQGa3XCb3FY234MMI=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=fDC2GzQV55bCw3v74xsKtkqr5whspzbhYpxqM/qfxRzG8/fkhRp+kYHvuusIeHVUE\n\tYYvu7oVlfIv8TJCHgO18/NrW3R94AgWVCPWtlLeuVewalMiYILt62EuPJ9wlzmaAN7\n\tkPQsHNweTUJdZ5XVBwhNE/8Mt5JXT5pbM0RhUdwhEmmsqdaJusVns6cSRh4pVgj1kv\n\tA2UxIMW0ic7OOBAA6xogQOdeiXnddwn/ISuE/MbVnP3lB9cWDAaVVVlr5LNKW/hOd6\n\temQxB0cEoA98aZ04opi9umhD/pU38dfc+0s2AWIe0hjJnmVSXEL705jWzLBqheEWNZ\n\thtjl9I2FUzJQQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1682608890; x=1685200890;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=fgI7ZIvXgmDCIiIo45cQE5HbV+npE5iWHwEXB1LztBk=;\n\tb=H4kjWnNb9ylFy3wBtpXr422GAbb8sF+hhnhXyxzOsECCD91T0hIkvUekLox3zIpq+6\n\teBLLbavUXz4IAwsVTVL2BZRVOQqMANjhEFcD/fvXP1mjNOb6a8xz32uFBjr8CgVW0k2g\n\tw6VuV2rGW9JmSu7L3ZTMyXm1yoresB9Dj2wTWvtb5mtwaES2ubt0EFLFnnN6Uw+px7kK\n\tQBUSfAzmnAH7cLTgRmhb/B9S6o6I6jQYdbCCC/5W/4QB9WKdZ5ByFUMBieCHGjZNfPPx\n\tem8tPpWknMmJL0jI4vaWZVwSMAtnw0rDI6dKb1iugRJae7ER8gJkEEuGpy3jRez7rVlE\n\tlk1w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"H4kjWnNb\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682608890; x=1685200890;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=fgI7ZIvXgmDCIiIo45cQE5HbV+npE5iWHwEXB1LztBk=;\n\tb=C/Gh1OfPD4MKy9k/rioIgEgyDVEbzPPiCmsrnrG7U3TnHz/C/l19qEEDaE+Zi+l96U\n\tDN3oa48Gpzpe5dijFOkseqZsFOQ6geXJ6HGJ5xPcO/zuQzWJp0rsSJk255UYREnDvZYh\n\tjk5bkIabFFDizTSwXVBbpxwc0QOO1ylQev4e16L9FkB6hOq7/Tbpy6ywL5CIo3e61qX3\n\tMv2HrOVuH30Jq3gOywqs6Tm/WuT4JuczJsQaMrvrkSFOboH3LUHlVqUquZDpp0Ccn+vl\n\tVZ9t4PY8PfCLsV43ziLTDZu0daxBujPxOlFrDbL1VfDQfsq0uhgzNjzkwk8ReevAI81h\n\txcug==","X-Gm-Message-State":"AC+VfDxPZlWFtuzkNZTzhP1SlUuCGgtlRq1/gNqaOGMZMpEDX0u3a1TQ\n\tM+uhf7fpb0QQIF+tgmJpqoDZVIvkvKz0dJZPGLrcsQ==","X-Google-Smtp-Source":"ACHHUZ7t4bm/wCCxXib/YgIleAqMQvcpbBR9WZB3wHOWpVFlRttcbl+NMRT7e3hHpVTA91DtlRaZeKToQTjxhfLkv9o=","X-Received":"by 2002:a25:58c4:0:b0:b99:e0ff:5f16 with SMTP id\n\tm187-20020a2558c4000000b00b99e0ff5f16mr1134080ybb.18.1682608889969;\n\tThu, 27 Apr 2023 08:21:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-8-naush@raspberrypi.com>\n\t<20230427150224.GH28489@pendragon.ideasonboard.com>","In-Reply-To":"<20230427150224.GH28489@pendragon.ideasonboard.com>","Date":"Thu, 27 Apr 2023 16:21:15 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 07/13] pipeline: ipa: raspberrypi:\n\tReplace IPA metadataReady() call","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]