[PATCH] ipa: rpi: Fix for incorrectly reported max shutter speed
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 9 13:48:22 CEST 2024
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 ¶ms,
> > 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
> >
More information about the libcamera-devel
mailing list