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

[Umang Jain]

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 at 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 at ideasonboard.com>
>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>> Signed-off-by: Jacopo Mondi <jacopo at 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 at 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_;