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

[Kieran Bingham]

Quoting Laurent Pinchart (2024-06-17 13:10:08)
> On Mon, Jun 17, 2024 at 03:09:01PM +0300, Laurent Pinchart wrote:
> > 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.
> 
> I meant 10/12, sorry

I've checked the other setLimits caller, and there's no equivalent
required there.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

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