[libcamera-devel] [PATCH v2 3/3] ipa: ipu3: Introduce a IPAFrameContext queue
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri May 6 20:12:44 CEST 2022
Quoting Umang Jain (2022-05-06 14:48:34)
> Hi Kieran,
>
> On 5/6/22 17:22, Kieran Bingham wrote:
> > Quoting Umang Jain via libcamera-devel (2022-05-06 10:53:07)
> >> Introduce a queue of IPAFrameContext which shall maintain the data
> >> required to track each frame. Currently, it is storing only the
> >> sensor controls values applied for the frame but new structures
> >> can be introduced in IPAFrameContext to extend any frame-related
> >> contexts (either in-processing or new frames being queued).
> > Technically I'm not sure those even need to be stored in the
> > FrameContext as they get set and then used in the same function I think.
> >
> > But ... I think it's fine for now, even as an example for the data - and
> > what will be more relevant is the controls that get set per-frame, and
> > anything that needs to be tracked to adapt to those.
> >
> > However that said, if we rework DelayedControls - then being able to
> > associate the expected Gain/Exposures to what was set is probably
> > useful to debug and check the implementation.
>
>
> I have a few ideas here, let me prepare a prototype branch and share it
> with you.
>
> >
> >
> >
> >> For example, this patch provides the foundation for the extension
> >> of IPAFrameContext to store incoming request controls.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> src/ipa/ipu3/algorithms/agc.cpp | 2 +-
> >> src/ipa/ipu3/ipa_context.cpp | 16 ++++++++++++++--
> >> src/ipa/ipu3/ipa_context.h | 8 +++++++-
> >> src/ipa/ipu3/ipu3.cpp | 31 ++++++++++++++++++++++++++-----
> >> 4 files changed, 48 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index fdeec09d..4784af00 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -187,7 +187,7 @@ void Agc::computeExposure(IPAContext &context, double yGain,
> >> double iqMeanGain)
> >> {
> >> const IPASessionConfiguration &configuration = context.configuration;
> >> - IPAFrameContext &frameContext = context.frameContext;
> >> + IPAFrameContext &frameContext = context.frameContextQueue.front();
> >> /* Get the effective exposure and gain applied on the sensor. */
> >> uint32_t exposure = frameContext.sensor.exposure;
> >> double analogueGain = frameContext.sensor.gain;
> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >> index 06eb2776..fb48bc9b 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::frameContextQueue
> >> + * \brief FIFO container of IPAFrameContext for all the frames being queued
> >> *
> >> * \var IPAContext::activeState
> >> * \brief The current state of IPA algorithms
> >> @@ -182,6 +182,15 @@ namespace libcamera::ipa::ipu3 {
> >> * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> >> */
> >>
> >> +/**
> >> + * \brief Construct a IPAFrameContext instance
> >> + */
> >> +IPAFrameContext::IPAFrameContext(uint32_t frame)
> >> + : frame(frame)
> >> +{
> >> + sensor = {};
> >> +}
> >> +
> >> /**
> >> * \var IPAFrameContext::sensor
> >> * \brief Effective sensor values that were applied for the frame
> >> @@ -191,6 +200,9 @@ namespace libcamera::ipa::ipu3 {
> >> *
> >> * \var IPAFrameContext::sensor.gain
> >> * \brief Analogue gain multiplier
> >> + *
> >> + * \var IPAFrameContext::frame
> >> + * \brief The frame number associated with this IPAFrameContext
> >> */
> >>
> >> } /* namespace libcamera::ipa::ipu3 */
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index 8d681131..20cccc97 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -8,6 +8,8 @@
> >>
> >> #pragma once
> >>
> >> +#include <queue>
> >> +
> >> #include <linux/intel-ipu3.h>
> >>
> >> #include <libcamera/base/utils.h>
> >> @@ -71,17 +73,21 @@ struct IPAActiveState {
> >> };
> >>
> >> struct IPAFrameContext {
> >> + IPAFrameContext(uint32_t frame);
> >> +
> >> struct {
> >> uint32_t exposure;
> >> double gain;
> >> } sensor;
> >> +
> >> + uint32_t frame;
> >> };
> >>
> >> struct IPAContext {
> >> IPASessionConfiguration configuration;
> >> IPAActiveState activeState;
> >>
> >> - IPAFrameContext frameContext;
> >> + std::queue<IPAFrameContext> frameContextQueue;
> >> };
> >>
> >> } /* namespace ipa::ipu3 */
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index 268c8f61..1b566f14 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -350,6 +350,8 @@ int IPAIPU3::start()
> >> */
> >> void IPAIPU3::stop()
> >> {
> >> + while (!context_.frameContextQueue.empty())
> >> + context_.frameContextQueue.pop();
> > Does context_.frameContextQueue.clear(); work here?
>
>
> no, std::queue::clear() non-existent, however we can use
>
> context_.frameContextQueue = {};
>
> to have the same effect.
>
> >
> >
> >> }
> >>
> >> /**
> >> @@ -458,7 +460,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >>
> >> /* Clean IPAActiveState at each reconfiguration. */
> >> context_.activeState = {};
> >> - context_.frameContext = {};
> >> + context_.frameContextQueue = {};
> >>
> >> if (!validateSensorControls()) {
> >> LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> >> @@ -510,6 +512,13 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> >>
> >> void IPAIPU3::frameCompleted([[maybe_unused]] const uint32_t frame)
> > I suspect patch 1/3 could be squashed with this patch ... There's not a
> > lot of distinction on the change - but I don't mind the split otherwise.
>
>
> Ack. I'll keep separate for now.
>
> >
> >
> >> {
> >> + while (!context_.frameContextQueue.empty()) {
> >> + auto &fc = context_.frameContextQueue.front();
> > I'm almost tempted to say we should have a warning/debug print in here
> > if (fc.frame < frame). If it's the current frame (== frame) it's
> > expected, but if it's earlier - then that's where some sort of frame
> > drop has occurred - and we need to make sure that any request is still
>
>
> Nice and good observation. It might not be 'true' frame drop detection I
> think,
> but yes, having a warn if it's < currentFrame, is a marker that
> something went wrong.
>
> > handled correctly if we're going to associated requests that get
> > returned to notify a dropped frame.
> >
> > But I expect any frame drop handling would have already been handled
> > before this frameCompleted gets called.
>
>
> Frame drop handling seems a growing pit-fire, we need to address the
> mechanism separately :-)
>
> >
> >> + if (fc.frame <= frame)
> >> + context_.frameContextQueue.pop();
> >> + else
> >> + break;
> >> + }
> >> }
> >>
> >> /**
> >> @@ -574,8 +583,18 @@ 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>());
> >> + auto &frameContext = context_.frameContextQueue.front();
> >> +
> >> + /*
> >> + * An assert might be too harsh here. We want to know the cases
> >> + * where the front of the queue (implies the current frame in processing,
> >> + * diverges from the frame parameter of this function
> >> + * \todo Identify those cases - e.g. frame drop?
> >> + */
> >> + ASSERT(context_.frameContextQueue.front().frame == frame);
> > This is certainly too hard I think. It would almost certainly cause
> > libcamera to close on a frame drop (which could be very easy to cause to
> > occur). So I don't think I'd like to merge this line.
>
>
> I am not very keen on it either. Either we guarantee and eliminate all
> the situations where
> frameContextQueue.front() != frame (which will involve listing down the
> possible edge scenarios
> for e.g. frame-drop handling, and have grace failure paths for each of them)
>
> Or we have this assert (or toned-down version of assert) to
> let us know, that the front of the queue doesn't match up with the frame
> being processed.
I think if we're processing a stats buffer for a frame, that means we
successfully had the request queued in time to keep everything flowing -
but there could still have been a hole or gap that means a request
wasn't queued in time to satisfy the completion of the CIO2 - and we
would probably in that case have to drop a frame/request - and not
process it through the ISP. I think that means we'd have a 'gap' in the
sequence numbers here - so it may not be the front of the queue - in
which case we have to drop (and already complete) frameContexts up to
that point.
Perhaps that would have already happened befroe getting into this
function though - It might be clearer to map out on a sequence diagram.
> >
> >
> >> +
> >> + 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;
> >> @@ -590,11 +609,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
> >> @@ -621,6 +640,8 @@ void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> >> [[maybe_unused]] const ControlList &controls)
> >> {
> >> /* \todo Start processing for 'frame' based on 'controls'. */
> >> +
> >> + context_.frameContextQueue.emplace(frame);
> > But I think this is good - from here we can start parsing the request
> > and controls and start doing something about request controls.
>
>
> Ack.
>
> >
> >
> > These patches are more or less ok to go in on their own I think (except
> > that ASSERT which is far too hard) - but I'd be really keen to start
> > seeing how we handle controls here soon.
> >
> > --
> > Kieran
> >
> >
> >> }
> >>
> >> /**
> >> --
> >> 2.31.1
> >>
More information about the libcamera-devel
mailing list