[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 13:32:55 CEST 2022
Hi Kieran,
On 5/18/22 12:44, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2022-05-18 10:39:11)
>> Hi JM,
>>
>> On 5/18/22 11:08, Jean-Michel Hautbois wrote:
>>> Hi Umang,
>>>
>>> On 17/05/2022 21:18, 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];
>>> Shouldn't the frame be "double-checked", verifying that
>>> (frameContext->frame == frame) before passing it to the algorithms
>>> somehow ?
>>
>> Well, we can surely verify that but we know in advance that the
>> verification is subjected to failure with known cases (for e.g. frame
>> drop) so I am not very keen putting the verification in. If you had seen
>> the v2, I had a ASSERT similar to this basically (and has some
>> discussion on this there)
> I do think some sort of check is warranted though, because the CIO2
> might be free-running with no requests coming in - so I can envisage we
> could easily end up overflowing the ring buffer in a (bad) scenario - so
> we should at least report that.
>
> I would expect that in that scenario the CIO2 will run but the IMGU
> might not - so we wouldn't get any statistics for that frame.
>
> It's something to think about later anyway ... but a check/print would
> be beneficial at least. For now it could still operate anyway, but with
> a warning print.
Fair enough, I have introduced a warning:
+ if (frameContext.frame != frame)
+ LOG(IPAIPU3, Warning) << "Frame " << frame << " does not
match its frame context";
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>>> Apart from that:
>>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>
>> Thanks for reviewing!
>>
>>>> +
>>>> + 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;
>>>> }
>>>> /**
More information about the libcamera-devel
mailing list