[{"id":36361,"web_url":"https://patchwork.libcamera.org/comment/36361/","msgid":"<176096394628.336133.5694591603975962693@localhost>","date":"2025-10-20T12:39:06","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nQuoting Jacopo Mondi (2025-10-17 11:00:06)\n> The limits (min and max values) for the Camera controls are\n> registered at Camera configuration time and never updated.\n> Some controls, like FrameDurationLimits have a direct impact on\n> the limits of other controls, such as the ExposureTime and as they\n> can be configured by the application, this has to be taken into account.\n> \n> Currently, when a user changes the frame duration, the limits for\n> both the ExposureTime and FrameDurationControls are not updated.\n> \n> The timing at which the controls limits should be updated is also\n> critical: the new control values take effect once the Request they\n> belong to is completed.\n> \n> To support this operation model introduce a new IPA function\n> 'updateControlsLimits()' which the pipeline handler calls before\n> completing a Request.\n> \n> Store the exposure time limits in the FrameContext (frame\n> duration limits were already there) and update the Camera::controls()\n> control info map with the limits as computed for the Request that has\n> just completed.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 ++\n> src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> src/ipa/rkisp1/ipa_context.h | 3 +++\n> src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> 5 files changed, 37 insertions(+)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> map streamConfig)\n> => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \n> + updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> +\n> mapBuffers(array buffers);\n> unmapBuffers(array ids);\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> * frameContext.agc.exposure;\n> maxExposureTime = minExposureTime;\n> }\n> + frameContext.agc.minExposureTime = minExposureTime;\n> + frameContext.agc.maxExposureTime = maxExposureTime;\n> \n> if (frameContext.agc.autoGainEnabled) {\n> minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> * applied to the sensor when the statistics were collected.\n> */\n> utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> + frameContext.agc.exposureTime = exposureTime;\n> double analogueGain = frameContext.sensor.gain;\n> utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -143,6 +143,9 @@ struct IPAActiveState {\n> \n> struct IPAFrameContext : public FrameContext {\n> struct {\n> + utils::Duration minExposureTime;\n> + utils::Duration maxExposureTime;\n> + utils::Duration exposureTime;\n> uint32_t exposure;\n> double gain;\n> double exposureValue;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -62,6 +62,8 @@ public:\n> int configure(const IPAConfigInfo &ipaConfig,\n> const std::map &streamConfig,\n> ControlInfoMap *ipaControls) override;\n> + int updateControlsLimits(const uint32_t frame,\n> + ControlInfoMap *ipaControls) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> \n> @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> return 0;\n> }\n> \n> +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> +{\n> + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> +\n> + /*\n> + * Update the exposure time and frame duration limits with the\n> + * settings computed by the AGC for the frame at hand.\n> + */\n> + assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> + assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> +\n> + ControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> + ControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> + ControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> + ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> +\n> + ControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> + ControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> + ControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> + ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n\nDo we really need to update the frame duration limits? Aren't these\nactually static by vblank min/max + output_size?\n\n> +\n> + return 0;\n> +}\n> +\n> void IPARkISP1::mapBuffers(const std::vector &buffers)\n> {\n> for (const IPABuffer &buffer : buffers) {\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> if (!isRaw_ && !info->paramDequeued)\n> return;\n> \n> + /* Update controls before completing the request */\n> + data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n\nMaybe I'm missing something here. How does this work in an isolated IPA?\nTo my understanding the ControlInfoMap would be deserialized on it's way\nback from the ipa and call the assignment operator on\ndata->controlInfo_, breaking any user application that holds a iterator\non controlInfo_.\n\nCould we assume that minFrameDuration doesn't change? Then we could\npotentially only add the new maxFrameDuration to the setSensorControls\nevent and spare a new call into the ipa?\n\nWhat do you think?\n\nBest regards,\nStefan\n\n> +\n> data->frameInfo_.destroy(info->frame);\n> \n> completeRequest(request);\n> \n> -- \n> 2.51.0\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 187FEBE080\n\tfor ;\n\tMon, 20 Oct 2025 12:39:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 255FE60750;\n\tMon, 20 Oct 2025 14:39:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B7F360722\n\tfor ;\n\tMon, 20 Oct 2025 14:39:09 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:b5d:2477:3cd5:195f])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id CE5C6E01;\n\tMon, 20 Oct 2025 14:37:26 +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=\"WbM2vAKH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760963846;\n\tbh=UzlVntWWOCNUACnWIYEhIaeJdYzPUIrAhkvyvS4bXKA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=WbM2vAKHkMgg7oltR2ewf8NgJKhzthntGm3iJBnO1AVlhqzscw6y5gSEXKmnKmFiG\n\tFCHASiY0X0hY1DwaLMOwzwjDbn11cUlXVwYKCCCE4HQ38dBHhm5S+fHZbndkNp+u56\n\tvc0ejldeLXX6eKOH3pgSNqoHGIkWGcbKb/xQ9D6Y=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","From":"Stefan Klug ","Cc":"Jacopo Mondi ","To":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 20 Oct 2025 14:39:06 +0200","Message-ID":"<176096394628.336133.5694591603975962693@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":36366,"web_url":"https://patchwork.libcamera.org/comment/36366/","msgid":"","date":"2025-10-21T07:05:01","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Oct 20, 2025 at 02:39:06PM +0200, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Quoting Jacopo Mondi (2025-10-17 11:00:06)\n> > The limits (min and max values) for the Camera controls are\n> > registered at Camera configuration time and never updated.\n> > Some controls, like FrameDurationLimits have a direct impact on\n> > the limits of other controls, such as the ExposureTime and as they\n> > can be configured by the application, this has to be taken into account.\n> >\n> > Currently, when a user changes the frame duration, the limits for\n> > both the ExposureTime and FrameDurationControls are not updated.\n> >\n> > The timing at which the controls limits should be updated is also\n> > critical: the new control values take effect once the Request they\n> > belong to is completed.\n> >\n> > To support this operation model introduce a new IPA function\n> > 'updateControlsLimits()' which the pipeline handler calls before\n> > completing a Request.\n> >\n> > Store the exposure time limits in the FrameContext (frame\n> > duration limits were already there) and update the Camera::controls()\n> > control info map with the limits as computed for the Request that has\n> > just completed.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 2 ++\n> > src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> > 5 files changed, 37 insertions(+)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> > map streamConfig)\n> > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> >\n> > + updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > +\n> > mapBuffers(array buffers);\n> > unmapBuffers(array ids);\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > * frameContext.agc.exposure;\n> > maxExposureTime = minExposureTime;\n> > }\n> > + frameContext.agc.minExposureTime = minExposureTime;\n> > + frameContext.agc.maxExposureTime = maxExposureTime;\n> >\n> > if (frameContext.agc.autoGainEnabled) {\n> > minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > * applied to the sensor when the statistics were collected.\n> > */\n> > utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> > + frameContext.agc.exposureTime = exposureTime;\n> > double analogueGain = frameContext.sensor.gain;\n> > utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> >\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -143,6 +143,9 @@ struct IPAActiveState {\n> >\n> > struct IPAFrameContext : public FrameContext {\n> > struct {\n> > + utils::Duration minExposureTime;\n> > + utils::Duration maxExposureTime;\n> > + utils::Duration exposureTime;\n> > uint32_t exposure;\n> > double gain;\n> > double exposureValue;\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -62,6 +62,8 @@ public:\n> > int configure(const IPAConfigInfo &ipaConfig,\n> > const std::map &streamConfig,\n> > ControlInfoMap *ipaControls) override;\n> > + int updateControlsLimits(const uint32_t frame,\n> > + ControlInfoMap *ipaControls) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> >\n> > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > return 0;\n> > }\n> >\n> > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> > +{\n> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > +\n> > + /*\n> > + * Update the exposure time and frame duration limits with the\n> > + * settings computed by the AGC for the frame at hand.\n> > + */\n> > + assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> > + assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> > +\n> > + ControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> > + ControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> > + ControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> > + ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> > +\n> > + ControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> > + ControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> > + ControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> > + ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n>\n> Do we really need to update the frame duration limits? Aren't these\n> actually static by vblank min/max + output_size?\n>\n\nShouldn't Camera::controls()[FrameDurationLimits] report what the user\nhas set as FrameDurationLimits ?\n\nAlthough, if your application sets a FrameDurationLimit to [x, x] to\nfix the frame rate, then your overall camera limits will be shown as\n[x, x] which is pretty useless.\n\nShould the Camera::controls[FrameDurationLimits] remain static for a\nthe whole capture session then ?\n\n> > +\n> > + return 0;\n> > +}\n> > +\n> > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > {\n> > for (const IPABuffer &buffer : buffers) {\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > if (!isRaw_ && !info->paramDequeued)\n> > return;\n> >\n> > + /* Update controls before completing the request */\n> > + data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n>\n> Maybe I'm missing something here. How does this work in an isolated IPA?\n> To my understanding the ControlInfoMap would be deserialized on it's way\n> back from the ipa and call the assignment operator on\n> data->controlInfo_, breaking any user application that holds a iterator\n> on controlInfo_.\n\nMy thinking was that updating an entry in a map won't invalidate\niterators, which could be invalidated if the map is resized.\n\nHowever, if I got you right, the ser/deser process will re-assign the\nmap. Is this your concern ?\n\nThere is however an underlying issue with concurrency. If the\napplication is accessing the ControlInfo exactly while the IPA udpates\nit. Note that we have the same issue when updating ScalerCrop in\nexample as many pipelines do.\n\nI don't have a solution for this at the moment. I'm afraid it will\nrequire some smart serialization and going throuhg a C ABI.\n\n>\n> Could we assume that minFrameDuration doesn't change? Then we could\n\nI think so, yes\n\n> potentially only add the new maxFrameDuration to the setSensorControls\n> event and spare a new call into the ipa?\n\nI think the most relevant control to update is ExposureTime and not\nFrameDurationLimits (see the above discussion).\n\nUpdating ExposureTime at setSensorControls() time doesn't however\nhappen at the \"right\" time. Actual controls are applied to the sensor\nwith a few frames of latency by DelayedControls. If we immediately\nupdate the Camera::controls() limits, we would do that before the\nactual Request that has triggered the update has completed.\n\nAnd honestly I would like to avoid ad-hoc things where a single\ncontrol is updated, but maybe ExposureTime is actually the only thing\nwe need to care about ?\n\n>\n> What do you think?\n>\n> Best regards,\n> Stefan\n>\n> > +\n> > data->frameInfo_.destroy(info->frame);\n> >\n> > completeRequest(request);\n> >\n> > --\n> > 2.51.0\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 F310FC3259\n\tfor ;\n\tTue, 21 Oct 2025 07:05:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 151486075F;\n\tTue, 21 Oct 2025 09:05:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67E766068F\n\tfor ;\n\tTue, 21 Oct 2025 09:05:05 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC21DEB4;\n\tTue, 21 Oct 2025 09:03:21 +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=\"OkHskWYL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761030201;\n\tbh=A6qa3RcJ4nnawAyODZ5pLBJvUmm7C/E3lerurSfnGcM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OkHskWYLzAIIqJLoYJN5b/vo9NoAmAWrLTrxM+WTCGCF4hpeMQHkGWROR50UIkNEp\n\tG8GCu1ufmBMDndv/iT5KG/6p8ICFiiT2PBatIYmBaM/12mvoUNJn/BQ+PAABIj6YUQ\n\tPi1xwG/vs9s60fvSy4SVn6nBWQhGBycySyU3Z33U=","Date":"Tue, 21 Oct 2025 09:05:01 +0200","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","Message-ID":"","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>\n\t<176096394628.336133.5694591603975962693@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176096394628.336133.5694591603975962693@localhost>","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":36368,"web_url":"https://patchwork.libcamera.org/comment/36368/","msgid":"<176103398116.336133.12647958159424364778@localhost>","date":"2025-10-21T08:06:21","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nQuoting Jacopo Mondi (2025-10-21 09:05:01)\n> Hi Stefan\n> \n> On Mon, Oct 20, 2025 at 02:39:06PM +0200, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > Quoting Jacopo Mondi (2025-10-17 11:00:06)\n> > > The limits (min and max values) for the Camera controls are\n> > > registered at Camera configuration time and never updated.\n> > > Some controls, like FrameDurationLimits have a direct impact on\n> > > the limits of other controls, such as the ExposureTime and as they\n> > > can be configured by the application, this has to be taken into account.\n> > >\n> > > Currently, when a user changes the frame duration, the limits for\n> > > both the ExposureTime and FrameDurationControls are not updated.\n> > >\n> > > The timing at which the controls limits should be updated is also\n> > > critical: the new control values take effect once the Request they\n> > > belong to is completed.\n> > >\n> > > To support this operation model introduce a new IPA function\n> > > 'updateControlsLimits()' which the pipeline handler calls before\n> > > completing a Request.\n> > >\n> > > Store the exposure time limits in the FrameContext (frame\n> > > duration limits were already there) and update the Camera::controls()\n> > > control info map with the limits as computed for the Request that has\n> > > just completed.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/ipa/rkisp1.mojom | 2 ++\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> > > 5 files changed, 37 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> > > map streamConfig)\n> > > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > >\n> > > + updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > +\n> > > mapBuffers(array buffers);\n> > > unmapBuffers(array ids);\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > * frameContext.agc.exposure;\n> > > maxExposureTime = minExposureTime;\n> > > }\n> > > + frameContext.agc.minExposureTime = minExposureTime;\n> > > + frameContext.agc.maxExposureTime = maxExposureTime;\n> > >\n> > > if (frameContext.agc.autoGainEnabled) {\n> > > minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > * applied to the sensor when the statistics were collected.\n> > > */\n> > > utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> > > + frameContext.agc.exposureTime = exposureTime;\n> > > double analogueGain = frameContext.sensor.gain;\n> > > utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> > >\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -143,6 +143,9 @@ struct IPAActiveState {\n> > >\n> > > struct IPAFrameContext : public FrameContext {\n> > > struct {\n> > > + utils::Duration minExposureTime;\n> > > + utils::Duration maxExposureTime;\n> > > + utils::Duration exposureTime;\n> > > uint32_t exposure;\n> > > double gain;\n> > > double exposureValue;\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -62,6 +62,8 @@ public:\n> > > int configure(const IPAConfigInfo &ipaConfig,\n> > > const std::map &streamConfig,\n> > > ControlInfoMap *ipaControls) override;\n> > > + int updateControlsLimits(const uint32_t frame,\n> > > + ControlInfoMap *ipaControls) override;\n> > > void mapBuffers(const std::vector &buffers) override;\n> > > void unmapBuffers(const std::vector &ids) override;\n> > >\n> > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > return 0;\n> > > }\n> > >\n> > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> > > +{\n> > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > +\n> > > + /*\n> > > + * Update the exposure time and frame duration limits with the\n> > > + * settings computed by the AGC for the frame at hand.\n> > > + */\n> > > + assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> > > + assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> > > +\n> > > + ControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> > > + ControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> > > + ControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> > > + ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> > > +\n> > > + ControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> > > + ControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> > > + ControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> > > + ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n> >\n> > Do we really need to update the frame duration limits? Aren't these\n> > actually static by vblank min/max + output_size?\n> >\n> \n> Shouldn't Camera::controls()[FrameDurationLimits] report what the user\n> has set as FrameDurationLimits ?\n\nMy understanding was that Camera::controls()[FrameDurationLimits]\nreports the limits of the control and not the current state of the\ncamera. So min/max shouldn't change as these are roughly a sensor\nproperty. (If we would change the max to the currently selected upper\nlimit and would check the limits, the user would never be able to raise\nthe upper limit again). Using the default value to represent the current\nstate would be possible, but is to my understanding not the current\ndesign idea. My understanding was that the default is the camera\ndefault/startup value, possibly modified once after configuration.\n\n> \n> Although, if your application sets a FrameDurationLimit to [x, x] to\n> fix the frame rate, then your overall camera limits will be shown as\n> [x, x] which is pretty useless.\n> \n> Should the Camera::controls[FrameDurationLimits] remain static for a\n> the whole capture session then ?\n\nYes, I believe so.\n\n> \n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > > {\n> > > for (const IPABuffer &buffer : buffers) {\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > > if (!isRaw_ && !info->paramDequeued)\n> > > return;\n> > >\n> > > + /* Update controls before completing the request */\n> > > + data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n> >\n> > Maybe I'm missing something here. How does this work in an isolated IPA?\n> > To my understanding the ControlInfoMap would be deserialized on it's way\n> > back from the ipa and call the assignment operator on\n> > data->controlInfo_, breaking any user application that holds a iterator\n> > on controlInfo_.\n> \n> My thinking was that updating an entry in a map won't invalidate\n> iterators, which could be invalidated if the map is resized.\n> \n> However, if I got you right, the ser/deser process will re-assign the\n> map. Is this your concern ?\n\nYes, exactly.\n\n> \n> There is however an underlying issue with concurrency. If the\n> application is accessing the ControlInfo exactly while the IPA udpates\n> it. Note that we have the same issue when updating ScalerCrop in\n> example as many pipelines do.\n> \n> I don't have a solution for this at the moment. I'm afraid it will\n> require some smart serialization and going throuhg a C ABI.\n\nYes, that is a pain. My strategy there was to keep controls static after\nconfigure(). But I agree that we need to update the max exposure time.\n\n> \n> >\n> > Could we assume that minFrameDuration doesn't change? Then we could\n> \n> I think so, yes\n> \n> > potentially only add the new maxFrameDuration to the setSensorControls\n> > event and spare a new call into the ipa?\n> \n> I think the most relevant control to update is ExposureTime and not\n> FrameDurationLimits (see the above discussion).\n\nOh yes, I picked the wrong variable. I meant max exposure time.\n\n> \n> Updating ExposureTime at setSensorControls() time doesn't however\n> happen at the \"right\" time. Actual controls are applied to the sensor\n> with a few frames of latency by DelayedControls. If we immediately\n> update the Camera::controls() limits, we would do that before the\n> actual Request that has triggered the update has completed.\n> \n> And honestly I would like to avoid ad-hoc things where a single\n> control is updated, but maybe ExposureTime is actually the only thing\n> we need to care about ?\n\nI eagerly need to post my regulation rework series, as that also impacts\ntiming and reorders a few things. I hope I can do that this week...\n\nBest regards,\nStefan\n\n> \n> >\n> > What do you think?\n> >\n> > Best regards,\n> > Stefan\n> >\n> > > +\n> > > data->frameInfo_.destroy(info->frame);\n> > >\n> > > completeRequest(request);\n> > >\n> > > --\n> > > 2.51.0\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 CAB41C3259\n\tfor ;\n\tTue, 21 Oct 2025 08:06:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA3DF6075C;\n\tTue, 21 Oct 2025 10:06:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED33C60757\n\tfor ;\n\tTue, 21 Oct 2025 10:06:24 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:b5d:2477:3cd5:195f])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 4AA5F1387; \n\tTue, 21 Oct 2025 10:04:41 +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=\"YKCa7dnY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761033881;\n\tbh=tQwpcnOq7ucnggyXijZ6Di1VKQa56sBkQQV5VnjeMkA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=YKCa7dnYe1Nrgp/Ttzj38fVba4PinTXg03xWKJg/Mp2Xp/UkejgBMLn1ZBKUS2mbl\n\t2wvQCo+hjMP4fbvsvBBt6RPmsK9un3m7Oy8VfACPGEYQTD5UbImk0/bDYtbFQmNger\n\tRVmyp5/BkwRJrBjEA7+9rC9ouw6TpzbtGrq8RUkE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>\n\t<176096394628.336133.5694591603975962693@localhost>\n\t","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","From":"Stefan Klug ","Cc":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ","Date":"Tue, 21 Oct 2025 10:06:21 +0200","Message-ID":"<176103398116.336133.12647958159424364778@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":36372,"web_url":"https://patchwork.libcamera.org/comment/36372/","msgid":"<20251021113205.GA1694476@ragnatech.se>","date":"2025-10-21T11:32:05","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:\n> The limits (min and max values) for the Camera controls are\n> registered at Camera configuration time and never updated.\n> Some controls, like FrameDurationLimits have a direct impact on\n> the limits of other controls, such as the ExposureTime and as they\n> can be configured by the application, this has to be taken into account.\n> \n> Currently, when a user changes the frame duration, the limits for\n> both the ExposureTime and FrameDurationControls are not updated.\n> \n> The timing at which the controls limits should be updated is also\n> critical: the new control values take effect once the Request they\n> belong to is completed.\n> \n> To support this operation model introduce a new IPA function\n> 'updateControlsLimits()' which the pipeline handler calls before\n> completing a Request.\n> \n> Store the exposure time limits in the FrameContext (frame\n> duration limits were already there) and update the Camera::controls()\n> control info map with the limits as computed for the Request that has\n> just completed.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 ++\n> src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> src/ipa/rkisp1/ipa_context.h | 3 +++\n> src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> 5 files changed, 37 insertions(+)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> \t\t map streamConfig)\n> \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \n> +\tupdateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> +\n> \tmapBuffers(array buffers);\n> \tunmapBuffers(array ids);\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t\t\t* frameContext.agc.exposure;\n> \t\tmaxExposureTime = minExposureTime;\n> \t}\n> +\tframeContext.agc.minExposureTime = minExposureTime;\n> +\tframeContext.agc.maxExposureTime = maxExposureTime;\n> \n> \tif (frameContext.agc.autoGainEnabled) {\n> \t\tminAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t * applied to the sensor when the statistics were collected.\n> \t */\n> \tutils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> +\tframeContext.agc.exposureTime = exposureTime;\n> \tdouble analogueGain = frameContext.sensor.gain;\n> \tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -143,6 +143,9 @@ struct IPAActiveState {\n> \n> struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> +\t\tutils::Duration minExposureTime;\n> +\t\tutils::Duration maxExposureTime;\n> +\t\tutils::Duration exposureTime;\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t\tdouble exposureValue;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -62,6 +62,8 @@ public:\n> \tint configure(const IPAConfigInfo &ipaConfig,\n> \t\t const std::map &streamConfig,\n> \t\t ControlInfoMap *ipaControls) override;\n> +\tint updateControlsLimits(const uint32_t frame,\n> +\t\t\t\t ControlInfoMap *ipaControls) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> \n> @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \treturn 0;\n> }\n> \n> +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> +{\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> +\n> +\t/*\n> +\t * Update the exposure time and frame duration limits with the\n> +\t * settings computed by the AGC for the frame at hand.\n> +\t */\n> +\tassert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> +\tassert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> +\n> +\tControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> +\tControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> +\tControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> +\tipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> +\n> +\tControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> +\tControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> +\tControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> +\tipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n\nIs this function not doing the reveres of what is expected? It takes the \ncontrols from the sensor and update them with new limits using values \ncalculated by the IPA. Should it not take the values updated by the \nsensor kernel driver and propagate them to the values used by the IPA?\n\nIn IPARkISP1::configure() we read the min and max exposure time from the \nV4L2 sensor control, here we override them with new values we calculated \nourself.\n\nIf we take the IMX219 as an example, when we update the V4L2_CID_VBLANK \ncontrol new max and default values for V4L2_CID_EXPOSURE are calculated \nand set, see imx219_set_ctrl(). Should not libcamera fetch and propagate \nthese values instead of calculating it's own set?\n\nIf not can't we make configure() simpler by removing the ABI sharing of \nthis dynamic control range? And in the longer run just drop \nV4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK? \nThat way we would have more static only controls to care about :-)\n\nI think it's dangerous to do both, init the IPA with the values from the \nkernel and then calculate our own later.\n\nSame comment for 1/3 where the new max exposure time is calculated.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> void IPARkISP1::mapBuffers(const std::vector &buffers)\n> {\n> \tfor (const IPABuffer &buffer : buffers) {\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> \tif (!isRaw_ && !info->paramDequeued)\n> \t\treturn;\n> \n> +\t/* Update controls before completing the request */\n> +\tdata->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n> +\n> \tdata->frameInfo_.destroy(info->frame);\n> \n> \tcompleteRequest(request);\n> \n> -- \n> 2.51.0\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 AD697C3259\n\tfor ;\n\tTue, 21 Oct 2025 11:32:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D394C6075B;\n\tTue, 21 Oct 2025 13:32:15 +0200 (CEST)","from fhigh-b5-smtp.messagingengine.com\n\t(fhigh-b5-smtp.messagingengine.com [202.12.124.156])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 523F96075B\n\tfor ;\n\tTue, 21 Oct 2025 13:32:13 +0200 (CEST)","from phl-compute-05.internal (phl-compute-05.internal\n\t[10.202.2.45])\n\tby mailfhigh.stl.internal (Postfix) with ESMTP id F40437A00AB;\n\tTue, 21 Oct 2025 07:32:11 -0400 (EDT)","from phl-mailfrontend-02 ([10.202.2.163])\n\tby phl-compute-05.internal (MEProxy); Tue, 21 Oct 2025 07:32:12 -0400","by mail.messagingengine.com (Postfix) with ESMTPA; Tue,\n\t21 Oct 2025 07:32:11 -0400 (EDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech.se header.i=@ragnatech.se\n\theader.b=\"gp4ouTkj\"; dkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"C3mTuiik\"; \n\tdkim-atps=neutral","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=ragnatech.se; h=\n\tcc:cc:content-transfer-encoding:content-type:content-type:date\n\t:date:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to; s=fm1; t=1761046331;\n\tx=1761132731; bh=0VjKI5MOgyZPV5QbO/OgFXod2APW9xM39ptuUrFLu/U=; b=\n\tgp4ouTkjIav6khg83sJA1hw7F2gSRYxLxtBJrbRIg5kz3jXqmuRlwIFsADpWaL8H\n\ttx6wWTXSMKJTCSn1qiloUvlfq7x1dGJ4ZloFiRL6+Sg26okPXnD52yoYJ2tynfcE\n\tIOugnk8XoorcLjGu2LG7FQgXtrLpBkuxka4m8RRnLjXJ2QdczNU81luJaLa1fZu6\n\tzFr5BfWnnPeEepcupN7PmvBe4R6msxvF6+3uz/0li2EHym7HOMgzFnCOlUQGSMWv\n\tR1guOb9HKT1KTpgSrAYJuAFtLYhCpt3nJJozrprwHttLsSolCo9qOITUAqBkbNl6\n\t7UCRDOoyvbpDVPKAn8HgPw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:cc:content-transfer-encoding\n\t:content-type:content-type:date:date:feedback-id:feedback-id\n\t:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to:x-me-proxy\n\t:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t=1761046331; x=\n\t1761132731; bh=0VjKI5MOgyZPV5QbO/OgFXod2APW9xM39ptuUrFLu/U=; b=C\n\t3mTuiikxDh57/LWPFGk8DXHQ8KEjQZpx0bLtD75bCoKUjWsO+baEGeuTOS4pVuk4\n\tMuhMjUNFNdkR7CRMs1ArAhYriB5Md89EMyPU/fabtkhqaSB0ju3/72DrUacRi+GM\n\tyXWgBya4XBQ/oWpUI2Re0ExMCXiBQMZEknbjDmacwhLSnBBV7th2lRKVj3qu7mYS\n\txqYVTioj15BqWZxpa+8Xc8d2LCO0xGck4OgHhXz9C++sigfPoe81NeVsLep1vaD3\n\tGWGa5QrO1GS9X3xNx6ZCPyVty3Yr7gL7IZDbADpYK1Al132cZKKj5tucP7rw6Ai8\n\te6AeFGfiDiFSjhnDhF+WQ=="],"X-ME-Sender":"\n\t","X-ME-Received":"","X-ME-Proxy-Cause":"gggruggvucftvghtrhhoucdtuddrgeeffedrtdeggddugedtheelucetufdoteggodetrf\n\tdotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu\n\trghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf\n\tgurhepfffhvfevuffkfhggtggugfgjsehtkeertddttdejnecuhfhrohhmpefpihhklhgr\n\tshcuufpnuggvrhhluhhnugcuoehnihhklhgrshdrshhouggvrhhluhhnugesrhgrghhnrg\n\thtvggthhdrshgvqeenucggtffrrghtthgvrhhnpeevteegtddvvdfhtdekgefhfeefheet\n\theekkeegfeejudeiudeuleegtdehkeekteenucevlhhushhtvghrufhiiigvpedtnecurf\n\tgrrhgrmhepmhgrihhlfhhrohhmpehnihhklhgrshdrshhouggvrhhluhhnugesrhgrghhn\n\trghtvggthhdrshgvpdhnsggprhgtphhtthhopedvpdhmohguvgepshhmthhpohhuthdprh\n\tgtphhtthhopehjrggtohhpohdrmhhonhguihesihguvggrshhonhgsohgrrhgurdgtohhm\n\tpdhrtghpthhtoheplhhisggtrghmvghrrgdquggvvhgvlheslhhishhtshdrlhhisggtrg\n\thmvghrrgdrohhrgh","X-ME-Proxy":"\n\t\n\t\n\t\n\t","Feedback-ID":"i80c9496c:Fastmail","Date":"Tue, 21 Oct 2025 13:32:05 +0200","From":"Niklas =?utf-8?q?S=C3=B6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","Message-ID":"<20251021113205.GA1694476@ragnatech.se>","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.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":36377,"web_url":"https://patchwork.libcamera.org/comment/36377/","msgid":"","date":"2025-10-21T14:01:03","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:\n> > The limits (min and max values) for the Camera controls are\n> > registered at Camera configuration time and never updated.\n> > Some controls, like FrameDurationLimits have a direct impact on\n> > the limits of other controls, such as the ExposureTime and as they\n> > can be configured by the application, this has to be taken into account.\n> >\n> > Currently, when a user changes the frame duration, the limits for\n> > both the ExposureTime and FrameDurationControls are not updated.\n> >\n> > The timing at which the controls limits should be updated is also\n> > critical: the new control values take effect once the Request they\n> > belong to is completed.\n> >\n> > To support this operation model introduce a new IPA function\n> > 'updateControlsLimits()' which the pipeline handler calls before\n> > completing a Request.\n> >\n> > Store the exposure time limits in the FrameContext (frame\n> > duration limits were already there) and update the Camera::controls()\n> > control info map with the limits as computed for the Request that has\n> > just completed.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 2 ++\n> > src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> > 5 files changed, 37 insertions(+)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> > \t\t map streamConfig)\n> > \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> >\n> > +\tupdateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > +\n> > \tmapBuffers(array buffers);\n> > \tunmapBuffers(array ids);\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \t\t\t\t* frameContext.agc.exposure;\n> > \t\tmaxExposureTime = minExposureTime;\n> > \t}\n> > +\tframeContext.agc.minExposureTime = minExposureTime;\n> > +\tframeContext.agc.maxExposureTime = maxExposureTime;\n> >\n> > \tif (frameContext.agc.autoGainEnabled) {\n> > \t\tminAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \t * applied to the sensor when the statistics were collected.\n> > \t */\n> > \tutils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> > +\tframeContext.agc.exposureTime = exposureTime;\n> > \tdouble analogueGain = frameContext.sensor.gain;\n> > \tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> >\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -143,6 +143,9 @@ struct IPAActiveState {\n> >\n> > struct IPAFrameContext : public FrameContext {\n> > \tstruct {\n> > +\t\tutils::Duration minExposureTime;\n> > +\t\tutils::Duration maxExposureTime;\n> > +\t\tutils::Duration exposureTime;\n> > \t\tuint32_t exposure;\n> > \t\tdouble gain;\n> > \t\tdouble exposureValue;\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -62,6 +62,8 @@ public:\n> > \tint configure(const IPAConfigInfo &ipaConfig,\n> > \t\t const std::map &streamConfig,\n> > \t\t ControlInfoMap *ipaControls) override;\n> > +\tint updateControlsLimits(const uint32_t frame,\n> > +\t\t\t\t ControlInfoMap *ipaControls) override;\n> > \tvoid mapBuffers(const std::vector &buffers) override;\n> > \tvoid unmapBuffers(const std::vector &ids) override;\n> >\n> > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > \treturn 0;\n> > }\n> >\n> > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> > +{\n> > +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > +\n> > +\t/*\n> > +\t * Update the exposure time and frame duration limits with the\n> > +\t * settings computed by the AGC for the frame at hand.\n> > +\t */\n> > +\tassert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> > +\tassert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> > +\n> > +\tControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> > +\tControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> > +\tControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> > +\tipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> > +\n> > +\tControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> > +\tControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> > +\tControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> > +\tipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n>\n> Is this function not doing the reveres of what is expected? It takes the\n> controls from the sensor and update them with new limits using values\n> calculated by the IPA. Should it not take the values updated by the\n\nWhere does it take controls from the sensor ?\n\n> sensor kernel driver and propagate them to the values used by the IPA?\n\nAs I see it, it takes the control values the IPA has calculated for\nthe sensor for frame/request X [*] and when request X completes it\npropagates those values to Camera::controls().\n\n[*] We have an outstanding duality between request number and frame\nnumber. Hopefully we should sooner or later move the whole PH/IPA\ninterface to be designed around frame numbers and not request ids.\n\n>\n> In IPARkISP1::configure() we read the min and max exposure time from the\n> V4L2 sensor control, here we override them with new values we calculated\n> ourself.\n>\n> If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK\n> control new max and default values for V4L2_CID_EXPOSURE are calculated\n> and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate\n> these values instead of calculating it's own set?\n>\n\nSo, you know very well that the model in which v4l2 controls interact\nbetween each other is ... not optimal. To make things worse controls\nfor a frame are applied at different times to compensate for their\ndelays. At what time would you read controls back from the sensor ?\n\nWe have DelayedControls::get() which works with 'sequence' numbers and\nour IPA reasons in terms of request numbers and I didn't want to\ndesign something around DelayedControls as some pipelines might not\nuse them.\n\n\n> If not can't we make configure() simpler by removing the ABI sharing of\n> this dynamic control range? And in the longer run just drop\n> V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?\n> That way we would have more static only controls to care about :-)\n>\n> I think it's dangerous to do both, init the IPA with the values from the\n> kernel and then calculate our own later.\n\nThose two things happen at very different times. configure() happens\nwhile no frames are captured and the driver state is a snapshot of the\ncurrent situation. updateControlsLimits() happens at runtime and the\ndelays (both the controls delays and the frame latency) should be\ntaken into account.\n\nHowever I understand your concern that \"our\" calculations could\ndiverge from the \"driver\" calculations, but to be honest the only way\nthis can happen is with a broken driver (or a broken IPA).\n\nAnyway, I'll drop this patch. Camera::controls() are already b0rken,\nupdating them would open space for bugs by invalidating references,\nand if it's not been an issue so far ... let's not be too concerned\nwith them\n\nI feel more strongly about 1/3 as that actually fixes a bug.\n\n>\n> Same comment for 1/3 where the new max exposure time is calculated.\n\nI don't agree here.\n\nIn 1/3 the max exposure time is calculated at the time where a new\nvblank is computed by the IPA. From that time on, all the IPA\ncalculations should use the new limits as all the IPA calculations\nwill produce control values to be applied to frames generated -after-\nthe just calculated vblank is applied to the sensor.\n\nThe actual VBLANK will be updated on the sensor with a latency of a\nvariable number of frames and we can't rely on the sensor calculated\nvalues to update the IPA internal limits. Plus, it's an IOCTL more for\nevery frame.\n\nI understand Stefan's argument about the exposure margin, and that\nshould be taken into account, but I would be suprised if poking at the\nsensor to update an internal IPA tuning paramter would be desirable,\nconsidering the potential latency and costs.\n\nWhat do you think ?\n>\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > {\n> > \tfor (const IPABuffer &buffer : buffers) {\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > \tif (!isRaw_ && !info->paramDequeued)\n> > \t\treturn;\n> >\n> > +\t/* Update controls before completing the request */\n> > +\tdata->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n> > +\n> > \tdata->frameInfo_.destroy(info->frame);\n> >\n> > \tcompleteRequest(request);\n> >\n> > --\n> > 2.51.0\n> >\n>\n> --\n> Kind Regards,\n> Niklas Söderlund","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 E78D7BE080\n\tfor ;\n\tTue, 21 Oct 2025 14:01:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2FFF6077E;\n\tTue, 21 Oct 2025 16:01:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A7DCB6075B\n\tfor ;\n\tTue, 21 Oct 2025 16:01:08 +0200 (CEST)","from ideasonboard.com (mob-5-90-52-220.net.vodafone.it\n\t[5.90.52.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C6AD86A6;\n\tTue, 21 Oct 2025 15:59: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=\"BeodGXMq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761055165;\n\tbh=8G6Oct0KuZV49vYPyn61OGVIqkJU839Wkh3pFpf/lgM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BeodGXMqnO3qFKs0V10W8dDXxh4tj5vn8rrAkuBnhVnarIClTWsANXYDxXN5fJTX9\n\tDjNSqgRzTbFi6nPtIFKgTAz4QRdJ7geN4ReS/VXnpdz+ycZCnxuYQHH1haQLdHFwQj\n\t6caXSj5Wkrlph4/q+zOyH2k4T6wD8BJkm/aBfK40=","Date":"Tue, 21 Oct 2025 16:01:03 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","Message-ID":"","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>\n\t<20251021113205.GA1694476@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20251021113205.GA1694476@ragnatech.se>","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":36378,"web_url":"https://patchwork.libcamera.org/comment/36378/","msgid":"","date":"2025-10-21T14:02:59","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Tue, Oct 21, 2025 at 10:06:21AM +0200, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Quoting Jacopo Mondi (2025-10-21 09:05:01)\n> > Hi Stefan\n> >\n> > On Mon, Oct 20, 2025 at 02:39:06PM +0200, Stefan Klug wrote:\n> > > Hi Jacopo,\n> > >\n> > > Quoting Jacopo Mondi (2025-10-17 11:00:06)\n> > > > The limits (min and max values) for the Camera controls are\n> > > > registered at Camera configuration time and never updated.\n> > > > Some controls, like FrameDurationLimits have a direct impact on\n> > > > the limits of other controls, such as the ExposureTime and as they\n> > > > can be configured by the application, this has to be taken into account.\n> > > >\n> > > > Currently, when a user changes the frame duration, the limits for\n> > > > both the ExposureTime and FrameDurationControls are not updated.\n> > > >\n> > > > The timing at which the controls limits should be updated is also\n> > > > critical: the new control values take effect once the Request they\n> > > > belong to is completed.\n> > > >\n> > > > To support this operation model introduce a new IPA function\n> > > > 'updateControlsLimits()' which the pipeline handler calls before\n> > > > completing a Request.\n> > > >\n> > > > Store the exposure time limits in the FrameContext (frame\n> > > > duration limits were already there) and update the Camera::controls()\n> > > > control info map with the limits as computed for the Request that has\n> > > > just completed.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/ipa/rkisp1.mojom | 2 ++\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> > > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> > > > 5 files changed, 37 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> > > > map streamConfig)\n> > > > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > >\n> > > > + updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > > +\n> > > > mapBuffers(array buffers);\n> > > > unmapBuffers(array ids);\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > * frameContext.agc.exposure;\n> > > > maxExposureTime = minExposureTime;\n> > > > }\n> > > > + frameContext.agc.minExposureTime = minExposureTime;\n> > > > + frameContext.agc.maxExposureTime = maxExposureTime;\n> > > >\n> > > > if (frameContext.agc.autoGainEnabled) {\n> > > > minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > * applied to the sensor when the statistics were collected.\n> > > > */\n> > > > utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> > > > + frameContext.agc.exposureTime = exposureTime;\n> > > > double analogueGain = frameContext.sensor.gain;\n> > > > utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -143,6 +143,9 @@ struct IPAActiveState {\n> > > >\n> > > > struct IPAFrameContext : public FrameContext {\n> > > > struct {\n> > > > + utils::Duration minExposureTime;\n> > > > + utils::Duration maxExposureTime;\n> > > > + utils::Duration exposureTime;\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > double exposureValue;\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -62,6 +62,8 @@ public:\n> > > > int configure(const IPAConfigInfo &ipaConfig,\n> > > > const std::map &streamConfig,\n> > > > ControlInfoMap *ipaControls) override;\n> > > > + int updateControlsLimits(const uint32_t frame,\n> > > > + ControlInfoMap *ipaControls) override;\n> > > > void mapBuffers(const std::vector &buffers) override;\n> > > > void unmapBuffers(const std::vector &ids) override;\n> > > >\n> > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> > > > +{\n> > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > +\n> > > > + /*\n> > > > + * Update the exposure time and frame duration limits with the\n> > > > + * settings computed by the AGC for the frame at hand.\n> > > > + */\n> > > > + assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> > > > + assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> > > > +\n> > > > + ControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> > > > + ControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> > > > + ControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> > > > + ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> > > > +\n> > > > + ControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> > > > + ControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> > > > + ControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> > > > + ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n> > >\n> > > Do we really need to update the frame duration limits? Aren't these\n> > > actually static by vblank min/max + output_size?\n> > >\n> >\n> > Shouldn't Camera::controls()[FrameDurationLimits] report what the user\n> > has set as FrameDurationLimits ?\n>\n> My understanding was that Camera::controls()[FrameDurationLimits]\n> reports the limits of the control and not the current state of the\n> camera. So min/max shouldn't change as these are roughly a sensor\n> property. (If we would change the max to the currently selected upper\n> limit and would check the limits, the user would never be able to raise\n> the upper limit again). Using the default value to represent the current\n> state would be possible, but is to my understanding not the current\n> design idea. My understanding was that the default is the camera\n> default/startup value, possibly modified once after configuration.\n>\n\nAck, makes sense. FrameDurationLimits should not be updated...\n\n> >\n> > Although, if your application sets a FrameDurationLimit to [x, x] to\n> > fix the frame rate, then your overall camera limits will be shown as\n> > [x, x] which is pretty useless.\n> >\n> > Should the Camera::controls[FrameDurationLimits] remain static for a\n> > the whole capture session then ?\n>\n> Yes, I believe so.\n>\n> >\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > > > {\n> > > > for (const IPABuffer &buffer : buffers) {\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > > > if (!isRaw_ && !info->paramDequeued)\n> > > > return;\n> > > >\n> > > > + /* Update controls before completing the request */\n> > > > + data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n> > >\n> > > Maybe I'm missing something here. How does this work in an isolated IPA?\n> > > To my understanding the ControlInfoMap would be deserialized on it's way\n> > > back from the ipa and call the assignment operator on\n> > > data->controlInfo_, breaking any user application that holds a iterator\n> > > on controlInfo_.\n> >\n> > My thinking was that updating an entry in a map won't invalidate\n> > iterators, which could be invalidated if the map is resized.\n> >\n> > However, if I got you right, the ser/deser process will re-assign the\n> > map. Is this your concern ?\n>\n> Yes, exactly.\n>\n> >\n> > There is however an underlying issue with concurrency. If the\n> > application is accessing the ControlInfo exactly while the IPA udpates\n> > it. Note that we have the same issue when updating ScalerCrop in\n> > example as many pipelines do.\n> >\n> > I don't have a solution for this at the moment. I'm afraid it will\n> > require some smart serialization and going throuhg a C ABI.\n>\n> Yes, that is a pain. My strategy there was to keep controls static after\n> configure(). But I agree that we need to update the max exposure time.\n>\n> >\n> > >\n> > > Could we assume that minFrameDuration doesn't change? Then we could\n> >\n> > I think so, yes\n> >\n> > > potentially only add the new maxFrameDuration to the setSensorControls\n> > > event and spare a new call into the ipa?\n> >\n> > I think the most relevant control to update is ExposureTime and not\n> > FrameDurationLimits (see the above discussion).\n>\n> Oh yes, I picked the wrong variable. I meant max exposure time.\n>\n> >\n> > Updating ExposureTime at setSensorControls() time doesn't however\n> > happen at the \"right\" time. Actual controls are applied to the sensor\n> > with a few frames of latency by DelayedControls. If we immediately\n> > update the Camera::controls() limits, we would do that before the\n> > actual Request that has triggered the update has completed.\n> >\n> > And honestly I would like to avoid ad-hoc things where a single\n> > control is updated, but maybe ExposureTime is actually the only thing\n> > we need to care about ?\n>\n> I eagerly need to post my regulation rework series, as that also impacts\n> timing and reorders a few things. I hope I can do that this week...\n\nAs said, I'll drop this patch. Things are already broken there and\nlet's not make them worse. What I mostly care about is 1/3.\n\nThanks\n j\n\n>\n> Best regards,\n> Stefan\n>\n> >\n> > >\n> > > What do you think?\n> > >\n> > > Best regards,\n> > > Stefan\n> > >\n> > > > +\n> > > > data->frameInfo_.destroy(info->frame);\n> > > >\n> > > > completeRequest(request);\n> > > >\n> > > > --\n> > > > 2.51.0\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 ACCF3BE080\n\tfor ;\n\tTue, 21 Oct 2025 14:03:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C701160781;\n\tTue, 21 Oct 2025 16:03:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66AE060777\n\tfor ;\n\tTue, 21 Oct 2025 16:03:04 +0200 (CEST)","from ideasonboard.com (mob-5-90-52-220.net.vodafone.it\n\t[5.90.52.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A2C266AF;\n\tTue, 21 Oct 2025 16:01:20 +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=\"aeGPRiQM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761055280;\n\tbh=WGcCUuQ4amz/tXFJ5nXNorckcaW557NvGaQier1pKNw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aeGPRiQMKtew+sWNsnr/xPm087X7VS/Kb2j0xd2vsQLGqkQNKduaqE0n+Aa3eOm83\n\tJatNXo6sO0JNeXPoWR/Xt9Etv09Nmf/2FTXJaEoUhc2psRZRPy3UNHYd5d8l27OfVO\n\t7HjjUgxR9RlxQn5Eyp59S+6nuGSADpztXC+TCJ04=","Date":"Tue, 21 Oct 2025 16:02:59 +0200","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","Message-ID":"","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>\n\t<176096394628.336133.5694591603975962693@localhost>\n\t\n\t<176103398116.336133.12647958159424364778@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176103398116.336133.12647958159424364778@localhost>","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":36382,"web_url":"https://patchwork.libcamera.org/comment/36382/","msgid":"<7t2nuxkavqlvbuljitjvd6lmcz7x5slbh5jlafx2b3vpat526l@gviqyotqqxus>","date":"2025-10-22T07:22:58","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Oct 21, 2025 at 06:50:40PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2025-10-21 16:01:03 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:\n> > > > The limits (min and max values) for the Camera controls are\n> > > > registered at Camera configuration time and never updated.\n> > > > Some controls, like FrameDurationLimits have a direct impact on\n> > > > the limits of other controls, such as the ExposureTime and as they\n> > > > can be configured by the application, this has to be taken into account.\n> > > >\n> > > > Currently, when a user changes the frame duration, the limits for\n> > > > both the ExposureTime and FrameDurationControls are not updated.\n> > > >\n> > > > The timing at which the controls limits should be updated is also\n> > > > critical: the new control values take effect once the Request they\n> > > > belong to is completed.\n> > > >\n> > > > To support this operation model introduce a new IPA function\n> > > > 'updateControlsLimits()' which the pipeline handler calls before\n> > > > completing a Request.\n> > > >\n> > > > Store the exposure time limits in the FrameContext (frame\n> > > > duration limits were already there) and update the Camera::controls()\n> > > > control info map with the limits as computed for the Request that has\n> > > > just completed.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/ipa/rkisp1.mojom | 2 ++\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> > > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> > > > 5 files changed, 37 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> > > > \t\t map streamConfig)\n> > > > \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > >\n> > > > +\tupdateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > > +\n> > > > \tmapBuffers(array buffers);\n> > > > \tunmapBuffers(array ids);\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > \t\t\t\t* frameContext.agc.exposure;\n> > > > \t\tmaxExposureTime = minExposureTime;\n> > > > \t}\n> > > > +\tframeContext.agc.minExposureTime = minExposureTime;\n> > > > +\tframeContext.agc.maxExposureTime = maxExposureTime;\n> > > >\n> > > > \tif (frameContext.agc.autoGainEnabled) {\n> > > > \t\tminAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > \t * applied to the sensor when the statistics were collected.\n> > > > \t */\n> > > > \tutils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> > > > +\tframeContext.agc.exposureTime = exposureTime;\n> > > > \tdouble analogueGain = frameContext.sensor.gain;\n> > > > \tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -143,6 +143,9 @@ struct IPAActiveState {\n> > > >\n> > > > struct IPAFrameContext : public FrameContext {\n> > > > \tstruct {\n> > > > +\t\tutils::Duration minExposureTime;\n> > > > +\t\tutils::Duration maxExposureTime;\n> > > > +\t\tutils::Duration exposureTime;\n> > > > \t\tuint32_t exposure;\n> > > > \t\tdouble gain;\n> > > > \t\tdouble exposureValue;\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -62,6 +62,8 @@ public:\n> > > > \tint configure(const IPAConfigInfo &ipaConfig,\n> > > > \t\t const std::map &streamConfig,\n> > > > \t\t ControlInfoMap *ipaControls) override;\n> > > > +\tint updateControlsLimits(const uint32_t frame,\n> > > > +\t\t\t\t ControlInfoMap *ipaControls) override;\n> > > > \tvoid mapBuffers(const std::vector &buffers) override;\n> > > > \tvoid unmapBuffers(const std::vector &ids) override;\n> > > >\n> > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> > > > +{\n> > > > +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > +\n> > > > +\t/*\n> > > > +\t * Update the exposure time and frame duration limits with the\n> > > > +\t * settings computed by the AGC for the frame at hand.\n> > > > +\t */\n> > > > +\tassert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> > > > +\tassert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> > > > +\n> > > > +\tControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> > > > +\tControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> > > > +\tControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> > > > +\tipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> > > > +\n> > > > +\tControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> > > > +\tControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> > > > +\tControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> > > > +\tipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n> > >\n> > > Is this function not doing the reveres of what is expected? It takes the\n> > > controls from the sensor and update them with new limits using values\n> > > calculated by the IPA. Should it not take the values updated by the\n> >\n> > Where does it take controls from the sensor ?\n> >\n> > > sensor kernel driver and propagate them to the values used by the IPA?\n> >\n> > As I see it, it takes the control values the IPA has calculated for\n> > the sensor for frame/request X [*] and when request X completes it\n> > propagates those values to Camera::controls().\n> >\n> > [*] We have an outstanding duality between request number and frame\n> > number. Hopefully we should sooner or later move the whole PH/IPA\n> > interface to be designed around frame numbers and not request ids.\n> >\n> > >\n> > > In IPARkISP1::configure() we read the min and max exposure time from the\n> > > V4L2 sensor control, here we override them with new values we calculated\n> > > ourself.\n> > >\n> > > If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK\n> > > control new max and default values for V4L2_CID_EXPOSURE are calculated\n> > > and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate\n> > > these values instead of calculating it's own set?\n> > >\n> >\n> > So, you know very well that the model in which v4l2 controls interact\n> > between each other is ... not optimal. To make things worse controls\n> > for a frame are applied at different times to compensate for their\n> > delays. At what time would you read controls back from the sensor ?\n> >\n> > We have DelayedControls::get() which works with 'sequence' numbers and\n> > our IPA reasons in terms of request numbers and I didn't want to\n> > design something around DelayedControls as some pipelines might not\n> > use them.\n> >\n> >\n> > > If not can't we make configure() simpler by removing the ABI sharing of\n> > > this dynamic control range? And in the longer run just drop\n> > > V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?\n> > > That way we would have more static only controls to care about :-)\n> > >\n> > > I think it's dangerous to do both, init the IPA with the values from the\n> > > kernel and then calculate our own later.\n> >\n> > Those two things happen at very different times. configure() happens\n> > while no frames are captured and the driver state is a snapshot of the\n> > current situation. updateControlsLimits() happens at runtime and the\n> > delays (both the controls delays and the frame latency) should be\n> > taken into account.\n> >\n> > However I understand your concern that \"our\" calculations could\n> > diverge from the \"driver\" calculations, but to be honest the only way\n> > this can happen is with a broken driver (or a broken IPA).\n> >\n> > Anyway, I'll drop this patch. Camera::controls() are already b0rken,\n> > updating them would open space for bugs by invalidating references,\n> > and if it's not been an issue so far ... let's not be too concerned\n> > with them\n> >\n> > I feel more strongly about 1/3 as that actually fixes a bug.\n> >\n> > >\n> > > Same comment for 1/3 where the new max exposure time is calculated.\n> >\n> > I don't agree here.\n> >\n> > In 1/3 the max exposure time is calculated at the time where a new\n> > vblank is computed by the IPA. From that time on, all the IPA\n> > calculations should use the new limits as all the IPA calculations\n> > will produce control values to be applied to frames generated -after-\n> > the just calculated vblank is applied to the sensor.\n> >\n> > The actual VBLANK will be updated on the sensor with a latency of a\n> > variable number of frames and we can't rely on the sensor calculated\n> > values to update the IPA internal limits. Plus, it's an IOCTL more for\n> > every frame.\n> >\n> > I understand Stefan's argument about the exposure margin, and that\n> > should be taken into account, but I would be suprised if poking at the\n> > sensor to update an internal IPA tuning paramter would be desirable,\n> > considering the potential latency and costs.\n> >\n> > What do you think ?\n>\n> I agree, the V4L2 control interface is not always optimal, and likely\n> that is at the core of my concern.\n>\n> The fact that we calculate the same thing (exposure time) both in the\n> kernel driver and in the de facto standard user-space consumer of V4L2,\n> without correlating the two hints at a larger issue.\n>\n> And to make issues even more \"fun\" the complete documentation for the\n> control we calculate the min/max/def values for independently on is:\n>\n> V4L2_CID_EXPOSURE (integer)\n>\n> Exposure (cameras). [Unit?]\n>\n> This is what I hinted at (poorly) in my original mail of this being\n> dangerous. I think it's not impossible for the kernel and libcamera to\n> disagree about that the control represent. At least by always fetching\n\nWe know the control's units are not specified. I think we tried to\nchange upstream to specify the control unit is in lines, but afair this\nwasn't appreciated as it would make some existing drivers not-compliant.\n\nThat's why we have Documentation/sensor_driver_requirements.rst\nwhere we say:\n\nWhile V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\nrequires it to be expressed as a number of image lines. Camera sensor drivers\nthat do not comply with this requirement will need to be adapted or will produce\nincorrect results.\n\n> the min/max/def values from the control we know the boundaries. But as\n> you hint at in your reply, and I on IRC, this would cost an IOCTL per\n> control everything something changes. Plus the plumbing to get it from\n> pipeline handler to IPA.\n>\n> And I think we should either\n>\n> - Fix or update the V4L2 control. This could be as \"simple\" as\n> updating the documentation for V4L2_CID_EXPOSURE to define what it\n> really is, it's unit and that its min and max values should not\n> change while streaming. Instead it shall be set to the min/max\n> values to correspond to the min/max values of the VBLANK (or any\n> other control effecting this sensors min/max value). But also\n> describe how they are limited by other controls and that it's up to\n> user-space to deal with figuring it out.\n\nI think we have a different interpretation of the issues around\nEXPOSURE. By points:\n\n- its unit: you're right, but it didn't fly upstream, so\n sensor_driver_requirements.rst\n- its min/max: min is a sensor-specific parameter and should stay\n constant for a mode. max is inhertely limited by the current frame\n length (visible + blankings) and I presume that's a given, isn't it?\n- min/max can't change while streaming: I agree with min, but isn't max\n changing according to current blanking the whole reason for this\n series ?\n\n>\n> Then we can fix drivers like the IMX219 to comply with this and then\n> we get the correct min/max values at IPA configure time. We know the\n\nWhy do you think imx219 is broken in regards to handling EXPOSURE ?\n\nAfaiu the driver does what's intended by adjusting the max exposure to the\ncurrent frame lenght. It's the IPA that doesn't take this into\naccount.\n\n\n> unit they are in. We know the rules and we know that user-space is\n> responsible to calculate a set of controls that work together. The\n> kernel will only validate and ether accept and reject them.\n>\n> or\n>\n> - Allow controls to dynamic change their min/max values while\n\nI might be missing something. Isn't that how it works already ? By\nchanging VBLANK you control the frame rate and, as a consequence, the\nmaximum exposure value changes as well. What did I miss ?\n\n> streaming. Have libcamera fetch new limit every time it changes a\n> control. This will be a lot of IOCTL callas which will cause pain\n> and hopefully spur someone to create a better control interface ;-)\n>\n> I would not block patch 1/3 as it makes things better for sensors like\n> IMX219 which is used and implement the V4L2_CID_EXPOSURE with dynamic\n> values but with a unit that is the same as libcamera uses internally.\n> But I do think 1/3 without also thinking about how to solve the API\n> issue with the kernel will create more issues down the line.\n>\n\nThe real solution here would be to drop V/HBLANK and introduce a\ncontrol for the whole frame/line lengths. In this way you\nautomatically know what the exposure time maximum is without having to\ntake into account the visible sizes + blankings. It's quite a yak to\nshave though and we do have anyway all the sensor drivers using\nVBLANK/HBLANK and they can't be changed as otherwise existing\nuserspace that relies on those controls presence would break.\n\nFeels like I'm missing something somewhere ?\n\n> > >\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > > > {\n> > > > \tfor (const IPABuffer &buffer : buffers) {\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > > > \tif (!isRaw_ && !info->paramDequeued)\n> > > > \t\treturn;\n> > > >\n> > > > +\t/* Update controls before completing the request */\n> > > > +\tdata->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n> > > > +\n> > > > \tdata->frameInfo_.destroy(info->frame);\n> > > >\n> > > > \tcompleteRequest(request);\n> > > >\n> > > > --\n> > > > 2.51.0\n> > > >\n> > >\n> > > --\n> > > Kind Regards,\n> > > Niklas Söderlund\n>\n> --\n> Kind Regards,\n> Niklas Söderlund","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 59DD2C3259\n\tfor ;\n\tWed, 22 Oct 2025 07:23:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42DAC607A1;\n\tWed, 22 Oct 2025 09:23:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2439606E9\n\tfor ;\n\tWed, 22 Oct 2025 09:23:04 +0200 (CEST)","from ideasonboard.com (mob-5-90-62-95.net.vodafone.it [5.90.62.95])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63CC0557;\n\tWed, 22 Oct 2025 09:21:20 +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=\"tCeRA0xa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761117680;\n\tbh=c171km+hpsYcKFiVuJPIoB0lOa2QjgB6Bh5Va+wRZ9M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tCeRA0xaRDWMcqOzUZ9NzPz/X3qpdhwXRSfu76PLlX5fZwxhl9jLuW4RtMiSnM9no\n\tU7b/RZgBGs2EDM7tBz1eUrFCuaDFwWF+/RvN64umNJXZemEwq241XW4droKFAWUoIs\n\tqEzlb52U7pBAMi6VZMC9AKmKB6TgZSVXDlfBhV3o=","Date":"Wed, 22 Oct 2025 09:22:58 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","Message-ID":"<7t2nuxkavqlvbuljitjvd6lmcz7x5slbh5jlafx2b3vpat526l@gviqyotqqxus>","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>\n\t<20251021113205.GA1694476@ragnatech.se>\n\t\n\t<20251021165040.GB1694476@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20251021165040.GB1694476@ragnatech.se>","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":36385,"web_url":"https://patchwork.libcamera.org/comment/36385/","msgid":"<2k2zueuunu6dj3xya5r4kefgnhzxiblr3lfkfvur6xij22pi42@lyyhscjb2377>","date":"2025-10-22T09:30:48","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, Oct 22, 2025 at 10:16:48AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2025-10-22 09:22:58 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Tue, Oct 21, 2025 at 06:50:40PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > On 2025-10-21 16:01:03 +0200, Jacopo Mondi wrote:\n> > > > Hi Niklas,\n> > > >\n> > > > On Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Thanks for your work.\n> > > > >\n> > > > > On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:\n> > > > > > The limits (min and max values) for the Camera controls are\n> > > > > > registered at Camera configuration time and never updated.\n> > > > > > Some controls, like FrameDurationLimits have a direct impact on\n> > > > > > the limits of other controls, such as the ExposureTime and as they\n> > > > > > can be configured by the application, this has to be taken into account.\n> > > > > >\n> > > > > > Currently, when a user changes the frame duration, the limits for\n> > > > > > both the ExposureTime and FrameDurationControls are not updated.\n> > > > > >\n> > > > > > The timing at which the controls limits should be updated is also\n> > > > > > critical: the new control values take effect once the Request they\n> > > > > > belong to is completed.\n> > > > > >\n> > > > > > To support this operation model introduce a new IPA function\n> > > > > > 'updateControlsLimits()' which the pipeline handler calls before\n> > > > > > completing a Request.\n> > > > > >\n> > > > > > Store the exposure time limits in the FrameContext (frame\n> > > > > > duration limits were already there) and update the Camera::controls()\n> > > > > > control info map with the limits as computed for the Request that has\n> > > > > > just completed.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > include/libcamera/ipa/rkisp1.mojom | 2 ++\n> > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> > > > > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > > > > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> > > > > > 5 files changed, 37 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> > > > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> > > > > > \t\t map streamConfig)\n> > > > > > \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > > > >\n> > > > > > +\tupdateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > > > > +\n> > > > > > \tmapBuffers(array buffers);\n> > > > > > \tunmapBuffers(array ids);\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > \t\t\t\t* frameContext.agc.exposure;\n> > > > > > \t\tmaxExposureTime = minExposureTime;\n> > > > > > \t}\n> > > > > > +\tframeContext.agc.minExposureTime = minExposureTime;\n> > > > > > +\tframeContext.agc.maxExposureTime = maxExposureTime;\n> > > > > >\n> > > > > > \tif (frameContext.agc.autoGainEnabled) {\n> > > > > > \t\tminAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > > > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > \t * applied to the sensor when the statistics were collected.\n> > > > > > \t */\n> > > > > > \tutils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> > > > > > +\tframeContext.agc.exposureTime = exposureTime;\n> > > > > > \tdouble analogueGain = frameContext.sensor.gain;\n> > > > > > \tutils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > @@ -143,6 +143,9 @@ struct IPAActiveState {\n> > > > > >\n> > > > > > struct IPAFrameContext : public FrameContext {\n> > > > > > \tstruct {\n> > > > > > +\t\tutils::Duration minExposureTime;\n> > > > > > +\t\tutils::Duration maxExposureTime;\n> > > > > > +\t\tutils::Duration exposureTime;\n> > > > > > \t\tuint32_t exposure;\n> > > > > > \t\tdouble gain;\n> > > > > > \t\tdouble exposureValue;\n> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > @@ -62,6 +62,8 @@ public:\n> > > > > > \tint configure(const IPAConfigInfo &ipaConfig,\n> > > > > > \t\t const std::map &streamConfig,\n> > > > > > \t\t ControlInfoMap *ipaControls) override;\n> > > > > > +\tint updateControlsLimits(const uint32_t frame,\n> > > > > > +\t\t\t\t ControlInfoMap *ipaControls) override;\n> > > > > > \tvoid mapBuffers(const std::vector &buffers) override;\n> > > > > > \tvoid unmapBuffers(const std::vector &ids) override;\n> > > > > >\n> > > > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > > > > \treturn 0;\n> > > > > > }\n> > > > > >\n> > > > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> > > > > > +{\n> > > > > > +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > > +\n> > > > > > +\t/*\n> > > > > > +\t * Update the exposure time and frame duration limits with the\n> > > > > > +\t * settings computed by the AGC for the frame at hand.\n> > > > > > +\t */\n> > > > > > +\tassert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> > > > > > +\tassert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> > > > > > +\n> > > > > > +\tControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> > > > > > +\tControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> > > > > > +\tControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> > > > > > +\tipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> > > > > > +\n> > > > > > +\tControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> > > > > > +\tControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> > > > > > +\tControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> > > > > > +\tipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n> > > > >\n> > > > > Is this function not doing the reveres of what is expected? It takes the\n> > > > > controls from the sensor and update them with new limits using values\n> > > > > calculated by the IPA. Should it not take the values updated by the\n> > > >\n> > > > Where does it take controls from the sensor ?\n> > > >\n> > > > > sensor kernel driver and propagate them to the values used by the IPA?\n> > > >\n> > > > As I see it, it takes the control values the IPA has calculated for\n> > > > the sensor for frame/request X [*] and when request X completes it\n> > > > propagates those values to Camera::controls().\n> > > >\n> > > > [*] We have an outstanding duality between request number and frame\n> > > > number. Hopefully we should sooner or later move the whole PH/IPA\n> > > > interface to be designed around frame numbers and not request ids.\n> > > >\n> > > > >\n> > > > > In IPARkISP1::configure() we read the min and max exposure time from the\n> > > > > V4L2 sensor control, here we override them with new values we calculated\n> > > > > ourself.\n> > > > >\n> > > > > If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK\n> > > > > control new max and default values for V4L2_CID_EXPOSURE are calculated\n> > > > > and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate\n> > > > > these values instead of calculating it's own set?\n> > > > >\n> > > >\n> > > > So, you know very well that the model in which v4l2 controls interact\n> > > > between each other is ... not optimal. To make things worse controls\n> > > > for a frame are applied at different times to compensate for their\n> > > > delays. At what time would you read controls back from the sensor ?\n> > > >\n> > > > We have DelayedControls::get() which works with 'sequence' numbers and\n> > > > our IPA reasons in terms of request numbers and I didn't want to\n> > > > design something around DelayedControls as some pipelines might not\n> > > > use them.\n> > > >\n> > > >\n> > > > > If not can't we make configure() simpler by removing the ABI sharing of\n> > > > > this dynamic control range? And in the longer run just drop\n> > > > > V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?\n> > > > > That way we would have more static only controls to care about :-)\n> > > > >\n> > > > > I think it's dangerous to do both, init the IPA with the values from the\n> > > > > kernel and then calculate our own later.\n> > > >\n> > > > Those two things happen at very different times. configure() happens\n> > > > while no frames are captured and the driver state is a snapshot of the\n> > > > current situation. updateControlsLimits() happens at runtime and the\n> > > > delays (both the controls delays and the frame latency) should be\n> > > > taken into account.\n> > > >\n> > > > However I understand your concern that \"our\" calculations could\n> > > > diverge from the \"driver\" calculations, but to be honest the only way\n> > > > this can happen is with a broken driver (or a broken IPA).\n> > > >\n> > > > Anyway, I'll drop this patch. Camera::controls() are already b0rken,\n> > > > updating them would open space for bugs by invalidating references,\n> > > > and if it's not been an issue so far ... let's not be too concerned\n> > > > with them\n> > > >\n> > > > I feel more strongly about 1/3 as that actually fixes a bug.\n> > > >\n> > > > >\n> > > > > Same comment for 1/3 where the new max exposure time is calculated.\n> > > >\n> > > > I don't agree here.\n> > > >\n> > > > In 1/3 the max exposure time is calculated at the time where a new\n> > > > vblank is computed by the IPA. From that time on, all the IPA\n> > > > calculations should use the new limits as all the IPA calculations\n> > > > will produce control values to be applied to frames generated -after-\n> > > > the just calculated vblank is applied to the sensor.\n> > > >\n> > > > The actual VBLANK will be updated on the sensor with a latency of a\n> > > > variable number of frames and we can't rely on the sensor calculated\n> > > > values to update the IPA internal limits. Plus, it's an IOCTL more for\n> > > > every frame.\n> > > >\n> > > > I understand Stefan's argument about the exposure margin, and that\n> > > > should be taken into account, but I would be suprised if poking at the\n> > > > sensor to update an internal IPA tuning paramter would be desirable,\n> > > > considering the potential latency and costs.\n> > > >\n> > > > What do you think ?\n> > >\n> > > I agree, the V4L2 control interface is not always optimal, and likely\n> > > that is at the core of my concern.\n> > >\n> > > The fact that we calculate the same thing (exposure time) both in the\n> > > kernel driver and in the de facto standard user-space consumer of V4L2,\n> > > without correlating the two hints at a larger issue.\n> > >\n> > > And to make issues even more \"fun\" the complete documentation for the\n> > > control we calculate the min/max/def values for independently on is:\n> > >\n> > > V4L2_CID_EXPOSURE (integer)\n> > >\n> > > Exposure (cameras). [Unit?]\n> > >\n> > > This is what I hinted at (poorly) in my original mail of this being\n> > > dangerous. I think it's not impossible for the kernel and libcamera to\n> > > disagree about that the control represent. At least by always fetching\n> >\n> > We know the control's units are not specified. I think we tried to\n> > change upstream to specify the control unit is in lines, but afair this\n> > wasn't appreciated as it would make some existing drivers not-compliant.\n> >\n> > That's why we have Documentation/sensor_driver_requirements.rst\n> > where we say:\n> >\n> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > requires it to be expressed as a number of image lines. Camera sensor drivers\n> > that do not comply with this requirement will need to be adapted or will produce\n> > incorrect results.\n>\n> Thanks I was not aware of this narrowing of the control by libcamera. I\n> think it's bonkers that it's needed and that upstream can't agree on a\n> definition of a control, but that is another topic. For this topic I'm\n> happy libcamera states what the control is.\n>\n> >\n> > > the min/max/def values from the control we know the boundaries. But as\n> > > you hint at in your reply, and I on IRC, this would cost an IOCTL per\n> > > control everything something changes. Plus the plumbing to get it from\n> > > pipeline handler to IPA.\n> > >\n> > > And I think we should either\n> > >\n> > > - Fix or update the V4L2 control. This could be as \"simple\" as\n> > > updating the documentation for V4L2_CID_EXPOSURE to define what it\n> > > really is, it's unit and that its min and max values should not\n> > > change while streaming. Instead it shall be set to the min/max\n> > > values to correspond to the min/max values of the VBLANK (or any\n> > > other control effecting this sensors min/max value). But also\n> > > describe how they are limited by other controls and that it's up to\n> > > user-space to deal with figuring it out.\n> >\n> > I think we have a different interpretation of the issues around\n> > EXPOSURE. By points:\n> >\n> > - its unit: you're right, but it didn't fly upstream, so\n> > sensor_driver_requirements.rst\n> > - its min/max: min is a sensor-specific parameter and should stay\n> > constant for a mode. max is inhertely limited by the current frame\n> > length (visible + blankings) and I presume that's a given, isn't it?\n> > - min/max can't change while streaming: I agree with min, but isn't max\n> > changing according to current blanking the whole reason for this\n> > series ?\n>\n> I think we both see the issue but assign different importance too it. My\n> concern is that we at IPA configure time take the effort to consult the\n> min/max values from the V4L2 control, but while streaming we adjust the\n> max value by our own calculations instead of consuming the updated value\n> calculated and provided by the V4L2 API.\n>\n> This demonstrates how cumbersome the API is and I think libcamera as the\n> de-facto user-space consumer of V4L2 API should either use the\n> cumbersome API, or fix it. Working around it will IMHO just create more\n> fragmentation and issues down the road.\n>\n> At the very least I think if the min/max values of the V4L2 API is to be\n> ignored for the exposure control our own method to calculate it shall be\n> used at IPA configure time too. Mixing the two is very confusing for\n> somebody reading the code.\n>\n\nI'm not sure I 100% share the concern here. configure() time happens\nonce per streaming session and I think we should have a snapshot of\nthe device state and reading control values seems the sane thing to do\n\n> >\n> > >\n> > > Then we can fix drivers like the IMX219 to comply with this and then\n> > > we get the correct min/max values at IPA configure time. We know the\n> >\n> > Why do you think imx219 is broken in regards to handling EXPOSURE ?\n> >\n> > Afaiu the driver does what's intended by adjusting the max exposure to the\n> > current frame lenght. It's the IPA that doesn't take this into\n> > account.\n>\n> I don't think it's broken. I think the dynamic update of controls\n> min/max (any control) while streaming is an edge case of the V4L2 not\n> very well designed. The comment above about fixing the IMX219 I tried to\n> hint at that if we documented this better we could update the drier to\n> not update min/max while streaming, or make it clear that user-space\n\nI don't see it as an edge case, changing blankings to control frame\nrate is \"the right thing to do\"(tm)\n\n> must be prepared to read new min/max values while streaming.\n>\n> >\n> >\n> > > unit they are in. We know the rules and we know that user-space is\n> > > responsible to calculate a set of controls that work together. The\n> > > kernel will only validate and ether accept and reject them.\n> > >\n> > > or\n> > >\n> > > - Allow controls to dynamic change their min/max values while\n> >\n> > I might be missing something. Isn't that how it works already ? By\n> > changing VBLANK you control the frame rate and, as a consequence, the\n> > maximum exposure value changes as well. What did I miss ?\n>\n> It is. But as this libcamera change don't consume it and instead\n> calculate the max by itself hints that this might not be the best\n> design?\n>\n> >\n> > > streaming. Have libcamera fetch new limit every time it changes a\n> > > control. This will be a lot of IOCTL callas which will cause pain\n> > > and hopefully spur someone to create a better control interface ;-)\n> > >\n> > > I would not block patch 1/3 as it makes things better for sensors like\n> > > IMX219 which is used and implement the V4L2_CID_EXPOSURE with dynamic\n> > > values but with a unit that is the same as libcamera uses internally.\n> > > But I do think 1/3 without also thinking about how to solve the API\n> > > issue with the kernel will create more issues down the line.\n> > >\n> >\n> > The real solution here would be to drop V/HBLANK and introduce a\n> > control for the whole frame/line lengths. In this way you\n> > automatically know what the exposure time maximum is without having to\n> > take into account the visible sizes + blankings. It's quite a yak to\n> > shave though and we do have anyway all the sensor drivers using\n> > VBLANK/HBLANK and they can't be changed as otherwise existing\n> > userspace that relies on those controls presence would break.\n> >\n> > Feels like I'm missing something somewhere ?\n>\n> Or maybe I am :-)\n>\n> But I think we both see the issue, the V4L2 controls in this area are\n> not optimal designed. And there are edge cases of them that make it even\n> worse to work with. Then we assign different interpretations on what to\n> do about it.\n>\n> My understanding of your argument is that you are OK with calculating\n> the min/max values both in kernel and user-space to avoid having to do\n> extra IOCTLs to fetch the new values. This works for exposure and may\n> very well be the best way forward.\n\nI'm ok starting with a known state read from the subdevice and\nI'm ok updating an IPA runtime parameter based on the IPA calculated\nframe length. This will be also updated on the device, but at a later\ntime and we can't rely on that.\n\nIt's not just the extra ioctl, it's that controls are applied to the\nsensor with frames of latencies while the IPA need to keep its limits\nup-to-date while calculating parameters for the next frames.\n\n>\n> My argument and worry is that it's dangerous to create this split. We\n> have an API and we should use it, and if possible improve it. If not I\n> fear copy-paste programming will spread this behavior and we will have\n> IPA with hard coded calculations for a subset of sensors as it's OK to\n> ignore the API which describes the controls. My strong feelings around\n> this is that libcamera tries to be the mesa of cameras so it should not\n> ignore API interfaces IMHO. More painful, and maybe not the fastest/best\n> way forward.\n>\n> I will not try to block this work based on the above as fixing the API\n> is likely a huge task. But I do think if you want to calculate the max\n> independently for the kernel you should use the same calculation\n> function at IPA configure time and not use the V4L2 min/max values\n> there.\n>\n\nI'll ask others what do they think as the different configure/runtime\nbehaviours are not a concern to me and I might be underestimating it.\n\n> >\n> > > > >\n> > > > > > +\n> > > > > > +\treturn 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > > > > > {\n> > > > > > \tfor (const IPABuffer &buffer : buffers) {\n> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > > > > > \tif (!isRaw_ && !info->paramDequeued)\n> > > > > > \t\treturn;\n> > > > > >\n> > > > > > +\t/* Update controls before completing the request */\n> > > > > > +\tdata->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n> > > > > > +\n> > > > > > \tdata->frameInfo_.destroy(info->frame);\n> > > > > >\n> > > > > > \tcompleteRequest(request);\n> > > > > >\n> > > > > > --\n> > > > > > 2.51.0\n> > > > > >\n> > > > >\n> > > > > --\n> > > > > Kind Regards,\n> > > > > Niklas Söderlund\n> > >\n> > > --\n> > > Kind Regards,\n> > > Niklas Söderlund\n>\n> --\n> Kind Regards,\n> Niklas Söderlund","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 EC36CC3259\n\tfor ;\n\tWed, 22 Oct 2025 09:30:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D16DB607A8;\n\tWed, 22 Oct 2025 11:30:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08E70603ED\n\tfor ;\n\tWed, 22 Oct 2025 11:30:53 +0200 (CEST)","from ideasonboard.com (mob-5-90-62-95.net.vodafone.it [5.90.62.95])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A4DDA83D;\n\tWed, 22 Oct 2025 11:29:08 +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=\"Sa9xBi2D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761125348;\n\tbh=QxYnsgCGLHxDmNZ6JhGOGs9Mxp6Xcm++MBlZv3Q1s4w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Sa9xBi2DGvFdy5C9D3GlHKwXgKT7KiOemjJNlgnFzL91GoVOb9jjFAeJU3+tzN++l\n\tYJnOR/VNIVqvOV1/L4Muh64E8clanS1l9yAu0ub8BX5sb/hpX+/M3vdye29sVj9juf\n\t5VT6kxCFFA0ooTN8w6s6Q3PyvbMtfaC/x7QWXtXU=","Date":"Wed, 22 Oct 2025 11:30:48 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","Message-ID":"<2k2zueuunu6dj3xya5r4kefgnhzxiblr3lfkfvur6xij22pi42@lyyhscjb2377>","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>\n\t<20251021113205.GA1694476@ragnatech.se>\n\t\n\t<20251021165040.GB1694476@ragnatech.se>\n\t<7t2nuxkavqlvbuljitjvd6lmcz7x5slbh5jlafx2b3vpat526l@gviqyotqqxus>\n\t<20251022081648.GC1694476@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20251022081648.GC1694476@ragnatech.se>","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":36402,"web_url":"https://patchwork.libcamera.org/comment/36402/","msgid":"","date":"2025-10-23T10:33:34","subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo and Niklas,\n\nJust wanted to provide some feedback on how we overcome this\nawkwardness in the RPi IPA.\n\nOn Wed, 22 Oct 2025 at 10:30, Jacopo Mondi\n wrote:\n>\n> Hi Niklas,\n>\n> On Wed, Oct 22, 2025 at 10:16:48AM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2025-10-22 09:22:58 +0200, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Tue, Oct 21, 2025 at 06:50:40PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > On 2025-10-21 16:01:03 +0200, Jacopo Mondi wrote:\n> > > > > Hi Niklas,\n> > > > >\n> > > > > On Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:\n> > > > > > Hi Jacopo,\n> > > > > >\n> > > > > > Thanks for your work.\n> > > > > >\n> > > > > > On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:\n> > > > > > > The limits (min and max values) for the Camera controls are\n> > > > > > > registered at Camera configuration time and never updated.\n> > > > > > > Some controls, like FrameDurationLimits have a direct impact on\n> > > > > > > the limits of other controls, such as the ExposureTime and as they\n> > > > > > > can be configured by the application, this has to be taken into account.\n> > > > > > >\n> > > > > > > Currently, when a user changes the frame duration, the limits for\n> > > > > > > both the ExposureTime and FrameDurationControls are not updated.\n> > > > > > >\n> > > > > > > The timing at which the controls limits should be updated is also\n> > > > > > > critical: the new control values take effect once the Request they\n> > > > > > > belong to is completed.\n> > > > > > >\n> > > > > > > To support this operation model introduce a new IPA function\n> > > > > > > 'updateControlsLimits()' which the pipeline handler calls before\n> > > > > > > completing a Request.\n> > > > > > >\n> > > > > > > Store the exposure time limits in the FrameContext (frame\n> > > > > > > duration limits were already there) and update the Camera::controls()\n> > > > > > > control info map with the limits as computed for the Request that has\n> > > > > > > just completed.\n> > > > > > >\n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > include/libcamera/ipa/rkisp1.mojom | 2 ++\n> > > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 3 +++\n> > > > > > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > > > > > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++++++++++++++++\n> > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +++\n> > > > > > > 5 files changed, 37 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644\n> > > > > > > --- a/include/libcamera/ipa/rkisp1.mojom\n> > > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {\n> > > > > > > map streamConfig)\n> > > > > > > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > > > > >\n> > > > > > > + updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > > > > > > +\n> > > > > > > mapBuffers(array buffers);\n> > > > > > > unmapBuffers(array ids);\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644\n> > > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > > * frameContext.agc.exposure;\n> > > > > > > maxExposureTime = minExposureTime;\n> > > > > > > }\n> > > > > > > + frameContext.agc.minExposureTime = minExposureTime;\n> > > > > > > + frameContext.agc.maxExposureTime = maxExposureTime;\n> > > > > > >\n> > > > > > > if (frameContext.agc.autoGainEnabled) {\n> > > > > > > minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > > > > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > > * applied to the sensor when the statistics were collected.\n> > > > > > > */\n> > > > > > > utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;\n> > > > > > > + frameContext.agc.exposureTime = exposureTime;\n> > > > > > > double analogueGain = frameContext.sensor.gain;\n> > > > > > > utils::Duration effectiveExposureValue = exposureTime * analogueGain;\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644\n> > > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > @@ -143,6 +143,9 @@ struct IPAActiveState {\n> > > > > > >\n> > > > > > > struct IPAFrameContext : public FrameContext {\n> > > > > > > struct {\n> > > > > > > + utils::Duration minExposureTime;\n> > > > > > > + utils::Duration maxExposureTime;\n> > > > > > > + utils::Duration exposureTime;\n> > > > > > > uint32_t exposure;\n> > > > > > > double gain;\n> > > > > > > double exposureValue;\n> > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644\n> > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > @@ -62,6 +62,8 @@ public:\n> > > > > > > int configure(const IPAConfigInfo &ipaConfig,\n> > > > > > > const std::map &streamConfig,\n> > > > > > > ControlInfoMap *ipaControls) override;\n> > > > > > > + int updateControlsLimits(const uint32_t frame,\n> > > > > > > + ControlInfoMap *ipaControls) override;\n> > > > > > > void mapBuffers(const std::vector &buffers) override;\n> > > > > > > void unmapBuffers(const std::vector &ids) override;\n> > > > > > >\n> > > > > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)\n> > > > > > > +{\n> > > > > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > > > +\n> > > > > > > + /*\n> > > > > > > + * Update the exposure time and frame duration limits with the\n> > > > > > > + * settings computed by the AGC for the frame at hand.\n> > > > > > > + */\n> > > > > > > + assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());\n> > > > > > > + assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());\n> > > > > > > +\n> > > > > > > + ControlValue eMin(static_cast(frameContext.agc.minExposureTime.get()));\n> > > > > > > + ControlValue eMax(static_cast(frameContext.agc.maxExposureTime.get()));\n> > > > > > > + ControlValue eDef(static_cast(frameContext.agc.exposureTime.get()));\n> > > > > > > + ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);\n> > > > > > > +\n> > > > > > > + ControlValue fMin(static_cast(frameContext.agc.minFrameDuration.get()));\n> > > > > > > + ControlValue fMax(static_cast(frameContext.agc.maxFrameDuration.get()));\n> > > > > > > + ControlValue fDef(static_cast(frameContext.agc.frameDuration.get()));\n> > > > > > > + ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);\n> > > > > >\n> > > > > > Is this function not doing the reveres of what is expected? It takes the\n> > > > > > controls from the sensor and update them with new limits using values\n> > > > > > calculated by the IPA. Should it not take the values updated by the\n> > > > >\n> > > > > Where does it take controls from the sensor ?\n> > > > >\n> > > > > > sensor kernel driver and propagate them to the values used by the IPA?\n> > > > >\n> > > > > As I see it, it takes the control values the IPA has calculated for\n> > > > > the sensor for frame/request X [*] and when request X completes it\n> > > > > propagates those values to Camera::controls().\n> > > > >\n> > > > > [*] We have an outstanding duality between request number and frame\n> > > > > number. Hopefully we should sooner or later move the whole PH/IPA\n> > > > > interface to be designed around frame numbers and not request ids.\n> > > > >\n> > > > > >\n> > > > > > In IPARkISP1::configure() we read the min and max exposure time from the\n> > > > > > V4L2 sensor control, here we override them with new values we calculated\n> > > > > > ourself.\n> > > > > >\n> > > > > > If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK\n> > > > > > control new max and default values for V4L2_CID_EXPOSURE are calculated\n> > > > > > and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate\n> > > > > > these values instead of calculating it's own set?\n> > > > > >\n> > > > >\n> > > > > So, you know very well that the model in which v4l2 controls interact\n> > > > > between each other is ... not optimal. To make things worse controls\n> > > > > for a frame are applied at different times to compensate for their\n> > > > > delays. At what time would you read controls back from the sensor ?\n> > > > >\n> > > > > We have DelayedControls::get() which works with 'sequence' numbers and\n> > > > > our IPA reasons in terms of request numbers and I didn't want to\n> > > > > design something around DelayedControls as some pipelines might not\n> > > > > use them.\n> > > > >\n> > > > >\n> > > > > > If not can't we make configure() simpler by removing the ABI sharing of\n> > > > > > this dynamic control range? And in the longer run just drop\n> > > > > > V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?\n> > > > > > That way we would have more static only controls to care about :-)\n> > > > > >\n> > > > > > I think it's dangerous to do both, init the IPA with the values from the\n> > > > > > kernel and then calculate our own later.\n> > > > >\n> > > > > Those two things happen at very different times. configure() happens\n> > > > > while no frames are captured and the driver state is a snapshot of the\n> > > > > current situation. updateControlsLimits() happens at runtime and the\n> > > > > delays (both the controls delays and the frame latency) should be\n> > > > > taken into account.\n> > > > >\n> > > > > However I understand your concern that \"our\" calculations could\n> > > > > diverge from the \"driver\" calculations, but to be honest the only way\n> > > > > this can happen is with a broken driver (or a broken IPA).\n> > > > >\n> > > > > Anyway, I'll drop this patch. Camera::controls() are already b0rken,\n> > > > > updating them would open space for bugs by invalidating references,\n> > > > > and if it's not been an issue so far ... let's not be too concerned\n> > > > > with them\n> > > > >\n> > > > > I feel more strongly about 1/3 as that actually fixes a bug.\n> > > > >\n> > > > > >\n> > > > > > Same comment for 1/3 where the new max exposure time is calculated.\n> > > > >\n> > > > > I don't agree here.\n> > > > >\n> > > > > In 1/3 the max exposure time is calculated at the time where a new\n> > > > > vblank is computed by the IPA. From that time on, all the IPA\n> > > > > calculations should use the new limits as all the IPA calculations\n> > > > > will produce control values to be applied to frames generated -after-\n> > > > > the just calculated vblank is applied to the sensor.\n> > > > >\n> > > > > The actual VBLANK will be updated on the sensor with a latency of a\n> > > > > variable number of frames and we can't rely on the sensor calculated\n> > > > > values to update the IPA internal limits. Plus, it's an IOCTL more for\n> > > > > every frame.\n> > > > >\n> > > > > I understand Stefan's argument about the exposure margin, and that\n> > > > > should be taken into account, but I would be suprised if poking at the\n> > > > > sensor to update an internal IPA tuning paramter would be desirable,\n> > > > > considering the potential latency and costs.\n> > > > >\n> > > > > What do you think ?\n> > > >\n> > > > I agree, the V4L2 control interface is not always optimal, and likely\n> > > > that is at the core of my concern.\n> > > >\n> > > > The fact that we calculate the same thing (exposure time) both in the\n> > > > kernel driver and in the de facto standard user-space consumer of V4L2,\n> > > > without correlating the two hints at a larger issue.\n> > > >\n> > > > And to make issues even more \"fun\" the complete documentation for the\n> > > > control we calculate the min/max/def values for independently on is:\n> > > >\n> > > > V4L2_CID_EXPOSURE (integer)\n> > > >\n> > > > Exposure (cameras). [Unit?]\n> > > >\n> > > > This is what I hinted at (poorly) in my original mail of this being\n> > > > dangerous. I think it's not impossible for the kernel and libcamera to\n> > > > disagree about that the control represent. At least by always fetching\n> > >\n> > > We know the control's units are not specified. I think we tried to\n> > > change upstream to specify the control unit is in lines, but afair this\n> > > wasn't appreciated as it would make some existing drivers not-compliant.\n> > >\n> > > That's why we have Documentation/sensor_driver_requirements.rst\n> > > where we say:\n> > >\n> > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera\n> > > requires it to be expressed as a number of image lines. Camera sensor drivers\n> > > that do not comply with this requirement will need to be adapted or will produce\n> > > incorrect results.\n> >\n> > Thanks I was not aware of this narrowing of the control by libcamera. I\n> > think it's bonkers that it's needed and that upstream can't agree on a\n> > definition of a control, but that is another topic. For this topic I'm\n> > happy libcamera states what the control is.\n> >\n> > >\n> > > > the min/max/def values from the control we know the boundaries. But as\n> > > > you hint at in your reply, and I on IRC, this would cost an IOCTL per\n> > > > control everything something changes. Plus the plumbing to get it from\n> > > > pipeline handler to IPA.\n> > > >\n> > > > And I think we should either\n> > > >\n> > > > - Fix or update the V4L2 control. This could be as \"simple\" as\n> > > > updating the documentation for V4L2_CID_EXPOSURE to define what it\n> > > > really is, it's unit and that its min and max values should not\n> > > > change while streaming. Instead it shall be set to the min/max\n> > > > values to correspond to the min/max values of the VBLANK (or any\n> > > > other control effecting this sensors min/max value). But also\n> > > > describe how they are limited by other controls and that it's up to\n> > > > user-space to deal with figuring it out.\n> > >\n> > > I think we have a different interpretation of the issues around\n> > > EXPOSURE. By points:\n> > >\n> > > - its unit: you're right, but it didn't fly upstream, so\n> > > sensor_driver_requirements.rst\n> > > - its min/max: min is a sensor-specific parameter and should stay\n> > > constant for a mode. max is inhertely limited by the current frame\n> > > length (visible + blankings) and I presume that's a given, isn't it?\n> > > - min/max can't change while streaming: I agree with min, but isn't max\n> > > changing according to current blanking the whole reason for this\n> > > series ?\n> >\n> > I think we both see the issue but assign different importance too it. My\n> > concern is that we at IPA configure time take the effort to consult the\n> > min/max values from the V4L2 control, but while streaming we adjust the\n> > max value by our own calculations instead of consuming the updated value\n> > calculated and provided by the V4L2 API.\n> >\n> > This demonstrates how cumbersome the API is and I think libcamera as the\n> > de-facto user-space consumer of V4L2 API should either use the\n> > cumbersome API, or fix it. Working around it will IMHO just create more\n> > fragmentation and issues down the road.\n> >\n> > At the very least I think if the min/max values of the V4L2 API is to be\n> > ignored for the exposure control our own method to calculate it shall be\n> > used at IPA configure time too. Mixing the two is very confusing for\n> > somebody reading the code.\n> >\n>\n> I'm not sure I 100% share the concern here. configure() time happens\n> once per streaming session and I think we should have a snapshot of\n> the device state and reading control values seems the sane thing to do\n>\n> > >\n> > > >\n> > > > Then we can fix drivers like the IMX219 to comply with this and then\n> > > > we get the correct min/max values at IPA configure time. We know the\n> > >\n> > > Why do you think imx219 is broken in regards to handling EXPOSURE ?\n> > >\n> > > Afaiu the driver does what's intended by adjusting the max exposure to the\n> > > current frame lenght. It's the IPA that doesn't take this into\n> > > account.\n> >\n> > I don't think it's broken. I think the dynamic update of controls\n> > min/max (any control) while streaming is an edge case of the V4L2 not\n> > very well designed. The comment above about fixing the IMX219 I tried to\n> > hint at that if we documented this better we could update the drier to\n> > not update min/max while streaming, or make it clear that user-space\n>\n> I don't see it as an edge case, changing blankings to control frame\n> rate is \"the right thing to do\"(tm)\n>\n> > must be prepared to read new min/max values while streaming.\n> >\n> > >\n> > >\n> > > > unit they are in. We know the rules and we know that user-space is\n> > > > responsible to calculate a set of controls that work together. The\n> > > > kernel will only validate and ether accept and reject them.\n> > > >\n> > > > or\n> > > >\n> > > > - Allow controls to dynamic change their min/max values while\n> > >\n> > > I might be missing something. Isn't that how it works already ? By\n> > > changing VBLANK you control the frame rate and, as a consequence, the\n> > > maximum exposure value changes as well. What did I miss ?\n> >\n> > It is. But as this libcamera change don't consume it and instead\n> > calculate the max by itself hints that this might not be the best\n> > design?\n> >\n> > >\n> > > > streaming. Have libcamera fetch new limit every time it changes a\n> > > > control. This will be a lot of IOCTL callas which will cause pain\n> > > > and hopefully spur someone to create a better control interface ;-)\n> > > >\n> > > > I would not block patch 1/3 as it makes things better for sensors like\n> > > > IMX219 which is used and implement the V4L2_CID_EXPOSURE with dynamic\n> > > > values but with a unit that is the same as libcamera uses internally.\n> > > > But I do think 1/3 without also thinking about how to solve the API\n> > > > issue with the kernel will create more issues down the line.\n> > > >\n> > >\n> > > The real solution here would be to drop V/HBLANK and introduce a\n> > > control for the whole frame/line lengths. In this way you\n> > > automatically know what the exposure time maximum is without having to\n> > > take into account the visible sizes + blankings. It's quite a yak to\n> > > shave though and we do have anyway all the sensor drivers using\n> > > VBLANK/HBLANK and they can't be changed as otherwise existing\n> > > userspace that relies on those controls presence would break.\n> > >\n> > > Feels like I'm missing something somewhere ?\n> >\n> > Or maybe I am :-)\n> >\n> > But I think we both see the issue, the V4L2 controls in this area are\n> > not optimal designed. And there are edge cases of them that make it even\n> > worse to work with. Then we assign different interpretations on what to\n> > do about it.\n> >\n> > My understanding of your argument is that you are OK with calculating\n> > the min/max values both in kernel and user-space to avoid having to do\n> > extra IOCTLs to fetch the new values. This works for exposure and may\n> > very well be the best way forward.\n>\n> I'm ok starting with a known state read from the subdevice and\n> I'm ok updating an IPA runtime parameter based on the IPA calculated\n> frame length. This will be also updated on the device, but at a later\n> time and we can't rely on that.\n>\n> It's not just the extra ioctl, it's that controls are applied to the\n> sensor with frames of latencies while the IPA need to keep its limits\n> up-to-date while calculating parameters for the next frames.\n>\n> >\n> > My argument and worry is that it's dangerous to create this split. We\n> > have an API and we should use it, and if possible improve it. If not I\n> > fear copy-paste programming will spread this behavior and we will have\n> > IPA with hard coded calculations for a subset of sensors as it's OK to\n> > ignore the API which describes the controls. My strong feelings around\n> > this is that libcamera tries to be the mesa of cameras so it should not\n> > ignore API interfaces IMHO. More painful, and maybe not the fastest/best\n> > way forward.\n> >\n> > I will not try to block this work based on the above as fixing the API\n> > is likely a huge task. But I do think if you want to calculate the max\n> > independently for the kernel you should use the same calculation\n> > function at IPA configure time and not use the V4L2 min/max values\n> > there.\n> >\n>\n> I'll ask others what do they think as the different configure/runtime\n> behaviours are not a concern to me and I might be underestimating it.\n\nFrom pretty much the start, our IPA has done exactly what (I think!)\nJacopo is suggesting:\n\n- We calculate the \"max\" shutter values based on the maximum\nV4L2_CID_VBLANK value advertised, but also clipped to the limits of\nthe FrameDurationLimits control set by the user. This calculation is\ndynamic since the user can set FrameDurationLimits at runtime. We\ndon't rely on the max value of the V4L2_CID_EXPOSURE control at all.\n\n- The camere helper has a notion of the maximum exposure allowed based\non the frame length, typically sensors call this \"frame integration\ndifference\", which is used in the above calculation.\n\n- We must ensure V4L2_CID_VBLANK is set before V4L2_CID_EXPOSURE,\nhence the notion of \"priorityWrite\" in the DelayedControls class.\nWithout this, V4L2 will possibly reject/clip the V4L2_CID_EXPOSURE\ncontrol unnecessarily if the existing cached V4L2_CID_VBLANK control\nclips the range.\n\nWith this we achieve what I think is needed here in a robust way. I\ndo agree some of what we do here is working around some of the API\ndeficiencies in the V4L2 control API, but it seems manageable enough.\n\nHope that helps!\n\nNaush\n\n\n>\n> > >\n> > > > > >\n> > > > > > > +\n> > > > > > > + return 0;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > > > > > > {\n> > > > > > > for (const IPABuffer &buffer : buffers) {\n> > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644\n> > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > > > > > > if (!isRaw_ && !info->paramDequeued)\n> > > > > > > return;\n> > > > > > >\n> > > > > > > + /* Update controls before completing the request */\n> > > > > > > + data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);\n> > > > > > > +\n> > > > > > > data->frameInfo_.destroy(info->frame);\n> > > > > > >\n> > > > > > > completeRequest(request);\n> > > > > > >\n> > > > > > > --\n> > > > > > > 2.51.0\n> > > > > > >\n> > > > > >\n> > > > > > --\n> > > > > > Kind Regards,\n> > > > > > Niklas Söderlund\n> > > >\n> > > > --\n> > > > Kind Regards,\n> > > > Niklas Söderlund\n> >\n> > --\n> > Kind Regards,\n> > Niklas Söderlund","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 69D4ABE080\n\tfor ;\n\tThu, 23 Oct 2025 10:34:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6B04607C1;\n\tThu, 23 Oct 2025 12:34:14 +0200 (CEST)","from mail-ua1-x92f.google.com (mail-ua1-x92f.google.com\n\t[IPv6:2607:f8b0:4864:20::92f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA32460768\n\tfor ;\n\tThu, 23 Oct 2025 12:34:12 +0200 (CEST)","by mail-ua1-x92f.google.com with SMTP id\n\ta1e0cc1a2514c-932d99337c5so15444241.2\n\tfor ;\n\tThu, 23 Oct 2025 03:34:12 -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=\"hdWwrz1j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1761215651; x=1761820451;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=l40cF205oCd+y7b9QsYk1EUrQG8++IxTTbijbZAyOpA=;\n\tb=hdWwrz1jtzdC9W1ODVbhpIFWpnhibJLyG2UT2FkbAlswQjQCBMKxjFlBbUOzUF4s++\n\tHIn5vf5sZENIN7ndXNOaNP9GrQIeL0NN4tAsb4JGbioyRJBad7ujLk7+MWMvy17Oi6Oa\n\tRW+7ZDFjMq4WCKDec6zqDZlBT+lgI/vGgvjrcLPpmupQOQC1dwuulsdWFqz+WR0VZymf\n\t4Yhqr2NkdrR6LfE7/nEDnOkuIvQOxz3+SsQY5fFF2P7+RddmeS+czYDKItgub1K5X4zg\n\tbkHSwrAa6dxTcgU39ItYyjfdODbJGAHlGRoVtdMBJgVh3TBz7W0wJsSpNzAoEXv6qeRg\n\tgZMg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1761215651; x=1761820451;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=l40cF205oCd+y7b9QsYk1EUrQG8++IxTTbijbZAyOpA=;\n\tb=Xr5NV3HdKPYMpFbZlAFHrN8nRLd9QIGWH4gEnrhZiUbz8ClBauIPlk+D8DvBtFqWYb\n\tjyAHJqAij74WaxsJu7TYh1G8IClgODi2ZiovnQK9B9iHeUz6IPVM6Ik36YuxmibM7J0t\n\tTxZVUQMPM8eROGCr+I/kpQsbFOR089YjPcW+xr0vPyB1tncjWJmC4G2Rss68DdS2oN8w\n\tcm2oDEWftZkbuMYHP0ccXpKOhJgZWM7Po6+ngUwZoCHvrG4ttJZFRDxtC0cVh44M/33L\n\tIk+FiAuqXdwOna59ICR5fOysBVLMwgC6wNN78eKwxoRHARgPzoU8+j9faouOJgDkfCIk\n\thlNg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUzZoTcvrAYrsaD+f9rb4xNXH8oII+du7cK2gPFwZoZza3GjxApEA+VIzHyDDAGl+FtnrJUgFsOIo5O5BKUAu4=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxtfE+Ij4TMWutsvtgW8nbl2tDZEExM1ihmsqRVkIuXZySyDCpP\n\tLgaNjQgwtVEQqX7ivoatNVSXVHs4SUl/1gx2oGtluoPMzFDphbs33BQSTjVG3kUc2Gkxfsz5wkQ\n\tTNlgq1l39tndM2QyNJEYbaPJ5ofD3doV8j+qOoKz+Qw==","X-Gm-Gg":"ASbGnctIameDBwLiNNRItX4oMg9AM8d266cQbRXbReoOyEOqz9pybfNsxYBAK31Swp/\n\tNULgCLT/2Dx7rJSTOicpChGCUjG5vNf3QEAo7y/u+pFUWWOMqKZcQchAmYWyWvmU3mENliTjhyv\n\te4PWRwNwMMEnrLQq/ucJp3gFN6h2ip5x5wmdsdo7//Ie13LRWTvvOnT7ywTj6TkhTm7a5rA5nIJ\n\tA8+B+zQv5FIYmj/ZQ9SBdrYLEaIZcbFSoPQEQzofcPp3LZzFEO3WdWXld4hQVz4Wrw+cJhsTaIj\n\tNW/VxznFi/CbGO9WJOfSWWdPu7blTL0tEJg3ag==","X-Google-Smtp-Source":"AGHT+IFwY6spoOHuTtYwChgGhjSjShFL8fFZNymVHq5ZYzB6SHVVsorALYdAJNG0bC2TWMr/grHAR5w3FNIj/jRZYdc=","X-Received":"by 2002:a05:6102:e0a:b0:525:53c5:e42c with SMTP id\n\tada2fe7eead31-5db0e0f9356mr1539251137.3.1761215651171;\n\tThu, 23 Oct 2025 03:34:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20251017-exposure-limits-v1-0-6288cd86e719@ideasonboard.com>\n\t<20251017-exposure-limits-v1-2-6288cd86e719@ideasonboard.com>\n\t<20251021113205.GA1694476@ragnatech.se>\n\t\n\t<20251021165040.GB1694476@ragnatech.se>\n\t<7t2nuxkavqlvbuljitjvd6lmcz7x5slbh5jlafx2b3vpat526l@gviqyotqqxus>\n\t<20251022081648.GC1694476@ragnatech.se>\n\t<2k2zueuunu6dj3xya5r4kefgnhzxiblr3lfkfvur6xij22pi42@lyyhscjb2377>","In-Reply-To":"<2k2zueuunu6dj3xya5r4kefgnhzxiblr3lfkfvur6xij22pi42@lyyhscjb2377>","From":"Naushir Patuck ","Date":"Thu, 23 Oct 2025 11:33:34 +0100","X-Gm-Features":"AWmQ_bnHpPV105SO2TDkFWc444UlhkX__c41pqIm7gxiZzCnYWxB1alZRTlkVYM","Message-ID":"","Subject":"Re: [PATCH 2/3] libcamera: rkisp1: Update camera::controls() on\n\tlimit changes","To":"Jacopo Mondi ","Cc":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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\" "}}]