[libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits to the session config

[Jacopo Mondi]

Hi Daniel

On Fri, Mar 31, 2023 at 10:19:22AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> Add information about focus lens position limits to the IPA session
> configuration. These information can then be used by IPA algorithms
> to know which focus positions are valid.
>
> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> ---
>  src/ipa/rkisp1/ipa_context.cpp | 17 +++++++++++++++++
>  src/ipa/rkisp1/ipa_context.h   |  9 +++++++++
>  src/ipa/rkisp1/rkisp1.cpp      | 23 ++++++++++++++++++++++-
>  3 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 9bbf3684..aea99299 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -14,6 +14,18 @@
>
>  namespace libcamera::ipa::rkisp1 {
>
> +/**
> + * \struct LensConfiguration
> + * \brief Lens-specific parameters
> + *
> + * \var LensConfiguration::minFocusPosition
> + * \brief Minimum position supported by the camera focus lens
> + *
> + * \var LensConfiguration::maxFocusPosition
> + * \brief Maximum position supported by the camera focus lens
> + *
> + */
> +
>  /**
>   * \struct IPASessionConfiguration
>   * \brief Session configuration for the IPA module
> @@ -89,6 +101,11 @@ namespace libcamera::ipa::rkisp1 {
>   * \brief Sensor output resolution
>   */
>
> +/**
> + * \var IPASessionConfiguration::lens
> + * \brief Contains lens-specific parameters if lens was detected
> + */
> +
>  /**
>   * \var IPASessionConfiguration::raw
>   * \brief Indicates if the camera is configured to capture raw frames
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index b9b20653..65b3fbfe 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -8,6 +8,8 @@
>
>  #pragma once
>
> +#include <optional>
> +
>  #include <linux/rkisp1-config.h>
>
>  #include <libcamera/base/utils.h>
> @@ -20,6 +22,11 @@ namespace libcamera {
>
>  namespace ipa::rkisp1 {
>
> +struct LensConfiguration {
> +	int32_t minFocusPosition = 0;
> +	int32_t maxFocusPosition = 0;
> +};
> +
>  struct IPASessionConfiguration {
>  	struct {
>  		struct rkisp1_cif_isp_window measureWindow;
> @@ -45,6 +52,8 @@ struct IPASessionConfiguration {
>  		Size size;
>  	} sensor;
>
> +	std::optional<LensConfiguration> lens;
> +
>  	struct {
>  		rkisp1_cif_isp_version revision;
>  	} hw;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index d338d374..292768cf 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -164,9 +164,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  	context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
>  						   * 1.0s / sensorInfo.pixelRate;
>
> -	if (!lensControls.empty())
> +	if (!lensControls.empty()) {
>  		lensControls_ = lensControls;
>
> +		const ControlInfo &focusAbsolute =
> +			lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);
> +
> +		LOG(IPARkISP1, Debug)
> +			<< "Focus lens: " << focusAbsolute.toString();
> +
> +		context_.configuration.lens = {
> +			.minFocusPosition = focusAbsolute.min().get<int32_t>(),
> +			.maxFocusPosition = focusAbsolute.max().get<int32_t>()
> +		};

Do you need to initialize context_.configuration.lens at init() time ?

It doesn't seem so to me as in updateControls() below you anyway
re-fetch V4L2_CID_FOCUS_ABSOLUTE from lensControls_.

Can't it be done in configure() to avoid ...

> +	}
> +
>  	/* Load the tuning data file. */
>  	File file(settings.configurationFile);
>  	if (!file.open(File::OpenModeFlag::ReadOnly)) {
> @@ -234,6 +246,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>  		<< "Exposure: [" << minExposure << ", " << maxExposure
>  		<< "], gain: [" << minGain << ", " << maxGain << "]";
>
> +	/*
> +	 * Save the existing lens configuration and restore it after context
> +	 * reset. It does not change since init().
> +	 */
> +	const std::optional<LensConfiguration> lensConfig =
> +		context_.configuration.lens;
> +

... this ?

The computation seems very cheap and camera configuration happens once
per streaming session, so I wonder if the it wouldn't be better to
maintain a cleaner coding patch at the expense of this little cost.

The rest looks good!


>  	/* Clear the IPA context before the streaming session. */
>  	context_.configuration = {};
>  	context_.activeState = {};
> @@ -272,6 +291,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>  			return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>  		});
>
> +	context_.configuration.lens = lensConfig;
> +
>  	for (auto const &a : algorithms()) {
>  		Algorithm *algo = static_cast<Algorithm *>(a.get());
>
> --
> 2.39.2
>