| Message ID | 20240319120517.362082-13-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Tue, Mar 19, 2024 at 01:05:13PM +0100, Stefan Klug wrote: > Assume for frame 10 a ExposureTime=42 is queued. We have a delay of 2. After > receiving stats for frame 8, the isp tries to queue for frame 9. As it's too > lae for that frame, delayed controls schedules the update for frame 11. But as > frame 10 was already queued, the request should be discarded. I think this use case is the main reason for the whole delayed control rework, isn't it ? So please let's try to first clarify if it is valid. Let's expand the above text: > Assume for frame 10 a ExposureTime=42 is queued. We have a delay of 2. After At what time is the request queued ? During the exposure of which frame ? > receiving stats for frame 8, the isp tries to queue for frame 9. As it's too s/isp/IPA ? Otherwise I can't make a sense out of this phrase. If so, frame 8 has been processed by the ISP. If frame 8 has been processed, it means (at best) frame 9 is already exposing. Whatever settings we apply at this time, they will get realized at (10 + max_delay). > lae for that frame, delayed controls schedules the update for frame 11. But as Why frame 11 ? > frame 10 was already queued, the request should be discarded. frame 10 is queued to what ? To the ISP ? Let's calrify this use case first then let's discuss the changes to delayed controls. Thanks j > --- > include/libcamera/internal/delayed_controls.h | 2 + > src/libcamera/delayed_controls.cpp | 19 +++++- > test/delayed_controls.cpp | 66 +++++++++++++++++++ > 3 files changed, 86 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h > index e2decbcc..91c9415a 100644 > --- a/include/libcamera/internal/delayed_controls.h > +++ b/include/libcamera/internal/delayed_controls.h > @@ -67,6 +67,8 @@ private: > { > return std::array<Info, listSize>::operator[](index % listSize); > } > + > + unsigned int largestValidIndex; > }; > > bool controlsAreQueued(unsigned int frame, const ControlList &controls); > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > index 3fd5a0f7..7f2bb479 100644 > --- a/src/libcamera/delayed_controls.cpp > +++ b/src/libcamera/delayed_controls.cpp > @@ -151,6 +151,7 @@ void DelayedControls::reset(ControlList *controls) > * to be written to to device on startup. > */ > values_[id][0] = Info(ctrl.second, 0, false); > + values_[id].largestValidIndex = 0; > } > > /* Propagate initial state */ > @@ -304,18 +305,34 @@ bool DelayedControls::push(const ControlList &controls, std::optional<uint32_t> > continue; > } > > - Info &info = values_[id][updateIndex]; > + ControlRingBuffer &ring = values_[id]; > + Info &info = ring[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); > + if (updateIndex > ring.largestValidIndex) > + ring.largestValidIndex = updateIndex; > > LOG(DelayedControls, Debug) > << "Queuing " << id->name() > << " to " << info.toString() > << " at index " << updateIndex; > + > + /* fill up the next indices with the new information */ > + unsigned int i = updateIndex + 1; > + while (i <= ring.largestValidIndex) { > + LOG(DelayedControls, Error) << "update " << i; > + Info &next = ring[i]; > + if (next.sourceSequence <= sequence) > + next = info; > + else > + break; > + > + i++; > + } > } else { > LOG(DelayedControls, Warning) > << "Skipped update " << id->name() > diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp > index 481334e7..7d671a0e 100644 > --- a/test/delayed_controls.cpp > +++ b/test/delayed_controls.cpp > @@ -271,6 +271,68 @@ protected: > return TestPass; > } > > + int updateTooLateMustSometimesBeIgnored() > + { > + std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = { > + { V4L2_CID_BRIGHTNESS, { 2, false } }, > + }; > + std::unique_ptr<DelayedControls> delayed = > + std::make_unique<DelayedControls>(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); > + ctrls.set(V4L2_CID_BRIGHTNESS, expected); > + delayed->push(ctrls, 2); > + delayed->applyControls(0); /* puts 10 on the bus */ > + > + /* > + * Post an update for frame 1. It's too late to fulfill that request, > + * delayed controls will therefore try to delay it to frame 3. But as > + * frame 2 is already queued, the update must be dropped. > + */ > + ctrls.set(V4L2_CID_BRIGHTNESS, 20); > + delayed->push(ctrls, 1); > + delayed->applyControls(1); > + delayed->applyControls(2); > + delayed->applyControls(3); > + > + int frame = 3; > + > + ControlList result = delayed->get(frame); > + int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); > + ControlList ctrlsV4L = dev_->getControls({ V4L2_CID_BRIGHTNESS }); > + int32_t brightnessV4L = ctrlsV4L.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); > + > + 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 updateTooLateGetsDelayed() > { > std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = { > @@ -503,6 +565,10 @@ protected: > if (ret) > failed = true; > > + ret = updateTooLateMustSometimesBeIgnored(); > + if (ret) > + failed = true; > + > ret = updateTooLateGetsDelayed(); > if (ret) > failed = true; > -- > 2.40.1 >
diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h index e2decbcc..91c9415a 100644 --- a/include/libcamera/internal/delayed_controls.h +++ b/include/libcamera/internal/delayed_controls.h @@ -67,6 +67,8 @@ private: { return std::array<Info, listSize>::operator[](index % listSize); } + + unsigned int largestValidIndex; }; bool controlsAreQueued(unsigned int frame, const ControlList &controls); diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp index 3fd5a0f7..7f2bb479 100644 --- a/src/libcamera/delayed_controls.cpp +++ b/src/libcamera/delayed_controls.cpp @@ -151,6 +151,7 @@ void DelayedControls::reset(ControlList *controls) * to be written to to device on startup. */ values_[id][0] = Info(ctrl.second, 0, false); + values_[id].largestValidIndex = 0; } /* Propagate initial state */ @@ -304,18 +305,34 @@ bool DelayedControls::push(const ControlList &controls, std::optional<uint32_t> continue; } - Info &info = values_[id][updateIndex]; + ControlRingBuffer &ring = values_[id]; + Info &info = ring[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); + if (updateIndex > ring.largestValidIndex) + ring.largestValidIndex = updateIndex; LOG(DelayedControls, Debug) << "Queuing " << id->name() << " to " << info.toString() << " at index " << updateIndex; + + /* fill up the next indices with the new information */ + unsigned int i = updateIndex + 1; + while (i <= ring.largestValidIndex) { + LOG(DelayedControls, Error) << "update " << i; + Info &next = ring[i]; + if (next.sourceSequence <= sequence) + next = info; + else + break; + + i++; + } } else { LOG(DelayedControls, Warning) << "Skipped update " << id->name() diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp index 481334e7..7d671a0e 100644 --- a/test/delayed_controls.cpp +++ b/test/delayed_controls.cpp @@ -271,6 +271,68 @@ protected: return TestPass; } + int updateTooLateMustSometimesBeIgnored() + { + std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = { + { V4L2_CID_BRIGHTNESS, { 2, false } }, + }; + std::unique_ptr<DelayedControls> delayed = + std::make_unique<DelayedControls>(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); + ctrls.set(V4L2_CID_BRIGHTNESS, expected); + delayed->push(ctrls, 2); + delayed->applyControls(0); /* puts 10 on the bus */ + + /* + * Post an update for frame 1. It's too late to fulfill that request, + * delayed controls will therefore try to delay it to frame 3. But as + * frame 2 is already queued, the update must be dropped. + */ + ctrls.set(V4L2_CID_BRIGHTNESS, 20); + delayed->push(ctrls, 1); + delayed->applyControls(1); + delayed->applyControls(2); + delayed->applyControls(3); + + int frame = 3; + + ControlList result = delayed->get(frame); + int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); + ControlList ctrlsV4L = dev_->getControls({ V4L2_CID_BRIGHTNESS }); + int32_t brightnessV4L = ctrlsV4L.get(V4L2_CID_BRIGHTNESS).get<int32_t>(); + + 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 updateTooLateGetsDelayed() { std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = { @@ -503,6 +565,10 @@ protected: if (ret) failed = true; + ret = updateTooLateMustSometimesBeIgnored(); + if (ret) + failed = true; + ret = updateTooLateGetsDelayed(); if (ret) failed = true;