[RFC,v2,20/43] ipa: Simplify sensor exposure/gain setting/getting
diff mbox series

Message ID 20260723154327.1357866-21-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: libipa: agc rework
Related show

Commit Message

Barnabás Pőcze July 23, 2026, 3:43 p.m. UTC 
Move the extraction and preparation of `V4L2_CID_{EXPOSURE,ANALOGUE_GAIN}`
into separate functions. This also implements support for not having a
a camera sensor helper.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp          | 14 +++++-----
 src/ipa/libipa/agc.h           | 49 ++++++++++++++++++++++++++++++++++
 src/ipa/libipa/meson.build     |  1 +
 src/ipa/mali-c55/mali-c55.cpp  | 17 +++++-------
 src/ipa/rkisp1/rkisp1.cpp      | 11 +++-----
 src/ipa/simple/soft_simple.cpp | 15 +++--------
 6 files changed, 72 insertions(+), 35 deletions(-)
 create mode 100644 src/ipa/libipa/agc.h

Comments

Jacopo Mondi July 24, 2026, 1:35 p.m. UTC | #1 
Hi Barnabás

On Thu, Jul 23, 2026 at 05:43:03PM +0200, Barnabás Pőcze wrote:
> Move the extraction and preparation of `V4L2_CID_{EXPOSURE,ANALOGUE_GAIN}`
> into separate functions. This also implements support for not having a
> a camera sensor helper.

I only checked the next patch, but these two helpers seems to not be
designed to become part of the AgcAlgorithm class

If that's the case, and they will stay helpers for the rest of the
series, I'm not 100% sure it's worth it.

Each helper saves a few lines, and for such reduced gain, I'm think
it's more clear for the reader to see what happens instead of having
to go and check what a function does.

Do these helpers have any other use along the rest of the series ?

>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp          | 14 +++++-----
>  src/ipa/libipa/agc.h           | 49 ++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/meson.build     |  1 +
>  src/ipa/mali-c55/mali-c55.cpp  | 17 +++++-------
>  src/ipa/rkisp1/rkisp1.cpp      | 11 +++-----
>  src/ipa/simple/soft_simple.cpp | 15 +++--------
>  6 files changed, 72 insertions(+), 35 deletions(-)
>  create mode 100644 src/ipa/libipa/agc.h
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 4bdc4b7677..51379808cd 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -36,6 +36,7 @@
>  #include "libcamera/internal/mapped_framebuffer.h"
>  #include "libcamera/internal/yaml_parser.h"
>
> +#include "libipa/agc.h"
>  #include "libipa/camera_sensor_helper.h"
>
>  #include "ipa_context.h"
> @@ -596,8 +597,8 @@ void IPAIPU3::processStats(const uint32_t frame,
>
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>
> -	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
> +		= agc::extractControls(sensorControls, camHelper_.get());
>
>  	ControlList metadata(controls::controls);
>
> @@ -642,12 +643,11 @@ void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>   */
>  void IPAIPU3::setControls(unsigned int frame)
>  {
> -	int32_t exposure = context_.activeState.agc.exposure;
> -	int32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
> -
>  	ControlList ctrls(sensorCtrls_);
> -	ctrls.set(V4L2_CID_EXPOSURE, exposure);
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);
> +	agc::prepareControls(
> +		ctrls, camHelper_.get(),
> +		context_.activeState.agc.exposure, context_.activeState.agc.gain
> +	);
>
>  	ControlList lensCtrls(lensCtrls_);
>  	lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> new file mode 100644
> index 0000000000..5247425952
> --- /dev/null
> +++ b/src/ipa/libipa/agc.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2026 Ideas on Board Oy
> + *
> + * AGC-related functionality
> + */
> +
> +#pragma once
> +
> +#include <utility>
> +
> +#include <linux/v4l2-controls.h>
> +
> +#include <libcamera/controls.h>
> +
> +#include "camera_sensor_helper.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +namespace agc {
> +
> +[[nodiscard]]
> +inline std::pair<uint32_t, double>
> +extractControls(const ControlList &controls, const CameraSensorHelper *sensor)
> +{
> +	auto exposure = controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +	auto gainCode = controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +
> +	return {
> +		uint32_t(exposure),
> +		sensor ? sensor->gain(gainCode) : gainCode,
> +	};
> +}
> +
> +inline void
> +prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
> +		int32_t exposure, double gain)
> +{
> +	controls.set(V4L2_CID_EXPOSURE, exposure);
> +	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
> +}
> +
> +} /* namespace agc */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 963c5ee730..05f1a8749c 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>
>  libipa_headers = files([
> +    'agc.h',
>      'agc_mean_luminance.h',
>      'algorithm.h',
>      'awb_bayes.h',
> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
> index 1d3af0627f..65e5297766 100644
> --- a/src/ipa/mali-c55/mali-c55.cpp
> +++ b/src/ipa/mali-c55/mali-c55.cpp
> @@ -27,6 +27,7 @@
>  #include "libcamera/internal/yaml_parser.h"
>
>  #include "algorithms/algorithm.h"
> +#include "libipa/agc.h"
>  #include "libipa/camera_sensor_helper.h"
>
>  #include "ipa_context.h"
> @@ -141,20 +142,18 @@ void IPAMaliC55::setControls()
>  {
>  	IPAActiveState &activeState = context_.activeState;
>  	uint32_t exposure;
> -	uint32_t gain;
> +	double gain;
>
>  	if (activeState.agc.autoEnabled) {
>  		exposure = activeState.agc.automatic.exposure;
> -		gain = camHelper_->gainCode(activeState.agc.automatic.sensorGain);
> +		gain = activeState.agc.automatic.sensorGain;
>  	} else {
>  		exposure = activeState.agc.manual.exposure;
> -		gain = camHelper_->gainCode(activeState.agc.manual.sensorGain);
> +		gain = activeState.agc.manual.sensorGain;
>  	}
>
>  	ControlList ctrls(sensorControls_);
> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> -
> +	agc::prepareControls(ctrls, camHelper_.get(), exposure, gain);
>  	setSensorControls.emit(ctrls);
>  }
>
> @@ -352,10 +351,8 @@ void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,
>  	stats = reinterpret_cast<mali_c55_stats_buffer *>(
>  		buffers_.at(bufferId).planes()[0].data());
>
> -	frameContext.agc.exposure =
> -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	frameContext.agc.sensorGain =
> -		camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +	std::tie(frameContext.agc.exposure, frameContext.agc.sensorGain)
> +		= agc::extractControls(sensorControls, camHelper_.get());
>
>  	ControlList metadata(controls::controls);
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 38f55b1d86..acb878d814 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -31,6 +31,7 @@
>  #include "libcamera/internal/yaml_parser.h"
>
>  #include "algorithms/algorithm.h"
> +#include "libipa/agc.h"
>
>  #include "ipa_context.h"
>  #include "params.h"
> @@ -328,10 +329,8 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
>  		stats = reinterpret_cast<rkisp1_stat_buffer *>(
>  			mappedBuffers_.at(bufferId).planes()[0].data());
>
> -	frameContext.sensor.exposure =
> -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	frameContext.sensor.gain =
> -		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
> +		= agc::extractControls(sensorControls, context_.camHelper.get());
>
>  	ControlList metadata(controls::controls);
>
> @@ -365,7 +364,6 @@ void IPARkISP1::setControls(unsigned int frame)
>
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>  	uint32_t exposure = frameContext.agc.exposure;
> -	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
>  	uint32_t vblank = frameContext.agc.vblank;
>
>  	LOG(IPARkISP1, Debug)
> @@ -373,8 +371,7 @@ void IPARkISP1::setControls(unsigned int frame)
>  		<< ", gain " << frameContext.agc.gain << ", vblank " << vblank;
>
>  	ControlList ctrls(context_.sensorControls);
> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> +	agc::prepareControls(ctrls, context_.camHelper.get(), exposure, frameContext.agc.gain);
>  	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
>
>  	setSensorControls.emit(frame, ctrls);
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 3932daaa1f..a38fcff7d8 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -27,6 +27,7 @@
>  #include "libcamera/internal/yaml_parser.h"
>
>  #include "algorithms/adjust.h"
> +#include "libipa/agc.h"
>  #include "libipa/camera_sensor_helper.h"
>
>  #include "module.h"
> @@ -301,10 +302,8 @@ void IPASoftSimple::processStats(const uint32_t frame,
>  {
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>
> -	frameContext.sensor.exposure =
> -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> -	frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;
> +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
> +		= agc::extractControls(sensorControls, camHelper_.get());
>
>  	ControlList metadata(controls::controls);
>  	for (const auto &algo : algorithms())
> @@ -312,13 +311,7 @@ void IPASoftSimple::processStats(const uint32_t frame,
>  	metadataReady.emit(frame, metadata);
>
>  	ControlList ctrls(sensorInfoMap_);
> -
> -	int32_t againNew = camHelper_
> -		? camHelper_->gainCode(frameContext.agc.gain)
> -		: static_cast<int32_t>(frameContext.agc.gain);
> -	ctrls.set(V4L2_CID_EXPOSURE, frameContext.agc.exposure);
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, againNew);
> -
> +	agc::prepareControls(ctrls, camHelper_.get(), frameContext.agc.exposure, frameContext.agc.gain);
>  	setSensorControls.emit(ctrls);
>  }
>
> --
> 2.55.0
>
Barnabás Pőcze July 24, 2026, 1:58 p.m. UTC | #2 
2026. 07. 24. 15:35 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Thu, Jul 23, 2026 at 05:43:03PM +0200, Barnabás Pőcze wrote:
>> Move the extraction and preparation of `V4L2_CID_{EXPOSURE,ANALOGUE_GAIN}`
>> into separate functions. This also implements support for not having a
>> a camera sensor helper.
> 
> I only checked the next patch, but these two helpers seems to not be
> designed to become part of the AgcAlgorithm class
> 
> If that's the case, and they will stay helpers for the rest of the
> series, I'm not 100% sure it's worth it.
> 
> Each helper saves a few lines, and for such reduced gain, I'm think
> it's more clear for the reader to see what happens instead of having
> to go and check what a function does.
> 
> Do these helpers have any other use along the rest of the series ?

They will stay as they are. I found them to be helpful in reducing the duplication,
especially wrt. checking the `CameraSensorHelper` presence, and in providing consistent
types everywhere (you need to get int32_t vs. uint32_t right).

`prepareControls()` could be extended to take the `FrameContext` (thus fewer arguments)
and set vblank as well. Do you think that would be worth it?


> 
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp          | 14 +++++-----
>>   src/ipa/libipa/agc.h           | 49 ++++++++++++++++++++++++++++++++++
>>   src/ipa/libipa/meson.build     |  1 +
>>   src/ipa/mali-c55/mali-c55.cpp  | 17 +++++-------
>>   src/ipa/rkisp1/rkisp1.cpp      | 11 +++-----
>>   src/ipa/simple/soft_simple.cpp | 15 +++--------
>>   6 files changed, 72 insertions(+), 35 deletions(-)
>>   create mode 100644 src/ipa/libipa/agc.h
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 4bdc4b7677..51379808cd 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -36,6 +36,7 @@
>>   #include "libcamera/internal/mapped_framebuffer.h"
>>   #include "libcamera/internal/yaml_parser.h"
>>
>> +#include "libipa/agc.h"
>>   #include "libipa/camera_sensor_helper.h"
>>
>>   #include "ipa_context.h"
>> @@ -596,8 +597,8 @@ void IPAIPU3::processStats(const uint32_t frame,
>>
>>   	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>>
>> -	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
>> +		= agc::extractControls(sensorControls, camHelper_.get());
>>
>>   	ControlList metadata(controls::controls);
>>
>> @@ -642,12 +643,11 @@ void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>>    */
>>   void IPAIPU3::setControls(unsigned int frame)
>>   {
>> -	int32_t exposure = context_.activeState.agc.exposure;
>> -	int32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
>> -
>>   	ControlList ctrls(sensorCtrls_);
>> -	ctrls.set(V4L2_CID_EXPOSURE, exposure);
>> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);
>> +	agc::prepareControls(
>> +		ctrls, camHelper_.get(),
>> +		context_.activeState.agc.exposure, context_.activeState.agc.gain
>> +	);
>>
>>   	ControlList lensCtrls(lensCtrls_);
>>   	lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
>> diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
>> new file mode 100644
>> index 0000000000..5247425952
>> --- /dev/null
>> +++ b/src/ipa/libipa/agc.h
>> @@ -0,0 +1,49 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2026 Ideas on Board Oy
>> + *
>> + * AGC-related functionality
>> + */
>> +
>> +#pragma once
>> +
>> +#include <utility>
>> +
>> +#include <linux/v4l2-controls.h>
>> +
>> +#include <libcamera/controls.h>
>> +
>> +#include "camera_sensor_helper.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +namespace agc {
>> +
>> +[[nodiscard]]
>> +inline std::pair<uint32_t, double>
>> +extractControls(const ControlList &controls, const CameraSensorHelper *sensor)
>> +{
>> +	auto exposure = controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +	auto gainCode = controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>> +
>> +	return {
>> +		uint32_t(exposure),
>> +		sensor ? sensor->gain(gainCode) : gainCode,
>> +	};
>> +}
>> +
>> +inline void
>> +prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
>> +		int32_t exposure, double gain)
>> +{
>> +	controls.set(V4L2_CID_EXPOSURE, exposure);
>> +	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
>> +}
>> +
>> +} /* namespace agc */
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>> index 963c5ee730..05f1a8749c 100644
>> --- a/src/ipa/libipa/meson.build
>> +++ b/src/ipa/libipa/meson.build
>> @@ -1,6 +1,7 @@
>>   # SPDX-License-Identifier: CC0-1.0
>>
>>   libipa_headers = files([
>> +    'agc.h',
>>       'agc_mean_luminance.h',
>>       'algorithm.h',
>>       'awb_bayes.h',
>> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
>> index 1d3af0627f..65e5297766 100644
>> --- a/src/ipa/mali-c55/mali-c55.cpp
>> +++ b/src/ipa/mali-c55/mali-c55.cpp
>> @@ -27,6 +27,7 @@
>>   #include "libcamera/internal/yaml_parser.h"
>>
>>   #include "algorithms/algorithm.h"
>> +#include "libipa/agc.h"
>>   #include "libipa/camera_sensor_helper.h"
>>
>>   #include "ipa_context.h"
>> @@ -141,20 +142,18 @@ void IPAMaliC55::setControls()
>>   {
>>   	IPAActiveState &activeState = context_.activeState;
>>   	uint32_t exposure;
>> -	uint32_t gain;
>> +	double gain;
>>
>>   	if (activeState.agc.autoEnabled) {
>>   		exposure = activeState.agc.automatic.exposure;
>> -		gain = camHelper_->gainCode(activeState.agc.automatic.sensorGain);
>> +		gain = activeState.agc.automatic.sensorGain;
>>   	} else {
>>   		exposure = activeState.agc.manual.exposure;
>> -		gain = camHelper_->gainCode(activeState.agc.manual.sensorGain);
>> +		gain = activeState.agc.manual.sensorGain;
>>   	}
>>
>>   	ControlList ctrls(sensorControls_);
>> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>> -
>> +	agc::prepareControls(ctrls, camHelper_.get(), exposure, gain);
>>   	setSensorControls.emit(ctrls);
>>   }
>>
>> @@ -352,10 +351,8 @@ void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,
>>   	stats = reinterpret_cast<mali_c55_stats_buffer *>(
>>   		buffers_.at(bufferId).planes()[0].data());
>>
>> -	frameContext.agc.exposure =
>> -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -	frameContext.agc.sensorGain =
>> -		camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +	std::tie(frameContext.agc.exposure, frameContext.agc.sensorGain)
>> +		= agc::extractControls(sensorControls, camHelper_.get());
>>
>>   	ControlList metadata(controls::controls);
>>
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 38f55b1d86..acb878d814 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -31,6 +31,7 @@
>>   #include "libcamera/internal/yaml_parser.h"
>>
>>   #include "algorithms/algorithm.h"
>> +#include "libipa/agc.h"
>>
>>   #include "ipa_context.h"
>>   #include "params.h"
>> @@ -328,10 +329,8 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
>>   		stats = reinterpret_cast<rkisp1_stat_buffer *>(
>>   			mappedBuffers_.at(bufferId).planes()[0].data());
>>
>> -	frameContext.sensor.exposure =
>> -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -	frameContext.sensor.gain =
>> -		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
>> +		= agc::extractControls(sensorControls, context_.camHelper.get());
>>
>>   	ControlList metadata(controls::controls);
>>
>> @@ -365,7 +364,6 @@ void IPARkISP1::setControls(unsigned int frame)
>>
>>   	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>>   	uint32_t exposure = frameContext.agc.exposure;
>> -	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
>>   	uint32_t vblank = frameContext.agc.vblank;
>>
>>   	LOG(IPARkISP1, Debug)
>> @@ -373,8 +371,7 @@ void IPARkISP1::setControls(unsigned int frame)
>>   		<< ", gain " << frameContext.agc.gain << ", vblank " << vblank;
>>
>>   	ControlList ctrls(context_.sensorControls);
>> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>> +	agc::prepareControls(ctrls, context_.camHelper.get(), exposure, frameContext.agc.gain);
>>   	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
>>
>>   	setSensorControls.emit(frame, ctrls);
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index 3932daaa1f..a38fcff7d8 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -27,6 +27,7 @@
>>   #include "libcamera/internal/yaml_parser.h"
>>
>>   #include "algorithms/adjust.h"
>> +#include "libipa/agc.h"
>>   #include "libipa/camera_sensor_helper.h"
>>
>>   #include "module.h"
>> @@ -301,10 +302,8 @@ void IPASoftSimple::processStats(const uint32_t frame,
>>   {
>>   	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>>
>> -	frameContext.sensor.exposure =
>> -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -	int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>> -	frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;
>> +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
>> +		= agc::extractControls(sensorControls, camHelper_.get());
>>
>>   	ControlList metadata(controls::controls);
>>   	for (const auto &algo : algorithms())
>> @@ -312,13 +311,7 @@ void IPASoftSimple::processStats(const uint32_t frame,
>>   	metadataReady.emit(frame, metadata);
>>
>>   	ControlList ctrls(sensorInfoMap_);
>> -
>> -	int32_t againNew = camHelper_
>> -		? camHelper_->gainCode(frameContext.agc.gain)
>> -		: static_cast<int32_t>(frameContext.agc.gain);
>> -	ctrls.set(V4L2_CID_EXPOSURE, frameContext.agc.exposure);
>> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, againNew);
>> -
>> +	agc::prepareControls(ctrls, camHelper_.get(), frameContext.agc.exposure, frameContext.agc.gain);
>>   	setSensorControls.emit(ctrls);
>>   }
>>
>> --
>> 2.55.0
>>
Jacopo Mondi July 24, 2026, 3:14 p.m. UTC | #3 
Hi Barnabás

On Fri, Jul 24, 2026 at 03:58:12PM +0200, Barnabás Pőcze wrote:
> 2026. 07. 24. 15:35 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Thu, Jul 23, 2026 at 05:43:03PM +0200, Barnabás Pőcze wrote:
> > > Move the extraction and preparation of `V4L2_CID_{EXPOSURE,ANALOGUE_GAIN}`
> > > into separate functions. This also implements support for not having a
> > > a camera sensor helper.
> >
> > I only checked the next patch, but these two helpers seems to not be
> > designed to become part of the AgcAlgorithm class
> >
> > If that's the case, and they will stay helpers for the rest of the
> > series, I'm not 100% sure it's worth it.
> >
> > Each helper saves a few lines, and for such reduced gain, I'm think
> > it's more clear for the reader to see what happens instead of having
> > to go and check what a function does.
> >
> > Do these helpers have any other use along the rest of the series ?
>
> They will stay as they are. I found them to be helpful in reducing the duplication,
> especially wrt. checking the `CameraSensorHelper` presence, and in providing consistent
> types everywhere (you need to get int32_t vs. uint32_t right).

I don't know, I might be biased, but I don't like little helpers that
require the reader to navigate to a different file to save 2 lines.

Looking at this diff

 -	int32_t exposure = context_.activeState.agc.exposure;
 -	int32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
 -	ctrls.set(V4L2_CID_EXPOSURE, exposure);
 -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);
 +	agc::prepareControls(
 +		ctrls, camHelper_.get(),
 +		context_.activeState.agc.exposure, context_.activeState.agc.gain
 +	);

I'm not even sure it's much less to type in

>
> `prepareControls()` could be extended to take the `FrameContext` (thus fewer arguments)
> and set vblank as well. Do you think that would be worth it?

Maybe yes..

>
>
> >
> > >
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >   src/ipa/ipu3/ipu3.cpp          | 14 +++++-----
> > >   src/ipa/libipa/agc.h           | 49 ++++++++++++++++++++++++++++++++++
> > >   src/ipa/libipa/meson.build     |  1 +
> > >   src/ipa/mali-c55/mali-c55.cpp  | 17 +++++-------
> > >   src/ipa/rkisp1/rkisp1.cpp      | 11 +++-----
> > >   src/ipa/simple/soft_simple.cpp | 15 +++--------
> > >   6 files changed, 72 insertions(+), 35 deletions(-)
> > >   create mode 100644 src/ipa/libipa/agc.h
> > >
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index 4bdc4b7677..51379808cd 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -36,6 +36,7 @@
> > >   #include "libcamera/internal/mapped_framebuffer.h"
> > >   #include "libcamera/internal/yaml_parser.h"
> > >
> > > +#include "libipa/agc.h"
> > >   #include "libipa/camera_sensor_helper.h"
> > >
> > >   #include "ipa_context.h"
> > > @@ -596,8 +597,8 @@ void IPAIPU3::processStats(const uint32_t frame,
> > >
> > >   	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > >
> > > -	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > -	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > > +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
> > > +		= agc::extractControls(sensorControls, camHelper_.get());
> > >
> > >   	ControlList metadata(controls::controls);
> > >
> > > @@ -642,12 +643,11 @@ void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > >    */
> > >   void IPAIPU3::setControls(unsigned int frame)
> > >   {
> > > -	int32_t exposure = context_.activeState.agc.exposure;
> > > -	int32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
> > > -
> > >   	ControlList ctrls(sensorCtrls_);
> > > -	ctrls.set(V4L2_CID_EXPOSURE, exposure);
> > > -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);
> > > +	agc::prepareControls(
> > > +		ctrls, camHelper_.get(),
> > > +		context_.activeState.agc.exposure, context_.activeState.agc.gain
> > > +	);
> > >
> > >   	ControlList lensCtrls(lensCtrls_);
> > >   	lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> > > diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
> > > new file mode 100644
> > > index 0000000000..5247425952
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/agc.h
> > > @@ -0,0 +1,49 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2026 Ideas on Board Oy
> > > + *
> > > + * AGC-related functionality
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <utility>
> > > +
> > > +#include <linux/v4l2-controls.h>
> > > +
> > > +#include <libcamera/controls.h>
> > > +
> > > +#include "camera_sensor_helper.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa {
> > > +
> > > +namespace agc {
> > > +
> > > +[[nodiscard]]
> > > +inline std::pair<uint32_t, double>
> > > +extractControls(const ControlList &controls, const CameraSensorHelper *sensor)
> > > +{
> > > +	auto exposure = controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > +	auto gainCode = controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> > > +
> > > +	return {
> > > +		uint32_t(exposure),
> > > +		sensor ? sensor->gain(gainCode) : gainCode,
> > > +	};
> > > +}
> > > +
> > > +inline void
> > > +prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
> > > +		int32_t exposure, double gain)
> > > +{
> > > +	controls.set(V4L2_CID_EXPOSURE, exposure);
> > > +	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
> > > +}
> > > +
> > > +} /* namespace agc */
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > index 963c5ee730..05f1a8749c 100644
> > > --- a/src/ipa/libipa/meson.build
> > > +++ b/src/ipa/libipa/meson.build
> > > @@ -1,6 +1,7 @@
> > >   # SPDX-License-Identifier: CC0-1.0
> > >
> > >   libipa_headers = files([
> > > +    'agc.h',
> > >       'agc_mean_luminance.h',
> > >       'algorithm.h',
> > >       'awb_bayes.h',
> > > diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
> > > index 1d3af0627f..65e5297766 100644
> > > --- a/src/ipa/mali-c55/mali-c55.cpp
> > > +++ b/src/ipa/mali-c55/mali-c55.cpp
> > > @@ -27,6 +27,7 @@
> > >   #include "libcamera/internal/yaml_parser.h"
> > >
> > >   #include "algorithms/algorithm.h"
> > > +#include "libipa/agc.h"
> > >   #include "libipa/camera_sensor_helper.h"
> > >
> > >   #include "ipa_context.h"
> > > @@ -141,20 +142,18 @@ void IPAMaliC55::setControls()
> > >   {
> > >   	IPAActiveState &activeState = context_.activeState;
> > >   	uint32_t exposure;
> > > -	uint32_t gain;
> > > +	double gain;
> > >
> > >   	if (activeState.agc.autoEnabled) {
> > >   		exposure = activeState.agc.automatic.exposure;
> > > -		gain = camHelper_->gainCode(activeState.agc.automatic.sensorGain);
> > > +		gain = activeState.agc.automatic.sensorGain;
> > >   	} else {
> > >   		exposure = activeState.agc.manual.exposure;
> > > -		gain = camHelper_->gainCode(activeState.agc.manual.sensorGain);
> > > +		gain = activeState.agc.manual.sensorGain;
> > >   	}
> > >
> > >   	ControlList ctrls(sensorControls_);
> > > -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > > -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > > -
> > > +	agc::prepareControls(ctrls, camHelper_.get(), exposure, gain);
> > >   	setSensorControls.emit(ctrls);
> > >   }
> > >
> > > @@ -352,10 +351,8 @@ void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,
> > >   	stats = reinterpret_cast<mali_c55_stats_buffer *>(
> > >   		buffers_.at(bufferId).planes()[0].data());
> > >
> > > -	frameContext.agc.exposure =
> > > -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > -	frameContext.agc.sensorGain =
> > > -		camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > > +	std::tie(frameContext.agc.exposure, frameContext.agc.sensorGain)
> > > +		= agc::extractControls(sensorControls, camHelper_.get());
> > >
> > >   	ControlList metadata(controls::controls);
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 38f55b1d86..acb878d814 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -31,6 +31,7 @@
> > >   #include "libcamera/internal/yaml_parser.h"
> > >
> > >   #include "algorithms/algorithm.h"
> > > +#include "libipa/agc.h"
> > >
> > >   #include "ipa_context.h"
> > >   #include "params.h"
> > > @@ -328,10 +329,8 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
> > >   		stats = reinterpret_cast<rkisp1_stat_buffer *>(
> > >   			mappedBuffers_.at(bufferId).planes()[0].data());
> > >
> > > -	frameContext.sensor.exposure =
> > > -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > -	frameContext.sensor.gain =
> > > -		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > > +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
> > > +		= agc::extractControls(sensorControls, context_.camHelper.get());
> > >
> > >   	ControlList metadata(controls::controls);
> > >
> > > @@ -365,7 +364,6 @@ void IPARkISP1::setControls(unsigned int frame)
> > >
> > >   	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > >   	uint32_t exposure = frameContext.agc.exposure;
> > > -	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > >   	uint32_t vblank = frameContext.agc.vblank;
> > >
> > >   	LOG(IPARkISP1, Debug)
> > > @@ -373,8 +371,7 @@ void IPARkISP1::setControls(unsigned int frame)
> > >   		<< ", gain " << frameContext.agc.gain << ", vblank " << vblank;
> > >
> > >   	ControlList ctrls(context_.sensorControls);
> > > -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > > -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > > +	agc::prepareControls(ctrls, context_.camHelper.get(), exposure, frameContext.agc.gain);
> > >   	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> > >
> > >   	setSensorControls.emit(frame, ctrls);
> > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> > > index 3932daaa1f..a38fcff7d8 100644
> > > --- a/src/ipa/simple/soft_simple.cpp
> > > +++ b/src/ipa/simple/soft_simple.cpp
> > > @@ -27,6 +27,7 @@
> > >   #include "libcamera/internal/yaml_parser.h"
> > >
> > >   #include "algorithms/adjust.h"
> > > +#include "libipa/agc.h"
> > >   #include "libipa/camera_sensor_helper.h"
> > >
> > >   #include "module.h"
> > > @@ -301,10 +302,8 @@ void IPASoftSimple::processStats(const uint32_t frame,
> > >   {
> > >   	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > >
> > > -	frameContext.sensor.exposure =
> > > -		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > -	int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> > > -	frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;
> > > +	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
> > > +		= agc::extractControls(sensorControls, camHelper_.get());
> > >
> > >   	ControlList metadata(controls::controls);
> > >   	for (const auto &algo : algorithms())
> > > @@ -312,13 +311,7 @@ void IPASoftSimple::processStats(const uint32_t frame,
> > >   	metadataReady.emit(frame, metadata);
> > >
> > >   	ControlList ctrls(sensorInfoMap_);
> > > -
> > > -	int32_t againNew = camHelper_
> > > -		? camHelper_->gainCode(frameContext.agc.gain)
> > > -		: static_cast<int32_t>(frameContext.agc.gain);
> > > -	ctrls.set(V4L2_CID_EXPOSURE, frameContext.agc.exposure);
> > > -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, againNew);
> > > -
> > > +	agc::prepareControls(ctrls, camHelper_.get(), frameContext.agc.exposure, frameContext.agc.gain);
> > >   	setSensorControls.emit(ctrls);
> > >   }
> > >
> > > --
> > > 2.55.0
> > >
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 4bdc4b7677..51379808cd 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -36,6 +36,7 @@ 
 #include "libcamera/internal/mapped_framebuffer.h"
 #include "libcamera/internal/yaml_parser.h"
 
+#include "libipa/agc.h"
 #include "libipa/camera_sensor_helper.h"
 
 #include "ipa_context.h"
@@ -596,8 +597,8 @@  void IPAIPU3::processStats(const uint32_t frame,
 
 	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
 
-	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
-	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
+	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
+		= agc::extractControls(sensorControls, camHelper_.get());
 
 	ControlList metadata(controls::controls);
 
@@ -642,12 +643,11 @@  void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
  */
 void IPAIPU3::setControls(unsigned int frame)
 {
-	int32_t exposure = context_.activeState.agc.exposure;
-	int32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
-
 	ControlList ctrls(sensorCtrls_);
-	ctrls.set(V4L2_CID_EXPOSURE, exposure);
-	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);
+	agc::prepareControls(
+		ctrls, camHelper_.get(),
+		context_.activeState.agc.exposure, context_.activeState.agc.gain
+	);
 
 	ControlList lensCtrls(lensCtrls_);
 	lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
diff --git a/src/ipa/libipa/agc.h b/src/ipa/libipa/agc.h
new file mode 100644
index 0000000000..5247425952
--- /dev/null
+++ b/src/ipa/libipa/agc.h
@@ -0,0 +1,49 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2026 Ideas on Board Oy
+ *
+ * AGC-related functionality
+ */
+
+#pragma once
+
+#include <utility>
+
+#include <linux/v4l2-controls.h>
+
+#include <libcamera/controls.h>
+
+#include "camera_sensor_helper.h"
+
+namespace libcamera {
+
+namespace ipa {
+
+namespace agc {
+
+[[nodiscard]]
+inline std::pair<uint32_t, double>
+extractControls(const ControlList &controls, const CameraSensorHelper *sensor)
+{
+	auto exposure = controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
+	auto gainCode = controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
+
+	return {
+		uint32_t(exposure),
+		sensor ? sensor->gain(gainCode) : gainCode,
+	};
+}
+
+inline void
+prepareControls(ControlList &controls, const CameraSensorHelper *sensor,
+		int32_t exposure, double gain)
+{
+	controls.set(V4L2_CID_EXPOSURE, exposure);
+	controls.set(V4L2_CID_ANALOGUE_GAIN, int32_t(sensor ? sensor->gainCode(gain) : gain));
+}
+
+} /* namespace agc */
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
index 963c5ee730..05f1a8749c 100644
--- a/src/ipa/libipa/meson.build
+++ b/src/ipa/libipa/meson.build
@@ -1,6 +1,7 @@ 
 # SPDX-License-Identifier: CC0-1.0
 
 libipa_headers = files([
+    'agc.h',
     'agc_mean_luminance.h',
     'algorithm.h',
     'awb_bayes.h',
diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
index 1d3af0627f..65e5297766 100644
--- a/src/ipa/mali-c55/mali-c55.cpp
+++ b/src/ipa/mali-c55/mali-c55.cpp
@@ -27,6 +27,7 @@ 
 #include "libcamera/internal/yaml_parser.h"
 
 #include "algorithms/algorithm.h"
+#include "libipa/agc.h"
 #include "libipa/camera_sensor_helper.h"
 
 #include "ipa_context.h"
@@ -141,20 +142,18 @@  void IPAMaliC55::setControls()
 {
 	IPAActiveState &activeState = context_.activeState;
 	uint32_t exposure;
-	uint32_t gain;
+	double gain;
 
 	if (activeState.agc.autoEnabled) {
 		exposure = activeState.agc.automatic.exposure;
-		gain = camHelper_->gainCode(activeState.agc.automatic.sensorGain);
+		gain = activeState.agc.automatic.sensorGain;
 	} else {
 		exposure = activeState.agc.manual.exposure;
-		gain = camHelper_->gainCode(activeState.agc.manual.sensorGain);
+		gain = activeState.agc.manual.sensorGain;
 	}
 
 	ControlList ctrls(sensorControls_);
-	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
-	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
-
+	agc::prepareControls(ctrls, camHelper_.get(), exposure, gain);
 	setSensorControls.emit(ctrls);
 }
 
@@ -352,10 +351,8 @@  void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,
 	stats = reinterpret_cast<mali_c55_stats_buffer *>(
 		buffers_.at(bufferId).planes()[0].data());
 
-	frameContext.agc.exposure =
-		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
-	frameContext.agc.sensorGain =
-		camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
+	std::tie(frameContext.agc.exposure, frameContext.agc.sensorGain)
+		= agc::extractControls(sensorControls, camHelper_.get());
 
 	ControlList metadata(controls::controls);
 
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 38f55b1d86..acb878d814 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -31,6 +31,7 @@ 
 #include "libcamera/internal/yaml_parser.h"
 
 #include "algorithms/algorithm.h"
+#include "libipa/agc.h"
 
 #include "ipa_context.h"
 #include "params.h"
@@ -328,10 +329,8 @@  void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
 		stats = reinterpret_cast<rkisp1_stat_buffer *>(
 			mappedBuffers_.at(bufferId).planes()[0].data());
 
-	frameContext.sensor.exposure =
-		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
-	frameContext.sensor.gain =
-		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
+	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
+		= agc::extractControls(sensorControls, context_.camHelper.get());
 
 	ControlList metadata(controls::controls);
 
@@ -365,7 +364,6 @@  void IPARkISP1::setControls(unsigned int frame)
 
 	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
 	uint32_t exposure = frameContext.agc.exposure;
-	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
 	uint32_t vblank = frameContext.agc.vblank;
 
 	LOG(IPARkISP1, Debug)
@@ -373,8 +371,7 @@  void IPARkISP1::setControls(unsigned int frame)
 		<< ", gain " << frameContext.agc.gain << ", vblank " << vblank;
 
 	ControlList ctrls(context_.sensorControls);
-	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
-	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
+	agc::prepareControls(ctrls, context_.camHelper.get(), exposure, frameContext.agc.gain);
 	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
 
 	setSensorControls.emit(frame, ctrls);
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index 3932daaa1f..a38fcff7d8 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -27,6 +27,7 @@ 
 #include "libcamera/internal/yaml_parser.h"
 
 #include "algorithms/adjust.h"
+#include "libipa/agc.h"
 #include "libipa/camera_sensor_helper.h"
 
 #include "module.h"
@@ -301,10 +302,8 @@  void IPASoftSimple::processStats(const uint32_t frame,
 {
 	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
 
-	frameContext.sensor.exposure =
-		sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
-	int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
-	frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;
+	std::tie(frameContext.sensor.exposure, frameContext.sensor.gain)
+		= agc::extractControls(sensorControls, camHelper_.get());
 
 	ControlList metadata(controls::controls);
 	for (const auto &algo : algorithms())
@@ -312,13 +311,7 @@  void IPASoftSimple::processStats(const uint32_t frame,
 	metadataReady.emit(frame, metadata);
 
 	ControlList ctrls(sensorInfoMap_);
-
-	int32_t againNew = camHelper_
-		? camHelper_->gainCode(frameContext.agc.gain)
-		: static_cast<int32_t>(frameContext.agc.gain);
-	ctrls.set(V4L2_CID_EXPOSURE, frameContext.agc.exposure);
-	ctrls.set(V4L2_CID_ANALOGUE_GAIN, againNew);
-
+	agc::prepareControls(ctrls, camHelper_.get(), frameContext.agc.exposure, frameContext.agc.gain);
 	setSensorControls.emit(ctrls);
 }