[{"id":38656,"web_url":"https://patchwork.libcamera.org/comment/38656/","msgid":"","date":"2026-04-27T07:53:23","subject":"Re: [PATCH v2 3/3] pipeline: rpi: Make control lists in requests\n\tproperly atomic","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David,\n\nOn Fri, Mar 27, 2026 at 02:42:36PM +0000, David Plowman wrote:\n> When a request is about to be processed, this commit separates\n> \"delayed controls\" (camera-related ones that take \"a few frames\" to\n> apply) from \"immediate controls\" (ISP-related controls) that will\n> happen instantly.\n>\n> The immediate controls are held back until the delayed controls that\n> they were submitted with, have happened. This means all the controls\n> submitted in a request happen atomically.\n>\n> We therefore already have the sequence number of the request whose\n> controls have just been applied, so we additionally attach this to the\n> current request as \"ControlListSequence\" metadata.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 24 ++++++----\n> .../pipeline/rpi/common/pipeline_base.cpp | 46 +++++++++++++++++++\n> .../pipeline/rpi/common/pipeline_base.h | 8 ++++\n> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 3 ++\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++\n> 5 files changed, 76 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index faa77719..dacafa57 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -431,6 +431,15 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms)\n> \trpiMetadata.clear();\n> \tfillDeviceStatus(params.sensorControls, ipaContext);\n>\n> +\t/*\n> +\t * When there are controls, it's important that we don't skip running the\n> +\t * IPAs, as that can mess with synchronisation. Crucially though, we need\n> +\t * to know whether there were controls when this comes back as the\n> +\t * _delayed_ metadata, hence why we flag this in the metadata itself.\n> +\t */\n> +\tif (!params.requestControls.empty())\n> +\t\trpiMetadata.set(\"ipa.request_controls\", true);\n> +\n> \tif (params.buffers.embedded) {\n> \t\t/*\n> \t\t * Pipeline handler has supplied us with an embedded data buffer,\n> @@ -451,7 +460,7 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms)\n> \t */\n> \tAgcStatus agcStatus;\n> \tbool hdrChange = false;\n> -\tRPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];\n> +\tRPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext % rpiMetadata_.size()];\n> \tif (!delayedMetadata.get(\"agc.status\", agcStatus)) {\n> \t\trpiMetadata.set(\"agc.delayed_status\", agcStatus);\n> \t\thdrChange = agcStatus.hdr.mode != hdrStatus_.mode;\n> @@ -464,9 +473,13 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms)\n> \t */\n> \thelper_->prepare(embeddedBuffer, rpiMetadata);\n>\n> +\tbool delayedRequestControls = false;\n> +\tdelayedMetadata.get(\"ipa.request_controls\", delayedRequestControls);\n> +\n> \t/* Allow a 10% margin on the comparison below. */\n> \tDuration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> -\tif (lastRunTimestamp_ && frameCount_ > invalidCount_ &&\n> +\tif (!delayedRequestControls && params.requestControls.empty() &&\n> +\t lastRunTimestamp_ && frameCount_ > invalidCount_ &&\n> \t delta < controllerMinFrameDuration_ * 0.9 && !hdrChange) {\n> \t\t/*\n> \t\t * Ensure we merge the previous frame's metadata with the current\n> @@ -535,7 +548,7 @@ void IpaBase::processStats(const ProcessParams ¶ms)\n> \t\tControlList ctrls(sensorCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> \t\trpiMetadata.set(\"agc.status\", agcStatus);\n> -\t\tsetDelayedControls.emit(ctrls, ipaContext);\n> +\t\tsetDelayedControls.emit(ctrls, params.ipaContext);\n\nSo this used to cycle DelayedControls over rpiMetadata_.size() while\nnow we're using an unbound counter (which the Request counter) ? How\ndid this used to work if the index used for DelayedControls::push()\nwas alywas cycling within rpiMetadata_.size() ?\n\n\n> \t\tsetCameraTimeoutValue();\n> \t}\n>\n> @@ -951,8 +964,6 @@ void IpaBase::applyControls(const ControlList &controls)\n>\n> \t\t\t/* The control provides units of microseconds. */\n> \t\t\tagc->setFixedExposureTime(0, ctrl.second.get() * 1.0us);\n> -\n> -\t\t\tlibcameraMetadata_.set(controls::ExposureTime, ctrl.second.get());\n> \t\t\tbreak;\n> \t\t}\n>\n> @@ -976,9 +987,6 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\t\tbreak;\n>\n> \t\t\tagc->setFixedGain(0, ctrl.second.get());\n> -\n> -\t\t\tlibcameraMetadata_.set(controls::AnalogueGain,\n> -\t\t\t\t\t ctrl.second.get());\n\nI might have missed where these two are now reported..\n\n> \t\t\tbreak;\n> \t\t}\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 867ecf1b..c6df0934 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -1528,4 +1528,50 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request\n> \t}\n> }\n>\n> +static bool isControlDelayed(unsigned int id)\n> +{\n> +\treturn id == controls::ExposureTime ||\n> +\t id == controls::AnalogueGain ||\n> +\t id == controls::FrameDurationLimits ||\n> +\t id == controls::AeEnable ||\n> +\t id == controls::ExposureTimeMode ||\n> +\t id == controls::AnalogueGainMode;\n> +}\n> +\n> +void CameraData::handleControlLists(uint32_t delayContext)\n> +{\n> +\t/*\n> +\t * THe delayContext is the sequence number after it's gone through the various\n\nThe\n\n> +\t * pipeline delays, so that's what gets reported as the \"ControlListSequence\"\n> +\t * in the metadata, being the sequence number of the request whose ControlList\n> +\t * has just been applied.\n> +\t */\n> +\tRequest *request = requestQueue_.front();\n> +\trequest->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext);\n> +\n> +\t/*\n> +\t * Controls that take effect immediately (typically ISP controls) have to be\n> +\t * delayed so as to synchronise with those controls that do get delayed. So we\n> +\t * must remove them from the current request, and push them onto a queue so\n> +\t * that they can be used later.\n> +\t */\n> +\tControlList controls = std::move(request->controls());\n> +\trequest->controls().clear();\n> +\timmediateControls_.push({ request->sequence(), {} });\n> +\tfor (const auto &ctrl : controls) {\n> +\t\tif (isControlDelayed(ctrl.first))\n> +\t\t\trequest->controls().set(ctrl.first, ctrl.second);\n> +\t\telse\n> +\t\t\timmediateControls_.back().controls.set(ctrl.first, ctrl.second);\n> +\t}\n> +\n> +\t/* \"Immediate\" controls that have become due are now merged back into this request. */\n> +\twhile (!immediateControls_.empty() &&\n> +\t immediateControls_.front().controlListId <= delayContext) {\n> +\t\trequest->controls().merge(immediateControls_.front().controls,\n> +\t\t\t\t\t ControlList::MergePolicy::OverwriteExisting);\n> +\t\timmediateControls_.pop();\n> +\t}\n\nSo this now returns a Request with a different list of controls than\nthe one the application had submitted.\n\nParticularly, the flow in my understanding is as follow (I'm using\npisp and cfe here)\n\n- A CfeBuffer has completed: a new frame is available\n - We use the Cfe buffer sequence to know from delayed controls which\n sensor controls have been applied for that frame and the id of the\n Request those controls where submitted with. The id of that\n Request becomes 'delayedContext' here\n\n- This function populates a queue of ISP controls indexed by the\n Request id they belong to\n\n- We here collect all ISP controls whose Request id is smaller than\n 'delayedContext' (aka the Request id whose controls have completed)\n\nNow, this ControlList goes into the IPA then at some point will get\nback to the pipeline which populates a parameters buffer with the\nresult of the IPA processing.\n\nThe parameters buffer gets queued, and the ISP consumes buffer one\nafter the other. When the parameters buffer completes, it will be\nre-associated with the Request's ControlList created here and\nsignalled as complete.\n\nDid I get it right ?\n\nI will save never ending discussions about the overall per-frame\ncontrol issues and design, this change if I got it right makes sure a\nRequest is returned with a list of Controls applied at the time the\nRequest was processed, and that list might be different than the one\nit was created with, right ?\n\n\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> index 597eb587..65d8efdc 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> @@ -180,10 +180,18 @@ public:\n>\n> \tClockRecovery wallClockRecovery_;\n>\n> +\tstruct ImmediateControlsEntry {\n> +\t\tuint64_t controlListId;\n> +\t\tControlList controls;\n> +\t};\n> +\tstd::queue immediateControls_;\n> +\n> protected:\n> \tvoid fillRequestMetadata(const ControlList &bufferControls,\n> \t\t\t\t Request *request);\n>\n> +\tvoid handleControlLists(uint32_t delayContext);\n> +\n> \tvirtual void tryRunPipeline() = 0;\n>\n> private:\n> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> index dff73a79..cc8aa4d4 100644\n> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> @@ -2322,6 +2322,9 @@ void PiSPCameraData::tryRunPipeline()\n>\n> \tfillRequestMetadata(job.sensorControls, request);\n>\n> +\t/* This sorts out synchronisation with ControlLists in earlier requests. */\n> +\thandleControlLists(job.delayContext);\n> +\n> \t/* Set our state to say the pipeline is active. */\n> \tstate_ = State::Busy;\n>\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index b734889d..f743c8a7 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -939,6 +939,9 @@ void Vc4CameraData::tryRunPipeline()\n>\n> \tfillRequestMetadata(bayerFrame.controls, request);\n>\n> +\t/* This sorts out synchronisation with ControlLists in earlier requests. */\n> +\thandleControlLists(bayerFrame.delayContext);\n> +\n> \t/* Set our state to say the pipeline is active. */\n> \tstate_ = State::Busy;\n>\n> --\n> 2.47.3\n>","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 8A3B4BE173\n\tfor ;\n\tMon, 27 Apr 2026 07:53:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DB7B62FB6;\n\tMon, 27 Apr 2026 09:53:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 31DB362010\n\tfor ;\n\tMon, 27 Apr 2026 09:53:27 +0200 (CEST)","from ideasonboard.com (mob-109-113-69-223.net.vodafone.it\n\t[109.113.69.223])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 072408D4;\n\tMon, 27 Apr 2026 09:51:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Jluoa+dt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1777276305;\n\tbh=XKuGDJUzXKq6WFvxNk3YORlyzOJWnT7Z0kgEgKy8KMU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Jluoa+dtMUaX0S8pyQElOXAnf1QglNw5NDOQM4Hh+7KE4T+xuM/eMttRRTyo8eH7U\n\t9/bjuCiI2BjfDKdaQOq942EPvSdFwc/AqOJh1vX/LsytI8g1c+UHZB5D7d3ebqzi2W\n\tNFLyjiOsJFB5nT6RFg4HNgz7Y0NtNrtoqFqCVMqE=","Date":"Mon, 27 Apr 2026 09:53:23 +0200","From":"Jacopo Mondi ","To":"David Plowman , \n\tStefan Klug ","Cc":"libcamera-devel@lists.libcamera.org, \n\tNaushir Patuck ","Subject":"Re: [PATCH v2 3/3] pipeline: rpi: Make control lists in requests\n\tproperly atomic","Message-ID":"","References":"<20260327144726.7983-1-david.plowman@raspberrypi.com>\n\t<20260327144726.7983-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260327144726.7983-4-david.plowman@raspberrypi.com>","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\" "}},{"id":38663,"web_url":"https://patchwork.libcamera.org/comment/38663/","msgid":"","date":"2026-04-27T14:23:52","subject":"Re: [PATCH v2 3/3] pipeline: rpi: Make control lists in requests\n\tproperly atomic","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for taking a look at this!\n\nOn Mon, 27 Apr 2026 at 08:53, Jacopo Mondi\n wrote:\n>\n> Hi David,\n>\n> On Fri, Mar 27, 2026 at 02:42:36PM +0000, David Plowman wrote:\n> > When a request is about to be processed, this commit separates\n> > \"delayed controls\" (camera-related ones that take \"a few frames\" to\n> > apply) from \"immediate controls\" (ISP-related controls) that will\n> > happen instantly.\n> >\n> > The immediate controls are held back until the delayed controls that\n> > they were submitted with, have happened. This means all the controls\n> > submitted in a request happen atomically.\n> >\n> > We therefore already have the sequence number of the request whose\n> > controls have just been applied, so we additionally attach this to the\n> > current request as \"ControlListSequence\" metadata.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 24 ++++++----\n> > .../pipeline/rpi/common/pipeline_base.cpp | 46 +++++++++++++++++++\n> > .../pipeline/rpi/common/pipeline_base.h | 8 ++++\n> > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 3 ++\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++\n> > 5 files changed, 76 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index faa77719..dacafa57 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -431,6 +431,15 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms)\n> > rpiMetadata.clear();\n> > fillDeviceStatus(params.sensorControls, ipaContext);\n> >\n> > + /*\n> > + * When there are controls, it's important that we don't skip running the\n> > + * IPAs, as that can mess with synchronisation. Crucially though, we need\n> > + * to know whether there were controls when this comes back as the\n> > + * _delayed_ metadata, hence why we flag this in the metadata itself.\n> > + */\n> > + if (!params.requestControls.empty())\n> > + rpiMetadata.set(\"ipa.request_controls\", true);\n> > +\n> > if (params.buffers.embedded) {\n> > /*\n> > * Pipeline handler has supplied us with an embedded data buffer,\n> > @@ -451,7 +460,7 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms)\n> > */\n> > AgcStatus agcStatus;\n> > bool hdrChange = false;\n> > - RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];\n> > + RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext % rpiMetadata_.size()];\n> > if (!delayedMetadata.get(\"agc.status\", agcStatus)) {\n> > rpiMetadata.set(\"agc.delayed_status\", agcStatus);\n> > hdrChange = agcStatus.hdr.mode != hdrStatus_.mode;\n> > @@ -464,9 +473,13 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms)\n> > */\n> > helper_->prepare(embeddedBuffer, rpiMetadata);\n> >\n> > + bool delayedRequestControls = false;\n> > + delayedMetadata.get(\"ipa.request_controls\", delayedRequestControls);\n> > +\n> > /* Allow a 10% margin on the comparison below. */\n> > Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;\n> > - if (lastRunTimestamp_ && frameCount_ > invalidCount_ &&\n> > + if (!delayedRequestControls && params.requestControls.empty() &&\n> > + lastRunTimestamp_ && frameCount_ > invalidCount_ &&\n> > delta < controllerMinFrameDuration_ * 0.9 && !hdrChange) {\n> > /*\n> > * Ensure we merge the previous frame's metadata with the current\n> > @@ -535,7 +548,7 @@ void IpaBase::processStats(const ProcessParams ¶ms)\n> > ControlList ctrls(sensorCtrls_);\n> > applyAGC(&agcStatus, ctrls);\n> > rpiMetadata.set(\"agc.status\", agcStatus);\n> > - setDelayedControls.emit(ctrls, ipaContext);\n> > + setDelayedControls.emit(ctrls, params.ipaContext);\n>\n> So this used to cycle DelayedControls over rpiMetadata_.size() while\n> now we're using an unbound counter (which the Request counter) ? How\n> did this used to work if the index used for DelayedControls::push()\n> was alywas cycling within rpiMetadata_.size() ?\n\nparams.ipaContext is actually given to DelayedControls::push as a\ncookie. It then comes back to us about 20 lines higher as\nparams.delayContext. Previously, we were doing the % here, and not\nabove. But now, we're doing % above, and not here, so it is actually\nthe same. But the difference is that we carry the actual request\nsequence number around instead, as that's now more useful to us.\n\n>\n>\n> > setCameraTimeoutValue();\n> > }\n> >\n> > @@ -951,8 +964,6 @@ void IpaBase::applyControls(const ControlList &controls)\n> >\n> > /* The control provides units of microseconds. */\n> > agc->setFixedExposureTime(0, ctrl.second.get() * 1.0us);\n> > -\n> > - libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get());\n> > break;\n> > }\n> >\n> > @@ -976,9 +987,6 @@ void IpaBase::applyControls(const ControlList &controls)\n> > break;\n> >\n> > agc->setFixedGain(0, ctrl.second.get());\n> > -\n> > - libcameraMetadata_.set(controls::AnalogueGain,\n> > - ctrl.second.get());\n>\n> I might have missed where these two are now reported..\n\nThis is already happening in IpaBase::reportMetadata a little further\ndown the file (line 1577). It's just a bug that it's here as well, as\nit will start writing these new values into the metadata when the IPA\nsees them, several frames before they actually happen.\n\n>\n> > break;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 867ecf1b..c6df0934 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -1528,4 +1528,50 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request\n> > }\n> > }\n> >\n> > +static bool isControlDelayed(unsigned int id)\n> > +{\n> > + return id == controls::ExposureTime ||\n> > + id == controls::AnalogueGain ||\n> > + id == controls::FrameDurationLimits ||\n> > + id == controls::AeEnable ||\n> > + id == controls::ExposureTimeMode ||\n> > + id == controls::AnalogueGainMode;\n> > +}\n> > +\n> > +void CameraData::handleControlLists(uint32_t delayContext)\n> > +{\n> > + /*\n> > + * THe delayContext is the sequence number after it's gone through the various\n>\n> The\n\nThanks!\n\n>\n> > + * pipeline delays, so that's what gets reported as the \"ControlListSequence\"\n> > + * in the metadata, being the sequence number of the request whose ControlList\n> > + * has just been applied.\n> > + */\n> > + Request *request = requestQueue_.front();\n> > + request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext);\n> > +\n> > + /*\n> > + * Controls that take effect immediately (typically ISP controls) have to be\n> > + * delayed so as to synchronise with those controls that do get delayed. So we\n> > + * must remove them from the current request, and push them onto a queue so\n> > + * that they can be used later.\n> > + */\n> > + ControlList controls = std::move(request->controls());\n> > + request->controls().clear();\n> > + immediateControls_.push({ request->sequence(), {} });\n> > + for (const auto &ctrl : controls) {\n> > + if (isControlDelayed(ctrl.first))\n> > + request->controls().set(ctrl.first, ctrl.second);\n> > + else\n> > + immediateControls_.back().controls.set(ctrl.first, ctrl.second);\n> > + }\n> > +\n> > + /* \"Immediate\" controls that have become due are now merged back into this request. */\n> > + while (!immediateControls_.empty() &&\n> > + immediateControls_.front().controlListId <= delayContext) {\n> > + request->controls().merge(immediateControls_.front().controls,\n> > + ControlList::MergePolicy::OverwriteExisting);\n> > + immediateControls_.pop();\n> > + }\n>\n> So this now returns a Request with a different list of controls than\n> the one the application had submitted.\n>\n> Particularly, the flow in my understanding is as follow (I'm using\n> pisp and cfe here)\n>\n> - A CfeBuffer has completed: a new frame is available\n> - We use the Cfe buffer sequence to know from delayed controls which\n> sensor controls have been applied for that frame and the id of the\n> Request those controls where submitted with. The id of that\n> Request becomes 'delayedContext' here\n>\n> - This function populates a queue of ISP controls indexed by the\n> Request id they belong to\n>\n> - We here collect all ISP controls whose Request id is smaller than\n> 'delayedContext' (aka the Request id whose controls have completed)\n>\n> Now, this ControlList goes into the IPA then at some point will get\n> back to the pipeline which populates a parameters buffer with the\n> result of the IPA processing.\n>\n> The parameters buffer gets queued, and the ISP consumes buffer one\n> after the other. When the parameters buffer completes, it will be\n> re-associated with the Request's ControlList created here and\n> signalled as complete.\n>\n> Did I get it right ?\n\nYes, that sounds right, I think you probably deserve a medal! When we\nsend stuff off to the backend, it needs both the sensor controls that\nneed to be dispatched asap, plus the immediate (ISP) controls that are\nnow due because the sensor controls they were originally submitted\nwith, have happened.\n\n>\n> I will save never ending discussions about the overall per-frame\n> control issues and design, this change if I got it right makes sure a\n> Request is returned with a list of Controls applied at the time the\n> Request was processed, and that list might be different than the one\n> it was created with, right ?\n\nYes. To be honest, I've never actually looked at the controls in a\nrequest when it completes because it contains nothing of interest.\nPreviously there would have been some controls that may have been\napplied, and some that may not have been applied, and the situation\nhere is no different. The metadata is clearly where you would go to\nfind out.\n\nBut that is a discussion one could have. If you wanted to preserve the\noriginal control list, you could save it somewhere else or restore it\nupon completion, but I can't really see any point. Alternatively, I'd\nbe quite happy to clear it, at least that's easy, and there's no\ndanger you'd think it means something when it doesn't!\n\nActually I don't really regard this commit as having very much to do\nwith per-frame controls, it's about making control lists atomic, but\nthat's kind of a prerequisite whatever we do next.\n\nThanks!\nDavid\n\n>\n>\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > index 597eb587..65d8efdc 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > @@ -180,10 +180,18 @@ public:\n> >\n> > ClockRecovery wallClockRecovery_;\n> >\n> > + struct ImmediateControlsEntry {\n> > + uint64_t controlListId;\n> > + ControlList controls;\n> > + };\n> > + std::queue immediateControls_;\n> > +\n> > protected:\n> > void fillRequestMetadata(const ControlList &bufferControls,\n> > Request *request);\n> >\n> > + void handleControlLists(uint32_t delayContext);\n> > +\n> > virtual void tryRunPipeline() = 0;\n> >\n> > private:\n> > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> > index dff73a79..cc8aa4d4 100644\n> > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> > @@ -2322,6 +2322,9 @@ void PiSPCameraData::tryRunPipeline()\n> >\n> > fillRequestMetadata(job.sensorControls, request);\n> >\n> > + /* This sorts out synchronisation with ControlLists in earlier requests. */\n> > + handleControlLists(job.delayContext);\n> > +\n> > /* Set our state to say the pipeline is active. */\n> > state_ = State::Busy;\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > index b734889d..f743c8a7 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > @@ -939,6 +939,9 @@ void Vc4CameraData::tryRunPipeline()\n> >\n> > fillRequestMetadata(bayerFrame.controls, request);\n> >\n> > + /* This sorts out synchronisation with ControlLists in earlier requests. */\n> > + handleControlLists(bayerFrame.delayContext);\n> > +\n> > /* Set our state to say the pipeline is active. */\n> > state_ = State::Busy;\n> >\n> > --\n> > 2.47.3\n> >","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 54804BDCB5\n\tfor ;\n\tMon, 27 Apr 2026 14:24:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F72D62FC8;\n\tMon, 27 Apr 2026 16:24:07 +0200 (CEST)","from mail-ej1-x629.google.com (mail-ej1-x629.google.com\n\t[IPv6:2a00:1450:4864:20::629])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06F7E6271A\n\tfor ;\n\tMon, 27 Apr 2026 16:24:04 +0200 (CEST)","by mail-ej1-x629.google.com with SMTP id\n\ta640c23a62f3a-b79f8f7ea43so1403920766b.2\n\tfor ;\n\tMon, 27 Apr 2026 07:24:04 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"equUH6Cq\"; dkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1777299844; cv=none;\n\td=google.com; s=arc-20240605;\n\tb=cSbLCKNPzHrf3t+6VdUQA413wUSG1xf/epT1UTmPVvEca8eRExL5mj7F3QfbFEGxeI\n\t+1jVN5X/9bIDmiadXJpGCV6j0eOR2IbehYwY53MBUXzpXOxZeoT7rMzpTenauHAbLeKx\n\tAv126xWq6+if7fc0r85UlfbxJFJwKVvqEWE/SIf5+GBz/SgzQYlBLiIgHWbN8My7A8BM\n\tt33mkRlQtDJD7YADrqzYFfknyTUPOXOtcZzD6sjsxIXWK19da2BQP6+PtOoaV0s2jpU7\n\tksBYxXhWSkdRlwKsML3RgOGvknd1nXhABlsnqOu6hFzDlI127HUksif0AWumzt6fTmJP\n\tjtPQ==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=arc-20240605; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:dkim-signature;\n\tbh=SrPQUlawY66aVTUsCKoUAb1JyqlvbsHSdblwt/at0ps=;\n\tfh=bOicNwNv/392inqnj/tPdMM6/eZtfDkkuxsa5AE9dIo=;\n\tb=UFIgwrzlGDXCfEFsuh3RSL8wKEHA8T0Nf3wywmhbzwHOItku5kbiddXzCWaWYdZ8Y0\n\t4yhmVVo9rMQ9yWnZZPBvZ/MW+lKP23vG/6WN2piL+84m3g6OPEHDNJkRXG1u+gO5kEH1\n\tVF2UwnEbWZCn30BX7RHY16rLKceZtAdvDf0TunzC1f3cWUJ3fwHBVOkOS2WSrhaNjbe/\n\ti8YnzGxRsN9aIcCLvC82YjtbrmwsGkv/c2oMOfjeZo4RfNJfUpBXmnyqC1uuLct9WW4y\n\txL6dUNCvwz9u54m9qnXIJ7S3UY4hTkOf4EKOsBZW4wK8itSsbgpjj6JV2x0K3Mf8K5hz\n\tQw/A==; darn=lists.libcamera.org","ARC-Authentication-Results":"i=1; mx.google.com; arc=none","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1777299844; x=1777904644;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=SrPQUlawY66aVTUsCKoUAb1JyqlvbsHSdblwt/at0ps=;\n\tb=equUH6CqCgz/SPYLPp5FpoYspjV9uRSa9P8sY0K+5rKX9HTT/Z1d99t3PFp4y56qzX\n\tqQMwOiJRnA7A2MH3PXF2SElyB86c81c3yYGCUHuIPEMe8LJbDcOPAW8HmjR7+MX1mJTi\n\tvhsjtVze8a3tB3QXEa2sPrezD7ux3rSHSIXQFDwOtLzLStY1sUTeiitJ/yXa1TfCmzUk\n\tU5FqB40mVWDqH/V5u5uzCsboVFN+081DxrhjeqrrNaBvW5+h5c6OdyJAgZ5DEw4RgPiM\n\tLia2zeA/RLrFa1cNw8dXy0/OmIRVIhzDqSIBJFGATPNAzv+7h9+MlxuqXVc8f0EPKq0D\n\t0e3A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20251104; t=1777299844; x=1777904644;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=SrPQUlawY66aVTUsCKoUAb1JyqlvbsHSdblwt/at0ps=;\n\tb=A49PnkykimL7JSa/UmY51PQ+uxU3qMtspLjqY/DUaoDzUT1KxvFLL8un7LoJe6OJnO\n\tQqs5WZCsv/VNGRVVHz5Zd/oPlBOO6c921HQo5dNI7mK7cGSuWhSJ8xhkT5WUT1bQJdii\n\tgQ4UNZSQCEBs4PVGS1bYL64hmeu027vWMinyjCJFyoXulAlAhflujFPLkDJit5bCRQpd\n\tT+VL/Hr5f1u7l7P39upvXlYVUmxHxP2idzbGS415N0WK9TJAjmhDzbaE1Oho1cbxZzhq\n\tz4FxEGwSB50q3lF9mgdDdk/6J24dOH5QnKHQ82Lx/Nr9nSAWJp73kascMKa2iPOEEPyl\n\tM2fQ==","X-Forwarded-Encrypted":"i=1;\n\tAFNElJ/BOslg9Gdll0nMpSpQwy3xz07tvpsfS4rW3Kz932sKcgXE0JCwQ7JxsUF17Ad8r52Tiqjp7fHHkShIA3nj5aI=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yw6HvHTLPSexKSb6bIpfY1bz2GwhQp7Rg7cvv5AuVhgoNBMFF7u\n\tOpDTqukxbxRqGAOCnzkad4nd6SwEE6l/UZXFsmpU+dmpepTVaaOMHYb64CuznJd0g11fJ9Nd7zG\n\tPcZCHQwYEA+N8REf5gPKugJlcnYKza+RAM6yVGRKSwA==","X-Gm-Gg":"AeBDieuMHcqEALopzMFs8NKleB3HT9na8zEDmIvPGaIpNUUktPHUPGzfXseSFNq6Jqp\n\tXc/3EFYrg/KWCQo8T2FZME/7S4teUGfkzstEpDccRI/GugX0WPWHJK5fUR7Qyp5hrLrecA+RuUo\n\tClnmXc8YVL2lKua+AGgF+gKf/COagVuoU+z240Az/Zbc9e9oaWX8jFptL2ci/RUcGCHRHX8bWEx\n\tLjI+lYY/AysSlDwywCZLejxb0FG9h7SMoRqRv6DfA7fypbVssePAQkqlnfLgh6qR+yoLThfxBj3\n\tPHfJiv5S3RTEFXyvUNpHkbjm4l88viWa56f1dNDU4pIkv/gFRaTpP3m8Wk7b2ItKYmOJDH+YUzN\n\tDC+npr8p+WY+7gc1Sul2ZQNrqaUSVTCKaL+Cl9H1bxerU4w==","X-Received":"by 2002:a17:907:3f27:b0:ba4:e5e5:58b4 with SMTP id\n\ta640c23a62f3a-ba4e5e55b50mr2168517066b.20.1777299843915;\n\tMon, 27 Apr 2026 07:24:03 -0700 (PDT)","MIME-Version":"1.0","References":"<20260327144726.7983-1-david.plowman@raspberrypi.com>\n\t<20260327144726.7983-4-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 27 Apr 2026 15:23:52 +0100","X-Gm-Features":"AVHnY4Lyn1VEADJfgPke_TefIijqLLWMsGR8EUbQqhFEPOpGe737eu0o5WYObvU","Message-ID":"","Subject":"Re: [PATCH v2 3/3] pipeline: rpi: Make control lists in requests\n\tproperly atomic","To":"Jacopo Mondi ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org, \n\tNaushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","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\" "}}]