[v1,19/35] libipa: algorithm: Update docs

24785 diff mbox series

Update the algorithm documentation to reflect the changed timing model.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Hi Stefan

On Fri, Oct 24, 2025 at 10:50:43AM +0200, Stefan Klug wrote:
> Update the algorithm documentation to reflect the changed timing model.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp
> index 201efdfdba25..44b6bdfa6972 100644
> --- a/src/ipa/libipa/algorithm.cpp
> +++ b/src/ipa/libipa/algorithm.cpp
> @@ -76,11 +76,12 @@ namespace ipa {
>   *
>   * This function is called for each request queued to the camera. It provides
>   * the controls stored in the request to the algorithm. The \a frame number
> - * is the Request sequence number and identifies the desired corresponding
> + * is the sensor sequence number and identifies the desired corresponding

I wonder if the frame/request sequence number is even meaningful for
algorithsm. After all, this function should mostly pass to the
algorithms the control to use to change the algorithm's state.

The only usage I see of frame in algos' queueRequest is to find out if
this is the first call (frame == 0) and the ISP block needs to be
fully configured or not (something which might be handled with a
flag set/reset at configure() time ?)

>   * frame to target for the controls to take effect.
>   *
>   * Algorithms shall read the applicable controls and store their value for later
> - * use during frame processing.
> + * use during frame processing. All values that are already known shall be
> + * updated in \a frameContext.

I'm not sure I get what you mean.

Is this to suggest that the IPA that has to accummulate controls which
have arrived too late ?

>   */
>
>  /**
> @@ -98,14 +99,18 @@ namespace ipa {
>   * Algorithms shall fill in the parameter structure fields appropriately to
>   * configure the ISP processing blocks that they are responsible for. This
>   * includes setting fields and flags that enable those processing blocks.
> + *
> + * Additionally \a frameContext shall be updated with the most up to date values
> + * from active state.

I think this needs to be expanded a little.

I interpret this as the frame context shall be kept up-to-date with
the values that are effectivelly set in the parameters buffer, right ?

Is the idea to move the values from the active state where they have
been populated by process() to the frame context at preprare() time,
so that the frame context is only updated when the parameters buffer is
programmed ?

I would keep this note as part of the previous paragraph, and take the
occasion to modify it slightly.

 * Algorithms shall fill in the \a params buffer appropriately to
 * configure the ISP processing blocks that they are responsible for.
 * This includes setting fields and flags that enable those processing
 * blocks. The values used to populate \a params should be used to
 * update \a frameContext so that it reflects the ISP configuration
 * used to produce \a frame.
 */

Take in whatever you think is approprate from this


>   */
>
>  /**
>   * \fn Algorithm::process()
>   * \brief Process ISP statistics, and run algorithm operations
>   * \param[in] context The shared IPA context
> - * \param[in] frame The frame context sequence number
> - * \param[in] frameContext The current frame's context
> + * \param[in] frame The frame sequence number that produces the stats

for which stats have been produced

?

> + * \param[in] frameContext The frame context for the frame that produced the
> + * stats

same here

>   * \param[in] stats The IPA statistics and ISP results
>   * \param[out] metadata Metadata for the frame, to be filled by the algorithm
>   *
> @@ -118,19 +123,14 @@ namespace ipa {
>   * computationally expensive calculations or operations must be handled
>   * asynchronously in a separate thread.
>   *
> - * Algorithms can store state in their respective IPAFrameContext structures,
> - * and reference state from the IPAFrameContext of other algorithms.
> - *
> - * \todo Historical data may be required as part of the processing.
> - * Either the previous frame, or the IPAFrameContext state of the frame
> - * that generated the statistics for this operation may be required for
> - * some advanced algorithms to prevent oscillations or support control
> - * loops correctly. Only a single IPAFrameContext is available currently,
> - * and so any data stored may represent the results of the previously
> - * completed operations.
> + * Algorithms shall update the active state. The frameContext shall *not* be
> + * updated as that frame was already produced.

I would rather modify the first paragraph otherwise it feels like this
has been added here to tell us not to do what we have been doing so
far.

The end result might be:

 * This function is called while camera is running for every frame
 * processed by the ISP, to process statistics generated from that
 * frame. Algorithms shall use this data to run calculations, update
 * the active state and fill the frame metadata  with the results of
 * the calculations.

>   *
>   * Care shall be taken to ensure the ordering of access to the information
>   * such that the algorithms use up to date state as required.
> + *
> + * The \a stats parameter can be null in which case ony the frame metadata shall

s/ony/only
s/frame metadata/ \a metadata /

> + * be filled.

s/filled/populated ?

>   */
>
>  /**
> --
> 2.48.1
>