[libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put IPAFrameContext(s) in a ring buffer
Umang Jain
umang.jain at ideasonboard.com
Wed May 18 12:34:15 CEST 2022
Hi Jacopo,
Thanks for reviewing.
On 5/18/22 09:47, Jacopo Mondi wrote:
> Hi Umang,
>
> On Tue, May 17, 2022 at 09:18:33PM +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 at ideasonboard.com>
>> ---
>> src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>> src/ipa/ipu3/algorithms/agc.h | 4 ++--
>> src/ipa/ipu3/ipa_context.cpp | 24 ++++++++++++++++++++++--
>> src/ipa/ipu3/ipa_context.h | 14 +++++++++++++-
>> src/ipa/ipu3/ipu3.cpp | 22 +++++++++++++---------
>> 5 files changed, 55 insertions(+), 20 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 06eb2776..81b30600 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -58,8 +58,8 @@ 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
>> @@ -183,6 +183,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
>> + * \brief The frame number
>> + *
>> + * \var IPAFrameContext::frameControls
>> + * \brief Controls sent in by the application while queuing the request
>> + *
>> * \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;
>> };
>>
>> } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 16e5028f..4322f96b 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,17 @@ 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];
>> +
>> + 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 +587,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 +612,11 @@ 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'. */
>> + IPAFrameContext newFrameContext(frame, controls);
>> + context_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;
> Just a little note, which now is not that relevant as we have a
> limited number of controls, but this goes through at least 2 copies a
> the 'controls' list.
>
> Constructor:
>
> IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> : frame(id), frameControls(reqControls)
>
> And the implicitely defined copy assignment operator.
>
> I wonder if
> context_.frameContexts[frame % kMaxFrameContexts] = {frame, controls};
>
> would spare a copy
I am also wondering. I think your version is still doing two copies as
per my understanding.
One would be through the constructor as you said above.
The other one would be while assigning `context_.frameContexts[frame %
kMaxFrameContexts]` as operator= would do a member-wise copy, does my
inference make sense?
But nevertheless,
Your version is a more clear to read hence I would keep this anyway.
>
> The rest looks good to me
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
> j
>
>
>> }
>>
>> /**
>> --
>> 2.31.0
>>
More information about the libcamera-devel
mailing list