[PATCH 3/4] libipa: FCQueue: Initialize FrameContext with activeState
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Oct 28 12:00:10 CET 2024
Hi again, sorry missed one comment (sorry but not having empty lines
between the quoted text and your reply makes it quite terse to read
for me)
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
> result in .get() calls before .alloc() calls for a FrameContext?
> > 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?
Not in this patch, but probably in 1/4 where I make the FrameContext
class derivable by introducing a virtual method.
Thanks for spotting.
j
> > 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