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

[Kieran Bingham]

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?


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.

--
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