[libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to PH and expose it to the IPA

[Michael Riesch]

Hi Daniel,

On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote:
> Extend the IPA init() function by additional lensControls input
> parameter. Check in pipeline handler if camera lens exists, and expose
> its controls to the IPA using the new parameter.
> 
> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> ---
>  include/libcamera/ipa/rkisp1.mojom       | 3 ++-
>  src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 1009e970..d4ff1230 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {
>  	init(libcamera.IPASettings settings,
>  	     uint32 hwRevision,
>  	     libcamera.IPACameraSensorInfo sensorInfo,
> -	     libcamera.ControlInfoMap sensorControls)
> +	     libcamera.ControlInfoMap sensorControls,
> +	     libcamera.ControlInfoMap lensControls)

One could imagine that in future other subdevices are supported. For
example, I expect that soon we'll have a "flashControls" parameter.
Would it make sense to gather all those controls in a single parameter
"hwControls" (name to be bikeshedded)? (In #libcamera the notion of
internal controls has been introduced recently. Maybe simply
"internalControls"?)

BTW I also could imagine that a "lensInfo" parameter is necessary for
complex lens controllers (e.g., does the lens have a zoom function, how
many zoom lenses does it have, model of the lens controller, ...). But
for simple VCM controllers this may not be necessary, so feel free to
ignore this at this point.

>  		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
>  	start() => (int32 ret);
>  	stop();

Shouldn't processStatsBuffer have an additional "lensControls"
parameter? Of course, the comment above would apply here as well and we
could get away with simply adjusting the name "sensorControls" ->
"internalControls".

Best regards,
Michael

> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 6544c925..d338d374 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -7,6 +7,7 @@
>  
>  #include <algorithm>
>  #include <math.h>
> +#include <optional>
>  #include <queue>
>  #include <stdint.h>
>  #include <string.h>
> @@ -52,6 +53,7 @@ public:
>  	int init(const IPASettings &settings, unsigned int hwRevision,
>  		 const IPACameraSensorInfo &sensorInfo,
>  		 const ControlInfoMap &sensorControls,
> +		 const ControlInfoMap &lensControls,
>  		 ControlInfoMap *ipaControls) override;
>  	int start() override;
>  	void stop() override;
> @@ -80,6 +82,7 @@ private:
>  	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>  
>  	ControlInfoMap sensorControls_;
> +	std::optional<ControlInfoMap> lensControls_;
>  
>  	/* revision-specific data */
>  	rkisp1_cif_isp_version hwRevision_;
> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const
>  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  		    const IPACameraSensorInfo &sensorInfo,
>  		    const ControlInfoMap &sensorControls,
> +		    const ControlInfoMap &lensControls,
>  		    ControlInfoMap *ipaControls)
>  {
>  	/* \todo Add support for other revisions */
> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  	context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
>  						   * 1.0s / sensorInfo.pixelRate;
>  
> +	if (!lensControls.empty())
> +		lensControls_ = lensControls;
> +
>  	/* Load the tuning data file. */
>  	File file(settings.configurationFile);
>  	if (!file.open(File::OpenModeFlag::ReadOnly)) {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8a30fe06..f966254a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -32,6 +32,7 @@
>  #include <libcamera/ipa/rkisp1_ipa_proxy.h>
>  
>  #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/camera_lens.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>  		return ret;
>  	}
>  
> +	ControlInfoMap lensControls;
> +	CameraLens *lens = sensor_->focusLens();
> +	if (lens)
> +		lensControls = lens->controls();
> +
>  	ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> -			 sensorInfo, sensor_->controls(), &controlInfo_);
> +			 sensorInfo, sensor_->controls(), lensControls,
> +			 &controlInfo_);
>  	if (ret < 0) {
>  		LOG(RkISP1, Error) << "IPA initialization failure";
>  		return ret;