[PATCH v2 2/2] ipa: rkisp1: Have algos initialize FrameContext

[Jacopo Mondi]

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

>
> >
> >  			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>

> >  /**
> >   * \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
> >