[PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Nov 13 09:57:02 CET 2024
Hi Stefan
On Wed, Nov 13, 2024 at 09:43:55AM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> On Wed, Nov 13, 2024 at 08:35:24AM +0100, Jacopo Mondi wrote:
> > Hi Stefan
> > thanks for the review
> >
> > On Tue, Nov 12, 2024 at 05:35:52PM +0100, Stefan Klug wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > On Wed, Oct 30, 2024 at 05:48:52PM +0100, Jacopo Mondi wrote:
> > > > The RkISP1 AGC algorithms assumes the metering mode to be
> > > > "MeteringMatrix" and pre-fill an array of weights associated
> > > > with it stored in meteringModes_[MeteringMatrix] when intializing
> > > > the algorithm in parseMeteringModes().
> > > >
> > > > It laters fetches the weights when computing parameters using the
> > > > FrameContext.agc.meteringMode as index of the meteringModes_ array.
> > > >
> > > > When a Request gets queued to the algorithm, the
> > > > FrameContext.agc.meteringMode index is populated from the algorithm's
> > > > active state, and optionally updated if the application has specified
> > > > a different metering mode.
> > > >
> > > > In case of Request underrun however, a non-initialized FrameContext
> > > > gets provided to the algorithm, causing an (unhandled) exception when
> > > > accessing the meteringModes_ array.
> > > >
> > > > To handle the situation when a Request underrun occours and let
> > > > algorithms initialize a FrameContext to safe defaults:
> > > >
> > > > - Add an 'underrun' flag to FrameContext
> > > > - Add an 'initFrameContext()' function to RkISP1 algorithms
> > > > - If a FrameContext is get() before being allocated, pass it through
> > > > the algorithms' initFrameContext() to safely initialize it
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > ---
> > > > src/ipa/libipa/fc_queue.cpp | 6 ++++++
> > > > src/ipa/libipa/fc_queue.h | 5 +++++
> > > > src/ipa/rkisp1/algorithms/agc.cpp | 8 ++++++++
> > > > src/ipa/rkisp1/algorithms/agc.h | 4 ++++
> > > > src/ipa/rkisp1/algorithms/algorithm.h | 5 +++++
> > > > src/ipa/rkisp1/rkisp1.cpp | 9 +++++++++
> > > > 6 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > > index 0365e9197748..44f9d866ad1b 100644
> > > > --- a/src/ipa/libipa/fc_queue.cpp
> > > > +++ b/src/ipa/libipa/fc_queue.cpp
> > > > @@ -36,6 +36,12 @@ namespace ipa {
> > > > *
> > > > * \var FrameContext::frame
> > > > * \brief The frame number
> > > > + *
> > > > + * \var FrameContext::underrun
> > > > + * \brief Boolean flag that reports if the FrameContext has been accessed with
> > > > + * a FCQeueu::get() call before being FCQueue::alloc()-ated. If the flag is set
> > > > + * to True then the frame context needs to be initialized by algorithms to safe
> > > > + * defaults as it won't be initialized with any non-user provided control.
> > > > */
> > > >
> > > > /**
> > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > > > index a1d136521107..d70ca9601bd7 100644
> > > > --- a/src/ipa/libipa/fc_queue.h
> > > > +++ b/src/ipa/libipa/fc_queue.h
> > > > @@ -22,6 +22,9 @@ template<typename FrameContext>
> > > > class FCQueue;
> > > >
> > > > struct FrameContext {
> > > > +public:
> > > > + bool underrun = false;
> > > > +
> > > > private:
> > > > template<typename T> friend class FCQueue;
> > > > uint32_t frame;
> > > > @@ -97,6 +100,7 @@ public:
> > > > * is called before alloc() by the IPA for frame#0.
> > > > */
> > > > init(frameContext, frame);
> > > > + frameContext.underrun = true;
> > >
> > > I can't find the place where the flag is reset to false (In case there
> > > are temporary underruns).
> >
> > It happens in init()
> >
> > void init(FrameContext &frameContext, const uint32_t frame)
> > {
> > frameContext = {};
> >
> > and 'underrun' gets set to true only in the case get(x) is called
> > before alloc(x)
> >
> > I should probably reset it in clear() too though
> >
>
> Oh right, sorry for the noise.
>
> > >
> > > >
> > > > return frameContext;
> > > > }
> > > > @@ -117,6 +121,7 @@ public:
> > > > << "Obtained an uninitialised FrameContext for " << frame;
> > > >
> > > > init(frameContext, frame);
> > > > + frameContext.underrun = true;
> > > >
> > > > return frameContext;
> > > > }
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > index 301b7ec26508..4122f665b3ee 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > @@ -204,6 +204,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > > return 0;
> > > > }
> > > >
> > > > +void Agc::initFrameContext(IPAContext &context,
> > > > + IPAFrameContext &frameContext)
> > > > +{
> > > > + auto &agc = context.activeState.agc;
> > > > +
> > > > + frameContext.agc.meteringMode = agc.meteringMode;
> > > > +}
> > > > +
> > >
> > > I'm unsure if we really need the separate function.
> > >
> > > From an algorithm point of view, I can't see the difference between
> > > calling
> > >
> > > initFrameContext(ipaContext, frameContext)
> > >
> > > and
> > >
> > > queueRequest(ipaContext, frameContext.frame, frameContext, {})
> >
> > I like this way of thinking about it
> >
> > >
> > > Couldn't we just rename queueRequest to initFrameContext, so that we
> > > don't have to implement two separate functions?
> > >
> >
> > I like this ideas as well
> >
> > Let's bikeshed the names a bit
> >
> > we have
> >
> > init()
> > configure()
> > queueRequest()
> > prepare()
> > process()
> >
> > with the last three operations being called repetidly in a streaming
> > session.
> >
> > I was never in love with the "prepare()" and "process()" names, and I
> > would rather use "prepare()" for queueRequest(), but that's what we
> > have right now.
> >
> > The idea is that queueRequest actually initializes a frame context,
> > optionally with user provided controls. So I think that using
> > initFrameContext() is actually a good suggestion.
> >
> > <End of bikeshedding>
>
> Glad you like it. The only thing we should then clarify is the
> definition of frame. In the docs it says "The frame number is the
> Request sequence number and identifies the desired corresponding frame
> to target for the controls to take effect." I guess in the new pfc
> scheme that will become the internal/sensor frame number, and
> translation to request sequence happens outside. From an algorithm point
> of view that seems sensible.
We have a mix of Request::sequence and HW frame number in the doc and
implementation. We should aim to clearly separate the two. We'll get
there ;)
>
> Best regards,
> Stefan
>
> >
> > > > /**
> > > > * \copydoc libcamera::ipa::Algorithm::queueRequest
> > > > */
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > > > index aa86f2c5bc21..c1adf91bbc4e 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.h
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > > > @@ -30,6 +30,10 @@ public:
> > > >
> > > > int init(IPAContext &context, const YamlObject &tuningData) override;
> > > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > > > +
> > > > + void initFrameContext(IPAContext &context,
> > > > + IPAFrameContext &frameContext) override;
> > > > +
> > > > void queueRequest(IPAContext &context,
> > > > const uint32_t frame,
> > > > IPAFrameContext &frameContext,
> > > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> > > > index 715cfcd8298b..82603b7b372d 100644
> > > > --- a/src/ipa/rkisp1/algorithms/algorithm.h
> > > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> > > > @@ -23,6 +23,11 @@ public:
> > > > {
> > > > }
> > > >
> > > > + virtual void initFrameContext([[maybe_unused]] IPAContext &context,
> > > > + [[maybe_unused]] IPAFrameContext &frameContext)
> > > > + {
> > > > + }
> > > > +
> > > > bool disabled_;
> > > > bool supportsRaw_;
> > > > };
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 9e161cabdea4..b743de9ff6af 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -353,6 +353,15 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> > > > {
> > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > >
> > > > + if (frameContext.underrun) {
> > > > + for (auto const &a : algorithms()) {
> > > > + Algorithm *algo = static_cast<Algorithm *>(a.get());
> > > > + if (algo->disabled_)
> > > > + continue;
> > > > + algo->initFrameContext(context_, frameContext);
> > > > + }
> > >
> > > Maybe that would be the place for frameContext.underrun = false?
> >
> > I don't think the FrameContext status flags should be handled outside
> > of the FCQ implementation, otherwise each algo implementation would
> > have to do that correctly by its own.
> >
> > Thanks
> > j
> >
> > >
> > > Best regards,
> > > Stefan
> > >
> > > > + }
> > > > +
> > > > /*
> > > > * In raw capture mode, the ISP is bypassed and no statistics buffer is
> > > > * provided.
> > > > --
> > > > 2.47.0
> > > >
More information about the libcamera-devel
mailing list