[libcamera-devel,04/11] ipa: ipu3: Document the IPAIPU3 class

13832 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 | 65 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 3 deletions(-)

Hi Jean-Michel,

Thank you for the patch.

On Mon, Sep 13, 2021 at 04:58:03PM +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 | 65 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index c0004ea6..df64b0f1 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -262,7 +262,7 @@ private:
>  };
>  
>  /**
> - * Initialize the IPA module and its controls.
> + * \brief Initialize the IPA module and its controls.

s/.$//

Same below.

>   *
>   * This function receives the camera sensor information from the pipeline
>   * handler, computes the limits of the controls it handles and returns
> @@ -337,8 +337,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;
> @@ -474,6 +481,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	return 0;
>  }
>  
> +/**
> + * \brief Map the parameters and stats buffers allocated in the Pipeline Handler
> + * \param[out] buffers A vector with param and stat buffers

Isn't it an in param ? I' also write "The vector containing all the
buffers to map", or just "The buffers to map" (I'm not a big fan of
stating the container type explicitly when it doesn't matter
particularly, as Doxygen shows types already).

> + */
>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>  {
>  	for (const IPABuffer &buffer : buffers) {
> @@ -483,6 +494,10 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>  	}
>  }
>  
> +/**
> + * \brief Unmap the parameters and stats buffers
> + * \param[in] ids A list of buffer ids

"The IDs of the buffers to unmap" ?

> + */
>  void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>  {
>  	for (unsigned int id : ids) {
> @@ -494,6 +509,10 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>  
> +/**
> + * \brief Process events generated by the Pipeline Handler

"Process an event" as the function processes a single event.

> + * \param[in] event An event sent from Pipeline Handler

s/An/The/

> + */
>  void IPAIPU3::processEvent(const IPU3Event &event)
>  {
>  	switch (event.op) {
> @@ -535,12 +554,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

s/IPA managed/IPA-managed/

> + * 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

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.

> + * \param[inout] params the parameter buffer to update

s/the/The/
s/update/fill/

Isn't it an out parameter ?

> + *
> + * 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)
>  {
>  	for (auto const &algo : algorithms_)
> @@ -552,6 +585,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)
> @@ -579,6 +622,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

s/V4L2/sensor/ in both lines.

> + *
> + * Send the desired sensor control values to the Pipeline handler to request

s/Pipeline/pipeline/

I would also write "pipeline handler" instead of "Pipeline Handler"
above.

> + * that they are applied on the Camera sensor.

s/Camera/camera/ (we don't need a link to the Camera class here :-))

> + */
>  void IPAIPU3::setControls(unsigned int frame)
>  {
>  	IPU3Action op;
> @@ -597,10 +647,15 @@ void IPAIPU3::setControls(unsigned int frame)
>  
>  } /* namespace ipa::ipu3 */
>  
> -/*
> +/**
>   * External IPA module interface

Missing "\brief".

> + *
> + * The IPAModuleInfo is required to match an IPA module construction against the
> + * intented pipeline handler with the module. The API and Pipeline handler

s/Pipeline/pipeline/

> + * versions must match the corresponding IPA interface and pipeline handler.
> + *
> + * \sa struct IPAModuleInfo
>   */
> -
>  extern "C" {
>  const struct IPAModuleInfo ipaModuleInfo = {
>  	IPA_MODULE_API_VERSION,
> @@ -609,6 +664,10 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	"ipu3",
>  };
>  
> +/**
> + * When matched against with a pipeline handler, the IPAManager will construct
> + * an IPA instance for each associated Camera.

Missing \brief too.

Should we document this from the point of view of the IPA module first,
and then mention the usage pattern ? I mean something along those lines:

 * \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.

> + */
>  IPAInterface *ipaCreate()
>  {
>  	return new ipa::ipu3::IPAIPU3();