[{"id":35972,"web_url":"https://patchwork.libcamera.org/comment/35972/","msgid":"<303a524d-cd58-4436-9f9e-6faa3e54391a@ideasonboard.com>","date":"2025-09-25T11:15:31","subject":"Re: [PATCH 4/4] ipa: rkisp1: agc: Add support for sync","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 29. 11:10 keltezéssel, Paul Elder írta:\n> Add sync support to the RkISP1 IPA using the SyncHelper from libipa. The\n> syncAdjustment is saved in the frameContext (as opposed to getting it\n> from the SyncHelper) to avoid potential races and ensure that the same\n> value that was set into vblank will be returned in metadata.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>   src/ipa/rkisp1/algorithms/agc.cpp | 22 +++++++++++++++++++++-\n>   src/ipa/rkisp1/algorithms/agc.h   |  3 ++-\n>   src/ipa/rkisp1/ipa_context.h      |  1 +\n>   3 files changed, 24 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index bb1558df5422..9a27696ff868 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -156,6 +156,15 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n>   \t\t\t\tControlValue(controls::AnalogueGainModeManual) } },\n>   \t\t\t    ControlValue(controls::AnalogueGainModeAuto));\n>   \tcontext.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n> +\t/*\n> +\t * Insert the controlInfo for sync. Since FrameDurationLimits is only\n> +\t * set *after* the algorithms are initialized, we have no information\n> +\t * on it here. Use a sensible default here and update it later in\n> +\t * configure().\n> +\t * \\todo Move FrameDurationLimits from the base IPA to AGC\n> +\t */\n> +\tcontext.ctrlMap[&controls::SyncAdjustment] = controlInfo(120000);\n\nJust a small thing, but could you write 120'000? It's easier to count the zeros that way.\n\n\n\n> +\t/* Insert the controls for agc */\n>   \tcontext.ctrlMap.merge(controls());\n>   \n>   \treturn 0;\n> @@ -202,7 +211,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n>   \t\t  context.configuration.sensor.minAnalogueGain,\n>   \t\t  context.configuration.sensor.maxAnalogueGain);\n>   \n> +\tcontext.ctrlMap[&controls::SyncAdjustment] =\n> +\t\tcontrolInfo(frameDurationLimits.max().get<int64_t>());\n> +\n>   \tresetFrameCount();\n> +\tresetSync();\n>   \n>   \treturn 0;\n>   }\n> @@ -324,6 +337,10 @@ void Agc::queueRequest(IPAContext &context,\n>   \t}\n>   \tframeContext.agc.minFrameDuration = agc.minFrameDuration;\n>   \tframeContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> +\n> +\tconst auto &sync = controls.get(controls::SyncAdjustment);\n> +\tif (sync)\n> +\t\tsetSync(*sync, frameContext.agc.minFrameDuration);\n>   }\n>   \n>   /**\n> @@ -413,6 +430,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n>   \tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n>   \tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n>   \tmetadata.set(controls::ExposureValue, frameContext.agc.exposureValue);\n> +\tmetadata.set(controls::SyncAdjustment, frameContext.agc.syncAdjustment.count());\n>   }\n>   \n>   /**\n> @@ -471,7 +489,9 @@ void Agc::processFrameDuration(IPAContext &context,\n>   \tIPACameraSensorInfo &sensorInfo = context.sensorInfo;\n>   \tutils::Duration lineDuration = context.configuration.sensor.lineDuration;\n>   \n> -\tframeContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> +\tutils::Duration sync = getSync();\n> +\tframeContext.agc.vblank = ((frameDuration + sync) / lineDuration) - sensorInfo.outputSize.height;\n> +\tframeContext.agc.syncAdjustment = sync;\n>   \n>   \t/* Update frame duration accounting for line length quantization. */\n>   \tframeContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index 7867eed9c4e3..4441a519e857 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -15,6 +15,7 @@\n>   #include <libcamera/geometry.h>\n>   \n>   #include \"libipa/agc_mean_luminance.h\"\n> +#include \"libipa/sync_helper.h\"\n>   \n>   #include \"algorithm.h\"\n>   \n> @@ -22,7 +23,7 @@ namespace libcamera {\n>   \n>   namespace ipa::rkisp1::algorithms {\n>   \n> -class Agc : public Algorithm, public AgcMeanLuminance\n> +class Agc : public Algorithm, public AgcMeanLuminance, public SyncHelper\n\nI think we could (should?) avoid inheritance here, and simply store a `SyncHelper sync`\nmember somewhere in `Agc`. This would allow you to drop the \"Sync\" suffix from the\nfunctions in `SyncHelper`. Or is there anything planned that will need virtual\nfunctions, etc?\n\n\nRegards,\nBarnabás Pőcze\n\n>   {\n>   public:\n>   \tAgc();\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 7ccc7b501aff..c21ab77b2ba5 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -143,6 +143,7 @@ struct IPAFrameContext : public FrameContext {\n>   \t\tbool updateMetering;\n>   \t\tbool autoExposureModeChange;\n>   \t\tbool autoGainModeChange;\n> +\t\tutils::Duration syncAdjustment;\n>   \t} agc;\n>   \n>   \tstruct {","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5CF0EBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 25 Sep 2025 11:15:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD3556B5F9;\n\tThu, 25 Sep 2025 13:15:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B13326B5AA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 25 Sep 2025 13:15:32 +0200 (CEST)","from [192.168.33.22] (185.221.140.70.nat.pool.zt.hu\n\t[185.221.140.70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC026142;\n\tThu, 25 Sep 2025 13:14: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=\"WY2B0JSU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758798848;\n\tbh=0Elh80dUivbTC8sw8CYZpuJY+s0Xpx/nmR+Mlf/dViM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=WY2B0JSUjqbSwPMLjnSXL3Zn0BsmX3Nxnt5TD+wFhePr7Cemt4Mo3fQhBvhieG2hT\n\tQXvFvUoXYRPa0zQJe56nOZnDYt4BBqUjkxDyCXMDensYWJnJxWL2t/TlvVciriIlp8\n\tTEGqVd9HVf96tTgWGssoZ0r4Ml8ZN3RhGnUcM8bM=","Message-ID":"<303a524d-cd58-4436-9f9e-6faa3e54391a@ideasonboard.com>","Date":"Thu, 25 Sep 2025 13:15:31 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 4/4] ipa: rkisp1: agc: Add support for sync","To":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Cc":"david.plowman@raspberrypi.com, naush@raspberrypi.com,\n\tkieran.bingham@ideasonboard.com, stefan.klug@ideasonboard.com","References":"<20250829091011.2628954-1-paul.elder@ideasonboard.com>\n\t<20250829091011.2628954-5-paul.elder@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250829091011.2628954-5-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":35993,"web_url":"https://patchwork.libcamera.org/comment/35993/","msgid":"<175887455382.3174118.4530746533648093280@neptunite.rasen.tech>","date":"2025-09-26T08:15:53","subject":"Re: [PATCH 4/4] ipa: rkisp1: agc: Add support for sync","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Barnabás,\n\nThanks for the review.\n\nQuoting Barnabás Pőcze (2025-09-25 20:15:31)\n> Hi\n> \n> 2025. 08. 29. 11:10 keltezéssel, Paul Elder írta:\n> > Add sync support to the RkISP1 IPA using the SyncHelper from libipa. The\n> > syncAdjustment is saved in the frameContext (as opposed to getting it\n> > from the SyncHelper) to avoid potential races and ensure that the same\n> > value that was set into vblank will be returned in metadata.\n> > \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > ---\n> >   src/ipa/rkisp1/algorithms/agc.cpp | 22 +++++++++++++++++++++-\n> >   src/ipa/rkisp1/algorithms/agc.h   |  3 ++-\n> >   src/ipa/rkisp1/ipa_context.h      |  1 +\n> >   3 files changed, 24 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index bb1558df5422..9a27696ff868 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -156,6 +156,15 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> >                               ControlValue(controls::AnalogueGainModeManual) } },\n> >                           ControlValue(controls::AnalogueGainModeAuto));\n> >       context.ctrlMap[&controls::ExposureValue] = ControlInfo(-8.0f, 8.0f, 0.0f);\n> > +     /*\n> > +      * Insert the controlInfo for sync. Since FrameDurationLimits is only\n> > +      * set *after* the algorithms are initialized, we have no information\n> > +      * on it here. Use a sensible default here and update it later in\n> > +      * configure().\n> > +      * \\todo Move FrameDurationLimits from the base IPA to AGC\n> > +      */\n> > +     context.ctrlMap[&controls::SyncAdjustment] = controlInfo(120000);\n> \n> Just a small thing, but could you write 120'000? It's easier to count the zeros that way.\n> \n> \n> \n> > +     /* Insert the controls for agc */\n> >       context.ctrlMap.merge(controls());\n> >   \n> >       return 0;\n> > @@ -202,7 +211,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> >                 context.configuration.sensor.minAnalogueGain,\n> >                 context.configuration.sensor.maxAnalogueGain);\n> >   \n> > +     context.ctrlMap[&controls::SyncAdjustment] =\n> > +             controlInfo(frameDurationLimits.max().get<int64_t>());\n> > +\n> >       resetFrameCount();\n> > +     resetSync();\n> >   \n> >       return 0;\n> >   }\n> > @@ -324,6 +337,10 @@ void Agc::queueRequest(IPAContext &context,\n> >       }\n> >       frameContext.agc.minFrameDuration = agc.minFrameDuration;\n> >       frameContext.agc.maxFrameDuration = agc.maxFrameDuration;\n> > +\n> > +     const auto &sync = controls.get(controls::SyncAdjustment);\n> > +     if (sync)\n> > +             setSync(*sync, frameContext.agc.minFrameDuration);\n> >   }\n> >   \n> >   /**\n> > @@ -413,6 +430,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> >       metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> >       metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> >       metadata.set(controls::ExposureValue, frameContext.agc.exposureValue);\n> > +     metadata.set(controls::SyncAdjustment, frameContext.agc.syncAdjustment.count());\n> >   }\n> >   \n> >   /**\n> > @@ -471,7 +489,9 @@ void Agc::processFrameDuration(IPAContext &context,\n> >       IPACameraSensorInfo &sensorInfo = context.sensorInfo;\n> >       utils::Duration lineDuration = context.configuration.sensor.lineDuration;\n> >   \n> > -     frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;\n> > +     utils::Duration sync = getSync();\n> > +     frameContext.agc.vblank = ((frameDuration + sync) / lineDuration) - sensorInfo.outputSize.height;\n> > +     frameContext.agc.syncAdjustment = sync;\n> >   \n> >       /* Update frame duration accounting for line length quantization. */\n> >       frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index 7867eed9c4e3..4441a519e857 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -15,6 +15,7 @@\n> >   #include <libcamera/geometry.h>\n> >   \n> >   #include \"libipa/agc_mean_luminance.h\"\n> > +#include \"libipa/sync_helper.h\"\n> >   \n> >   #include \"algorithm.h\"\n> >   \n> > @@ -22,7 +23,7 @@ namespace libcamera {\n> >   \n> >   namespace ipa::rkisp1::algorithms {\n> >   \n> > -class Agc : public Algorithm, public AgcMeanLuminance\n> > +class Agc : public Algorithm, public AgcMeanLuminance, public SyncHelper\n> \n> I think we could (should?) avoid inheritance here, and simply store a `SyncHelper sync`\n> member somewhere in `Agc`. This would allow you to drop the \"Sync\" suffix from the\n> functions in `SyncHelper`. Or is there anything planned that will need virtual\n> functions, etc?\n\nThe intention was that in the (hopefully near) future, IPAs/algorithms could\ninherit functionality from libipa and not even have to plumb anything like this\npatch does. But I suppose until that future comes, it might indeed be cleaner\nto store a SyncHelper member.\n\n\nThanks,\n\nPaul\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> >   {\n> >   public:\n> >       Agc();\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 7ccc7b501aff..c21ab77b2ba5 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -143,6 +143,7 @@ struct IPAFrameContext : public FrameContext {\n> >               bool updateMetering;\n> >               bool autoExposureModeChange;\n> >               bool autoGainModeChange;\n> > +             utils::Duration syncAdjustment;\n> >       } agc;\n> >   \n> >       struct {\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 8C41CC328C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 26 Sep 2025 08:16:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F6BE6B5F3;\n\tFri, 26 Sep 2025 10:16:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F84B6936E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 26 Sep 2025 10:16:02 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:80d3:5bdc:c6ac:4dd3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 4ECCDF04;\n\tFri, 26 Sep 2025 10:14:36 +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=\"E1naeVnz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758874476;\n\tbh=TP9K48gM5XF91h6gXc72XOaliipkAdGZGhVSXm1kplA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=E1naeVnzNwacNGakfrmMeSy5j0ssz1QoYQsU5SXmzfrcHerW++FdbqkhiYP64Iflg\n\tZVQ3Nmy42+u7fYhrBizxgL8AXAKFSSjQirAQm1DRcXDt1W1S9mFG/0JaR1w/PddjLf\n\tZOtggMYIZemiQKb8azc9SXv1mTzyNWplgiNfIfyE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<303a524d-cd58-4436-9f9e-6faa3e54391a@ideasonboard.com>","References":"<20250829091011.2628954-1-paul.elder@ideasonboard.com>\n\t<20250829091011.2628954-5-paul.elder@ideasonboard.com>\n\t<303a524d-cd58-4436-9f9e-6faa3e54391a@ideasonboard.com>","Subject":"Re: [PATCH 4/4] ipa: rkisp1: agc: Add support for sync","From":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"david.plowman@raspberrypi.com, naush@raspberrypi.com,\n\tkieran.bingham@ideasonboard.com, stefan.klug@ideasonboard.com","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 26 Sep 2025 17:15:53 +0900","Message-ID":"<175887455382.3174118.4530746533648093280@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]