[PATCH 3/4] libipa: FCQueue: Initialize FrameContext with activeState
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Oct 28 11:56:45 CET 2024
Hi Dan
On Mon, Oct 28, 2024 at 10:27:02AM +0000, Dan Scally wrote:
> Hi Jacopo
>
> On 16/10/2024 18:03, Jacopo Mondi wrote:
> > Pass to the FCQueue the algorithm's active state to use the most
> > recent state of IPA algorithms to initialize a FrameContext.
> >
> > Modify all IPA modules that use libipa to pass a const ActiveState
> > reference to the FCQueue function and make their IPAActiveState
> > implementation derive a base ActiveState structure.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > src/ipa/ipu3/ipa_context.h | 2 +-
> > src/ipa/ipu3/ipu3.cpp | 9 ++++++---
> > src/ipa/libipa/fc_queue.cpp | 10 +++++++---
> > src/ipa/libipa/fc_queue.h | 19 +++++++++++++------
> > src/ipa/rkisp1/ipa_context.cpp | 5 +++--
> > src/ipa/rkisp1/ipa_context.h | 5 +++--
> > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++----
> > src/ipa/simple/ipa_context.h | 2 +-
> > src/ipa/simple/soft_simple.cpp | 9 ++++++---
> > 9 files changed, 48 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index c85d1e34ea85..526af2ac0b06 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -46,7 +46,7 @@ struct IPASessionConfiguration {
> > } sensor;
> > };
> > -struct IPAActiveState {
> > +struct IPAActiveState : public ActiveState {
> > struct {
> > uint32_t focus;
> > double maxVariance;
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 10a8c86d8e64..84c443a009fd 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -561,7 +561,8 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> > */
> > params->use = {};
> > - IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame,
> > + context_.activeState);
> > for (auto const &algo : algorithms())
> > algo->prepare(context_, frame, frameContext, params);
> > @@ -594,7 +595,8 @@ 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.get(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame,
> > + context_.activeState);
> I get why it needs to be done, but I don't particularly like that we then
> have to pass activeState to .get()...do we know what kinds of situations
Because it's more letters to type or for other reasons ?
> result in .get() calls before .alloc() calls for a FrameContext?
I'll explain my use case, which is detailed in 4/4.
Currently the ph/ipa interactions are clocked by a user submitting a
Request. A Request gets queued, it alloc() a frame context and
it goes through algo->queueRequest(). AGC::queueRequest() initializes
FrameContext.agc.meteringMode using the active state.
Now, we move towards a model where the IPA is instead clocked by
frames being produced by the sensor and thus we can have Request
underruns if an app doesn't queue requests fast enough to keep it up
with the sensor's frame rate. In this case we'll go through
processStats() without going through IPA::queueRequest. In this case
we get() a frame context, and if we get it before it has been
allocated, we should make sure it can be processed by algo->process()
without causing issues as the one that 4/4 fixes.
I presume however, that even if we get through an alloc() making sure
that the initialization made by algorithms using the active state are
handled in IPAFrameContext::init() we would probably get more robust
code, making sure the algorithms' implementation do not rely on too
many internal state-keeping and instead work on what's implemented in
IPAFrameContext.
> > 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>());
> > @@ -627,7 +629,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > */
> > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > {
> > - IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.alloc(frame,
> > + context_.activeState);
> > for (auto const &algo : algorithms())
> > algo->queueRequest(context_, frame, frameContext, controls);
> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > index fa2454fd5706..56c7c75a8f48 100644
> > --- a/src/ipa/libipa/fc_queue.cpp
> > +++ b/src/ipa/libipa/fc_queue.cpp
> > @@ -42,6 +42,7 @@ namespace ipa {
> > * \fn FrameContext::init()
> > * \brief Initialize a frame context
> > * \param[in] frameNum The frame number to assign to this FrameContext
> > + * \param[in] activeState The IPA current active state
> > *
> > * This function initializes a frame context by assigning it a frame number.
> > * The single IPA modules are expected to override this function to initialize
> > @@ -117,9 +118,10 @@ namespace ipa {
> > */
> > /**
> > - * \fn FCQueue::alloc(uint32_t frame)
> > + * \fn FCQueue::alloc(uint32_t frame, const ActiveState &activeState)
> > * \brief Allocate and return a FrameContext for the \a frame
> > * \param[in] frame The frame context sequence number
> > + * \param[in] activeState The IPA current active state
> > *
> > * The first call to obtain a FrameContext from the FCQueue should be handled
> > * through this function. The FrameContext will be initialised, if not
> > @@ -135,12 +137,14 @@ namespace ipa {
> > */
> > /**
> > - * \fn FCQueue::get(uint32_t frame)
> > + * \fn FCQueue::get(uint32_t frame, const ActiveState &activeState)
> > * \brief Obtain the FrameContext for the \a frame
> > * \param[in] frame The frame context sequence number
> > + * \param[in] activeState The IPA current active state
> > *
> > * If the FrameContext is not correctly initialised for the \a frame, it will be
> > - * initialised.
> > + * initialised using the most current state of IPA algorithm contained in
> > + * \a activeState.
> > *
> > * \return A reference to the FrameContext for sequence \a frame
> > */
> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > index bfcce5a81356..48842e54cd35 100644
> > --- a/src/ipa/libipa/fc_queue.h
> > +++ b/src/ipa/libipa/fc_queue.h
> > @@ -21,9 +21,16 @@ namespace ipa {
> > template<typename FrameContext>
> > class FCQueue;
> > +struct ActiveState {
> > +};
> > +
> > struct FrameContext {
> > +public:
> > + virtual ~FrameContext() = default;
> > +
> Is this addition related?
> > protected:
> > - virtual void init(const uint32_t frameNum)
> > + virtual void init(const uint32_t frameNum,
> > + [[maybe_unused]] const ActiveState &activeState)
> > {
> > frame = frameNum;
> > initialised = true;
> > @@ -52,7 +59,7 @@ public:
> > }
> > }
> > - FrameContext &alloc(const uint32_t frame)
> > + FrameContext &alloc(const uint32_t frame, const ActiveState &activeState)
> > {
> > FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > @@ -71,12 +78,12 @@ public:
> > LOG(FCQueue, Warning)
> > << "Frame " << frame << " already initialised";
> > else
> > - frameContext.init(frame);
> > + frameContext.init(frame, activeState);
> > return frameContext;
> > }
> > - FrameContext &get(uint32_t frame)
> > + FrameContext &get(uint32_t frame, const ActiveState &activeState)
> > {
> > FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > @@ -103,7 +110,7 @@ public:
> > * Make sure the FrameContext gets initialised if get()
> > * is called before alloc() by the IPA for frame#0.
> > */
> > - frameContext.init(frame);
> > + frameContext.init(frame, activeState);
> > return frameContext;
> > }
> > @@ -123,7 +130,7 @@ public:
> > LOG(FCQueue, Warning)
> > << "Obtained an uninitialised FrameContext for " << frame;
> > - frameContext.init(frame);
> > + frameContext.init(frame, activeState);
> > return frameContext;
> > }
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 4e4fe5f4ae96..2dad42b3154f 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -417,9 +417,10 @@ namespace libcamera::ipa::rkisp1 {
> > * \brief Analogue gain multiplier
> > */
> > -void IPAFrameContext::init(const uint32_t frameNum)
> > +void IPAFrameContext::init(const uint32_t frameNum,
> > + const ActiveState &activeState)
> > {
> > - FrameContext::init(frameNum);
> > + FrameContext::init(frameNum, activeState);
> > }
> > /**
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 51e04bc30627..f708b32111af 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -64,7 +64,7 @@ struct IPASessionConfiguration {
> > uint32_t paramFormat;
> > };
> > -struct IPAActiveState {
> > +struct IPAActiveState : public ActiveState {
> > struct {
> > struct {
> > uint32_t exposure;
> > @@ -178,7 +178,8 @@ struct IPAFrameContext : public FrameContext {
> > Matrix<float, 3, 3> ccm;
> > } ccm;
> > - void init(const uint32_t frame) override;
> > + void init(const uint32_t frame,
> > + const ActiveState &activeState) override;
> > };
> > struct IPAContext {
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 9e161cabdea4..7c1cefc1c7fa 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -325,7 +325,8 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> > void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
> > {
> > - IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.alloc(frame,
> > + context_.activeState);
> > for (auto const &a : algorithms()) {
> > Algorithm *algo = static_cast<Algorithm *>(a.get());
> > @@ -337,7 +338,8 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
> > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> > {
> > - IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame,
> > + context_.activeState);
> > RkISP1Params params(context_.configuration.paramFormat,
> > mappedBuffers_.at(bufferId).planes()[0]);
> > @@ -351,7 +353,8 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> > void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
> > const ControlList &sensorControls)
> > {
> > - IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame,
> > + context_.activeState);
> > /*
> > * In raw capture mode, the ISP is bypassed and no statistics buffer is
> > @@ -447,7 +450,8 @@ void IPARkISP1::setControls(unsigned int frame)
> > * internal sensor delays and other timing parameters into account.
> > */
> > - IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame,
> > + context_.activeState);
> > uint32_t exposure = frameContext.agc.exposure;
> > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> > index 3519f20f6415..956f4fb71abf 100644
> > --- a/src/ipa/simple/ipa_context.h
> > +++ b/src/ipa/simple/ipa_context.h
> > @@ -24,7 +24,7 @@ struct IPASessionConfiguration {
> > } agc;
> > };
> > -struct IPAActiveState {
> > +struct IPAActiveState : public ActiveState {
> > struct {
> > uint8_t level;
> > } blc;
> > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> > index b28c7039f7bd..71b31d728117 100644
> > --- a/src/ipa/simple/soft_simple.cpp
> > +++ b/src/ipa/simple/soft_simple.cpp
> > @@ -249,7 +249,8 @@ void IPASoftSimple::stop()
> > void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &controls)
> > {
> > - IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.alloc(frame,
> > + context_.activeState);
> > for (auto const &algo : algorithms())
> > algo->queueRequest(context_, frame, frameContext, controls);
> > @@ -257,7 +258,8 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro
> > void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
> > {
> > - IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame,
> > + context_.activeState);
> > for (auto const &algo : algorithms())
> > algo->prepare(context_, frame, frameContext, params_);
> > setIspParams.emit();
> > @@ -267,7 +269,8 @@ void IPASoftSimple::processStats(const uint32_t frame,
> > [[maybe_unused]] const uint32_t bufferId,
> > const ControlList &sensorControls)
> > {
> > - IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame,
> > + context_.activeState);
> > frameContext.sensor.exposure =
> > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
More information about the libcamera-devel
mailing list