[libcamera-devel,v2,04/10] ipa: libipa: Introduce FrameContextQueue

16979 diff mbox series

From: Umang Jain <umang.jain@ideasonboard.com>

Introduce a common implementation in libipa to represent the queue of
frame contexts.

Adjust the IPU3 IPAFrameContext to provide the basic class members
required to work with the generic FCQueue implementation, before
introducing an IPAFrameContext class in the next patches.

Opportunely handle cleaning up the queue and the IPA context at
IPA::stop() time.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/ipa/ipu3/ipa_context.cpp |  20 +------
 src/ipa/ipu3/ipa_context.h   |  16 ++---
 src/ipa/ipu3/ipu3.cpp        |  18 +++---
 src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
 src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
 src/ipa/libipa/meson.build   |   2 +
 src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
 7 files changed, 250 insertions(+), 38 deletions(-)
 create mode 100644 src/ipa/libipa/fc_queue.cpp
 create mode 100644 src/ipa/libipa/fc_queue.h

Hi all,

Few comments and some open question for discussion on underruns...

On 8/5/22 19:23, Jacopo Mondi wrote:
> From: Umang Jain <umang.jain@ideasonboard.com>
>
> Introduce a common implementation in libipa to represent the queue of
> frame contexts.
>
> Adjust the IPU3 IPAFrameContext to provide the basic class members
> required to work with the generic FCQueue implementation, before
> introducing an IPAFrameContext class in the next patches.
>
> Opportunely handle cleaning up the queue and the IPA context at
> IPA::stop() time.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   src/ipa/ipu3/ipa_context.cpp |  20 +------
>   src/ipa/ipu3/ipa_context.h   |  16 ++---
>   src/ipa/ipu3/ipu3.cpp        |  18 +++---
>   src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/meson.build   |   2 +
>   src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
>   7 files changed, 250 insertions(+), 38 deletions(-)
>   create mode 100644 src/ipa/libipa/fc_queue.cpp
>   create mode 100644 src/ipa/libipa/fc_queue.h
>
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 13cdb835ef7f..9605c8a1106d 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
>    * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>    */
>   
> -/**
> - * \brief Default constructor for IPAFrameContext
> - */
> -IPAFrameContext::IPAFrameContext() = default;
> -
> -/**
> - * \brief Construct a IPAFrameContext instance
> - */
> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> -	: frame(id), frameControls(reqControls)
> -{
> -	sensor = {};
> -}
> -
>   /**
>    * \var IPAFrameContext::frame
>    * \brief The frame number
>    *
> - * \var IPAFrameContext::frameControls
> - * \brief Controls sent in by the application while queuing the request
> - *
>    * \var IPAFrameContext::sensor
>    * \brief Effective sensor values that were applied for the frame
>    *
> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>    *
>    * \var IPAFrameContext::sensor.gain
>    * \brief Analogue gain multiplier
> + *
> + * \var IPAFrameContext::error
> + * \brief The error flags for this frame context
>    */
>   
>   } /* namespace libcamera::ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 42e11141d3a1..dc5b7c30aaf9 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -8,22 +8,20 @@
>   
>   #pragma once
>   
> -#include <array>
> -
>   #include <linux/intel-ipu3.h>
>   
>   #include <libcamera/base/utils.h>
>   
>   #include <libcamera/controls.h>
>   #include <libcamera/geometry.h>
> +#include <libcamera/request.h>
> +
> +#include <libipa/fc_queue.h>
>   
>   namespace libcamera {
>   
>   namespace ipa::ipu3 {
>   
> -/* Maximum number of frame contexts to be held */
> -static constexpr uint32_t kMaxFrameContexts = 16;
> -
>   struct IPASessionConfiguration {
>   	struct {
>   		ipu3_uapi_grid_config bdsGrid;
> @@ -77,23 +75,21 @@ struct IPAActiveState {
>   };
>   
>   struct IPAFrameContext {
> -	IPAFrameContext();
> -	IPAFrameContext(uint32_t id, const ControlList &reqControls);
> +	uint32_t frame;
>   
>   	struct {
>   		uint32_t exposure;
>   		double gain;
>   	} sensor;
>   
> -	uint32_t frame;
> -	ControlList frameControls;
> +	Request::Errors error;
>   };
>   
>   struct IPAContext {
>   	IPASessionConfiguration configuration;
>   	IPAActiveState activeState;
>   
> -	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> +	FCQueue<IPAFrameContext> frameContexts;
>   };
>   
>   } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672f7bb..209a6b040f8f 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -38,6 +38,8 @@
>   #include "algorithms/tone_mapping.h"
>   #include "libipa/camera_sensor_helper.h"
>   
> +#include "ipa_context.h"
> +
>   /* Minimum grid width, expressed as a number of cells */
>   static constexpr uint32_t kMinGridWidth = 16;
>   /* Maximum grid width, expressed as a number of cells */
> @@ -348,6 +350,9 @@ int IPAIPU3::start()
>    */
>   void IPAIPU3::stop()
>   {
> +	/* Clean the IPA context at the end of the streaming session. */
> +	context_.frameContexts.clear();
> +	context_ = {};
>   }
>   
>   /**
> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   
>   	calculateBdsGrid(configInfo.bdsOutputSize);
>   
> -	/* Clean IPAActiveState at each reconfiguration. */
> -	context_.activeState = {};
> -	IPAFrameContext initFrameContext;
> -	context_.frameContexts.fill(initFrameContext);
> -
>   	if (!validateSensorControls()) {
>   		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>   		return -EINVAL;
> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>   
>   	if (frameContext.frame != frame)
>   		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>   {
>   	/* \todo Start processing for 'frame' based on 'controls'. */
> -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> +	IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> +
> +	/* \todo Implement queueRequest to each algorithm. */
> +	(void)frameContext;
> +	(void)controls;
>   }
>   
>   /**
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> new file mode 100644
> index 000000000000..37ffca60b992
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * fc_queue.cpp - IPA Frame context queue
> + */
> +
> +#include "fc_queue.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(FCQueue)
> +
> +namespace ipa {
> +
> +/**
> + * \file fc_queue.h
> + * \brief Queue to access per-frame Context
> + */
> +
> +/**
> + * \class FCQueue
> + * \brief A support class for queueing FrameContext instances through the IPA
> + * \tparam FrameContext The IPA specific FrameContext derived class type
> + *
> + * The frame context queue provides a simple circular buffer implementation to
> + * store IPA specific context for each frame through its lifetime within the
> + * IPA.
> + *
> + * FrameContext instances are expected to be referenced by a monotonically
> + * increasing sequence count corresponding to a frame sequence number.
> + *
> + * A FrameContext is initialised for a given frame when the corresponding
> + * Request is first queued into the IPA. From that point on the FrameContext can
> + * be obtained by the IPA and its algorithms by referencing it from the frame
> + * sequence number.
> + *
> + * A frame sequence number from the image source must correspond to the request
> + * sequence number for this implementation to be supported in an IPA. It is
> + * expected that the same sequence number will be used to reference the context
> + * of the Frame from the point of queueing the request, specifying controls for


s/Frame/frame

> + * a given frame, and processing of any ISP related controls and statistics for
> + * the same corresponding image.
> + *
> + * IPA specific frame context implementations shall inherit from the
> + * IPAFrameContext base class to support the minimum required features for a
> + * FrameContext, including the frame number and the error flags that relate to
> + * the frame.
> + *
> + * FrameContexts are overwritten when they are recycled and re-initialised by
> + * the first access made on them by either initialise(frame) or get(frame). It
> + * is required that the number of available slots in the frame context queue is
> + * larger or equal to the maximum number of in-flight requests a pipeline can
> + * handle to avoid overwriting frame context instances that still have to be
> + * processed.
> + *
> + * In the case an application does not queue requests to the Camera fast


s/Camera/camera/

> + * enough, frames can be produced and processed by the IPA without a
> + * corresponding Request being queued. In this case the IPA algorithm
> + * will try to access the FrameContext with a call to get() before it
> + * had been initialized at queueRequest() time. In this case there


s/case/case,

> + * is now way the controls associated with the late Request could be


s/now/no

> + * applied in time, as the frame as already been processed by the IPA,
> + * the PFCError flag is automatically raised on the FrameContext.
> + */
> +
> +/**
> + * \fn FCQueue::clear()
> + * \brief Clear the context queue
> + *
> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> + * IPA modules are expected to clear the frame context queue at the beginning of
> + * a new streaming session, in IPAModule::start().
> + */
> +
> +/**
> + * \fn FCQueue::initialise(uint32_t frame)
> + * \brief Initialize and return the Frame Context at slot specified by \a frame
> + * \param[in] frame The frame context sequence number
> + *
> + * The first call to obtain a FrameContext from the FCQueue should be handled
> + * through this call. The FrameContext will be initialised for the frame and
> + * returned to the caller if it was not already initialised.
> + *
> + * If the FrameContext was already initialized for this sequence, a warning will
> + * be reported and the previously initialized FrameContext is returned.
> + *
> + * Frame contexts are expected to be initialised when a Request is first passed
> + * to the IPA module in IPAModule::queueRequest().
> + *
> + * \return A reference to the FrameContext for sequence \a frame
> + */
> +
> +/**
> + * \fn FCQueue::get()
> + * \brief Obtain the Frame Context at slot specified by \a frame
> + * \param[in] frame The frame context sequence number
> + *
> + * Obtains an existing FrameContext from the queue and returns it to the caller.
> + *
> + * If the FrameContext is not correctly initialised for the \a frame, it will be
> + * initialised and a PFCError will be flagged on the context to be transferred
> + * to the Request when it completes.
> + *
> + * \return A reference to the FrameContext for sequence \a frame
> + */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> new file mode 100644
> index 000000000000..023b52420fe7
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * fc_queue.h - IPA Frame context queue
> + */
> +
> +#pragma once
> +
> +#include <array>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/request.h>
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(FCQueue)
> +
> +namespace ipa {
> +
> +/*
> + * Maximum number of frame contexts to be held onto
> + *
> + * \todo Should be platform-specific and match the pipeline depth
> + */
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
> +template<typename FrameContext>
> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> +{
> +private:
> +	void initialise(FrameContext &frameContext, const uint32_t frame)
> +	{
> +		frameContext = {};
> +		frameContext.frame = frame;
> +	}
> +
> +public:
> +	void clear()
> +	{
> +		this->fill({});
> +	}
> +
> +	FrameContext &initialise(const uint32_t frame)
> +	{
> +		FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> +
> +		/*
> +		 * Do not re-initialise if a get() call has already fetched this
> +		 * frame context to preseve the error flags already raised.
> +		 *
> +		 * \todo If the the sequence number of the context to initialise
> +		 * is smaller than the sequence number of the queue slot to use,
> +		 * it means that we had a serious request underrun and more
> +		 * frames than the queue size has been produced since the last
> +		 * time the application has queued a request. Does this deserve
> +		 * an error condition ?


I believe we should log under-runs conditions. Whether we log it as 
ERROR or WARN or DEBUG might be a candidate for discussion. It depends 
on how we classify such condition

Are under-run requests expected to completed with non-fatal errors set? 
If yes,  I believe we set RequestError::Underrun here and return the 
initialised frameContext. But then how the Request will be completed ? 
As the frame number passed here is quite < the current ongoing capture 
sequence on sensor side. So, we might end up initialising the context 
but it's never .get() with this frame number, for e.g. while populating 
metadata in processStatsBuffer()? So we might require some special 
handling there to detect that a frameContext belongs to a under-run 
request or not.

I think I need to map out how will a under-run request will get 
completed successfully - IFF we are supporting that case.

> +		 */
> +		if (frame != 0 && frame <= frameContext.frame)
> +			LOG(FCQueue, Warning)
> +				<< "Frame " << frame << " already initialised";
> +		else
> +			initialise(frameContext, frame);
> +
> +		return frameContext;
> +	}
> +
> +	FrameContext &get(uint32_t frame)
> +	{
> +		FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> +
> +		/*
> +		 * If the IPA algorithms try to access a frame context slot which
> +		 * has been already overwritten by a newer context, it means the
> +		 * frame context queue has overflowed and the desired context
> +		 * has been forever lost. The pipeline handler shall avoid
> +		 * queueing more requests to the IPA than the frame context
> +		 * queue size.
> +		 */
> +		if (frame < frameContext.frame)
> +			LOG(FCQueue, Fatal)
> +				<< "Frame " << frame << " has been overwritten";


Maybe the log can be improved:

	LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
			    << frameContext.frame;

> +
> +		if (frame == frameContext.frame)
> +			return frameContext;
> +
> +		LOG(FCQueue, Warning)
> +			<< "Obtained an uninitialised FrameContext for " << frame;


I think currently this Warning is being logged for both cases :

     "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"

Is this expected ? Or are we counting on the "Fatal" log above, which 
shall terminate the entire program?

> +
> +		initialise(frameContext, frame);
> +
> +		/*
> +		 * The frame context has been retrieved before it was
> +		 * initialised through the initialise() call. This indicates an
> +		 * algorithm attempted to access a Frame context before it was
> +		 * queued to the IPA.
> +		 *
> +		 * Controls applied for this request may be left unhandled.
> +		 */
> +		frameContext.error |= Request::PFCError;
> +
> +		return frameContext;
> +	}
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index fb894bc614af..016b8e0ec9be 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -3,6 +3,7 @@
>   libipa_headers = files([
>       'algorithm.h',
>       'camera_sensor_helper.h',
> +    'fc_queue.h',
>       'histogram.h',
>       'module.h',
>   ])
> @@ -10,6 +11,7 @@ libipa_headers = files([
>   libipa_sources = files([
>       'algorithm.cpp',
>       'camera_sensor_helper.cpp',
> +    'fc_queue.cpp',
>       'histogram.cpp',
>       'module.cpp',
>   ])
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 6cf4d1699ce5..af22dbeb3da5 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -49,7 +49,7 @@ public:
>   	int init(const IPASettings &settings, unsigned int hwRevision,
>   		 ControlInfoMap *ipaControls) override;
>   	int start() override;
> -	void stop() override {}
> +	void stop() override;
>   
>   	int configure(const IPACameraSensorInfo &info,
>   		      const std::map<uint32_t, IPAStream> &streamConfig,
> @@ -189,6 +189,12 @@ int IPARkISP1::start()
>   	return 0;
>   }
>   
> +void IPARkISP1::stop()
> +{
> +	/* Clean the IPA context at the end of the streaming session. */
> +	context_ = {};
> +}
> +
>   /**
>    * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
>    * if the connected sensor does not provide enough information to properly
> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>   		<< "Exposure: " << minExposure << "-" << maxExposure
>   		<< " Gain: " << minGain << "-" << maxGain;
>   
> -	/* Clean context at configuration */
> -	context_ = {};
> -
>   	/* Set the hardware revision for the algorithms. */
>   	context_.configuration.hw.revision = hwRevision_;
>

Hi Umang

On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> Hi all,
>
> Few comments and some open question for discussion on underruns...
>
> On 8/5/22 19:23, Jacopo Mondi wrote:
> > From: Umang Jain <umang.jain@ideasonboard.com>
> >
> > Introduce a common implementation in libipa to represent the queue of
> > frame contexts.
> >
> > Adjust the IPU3 IPAFrameContext to provide the basic class members
> > required to work with the generic FCQueue implementation, before
> > introducing an IPAFrameContext class in the next patches.
> >
> > Opportunely handle cleaning up the queue and the IPA context at
> > IPA::stop() time.
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >   src/ipa/ipu3/ipa_context.cpp |  20 +------
> >   src/ipa/ipu3/ipa_context.h   |  16 ++---
> >   src/ipa/ipu3/ipu3.cpp        |  18 +++---
> >   src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/meson.build   |   2 +
> >   src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
> >   7 files changed, 250 insertions(+), 38 deletions(-)
> >   create mode 100644 src/ipa/libipa/fc_queue.cpp
> >   create mode 100644 src/ipa/libipa/fc_queue.h
> >
> > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > index 13cdb835ef7f..9605c8a1106d 100644
> > --- a/src/ipa/ipu3/ipa_context.cpp
> > +++ b/src/ipa/ipu3/ipa_context.cpp
> > @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> >    * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> >    */
> > -/**
> > - * \brief Default constructor for IPAFrameContext
> > - */
> > -IPAFrameContext::IPAFrameContext() = default;
> > -
> > -/**
> > - * \brief Construct a IPAFrameContext instance
> > - */
> > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > -	: frame(id), frameControls(reqControls)
> > -{
> > -	sensor = {};
> > -}
> > -
> >   /**
> >    * \var IPAFrameContext::frame
> >    * \brief The frame number
> >    *
> > - * \var IPAFrameContext::frameControls
> > - * \brief Controls sent in by the application while queuing the request
> > - *
> >    * \var IPAFrameContext::sensor
> >    * \brief Effective sensor values that were applied for the frame
> >    *
> > @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> >    *
> >    * \var IPAFrameContext::sensor.gain
> >    * \brief Analogue gain multiplier
> > + *
> > + * \var IPAFrameContext::error
> > + * \brief The error flags for this frame context
> >    */
> >   } /* namespace libcamera::ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index 42e11141d3a1..dc5b7c30aaf9 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -8,22 +8,20 @@
> >   #pragma once
> > -#include <array>
> > -
> >   #include <linux/intel-ipu3.h>
> >   #include <libcamera/base/utils.h>
> >   #include <libcamera/controls.h>
> >   #include <libcamera/geometry.h>
> > +#include <libcamera/request.h>
> > +
> > +#include <libipa/fc_queue.h>
> >   namespace libcamera {
> >   namespace ipa::ipu3 {
> > -/* Maximum number of frame contexts to be held */
> > -static constexpr uint32_t kMaxFrameContexts = 16;
> > -
> >   struct IPASessionConfiguration {
> >   	struct {
> >   		ipu3_uapi_grid_config bdsGrid;
> > @@ -77,23 +75,21 @@ struct IPAActiveState {
> >   };
> >   struct IPAFrameContext {
> > -	IPAFrameContext();
> > -	IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > +	uint32_t frame;
> >   	struct {
> >   		uint32_t exposure;
> >   		double gain;
> >   	} sensor;
> > -	uint32_t frame;
> > -	ControlList frameControls;
> > +	Request::Errors error;
> >   };
> >   struct IPAContext {
> >   	IPASessionConfiguration configuration;
> >   	IPAActiveState activeState;
> > -	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> > +	FCQueue<IPAFrameContext> frameContexts;
> >   };
> >   } /* namespace ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 2f6bb672f7bb..209a6b040f8f 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -38,6 +38,8 @@
> >   #include "algorithms/tone_mapping.h"
> >   #include "libipa/camera_sensor_helper.h"
> > +#include "ipa_context.h"
> > +
> >   /* Minimum grid width, expressed as a number of cells */
> >   static constexpr uint32_t kMinGridWidth = 16;
> >   /* Maximum grid width, expressed as a number of cells */
> > @@ -348,6 +350,9 @@ int IPAIPU3::start()
> >    */
> >   void IPAIPU3::stop()
> >   {
> > +	/* Clean the IPA context at the end of the streaming session. */
> > +	context_.frameContexts.clear();
> > +	context_ = {};
> >   }
> >   /**
> > @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >   	calculateBdsGrid(configInfo.bdsOutputSize);
> > -	/* Clean IPAActiveState at each reconfiguration. */
> > -	context_.activeState = {};
> > -	IPAFrameContext initFrameContext;
> > -	context_.frameContexts.fill(initFrameContext);
> > -
> >   	if (!validateSensorControls()) {
> >   		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> >   		return -EINVAL;
> > @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> > +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> >   	if (frameContext.frame != frame)
> >   		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> > @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> >   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> >   {
> >   	/* \todo Start processing for 'frame' based on 'controls'. */
> > -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > +	IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> > +
> > +	/* \todo Implement queueRequest to each algorithm. */
> > +	(void)frameContext;
> > +	(void)controls;
> >   }
> >   /**
> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > new file mode 100644
> > index 000000000000..37ffca60b992
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.cpp
> > @@ -0,0 +1,112 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * fc_queue.cpp - IPA Frame context queue
> > + */
> > +
> > +#include "fc_queue.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(FCQueue)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \file fc_queue.h
> > + * \brief Queue to access per-frame Context
> > + */
> > +
> > +/**
> > + * \class FCQueue
> > + * \brief A support class for queueing FrameContext instances through the IPA
> > + * \tparam FrameContext The IPA specific FrameContext derived class type
> > + *
> > + * The frame context queue provides a simple circular buffer implementation to
> > + * store IPA specific context for each frame through its lifetime within the
> > + * IPA.
> > + *
> > + * FrameContext instances are expected to be referenced by a monotonically
> > + * increasing sequence count corresponding to a frame sequence number.
> > + *
> > + * A FrameContext is initialised for a given frame when the corresponding
> > + * Request is first queued into the IPA. From that point on the FrameContext can
> > + * be obtained by the IPA and its algorithms by referencing it from the frame
> > + * sequence number.
> > + *
> > + * A frame sequence number from the image source must correspond to the request
> > + * sequence number for this implementation to be supported in an IPA. It is
> > + * expected that the same sequence number will be used to reference the context
> > + * of the Frame from the point of queueing the request, specifying controls for
>
>
> s/Frame/frame
>
> > + * a given frame, and processing of any ISP related controls and statistics for
> > + * the same corresponding image.
> > + *
> > + * IPA specific frame context implementations shall inherit from the
> > + * IPAFrameContext base class to support the minimum required features for a
> > + * FrameContext, including the frame number and the error flags that relate to
> > + * the frame.
> > + *
> > + * FrameContexts are overwritten when they are recycled and re-initialised by
> > + * the first access made on them by either initialise(frame) or get(frame). It
> > + * is required that the number of available slots in the frame context queue is
> > + * larger or equal to the maximum number of in-flight requests a pipeline can
> > + * handle to avoid overwriting frame context instances that still have to be
> > + * processed.
> > + *
> > + * In the case an application does not queue requests to the Camera fast
>
>
> s/Camera/camera/
>
> > + * enough, frames can be produced and processed by the IPA without a
> > + * corresponding Request being queued. In this case the IPA algorithm
> > + * will try to access the FrameContext with a call to get() before it
> > + * had been initialized at queueRequest() time. In this case there
>
>
> s/case/case,
>
> > + * is now way the controls associated with the late Request could be
>
>
> s/now/no
>

Ack to all the above ones, thanks

> > + * applied in time, as the frame as already been processed by the IPA,
> > + * the PFCError flag is automatically raised on the FrameContext.
> > + */
> > +
> > +/**
> > + * \fn FCQueue::clear()
> > + * \brief Clear the context queue
> > + *
> > + * Reset the queue and ensure all FrameContext slots are re-initialised.
> > + * IPA modules are expected to clear the frame context queue at the beginning of
> > + * a new streaming session, in IPAModule::start().
> > + */
> > +
> > +/**
> > + * \fn FCQueue::initialise(uint32_t frame)
> > + * \brief Initialize and return the Frame Context at slot specified by \a frame
> > + * \param[in] frame The frame context sequence number
> > + *
> > + * The first call to obtain a FrameContext from the FCQueue should be handled
> > + * through this call. The FrameContext will be initialised for the frame and
> > + * returned to the caller if it was not already initialised.
> > + *
> > + * If the FrameContext was already initialized for this sequence, a warning will
> > + * be reported and the previously initialized FrameContext is returned.
> > + *
> > + * Frame contexts are expected to be initialised when a Request is first passed
> > + * to the IPA module in IPAModule::queueRequest().
> > + *
> > + * \return A reference to the FrameContext for sequence \a frame
> > + */
> > +
> > +/**
> > + * \fn FCQueue::get()
> > + * \brief Obtain the Frame Context at slot specified by \a frame
> > + * \param[in] frame The frame context sequence number
> > + *
> > + * Obtains an existing FrameContext from the queue and returns it to the caller.
> > + *
> > + * If the FrameContext is not correctly initialised for the \a frame, it will be
> > + * initialised and a PFCError will be flagged on the context to be transferred
> > + * to the Request when it completes.
> > + *
> > + * \return A reference to the FrameContext for sequence \a frame
> > + */
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > new file mode 100644
> > index 000000000000..023b52420fe7
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.h
> > @@ -0,0 +1,109 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * fc_queue.h - IPA Frame context queue
> > + */
> > +
> > +#pragma once
> > +
> > +#include <array>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/request.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(FCQueue)
> > +
> > +namespace ipa {
> > +
> > +/*
> > + * Maximum number of frame contexts to be held onto
> > + *
> > + * \todo Should be platform-specific and match the pipeline depth
> > + */
> > +static constexpr uint32_t kMaxFrameContexts = 16;
> > +
> > +template<typename FrameContext>
> > +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> > +{
> > +private:
> > +	void initialise(FrameContext &frameContext, const uint32_t frame)
> > +	{
> > +		frameContext = {};
> > +		frameContext.frame = frame;
> > +	}
> > +
> > +public:
> > +	void clear()
> > +	{
> > +		this->fill({});
> > +	}
> > +
> > +	FrameContext &initialise(const uint32_t frame)
> > +	{
> > +		FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > +
> > +		/*
> > +		 * Do not re-initialise if a get() call has already fetched this
> > +		 * frame context to preseve the error flags already raised.
> > +		 *
> > +		 * \todo If the the sequence number of the context to initialise
> > +		 * is smaller than the sequence number of the queue slot to use,
> > +		 * it means that we had a serious request underrun and more
> > +		 * frames than the queue size has been produced since the last
> > +		 * time the application has queued a request. Does this deserve
> > +		 * an error condition ?
>
>
> I believe we should log under-runs conditions. Whether we log it as ERROR or
> WARN or DEBUG might be a candidate for discussion. It depends on how we
> classify such condition
>
> Are under-run requests expected to completed with non-fatal errors set? If

They indeed expect to be completed :)

> yes,  I believe we set RequestError::Underrun here and return the
> initialised frameContext. But then how the Request will be completed ? As
> the frame number passed here is quite < the current ongoing capture sequence
> on sensor side. So, we might end up initialising the context but it's never
> .get() with this frame number, for e.g. while populating metadata in
> processStatsBuffer()? So we might require some special handling there to
> detect that a frameContext belongs to a under-run request or not.
>
> I think I need to map out how will a under-run request will get completed
> successfully - IFF we are supporting that case.
>

There are legit question, but to properly reply to them I think we
need to finalise the design of the per-frame-control model. In
example, for some devices with limited memory (Pi Zero, in example),
operating in underrun mode seems to be the default, and we have to
handle that case properly.

> > +		 */
> > +		if (frame != 0 && frame <= frameContext.frame)
> > +			LOG(FCQueue, Warning)
> > +				<< "Frame " << frame << " already initialised";
> > +		else
> > +			initialise(frameContext, frame);
> > +
> > +		return frameContext;
> > +	}
> > +
> > +	FrameContext &get(uint32_t frame)
> > +	{
> > +		FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > +
> > +		/*
> > +		 * If the IPA algorithms try to access a frame context slot which
> > +		 * has been already overwritten by a newer context, it means the
> > +		 * frame context queue has overflowed and the desired context
> > +		 * has been forever lost. The pipeline handler shall avoid
> > +		 * queueing more requests to the IPA than the frame context
> > +		 * queue size.
> > +		 */
> > +		if (frame < frameContext.frame)
> > +			LOG(FCQueue, Fatal)
> > +				<< "Frame " << frame << " has been overwritten";
>
>
> Maybe the log can be improved:
>
> 	LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> 			    << frameContext.frame;
>
> > +
> > +		if (frame == frameContext.frame)
> > +			return frameContext;
> > +
> > +		LOG(FCQueue, Warning)
> > +			<< "Obtained an uninitialised FrameContext for " << frame;
>
>
> I think currently this Warning is being logged for both cases :
>
>     "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"

The
		if (frame < frameContext.frame)
			LOG(FCQueue, Fatal)
				<< "Frame " << frame << " has been overwritten";

will abort execution, so we don't get to this warning.

>
> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> terminate the entire program?
>

That's the idea, yes.

The above Fatal is triggered by a programming error in the PH/IPA as
we should never get more requests queued than the FCQ size, so the
assertion above should not be triggered if not during development of
new platforms.

> > +
> > +		initialise(frameContext, frame);
> > +
> > +		/*
> > +		 * The frame context has been retrieved before it was
> > +		 * initialised through the initialise() call. This indicates an
> > +		 * algorithm attempted to access a Frame context before it was
> > +		 * queued to the IPA.
> > +		 *
> > +		 * Controls applied for this request may be left unhandled.
> > +		 */
> > +		frameContext.error |= Request::PFCError;
> > +
> > +		return frameContext;
> > +	}
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index fb894bc614af..016b8e0ec9be 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -3,6 +3,7 @@
> >   libipa_headers = files([
> >       'algorithm.h',
> >       'camera_sensor_helper.h',
> > +    'fc_queue.h',
> >       'histogram.h',
> >       'module.h',
> >   ])
> > @@ -10,6 +11,7 @@ libipa_headers = files([
> >   libipa_sources = files([
> >       'algorithm.cpp',
> >       'camera_sensor_helper.cpp',
> > +    'fc_queue.cpp',
> >       'histogram.cpp',
> >       'module.cpp',
> >   ])
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 6cf4d1699ce5..af22dbeb3da5 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -49,7 +49,7 @@ public:
> >   	int init(const IPASettings &settings, unsigned int hwRevision,
> >   		 ControlInfoMap *ipaControls) override;
> >   	int start() override;
> > -	void stop() override {}
> > +	void stop() override;
> >   	int configure(const IPACameraSensorInfo &info,
> >   		      const std::map<uint32_t, IPAStream> &streamConfig,
> > @@ -189,6 +189,12 @@ int IPARkISP1::start()
> >   	return 0;
> >   }
> > +void IPARkISP1::stop()
> > +{
> > +	/* Clean the IPA context at the end of the streaming session. */
> > +	context_ = {};
> > +}
> > +
> >   /**
> >    * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> >    * if the connected sensor does not provide enough information to properly
> > @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >   		<< "Exposure: " << minExposure << "-" << maxExposure
> >   		<< " Gain: " << minGain << "-" << maxGain;
> > -	/* Clean context at configuration */
> > -	context_ = {};
> > -
> >   	/* Set the hardware revision for the algorithms. */
> >   	context_.configuration.hw.revision = hwRevision_;

Hi Jacopo,

Thank you for the clarifications,

On 8/11/22 14:08, Jacopo Mondi wrote:
> Hi Umang
>
> On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
>> Hi all,
>>
>> Few comments and some open question for discussion on underruns...
>>
>> On 8/5/22 19:23, Jacopo Mondi wrote:
>>> From: Umang Jain <umang.jain@ideasonboard.com>
>>>
>>> Introduce a common implementation in libipa to represent the queue of
>>> frame contexts.
>>>
>>> Adjust the IPU3 IPAFrameContext to provide the basic class members
>>> required to work with the generic FCQueue implementation, before
>>> introducing an IPAFrameContext class in the next patches.
>>>
>>> Opportunely handle cleaning up the queue and the IPA context at
>>> IPA::stop() time.
>>>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>    src/ipa/ipu3/ipa_context.cpp |  20 +------
>>>    src/ipa/ipu3/ipa_context.h   |  16 ++---
>>>    src/ipa/ipu3/ipu3.cpp        |  18 +++---
>>>    src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
>>>    src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
>>>    src/ipa/libipa/meson.build   |   2 +
>>>    src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
>>>    7 files changed, 250 insertions(+), 38 deletions(-)
>>>    create mode 100644 src/ipa/libipa/fc_queue.cpp
>>>    create mode 100644 src/ipa/libipa/fc_queue.h
>>>
>>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>>> index 13cdb835ef7f..9605c8a1106d 100644
>>> --- a/src/ipa/ipu3/ipa_context.cpp
>>> +++ b/src/ipa/ipu3/ipa_context.cpp
>>> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
>>>     * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>>>     */
>>> -/**
>>> - * \brief Default constructor for IPAFrameContext
>>> - */
>>> -IPAFrameContext::IPAFrameContext() = default;
>>> -
>>> -/**
>>> - * \brief Construct a IPAFrameContext instance
>>> - */
>>> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>>> -	: frame(id), frameControls(reqControls)
>>> -{
>>> -	sensor = {};
>>> -}
>>> -
>>>    /**
>>>     * \var IPAFrameContext::frame
>>>     * \brief The frame number
>>>     *
>>> - * \var IPAFrameContext::frameControls
>>> - * \brief Controls sent in by the application while queuing the request
>>> - *
>>>     * \var IPAFrameContext::sensor
>>>     * \brief Effective sensor values that were applied for the frame
>>>     *
>>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>>>     *
>>>     * \var IPAFrameContext::sensor.gain
>>>     * \brief Analogue gain multiplier
>>> + *
>>> + * \var IPAFrameContext::error
>>> + * \brief The error flags for this frame context
>>>     */
>>>    } /* namespace libcamera::ipa::ipu3 */
>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>>> index 42e11141d3a1..dc5b7c30aaf9 100644
>>> --- a/src/ipa/ipu3/ipa_context.h
>>> +++ b/src/ipa/ipu3/ipa_context.h
>>> @@ -8,22 +8,20 @@
>>>    #pragma once
>>> -#include <array>
>>> -
>>>    #include <linux/intel-ipu3.h>
>>>    #include <libcamera/base/utils.h>
>>>    #include <libcamera/controls.h>
>>>    #include <libcamera/geometry.h>
>>> +#include <libcamera/request.h>
>>> +
>>> +#include <libipa/fc_queue.h>
>>>    namespace libcamera {
>>>    namespace ipa::ipu3 {
>>> -/* Maximum number of frame contexts to be held */
>>> -static constexpr uint32_t kMaxFrameContexts = 16;
>>> -
>>>    struct IPASessionConfiguration {
>>>    	struct {
>>>    		ipu3_uapi_grid_config bdsGrid;
>>> @@ -77,23 +75,21 @@ struct IPAActiveState {
>>>    };
>>>    struct IPAFrameContext {
>>> -	IPAFrameContext();
>>> -	IPAFrameContext(uint32_t id, const ControlList &reqControls);
>>> +	uint32_t frame;
>>>    	struct {
>>>    		uint32_t exposure;
>>>    		double gain;
>>>    	} sensor;
>>> -	uint32_t frame;
>>> -	ControlList frameControls;
>>> +	Request::Errors error;
>>>    };
>>>    struct IPAContext {
>>>    	IPASessionConfiguration configuration;
>>>    	IPAActiveState activeState;
>>> -	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>>> +	FCQueue<IPAFrameContext> frameContexts;
>>>    };
>>>    } /* namespace ipa::ipu3 */
>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>> index 2f6bb672f7bb..209a6b040f8f 100644
>>> --- a/src/ipa/ipu3/ipu3.cpp
>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>> @@ -38,6 +38,8 @@
>>>    #include "algorithms/tone_mapping.h"
>>>    #include "libipa/camera_sensor_helper.h"
>>> +#include "ipa_context.h"
>>> +
>>>    /* Minimum grid width, expressed as a number of cells */
>>>    static constexpr uint32_t kMinGridWidth = 16;
>>>    /* Maximum grid width, expressed as a number of cells */
>>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
>>>     */
>>>    void IPAIPU3::stop()
>>>    {
>>> +	/* Clean the IPA context at the end of the streaming session. */
>>> +	context_.frameContexts.clear();
>>> +	context_ = {};
>>>    }
>>>    /**
>>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>>    	calculateBdsGrid(configInfo.bdsOutputSize);
>>> -	/* Clean IPAActiveState at each reconfiguration. */
>>> -	context_.activeState = {};
>>> -	IPAFrameContext initFrameContext;
>>> -	context_.frameContexts.fill(initFrameContext);
>>> -
>>>    	if (!validateSensorControls()) {
>>>    		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>>>    		return -EINVAL;
>>> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
>>> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>>>    	if (frameContext.frame != frame)
>>>    		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
>>> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>>    void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>>>    {
>>>    	/* \todo Start processing for 'frame' based on 'controls'. */
>>> -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
>>> +	IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
>>> +
>>> +	/* \todo Implement queueRequest to each algorithm. */
>>> +	(void)frameContext;
>>> +	(void)controls;
>>>    }
>>>    /**
>>> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
>>> new file mode 100644
>>> index 000000000000..37ffca60b992
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/fc_queue.cpp
>>> @@ -0,0 +1,112 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2022, Google Inc.
>>> + *
>>> + * fc_queue.cpp - IPA Frame context queue
>>> + */
>>> +
>>> +#include "fc_queue.h"
>>> +
>>> +#include <libcamera/base/log.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +LOG_DEFINE_CATEGORY(FCQueue)
>>> +
>>> +namespace ipa {
>>> +
>>> +/**
>>> + * \file fc_queue.h
>>> + * \brief Queue to access per-frame Context
>>> + */
>>> +
>>> +/**
>>> + * \class FCQueue
>>> + * \brief A support class for queueing FrameContext instances through the IPA
>>> + * \tparam FrameContext The IPA specific FrameContext derived class type
>>> + *
>>> + * The frame context queue provides a simple circular buffer implementation to
>>> + * store IPA specific context for each frame through its lifetime within the
>>> + * IPA.
>>> + *
>>> + * FrameContext instances are expected to be referenced by a monotonically
>>> + * increasing sequence count corresponding to a frame sequence number.
>>> + *
>>> + * A FrameContext is initialised for a given frame when the corresponding
>>> + * Request is first queued into the IPA. From that point on the FrameContext can
>>> + * be obtained by the IPA and its algorithms by referencing it from the frame
>>> + * sequence number.
>>> + *
>>> + * A frame sequence number from the image source must correspond to the request
>>> + * sequence number for this implementation to be supported in an IPA. It is
>>> + * expected that the same sequence number will be used to reference the context
>>> + * of the Frame from the point of queueing the request, specifying controls for
>>
>> s/Frame/frame
>>
>>> + * a given frame, and processing of any ISP related controls and statistics for
>>> + * the same corresponding image.
>>> + *
>>> + * IPA specific frame context implementations shall inherit from the
>>> + * IPAFrameContext base class to support the minimum required features for a
>>> + * FrameContext, including the frame number and the error flags that relate to
>>> + * the frame.
>>> + *
>>> + * FrameContexts are overwritten when they are recycled and re-initialised by
>>> + * the first access made on them by either initialise(frame) or get(frame). It
>>> + * is required that the number of available slots in the frame context queue is
>>> + * larger or equal to the maximum number of in-flight requests a pipeline can
>>> + * handle to avoid overwriting frame context instances that still have to be
>>> + * processed.
>>> + *
>>> + * In the case an application does not queue requests to the Camera fast
>>
>> s/Camera/camera/
>>
>>> + * enough, frames can be produced and processed by the IPA without a
>>> + * corresponding Request being queued. In this case the IPA algorithm
>>> + * will try to access the FrameContext with a call to get() before it
>>> + * had been initialized at queueRequest() time. In this case there
>>
>> s/case/case,
>>
>>> + * is now way the controls associated with the late Request could be
>>
>> s/now/no
>>
> Ack to all the above ones, thanks
>
>>> + * applied in time, as the frame as already been processed by the IPA,
>>> + * the PFCError flag is automatically raised on the FrameContext.
>>> + */
>>> +
>>> +/**
>>> + * \fn FCQueue::clear()
>>> + * \brief Clear the context queue
>>> + *
>>> + * Reset the queue and ensure all FrameContext slots are re-initialised.
>>> + * IPA modules are expected to clear the frame context queue at the beginning of
>>> + * a new streaming session, in IPAModule::start().
>>> + */
>>> +
>>> +/**
>>> + * \fn FCQueue::initialise(uint32_t frame)
>>> + * \brief Initialize and return the Frame Context at slot specified by \a frame
>>> + * \param[in] frame The frame context sequence number
>>> + *
>>> + * The first call to obtain a FrameContext from the FCQueue should be handled
>>> + * through this call. The FrameContext will be initialised for the frame and
>>> + * returned to the caller if it was not already initialised.
>>> + *
>>> + * If the FrameContext was already initialized for this sequence, a warning will
>>> + * be reported and the previously initialized FrameContext is returned.
>>> + *
>>> + * Frame contexts are expected to be initialised when a Request is first passed
>>> + * to the IPA module in IPAModule::queueRequest().
>>> + *
>>> + * \return A reference to the FrameContext for sequence \a frame
>>> + */
>>> +
>>> +/**
>>> + * \fn FCQueue::get()
>>> + * \brief Obtain the Frame Context at slot specified by \a frame
>>> + * \param[in] frame The frame context sequence number
>>> + *
>>> + * Obtains an existing FrameContext from the queue and returns it to the caller.
>>> + *
>>> + * If the FrameContext is not correctly initialised for the \a frame, it will be
>>> + * initialised and a PFCError will be flagged on the context to be transferred
>>> + * to the Request when it completes.
>>> + *
>>> + * \return A reference to the FrameContext for sequence \a frame
>>> + */
>>> +
>>> +} /* namespace ipa */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
>>> new file mode 100644
>>> index 000000000000..023b52420fe7
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/fc_queue.h
>>> @@ -0,0 +1,109 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2022, Google Inc.
>>> + *
>>> + * fc_queue.h - IPA Frame context queue
>>> + */
>>> +
>>> +#pragma once
>>> +
>>> +#include <array>
>>> +
>>> +#include <libcamera/base/log.h>
>>> +
>>> +#include <libcamera/request.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +LOG_DECLARE_CATEGORY(FCQueue)
>>> +
>>> +namespace ipa {
>>> +
>>> +/*
>>> + * Maximum number of frame contexts to be held onto
>>> + *
>>> + * \todo Should be platform-specific and match the pipeline depth
>>> + */
>>> +static constexpr uint32_t kMaxFrameContexts = 16;
>>> +
>>> +template<typename FrameContext>
>>> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
>>> +{
>>> +private:
>>> +	void initialise(FrameContext &frameContext, const uint32_t frame)
>>> +	{
>>> +		frameContext = {};
>>> +		frameContext.frame = frame;
>>> +	}
>>> +
>>> +public:
>>> +	void clear()
>>> +	{
>>> +		this->fill({});
>>> +	}
>>> +
>>> +	FrameContext &initialise(const uint32_t frame)
>>> +	{
>>> +		FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>>> +
>>> +		/*
>>> +		 * Do not re-initialise if a get() call has already fetched this
>>> +		 * frame context to preseve the error flags already raised.
>>> +		 *
>>> +		 * \todo If the the sequence number of the context to initialise
>>> +		 * is smaller than the sequence number of the queue slot to use,
>>> +		 * it means that we had a serious request underrun and more
>>> +		 * frames than the queue size has been produced since the last
>>> +		 * time the application has queued a request. Does this deserve
>>> +		 * an error condition ?
>>
>> I believe we should log under-runs conditions. Whether we log it as ERROR or
>> WARN or DEBUG might be a candidate for discussion. It depends on how we
>> classify such condition
>>
>> Are under-run requests expected to completed with non-fatal errors set? If
> They indeed expect to be completed :)


Ok. This was somehow missing from my mind-map about so many discussions 
and PFC document.

Thanks for clarifying that underruns need to be supported.

>
>> yes,  I believe we set RequestError::Underrun here and return the
>> initialised frameContext. But then how the Request will be completed ? As
>> the frame number passed here is quite < the current ongoing capture sequence
>> on sensor side. So, we might end up initialising the context but it's never
>> .get() with this frame number, for e.g. while populating metadata in
>> processStatsBuffer()? So we might require some special handling there to
>> detect that a frameContext belongs to a under-run request or not.
>>
>> I think I need to map out how will a under-run request will get completed
>> successfully - IFF we are supporting that case.
>>
> There are legit question, but to properly reply to them I think we
> need to finalise the design of the per-frame-control model. In
> example, for some devices with limited memory (Pi Zero, in example),
> operating in underrun mode seems to be the default, and we have to
> handle that case properly.


Agreed.

Do those discussions block merging of the series? I see you already have 
a \todo here, so maybe it is meant to be handled on top and we can merge 
this set?

>
>>> +		 */
>>> +		if (frame != 0 && frame <= frameContext.frame)
>>> +			LOG(FCQueue, Warning)
>>> +				<< "Frame " << frame << " already initialised";
>>> +		else
>>> +			initialise(frameContext, frame);
>>> +
>>> +		return frameContext;
>>> +	}
>>> +
>>> +	FrameContext &get(uint32_t frame)
>>> +	{
>>> +		FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>>> +
>>> +		/*
>>> +		 * If the IPA algorithms try to access a frame context slot which
>>> +		 * has been already overwritten by a newer context, it means the
>>> +		 * frame context queue has overflowed and the desired context
>>> +		 * has been forever lost. The pipeline handler shall avoid
>>> +		 * queueing more requests to the IPA than the frame context
>>> +		 * queue size.
>>> +		 */
>>> +		if (frame < frameContext.frame)
>>> +			LOG(FCQueue, Fatal)
>>> +				<< "Frame " << frame << " has been overwritten";
>>
>> Maybe the log can be improved:
>>
>> 	LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
>> 			    << frameContext.frame;
>>
>>> +
>>> +		if (frame == frameContext.frame)
>>> +			return frameContext;
>>> +
>>> +		LOG(FCQueue, Warning)
>>> +			<< "Obtained an uninitialised FrameContext for " << frame;
>>
>> I think currently this Warning is being logged for both cases :
>>
>>      "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> The
> 		if (frame < frameContext.frame)
> 			LOG(FCQueue, Fatal)
> 				<< "Frame " << frame << " has been overwritten";
>
> will abort execution, so we don't get to this warning.
>
>> Is this expected ? Or are we counting on the "Fatal" log above, which shall
>> terminate the entire program?
>>
> That's the idea, yes.


Got it!

I don't see any other issues with the code here, other than the minors 
pointed out so,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

Not sure how would tags work with multiple authors in this case.. but 
anyways, we can figure it out on the way!

>
> The above Fatal is triggered by a programming error in the PH/IPA as
> we should never get more requests queued than the FCQ size, so the
> assertion above should not be triggered if not during development of
> new platforms.
>
>>> +
>>> +		initialise(frameContext, frame);
>>> +
>>> +		/*
>>> +		 * The frame context has been retrieved before it was
>>> +		 * initialised through the initialise() call. This indicates an
>>> +		 * algorithm attempted to access a Frame context before it was
>>> +		 * queued to the IPA.
>>> +		 *
>>> +		 * Controls applied for this request may be left unhandled.
>>> +		 */
>>> +		frameContext.error |= Request::PFCError;
>>> +
>>> +		return frameContext;
>>> +	}
>>> +};
>>> +
>>> +} /* namespace ipa */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>>> index fb894bc614af..016b8e0ec9be 100644
>>> --- a/src/ipa/libipa/meson.build
>>> +++ b/src/ipa/libipa/meson.build
>>> @@ -3,6 +3,7 @@
>>>    libipa_headers = files([
>>>        'algorithm.h',
>>>        'camera_sensor_helper.h',
>>> +    'fc_queue.h',
>>>        'histogram.h',
>>>        'module.h',
>>>    ])
>>> @@ -10,6 +11,7 @@ libipa_headers = files([
>>>    libipa_sources = files([
>>>        'algorithm.cpp',
>>>        'camera_sensor_helper.cpp',
>>> +    'fc_queue.cpp',
>>>        'histogram.cpp',
>>>        'module.cpp',
>>>    ])
>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>>> index 6cf4d1699ce5..af22dbeb3da5 100644
>>> --- a/src/ipa/rkisp1/rkisp1.cpp
>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>>> @@ -49,7 +49,7 @@ public:
>>>    	int init(const IPASettings &settings, unsigned int hwRevision,
>>>    		 ControlInfoMap *ipaControls) override;
>>>    	int start() override;
>>> -	void stop() override {}
>>> +	void stop() override;
>>>    	int configure(const IPACameraSensorInfo &info,
>>>    		      const std::map<uint32_t, IPAStream> &streamConfig,
>>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
>>>    	return 0;
>>>    }
>>> +void IPARkISP1::stop()
>>> +{
>>> +	/* Clean the IPA context at the end of the streaming session. */
>>> +	context_ = {};
>>> +}
>>> +
>>>    /**
>>>     * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
>>>     * if the connected sensor does not provide enough information to properly
>>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>>>    		<< "Exposure: " << minExposure << "-" << maxExposure
>>>    		<< " Gain: " << minGain << "-" << maxGain;
>>> -	/* Clean context at configuration */
>>> -	context_ = {};
>>> -
>>>    	/* Set the hardware revision for the algorithms. */
>>>    	context_.configuration.hw.revision = hwRevision_;

Quoting Umang Jain via libcamera-devel (2022-08-11 10:13:42)
> Hi Jacopo,
> 
> Thank you for the clarifications,
> 
> On 8/11/22 14:08, Jacopo Mondi wrote:
> > Hi Umang
> >
> > On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> >> Hi all,
> >>
> >> Few comments and some open question for discussion on underruns...
> >>
> >> On 8/5/22 19:23, Jacopo Mondi wrote:
> >>> From: Umang Jain <umang.jain@ideasonboard.com>
> >>>
> >>> Introduce a common implementation in libipa to represent the queue of
> >>> frame contexts.
> >>>
> >>> Adjust the IPU3 IPAFrameContext to provide the basic class members
> >>> required to work with the generic FCQueue implementation, before
> >>> introducing an IPAFrameContext class in the next patches.
> >>>
> >>> Opportunely handle cleaning up the queue and the IPA context at
> >>> IPA::stop() time.

This is bad. It breaks and causes the IPA to crash...

The CTS test
android.hardware.cts.CameraTest#testJpegCallbackStartPreview triggers
this first...

This may be the first test that has a configure ... start ... stop ...
start sequence without a reconfigure. Which I think is where we break.

Essentially - we can't just clear the entire context_ now. Certainly not
between stop/start cycles.



> >>>
> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>    src/ipa/ipu3/ipa_context.cpp |  20 +------
> >>>    src/ipa/ipu3/ipa_context.h   |  16 ++---
> >>>    src/ipa/ipu3/ipu3.cpp        |  18 +++---
> >>>    src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
> >>>    src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
> >>>    src/ipa/libipa/meson.build   |   2 +
> >>>    src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
> >>>    7 files changed, 250 insertions(+), 38 deletions(-)
> >>>    create mode 100644 src/ipa/libipa/fc_queue.cpp
> >>>    create mode 100644 src/ipa/libipa/fc_queue.h
> >>>
> >>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >>> index 13cdb835ef7f..9605c8a1106d 100644
> >>> --- a/src/ipa/ipu3/ipa_context.cpp
> >>> +++ b/src/ipa/ipu3/ipa_context.cpp
> >>> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> >>>     * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> >>>     */
> >>> -/**
> >>> - * \brief Default constructor for IPAFrameContext
> >>> - */
> >>> -IPAFrameContext::IPAFrameContext() = default;
> >>> -
> >>> -/**
> >>> - * \brief Construct a IPAFrameContext instance
> >>> - */
> >>> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> >>> -   : frame(id), frameControls(reqControls)
> >>> -{
> >>> -   sensor = {};
> >>> -}
> >>> -
> >>>    /**
> >>>     * \var IPAFrameContext::frame
> >>>     * \brief The frame number
> >>>     *
> >>> - * \var IPAFrameContext::frameControls
> >>> - * \brief Controls sent in by the application while queuing the request
> >>> - *
> >>>     * \var IPAFrameContext::sensor
> >>>     * \brief Effective sensor values that were applied for the frame
> >>>     *
> >>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> >>>     *
> >>>     * \var IPAFrameContext::sensor.gain
> >>>     * \brief Analogue gain multiplier
> >>> + *
> >>> + * \var IPAFrameContext::error
> >>> + * \brief The error flags for this frame context
> >>>     */
> >>>    } /* namespace libcamera::ipa::ipu3 */
> >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >>> index 42e11141d3a1..dc5b7c30aaf9 100644
> >>> --- a/src/ipa/ipu3/ipa_context.h
> >>> +++ b/src/ipa/ipu3/ipa_context.h
> >>> @@ -8,22 +8,20 @@
> >>>    #pragma once
> >>> -#include <array>
> >>> -
> >>>    #include <linux/intel-ipu3.h>
> >>>    #include <libcamera/base/utils.h>
> >>>    #include <libcamera/controls.h>
> >>>    #include <libcamera/geometry.h>
> >>> +#include <libcamera/request.h>
> >>> +
> >>> +#include <libipa/fc_queue.h>
> >>>    namespace libcamera {
> >>>    namespace ipa::ipu3 {
> >>> -/* Maximum number of frame contexts to be held */
> >>> -static constexpr uint32_t kMaxFrameContexts = 16;
> >>> -
> >>>    struct IPASessionConfiguration {
> >>>     struct {
> >>>             ipu3_uapi_grid_config bdsGrid;
> >>> @@ -77,23 +75,21 @@ struct IPAActiveState {
> >>>    };
> >>>    struct IPAFrameContext {
> >>> -   IPAFrameContext();
> >>> -   IPAFrameContext(uint32_t id, const ControlList &reqControls);
> >>> +   uint32_t frame;
> >>>     struct {
> >>>             uint32_t exposure;
> >>>             double gain;
> >>>     } sensor;
> >>> -   uint32_t frame;
> >>> -   ControlList frameControls;
> >>> +   Request::Errors error;
> >>>    };
> >>>    struct IPAContext {
> >>>     IPASessionConfiguration configuration;
> >>>     IPAActiveState activeState;
> >>> -   std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> >>> +   FCQueue<IPAFrameContext> frameContexts;
> >>>    };
> >>>    } /* namespace ipa::ipu3 */
> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >>> index 2f6bb672f7bb..209a6b040f8f 100644
> >>> --- a/src/ipa/ipu3/ipu3.cpp
> >>> +++ b/src/ipa/ipu3/ipu3.cpp
> >>> @@ -38,6 +38,8 @@
> >>>    #include "algorithms/tone_mapping.h"
> >>>    #include "libipa/camera_sensor_helper.h"
> >>> +#include "ipa_context.h"
> >>> +
> >>>    /* Minimum grid width, expressed as a number of cells */
> >>>    static constexpr uint32_t kMinGridWidth = 16;
> >>>    /* Maximum grid width, expressed as a number of cells */
> >>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
> >>>     */
> >>>    void IPAIPU3::stop()
> >>>    {
> >>> +   /* Clean the IPA context at the end of the streaming session. */
> >>> +   context_.frameContexts.clear();
> >>> +   context_ = {};

I think this context_ = {}; is the line causing me trouble right now.

The IPU3 IPA crashes with a divide by zero error in :

 libcamera::ipa::ipu3::algorithms::Af::afEstimateVariance()

where y_items.size() = 0, because Af::process() has set afRawBufferLen =
0 from 

	/* Evaluate the AF buffer length */
	uint32_t afRawBufferLen = context.configuration.af.afGrid.width *
				  context.configuration.af.afGrid.height;

So because context has been completely zeroed, and not reconfigured - we
end up with a zero length span, and a div/0.

I expect simply removing this context_ = {}; may be sufficient. This may
need to be considered for the RKISP too.


Yes, I've removed this line, and the test doesn't fail now. I'll rerun
the whole CTS suite.


Same for RKISP below...


> >>>    }
> >>>    /**
> >>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >>>     calculateBdsGrid(configInfo.bdsOutputSize);
> >>> -   /* Clean IPAActiveState at each reconfiguration. */
> >>> -   context_.activeState = {};
> >>> -   IPAFrameContext initFrameContext;
> >>> -   context_.frameContexts.fill(initFrameContext);
> >>> -
> >>>     if (!validateSensorControls()) {
> >>>             LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> >>>             return -EINVAL;
> >>> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> >>> +   IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> >>>     if (frameContext.frame != frame)
> >>>             LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> >>> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> >>>    void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> >>>    {
> >>>     /* \todo Start processing for 'frame' based on 'controls'. */
> >>> -   context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> >>> +   IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> >>> +
> >>> +   /* \todo Implement queueRequest to each algorithm. */
> >>> +   (void)frameContext;
> >>> +   (void)controls;
> >>>    }
> >>>    /**
> >>> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> >>> new file mode 100644
> >>> index 000000000000..37ffca60b992
> >>> --- /dev/null
> >>> +++ b/src/ipa/libipa/fc_queue.cpp
> >>> @@ -0,0 +1,112 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2022, Google Inc.
> >>> + *
> >>> + * fc_queue.cpp - IPA Frame context queue
> >>> + */
> >>> +
> >>> +#include "fc_queue.h"
> >>> +
> >>> +#include <libcamera/base/log.h>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +LOG_DEFINE_CATEGORY(FCQueue)
> >>> +
> >>> +namespace ipa {
> >>> +
> >>> +/**
> >>> + * \file fc_queue.h
> >>> + * \brief Queue to access per-frame Context
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class FCQueue
> >>> + * \brief A support class for queueing FrameContext instances through the IPA
> >>> + * \tparam FrameContext The IPA specific FrameContext derived class type
> >>> + *
> >>> + * The frame context queue provides a simple circular buffer implementation to
> >>> + * store IPA specific context for each frame through its lifetime within the
> >>> + * IPA.
> >>> + *
> >>> + * FrameContext instances are expected to be referenced by a monotonically
> >>> + * increasing sequence count corresponding to a frame sequence number.
> >>> + *
> >>> + * A FrameContext is initialised for a given frame when the corresponding
> >>> + * Request is first queued into the IPA. From that point on the FrameContext can
> >>> + * be obtained by the IPA and its algorithms by referencing it from the frame
> >>> + * sequence number.
> >>> + *
> >>> + * A frame sequence number from the image source must correspond to the request
> >>> + * sequence number for this implementation to be supported in an IPA. It is
> >>> + * expected that the same sequence number will be used to reference the context
> >>> + * of the Frame from the point of queueing the request, specifying controls for
> >>
> >> s/Frame/frame
> >>
> >>> + * a given frame, and processing of any ISP related controls and statistics for
> >>> + * the same corresponding image.
> >>> + *
> >>> + * IPA specific frame context implementations shall inherit from the
> >>> + * IPAFrameContext base class to support the minimum required features for a
> >>> + * FrameContext, including the frame number and the error flags that relate to
> >>> + * the frame.
> >>> + *
> >>> + * FrameContexts are overwritten when they are recycled and re-initialised by
> >>> + * the first access made on them by either initialise(frame) or get(frame). It
> >>> + * is required that the number of available slots in the frame context queue is
> >>> + * larger or equal to the maximum number of in-flight requests a pipeline can
> >>> + * handle to avoid overwriting frame context instances that still have to be
> >>> + * processed.
> >>> + *
> >>> + * In the case an application does not queue requests to the Camera fast
> >>
> >> s/Camera/camera/
> >>
> >>> + * enough, frames can be produced and processed by the IPA without a
> >>> + * corresponding Request being queued. In this case the IPA algorithm
> >>> + * will try to access the FrameContext with a call to get() before it
> >>> + * had been initialized at queueRequest() time. In this case there
> >>
> >> s/case/case,
> >>
> >>> + * is now way the controls associated with the late Request could be
> >>
> >> s/now/no
> >>
> > Ack to all the above ones, thanks
> >
> >>> + * applied in time, as the frame as already been processed by the IPA,
> >>> + * the PFCError flag is automatically raised on the FrameContext.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn FCQueue::clear()
> >>> + * \brief Clear the context queue
> >>> + *
> >>> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> >>> + * IPA modules are expected to clear the frame context queue at the beginning of
> >>> + * a new streaming session, in IPAModule::start().
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn FCQueue::initialise(uint32_t frame)
> >>> + * \brief Initialize and return the Frame Context at slot specified by \a frame
> >>> + * \param[in] frame The frame context sequence number
> >>> + *
> >>> + * The first call to obtain a FrameContext from the FCQueue should be handled
> >>> + * through this call. The FrameContext will be initialised for the frame and
> >>> + * returned to the caller if it was not already initialised.
> >>> + *
> >>> + * If the FrameContext was already initialized for this sequence, a warning will
> >>> + * be reported and the previously initialized FrameContext is returned.
> >>> + *
> >>> + * Frame contexts are expected to be initialised when a Request is first passed
> >>> + * to the IPA module in IPAModule::queueRequest().
> >>> + *
> >>> + * \return A reference to the FrameContext for sequence \a frame
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn FCQueue::get()
> >>> + * \brief Obtain the Frame Context at slot specified by \a frame
> >>> + * \param[in] frame The frame context sequence number
> >>> + *
> >>> + * Obtains an existing FrameContext from the queue and returns it to the caller.
> >>> + *
> >>> + * If the FrameContext is not correctly initialised for the \a frame, it will be
> >>> + * initialised and a PFCError will be flagged on the context to be transferred
> >>> + * to the Request when it completes.
> >>> + *
> >>> + * \return A reference to the FrameContext for sequence \a frame
> >>> + */
> >>> +
> >>> +} /* namespace ipa */
> >>> +
> >>> +} /* namespace libcamera */
> >>> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> >>> new file mode 100644
> >>> index 000000000000..023b52420fe7
> >>> --- /dev/null
> >>> +++ b/src/ipa/libipa/fc_queue.h
> >>> @@ -0,0 +1,109 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2022, Google Inc.
> >>> + *
> >>> + * fc_queue.h - IPA Frame context queue
> >>> + */
> >>> +
> >>> +#pragma once
> >>> +
> >>> +#include <array>
> >>> +
> >>> +#include <libcamera/base/log.h>
> >>> +
> >>> +#include <libcamera/request.h>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +LOG_DECLARE_CATEGORY(FCQueue)
> >>> +
> >>> +namespace ipa {
> >>> +
> >>> +/*
> >>> + * Maximum number of frame contexts to be held onto
> >>> + *
> >>> + * \todo Should be platform-specific and match the pipeline depth
> >>> + */
> >>> +static constexpr uint32_t kMaxFrameContexts = 16;
> >>> +
> >>> +template<typename FrameContext>
> >>> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> >>> +{
> >>> +private:
> >>> +   void initialise(FrameContext &frameContext, const uint32_t frame)
> >>> +   {
> >>> +           frameContext = {};
> >>> +           frameContext.frame = frame;
> >>> +   }
> >>> +
> >>> +public:
> >>> +   void clear()
> >>> +   {
> >>> +           this->fill({});
> >>> +   }
> >>> +
> >>> +   FrameContext &initialise(const uint32_t frame)
> >>> +   {
> >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> >>> +
> >>> +           /*
> >>> +            * Do not re-initialise if a get() call has already fetched this
> >>> +            * frame context to preseve the error flags already raised.
> >>> +            *
> >>> +            * \todo If the the sequence number of the context to initialise
> >>> +            * is smaller than the sequence number of the queue slot to use,
> >>> +            * it means that we had a serious request underrun and more
> >>> +            * frames than the queue size has been produced since the last
> >>> +            * time the application has queued a request. Does this deserve
> >>> +            * an error condition ?
> >>
> >> I believe we should log under-runs conditions. Whether we log it as ERROR or
> >> WARN or DEBUG might be a candidate for discussion. It depends on how we
> >> classify such condition
> >>
> >> Are under-run requests expected to completed with non-fatal errors set? If
> > They indeed expect to be completed :)
> 
> 
> Ok. This was somehow missing from my mind-map about so many discussions 
> and PFC document.
> 
> Thanks for clarifying that underruns need to be supported.
> 
> >
> >> yes,  I believe we set RequestError::Underrun here and return the
> >> initialised frameContext. But then how the Request will be completed ? As
> >> the frame number passed here is quite < the current ongoing capture sequence
> >> on sensor side. So, we might end up initialising the context but it's never
> >> .get() with this frame number, for e.g. while populating metadata in
> >> processStatsBuffer()? So we might require some special handling there to
> >> detect that a frameContext belongs to a under-run request or not.
> >>
> >> I think I need to map out how will a under-run request will get completed
> >> successfully - IFF we are supporting that case.
> >>
> > There are legit question, but to properly reply to them I think we
> > need to finalise the design of the per-frame-control model. In
> > example, for some devices with limited memory (Pi Zero, in example),
> > operating in underrun mode seems to be the default, and we have to
> > handle that case properly.
> 
> 
> Agreed.
> 
> Do those discussions block merging of the series? I see you already have 
> a \todo here, so maybe it is meant to be handled on top and we can merge 
> this set?
> 
> >
> >>> +            */
> >>> +           if (frame != 0 && frame <= frameContext.frame)
> >>> +                   LOG(FCQueue, Warning)
> >>> +                           << "Frame " << frame << " already initialised";
> >>> +           else
> >>> +                   initialise(frameContext, frame);
> >>> +
> >>> +           return frameContext;
> >>> +   }
> >>> +
> >>> +   FrameContext &get(uint32_t frame)
> >>> +   {
> >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> >>> +
> >>> +           /*
> >>> +            * If the IPA algorithms try to access a frame context slot which
> >>> +            * has been already overwritten by a newer context, it means the
> >>> +            * frame context queue has overflowed and the desired context
> >>> +            * has been forever lost. The pipeline handler shall avoid
> >>> +            * queueing more requests to the IPA than the frame context
> >>> +            * queue size.
> >>> +            */
> >>> +           if (frame < frameContext.frame)
> >>> +                   LOG(FCQueue, Fatal)
> >>> +                           << "Frame " << frame << " has been overwritten";
> >>
> >> Maybe the log can be improved:
> >>
> >>      LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> >>                          << frameContext.frame;
> >>
> >>> +
> >>> +           if (frame == frameContext.frame)
> >>> +                   return frameContext;
> >>> +
> >>> +           LOG(FCQueue, Warning)
> >>> +                   << "Obtained an uninitialised FrameContext for " << frame;
> >>
> >> I think currently this Warning is being logged for both cases :
> >>
> >>      "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> > The
> >               if (frame < frameContext.frame)
> >                       LOG(FCQueue, Fatal)
> >                               << "Frame " << frame << " has been overwritten";
> >
> > will abort execution, so we don't get to this warning.
> >
> >> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> >> terminate the entire program?
> >>
> > That's the idea, yes.
> 
> 
> Got it!
> 
> I don't see any other issues with the code here, other than the minors 
> pointed out so,
> 
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 
> Not sure how would tags work with multiple authors in this case.. but 
> anyways, we can figure it out on the way!
> 
> >
> > The above Fatal is triggered by a programming error in the PH/IPA as
> > we should never get more requests queued than the FCQ size, so the
> > assertion above should not be triggered if not during development of
> > new platforms.
> >
> >>> +
> >>> +           initialise(frameContext, frame);
> >>> +
> >>> +           /*
> >>> +            * The frame context has been retrieved before it was
> >>> +            * initialised through the initialise() call. This indicates an
> >>> +            * algorithm attempted to access a Frame context before it was
> >>> +            * queued to the IPA.
> >>> +            *
> >>> +            * Controls applied for this request may be left unhandled.
> >>> +            */
> >>> +           frameContext.error |= Request::PFCError;
> >>> +
> >>> +           return frameContext;
> >>> +   }
> >>> +};
> >>> +
> >>> +} /* namespace ipa */
> >>> +
> >>> +} /* namespace libcamera */
> >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> >>> index fb894bc614af..016b8e0ec9be 100644
> >>> --- a/src/ipa/libipa/meson.build
> >>> +++ b/src/ipa/libipa/meson.build
> >>> @@ -3,6 +3,7 @@
> >>>    libipa_headers = files([
> >>>        'algorithm.h',
> >>>        'camera_sensor_helper.h',
> >>> +    'fc_queue.h',
> >>>        'histogram.h',
> >>>        'module.h',
> >>>    ])
> >>> @@ -10,6 +11,7 @@ libipa_headers = files([
> >>>    libipa_sources = files([
> >>>        'algorithm.cpp',
> >>>        'camera_sensor_helper.cpp',
> >>> +    'fc_queue.cpp',
> >>>        'histogram.cpp',
> >>>        'module.cpp',
> >>>    ])
> >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >>> index 6cf4d1699ce5..af22dbeb3da5 100644
> >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >>> @@ -49,7 +49,7 @@ public:
> >>>     int init(const IPASettings &settings, unsigned int hwRevision,
> >>>              ControlInfoMap *ipaControls) override;
> >>>     int start() override;
> >>> -   void stop() override {}
> >>> +   void stop() override;
> >>>     int configure(const IPACameraSensorInfo &info,
> >>>                   const std::map<uint32_t, IPAStream> &streamConfig,
> >>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
> >>>     return 0;
> >>>    }
> >>> +void IPARkISP1::stop()
> >>> +{
> >>> +   /* Clean the IPA context at the end of the streaming session. */
> >>> +   context_ = {};

Indeed - watch out here. I think this breaks start/stop/start cycles as
suddenly the session configuration is now zeroed.

--
Kieran


> >>> +}
> >>> +
> >>>    /**
> >>>     * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> >>>     * if the connected sensor does not provide enough information to properly
> >>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >>>             << "Exposure: " << minExposure << "-" << maxExposure
> >>>             << " Gain: " << minGain << "-" << maxGain;
> >>> -   /* Clean context at configuration */
> >>> -   context_ = {};
> >>> -
> >>>     /* Set the hardware revision for the algorithms. */
> >>>     context_.configuration.hw.revision = hwRevision_;

Hi Kieran

  thanks a lot for getting to the bottom of this.

I still can't reproduce it here, but your analysis seems correct to me
and indeed the context = {} might prove problematic.

See below

On Wed, Aug 17, 2022 at 09:33:08PM +0100, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2022-08-11 10:13:42)
> > Hi Jacopo,
> >
> > Thank you for the clarifications,
> >
> > On 8/11/22 14:08, Jacopo Mondi wrote:
> > > Hi Umang
> > >
> > > On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> > >> Hi all,
> > >>
> > >> Few comments and some open question for discussion on underruns...
> > >>
> > >> On 8/5/22 19:23, Jacopo Mondi wrote:
> > >>> From: Umang Jain <umang.jain@ideasonboard.com>
> > >>>
> > >>> Introduce a common implementation in libipa to represent the queue of
> > >>> frame contexts.
> > >>>
> > >>> Adjust the IPU3 IPAFrameContext to provide the basic class members
> > >>> required to work with the generic FCQueue implementation, before
> > >>> introducing an IPAFrameContext class in the next patches.
> > >>>
> > >>> Opportunely handle cleaning up the queue and the IPA context at
> > >>> IPA::stop() time.
>
> This is bad. It breaks and causes the IPA to crash...
>
> The CTS test
> android.hardware.cts.CameraTest#testJpegCallbackStartPreview triggers
> this first...
>
> This may be the first test that has a configure ... start ... stop ...
> start sequence without a reconfigure. Which I think is where we break.
>
> Essentially - we can't just clear the entire context_ now. Certainly not
> between stop/start cycles.
>
>
>
> > >>>
> > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>> ---
> > >>>    src/ipa/ipu3/ipa_context.cpp |  20 +------
> > >>>    src/ipa/ipu3/ipa_context.h   |  16 ++---
> > >>>    src/ipa/ipu3/ipu3.cpp        |  18 +++---
> > >>>    src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
> > >>>    src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
> > >>>    src/ipa/libipa/meson.build   |   2 +
> > >>>    src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
> > >>>    7 files changed, 250 insertions(+), 38 deletions(-)
> > >>>    create mode 100644 src/ipa/libipa/fc_queue.cpp
> > >>>    create mode 100644 src/ipa/libipa/fc_queue.h
> > >>>
> > >>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > >>> index 13cdb835ef7f..9605c8a1106d 100644
> > >>> --- a/src/ipa/ipu3/ipa_context.cpp
> > >>> +++ b/src/ipa/ipu3/ipa_context.cpp
> > >>> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> > >>>     * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> > >>>     */
> > >>> -/**
> > >>> - * \brief Default constructor for IPAFrameContext
> > >>> - */
> > >>> -IPAFrameContext::IPAFrameContext() = default;
> > >>> -
> > >>> -/**
> > >>> - * \brief Construct a IPAFrameContext instance
> > >>> - */
> > >>> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > >>> -   : frame(id), frameControls(reqControls)
> > >>> -{
> > >>> -   sensor = {};
> > >>> -}
> > >>> -
> > >>>    /**
> > >>>     * \var IPAFrameContext::frame
> > >>>     * \brief The frame number
> > >>>     *
> > >>> - * \var IPAFrameContext::frameControls
> > >>> - * \brief Controls sent in by the application while queuing the request
> > >>> - *
> > >>>     * \var IPAFrameContext::sensor
> > >>>     * \brief Effective sensor values that were applied for the frame
> > >>>     *
> > >>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > >>>     *
> > >>>     * \var IPAFrameContext::sensor.gain
> > >>>     * \brief Analogue gain multiplier
> > >>> + *
> > >>> + * \var IPAFrameContext::error
> > >>> + * \brief The error flags for this frame context
> > >>>     */
> > >>>    } /* namespace libcamera::ipa::ipu3 */
> > >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > >>> index 42e11141d3a1..dc5b7c30aaf9 100644
> > >>> --- a/src/ipa/ipu3/ipa_context.h
> > >>> +++ b/src/ipa/ipu3/ipa_context.h
> > >>> @@ -8,22 +8,20 @@
> > >>>    #pragma once
> > >>> -#include <array>
> > >>> -
> > >>>    #include <linux/intel-ipu3.h>
> > >>>    #include <libcamera/base/utils.h>
> > >>>    #include <libcamera/controls.h>
> > >>>    #include <libcamera/geometry.h>
> > >>> +#include <libcamera/request.h>
> > >>> +
> > >>> +#include <libipa/fc_queue.h>
> > >>>    namespace libcamera {
> > >>>    namespace ipa::ipu3 {
> > >>> -/* Maximum number of frame contexts to be held */
> > >>> -static constexpr uint32_t kMaxFrameContexts = 16;
> > >>> -
> > >>>    struct IPASessionConfiguration {
> > >>>     struct {
> > >>>             ipu3_uapi_grid_config bdsGrid;
> > >>> @@ -77,23 +75,21 @@ struct IPAActiveState {
> > >>>    };
> > >>>    struct IPAFrameContext {
> > >>> -   IPAFrameContext();
> > >>> -   IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > >>> +   uint32_t frame;
> > >>>     struct {
> > >>>             uint32_t exposure;
> > >>>             double gain;
> > >>>     } sensor;
> > >>> -   uint32_t frame;
> > >>> -   ControlList frameControls;
> > >>> +   Request::Errors error;
> > >>>    };
> > >>>    struct IPAContext {
> > >>>     IPASessionConfiguration configuration;
> > >>>     IPAActiveState activeState;
> > >>> -   std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> > >>> +   FCQueue<IPAFrameContext> frameContexts;
> > >>>    };
> > >>>    } /* namespace ipa::ipu3 */
> > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > >>> index 2f6bb672f7bb..209a6b040f8f 100644
> > >>> --- a/src/ipa/ipu3/ipu3.cpp
> > >>> +++ b/src/ipa/ipu3/ipu3.cpp
> > >>> @@ -38,6 +38,8 @@
> > >>>    #include "algorithms/tone_mapping.h"
> > >>>    #include "libipa/camera_sensor_helper.h"
> > >>> +#include "ipa_context.h"
> > >>> +
> > >>>    /* Minimum grid width, expressed as a number of cells */
> > >>>    static constexpr uint32_t kMinGridWidth = 16;
> > >>>    /* Maximum grid width, expressed as a number of cells */
> > >>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
> > >>>     */
> > >>>    void IPAIPU3::stop()
> > >>>    {
> > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > >>> +   context_.frameContexts.clear();
> > >>> +   context_ = {};
>
> I think this context_ = {}; is the line causing me trouble right now.
>
> The IPU3 IPA crashes with a divide by zero error in :
>
>  libcamera::ipa::ipu3::algorithms::Af::afEstimateVariance()
>
> where y_items.size() = 0, because Af::process() has set afRawBufferLen =
> 0 from
>
> 	/* Evaluate the AF buffer length */
> 	uint32_t afRawBufferLen = context.configuration.af.afGrid.width *
> 				  context.configuration.af.afGrid.height;
>
> So because context has been completely zeroed, and not reconfigured - we
> end up with a zero length span, and a div/0.

Ouch

Still doesn't make sense to me that here it doesn't fail..

>
> I expect simply removing this context_ = {}; may be sufficient. This may
> need to be considered for the RKISP too.
>

Ok, looking at the context in more detail, it indeed contains the
session configuration, which shall not be zeroed, but just overwritten
when a new configure() is issued.

context_ contains the active state as well, I'm still trying to figure
out if that one needs to be cleaned in order not to operate on
stale data when a new streaming session is started.

The frame context queue does indeed need to be cleared between
streaming session instead.

Overall we have a context_ which contains 3 items with different
lifetime management requirements.

I wonder if it's a good idea to store all of them together or it would
be easier to break the FCQ and the active state out from the
context_. I actually wonder if a struct context {}  makes any sense,
when the IPA class can have the three items (configuration, queue and
state) as three class members and handle their lifetime separately.

To be honest, the role of active state is still a little bit unclear
to me. It contains a global state of algorithm results without any
synchronization with the current frame. Re-reading Laurent's comment
on 05/10 of this series and your reply, it seems we agree the active
state shall be removed or reworked.

For now I think it's then fine to remove the context = {}; from this
series, and then rework how the IPA context is modeled on top. Do you
all agree with this ?

Thanks
   j

>
> Yes, I've removed this line, and the test doesn't fail now. I'll rerun
> the whole CTS suite.
>
>
> Same for RKISP below...
>
>
> > >>>    }
> > >>>    /**
> > >>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > >>>     calculateBdsGrid(configInfo.bdsOutputSize);
> > >>> -   /* Clean IPAActiveState at each reconfiguration. */
> > >>> -   context_.activeState = {};
> > >>> -   IPAFrameContext initFrameContext;
> > >>> -   context_.frameContexts.fill(initFrameContext);
> > >>> -
> > >>>     if (!validateSensorControls()) {
> > >>>             LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > >>>             return -EINVAL;
> > >>> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> > >>> +   IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > >>>     if (frameContext.frame != frame)
> > >>>             LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> > >>> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >>>    void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > >>>    {
> > >>>     /* \todo Start processing for 'frame' based on 'controls'. */
> > >>> -   context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > >>> +   IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> > >>> +
> > >>> +   /* \todo Implement queueRequest to each algorithm. */
> > >>> +   (void)frameContext;
> > >>> +   (void)controls;
> > >>>    }
> > >>>    /**
> > >>> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > >>> new file mode 100644
> > >>> index 000000000000..37ffca60b992
> > >>> --- /dev/null
> > >>> +++ b/src/ipa/libipa/fc_queue.cpp
> > >>> @@ -0,0 +1,112 @@
> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2022, Google Inc.
> > >>> + *
> > >>> + * fc_queue.cpp - IPA Frame context queue
> > >>> + */
> > >>> +
> > >>> +#include "fc_queue.h"
> > >>> +
> > >>> +#include <libcamera/base/log.h>
> > >>> +
> > >>> +namespace libcamera {
> > >>> +
> > >>> +LOG_DEFINE_CATEGORY(FCQueue)
> > >>> +
> > >>> +namespace ipa {
> > >>> +
> > >>> +/**
> > >>> + * \file fc_queue.h
> > >>> + * \brief Queue to access per-frame Context
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \class FCQueue
> > >>> + * \brief A support class for queueing FrameContext instances through the IPA
> > >>> + * \tparam FrameContext The IPA specific FrameContext derived class type
> > >>> + *
> > >>> + * The frame context queue provides a simple circular buffer implementation to
> > >>> + * store IPA specific context for each frame through its lifetime within the
> > >>> + * IPA.
> > >>> + *
> > >>> + * FrameContext instances are expected to be referenced by a monotonically
> > >>> + * increasing sequence count corresponding to a frame sequence number.
> > >>> + *
> > >>> + * A FrameContext is initialised for a given frame when the corresponding
> > >>> + * Request is first queued into the IPA. From that point on the FrameContext can
> > >>> + * be obtained by the IPA and its algorithms by referencing it from the frame
> > >>> + * sequence number.
> > >>> + *
> > >>> + * A frame sequence number from the image source must correspond to the request
> > >>> + * sequence number for this implementation to be supported in an IPA. It is
> > >>> + * expected that the same sequence number will be used to reference the context
> > >>> + * of the Frame from the point of queueing the request, specifying controls for
> > >>
> > >> s/Frame/frame
> > >>
> > >>> + * a given frame, and processing of any ISP related controls and statistics for
> > >>> + * the same corresponding image.
> > >>> + *
> > >>> + * IPA specific frame context implementations shall inherit from the
> > >>> + * IPAFrameContext base class to support the minimum required features for a
> > >>> + * FrameContext, including the frame number and the error flags that relate to
> > >>> + * the frame.
> > >>> + *
> > >>> + * FrameContexts are overwritten when they are recycled and re-initialised by
> > >>> + * the first access made on them by either initialise(frame) or get(frame). It
> > >>> + * is required that the number of available slots in the frame context queue is
> > >>> + * larger or equal to the maximum number of in-flight requests a pipeline can
> > >>> + * handle to avoid overwriting frame context instances that still have to be
> > >>> + * processed.
> > >>> + *
> > >>> + * In the case an application does not queue requests to the Camera fast
> > >>
> > >> s/Camera/camera/
> > >>
> > >>> + * enough, frames can be produced and processed by the IPA without a
> > >>> + * corresponding Request being queued. In this case the IPA algorithm
> > >>> + * will try to access the FrameContext with a call to get() before it
> > >>> + * had been initialized at queueRequest() time. In this case there
> > >>
> > >> s/case/case,
> > >>
> > >>> + * is now way the controls associated with the late Request could be
> > >>
> > >> s/now/no
> > >>
> > > Ack to all the above ones, thanks
> > >
> > >>> + * applied in time, as the frame as already been processed by the IPA,
> > >>> + * the PFCError flag is automatically raised on the FrameContext.
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn FCQueue::clear()
> > >>> + * \brief Clear the context queue
> > >>> + *
> > >>> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> > >>> + * IPA modules are expected to clear the frame context queue at the beginning of
> > >>> + * a new streaming session, in IPAModule::start().
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn FCQueue::initialise(uint32_t frame)
> > >>> + * \brief Initialize and return the Frame Context at slot specified by \a frame
> > >>> + * \param[in] frame The frame context sequence number
> > >>> + *
> > >>> + * The first call to obtain a FrameContext from the FCQueue should be handled
> > >>> + * through this call. The FrameContext will be initialised for the frame and
> > >>> + * returned to the caller if it was not already initialised.
> > >>> + *
> > >>> + * If the FrameContext was already initialized for this sequence, a warning will
> > >>> + * be reported and the previously initialized FrameContext is returned.
> > >>> + *
> > >>> + * Frame contexts are expected to be initialised when a Request is first passed
> > >>> + * to the IPA module in IPAModule::queueRequest().
> > >>> + *
> > >>> + * \return A reference to the FrameContext for sequence \a frame
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \fn FCQueue::get()
> > >>> + * \brief Obtain the Frame Context at slot specified by \a frame
> > >>> + * \param[in] frame The frame context sequence number
> > >>> + *
> > >>> + * Obtains an existing FrameContext from the queue and returns it to the caller.
> > >>> + *
> > >>> + * If the FrameContext is not correctly initialised for the \a frame, it will be
> > >>> + * initialised and a PFCError will be flagged on the context to be transferred
> > >>> + * to the Request when it completes.
> > >>> + *
> > >>> + * \return A reference to the FrameContext for sequence \a frame
> > >>> + */
> > >>> +
> > >>> +} /* namespace ipa */
> > >>> +
> > >>> +} /* namespace libcamera */
> > >>> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > >>> new file mode 100644
> > >>> index 000000000000..023b52420fe7
> > >>> --- /dev/null
> > >>> +++ b/src/ipa/libipa/fc_queue.h
> > >>> @@ -0,0 +1,109 @@
> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2022, Google Inc.
> > >>> + *
> > >>> + * fc_queue.h - IPA Frame context queue
> > >>> + */
> > >>> +
> > >>> +#pragma once
> > >>> +
> > >>> +#include <array>
> > >>> +
> > >>> +#include <libcamera/base/log.h>
> > >>> +
> > >>> +#include <libcamera/request.h>
> > >>> +
> > >>> +namespace libcamera {
> > >>> +
> > >>> +LOG_DECLARE_CATEGORY(FCQueue)
> > >>> +
> > >>> +namespace ipa {
> > >>> +
> > >>> +/*
> > >>> + * Maximum number of frame contexts to be held onto
> > >>> + *
> > >>> + * \todo Should be platform-specific and match the pipeline depth
> > >>> + */
> > >>> +static constexpr uint32_t kMaxFrameContexts = 16;
> > >>> +
> > >>> +template<typename FrameContext>
> > >>> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> > >>> +{
> > >>> +private:
> > >>> +   void initialise(FrameContext &frameContext, const uint32_t frame)
> > >>> +   {
> > >>> +           frameContext = {};
> > >>> +           frameContext.frame = frame;
> > >>> +   }
> > >>> +
> > >>> +public:
> > >>> +   void clear()
> > >>> +   {
> > >>> +           this->fill({});
> > >>> +   }
> > >>> +
> > >>> +   FrameContext &initialise(const uint32_t frame)
> > >>> +   {
> > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > >>> +
> > >>> +           /*
> > >>> +            * Do not re-initialise if a get() call has already fetched this
> > >>> +            * frame context to preseve the error flags already raised.
> > >>> +            *
> > >>> +            * \todo If the the sequence number of the context to initialise
> > >>> +            * is smaller than the sequence number of the queue slot to use,
> > >>> +            * it means that we had a serious request underrun and more
> > >>> +            * frames than the queue size has been produced since the last
> > >>> +            * time the application has queued a request. Does this deserve
> > >>> +            * an error condition ?
> > >>
> > >> I believe we should log under-runs conditions. Whether we log it as ERROR or
> > >> WARN or DEBUG might be a candidate for discussion. It depends on how we
> > >> classify such condition
> > >>
> > >> Are under-run requests expected to completed with non-fatal errors set? If
> > > They indeed expect to be completed :)
> >
> >
> > Ok. This was somehow missing from my mind-map about so many discussions
> > and PFC document.
> >
> > Thanks for clarifying that underruns need to be supported.
> >
> > >
> > >> yes,  I believe we set RequestError::Underrun here and return the
> > >> initialised frameContext. But then how the Request will be completed ? As
> > >> the frame number passed here is quite < the current ongoing capture sequence
> > >> on sensor side. So, we might end up initialising the context but it's never
> > >> .get() with this frame number, for e.g. while populating metadata in
> > >> processStatsBuffer()? So we might require some special handling there to
> > >> detect that a frameContext belongs to a under-run request or not.
> > >>
> > >> I think I need to map out how will a under-run request will get completed
> > >> successfully - IFF we are supporting that case.
> > >>
> > > There are legit question, but to properly reply to them I think we
> > > need to finalise the design of the per-frame-control model. In
> > > example, for some devices with limited memory (Pi Zero, in example),
> > > operating in underrun mode seems to be the default, and we have to
> > > handle that case properly.
> >
> >
> > Agreed.
> >
> > Do those discussions block merging of the series? I see you already have
> > a \todo here, so maybe it is meant to be handled on top and we can merge
> > this set?
> >
> > >
> > >>> +            */
> > >>> +           if (frame != 0 && frame <= frameContext.frame)
> > >>> +                   LOG(FCQueue, Warning)
> > >>> +                           << "Frame " << frame << " already initialised";
> > >>> +           else
> > >>> +                   initialise(frameContext, frame);
> > >>> +
> > >>> +           return frameContext;
> > >>> +   }
> > >>> +
> > >>> +   FrameContext &get(uint32_t frame)
> > >>> +   {
> > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > >>> +
> > >>> +           /*
> > >>> +            * If the IPA algorithms try to access a frame context slot which
> > >>> +            * has been already overwritten by a newer context, it means the
> > >>> +            * frame context queue has overflowed and the desired context
> > >>> +            * has been forever lost. The pipeline handler shall avoid
> > >>> +            * queueing more requests to the IPA than the frame context
> > >>> +            * queue size.
> > >>> +            */
> > >>> +           if (frame < frameContext.frame)
> > >>> +                   LOG(FCQueue, Fatal)
> > >>> +                           << "Frame " << frame << " has been overwritten";
> > >>
> > >> Maybe the log can be improved:
> > >>
> > >>      LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> > >>                          << frameContext.frame;
> > >>
> > >>> +
> > >>> +           if (frame == frameContext.frame)
> > >>> +                   return frameContext;
> > >>> +
> > >>> +           LOG(FCQueue, Warning)
> > >>> +                   << "Obtained an uninitialised FrameContext for " << frame;
> > >>
> > >> I think currently this Warning is being logged for both cases :
> > >>
> > >>      "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> > > The
> > >               if (frame < frameContext.frame)
> > >                       LOG(FCQueue, Fatal)
> > >                               << "Frame " << frame << " has been overwritten";
> > >
> > > will abort execution, so we don't get to this warning.
> > >
> > >> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> > >> terminate the entire program?
> > >>
> > > That's the idea, yes.
> >
> >
> > Got it!
> >
> > I don't see any other issues with the code here, other than the minors
> > pointed out so,
> >
> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> >
> > Not sure how would tags work with multiple authors in this case.. but
> > anyways, we can figure it out on the way!
> >
> > >
> > > The above Fatal is triggered by a programming error in the PH/IPA as
> > > we should never get more requests queued than the FCQ size, so the
> > > assertion above should not be triggered if not during development of
> > > new platforms.
> > >
> > >>> +
> > >>> +           initialise(frameContext, frame);
> > >>> +
> > >>> +           /*
> > >>> +            * The frame context has been retrieved before it was
> > >>> +            * initialised through the initialise() call. This indicates an
> > >>> +            * algorithm attempted to access a Frame context before it was
> > >>> +            * queued to the IPA.
> > >>> +            *
> > >>> +            * Controls applied for this request may be left unhandled.
> > >>> +            */
> > >>> +           frameContext.error |= Request::PFCError;
> > >>> +
> > >>> +           return frameContext;
> > >>> +   }
> > >>> +};
> > >>> +
> > >>> +} /* namespace ipa */
> > >>> +
> > >>> +} /* namespace libcamera */
> > >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > >>> index fb894bc614af..016b8e0ec9be 100644
> > >>> --- a/src/ipa/libipa/meson.build
> > >>> +++ b/src/ipa/libipa/meson.build
> > >>> @@ -3,6 +3,7 @@
> > >>>    libipa_headers = files([
> > >>>        'algorithm.h',
> > >>>        'camera_sensor_helper.h',
> > >>> +    'fc_queue.h',
> > >>>        'histogram.h',
> > >>>        'module.h',
> > >>>    ])
> > >>> @@ -10,6 +11,7 @@ libipa_headers = files([
> > >>>    libipa_sources = files([
> > >>>        'algorithm.cpp',
> > >>>        'camera_sensor_helper.cpp',
> > >>> +    'fc_queue.cpp',
> > >>>        'histogram.cpp',
> > >>>        'module.cpp',
> > >>>    ])
> > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > >>> index 6cf4d1699ce5..af22dbeb3da5 100644
> > >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > >>> @@ -49,7 +49,7 @@ public:
> > >>>     int init(const IPASettings &settings, unsigned int hwRevision,
> > >>>              ControlInfoMap *ipaControls) override;
> > >>>     int start() override;
> > >>> -   void stop() override {}
> > >>> +   void stop() override;
> > >>>     int configure(const IPACameraSensorInfo &info,
> > >>>                   const std::map<uint32_t, IPAStream> &streamConfig,
> > >>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
> > >>>     return 0;
> > >>>    }
> > >>> +void IPARkISP1::stop()
> > >>> +{
> > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > >>> +   context_ = {};
>
> Indeed - watch out here. I think this breaks start/stop/start cycles as
> suddenly the session configuration is now zeroed.
>
> --
> Kieran
>
>
> > >>> +}
> > >>> +
> > >>>    /**
> > >>>     * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> > >>>     * if the connected sensor does not provide enough information to properly
> > >>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > >>>             << "Exposure: " << minExposure << "-" << maxExposure
> > >>>             << " Gain: " << minGain << "-" << maxGain;
> > >>> -   /* Clean context at configuration */
> > >>> -   context_ = {};
> > >>> -
> > >>>     /* Set the hardware revision for the algorithms. */
> > >>>     context_.configuration.hw.revision = hwRevision_;

Quoting Jacopo Mondi (2022-08-18 08:44:55)
> Hi Kieran
> 
>   thanks a lot for getting to the bottom of this.
> 
> I still can't reproduce it here, but your analysis seems correct to me
> and indeed the context = {} might prove problematic.
> 
> See below
> 
> On Wed, Aug 17, 2022 at 09:33:08PM +0100, Kieran Bingham wrote:
> > Quoting Umang Jain via libcamera-devel (2022-08-11 10:13:42)
> > > Hi Jacopo,
> > >
> > > Thank you for the clarifications,
> > >
> > > On 8/11/22 14:08, Jacopo Mondi wrote:
> > > > Hi Umang
> > > >
> > > > On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> > > >> Hi all,
> > > >>
> > > >> Few comments and some open question for discussion on underruns...
> > > >>
> > > >> On 8/5/22 19:23, Jacopo Mondi wrote:
> > > >>> From: Umang Jain <umang.jain@ideasonboard.com>
> > > >>>
> > > >>> Introduce a common implementation in libipa to represent the queue of
> > > >>> frame contexts.
> > > >>>
> > > >>> Adjust the IPU3 IPAFrameContext to provide the basic class members
> > > >>> required to work with the generic FCQueue implementation, before
> > > >>> introducing an IPAFrameContext class in the next patches.
> > > >>>
> > > >>> Opportunely handle cleaning up the queue and the IPA context at
> > > >>> IPA::stop() time.
> >
> > This is bad. It breaks and causes the IPA to crash...
> >
> > The CTS test
> > android.hardware.cts.CameraTest#testJpegCallbackStartPreview triggers
> > this first...
> >
> > This may be the first test that has a configure ... start ... stop ...
> > start sequence without a reconfigure. Which I think is where we break.
> >
> > Essentially - we can't just clear the entire context_ now. Certainly not
> > between stop/start cycles.
> >
> >
> >
> > > >>>
> > > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >>> ---
> > > >>>    src/ipa/ipu3/ipa_context.cpp |  20 +------
> > > >>>    src/ipa/ipu3/ipa_context.h   |  16 ++---
> > > >>>    src/ipa/ipu3/ipu3.cpp        |  18 +++---
> > > >>>    src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
> > > >>>    src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
> > > >>>    src/ipa/libipa/meson.build   |   2 +
> > > >>>    src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
> > > >>>    7 files changed, 250 insertions(+), 38 deletions(-)
> > > >>>    create mode 100644 src/ipa/libipa/fc_queue.cpp
> > > >>>    create mode 100644 src/ipa/libipa/fc_queue.h
> > > >>>
> > > >>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > > >>> index 13cdb835ef7f..9605c8a1106d 100644
> > > >>> --- a/src/ipa/ipu3/ipa_context.cpp
> > > >>> +++ b/src/ipa/ipu3/ipa_context.cpp
> > > >>> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> > > >>>     * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> > > >>>     */
> > > >>> -/**
> > > >>> - * \brief Default constructor for IPAFrameContext
> > > >>> - */
> > > >>> -IPAFrameContext::IPAFrameContext() = default;
> > > >>> -
> > > >>> -/**
> > > >>> - * \brief Construct a IPAFrameContext instance
> > > >>> - */
> > > >>> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > >>> -   : frame(id), frameControls(reqControls)
> > > >>> -{
> > > >>> -   sensor = {};
> > > >>> -}
> > > >>> -
> > > >>>    /**
> > > >>>     * \var IPAFrameContext::frame
> > > >>>     * \brief The frame number
> > > >>>     *
> > > >>> - * \var IPAFrameContext::frameControls
> > > >>> - * \brief Controls sent in by the application while queuing the request
> > > >>> - *
> > > >>>     * \var IPAFrameContext::sensor
> > > >>>     * \brief Effective sensor values that were applied for the frame
> > > >>>     *
> > > >>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > >>>     *
> > > >>>     * \var IPAFrameContext::sensor.gain
> > > >>>     * \brief Analogue gain multiplier
> > > >>> + *
> > > >>> + * \var IPAFrameContext::error
> > > >>> + * \brief The error flags for this frame context
> > > >>>     */
> > > >>>    } /* namespace libcamera::ipa::ipu3 */
> > > >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > >>> index 42e11141d3a1..dc5b7c30aaf9 100644
> > > >>> --- a/src/ipa/ipu3/ipa_context.h
> > > >>> +++ b/src/ipa/ipu3/ipa_context.h
> > > >>> @@ -8,22 +8,20 @@
> > > >>>    #pragma once
> > > >>> -#include <array>
> > > >>> -
> > > >>>    #include <linux/intel-ipu3.h>
> > > >>>    #include <libcamera/base/utils.h>
> > > >>>    #include <libcamera/controls.h>
> > > >>>    #include <libcamera/geometry.h>
> > > >>> +#include <libcamera/request.h>
> > > >>> +
> > > >>> +#include <libipa/fc_queue.h>
> > > >>>    namespace libcamera {
> > > >>>    namespace ipa::ipu3 {
> > > >>> -/* Maximum number of frame contexts to be held */
> > > >>> -static constexpr uint32_t kMaxFrameContexts = 16;
> > > >>> -
> > > >>>    struct IPASessionConfiguration {
> > > >>>     struct {
> > > >>>             ipu3_uapi_grid_config bdsGrid;
> > > >>> @@ -77,23 +75,21 @@ struct IPAActiveState {
> > > >>>    };
> > > >>>    struct IPAFrameContext {
> > > >>> -   IPAFrameContext();
> > > >>> -   IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > > >>> +   uint32_t frame;
> > > >>>     struct {
> > > >>>             uint32_t exposure;
> > > >>>             double gain;
> > > >>>     } sensor;
> > > >>> -   uint32_t frame;
> > > >>> -   ControlList frameControls;
> > > >>> +   Request::Errors error;
> > > >>>    };
> > > >>>    struct IPAContext {
> > > >>>     IPASessionConfiguration configuration;
> > > >>>     IPAActiveState activeState;
> > > >>> -   std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> > > >>> +   FCQueue<IPAFrameContext> frameContexts;
> > > >>>    };
> > > >>>    } /* namespace ipa::ipu3 */
> > > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > >>> index 2f6bb672f7bb..209a6b040f8f 100644
> > > >>> --- a/src/ipa/ipu3/ipu3.cpp
> > > >>> +++ b/src/ipa/ipu3/ipu3.cpp
> > > >>> @@ -38,6 +38,8 @@
> > > >>>    #include "algorithms/tone_mapping.h"
> > > >>>    #include "libipa/camera_sensor_helper.h"
> > > >>> +#include "ipa_context.h"
> > > >>> +
> > > >>>    /* Minimum grid width, expressed as a number of cells */
> > > >>>    static constexpr uint32_t kMinGridWidth = 16;
> > > >>>    /* Maximum grid width, expressed as a number of cells */
> > > >>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
> > > >>>     */
> > > >>>    void IPAIPU3::stop()
> > > >>>    {
> > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > >>> +   context_.frameContexts.clear();
> > > >>> +   context_ = {};
> >
> > I think this context_ = {}; is the line causing me trouble right now.
> >
> > The IPU3 IPA crashes with a divide by zero error in :
> >
> >  libcamera::ipa::ipu3::algorithms::Af::afEstimateVariance()
> >
> > where y_items.size() = 0, because Af::process() has set afRawBufferLen =
> > 0 from
> >
> >       /* Evaluate the AF buffer length */
> >       uint32_t afRawBufferLen = context.configuration.af.afGrid.width *
> >                                 context.configuration.af.afGrid.height;
> >
> > So because context has been completely zeroed, and not reconfigured - we
> > end up with a zero length span, and a div/0.
> 
> Ouch
> 
> Still doesn't make sense to me that here it doesn't fail..
> 
> >
> > I expect simply removing this context_ = {}; may be sufficient. This may
> > need to be considered for the RKISP too.
> >
> 
> Ok, looking at the context in more detail, it indeed contains the
> session configuration, which shall not be zeroed, but just overwritten
> when a new configure() is issued.
> 
> context_ contains the active state as well, I'm still trying to figure
> out if that one needs to be cleaned in order not to operate on
> stale data when a new streaming session is started.
> 
> The frame context queue does indeed need to be cleared between
> streaming session instead.
> 
> Overall we have a context_ which contains 3 items with different
> lifetime management requirements.
> 
> I wonder if it's a good idea to store all of them together or it would
> be easier to break the FCQ and the active state out from the
> context_. I actually wonder if a struct context {}  makes any sense,
> when the IPA class can have the three items (configuration, queue and
> state) as three class members and handle their lifetime separately.
> 
> To be honest, the role of active state is still a little bit unclear
> to me. It contains a global state of algorithm results without any
> synchronization with the current frame. Re-reading Laurent's comment
> on 05/10 of this series and your reply, it seems we agree the active
> state shall be removed or reworked.

Potentially removed, but I think there's still scope for it. Things like
the current modes of the algorithms might need to be there, it depends
on if algorithms need to look at the modes of other algorithms.

I know for instance, Kate would like to disable AWB during AF scanning.
[0]

I would expect this to happen through the active state, rather than the
frame context.

https://patchwork.libcamera.org/project/libcamera/list/?series=3096

Overall I don't expect the ActiveState to be large, as algorithms could
probably put that information in their own private storage. But maybe if
it's stored as part of the overall context it provides a convenience.

In otherwords, I think it needs to be considered 'case by case' as we go
through and move data into the FrameContext (and thus the fcq).


> For now I think it's then fine to remove the context = {}; from this
> series, and then rework how the IPA context is modeled on top. Do you
> all agree with this ?

Yes, I think as long as we don't clear the entire context structure it's
fine.

--
Kieran



> Thanks
>    j
> 
> >
> > Yes, I've removed this line, and the test doesn't fail now. I'll rerun
> > the whole CTS suite.
> >
> >
> > Same for RKISP below...
> >
> >
> > > >>>    }
> > > >>>    /**
> > > >>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > > >>>     calculateBdsGrid(configInfo.bdsOutputSize);
> > > >>> -   /* Clean IPAActiveState at each reconfiguration. */
> > > >>> -   context_.activeState = {};
> > > >>> -   IPAFrameContext initFrameContext;
> > > >>> -   context_.frameContexts.fill(initFrameContext);
> > > >>> -
> > > >>>     if (!validateSensorControls()) {
> > > >>>             LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > > >>>             return -EINVAL;
> > > >>> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > >>>     if (frameContext.frame != frame)
> > > >>>             LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> > > >>> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > >>>    void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > > >>>    {
> > > >>>     /* \todo Start processing for 'frame' based on 'controls'. */
> > > >>> -   context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> > > >>> +
> > > >>> +   /* \todo Implement queueRequest to each algorithm. */
> > > >>> +   (void)frameContext;
> > > >>> +   (void)controls;
> > > >>>    }
> > > >>>    /**
> > > >>> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > >>> new file mode 100644
> > > >>> index 000000000000..37ffca60b992
> > > >>> --- /dev/null
> > > >>> +++ b/src/ipa/libipa/fc_queue.cpp
> > > >>> @@ -0,0 +1,112 @@
> > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > >>> +/*
> > > >>> + * Copyright (C) 2022, Google Inc.
> > > >>> + *
> > > >>> + * fc_queue.cpp - IPA Frame context queue
> > > >>> + */
> > > >>> +
> > > >>> +#include "fc_queue.h"
> > > >>> +
> > > >>> +#include <libcamera/base/log.h>
> > > >>> +
> > > >>> +namespace libcamera {
> > > >>> +
> > > >>> +LOG_DEFINE_CATEGORY(FCQueue)
> > > >>> +
> > > >>> +namespace ipa {
> > > >>> +
> > > >>> +/**
> > > >>> + * \file fc_queue.h
> > > >>> + * \brief Queue to access per-frame Context
> > > >>> + */
> > > >>> +
> > > >>> +/**
> > > >>> + * \class FCQueue
> > > >>> + * \brief A support class for queueing FrameContext instances through the IPA
> > > >>> + * \tparam FrameContext The IPA specific FrameContext derived class type
> > > >>> + *
> > > >>> + * The frame context queue provides a simple circular buffer implementation to
> > > >>> + * store IPA specific context for each frame through its lifetime within the
> > > >>> + * IPA.
> > > >>> + *
> > > >>> + * FrameContext instances are expected to be referenced by a monotonically
> > > >>> + * increasing sequence count corresponding to a frame sequence number.
> > > >>> + *
> > > >>> + * A FrameContext is initialised for a given frame when the corresponding
> > > >>> + * Request is first queued into the IPA. From that point on the FrameContext can
> > > >>> + * be obtained by the IPA and its algorithms by referencing it from the frame
> > > >>> + * sequence number.
> > > >>> + *
> > > >>> + * A frame sequence number from the image source must correspond to the request
> > > >>> + * sequence number for this implementation to be supported in an IPA. It is
> > > >>> + * expected that the same sequence number will be used to reference the context
> > > >>> + * of the Frame from the point of queueing the request, specifying controls for
> > > >>
> > > >> s/Frame/frame
> > > >>
> > > >>> + * a given frame, and processing of any ISP related controls and statistics for
> > > >>> + * the same corresponding image.
> > > >>> + *
> > > >>> + * IPA specific frame context implementations shall inherit from the
> > > >>> + * IPAFrameContext base class to support the minimum required features for a
> > > >>> + * FrameContext, including the frame number and the error flags that relate to
> > > >>> + * the frame.
> > > >>> + *
> > > >>> + * FrameContexts are overwritten when they are recycled and re-initialised by
> > > >>> + * the first access made on them by either initialise(frame) or get(frame). It
> > > >>> + * is required that the number of available slots in the frame context queue is
> > > >>> + * larger or equal to the maximum number of in-flight requests a pipeline can
> > > >>> + * handle to avoid overwriting frame context instances that still have to be
> > > >>> + * processed.
> > > >>> + *
> > > >>> + * In the case an application does not queue requests to the Camera fast
> > > >>
> > > >> s/Camera/camera/
> > > >>
> > > >>> + * enough, frames can be produced and processed by the IPA without a
> > > >>> + * corresponding Request being queued. In this case the IPA algorithm
> > > >>> + * will try to access the FrameContext with a call to get() before it
> > > >>> + * had been initialized at queueRequest() time. In this case there
> > > >>
> > > >> s/case/case,
> > > >>
> > > >>> + * is now way the controls associated with the late Request could be
> > > >>
> > > >> s/now/no
> > > >>
> > > > Ack to all the above ones, thanks
> > > >
> > > >>> + * applied in time, as the frame as already been processed by the IPA,
> > > >>> + * the PFCError flag is automatically raised on the FrameContext.
> > > >>> + */
> > > >>> +
> > > >>> +/**
> > > >>> + * \fn FCQueue::clear()
> > > >>> + * \brief Clear the context queue
> > > >>> + *
> > > >>> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> > > >>> + * IPA modules are expected to clear the frame context queue at the beginning of
> > > >>> + * a new streaming session, in IPAModule::start().
> > > >>> + */
> > > >>> +
> > > >>> +/**
> > > >>> + * \fn FCQueue::initialise(uint32_t frame)
> > > >>> + * \brief Initialize and return the Frame Context at slot specified by \a frame
> > > >>> + * \param[in] frame The frame context sequence number
> > > >>> + *
> > > >>> + * The first call to obtain a FrameContext from the FCQueue should be handled
> > > >>> + * through this call. The FrameContext will be initialised for the frame and
> > > >>> + * returned to the caller if it was not already initialised.
> > > >>> + *
> > > >>> + * If the FrameContext was already initialized for this sequence, a warning will
> > > >>> + * be reported and the previously initialized FrameContext is returned.
> > > >>> + *
> > > >>> + * Frame contexts are expected to be initialised when a Request is first passed
> > > >>> + * to the IPA module in IPAModule::queueRequest().
> > > >>> + *
> > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > >>> + */
> > > >>> +
> > > >>> +/**
> > > >>> + * \fn FCQueue::get()
> > > >>> + * \brief Obtain the Frame Context at slot specified by \a frame
> > > >>> + * \param[in] frame The frame context sequence number
> > > >>> + *
> > > >>> + * Obtains an existing FrameContext from the queue and returns it to the caller.
> > > >>> + *
> > > >>> + * If the FrameContext is not correctly initialised for the \a frame, it will be
> > > >>> + * initialised and a PFCError will be flagged on the context to be transferred
> > > >>> + * to the Request when it completes.
> > > >>> + *
> > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > >>> + */
> > > >>> +
> > > >>> +} /* namespace ipa */
> > > >>> +
> > > >>> +} /* namespace libcamera */
> > > >>> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > > >>> new file mode 100644
> > > >>> index 000000000000..023b52420fe7
> > > >>> --- /dev/null
> > > >>> +++ b/src/ipa/libipa/fc_queue.h
> > > >>> @@ -0,0 +1,109 @@
> > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > >>> +/*
> > > >>> + * Copyright (C) 2022, Google Inc.
> > > >>> + *
> > > >>> + * fc_queue.h - IPA Frame context queue
> > > >>> + */
> > > >>> +
> > > >>> +#pragma once
> > > >>> +
> > > >>> +#include <array>
> > > >>> +
> > > >>> +#include <libcamera/base/log.h>
> > > >>> +
> > > >>> +#include <libcamera/request.h>
> > > >>> +
> > > >>> +namespace libcamera {
> > > >>> +
> > > >>> +LOG_DECLARE_CATEGORY(FCQueue)
> > > >>> +
> > > >>> +namespace ipa {
> > > >>> +
> > > >>> +/*
> > > >>> + * Maximum number of frame contexts to be held onto
> > > >>> + *
> > > >>> + * \todo Should be platform-specific and match the pipeline depth
> > > >>> + */
> > > >>> +static constexpr uint32_t kMaxFrameContexts = 16;
> > > >>> +
> > > >>> +template<typename FrameContext>
> > > >>> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> > > >>> +{
> > > >>> +private:
> > > >>> +   void initialise(FrameContext &frameContext, const uint32_t frame)
> > > >>> +   {
> > > >>> +           frameContext = {};
> > > >>> +           frameContext.frame = frame;
> > > >>> +   }
> > > >>> +
> > > >>> +public:
> > > >>> +   void clear()
> > > >>> +   {
> > > >>> +           this->fill({});
> > > >>> +   }
> > > >>> +
> > > >>> +   FrameContext &initialise(const uint32_t frame)
> > > >>> +   {
> > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > >>> +
> > > >>> +           /*
> > > >>> +            * Do not re-initialise if a get() call has already fetched this
> > > >>> +            * frame context to preseve the error flags already raised.
> > > >>> +            *
> > > >>> +            * \todo If the the sequence number of the context to initialise
> > > >>> +            * is smaller than the sequence number of the queue slot to use,
> > > >>> +            * it means that we had a serious request underrun and more
> > > >>> +            * frames than the queue size has been produced since the last
> > > >>> +            * time the application has queued a request. Does this deserve
> > > >>> +            * an error condition ?
> > > >>
> > > >> I believe we should log under-runs conditions. Whether we log it as ERROR or
> > > >> WARN or DEBUG might be a candidate for discussion. It depends on how we
> > > >> classify such condition
> > > >>
> > > >> Are under-run requests expected to completed with non-fatal errors set? If
> > > > They indeed expect to be completed :)
> > >
> > >
> > > Ok. This was somehow missing from my mind-map about so many discussions
> > > and PFC document.
> > >
> > > Thanks for clarifying that underruns need to be supported.
> > >
> > > >
> > > >> yes,  I believe we set RequestError::Underrun here and return the
> > > >> initialised frameContext. But then how the Request will be completed ? As
> > > >> the frame number passed here is quite < the current ongoing capture sequence
> > > >> on sensor side. So, we might end up initialising the context but it's never
> > > >> .get() with this frame number, for e.g. while populating metadata in
> > > >> processStatsBuffer()? So we might require some special handling there to
> > > >> detect that a frameContext belongs to a under-run request or not.
> > > >>
> > > >> I think I need to map out how will a under-run request will get completed
> > > >> successfully - IFF we are supporting that case.
> > > >>
> > > > There are legit question, but to properly reply to them I think we
> > > > need to finalise the design of the per-frame-control model. In
> > > > example, for some devices with limited memory (Pi Zero, in example),
> > > > operating in underrun mode seems to be the default, and we have to
> > > > handle that case properly.
> > >
> > >
> > > Agreed.
> > >
> > > Do those discussions block merging of the series? I see you already have
> > > a \todo here, so maybe it is meant to be handled on top and we can merge
> > > this set?
> > >
> > > >
> > > >>> +            */
> > > >>> +           if (frame != 0 && frame <= frameContext.frame)
> > > >>> +                   LOG(FCQueue, Warning)
> > > >>> +                           << "Frame " << frame << " already initialised";
> > > >>> +           else
> > > >>> +                   initialise(frameContext, frame);
> > > >>> +
> > > >>> +           return frameContext;
> > > >>> +   }
> > > >>> +
> > > >>> +   FrameContext &get(uint32_t frame)
> > > >>> +   {
> > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > >>> +
> > > >>> +           /*
> > > >>> +            * If the IPA algorithms try to access a frame context slot which
> > > >>> +            * has been already overwritten by a newer context, it means the
> > > >>> +            * frame context queue has overflowed and the desired context
> > > >>> +            * has been forever lost. The pipeline handler shall avoid
> > > >>> +            * queueing more requests to the IPA than the frame context
> > > >>> +            * queue size.
> > > >>> +            */
> > > >>> +           if (frame < frameContext.frame)
> > > >>> +                   LOG(FCQueue, Fatal)
> > > >>> +                           << "Frame " << frame << " has been overwritten";
> > > >>
> > > >> Maybe the log can be improved:
> > > >>
> > > >>      LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> > > >>                          << frameContext.frame;
> > > >>
> > > >>> +
> > > >>> +           if (frame == frameContext.frame)
> > > >>> +                   return frameContext;
> > > >>> +
> > > >>> +           LOG(FCQueue, Warning)
> > > >>> +                   << "Obtained an uninitialised FrameContext for " << frame;
> > > >>
> > > >> I think currently this Warning is being logged for both cases :
> > > >>
> > > >>      "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> > > > The
> > > >               if (frame < frameContext.frame)
> > > >                       LOG(FCQueue, Fatal)
> > > >                               << "Frame " << frame << " has been overwritten";
> > > >
> > > > will abort execution, so we don't get to this warning.
> > > >
> > > >> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> > > >> terminate the entire program?
> > > >>
> > > > That's the idea, yes.
> > >
> > >
> > > Got it!
> > >
> > > I don't see any other issues with the code here, other than the minors
> > > pointed out so,
> > >
> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > >
> > > Not sure how would tags work with multiple authors in this case.. but
> > > anyways, we can figure it out on the way!
> > >
> > > >
> > > > The above Fatal is triggered by a programming error in the PH/IPA as
> > > > we should never get more requests queued than the FCQ size, so the
> > > > assertion above should not be triggered if not during development of
> > > > new platforms.
> > > >
> > > >>> +
> > > >>> +           initialise(frameContext, frame);
> > > >>> +
> > > >>> +           /*
> > > >>> +            * The frame context has been retrieved before it was
> > > >>> +            * initialised through the initialise() call. This indicates an
> > > >>> +            * algorithm attempted to access a Frame context before it was
> > > >>> +            * queued to the IPA.
> > > >>> +            *
> > > >>> +            * Controls applied for this request may be left unhandled.
> > > >>> +            */
> > > >>> +           frameContext.error |= Request::PFCError;
> > > >>> +
> > > >>> +           return frameContext;
> > > >>> +   }
> > > >>> +};
> > > >>> +
> > > >>> +} /* namespace ipa */
> > > >>> +
> > > >>> +} /* namespace libcamera */
> > > >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > >>> index fb894bc614af..016b8e0ec9be 100644
> > > >>> --- a/src/ipa/libipa/meson.build
> > > >>> +++ b/src/ipa/libipa/meson.build
> > > >>> @@ -3,6 +3,7 @@
> > > >>>    libipa_headers = files([
> > > >>>        'algorithm.h',
> > > >>>        'camera_sensor_helper.h',
> > > >>> +    'fc_queue.h',
> > > >>>        'histogram.h',
> > > >>>        'module.h',
> > > >>>    ])
> > > >>> @@ -10,6 +11,7 @@ libipa_headers = files([
> > > >>>    libipa_sources = files([
> > > >>>        'algorithm.cpp',
> > > >>>        'camera_sensor_helper.cpp',
> > > >>> +    'fc_queue.cpp',
> > > >>>        'histogram.cpp',
> > > >>>        'module.cpp',
> > > >>>    ])
> > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > >>> index 6cf4d1699ce5..af22dbeb3da5 100644
> > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > >>> @@ -49,7 +49,7 @@ public:
> > > >>>     int init(const IPASettings &settings, unsigned int hwRevision,
> > > >>>              ControlInfoMap *ipaControls) override;
> > > >>>     int start() override;
> > > >>> -   void stop() override {}
> > > >>> +   void stop() override;
> > > >>>     int configure(const IPACameraSensorInfo &info,
> > > >>>                   const std::map<uint32_t, IPAStream> &streamConfig,
> > > >>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
> > > >>>     return 0;
> > > >>>    }
> > > >>> +void IPARkISP1::stop()
> > > >>> +{
> > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > >>> +   context_ = {};
> >
> > Indeed - watch out here. I think this breaks start/stop/start cycles as
> > suddenly the session configuration is now zeroed.
> >
> > --
> > Kieran
> >
> >
> > > >>> +}
> > > >>> +
> > > >>>    /**
> > > >>>     * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> > > >>>     * if the connected sensor does not provide enough information to properly
> > > >>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > >>>             << "Exposure: " << minExposure << "-" << maxExposure
> > > >>>             << " Gain: " << minGain << "-" << maxGain;
> > > >>> -   /* Clean context at configuration */
> > > >>> -   context_ = {};
> > > >>> -
> > > >>>     /* Set the hardware revision for the algorithms. */
> > > >>>     context_.configuration.hw.revision = hwRevision_;

Hi Kieran,

On Tue, Aug 23, 2022 at 12:55:24PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi (2022-08-18 08:44:55)
> > Hi Kieran
> > 
> >   thanks a lot for getting to the bottom of this.
> > 
> > I still can't reproduce it here, but your analysis seems correct to me
> > and indeed the context = {} might prove problematic.
> > 
> > See below
> > 
> > On Wed, Aug 17, 2022 at 09:33:08PM +0100, Kieran Bingham wrote:
> > > Quoting Umang Jain via libcamera-devel (2022-08-11 10:13:42)
> > > > Hi Jacopo,
> > > >
> > > > Thank you for the clarifications,
> > > >
> > > > On 8/11/22 14:08, Jacopo Mondi wrote:
> > > > > Hi Umang
> > > > >
> > > > > On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> > > > >> Hi all,
> > > > >>
> > > > >> Few comments and some open question for discussion on underruns...
> > > > >>
> > > > >> On 8/5/22 19:23, Jacopo Mondi wrote:
> > > > >>> From: Umang Jain <umang.jain@ideasonboard.com>
> > > > >>>
> > > > >>> Introduce a common implementation in libipa to represent the queue of
> > > > >>> frame contexts.
> > > > >>>
> > > > >>> Adjust the IPU3 IPAFrameContext to provide the basic class members
> > > > >>> required to work with the generic FCQueue implementation, before
> > > > >>> introducing an IPAFrameContext class in the next patches.
> > > > >>>
> > > > >>> Opportunely handle cleaning up the queue and the IPA context at
> > > > >>> IPA::stop() time.
> > >
> > > This is bad. It breaks and causes the IPA to crash...
> > >
> > > The CTS test
> > > android.hardware.cts.CameraTest#testJpegCallbackStartPreview triggers
> > > this first...
> > >
> > > This may be the first test that has a configure ... start ... stop ...
> > > start sequence without a reconfigure. Which I think is where we break.
> > >
> > > Essentially - we can't just clear the entire context_ now. Certainly not
> > > between stop/start cycles.
> > >
> > > > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > >>> ---
> > > > >>>    src/ipa/ipu3/ipa_context.cpp |  20 +------
> > > > >>>    src/ipa/ipu3/ipa_context.h   |  16 ++---
> > > > >>>    src/ipa/ipu3/ipu3.cpp        |  18 +++---
> > > > >>>    src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
> > > > >>>    src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
> > > > >>>    src/ipa/libipa/meson.build   |   2 +
> > > > >>>    src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
> > > > >>>    7 files changed, 250 insertions(+), 38 deletions(-)
> > > > >>>    create mode 100644 src/ipa/libipa/fc_queue.cpp
> > > > >>>    create mode 100644 src/ipa/libipa/fc_queue.h
> > > > >>>
> > > > >>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > > > >>> index 13cdb835ef7f..9605c8a1106d 100644
> > > > >>> --- a/src/ipa/ipu3/ipa_context.cpp
> > > > >>> +++ b/src/ipa/ipu3/ipa_context.cpp
> > > > >>> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> > > > >>>     * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> > > > >>>     */
> > > > >>> -/**
> > > > >>> - * \brief Default constructor for IPAFrameContext
> > > > >>> - */
> > > > >>> -IPAFrameContext::IPAFrameContext() = default;
> > > > >>> -
> > > > >>> -/**
> > > > >>> - * \brief Construct a IPAFrameContext instance
> > > > >>> - */
> > > > >>> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > > >>> -   : frame(id), frameControls(reqControls)
> > > > >>> -{
> > > > >>> -   sensor = {};
> > > > >>> -}
> > > > >>> -
> > > > >>>    /**
> > > > >>>     * \var IPAFrameContext::frame
> > > > >>>     * \brief The frame number
> > > > >>>     *
> > > > >>> - * \var IPAFrameContext::frameControls
> > > > >>> - * \brief Controls sent in by the application while queuing the request
> > > > >>> - *
> > > > >>>     * \var IPAFrameContext::sensor
> > > > >>>     * \brief Effective sensor values that were applied for the frame
> > > > >>>     *
> > > > >>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > > >>>     *
> > > > >>>     * \var IPAFrameContext::sensor.gain
> > > > >>>     * \brief Analogue gain multiplier
> > > > >>> + *
> > > > >>> + * \var IPAFrameContext::error
> > > > >>> + * \brief The error flags for this frame context
> > > > >>>     */
> > > > >>>    } /* namespace libcamera::ipa::ipu3 */
> > > > >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > > >>> index 42e11141d3a1..dc5b7c30aaf9 100644
> > > > >>> --- a/src/ipa/ipu3/ipa_context.h
> > > > >>> +++ b/src/ipa/ipu3/ipa_context.h
> > > > >>> @@ -8,22 +8,20 @@
> > > > >>>    #pragma once
> > > > >>> -#include <array>
> > > > >>> -
> > > > >>>    #include <linux/intel-ipu3.h>
> > > > >>>    #include <libcamera/base/utils.h>
> > > > >>>    #include <libcamera/controls.h>
> > > > >>>    #include <libcamera/geometry.h>
> > > > >>> +#include <libcamera/request.h>
> > > > >>> +
> > > > >>> +#include <libipa/fc_queue.h>
> > > > >>>    namespace libcamera {
> > > > >>>    namespace ipa::ipu3 {
> > > > >>> -/* Maximum number of frame contexts to be held */
> > > > >>> -static constexpr uint32_t kMaxFrameContexts = 16;
> > > > >>> -
> > > > >>>    struct IPASessionConfiguration {
> > > > >>>     struct {
> > > > >>>             ipu3_uapi_grid_config bdsGrid;
> > > > >>> @@ -77,23 +75,21 @@ struct IPAActiveState {
> > > > >>>    };
> > > > >>>    struct IPAFrameContext {
> > > > >>> -   IPAFrameContext();
> > > > >>> -   IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > > > >>> +   uint32_t frame;
> > > > >>>     struct {
> > > > >>>             uint32_t exposure;
> > > > >>>             double gain;
> > > > >>>     } sensor;
> > > > >>> -   uint32_t frame;
> > > > >>> -   ControlList frameControls;
> > > > >>> +   Request::Errors error;
> > > > >>>    };
> > > > >>>    struct IPAContext {
> > > > >>>     IPASessionConfiguration configuration;
> > > > >>>     IPAActiveState activeState;
> > > > >>> -   std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> > > > >>> +   FCQueue<IPAFrameContext> frameContexts;
> > > > >>>    };
> > > > >>>    } /* namespace ipa::ipu3 */
> > > > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > > >>> index 2f6bb672f7bb..209a6b040f8f 100644
> > > > >>> --- a/src/ipa/ipu3/ipu3.cpp
> > > > >>> +++ b/src/ipa/ipu3/ipu3.cpp
> > > > >>> @@ -38,6 +38,8 @@
> > > > >>>    #include "algorithms/tone_mapping.h"
> > > > >>>    #include "libipa/camera_sensor_helper.h"
> > > > >>> +#include "ipa_context.h"
> > > > >>> +
> > > > >>>    /* Minimum grid width, expressed as a number of cells */
> > > > >>>    static constexpr uint32_t kMinGridWidth = 16;
> > > > >>>    /* Maximum grid width, expressed as a number of cells */
> > > > >>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
> > > > >>>     */
> > > > >>>    void IPAIPU3::stop()
> > > > >>>    {
> > > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > > >>> +   context_.frameContexts.clear();
> > > > >>> +   context_ = {};
> > >
> > > I think this context_ = {}; is the line causing me trouble right now.
> > >
> > > The IPU3 IPA crashes with a divide by zero error in :
> > >
> > >  libcamera::ipa::ipu3::algorithms::Af::afEstimateVariance()
> > >
> > > where y_items.size() = 0, because Af::process() has set afRawBufferLen =
> > > 0 from
> > >
> > >       /* Evaluate the AF buffer length */
> > >       uint32_t afRawBufferLen = context.configuration.af.afGrid.width *
> > >                                 context.configuration.af.afGrid.height;
> > >
> > > So because context has been completely zeroed, and not reconfigured - we
> > > end up with a zero length span, and a div/0.
> > 
> > Ouch
> > 
> > Still doesn't make sense to me that here it doesn't fail..
> > 
> > >
> > > I expect simply removing this context_ = {}; may be sufficient. This may
> > > need to be considered for the RKISP too.
> > >
> > 
> > Ok, looking at the context in more detail, it indeed contains the
> > session configuration, which shall not be zeroed, but just overwritten
> > when a new configure() is issued.
> > 
> > context_ contains the active state as well, I'm still trying to figure
> > out if that one needs to be cleaned in order not to operate on
> > stale data when a new streaming session is started.
> > 
> > The frame context queue does indeed need to be cleared between
> > streaming session instead.
> > 
> > Overall we have a context_ which contains 3 items with different
> > lifetime management requirements.
> > 
> > I wonder if it's a good idea to store all of them together or it would
> > be easier to break the FCQ and the active state out from the
> > context_. I actually wonder if a struct context {}  makes any sense,
> > when the IPA class can have the three items (configuration, queue and
> > state) as three class members and handle their lifetime separately.
> > 
> > To be honest, the role of active state is still a little bit unclear
> > to me. It contains a global state of algorithm results without any
> > synchronization with the current frame. Re-reading Laurent's comment
> > on 05/10 of this series and your reply, it seems we agree the active
> > state shall be removed or reworked.
> 
> Potentially removed, but I think there's still scope for it. Things like
> the current modes of the algorithms might need to be there, it depends
> on if algorithms need to look at the modes of other algorithms.

They important word here is "current". If we want to keep an "active
state", I'll want a formal definition of "current".

> I know for instance, Kate would like to disable AWB during AF scanning.
> [0]
> 
> I would expect this to happen through the active state, rather than the
> frame context.
> 
> https://patchwork.libcamera.org/project/libcamera/list/?series=3096
> 
> Overall I don't expect the ActiveState to be large, as algorithms could
> probably put that information in their own private storage. But maybe if
> it's stored as part of the overall context it provides a convenience.
> 
> In otherwords, I think it needs to be considered 'case by case' as we go
> through and move data into the FrameContext (and thus the fcq).
> 
> > For now I think it's then fine to remove the context = {}; from this
> > series, and then rework how the IPA context is modeled on top. Do you
> > all agree with this ?
> 
> Yes, I think as long as we don't clear the entire context structure it's
> fine.
> 
> > > Yes, I've removed this line, and the test doesn't fail now. I'll rerun
> > > the whole CTS suite.
> > >
> > > Same for RKISP below...
> > >
> > > > >>>    }
> > > > >>>    /**
> > > > >>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > > > >>>     calculateBdsGrid(configInfo.bdsOutputSize);
> > > > >>> -   /* Clean IPAActiveState at each reconfiguration. */
> > > > >>> -   context_.activeState = {};
> > > > >>> -   IPAFrameContext initFrameContext;
> > > > >>> -   context_.frameContexts.fill(initFrameContext);
> > > > >>> -
> > > > >>>     if (!validateSensorControls()) {
> > > > >>>             LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > > > >>>             return -EINVAL;
> > > > >>> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> > > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > >>>     if (frameContext.frame != frame)
> > > > >>>             LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> > > > >>> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > > >>>    void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > > > >>>    {
> > > > >>>     /* \todo Start processing for 'frame' based on 'controls'. */
> > > > >>> -   context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> > > > >>> +
> > > > >>> +   /* \todo Implement queueRequest to each algorithm. */
> > > > >>> +   (void)frameContext;
> > > > >>> +   (void)controls;
> > > > >>>    }
> > > > >>>    /**
> > > > >>> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > > >>> new file mode 100644
> > > > >>> index 000000000000..37ffca60b992
> > > > >>> --- /dev/null
> > > > >>> +++ b/src/ipa/libipa/fc_queue.cpp
> > > > >>> @@ -0,0 +1,112 @@
> > > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > >>> +/*
> > > > >>> + * Copyright (C) 2022, Google Inc.
> > > > >>> + *
> > > > >>> + * fc_queue.cpp - IPA Frame context queue
> > > > >>> + */
> > > > >>> +
> > > > >>> +#include "fc_queue.h"
> > > > >>> +
> > > > >>> +#include <libcamera/base/log.h>
> > > > >>> +
> > > > >>> +namespace libcamera {
> > > > >>> +
> > > > >>> +LOG_DEFINE_CATEGORY(FCQueue)
> > > > >>> +
> > > > >>> +namespace ipa {
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * \file fc_queue.h
> > > > >>> + * \brief Queue to access per-frame Context
> > > > >>> + */
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * \class FCQueue
> > > > >>> + * \brief A support class for queueing FrameContext instances through the IPA
> > > > >>> + * \tparam FrameContext The IPA specific FrameContext derived class type
> > > > >>> + *
> > > > >>> + * The frame context queue provides a simple circular buffer implementation to
> > > > >>> + * store IPA specific context for each frame through its lifetime within the
> > > > >>> + * IPA.
> > > > >>> + *
> > > > >>> + * FrameContext instances are expected to be referenced by a monotonically
> > > > >>> + * increasing sequence count corresponding to a frame sequence number.
> > > > >>> + *
> > > > >>> + * A FrameContext is initialised for a given frame when the corresponding
> > > > >>> + * Request is first queued into the IPA. From that point on the FrameContext can
> > > > >>> + * be obtained by the IPA and its algorithms by referencing it from the frame
> > > > >>> + * sequence number.
> > > > >>> + *
> > > > >>> + * A frame sequence number from the image source must correspond to the request
> > > > >>> + * sequence number for this implementation to be supported in an IPA. It is
> > > > >>> + * expected that the same sequence number will be used to reference the context
> > > > >>> + * of the Frame from the point of queueing the request, specifying controls for
> > > > >>
> > > > >> s/Frame/frame
> > > > >>
> > > > >>> + * a given frame, and processing of any ISP related controls and statistics for
> > > > >>> + * the same corresponding image.
> > > > >>> + *
> > > > >>> + * IPA specific frame context implementations shall inherit from the
> > > > >>> + * IPAFrameContext base class to support the minimum required features for a
> > > > >>> + * FrameContext, including the frame number and the error flags that relate to
> > > > >>> + * the frame.
> > > > >>> + *
> > > > >>> + * FrameContexts are overwritten when they are recycled and re-initialised by
> > > > >>> + * the first access made on them by either initialise(frame) or get(frame). It
> > > > >>> + * is required that the number of available slots in the frame context queue is
> > > > >>> + * larger or equal to the maximum number of in-flight requests a pipeline can
> > > > >>> + * handle to avoid overwriting frame context instances that still have to be
> > > > >>> + * processed.
> > > > >>> + *
> > > > >>> + * In the case an application does not queue requests to the Camera fast
> > > > >>
> > > > >> s/Camera/camera/
> > > > >>
> > > > >>> + * enough, frames can be produced and processed by the IPA without a
> > > > >>> + * corresponding Request being queued. In this case the IPA algorithm
> > > > >>> + * will try to access the FrameContext with a call to get() before it
> > > > >>> + * had been initialized at queueRequest() time. In this case there
> > > > >>
> > > > >> s/case/case,
> > > > >>
> > > > >>> + * is now way the controls associated with the late Request could be
> > > > >>
> > > > >> s/now/no
> > > > >>
> > > > > Ack to all the above ones, thanks
> > > > >
> > > > >>> + * applied in time, as the frame as already been processed by the IPA,
> > > > >>> + * the PFCError flag is automatically raised on the FrameContext.
> > > > >>> + */
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * \fn FCQueue::clear()
> > > > >>> + * \brief Clear the context queue
> > > > >>> + *
> > > > >>> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> > > > >>> + * IPA modules are expected to clear the frame context queue at the beginning of
> > > > >>> + * a new streaming session, in IPAModule::start().
> > > > >>> + */
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * \fn FCQueue::initialise(uint32_t frame)
> > > > >>> + * \brief Initialize and return the Frame Context at slot specified by \a frame
> > > > >>> + * \param[in] frame The frame context sequence number
> > > > >>> + *
> > > > >>> + * The first call to obtain a FrameContext from the FCQueue should be handled
> > > > >>> + * through this call. The FrameContext will be initialised for the frame and
> > > > >>> + * returned to the caller if it was not already initialised.
> > > > >>> + *
> > > > >>> + * If the FrameContext was already initialized for this sequence, a warning will
> > > > >>> + * be reported and the previously initialized FrameContext is returned.
> > > > >>> + *
> > > > >>> + * Frame contexts are expected to be initialised when a Request is first passed
> > > > >>> + * to the IPA module in IPAModule::queueRequest().
> > > > >>> + *
> > > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > > >>> + */
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * \fn FCQueue::get()
> > > > >>> + * \brief Obtain the Frame Context at slot specified by \a frame
> > > > >>> + * \param[in] frame The frame context sequence number
> > > > >>> + *
> > > > >>> + * Obtains an existing FrameContext from the queue and returns it to the caller.
> > > > >>> + *
> > > > >>> + * If the FrameContext is not correctly initialised for the \a frame, it will be
> > > > >>> + * initialised and a PFCError will be flagged on the context to be transferred
> > > > >>> + * to the Request when it completes.
> > > > >>> + *
> > > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > > >>> + */
> > > > >>> +
> > > > >>> +} /* namespace ipa */
> > > > >>> +
> > > > >>> +} /* namespace libcamera */
> > > > >>> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > > > >>> new file mode 100644
> > > > >>> index 000000000000..023b52420fe7
> > > > >>> --- /dev/null
> > > > >>> +++ b/src/ipa/libipa/fc_queue.h
> > > > >>> @@ -0,0 +1,109 @@
> > > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > >>> +/*
> > > > >>> + * Copyright (C) 2022, Google Inc.
> > > > >>> + *
> > > > >>> + * fc_queue.h - IPA Frame context queue
> > > > >>> + */
> > > > >>> +
> > > > >>> +#pragma once
> > > > >>> +
> > > > >>> +#include <array>
> > > > >>> +
> > > > >>> +#include <libcamera/base/log.h>
> > > > >>> +
> > > > >>> +#include <libcamera/request.h>
> > > > >>> +
> > > > >>> +namespace libcamera {
> > > > >>> +
> > > > >>> +LOG_DECLARE_CATEGORY(FCQueue)
> > > > >>> +
> > > > >>> +namespace ipa {
> > > > >>> +
> > > > >>> +/*
> > > > >>> + * Maximum number of frame contexts to be held onto
> > > > >>> + *
> > > > >>> + * \todo Should be platform-specific and match the pipeline depth
> > > > >>> + */
> > > > >>> +static constexpr uint32_t kMaxFrameContexts = 16;
> > > > >>> +
> > > > >>> +template<typename FrameContext>
> > > > >>> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> > > > >>> +{
> > > > >>> +private:
> > > > >>> +   void initialise(FrameContext &frameContext, const uint32_t frame)
> > > > >>> +   {
> > > > >>> +           frameContext = {};
> > > > >>> +           frameContext.frame = frame;
> > > > >>> +   }
> > > > >>> +
> > > > >>> +public:
> > > > >>> +   void clear()
> > > > >>> +   {
> > > > >>> +           this->fill({});
> > > > >>> +   }
> > > > >>> +
> > > > >>> +   FrameContext &initialise(const uint32_t frame)
> > > > >>> +   {
> > > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > > >>> +
> > > > >>> +           /*
> > > > >>> +            * Do not re-initialise if a get() call has already fetched this
> > > > >>> +            * frame context to preseve the error flags already raised.
> > > > >>> +            *
> > > > >>> +            * \todo If the the sequence number of the context to initialise
> > > > >>> +            * is smaller than the sequence number of the queue slot to use,
> > > > >>> +            * it means that we had a serious request underrun and more
> > > > >>> +            * frames than the queue size has been produced since the last
> > > > >>> +            * time the application has queued a request. Does this deserve
> > > > >>> +            * an error condition ?
> > > > >>
> > > > >> I believe we should log under-runs conditions. Whether we log it as ERROR or
> > > > >> WARN or DEBUG might be a candidate for discussion. It depends on how we
> > > > >> classify such condition
> > > > >>
> > > > >> Are under-run requests expected to completed with non-fatal errors set? If
> > > > > They indeed expect to be completed :)
> > > >
> > > >
> > > > Ok. This was somehow missing from my mind-map about so many discussions
> > > > and PFC document.
> > > >
> > > > Thanks for clarifying that underruns need to be supported.
> > > >
> > > > >
> > > > >> yes,  I believe we set RequestError::Underrun here and return the
> > > > >> initialised frameContext. But then how the Request will be completed ? As
> > > > >> the frame number passed here is quite < the current ongoing capture sequence
> > > > >> on sensor side. So, we might end up initialising the context but it's never
> > > > >> .get() with this frame number, for e.g. while populating metadata in
> > > > >> processStatsBuffer()? So we might require some special handling there to
> > > > >> detect that a frameContext belongs to a under-run request or not.
> > > > >>
> > > > >> I think I need to map out how will a under-run request will get completed
> > > > >> successfully - IFF we are supporting that case.
> > > > >>
> > > > > There are legit question, but to properly reply to them I think we
> > > > > need to finalise the design of the per-frame-control model. In
> > > > > example, for some devices with limited memory (Pi Zero, in example),
> > > > > operating in underrun mode seems to be the default, and we have to
> > > > > handle that case properly.
> > > >
> > > >
> > > > Agreed.
> > > >
> > > > Do those discussions block merging of the series? I see you already have
> > > > a \todo here, so maybe it is meant to be handled on top and we can merge
> > > > this set?
> > > >
> > > > >
> > > > >>> +            */
> > > > >>> +           if (frame != 0 && frame <= frameContext.frame)
> > > > >>> +                   LOG(FCQueue, Warning)
> > > > >>> +                           << "Frame " << frame << " already initialised";
> > > > >>> +           else
> > > > >>> +                   initialise(frameContext, frame);
> > > > >>> +
> > > > >>> +           return frameContext;
> > > > >>> +   }
> > > > >>> +
> > > > >>> +   FrameContext &get(uint32_t frame)
> > > > >>> +   {
> > > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > > >>> +
> > > > >>> +           /*
> > > > >>> +            * If the IPA algorithms try to access a frame context slot which
> > > > >>> +            * has been already overwritten by a newer context, it means the
> > > > >>> +            * frame context queue has overflowed and the desired context
> > > > >>> +            * has been forever lost. The pipeline handler shall avoid
> > > > >>> +            * queueing more requests to the IPA than the frame context
> > > > >>> +            * queue size.
> > > > >>> +            */
> > > > >>> +           if (frame < frameContext.frame)
> > > > >>> +                   LOG(FCQueue, Fatal)
> > > > >>> +                           << "Frame " << frame << " has been overwritten";
> > > > >>
> > > > >> Maybe the log can be improved:
> > > > >>
> > > > >>      LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> > > > >>                          << frameContext.frame;
> > > > >>
> > > > >>> +
> > > > >>> +           if (frame == frameContext.frame)
> > > > >>> +                   return frameContext;
> > > > >>> +
> > > > >>> +           LOG(FCQueue, Warning)
> > > > >>> +                   << "Obtained an uninitialised FrameContext for " << frame;
> > > > >>
> > > > >> I think currently this Warning is being logged for both cases :
> > > > >>
> > > > >>      "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> > > > > The
> > > > >               if (frame < frameContext.frame)
> > > > >                       LOG(FCQueue, Fatal)
> > > > >                               << "Frame " << frame << " has been overwritten";
> > > > >
> > > > > will abort execution, so we don't get to this warning.
> > > > >
> > > > >> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> > > > >> terminate the entire program?
> > > > >>
> > > > > That's the idea, yes.
> > > >
> > > >
> > > > Got it!
> > > >
> > > > I don't see any other issues with the code here, other than the minors
> > > > pointed out so,
> > > >
> > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > > >
> > > > Not sure how would tags work with multiple authors in this case.. but
> > > > anyways, we can figure it out on the way!
> > > >
> > > > >
> > > > > The above Fatal is triggered by a programming error in the PH/IPA as
> > > > > we should never get more requests queued than the FCQ size, so the
> > > > > assertion above should not be triggered if not during development of
> > > > > new platforms.
> > > > >
> > > > >>> +
> > > > >>> +           initialise(frameContext, frame);
> > > > >>> +
> > > > >>> +           /*
> > > > >>> +            * The frame context has been retrieved before it was
> > > > >>> +            * initialised through the initialise() call. This indicates an
> > > > >>> +            * algorithm attempted to access a Frame context before it was
> > > > >>> +            * queued to the IPA.
> > > > >>> +            *
> > > > >>> +            * Controls applied for this request may be left unhandled.
> > > > >>> +            */
> > > > >>> +           frameContext.error |= Request::PFCError;
> > > > >>> +
> > > > >>> +           return frameContext;
> > > > >>> +   }
> > > > >>> +};
> > > > >>> +
> > > > >>> +} /* namespace ipa */
> > > > >>> +
> > > > >>> +} /* namespace libcamera */
> > > > >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > > >>> index fb894bc614af..016b8e0ec9be 100644
> > > > >>> --- a/src/ipa/libipa/meson.build
> > > > >>> +++ b/src/ipa/libipa/meson.build
> > > > >>> @@ -3,6 +3,7 @@
> > > > >>>    libipa_headers = files([
> > > > >>>        'algorithm.h',
> > > > >>>        'camera_sensor_helper.h',
> > > > >>> +    'fc_queue.h',
> > > > >>>        'histogram.h',
> > > > >>>        'module.h',
> > > > >>>    ])
> > > > >>> @@ -10,6 +11,7 @@ libipa_headers = files([
> > > > >>>    libipa_sources = files([
> > > > >>>        'algorithm.cpp',
> > > > >>>        'camera_sensor_helper.cpp',
> > > > >>> +    'fc_queue.cpp',
> > > > >>>        'histogram.cpp',
> > > > >>>        'module.cpp',
> > > > >>>    ])
> > > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > >>> index 6cf4d1699ce5..af22dbeb3da5 100644
> > > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > >>> @@ -49,7 +49,7 @@ public:
> > > > >>>     int init(const IPASettings &settings, unsigned int hwRevision,
> > > > >>>              ControlInfoMap *ipaControls) override;
> > > > >>>     int start() override;
> > > > >>> -   void stop() override {}
> > > > >>> +   void stop() override;
> > > > >>>     int configure(const IPACameraSensorInfo &info,
> > > > >>>                   const std::map<uint32_t, IPAStream> &streamConfig,
> > > > >>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
> > > > >>>     return 0;
> > > > >>>    }
> > > > >>> +void IPARkISP1::stop()
> > > > >>> +{
> > > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > > >>> +   context_ = {};
> > >
> > > Indeed - watch out here. I think this breaks start/stop/start cycles as
> > > suddenly the session configuration is now zeroed.
> > >
> > > > >>> +}
> > > > >>> +
> > > > >>>    /**
> > > > >>>     * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> > > > >>>     * if the connected sensor does not provide enough information to properly
> > > > >>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > > >>>             << "Exposure: " << minExposure << "-" << maxExposure
> > > > >>>             << " Gain: " << minGain << "-" << maxGain;
> > > > >>> -   /* Clean context at configuration */
> > > > >>> -   context_ = {};
> > > > >>> -
> > > > >>>     /* Set the hardware revision for the algorithms. */
> > > > >>>     context_.configuration.hw.revision = hwRevision_;

Quoting Laurent Pinchart (2022-08-23 23:52:05)
> Hi Kieran,
> 
> On Tue, Aug 23, 2022 at 12:55:24PM +0100, Kieran Bingham via libcamera-devel wrote:
> > Quoting Jacopo Mondi (2022-08-18 08:44:55)
> > > Hi Kieran
> > > 
> > >   thanks a lot for getting to the bottom of this.
> > > 
> > > I still can't reproduce it here, but your analysis seems correct to me
> > > and indeed the context = {} might prove problematic.
> > > 
> > > See below
> > > 
> > > On Wed, Aug 17, 2022 at 09:33:08PM +0100, Kieran Bingham wrote:
> > > > Quoting Umang Jain via libcamera-devel (2022-08-11 10:13:42)
> > > > > Hi Jacopo,
> > > > >
> > > > > Thank you for the clarifications,
> > > > >
> > > > > On 8/11/22 14:08, Jacopo Mondi wrote:
> > > > > > Hi Umang
> > > > > >
> > > > > > On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> > > > > >> Hi all,
> > > > > >>
> > > > > >> Few comments and some open question for discussion on underruns...
> > > > > >>
> > > > > >> On 8/5/22 19:23, Jacopo Mondi wrote:
> > > > > >>> From: Umang Jain <umang.jain@ideasonboard.com>
> > > > > >>>
> > > > > >>> Introduce a common implementation in libipa to represent the queue of
> > > > > >>> frame contexts.
> > > > > >>>
> > > > > >>> Adjust the IPU3 IPAFrameContext to provide the basic class members
> > > > > >>> required to work with the generic FCQueue implementation, before
> > > > > >>> introducing an IPAFrameContext class in the next patches.
> > > > > >>>
> > > > > >>> Opportunely handle cleaning up the queue and the IPA context at
> > > > > >>> IPA::stop() time.
> > > >
> > > > This is bad. It breaks and causes the IPA to crash...
> > > >
> > > > The CTS test
> > > > android.hardware.cts.CameraTest#testJpegCallbackStartPreview triggers
> > > > this first...
> > > >
> > > > This may be the first test that has a configure ... start ... stop ...
> > > > start sequence without a reconfigure. Which I think is where we break.
> > > >
> > > > Essentially - we can't just clear the entire context_ now. Certainly not
> > > > between stop/start cycles.
> > > >
> > > > > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > >>> ---
> > > > > >>>    src/ipa/ipu3/ipa_context.cpp |  20 +------
> > > > > >>>    src/ipa/ipu3/ipa_context.h   |  16 ++---
> > > > > >>>    src/ipa/ipu3/ipu3.cpp        |  18 +++---
> > > > > >>>    src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
> > > > > >>>    src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
> > > > > >>>    src/ipa/libipa/meson.build   |   2 +
> > > > > >>>    src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
> > > > > >>>    7 files changed, 250 insertions(+), 38 deletions(-)
> > > > > >>>    create mode 100644 src/ipa/libipa/fc_queue.cpp
> > > > > >>>    create mode 100644 src/ipa/libipa/fc_queue.h
> > > > > >>>
> > > > > >>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > > > > >>> index 13cdb835ef7f..9605c8a1106d 100644
> > > > > >>> --- a/src/ipa/ipu3/ipa_context.cpp
> > > > > >>> +++ b/src/ipa/ipu3/ipa_context.cpp
> > > > > >>> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> > > > > >>>     * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> > > > > >>>     */
> > > > > >>> -/**
> > > > > >>> - * \brief Default constructor for IPAFrameContext
> > > > > >>> - */
> > > > > >>> -IPAFrameContext::IPAFrameContext() = default;
> > > > > >>> -
> > > > > >>> -/**
> > > > > >>> - * \brief Construct a IPAFrameContext instance
> > > > > >>> - */
> > > > > >>> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > > > >>> -   : frame(id), frameControls(reqControls)
> > > > > >>> -{
> > > > > >>> -   sensor = {};
> > > > > >>> -}
> > > > > >>> -
> > > > > >>>    /**
> > > > > >>>     * \var IPAFrameContext::frame
> > > > > >>>     * \brief The frame number
> > > > > >>>     *
> > > > > >>> - * \var IPAFrameContext::frameControls
> > > > > >>> - * \brief Controls sent in by the application while queuing the request
> > > > > >>> - *
> > > > > >>>     * \var IPAFrameContext::sensor
> > > > > >>>     * \brief Effective sensor values that were applied for the frame
> > > > > >>>     *
> > > > > >>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > > > >>>     *
> > > > > >>>     * \var IPAFrameContext::sensor.gain
> > > > > >>>     * \brief Analogue gain multiplier
> > > > > >>> + *
> > > > > >>> + * \var IPAFrameContext::error
> > > > > >>> + * \brief The error flags for this frame context
> > > > > >>>     */
> > > > > >>>    } /* namespace libcamera::ipa::ipu3 */
> > > > > >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > > > >>> index 42e11141d3a1..dc5b7c30aaf9 100644
> > > > > >>> --- a/src/ipa/ipu3/ipa_context.h
> > > > > >>> +++ b/src/ipa/ipu3/ipa_context.h
> > > > > >>> @@ -8,22 +8,20 @@
> > > > > >>>    #pragma once
> > > > > >>> -#include <array>
> > > > > >>> -
> > > > > >>>    #include <linux/intel-ipu3.h>
> > > > > >>>    #include <libcamera/base/utils.h>
> > > > > >>>    #include <libcamera/controls.h>
> > > > > >>>    #include <libcamera/geometry.h>
> > > > > >>> +#include <libcamera/request.h>
> > > > > >>> +
> > > > > >>> +#include <libipa/fc_queue.h>
> > > > > >>>    namespace libcamera {
> > > > > >>>    namespace ipa::ipu3 {
> > > > > >>> -/* Maximum number of frame contexts to be held */
> > > > > >>> -static constexpr uint32_t kMaxFrameContexts = 16;
> > > > > >>> -
> > > > > >>>    struct IPASessionConfiguration {
> > > > > >>>     struct {
> > > > > >>>             ipu3_uapi_grid_config bdsGrid;
> > > > > >>> @@ -77,23 +75,21 @@ struct IPAActiveState {
> > > > > >>>    };
> > > > > >>>    struct IPAFrameContext {
> > > > > >>> -   IPAFrameContext();
> > > > > >>> -   IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > > > > >>> +   uint32_t frame;
> > > > > >>>     struct {
> > > > > >>>             uint32_t exposure;
> > > > > >>>             double gain;
> > > > > >>>     } sensor;
> > > > > >>> -   uint32_t frame;
> > > > > >>> -   ControlList frameControls;
> > > > > >>> +   Request::Errors error;
> > > > > >>>    };
> > > > > >>>    struct IPAContext {
> > > > > >>>     IPASessionConfiguration configuration;
> > > > > >>>     IPAActiveState activeState;
> > > > > >>> -   std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> > > > > >>> +   FCQueue<IPAFrameContext> frameContexts;
> > > > > >>>    };
> > > > > >>>    } /* namespace ipa::ipu3 */
> > > > > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > > > >>> index 2f6bb672f7bb..209a6b040f8f 100644
> > > > > >>> --- a/src/ipa/ipu3/ipu3.cpp
> > > > > >>> +++ b/src/ipa/ipu3/ipu3.cpp
> > > > > >>> @@ -38,6 +38,8 @@
> > > > > >>>    #include "algorithms/tone_mapping.h"
> > > > > >>>    #include "libipa/camera_sensor_helper.h"
> > > > > >>> +#include "ipa_context.h"
> > > > > >>> +
> > > > > >>>    /* Minimum grid width, expressed as a number of cells */
> > > > > >>>    static constexpr uint32_t kMinGridWidth = 16;
> > > > > >>>    /* Maximum grid width, expressed as a number of cells */
> > > > > >>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
> > > > > >>>     */
> > > > > >>>    void IPAIPU3::stop()
> > > > > >>>    {
> > > > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > > > >>> +   context_.frameContexts.clear();
> > > > > >>> +   context_ = {};
> > > >
> > > > I think this context_ = {}; is the line causing me trouble right now.
> > > >
> > > > The IPU3 IPA crashes with a divide by zero error in :
> > > >
> > > >  libcamera::ipa::ipu3::algorithms::Af::afEstimateVariance()
> > > >
> > > > where y_items.size() = 0, because Af::process() has set afRawBufferLen =
> > > > 0 from
> > > >
> > > >       /* Evaluate the AF buffer length */
> > > >       uint32_t afRawBufferLen = context.configuration.af.afGrid.width *
> > > >                                 context.configuration.af.afGrid.height;
> > > >
> > > > So because context has been completely zeroed, and not reconfigured - we
> > > > end up with a zero length span, and a div/0.
> > > 
> > > Ouch
> > > 
> > > Still doesn't make sense to me that here it doesn't fail..
> > > 
> > > >
> > > > I expect simply removing this context_ = {}; may be sufficient. This may
> > > > need to be considered for the RKISP too.
> > > >
> > > 
> > > Ok, looking at the context in more detail, it indeed contains the
> > > session configuration, which shall not be zeroed, but just overwritten
> > > when a new configure() is issued.
> > > 
> > > context_ contains the active state as well, I'm still trying to figure
> > > out if that one needs to be cleaned in order not to operate on
> > > stale data when a new streaming session is started.
> > > 
> > > The frame context queue does indeed need to be cleared between
> > > streaming session instead.
> > > 
> > > Overall we have a context_ which contains 3 items with different
> > > lifetime management requirements.
> > > 
> > > I wonder if it's a good idea to store all of them together or it would
> > > be easier to break the FCQ and the active state out from the
> > > context_. I actually wonder if a struct context {}  makes any sense,
> > > when the IPA class can have the three items (configuration, queue and
> > > state) as three class members and handle their lifetime separately.
> > > 
> > > To be honest, the role of active state is still a little bit unclear
> > > to me. It contains a global state of algorithm results without any
> > > synchronization with the current frame. Re-reading Laurent's comment
> > > on 05/10 of this series and your reply, it seems we agree the active
> > > state shall be removed or reworked.
> > 
> > Potentially removed, but I think there's still scope for it. Things like
> > the current modes of the algorithms might need to be there, it depends
> > on if algorithms need to look at the modes of other algorithms.
> 
> They important word here is "current". If we want to keep an "active
> state", I'll want a formal definition of "current".

Let me phrase it another way ... Active state will affect all future
processing. *How algorithms process that is up to them* and we'll know
more about that when we actually start trying to do it... which we are
not yet doing.

 
> > I know for instance, Kate would like to disable AWB during AF scanning.
> > [0]

Like in this example for instance, if the AWB is disabled, or held, then
without an active state (or an explicitly named call for every possible
use case into each algorithm) then we have to modify *every* frame
context in the queue, including future ones. *Until* the mode is changed
back.

Feel free to define something now - but I think that can be handled when
we get there, and we actually know what it will be rather than trying to
'guess' in advance.


> > I would expect this to happen through the active state, rather than the
> > frame context.
> > 
> > https://patchwork.libcamera.org/project/libcamera/list/?series=3096
> > 
> > Overall I don't expect the ActiveState to be large, as algorithms could
> > probably put that information in their own private storage. But maybe if
> > it's stored as part of the overall context it provides a convenience.
> > 
> > In otherwords, I think it needs to be considered 'case by case' as we go
> > through and move data into the FrameContext (and thus the fcq).
> > 
> > > For now I think it's then fine to remove the context = {}; from this
> > > series, and then rework how the IPA context is modeled on top. Do you
> > > all agree with this ?
> > 
> > Yes, I think as long as we don't clear the entire context structure it's
> > fine.
> > 
> > > > Yes, I've removed this line, and the test doesn't fail now. I'll rerun
> > > > the whole CTS suite.
> > > >
> > > > Same for RKISP below...
> > > >
> > > > > >>>    }
> > > > > >>>    /**
> > > > > >>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > > > > >>>     calculateBdsGrid(configInfo.bdsOutputSize);
> > > > > >>> -   /* Clean IPAActiveState at each reconfiguration. */
> > > > > >>> -   context_.activeState = {};
> > > > > >>> -   IPAFrameContext initFrameContext;
> > > > > >>> -   context_.frameContexts.fill(initFrameContext);
> > > > > >>> -
> > > > > >>>     if (!validateSensorControls()) {
> > > > > >>>             LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > > > > >>>             return -EINVAL;
> > > > > >>> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> > > > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > > >>>     if (frameContext.frame != frame)
> > > > > >>>             LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> > > > > >>> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > > > >>>    void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > > > > >>>    {
> > > > > >>>     /* \todo Start processing for 'frame' based on 'controls'. */
> > > > > >>> -   context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > > > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> > > > > >>> +
> > > > > >>> +   /* \todo Implement queueRequest to each algorithm. */
> > > > > >>> +   (void)frameContext;
> > > > > >>> +   (void)controls;
> > > > > >>>    }
> > > > > >>>    /**
> > > > > >>> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > > > >>> new file mode 100644
> > > > > >>> index 000000000000..37ffca60b992
> > > > > >>> --- /dev/null
> > > > > >>> +++ b/src/ipa/libipa/fc_queue.cpp
> > > > > >>> @@ -0,0 +1,112 @@
> > > > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > >>> +/*
> > > > > >>> + * Copyright (C) 2022, Google Inc.
> > > > > >>> + *
> > > > > >>> + * fc_queue.cpp - IPA Frame context queue
> > > > > >>> + */
> > > > > >>> +
> > > > > >>> +#include "fc_queue.h"
> > > > > >>> +
> > > > > >>> +#include <libcamera/base/log.h>
> > > > > >>> +
> > > > > >>> +namespace libcamera {
> > > > > >>> +
> > > > > >>> +LOG_DEFINE_CATEGORY(FCQueue)
> > > > > >>> +
> > > > > >>> +namespace ipa {
> > > > > >>> +
> > > > > >>> +/**
> > > > > >>> + * \file fc_queue.h
> > > > > >>> + * \brief Queue to access per-frame Context
> > > > > >>> + */
> > > > > >>> +
> > > > > >>> +/**
> > > > > >>> + * \class FCQueue
> > > > > >>> + * \brief A support class for queueing FrameContext instances through the IPA
> > > > > >>> + * \tparam FrameContext The IPA specific FrameContext derived class type
> > > > > >>> + *
> > > > > >>> + * The frame context queue provides a simple circular buffer implementation to
> > > > > >>> + * store IPA specific context for each frame through its lifetime within the
> > > > > >>> + * IPA.
> > > > > >>> + *
> > > > > >>> + * FrameContext instances are expected to be referenced by a monotonically
> > > > > >>> + * increasing sequence count corresponding to a frame sequence number.
> > > > > >>> + *
> > > > > >>> + * A FrameContext is initialised for a given frame when the corresponding
> > > > > >>> + * Request is first queued into the IPA. From that point on the FrameContext can
> > > > > >>> + * be obtained by the IPA and its algorithms by referencing it from the frame
> > > > > >>> + * sequence number.
> > > > > >>> + *
> > > > > >>> + * A frame sequence number from the image source must correspond to the request
> > > > > >>> + * sequence number for this implementation to be supported in an IPA. It is
> > > > > >>> + * expected that the same sequence number will be used to reference the context
> > > > > >>> + * of the Frame from the point of queueing the request, specifying controls for
> > > > > >>
> > > > > >> s/Frame/frame
> > > > > >>
> > > > > >>> + * a given frame, and processing of any ISP related controls and statistics for
> > > > > >>> + * the same corresponding image.
> > > > > >>> + *
> > > > > >>> + * IPA specific frame context implementations shall inherit from the
> > > > > >>> + * IPAFrameContext base class to support the minimum required features for a
> > > > > >>> + * FrameContext, including the frame number and the error flags that relate to
> > > > > >>> + * the frame.
> > > > > >>> + *
> > > > > >>> + * FrameContexts are overwritten when they are recycled and re-initialised by
> > > > > >>> + * the first access made on them by either initialise(frame) or get(frame). It
> > > > > >>> + * is required that the number of available slots in the frame context queue is
> > > > > >>> + * larger or equal to the maximum number of in-flight requests a pipeline can
> > > > > >>> + * handle to avoid overwriting frame context instances that still have to be
> > > > > >>> + * processed.
> > > > > >>> + *
> > > > > >>> + * In the case an application does not queue requests to the Camera fast
> > > > > >>
> > > > > >> s/Camera/camera/
> > > > > >>
> > > > > >>> + * enough, frames can be produced and processed by the IPA without a
> > > > > >>> + * corresponding Request being queued. In this case the IPA algorithm
> > > > > >>> + * will try to access the FrameContext with a call to get() before it
> > > > > >>> + * had been initialized at queueRequest() time. In this case there
> > > > > >>
> > > > > >> s/case/case,
> > > > > >>
> > > > > >>> + * is now way the controls associated with the late Request could be
> > > > > >>
> > > > > >> s/now/no
> > > > > >>
> > > > > > Ack to all the above ones, thanks
> > > > > >
> > > > > >>> + * applied in time, as the frame as already been processed by the IPA,
> > > > > >>> + * the PFCError flag is automatically raised on the FrameContext.
> > > > > >>> + */
> > > > > >>> +
> > > > > >>> +/**
> > > > > >>> + * \fn FCQueue::clear()
> > > > > >>> + * \brief Clear the context queue
> > > > > >>> + *
> > > > > >>> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> > > > > >>> + * IPA modules are expected to clear the frame context queue at the beginning of
> > > > > >>> + * a new streaming session, in IPAModule::start().
> > > > > >>> + */
> > > > > >>> +
> > > > > >>> +/**
> > > > > >>> + * \fn FCQueue::initialise(uint32_t frame)
> > > > > >>> + * \brief Initialize and return the Frame Context at slot specified by \a frame
> > > > > >>> + * \param[in] frame The frame context sequence number
> > > > > >>> + *
> > > > > >>> + * The first call to obtain a FrameContext from the FCQueue should be handled
> > > > > >>> + * through this call. The FrameContext will be initialised for the frame and
> > > > > >>> + * returned to the caller if it was not already initialised.
> > > > > >>> + *
> > > > > >>> + * If the FrameContext was already initialized for this sequence, a warning will
> > > > > >>> + * be reported and the previously initialized FrameContext is returned.
> > > > > >>> + *
> > > > > >>> + * Frame contexts are expected to be initialised when a Request is first passed
> > > > > >>> + * to the IPA module in IPAModule::queueRequest().
> > > > > >>> + *
> > > > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > > > >>> + */
> > > > > >>> +
> > > > > >>> +/**
> > > > > >>> + * \fn FCQueue::get()
> > > > > >>> + * \brief Obtain the Frame Context at slot specified by \a frame
> > > > > >>> + * \param[in] frame The frame context sequence number
> > > > > >>> + *
> > > > > >>> + * Obtains an existing FrameContext from the queue and returns it to the caller.
> > > > > >>> + *
> > > > > >>> + * If the FrameContext is not correctly initialised for the \a frame, it will be
> > > > > >>> + * initialised and a PFCError will be flagged on the context to be transferred
> > > > > >>> + * to the Request when it completes.
> > > > > >>> + *
> > > > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > > > >>> + */
> > > > > >>> +
> > > > > >>> +} /* namespace ipa */
> > > > > >>> +
> > > > > >>> +} /* namespace libcamera */
> > > > > >>> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > > > > >>> new file mode 100644
> > > > > >>> index 000000000000..023b52420fe7
> > > > > >>> --- /dev/null
> > > > > >>> +++ b/src/ipa/libipa/fc_queue.h
> > > > > >>> @@ -0,0 +1,109 @@
> > > > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > >>> +/*
> > > > > >>> + * Copyright (C) 2022, Google Inc.
> > > > > >>> + *
> > > > > >>> + * fc_queue.h - IPA Frame context queue
> > > > > >>> + */
> > > > > >>> +
> > > > > >>> +#pragma once
> > > > > >>> +
> > > > > >>> +#include <array>
> > > > > >>> +
> > > > > >>> +#include <libcamera/base/log.h>
> > > > > >>> +
> > > > > >>> +#include <libcamera/request.h>
> > > > > >>> +
> > > > > >>> +namespace libcamera {
> > > > > >>> +
> > > > > >>> +LOG_DECLARE_CATEGORY(FCQueue)
> > > > > >>> +
> > > > > >>> +namespace ipa {
> > > > > >>> +
> > > > > >>> +/*
> > > > > >>> + * Maximum number of frame contexts to be held onto
> > > > > >>> + *
> > > > > >>> + * \todo Should be platform-specific and match the pipeline depth
> > > > > >>> + */
> > > > > >>> +static constexpr uint32_t kMaxFrameContexts = 16;
> > > > > >>> +
> > > > > >>> +template<typename FrameContext>
> > > > > >>> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> > > > > >>> +{
> > > > > >>> +private:
> > > > > >>> +   void initialise(FrameContext &frameContext, const uint32_t frame)
> > > > > >>> +   {
> > > > > >>> +           frameContext = {};
> > > > > >>> +           frameContext.frame = frame;
> > > > > >>> +   }
> > > > > >>> +
> > > > > >>> +public:
> > > > > >>> +   void clear()
> > > > > >>> +   {
> > > > > >>> +           this->fill({});
> > > > > >>> +   }
> > > > > >>> +
> > > > > >>> +   FrameContext &initialise(const uint32_t frame)
> > > > > >>> +   {
> > > > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > > > >>> +
> > > > > >>> +           /*
> > > > > >>> +            * Do not re-initialise if a get() call has already fetched this
> > > > > >>> +            * frame context to preseve the error flags already raised.
> > > > > >>> +            *
> > > > > >>> +            * \todo If the the sequence number of the context to initialise
> > > > > >>> +            * is smaller than the sequence number of the queue slot to use,
> > > > > >>> +            * it means that we had a serious request underrun and more
> > > > > >>> +            * frames than the queue size has been produced since the last
> > > > > >>> +            * time the application has queued a request. Does this deserve
> > > > > >>> +            * an error condition ?
> > > > > >>
> > > > > >> I believe we should log under-runs conditions. Whether we log it as ERROR or
> > > > > >> WARN or DEBUG might be a candidate for discussion. It depends on how we
> > > > > >> classify such condition
> > > > > >>
> > > > > >> Are under-run requests expected to completed with non-fatal errors set? If
> > > > > > They indeed expect to be completed :)
> > > > >
> > > > >
> > > > > Ok. This was somehow missing from my mind-map about so many discussions
> > > > > and PFC document.
> > > > >
> > > > > Thanks for clarifying that underruns need to be supported.
> > > > >
> > > > > >
> > > > > >> yes,  I believe we set RequestError::Underrun here and return the
> > > > > >> initialised frameContext. But then how the Request will be completed ? As
> > > > > >> the frame number passed here is quite < the current ongoing capture sequence
> > > > > >> on sensor side. So, we might end up initialising the context but it's never
> > > > > >> .get() with this frame number, for e.g. while populating metadata in
> > > > > >> processStatsBuffer()? So we might require some special handling there to
> > > > > >> detect that a frameContext belongs to a under-run request or not.
> > > > > >>
> > > > > >> I think I need to map out how will a under-run request will get completed
> > > > > >> successfully - IFF we are supporting that case.
> > > > > >>
> > > > > > There are legit question, but to properly reply to them I think we
> > > > > > need to finalise the design of the per-frame-control model. In
> > > > > > example, for some devices with limited memory (Pi Zero, in example),
> > > > > > operating in underrun mode seems to be the default, and we have to
> > > > > > handle that case properly.
> > > > >
> > > > >
> > > > > Agreed.
> > > > >
> > > > > Do those discussions block merging of the series? I see you already have
> > > > > a \todo here, so maybe it is meant to be handled on top and we can merge
> > > > > this set?
> > > > >
> > > > > >
> > > > > >>> +            */
> > > > > >>> +           if (frame != 0 && frame <= frameContext.frame)
> > > > > >>> +                   LOG(FCQueue, Warning)
> > > > > >>> +                           << "Frame " << frame << " already initialised";
> > > > > >>> +           else
> > > > > >>> +                   initialise(frameContext, frame);
> > > > > >>> +
> > > > > >>> +           return frameContext;
> > > > > >>> +   }
> > > > > >>> +
> > > > > >>> +   FrameContext &get(uint32_t frame)
> > > > > >>> +   {
> > > > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > > > >>> +
> > > > > >>> +           /*
> > > > > >>> +            * If the IPA algorithms try to access a frame context slot which
> > > > > >>> +            * has been already overwritten by a newer context, it means the
> > > > > >>> +            * frame context queue has overflowed and the desired context
> > > > > >>> +            * has been forever lost. The pipeline handler shall avoid
> > > > > >>> +            * queueing more requests to the IPA than the frame context
> > > > > >>> +            * queue size.
> > > > > >>> +            */
> > > > > >>> +           if (frame < frameContext.frame)
> > > > > >>> +                   LOG(FCQueue, Fatal)
> > > > > >>> +                           << "Frame " << frame << " has been overwritten";
> > > > > >>
> > > > > >> Maybe the log can be improved:
> > > > > >>
> > > > > >>      LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> > > > > >>                          << frameContext.frame;
> > > > > >>
> > > > > >>> +
> > > > > >>> +           if (frame == frameContext.frame)
> > > > > >>> +                   return frameContext;
> > > > > >>> +
> > > > > >>> +           LOG(FCQueue, Warning)
> > > > > >>> +                   << "Obtained an uninitialised FrameContext for " << frame;
> > > > > >>
> > > > > >> I think currently this Warning is being logged for both cases :
> > > > > >>
> > > > > >>      "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> > > > > > The
> > > > > >               if (frame < frameContext.frame)
> > > > > >                       LOG(FCQueue, Fatal)
> > > > > >                               << "Frame " << frame << " has been overwritten";
> > > > > >
> > > > > > will abort execution, so we don't get to this warning.
> > > > > >
> > > > > >> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> > > > > >> terminate the entire program?
> > > > > >>
> > > > > > That's the idea, yes.
> > > > >
> > > > >
> > > > > Got it!
> > > > >
> > > > > I don't see any other issues with the code here, other than the minors
> > > > > pointed out so,
> > > > >
> > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > >
> > > > > Not sure how would tags work with multiple authors in this case.. but
> > > > > anyways, we can figure it out on the way!
> > > > >
> > > > > >
> > > > > > The above Fatal is triggered by a programming error in the PH/IPA as
> > > > > > we should never get more requests queued than the FCQ size, so the
> > > > > > assertion above should not be triggered if not during development of
> > > > > > new platforms.
> > > > > >
> > > > > >>> +
> > > > > >>> +           initialise(frameContext, frame);
> > > > > >>> +
> > > > > >>> +           /*
> > > > > >>> +            * The frame context has been retrieved before it was
> > > > > >>> +            * initialised through the initialise() call. This indicates an
> > > > > >>> +            * algorithm attempted to access a Frame context before it was
> > > > > >>> +            * queued to the IPA.
> > > > > >>> +            *
> > > > > >>> +            * Controls applied for this request may be left unhandled.
> > > > > >>> +            */
> > > > > >>> +           frameContext.error |= Request::PFCError;
> > > > > >>> +
> > > > > >>> +           return frameContext;
> > > > > >>> +   }
> > > > > >>> +};
> > > > > >>> +
> > > > > >>> +} /* namespace ipa */
> > > > > >>> +
> > > > > >>> +} /* namespace libcamera */
> > > > > >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > > > >>> index fb894bc614af..016b8e0ec9be 100644
> > > > > >>> --- a/src/ipa/libipa/meson.build
> > > > > >>> +++ b/src/ipa/libipa/meson.build
> > > > > >>> @@ -3,6 +3,7 @@
> > > > > >>>    libipa_headers = files([
> > > > > >>>        'algorithm.h',
> > > > > >>>        'camera_sensor_helper.h',
> > > > > >>> +    'fc_queue.h',
> > > > > >>>        'histogram.h',
> > > > > >>>        'module.h',
> > > > > >>>    ])
> > > > > >>> @@ -10,6 +11,7 @@ libipa_headers = files([
> > > > > >>>    libipa_sources = files([
> > > > > >>>        'algorithm.cpp',
> > > > > >>>        'camera_sensor_helper.cpp',
> > > > > >>> +    'fc_queue.cpp',
> > > > > >>>        'histogram.cpp',
> > > > > >>>        'module.cpp',
> > > > > >>>    ])
> > > > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > >>> index 6cf4d1699ce5..af22dbeb3da5 100644
> > > > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > >>> @@ -49,7 +49,7 @@ public:
> > > > > >>>     int init(const IPASettings &settings, unsigned int hwRevision,
> > > > > >>>              ControlInfoMap *ipaControls) override;
> > > > > >>>     int start() override;
> > > > > >>> -   void stop() override {}
> > > > > >>> +   void stop() override;
> > > > > >>>     int configure(const IPACameraSensorInfo &info,
> > > > > >>>                   const std::map<uint32_t, IPAStream> &streamConfig,
> > > > > >>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
> > > > > >>>     return 0;
> > > > > >>>    }
> > > > > >>> +void IPARkISP1::stop()
> > > > > >>> +{
> > > > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > > > >>> +   context_ = {};
> > > >
> > > > Indeed - watch out here. I think this breaks start/stop/start cycles as
> > > > suddenly the session configuration is now zeroed.
> > > >
> > > > > >>> +}
> > > > > >>> +
> > > > > >>>    /**
> > > > > >>>     * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> > > > > >>>     * if the connected sensor does not provide enough information to properly
> > > > > >>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > > > >>>             << "Exposure: " << minExposure << "-" << maxExposure
> > > > > >>>             << " Gain: " << minGain << "-" << maxGain;
> > > > > >>> -   /* Clean context at configuration */
> > > > > >>> -   context_ = {};
> > > > > >>> -
> > > > > >>>     /* Set the hardware revision for the algorithms. */
> > > > > >>>     context_.configuration.hw.revision = hwRevision_;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Kieran,

On Wed, Aug 24, 2022 at 10:37:19AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-08-23 23:52:05)
> > On Tue, Aug 23, 2022 at 12:55:24PM +0100, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Jacopo Mondi (2022-08-18 08:44:55)
> > > > Hi Kieran
> > > > 
> > > >   thanks a lot for getting to the bottom of this.
> > > > 
> > > > I still can't reproduce it here, but your analysis seems correct to me
> > > > and indeed the context = {} might prove problematic.
> > > > 
> > > > See below
> > > > 
> > > > On Wed, Aug 17, 2022 at 09:33:08PM +0100, Kieran Bingham wrote:
> > > > > Quoting Umang Jain via libcamera-devel (2022-08-11 10:13:42)
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > Thank you for the clarifications,
> > > > > >
> > > > > > On 8/11/22 14:08, Jacopo Mondi wrote:
> > > > > > > Hi Umang
> > > > > > >
> > > > > > > On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> > > > > > >> Hi all,
> > > > > > >>
> > > > > > >> Few comments and some open question for discussion on underruns...
> > > > > > >>
> > > > > > >> On 8/5/22 19:23, Jacopo Mondi wrote:
> > > > > > >>> From: Umang Jain <umang.jain@ideasonboard.com>
> > > > > > >>>
> > > > > > >>> Introduce a common implementation in libipa to represent the queue of
> > > > > > >>> frame contexts.
> > > > > > >>>
> > > > > > >>> Adjust the IPU3 IPAFrameContext to provide the basic class members
> > > > > > >>> required to work with the generic FCQueue implementation, before
> > > > > > >>> introducing an IPAFrameContext class in the next patches.
> > > > > > >>>
> > > > > > >>> Opportunely handle cleaning up the queue and the IPA context at
> > > > > > >>> IPA::stop() time.
> > > > >
> > > > > This is bad. It breaks and causes the IPA to crash...
> > > > >
> > > > > The CTS test
> > > > > android.hardware.cts.CameraTest#testJpegCallbackStartPreview triggers
> > > > > this first...
> > > > >
> > > > > This may be the first test that has a configure ... start ... stop ...
> > > > > start sequence without a reconfigure. Which I think is where we break.
> > > > >
> > > > > Essentially - we can't just clear the entire context_ now. Certainly not
> > > > > between stop/start cycles.
> > > > >
> > > > > > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > >>> ---
> > > > > > >>>    src/ipa/ipu3/ipa_context.cpp |  20 +------
> > > > > > >>>    src/ipa/ipu3/ipa_context.h   |  16 ++---
> > > > > > >>>    src/ipa/ipu3/ipu3.cpp        |  18 +++---
> > > > > > >>>    src/ipa/libipa/fc_queue.cpp  | 112 +++++++++++++++++++++++++++++++++++
> > > > > > >>>    src/ipa/libipa/fc_queue.h    | 109 ++++++++++++++++++++++++++++++++++
> > > > > > >>>    src/ipa/libipa/meson.build   |   2 +
> > > > > > >>>    src/ipa/rkisp1/rkisp1.cpp    |  11 ++--
> > > > > > >>>    7 files changed, 250 insertions(+), 38 deletions(-)
> > > > > > >>>    create mode 100644 src/ipa/libipa/fc_queue.cpp
> > > > > > >>>    create mode 100644 src/ipa/libipa/fc_queue.h
> > > > > > >>>
> > > > > > >>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > > > > > >>> index 13cdb835ef7f..9605c8a1106d 100644
> > > > > > >>> --- a/src/ipa/ipu3/ipa_context.cpp
> > > > > > >>> +++ b/src/ipa/ipu3/ipa_context.cpp
> > > > > > >>> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> > > > > > >>>     * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> > > > > > >>>     */
> > > > > > >>> -/**
> > > > > > >>> - * \brief Default constructor for IPAFrameContext
> > > > > > >>> - */
> > > > > > >>> -IPAFrameContext::IPAFrameContext() = default;
> > > > > > >>> -
> > > > > > >>> -/**
> > > > > > >>> - * \brief Construct a IPAFrameContext instance
> > > > > > >>> - */
> > > > > > >>> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > > > > >>> -   : frame(id), frameControls(reqControls)
> > > > > > >>> -{
> > > > > > >>> -   sensor = {};
> > > > > > >>> -}
> > > > > > >>> -
> > > > > > >>>    /**
> > > > > > >>>     * \var IPAFrameContext::frame
> > > > > > >>>     * \brief The frame number
> > > > > > >>>     *
> > > > > > >>> - * \var IPAFrameContext::frameControls
> > > > > > >>> - * \brief Controls sent in by the application while queuing the request
> > > > > > >>> - *
> > > > > > >>>     * \var IPAFrameContext::sensor
> > > > > > >>>     * \brief Effective sensor values that were applied for the frame
> > > > > > >>>     *
> > > > > > >>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > > > > >>>     *
> > > > > > >>>     * \var IPAFrameContext::sensor.gain
> > > > > > >>>     * \brief Analogue gain multiplier
> > > > > > >>> + *
> > > > > > >>> + * \var IPAFrameContext::error
> > > > > > >>> + * \brief The error flags for this frame context
> > > > > > >>>     */
> > > > > > >>>    } /* namespace libcamera::ipa::ipu3 */
> > > > > > >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > > > > >>> index 42e11141d3a1..dc5b7c30aaf9 100644
> > > > > > >>> --- a/src/ipa/ipu3/ipa_context.h
> > > > > > >>> +++ b/src/ipa/ipu3/ipa_context.h
> > > > > > >>> @@ -8,22 +8,20 @@
> > > > > > >>>    #pragma once
> > > > > > >>> -#include <array>
> > > > > > >>> -
> > > > > > >>>    #include <linux/intel-ipu3.h>
> > > > > > >>>    #include <libcamera/base/utils.h>
> > > > > > >>>    #include <libcamera/controls.h>
> > > > > > >>>    #include <libcamera/geometry.h>
> > > > > > >>> +#include <libcamera/request.h>
> > > > > > >>> +
> > > > > > >>> +#include <libipa/fc_queue.h>
> > > > > > >>>    namespace libcamera {
> > > > > > >>>    namespace ipa::ipu3 {
> > > > > > >>> -/* Maximum number of frame contexts to be held */
> > > > > > >>> -static constexpr uint32_t kMaxFrameContexts = 16;
> > > > > > >>> -
> > > > > > >>>    struct IPASessionConfiguration {
> > > > > > >>>     struct {
> > > > > > >>>             ipu3_uapi_grid_config bdsGrid;
> > > > > > >>> @@ -77,23 +75,21 @@ struct IPAActiveState {
> > > > > > >>>    };
> > > > > > >>>    struct IPAFrameContext {
> > > > > > >>> -   IPAFrameContext();
> > > > > > >>> -   IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > > > > > >>> +   uint32_t frame;
> > > > > > >>>     struct {
> > > > > > >>>             uint32_t exposure;
> > > > > > >>>             double gain;
> > > > > > >>>     } sensor;
> > > > > > >>> -   uint32_t frame;
> > > > > > >>> -   ControlList frameControls;
> > > > > > >>> +   Request::Errors error;
> > > > > > >>>    };
> > > > > > >>>    struct IPAContext {
> > > > > > >>>     IPASessionConfiguration configuration;
> > > > > > >>>     IPAActiveState activeState;
> > > > > > >>> -   std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> > > > > > >>> +   FCQueue<IPAFrameContext> frameContexts;
> > > > > > >>>    };
> > > > > > >>>    } /* namespace ipa::ipu3 */
> > > > > > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > > > > >>> index 2f6bb672f7bb..209a6b040f8f 100644
> > > > > > >>> --- a/src/ipa/ipu3/ipu3.cpp
> > > > > > >>> +++ b/src/ipa/ipu3/ipu3.cpp
> > > > > > >>> @@ -38,6 +38,8 @@
> > > > > > >>>    #include "algorithms/tone_mapping.h"
> > > > > > >>>    #include "libipa/camera_sensor_helper.h"
> > > > > > >>> +#include "ipa_context.h"
> > > > > > >>> +
> > > > > > >>>    /* Minimum grid width, expressed as a number of cells */
> > > > > > >>>    static constexpr uint32_t kMinGridWidth = 16;
> > > > > > >>>    /* Maximum grid width, expressed as a number of cells */
> > > > > > >>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
> > > > > > >>>     */
> > > > > > >>>    void IPAIPU3::stop()
> > > > > > >>>    {
> > > > > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > > > > >>> +   context_.frameContexts.clear();
> > > > > > >>> +   context_ = {};
> > > > >
> > > > > I think this context_ = {}; is the line causing me trouble right now.
> > > > >
> > > > > The IPU3 IPA crashes with a divide by zero error in :
> > > > >
> > > > >  libcamera::ipa::ipu3::algorithms::Af::afEstimateVariance()
> > > > >
> > > > > where y_items.size() = 0, because Af::process() has set afRawBufferLen =
> > > > > 0 from
> > > > >
> > > > >       /* Evaluate the AF buffer length */
> > > > >       uint32_t afRawBufferLen = context.configuration.af.afGrid.width *
> > > > >                                 context.configuration.af.afGrid.height;
> > > > >
> > > > > So because context has been completely zeroed, and not reconfigured - we
> > > > > end up with a zero length span, and a div/0.
> > > > 
> > > > Ouch
> > > > 
> > > > Still doesn't make sense to me that here it doesn't fail..
> > > > 
> > > > >
> > > > > I expect simply removing this context_ = {}; may be sufficient. This may
> > > > > need to be considered for the RKISP too.
> > > > >
> > > > 
> > > > Ok, looking at the context in more detail, it indeed contains the
> > > > session configuration, which shall not be zeroed, but just overwritten
> > > > when a new configure() is issued.
> > > > 
> > > > context_ contains the active state as well, I'm still trying to figure
> > > > out if that one needs to be cleaned in order not to operate on
> > > > stale data when a new streaming session is started.
> > > > 
> > > > The frame context queue does indeed need to be cleared between
> > > > streaming session instead.
> > > > 
> > > > Overall we have a context_ which contains 3 items with different
> > > > lifetime management requirements.
> > > > 
> > > > I wonder if it's a good idea to store all of them together or it would
> > > > be easier to break the FCQ and the active state out from the
> > > > context_. I actually wonder if a struct context {}  makes any sense,
> > > > when the IPA class can have the three items (configuration, queue and
> > > > state) as three class members and handle their lifetime separately.
> > > > 
> > > > To be honest, the role of active state is still a little bit unclear
> > > > to me. It contains a global state of algorithm results without any
> > > > synchronization with the current frame. Re-reading Laurent's comment
> > > > on 05/10 of this series and your reply, it seems we agree the active
> > > > state shall be removed or reworked.
> > > 
> > > Potentially removed, but I think there's still scope for it. Things like
> > > the current modes of the algorithms might need to be there, it depends
> > > on if algorithms need to look at the modes of other algorithms.
> > 
> > They important word here is "current". If we want to keep an "active
> > state", I'll want a formal definition of "current".
> 
> Let me phrase it another way ... Active state will affect all future
> processing. *How algorithms process that is up to them* and we'll know
> more about that when we actually start trying to do it... which we are
> not yet doing.

I agree the best way to find out is to try it. This is what I'm doing at
the moment, let's see how it pans out.

> > > I know for instance, Kate would like to disable AWB during AF scanning.
> > > [0]
> 
> Like in this example for instance, if the AWB is disabled, or held, then
> without an active state (or an explicitly named call for every possible
> use case into each algorithm) then we have to modify *every* frame
> context in the queue, including future ones. *Until* the mode is changed
> back.
> 
> Feel free to define something now - but I think that can be handled when
> we get there, and we actually know what it will be rather than trying to
> 'guess' in advance.
> 
> > > I would expect this to happen through the active state, rather than the
> > > frame context.
> > > 
> > > https://patchwork.libcamera.org/project/libcamera/list/?series=3096
> > > 
> > > Overall I don't expect the ActiveState to be large, as algorithms could
> > > probably put that information in their own private storage. But maybe if
> > > it's stored as part of the overall context it provides a convenience.
> > > 
> > > In otherwords, I think it needs to be considered 'case by case' as we go
> > > through and move data into the FrameContext (and thus the fcq).
> > > 
> > > > For now I think it's then fine to remove the context = {}; from this
> > > > series, and then rework how the IPA context is modeled on top. Do you
> > > > all agree with this ?
> > > 
> > > Yes, I think as long as we don't clear the entire context structure it's
> > > fine.
> > > 
> > > > > Yes, I've removed this line, and the test doesn't fail now. I'll rerun
> > > > > the whole CTS suite.
> > > > >
> > > > > Same for RKISP below...
> > > > >
> > > > > > >>>    }
> > > > > > >>>    /**
> > > > > > >>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > > > > > >>>     calculateBdsGrid(configInfo.bdsOutputSize);
> > > > > > >>> -   /* Clean IPAActiveState at each reconfiguration. */
> > > > > > >>> -   context_.activeState = {};
> > > > > > >>> -   IPAFrameContext initFrameContext;
> > > > > > >>> -   context_.frameContexts.fill(initFrameContext);
> > > > > > >>> -
> > > > > > >>>     if (!validateSensorControls()) {
> > > > > > >>>             LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > > > > > >>>             return -EINVAL;
> > > > > > >>> @@ -569,7 +569,7 @@ 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[frame % kMaxFrameContexts];
> > > > > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > > > >>>     if (frameContext.frame != frame)
> > > > > > >>>             LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> > > > > > >>> @@ -618,7 +618,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > > > > >>>    void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > > > > > >>>    {
> > > > > > >>>     /* \todo Start processing for 'frame' based on 'controls'. */
> > > > > > >>> -   context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > > > > > >>> +   IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> > > > > > >>> +
> > > > > > >>> +   /* \todo Implement queueRequest to each algorithm. */
> > > > > > >>> +   (void)frameContext;
> > > > > > >>> +   (void)controls;
> > > > > > >>>    }
> > > > > > >>>    /**
> > > > > > >>> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > > > > >>> new file mode 100644
> > > > > > >>> index 000000000000..37ffca60b992
> > > > > > >>> --- /dev/null
> > > > > > >>> +++ b/src/ipa/libipa/fc_queue.cpp
> > > > > > >>> @@ -0,0 +1,112 @@
> > > > > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > >>> +/*
> > > > > > >>> + * Copyright (C) 2022, Google Inc.
> > > > > > >>> + *
> > > > > > >>> + * fc_queue.cpp - IPA Frame context queue
> > > > > > >>> + */
> > > > > > >>> +
> > > > > > >>> +#include "fc_queue.h"
> > > > > > >>> +
> > > > > > >>> +#include <libcamera/base/log.h>
> > > > > > >>> +
> > > > > > >>> +namespace libcamera {
> > > > > > >>> +
> > > > > > >>> +LOG_DEFINE_CATEGORY(FCQueue)
> > > > > > >>> +
> > > > > > >>> +namespace ipa {
> > > > > > >>> +
> > > > > > >>> +/**
> > > > > > >>> + * \file fc_queue.h
> > > > > > >>> + * \brief Queue to access per-frame Context
> > > > > > >>> + */
> > > > > > >>> +
> > > > > > >>> +/**
> > > > > > >>> + * \class FCQueue
> > > > > > >>> + * \brief A support class for queueing FrameContext instances through the IPA
> > > > > > >>> + * \tparam FrameContext The IPA specific FrameContext derived class type
> > > > > > >>> + *
> > > > > > >>> + * The frame context queue provides a simple circular buffer implementation to
> > > > > > >>> + * store IPA specific context for each frame through its lifetime within the
> > > > > > >>> + * IPA.
> > > > > > >>> + *
> > > > > > >>> + * FrameContext instances are expected to be referenced by a monotonically
> > > > > > >>> + * increasing sequence count corresponding to a frame sequence number.
> > > > > > >>> + *
> > > > > > >>> + * A FrameContext is initialised for a given frame when the corresponding
> > > > > > >>> + * Request is first queued into the IPA. From that point on the FrameContext can
> > > > > > >>> + * be obtained by the IPA and its algorithms by referencing it from the frame
> > > > > > >>> + * sequence number.
> > > > > > >>> + *
> > > > > > >>> + * A frame sequence number from the image source must correspond to the request
> > > > > > >>> + * sequence number for this implementation to be supported in an IPA. It is
> > > > > > >>> + * expected that the same sequence number will be used to reference the context
> > > > > > >>> + * of the Frame from the point of queueing the request, specifying controls for
> > > > > > >>
> > > > > > >> s/Frame/frame
> > > > > > >>
> > > > > > >>> + * a given frame, and processing of any ISP related controls and statistics for
> > > > > > >>> + * the same corresponding image.
> > > > > > >>> + *
> > > > > > >>> + * IPA specific frame context implementations shall inherit from the
> > > > > > >>> + * IPAFrameContext base class to support the minimum required features for a
> > > > > > >>> + * FrameContext, including the frame number and the error flags that relate to
> > > > > > >>> + * the frame.
> > > > > > >>> + *
> > > > > > >>> + * FrameContexts are overwritten when they are recycled and re-initialised by
> > > > > > >>> + * the first access made on them by either initialise(frame) or get(frame). It
> > > > > > >>> + * is required that the number of available slots in the frame context queue is
> > > > > > >>> + * larger or equal to the maximum number of in-flight requests a pipeline can
> > > > > > >>> + * handle to avoid overwriting frame context instances that still have to be
> > > > > > >>> + * processed.
> > > > > > >>> + *
> > > > > > >>> + * In the case an application does not queue requests to the Camera fast
> > > > > > >>
> > > > > > >> s/Camera/camera/
> > > > > > >>
> > > > > > >>> + * enough, frames can be produced and processed by the IPA without a
> > > > > > >>> + * corresponding Request being queued. In this case the IPA algorithm
> > > > > > >>> + * will try to access the FrameContext with a call to get() before it
> > > > > > >>> + * had been initialized at queueRequest() time. In this case there
> > > > > > >>
> > > > > > >> s/case/case,
> > > > > > >>
> > > > > > >>> + * is now way the controls associated with the late Request could be
> > > > > > >>
> > > > > > >> s/now/no
> > > > > > >>
> > > > > > > Ack to all the above ones, thanks
> > > > > > >
> > > > > > >>> + * applied in time, as the frame as already been processed by the IPA,
> > > > > > >>> + * the PFCError flag is automatically raised on the FrameContext.
> > > > > > >>> + */
> > > > > > >>> +
> > > > > > >>> +/**
> > > > > > >>> + * \fn FCQueue::clear()
> > > > > > >>> + * \brief Clear the context queue
> > > > > > >>> + *
> > > > > > >>> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> > > > > > >>> + * IPA modules are expected to clear the frame context queue at the beginning of
> > > > > > >>> + * a new streaming session, in IPAModule::start().
> > > > > > >>> + */
> > > > > > >>> +
> > > > > > >>> +/**
> > > > > > >>> + * \fn FCQueue::initialise(uint32_t frame)
> > > > > > >>> + * \brief Initialize and return the Frame Context at slot specified by \a frame
> > > > > > >>> + * \param[in] frame The frame context sequence number
> > > > > > >>> + *
> > > > > > >>> + * The first call to obtain a FrameContext from the FCQueue should be handled
> > > > > > >>> + * through this call. The FrameContext will be initialised for the frame and
> > > > > > >>> + * returned to the caller if it was not already initialised.
> > > > > > >>> + *
> > > > > > >>> + * If the FrameContext was already initialized for this sequence, a warning will
> > > > > > >>> + * be reported and the previously initialized FrameContext is returned.
> > > > > > >>> + *
> > > > > > >>> + * Frame contexts are expected to be initialised when a Request is first passed
> > > > > > >>> + * to the IPA module in IPAModule::queueRequest().
> > > > > > >>> + *
> > > > > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > > > > >>> + */
> > > > > > >>> +
> > > > > > >>> +/**
> > > > > > >>> + * \fn FCQueue::get()
> > > > > > >>> + * \brief Obtain the Frame Context at slot specified by \a frame
> > > > > > >>> + * \param[in] frame The frame context sequence number
> > > > > > >>> + *
> > > > > > >>> + * Obtains an existing FrameContext from the queue and returns it to the caller.
> > > > > > >>> + *
> > > > > > >>> + * If the FrameContext is not correctly initialised for the \a frame, it will be
> > > > > > >>> + * initialised and a PFCError will be flagged on the context to be transferred
> > > > > > >>> + * to the Request when it completes.
> > > > > > >>> + *
> > > > > > >>> + * \return A reference to the FrameContext for sequence \a frame
> > > > > > >>> + */
> > > > > > >>> +
> > > > > > >>> +} /* namespace ipa */
> > > > > > >>> +
> > > > > > >>> +} /* namespace libcamera */
> > > > > > >>> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > > > > > >>> new file mode 100644
> > > > > > >>> index 000000000000..023b52420fe7
> > > > > > >>> --- /dev/null
> > > > > > >>> +++ b/src/ipa/libipa/fc_queue.h
> > > > > > >>> @@ -0,0 +1,109 @@
> > > > > > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > >>> +/*
> > > > > > >>> + * Copyright (C) 2022, Google Inc.
> > > > > > >>> + *
> > > > > > >>> + * fc_queue.h - IPA Frame context queue
> > > > > > >>> + */
> > > > > > >>> +
> > > > > > >>> +#pragma once
> > > > > > >>> +
> > > > > > >>> +#include <array>
> > > > > > >>> +
> > > > > > >>> +#include <libcamera/base/log.h>
> > > > > > >>> +
> > > > > > >>> +#include <libcamera/request.h>
> > > > > > >>> +
> > > > > > >>> +namespace libcamera {
> > > > > > >>> +
> > > > > > >>> +LOG_DECLARE_CATEGORY(FCQueue)
> > > > > > >>> +
> > > > > > >>> +namespace ipa {
> > > > > > >>> +
> > > > > > >>> +/*
> > > > > > >>> + * Maximum number of frame contexts to be held onto
> > > > > > >>> + *
> > > > > > >>> + * \todo Should be platform-specific and match the pipeline depth
> > > > > > >>> + */
> > > > > > >>> +static constexpr uint32_t kMaxFrameContexts = 16;
> > > > > > >>> +
> > > > > > >>> +template<typename FrameContext>
> > > > > > >>> +class FCQueue : private std::array<FrameContext, kMaxFrameContexts>
> > > > > > >>> +{
> > > > > > >>> +private:
> > > > > > >>> +   void initialise(FrameContext &frameContext, const uint32_t frame)
> > > > > > >>> +   {
> > > > > > >>> +           frameContext = {};
> > > > > > >>> +           frameContext.frame = frame;
> > > > > > >>> +   }
> > > > > > >>> +
> > > > > > >>> +public:
> > > > > > >>> +   void clear()
> > > > > > >>> +   {
> > > > > > >>> +           this->fill({});
> > > > > > >>> +   }
> > > > > > >>> +
> > > > > > >>> +   FrameContext &initialise(const uint32_t frame)
> > > > > > >>> +   {
> > > > > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > > > > >>> +
> > > > > > >>> +           /*
> > > > > > >>> +            * Do not re-initialise if a get() call has already fetched this
> > > > > > >>> +            * frame context to preseve the error flags already raised.
> > > > > > >>> +            *
> > > > > > >>> +            * \todo If the the sequence number of the context to initialise
> > > > > > >>> +            * is smaller than the sequence number of the queue slot to use,
> > > > > > >>> +            * it means that we had a serious request underrun and more
> > > > > > >>> +            * frames than the queue size has been produced since the last
> > > > > > >>> +            * time the application has queued a request. Does this deserve
> > > > > > >>> +            * an error condition ?
> > > > > > >>
> > > > > > >> I believe we should log under-runs conditions. Whether we log it as ERROR or
> > > > > > >> WARN or DEBUG might be a candidate for discussion. It depends on how we
> > > > > > >> classify such condition
> > > > > > >>
> > > > > > >> Are under-run requests expected to completed with non-fatal errors set? If
> > > > > > > They indeed expect to be completed :)
> > > > > >
> > > > > >
> > > > > > Ok. This was somehow missing from my mind-map about so many discussions
> > > > > > and PFC document.
> > > > > >
> > > > > > Thanks for clarifying that underruns need to be supported.
> > > > > >
> > > > > > >
> > > > > > >> yes,  I believe we set RequestError::Underrun here and return the
> > > > > > >> initialised frameContext. But then how the Request will be completed ? As
> > > > > > >> the frame number passed here is quite < the current ongoing capture sequence
> > > > > > >> on sensor side. So, we might end up initialising the context but it's never
> > > > > > >> .get() with this frame number, for e.g. while populating metadata in
> > > > > > >> processStatsBuffer()? So we might require some special handling there to
> > > > > > >> detect that a frameContext belongs to a under-run request or not.
> > > > > > >>
> > > > > > >> I think I need to map out how will a under-run request will get completed
> > > > > > >> successfully - IFF we are supporting that case.
> > > > > > >>
> > > > > > > There are legit question, but to properly reply to them I think we
> > > > > > > need to finalise the design of the per-frame-control model. In
> > > > > > > example, for some devices with limited memory (Pi Zero, in example),
> > > > > > > operating in underrun mode seems to be the default, and we have to
> > > > > > > handle that case properly.
> > > > > >
> > > > > >
> > > > > > Agreed.
> > > > > >
> > > > > > Do those discussions block merging of the series? I see you already have
> > > > > > a \todo here, so maybe it is meant to be handled on top and we can merge
> > > > > > this set?
> > > > > >
> > > > > > >
> > > > > > >>> +            */
> > > > > > >>> +           if (frame != 0 && frame <= frameContext.frame)
> > > > > > >>> +                   LOG(FCQueue, Warning)
> > > > > > >>> +                           << "Frame " << frame << " already initialised";
> > > > > > >>> +           else
> > > > > > >>> +                   initialise(frameContext, frame);
> > > > > > >>> +
> > > > > > >>> +           return frameContext;
> > > > > > >>> +   }
> > > > > > >>> +
> > > > > > >>> +   FrameContext &get(uint32_t frame)
> > > > > > >>> +   {
> > > > > > >>> +           FrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> > > > > > >>> +
> > > > > > >>> +           /*
> > > > > > >>> +            * If the IPA algorithms try to access a frame context slot which
> > > > > > >>> +            * has been already overwritten by a newer context, it means the
> > > > > > >>> +            * frame context queue has overflowed and the desired context
> > > > > > >>> +            * has been forever lost. The pipeline handler shall avoid
> > > > > > >>> +            * queueing more requests to the IPA than the frame context
> > > > > > >>> +            * queue size.
> > > > > > >>> +            */
> > > > > > >>> +           if (frame < frameContext.frame)
> > > > > > >>> +                   LOG(FCQueue, Fatal)
> > > > > > >>> +                           << "Frame " << frame << " has been overwritten";
> > > > > > >>
> > > > > > >> Maybe the log can be improved:
> > > > > > >>
> > > > > > >>      LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> > > > > > >>                          << frameContext.frame;
> > > > > > >>
> > > > > > >>> +
> > > > > > >>> +           if (frame == frameContext.frame)
> > > > > > >>> +                   return frameContext;
> > > > > > >>> +
> > > > > > >>> +           LOG(FCQueue, Warning)
> > > > > > >>> +                   << "Obtained an uninitialised FrameContext for " << frame;
> > > > > > >>
> > > > > > >> I think currently this Warning is being logged for both cases :
> > > > > > >>
> > > > > > >>      "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> > > > > > > The
> > > > > > >               if (frame < frameContext.frame)
> > > > > > >                       LOG(FCQueue, Fatal)
> > > > > > >                               << "Frame " << frame << " has been overwritten";
> > > > > > >
> > > > > > > will abort execution, so we don't get to this warning.
> > > > > > >
> > > > > > >> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> > > > > > >> terminate the entire program?
> > > > > > >>
> > > > > > > That's the idea, yes.
> > > > > >
> > > > > >
> > > > > > Got it!
> > > > > >
> > > > > > I don't see any other issues with the code here, other than the minors
> > > > > > pointed out so,
> > > > > >
> > > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > >
> > > > > > Not sure how would tags work with multiple authors in this case.. but
> > > > > > anyways, we can figure it out on the way!
> > > > > >
> > > > > > >
> > > > > > > The above Fatal is triggered by a programming error in the PH/IPA as
> > > > > > > we should never get more requests queued than the FCQ size, so the
> > > > > > > assertion above should not be triggered if not during development of
> > > > > > > new platforms.
> > > > > > >
> > > > > > >>> +
> > > > > > >>> +           initialise(frameContext, frame);
> > > > > > >>> +
> > > > > > >>> +           /*
> > > > > > >>> +            * The frame context has been retrieved before it was
> > > > > > >>> +            * initialised through the initialise() call. This indicates an
> > > > > > >>> +            * algorithm attempted to access a Frame context before it was
> > > > > > >>> +            * queued to the IPA.
> > > > > > >>> +            *
> > > > > > >>> +            * Controls applied for this request may be left unhandled.
> > > > > > >>> +            */
> > > > > > >>> +           frameContext.error |= Request::PFCError;
> > > > > > >>> +
> > > > > > >>> +           return frameContext;
> > > > > > >>> +   }
> > > > > > >>> +};
> > > > > > >>> +
> > > > > > >>> +} /* namespace ipa */
> > > > > > >>> +
> > > > > > >>> +} /* namespace libcamera */
> > > > > > >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > > > > >>> index fb894bc614af..016b8e0ec9be 100644
> > > > > > >>> --- a/src/ipa/libipa/meson.build
> > > > > > >>> +++ b/src/ipa/libipa/meson.build
> > > > > > >>> @@ -3,6 +3,7 @@
> > > > > > >>>    libipa_headers = files([
> > > > > > >>>        'algorithm.h',
> > > > > > >>>        'camera_sensor_helper.h',
> > > > > > >>> +    'fc_queue.h',
> > > > > > >>>        'histogram.h',
> > > > > > >>>        'module.h',
> > > > > > >>>    ])
> > > > > > >>> @@ -10,6 +11,7 @@ libipa_headers = files([
> > > > > > >>>    libipa_sources = files([
> > > > > > >>>        'algorithm.cpp',
> > > > > > >>>        'camera_sensor_helper.cpp',
> > > > > > >>> +    'fc_queue.cpp',
> > > > > > >>>        'histogram.cpp',
> > > > > > >>>        'module.cpp',
> > > > > > >>>    ])
> > > > > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > >>> index 6cf4d1699ce5..af22dbeb3da5 100644
> > > > > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > >>> @@ -49,7 +49,7 @@ public:
> > > > > > >>>     int init(const IPASettings &settings, unsigned int hwRevision,
> > > > > > >>>              ControlInfoMap *ipaControls) override;
> > > > > > >>>     int start() override;
> > > > > > >>> -   void stop() override {}
> > > > > > >>> +   void stop() override;
> > > > > > >>>     int configure(const IPACameraSensorInfo &info,
> > > > > > >>>                   const std::map<uint32_t, IPAStream> &streamConfig,
> > > > > > >>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
> > > > > > >>>     return 0;
> > > > > > >>>    }
> > > > > > >>> +void IPARkISP1::stop()
> > > > > > >>> +{
> > > > > > >>> +   /* Clean the IPA context at the end of the streaming session. */
> > > > > > >>> +   context_ = {};
> > > > >
> > > > > Indeed - watch out here. I think this breaks start/stop/start cycles as
> > > > > suddenly the session configuration is now zeroed.
> > > > >
> > > > > > >>> +}
> > > > > > >>> +
> > > > > > >>>    /**
> > > > > > >>>     * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> > > > > > >>>     * if the connected sensor does not provide enough information to properly
> > > > > > >>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > > > > >>>             << "Exposure: " << minExposure << "-" << maxExposure
> > > > > > >>>             << " Gain: " << minGain << "-" << maxGain;
> > > > > > >>> -   /* Clean context at configuration */
> > > > > > >>> -   context_ = {};
> > > > > > >>> -
> > > > > > >>>     /* Set the hardware revision for the algorithms. */
> > > > > > >>>     context_.configuration.hw.revision = hwRevision_;