[PATCH v3 12/16] libcamera: delayed_controls: Ignore delayed request, if there is a newer one

[Jacopo Mondi]

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
>