Patch Detail
Show a patch.
GET /api/patches/19708/?format=api
{ "id": 19708, "url": "https://patchwork.libcamera.org/api/patches/19708/?format=api", "web_url": "https://patchwork.libcamera.org/patch/19708/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/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": "<20240313121223.138150-7-stefan.klug@ideasonboard.com>", "date": "2024-03-13T12:12:17", "name": "[v2,06/12] libcamera: delayed_controls: Rework delayed controls implementation", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "3e6dd9c84b79b828f6ce494e7b630b8b48518435", "submitter": { "id": 184, "url": "https://patchwork.libcamera.org/api/people/184/?format=api", "name": "Stefan Klug", "email": "stefan.klug@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/19708/mbox/", "series": [ { "id": 4221, "url": "https://patchwork.libcamera.org/api/series/4221/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4221", "date": "2024-03-13T12:12:11", "name": "Preparation for per-frame-controls and initial tests", "version": 2, "mbox": "https://patchwork.libcamera.org/series/4221/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/19708/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/19708/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 CACCDC32A3\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 13 Mar 2024 12:12:58 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3570A62C9B;\n\tWed, 13 Mar 2024 13:12:58 +0100 (CET)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77E2162C95\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 13 Mar 2024 13:12:45 +0100 (CET)", "from jasper.fritz.box (unknown\n\t[IPv6:2a00:6020:448c:6c00:9b07:31b5:38e1:e957])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D6EFFA8F;\n\tWed, 13 Mar 2024 13:12:22 +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=\"f75teChM\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710331943;\n\tbh=KrTXobubEcNjNHEuv7yp0VY2NjrjW5FghfhtaFzy0pY=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=f75teChM2Q9Cn66jNPd0/9CZEZmgt4peSi6n9HGSHZb2beds/fM2KsOlJYzHCEIMZ\n\tRwKdAXT37y8e1JSJd7hg7jkRxqiljscyc58AqPWlh2Cb36mE5aYA6+cyCsDPrJjF9p\n\tCrMaNAj5MO6EcQBZfPQtJE+YyaFQsBe8o1KJL/kA=", "From": "Stefan Klug <stefan.klug@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Subject": "[PATCH v2 06/12] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation", "Date": "Wed, 13 Mar 2024 13:12:17 +0100", "Message-Id": "<20240313121223.138150-7-stefan.klug@ideasonboard.com>", "X-Mailer": "git-send-email 2.40.1", "In-Reply-To": "<20240313121223.138150-1-stefan.klug@ideasonboard.com>", "References": "<20240313121223.138150-1-stefan.klug@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>", "Cc": "Stefan Klug <stefan.klug@ideasonboard.com>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "Functional changes:\n- Requests need to be queued for every frame\n- The startup phase is no longer treated as special case\n- Requests carry a sequence number, so that we can detect when the system\n gets out of sync\n- Controls for a given sequence number can be updated multiple times\n as long as they are not yet sent out\n- If it's too late, controls get delayed but they are not lost\n- Requests attached to frame 0 don't get lost\n\nTechnical notes:\nA sourceSequence_ replaces the updated flag to be able to track from which\nframe the update stems. This is needed for the following use case:\nAssume e.g. On frame 10 an ExposureTime=42 is queued because the user wants\nmanual exposure. Now the agc gets the stats for frame 8 (where auto\nregulation is still active) and pushes a new ExposureTime for frame 9.\nFrame 9 was already sent out, so it gets delayed to frame 11 (assuming a\ndelay of 2 on ExposureTime). This would revert the request from frame 10.\nTaking the sourceSequence into account, the delayed request from frame\n9 will be discarded, which is correct.\n\nSigned-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n---\n include/libcamera/internal/delayed_controls.h | 12 +-\n src/libcamera/delayed_controls.cpp | 207 ++++++++++++++----\n 2 files changed, 174 insertions(+), 45 deletions(-)", "diff": "diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\nindex 4f8d2424..2738e8bf 100644\n--- a/include/libcamera/internal/delayed_controls.h\n+++ b/include/libcamera/internal/delayed_controls.h\n@@ -30,25 +30,29 @@ public:\n \tvoid reset();\n \n \tbool push(const ControlList &controls);\n+\tbool pushForFrame(uint32_t sequence, const ControlList &controls);\n \tControlList get(uint32_t sequence);\n \n \tvoid applyControls(uint32_t sequence);\n \n private:\n+\tbool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);\n+\n \tclass Info : public ControlValue\n \t{\n \tpublic:\n \t\tInfo()\n-\t\t\t: updated(false)\n+\t\t\t: sourceSequence_(0)\n \t\t{\n \t\t}\n \n-\t\tInfo(const ControlValue &v, bool updated_ = true)\n-\t\t\t: ControlValue(v), updated(updated_)\n+\t\tInfo(const ControlValue &v, std::optional<uint32_t> sourceSequence = std::nullopt)\n+\t\t\t: ControlValue(v), sourceSequence_(sourceSequence)\n \t\t{\n \t\t}\n \n-\t\tbool updated;\n+\t\t/* The sequence id, this info stems from*/\n+\t\tstd::optional<uint32_t> sourceSequence_;\n \t};\n \n \t/* \\todo Make the listSize configurable at instance creation time. */\ndiff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\nindex 86571cd4..59314388 100644\n--- a/src/libcamera/delayed_controls.cpp\n+++ b/src/libcamera/delayed_controls.cpp\n@@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,\n */\n void DelayedControls::reset()\n {\n-\tqueueIndex_ = 1;\n-\twriteIndex_ = 0;\n+\tqueueIndex_ = 0;\n+\t/* Frames up to maxDelay_ will be based on sensor init values. */\n+\twriteIndex_ = maxDelay_;\n \n \t/* Retrieve control as reported by the device. */\n \tstd::vector<uint32_t> ids;\n@@ -133,26 +134,129 @@ void DelayedControls::reset()\n \t\t * Do not mark this control value as updated, it does not need\n \t\t * to be written to to device on startup.\n \t\t */\n-\t\tvalues_[id][0] = Info(ctrl.second, false);\n+\t\tvalues_[id][0] = Info(ctrl.second, 0);\n \t}\n+\n+\t/* Copy state from previous frames. */\n+\tfor (auto &ctrl : values_) {\n+\t\tfor (auto i = queueIndex_; i < writeIndex_; i++) {\n+\t\t\tInfo &info = ctrl.second[i + 1];\n+\t\t\tinfo = ctrl.second[i];\n+\t\t\tinfo.sourceSequence_.reset();\n+\t\t}\n+\t}\n+}\n+\n+/**\n+ * \\brief Helper function to check if controls are queued already\n+ * \\param[in] sequence Sequence number to check\n+ * \\param[in] controls List of controls to compare against\n+ *\n+ * This function checks if the controls queued for frame \\a sequence\n+ * are equal to \\a controls. This is helpful in cases where a control algorithm\n+ * unconditionally queues controls for every frame, but always too late.\n+ * In that case this can be used to check if incoming controls are already queued\n+ * or need to be queued for a later frame.\n+ *\n+ * \\returns true if \\a controls are queued for the given sequence\n+ */\n+bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)\n+{\n+\tauto idMap = controls.idMap();\n+\tif (!idMap) {\n+\t\tLOG(DelayedControls, Warning) << \" idmap is null\";\n+\t\treturn false;\n+\t} else {\n+\t\tfor (const auto &[id, value] : controls) {\n+\t\t\tif (values_[idMap->at(id)][sequence] != value) {\n+\t\t\t\treturn false;\n+\t\t\t}\n+\t\t}\n+\t}\n+\treturn true;\n }\n \n /**\n * \\brief Push a set of controls on the queue\n * \\param[in] controls List of controls to add to the device queue\n+ * \\deprecated This function is deprecated in favour of pushForFrame\n *\n * Push a set of controls to the control queue. This increases the control queue\n * depth by one.\n *\n- * \\returns true if \\a controls are accepted, or false otherwise\n+ * \\returns See return value of DelayedControls::pushForFrame\n */\n bool DelayedControls::push(const ControlList &controls)\n {\n-\t/* Copy state from previous frame. */\n-\tfor (auto &ctrl : values_) {\n-\t\tInfo &info = ctrl.second[queueIndex_];\n-\t\tinfo = values_[ctrl.first][queueIndex_ - 1];\n-\t\tinfo.updated = false;\n+\tLOG(DelayedControls, Debug) << \"Using deprecated function push(controls): \" << queueIndex_;\n+\treturn pushForFrame(queueIndex_, controls);\n+}\n+\n+/**\n+ * \\brief Push a set of controls for a given frame\n+ * \\param[in] sequence Sequence to push the controls for\n+ * \\param[in] controls List of controls to add to the device queue\n+ *\n+ * Pushes the controls given by \\a controls, to be applied at frame \\a sequence.\n+ *\n+ * If there are controls already queued for that frame, these get updated.\n+ *\n+ * If it's too late for frame \\a sequence (controls are already sent to the sensor),\n+ * the system checks if the controls that where written out for frame \\a sequence\n+ * are the same as the requested ones. In this case, nothing is done.\n+ * If they differ, the controls get queued for the earliest frame possible\n+ * if no other controls with a higher sequence number are queued for that frame already.\n+ *\n+ * \\returns true if \\a controls are accepted, or false otherwise\n+ */\n+\n+bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)\n+{\n+\tif (sequence < queueIndex_) {\n+\t\tLOG(DelayedControls, Debug) << \"Got updated data for frame:\" << sequence;\n+\t}\n+\n+\tif (sequence > queueIndex_) {\n+\t\tLOG(DelayedControls, Warning) << \"Hole in queue sequence. This should not happen. Expected: \"\n+\t\t\t\t\t << queueIndex_ << \" got \" << sequence;\n+\t}\n+\n+\tuint32_t updateIndex = sequence;\n+\t/* check if its too late for the request */\n+\tif (sequence < writeIndex_) {\n+\t\t/* Check if we can safely ignore the request */\n+\t\tif (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {\n+\t\t\tif (sequence >= queueIndex_) {\n+\t\t\t\tqueueIndex_ = sequence + 1;\n+\t\t\t\treturn true;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tLOG(DelayedControls, Debug) << \"Controls for frame \" << sequence\n+\t\t\t\t\t\t << \" are already in flight. Will be queued for frame \" << writeIndex_;\n+\t\t\tupdateIndex = writeIndex_;\n+\t\t}\n+\t}\n+\n+\tif (static_cast<signed>(updateIndex) - static_cast<signed>(writeIndex_) >= listSize - static_cast<signed>(maxDelay_)) {\n+\t\t/* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */\n+\t\tLOG(DelayedControls, Error) << \"Queue length exceeded. The system is out of sync. Index to update:\"\n+\t\t\t\t\t << updateIndex << \" Next Index to apply: \" << writeIndex_;\n+\t}\n+\n+\t/**\n+\t * Prepare the ringbuffer entries with previous data.\n+\t * Data up to [writeIndex_] gets prepared in applyControls.\n+\t */\n+\tif (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {\n+\t\tLOG(DelayedControls, Debug) << \"Copy from previous \" << queueIndex_;\n+\t\tfor (auto i = queueIndex_; i < updateIndex; i++) {\n+\t\t\t/* Copy state from previous queue. */\n+\t\t\tfor (auto &ctrl : values_) {\n+\t\t\t\tauto &ringBuffer = ctrl.second;\n+\t\t\t\tringBuffer[i] = ringBuffer[i - 1];\n+\t\t\t\tringBuffer[i].sourceSequence_.reset();\n+\t\t\t}\n+\t\t}\n \t}\n \n \t/* Update with new controls. */\n@@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)\n \n \t\tconst ControlId *id = it->second;\n \n-\t\tif (controlParams_.find(id) == controlParams_.end())\n-\t\t\treturn false;\n-\n-\t\tInfo &info = values_[id][queueIndex_];\n+\t\tif (controlParams_.find(id) == controlParams_.end()) {\n+\t\t\tLOG(DelayedControls, Error) << \"Could not find params for control \" << id << \" ignored\";\n+\t\t\tcontinue;\n+\t\t}\n \n-\t\tinfo = Info(control.second);\n+\t\tInfo &info = values_[id][updateIndex];\n+\t\t/*\n+\t\t * Update the control only, if the already existing value stems from a request\n+\t\t * with a sequence number smaller or equal to the current one\n+\t\t */\n+\t\tif (info.sourceSequence_.value_or(0) <= sequence) {\n+\t\t\tinfo = Info(control.second, sequence);\n \n-\t\tLOG(DelayedControls, Debug)\n-\t\t\t<< \"Queuing \" << id->name()\n-\t\t\t<< \" to \" << info.toString()\n-\t\t\t<< \" at index \" << queueIndex_;\n+\t\t\tLOG(DelayedControls, Debug)\n+\t\t\t\t<< \"Queuing \" << id->name()\n+\t\t\t\t<< \" to \" << info.toString()\n+\t\t\t\t<< \" at index \" << updateIndex;\n+\t\t} else {\n+\t\t\tLOG(DelayedControls, Warning)\n+\t\t\t\t<< \"Skipped update \" << id->name()\n+\t\t\t\t<< \" at index \" << updateIndex;\n+\t\t}\n \t}\n \n-\tqueueIndex_++;\n+\tLOG(DelayedControls, Debug) << \"Queued frame: \" << queueIndex_ << \" it will be active in \" << updateIndex;\n+\n+\tif (sequence >= queueIndex_)\n+\t\tqueueIndex_ = sequence + 1;\n \n \treturn true;\n }\n@@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)\n */\n ControlList DelayedControls::get(uint32_t sequence)\n {\n-\tunsigned int index = std::max<int>(0, sequence - maxDelay_);\n-\n \tControlList out(device_->controls());\n \tfor (const auto &ctrl : values_) {\n \t\tconst ControlId *id = ctrl.first;\n-\t\tconst Info &info = ctrl.second[index];\n+\t\tconst Info &info = ctrl.second[sequence];\n \n \t\tout.set(id->id(), info);\n \n \t\tLOG(DelayedControls, Debug)\n \t\t\t<< \"Reading \" << id->name()\n \t\t\t<< \" to \" << info.toString()\n-\t\t\t<< \" at index \" << index;\n+\t\t\t<< \" at index \" << sequence;\n \t}\n \n \treturn out;\n@@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)\n \n /**\n * \\brief Inform DelayedControls of the start of a new frame\n- * \\param[in] sequence Sequence number of the frame that started\n+ * \\param[in] sequence Sequence number of the frame that started (0-based)\n *\n- * Inform the state machine that a new frame has started and of its sequence\n- * number. Any user of these helpers is responsible to inform the helper about\n- * the start of any frame. This can be connected with ease to the start of a\n- * exposure (SOE) V4L2 event.\n+ * Inform the state machine that a new frame has started to arrive at the receiver\n+ * (e.g. the sensor started to clock out pixels) and of its sequence\n+ * number. This is usually the earliest point in time to update registers in the\n+ * sensor for upcoming frames. Any user of these helpers is responsible to inform\n+ * the helper about the start of any frame. This can be connected with ease to\n+ * the start of a exposure (SOE) V4L2 event.\n */\n void DelayedControls::applyControls(uint32_t sequence)\n {\n-\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n+\tLOG(DelayedControls, Debug) << \"Apply controls \" << sequence;\n+\tif (sequence != writeIndex_ - maxDelay_) {\n+\t\tLOG(DelayedControls, Warning) << \"Sequence and writeIndex are out of sync. Expected seq: \" << writeIndex_ - maxDelay_ << \" got \" << sequence;\n+\t}\n \n \t/*\n \t * Create control list peeking ahead in the value queue to ensure\n@@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)\n \tfor (auto &ctrl : values_) {\n \t\tconst ControlId *id = ctrl.first;\n \t\tunsigned int delayDiff = maxDelay_ - controlParams_[id].delay;\n-\t\tunsigned int index = std::max<int>(0, writeIndex_ - delayDiff);\n+\t\tunsigned int index = writeIndex_ - delayDiff;\n \t\tInfo &info = ctrl.second[index];\n \n-\t\tif (info.updated) {\n+\t\tif (info.sourceSequence_.has_value()) {\n \t\t\tif (controlParams_[id].priorityWrite) {\n \t\t\t\t/*\n \t\t\t\t * This control must be written now, it could\n@@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)\n \t\t\t}\n \n \t\t\tLOG(DelayedControls, Debug)\n-\t\t\t\t<< \"Setting \" << id->name()\n-\t\t\t\t<< \" to \" << info.toString()\n-\t\t\t\t<< \" at index \" << index;\n-\n-\t\t\t/* Done with this update, so mark as completed. */\n-\t\t\tinfo.updated = false;\n+\t\t\t\t<< \"Writing \" << id->name()\n+\t\t\t\t<< \" (\" << info.toString() << \") \"\n+\t\t\t\t<< \" for frame \" << index;\n \t\t}\n \t}\n \n-\twriteIndex_ = sequence + 1;\n+\tauto oldWriteIndex = writeIndex_;\n+\twriteIndex_ = sequence + maxDelay_ + 1;\n \n-\twhile (writeIndex_ > queueIndex_) {\n+\tif (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {\n \t\tLOG(DelayedControls, Debug)\n-\t\t\t<< \"Queue is empty, auto queue no-op.\";\n-\t\tpush({});\n+\t\t\t<< \"Index \" << writeIndex_ << \" is not yet queued. Prepare with old state\";\n+\t\t/* Copy state from previous frames without resetting the sourceSequence */\n+\t\tfor (auto &ctrl : values_) {\n+\t\t\tfor (auto i = oldWriteIndex; i < writeIndex_; i++) {\n+\t\t\t\tInfo &info = ctrl.second[i + 1];\n+\t\t\t\tinfo = values_[ctrl.first][i];\n+\t\t\t}\n+\t\t}\n \t}\n \n \tdevice_->setControls(&out);\n", "prefixes": [ "v2", "06/12" ] }