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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart