[libcamera-devel,v4,3/3] ipa: ipu3: Put IPAFrameContext(s) in a ring buffer

15949 diff mbox series

Instead of having one frame context constantly being updated,
this patch aims to introduce per-frame IPAFrameContext which
are stored in a ring buffer. Whenever a request is queued, a new
IPAFrameContext is created and inserted into the ring buffer.

The IPAFrameContext structure itself has been slightly extended
to store a frame id and a ControlList for incoming frame
controls (sent in by the application). The next step would be to
read and set these controls whenever the request is actually queued
to the hardware.

Since now we are working in multiples of IPAFrameContext, the
Algorithm::process() will actually take in a IPAFrameContext pointer
(as opposed to a nullptr while preparing for this change).

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
 src/ipa/ipu3/algorithms/agc.h   |  4 ++--
 src/ipa/ipu3/ipa_context.cpp    | 26 ++++++++++++++++++++++----
 src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
 src/ipa/ipu3/ipu3.cpp           | 24 +++++++++++++++---------
 5 files changed, 57 insertions(+), 22 deletions(-)

Hi Umang,

Thank you for the patch.

On Wed, May 18, 2022 at 03:10:30PM +0200, Umang Jain via libcamera-devel wrote:
> Instead of having one frame context constantly being updated,
> this patch aims to introduce per-frame IPAFrameContext which
> are stored in a ring buffer. Whenever a request is queued, a new
> IPAFrameContext is created and inserted into the ring buffer.
> 
> The IPAFrameContext structure itself has been slightly extended
> to store a frame id and a ControlList for incoming frame
> controls (sent in by the application). The next step would be to
> read and set these controls whenever the request is actually queued
> to the hardware.
> 
> Since now we are working in multiples of IPAFrameContext, the
> Algorithm::process() will actually take in a IPAFrameContext pointer
> (as opposed to a nullptr while preparing for this change).
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>  src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>  src/ipa/ipu3/ipa_context.cpp    | 26 ++++++++++++++++++++++----
>  src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
>  src/ipa/ipu3/ipu3.cpp           | 24 +++++++++++++++---------
>  5 files changed, 57 insertions(+), 22 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 383a8deb..f16be534 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
>   * \param[in] yGain The gain calculated based on the relative luminance target
>   * \param[in] iqMeanGain The gain calculated based on the relative luminance target
>   */
> -void Agc::computeExposure(IPAContext &context, double yGain,
> -			  double iqMeanGain)
> +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> +			  double yGain, double iqMeanGain)
>  {
>  	const IPASessionConfiguration &configuration = context.configuration;
> -	IPAFrameContext &frameContext = context.frameContext;
>  	/* Get the effective exposure and gain applied on the sensor. */
> -	uint32_t exposure = frameContext.sensor.exposure;
> -	double analogueGain = frameContext.sensor.gain;
> +	uint32_t exposure = frameContext->sensor.exposure;
> +	double analogueGain = frameContext->sensor.gain;
>  
>  	/* Use the highest of the two gain estimates. */
>  	double evGain = std::max(yGain, iqMeanGain);
> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo
>  			break;
>  	}
>  
> -	computeExposure(context, yGain, iqMeanGain);
> +	computeExposure(context, frameContext, yGain, iqMeanGain);
>  	frameCount_++;
>  }
>  
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 219a1a96..105ae0f2 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -35,8 +35,8 @@ private:
>  	double measureBrightness(const ipu3_uapi_stats_3a *stats,
>  				 const ipu3_uapi_grid_config &grid) const;
>  	utils::Duration filterExposure(utils::Duration currentExposure);
> -	void computeExposure(IPAContext &context, double yGain,
> -			     double iqMeanGain);
> +	void computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> +			     double yGain, double iqMeanGain);
>  	double estimateLuminance(IPAActiveState &activeState,
>  				 const ipu3_uapi_grid_config &grid,
>  				 const ipu3_uapi_stats_3a *stats,
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 383c2e37..13cdb835 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -58,13 +58,11 @@ namespace libcamera::ipa::ipu3 {
>   * \var IPAContext::configuration
>   * \brief The IPA session configuration, immutable during the session
>   *
> - * \var IPAContext::frameContext
> - * \brief The frame context for the frame being processed
> + * \var IPAContext::frameContexts
> + * \brief Ring buffer of the IPAFrameContext(s)
>   *
>   * \var IPAContext::activeState
>   * \brief The current state of IPA algorithms
> - *
> - * \todo The frame context needs to be turned into real per-frame data storage.
>   */
>  
>  /**
> @@ -183,6 +181,26 @@ namespace libcamera::ipa::ipu3 {
>   */
>  
>  /**
> + * \brief Default constructor for IPAFrameContext
> + */
> +IPAFrameContext::IPAFrameContext() = default;
> +
> +/**
> + * \brief Construct a IPAFrameContext instance
> + */
> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> +	: frame(id), frameControls(reqControls)
> +{
> +	sensor = {};
> +}
> +
> +/**
> + * \var IPAFrameContext::frame

frameId may be more explicit. Or maybe even just id ?

> + * \brief The frame number
> + *
> + * \var IPAFrameContext::frameControls
> + * \brief Controls sent in by the application while queuing the request

Same here, I'd call this controls. Everything in the frame context is
related to a frame.

> + *
>   * \var IPAFrameContext::sensor
>   * \brief Effective sensor values that were applied for the frame
>   *
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 8d681131..42e11141 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -8,16 +8,22 @@
>  
>  #pragma once
>  
> +#include <array>
> +
>  #include <linux/intel-ipu3.h>
>  
>  #include <libcamera/base/utils.h>
>  
> +#include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
>  
>  namespace libcamera {
>  
>  namespace ipa::ipu3 {
>  
> +/* Maximum number of frame contexts to be held */
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
>  struct IPASessionConfiguration {
>  	struct {
>  		ipu3_uapi_grid_config bdsGrid;
> @@ -71,17 +77,23 @@ struct IPAActiveState {
>  };
>  
>  struct IPAFrameContext {
> +	IPAFrameContext();
> +	IPAFrameContext(uint32_t id, const ControlList &reqControls);
> +
>  	struct {
>  		uint32_t exposure;
>  		double gain;
>  	} sensor;
> +
> +	uint32_t frame;
> +	ControlList frameControls;
>  };
>  
>  struct IPAContext {
>  	IPASessionConfiguration configuration;
>  	IPAActiveState activeState;
>  
> -	IPAFrameContext frameContext;
> +	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;

I'd be tempted to pass the configuration and active state to algorithms
instead of the IPAContext, to avoid exposing frameContexts. That will
make the algorithm functions take quite a few arguments though, it may
not be very nice.

>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 16e5028f..2f6bb672 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
>  	}
>  
>  	/* Clean context */
> -	context_ = {};
> +	context_.configuration = {};
>  	context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>  
>  	/* Construct our Algorithms */
> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>  	/* Clean IPAActiveState at each reconfiguration. */
>  	context_.activeState = {};
> -	context_.frameContext = {};
> +	IPAFrameContext initFrameContext;
> +	context_.frameContexts.fill(initFrameContext);
>  
>  	if (!validateSensorControls()) {
>  		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -568,15 +569,20 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  	const ipu3_uapi_stats_3a *stats =
>  		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>  
> -	context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> +
> +	if (frameContext.frame != frame)
> +		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";

Hmmm... I think the ring buffer needs to be extracted to a separate
class. We should avoid the need for spurious initialization, and offer
safety against overflows.

> +
> +	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>());
>  
>  	double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>  	int32_t vBlank = context_.configuration.sensor.defVBlank;
>  	ControlList ctrls(controls::controls);
>  
>  	for (auto const &algo : algorithms_)
> -		algo->process(context_, nullptr, stats);
> +		algo->process(context_, &frameContext, stats);
>  
>  	setControls(frame);
>  
> @@ -584,11 +590,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  	int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
>  	ctrls.set(controls::FrameDuration, frameDuration);
>  
> -	ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> +	ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>  
>  	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>  
> -	ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> +	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>  
>  	/*
>  	 * \todo The Metadata provides a path to getting extended data
> @@ -609,10 +615,10 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>   * Parse the request to handle any IPA-managed controls that were set from the
>   * application such as manual sensor settings.
>   */
> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> -			   [[maybe_unused]] const ControlList &controls)
> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>  {
>  	/* \todo Start processing for 'frame' based on 'controls'. */
> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
>  }
>  
>  /**