[{"id":33032,"web_url":"https://patchwork.libcamera.org/comment/33032/","msgid":"<20250112221237.GC13236@pendragon.ideasonboard.com>","date":"2025-01-12T22:12:37","subject":"Re: [RFC PATCH 6/7] ipa: rkisp1: Add a lookahead of one frame when\n\tsending sensor controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Fri, Dec 20, 2024 at 05:26:52PM +0100, Stefan Klug wrote:\n> When the delayed controls instance of the pipeline is reset, it initializes\n> the values for frame 0 as being sent out to the sensor (which is\n> correct). The next sequence number that can be pushed to delayed\n> controls is therefore number 1. Ensure that the IPA never tries to\n> queue controls for frame 0.\n\nWhat happens to the controls passed in request 0 ?\n\nI know the existing code doesn't behave correctly, but it feels that\nthis series is piling hacks towards a result that will be even more\ndifficult to untangle :-S I'd like a second opinion on this.\n\n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/rkisp1.cpp | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 799fe0846635..5d439e0d727b 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -386,10 +386,13 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n> \n> \t/*\n> \t * \\todo: Here we should do a lookahead that takes the sensor delays\n> -\t * into account.\n> +\t * into account. A lookahead of 1 is the smallest lookahead possible to\n> +\t * ensure we don't try to send the controls for a frame that we already\n> +\t * received.\n> \t */\n> -\tControlList ctrls = getSensorControls(frame);\n> -\tsetSensorControls.emit(frame, ctrls);\n> +\tint lookahead = 1;\n> +\tControlList ctrls = getSensorControls(frame + lookahead);\n> +\tsetSensorControls.emit(frame + lookahead, ctrls);\n> \n> \tcontext_.debugMetadata.moveEntries(metadata);\n> \tmetadataReady.emit(frame, metadata);","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 6186BC32F6\n\tfor ;\n\tSun, 12 Jan 2025 22:12:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CD2268500;\n\tSun, 12 Jan 2025 23:12:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5ED6A684E0\n\tfor ;\n\tSun, 12 Jan 2025 23:12:41 +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 24FFA8CC;\n\tSun, 12 Jan 2025 23:11:45 +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=\"s5asqFgo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736719905;\n\tbh=l2zeiLze3F0jHD0RNdGPEd0CIRyxeT8751J+tvnhu8U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s5asqFgo64a4wljVpEQz/al9p8vNBirBBZmXojqK2+FKWduu+iTqE0r+g7cECdBzh\n\tPYCW4wDL7oz7mggkLYwiwtzhWR5WCvhmlOfZB+VRbje6iJ1MvjWpCJY3P8tjr8sA6K\n\tm2ALIgHlf78XUSfuteOAK7tfuQ9FuFurYtDXUzaw=","Date":"Mon, 13 Jan 2025 00:12:37 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 6/7] ipa: rkisp1: Add a lookahead of one frame when\n\tsending sensor controls","Message-ID":"<20250112221237.GC13236@pendragon.ideasonboard.com>","References":"<20241220162724.756494-1-stefan.klug@ideasonboard.com>\n\t<20241220162724.756494-7-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241220162724.756494-7-stefan.klug@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\" "}},{"id":33043,"web_url":"https://patchwork.libcamera.org/comment/33043/","msgid":"","date":"2025-01-13T12:04:36","subject":"Re: [RFC PATCH 6/7] ipa: rkisp1: Add a lookahead of one frame when\n\tsending sensor controls","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Mon, Jan 13, 2025 at 12:12:37AM +0200, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Fri, Dec 20, 2024 at 05:26:52PM +0100, Stefan Klug wrote:\n> > When the delayed controls instance of the pipeline is reset, it initializes\n> > the values for frame 0 as being sent out to the sensor (which is\n> > correct). The next sequence number that can be pushed to delayed\n> > controls is therefore number 1. Ensure that the IPA never tries to\n> > queue controls for frame 0.\n> \n> What happens to the controls passed in request 0 ?\n\nThese were correctly applied to the activeState so they are not lost but\nwill get picked up in frame 1.\n\n> \n> I know the existing code doesn't behave correctly, but it feels that\n> this series is piling hacks towards a result that will be even more\n> difficult to untangle :-S I'd like a second opinion on this.\n\nIs it really more hacks, or merely making the existing deficiencies \nmore visible?\n\nMaybe we can take this as discussion basis for the big picture. In my\nfirst PFC series the lookahead was pushed down to DelayedControls (which\nI maybe should have renamed to LookaheadControls) which didn't get much\napproval. But the changes were also difficult to digest. So I see there\nis a bit of work to be done conceptually also. I still believe we need\nto move the sensor lookahead out of the IPA so I didn't try to implement\na full fledged lookahead here.\n\nCheers,\nStefan\n\n> \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/rkisp1.cpp | 9 ++++++---\n> > 1 file changed, 6 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 799fe0846635..5d439e0d727b 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -386,10 +386,13 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n> > \n> > \t/*\n> > \t * \\todo: Here we should do a lookahead that takes the sensor delays\n> > -\t * into account.\n> > +\t * into account. A lookahead of 1 is the smallest lookahead possible to\n> > +\t * ensure we don't try to send the controls for a frame that we already\n> > +\t * received.\n> > \t */\n> > -\tControlList ctrls = getSensorControls(frame);\n> > -\tsetSensorControls.emit(frame, ctrls);\n> > +\tint lookahead = 1;\n> > +\tControlList ctrls = getSensorControls(frame + lookahead);\n> > +\tsetSensorControls.emit(frame + lookahead, ctrls);\n> > \n> > \tcontext_.debugMetadata.moveEntries(metadata);\n> > \tmetadataReady.emit(frame, metadata);\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 0F9DFC32F6\n\tfor ;\n\tMon, 13 Jan 2025 12:04:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27E2B68503;\n\tMon, 13 Jan 2025 13:04:41 +0100 (CET)","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 C8856684E7\n\tfor ;\n\tMon, 13 Jan 2025 13:04:39 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3c15:8eea:8742:2e5a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2E031788;\n\tMon, 13 Jan 2025 13:03:43 +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=\"id9DHkYz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736769823;\n\tbh=8NzcrU45qlJz1DknzSrTpfDoZBjUFTqK6njD3n0N5TY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=id9DHkYzGAd9hnT+ab9Va0s64xod0bfcdd8vYaXAAV3TtzBaRi8XMsiksqdd6mhJe\n\tOPLcIFsR51DHst7S9f0XMLn58dRKHLNZaQeWV6a+A+y9pHZmR6Xx24ykRsGH3+Ophy\n\tGjOaMjFsX8RLQRczK+nteayGPKeetDHC3qawwamo=","Date":"Mon, 13 Jan 2025 13:04:36 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 6/7] ipa: rkisp1: Add a lookahead of one frame when\n\tsending sensor controls","Message-ID":"","References":"<20241220162724.756494-1-stefan.klug@ideasonboard.com>\n\t<20241220162724.756494-7-stefan.klug@ideasonboard.com>\n\t<20250112221237.GC13236@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250112221237.GC13236@pendragon.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\" "}},{"id":33408,"web_url":"https://patchwork.libcamera.org/comment/33408/","msgid":"","date":"2025-02-20T09:04:23","subject":"Re: [RFC PATCH 6/7] ipa: rkisp1: Add a lookahead of one frame when\n\tsending sensor controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Jan 13, 2025 at 01:04:36PM +0100, Stefan Klug wrote:\n> Hi Laurent,\n> \n> Thank you for the review. \n> \n> On Mon, Jan 13, 2025 at 12:12:37AM +0200, Laurent Pinchart wrote:\n> > Hi Stefan,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Dec 20, 2024 at 05:26:52PM +0100, Stefan Klug wrote:\n> > > When the delayed controls instance of the pipeline is reset, it initializes\n> > > the values for frame 0 as being sent out to the sensor (which is\n> > > correct). The next sequence number that can be pushed to delayed\n> > > controls is therefore number 1. Ensure that the IPA never tries to\n> > > queue controls for frame 0.\n> > \n> > What happens to the controls passed in request 0 ?\n> \n> These were correctly applied to the activeState so they are not lost but\n> will get picked up in frame 1.\n\nFrom my understanding, 5/7 handles this with ipa_->start() returning the\nsensor controls to be set, and then the pipeline handler sets the sensor\ncontrols synchronously after ipa_->start() returns and before\nstreamOn()ing. Then it resets the delayed controls and so here we are.\n\n> > \n> > I know the existing code doesn't behave correctly, but it feels that\n> > this series is piling hacks towards a result that will be even more\n> > difficult to untangle :-S I'd like a second opinion on this.\n> \n> Is it really more hacks, or merely making the existing deficiencies \n> more visible?\n\nWell it does indeed kind of feel like a hack, but it's not that bad of a\nhack...\n\n> Maybe we can take this as discussion basis for the big picture. In my\n> first PFC series the lookahead was pushed down to DelayedControls (which\n> I maybe should have renamed to LookaheadControls) which didn't get much\n> approval. But the changes were also difficult to digest. So I see there\n> is a bit of work to be done conceptually also. I still believe we need\n> to move the sensor lookahead out of the IPA so I didn't try to implement\n> a full fledged lookahead here.\n\nI think it is indeed more visible here, at least compared to in\nDelayedControls.\n\n\nI think it's fine...\n\nReviewed-by: Paul Elder \n\n> \n> > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/rkisp1/rkisp1.cpp | 9 ++++++---\n> > > 1 file changed, 6 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 799fe0846635..5d439e0d727b 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -386,10 +386,13 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n> > > \n> > > \t/*\n> > > \t * \\todo: Here we should do a lookahead that takes the sensor delays\n> > > -\t * into account.\n> > > +\t * into account. A lookahead of 1 is the smallest lookahead possible to\n> > > +\t * ensure we don't try to send the controls for a frame that we already\n> > > +\t * received.\n> > > \t */\n> > > -\tControlList ctrls = getSensorControls(frame);\n> > > -\tsetSensorControls.emit(frame, ctrls);\n> > > +\tint lookahead = 1;\n> > > +\tControlList ctrls = getSensorControls(frame + lookahead);\n> > > +\tsetSensorControls.emit(frame + lookahead, ctrls);\n> > > \n> > > \tcontext_.debugMetadata.moveEntries(metadata);\n> > > \tmetadataReady.emit(frame, metadata);\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 53683BE175\n\tfor ;\n\tThu, 20 Feb 2025 09:04:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FBF968697;\n\tThu, 20 Feb 2025 10:04:33 +0100 (CET)","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 09DFF6185D\n\tfor ;\n\tThu, 20 Feb 2025 10:04:31 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:4292:75df:25cd:1836])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E32BE79;\n\tThu, 20 Feb 2025 10:03:06 +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=\"ICBFMx5O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740042188;\n\tbh=wf/NnPS/i42dYoNdcf5hyunLqGy625LrNKxO3scIBL4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ICBFMx5ODRpasJzGhq1DLtnHLXpLkWSY1kBLt3N7wZNEoXXb2D7No9iRS+mLOtzm9\n\tCTkC1drn7qkFtDGwfL8Sx5NC2LVMCTtmMUmigWVSz3UYJaT1WATN35MJQLOEksZw0m\n\tVZBxFjArgUE8UG1FBHSELsUs/HLyGwE086AkfIfc=","Date":"Thu, 20 Feb 2025 18:04:23 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 6/7] ipa: rkisp1: Add a lookahead of one frame when\n\tsending sensor controls","Message-ID":"","References":"<20241220162724.756494-1-stefan.klug@ideasonboard.com>\n\t<20241220162724.756494-7-stefan.klug@ideasonboard.com>\n\t<20250112221237.GC13236@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]