[libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put IPAFrameContext(s) in a ring buffer
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed May 18 12:44:09 CEST 2022
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.
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