[libcamera-devel,v1,3/3] ipa: rpi: Set lens position to hyperfocal on startup
diff mbox series

Message ID 20230601095630.25443-4-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Default lens behaviour
Related show

Commit Message

Naushir Patuck June 1, 2023, 9:56 a.m. UTC
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@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Nick Hollinghurst June 1, 2023, 10:33 a.m. UTC | #1
On Thu, 1 Jun 2023 at 10:56, Naushir Patuck <naush@raspberrypi.com> 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@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) {
> +                       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);
> +               }
>         }
>
>         result->sensorControls = std::move(ctrls);
> --
> 2.34.1
>

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.

 Nick
Laurent Pinchart June 2, 2023, 6:43 a.m. UTC | #2
Hello,

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 <naush@raspberrypi.com> 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@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.

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

> > +               }
> >         }
> >
> >         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.
Naushir Patuck June 2, 2023, 8:39 a.m. UTC | #3
Hi Laurent,

Thank you for the feedback!

On Fri, 2 Jun 2023 at 07:43, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> 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 <naush@raspberrypi.com> 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@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?

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

Regards,
Naush

>
> > > +               }
> > >         }
> > >
> > >         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
Laurent Pinchart June 5, 2023, 7:53 a.m. UTC | #4
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@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.
David Plowman June 12, 2023, 9:08 a.m. UTC | #5
Hi everyone

I just wanted to pick up Nick's points, which I think are valid.

The default behaviour that you might want will often be hyperfocal,
but not always. Particularly if, as Nick said, the AfRange were set to
macro you'd probably want to define a specific default if the AF
algorithm is put into Macro on startup.

I'm always a bit paranoid about unnecessary lens movements too. As
things stand, if you wanted a different lens position, you couldn't
avoid sending the lens to hyperfocal, and then to the place that you
wanted. Could we delay the default movement until the camera starts,
then we can replace this by a single movement? It also lets us do
things like define a default which is appropriate for the range that
is being set, and gives an application control of it.

It would also help when you wanted to restart your camera app, but
leave the lens wherever it was when the app last finished. Delaying
the default movement until the user has a chance to set the lens
position where it was previously means you could avoid moving it
altogether. (There might still be a slight problem in finding out
where the lens ended up "last time"?)

Any thoughts?

Thanks!
David

On Mon, 5 Jun 2023 at 08:53, Laurent Pinchart via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> 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@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
Kieran Bingham June 14, 2023, 10:15 a.m. UTC | #6
Quoting David Plowman via libcamera-devel (2023-06-12 10:08:43)
> Hi everyone
> 
> I just wanted to pick up Nick's points, which I think are valid.
> 
> The default behaviour that you might want will often be hyperfocal,
> but not always. Particularly if, as Nick said, the AfRange were set to
> macro you'd probably want to define a specific default if the AF
> algorithm is put into Macro on startup.
> 
> I'm always a bit paranoid about unnecessary lens movements too. As
> things stand, if you wanted a different lens position, you couldn't
> avoid sending the lens to hyperfocal, and then to the place that you
> wanted. Could we delay the default movement until the camera starts,

It sounds reasonable that the first update to the lens position could be
set during camera->start() - perhaps by having a hook into something
like CameraLens->start() which would be the first point any actual
change is made?

(I wonder if this means we should also have a CameraSensor->start() ....
would that let us clean up controls that currently get passed during
camera->start()?)

But are there any conditions where this would be undesirable? Do we need
to wait for the lens to settle before handling any 3a for instance? I
expect 'most' of the AWB/AEGC shouldn't be too affected by the focus...

> then we can replace this by a single movement? It also lets us do
> things like define a default which is appropriate for the range that
> is being set, and gives an application control of it.

I could expect that during configure the CameraLens class could be
configured accordingly indeed. It would then cache the most recent state
until start, and would mean multiple configure calls wouldn't actually
cause an unnecessary movement...


> It would also help when you wanted to restart your camera app, but
> leave the lens wherever it was when the app last finished. Delaying
> the default movement until the user has a chance to set the lens
> position where it was previously means you could avoid moving it
> altogether. (There might still be a slight problem in finding out
> where the lens ended up "last time"?)

I think that would be up to the camera application to save metadata
state and continue from there if it desired.

--
Kieran


> 
> Any thoughts?
> 
> Thanks!
> David
> 
> On Mon, 5 Jun 2023 at 08:53, Laurent Pinchart via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
> >
> > 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@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

Patch
diff mbox series

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) {
+			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);
+		}
 	}
 
 	result->sensorControls = std::move(ctrls);