[{"id":38417,"web_url":"https://patchwork.libcamera.org/comment/38417/","msgid":"","date":"2026-03-26T09:41:35","subject":"Re: [PATCH v1 3/3] pipeline: rpi: Make control lists in requests\n\tproperly atomic","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for this patch.\n\nOn Tue, 24 Mar 2026 at 15:17, David Plowman\n wrote:\n>\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 \"ControlId\" metadata.\n>\n> Signed-off-by: David Plowman \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\nI wish we could just remove this IPA rate handling :(\n\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\nNot related to this patch, but ipaContext vs params.ipaContext is a\nbit confusing. Perhaps we should replace ipaContext with\nipaContextWrapped or something in a future commit.\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> 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..9e162870 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\ns/THe/The/\n\nWith the typo fixed:\n\nReviewed-by: Naushir Patuck \n\n> + * pipeline delays, so that's what gets reported as the \"ControlId\" in the\n> + * metadata, being the sequence number of the request whose ControlList has\n> + * just been applied.\n> + */\n> + Request *request = requestQueue_.front();\n> + request->_d()->metadata().set(controls::rpi::ControlId, 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> +\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 52444BE086\n\tfor ;\n\tThu, 26 Mar 2026 09:42:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D1636284B;\n\tThu, 26 Mar 2026 10:42:14 +0100 (CET)","from mail-ua1-x929.google.com (mail-ua1-x929.google.com\n\t[IPv6:2607:f8b0:4864:20::929])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2891B62655\n\tfor ;\n\tThu, 26 Mar 2026 10:42:13 +0100 (CET)","by mail-ua1-x929.google.com with SMTP id\n\ta1e0cc1a2514c-94ad2aa1b9bso14023241.1\n\tfor ;\n\tThu, 26 Mar 2026 02:42:13 -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=\"TgCnpsyH\"; dkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1774518132; cv=none;\n\td=google.com; s=arc-20240605;\n\tb=jfsd7aoElJL5u8q5Nh7MXPwM085MuDqtsTrK/BaM+mUOakV5LLjBGVbs0/qhyJUeoq\n\tt0IVN/2dpCzKqM+ugqLNbs5HI8bQKJiBYpTYUuVyA/l3zbWGt7pNVIuglWx/tGVwTEIc\n\tA0XpGlKKAyRPK8kvtrNLPlnfzf3c/s1tHEWb4sILpD7M1shhm9SAGA9895Iagameb3gM\n\tp3BDfkv2CbMvrLibBY0Fs5mJvr/75Km+NF10N9e4DIiKqQlXa32J5lMpIjk77o9jHvzx\n\tRwBBNro/XMKKLiYNHWDlLIVBT0xUBA2P98CHeD8gWeW6dwzhKvH8e9bqOwvUNzb/jn1J\n\thaIA==","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=ggUMR1wMVK1XVxMvNYSNCziLUxc/QCdY09gZZ5pGoKg=;\n\tfh=raf7gMhxwhTsDETG3li6wpOfdJf6YyDg/c7aYJ8coN8=;\n\tb=HKTGsvBiqK1uSw9QCiIPHdM6MVf4pWv4qOX4sM4C6+HbPlwlj45Go83PpieMcItFse\n\txYjWM/cY0ZHyh39sykOZHEem/hikgTQn7+Gje0j44P6GkptYDJ/mM7Wb66Jb81ODHc5L\n\tRpAj9aIfPs+wCglbz4flqYpqW2rP3P80877sP0YtCvc0ocZ0O5K6JU2UTH0/aaXvC8nL\n\t74qEEI4ZDGi1GloOZ9A6l1RB6i/aUHb11hy5Zy1GEfBrqxo7mrvvDJSVpwkvhKqYtWnu\n\tNCjUrLaUR/5htEWXOsquiOHqLIsX0ImruTPoswg3pcjbYD32PhVSxtQELFyn+eLF2mdQ\n\tZMkQ==; 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=1774518132; x=1775122932;\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=ggUMR1wMVK1XVxMvNYSNCziLUxc/QCdY09gZZ5pGoKg=;\n\tb=TgCnpsyHgfweOZGpl+QA8zqVA3Wnt5TvHbYWvrZYHbxxJfkpb/t2KGw6JZ4M27HEmA\n\tUVvsk96PBXfTrk0NOkOSwFoN78B+1PGmV7/bJM6WRHIte/SzSNxr7he03yRAURLQcr2r\n\tcsxISzd+ZEO6JRYIKDDxupP3G86DqLai+GtlQivqJzHD4+QrFL0rSNlsf0C/F2wMGxy8\n\tPrzmJyK2XYjsSQw5PoTn64Z+lHl6/ZL82touuEZTiMOwnvMWN21mYgKtwacfJIVE1geI\n\tETT2nxgKl6Cf6+IIV5Bcb0Z5gIBrxeAf+O5c5MZhsmTMAWzOdO0W++wOM5HB+mpsKBVy\n\twYMQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20251104; t=1774518132; x=1775122932;\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=ggUMR1wMVK1XVxMvNYSNCziLUxc/QCdY09gZZ5pGoKg=;\n\tb=LY57HjaBIKXiOQC/B9DcwpmX64nzHcpOi1bxVe6/R/9pWY3UDqFEX9HD2Y3OYj1HdP\n\t4ZhH6r7P/Ux8rzd18QawPoZKee9cI0QxhnL3CZMK6ifngQbNo4IumHWOy5flJxFXfjfG\n\tHow0pp3qxE4vyMHFbbJ4QUMBQjhFzpq+sVWeJgtOvp/99vuGsa5A2PuwuYcyaVZyV8e6\n\tlWXJoXiz3DaPYi87uiFUnUphKULJq1NuGYcJr9vMCtEolAvpFQx8OMhgM1cyZjaMytEc\n\tv4pjguNmNZkAv3ij2fAlLC26HbNXZokH9MV2DvQKo/KBVsWmErR9VuzNe5M5Zr3n5Fq2\n\tJxig==","X-Gm-Message-State":"AOJu0YxgzffQH4lfcmvZxvGHCfG00FU9UbMkbmPzWGDOuY6ecgWafb1k\n\tp31ySrP0o4m9i0btQF7Vt7w2k9ALMCgULwx1PYNH3DGHfOml9uANym2vjEFMtUNEpgI1sDeqsSI\n\tHxuAlBdIsoHL7ObwKPzeNoBgHYZb9TEr5al+eG7IxPw==","X-Gm-Gg":"ATEYQzy3jY7ZDf95QZ9Ql6LoZUPXSwckz3Bsi8zwRTq24GqzhIT+kff5t/+JPIlSJtK\n\tt3Rv/KY0IlUwOctzbCQlg/efjw4/0+sp4rw9IikQDPyD3bCeyjcoQwkMLaR8iLnGiXOw6i3I6qq\n\tDa9rHTbfpMWCQHzqmFz9TJOL18u+HA7YuDQLlcudodRVFLVeTgPhGcrvkaO9X8Yk2DAiA83savC\n\t0a2XzecktKwP5pmeiwjfPITr+kDqI5lkJmn4T3c/iT28Pbj/+PE651qIbvrPwkzrZfwuXhP1iUH\n\t1laKxqa/u4QoBh6yWTQkWY6DiNK6uAaz/l1laH9MzL1Or0HRQBcsOwzxGtTee9fovnsvIftr8V/\n\ttAHm86+Dior5MLQoq0m8RBA==","X-Received":"by 2002:a05:6102:418c:b0:5df:b2cd:12ad with SMTP id\n\tada2fe7eead31-6038721e98emr1743752137.4.1774518131815;\n\tThu, 26 Mar 2026 02:42:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20260324151714.3345-1-david.plowman@raspberrypi.com>\n\t<20260324151714.3345-4-david.plowman@raspberrypi.com>","In-Reply-To":"<20260324151714.3345-4-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 26 Mar 2026 09:41:35 +0000","X-Gm-Features":"AQROBzDZ4io60sW4QY3pKByYTdDs7E5Ve45X9dY0dHHGM9j_nuXD-VJK7HO_nQU","Message-ID":"","Subject":"Re: [PATCH v1 3/3] pipeline: rpi: Make control lists in requests\n\tproperly atomic","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","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\" "}}]