[libcamera-devel] [PATCH v2 3/3] ipa: ipu3: Introduce a IPAFrameContext queue
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri May 6 13:52:55 CEST 2022
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.
> 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?
> }
>
> /**
> @@ -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.
> {
> + 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
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.
> + 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.
> +
> + 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.
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