[{"id":26943,"web_url":"https://patchwork.libcamera.org/comment/26943/","msgid":"","date":"2023-04-26T16:24:46","subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","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:57PM +0100, Naushir Patuck via libcamera-devel wrote:\n> We currently rely on a state check to see if any of the IPA and buffer\n> dequeue signal functions need to run. Replace this check by explicitly\n> disconnecting the appropriate signals on camera stop. Re-connect the\n> signals on camera start.\n>\n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++---------------\n> 1 file changed, 23 insertions(+), 31 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index bd7bfb3a7663..4b3f5a7fc9fe 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n>\n> \t/* An embedded data node will not be present if the sensor does not support it. */\n> \tMediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n> -\tif (unicamEmbedded) {\n> +\tif (unicamEmbedded)\n> \t\tdata->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> -\t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data,\n> -\t\t\t\t\t\t\t\t\t &Vc4CameraData::unicamBufferDequeue);\n> -\t}\n>\n> \t/* Tag the ISP input stream as an import stream. */\n> \tdata->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, Flag::ImportOnly);\n> @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> \tdata->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> \tdata->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n>\n> -\t/* Wire up all the buffer connections. */\n> -\tdata->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue);\n> -\tdata->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue);\n> -\tdata->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> -\tdata->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> -\tdata->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> -\n> \tif (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) {\n> \t\tLOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> \t\tdata->sensorMetadata_ = false;\n> -\t\tif (data->unicam_[Unicam::Embedded].dev())\n> -\t\t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> \t}\n>\n> \t/*\n> @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> \t\treturn -EINVAL;\n> \t}\n>\n> -\t/* Write up all the IPA connections. */\n> -\tdata->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete);\n> -\tdata->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete);\n> +\t/* Wire up the default IPA connections. The others get connected on start() */\n> \tdata->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls);\n> \tdata->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout);\n>\n> @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)\n>\n> void Vc4CameraData::platformStart()\n> {\n> +\tunicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> +\tisp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue);\n> +\tisp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> +\tisp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> +\tisp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> +\tipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete);\n> +\tipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete);\n> +\n> +\tif (sensorMetadata_)\n> +\t\tunicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> }\n>\n> void Vc4CameraData::platformStop()\n> {\n> +\tunicam_[Unicam::Image].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Input].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Output0].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Output1].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Stats].dev()->bufferReady.disconnect();\n> +\tipa_->processStatsComplete.disconnect();\n> +\tipa_->prepareIspComplete.disconnect();\n> +\n> +\tif (sensorMetadata_)\n> +\t\tunicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> +\n> \tbayerQueue_ = {};\n> \tembeddedQueue_ = {};\n> }\n> @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \tRPi::Stream *stream = nullptr;\n> \tunsigned int index;\n>\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tfor (RPi::Stream &s : unicam_) {\n> \t\tindex = s.getBufferId(buffer);\n> \t\tif (index) {\n> @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n>\n> void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer)\n> {\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tLOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> \t\t\t<< \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \tRPi::Stream *stream = nullptr;\n> \tunsigned int index;\n>\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tfor (RPi::Stream &s : isp_) {\n> \t\tindex = s.getBufferId(buffer);\n> \t\tif (index) {\n> @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n>\n> void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> {\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n>\n> \thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> \tunsigned int bayer = buffers.bayer & RPi::MaskID;\n> \tFrameBuffer *buffer;\n>\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tbuffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> \tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\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 3FFBDC3272\n\tfor ;\n\tWed, 26 Apr 2023 16:24:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D243627E0;\n\tWed, 26 Apr 2023 18:24:51 +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 78082627D4\n\tfor ;\n\tWed, 26 Apr 2023 18:24:49 +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 F0F9FD8B;\n\tWed, 26 Apr 2023 18:24:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682526291;\n\tbh=LRH10eTNoJXrTj9S5rMIFyDD57qCpo6Z4YBbAF3o6Ns=;\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=iKVRJBZ2GcJRvskBQ6l9kA6r1Kxp5Jc9yKL6QHCtFBPB0faZyk+BCjZ+6c4bjBE25\n\tAcCV842WI6mlE7gK2kc3IfCvdV30cKdDSjszeiaklZHuaM9wunnYPTIyBMR9qjePQ7\n\t/shNahbaeM81F4erb0PTu9V/qO23pV4xj5Mv48EITqtA0FMM26nK17TnIpbbjxrsb9\n\tqTMIZ7DMZNlCNFHgR/GHYOLuMAmxVHTvrvPGpSyYPbPzwGU1WPYqlDjZI+o12DNG5G\n\tU/0ejH53Dw3WF/WqevxkzV1iyn+6wsReROShoirzFFelO7XRr3Yl+2jUt+TJvPurJC\n\txZaW/udyE5i6g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682526278;\n\tbh=LRH10eTNoJXrTj9S5rMIFyDD57qCpo6Z4YBbAF3o6Ns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=npdylBq4ntQloODAfcxludUq676wQTf9gBJ3gQHmYgfSilvQeiWnkfKmNaj+f+2VC\n\tC1XatOZVAogfNH1Q3MInFhhuqCDlVXLeAfR4+lOnL4Ks5lcE878TtZi0vvn/dMq4k2\n\tZ3AM4jK/wQHk90XMv9pFQnCp75ci5Gipw+EirmhM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"npdylBq4\"; dkim-atps=neutral","Date":"Wed, 26 Apr 2023 18:24:46 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-14-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-14-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","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":26967,"web_url":"https://patchwork.libcamera.org/comment/26967/","msgid":"<20230427135518.GC28489@pendragon.ideasonboard.com>","date":"2023-04-27T13:55:18","subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote:\n> We currently rely on a state check to see if any of the IPA and buffer\n> dequeue signal functions need to run. Replace this check by explicitly\n> disconnecting the appropriate signals on camera stop. Re-connect the\n> signals on camera start.\n\nI'm a bit concerned about this. I've debugged an issue no later than\ntoday where a race condition caused events to be processed after a stop\ncall. I briefly considered disabling the event notifier at stop time\n(that's equivalent to disconnecting the signal here), but I then\nrealized that a quick stop/start cycle would reenable the notifier\nbefore the event loop would get a chance to process and drop the event.\nDisconnecting signals to ignore events makes state handling implicit,\nand that in turn makes it more difficult to prove correctness of the\ncode. The resulting race conditions are also likely harder to debug as\nthey will occur less often.\n\n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++---------------\n> 1 file changed, 23 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index bd7bfb3a7663..4b3f5a7fc9fe 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> \n> \t/* An embedded data node will not be present if the sensor does not support it. */\n> \tMediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n> -\tif (unicamEmbedded) {\n> +\tif (unicamEmbedded)\n> \t\tdata->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> -\t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data,\n> -\t\t\t\t\t\t\t\t\t &Vc4CameraData::unicamBufferDequeue);\n> -\t}\n> \n> \t/* Tag the ISP input stream as an import stream. */\n> \tdata->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, Flag::ImportOnly);\n> @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> \tdata->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> \tdata->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> \n> -\t/* Wire up all the buffer connections. */\n> -\tdata->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue);\n> -\tdata->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue);\n> -\tdata->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> -\tdata->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> -\tdata->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> -\n> \tif (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) {\n> \t\tLOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> \t\tdata->sensorMetadata_ = false;\n> -\t\tif (data->unicam_[Unicam::Embedded].dev())\n> -\t\t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> \t}\n> \n> \t/*\n> @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\t/* Write up all the IPA connections. */\n> -\tdata->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete);\n> -\tdata->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete);\n> +\t/* Wire up the default IPA connections. The others get connected on start() */\n> \tdata->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls);\n> \tdata->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout);\n> \n> @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)\n> \n> void Vc4CameraData::platformStart()\n> {\n> +\tunicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> +\tisp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue);\n> +\tisp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> +\tisp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> +\tisp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> +\tipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete);\n> +\tipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete);\n> +\n> +\tif (sensorMetadata_)\n> +\t\tunicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> }\n> \n> void Vc4CameraData::platformStop()\n> {\n> +\tunicam_[Unicam::Image].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Input].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Output0].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Output1].dev()->bufferReady.disconnect();\n> +\tisp_[Isp::Stats].dev()->bufferReady.disconnect();\n> +\tipa_->processStatsComplete.disconnect();\n> +\tipa_->prepareIspComplete.disconnect();\n> +\n> +\tif (sensorMetadata_)\n> +\t\tunicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> +\n> \tbayerQueue_ = {};\n> \tembeddedQueue_ = {};\n> }\n> @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \tRPi::Stream *stream = nullptr;\n> \tunsigned int index;\n> \n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tfor (RPi::Stream &s : unicam_) {\n> \t\tindex = s.getBufferId(buffer);\n> \t\tif (index) {\n> @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \n> void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer)\n> {\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tLOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> \t\t\t<< \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \tRPi::Stream *stream = nullptr;\n> \tunsigned int index;\n> \n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tfor (RPi::Stream &s : isp_) {\n> \t\tindex = s.getBufferId(buffer);\n> \t\tif (index) {\n> @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \n> void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> {\n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> \n> \thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> \tunsigned int bayer = buffers.bayer & RPi::MaskID;\n> \tFrameBuffer *buffer;\n> \n> -\tif (!isRunning())\n> -\t\treturn;\n> -\n> \tbuffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> \tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;","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 643EEC0DA4\n\tfor ;\n\tThu, 27 Apr 2023 13:55:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6550627DF;\n\tThu, 27 Apr 2023 15:55:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01F53627B7\n\tfor ;\n\tThu, 27 Apr 2023 15:55:07 +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 CB1DB802;\n\tThu, 27 Apr 2023 15:54:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682603709;\n\tbh=/n1VkvL5YUj/LK+9uwiXhpjl0yjFT/vkpdq3ArPe71A=;\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=iMbqx8xQ8RWhhSHyUGArIHFJgMerGspPqPwbg7jJKFyvieC6gqwwRtAWX1HrXLRrw\n\tCsp1ytvC1wSLp3XL/3rHlZYNZlPV6+2/vp7kl0YVFBkCy8uLKS68y3kKcAJELyfVSw\n\tJQY3avPczkG83pkJLKQPff+tQsMw1K1lltV/SQOt1UmaWIqJeg3iLJRafvmfsrmoBf\n\tIIa6lzVl2z7UTNU4eOC/j6tdChsl7IoMAv4NZjKfdtsUPQolIgcM6VSASlDZANbLsu\n\tlpei+eiH8gNSaFE1KJJmuXBXnD+FwJZW5+9i+rWrWPQx81/vKiDlYauUHlnso6VA11\n\trhvcTZTzBhEtw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682603695;\n\tbh=/n1VkvL5YUj/LK+9uwiXhpjl0yjFT/vkpdq3ArPe71A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CAjG0E3rrLxsb77NnPSz5xc6Kle9gLCLDBXkw31LEkBbDcgjxJ46PtPdUikwKKHGx\n\tey0RH1IMLe6kaNAfPbJ1g3se84LTUdKBZJ6JVc0XarnaOQu2fjb8qN9qhhKWsn+jjb\n\ttz6rArwt81WYiLcIYRF/Ob7eg1RBb/0ihUOwa3/U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CAjG0E3r\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 16:55:18 +0300","To":"Naushir Patuck ","Message-ID":"<20230427135518.GC28489@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-14-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-14-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","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":26970,"web_url":"https://patchwork.libcamera.org/comment/26970/","msgid":"","date":"2023-04-27T14:03:44","subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Thu, 27 Apr 2023 at 14:55, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > We currently rely on a state check to see if any of the IPA and buffer\n> > dequeue signal functions need to run. Replace this check by explicitly\n> > disconnecting the appropriate signals on camera stop. Re-connect the\n> > signals on camera start.\n>\n> I'm a bit concerned about this. I've debugged an issue no later than\n> today where a race condition caused events to be processed after a stop\n> call. I briefly considered disabling the event notifier at stop time\n> (that's equivalent to disconnecting the signal here), but I then\n> realized that a quick stop/start cycle would reenable the notifier\n> before the event loop would get a chance to process and drop the event.\n> Disconnecting signals to ignore events makes state handling implicit,\n> and that in turn makes it more difficult to prove correctness of the\n> code. The resulting race conditions are also likely harder to debug as\n> they will occur less often.\n\nI can remove this patch and revert to using our original state test mechanism to\nhandle this.\n\nNaush\n\n>\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++---------------\n> > 1 file changed, 23 insertions(+), 31 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > index bd7bfb3a7663..4b3f5a7fc9fe 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> >\n> > /* An embedded data node will not be present if the sensor does not support it. */\n> > MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n> > - if (unicamEmbedded) {\n> > + if (unicamEmbedded)\n> > data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data,\n> > - &Vc4CameraData::unicamBufferDequeue);\n> > - }\n> >\n> > /* Tag the ISP input stream as an import stream. */\n> > data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, Flag::ImportOnly);\n> > @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> > data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> >\n> > - /* Wire up all the buffer connections. */\n> > - data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue);\n> > - data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue);\n> > - data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> > - data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> > - data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> > -\n> > if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) {\n> > LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > data->sensorMetadata_ = false;\n> > - if (data->unicam_[Unicam::Embedded].dev())\n> > - data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > }\n> >\n> > /*\n> > @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > return -EINVAL;\n> > }\n> >\n> > - /* Write up all the IPA connections. */\n> > - data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete);\n> > - data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete);\n> > + /* Wire up the default IPA connections. The others get connected on start() */\n> > data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls);\n> > data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout);\n> >\n> > @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)\n> >\n> > void Vc4CameraData::platformStart()\n> > {\n> > + unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> > + isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue);\n> > + isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> > + isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> > + isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> > + ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete);\n> > + ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete);\n> > +\n> > + if (sensorMetadata_)\n> > + unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> > }\n> >\n> > void Vc4CameraData::platformStop()\n> > {\n> > + unicam_[Unicam::Image].dev()->bufferReady.disconnect();\n> > + isp_[Isp::Input].dev()->bufferReady.disconnect();\n> > + isp_[Isp::Output0].dev()->bufferReady.disconnect();\n> > + isp_[Isp::Output1].dev()->bufferReady.disconnect();\n> > + isp_[Isp::Stats].dev()->bufferReady.disconnect();\n> > + ipa_->processStatsComplete.disconnect();\n> > + ipa_->prepareIspComplete.disconnect();\n> > +\n> > + if (sensorMetadata_)\n> > + unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > +\n> > bayerQueue_ = {};\n> > embeddedQueue_ = {};\n> > }\n> > @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > RPi::Stream *stream = nullptr;\n> > unsigned int index;\n> >\n> > - if (!isRunning())\n> > - return;\n> > -\n> > for (RPi::Stream &s : unicam_) {\n> > index = s.getBufferId(buffer);\n> > if (index) {\n> > @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> >\n> > void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer)\n> > {\n> > - if (!isRunning())\n> > - return;\n> > -\n> > LOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> > << \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n> > << \", timestamp: \" << buffer->metadata().timestamp;\n> > @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > RPi::Stream *stream = nullptr;\n> > unsigned int index;\n> >\n> > - if (!isRunning())\n> > - return;\n> > -\n> > for (RPi::Stream &s : isp_) {\n> > index = s.getBufferId(buffer);\n> > if (index) {\n> > @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n> >\n> > void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> > {\n> > - if (!isRunning())\n> > - return;\n> > -\n> > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> >\n> > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> > unsigned int bayer = buffers.bayer & RPi::MaskID;\n> > FrameBuffer *buffer;\n> >\n> > - if (!isRunning())\n> > - return;\n> > -\n> > buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> > LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> > << \", timestamp: \" << buffer->metadata().timestamp;\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 DAE90BDCBD\n\tfor ;\n\tThu, 27 Apr 2023 14:04:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93092627E0;\n\tThu, 27 Apr 2023 16:04:01 +0200 (CEST)","from mail-yw1-x1135.google.com (mail-yw1-x1135.google.com\n\t[IPv6:2607:f8b0:4864:20::1135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09943627B7\n\tfor ;\n\tThu, 27 Apr 2023 16:04:00 +0200 (CEST)","by mail-yw1-x1135.google.com with SMTP id\n\t00721157ae682-54f8d59a8a9so103869367b3.0\n\tfor ;\n\tThu, 27 Apr 2023 07:03:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682604241;\n\tbh=t6X69xH8QAKGolU9ykpgS2LwYXQaq7cuSjzTvlDMPP0=;\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=dRvMbZPowtZxpo1uTLSOWEitW+3E8YPvPvSIymTJ+2/Cy5V4hZxG/ot8G8jUY56M9\n\tKL3dmsH7LnwhMZfdrNrwIBWhiK8GT5cq1BTvHXRRo7TMtFgxIVbQJHi72b1zappSQk\n\tgBvguGtQVHHq6K8fhDOMhCI/5Oy3KGOQSM8NKC4jlRCeGFErORWWBH7Jldu5MBWsRm\n\tD6CGcvk2jbGVb/jVcVRsQmLZaLUtlq4a/7ziXVFHB1qxC4nb3X84JZh9gfDs/o/vkD\n\tmmlkmG6nwqZlMlS6ENJz6MQcMpw3umZMGoN1cTffoeUsqqAYsIXBZ2BxjCahKFntUj\n\tI54ZLgDPulQfg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1682604239; x=1685196239;\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=RgezhMWJ14FqnJszogZq3dWbnE71ZMOa/s1XWhXk/Fo=;\n\tb=Y6uFAGd0d4LhZAaTp60JDfDvV6fxH5eHqULLYnpRGntSQlHhgehbZIRfdgdWYj9t9u\n\tBszLPopB7VibkyFt7cs+cS8nCxgvXO8eXpch/FkBiTdGgvGRwnQ7lVxFpoI422d+GG4V\n\t44Cya9t1f9vdCBcar0itbjQIY1GFOz+Tu4xpz6Yf+0uOPBOS769mvTDTPCF8iUeNtmKz\n\tYyr2XUPSSiElb610nFBSv3s5NMmpGB96ACN5ybck769srqswASw6qGS7WuwBepr5IzJr\n\trs4tn55qsP+qpCrvnxXHmQrMysMKTo3rGL9TdHt1v9gwG9U1+yjbBWpigGJeRNG7/TVG\n\tgMlA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Y6uFAGd0\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1682604239; x=1685196239;\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=RgezhMWJ14FqnJszogZq3dWbnE71ZMOa/s1XWhXk/Fo=;\n\tb=Z7ysEd7eOWknfaROLlFbeaQtw4kyC7aErZf+jcydfoNPIaKkz9s53p+YJJH0ITEF2w\n\tuolhDjnPZU1KN/dPL4jF0//SLzAFHVHP/CBPwbxEDTdNj6PMl9ME7rmsREEczTQlY0E2\n\tMfCPYkR2Z9t9TI6dp3oxDymccFORpsGk3Z4lRw8Q1u15bzVhq9TuN26kC6EXeX671XYw\n\tMFREGv3wYF2dNCw7+0WBXImNaUTjqHhH0O62P7hezlLNqo8ZU3OAgwz5FpWvdgb4I7sk\n\tFkdy3EGx0/mDNqFxZ0jJbnVW0nIIxdBuPkBYGElbBUypdpdHRIAy/TKGx2WijBZj6MxI\n\t9qqg==","X-Gm-Message-State":"AC+VfDwKJsgWZ108EWwzsIKhitqgJufGI8kmKB4fQyEyWUjYJjjy2S+a\n\t330RGJIkBIHBQhbjne5JSD64Rw6ovknLyanxUHg3/jnNff9UYJPl3l/6qA==","X-Google-Smtp-Source":"ACHHUZ7q0ZKC2+J6ozUNE4je2D1U/TYmFDWFJV1Rk/dkTgYj1HCXUrri1nesLtmZHUTWrI66+nm45hnxavhB+Ifai7Q=","X-Received":"by 2002:a0d:ed07:0:b0:54f:4f1a:7ce0 with SMTP id\n\tw7-20020a0ded07000000b0054f4f1a7ce0mr1288263ywe.35.1682604238867;\n\tThu, 27 Apr 2023 07:03:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-14-naush@raspberrypi.com>\n\t<20230427135518.GC28489@pendragon.ideasonboard.com>","In-Reply-To":"<20230427135518.GC28489@pendragon.ideasonboard.com>","Date":"Thu, 27 Apr 2023 15:03:44 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","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\" "}},{"id":26973,"web_url":"https://patchwork.libcamera.org/comment/26973/","msgid":"<20230427145708.GG28489@pendragon.ideasonboard.com>","date":"2023-04-27T14:57:08","subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, Apr 27, 2023 at 03:03:44PM +0100, Naushir Patuck wrote:\n> On Thu, 27 Apr 2023 at 14:55, Laurent Pinchart wrote:\n> > On Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > > We currently rely on a state check to see if any of the IPA and buffer\n> > > dequeue signal functions need to run. Replace this check by explicitly\n> > > disconnecting the appropriate signals on camera stop. Re-connect the\n> > > signals on camera start.\n> >\n> > I'm a bit concerned about this. I've debugged an issue no later than\n> > today where a race condition caused events to be processed after a stop\n> > call. I briefly considered disabling the event notifier at stop time\n> > (that's equivalent to disconnecting the signal here), but I then\n> > realized that a quick stop/start cycle would reenable the notifier\n> > before the event loop would get a chance to process and drop the event.\n> > Disconnecting signals to ignore events makes state handling implicit,\n> > and that in turn makes it more difficult to prove correctness of the\n> > code. The resulting race conditions are also likely harder to debug as\n> > they will occur less often.\n> \n> I can remove this patch and revert to using our original state test\n> mechanism to handle this.\n\nI think that would be best, but if you're confident that this patch\ncan't cause any race condition or other problem, neither now nor when\ncombined with future changes, I'm not opposed to merging it. It's up to\nyou, whether you consider the issue valid or not.\n\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++---------------\n> > > 1 file changed, 23 insertions(+), 31 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > index bd7bfb3a7663..4b3f5a7fc9fe 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > >\n> > > /* An embedded data node will not be present if the sensor does not support it. */\n> > > MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n> > > - if (unicamEmbedded) {\n> > > + if (unicamEmbedded)\n> > > data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > > - data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data,\n> > > - &Vc4CameraData::unicamBufferDequeue);\n> > > - }\n> > >\n> > > /* Tag the ISP input stream as an import stream. */\n> > > data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, Flag::ImportOnly);\n> > > @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > > data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> > > data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > >\n> > > - /* Wire up all the buffer connections. */\n> > > - data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue);\n> > > - data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue);\n> > > - data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> > > - data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> > > - data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);\n> > > -\n> > > if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) {\n> > > LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > > data->sensorMetadata_ = false;\n> > > - if (data->unicam_[Unicam::Embedded].dev())\n> > > - data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > }\n> > >\n> > > /*\n> > > @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > > return -EINVAL;\n> > > }\n> > >\n> > > - /* Write up all the IPA connections. */\n> > > - data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete);\n> > > - data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete);\n> > > + /* Wire up the default IPA connections. The others get connected on start() */\n> > > data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls);\n> > > data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout);\n> > >\n> > > @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams ¶ms)\n> > >\n> > > void Vc4CameraData::platformStart()\n> > > {\n> > > + unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> > > + isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue);\n> > > + isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> > > + isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> > > + isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);\n> > > + ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete);\n> > > + ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete);\n> > > +\n> > > + if (sensorMetadata_)\n> > > + unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);\n> > > }\n> > >\n> > > void Vc4CameraData::platformStop()\n> > > {\n> > > + unicam_[Unicam::Image].dev()->bufferReady.disconnect();\n> > > + isp_[Isp::Input].dev()->bufferReady.disconnect();\n> > > + isp_[Isp::Output0].dev()->bufferReady.disconnect();\n> > > + isp_[Isp::Output1].dev()->bufferReady.disconnect();\n> > > + isp_[Isp::Stats].dev()->bufferReady.disconnect();\n> > > + ipa_->processStatsComplete.disconnect();\n> > > + ipa_->prepareIspComplete.disconnect();\n> > > +\n> > > + if (sensorMetadata_)\n> > > + unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > +\n> > > bayerQueue_ = {};\n> > > embeddedQueue_ = {};\n> > > }\n> > > @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > RPi::Stream *stream = nullptr;\n> > > unsigned int index;\n> > >\n> > > - if (!isRunning())\n> > > - return;\n> > > -\n> > > for (RPi::Stream &s : unicam_) {\n> > > index = s.getBufferId(buffer);\n> > > if (index) {\n> > > @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > >\n> > > void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer)\n> > > {\n> > > - if (!isRunning())\n> > > - return;\n> > > -\n> > > LOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> > > << \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n> > > << \", timestamp: \" << buffer->metadata().timestamp;\n> > > @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > > RPi::Stream *stream = nullptr;\n> > > unsigned int index;\n> > >\n> > > - if (!isRunning())\n> > > - return;\n> > > -\n> > > for (RPi::Stream &s : isp_) {\n> > > index = s.getBufferId(buffer);\n> > > if (index) {\n> > > @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > >\n> > > void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> > > {\n> > > - if (!isRunning())\n> > > - return;\n> > > -\n> > > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> > >\n> > > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> > > unsigned int bayer = buffers.bayer & RPi::MaskID;\n> > > FrameBuffer *buffer;\n> > >\n> > > - if (!isRunning())\n> > > - return;\n> > > -\n> > > buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> > > LOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> > > << \", timestamp: \" << buffer->metadata().timestamp;","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 672DBBDCBD\n\tfor ;\n\tThu, 27 Apr 2023 14:57:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E80AA627D6;\n\tThu, 27 Apr 2023 16:56:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89B20627B7\n\tfor ;\n\tThu, 27 Apr 2023 16:56:58 +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 C6139802;\n\tThu, 27 Apr 2023 16:56:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682607419;\n\tbh=33lVAIJbfBYy8HHhiX1m4EByDAfJ9kwSWAd4+xAlc1k=;\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=JbbRfVycVZnPa/XqtwS93ax//P7yRNW/18ZOBtR8UAgdKPrrK4JQzr7zY2ZjM3zf3\n\tE74UDOAfvra3UDxm3ZKkmHjfkRQdPBHetYntO5NbCB+WJjCj/HnmDYrf3Wkq0BHuUt\n\tdlMM2CQDv+k2sPku8y2Ml4/oaZ/sbOhL8kr84uD1H7hGn2NeTPVZjRAjK8MyoX1GMm\n\tJKhM5NxczPnZhTbgL33qalav6V5ytxoP1S3UA0P267bJGSVrIfcHUmmW7yi782zRhF\n\txio7tA83hIp5CQirtu8z6/G0pAc7rf7UUvBG9Kvxvl2nmrCs7fmtL5kHIz495EnKO0\n\tWC2Tvq1Z19KAg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682607406;\n\tbh=33lVAIJbfBYy8HHhiX1m4EByDAfJ9kwSWAd4+xAlc1k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qkKYzYltfaUEbDUVXr4tLBKjY10j6Am30r1zTLhRNqfEgyfeNbZUa47Z53FRTMbqh\n\tJC02gOFhMSzcJwv8BNVPuhWPE/GbMYAbbCBc/zE93J3MQOS5jYMC0iW3agQ4SQqMjL\n\tP4tk4Tyb8fDBpCdaWhdgnCQAfwBHn8D8sRsO9cGo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qkKYzYlt\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 17:57:08 +0300","To":"Naushir Patuck ","Message-ID":"<20230427145708.GG28489@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-14-naush@raspberrypi.com>\n\t<20230427135518.GC28489@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 13/13] pipeline: vc4:\n\tConnect/disconnect IPA and dequeue signals on start/stop","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\" "}}]