[2/3] libcamera: rkisp1: Update camera::controls() on limit changes

24695 diff mbox series

The limits (min and max values) for the Camera controls are
registered at Camera configuration time and never updated.
Some controls, like FrameDurationLimits have a direct impact on
the limits of other controls, such as the ExposureTime and as they
can be configured by the application, this has to be taken into account.

Currently, when a user changes the frame duration, the limits for
both the ExposureTime and FrameDurationControls are not updated.

The timing at which the controls limits should be updated is also
critical: the new control values take effect once the Request they
belong to is completed.

To support this operation model introduce a new IPA function
'updateControlsLimits()' which the pipeline handler calls before
completing a Request.

Store the exposure time limits in the FrameContext (frame
duration limits were already there) and update the Camera::controls()
control info map with the limits as computed for the Request that has
just completed.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/ipa/rkisp1.mojom       |  2 ++
 src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
 src/ipa/rkisp1/ipa_context.h             |  3 +++
 src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
 5 files changed, 37 insertions(+)

Hi Jacopo,

Quoting Jacopo Mondi (2025-10-17 11:00:06)
> The limits (min and max values) for the Camera controls are
> registered at Camera configuration time and never updated.
> Some controls, like FrameDurationLimits have a direct impact on
> the limits of other controls, such as the ExposureTime and as they
> can be configured by the application, this has to be taken into account.
> 
> Currently, when a user changes the frame duration, the limits for
> both the ExposureTime and FrameDurationControls are not updated.
> 
> The timing at which the controls limits should be updated is also
> critical: the new control values take effect once the Request they
> belong to is completed.
> 
> To support this operation model introduce a new IPA function
> 'updateControlsLimits()' which the pipeline handler calls before
> completing a Request.
> 
> Store the exposure time limits in the FrameContext (frame
> duration limits were already there) and update the Camera::controls()
> control info map with the limits as computed for the Request that has
> just completed.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/ipa/rkisp1.mojom       |  2 ++
>  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
>  src/ipa/rkisp1/ipa_context.h             |  3 +++
>  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
>  5 files changed, 37 insertions(+)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
>                   map<uint32, libcamera.IPAStream> streamConfig)
>                 => (int32 ret, libcamera.ControlInfoMap ipaControls);
>  
> +       updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> +
>         mapBuffers(array<libcamera.IPABuffer> buffers);
>         unmapBuffers(array<uint32> ids);
>  
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>                                 * frameContext.agc.exposure;
>                 maxExposureTime = minExposureTime;
>         }
> +       frameContext.agc.minExposureTime = minExposureTime;
> +       frameContext.agc.maxExposureTime = maxExposureTime;
>  
>         if (frameContext.agc.autoGainEnabled) {
>                 minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>          * applied to the sensor when the statistics were collected.
>          */
>         utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> +       frameContext.agc.exposureTime = exposureTime;
>         double analogueGain = frameContext.sensor.gain;
>         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>  
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -143,6 +143,9 @@ struct IPAActiveState {
>  
>  struct IPAFrameContext : public FrameContext {
>         struct {
> +               utils::Duration minExposureTime;
> +               utils::Duration maxExposureTime;
> +               utils::Duration exposureTime;
>                 uint32_t exposure;
>                 double gain;
>                 double exposureValue;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -62,6 +62,8 @@ public:
>         int configure(const IPAConfigInfo &ipaConfig,
>                       const std::map<uint32_t, IPAStream> &streamConfig,
>                       ControlInfoMap *ipaControls) override;
> +       int updateControlsLimits(const uint32_t frame,
> +                                ControlInfoMap *ipaControls) override;
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  
> @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>         return 0;
>  }
>  
> +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> +{
> +       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> +
> +       /*
> +        * Update the exposure time and frame duration limits with the
> +        * settings computed by the AGC for the frame at hand.
> +        */
> +       assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> +       assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> +
> +       ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> +       ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> +       ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> +       ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> +
> +       ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> +       ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> +       ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> +       ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);

Do we really need to update the frame duration limits? Aren't these
actually static by vblank min/max + output_size?

> +
> +       return 0;
> +}
> +
>  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>  {
>         for (const IPABuffer &buffer : buffers) {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
>         if (!isRaw_ && !info->paramDequeued)
>                 return;
>  
> +       /* Update controls before completing the request */
> +       data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);

Maybe I'm missing something here. How does this work in an isolated IPA?
To my understanding the ControlInfoMap would be deserialized on it's way
back from the ipa and call the assignment operator on
data->controlInfo_, breaking any user application that holds a iterator
on controlInfo_.

Could we assume that minFrameDuration doesn't change? Then we could
potentially only add the new maxFrameDuration to the setSensorControls
event and spare a new call into the ipa?

What do you think?

Best regards,
Stefan

> +
>         data->frameInfo_.destroy(info->frame);
>  
>         completeRequest(request);
> 
> -- 
> 2.51.0
>

Hi Stefan

On Mon, Oct 20, 2025 at 02:39:06PM +0200, Stefan Klug wrote:
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2025-10-17 11:00:06)
> > The limits (min and max values) for the Camera controls are
> > registered at Camera configuration time and never updated.
> > Some controls, like FrameDurationLimits have a direct impact on
> > the limits of other controls, such as the ExposureTime and as they
> > can be configured by the application, this has to be taken into account.
> >
> > Currently, when a user changes the frame duration, the limits for
> > both the ExposureTime and FrameDurationControls are not updated.
> >
> > The timing at which the controls limits should be updated is also
> > critical: the new control values take effect once the Request they
> > belong to is completed.
> >
> > To support this operation model introduce a new IPA function
> > 'updateControlsLimits()' which the pipeline handler calls before
> > completing a Request.
> >
> > Store the exposure time limits in the FrameContext (frame
> > duration limits were already there) and update the Camera::controls()
> > control info map with the limits as computed for the Request that has
> > just completed.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       |  2 ++
> >  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
> >  src/ipa/rkisp1/ipa_context.h             |  3 +++
> >  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
> >  5 files changed, 37 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
> >                   map<uint32, libcamera.IPAStream> streamConfig)
> >                 => (int32 ret, libcamera.ControlInfoMap ipaControls);
> >
> > +       updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > +
> >         mapBuffers(array<libcamera.IPABuffer> buffers);
> >         unmapBuffers(array<uint32> ids);
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >                                 * frameContext.agc.exposure;
> >                 maxExposureTime = minExposureTime;
> >         }
> > +       frameContext.agc.minExposureTime = minExposureTime;
> > +       frameContext.agc.maxExposureTime = maxExposureTime;
> >
> >         if (frameContext.agc.autoGainEnabled) {
> >                 minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >          * applied to the sensor when the statistics were collected.
> >          */
> >         utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> > +       frameContext.agc.exposureTime = exposureTime;
> >         double analogueGain = frameContext.sensor.gain;
> >         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> >
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -143,6 +143,9 @@ struct IPAActiveState {
> >
> >  struct IPAFrameContext : public FrameContext {
> >         struct {
> > +               utils::Duration minExposureTime;
> > +               utils::Duration maxExposureTime;
> > +               utils::Duration exposureTime;
> >                 uint32_t exposure;
> >                 double gain;
> >                 double exposureValue;
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -62,6 +62,8 @@ public:
> >         int configure(const IPAConfigInfo &ipaConfig,
> >                       const std::map<uint32_t, IPAStream> &streamConfig,
> >                       ControlInfoMap *ipaControls) override;
> > +       int updateControlsLimits(const uint32_t frame,
> > +                                ControlInfoMap *ipaControls) override;
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >
> > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> >         return 0;
> >  }
> >
> > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> > +{
> > +       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > +
> > +       /*
> > +        * Update the exposure time and frame duration limits with the
> > +        * settings computed by the AGC for the frame at hand.
> > +        */
> > +       assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> > +       assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> > +
> > +       ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> > +       ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> > +       ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> > +       ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> > +
> > +       ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> > +       ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> > +       ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> > +       ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);
>
> Do we really need to update the frame duration limits? Aren't these
> actually static by vblank min/max + output_size?
>

Shouldn't Camera::controls()[FrameDurationLimits] report what the user
has set as FrameDurationLimits ?

Although, if your application sets a FrameDurationLimit to [x, x] to
fix the frame rate, then your overall camera limits will be shown as
[x, x] which is pretty useless.

Should the Camera::controls[FrameDurationLimits] remain static for a
the whole capture session then ?

> > +
> > +       return 0;
> > +}
> > +
> >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> >  {
> >         for (const IPABuffer &buffer : buffers) {
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> >         if (!isRaw_ && !info->paramDequeued)
> >                 return;
> >
> > +       /* Update controls before completing the request */
> > +       data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
>
> Maybe I'm missing something here. How does this work in an isolated IPA?
> To my understanding the ControlInfoMap would be deserialized on it's way
> back from the ipa and call the assignment operator on
> data->controlInfo_, breaking any user application that holds a iterator
> on controlInfo_.

My thinking was that updating an entry in a map won't invalidate
iterators, which could be invalidated if the map is resized.

However, if I got you right, the ser/deser process will re-assign the
map. Is this your concern ?

There is however an underlying issue with concurrency. If the
application is accessing the ControlInfo exactly while the IPA udpates
it. Note that we have the same issue when updating ScalerCrop in
example as many pipelines do.

I don't have a solution for this at the moment. I'm afraid it will
require some smart serialization and going throuhg a C ABI.

>
> Could we assume that minFrameDuration doesn't change? Then we could

I think so, yes

> potentially only add the new maxFrameDuration to the setSensorControls
> event and spare a new call into the ipa?

I think the most relevant control to update is ExposureTime and not
FrameDurationLimits (see the above discussion).

Updating ExposureTime at setSensorControls() time doesn't however
happen at the "right" time. Actual controls are applied to the sensor
with a few frames of latency by DelayedControls. If we immediately
update the Camera::controls() limits, we would do that before the
actual Request that has triggered the update has completed.

And honestly I would like to avoid ad-hoc things where a single
control is updated, but maybe ExposureTime is actually the only thing
we need to care about ?

>
> What do you think?
>
> Best regards,
> Stefan
>
> > +
> >         data->frameInfo_.destroy(info->frame);
> >
> >         completeRequest(request);
> >
> > --
> > 2.51.0
> >

Hi Jacopo,

Quoting Jacopo Mondi (2025-10-21 09:05:01)
> Hi Stefan
> 
> On Mon, Oct 20, 2025 at 02:39:06PM +0200, Stefan Klug wrote:
> > Hi Jacopo,
> >
> > Quoting Jacopo Mondi (2025-10-17 11:00:06)
> > > The limits (min and max values) for the Camera controls are
> > > registered at Camera configuration time and never updated.
> > > Some controls, like FrameDurationLimits have a direct impact on
> > > the limits of other controls, such as the ExposureTime and as they
> > > can be configured by the application, this has to be taken into account.
> > >
> > > Currently, when a user changes the frame duration, the limits for
> > > both the ExposureTime and FrameDurationControls are not updated.
> > >
> > > The timing at which the controls limits should be updated is also
> > > critical: the new control values take effect once the Request they
> > > belong to is completed.
> > >
> > > To support this operation model introduce a new IPA function
> > > 'updateControlsLimits()' which the pipeline handler calls before
> > > completing a Request.
> > >
> > > Store the exposure time limits in the FrameContext (frame
> > > duration limits were already there) and update the Camera::controls()
> > > control info map with the limits as computed for the Request that has
> > > just completed.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  include/libcamera/ipa/rkisp1.mojom       |  2 ++
> > >  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
> > >  src/ipa/rkisp1/ipa_context.h             |  3 +++
> > >  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
> > >  5 files changed, 37 insertions(+)
> > >
> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
> > >                   map<uint32, libcamera.IPAStream> streamConfig)
> > >                 => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > >
> > > +       updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > +
> > >         mapBuffers(array<libcamera.IPABuffer> buffers);
> > >         unmapBuffers(array<uint32> ids);
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >                                 * frameContext.agc.exposure;
> > >                 maxExposureTime = minExposureTime;
> > >         }
> > > +       frameContext.agc.minExposureTime = minExposureTime;
> > > +       frameContext.agc.maxExposureTime = maxExposureTime;
> > >
> > >         if (frameContext.agc.autoGainEnabled) {
> > >                 minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >          * applied to the sensor when the statistics were collected.
> > >          */
> > >         utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> > > +       frameContext.agc.exposureTime = exposureTime;
> > >         double analogueGain = frameContext.sensor.gain;
> > >         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > >
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -143,6 +143,9 @@ struct IPAActiveState {
> > >
> > >  struct IPAFrameContext : public FrameContext {
> > >         struct {
> > > +               utils::Duration minExposureTime;
> > > +               utils::Duration maxExposureTime;
> > > +               utils::Duration exposureTime;
> > >                 uint32_t exposure;
> > >                 double gain;
> > >                 double exposureValue;
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -62,6 +62,8 @@ public:
> > >         int configure(const IPAConfigInfo &ipaConfig,
> > >                       const std::map<uint32_t, IPAStream> &streamConfig,
> > >                       ControlInfoMap *ipaControls) override;
> > > +       int updateControlsLimits(const uint32_t frame,
> > > +                                ControlInfoMap *ipaControls) override;
> > >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > >
> > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > >         return 0;
> > >  }
> > >
> > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> > > +{
> > > +       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > +
> > > +       /*
> > > +        * Update the exposure time and frame duration limits with the
> > > +        * settings computed by the AGC for the frame at hand.
> > > +        */
> > > +       assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> > > +       assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> > > +
> > > +       ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> > > +       ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> > > +       ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> > > +       ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> > > +
> > > +       ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> > > +       ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> > > +       ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> > > +       ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);
> >
> > Do we really need to update the frame duration limits? Aren't these
> > actually static by vblank min/max + output_size?
> >
> 
> Shouldn't Camera::controls()[FrameDurationLimits] report what the user
> has set as FrameDurationLimits ?

My understanding was that Camera::controls()[FrameDurationLimits]
reports the limits of the control and not the current state of the
camera. So min/max shouldn't change as these are roughly a sensor
property. (If we would change the max to the currently selected upper
limit and would check the limits, the user would never be able to raise
the upper limit again). Using the default value to represent the current
state would be possible, but is to my understanding not the current
design idea. My understanding was that the default is the camera
default/startup value, possibly modified once after configuration.

> 
> Although, if your application sets a FrameDurationLimit to [x, x] to
> fix the frame rate, then your overall camera limits will be shown as
> [x, x] which is pretty useless.
> 
> Should the Camera::controls[FrameDurationLimits] remain static for a
> the whole capture session then ?

Yes, I believe so.

> 
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > >  {
> > >         for (const IPABuffer &buffer : buffers) {
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> > >         if (!isRaw_ && !info->paramDequeued)
> > >                 return;
> > >
> > > +       /* Update controls before completing the request */
> > > +       data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
> >
> > Maybe I'm missing something here. How does this work in an isolated IPA?
> > To my understanding the ControlInfoMap would be deserialized on it's way
> > back from the ipa and call the assignment operator on
> > data->controlInfo_, breaking any user application that holds a iterator
> > on controlInfo_.
> 
> My thinking was that updating an entry in a map won't invalidate
> iterators, which could be invalidated if the map is resized.
> 
> However, if I got you right, the ser/deser process will re-assign the
> map. Is this your concern ?

Yes, exactly.

> 
> There is however an underlying issue with concurrency. If the
> application is accessing the ControlInfo exactly while the IPA udpates
> it. Note that we have the same issue when updating ScalerCrop in
> example as many pipelines do.
> 
> I don't have a solution for this at the moment. I'm afraid it will
> require some smart serialization and going throuhg a C ABI.

Yes, that is a pain. My strategy there was to keep controls static after
configure(). But I agree that we need to update the max exposure time.

> 
> >
> > Could we assume that minFrameDuration doesn't change? Then we could
> 
> I think so, yes
> 
> > potentially only add the new maxFrameDuration to the setSensorControls
> > event and spare a new call into the ipa?
> 
> I think the most relevant control to update is ExposureTime and not
> FrameDurationLimits (see the above discussion).

Oh yes, I picked the wrong variable. I meant max exposure time.

> 
> Updating ExposureTime at setSensorControls() time doesn't however
> happen at the "right" time. Actual controls are applied to the sensor
> with a few frames of latency by DelayedControls. If we immediately
> update the Camera::controls() limits, we would do that before the
> actual Request that has triggered the update has completed.
> 
> And honestly I would like to avoid ad-hoc things where a single
> control is updated, but maybe ExposureTime is actually the only thing
> we need to care about ?

I eagerly need to post my regulation rework series, as that also impacts
timing and reorders a few things. I hope I can do that this week...

Best regards,
Stefan

> 
> >
> > What do you think?
> >
> > Best regards,
> > Stefan
> >
> > > +
> > >         data->frameInfo_.destroy(info->frame);
> > >
> > >         completeRequest(request);
> > >
> > > --
> > > 2.51.0
> > >

Hi Jacopo,

Thanks for your work.

On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:
> The limits (min and max values) for the Camera controls are
> registered at Camera configuration time and never updated.
> Some controls, like FrameDurationLimits have a direct impact on
> the limits of other controls, such as the ExposureTime and as they
> can be configured by the application, this has to be taken into account.
> 
> Currently, when a user changes the frame duration, the limits for
> both the ExposureTime and FrameDurationControls are not updated.
> 
> The timing at which the controls limits should be updated is also
> critical: the new control values take effect once the Request they
> belong to is completed.
> 
> To support this operation model introduce a new IPA function
> 'updateControlsLimits()' which the pipeline handler calls before
> completing a Request.
> 
> Store the exposure time limits in the FrameContext (frame
> duration limits were already there) and update the Camera::controls()
> control info map with the limits as computed for the Request that has
> just completed.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/ipa/rkisp1.mojom       |  2 ++
>  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
>  src/ipa/rkisp1/ipa_context.h             |  3 +++
>  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
>  5 files changed, 37 insertions(+)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
>  		  map<uint32, libcamera.IPAStream> streamConfig)
>  		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
>  
> +	updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> +
>  	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
>  
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  				* frameContext.agc.exposure;
>  		maxExposureTime = minExposureTime;
>  	}
> +	frameContext.agc.minExposureTime = minExposureTime;
> +	frameContext.agc.maxExposureTime = maxExposureTime;
>  
>  	if (frameContext.agc.autoGainEnabled) {
>  		minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	 * applied to the sensor when the statistics were collected.
>  	 */
>  	utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> +	frameContext.agc.exposureTime = exposureTime;
>  	double analogueGain = frameContext.sensor.gain;
>  	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>  
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -143,6 +143,9 @@ struct IPAActiveState {
>  
>  struct IPAFrameContext : public FrameContext {
>  	struct {
> +		utils::Duration minExposureTime;
> +		utils::Duration maxExposureTime;
> +		utils::Duration exposureTime;
>  		uint32_t exposure;
>  		double gain;
>  		double exposureValue;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -62,6 +62,8 @@ public:
>  	int configure(const IPAConfigInfo &ipaConfig,
>  		      const std::map<uint32_t, IPAStream> &streamConfig,
>  		      ControlInfoMap *ipaControls) override;
> +	int updateControlsLimits(const uint32_t frame,
> +				 ControlInfoMap *ipaControls) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  
> @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>  	return 0;
>  }
>  
> +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> +{
> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> +
> +	/*
> +	 * Update the exposure time and frame duration limits with the
> +	 * settings computed by the AGC for the frame at hand.
> +	 */
> +	assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> +	assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> +
> +	ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> +	ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> +	ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> +	ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> +
> +	ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> +	ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> +	ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> +	ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);

Is this function not doing the reveres of what is expected? It takes the 
controls from the sensor and update them with new limits using values 
calculated by the IPA. Should it not take the values updated by the 
sensor kernel driver and propagate them to the values used by the IPA?

In IPARkISP1::configure() we read the min and max exposure time from the 
V4L2 sensor control, here we override them with new values we calculated 
ourself.

If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK 
control new max and default values for V4L2_CID_EXPOSURE are calculated 
and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate 
these values instead of calculating it's own set?

If not can't we make configure() simpler by removing the ABI sharing of 
this dynamic control range? And in the longer run just drop 
V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?  
That way we would have more static only controls to care about :-)

I think it's dangerous to do both, init the IPA with the values from the 
kernel and then calculate our own later.

Same comment for 1/3 where the new max exposure time is calculated.

> +
> +	return 0;
> +}
> +
>  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>  {
>  	for (const IPABuffer &buffer : buffers) {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
>  	if (!isRaw_ && !info->paramDequeued)
>  		return;
>  
> +	/* Update controls before completing the request */
> +	data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
> +
>  	data->frameInfo_.destroy(info->frame);
>  
>  	completeRequest(request);
> 
> -- 
> 2.51.0
>

Hi Niklas,

On Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:
> > The limits (min and max values) for the Camera controls are
> > registered at Camera configuration time and never updated.
> > Some controls, like FrameDurationLimits have a direct impact on
> > the limits of other controls, such as the ExposureTime and as they
> > can be configured by the application, this has to be taken into account.
> >
> > Currently, when a user changes the frame duration, the limits for
> > both the ExposureTime and FrameDurationControls are not updated.
> >
> > The timing at which the controls limits should be updated is also
> > critical: the new control values take effect once the Request they
> > belong to is completed.
> >
> > To support this operation model introduce a new IPA function
> > 'updateControlsLimits()' which the pipeline handler calls before
> > completing a Request.
> >
> > Store the exposure time limits in the FrameContext (frame
> > duration limits were already there) and update the Camera::controls()
> > control info map with the limits as computed for the Request that has
> > just completed.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       |  2 ++
> >  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
> >  src/ipa/rkisp1/ipa_context.h             |  3 +++
> >  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
> >  5 files changed, 37 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
> >  		  map<uint32, libcamera.IPAStream> streamConfig)
> >  		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
> >
> > +	updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > +
> >  	mapBuffers(array<libcamera.IPABuffer> buffers);
> >  	unmapBuffers(array<uint32> ids);
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  				* frameContext.agc.exposure;
> >  		maxExposureTime = minExposureTime;
> >  	}
> > +	frameContext.agc.minExposureTime = minExposureTime;
> > +	frameContext.agc.maxExposureTime = maxExposureTime;
> >
> >  	if (frameContext.agc.autoGainEnabled) {
> >  		minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  	 * applied to the sensor when the statistics were collected.
> >  	 */
> >  	utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> > +	frameContext.agc.exposureTime = exposureTime;
> >  	double analogueGain = frameContext.sensor.gain;
> >  	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> >
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -143,6 +143,9 @@ struct IPAActiveState {
> >
> >  struct IPAFrameContext : public FrameContext {
> >  	struct {
> > +		utils::Duration minExposureTime;
> > +		utils::Duration maxExposureTime;
> > +		utils::Duration exposureTime;
> >  		uint32_t exposure;
> >  		double gain;
> >  		double exposureValue;
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -62,6 +62,8 @@ public:
> >  	int configure(const IPAConfigInfo &ipaConfig,
> >  		      const std::map<uint32_t, IPAStream> &streamConfig,
> >  		      ControlInfoMap *ipaControls) override;
> > +	int updateControlsLimits(const uint32_t frame,
> > +				 ControlInfoMap *ipaControls) override;
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >
> > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> >  	return 0;
> >  }
> >
> > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> > +{
> > +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > +
> > +	/*
> > +	 * Update the exposure time and frame duration limits with the
> > +	 * settings computed by the AGC for the frame at hand.
> > +	 */
> > +	assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> > +	assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> > +
> > +	ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> > +	ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> > +	ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> > +	ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> > +
> > +	ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> > +	ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> > +	ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> > +	ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);
>
> Is this function not doing the reveres of what is expected? It takes the
> controls from the sensor and update them with new limits using values
> calculated by the IPA. Should it not take the values updated by the

Where does it take controls from the sensor ?

> sensor kernel driver and propagate them to the values used by the IPA?

As I see it, it takes the control values the IPA has calculated for
the sensor for frame/request X [*] and when request X completes it
propagates those values to Camera::controls().

[*] We have an outstanding duality between request number and frame
number. Hopefully we should sooner or later move the whole PH/IPA
interface to be designed around frame numbers and not request ids.

>
> In IPARkISP1::configure() we read the min and max exposure time from the
> V4L2 sensor control, here we override them with new values we calculated
> ourself.
>
> If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK
> control new max and default values for V4L2_CID_EXPOSURE are calculated
> and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate
> these values instead of calculating it's own set?
>

So, you know very well that the model in which v4l2 controls interact
between each other is ... not optimal. To make things worse controls
for a frame are applied at different times to compensate for their
delays. At what time would you read controls back from the sensor ?

We have DelayedControls::get() which works with 'sequence' numbers and
our IPA reasons in terms of request numbers and I didn't want to
design something around DelayedControls as some pipelines might not
use them.


> If not can't we make configure() simpler by removing the ABI sharing of
> this dynamic control range? And in the longer run just drop
> V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?
> That way we would have more static only controls to care about :-)
>
> I think it's dangerous to do both, init the IPA with the values from the
> kernel and then calculate our own later.

Those two things happen at very different times. configure() happens
while no frames are captured and the driver state is a snapshot of the
current situation. updateControlsLimits() happens at runtime and the
delays (both the controls delays and the frame latency) should be
taken into account.

However I understand your concern that "our" calculations could
diverge from the "driver" calculations, but to be honest the only way
this can happen is with a broken driver (or a broken IPA).

Anyway, I'll drop this patch. Camera::controls() are already b0rken,
updating them would open space for bugs by invalidating references,
and if it's not been an issue so far ... let's not be too concerned
with them

I feel more strongly about 1/3 as that actually fixes a bug.

>
> Same comment for 1/3 where the new max exposure time is calculated.

I don't agree here.

In 1/3 the max exposure time is calculated at the time where a new
vblank is computed by the IPA. From that time on, all the IPA
calculations should use the new limits as all the IPA calculations
will produce control values to be applied to frames generated -after-
the just calculated vblank is applied to the sensor.

The actual VBLANK will be updated on the sensor with a latency of a
variable number of frames and we can't rely on the sensor calculated
values to update the IPA internal limits. Plus, it's an IOCTL more for
every frame.

I understand Stefan's argument about the exposure margin, and that
should be taken into account, but I would be suprised if poking at the
sensor to update an internal IPA tuning paramter would be desirable,
considering the potential latency and costs.

What do you think ?
>
> > +
> > +	return 0;
> > +}
> > +
> >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> >  {
> >  	for (const IPABuffer &buffer : buffers) {
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> >  	if (!isRaw_ && !info->paramDequeued)
> >  		return;
> >
> > +	/* Update controls before completing the request */
> > +	data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
> > +
> >  	data->frameInfo_.destroy(info->frame);
> >
> >  	completeRequest(request);
> >
> > --
> > 2.51.0
> >
>
> --
> Kind Regards,
> Niklas Söderlund

Hi Stefan

On Tue, Oct 21, 2025 at 10:06:21AM +0200, Stefan Klug wrote:
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2025-10-21 09:05:01)
> > Hi Stefan
> >
> > On Mon, Oct 20, 2025 at 02:39:06PM +0200, Stefan Klug wrote:
> > > Hi Jacopo,
> > >
> > > Quoting Jacopo Mondi (2025-10-17 11:00:06)
> > > > The limits (min and max values) for the Camera controls are
> > > > registered at Camera configuration time and never updated.
> > > > Some controls, like FrameDurationLimits have a direct impact on
> > > > the limits of other controls, such as the ExposureTime and as they
> > > > can be configured by the application, this has to be taken into account.
> > > >
> > > > Currently, when a user changes the frame duration, the limits for
> > > > both the ExposureTime and FrameDurationControls are not updated.
> > > >
> > > > The timing at which the controls limits should be updated is also
> > > > critical: the new control values take effect once the Request they
> > > > belong to is completed.
> > > >
> > > > To support this operation model introduce a new IPA function
> > > > 'updateControlsLimits()' which the pipeline handler calls before
> > > > completing a Request.
> > > >
> > > > Store the exposure time limits in the FrameContext (frame
> > > > duration limits were already there) and update the Camera::controls()
> > > > control info map with the limits as computed for the Request that has
> > > > just completed.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > >  include/libcamera/ipa/rkisp1.mojom       |  2 ++
> > > >  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
> > > >  src/ipa/rkisp1/ipa_context.h             |  3 +++
> > > >  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
> > > >  5 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> > > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
> > > >                   map<uint32, libcamera.IPAStream> streamConfig)
> > > >                 => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > >
> > > > +       updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > > +
> > > >         mapBuffers(array<libcamera.IPABuffer> buffers);
> > > >         unmapBuffers(array<uint32> ids);
> > > >
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >                                 * frameContext.agc.exposure;
> > > >                 maxExposureTime = minExposureTime;
> > > >         }
> > > > +       frameContext.agc.minExposureTime = minExposureTime;
> > > > +       frameContext.agc.maxExposureTime = maxExposureTime;
> > > >
> > > >         if (frameContext.agc.autoGainEnabled) {
> > > >                 minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >          * applied to the sensor when the statistics were collected.
> > > >          */
> > > >         utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> > > > +       frameContext.agc.exposureTime = exposureTime;
> > > >         double analogueGain = frameContext.sensor.gain;
> > > >         utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > > >
> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > @@ -143,6 +143,9 @@ struct IPAActiveState {
> > > >
> > > >  struct IPAFrameContext : public FrameContext {
> > > >         struct {
> > > > +               utils::Duration minExposureTime;
> > > > +               utils::Duration maxExposureTime;
> > > > +               utils::Duration exposureTime;
> > > >                 uint32_t exposure;
> > > >                 double gain;
> > > >                 double exposureValue;
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -62,6 +62,8 @@ public:
> > > >         int configure(const IPAConfigInfo &ipaConfig,
> > > >                       const std::map<uint32_t, IPAStream> &streamConfig,
> > > >                       ControlInfoMap *ipaControls) override;
> > > > +       int updateControlsLimits(const uint32_t frame,
> > > > +                                ControlInfoMap *ipaControls) override;
> > > >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > >
> > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > >         return 0;
> > > >  }
> > > >
> > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> > > > +{
> > > > +       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > +
> > > > +       /*
> > > > +        * Update the exposure time and frame duration limits with the
> > > > +        * settings computed by the AGC for the frame at hand.
> > > > +        */
> > > > +       assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> > > > +       assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> > > > +
> > > > +       ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> > > > +       ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> > > > +       ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> > > > +       ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> > > > +
> > > > +       ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> > > > +       ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> > > > +       ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> > > > +       ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);
> > >
> > > Do we really need to update the frame duration limits? Aren't these
> > > actually static by vblank min/max + output_size?
> > >
> >
> > Shouldn't Camera::controls()[FrameDurationLimits] report what the user
> > has set as FrameDurationLimits ?
>
> My understanding was that Camera::controls()[FrameDurationLimits]
> reports the limits of the control and not the current state of the
> camera. So min/max shouldn't change as these are roughly a sensor
> property. (If we would change the max to the currently selected upper
> limit and would check the limits, the user would never be able to raise
> the upper limit again). Using the default value to represent the current
> state would be possible, but is to my understanding not the current
> design idea. My understanding was that the default is the camera
> default/startup value, possibly modified once after configuration.
>

Ack, makes sense. FrameDurationLimits should not be updated...

> >
> > Although, if your application sets a FrameDurationLimit to [x, x] to
> > fix the frame rate, then your overall camera limits will be shown as
> > [x, x] which is pretty useless.
> >
> > Should the Camera::controls[FrameDurationLimits] remain static for a
> > the whole capture session then ?
>
> Yes, I believe so.
>
> >
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > >  {
> > > >         for (const IPABuffer &buffer : buffers) {
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> > > >         if (!isRaw_ && !info->paramDequeued)
> > > >                 return;
> > > >
> > > > +       /* Update controls before completing the request */
> > > > +       data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
> > >
> > > Maybe I'm missing something here. How does this work in an isolated IPA?
> > > To my understanding the ControlInfoMap would be deserialized on it's way
> > > back from the ipa and call the assignment operator on
> > > data->controlInfo_, breaking any user application that holds a iterator
> > > on controlInfo_.
> >
> > My thinking was that updating an entry in a map won't invalidate
> > iterators, which could be invalidated if the map is resized.
> >
> > However, if I got you right, the ser/deser process will re-assign the
> > map. Is this your concern ?
>
> Yes, exactly.
>
> >
> > There is however an underlying issue with concurrency. If the
> > application is accessing the ControlInfo exactly while the IPA udpates
> > it. Note that we have the same issue when updating ScalerCrop in
> > example as many pipelines do.
> >
> > I don't have a solution for this at the moment. I'm afraid it will
> > require some smart serialization and going throuhg a C ABI.
>
> Yes, that is a pain. My strategy there was to keep controls static after
> configure(). But I agree that we need to update the max exposure time.
>
> >
> > >
> > > Could we assume that minFrameDuration doesn't change? Then we could
> >
> > I think so, yes
> >
> > > potentially only add the new maxFrameDuration to the setSensorControls
> > > event and spare a new call into the ipa?
> >
> > I think the most relevant control to update is ExposureTime and not
> > FrameDurationLimits (see the above discussion).
>
> Oh yes, I picked the wrong variable. I meant max exposure time.
>
> >
> > Updating ExposureTime at setSensorControls() time doesn't however
> > happen at the "right" time. Actual controls are applied to the sensor
> > with a few frames of latency by DelayedControls. If we immediately
> > update the Camera::controls() limits, we would do that before the
> > actual Request that has triggered the update has completed.
> >
> > And honestly I would like to avoid ad-hoc things where a single
> > control is updated, but maybe ExposureTime is actually the only thing
> > we need to care about ?
>
> I eagerly need to post my regulation rework series, as that also impacts
> timing and reorders a few things. I hope I can do that this week...

As said, I'll drop this patch. Things are already broken there and
let's not make them worse. What I mostly care about is 1/3.

Thanks
  j

>
> Best regards,
> Stefan
>
> >
> > >
> > > What do you think?
> > >
> > > Best regards,
> > > Stefan
> > >
> > > > +
> > > >         data->frameInfo_.destroy(info->frame);
> > > >
> > > >         completeRequest(request);
> > > >
> > > > --
> > > > 2.51.0
> > > >

Hi Niklas,

On Tue, Oct 21, 2025 at 06:50:40PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2025-10-21 16:01:03 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:
> > > > The limits (min and max values) for the Camera controls are
> > > > registered at Camera configuration time and never updated.
> > > > Some controls, like FrameDurationLimits have a direct impact on
> > > > the limits of other controls, such as the ExposureTime and as they
> > > > can be configured by the application, this has to be taken into account.
> > > >
> > > > Currently, when a user changes the frame duration, the limits for
> > > > both the ExposureTime and FrameDurationControls are not updated.
> > > >
> > > > The timing at which the controls limits should be updated is also
> > > > critical: the new control values take effect once the Request they
> > > > belong to is completed.
> > > >
> > > > To support this operation model introduce a new IPA function
> > > > 'updateControlsLimits()' which the pipeline handler calls before
> > > > completing a Request.
> > > >
> > > > Store the exposure time limits in the FrameContext (frame
> > > > duration limits were already there) and update the Camera::controls()
> > > > control info map with the limits as computed for the Request that has
> > > > just completed.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > >  include/libcamera/ipa/rkisp1.mojom       |  2 ++
> > > >  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
> > > >  src/ipa/rkisp1/ipa_context.h             |  3 +++
> > > >  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
> > > >  5 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> > > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
> > > >  		  map<uint32, libcamera.IPAStream> streamConfig)
> > > >  		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > >
> > > > +	updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > > +
> > > >  	mapBuffers(array<libcamera.IPABuffer> buffers);
> > > >  	unmapBuffers(array<uint32> ids);
> > > >
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >  				* frameContext.agc.exposure;
> > > >  		maxExposureTime = minExposureTime;
> > > >  	}
> > > > +	frameContext.agc.minExposureTime = minExposureTime;
> > > > +	frameContext.agc.maxExposureTime = maxExposureTime;
> > > >
> > > >  	if (frameContext.agc.autoGainEnabled) {
> > > >  		minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >  	 * applied to the sensor when the statistics were collected.
> > > >  	 */
> > > >  	utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> > > > +	frameContext.agc.exposureTime = exposureTime;
> > > >  	double analogueGain = frameContext.sensor.gain;
> > > >  	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > > >
> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > @@ -143,6 +143,9 @@ struct IPAActiveState {
> > > >
> > > >  struct IPAFrameContext : public FrameContext {
> > > >  	struct {
> > > > +		utils::Duration minExposureTime;
> > > > +		utils::Duration maxExposureTime;
> > > > +		utils::Duration exposureTime;
> > > >  		uint32_t exposure;
> > > >  		double gain;
> > > >  		double exposureValue;
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -62,6 +62,8 @@ public:
> > > >  	int configure(const IPAConfigInfo &ipaConfig,
> > > >  		      const std::map<uint32_t, IPAStream> &streamConfig,
> > > >  		      ControlInfoMap *ipaControls) override;
> > > > +	int updateControlsLimits(const uint32_t frame,
> > > > +				 ControlInfoMap *ipaControls) override;
> > > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > >
> > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> > > > +{
> > > > +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > +
> > > > +	/*
> > > > +	 * Update the exposure time and frame duration limits with the
> > > > +	 * settings computed by the AGC for the frame at hand.
> > > > +	 */
> > > > +	assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> > > > +	assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> > > > +
> > > > +	ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> > > > +	ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> > > > +	ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> > > > +	ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> > > > +
> > > > +	ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> > > > +	ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> > > > +	ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> > > > +	ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);
> > >
> > > Is this function not doing the reveres of what is expected? It takes the
> > > controls from the sensor and update them with new limits using values
> > > calculated by the IPA. Should it not take the values updated by the
> >
> > Where does it take controls from the sensor ?
> >
> > > sensor kernel driver and propagate them to the values used by the IPA?
> >
> > As I see it, it takes the control values the IPA has calculated for
> > the sensor for frame/request X [*] and when request X completes it
> > propagates those values to Camera::controls().
> >
> > [*] We have an outstanding duality between request number and frame
> > number. Hopefully we should sooner or later move the whole PH/IPA
> > interface to be designed around frame numbers and not request ids.
> >
> > >
> > > In IPARkISP1::configure() we read the min and max exposure time from the
> > > V4L2 sensor control, here we override them with new values we calculated
> > > ourself.
> > >
> > > If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK
> > > control new max and default values for V4L2_CID_EXPOSURE are calculated
> > > and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate
> > > these values instead of calculating it's own set?
> > >
> >
> > So, you know very well that the model in which v4l2 controls interact
> > between each other is ... not optimal. To make things worse controls
> > for a frame are applied at different times to compensate for their
> > delays. At what time would you read controls back from the sensor ?
> >
> > We have DelayedControls::get() which works with 'sequence' numbers and
> > our IPA reasons in terms of request numbers and I didn't want to
> > design something around DelayedControls as some pipelines might not
> > use them.
> >
> >
> > > If not can't we make configure() simpler by removing the ABI sharing of
> > > this dynamic control range? And in the longer run just drop
> > > V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?
> > > That way we would have more static only controls to care about :-)
> > >
> > > I think it's dangerous to do both, init the IPA with the values from the
> > > kernel and then calculate our own later.
> >
> > Those two things happen at very different times. configure() happens
> > while no frames are captured and the driver state is a snapshot of the
> > current situation. updateControlsLimits() happens at runtime and the
> > delays (both the controls delays and the frame latency) should be
> > taken into account.
> >
> > However I understand your concern that "our" calculations could
> > diverge from the "driver" calculations, but to be honest the only way
> > this can happen is with a broken driver (or a broken IPA).
> >
> > Anyway, I'll drop this patch. Camera::controls() are already b0rken,
> > updating them would open space for bugs by invalidating references,
> > and if it's not been an issue so far ... let's not be too concerned
> > with them
> >
> > I feel more strongly about 1/3 as that actually fixes a bug.
> >
> > >
> > > Same comment for 1/3 where the new max exposure time is calculated.
> >
> > I don't agree here.
> >
> > In 1/3 the max exposure time is calculated at the time where a new
> > vblank is computed by the IPA. From that time on, all the IPA
> > calculations should use the new limits as all the IPA calculations
> > will produce control values to be applied to frames generated -after-
> > the just calculated vblank is applied to the sensor.
> >
> > The actual VBLANK will be updated on the sensor with a latency of a
> > variable number of frames and we can't rely on the sensor calculated
> > values to update the IPA internal limits. Plus, it's an IOCTL more for
> > every frame.
> >
> > I understand Stefan's argument about the exposure margin, and that
> > should be taken into account, but I would be suprised if poking at the
> > sensor to update an internal IPA tuning paramter would be desirable,
> > considering the potential latency and costs.
> >
> > What do you think ?
>
> I agree, the V4L2 control interface is not always optimal, and likely
> that is at the core of my concern.
>
> The fact that we calculate the same thing (exposure time) both in the
> kernel driver and in the de facto standard user-space consumer of V4L2,
> without correlating the two hints at a larger issue.
>
> And to make issues even more "fun" the complete documentation for the
> control we calculate the min/max/def values for independently on is:
>
>     V4L2_CID_EXPOSURE (integer)
>
>         Exposure (cameras). [Unit?]
>
> This is what I hinted at (poorly) in my original mail of this being
> dangerous. I think it's not impossible for the kernel and libcamera to
> disagree about that the control represent. At least by always fetching

We know the control's units are not specified. I think we tried to
change upstream to specify the control unit is in lines, but afair this
wasn't appreciated as it would make some existing drivers not-compliant.

That's why we have Documentation/sensor_driver_requirements.rst
where we say:

While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera
requires it to be expressed as a number of image lines. Camera sensor drivers
that do not comply with this requirement will need to be adapted or will produce
incorrect results.

> the min/max/def values from the control we know the boundaries. But as
> you hint at in your reply, and I on IRC, this would cost an IOCTL per
> control everything something changes. Plus the plumbing to get it from
> pipeline handler to IPA.
>
> And I think we should either
>
>   - Fix or update the V4L2 control. This could be as "simple" as
>     updating the documentation for V4L2_CID_EXPOSURE to define what it
>     really is, it's unit and that its min and max values should not
>     change while streaming. Instead it shall be set to the min/max
>     values to correspond to the min/max values of the VBLANK (or any
>     other control effecting this sensors min/max value). But also
>     describe how they are limited by other controls and that it's up to
>     user-space to deal with figuring it out.

I think we have a different interpretation of the issues around
EXPOSURE. By points:

- its unit: you're right, but it didn't fly upstream, so
  sensor_driver_requirements.rst
- its min/max: min is a sensor-specific parameter and should stay
  constant for a mode. max is inhertely limited by the current frame
  length (visible + blankings) and I presume that's a given, isn't it?
- min/max can't change while streaming: I agree with min, but isn't max
  changing according to current blanking the whole reason for this
  series ?

>
>     Then we can fix drivers like the IMX219 to comply with this and then
>     we get the correct min/max values at IPA configure time. We know the

Why do you think imx219 is broken in regards to handling EXPOSURE ?

Afaiu the driver does what's intended by adjusting the max exposure to the
current frame lenght. It's the IPA that doesn't take this into
account.


>     unit they are in. We know the rules and we know that user-space is
>     responsible to calculate a set of controls that work together. The
>     kernel will only validate and ether accept and reject them.
>
> or
>
>   - Allow controls to dynamic change their min/max values while

I might be missing something. Isn't that how it works already ? By
changing VBLANK you control the frame rate and, as a consequence, the
maximum exposure value changes as well. What did I miss ?

>     streaming. Have libcamera fetch new limit every time it changes a
>     control. This will be a lot of IOCTL callas which will cause pain
>     and hopefully spur someone to create a better control interface ;-)
>
> I would not block patch 1/3 as it makes things better for sensors like
> IMX219 which is used and implement the V4L2_CID_EXPOSURE with dynamic
> values but with a unit that is the same as libcamera uses internally.
> But I do think 1/3 without also thinking about how to solve the API
> issue with the kernel will create more issues down the line.
>

The real solution here would be to drop V/HBLANK and introduce a
control for the whole frame/line lengths. In this way you
automatically know what the exposure time maximum is without having to
take into account the visible sizes + blankings. It's quite a yak to
shave though and we do have anyway all the sensor drivers using
VBLANK/HBLANK and they can't be changed as otherwise existing
userspace that relies on those controls presence would break.

Feels like I'm missing something somewhere ?

> > >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > >  {
> > > >  	for (const IPABuffer &buffer : buffers) {
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> > > >  	if (!isRaw_ && !info->paramDequeued)
> > > >  		return;
> > > >
> > > > +	/* Update controls before completing the request */
> > > > +	data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
> > > > +
> > > >  	data->frameInfo_.destroy(info->frame);
> > > >
> > > >  	completeRequest(request);
> > > >
> > > > --
> > > > 2.51.0
> > > >
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
>
> --
> Kind Regards,
> Niklas Söderlund

Hi Niklas,

On Wed, Oct 22, 2025 at 10:16:48AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2025-10-22 09:22:58 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Oct 21, 2025 at 06:50:40PM +0200, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > On 2025-10-21 16:01:03 +0200, Jacopo Mondi wrote:
> > > > Hi Niklas,
> > > >
> > > > On Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Thanks for your work.
> > > > >
> > > > > On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:
> > > > > > The limits (min and max values) for the Camera controls are
> > > > > > registered at Camera configuration time and never updated.
> > > > > > Some controls, like FrameDurationLimits have a direct impact on
> > > > > > the limits of other controls, such as the ExposureTime and as they
> > > > > > can be configured by the application, this has to be taken into account.
> > > > > >
> > > > > > Currently, when a user changes the frame duration, the limits for
> > > > > > both the ExposureTime and FrameDurationControls are not updated.
> > > > > >
> > > > > > The timing at which the controls limits should be updated is also
> > > > > > critical: the new control values take effect once the Request they
> > > > > > belong to is completed.
> > > > > >
> > > > > > To support this operation model introduce a new IPA function
> > > > > > 'updateControlsLimits()' which the pipeline handler calls before
> > > > > > completing a Request.
> > > > > >
> > > > > > Store the exposure time limits in the FrameContext (frame
> > > > > > duration limits were already there) and update the Camera::controls()
> > > > > > control info map with the limits as computed for the Request that has
> > > > > > just completed.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > > ---
> > > > > >  include/libcamera/ipa/rkisp1.mojom       |  2 ++
> > > > > >  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
> > > > > >  src/ipa/rkisp1/ipa_context.h             |  3 +++
> > > > > >  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
> > > > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
> > > > > >  5 files changed, 37 insertions(+)
> > > > > >
> > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> > > > > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
> > > > > >  		  map<uint32, libcamera.IPAStream> streamConfig)
> > > > > >  		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > > > >
> > > > > > +	updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > > > > +
> > > > > >  	mapBuffers(array<libcamera.IPABuffer> buffers);
> > > > > >  	unmapBuffers(array<uint32> ids);
> > > > > >
> > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > > >  				* frameContext.agc.exposure;
> > > > > >  		maxExposureTime = minExposureTime;
> > > > > >  	}
> > > > > > +	frameContext.agc.minExposureTime = minExposureTime;
> > > > > > +	frameContext.agc.maxExposureTime = maxExposureTime;
> > > > > >
> > > > > >  	if (frameContext.agc.autoGainEnabled) {
> > > > > >  		minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > > > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > > >  	 * applied to the sensor when the statistics were collected.
> > > > > >  	 */
> > > > > >  	utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> > > > > > +	frameContext.agc.exposureTime = exposureTime;
> > > > > >  	double analogueGain = frameContext.sensor.gain;
> > > > > >  	utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > > > > >
> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> > > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > > @@ -143,6 +143,9 @@ struct IPAActiveState {
> > > > > >
> > > > > >  struct IPAFrameContext : public FrameContext {
> > > > > >  	struct {
> > > > > > +		utils::Duration minExposureTime;
> > > > > > +		utils::Duration maxExposureTime;
> > > > > > +		utils::Duration exposureTime;
> > > > > >  		uint32_t exposure;
> > > > > >  		double gain;
> > > > > >  		double exposureValue;
> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > @@ -62,6 +62,8 @@ public:
> > > > > >  	int configure(const IPAConfigInfo &ipaConfig,
> > > > > >  		      const std::map<uint32_t, IPAStream> &streamConfig,
> > > > > >  		      ControlInfoMap *ipaControls) override;
> > > > > > +	int updateControlsLimits(const uint32_t frame,
> > > > > > +				 ControlInfoMap *ipaControls) override;
> > > > > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > > >
> > > > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> > > > > > +{
> > > > > > +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Update the exposure time and frame duration limits with the
> > > > > > +	 * settings computed by the AGC for the frame at hand.
> > > > > > +	 */
> > > > > > +	assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> > > > > > +	assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> > > > > > +
> > > > > > +	ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> > > > > > +	ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> > > > > > +	ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> > > > > > +	ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> > > > > > +
> > > > > > +	ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> > > > > > +	ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> > > > > > +	ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> > > > > > +	ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);
> > > > >
> > > > > Is this function not doing the reveres of what is expected? It takes the
> > > > > controls from the sensor and update them with new limits using values
> > > > > calculated by the IPA. Should it not take the values updated by the
> > > >
> > > > Where does it take controls from the sensor ?
> > > >
> > > > > sensor kernel driver and propagate them to the values used by the IPA?
> > > >
> > > > As I see it, it takes the control values the IPA has calculated for
> > > > the sensor for frame/request X [*] and when request X completes it
> > > > propagates those values to Camera::controls().
> > > >
> > > > [*] We have an outstanding duality between request number and frame
> > > > number. Hopefully we should sooner or later move the whole PH/IPA
> > > > interface to be designed around frame numbers and not request ids.
> > > >
> > > > >
> > > > > In IPARkISP1::configure() we read the min and max exposure time from the
> > > > > V4L2 sensor control, here we override them with new values we calculated
> > > > > ourself.
> > > > >
> > > > > If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK
> > > > > control new max and default values for V4L2_CID_EXPOSURE are calculated
> > > > > and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate
> > > > > these values instead of calculating it's own set?
> > > > >
> > > >
> > > > So, you know very well that the model in which v4l2 controls interact
> > > > between each other is ... not optimal. To make things worse controls
> > > > for a frame are applied at different times to compensate for their
> > > > delays. At what time would you read controls back from the sensor ?
> > > >
> > > > We have DelayedControls::get() which works with 'sequence' numbers and
> > > > our IPA reasons in terms of request numbers and I didn't want to
> > > > design something around DelayedControls as some pipelines might not
> > > > use them.
> > > >
> > > >
> > > > > If not can't we make configure() simpler by removing the ABI sharing of
> > > > > this dynamic control range? And in the longer run just drop
> > > > > V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?
> > > > > That way we would have more static only controls to care about :-)
> > > > >
> > > > > I think it's dangerous to do both, init the IPA with the values from the
> > > > > kernel and then calculate our own later.
> > > >
> > > > Those two things happen at very different times. configure() happens
> > > > while no frames are captured and the driver state is a snapshot of the
> > > > current situation. updateControlsLimits() happens at runtime and the
> > > > delays (both the controls delays and the frame latency) should be
> > > > taken into account.
> > > >
> > > > However I understand your concern that "our" calculations could
> > > > diverge from the "driver" calculations, but to be honest the only way
> > > > this can happen is with a broken driver (or a broken IPA).
> > > >
> > > > Anyway, I'll drop this patch. Camera::controls() are already b0rken,
> > > > updating them would open space for bugs by invalidating references,
> > > > and if it's not been an issue so far ... let's not be too concerned
> > > > with them
> > > >
> > > > I feel more strongly about 1/3 as that actually fixes a bug.
> > > >
> > > > >
> > > > > Same comment for 1/3 where the new max exposure time is calculated.
> > > >
> > > > I don't agree here.
> > > >
> > > > In 1/3 the max exposure time is calculated at the time where a new
> > > > vblank is computed by the IPA. From that time on, all the IPA
> > > > calculations should use the new limits as all the IPA calculations
> > > > will produce control values to be applied to frames generated -after-
> > > > the just calculated vblank is applied to the sensor.
> > > >
> > > > The actual VBLANK will be updated on the sensor with a latency of a
> > > > variable number of frames and we can't rely on the sensor calculated
> > > > values to update the IPA internal limits. Plus, it's an IOCTL more for
> > > > every frame.
> > > >
> > > > I understand Stefan's argument about the exposure margin, and that
> > > > should be taken into account, but I would be suprised if poking at the
> > > > sensor to update an internal IPA tuning paramter would be desirable,
> > > > considering the potential latency and costs.
> > > >
> > > > What do you think ?
> > >
> > > I agree, the V4L2 control interface is not always optimal, and likely
> > > that is at the core of my concern.
> > >
> > > The fact that we calculate the same thing (exposure time) both in the
> > > kernel driver and in the de facto standard user-space consumer of V4L2,
> > > without correlating the two hints at a larger issue.
> > >
> > > And to make issues even more "fun" the complete documentation for the
> > > control we calculate the min/max/def values for independently on is:
> > >
> > >     V4L2_CID_EXPOSURE (integer)
> > >
> > >         Exposure (cameras). [Unit?]
> > >
> > > This is what I hinted at (poorly) in my original mail of this being
> > > dangerous. I think it's not impossible for the kernel and libcamera to
> > > disagree about that the control represent. At least by always fetching
> >
> > We know the control's units are not specified. I think we tried to
> > change upstream to specify the control unit is in lines, but afair this
> > wasn't appreciated as it would make some existing drivers not-compliant.
> >
> > That's why we have Documentation/sensor_driver_requirements.rst
> > where we say:
> >
> > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera
> > requires it to be expressed as a number of image lines. Camera sensor drivers
> > that do not comply with this requirement will need to be adapted or will produce
> > incorrect results.
>
> Thanks I was not aware of this narrowing of the control by libcamera. I
> think it's bonkers that it's needed and that upstream can't agree on a
> definition of a control, but that is another topic. For this topic I'm
> happy libcamera states what the control is.
>
> >
> > > the min/max/def values from the control we know the boundaries. But as
> > > you hint at in your reply, and I on IRC, this would cost an IOCTL per
> > > control everything something changes. Plus the plumbing to get it from
> > > pipeline handler to IPA.
> > >
> > > And I think we should either
> > >
> > >   - Fix or update the V4L2 control. This could be as "simple" as
> > >     updating the documentation for V4L2_CID_EXPOSURE to define what it
> > >     really is, it's unit and that its min and max values should not
> > >     change while streaming. Instead it shall be set to the min/max
> > >     values to correspond to the min/max values of the VBLANK (or any
> > >     other control effecting this sensors min/max value). But also
> > >     describe how they are limited by other controls and that it's up to
> > >     user-space to deal with figuring it out.
> >
> > I think we have a different interpretation of the issues around
> > EXPOSURE. By points:
> >
> > - its unit: you're right, but it didn't fly upstream, so
> >   sensor_driver_requirements.rst
> > - its min/max: min is a sensor-specific parameter and should stay
> >   constant for a mode. max is inhertely limited by the current frame
> >   length (visible + blankings) and I presume that's a given, isn't it?
> > - min/max can't change while streaming: I agree with min, but isn't max
> >   changing according to current blanking the whole reason for this
> >   series ?
>
> I think we both see the issue but assign different importance too it. My
> concern is that we at IPA configure time take the effort to consult the
> min/max values from the V4L2 control, but while streaming we adjust the
> max value by our own calculations instead of consuming the updated value
> calculated and provided by the V4L2 API.
>
> This demonstrates how cumbersome the API is and I think libcamera as the
> de-facto user-space consumer of V4L2 API should either use the
> cumbersome API, or fix it. Working around it will IMHO just create more
> fragmentation and issues down the road.
>
> At the very least I think if the min/max values of the V4L2 API is to be
> ignored for the exposure control our own method to calculate it shall be
> used at IPA configure time too. Mixing the two is very confusing for
> somebody reading the code.
>

I'm not sure I 100% share the concern here. configure() time happens
once per streaming session and I think we should have a snapshot of
the device state and reading control values seems the sane thing to do

> >
> > >
> > >     Then we can fix drivers like the IMX219 to comply with this and then
> > >     we get the correct min/max values at IPA configure time. We know the
> >
> > Why do you think imx219 is broken in regards to handling EXPOSURE ?
> >
> > Afaiu the driver does what's intended by adjusting the max exposure to the
> > current frame lenght. It's the IPA that doesn't take this into
> > account.
>
> I don't think it's broken. I think the dynamic update of controls
> min/max (any control) while streaming is an edge case of the V4L2 not
> very well designed. The comment above about fixing the IMX219 I tried to
> hint at that if we documented this better we could update the drier to
> not update min/max while streaming, or make it clear that user-space

I don't see it as an edge case, changing blankings to control frame
rate is "the right thing to do"(tm)

> must be prepared to read new min/max values while streaming.
>
> >
> >
> > >     unit they are in. We know the rules and we know that user-space is
> > >     responsible to calculate a set of controls that work together. The
> > >     kernel will only validate and ether accept and reject them.
> > >
> > > or
> > >
> > >   - Allow controls to dynamic change their min/max values while
> >
> > I might be missing something. Isn't that how it works already ? By
> > changing VBLANK you control the frame rate and, as a consequence, the
> > maximum exposure value changes as well. What did I miss ?
>
> It is. But as this libcamera change don't consume it and instead
> calculate the max by itself hints that this might not be the best
> design?
>
> >
> > >     streaming. Have libcamera fetch new limit every time it changes a
> > >     control. This will be a lot of IOCTL callas which will cause pain
> > >     and hopefully spur someone to create a better control interface ;-)
> > >
> > > I would not block patch 1/3 as it makes things better for sensors like
> > > IMX219 which is used and implement the V4L2_CID_EXPOSURE with dynamic
> > > values but with a unit that is the same as libcamera uses internally.
> > > But I do think 1/3 without also thinking about how to solve the API
> > > issue with the kernel will create more issues down the line.
> > >
> >
> > The real solution here would be to drop V/HBLANK and introduce a
> > control for the whole frame/line lengths. In this way you
> > automatically know what the exposure time maximum is without having to
> > take into account the visible sizes + blankings. It's quite a yak to
> > shave though and we do have anyway all the sensor drivers using
> > VBLANK/HBLANK and they can't be changed as otherwise existing
> > userspace that relies on those controls presence would break.
> >
> > Feels like I'm missing something somewhere ?
>
> Or maybe I am :-)
>
> But I think we both see the issue, the V4L2 controls in this area are
> not optimal designed. And there are edge cases of them that make it even
> worse to work with. Then we assign different interpretations on what to
> do about it.
>
> My understanding of your argument is that you are OK with calculating
> the min/max values both in kernel and user-space to avoid having to do
> extra IOCTLs to fetch the new values. This works for exposure and may
> very well be the best way forward.

I'm ok starting with a known state read from the subdevice and
I'm ok updating an IPA runtime parameter based on the IPA calculated
frame length. This will be also updated on the device, but at a later
time and we can't rely on that.

It's not just the extra ioctl, it's that controls are applied to the
sensor with frames of latencies while the IPA need to keep its limits
up-to-date while calculating parameters for the next frames.

>
> My argument and worry is that it's dangerous to create this split. We
> have an API and we should use it, and if possible improve it. If not I
> fear copy-paste programming will spread this behavior and we will have
> IPA with hard coded calculations for a subset of sensors as it's OK to
> ignore the API which describes the controls. My strong feelings around
> this is that libcamera tries to be the mesa of cameras so it should not
> ignore API interfaces IMHO. More painful, and maybe not the fastest/best
> way forward.
>
> I will not try to block this work based on the above as fixing the API
> is likely a huge task. But I do think if you want to calculate the max
> independently for the kernel you should use the same calculation
> function at IPA configure time and not use the V4L2 min/max values
> there.
>

I'll ask others what do they think as the different configure/runtime
behaviours are not a concern to me and I might be underestimating it.

> >
> > > > >
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > > > >  {
> > > > > >  	for (const IPABuffer &buffer : buffers) {
> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> > > > > >  	if (!isRaw_ && !info->paramDequeued)
> > > > > >  		return;
> > > > > >
> > > > > > +	/* Update controls before completing the request */
> > > > > > +	data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
> > > > > > +
> > > > > >  	data->frameInfo_.destroy(info->frame);
> > > > > >
> > > > > >  	completeRequest(request);
> > > > > >
> > > > > > --
> > > > > > 2.51.0
> > > > > >
> > > > >
> > > > > --
> > > > > Kind Regards,
> > > > > Niklas Söderlund
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
>
> --
> Kind Regards,
> Niklas Söderlund

Hi Jacopo and Niklas,

Just wanted to provide some feedback on how we overcome this
awkwardness in the RPi IPA.

On Wed, 22 Oct 2025 at 10:30, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Niklas,
>
> On Wed, Oct 22, 2025 at 10:16:48AM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > On 2025-10-22 09:22:58 +0200, Jacopo Mondi wrote:
> > > Hi Niklas,
> > >
> > > On Tue, Oct 21, 2025 at 06:50:40PM +0200, Niklas Söderlund wrote:
> > > > Hi Jacopo,
> > > >
> > > > On 2025-10-21 16:01:03 +0200, Jacopo Mondi wrote:
> > > > > Hi Niklas,
> > > > >
> > > > > On Tue, Oct 21, 2025 at 01:32:05PM +0200, Niklas Söderlund wrote:
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > Thanks for your work.
> > > > > >
> > > > > > On 2025-10-17 11:00:06 +0200, Jacopo Mondi wrote:
> > > > > > > The limits (min and max values) for the Camera controls are
> > > > > > > registered at Camera configuration time and never updated.
> > > > > > > Some controls, like FrameDurationLimits have a direct impact on
> > > > > > > the limits of other controls, such as the ExposureTime and as they
> > > > > > > can be configured by the application, this has to be taken into account.
> > > > > > >
> > > > > > > Currently, when a user changes the frame duration, the limits for
> > > > > > > both the ExposureTime and FrameDurationControls are not updated.
> > > > > > >
> > > > > > > The timing at which the controls limits should be updated is also
> > > > > > > critical: the new control values take effect once the Request they
> > > > > > > belong to is completed.
> > > > > > >
> > > > > > > To support this operation model introduce a new IPA function
> > > > > > > 'updateControlsLimits()' which the pipeline handler calls before
> > > > > > > completing a Request.
> > > > > > >
> > > > > > > Store the exposure time limits in the FrameContext (frame
> > > > > > > duration limits were already there) and update the Camera::controls()
> > > > > > > control info map with the limits as computed for the Request that has
> > > > > > > just completed.
> > > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > > > ---
> > > > > > >  include/libcamera/ipa/rkisp1.mojom       |  2 ++
> > > > > > >  src/ipa/rkisp1/algorithms/agc.cpp        |  3 +++
> > > > > > >  src/ipa/rkisp1/ipa_context.h             |  3 +++
> > > > > > >  src/ipa/rkisp1/rkisp1.cpp                | 26 ++++++++++++++++++++++++++
> > > > > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  3 +++
> > > > > > >  5 files changed, 37 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > > > > > index 068e898848c4943282b4a6a05362a99016560afd..8d9446f5cbe9851886832f5cf7fd41d1a6d23a11 100644
> > > > > > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > > > > > @@ -27,6 +27,8 @@ interface IPARkISP1Interface {
> > > > > > >               map<uint32, libcamera.IPAStream> streamConfig)
> > > > > > >             => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > > > > >
> > > > > > > +   updateControlsLimits(uint32 frame) => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > > > > > +
> > > > > > >     mapBuffers(array<libcamera.IPABuffer> buffers);
> > > > > > >     unmapBuffers(array<uint32> ids);
> > > > > > >
> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..0fd7541c9f1ab9b4cbae1f8fa60c39b032c3bcd1 100644
> > > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > > @@ -585,6 +585,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > > > >                             * frameContext.agc.exposure;
> > > > > > >             maxExposureTime = minExposureTime;
> > > > > > >     }
> > > > > > > +   frameContext.agc.minExposureTime = minExposureTime;
> > > > > > > +   frameContext.agc.maxExposureTime = maxExposureTime;
> > > > > > >
> > > > > > >     if (frameContext.agc.autoGainEnabled) {
> > > > > > >             minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > > > > > > @@ -606,6 +608,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > > > >      * applied to the sensor when the statistics were collected.
> > > > > > >      */
> > > > > > >     utils::Duration exposureTime = lineDuration * frameContext.sensor.exposure;
> > > > > > > +   frameContext.agc.exposureTime = exposureTime;
> > > > > > >     double analogueGain = frameContext.sensor.gain;
> > > > > > >     utils::Duration effectiveExposureValue = exposureTime * analogueGain;
> > > > > > >
> > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > > > index af66a749052bc82bbbe7fbb0c4626f2422700926..54ff5c114f5c6cdfaf515475a3892f76e2e022d6 100644
> > > > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > > > @@ -143,6 +143,9 @@ struct IPAActiveState {
> > > > > > >
> > > > > > >  struct IPAFrameContext : public FrameContext {
> > > > > > >     struct {
> > > > > > > +           utils::Duration minExposureTime;
> > > > > > > +           utils::Duration maxExposureTime;
> > > > > > > +           utils::Duration exposureTime;
> > > > > > >             uint32_t exposure;
> > > > > > >             double gain;
> > > > > > >             double exposureValue;
> > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > > index 54bd1434e0f4e34834beb1f9e9c39b77590f8b34..ad5c24a7159a8634aae883915b54ae0f4456692b 100644
> > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > > @@ -62,6 +62,8 @@ public:
> > > > > > >     int configure(const IPAConfigInfo &ipaConfig,
> > > > > > >                   const std::map<uint32_t, IPAStream> &streamConfig,
> > > > > > >                   ControlInfoMap *ipaControls) override;
> > > > > > > +   int updateControlsLimits(const uint32_t frame,
> > > > > > > +                            ControlInfoMap *ipaControls) override;
> > > > > > >     void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > > > >     void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > > > >
> > > > > > > @@ -294,6 +296,30 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > > > > >     return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +int IPARkISP1::updateControlsLimits(const uint32_t frame, ControlInfoMap *ipaControls)
> > > > > > > +{
> > > > > > > +   IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > > > > +
> > > > > > > +   /*
> > > > > > > +    * Update the exposure time and frame duration limits with the
> > > > > > > +    * settings computed by the AGC for the frame at hand.
> > > > > > > +    */
> > > > > > > +   assert(ipaControls->find(&controls::ExposureTime) != ipaControls->end());
> > > > > > > +   assert(ipaControls->find(&controls::FrameDurationLimits) != ipaControls->end());
> > > > > > > +
> > > > > > > +   ControlValue eMin(static_cast<int32_t>(frameContext.agc.minExposureTime.get<std::micro>()));
> > > > > > > +   ControlValue eMax(static_cast<int32_t>(frameContext.agc.maxExposureTime.get<std::micro>()));
> > > > > > > +   ControlValue eDef(static_cast<int32_t>(frameContext.agc.exposureTime.get<std::micro>()));
> > > > > > > +   ipaControls->at(controls::ExposureTime.id()) = ControlInfo(eMin, eMax, eDef);
> > > > > > > +
> > > > > > > +   ControlValue fMin(static_cast<int32_t>(frameContext.agc.minFrameDuration.get<std::micro>()));
> > > > > > > +   ControlValue fMax(static_cast<int32_t>(frameContext.agc.maxFrameDuration.get<std::micro>()));
> > > > > > > +   ControlValue fDef(static_cast<int32_t>(frameContext.agc.frameDuration.get<std::micro>()));
> > > > > > > +   ipaControls->at(controls::FrameDurationLimits.id()) = ControlInfo(fMin, fMax, fDef);
> > > > > >
> > > > > > Is this function not doing the reveres of what is expected? It takes the
> > > > > > controls from the sensor and update them with new limits using values
> > > > > > calculated by the IPA. Should it not take the values updated by the
> > > > >
> > > > > Where does it take controls from the sensor ?
> > > > >
> > > > > > sensor kernel driver and propagate them to the values used by the IPA?
> > > > >
> > > > > As I see it, it takes the control values the IPA has calculated for
> > > > > the sensor for frame/request X [*] and when request X completes it
> > > > > propagates those values to Camera::controls().
> > > > >
> > > > > [*] We have an outstanding duality between request number and frame
> > > > > number. Hopefully we should sooner or later move the whole PH/IPA
> > > > > interface to be designed around frame numbers and not request ids.
> > > > >
> > > > > >
> > > > > > In IPARkISP1::configure() we read the min and max exposure time from the
> > > > > > V4L2 sensor control, here we override them with new values we calculated
> > > > > > ourself.
> > > > > >
> > > > > > If we take the IMX219 as an example, when we update the V4L2_CID_VBLANK
> > > > > > control new max and default values for V4L2_CID_EXPOSURE are calculated
> > > > > > and set, see imx219_set_ctrl(). Should not libcamera fetch and propagate
> > > > > > these values instead of calculating it's own set?
> > > > > >
> > > > >
> > > > > So, you know very well that the model in which v4l2 controls interact
> > > > > between each other is ... not optimal. To make things worse controls
> > > > > for a frame are applied at different times to compensate for their
> > > > > delays. At what time would you read controls back from the sensor ?
> > > > >
> > > > > We have DelayedControls::get() which works with 'sequence' numbers and
> > > > > our IPA reasons in terms of request numbers and I didn't want to
> > > > > design something around DelayedControls as some pipelines might not
> > > > > use them.
> > > > >
> > > > >
> > > > > > If not can't we make configure() simpler by removing the ABI sharing of
> > > > > > this dynamic control range? And in the longer run just drop
> > > > > > V4L2_CID_EXPOSURE from sensors that also implements V4L2_CID_VBLANK?
> > > > > > That way we would have more static only controls to care about :-)
> > > > > >
> > > > > > I think it's dangerous to do both, init the IPA with the values from the
> > > > > > kernel and then calculate our own later.
> > > > >
> > > > > Those two things happen at very different times. configure() happens
> > > > > while no frames are captured and the driver state is a snapshot of the
> > > > > current situation. updateControlsLimits() happens at runtime and the
> > > > > delays (both the controls delays and the frame latency) should be
> > > > > taken into account.
> > > > >
> > > > > However I understand your concern that "our" calculations could
> > > > > diverge from the "driver" calculations, but to be honest the only way
> > > > > this can happen is with a broken driver (or a broken IPA).
> > > > >
> > > > > Anyway, I'll drop this patch. Camera::controls() are already b0rken,
> > > > > updating them would open space for bugs by invalidating references,
> > > > > and if it's not been an issue so far ... let's not be too concerned
> > > > > with them
> > > > >
> > > > > I feel more strongly about 1/3 as that actually fixes a bug.
> > > > >
> > > > > >
> > > > > > Same comment for 1/3 where the new max exposure time is calculated.
> > > > >
> > > > > I don't agree here.
> > > > >
> > > > > In 1/3 the max exposure time is calculated at the time where a new
> > > > > vblank is computed by the IPA. From that time on, all the IPA
> > > > > calculations should use the new limits as all the IPA calculations
> > > > > will produce control values to be applied to frames generated -after-
> > > > > the just calculated vblank is applied to the sensor.
> > > > >
> > > > > The actual VBLANK will be updated on the sensor with a latency of a
> > > > > variable number of frames and we can't rely on the sensor calculated
> > > > > values to update the IPA internal limits. Plus, it's an IOCTL more for
> > > > > every frame.
> > > > >
> > > > > I understand Stefan's argument about the exposure margin, and that
> > > > > should be taken into account, but I would be suprised if poking at the
> > > > > sensor to update an internal IPA tuning paramter would be desirable,
> > > > > considering the potential latency and costs.
> > > > >
> > > > > What do you think ?
> > > >
> > > > I agree, the V4L2 control interface is not always optimal, and likely
> > > > that is at the core of my concern.
> > > >
> > > > The fact that we calculate the same thing (exposure time) both in the
> > > > kernel driver and in the de facto standard user-space consumer of V4L2,
> > > > without correlating the two hints at a larger issue.
> > > >
> > > > And to make issues even more "fun" the complete documentation for the
> > > > control we calculate the min/max/def values for independently on is:
> > > >
> > > >     V4L2_CID_EXPOSURE (integer)
> > > >
> > > >         Exposure (cameras). [Unit?]
> > > >
> > > > This is what I hinted at (poorly) in my original mail of this being
> > > > dangerous. I think it's not impossible for the kernel and libcamera to
> > > > disagree about that the control represent. At least by always fetching
> > >
> > > We know the control's units are not specified. I think we tried to
> > > change upstream to specify the control unit is in lines, but afair this
> > > wasn't appreciated as it would make some existing drivers not-compliant.
> > >
> > > That's why we have Documentation/sensor_driver_requirements.rst
> > > where we say:
> > >
> > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera
> > > requires it to be expressed as a number of image lines. Camera sensor drivers
> > > that do not comply with this requirement will need to be adapted or will produce
> > > incorrect results.
> >
> > Thanks I was not aware of this narrowing of the control by libcamera. I
> > think it's bonkers that it's needed and that upstream can't agree on a
> > definition of a control, but that is another topic. For this topic I'm
> > happy libcamera states what the control is.
> >
> > >
> > > > the min/max/def values from the control we know the boundaries. But as
> > > > you hint at in your reply, and I on IRC, this would cost an IOCTL per
> > > > control everything something changes. Plus the plumbing to get it from
> > > > pipeline handler to IPA.
> > > >
> > > > And I think we should either
> > > >
> > > >   - Fix or update the V4L2 control. This could be as "simple" as
> > > >     updating the documentation for V4L2_CID_EXPOSURE to define what it
> > > >     really is, it's unit and that its min and max values should not
> > > >     change while streaming. Instead it shall be set to the min/max
> > > >     values to correspond to the min/max values of the VBLANK (or any
> > > >     other control effecting this sensors min/max value). But also
> > > >     describe how they are limited by other controls and that it's up to
> > > >     user-space to deal with figuring it out.
> > >
> > > I think we have a different interpretation of the issues around
> > > EXPOSURE. By points:
> > >
> > > - its unit: you're right, but it didn't fly upstream, so
> > >   sensor_driver_requirements.rst
> > > - its min/max: min is a sensor-specific parameter and should stay
> > >   constant for a mode. max is inhertely limited by the current frame
> > >   length (visible + blankings) and I presume that's a given, isn't it?
> > > - min/max can't change while streaming: I agree with min, but isn't max
> > >   changing according to current blanking the whole reason for this
> > >   series ?
> >
> > I think we both see the issue but assign different importance too it. My
> > concern is that we at IPA configure time take the effort to consult the
> > min/max values from the V4L2 control, but while streaming we adjust the
> > max value by our own calculations instead of consuming the updated value
> > calculated and provided by the V4L2 API.
> >
> > This demonstrates how cumbersome the API is and I think libcamera as the
> > de-facto user-space consumer of V4L2 API should either use the
> > cumbersome API, or fix it. Working around it will IMHO just create more
> > fragmentation and issues down the road.
> >
> > At the very least I think if the min/max values of the V4L2 API is to be
> > ignored for the exposure control our own method to calculate it shall be
> > used at IPA configure time too. Mixing the two is very confusing for
> > somebody reading the code.
> >
>
> I'm not sure I 100% share the concern here. configure() time happens
> once per streaming session and I think we should have a snapshot of
> the device state and reading control values seems the sane thing to do
>
> > >
> > > >
> > > >     Then we can fix drivers like the IMX219 to comply with this and then
> > > >     we get the correct min/max values at IPA configure time. We know the
> > >
> > > Why do you think imx219 is broken in regards to handling EXPOSURE ?
> > >
> > > Afaiu the driver does what's intended by adjusting the max exposure to the
> > > current frame lenght. It's the IPA that doesn't take this into
> > > account.
> >
> > I don't think it's broken. I think the dynamic update of controls
> > min/max (any control) while streaming is an edge case of the V4L2 not
> > very well designed. The comment above about fixing the IMX219 I tried to
> > hint at that if we documented this better we could update the drier to
> > not update min/max while streaming, or make it clear that user-space
>
> I don't see it as an edge case, changing blankings to control frame
> rate is "the right thing to do"(tm)
>
> > must be prepared to read new min/max values while streaming.
> >
> > >
> > >
> > > >     unit they are in. We know the rules and we know that user-space is
> > > >     responsible to calculate a set of controls that work together. The
> > > >     kernel will only validate and ether accept and reject them.
> > > >
> > > > or
> > > >
> > > >   - Allow controls to dynamic change their min/max values while
> > >
> > > I might be missing something. Isn't that how it works already ? By
> > > changing VBLANK you control the frame rate and, as a consequence, the
> > > maximum exposure value changes as well. What did I miss ?
> >
> > It is. But as this libcamera change don't consume it and instead
> > calculate the max by itself hints that this might not be the best
> > design?
> >
> > >
> > > >     streaming. Have libcamera fetch new limit every time it changes a
> > > >     control. This will be a lot of IOCTL callas which will cause pain
> > > >     and hopefully spur someone to create a better control interface ;-)
> > > >
> > > > I would not block patch 1/3 as it makes things better for sensors like
> > > > IMX219 which is used and implement the V4L2_CID_EXPOSURE with dynamic
> > > > values but with a unit that is the same as libcamera uses internally.
> > > > But I do think 1/3 without also thinking about how to solve the API
> > > > issue with the kernel will create more issues down the line.
> > > >
> > >
> > > The real solution here would be to drop V/HBLANK and introduce a
> > > control for the whole frame/line lengths. In this way you
> > > automatically know what the exposure time maximum is without having to
> > > take into account the visible sizes + blankings. It's quite a yak to
> > > shave though and we do have anyway all the sensor drivers using
> > > VBLANK/HBLANK and they can't be changed as otherwise existing
> > > userspace that relies on those controls presence would break.
> > >
> > > Feels like I'm missing something somewhere ?
> >
> > Or maybe I am :-)
> >
> > But I think we both see the issue, the V4L2 controls in this area are
> > not optimal designed. And there are edge cases of them that make it even
> > worse to work with. Then we assign different interpretations on what to
> > do about it.
> >
> > My understanding of your argument is that you are OK with calculating
> > the min/max values both in kernel and user-space to avoid having to do
> > extra IOCTLs to fetch the new values. This works for exposure and may
> > very well be the best way forward.
>
> I'm ok starting with a known state read from the subdevice and
> I'm ok updating an IPA runtime parameter based on the IPA calculated
> frame length. This will be also updated on the device, but at a later
> time and we can't rely on that.
>
> It's not just the extra ioctl, it's that controls are applied to the
> sensor with frames of latencies while the IPA need to keep its limits
> up-to-date while calculating parameters for the next frames.
>
> >
> > My argument and worry is that it's dangerous to create this split. We
> > have an API and we should use it, and if possible improve it. If not I
> > fear copy-paste programming will spread this behavior and we will have
> > IPA with hard coded calculations for a subset of sensors as it's OK to
> > ignore the API which describes the controls. My strong feelings around
> > this is that libcamera tries to be the mesa of cameras so it should not
> > ignore API interfaces IMHO. More painful, and maybe not the fastest/best
> > way forward.
> >
> > I will not try to block this work based on the above as fixing the API
> > is likely a huge task. But I do think if you want to calculate the max
> > independently for the kernel you should use the same calculation
> > function at IPA configure time and not use the V4L2 min/max values
> > there.
> >
>
> I'll ask others what do they think as the different configure/runtime
> behaviours are not a concern to me and I might be underestimating it.

From pretty much the start, our IPA has done exactly what (I think!)
Jacopo is suggesting:

- We calculate the "max" shutter values based on the maximum
V4L2_CID_VBLANK value advertised, but also clipped to the limits of
the FrameDurationLimits control set by the user.   This calculation is
dynamic since the user can set FrameDurationLimits at runtime.  We
don't rely on the max value of the V4L2_CID_EXPOSURE control at all.

- The camere helper has a notion of the maximum exposure allowed based
on the frame length, typically sensors call this "frame integration
difference", which is used in the above calculation.

- We must ensure V4L2_CID_VBLANK is set before V4L2_CID_EXPOSURE,
hence the notion of "priorityWrite" in the DelayedControls class.
Without this, V4L2 will possibly reject/clip the V4L2_CID_EXPOSURE
control unnecessarily if the existing cached V4L2_CID_VBLANK control
clips the range.

With this we achieve what I think is needed here in a robust way.  I
do agree some of what we do here is working around some of the API
deficiencies in the V4L2 control API, but it seems manageable enough.

Hope that helps!

Naush


>
> > >
> > > > > >
> > > > > > > +
> > > > > > > +   return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > > > > >  {
> > > > > > >     for (const IPABuffer &buffer : buffers) {
> > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > > index ecd13831539fdf5cb79da2ea4b33a353514328ae..ad1ff7a8be8d6e3268e21ec92ae4e07500b38c66 100644
> > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > > @@ -1494,6 +1494,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> > > > > > >     if (!isRaw_ && !info->paramDequeued)
> > > > > > >             return;
> > > > > > >
> > > > > > > +   /* Update controls before completing the request */
> > > > > > > +   data->ipa_->updateControlsLimits(info->frame, &data->controlInfo_);
> > > > > > > +
> > > > > > >     data->frameInfo_.destroy(info->frame);
> > > > > > >
> > > > > > >     completeRequest(request);
> > > > > > >
> > > > > > > --
> > > > > > > 2.51.0
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Kind Regards,
> > > > > > Niklas Söderlund
> > > >
> > > > --
> > > > Kind Regards,
> > > > Niklas Söderlund
> >
> > --
> > Kind Regards,
> > Niklas Söderlund