[libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put IPAFrameContext(s) in a ring buffer
Jacopo Mondi
jacopo at jmondi.org
Wed May 18 12:59:05 CEST 2022
Hi Umang,
On Wed, May 18, 2022 at 12:34:15PM +0200, Umang Jain wrote:
> 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.
Ah, {frame, controls} would construct an instance anyway, you're
right.
I assume that's a recurring problem, how do you re-initialize elements in
a container without going through their copy operators (which require
an instance of the same type to be passed in as parameter) ?
>
> 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