[libcamera-devel] [PATCH 04/11] ipa: ipu3: Document the IPAIPU3 class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 14 05:37:31 CEST 2021
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 at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at 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();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list