[PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter speed

[Laurent Pinchart]

On Mon, Jun 17, 2024 at 08:52:01PM +0900, Paul Elder wrote:
> On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2024-06-16 17:39:09)
> > > The sensor's maximum shutter speed is clamped by the maximum frame
> > > duration specified in requests. If the requested maximum frame duration
> > > is lower than the sensor's minimum shutter speed, the Agc::process()
> > > function will pass a minimum value higher than the maximum to the
> > > setLimits() function, resulting in an assertion failure. Fix it by
> > > clamping the value to both the lower and the upper bounds.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 5b917557b887..0018c43f18cf 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >                        [](uint32_t x) { return x >> 4; });
> > >         expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > >  
> > > -       utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,
> > > -                                                  frameContext.agc.maxFrameDuration);
> > > +       utils::Duration maxShutterSpeed =
> > > +               std::clamp(frameContext.agc.maxFrameDuration,
> > > +                          context.configuration.sensor.minShutterSpeed,
> > > +                          context.configuration.sensor.maxShutterSpeed);
> > >         setLimits(context.configuration.sensor.minShutterSpeed,
> > >                   maxShutterSpeed,
> > 
> > I'm somehow confused here, as we're passing sensor.minShutterSpeed in
> > here as well. Does that now become redundant? Or could/should this
> > clamping be done within setLimits? Does the other call site of setLimits
> > experience the same issue?
> 
> setLimits() is to configure the ExposureModeHelper, so that you can use
> it to split the exposure duration into gain and shutter time.
> sensor.minShutterSpeed is the minimum shutter speed for the exposure
> mode helper, and the maxShutterSpeed is, well, the maximum.

I got confused too, and implemented code to using the minimum
FrameDurationLimits to compute the minShutterSpeed. The result was
interesting, but clearly wrong :-) That's when I realized that we're
doing with shutter speeds here, not frame durations. That prompted me to
rename the variable in patch 11/12.

> Looks fine to me.
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> > >                   context.configuration.sensor.minAnalogueGain,

-- 
Regards,

Laurent Pinchart