[libcamera-devel] [PATCH v3 2/9] ipa: ipu3: Introduce a Context structure

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Wed, Aug 18, 2021 at 05:53:56PM +0200, Jean-Michel Hautbois wrote:
> An increasing amount of data and information needs to be shared between
> the components that build up to implement image processing algorithms.
> 
> Create a context structure which will allow us to work towards calling
> algorithms in a modular way, and sharing information between the modules.
> 
> The IPA context is a global context set at configure time (IPAConfiguration)
> and a per-frame context (IPAFrameContext) used while streaming.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.h | 53 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 src/ipa/ipu3/ipa_context.h
> 
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> new file mode 100644
> index 00000000..76534f7f
> --- /dev/null
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * ipu3_ipa_context.h - IPU3 IPA Context
> + *
> + */
> +#ifndef __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +#define __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3 {
> +

Normally documentation would go to .cpp files, but as this is private to
the IPU3, I'm OK relaxing the rule.

> +/**
> + * Fixed configuration of the IPA

Doxygen style, so this should be a \brief.

> + *
> + * This structure contains data set at the configure state of the IPA.
> + * It can't be modified while streaming.

"Fixed configuration" sounds like it's fixed, while I suspect this is a
session configuration that is set when from configure(), and can be
modified for the next session. If that's correct, How about naming the
structure IPASessionConfiguration to make this clear ? I may be biased
on the terminology, "session" probably comes from Android, and I'm open
to alternative terms.

As for the documentation, I'd then write it as follows

/**
 * \brief Session configuration for the IPA module
 *
 * The session configuration contains all IPA configuration parameters that
 * remain constant during the capture session, from IPA module start to stop.
 * It is typically set during the configure() operation of the IPA module, but
 * may also be updated in the start() operation.
 */

> + */
> +struct IPAConfiguration {
> +};
> +
> +/**
> + * Context of a frame for each algorithms
> + *
> + * This may be stored in a way that is associated with a given request
> + * lifetime, though for now a single instance is used.
> + * It contains informations updated while streaming by the algorithms.

Let's expand that, with how I understand the structure is meant to be
used today.

/**
 * \brief Per-frame context for algorithms
 *
 * The frame context stores data specific to a single frame processed by the
 * IPA. Each frame processed by the IPA has a context associated with it,
 * accessible through the IPAContext structure.
 *
 * \todo Detail how to access contexts for a particular frame
 *
 * Each of the fields in the frame context belongs to either a specific
 * algorithm, or to the top-level IPA module. A field may be read by any
 * algorithm, but only be written by its owner.
 */

> + */
> +struct IPAFrameContext {
> +};
> +
> +/**
> + * Context information shared between the algorithms
> + *
> + * The global context structure contains:
> + * - a configuration structure set while the IPA is configured, and not
> + * modified when streaming
> + * - a frameContext structure updated by the algorithms while streaming

This should use doxygen documentation for the fields.

> + */

/**
 * \brief Global IPA context data shared between all algorithms
 *
 * \var IPAContext::configuration
 * \brief The IPA session configuration, immutable during the session
 *
 * \var IPAContext::frameContext
 * \brief The frame context for the frame being processed
 * \todo While the frame context is supposed to be per-frame, this
 * single frame context stores data related to both the current frame
 * and the previous frames, with fields being updated as the algorithms
 * are run. This needs to be turned into real per-frame data storage.
 */

> +struct IPAContext {
> +	IPAConfiguration configuration;
> +	IPAFrameContext frameContext;
> +};
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera*/
> +
> +#endif /* __LIBCAMERA_IPU3_IPA_CONTEXT_H__ */

-- 
Regards,

Laurent Pinchart