[PATCH v2 1/1] libcamera: software_isp: Actually apply black level from tuning data

[Stanislaw Gruszka]

On Tue, Dec 03, 2024 at 10:38:13AM +0100, Milan Zamazal wrote:
> The black level obtained from the tuning file in software ISP is
> retrieved in init (because this is the standard algorithm method with
> access to tuning data) and stored into context.  But the context gets
> reset in configure and the black level is lost and never applied.
> 
> Let's store the black level from the tuning file into an algorithm
> instance variable and put it into the context only later in configure.
> This is similar to what rkisp1 IPA does with the values obtained from
> the tuning file.
> 
> Fixes: 41e3d61c745153779ed5a38634d7266bffd60d61 ("libcamera: software_isp: Clear IPA context on configure and stop")
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>

> ---
>  src/ipa/simple/algorithms/blc.cpp | 7 +++++--
>  src/ipa/simple/algorithms/blc.h   | 4 ++++
>  src/ipa/simple/soft_simple.cpp    | 3 +--
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index b4e32fe1..1d7d370b 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -21,7 +21,8 @@ BlackLevel::BlackLevel()
>  {
>  }
>  
> -int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
> +int BlackLevel::init([[maybe_unused]] IPAContext &context,
> +		     const YamlObject &tuningData)
>  {
>  	auto blackLevel = tuningData["blackLevel"].get<int16_t>();
>  	if (blackLevel.has_value()) {
> @@ -29,7 +30,7 @@ int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
>  		 * Convert 16 bit values from the tuning file to 8 bit black
>  		 * level for the SoftISP.
>  		 */
> -		context.configuration.black.level = blackLevel.value() >> 8;
> +		definedLevel_ = blackLevel.value() >> 8;
>  	}
>  	return 0;
>  }
> @@ -37,6 +38,8 @@ int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
>  int BlackLevel::configure(IPAContext &context,
>  			  [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
> +	if (definedLevel_.has_value())
> +		context.configuration.black.level = definedLevel_;
>  	context.activeState.blc.level =
>  		context.configuration.black.level.value_or(255);
>  	return 0;
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> index 67c688ae..52d59cab 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -7,6 +7,9 @@
>  
>  #pragma once
>  
> +#include <optional>
> +#include <stdint.h>
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
> @@ -29,6 +32,7 @@ public:
>  private:
>  	int32_t exposure_;
>  	double gain_;
> +	std::optional<uint8_t> definedLevel_;
>  };
>  
>  } /* namespace ipa::soft::algorithms */
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index ba3d5265..e1b6d3af 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -206,8 +206,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>  			(context_.configuration.agc.againMax -
>  			 context_.configuration.agc.againMin) /
>  			100.0;
> -		if (!context_.configuration.black.level.has_value() &&
> -		    camHelper_->blackLevel().has_value()) {
> +		if (camHelper_->blackLevel().has_value()) {
>  			/*
>  			 * The black level from camHelper_ is a 16 bit value, software ISP
>  			 * works with 8 bit pixel values, both regardless of the actual
> -- 
> 2.44.2
>