[{"id":35963,"web_url":"https://patchwork.libcamera.org/comment/35963/","msgid":"<85bjn0m6gj.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-24T11:32:28","subject":"Re: [PATCH 5/5] ipa: software_isp: AGC: Stop using delayed control\n\tfor previous values","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Hans,\n\nthank you for the patch.\n\nHans de Goede writes:\n\n> Due to a combination of not having correct control-delay information for\n> various sensors as well as the generic nature of the simple-pipeline +\n> software-ISP meaning that any pipeline delays are unknown \n\nThe issue is that we cannot be sure at which frame the pipeline applies\nexposure changes and that the drivers don't report exposure/gain in\nmetadata? It might be useful to mention the reasons in the commit\nmessage.\n\n> it is impossible to get reliable control-delay values.\n>\n> Wrong control-delay values can lead to pretty bad oscilation. Since atm\n> it is not possible to fix the wrong control-delay values, stop using\n> the old sensor values reported by the delayed-controlled mechanism.\n>\n> Instead remember the gain and exposures set the last time the algorithm\n> runs (initializing the cached values with the actual sensor values on\n> the first frame), combined with skipping a fixed number of frames after\n> changing values.\n\nA question is whether we, now when we do it less frequently, should\nadjust the exposure/gain in larger increments, possibly based on the\ndeviation from the optimal exposure and/or recent adjustments. Just a\nthought for followup stuff, not suggesting addressing it here.\n\n> Signed-off-by: Hans de Goede \n> ---\n> src/ipa/simple/algorithms/agc.cpp | 30 +++++++++++++++++++++++++-----\n> src/ipa/simple/algorithms/agc.h | 3 ++-\n> src/ipa/simple/ipa_context.h | 6 ++++++\n> src/ipa/simple/soft_simple.cpp | 4 ++--\n> 4 files changed, 35 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp\n> index cdde56ba2..3c0e20ddc 100644\n> --- a/src/ipa/simple/algorithms/agc.cpp\n> +++ b/src/ipa/simple/algorithms/agc.cpp\n> @@ -41,7 +41,15 @@ Agc::Agc()\n> {\n> }\n> \n> -void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV)\n> +int Agc::configure(IPAContext &context,\n> +\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> +{\n> +\tcontext.activeState.agc.skipFrames = 0;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Agc::updateExposure(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, double exposureMSV)\n> {\n> \t/*\n> \t * kExpDenominator of 10 gives ~10% increment/decrement;\n> @@ -51,8 +59,18 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou\n> \tstatic constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;\n> \tstatic constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;\n> \n> -\tint32_t &exposure = frameContext.sensor.exposure;\n> -\tdouble &again = frameContext.sensor.gain;\n> +\tint32_t &skipFrames = context.activeState.agc.skipFrames;\n> +\tint32_t &exposure = context.activeState.agc.exposure;\n> +\tdouble &again = context.activeState.agc.again;\n> +\n> +\t/* Set initial-gain values from sensor on first frame */\n> +\tif (frame == 0) {\n> +\t\texposure = frameContext.sensor.exposure;\n> +\t\tagain = frameContext.sensor.gain;\n> +\t}\n> +\n> +\tif (skipFrames && --skipFrames)\n> +\t\treturn;\n\nI think something like\n\n if (skipFrames > 1) {\n skipFrames--; \n return;\n }\n\nwould be much clearer.\n\n> \tif (exposureMSV < kExposureOptimal - kExposureSatisfactory) {\n> \t\tif (exposure < context.configuration.agc.exposureMax) {\n> @@ -68,6 +86,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou\n> \t\t\telse\n> \t\t\t\tagain = next;\n> \t\t}\n> +\t\tskipFrames = 3;\n\nPerhaps the value of `3' deserves a declared constant, with an\nexplanation why 3 is a good value.\n\n> \t}\n> \n> \tif (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n> @@ -84,6 +103,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou\n> \t\t\telse\n> \t\t\t\texposure = next;\n> \t\t}\n> +\t\tskipFrames = 3;\n> \t}\n> \n> \texposure = std::clamp(exposure, context.configuration.agc.exposureMin,\n> @@ -97,7 +117,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou\n> }\n> \n> void Agc::process(IPAContext &context,\n> -\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const SwIspStats *stats,\n> \t\t ControlList &metadata)\n> @@ -135,7 +155,7 @@ void Agc::process(IPAContext &context,\n> \t}\n> \n> \tfloat exposureMSV = (denom == 0 ? 0 : static_cast(num) / denom);\n> -\tupdateExposure(context, frameContext, exposureMSV);\n> +\tupdateExposure(context, frame, frameContext, exposureMSV);\n> }\n> \n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/simple/algorithms/agc.h\n> index 112d9f5a1..ef387664f 100644\n> --- a/src/ipa/simple/algorithms/agc.h\n> +++ b/src/ipa/simple/algorithms/agc.h\n> @@ -19,13 +19,14 @@ public:\n> \tAgc();\n> \t~Agc() = default;\n> \n> +\tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const SwIspStats *stats,\n> \t\t ControlList &metadata) override;\n> \n> private:\n> -\tvoid updateExposure(IPAContext &context, IPAFrameContext &frameContext, double exposureMSV);\n> +\tvoid updateExposure(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, double exposureMSV);\n> };\n> \n> } /* namespace ipa::soft::algorithms */\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index ba525a881..cdab980a1 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -37,6 +37,12 @@ struct IPASessionConfiguration {\n> };\n> \n> struct IPAActiveState {\n> +\tstruct {\n> +\t\tint32_t skipFrames;\n\nUnsigned?\n\n> +\t\tint32_t exposure;\n> +\t\tdouble again;\n> +\t} agc;\n> +\n> \tstruct {\n> \t\tuint8_t level;\n> \t\tint32_t lastExposure;\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index f0764ef46..f8c6291e2 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -327,8 +327,8 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> \n> \tControlList ctrls(sensorInfoMap_);\n> \n> -\tauto &againNew = frameContext.sensor.gain;\n> -\tctrls.set(V4L2_CID_EXPOSURE, frameContext.sensor.exposure);\n> +\tauto &againNew = context_.activeState.agc.again;\n> +\tctrls.set(V4L2_CID_EXPOSURE, context_.activeState.agc.exposure);\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN,\n> \t\t static_cast(camHelper_ ? camHelper_->gainCode(againNew) : againNew));","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 8D2D1C328C\n\tfor ;\n\tWed, 24 Sep 2025 11:32:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F80D6B5FB;\n\tWed, 24 Sep 2025 13:32:37 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1EB06B5F0\n\tfor ;\n\tWed, 24 Sep 2025 13:32:34 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-202-7tNCnJhyOjaVXWWunQDupA-1; Wed, 24 Sep 2025 07:32:32 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-3f030846a41so3092530f8f.2\n\tfor ;\n\tWed, 24 Sep 2025 04:32:31 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-46e2a9be025sm28444795e9.11.2025.09.24.04.32.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 24 Sep 2025 04:32:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"NLMuuib0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1758713553;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=cyGkaWBalViKHZS1LpT5Nht7tF28aigQkfJP+T6xBgY=;\n\tb=NLMuuib0oJpbSprC99Lb9T8wKS9Uj+j9UfsXgxqWgR4ol7lkr02SNZKwP85y1NUdC/hvPz\n\tP1+G0qtj7NJMMuTLg32xwTk5eldoPF/mTjbxZ+EhQsYyRSWtlzgD5+8POo4MD0AueJB5jG\n\tLVL08T6dGCEFLLWTFmX3zTjKWYOm9jc=","X-MC-Unique":"7tNCnJhyOjaVXWWunQDupA-1","X-Mimecast-MFC-AGG-ID":"7tNCnJhyOjaVXWWunQDupA_1758713551","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1758713550; x=1759318350;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=cyGkaWBalViKHZS1LpT5Nht7tF28aigQkfJP+T6xBgY=;\n\tb=Cut5BFJl5jZtQ/jZJVb8ADj0bpHZ/uA1EQxQRUMctmktGsHp6HFe1K+db+PdiGbR0N\n\tOO08iDXmxk3aKcAhLf73ncC/BaMSjCvUbbF2G3U+5NFQh5neWtRQe0Q9TfnI0GDZjvv7\n\tuFBuHbOTmA1Wj8Kgyxo/rHT0/oEj6jp2OYp5Wbk3+74ZmjnYU7CORPGfMawXlMGXxEH/\n\t0bwjXvmxNBcyEJmk3s3dDSQbk7T5yLAonDVelSLzOH/jnml4xtc9QP+yzbhTcGGODKA/\n\t6v+j0uBak6xhaj4F2OpHwAThYHnO0ZgP2RCjbzKFOvQo2aEzGz/xeRFa9Ob7VB4yzpFe\n\t3xhw==","X-Gm-Message-State":"AOJu0YwfEpAp51bBtp3cjwNEGG4Psh8ND0DDQz9DiHMrnZbNDh2ES3UE\n\tWmGZCrhAV2/alIjDNPUyUJxfyU22Rc9+yvXEPra8QPVwIYBQJkZeNgCWKLwo7RnxxEm3fMGKSIw\n\tDDOU9oW3+FPwhAjYrOu0kKZsO9DbwF8oP26QE8LPKU2zt2n76a9kW18bXV1MVJyd4J6WEnYjlbm\n\tH/gsp511WGgd4ZSVVNz6+kYL6tHr8iZLk1ptD2Jl3KShfrJdVcB1Bfj/etReg=","X-Gm-Gg":"ASbGnctQI1J38f3GqTHxBQoMeepyFzlZ+kVGPRv7A2sk9f9fdDRvBpWYmkoNjc5nelN\n\tW6K4Wy/FakgGa2KYqIp0e/YnSjd52p0UQlwPg/181W6FFby3P8LYJCyJAQuvAvAPVTlXZbcy15T\n\t2pCo3dwj6xmCs+yKi7kF8j1SQgIeFwoNmf+r9JcHwy5iCzyfn1kw9UH5RE+i68Jkv/MuWTdVsk9\n\tHWv+Y16H3pc8GXDyuyA95XQR7Az+ldD40MFsLn9+FM3PQ+VMPRSNxWBzsoBDaaYfCANy71bT2Lb\n\tV4/PsA+iVManEyahpoDZDSHspmtVUqtJxA9pOoTMBJNYGCfzC4gDunZ/S2SMsHjl2Q==","X-Received":["by 2002:a05:6000:2509:b0:3ed:a43d:8eba with SMTP id\n\tffacd0b85a97d-405caa51bd0mr5434680f8f.52.1758713550456; \n\tWed, 24 Sep 2025 04:32:30 -0700 (PDT)","by 2002:a05:6000:2509:b0:3ed:a43d:8eba with SMTP id\n\tffacd0b85a97d-405caa51bd0mr5434645f8f.52.1758713549749; \n\tWed, 24 Sep 2025 04:32:29 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFZ9lhUXIba8QMjMOtFUczmUs2cdq9UCQEz34hlP3+dq22EorRbtCf9thHmKlgTs/HoX5VuHQ==","From":"Milan Zamazal ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 5/5] ipa: software_isp: AGC: Stop using delayed control\n\tfor previous values","In-Reply-To":"<20250923190657.21453-6-hansg@kernel.org> (Hans de Goede's\n\tmessage of \"Tue, 23 Sep 2025 21:06:57 +0200\")","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-6-hansg@kernel.org>","Date":"Wed, 24 Sep 2025 13:32:28 +0200","Message-ID":"<85bjn0m6gj.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"C-7nxTrsBvhJD2eW1K2HC7s3Ga2hXIjWFtQu_husnAw_1758713551","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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\" "}}]