[libcamera-devel] [PATCH v2 3/3] ipa: ipu3: Introduce a IPAFrameContext queue
Umang Jain
umang.jain at ideasonboard.com
Fri May 6 15:48:34 CEST 2022
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.
>
>
>> +
>> + 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