[libcamera-devel,5/5] ipa: ipu3: af: Remove v4l2 interaction from AF algorithm
diff mbox series

Message ID 20211126003118.42356-6-djrscally@gmail.com
State Superseded
Headers show
Series
  • Enumerate CameraLens by following sensor's ancillary links
Related show

Commit Message

Daniel Scally Nov. 26, 2021, 12:31 a.m. UTC 
Now that the actual interaction with the VCM's V4L2 subdev can be
done through the CameraLens class, remove it from the auto focus
algorithm code.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 src/ipa/ipu3/algorithms/af.cpp | 29 +----------------------------
 src/ipa/ipu3/algorithms/af.h   |  3 ---
 2 files changed, 1 insertion(+), 31 deletions(-)

Comments

Laurent Pinchart Nov. 28, 2021, 10:24 p.m. UTC | #1 
Hi Daniel,

Thank you for the patch.

On Fri, Nov 26, 2021 at 12:31:18AM +0000, Daniel Scally wrote:
> Now that the actual interaction with the VCM's V4L2 subdev can be
> done through the CameraLens class, remove it from the auto focus
> algorithm code.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  src/ipa/ipu3/algorithms/af.cpp | 29 +----------------------------
>  src/ipa/ipu3/algorithms/af.h   |  3 ---
>  2 files changed, 1 insertion(+), 31 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index 2859a6a8..1bbf64aa 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -66,17 +66,10 @@ static constexpr double MaxChange_ = 0.8;
>  Af::Af()
>  	: focus_(0), currentVariance_(0.0)
>  {
> -	/**
> -	 * For surface Go 2 back camera VCM (dw9719)
> -	 * \todo move to control class
> -	*/
> -	vcmFd_ = open("/dev/v4l-subdev8", O_RDWR);
>  }
>  
>  Af::~Af()
>  {
> -	if (vcmFd_ != -1)
> -		close(vcmFd_);
>  }
>  
>  void Af::prepare(IPAContext &context, ipu3_uapi_params *params)
> @@ -169,25 +162,6 @@ int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &con
>  	return 0;
>  }
>  
> -/**
> - * \brief Send focus step to the VCM.
> - * \param[in] value Set lens position.
> - * \todo It is hardcoded here for the dw9717 VCM and will be moved to the
> - * subdev control in the future.
> - */
> -int Af::vcmSet(int value)
> -{
> -	int ret;
> -	struct v4l2_control ctrl;
> -	if (vcmFd_ == -1)
> -		return -EINVAL;
> -	memset(&ctrl, 0, sizeof(struct v4l2_control));
> -	ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;
> -	ctrl.value = value;
> -	ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);

We can probably drop some #include statements, for the headers used by
open() and ioctl().

The patch looks fine overall, I expect Kate will squash it into her
work.

> -	return ret;
> -}
> -
>  /**
>   * \brief Determine the max contrast image and lens position. y_table is the
>   * statictic data from IPU3 and is composed of low pass and high pass filtered
> @@ -263,10 +237,9 @@ void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  			if (focus_ > MaxFocusSteps_) {
>  				/* If reach the max step, move lens to the position and set "focus stable". */
>  				context.frameContext.af.stable = true;
> -				vcmSet(context.frameContext.af.focus);
>  			} else {
>  				focus_ += MinSearchStep_;
> -				vcmSet(focus_);
> +				context.frameContext.af.focus = focus_;
>  			}
>  			LOG(IPU3Af, Debug) << "Focus searching max variance is: "
>  					   << context.frameContext.af.maxVariance
> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> index d32d5184..b21084e9 100644
> --- a/src/ipa/ipu3/algorithms/af.h
> +++ b/src/ipa/ipu3/algorithms/af.h
> @@ -36,9 +36,6 @@ public:
>  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>  
>  private:
> -	int vcmSet(int value);
> -
> -	int vcmFd_;
>  	/* Used for focus scan. */
>  	uint32_t focus_;
>  	/* Recent AF statistic variance. */
Daniel Scally Nov. 28, 2021, 11:14 p.m. UTC | #2 
Hi Laurent

On 28/11/2021 22:24, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Fri, Nov 26, 2021 at 12:31:18AM +0000, Daniel Scally wrote:
>> Now that the actual interaction with the VCM's V4L2 subdev can be
>> done through the CameraLens class, remove it from the auto focus
>> algorithm code.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  src/ipa/ipu3/algorithms/af.cpp | 29 +----------------------------
>>  src/ipa/ipu3/algorithms/af.h   |  3 ---
>>  2 files changed, 1 insertion(+), 31 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
>> index 2859a6a8..1bbf64aa 100644
>> --- a/src/ipa/ipu3/algorithms/af.cpp
>> +++ b/src/ipa/ipu3/algorithms/af.cpp
>> @@ -66,17 +66,10 @@ static constexpr double MaxChange_ = 0.8;
>>  Af::Af()
>>  	: focus_(0), currentVariance_(0.0)
>>  {
>> -	/**
>> -	 * For surface Go 2 back camera VCM (dw9719)
>> -	 * \todo move to control class
>> -	*/
>> -	vcmFd_ = open("/dev/v4l-subdev8", O_RDWR);
>>  }
>>  
>>  Af::~Af()
>>  {
>> -	if (vcmFd_ != -1)
>> -		close(vcmFd_);
>>  }
>>  
>>  void Af::prepare(IPAContext &context, ipu3_uapi_params *params)
>> @@ -169,25 +162,6 @@ int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &con
>>  	return 0;
>>  }
>>  
>> -/**
>> - * \brief Send focus step to the VCM.
>> - * \param[in] value Set lens position.
>> - * \todo It is hardcoded here for the dw9717 VCM and will be moved to the
>> - * subdev control in the future.
>> - */
>> -int Af::vcmSet(int value)
>> -{
>> -	int ret;
>> -	struct v4l2_control ctrl;
>> -	if (vcmFd_ == -1)
>> -		return -EINVAL;
>> -	memset(&ctrl, 0, sizeof(struct v4l2_control));
>> -	ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;
>> -	ctrl.value = value;
>> -	ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);
> We can probably drop some #include statements, for the headers used by
> open() and ioctl().
>
> The patch looks fine overall, I expect Kate will squash it into her
> work.


That was my thought too, as long as Kate's ok with that

>
>> -	return ret;
>> -}
>> -
>>  /**
>>   * \brief Determine the max contrast image and lens position. y_table is the
>>   * statictic data from IPU3 and is composed of low pass and high pass filtered
>> @@ -263,10 +237,9 @@ void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>>  			if (focus_ > MaxFocusSteps_) {
>>  				/* If reach the max step, move lens to the position and set "focus stable". */
>>  				context.frameContext.af.stable = true;
>> -				vcmSet(context.frameContext.af.focus);
>>  			} else {
>>  				focus_ += MinSearchStep_;
>> -				vcmSet(focus_);
>> +				context.frameContext.af.focus = focus_;
>>  			}
>>  			LOG(IPU3Af, Debug) << "Focus searching max variance is: "
>>  					   << context.frameContext.af.maxVariance
>> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
>> index d32d5184..b21084e9 100644
>> --- a/src/ipa/ipu3/algorithms/af.h
>> +++ b/src/ipa/ipu3/algorithms/af.h
>> @@ -36,9 +36,6 @@ public:
>>  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>  
>>  private:
>> -	int vcmSet(int value);
>> -
>> -	int vcmFd_;
>>  	/* Used for focus scan. */
>>  	uint32_t focus_;
>>  	/* Recent AF statistic variance. */
Kate Hsuan Nov. 29, 2021, 2:04 a.m. UTC | #3 
Hi Daniel,

On Mon, Nov 29, 2021 at 7:14 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> Hi Laurent
>
> On 28/11/2021 22:24, Laurent Pinchart wrote:
> > Hi Daniel,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 26, 2021 at 12:31:18AM +0000, Daniel Scally wrote:
> >> Now that the actual interaction with the VCM's V4L2 subdev can be
> >> done through the CameraLens class, remove it from the auto focus
> >> algorithm code.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >>  src/ipa/ipu3/algorithms/af.cpp | 29 +----------------------------
> >>  src/ipa/ipu3/algorithms/af.h   |  3 ---
> >>  2 files changed, 1 insertion(+), 31 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> >> index 2859a6a8..1bbf64aa 100644
> >> --- a/src/ipa/ipu3/algorithms/af.cpp
> >> +++ b/src/ipa/ipu3/algorithms/af.cpp
> >> @@ -66,17 +66,10 @@ static constexpr double MaxChange_ = 0.8;
> >>  Af::Af()
> >>      : focus_(0), currentVariance_(0.0)
> >>  {
> >> -    /**
> >> -     * For surface Go 2 back camera VCM (dw9719)
> >> -     * \todo move to control class
> >> -    */
> >> -    vcmFd_ = open("/dev/v4l-subdev8", O_RDWR);
> >>  }
> >>
> >>  Af::~Af()
> >>  {
> >> -    if (vcmFd_ != -1)
> >> -            close(vcmFd_);
> >>  }
> >>
> >>  void Af::prepare(IPAContext &context, ipu3_uapi_params *params)
> >> @@ -169,25 +162,6 @@ int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &con
> >>      return 0;
> >>  }
> >>
> >> -/**
> >> - * \brief Send focus step to the VCM.
> >> - * \param[in] value Set lens position.
> >> - * \todo It is hardcoded here for the dw9717 VCM and will be moved to the
> >> - * subdev control in the future.
> >> - */
> >> -int Af::vcmSet(int value)
> >> -{
> >> -    int ret;
> >> -    struct v4l2_control ctrl;
> >> -    if (vcmFd_ == -1)
> >> -            return -EINVAL;
> >> -    memset(&ctrl, 0, sizeof(struct v4l2_control));
> >> -    ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;
> >> -    ctrl.value = value;
> >> -    ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);
> > We can probably drop some #include statements, for the headers used by
> > open() and ioctl().
> >
> > The patch looks fine overall, I expect Kate will squash it into her
> > work.
>
>
> That was my thought too, as long as Kate's ok with that
>
> >
> >> -    return ret;
> >> -}
> >> -
> >>  /**
> >>   * \brief Determine the max contrast image and lens position. y_table is the
> >>   * statictic data from IPU3 and is composed of low pass and high pass filtered
> >> @@ -263,10 +237,9 @@ void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >>                      if (focus_ > MaxFocusSteps_) {
> >>                              /* If reach the max step, move lens to the position and set "focus stable". */
> >>                              context.frameContext.af.stable = true;
> >> -                            vcmSet(context.frameContext.af.focus);
> >>                      } else {
> >>                              focus_ += MinSearchStep_;
> >> -                            vcmSet(focus_);
> >> +                            context.frameContext.af.focus = focus_;
> >>                      }
> >>                      LOG(IPU3Af, Debug) << "Focus searching max variance is: "
> >>                                         << context.frameContext.af.maxVariance
> >> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> >> index d32d5184..b21084e9 100644
> >> --- a/src/ipa/ipu3/algorithms/af.h
> >> +++ b/src/ipa/ipu3/algorithms/af.h
> >> @@ -36,9 +36,6 @@ public:
> >>      void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> >>
> >>  private:
> >> -    int vcmSet(int value);
> >> -
> >> -    int vcmFd_;
> >>      /* Used for focus scan. */
> >>      uint32_t focus_;
> >>      /* Recent AF statistic variance. */
>

It's good to me :)

Reviewed-by: Kate Hsuan <hpa@redhat.com>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
index 2859a6a8..1bbf64aa 100644
--- a/src/ipa/ipu3/algorithms/af.cpp
+++ b/src/ipa/ipu3/algorithms/af.cpp
@@ -66,17 +66,10 @@  static constexpr double MaxChange_ = 0.8;
 Af::Af()
 	: focus_(0), currentVariance_(0.0)
 {
-	/**
-	 * For surface Go 2 back camera VCM (dw9719)
-	 * \todo move to control class
-	*/
-	vcmFd_ = open("/dev/v4l-subdev8", O_RDWR);
 }
 
 Af::~Af()
 {
-	if (vcmFd_ != -1)
-		close(vcmFd_);
 }
 
 void Af::prepare(IPAContext &context, ipu3_uapi_params *params)
@@ -169,25 +162,6 @@  int Af::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &con
 	return 0;
 }
 
-/**
- * \brief Send focus step to the VCM.
- * \param[in] value Set lens position.
- * \todo It is hardcoded here for the dw9717 VCM and will be moved to the
- * subdev control in the future.
- */
-int Af::vcmSet(int value)
-{
-	int ret;
-	struct v4l2_control ctrl;
-	if (vcmFd_ == -1)
-		return -EINVAL;
-	memset(&ctrl, 0, sizeof(struct v4l2_control));
-	ctrl.id = V4L2_CID_FOCUS_ABSOLUTE;
-	ctrl.value = value;
-	ret = ioctl(vcmFd_, VIDIOC_S_CTRL, &ctrl);
-	return ret;
-}
-
 /**
  * \brief Determine the max contrast image and lens position. y_table is the
  * statictic data from IPU3 and is composed of low pass and high pass filtered
@@ -263,10 +237,9 @@  void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
 			if (focus_ > MaxFocusSteps_) {
 				/* If reach the max step, move lens to the position and set "focus stable". */
 				context.frameContext.af.stable = true;
-				vcmSet(context.frameContext.af.focus);
 			} else {
 				focus_ += MinSearchStep_;
-				vcmSet(focus_);
+				context.frameContext.af.focus = focus_;
 			}
 			LOG(IPU3Af, Debug) << "Focus searching max variance is: "
 					   << context.frameContext.af.maxVariance
diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
index d32d5184..b21084e9 100644
--- a/src/ipa/ipu3/algorithms/af.h
+++ b/src/ipa/ipu3/algorithms/af.h
@@ -36,9 +36,6 @@  public:
 	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
 
 private:
-	int vcmSet(int value);
-
-	int vcmFd_;
 	/* Used for focus scan. */
 	uint32_t focus_;
 	/* Recent AF statistic variance. */