[{"id":38845,"web_url":"https://patchwork.libcamera.org/comment/38845/","msgid":"","date":"2026-05-11T07:29:27","subject":"Re: [PATCH v5 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, May 08, 2026 at 01:57:43PM +0100, 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. We merge back together the\n> immediate controls that must happen now, along with the delayed\n> controls that must be forwarded early, the result being that control\n> lists submitted with a request happen atomically. We avoid overwriting\n> the original control list submitted with the request.\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 \"ControlId\" metadata.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n\nThanks for addressing the latest comments\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 24 ++++++---\n> .../pipeline/rpi/common/pipeline_base.cpp | 49 +++++++++++++++++++\n> .../pipeline/rpi/common/pipeline_base.h | 9 ++++\n> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++--\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 11 +++--\n> 5 files changed, 88 insertions(+), 16 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> \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> \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 f6ef8a3b..ace38997 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,53 @@ 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, ControlList ¶mControls)\n> +{\n> +\t/*\n> +\t * The delayContext is the sequence number after it's gone through the various\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> +\t * Note that we are given a separate control list (paramControls) so that\n> +\t * we can pass back the controls that really need to happen now, without\n> +\t * disturbing the controls that were submitted with the request.\n> +\t */\n> +\tASSERT(paramControls.empty());\n> +\timmediateControls_.push({ request->sequence(), {} });\n> +\tfor (const auto &ctrl : request->controls()) {\n> +\t\tif (isControlDelayed(ctrl.first))\n> +\t\t\tparamControls.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\tparamControls.merge(immediateControls_.front().controls,\n> +\t\t\t\t ControlList::MergePolicy::OverwriteExisting);\n> +\t\timmediateControls_.pop();\n> +\t}\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 7bfac33e..758155ee 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,19 @@ 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> +\t\t\t\tControlList ¶mControls);\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 cc7e32c3..b744c901 100644\n> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp\n> @@ -2322,9 +2322,6 @@ void PiSPCameraData::tryRunPipeline()\n>\n> \tfillRequestMetadata(job.sensorControls, request);\n>\n> -\t/* Set our state to say the pipeline is active. */\n> -\tstate_ = State::Busy;\n> -\n> \tunsigned int bayerId = cfe_[Cfe::Output0].getBufferId(job.buffers[&cfe_[Cfe::Output0]]);\n> \tunsigned int statsId = cfe_[Cfe::Stats].getBufferId(job.buffers[&cfe_[Cfe::Stats]]);\n> \tASSERT(bayerId && statsId);\n> @@ -2341,7 +2338,13 @@ void PiSPCameraData::tryRunPipeline()\n> \tparams.ipaContext = requestQueue_.front()->sequence();\n> \tparams.delayContext = job.delayContext;\n> \tparams.sensorControls = std::move(job.sensorControls);\n> -\tparams.requestControls = request->controls();\n> +\t/* params.requestControls is set by handleControlLists. */\n> +\n> +\t/* This sorts out synchronisation with ControlLists in earlier requests. */\n> +\thandleControlLists(job.delayContext, params.requestControls);\n> +\n> +\t/* Set our state to say the pipeline is active. */\n> +\tstate_ = State::Busy;\n>\n> \tif (sensorMetadata_) {\n> \t\tunsigned int embeddedId =\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index f99cfdbc..3e9a4905 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -939,9 +939,6 @@ void Vc4CameraData::tryRunPipeline()\n>\n> \tfillRequestMetadata(bayerFrame.controls, request);\n>\n> -\t/* Set our state to say the pipeline is active. */\n> -\tstate_ = State::Busy;\n> -\n> \tunsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n>\n> \tLOG(RPI, Debug) << \"Signalling prepareIsp:\"\n> @@ -950,10 +947,16 @@ void Vc4CameraData::tryRunPipeline()\n> \tipa::RPi::PrepareParams params;\n> \tparams.buffers.bayer = RPi::MaskBayerData | bayer;\n> \tparams.sensorControls = std::move(bayerFrame.controls);\n> -\tparams.requestControls = request->controls();\n> \tparams.ipaContext = request->sequence();\n> \tparams.delayContext = bayerFrame.delayContext;\n> \tparams.buffers.embedded = 0;\n> +\t/* params.requestControls is set by handleControlLists. */\n> +\n> +\t/* This sorts out synchronisation with ControlLists in earlier requests. */\n> +\thandleControlLists(bayerFrame.delayContext, params.requestControls);\n> +\n> +\t/* Set our state to say the pipeline is active. */\n> +\tstate_ = State::Busy;\n>\n> \tif (embeddedBuffer) {\n> \t\tunsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\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 B411ABDCBD\n\tfor ;\n\tMon, 11 May 2026 07:29:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C66686301E;\n\tMon, 11 May 2026 09:29:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 882F462010\n\tfor ;\n\tMon, 11 May 2026 09:29:32 +0200 (CEST)","from ideasonboard.com (mob-109-113-28-211.net.vodafone.it\n\t[109.113.28.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2863473B;\n\tMon, 11 May 2026 09:29:24 +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=\"n74QD238\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778484565;\n\tbh=gStajGSr3xpkj4sUjSTbduL7VJ9uA5xEZCS93Ja6V0k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n74QD238BO5bRQHgrQyjM4/km3lfSDrMNRImZ4FTObpyww3ahnjrCgYoc14rt8hzJ\n\tW84JEfes6xWidnaRaOV1NZG6w8CKKvzbw64JKZEvKZXHhGDkouHce5l4cve160yy/Q\n\tmxjFIc2HASbwe2wykBUkhNI2BP1AiUhhmSN8H1Rk=","Date":"Mon, 11 May 2026 09:29:27 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org, \n\tNaushir Patuck ","Subject":"Re: [PATCH v5 3/3] pipeline: rpi: Make control lists in requests\n\tproperly atomic","Message-ID":"","References":"<20260508130804.8238-1-david.plowman@raspberrypi.com>\n\t<20260508130804.8238-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260508130804.8238-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\" "}}]