[libcamera-devel] [PATCH 2/3] ipa: rkisp1: lsc: Enable/disable LSC algorithm

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 08, 2023 at 05:40:27PM +0100, Jacopo Mondi via libcamera-devel wrote:
> Implement LSC algorithm enable/disable by parsing the
> "LensShadingEnable" control in queueRequest().

I'll assume here that we have use cases for this control, that question
is better discussed in replies to 1/3.

> Start with the LSC algorithm enabled by default and disable it
> on application request.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/lsc.cpp | 54 ++++++++++++++++++++++++++++++-
>  src/ipa/rkisp1/algorithms/lsc.h   |  4 +++
>  src/ipa/rkisp1/ipa_context.h      |  5 +++
>  src/ipa/rkisp1/rkisp1.cpp         |  1 +
>  4 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index a7ccedb1ed3b..0c9f6cbd1dec 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -14,6 +14,8 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
>  
> +#include <libcamera/control_ids.h>
> +
>  #include "libcamera/internal/yaml_parser.h"
>  
>  #include "linux/rkisp1-config.h"
> @@ -147,6 +149,8 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>  		return -EINVAL;
>  	}
>  
> +	context.configuration.lsc.enabled = false;
> +
>  	return 0;
>  }
>  
> @@ -181,10 +185,20 @@ int LensShadingCorrection::configure(IPAContext &context,
>  		yGrad_[i] = std::round(32768 / ySizes_[i]);
>  	}
>  
> -	context.configuration.lsc.enabled = true;
> +	/* Enable LSC on first run unless explicitly disabled by application. */
> +	context.activeState.lsc.enable = true;
> +	context.activeState.lsc.active = false;
> +
>  	return 0;
>  }
>  
> +void LensShadingCorrection::disableLSC(rkisp1_params_cfg *params)
> +{
> +	params->module_en_update |= RKISP1_CIF_ISP_MODULE_LSC;
> +	params->module_ens &= ~RKISP1_CIF_ISP_MODULE_LSC;
> +	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_LSC;
> +}
> +
>  void LensShadingCorrection::setParameters(rkisp1_params_cfg *params)
>  {
>  	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
> @@ -242,6 +256,25 @@ void LensShadingCorrection::interpolateTable(rkisp1_cif_isp_lsc_config &config,
>  	}
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::queueRequest
> + */
> +void LensShadingCorrection::queueRequest(IPAContext &context,
> +					 [[maybe_unused]] const uint32_t frame,
> +					 [[maybe_unused]] IPAFrameContext &frameContext,
> +					 const ControlList &controls)
> +{
> +	auto lsc = &context.activeState.lsc;
> +
> +	const auto lscEnable = controls.get(controls::LensShadingEnable);
> +	if (lscEnable && *lscEnable != lsc->enable) {
> +		LOG(RkISP1Lsc, Debug)
> +			<< (*lscEnable ? "Enabling" : "Disabling") << " LSC";
> +
> +		lsc->enable = *lscEnable;
> +	}
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> @@ -250,6 +283,25 @@ void LensShadingCorrection::prepare(IPAContext &context,
>  				    [[maybe_unused]] IPAFrameContext &frameContext,
>  				    rkisp1_params_cfg *params)
>  {
> +	auto lsc = &context.activeState.lsc;

This doesn't look right, you will apply the LSC enable control value of
the very last request that was queued too early. You need to use the
frame context.

> +
> +	if (lsc->enable != lsc->active) {
> +		/* If LSC has to be disabled, disable it and return here. */
> +		if (!lsc->enable) {
> +			disableLSC(params);
> +			lsc->active = false;
> +			context.configuration.lsc.enabled = false;
> +			return;
> +		}
> +
> +		lsc->active = true;
> +		context.configuration.lsc.enabled = true;
> +	}
> +
> +	/* Nothing more to do here if LSC is not active. */
> +	if (!lsc->active)
> +		return;
> +
>  	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
>  
>  	/*
> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> index e2a93a566973..4708065bcfb7 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.h
> +++ b/src/ipa/rkisp1/algorithms/lsc.h
> @@ -23,6 +23,9 @@ public:
>  
>  	int init(IPAContext &context, const YamlObject &tuningData) override;
>  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> +	void queueRequest(IPAContext &context, const uint32_t frame,
> +			  IPAFrameContext &frameContext,
> +			  const ControlList &controls) override;
>  	void prepare(IPAContext &context, const uint32_t frame,
>  		     IPAFrameContext &frameContext,
>  		     rkisp1_params_cfg *params) override;
> @@ -36,6 +39,7 @@ private:
>  		std::vector<uint16_t> b;
>  	};
>  
> +	void disableLSC(rkisp1_params_cfg *params);
>  	void setParameters(rkisp1_params_cfg *params);
>  	void copyTable(rkisp1_cif_isp_lsc_config &config, const Components &set0);
>  	void interpolateTable(rkisp1_cif_isp_lsc_config &config,
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index b9b2065328d6..ada995274ebf 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -98,6 +98,11 @@ struct IPAActiveState {
>  		uint8_t denoise;
>  		uint8_t sharpness;
>  	} filter;
> +
> +	struct {
> +		bool enable;
> +		bool active;
> +	} lsc;
>  };
>  
>  struct IPAFrameContext : public FrameContext {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 6544c925ba25..2772592eda66 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -106,6 +106,7 @@ const ControlInfoMap::Map rkisp1Controls{
>  	{ &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> +	{ &controls::LensShadingEnable, ControlInfo(false, true) },

Shouldn't the default control value be true ?

>  };
>  
>  } /* namespace */

-- 
Regards,

Laurent Pinchart