[{"id":36093,"web_url":"https://patchwork.libcamera.org/comment/36093/","msgid":"<175944317364.3917877.6504301274229284836@ping.linuxembedded.co.uk>","date":"2025-10-02T22:12:53","subject":"Re: [PATCH v1 01/33] libcamera: rkisp1: Only connect delayed\n\tcontrols at start/stop","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-09-30 13:26:22)\n> From: Kieran Bingham \n> \n> The RKISP1 path may potentially have multiple cameras connected through\n> complex pipelines such as video multiplexors or multiple FPGA paths.\n> \n> The RKISP1 pipeline handler notifies DelayedControls that a new frame is\n> commencing by using the frameStart event on the ISP and using that to\n> signal to DelayedControls that it is time to process the controls for\n> the next frame.\n> \n> When more than one camera is connected to an ISP it is important not to\n> signal events to an inactive Camera.\n> \n> Move the frameStart signal connection from CreateCamera() to start() and\n> introduce a corresponding disconnect at stop().\n> \n> Signed-off-by: Kieran Bingham \n\nRelaying the review comments from Laurent when I originally posted this:\n\n > When developing on an i.MX8MP, with multiple cameras connected to a\n > single ISP this caused a horrible bug which I eventually found using\n > UBSAN/ASAN to spot that actaully there was a use-after-free in calling\n > delayedControls on a camera object that wasn't active.\n\n I've been pondering about requiring the receiver of a signal to inherit\n from the Object class. That would solve this kind of issues.\n\n > One other alternative here, could be to move the connection of the\n > signal into the setFrameStartEnabled() call, if we pass in the\n > CameraData to be explicit on where we are connecting and disconnect. But\n > maybe that's overkill too.\n\n Conceptually, the isp_->frameStart signal is emitted by a resource\n shared by multiple cameras. It would make sense to model this with a\n slot in the PipelineHandlerRkISP1 class instead of the RkISP1CameraData\n class, and relay it to the delayed controls class through activeCamera_.\n Would that be a better model ?\n\n\nThat could be an option from a quick glance through.\n\n\n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ecd13831539f..605a0724615d 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1097,6 +1097,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> utils::ScopeExitActions actions;\n> int ret;\n> \n> + isp_->frameStart.connect(data->delayedCtrls_.get(),\n> + &DelayedControls::applyControls);\n> +\n> + actions += [&]() { isp_->frameStart.disconnect(data->delayedCtrls_.get()); };\n> +\n> /* Allocate buffers for internal pipeline usage. */\n> ret = allocateBuffers(camera);\n> if (ret)\n> @@ -1168,6 +1173,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> \n> isp_->setFrameStartEnabled(false);\n> \n> + isp_->frameStart.disconnect(data->delayedCtrls_.get());\n> +\n> data->ipa_->stop();\n> \n> if (hasSelfPath_)\n> @@ -1354,8 +1361,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> data->delayedCtrls_ =\n> std::make_unique(data->sensor_->device(),\n> params);\n> - isp_->frameStart.connect(data->delayedCtrls_.get(),\n> - &DelayedControls::applyControls);\n> \n> uint32_t supportedBlocks = kDefaultExtParamsBlocks;\n> \n> -- \n> 2.48.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 68ECAC328C\n\tfor ;\n\tThu, 2 Oct 2025 22:13:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46A126B5F3;\n\tFri, 3 Oct 2025 00:12:59 +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 DFF206B5AA\n\tfor ;\n\tFri, 3 Oct 2025 00:12:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8936282E;\n\tFri, 3 Oct 2025 00:11:26 +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=\"A8jgIuvO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1759443086;\n\tbh=wYWNIDI9Uhh7ey8vDyWHTVQ3YEGqmmpxUOvcz84b4AQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=A8jgIuvO1klk4miSVyz6VFt4CdyeVg/7jzYBZ+FDIzuponv2meAJRKQXmwNZHgyKG\n\tz06QuGfE9vjbssgBJfDXT3uf5STqW/7rhV/nRPcP0Apzm5ViApKJlfjaiyMzozE/qT\n\t3vrxjRbGU1dmdDeR6KWHRDG3bnt8KJgs4RqXDOUU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250930122726.1837524-2-stefan.klug@ideasonboard.com>","References":"<20250930122726.1837524-1-stefan.klug@ideasonboard.com>\n\t<20250930122726.1837524-2-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v1 01/33] libcamera: rkisp1: Only connect delayed\n\tcontrols at start/stop","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 02 Oct 2025 23:12:53 +0100","Message-ID":"<175944317364.3917877.6504301274229284836@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":36390,"web_url":"https://patchwork.libcamera.org/comment/36390/","msgid":"<176113930826.6155.17880459818612415702@localhost>","date":"2025-10-22T13:21:48","subject":"Re: [PATCH v1 01/33] libcamera: rkisp1: Only connect delayed\n\tcontrols at start/stop","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nQuoting Kieran Bingham (2025-10-03 00:12:53)\n> Quoting Stefan Klug (2025-09-30 13:26:22)\n> > From: Kieran Bingham \n> > \n> > The RKISP1 path may potentially have multiple cameras connected through\n> > complex pipelines such as video multiplexors or multiple FPGA paths.\n> > \n> > The RKISP1 pipeline handler notifies DelayedControls that a new frame is\n> > commencing by using the frameStart event on the ISP and using that to\n> > signal to DelayedControls that it is time to process the controls for\n> > the next frame.\n> > \n> > When more than one camera is connected to an ISP it is important not to\n> > signal events to an inactive Camera.\n> > \n> > Move the frameStart signal connection from CreateCamera() to start() and\n> > introduce a corresponding disconnect at stop().\n> > \n> > Signed-off-by: Kieran Bingham \n> \n> Relaying the review comments from Laurent when I originally posted this:\n> \n> > When developing on an i.MX8MP, with multiple cameras connected to a\n> > single ISP this caused a horrible bug which I eventually found using\n> > UBSAN/ASAN to spot that actaully there was a use-after-free in calling\n> > delayedControls on a camera object that wasn't active.\n> \n> I've been pondering about requiring the receiver of a signal to inherit\n> from the Object class. That would solve this kind of issues.\n> \n> > One other alternative here, could be to move the connection of the\n> > signal into the setFrameStartEnabled() call, if we pass in the\n> > CameraData to be explicit on where we are connecting and disconnect. But\n> > maybe that's overkill too.\n> \n> Conceptually, the isp_->frameStart signal is emitted by a resource\n> shared by multiple cameras. It would make sense to model this with a\n> slot in the PipelineHandlerRkISP1 class instead of the RkISP1CameraData\n> class, and relay it to the delayed controls class through activeCamera_.\n> Would that be a better model ?\n> \n> \n> That could be an option from a quick glance through.\n\nAs discussed, I moved this patch to the next series.\n\nBest regards,\nStefan\n\n> \n> \n> \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++++++--\n> > 1 file changed, 7 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index ecd13831539f..605a0724615d 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1097,6 +1097,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > utils::ScopeExitActions actions;\n> > int ret;\n> > \n> > + isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > + &DelayedControls::applyControls);\n> > +\n> > + actions += [&]() { isp_->frameStart.disconnect(data->delayedCtrls_.get()); };\n> > +\n> > /* Allocate buffers for internal pipeline usage. */\n> > ret = allocateBuffers(camera);\n> > if (ret)\n> > @@ -1168,6 +1173,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > \n> > isp_->setFrameStartEnabled(false);\n> > \n> > + isp_->frameStart.disconnect(data->delayedCtrls_.get());\n> > +\n> > data->ipa_->stop();\n> > \n> > if (hasSelfPath_)\n> > @@ -1354,8 +1361,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > data->delayedCtrls_ =\n> > std::make_unique(data->sensor_->device(),\n> > params);\n> > - isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > - &DelayedControls::applyControls);\n> > \n> > uint32_t supportedBlocks = kDefaultExtParamsBlocks;\n> > \n> > -- \n> > 2.48.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 DB7C0C3259\n\tfor ;\n\tWed, 22 Oct 2025 13:21:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DCB6C607A6;\n\tWed, 22 Oct 2025 15:21:52 +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 80506606CE\n\tfor ;\n\tWed, 22 Oct 2025 15:21:51 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:743e:771f:a1ba:58db])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 3D9DF591;\n\tWed, 22 Oct 2025 15:20:07 +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=\"VXc8GOKS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761139207;\n\tbh=mOgtXptAQfyP7Nu8AtXm79k58opiXJemHz4KwOQhgU4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VXc8GOKSxkRiRA8nR8p9645OZZ7MhYN/8PLh3iZn6hID6wbsBwtDnOmQu+Gh/o6ev\n\tgOP4LifXnuLrfS40VKgaMDLOUxPnz6KmybYy1NYrGiqDTApnhJFth3HTQuIlPPhnQ5\n\tzxJC6kU4zpFm6PGcV6Ps63ktntwSA8ZlGuOQH/jQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175944317364.3917877.6504301274229284836@ping.linuxembedded.co.uk>","References":"<20250930122726.1837524-1-stefan.klug@ideasonboard.com>\n\t<20250930122726.1837524-2-stefan.klug@ideasonboard.com>\n\t<175944317364.3917877.6504301274229284836@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v1 01/33] libcamera: rkisp1: Only connect delayed\n\tcontrols at start/stop","From":"Stefan Klug ","Cc":"","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 22 Oct 2025 15:21:48 +0200","Message-ID":"<176113930826.6155.17880459818612415702@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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\" "}}]