[{"id":33354,"web_url":"https://patchwork.libcamera.org/comment/33354/","msgid":"<20250214000948.GA8393@pendragon.ideasonboard.com>","date":"2025-02-14T00:09:48","subject":"Re: [PATCH] libcamera: rkisp1: Only connect delayed controls at\n\tstart/stop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Feb 07, 2025 at 01:37:43PM +0000, Kieran Bingham wrote:\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> \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\nI've been pondering about requiring the receiver of a signal to inherit\nfrom 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\nConceptually, the isp_->frameStart signal is emitted by a resource\nshared by multiple cameras. It would make sense to model this with a\nslot in the PipelineHandlerRkISP1 class instead of the RkISP1CameraData\nclass, and relay it to the delayed controls class through activeCamera_.\nWould that be a better model ?\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 1ac8d8ae7ed9..b1b0bde7fd00 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1071,6 +1071,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \tutils::ScopeExitActions actions;\n> \tint ret;\n> \n> +\tisp_->frameStart.connect(data->delayedCtrls_.get(),\n> +\t\t\t\t &DelayedControls::applyControls);\n> +\n> +\tactions += [&]() { isp_->frameStart.disconnect(data->delayedCtrls_.get()); };\n> +\n> \t/* Allocate buffers for internal pipeline usage. */\n> \tret = allocateBuffers(camera);\n> \tif (ret)\n> @@ -1142,6 +1147,8 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> \n> \tisp_->setFrameStartEnabled(false);\n> \n> +\tisp_->frameStart.disconnect(data->delayedCtrls_.get());\n> +\n> \tdata->ipa_->stop();\n> \n> \tif (hasSelfPath_)\n> @@ -1330,8 +1337,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tdata->delayedCtrls_ =\n> \t\tstd::make_unique(data->sensor_->device(),\n> \t\t\t\t\t\t params);\n> -\tisp_->frameStart.connect(data->delayedCtrls_.get(),\n> -\t\t\t\t &DelayedControls::applyControls);\n> \n> \tret = data->loadIPA(media_->hwRevision());\n> \tif (ret)","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 8C152C3272\n\tfor ;\n\tFri, 14 Feb 2025 00:10:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F0F668646;\n\tFri, 14 Feb 2025 01:10:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A4F16032B\n\tfor ;\n\tFri, 14 Feb 2025 01:10:00 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1EAA36B5;\n\tFri, 14 Feb 2025 01:08:41 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LjwDdmF8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739491721;\n\tbh=gSuYsGlQpPONIZhstFZoQ/5sccQ9ZExlhCJrRUBVuMA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LjwDdmF8LOsbx9+cLKHD+vo2FAiHfWPqJ0R2sued4gqZKNKNAZpl/6HaKFSD2sjhg\n\tA0ACi8l2cAFQ4Tm7XNOXGn41Ause2XdO2g4peVbDumCI9wSlF/saHpPQ3szFs+qN6E\n\tHV9bOjtYlRtkvLOeohZfqOK5HJ5AasiCuhsgh+XI=","Date":"Fri, 14 Feb 2025 02:09:48 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH] libcamera: rkisp1: Only connect delayed controls at\n\tstart/stop","Message-ID":"<20250214000948.GA8393@pendragon.ideasonboard.com>","References":"<20250207133743.1074164-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250207133743.1074164-1-kieran.bingham@ideasonboard.com>","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\" "}}]