[PATCH 3/4] libipa: FCQueue: Initialize FrameContext with activeState
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Oct 29 11:51:37 CET 2024
Hi Dan
On Tue, Oct 29, 2024 at 09:51:03AM +0000, Dan Scally wrote:
> Hi Jacopo
>
> On 28/10/2024 10:56, Jacopo Mondi wrote:
> > 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 ?
>
>
> More because it feels like we're stacking workarounds on top of a problem
> that it would be better to avoid if possible, though it may not be
> reasonably possible, in which case c'est la vie.
>
> >
> > > 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.
>
> Yeah ok. Do we get those Request underruns in the current implementation? I
Nope, because we clock the IPA with user provided Requests, so they
can't underrun :)
> lean towards the idea that perhaps alloc()ing the FrameContext in
> queueRequest() is the wrong approach when the IPA is clocked by the sensor,
> and perhaps we ought to switch to indexing the FCQueue using the sensor
> frame number...but I imagine that's a large bit of work.
>
that's indeed where we want to go, all the ph/ipa/fcq interface
should be modeled around the HW frame sequence number not the user
request number.
And yes, I presume we want to rework the IPA/FCQ interface for
alloc/get to better handle this, maybe involving algos as Stefan
suggested for the frame context initialisation.
I can have a look at how that would look like and I'm open to
suggestions ;)
> >
> > 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.
> Probably true.
Right now I'm debated between two models.
In one, where algos are more seen like pluggable modules, the
initialization of FrameContext is done by the FCQ using the active
state, as done in this patch, to make sure the FrameContext has all the
information required to be processed by all algorithms
implementations. This would make it easier to swap algos but might
lead to a more bloated (and costly) initialization in the FCQ
implementation.
On the other side, if we let algos initialize FrameContext when we
get/alloc it (using the ActiveState if the want to or defaults), the
algo implementation is self contained as there will be assumption made
in process()/prepare() on the initialization done with the algorithm
implementation itself.
I was more leaning towards the former, but maybe the latter is easier
and more self-contained. Opinions ?
> >
> > > > 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