[libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to hyperfocal on startup

[Laurent Pinchart]

Hi Naush,

On Fri, Jun 02, 2023 at 09:39:30AM +0100, Naushir Patuck wrote:
> On Fri, 2 Jun 2023 at 07:43, Laurent Pinchart wrote:
> > On Thu, Jun 01, 2023 at 11:33:15AM +0100, Nick Hollinghurst via libcamera-devel wrote:
> > > On Thu, 1 Jun 2023 at 10:56, Naushir Patuck wrote:
> > > >
> > > > On the first ipa->configure() call, set the lens position (if a lens is
> > > > present) to the default position. Typically this would be the hyperfocal
> > > > position based on the tuning data.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > >  src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > > > index 599ad146a863..4413689b7bef 100644
> > > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > > @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
> > > >                 agcStatus.shutterTime = defaultExposureTime;
> > > >                 agcStatus.analogueGain = defaultAnalogueGain;
> > > >                 applyAGC(&agcStatus, ctrls);
> > > > +
> > > > +               /* Set the lens to the default (typically hyperfocal) position on first start. */
> > > > +               RPiController::AfAlgorithm *af =
> > > > +                       dynamic_cast<RPiController::AfAlgorithm *>(controller_.getAlgorithm("af"));
> > > > +               if (lensPresent_ && af) {
> >
> > You could avoid looking up the AF algorithm is lensPresent_ is false.
> > That's a small optimization in a non-hot path, so it's not mandatory.
> 
> That's fine to do.  Would you want me to post another patch or can it be done
> when applying?

As it changes the structure of the code a bit, I'd prefer a new version.
I don't want to risk making a mistake with an untested change (I've made
enough of those already).

> > > > +                       float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get<float>();
> > > > +                       ControlList lensCtrl(lensCtrls_);
> > > > +                       int32_t hwpos;
> > > > +
> > > > +                       af->setLensPosition(defaultPos, &hwpos);
> > > > +                       lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);
> > > > +                       result->lensControls = std::move(lensCtrl);
> >
> > Will this work as expected if the user sets the AF mode to continuous at
> > start time ? Would it cause the lens to first be moved to the hyperfocal
> > distance, to be overridden right after ?
> 
> Yes, it does exactly that.  It's only a minor inconvenience, and in reality the
> user is not going to get to see this movement as the startup frames are dropped
> anyway.

I suppose we can enhance it later if needed.

> > > > +               }
> > > >         }
> > > >
> > > >         result->sensorControls = std::move(ctrls);
> > >
> > > I think this is good, but it suggests future cleanups:
> > >
> > > There's an opportunity to simplify the RPI AF algorithm, which
> > > deliberately supports an initial state in which it doesn't touch the
> > > lens and doesn't know where it is. If lens position is always
> > > initialized, a flag and some code paths can be removed there. But
> > > there's currently no way to retrieve "hwpos" without either setting a
> > > lens position or processing a frame; so it might entail a change to
> > > AfAlgorithm interface.
> > >
> > > Based on the existing interface, this patch is doing the best thing.
> > >
> > > There's a complication with the concept of "default" too: The tuning
> > > file can define defaults for each range ("normal", "macro", "full"),
> > > but none of those match the static default and limits in the
> > > ControlInfo. This might be part of a more general issue with control
> > > limits, so I don't think it needs addressing here.

-- 
Regards,

Laurent Pinchart