[libcamera-devel,RFC,v1,5/5] ipa: ipu3: af: Remove hardcoded maximum VCM steps

15676 diff mbox series

The hardcoded VCM step variable was removed and was replaced by the
context configuration.

Signed-off-by: Kate Hsuan<hpa@redhat.com>
---
 src/ipa/ipu3/algorithms/af.cpp | 6 +++---
 src/ipa/ipu3/ipu3.cpp          | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

Quoting Kate Hsuan via libcamera-devel (2022-04-14 08:43:42)
> The hardcoded VCM step variable was removed and was replaced by the
> context configuration.
> 
> Signed-off-by: Kate Hsuan<hpa@redhat.com>
> ---
>  src/ipa/ipu3/algorithms/af.cpp | 6 +++---
>  src/ipa/ipu3/ipu3.cpp          | 1 +
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index addf98af..95fb39f5 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -179,7 +179,7 @@ int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>         grid.y_start |= IPU3_UAPI_GRID_Y_START_EN;
>  
>         /* Initial max focus step */
> -       maxStep_ = kMaxFocusSteps;
> +       maxStep_ = context.configuration.af.maxVcmSteps;

Aha I see (from below) you're setting
context.configuration.af.maxVcmSteps just to assign it here, but you
have direct access to configInfo.sensorInfo.maxVcmSteps.

Seems like the af.maxVcmSteps addition might not be needed?

>  
>         /* Initial focus value */
>         context.frameContext.af.focus = 0;
> @@ -214,7 +214,7 @@ void Af::afCoarseScan(IPAContext &context)
>                 context.frameContext.af.focus = focus_;
>                 previousVariance_ = 0;
>                 maxStep_ = std::clamp(focus_ + static_cast<uint32_t>((focus_ * kFineRange)),
> -                                     0U, kMaxFocusSteps);
> +                                     0U, static_cast<uint32_t>(context.configuration.af.maxVcmSteps));

The static_cast shouldn't be needed here. I think maxVcmSteps should be
uint32_t.

>         }
>  }
>  
> @@ -259,7 +259,7 @@ void Af::afReset(IPAContext &context)
>         previousVariance_ = 0.0;
>         coarseCompleted_ = false;
>         fineCompleted_ = false;
> -       maxStep_ = kMaxFocusSteps;
> +       maxStep_ = context.configuration.af.maxVcmSteps;
>  }
>  
>  /**
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index dd6cfd79..d46f7853 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -456,6 +456,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>         /* Clean frameContext at each reconfiguration. */
>         context_.frameContext = {};
> +       context_.configuration.af.maxVcmSteps = configInfo.sensorInfo.maxVcmSteps;

This should be in the configure call of the AF, which will then get
called by the "for (auto const &algo : algorithms_)" loop at the bottom
of this function.

>         if (!validateSensorControls()) {
>                 LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> -- 
> 2.35.1
>

Hi Kieran,

On Thu, Apr 14, 2022 at 4:55 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Kate Hsuan via libcamera-devel (2022-04-14 08:43:42)
> > The hardcoded VCM step variable was removed and was replaced by the
> > context configuration.
> >
> > Signed-off-by: Kate Hsuan<hpa@redhat.com>
> > ---
> >  src/ipa/ipu3/algorithms/af.cpp | 6 +++---
> >  src/ipa/ipu3/ipu3.cpp          | 1 +
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> > index addf98af..95fb39f5 100644
> > --- a/src/ipa/ipu3/algorithms/af.cpp
> > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > @@ -179,7 +179,7 @@ int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >         grid.y_start |= IPU3_UAPI_GRID_Y_START_EN;
> >
> >         /* Initial max focus step */
> > -       maxStep_ = kMaxFocusSteps;
> > +       maxStep_ = context.configuration.af.maxVcmSteps;
>
> Aha I see (from below) you're setting
> context.configuration.af.maxVcmSteps just to assign it here, but you
> have direct access to configInfo.sensorInfo.maxVcmSteps.
>
> Seems like the af.maxVcmSteps addition might not be needed?

Seems can be removed. I could get the value in configure() of the
algorithm. I'll update them in v2.


Thank you :)

>
> >
> >         /* Initial focus value */
> >         context.frameContext.af.focus = 0;
> > @@ -214,7 +214,7 @@ void Af::afCoarseScan(IPAContext &context)
> >                 context.frameContext.af.focus = focus_;
> >                 previousVariance_ = 0;
> >                 maxStep_ = std::clamp(focus_ + static_cast<uint32_t>((focus_ * kFineRange)),
> > -                                     0U, kMaxFocusSteps);
> > +                                     0U, static_cast<uint32_t>(context.configuration.af.maxVcmSteps));
>
> The static_cast shouldn't be needed here. I think maxVcmSteps should be
> uint32_t.


>
> >         }
> >  }
> >
> > @@ -259,7 +259,7 @@ void Af::afReset(IPAContext &context)
> >         previousVariance_ = 0.0;
> >         coarseCompleted_ = false;
> >         fineCompleted_ = false;
> > -       maxStep_ = kMaxFocusSteps;
> > +       maxStep_ = context.configuration.af.maxVcmSteps;
> >  }
> >
> >  /**
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index dd6cfd79..d46f7853 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -456,6 +456,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >
> >         /* Clean frameContext at each reconfiguration. */
> >         context_.frameContext = {};
> > +       context_.configuration.af.maxVcmSteps = configInfo.sensorInfo.maxVcmSteps;
>
> This should be in the configure call of the AF, which will then get
> called by the "for (auto const &algo : algorithms_)" loop at the bottom
> of this function.
>
> >         if (!validateSensorControls()) {
> >                 LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > --
> > 2.35.1
> >
>