[libcamera-devel] [PATCH v3 1/4] ipa: ipu3: Separate out frame context queue as a distinct class

[Umang Jain]

Hi Jacopo

On 6/20/22 15:14, Jacopo Mondi wrote:
> Hi Umang,
>
> On Mon, Jun 20, 2022 at 02:10:21AM +0530, Umang Jain via libcamera-devel wrote:
>> There are cases where we need more checks and balance to be carried out
>> by the frame context queue class. For that, separate it out as a
>> distinct class on which we can build upon.
>>
>> For now, a minimialistic implementation is provided with .get(frame)
>> helper which returns a IPAFrameContext pointer for the required frame.
>> Going ahead FCQueue::get() can be extended to access/modify the frame
>> context queue.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipa_context.cpp | 65 ++++++++++++++++++++++++++++++++++--
>>   src/ipa/ipu3/ipa_context.h   | 13 +++++++-
>>   src/ipa/ipu3/ipu3.cpp        | 18 +++++-----
>>   3 files changed, 84 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 13cdb835..36d20b1b 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -7,12 +7,18 @@
>>
>>   #include "ipa_context.h"
>>
>> +#include <libcamera/base/log.h>
>> +
>>   /**
>>    * \file ipa_context.h
>>    * \brief Context and state information shared between the algorithms
>>    */
>>
>> -namespace libcamera::ipa::ipu3 {
>> +namespace libcamera {
>> +
>> +LOG_DECLARE_CATEGORY(IPAIPU3)
>> +
>> +namespace ipa::ipu3 {
>>
>>   /**
>>    * \struct IPASessionConfiguration
>> @@ -211,4 +217,59 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>>    * \brief Analogue gain multiplier
>>    */
>>
>> -} /* namespace libcamera::ipa::ipu3 */
>> +/**
>> + * \class FCQueue
>> + * \brief A FIFO circular queue holding IPAFrameContext(s)
>> + *
>> + * FCQueue holds all the IPAFrameContext(s) related to frames required
>> + * to be processed by the IPA at a given point.
>> + */
>> +
>> +/**
>> + * \brief FCQueue constructor
>> + */
>> +FCQueue::FCQueue()
>> +{
>> +	reset();
>> +}
>> +
>> +/**
>> + * \brief Retrieve the IPAFrameContext for the frame
>> + * \param[in] frame Frame number for which the IPAFrameContext needs to
>> + * retrieved
>> + *
>> + * \return Pointer to the IPAFrameContext
>> + */
>> +IPAFrameContext *FCQueue::get(uint32_t frame)
>> +{
>> +	IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>> +
>> +	if (frame != frameContext.frame) {
>> +		LOG(IPAIPU3, Warning)
>> +			<< "Got wrong frame context for frame" << frame
>> +			<< " or frame context doesn't exist yet";
>> +	}
>> +
>> +	return &frameContext;
>> +}
>> +
>> +/**
>> + * \brief Sets the size of the FCQueue
>> + * \param[in] size The intended size of the FCQueue
>> + */
>> +void FCQueue::setSize(uint32_t size)
> Will the size be changed at run time or is it fixed ? Because if
> that's the case this could be made a compile-time parameter..


FCQueue should re-configuration at run time, in the sense, it can be 
re-configured to change it's size between start() > stop() > configure() 
 > stop() cycles.

That's what I have tried to address.

>
>> +{
>> +	resize(size);
>> +}
>> +
>> +/**
>> + * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
>> + */
>> +void FCQueue::reset()
>> +{
>> +	clear();
>> +}
> If reset() doesn't get augmented, I would rather use clear() directly.
> Even more so when you'll move inherit privately and decide which
> std::vector<> functions to expose.


Private inheritance is done in 2/4 and we extend reset() as well in 2/4 
and 3/4

Hence, it might look here we can use clear() but this is just a 
placeholder for later extension...

>
>> +
>> +} /* namespace ipa::ipu3 */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 42e11141..e487d34b 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -89,11 +89,22 @@ struct IPAFrameContext {
>>   	ControlList frameControls;
>>   };
>>
>> +class FCQueue : public std::vector<IPAFrameContext>
> Is there anything that prevents you from inheriting privately already?
>  From a quick look I don't see usages of the std::vector<> functions on
> frameContexts (I might have missed some though)


We use [] operator in IPAIPU3::queueRequest() hence we need to be public 
here

Private inheritance is introduced in 2/4 (mentioned in its commit message)

>
>> +{
>> +public:
>> +	FCQueue();
>> +
>> +	void reset();
>> +	void setSize(uint32_t size);
>> +
>> +	IPAFrameContext *get(uint32_t frame);
>> +};
>> +
>>   struct IPAContext {
>>   	IPASessionConfiguration configuration;
>>   	IPAActiveState activeState;
>>
>> -	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>> +	FCQueue frameContexts;
>>   };
>>
>>   } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 2f6bb672..d770c254 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -456,8 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>
>>   	/* Clean IPAActiveState at each reconfiguration. */
>>   	context_.activeState = {};
>> -	IPAFrameContext initFrameContext;
>> -	context_.frameContexts.fill(initFrameContext);
>> +	context_.frameContexts.reset();
>> +	context_.frameContexts.setSize(kMaxFrameContexts);
>>
>>   	if (!validateSensorControls()) {
>>   		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>> @@ -569,20 +569,20 @@ 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)
>> +	if (frameContext->frame != frame)
>>   		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
>>
>> -	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +	frameContext->sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +	frameContext->sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>>
>>   	double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>>   	int32_t vBlank = context_.configuration.sensor.defVBlank;
>>   	ControlList ctrls(controls::controls);
>>
>>   	for (auto const &algo : algorithms_)
>> -		algo->process(context_, &frameContext, stats);
>> +		algo->process(context_, frameContext, stats);
>>
>>   	setControls(frame);
>>
>> @@ -590,11 +590,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>   	int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
>>   	ctrls.set(controls::FrameDuration, frameDuration);
>>
>> -	ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>> +	ctrls.set(controls::AnalogueGain, frameContext->sensor.gain);
>>
>>   	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>>
>> -	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>> +	ctrls.set(controls::ExposureTime, frameContext->sensor.exposure * lineDuration);
>>
>>   	/*
>>   	 * \todo The Metadata provides a path to getting extended data
>> --
>> 2.31.1
>>