[libcamera-devel,v3,03/19] ipa: ipu3: Document the IPAIPU3 class

14269 diff mbox series

Clarify the roles and interactions between the pipeline handler events
and the algorithm calls by documenting all the remaining functions of
the class.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 10 deletions(-)

Hi Jean-Michel,

Thank you for the patch.

On Fri, Oct 22, 2021 at 05:12:02PM +0200, Jean-Michel Hautbois wrote:
> Clarify the roles and interactions between the pipeline handler events
> and the algorithm calls by documenting all the remaining functions of
> the class.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 73 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 6e28a5aa..c5461d8e 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -301,9 +301,9 @@ private:
>  	struct IPAContext context_;
>  };
>  
> -/*
> - * Compute IPASessionConfiguration using the sensor information and the sensor
> - * v4l2 controls.
> +/**
> + * \brief Compute IPASessionConfiguration using the sensor information and the
> + * sensor v4l2 controls

s/v4l2/V4L2/

>   */
>  void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
>  					 const ControlInfoMap &sensorControls)
> @@ -332,9 +332,9 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
>  	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
>  }
>  
> -/*
> - * Compute camera controls using the sensor information and the sensor
> - * v4l2 controls.
> +/**
> + * \brief Compute camera controls using the sensor information and the sensor
> + * v4l2 controls

Ditto.

>   *
>   * Some of the camera controls are computed by the pipeline handler, some others
>   * by the IPA module which is in charge of handling, for example, the exposure
> @@ -395,7 +395,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>  }
>  
>  /**
> - * Initialize the IPA module and its controls.
> + * \brief Initialize the IPA module and its controls
>   *
>   * This function receives the camera sensor information from the pipeline
>   * handler, computes the limits of the controls it handles and returns
> @@ -426,8 +426,15 @@ int IPAIPU3::init(const IPASettings &settings,
>  	return 0;
>  }
>  
> +/**
> + * \brief Perform any processing required before the first frame
> + */
>  int IPAIPU3::start()
>  {
> +	/*
> +	 * Set the sensors V4L2 controls before the first frame to ensure that
> +	 * we have an expected and known configuration from the start.
> +	 */
>  	setControls(0);
>  
>  	return 0;
> @@ -581,6 +588,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  	return 0;
>  }
>  
> +/**
> + * \brief Map the parameters and stats buffers allocated in the Pipeline Handler
> + * \param[in] buffers The buffers to map
> + */
>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>  {
>  	for (const IPABuffer &buffer : buffers) {
> @@ -590,6 +601,10 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>  	}
>  }
>  
> +/**
> + * \brief Unmap the parameters and stats buffers
> + * \param[in] ids The IDs of the buffers to unmap
> + */
>  void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>  {
>  	for (unsigned int id : ids) {
> @@ -601,6 +616,10 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>  
> +/**
> + * \brief Process an event generated by the Pipeline Handler
> + * \param[in] event The event sent from Pipeline Handler
> + */
>  void IPAIPU3::processEvent(const IPU3Event &event)
>  {
>  	switch (event.op) {
> @@ -642,12 +661,26 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>  	}
>  }
>  
> +/**
> + * \brief Process a control list for a request from the application
> + *
> + * Parse the request to handle any IPA managed controls that were set from the

My comments on v1 for the next 4 functions seem to have been ignored.
I'll review v4.

> + * application such as manual sensor settings.
> + */
>  void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  			      [[maybe_unused]] const ControlList &controls)
>  {
>  	/* \todo Start processing for 'frame' based on 'controls'. */
>  }
>  
> +/**
> + * \brief Fill the ImgU parameter buffer for the next frame
> + * \param[in] frame The current frame number
> + * \param[inout] params the parameter buffer to update
> + *
> + * Algorithms are expected to fill the IPU3 parameter buffer for the next
> + * frame given their most recent processing of the ImgU statistics.
> + */
>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  {
>  	/*
> @@ -670,6 +703,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  	queueFrameAction.emit(frame, op);
>  }
>  
> +/**
> + * \brief Process the statistics generated by the ImgU
> + * \param[in] frame The current frame number
> + * \param[in] frameTimestamp The current frame timestamp
> + * \param[in] stats The IPU3 statistics and ISP results
> + *
> + * Parse the most recently processed image statistics from the ImgU. The
> + * statistics are passed to each algorithm module to run their calculations and
> + * update their state accordingly.
> + */
>  void IPAIPU3::parseStatistics(unsigned int frame,
>  			      [[maybe_unused]] int64_t frameTimestamp,
>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> @@ -693,6 +736,13 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  	queueFrameAction.emit(frame, op);
>  }
>  
> +/**
> + * \brief Handle V4L2 controls for a given \a frame number
> + * \param[in] frame The frame on which the V4L2 controls should be set
> + *
> + * Send the desired sensor control values to the Pipeline handler to request
> + * that they are applied on the Camera sensor.
> + */
>  void IPAIPU3::setControls(unsigned int frame)
>  {
>  	IPU3Action op;
> @@ -711,10 +761,15 @@ void IPAIPU3::setControls(unsigned int frame)
>  
>  } /* namespace ipa::ipu3 */
>  
> -/*
> - * External IPA module interface
> +/**
> + * \brief External IPA module interface
> + *
> + * The IPAModuleInfo is required to match an IPA module construction against the
> + * intented pipeline handler with the module. The API and Pipeline handler
> + * versions must match the corresponding IPA interface and pipeline handler.
> + *
> + * \sa struct IPAModuleInfo
>   */
> -
>  extern "C" {
>  const struct IPAModuleInfo ipaModuleInfo = {
>  	IPA_MODULE_API_VERSION,
> @@ -723,6 +778,14 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	"ipu3",
>  };
>  
> +/**
> + * \brief Create an instance of the IPA interface
> + *
> + * This function is the entry point of the IPA module. It is called by the IPA
> + * manager to create an instance of the IPA interface for each camera. When
> + * matched against with a pipeline handler, the IPAManager will construct an IPA
> + * instance for each associated Camera.
> + */
>  IPAInterface *ipaCreate()
>  {
>  	return new ipa::ipu3::IPAIPU3();

Hi Laurent,

On 25/10/2021 22:05, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Fri, Oct 22, 2021 at 05:12:02PM +0200, Jean-Michel Hautbois wrote:
>> Clarify the roles and interactions between the pipeline handler events
>> and the algorithm calls by documenting all the remaining functions of
>> the class.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 73 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 6e28a5aa..c5461d8e 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -301,9 +301,9 @@ private:
>>   	struct IPAContext context_;
>>   };
>>   
>> -/*
>> - * Compute IPASessionConfiguration using the sensor information and the sensor
>> - * v4l2 controls.
>> +/**
>> + * \brief Compute IPASessionConfiguration using the sensor information and the
>> + * sensor v4l2 controls
> 
> s/v4l2/V4L2/
> 
>>    */
>>   void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
>>   					 const ControlInfoMap &sensorControls)
>> @@ -332,9 +332,9 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
>>   	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
>>   }
>>   
>> -/*
>> - * Compute camera controls using the sensor information and the sensor
>> - * v4l2 controls.
>> +/**
>> + * \brief Compute camera controls using the sensor information and the sensor
>> + * v4l2 controls
> 
> Ditto.
> 
>>    *
>>    * Some of the camera controls are computed by the pipeline handler, some others
>>    * by the IPA module which is in charge of handling, for example, the exposure
>> @@ -395,7 +395,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>>   }
>>   
>>   /**
>> - * Initialize the IPA module and its controls.
>> + * \brief Initialize the IPA module and its controls
>>    *
>>    * This function receives the camera sensor information from the pipeline
>>    * handler, computes the limits of the controls it handles and returns
>> @@ -426,8 +426,15 @@ int IPAIPU3::init(const IPASettings &settings,
>>   	return 0;
>>   }
>>   
>> +/**
>> + * \brief Perform any processing required before the first frame
>> + */
>>   int IPAIPU3::start()
>>   {
>> +	/*
>> +	 * Set the sensors V4L2 controls before the first frame to ensure that
>> +	 * we have an expected and known configuration from the start.
>> +	 */
>>   	setControls(0);
>>   
>>   	return 0;
>> @@ -581,6 +588,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>   	return 0;
>>   }
>>   
>> +/**
>> + * \brief Map the parameters and stats buffers allocated in the Pipeline Handler
>> + * \param[in] buffers The buffers to map
>> + */
>>   void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>>   {
>>   	for (const IPABuffer &buffer : buffers) {
>> @@ -590,6 +601,10 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>>   	}
>>   }
>>   
>> +/**
>> + * \brief Unmap the parameters and stats buffers
>> + * \param[in] ids The IDs of the buffers to unmap
>> + */
>>   void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>>   {
>>   	for (unsigned int id : ids) {
>> @@ -601,6 +616,10 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>>   	}
>>   }
>>   
>> +/**
>> + * \brief Process an event generated by the Pipeline Handler
>> + * \param[in] event The event sent from Pipeline Handler
>> + */
>>   void IPAIPU3::processEvent(const IPU3Event &event)
>>   {
>>   	switch (event.op) {
>> @@ -642,12 +661,26 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>>   	}
>>   }
>>   
>> +/**
>> + * \brief Process a control list for a request from the application
>> + *
>> + * Parse the request to handle any IPA managed controls that were set from the
> 
> My comments on v1 for the next 4 functions seem to have been ignored.
> I'll review v4.

quoting you on v1 (sorry to have missed it):
"Is it the current frame number or the next frame number ? We'll 
probably have to document "current" and "next" at some point, when we'll 
match per-frame controls with the corresponding frame."

It is the next frame that will be received on cio2 side, and fillParams 
and parseStatistics will have the current 'output' frame number.

The logs look like:

[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 0
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 1
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 2
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 3
[11818] DEBUG IPAIPU3 ipu3.cpp:573 fillParams: frame 0
[11818] DEBUG IPAIPU3 ipu3.cpp:608 parseStatistics: frame 0
[11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 4
[11818] DEBUG IPAIPU3 ipu3.cpp:573 fillParams: frame 1
[11818] DEBUG IPAIPU3 ipu3.cpp:608 parseStatistics: frame 1
etc.

This is where the IPAFrameContext could be useful probably if it was a 
queue and not the latest frame only, as we could have the 
delayedControls received for frame 3 when we are processing frame 3. 
This is where I am right now with per-frame controls btw, I can't see 
another way to do it correctly :-).

Which means that for this function, frame is the next frame to be 
processed, for parseStatistics it is the latest frame processed, and for 
fillParams... Well technically it is also the latest frame processed but 
it applies to the next frame... ?

> 
>> + * application such as manual sensor settings.
>> + */
>>   void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>>   			      [[maybe_unused]] const ControlList &controls)
>>   {
>>   	/* \todo Start processing for 'frame' based on 'controls'. */
>>   }
>>   
>> +/**
>> + * \brief Fill the ImgU parameter buffer for the next frame
>> + * \param[in] frame The current frame number
>> + * \param[inout] params the parameter buffer to update
>> + *
>> + * Algorithms are expected to fill the IPU3 parameter buffer for the next
>> + * frame given their most recent processing of the ImgU statistics.
>> + */
>>   void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>   {
>>   	/*
>> @@ -670,6 +703,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>   	queueFrameAction.emit(frame, op);
>>   }
>>   
>> +/**
>> + * \brief Process the statistics generated by the ImgU
>> + * \param[in] frame The current frame number
>> + * \param[in] frameTimestamp The current frame timestamp
>> + * \param[in] stats The IPU3 statistics and ISP results
>> + *
>> + * Parse the most recently processed image statistics from the ImgU. The
>> + * statistics are passed to each algorithm module to run their calculations and
>> + * update their state accordingly.
>> + */
>>   void IPAIPU3::parseStatistics(unsigned int frame,
>>   			      [[maybe_unused]] int64_t frameTimestamp,
>>   			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>> @@ -693,6 +736,13 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>   	queueFrameAction.emit(frame, op);
>>   }
>>   
>> +/**
>> + * \brief Handle V4L2 controls for a given \a frame number
>> + * \param[in] frame The frame on which the V4L2 controls should be set
>> + *
>> + * Send the desired sensor control values to the Pipeline handler to request
>> + * that they are applied on the Camera sensor.
>> + */
>>   void IPAIPU3::setControls(unsigned int frame)
>>   {
>>   	IPU3Action op;
>> @@ -711,10 +761,15 @@ void IPAIPU3::setControls(unsigned int frame)
>>   
>>   } /* namespace ipa::ipu3 */
>>   
>> -/*
>> - * External IPA module interface
>> +/**
>> + * \brief External IPA module interface
>> + *
>> + * The IPAModuleInfo is required to match an IPA module construction against the
>> + * intented pipeline handler with the module. The API and Pipeline handler
>> + * versions must match the corresponding IPA interface and pipeline handler.
>> + *
>> + * \sa struct IPAModuleInfo
>>    */
>> -
>>   extern "C" {
>>   const struct IPAModuleInfo ipaModuleInfo = {
>>   	IPA_MODULE_API_VERSION,
>> @@ -723,6 +778,14 @@ const struct IPAModuleInfo ipaModuleInfo = {
>>   	"ipu3",
>>   };
>>   
>> +/**
>> + * \brief Create an instance of the IPA interface
>> + *
>> + * This function is the entry point of the IPA module. It is called by the IPA
>> + * manager to create an instance of the IPA interface for each camera. When
>> + * matched against with a pipeline handler, the IPAManager will construct an IPA
>> + * instance for each associated Camera.
>> + */
>>   IPAInterface *ipaCreate()
>>   {
>>   	return new ipa::ipu3::IPAIPU3();
>

Hi Jean-Michel,

On Tue, Oct 26, 2021 at 07:49:19AM +0200, Jean-Michel Hautbois wrote:
> On 25/10/2021 22:05, Laurent Pinchart wrote:
> > On Fri, Oct 22, 2021 at 05:12:02PM +0200, Jean-Michel Hautbois wrote:
> >> Clarify the roles and interactions between the pipeline handler events
> >> and the algorithm calls by documenting all the remaining functions of
> >> the class.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>   src/ipa/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++++++++++++++------
> >>   1 file changed, 73 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index 6e28a5aa..c5461d8e 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -301,9 +301,9 @@ private:
> >>   	struct IPAContext context_;
> >>   };
> >>   
> >> -/*
> >> - * Compute IPASessionConfiguration using the sensor information and the sensor
> >> - * v4l2 controls.
> >> +/**
> >> + * \brief Compute IPASessionConfiguration using the sensor information and the
> >> + * sensor v4l2 controls
> > 
> > s/v4l2/V4L2/
> > 
> >>    */
> >>   void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
> >>   					 const ControlInfoMap &sensorControls)
> >> @@ -332,9 +332,9 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,
> >>   	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> >>   }
> >>   
> >> -/*
> >> - * Compute camera controls using the sensor information and the sensor
> >> - * v4l2 controls.
> >> +/**
> >> + * \brief Compute camera controls using the sensor information and the sensor
> >> + * v4l2 controls
> > 
> > Ditto.
> > 
> >>    *
> >>    * Some of the camera controls are computed by the pipeline handler, some others
> >>    * by the IPA module which is in charge of handling, for example, the exposure
> >> @@ -395,7 +395,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> >>   }
> >>   
> >>   /**
> >> - * Initialize the IPA module and its controls.
> >> + * \brief Initialize the IPA module and its controls
> >>    *
> >>    * This function receives the camera sensor information from the pipeline
> >>    * handler, computes the limits of the controls it handles and returns
> >> @@ -426,8 +426,15 @@ int IPAIPU3::init(const IPASettings &settings,
> >>   	return 0;
> >>   }
> >>   
> >> +/**
> >> + * \brief Perform any processing required before the first frame
> >> + */
> >>   int IPAIPU3::start()
> >>   {
> >> +	/*
> >> +	 * Set the sensors V4L2 controls before the first frame to ensure that
> >> +	 * we have an expected and known configuration from the start.
> >> +	 */
> >>   	setControls(0);
> >>   
> >>   	return 0;
> >> @@ -581,6 +588,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >>   	return 0;
> >>   }
> >>   
> >> +/**
> >> + * \brief Map the parameters and stats buffers allocated in the Pipeline Handler
> >> + * \param[in] buffers The buffers to map
> >> + */
> >>   void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> >>   {
> >>   	for (const IPABuffer &buffer : buffers) {
> >> @@ -590,6 +601,10 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> >>   	}
> >>   }
> >>   
> >> +/**
> >> + * \brief Unmap the parameters and stats buffers
> >> + * \param[in] ids The IDs of the buffers to unmap
> >> + */
> >>   void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> >>   {
> >>   	for (unsigned int id : ids) {
> >> @@ -601,6 +616,10 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> >>   	}
> >>   }
> >>   
> >> +/**
> >> + * \brief Process an event generated by the Pipeline Handler
> >> + * \param[in] event The event sent from Pipeline Handler
> >> + */
> >>   void IPAIPU3::processEvent(const IPU3Event &event)
> >>   {
> >>   	switch (event.op) {
> >> @@ -642,12 +661,26 @@ void IPAIPU3::processEvent(const IPU3Event &event)
> >>   	}
> >>   }
> >>   
> >> +/**
> >> + * \brief Process a control list for a request from the application
> >> + *
> >> + * Parse the request to handle any IPA managed controls that were set from the
> > 
> > My comments on v1 for the next 4 functions seem to have been ignored.
> > I'll review v4.
> 
> quoting you on v1 (sorry to have missed it):
> "Is it the current frame number or the next frame number ? We'll 
> probably have to document "current" and "next" at some point, when we'll 
> match per-frame controls with the corresponding frame."
> 
> It is the next frame that will be received on cio2 side, and fillParams 
> and parseStatistics will have the current 'output' frame number.
> 
> The logs look like:
> 
> [11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 0
> [11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 1
> [11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 2
> [11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 3
> [11818] DEBUG IPAIPU3 ipu3.cpp:573 fillParams: frame 0
> [11818] DEBUG IPAIPU3 ipu3.cpp:608 parseStatistics: frame 0
> [11818] DEBUG IPAIPU3 ipu3.cpp:560 processControls: frame 4
> [11818] DEBUG IPAIPU3 ipu3.cpp:573 fillParams: frame 1
> [11818] DEBUG IPAIPU3 ipu3.cpp:608 parseStatistics: frame 1
> etc.
> 
> This is where the IPAFrameContext could be useful probably if it was a 
> queue and not the latest frame only, as we could have the 
> delayedControls received for frame 3 when we are processing frame 3. 
> This is where I am right now with per-frame controls btw, I can't see 
> another way to do it correctly :-).

Moving the frame context to a queue sounds good to me.

> Which means that for this function, frame is the next frame to be 
> processed, for parseStatistics it is the latest frame processed, and for 
> fillParams... Well technically it is also the latest frame processed but 
> it applies to the next frame... ?

OK, thanks for the clarification.

BTW, it seems you're missing the parameters documentation for this
function (I can't recall if they're added in a patch later in the
series).

> >> + * application such as manual sensor settings.
> >> + */
> >>   void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
> >>   			      [[maybe_unused]] const ControlList &controls)
> >>   {
> >>   	/* \todo Start processing for 'frame' based on 'controls'. */
> >>   }
> >>   
> >> +/**
> >> + * \brief Fill the ImgU parameter buffer for the next frame
> >> + * \param[in] frame The current frame number
> >> + * \param[inout] params the parameter buffer to update
> >> + *
> >> + * Algorithms are expected to fill the IPU3 parameter buffer for the next
> >> + * frame given their most recent processing of the ImgU statistics.
> >> + */
> >>   void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >>   {
> >>   	/*
> >> @@ -670,6 +703,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >>   	queueFrameAction.emit(frame, op);
> >>   }
> >>   
> >> +/**
> >> + * \brief Process the statistics generated by the ImgU
> >> + * \param[in] frame The current frame number
> >> + * \param[in] frameTimestamp The current frame timestamp
> >> + * \param[in] stats The IPU3 statistics and ISP results
> >> + *
> >> + * Parse the most recently processed image statistics from the ImgU. The
> >> + * statistics are passed to each algorithm module to run their calculations and
> >> + * update their state accordingly.
> >> + */
> >>   void IPAIPU3::parseStatistics(unsigned int frame,
> >>   			      [[maybe_unused]] int64_t frameTimestamp,
> >>   			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> >> @@ -693,6 +736,13 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >>   	queueFrameAction.emit(frame, op);
> >>   }
> >>   
> >> +/**
> >> + * \brief Handle V4L2 controls for a given \a frame number
> >> + * \param[in] frame The frame on which the V4L2 controls should be set
> >> + *
> >> + * Send the desired sensor control values to the Pipeline handler to request
> >> + * that they are applied on the Camera sensor.
> >> + */
> >>   void IPAIPU3::setControls(unsigned int frame)
> >>   {
> >>   	IPU3Action op;
> >> @@ -711,10 +761,15 @@ void IPAIPU3::setControls(unsigned int frame)
> >>   
> >>   } /* namespace ipa::ipu3 */
> >>   
> >> -/*
> >> - * External IPA module interface
> >> +/**
> >> + * \brief External IPA module interface
> >> + *
> >> + * The IPAModuleInfo is required to match an IPA module construction against the
> >> + * intented pipeline handler with the module. The API and Pipeline handler
> >> + * versions must match the corresponding IPA interface and pipeline handler.
> >> + *
> >> + * \sa struct IPAModuleInfo
> >>    */
> >> -
> >>   extern "C" {
> >>   const struct IPAModuleInfo ipaModuleInfo = {
> >>   	IPA_MODULE_API_VERSION,
> >> @@ -723,6 +778,14 @@ const struct IPAModuleInfo ipaModuleInfo = {
> >>   	"ipu3",
> >>   };
> >>   
> >> +/**
> >> + * \brief Create an instance of the IPA interface
> >> + *
> >> + * This function is the entry point of the IPA module. It is called by the IPA
> >> + * manager to create an instance of the IPA interface for each camera. When
> >> + * matched against with a pipeline handler, the IPAManager will construct an IPA
> >> + * instance for each associated Camera.
> >> + */
> >>   IPAInterface *ipaCreate()
> >>   {
> >>   	return new ipa::ipu3::IPAIPU3();