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

[Stefan Klug]

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

>  
>  			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, {})

Couldn't we just rename queueRequest to initFrameContext, so that we
don't have to implement two separate functions?

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

Best regards,
Stefan

> +	}
> +
>  	/*
>  	 * In raw capture mode, the ISP is bypassed and no statistics buffer is
>  	 * provided.
> -- 
> 2.47.0
>