[v4,3/3] pipeline: rpi: Make control lists in requests properly atomic

26674 diff mbox series

When a request is about to be processed, this commit separates
"delayed controls" (camera-related ones that take "a few frames" to
apply) from "immediate controls" (ISP-related controls) that will
happen instantly. It does so in a copy of the request's control list,
so as to avoid disturbing the request's original version.

The immediate controls are held back until the delayed controls that
they were submitted with, have happened. This means all the controls
submitted in a request happen atomically.

We therefore already have the sequence number of the request whose
controls have just been applied, so we additionally attach this to the
current request as "ControlId" metadata.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp               | 24 ++++++---
 .../pipeline/rpi/common/pipeline_base.cpp     | 49 +++++++++++++++++++
 .../pipeline/rpi/common/pipeline_base.h       |  9 ++++
 src/libcamera/pipeline/rpi/pisp/pisp.cpp      |  9 ++--
 src/libcamera/pipeline/rpi/vc4/vc4.cpp        |  9 ++--
 5 files changed, 86 insertions(+), 14 deletions(-)

Hi David

On Thu, May 07, 2026 at 01:20:36PM +0100, David Plowman wrote:
> When a request is about to be processed, this commit separates
> "delayed controls" (camera-related ones that take "a few frames" to
> apply) from "immediate controls" (ISP-related controls) that will
> happen instantly. It does so in a copy of the request's control list,
> so as to avoid disturbing the request's original version.
>
> The immediate controls are held back until the delayed controls that
> they were submitted with, have happened. This means all the controls
> submitted in a request happen atomically.
>
> We therefore already have the sequence number of the request whose
> controls have just been applied, so we additionally attach this to the
> current request as "ControlId" metadata.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp               | 24 ++++++---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 49 +++++++++++++++++++
>  .../pipeline/rpi/common/pipeline_base.h       |  9 ++++
>  src/libcamera/pipeline/rpi/pisp/pisp.cpp      |  9 ++--
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp        |  9 ++--
>  5 files changed, 86 insertions(+), 14 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index faa77719..dacafa57 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -431,6 +431,15 @@ void IpaBase::prepareIsp(const PrepareParams &params)
>  	rpiMetadata.clear();
>  	fillDeviceStatus(params.sensorControls, ipaContext);
>
> +	/*
> +	 * When there are controls, it's important that we don't skip running the
> +	 * IPAs, as that can mess with synchronisation. Crucially though, we need
> +	 * to know whether there were controls when this comes back as the
> +	 * _delayed_ metadata, hence why we flag this in the metadata itself.
> +	 */
> +	if (!params.requestControls.empty())
> +		rpiMetadata.set("ipa.request_controls", true);
> +
>  	if (params.buffers.embedded) {
>  		/*
>  		 * Pipeline handler has supplied us with an embedded data buffer,
> @@ -451,7 +460,7 @@ void IpaBase::prepareIsp(const PrepareParams &params)
>  	 */
>  	AgcStatus agcStatus;
>  	bool hdrChange = false;
> -	RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];
> +	RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext % rpiMetadata_.size()];
>  	if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus)) {
>  		rpiMetadata.set("agc.delayed_status", agcStatus);
>  		hdrChange = agcStatus.hdr.mode != hdrStatus_.mode;
> @@ -464,9 +473,13 @@ void IpaBase::prepareIsp(const PrepareParams &params)
>  	 */
>  	helper_->prepare(embeddedBuffer, rpiMetadata);
>
> +	bool delayedRequestControls = false;
> +	delayedMetadata.get<bool>("ipa.request_controls", delayedRequestControls);
> +
>  	/* Allow a 10% margin on the comparison below. */
>  	Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> -	if (lastRunTimestamp_ && frameCount_ > invalidCount_ &&
> +	if (!delayedRequestControls && params.requestControls.empty() &&
> +	    lastRunTimestamp_ && frameCount_ > invalidCount_ &&
>  	    delta < controllerMinFrameDuration_ * 0.9 && !hdrChange) {
>  		/*
>  		 * Ensure we merge the previous frame's metadata with the current
> @@ -535,7 +548,7 @@ void IpaBase::processStats(const ProcessParams &params)
>  		ControlList ctrls(sensorCtrls_);
>  		applyAGC(&agcStatus, ctrls);
>  		rpiMetadata.set("agc.status", agcStatus);
> -		setDelayedControls.emit(ctrls, ipaContext);
> +		setDelayedControls.emit(ctrls, params.ipaContext);
>  		setCameraTimeoutValue();
>  	}
>
> @@ -951,8 +964,6 @@ void IpaBase::applyControls(const ControlList &controls)
>
>  			/* The control provides units of microseconds. */
>  			agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);
> -
> -			libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
>  			break;
>  		}
>
> @@ -976,9 +987,6 @@ void IpaBase::applyControls(const ControlList &controls)
>  				break;
>
>  			agc->setFixedGain(0, ctrl.second.get<float>());
> -
> -			libcameraMetadata_.set(controls::AnalogueGain,
> -					       ctrl.second.get<float>());
>  			break;
>  		}
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index f6ef8a3b..0c82f549 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -1528,4 +1528,53 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
>  	}
>  }
>
> +static bool isControlDelayed(unsigned int id)
> +{
> +	return id == controls::ExposureTime ||
> +	       id == controls::AnalogueGain ||
> +	       id == controls::FrameDurationLimits ||
> +	       id == controls::AeEnable ||
> +	       id == controls::ExposureTimeMode ||
> +	       id == controls::AnalogueGainMode;
> +}
> +
> +void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestControls)
> +{
> +	/*
> +	 * The delayContext is the sequence number after it's gone through the various
> +	 * pipeline delays, so that's what gets reported as the "ControlListSequence"
> +	 * in the metadata, being the sequence number of the request whose ControlList
> +	 * has just been applied.
> +	 */
> +	Request *request = requestQueue_.front();
> +	request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext);
> +
> +	/*
> +	 * Controls that take effect immediately (typically ISP controls) have to be
> +	 * delayed so as to synchronise with those controls that do get delayed. So we
> +	 * must remove them from the current request, and push them onto a queue so
> +	 * that they can be used later.
> +	 *
> +	 * Note that we are passed a copy of the request's controls, so that we can
> +	 * avoid disturbing what was in the original request.
> +	 */
> +	ControlList controls = std::move(requestControls);

Does ControlList have a move() assignment operator ?
Is this going through a plain copy ?

> +	requestControls.clear();

Probably so, otherwise a proper move() would have emptied
requestControls.

So this makes two copies overall

        params.requestControls = request->controls();
        in the caller

        ControlList controls = std::move(requestControls);
        here

You could probably pass to this function

        request->controls() as a const by reference
        params.requestControls as mutable by pointer

and have here

        params.requestControls->clear();

> +	immediateControls_.push({ request->sequence(), {} });
> +	for (const auto &ctrl : controls) {

	for (const auto &ctrl : request->controls()) {

> +		if (isControlDelayed(ctrl.first))
> +			requestControls.set(ctrl.first, ctrl.second);
                        params.requestControls->set()

> +		else
> +			immediateControls_.back().controls.set(ctrl.first, ctrl.second);
> +	}
> +
> +	/* "Immediate" controls that have become due are now merged back into this request. */
> +	while (!immediateControls_.empty() &&
> +	       immediateControls_.front().controlListId <= delayContext) {
> +		requestControls.merge(immediateControls_.front().controls,

                params.requestControls->merge();

> +				      ControlList::MergePolicy::OverwriteExisting);
> +		immediateControls_.pop();
> +	}
> +}
> +

This would be the resulting patch

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 0c82f549675b..0318cf55a7b2 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -1538,7 +1538,9 @@ static bool isControlDelayed(unsigned int id)
               id == controls::AnalogueGainMode;
 }

-void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestControls)
+void CameraData::handleControlLists(uint32_t delayContext,
+                                   const ControlList &requestControls,
+                                   ControlList *paramsControls)
 {
        /*
         * The delayContext is the sequence number after it's gone through the various
@@ -1558,12 +1560,11 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestC
         * Note that we are passed a copy of the request's controls, so that we can
         * avoid disturbing what was in the original request.
         */
-       ControlList controls = std::move(requestControls);
-       requestControls.clear();
        immediateControls_.push({ request->sequence(), {} });
-       for (const auto &ctrl : controls) {
+       paramsControls->clear();
+       for (const auto &ctrl : requestControls) {
                if (isControlDelayed(ctrl.first))
-                       requestControls.set(ctrl.first, ctrl.second);
+                       paramsControls->set(ctrl.first, ctrl.second);
                else
                        immediateControls_.back().controls.set(ctrl.first, ctrl.second);
        }
@@ -1571,7 +1572,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestC
        /* "Immediate" controls that have become due are now merged back into this request. */
        while (!immediateControls_.empty() &&
               immediateControls_.front().controlListId <= delayContext) {
-               requestControls.merge(immediateControls_.front().controls,
+               paramsControls->merge(immediateControls_.front().controls,
                                      ControlList::MergePolicy::OverwriteExisting);
                immediateControls_.pop();
        }
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
index 134262b66a04..856aa4f632b4 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
@@ -191,7 +191,8 @@ protected:
                                 Request *request);

        void handleControlLists(uint32_t delayContext,
-                               ControlList &requestControls);
+                               const ControlList &requestControls,
+                               ControlList *paramsControls);

        virtual void tryRunPipeline() = 0;

diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
index 690d25118aba..4d2db33cd144 100644
--- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
+++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
@@ -2338,10 +2338,9 @@ void PiSPCameraData::tryRunPipeline()
        params.ipaContext = requestQueue_.front()->sequence();
        params.delayContext = job.delayContext;
        params.sensorControls = std::move(job.sensorControls);
-       params.requestControls = request->controls();

        /* This sorts out synchronisation with ControlLists in earlier requests. */
-       handleControlLists(job.delayContext, params.requestControls);
+       handleControlLists(job.delayContext, request->controls(), &params.requestControls);

        /* Set our state to say the pipeline is active. */
        state_ = State::Busy;
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
index 28e7ad4698bf..f010c877a927 100644
--- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
+++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
@@ -947,13 +947,12 @@ void Vc4CameraData::tryRunPipeline()
        ipa::RPi::PrepareParams params;
        params.buffers.bayer = RPi::MaskBayerData | bayer;
        params.sensorControls = std::move(bayerFrame.controls);
-       params.requestControls = request->controls();
        params.ipaContext = request->sequence();
        params.delayContext = bayerFrame.delayContext;
        params.buffers.embedded = 0;

        /* This sorts out synchronisation with ControlLists in earlier requests. */
-       handleControlLists(bayerFrame.delayContext, params.requestControls);
+       handleControlLists(bayerFrame.delayContext, request->controls(), &params.requestControls);

        /* Set our state to say the pipeline is active. */
        state_ = State::Busy;

I admit I have onlt compiled it though but this should avoid the two
copies ?


>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 7bfac33e..134262b6 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -180,10 +180,19 @@ public:
>
>  	ClockRecovery wallClockRecovery_;
>
> +	struct ImmediateControlsEntry {
> +		uint64_t controlListId;
> +		ControlList controls;
> +	};
> +	std::queue<ImmediateControlsEntry> immediateControls_;
> +
>  protected:
>  	void fillRequestMetadata(const ControlList &bufferControls,
>  				 Request *request);
>
> +	void handleControlLists(uint32_t delayContext,
> +				ControlList &requestControls);
> +
>  	virtual void tryRunPipeline() = 0;
>
>  private:
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index cc7e32c3..690d2511 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -2322,9 +2322,6 @@ void PiSPCameraData::tryRunPipeline()
>
>  	fillRequestMetadata(job.sensorControls, request);
>
> -	/* Set our state to say the pipeline is active. */
> -	state_ = State::Busy;
> -
>  	unsigned int bayerId = cfe_[Cfe::Output0].getBufferId(job.buffers[&cfe_[Cfe::Output0]]);
>  	unsigned int statsId = cfe_[Cfe::Stats].getBufferId(job.buffers[&cfe_[Cfe::Stats]]);
>  	ASSERT(bayerId && statsId);
> @@ -2343,6 +2340,12 @@ void PiSPCameraData::tryRunPipeline()
>  	params.sensorControls = std::move(job.sensorControls);
>  	params.requestControls = request->controls();
>
> +	/* This sorts out synchronisation with ControlLists in earlier requests. */
> +	handleControlLists(job.delayContext, params.requestControls);
> +
> +	/* Set our state to say the pipeline is active. */
> +	state_ = State::Busy;
> +
>  	if (sensorMetadata_) {
>  		unsigned int embeddedId =
>  			cfe_[Cfe::Embedded].getBufferId(job.buffers[&cfe_[Cfe::Embedded]]);
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index f99cfdbc..28e7ad46 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -939,9 +939,6 @@ void Vc4CameraData::tryRunPipeline()
>
>  	fillRequestMetadata(bayerFrame.controls, request);
>
> -	/* Set our state to say the pipeline is active. */
> -	state_ = State::Busy;
> -
>  	unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
>
>  	LOG(RPI, Debug) << "Signalling prepareIsp:"
> @@ -955,6 +952,12 @@ void Vc4CameraData::tryRunPipeline()
>  	params.delayContext = bayerFrame.delayContext;
>  	params.buffers.embedded = 0;
>
> +	/* This sorts out synchronisation with ControlLists in earlier requests. */
> +	handleControlLists(bayerFrame.delayContext, params.requestControls);
> +
> +	/* Set our state to say the pipeline is active. */
> +	state_ = State::Busy;
> +

With or without the proposed patch, as this is ultimately your
pipeline code:
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  	if (embeddedBuffer) {
>  		unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>
> --
> 2.47.3
>

Hi Jacopo

Thanks for the review.

On Fri, 8 May 2026 at 09:15, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi David
>
> On Thu, May 07, 2026 at 01:20:36PM +0100, David Plowman wrote:
> > When a request is about to be processed, this commit separates
> > "delayed controls" (camera-related ones that take "a few frames" to
> > apply) from "immediate controls" (ISP-related controls) that will
> > happen instantly. It does so in a copy of the request's control list,
> > so as to avoid disturbing the request's original version.
> >
> > The immediate controls are held back until the delayed controls that
> > they were submitted with, have happened. This means all the controls
> > submitted in a request happen atomically.
> >
> > We therefore already have the sequence number of the request whose
> > controls have just been applied, so we additionally attach this to the
> > current request as "ControlId" metadata.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp               | 24 ++++++---
> >  .../pipeline/rpi/common/pipeline_base.cpp     | 49 +++++++++++++++++++
> >  .../pipeline/rpi/common/pipeline_base.h       |  9 ++++
> >  src/libcamera/pipeline/rpi/pisp/pisp.cpp      |  9 ++--
> >  src/libcamera/pipeline/rpi/vc4/vc4.cpp        |  9 ++--
> >  5 files changed, 86 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index faa77719..dacafa57 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -431,6 +431,15 @@ void IpaBase::prepareIsp(const PrepareParams &params)
> >       rpiMetadata.clear();
> >       fillDeviceStatus(params.sensorControls, ipaContext);
> >
> > +     /*
> > +      * When there are controls, it's important that we don't skip running the
> > +      * IPAs, as that can mess with synchronisation. Crucially though, we need
> > +      * to know whether there were controls when this comes back as the
> > +      * _delayed_ metadata, hence why we flag this in the metadata itself.
> > +      */
> > +     if (!params.requestControls.empty())
> > +             rpiMetadata.set("ipa.request_controls", true);
> > +
> >       if (params.buffers.embedded) {
> >               /*
> >                * Pipeline handler has supplied us with an embedded data buffer,
> > @@ -451,7 +460,7 @@ void IpaBase::prepareIsp(const PrepareParams &params)
> >        */
> >       AgcStatus agcStatus;
> >       bool hdrChange = false;
> > -     RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];
> > +     RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext % rpiMetadata_.size()];
> >       if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus)) {
> >               rpiMetadata.set("agc.delayed_status", agcStatus);
> >               hdrChange = agcStatus.hdr.mode != hdrStatus_.mode;
> > @@ -464,9 +473,13 @@ void IpaBase::prepareIsp(const PrepareParams &params)
> >        */
> >       helper_->prepare(embeddedBuffer, rpiMetadata);
> >
> > +     bool delayedRequestControls = false;
> > +     delayedMetadata.get<bool>("ipa.request_controls", delayedRequestControls);
> > +
> >       /* Allow a 10% margin on the comparison below. */
> >       Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> > -     if (lastRunTimestamp_ && frameCount_ > invalidCount_ &&
> > +     if (!delayedRequestControls && params.requestControls.empty() &&
> > +         lastRunTimestamp_ && frameCount_ > invalidCount_ &&
> >           delta < controllerMinFrameDuration_ * 0.9 && !hdrChange) {
> >               /*
> >                * Ensure we merge the previous frame's metadata with the current
> > @@ -535,7 +548,7 @@ void IpaBase::processStats(const ProcessParams &params)
> >               ControlList ctrls(sensorCtrls_);
> >               applyAGC(&agcStatus, ctrls);
> >               rpiMetadata.set("agc.status", agcStatus);
> > -             setDelayedControls.emit(ctrls, ipaContext);
> > +             setDelayedControls.emit(ctrls, params.ipaContext);
> >               setCameraTimeoutValue();
> >       }
> >
> > @@ -951,8 +964,6 @@ void IpaBase::applyControls(const ControlList &controls)
> >
> >                       /* The control provides units of microseconds. */
> >                       agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);
> > -
> > -                     libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
> >                       break;
> >               }
> >
> > @@ -976,9 +987,6 @@ void IpaBase::applyControls(const ControlList &controls)
> >                               break;
> >
> >                       agc->setFixedGain(0, ctrl.second.get<float>());
> > -
> > -                     libcameraMetadata_.set(controls::AnalogueGain,
> > -                                            ctrl.second.get<float>());
> >                       break;
> >               }
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index f6ef8a3b..0c82f549 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -1528,4 +1528,53 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
> >       }
> >  }
> >
> > +static bool isControlDelayed(unsigned int id)
> > +{
> > +     return id == controls::ExposureTime ||
> > +            id == controls::AnalogueGain ||
> > +            id == controls::FrameDurationLimits ||
> > +            id == controls::AeEnable ||
> > +            id == controls::ExposureTimeMode ||
> > +            id == controls::AnalogueGainMode;
> > +}
> > +
> > +void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestControls)
> > +{
> > +     /*
> > +      * The delayContext is the sequence number after it's gone through the various
> > +      * pipeline delays, so that's what gets reported as the "ControlListSequence"
> > +      * in the metadata, being the sequence number of the request whose ControlList
> > +      * has just been applied.
> > +      */
> > +     Request *request = requestQueue_.front();
> > +     request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext);
> > +
> > +     /*
> > +      * Controls that take effect immediately (typically ISP controls) have to be
> > +      * delayed so as to synchronise with those controls that do get delayed. So we
> > +      * must remove them from the current request, and push them onto a queue so
> > +      * that they can be used later.
> > +      *
> > +      * Note that we are passed a copy of the request's controls, so that we can
> > +      * avoid disturbing what was in the original request.
> > +      */
> > +     ControlList controls = std::move(requestControls);
>
> Does ControlList have a move() assignment operator ?
> Is this going through a plain copy ?
>
> > +     requestControls.clear();
>
> Probably so, otherwise a proper move() would have emptied
> requestControls.
>
> So this makes two copies overall
>
>         params.requestControls = request->controls();
>         in the caller
>
>         ControlList controls = std::move(requestControls);
>         here
>
> You could probably pass to this function
>
>         request->controls() as a const by reference
>         params.requestControls as mutable by pointer
>
> and have here
>
>         params.requestControls->clear();
>
> > +     immediateControls_.push({ request->sequence(), {} });
> > +     for (const auto &ctrl : controls) {
>
>         for (const auto &ctrl : request->controls()) {
>
> > +             if (isControlDelayed(ctrl.first))
> > +                     requestControls.set(ctrl.first, ctrl.second);
>                         params.requestControls->set()
>
> > +             else
> > +                     immediateControls_.back().controls.set(ctrl.first, ctrl.second);
> > +     }
> > +
> > +     /* "Immediate" controls that have become due are now merged back into this request. */
> > +     while (!immediateControls_.empty() &&
> > +            immediateControls_.front().controlListId <= delayContext) {
> > +             requestControls.merge(immediateControls_.front().controls,
>
>                 params.requestControls->merge();
>
> > +                                   ControlList::MergePolicy::OverwriteExisting);
> > +             immediateControls_.pop();
> > +     }
> > +}
> > +
>
> This would be the resulting patch
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 0c82f549675b..0318cf55a7b2 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -1538,7 +1538,9 @@ static bool isControlDelayed(unsigned int id)
>                id == controls::AnalogueGainMode;
>  }
>
> -void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestControls)
> +void CameraData::handleControlLists(uint32_t delayContext,
> +                                   const ControlList &requestControls,
> +                                   ControlList *paramsControls)
>  {
>         /*
>          * The delayContext is the sequence number after it's gone through the various
> @@ -1558,12 +1560,11 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestC
>          * Note that we are passed a copy of the request's controls, so that we can
>          * avoid disturbing what was in the original request.
>          */
> -       ControlList controls = std::move(requestControls);
> -       requestControls.clear();
>         immediateControls_.push({ request->sequence(), {} });
> -       for (const auto &ctrl : controls) {
> +       paramsControls->clear();
> +       for (const auto &ctrl : requestControls) {
>                 if (isControlDelayed(ctrl.first))
> -                       requestControls.set(ctrl.first, ctrl.second);
> +                       paramsControls->set(ctrl.first, ctrl.second);
>                 else
>                         immediateControls_.back().controls.set(ctrl.first, ctrl.second);
>         }
> @@ -1571,7 +1572,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList &requestC
>         /* "Immediate" controls that have become due are now merged back into this request. */
>         while (!immediateControls_.empty() &&
>                immediateControls_.front().controlListId <= delayContext) {
> -               requestControls.merge(immediateControls_.front().controls,
> +               paramsControls->merge(immediateControls_.front().controls,
>                                       ControlList::MergePolicy::OverwriteExisting);
>                 immediateControls_.pop();
>         }
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 134262b66a04..856aa4f632b4 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -191,7 +191,8 @@ protected:
>                                  Request *request);
>
>         void handleControlLists(uint32_t delayContext,
> -                               ControlList &requestControls);
> +                               const ControlList &requestControls,
> +                               ControlList *paramsControls);
>
>         virtual void tryRunPipeline() = 0;
>
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index 690d25118aba..4d2db33cd144 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -2338,10 +2338,9 @@ void PiSPCameraData::tryRunPipeline()
>         params.ipaContext = requestQueue_.front()->sequence();
>         params.delayContext = job.delayContext;
>         params.sensorControls = std::move(job.sensorControls);
> -       params.requestControls = request->controls();
>
>         /* This sorts out synchronisation with ControlLists in earlier requests. */
> -       handleControlLists(job.delayContext, params.requestControls);
> +       handleControlLists(job.delayContext, request->controls(), &params.requestControls);
>
>         /* Set our state to say the pipeline is active. */
>         state_ = State::Busy;
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 28e7ad4698bf..f010c877a927 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -947,13 +947,12 @@ void Vc4CameraData::tryRunPipeline()
>         ipa::RPi::PrepareParams params;
>         params.buffers.bayer = RPi::MaskBayerData | bayer;
>         params.sensorControls = std::move(bayerFrame.controls);
> -       params.requestControls = request->controls();
>         params.ipaContext = request->sequence();
>         params.delayContext = bayerFrame.delayContext;
>         params.buffers.embedded = 0;
>
>         /* This sorts out synchronisation with ControlLists in earlier requests. */
> -       handleControlLists(bayerFrame.delayContext, params.requestControls);
> +       handleControlLists(bayerFrame.delayContext, request->controls(), &params.requestControls);
>
>         /* Set our state to say the pipeline is active. */
>         state_ = State::Busy;
>
> I admit I have onlt compiled it though but this should avoid the two
> copies ?

Yes you're right that there's a copy that can be avoided (control
lists can be moved, so don't think there's a second copy, though
something in my head is nagging me you can't assume too much about the
state of the left-behind object after a move). Anyway I'll amend the
patch along these lines (might not pass in the request->controls()
explicitly because it already has the request, but functionally the
same).

Thanks for pointing this out!

David

>
>
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > index 7bfac33e..134262b6 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > @@ -180,10 +180,19 @@ public:
> >
> >       ClockRecovery wallClockRecovery_;
> >
> > +     struct ImmediateControlsEntry {
> > +             uint64_t controlListId;
> > +             ControlList controls;
> > +     };
> > +     std::queue<ImmediateControlsEntry> immediateControls_;
> > +
> >  protected:
> >       void fillRequestMetadata(const ControlList &bufferControls,
> >                                Request *request);
> >
> > +     void handleControlLists(uint32_t delayContext,
> > +                             ControlList &requestControls);
> > +
> >       virtual void tryRunPipeline() = 0;
> >
> >  private:
> > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > index cc7e32c3..690d2511 100644
> > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > @@ -2322,9 +2322,6 @@ void PiSPCameraData::tryRunPipeline()
> >
> >       fillRequestMetadata(job.sensorControls, request);
> >
> > -     /* Set our state to say the pipeline is active. */
> > -     state_ = State::Busy;
> > -
> >       unsigned int bayerId = cfe_[Cfe::Output0].getBufferId(job.buffers[&cfe_[Cfe::Output0]]);
> >       unsigned int statsId = cfe_[Cfe::Stats].getBufferId(job.buffers[&cfe_[Cfe::Stats]]);
> >       ASSERT(bayerId && statsId);
> > @@ -2343,6 +2340,12 @@ void PiSPCameraData::tryRunPipeline()
> >       params.sensorControls = std::move(job.sensorControls);
> >       params.requestControls = request->controls();
> >
> > +     /* This sorts out synchronisation with ControlLists in earlier requests. */
> > +     handleControlLists(job.delayContext, params.requestControls);
> > +
> > +     /* Set our state to say the pipeline is active. */
> > +     state_ = State::Busy;
> > +
> >       if (sensorMetadata_) {
> >               unsigned int embeddedId =
> >                       cfe_[Cfe::Embedded].getBufferId(job.buffers[&cfe_[Cfe::Embedded]]);
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index f99cfdbc..28e7ad46 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -939,9 +939,6 @@ void Vc4CameraData::tryRunPipeline()
> >
> >       fillRequestMetadata(bayerFrame.controls, request);
> >
> > -     /* Set our state to say the pipeline is active. */
> > -     state_ = State::Busy;
> > -
> >       unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> >
> >       LOG(RPI, Debug) << "Signalling prepareIsp:"
> > @@ -955,6 +952,12 @@ void Vc4CameraData::tryRunPipeline()
> >       params.delayContext = bayerFrame.delayContext;
> >       params.buffers.embedded = 0;
> >
> > +     /* This sorts out synchronisation with ControlLists in earlier requests. */
> > +     handleControlLists(bayerFrame.delayContext, params.requestControls);
> > +
> > +     /* Set our state to say the pipeline is active. */
> > +     state_ = State::Busy;
> > +
>
> With or without the proposed patch, as this is ultimately your
> pipeline code:
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> >       if (embeddedBuffer) {
> >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> >
> > --
> > 2.47.3
> >