From patchwork Tue Mar 19 12:05:12 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Klug X-Patchwork-Id: 19751 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 95F68C32C3 for ; Tue, 19 Mar 2024 12:05:44 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 010F762D8E; Tue, 19 Mar 2024 13:05:44 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="jVnGyAVL"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 39B1062CEC for ; Tue, 19 Mar 2024 13:05:30 +0100 (CET) Received: from jasper.fritz.box (unknown [IPv6:2a00:6020:448c:6c00:1478:344b:8fcb:baf5]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 86C87480; Tue, 19 Mar 2024 13:05:03 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1710849903; bh=Lu4hJ17sxhpzAJDwsSY3jzJ+WHfKBnj/pqZDaUS7apc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=jVnGyAVLZWdSCX8A8yITTso5C8TyB/nlG4flvDeLW3Wni7FxStmZQxWsqZxZ1V2dE h1snP4DBx2SbqhBoXuVn9tMLM49tv0U69TUT/EkRnjBJgu+I9msdkLwsGJE8LPeynv 8KcJv9Dy/8TImOtn6a0Uuz+rOhtKrngCr5o10gqI= From: Stefan Klug To: libcamera-devel@lists.libcamera.org Cc: Stefan Klug Subject: [PATCH v3 11/16] libcamera: delayed_controls: Rework delayed controls 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 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Functional changes: - Requests need to be queued for every frame. That was already true in the old implementation. Still the unit tests did a applyControls before the first push which seems counterintuitive - The startup phase is no longer treated as special case - Controls are now pushed together with the sequence number where they should be active. This way we can detect when the system gets out of sync - Controls for a given sequence number can be updated multiple times as long as they are not yet sent out - If a request is too late, controls get delayed but they are not lost - Requests attached to frame 0 don't get lost (they get delayed by maxDelay frames) Signed-off-by: Stefan Klug --- include/libcamera/internal/delayed_controls.h | 2 +- src/libcamera/delayed_controls.cpp | 163 ++++++++++++++---- test/delayed_controls.cpp | 67 +++++++ 3 files changed, 194 insertions(+), 38 deletions(-) diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h index db5c29e9..e2decbcc 100644 --- a/include/libcamera/internal/delayed_controls.h +++ b/include/libcamera/internal/delayed_controls.h @@ -29,7 +29,7 @@ public: void reset(ControlList *controls = nullptr); - bool push(const ControlList &controls); + bool push(const ControlList &controls, std::optional sequence = std::nullopt); ControlList get(uint32_t sequence); void applyControls(uint32_t sequence); diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp index e325b6b2..3fd5a0f7 100644 --- a/src/libcamera/delayed_controls.cpp +++ b/src/libcamera/delayed_controls.cpp @@ -104,6 +104,8 @@ DelayedControls::DelayedControls(V4L2Device *device, maxDelay_ = std::max(maxDelay_, controlParams_[id].delay); } + LOG(DelayedControls, Debug) << "Maximum delay: " << maxDelay_; + reset(); } @@ -117,8 +119,9 @@ DelayedControls::DelayedControls(V4L2Device *device, */ void DelayedControls::reset(ControlList *controls) { - queueIndex_ = 1; - writeIndex_ = 0; + queueIndex_ = 0; + /* Frames up to maxDelay_ will be based on sensor init values. */ + writeIndex_ = maxDelay_; /* Retrieve control as reported by the device. */ std::vector ids; @@ -149,6 +152,9 @@ void DelayedControls::reset(ControlList *controls) */ values_[id][0] = Info(ctrl.second, 0, false); } + + /* Propagate initial state */ + fillValues(0, writeIndex_); } /** @@ -207,17 +213,78 @@ void DelayedControls::fillValues(unsigned int fromIndex, unsigned int toIndex) } /** - * \brief Push a set of controls on the queue + * \brief Push a set of controls for a given frame * \param[in] controls List of controls to add to the device queue + * \param[in] sequence_ Sequence to push the \a controls for + + * + * Pushes the controls given by \a controls, to be applied at frame \a sequence. * - * Push a set of controls to the control queue. This increases the control queue - * depth by one. + * If there are controls already queued for that frame, they get updated. + * + * If it's too late for frame \a sequence (controls are already sent to the + * sensor), the system checks if the controls that where written out for frame + * \a sequence are the same as the requested ones. In this case, nothing is + * done. If they differ, the controls get queued for the earliest frame possible + * if no other controls with a higher sequence number are queued for that frame + * already. * * \returns true if \a controls are accepted, or false otherwise */ -bool DelayedControls::push(const ControlList &controls) +bool DelayedControls::push(const ControlList &controls, std::optional sequence_) { - fillValues(queueIndex_ - 1, queueIndex_); + uint32_t sequence = sequence_.value_or(queueIndex_); + + LOG(DelayedControls, Debug) << "push: " << sequence; + auto idMap = controls.idMap(); + if (idMap) { + for (const auto &[id, value] : controls) + LOG(DelayedControls, Debug) << " " << idMap->at(id)->name() + << " : " << value.toString(); + } + + if (sequence < queueIndex_) + LOG(DelayedControls, Debug) << "Got updated data for frame:" << sequence; + + if (sequence > queueIndex_) + LOG(DelayedControls, Warning) << "Hole in queue sequence." + << " This should not happen. Expected: " + << queueIndex_ << " got " << sequence; + + uint32_t updateIndex = sequence; + /* check if its too late for the request */ + if (sequence < writeIndex_) { + /* Check if we can safely ignore the request */ + if (controls.empty() || controlsAreQueued(sequence, controls)) { + if (sequence >= queueIndex_) { + queueIndex_ = sequence + 1; + return true; + } + } else { + LOG(DelayedControls, Debug) << "Controls for frame " << sequence + << " are already in flight. Will be queued for frame " + << writeIndex_; + updateIndex = writeIndex_; + } + } + + if (updateIndex - writeIndex_ >= listSize - maxDelay_) + /* + * The system is in an undefined state now. This will heal itself, + * as soon as all controls where rewritten + */ + LOG(DelayedControls, Error) << "Queue length exceeded." + << " The system is out of sync. Index to update:" + << updateIndex << " Next Index to apply: " << writeIndex_; + + /* + * Prepare the ringbuffer entries with previous data. + * Data up to [writeIndex_] gets prepared in applyControls. + */ + if (updateIndex > writeIndex_ && updateIndex >= queueIndex_) { + LOG(DelayedControls, Debug) << "Copy from previous " << queueIndex_; + fillValues(queueIndex_ - 1, updateIndex); + } /* Update with new controls. */ const ControlIdMap &idmap = device_->controls().idmap(); @@ -231,20 +298,36 @@ bool DelayedControls::push(const ControlList &controls) const ControlId *id = it->second; - if (controlParams_.find(id) == controlParams_.end()) - return false; - - Info &info = values_[id][queueIndex_]; + if (controlParams_.find(id) == controlParams_.end()) { + LOG(DelayedControls, Error) << "Could not find params for control " + << id << " ignored"; + continue; + } - info = Info(control.second, 0); + Info &info = values_[id][updateIndex]; + /* + * Update the control only if the already existing value stems from a + * request with a sequence number smaller or equal to the current one + */ + if (info.sourceSequence <= sequence) { + info = Info(control.second, sequence); - LOG(DelayedControls, Debug) - << "Queuing " << id->name() - << " to " << info.toString() - << " at index " << queueIndex_; + LOG(DelayedControls, Debug) + << "Queuing " << id->name() + << " to " << info.toString() + << " at index " << updateIndex; + } else { + LOG(DelayedControls, Warning) + << "Skipped update " << id->name() + << " at index " << updateIndex; + } } - queueIndex_++; + LOG(DelayedControls, Debug) << "Queued frame: " << sequence + << " it will be active in " << updateIndex; + + if (sequence >= queueIndex_) + queueIndex_ = sequence + 1; return true; } @@ -266,19 +349,17 @@ bool DelayedControls::push(const ControlList &controls) */ ControlList DelayedControls::get(uint32_t sequence) { - unsigned int index = std::max(0, sequence - maxDelay_); - + LOG(DelayedControls, Debug) << "get " << sequence << ":"; ControlList out(device_->controls()); for (const auto &ctrl : values_) { const ControlId *id = ctrl.first; - const Info &info = ctrl.second[index]; + const Info &info = ctrl.second[sequence]; out.set(id->id(), info); LOG(DelayedControls, Debug) - << "Reading " << id->name() - << " to " << info.toString() - << " at index " << index; + << " " << id->name() + << ": " << info.toString(); } return out; @@ -286,16 +367,22 @@ ControlList DelayedControls::get(uint32_t sequence) /** * \brief Inform DelayedControls of the start of a new frame - * \param[in] sequence Sequence number of the frame that started + * \param[in] sequence Sequence number of the frame that started (0-based) * - * Inform the state machine that a new frame has started and of its sequence - * number. Any user of these helpers is responsible to inform the helper about - * the start of any frame. This can be connected with ease to the start of a - * exposure (SOE) V4L2 event. + * Inform the state machine that a new frame has started to arrive at the receiver + * (e.g. the sensor started to clock out pixels) and of its sequence + * number. This is usually the earliest point in time to update registers in the + * sensor for upcoming frames. Any user of these helpers is responsible to inform + * the helper about the start of any frame. This can be connected with ease to + * the start of a exposure (SOE) V4L2 event. */ void DelayedControls::applyControls(uint32_t sequence) { - LOG(DelayedControls, Debug) << "frame " << sequence << " started"; + LOG(DelayedControls, Debug) << "Apply controls " << sequence; + if (sequence != writeIndex_ - maxDelay_) + LOG(DelayedControls, Warning) << "Sequence and writeIndex are out of sync." + << " Expected seq: " << writeIndex_ - maxDelay_ + << " got " << sequence; /* * Create control list peeking ahead in the value queue to ensure @@ -305,7 +392,7 @@ void DelayedControls::applyControls(uint32_t sequence) for (auto &ctrl : values_) { const ControlId *id = ctrl.first; unsigned int delayDiff = maxDelay_ - controlParams_[id].delay; - unsigned int index = std::max(0, writeIndex_ - delayDiff); + unsigned int index = writeIndex_ - delayDiff; Info &info = ctrl.second[index]; if (info.updated) { @@ -326,21 +413,23 @@ void DelayedControls::applyControls(uint32_t sequence) } LOG(DelayedControls, Debug) - << "Setting " << id->name() - << " to " << info.toString() - << " at index " << index; + << "Writing " << id->name() + << " (" << info.toString() << ") " + << " for frame " << index; /* Done with this update, so mark as completed. */ info.updated = false; } } - writeIndex_ = sequence + 1; + auto oldWriteIndex = writeIndex_; + writeIndex_ = sequence + maxDelay_ + 1; - while (writeIndex_ > queueIndex_) { + if (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) { LOG(DelayedControls, Debug) - << "Queue is empty, auto queue no-op."; - push({}); + << "Index " << writeIndex_ + << " is not yet queued. Prepare with old state"; + fillValues(oldWriteIndex, writeIndex_); } device_->setControls(&out); diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp index fe183de5..481334e7 100644 --- a/test/delayed_controls.cpp +++ b/test/delayed_controls.cpp @@ -271,6 +271,69 @@ protected: return TestPass; } + int updateTooLateGetsDelayed() + { + std::unordered_map delays = { + { V4L2_CID_BRIGHTNESS, { 2, false } }, + }; + std::unique_ptr delayed = + std::make_unique(dev_.get(), delays); + ControlList ctrls; + + /* Reset control to value that will be first in test. */ + int32_t initial = 4; + ctrls.set(V4L2_CID_BRIGHTNESS, initial); + dev_->setControls(&ctrls); + delayed->reset(); + + int32_t expected = 10; + + delayed->push({}, 0); + delayed->push({}, 1); + /* push a request for frame 2 */ + ctrls.set(V4L2_CID_BRIGHTNESS, 40); + delayed->push(ctrls, 2); + + delayed->applyControls(0); + delayed->applyControls(1); + /* + * update frame 2 to the correct value. But it is too late, delayed + * controls will delay and the value shall be available on frame 4 + */ + ctrls.set(V4L2_CID_BRIGHTNESS, expected); + delayed->push(ctrls, 2); + delayed->applyControls(2); + delayed->applyControls(3); + delayed->applyControls(4); + + int frame = 4; + + ControlList result = delayed->get(frame); + int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get(); + ControlList ctrlsV4L = dev_->getControls({ V4L2_CID_BRIGHTNESS }); + int32_t brightnessV4L = ctrlsV4L.get(V4L2_CID_BRIGHTNESS).get(); + + if (brightness != expected) { + cerr << "Failed " << __func__ + << " frame " << frame + << " expected " << expected + << " got " << brightness + << endl; + return TestFail; + } + + if (brightnessV4L != expected) { + cerr << "Failed " << __func__ + << " frame " << frame + << " expected V4L " << expected + << " got " << brightnessV4L + << endl; + return TestFail; + } + + return TestPass; + } + int dualControlsWithDelay() { static const int maxDelay = 2; @@ -440,6 +503,10 @@ protected: if (ret) failed = true; + ret = updateTooLateGetsDelayed(); + if (ret) + failed = true; + /* Test dual controls with different delays. */ ret = dualControlsWithDelay(); if (ret)