{"id":19751,"url":"https://patchwork.libcamera.org/api/1.1/patches/19751/?format=json","web_url":"https://patchwork.libcamera.org/patch/19751/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20240319120517.362082-12-stefan.klug@ideasonboard.com>","date":"2024-03-19T12:05:12","name":"[v3,11/16] libcamera: delayed_controls: Rework delayed controls implementation","commit_ref":null,"pull_url":null,"state":"new","archived":false,"hash":"bd46fb612cc44058a567b5378195d561fd4dcc9d","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/1.1/people/184/?format=json","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/19751/mbox/","series":[{"id":4230,"url":"https://patchwork.libcamera.org/api/1.1/series/4230/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=4230","date":"2024-03-19T12:05:01","name":"Preparation for per-frame-controls and initial tests","version":3,"mbox":"https://patchwork.libcamera.org/series/4230/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/19751/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/19751/checks/","tags":{},"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 95F68C32C3\n\tfor ;\n\tTue, 19 Mar 2024 12:05:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 010F762D8E;\n\tTue, 19 Mar 2024 13:05:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39B1062CEC\n\tfor ;\n\tTue, 19 Mar 2024 13:05:30 +0100 (CET)","from jasper.fritz.box (unknown\n\t[IPv6:2a00:6020:448c:6c00:1478:344b:8fcb:baf5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86C87480;\n\tTue, 19 Mar 2024 13:05:03 +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=\"jVnGyAVL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710849903;\n\tbh=Lu4hJ17sxhpzAJDwsSY3jzJ+WHfKBnj/pqZDaUS7apc=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=jVnGyAVLZWdSCX8A8yITTso5C8TyB/nlG4flvDeLW3Wni7FxStmZQxWsqZxZ1V2dE\n\th1snP4DBx2SbqhBoXuVn9tMLM49tv0U69TUT/EkRnjBJgu+I9msdkLwsGJE8LPeynv\n\t8KcJv9Dy/8TImOtn6a0Uuz+rOhtKrngCr5o10gqI=","From":"Stefan Klug ","To":"libcamera-devel@lists.libcamera.org","Cc":"Stefan Klug ","Subject":"[PATCH v3 11/16] libcamera: delayed_controls: Rework delayed\n\tcontrols implementation","Date":"Tue, 19 Mar 2024 13:05:12 +0100","Message-Id":"<20240319120517.362082-12-stefan.klug@ideasonboard.com>","X-Mailer":"git-send-email 2.40.1","In-Reply-To":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>","References":"<20240319120517.362082-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":"","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\" "},"content":"Functional changes:\n- Requests need to be queued for every frame. That was already true in the\n old implementation. Still the unit tests did a applyControls before the\n first push which seems counterintuitive\n- The startup phase is no longer treated as special case\n- Controls are now pushed together with the sequence number where they\n should be active. This way we can detect when the system gets out of\n 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 a request is too late, controls get delayed but they are not lost\n- Requests attached to frame 0 don't get lost (they get delayed by\n maxDelay frames)\n\nSigned-off-by: Stefan Klug \n---\n include/libcamera/internal/delayed_controls.h | 2 +-\n src/libcamera/delayed_controls.cpp | 163 ++++++++++++++----\n test/delayed_controls.cpp | 67 +++++++\n 3 files changed, 194 insertions(+), 38 deletions(-)","diff":"diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\nindex db5c29e9..e2decbcc 100644\n--- a/include/libcamera/internal/delayed_controls.h\n+++ b/include/libcamera/internal/delayed_controls.h\n@@ -29,7 +29,7 @@ public:\n \n \tvoid reset(ControlList *controls = nullptr);\n \n-\tbool push(const ControlList &controls);\n+\tbool push(const ControlList &controls, std::optional sequence = std::nullopt);\n \tControlList get(uint32_t sequence);\n \n \tvoid applyControls(uint32_t sequence);\ndiff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\nindex e325b6b2..3fd5a0f7 100644\n--- a/src/libcamera/delayed_controls.cpp\n+++ b/src/libcamera/delayed_controls.cpp\n@@ -104,6 +104,8 @@ DelayedControls::DelayedControls(V4L2Device *device,\n \t\tmaxDelay_ = std::max(maxDelay_, controlParams_[id].delay);\n \t}\n \n+\tLOG(DelayedControls, Debug) << \"Maximum delay: \" << maxDelay_;\n+\n \treset();\n }\n \n@@ -117,8 +119,9 @@ DelayedControls::DelayedControls(V4L2Device *device,\n */\n void DelayedControls::reset(ControlList *controls)\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 ids;\n@@ -149,6 +152,9 @@ void DelayedControls::reset(ControlList *controls)\n \t\t */\n \t\tvalues_[id][0] = Info(ctrl.second, 0, false);\n \t}\n+\n+\t/* Propagate initial state */\n+\tfillValues(0, writeIndex_);\n }\n \n /**\n@@ -207,17 +213,78 @@ void DelayedControls::fillValues(unsigned int fromIndex, unsigned int toIndex)\n }\n \n /**\n- * \\brief Push a set of controls on the queue\n+ * \\brief Push a set of controls for a given frame\n * \\param[in] controls List of controls to add to the device queue\n+ * \\param[in] sequence_ Sequence to push the \\a controls for\n+\n+ *\n+ * Pushes the controls given by \\a controls, to be applied at frame \\a sequence.\n *\n- * Push a set of controls to the control queue. This increases the control queue\n- * depth by one.\n+ * If there are controls already queued for that frame, they get updated.\n+ *\n+ * If it's too late for frame \\a sequence (controls are already sent to the\n+ * sensor), the system checks if the controls that where written out for frame\n+ * \\a sequence are the same as the requested ones. In this case, nothing is\n+ * done. 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\n+ * already.\n *\n * \\returns true if \\a controls are accepted, or false otherwise\n */\n-bool DelayedControls::push(const ControlList &controls)\n+bool DelayedControls::push(const ControlList &controls, std::optional sequence_)\n {\n-\tfillValues(queueIndex_ - 1, queueIndex_);\n+\tuint32_t sequence = sequence_.value_or(queueIndex_);\n+\n+\tLOG(DelayedControls, Debug) << \"push: \" << sequence;\n+\tauto idMap = controls.idMap();\n+\tif (idMap) {\n+\t\tfor (const auto &[id, value] : controls)\n+\t\t\tLOG(DelayedControls, Debug) << \" \" << idMap->at(id)->name()\n+\t\t\t\t\t\t << \" : \" << value.toString();\n+\t}\n+\n+\tif (sequence < queueIndex_)\n+\t\tLOG(DelayedControls, Debug) << \"Got updated data for frame:\" << sequence;\n+\n+\tif (sequence > queueIndex_)\n+\t\tLOG(DelayedControls, Warning) << \"Hole in queue sequence.\"\n+\t\t\t\t\t << \" This should not happen. Expected: \"\n+\t\t\t\t\t << queueIndex_ << \" got \" << sequence;\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() || controlsAreQueued(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 \"\n+\t\t\t\t\t\t << writeIndex_;\n+\t\t\tupdateIndex = writeIndex_;\n+\t\t}\n+\t}\n+\n+\tif (updateIndex - writeIndex_ >= listSize - maxDelay_)\n+\t\t/*\n+\t\t * The system is in an undefined state now. This will heal itself,\n+\t\t * as soon as all controls where rewritten\n+\t\t */\n+\t\tLOG(DelayedControls, Error) << \"Queue length exceeded.\"\n+\t\t\t\t\t << \" The system is out of sync. Index to update:\"\n+\t\t\t\t\t << updateIndex << \" Next Index to apply: \" << writeIndex_;\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\tfillValues(queueIndex_ - 1, updateIndex);\n+\t}\n \n \t/* Update with new controls. */\n \tconst ControlIdMap &idmap = device_->controls().idmap();\n@@ -231,20 +298,36 @@ 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 \"\n+\t\t\t\t\t\t << id << \" ignored\";\n+\t\t\tcontinue;\n+\t\t}\n \n-\t\tinfo = Info(control.second, 0);\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\n+\t\t * request with a sequence number smaller or equal to the current one\n+\t\t */\n+\t\tif (info.sourceSequence <= 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: \" << sequence\n+\t\t\t\t << \" it will be active in \" << updateIndex;\n+\n+\tif (sequence >= queueIndex_)\n+\t\tqueueIndex_ = sequence + 1;\n \n \treturn true;\n }\n@@ -266,19 +349,17 @@ bool DelayedControls::push(const ControlList &controls)\n */\n ControlList DelayedControls::get(uint32_t sequence)\n {\n-\tunsigned int index = std::max(0, sequence - maxDelay_);\n-\n+\tLOG(DelayedControls, Debug) << \"get \" << sequence << \":\";\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<< \" \" << id->name()\n+\t\t\t<< \": \" << info.toString();\n \t}\n \n \treturn out;\n@@ -286,16 +367,22 @@ 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.\"\n+\t\t\t\t\t << \" Expected seq: \" << writeIndex_ - maxDelay_\n+\t\t\t\t\t << \" got \" << sequence;\n \n \t/*\n \t * Create control list peeking ahead in the value queue to ensure\n@@ -305,7 +392,7 @@ 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(0, writeIndex_ - delayDiff);\n+\t\tunsigned int index = writeIndex_ - delayDiff;\n \t\tInfo &info = ctrl.second[index];\n \n \t\tif (info.updated) {\n@@ -326,21 +413,23 @@ 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+\t\t\t\t<< \"Writing \" << id->name()\n+\t\t\t\t<< \" (\" << info.toString() << \") \"\n+\t\t\t\t<< \" for frame \" << index;\n \n \t\t\t/* Done with this update, so mark as completed. */\n \t\t\tinfo.updated = false;\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_\n+\t\t\t<< \" is not yet queued. Prepare with old state\";\n+\t\tfillValues(oldWriteIndex, writeIndex_);\n \t}\n \n \tdevice_->setControls(&out);\ndiff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp\nindex fe183de5..481334e7 100644\n--- a/test/delayed_controls.cpp\n+++ b/test/delayed_controls.cpp\n@@ -271,6 +271,69 @@ protected:\n \t\treturn TestPass;\n \t}\n \n+\tint updateTooLateGetsDelayed()\n+\t{\n+\t\tstd::unordered_map delays = {\n+\t\t\t{ V4L2_CID_BRIGHTNESS, { 2, false } },\n+\t\t};\n+\t\tstd::unique_ptr delayed =\n+\t\t\tstd::make_unique(dev_.get(), delays);\n+\t\tControlList ctrls;\n+\n+\t\t/* Reset control to value that will be first in test. */\n+\t\tint32_t initial = 4;\n+\t\tctrls.set(V4L2_CID_BRIGHTNESS, initial);\n+\t\tdev_->setControls(&ctrls);\n+\t\tdelayed->reset();\n+\n+\t\tint32_t expected = 10;\n+\n+\t\tdelayed->push({}, 0);\n+\t\tdelayed->push({}, 1);\n+\t\t/* push a request for frame 2 */\n+\t\tctrls.set(V4L2_CID_BRIGHTNESS, 40);\n+\t\tdelayed->push(ctrls, 2);\n+\n+\t\tdelayed->applyControls(0);\n+\t\tdelayed->applyControls(1);\n+\t\t/*\n+\t\t * update frame 2 to the correct value. But it is too late, delayed\n+\t\t * controls will delay and the value shall be available on frame 4\n+\t\t */\n+\t\tctrls.set(V4L2_CID_BRIGHTNESS, expected);\n+\t\tdelayed->push(ctrls, 2);\n+\t\tdelayed->applyControls(2);\n+\t\tdelayed->applyControls(3);\n+\t\tdelayed->applyControls(4);\n+\n+\t\tint frame = 4;\n+\n+\t\tControlList result = delayed->get(frame);\n+\t\tint32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get();\n+\t\tControlList ctrlsV4L = dev_->getControls({ V4L2_CID_BRIGHTNESS });\n+\t\tint32_t brightnessV4L = ctrlsV4L.get(V4L2_CID_BRIGHTNESS).get();\n+\n+\t\tif (brightness != expected) {\n+\t\t\tcerr << \"Failed \" << __func__\n+\t\t\t << \" frame \" << frame\n+\t\t\t << \" expected \" << expected\n+\t\t\t << \" got \" << brightness\n+\t\t\t << endl;\n+\t\t\treturn TestFail;\n+\t\t}\n+\n+\t\tif (brightnessV4L != expected) {\n+\t\t\tcerr << \"Failed \" << __func__\n+\t\t\t << \" frame \" << frame\n+\t\t\t << \" expected V4L \" << expected\n+\t\t\t << \" got \" << brightnessV4L\n+\t\t\t << endl;\n+\t\t\treturn TestFail;\n+\t\t}\n+\n+\t\treturn TestPass;\n+\t}\n+\n \tint dualControlsWithDelay()\n \t{\n \t\tstatic const int maxDelay = 2;\n@@ -440,6 +503,10 @@ protected:\n \t\tif (ret)\n \t\t\tfailed = true;\n \n+\t\tret = updateTooLateGetsDelayed();\n+\t\tif (ret)\n+\t\t\tfailed = true;\n+\n \t\t/* Test dual controls with different delays. */\n \t\tret = dualControlsWithDelay();\n \t\tif (ret)\n","prefixes":["v3","11/16"]}