Patch Detail
Show a patch.
GET /api/1.1/patches/21888/?format=api
{ "id": 21888, "url": "https://patchwork.libcamera.org/api/1.1/patches/21888/?format=api", "web_url": "https://patchwork.libcamera.org/patch/21888/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20241113131256.3170817-6-paul.elder@ideasonboard.com>", "date": "2024-11-13T13:12:53", "name": "[v3,5/8] ipa: raspberry: Port to the new AEGC controls", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "bd299174985ec561e72530dc3166dc549dc6b62a", "submitter": { "id": 17, "url": "https://patchwork.libcamera.org/api/1.1/people/17/?format=api", "name": "Paul Elder", "email": "paul.elder@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/21888/mbox/", "series": [ { "id": 4790, "url": "https://patchwork.libcamera.org/api/1.1/series/4790/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4790", "date": "2024-11-13T13:12:48", "name": "AEGC controls", "version": 3, "mbox": "https://patchwork.libcamera.org/series/4790/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/21888/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/21888/checks/", "tags": {}, "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 12362BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Nov 2024 13:13:35 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2B1865828;\n\tWed, 13 Nov 2024 14:13:34 +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 D979C65829\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Nov 2024 14:13:31 +0100 (CET)", "from neptunite.flets-east.jp (unknown\n\t[IPv6:2404:7a81:160:2100:eb25:7aa0:7da7:eb24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8A15119AE;\n\tWed, 13 Nov 2024 14:13:16 +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=\"rC9d1K2l\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731503598;\n\tbh=KvbLXLk7pFvdhO4NLtCTjG8JWMuBAf3sjj9X69UWPcM=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=rC9d1K2lIqflen9juH0COC4oETXcA9QhGBYs8OQdPJbr8aWA2SB1sXbnlj/CgNi+n\n\tI2J1vUDfEj1N0AheU1xizrZe7AcOnjzRaD/JOa18d/d+Hm1RHBmC6K7MyiWNI3SPMK\n\tXrEG6NAghkvEq44PIFbxFGYGejR5T6v4fyJlcfcA=", "From": "Paul Elder <paul.elder@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Cc": "laurent.pinchart@ideasonboard.com, jacopo.mondi@ideasonboard.com,\n\tnaush@raspberrypi.com, david.plowman@raspberrypi.com,\n\tJacopo Mondi <jacopo@jmondi.org>,\n\tPaul Elder <paul.elder@ideasonboard.com>", "Subject": "[PATCH v3 5/8] ipa: raspberry: Port to the new AEGC controls", "Date": "Wed, 13 Nov 2024 22:12:53 +0900", "Message-Id": "<20241113131256.3170817-6-paul.elder@ideasonboard.com>", "X-Mailer": "git-send-email 2.39.2", "In-Reply-To": "<20241113131256.3170817-1-paul.elder@ideasonboard.com>", "References": "<20241113131256.3170817-1-paul.elder@ideasonboard.com>", "MIME-Version": "1.0", "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>" }, "content": "From: Jacopo Mondi <jacopo@jmondi.org>\n\nThe newly introduced controls to drive the AEGC algorithm allow\nto control the computation of the exposure time and analogue gain\nseparately.\n\nThe RPi AEGC implementation already computes the shutter and gain\nvalues separately but does not expose separate functions to control\nthem.\n\nAugment the AgcAlgorithm interface to allow pausing/resuming the shutter\nand gain automatic computations separately and plumb them to the newly\nintroduced controls.\n\nAdd safety checks to ignore ExposureTime and AnalogueGain values if the\nalgorithms are not paused, and report the correct AeState value by\nchecking if both algorithms have been paused or if they have converged.\n\nSigned-off-by: Jacopo Mondi <jacopo@jmondi.org>\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n---\nChanges in v3:\n- recovered from 2-year-old bitrot\n---\n src/ipa/rpi/common/ipa_base.cpp | 74 +++++++++++++++++++---\n src/ipa/rpi/controller/agc_algorithm.h | 8 ++-\n src/ipa/rpi/controller/rpi/agc.cpp | 52 +++++++++++++--\n src/ipa/rpi/controller/rpi/agc.h | 8 ++-\n src/ipa/rpi/controller/rpi/agc_channel.cpp | 24 ++++++-\n src/ipa/rpi/controller/rpi/agc_channel.h | 8 ++-\n 6 files changed, 150 insertions(+), 24 deletions(-)", "diff": "diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\nindex 468f36a82ab2..2d26d497be85 100644\n--- a/src/ipa/rpi/common/ipa_base.cpp\n+++ b/src/ipa/rpi/common/ipa_base.cpp\n@@ -55,8 +55,13 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n \n /* List of controls handled by the Raspberry Pi IPA */\n const ControlInfoMap::Map ipaControls{\n-\t{ &controls::AeEnable, ControlInfo(false, true) },\n+\t{ &controls::ExposureTimeMode,\n+\t ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),\n+\t\t static_cast<int32_t>(controls::ExposureTimeModeManual)) },\n \t{ &controls::ExposureTime, ControlInfo(0, 66666) },\n+\t{ &controls::AnalogueGainMode,\n+\t ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),\n+\t\t static_cast<int32_t>(controls::AnalogueGainModeManual)) },\n \t{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n \t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n@@ -760,21 +765,22 @@ void IpaBase::applyControls(const ControlList &controls)\n \t\t\t\t << \" = \" << ctrl.second.toString();\n \n \t\tswitch (ctrl.first) {\n-\t\tcase controls::AE_ENABLE: {\n+\t\tcase controls::EXPOSURE_TIME_MODE: {\n \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(\n \t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n \t\t\tif (!agc) {\n \t\t\t\tLOG(IPARPI, Warning)\n-\t\t\t\t\t<< \"Could not set AE_ENABLE - no AGC algorithm\";\n+\t\t\t\t\t<< \"Could not set EXPOSURE_TIME_MODE - no AGC algorithm\";\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tif (ctrl.second.get<bool>() == false)\n-\t\t\t\tagc->disableAuto();\n+\t\t\tif (ctrl.second.get<int32_t>() == controls::ExposureTimeModeManual)\n+\t\t\t\tagc->disableAutoShutter();\n \t\t\telse\n-\t\t\t\tagc->enableAuto();\n+\t\t\t\tagc->enableAutoShutter();\n \n-\t\t\tlibcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());\n+\t\t\tlibcameraMetadata_.set(controls::ExposureTimeMode,\n+\t\t\t\t\t ctrl.second.get<int32_t>());\n \t\t\tbreak;\n \t\t}\n \n@@ -787,6 +793,13 @@ void IpaBase::applyControls(const ControlList &controls)\n \t\t\t\tbreak;\n \t\t\t}\n \n+\t\t\t/*\n+\t\t\t * Ignore manual exposure time when the auto exposure\n+\t\t\t * algorithm is running.\n+\t\t\t */\n+\t\t\tif (agc->autoShutterEnabled())\n+\t\t\t\tbreak;\n+\n \t\t\t/* The control provides units of microseconds. */\n \t\t\tagc->setFixedShutter(0, ctrl.second.get<int32_t>() * 1.0us);\n \n@@ -794,6 +807,25 @@ void IpaBase::applyControls(const ControlList &controls)\n \t\t\tbreak;\n \t\t}\n \n+\t\tcase controls::ANALOGUE_GAIN_MODE: {\n+\t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(\n+\t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n+\t\t\tif (!agc) {\n+\t\t\t\tLOG(IPARPI, Warning)\n+\t\t\t\t\t<< \"Could not set ANALOGUE_GAIN_MODE - no AGC algorithm\";\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tif (ctrl.second.get<int32_t>() == controls::AnalogueGainModeManual)\n+\t\t\t\tagc->disableAutoGain();\n+\t\t\telse\n+\t\t\t\tagc->enableAutoGain();\n+\n+\t\t\tlibcameraMetadata_.set(controls::AnalogueGainMode,\n+\t\t\t\t\t ctrl.second.get<int32_t>());\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tcase controls::ANALOGUE_GAIN: {\n \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(\n \t\t\t\tcontroller_.getAlgorithm(\"agc\"));\n@@ -803,6 +835,13 @@ void IpaBase::applyControls(const ControlList &controls)\n \t\t\t\tbreak;\n \t\t\t}\n \n+\t\t\t/*\n+\t\t\t * Ignore manual analogue gain value when the auto gain\n+\t\t\t * algorithm is running.\n+\t\t\t */\n+\t\t\tif (agc->autoGainEnabled())\n+\t\t\t\tbreak;\n+\n \t\t\tagc->setFixedAnalogueGain(0, ctrl.second.get<float>());\n \n \t\t\tlibcameraMetadata_.set(controls::AnalogueGain,\n@@ -859,6 +898,13 @@ void IpaBase::applyControls(const ControlList &controls)\n \t\t\t\tbreak;\n \t\t\t}\n \n+\t\t\t/*\n+\t\t\t * Ignore AE_EXPOSURE_MODE if the shutter or the gain\n+\t\t\t * are in auto mode.\n+\t\t\t */\n+\t\t\tif (agc->autoShutterEnabled() || agc->autoGainEnabled())\n+\t\t\t\tbreak;\n+\n \t\t\tint32_t idx = ctrl.second.get<int32_t>();\n \t\t\tif (ExposureModeTable.count(idx)) {\n \t\t\t\tagc->setExposureMode(ExposureModeTable.at(idx));\n@@ -1319,9 +1365,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n \t}\n \n \tAgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>(\"agc.prepare_status\");\n-\tif (agcPrepareStatus) {\n-\t\tlibcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);\n+\tif (agcPrepareStatus)\n \t\tlibcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);\n+\n+\tRPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(\n+\t\tcontroller_.getAlgorithm(\"agc\"));\n+\tif (agc) {\n+\t\tif (!agc->autoShutterEnabled() && !agc->autoGainEnabled())\n+\t\t\tlibcameraMetadata_.set(controls::AeState, controls::AeStateIdle);\n+\t\telse if (agcPrepareStatus)\n+\t\t\tlibcameraMetadata_.set(controls::AeState,\n+\t\t\t\t\t agcPrepareStatus->locked\n+\t\t\t\t\t\t ? controls::AeStateConverged\n+\t\t\t\t\t\t : controls::AeStateSearching);\n \t}\n \n \tLuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>(\"lux.status\");\ndiff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h\nindex 1132de7e050e..64ab024beecd 100644\n--- a/src/ipa/rpi/controller/agc_algorithm.h\n+++ b/src/ipa/rpi/controller/agc_algorithm.h\n@@ -30,8 +30,12 @@ public:\n \tvirtual void setMeteringMode(std::string const &meteringModeName) = 0;\n \tvirtual void setExposureMode(std::string const &exposureModeName) = 0;\n \tvirtual void setConstraintMode(std::string const &contraintModeName) = 0;\n-\tvirtual void enableAuto() = 0;\n-\tvirtual void disableAuto() = 0;\n+\tvirtual void enableAutoShutter() = 0;\n+\tvirtual void disableAutoShutter() = 0;\n+\tvirtual bool autoShutterEnabled() const = 0;\n+\tvirtual void enableAutoGain() = 0;\n+\tvirtual void disableAutoGain() = 0;\n+\tvirtual bool autoGainEnabled() const = 0;\n \tvirtual void setActiveChannels(const std::vector<unsigned int> &activeChannels) = 0;\n };\n \ndiff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\nindex fcf7aec99375..6b038a0197be 100644\n--- a/src/ipa/rpi/controller/rpi/agc.cpp\n+++ b/src/ipa/rpi/controller/rpi/agc.cpp\n@@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const\n \treturn 0;\n }\n \n-void Agc::disableAuto()\n+void Agc::disableAutoShutter()\n {\n-\tLOG(RPiAgc, Debug) << \"disableAuto\";\n+\tLOG(RPiAgc, Debug) << \"disableAutoShutter\";\n \n \t/* All channels are enabled/disabled together. */\n \tfor (auto &data : channelData_)\n-\t\tdata.channel.disableAuto();\n+\t\tdata.channel.disableAutoShutter();\n }\n \n-void Agc::enableAuto()\n+void Agc::enableAutoShutter()\n {\n-\tLOG(RPiAgc, Debug) << \"enableAuto\";\n+\tLOG(RPiAgc, Debug) << \"enableAutoShutter\";\n \n \t/* All channels are enabled/disabled together. */\n \tfor (auto &data : channelData_)\n-\t\tdata.channel.enableAuto();\n+\t\tdata.channel.enableAutoShutter();\n+}\n+\n+bool Agc::autoShutterEnabled() const\n+{\n+\tLOG(RPiAgc, Debug) << \"autoShutterEnabled\";\n+\n+\t/*\n+\t * We always have at least one channel, and since all channels are\n+\t * enabled and disabled together we can simply check the first one.\n+\t */\n+\treturn channelData_[0].channel.autoShutterEnabled();\n+}\n+\n+void Agc::disableAutoGain()\n+{\n+\tLOG(RPiAgc, Debug) << \"disableAutoGain\";\n+\n+\t/* All channels are enabled/disabled together. */\n+\tfor (auto &data : channelData_)\n+\t\tdata.channel.disableAutoGain();\n+}\n+\n+void Agc::enableAutoGain()\n+{\n+\tLOG(RPiAgc, Debug) << \"enableAutoGain\";\n+\n+\t/* All channels are enabled/disabled together. */\n+\tfor (auto &data : channelData_)\n+\t\tdata.channel.enableAutoGain();\n+}\n+\n+bool Agc::autoGainEnabled() const\n+{\n+\tLOG(RPiAgc, Debug) << \"autoGainEnabled\";\n+\n+\t/*\n+\t * We always have at least one channel, and since all channels are\n+\t * enabled and disabled together we can simply check the first one.\n+\t */\n+\treturn channelData_[0].channel.autoGainEnabled();\n }\n \n unsigned int Agc::getConvergenceFrames() const\ndiff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\nindex 5d056f02e2ac..f7519074b022 100644\n--- a/src/ipa/rpi/controller/rpi/agc.h\n+++ b/src/ipa/rpi/controller/rpi/agc.h\n@@ -40,8 +40,12 @@ public:\n \tvoid setMeteringMode(std::string const &meteringModeName) override;\n \tvoid setExposureMode(std::string const &exposureModeName) override;\n \tvoid setConstraintMode(std::string const &contraintModeName) override;\n-\tvoid enableAuto() override;\n-\tvoid disableAuto() override;\n+\tvoid enableAutoShutter() override;\n+\tvoid disableAutoShutter() override;\n+\tbool autoShutterEnabled() const override;\n+\tvoid enableAutoGain() override;\n+\tvoid disableAutoGain() override;\n+\tbool autoGainEnabled() const override;\n \tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata) override;\n \tvoid prepare(Metadata *imageMetadata) override;\n \tvoid process(StatisticsPtr &stats, Metadata *imageMetadata) override;\ndiff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\nindex c9df9b5b6e1d..bea08252029f 100644\n--- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n+++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n@@ -316,18 +316,36 @@ int AgcChannel::read(const libcamera::YamlObject ¶ms,\n \treturn 0;\n }\n \n-void AgcChannel::disableAuto()\n+void AgcChannel::disableAutoShutter()\n {\n \tfixedShutter_ = status_.shutterTime;\n-\tfixedAnalogueGain_ = status_.analogueGain;\n }\n \n-void AgcChannel::enableAuto()\n+void AgcChannel::enableAutoShutter()\n {\n \tfixedShutter_ = 0s;\n+}\n+\n+bool AgcChannel::autoShutterEnabled() const\n+{\n+\treturn fixedShutter_ == 0s;\n+}\n+\n+void AgcChannel::disableAutoGain()\n+{\n+\tfixedAnalogueGain_ = status_.analogueGain;\n+}\n+\n+void AgcChannel::enableAutoGain()\n+{\n \tfixedAnalogueGain_ = 0;\n }\n \n+bool AgcChannel::autoGainEnabled() const\n+{\n+\treturn fixedAnalogueGain_ == 0;\n+}\n+\n unsigned int AgcChannel::getConvergenceFrames() const\n {\n \t/*\ndiff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\nindex 58368889e6a2..5548d772725d 100644\n--- a/src/ipa/rpi/controller/rpi/agc_channel.h\n+++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n@@ -96,8 +96,12 @@ public:\n \tvoid setMeteringMode(std::string const &meteringModeName);\n \tvoid setExposureMode(std::string const &exposureModeName);\n \tvoid setConstraintMode(std::string const &contraintModeName);\n-\tvoid enableAuto();\n-\tvoid disableAuto();\n+\tvoid enableAutoShutter();\n+\tvoid disableAutoShutter();\n+\tbool autoShutterEnabled() const;\n+\tvoid enableAutoGain();\n+\tvoid disableAutoGain();\n+\tbool autoGainEnabled() const;\n \tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata);\n \tvoid prepare(Metadata *imageMetadata);\n \tvoid process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,\n", "prefixes": [ "v3", "5/8" ] }