[PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed

[Kieran Bingham]

Quoting Naushir Patuck (2024-05-09 12:43:03)
> On Thu, 9 May 2024 at 12:02, Kieran Bingham <kieran.bingham at ideasonboard.com>
> wrote:
> 
> > Quoting Laurent Pinchart (2024-05-09 11:23:14)
> > > Hi Naush,
> > >
> > > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:
> > > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:
> > > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:
> > > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:
> > > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:
> > > > > > > > The maximum shutter speed calculation in the cam-helper relied
> > on
> > > > > > > > the frame duration limits being correctly set in the
> > cam-helper's mode
> > > > > > > > structure. This was not the case on first startup, so the
> > maximum
> > > > > > > > shutter speed reported back via the ControlInfo was incorrect.
> > > > > > > >
> > > > > > > > Fix this by setting up the camera mode in the cam-helper
> > before querying
> > > > > > > > for the max shutter value.
> > > > > > > >
> > > > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > > > ---
> > > > > > > >  src/ipa/rpi/common/ipa_base.cpp | 6 ++++++
> > > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp
> > b/src/ipa/rpi/common/ipa_base.cpp
> > > > > > > > index 149a133ab662..1d12262bda01 100644
> > > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const
> > IPACameraSensorInfo &sensorInfo)
> > > > > > > >       mode_.minAnalogueGain =
> > helper_->gain(gainCtrl.min().get<int32_t>());
> > > > > > > >       mode_.maxAnalogueGain =
> > helper_->gain(gainCtrl.max().get<int32_t>());
> > > > > > > >
> > > > > > > > +     /*
> > > > > > > > +      * We need to give the helper the min/max frame
> > durations so it can calculate
> > > > > > > > +      * the correct exposure limits below.
> > > > > > > > +      */
> > > > > > > > +     helper_->setCameraMode(mode_);
> > > > > > > > +
> > > > > > >
> > > > > > > Don't you end up doing this twice, once here, and once in
> > > > > > > IpaBase::configure(), which calls IpaBase::setMode() ?
> > > > > >
> > > > > > Yes, it does happen twice.  The other option would be to pass a
> > const
> > > > > > reference to mode_ into the helper through setMode(), allowing us
> > only do
> > > > > > it once.
> > > > >
> > > > > I don't think I follow you.
> > > >
> > > > What I mean is that we don't give the cam helper its own copy of the
> > > > mode structure, but pass it a const reference to the IPA's mode
> > > > structure.   This would solve the problem with the calculation here.
> > >
> > > It's probably me, but I still don't get it :-) We have
> > >
> > > void CamHelper::setCameraMode(const CameraMode &mode)
> > > {
> > >         mode_ = mode;
> > >         if (parser_) {
> > >                 parser_->reset();
> > >                 parser_->setBitsPerPixel(mode.bitdepth);
> > >                 parser_->setLineLengthBytes(0); /* We use SetBufferSize.
> > */
> > >         }
> > > }
> > >
> > > called in IpaBase::configure(). This patch adds another call in
> > > IpaBase::setMode(). You wrote
> > >
> > > > The other option would be to pass a const reference to mode_ into the
> > > > helper through setMode()
> > >
> > > Doesn't this patch does exactly that ? How is it another option ?
> > >
> > > /me is confused
> >
> >
> > I can see the duplication.
> >
> > I guess the question is ... Naush - do you want to remove the second
> > call? I don't see any need to change the constness of it currently?
> >
> >
> > The call flow I see is:
> >
> > IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const
> > ConfigParams &params,
> >                            ConfigResult *result)
> > {
> > ...
> >         /* Re-assemble camera mode using the sensor info. */
> >         setMode(sensorInfo);
> >          ->
> >            IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)
> >            { /* KB: Which with this patch calls */
> >                 ...
> >                 mode_.minAnalogueGain =
> > helper_->gain(gainCtrl.min().get<int32_t>());
> >                 mode_.maxAnalogueGain =
> > helper_->gain(gainCtrl.max().get<int32_t>());
> >
> > +               /*
> > +                * We need to give the helper the min/max frame durations
> > so it can calculate
> > +                * the correct exposure limits below.
> > +                */
> > +               helper_->setCameraMode(mode_);
> >
> >                 /* Shutter speed is calculated based on the limits of the
> > frame durations. */
> >                 mode_.minShutter =
> > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength);
> >                 mode_.maxShutter = Duration::max();
> >                 helper_->getBlanking(mode_.maxShutter,
> >                                      mode_.minFrameDuration,
> > mode_.maxFrameDuration);
> >                 ...
> >            }
> >
> >         /* KB: And then flows directly into the next two lines to call
> >          * helper_->setCameraMode(mode_); again */
> >
> >         mode_.transform =
> > static_cast<libcamera::Transform>(params.transform);
> > -
> > -       /* Pass the camera mode to the CamHelper to setup algorithms. */
> > -       helper_->setCameraMode(mode_);
> >
> > ...
> > }
> >
> > So I think the open question is simply, should this patch remove the
> > lines I've prefixed with '-' as it has added the lines prefixed with
> > '+'....
> >
> > I think doing so require setting the mode_.transform before calling
> > setMode(sensorInfo), but that looks like it's all it would do?
> >
> 
> Almost but not quite.  The copy in the helper will not have the updated
> min/max shutter.
> 
> 
> >
> >
> > Naush - if you're happy with the patch as is, I can merge it already.
> > There's probably some other interactions we've missed that you have
> > better visibility on.
> >
> 
> I would suggest sticking with the current patch.  It can get fiddly quite
> quickly moving these bits of logic around.

Sold, merging.
--
Kieran

> 
> Regards,
> Naush
> 
> 
> >
> > --
> > Kieran
> >
> >
> >
> > >
> > > > > > But that was a bit more involved over this more trivial fix.  I
> > > > > > can change to do that if folks prefer.
> > > > > >
> > > > > > > >       /* Shutter speed is calculated based on the limits of
> > the frame durations. */
> > > > > > > >       mode_.minShutter =
> > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength);
> > > > > > > >       mode_.maxShutter = Duration::max();
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >