[libcamera-devel,4/6] ipu3: Apply auto focus and send lens controls to pipeline handler

14393 diff mbox series

Apply auto focus and send lens controls to pipeline handler.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com>
---
 aiq/aiq.cpp |  5 ++++-
 aiq/aiq.h   |  2 +-
 ipu3.cpp    | 27 ++++++++++++++++++++++++++-
 3 files changed, 31 insertions(+), 3 deletions(-)

Hi Han-Lin,

Thank you for the patch.

Some high level comments.

On 10/28/21 3:33 PM, Han-Lin Chen wrote:
> Apply auto focus and send lens controls to pipeline handler.
>
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com>
> ---
>   aiq/aiq.cpp |  5 ++++-
>   aiq/aiq.h   |  2 +-
>   ipu3.cpp    | 27 ++++++++++++++++++++++++++-
>   3 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp
> index 24c61cb..9c7f9d6 100644
> --- a/aiq/aiq.cpp
> +++ b/aiq/aiq.cpp
> @@ -139,7 +139,7 @@ int AIQ::setStatistics(unsigned int frame,
>    * of the parameter buffer being the only part handled when called for.
>    */
>   int AIQ::run2a(unsigned int frame, AiqInputParameters &params,
> -	       AiqResults &results)
> +	             AiqResults &results, int lensPosition, int64_t lensMovementStartTime)


Since we are now passing arguments to the algorithm's input parameters, 
I think we should be consider some refactoring in AiqInputParameters to 
accomodate our needs. One quick option I can think of, is to split and 
have setX(list of args) for (X = each algorithm's input parameters) 
functions and then call setDefaults() which shall mimick a 
setAeAwbAfDefaults() we have today (setDefaults() shall call setX() each 
with default args)


> l
>   {
>   	(void)frame;
>   
> @@ -157,6 +157,9 @@ int AIQ::run2a(unsigned int frame, AiqInputParameters &params,
>   	params.saParams.awb_results = results.awb();
>   	shadingAdapterRun(params.saParams, results);
>   
> +	params.afParams.lens_position = lensPosition;
> +	params.afParams.lens_movement_start_timestamp = lensMovementStartTime;
> +


That's how I see setting these arguments for afParams as well, passing

         setAfParams(...,  int lensPosition, int64_t lensMovementStartTime);

Have you considered any such refactoring in design? I get it that this 
may well be a quick AF functionality driven patch to test the waters.

>   	afRun(params.afParams, results);
>   
>   	return 0;
> diff --git a/aiq/aiq.h b/aiq/aiq.h
> index fcd02d2..4f5f868 100644
> --- a/aiq/aiq.h
> +++ b/aiq/aiq.h
> @@ -41,7 +41,7 @@ public:
>   			  const ipu3_uapi_stats_3a *stats);
>   
>   	int run2a(unsigned int frame, AiqInputParameters &params,
> -		  AiqResults &results);
> +	          AiqResults &results, int lensPosition, int64_t lensMovementStartTime);
>   
>   private:
>   	std::string decodeError(ia_err err);
> diff --git a/ipu3.cpp b/ipu3.cpp
> index b7de901..45330ca 100644
> --- a/ipu3.cpp
> +++ b/ipu3.cpp
> @@ -77,6 +77,10 @@ private:
>   	uint32_t gain_;
>   	uint32_t minGain_;
>   	uint32_t maxGain_;
> +	int32_t lensPosition_;
> +
> +	/* Intel AF library relies on timestamp to wait for lens movement */
> +	uint64_t lensMovementStartTime_;
>   
>   	/* Intel Library Instances. */
>   	aiq::AIQ aiq_;
> @@ -258,6 +262,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   	maxGain_ = itGain->second.max().get<int32_t>();
>   	gain_ = maxGain_;
>   
> +	lensMovementStartTime_ = 0;
> +	lensPosition_ = 0;
> +
>   	int ret;
>   
>   	ret = aiq_.configure();
> @@ -381,7 +388,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>   	*/
>   
>   	/* Run algorithms into/using this context structure */
> -	aiq_.run2a(frame, aiqInputParams_, results_);
> +	aiq_.run2a(frame, aiqInputParams_, results_, lensPosition_, lensMovementStartTime_);
>   
>   	aic_.updateRuntimeParams(results_);
>   	aic_.run(params);
> @@ -389,6 +396,19 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>   	exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time;
>   	gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global;
>   
> +	/*
> +	* Af algorithm compares the timestamp of start of lens movement start and the


s/*/ */  and all for comment lines below.

Sentence is a bit confusing. I guess among the 2 'start', one should be 
removed to be read like:

         * Af algorithm compares the timestamp of start of lens movement and

> +	* that of the statistics generated to estimate whether next lens positon
> +	* should be produced.
> +	* Todo: Uses the lens movement start time reported by the pipeline handler.


s/Todo: Uses / \todo: Use /


> +	*/
> +	if (lensPosition_ != results_.af()->next_lens_position) {
> +		utils::time_point time = utils::clock::now();
> +		uint64_t msecs = std::chrono::duration_cast<std::chrono::microseconds>(time.time_since_epoch()).count();
> +		lensMovementStartTime_ = msecs;
> +	}
> +	lensPosition_ = results_.af()->next_lens_position;
> +
>   	resultsHistory_.Push(results_);
>   
>   	setControls(frame);
> @@ -472,6 +492,11 @@ void IPAIPU3::setControls(unsigned int frame)
>   
>   	op.sensorControls = sensorCtrls;
>   
> +	ControlList lensCtrls;
> +	lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, lensPosition_);
> +
> +	op.lensControls = lensCtrls;
> +
>   	queueFrameAction.emit(frame, op);
>   }
>

Hi Umang,
Thanks for the review.

On Fri, Oct 29, 2021 at 2:19 AM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Hi Han-Lin,
>
> Thank you for the patch.
>
> Some high level comments.
>
> On 10/28/21 3:33 PM, Han-Lin Chen wrote:
> > Apply auto focus and send lens controls to pipeline handler.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com>
> > ---
> >   aiq/aiq.cpp |  5 ++++-
> >   aiq/aiq.h   |  2 +-
> >   ipu3.cpp    | 27 ++++++++++++++++++++++++++-
> >   3 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp
> > index 24c61cb..9c7f9d6 100644
> > --- a/aiq/aiq.cpp
> > +++ b/aiq/aiq.cpp
> > @@ -139,7 +139,7 @@ int AIQ::setStatistics(unsigned int frame,
> >    * of the parameter buffer being the only part handled when called for.
> >    */
> >   int AIQ::run2a(unsigned int frame, AiqInputParameters &params,
> > -            AiqResults &results)
> > +                  AiqResults &results, int lensPosition, int64_t lensMovementStartTime)
>
>
> Since we are now passing arguments to the algorithm's input parameters,
> I think we should be consider some refactoring in AiqInputParameters to
> accomodate our needs. One quick option I can think of, is to split and
> have setX(list of args) for (X = each algorithm's input parameters)
> functions and then call setDefaults() which shall mimick a
> setAeAwbAfDefaults() we have today (setDefaults() shall call setX() each
> with default args)
>
>
> > l
> >   {
> >       (void)frame;
> >
> > @@ -157,6 +157,9 @@ int AIQ::run2a(unsigned int frame, AiqInputParameters &params,
> >       params.saParams.awb_results = results.awb();
> >       shadingAdapterRun(params.saParams, results);
> >
> > +     params.afParams.lens_position = lensPosition;
> > +     params.afParams.lens_movement_start_timestamp = lensMovementStartTime;
> > +
>
>
> That's how I see setting these arguments for afParams as well, passing
>
>          setAfParams(...,  int lensPosition, int64_t lensMovementStartTime);
>
> Have you considered any such refactoring in design? I get it that this
> may well be a quick AF functionality driven patch to test the waters.
>

Thanks! This does sound great to me.
I think I will prepare another patch for the refactoring of
AiqInputParameters, since it's not only related to AF.

> >       afRun(params.afParams, results);
> >
> >       return 0;
> > diff --git a/aiq/aiq.h b/aiq/aiq.h
> > index fcd02d2..4f5f868 100644
> > --- a/aiq/aiq.h
> > +++ b/aiq/aiq.h
> > @@ -41,7 +41,7 @@ public:
> >                         const ipu3_uapi_stats_3a *stats);
> >
> >       int run2a(unsigned int frame, AiqInputParameters &params,
> > -               AiqResults &results);
> > +               AiqResults &results, int lensPosition, int64_t lensMovementStartTime);
> >
> >   private:
> >       std::string decodeError(ia_err err);
> > diff --git a/ipu3.cpp b/ipu3.cpp
> > index b7de901..45330ca 100644
> > --- a/ipu3.cpp
> > +++ b/ipu3.cpp
> > @@ -77,6 +77,10 @@ private:
> >       uint32_t gain_;
> >       uint32_t minGain_;
> >       uint32_t maxGain_;
> > +     int32_t lensPosition_;
> > +
> > +     /* Intel AF library relies on timestamp to wait for lens movement */
> > +     uint64_t lensMovementStartTime_;
> >
> >       /* Intel Library Instances. */
> >       aiq::AIQ aiq_;
> > @@ -258,6 +262,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >       maxGain_ = itGain->second.max().get<int32_t>();
> >       gain_ = maxGain_;
> >
> > +     lensMovementStartTime_ = 0;
> > +     lensPosition_ = 0;
> > +
> >       int ret;
> >
> >       ret = aiq_.configure();
> > @@ -381,7 +388,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >       */
> >
> >       /* Run algorithms into/using this context structure */
> > -     aiq_.run2a(frame, aiqInputParams_, results_);
> > +     aiq_.run2a(frame, aiqInputParams_, results_, lensPosition_, lensMovementStartTime_);
> >
> >       aic_.updateRuntimeParams(results_);
> >       aic_.run(params);
> > @@ -389,6 +396,19 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >       exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time;
> >       gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global;
> >
> > +     /*
> > +     * Af algorithm compares the timestamp of start of lens movement start and the
>
>
> s/*/ */  and all for comment lines below.
>
> Sentence is a bit confusing. I guess among the 2 'start', one should be
> removed to be read like:
>
>          * Af algorithm compares the timestamp of start of lens movement and
>
> > +     * that of the statistics generated to estimate whether next lens positon
> > +     * should be produced.
> > +     * Todo: Uses the lens movement start time reported by the pipeline handler.
>
>
> s/Todo: Uses / \todo: Use /
>
>
> > +     */
> > +     if (lensPosition_ != results_.af()->next_lens_position) {
> > +             utils::time_point time = utils::clock::now();
> > +             uint64_t msecs = std::chrono::duration_cast<std::chrono::microseconds>(time.time_since_epoch()).count();
> > +             lensMovementStartTime_ = msecs;
> > +     }
> > +     lensPosition_ = results_.af()->next_lens_position;
> > +
> >       resultsHistory_.Push(results_);
> >
> >       setControls(frame);
> > @@ -472,6 +492,11 @@ void IPAIPU3::setControls(unsigned int frame)
> >
> >       op.sensorControls = sensorCtrls;
> >
> > +     ControlList lensCtrls;
> > +     lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, lensPosition_);
> > +
> > +     op.lensControls = lensCtrls;
> > +
> >       queueFrameAction.emit(frame, op);
> >   }
> >