[libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support raw capture

[Jacopo Mondi]

Hi Laurent

On Mon, Oct 24, 2022 at 03:03:48AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Support raw capture by allowing manual control of the exposure time and
> analogue gain.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 51 ++++++++++++++++++++-----------
>  src/ipa/rkisp1/algorithms/agc.h   |  2 ++
>  2 files changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 377a3e8a0efd..973965babed2 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -62,6 +62,7 @@ static constexpr double kRelativeLuminanceTarget = 0.4;
>  Agc::Agc()
>  	: frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s)
>  {
> +	supportsRaw_ = true;
>  }
>
>  /**
> @@ -81,7 +82,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  		10ms / context.configuration.sensor.lineDuration;
>  	context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
>  	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> -	context.activeState.agc.autoEnabled = true;
> +	context.activeState.agc.autoEnabled = !context.configuration.raw;
>
>  	/*
>  	 * According to the RkISP1 documentation:
> @@ -123,12 +124,14 @@ void Agc::queueRequest(IPAContext &context,
>  {
>  	auto &agc = context.activeState.agc;
>
> -	const auto &agcEnable = controls.get(controls::AeEnable);
> -	if (agcEnable && *agcEnable != agc.autoEnabled) {
> -		agc.autoEnabled = *agcEnable;
> +	if (!context.configuration.raw) {

I wonder if we shouldn't complain loud when the application tries to
enable AutoExposure with a RAW configuration..

> +		const auto &agcEnable = controls.get(controls::AeEnable);
> +		if (agcEnable && *agcEnable != agc.autoEnabled) {
> +			agc.autoEnabled = *agcEnable;
>
> -		LOG(RkISP1Agc, Debug)
> -			<< (*agcEnable ? "Enabling" : "Disabling") << " AGC";
> +			LOG(RkISP1Agc, Debug)
> +				<< (*agcEnable ? "Enabling" : "Disabling") << " AGC";
> +		}
>  	}
>
>  	const auto &exposure = controls.get(controls::ExposureTime);
> @@ -160,6 +163,9 @@ void Agc::queueRequest(IPAContext &context,
>  void Agc::prepare(IPAContext &context, const uint32_t frame,
>  		  IPAFrameContext &frameContext, rkisp1_params_cfg *params)
>  {
> +	if (!params)
> +		return;
> +

I might have missed where params is set to nullptr in the IPA module.


void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
{
	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
	rkisp1_params_cfg *params =
		reinterpret_cast<rkisp1_params_cfg *>(
			mappedBuffers_.at(bufferId).planes()[0].data());

	/* Prepare parameters buffer. */
	memset(params, 0, sizeof(*params));

	for (auto const &algo : algorithms())
		algo->prepare(context_, frame, frameContext, params);

Does 'params' need to be treated as 'stats' in
IPARkISP1::processStatsBuffer() (ie set to nullptr if
configuration.raw) ?


>  	if (frameContext.agc.autoEnabled) {
>  		frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
>  		frameContext.agc.gain = context.activeState.agc.automatic.gain;
> @@ -367,6 +373,22 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
>  	return histogram.interQuantileMean(0.98, 1.0);
>  }
>
> +void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> +		       ControlList &metadata)
> +{
> +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> +				     * frameContext.sensor.exposure;
> +	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> +	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> +
> +	/* \todo Use VBlank value calculated from each frame exposure. */
> +	uint32_t vTotal = context.configuration.sensor.size.height
> +			+ context.configuration.sensor.defVBlank;
> +	utils::Duration frameDuration = context.configuration.sensor.lineDuration
> +				      * vTotal;
> +	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> +}
> +
>  /**
>   * \brief Process RkISP1 statistics, and run AGC operations
>   * \param[in] context The shared IPA context
> @@ -382,6 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  		  IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats,
>  		  ControlList &metadata)
>  {
> +	if (!stats) {
> +		fillMetadata(context, frameContext, metadata);
> +		return;
> +	}
> +
>  	/*
>  	 * \todo Verify that the exposure and gain applied by the sensor for
>  	 * this frame match what has been requested. This isn't a hard
> @@ -424,17 +451,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	computeExposure(context, frameContext, yGain, iqMeanGain);
>  	frameCount_++;
>
> -	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> -				     * frameContext.sensor.exposure;
> -	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> -	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> -
> -	/* \todo Use VBlank value calculated from each frame exposure. */
> -	uint32_t vTotal = context.configuration.sensor.size.height
> -			+ context.configuration.sensor.defVBlank;
> -	utils::Duration frameDuration = context.configuration.sensor.lineDuration
> -				      * vTotal;
> -	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> +	fillMetadata(context, frameContext, metadata);
>  }
>
>  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index a228d0c37768..8a22263741b6 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -44,6 +44,8 @@ private:
>  	utils::Duration filterExposure(utils::Duration exposureValue);
>  	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
>  	double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;
> +	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> +			  ControlList &metadata);
>
>  	uint64_t frameCount_;
>
> --
> Regards,
>
> Laurent Pinchart
>