[libcamera-devel] [PATCH v1 1/4] ipa: ipu3: Separate out frame context queue as a distinct class
Umang Jain
umang.jain at ideasonboard.com
Thu Jun 9 11:13:01 CEST 2022
Hi Jacopo
On 6/8/22 14:04, Jacopo Mondi wrote:
> Hi Umang,
>
> On Fri, Jun 03, 2022 at 03:22:56PM +0200, Umang Jain via libcamera-devel wrote:
>> There are cases where we need more checks and balance to be carried out
>> by the frame context queue class. For that, separate it out as a
>> distinct class on which we can build upon.
>>
>> For now, a minimialistic implementation is provided with .get(frame)
>> helper which returns a IPAFrameContext pointer for the required frame.
>> Going ahead more such helpers can be provided to access/modify the
>> frame context queue.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> ---
>> src/ipa/ipu3/ipa_context.cpp | 57 ++++++++++++++++++++++++++++++++++--
>> src/ipa/ipu3/ipa_context.h | 11 ++++++-
>> src/ipa/ipu3/ipu3.cpp | 18 ++++++------
>> 3 files changed, 74 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 13cdb835..9f95a61c 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -7,12 +7,18 @@
>>
>> #include "ipa_context.h"
>>
>> +#include <libcamera/base/log.h>
>> +
>> /**
>> * \file ipa_context.h
>> * \brief Context and state information shared between the algorithms
>> */
>>
>> -namespace libcamera::ipa::ipu3 {
>> +namespace libcamera {
>> +
>> +LOG_DECLARE_CATEGORY(IPAIPU3)
>> +
>> +namespace ipa::ipu3 {
>>
>> /**
>> * \struct IPASessionConfiguration
>> @@ -211,4 +217,51 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>> * \brief Analogue gain multiplier
>> */
>>
>> -} /* namespace libcamera::ipa::ipu3 */
>> +/**
>> + * \class FCQueue
>> + * \brief A FIFO circular queue holding IPAFrameContext(s)
>> + *
>> + * FCQueue holds all the IPAFrameContext(s) related to frames required
>> + * to be processed by the IPA at a given point.
>> + */
>> +
>> +/**
>> + * \brief FCQueue constructor
>> + */
>> +FCQueue::FCQueue()
>> +{
>> + clear();
>> +}
>> +
>> +/**
>> + * \brief Retrieve the IPAFrameContext for the frame
>> + * \param[in] frame Frame number for which the IPAFrameContext needs to
>> + * retrieved
>> + *
>> + * \return Pointer to the IPAFrameContext
>> + */
>> +IPAFrameContext *FCQueue::get(uint32_t frame)
>> +{
>> + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>> +
>> + if (frame != frameContext.frame) {
>> + LOG(IPAIPU3, Warning)
>> + << "Got wrong frame context for frame" << frame
>> + << " or frame context doesn't exist yet";
>> + }
>> +
>> + return &frameContext;
>> +}
>> +
>> +/**
>> + * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
>> + */
>> +void FCQueue::clear()
>> +{
>> + IPAFrameContext initFrameContext;
>> + this->fill(initFrameContext);
>> +}
>> +
>> +} /* namespace ipa::ipu3 */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 42e11141..56d281f6 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -89,11 +89,20 @@ struct IPAFrameContext {
>> ControlList frameControls;
>> };
>>
>> +class FCQueue : public std::array<IPAFrameContext, kMaxFrameContexts>
> This will expose the full std::array<> interface.
> I wonder if this should not rather inherit std::array<> as private and
> selectively expose only certain functions. That's what ControlInfoMap
> does, as an example.
Ah correct, I don't want to expose the entire std::array<> interface.
If you have seen I disable the [] operator in 2/3 but still .at() is
exposed.
So better to inherit it as private. I'll checkout ControlInfoMap.
>
> Then the next question is if we want to have a circular buffer class
> in libcamera and make
>
> class FCQueue : public libcamera::CircualBuffer<IPAFrameContext, kMaxFrameContexts>
> {
>
> }
Feels a bit early, as I have mentioned before. The design discussion is
still
underway it seems (thanks to your reviews), which I assumed was solidified
already (my mistake probably)
Now if it seems they aren't, putting something generic in libcamera core for
this purpose - doesn't seem to be a good idea to me because we will probably
end up customizing the "generic" CircularBuffer to suit the needs of IPA
- which
will be living in libcamera-core. I probably resist doing it *for now*,
until we have
a clear idea of the design needs (and putting a design where other IPAs
can use it
hopefully).
I get the temptation to do it generically, but I suppose the priority at
the moment
is get build a support layer for 'per-frame' controls. Get it working on
one platform,
then pull out the parts to make it generic - without regressing PFC.
Does this sound
good?
>
>> +{
>> +public:
>> + FCQueue();
>> +
>> + void clear();
>> + IPAFrameContext *get(uint32_t frame);
>> +};
>> +
>> struct IPAContext {
>> IPASessionConfiguration configuration;
>> IPAActiveState activeState;
>>
>> - std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>> + FCQueue frameContexts;
>> };
>>
>> } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 2f6bb672..0843d882 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -456,8 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>
>> /* Clean IPAActiveState at each reconfiguration. */
>> context_.activeState = {};
>> - IPAFrameContext initFrameContext;
>> - context_.frameContexts.fill(initFrameContext);
>> + context_.frameContexts.clear();
>> +
> Unecessary empty line
>
>> if (!validateSensorControls()) {
>> LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>> @@ -569,20 +569,20 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>> const ipu3_uapi_stats_3a *stats =
>> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>>
>> - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
>> + IPAFrameContext *frameContext = context_.frameContexts.get(frame);
>>
>> - if (frameContext.frame != frame)
>> + if (frameContext->frame != frame)
>> LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
>>
>> - 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>());
>> + 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_, &frameContext, stats);
>> + algo->process(context_, frameContext, stats);
>>
>> setControls(frame);
>>
>> @@ -590,11 +590,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, frameContext.sensor.gain);
>> + ctrls.set(controls::AnalogueGain, frameContext->sensor.gain);
>>
>> ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>>
>> - ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>> + ctrls.set(controls::ExposureTime, frameContext->sensor.exposure * lineDuration);
>>
>> /*
>> * \todo The Metadata provides a path to getting extended data
>> --
>> 2.31.1
>>
More information about the libcamera-devel
mailing list