[libcamera-devel,v3,15/19] ipa: ipu3: Isolate ipa_context documentation

14282 diff mbox series

The IPU3 IPA core is growing with additional documentation. The
ipa_context documentation is stored here, but it pushes the IPU3
documentation and implementation further from the head of the file.

Furthermore, the ipa_context documentation is outside of the ipa::ipu3
namespace and isn't identified correctly by Doxygen.

Move the ipa_context to its own compilation object even though there
isn't any code, but to maintain consistency with our documentation
model.

Correctly re-introduce the documentation into the libcamera::ipa::ipu3
namespace during the move.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/ipa_context.cpp | 136 +++++++++++++++++++++++++++++++++++
 src/ipa/ipu3/ipu3.cpp        | 124 --------------------------------
 src/ipa/ipu3/meson.build     |   1 +
 3 files changed, 137 insertions(+), 124 deletions(-)
 create mode 100644 src/ipa/ipu3/ipa_context.cpp

Quoting Jean-Michel Hautbois (2021-10-22 16:12:14)

This doesn't have a 'From' (me) line here, so I assume the commit author
has been set to you.


> The IPU3 IPA core is growing with additional documentation. The
> ipa_context documentation is stored here, but it pushes the IPU3
> documentation and implementation further from the head of the file.
> 
> Furthermore, the ipa_context documentation is outside of the ipa::ipu3
> namespace and isn't identified correctly by Doxygen.
> 
> Move the ipa_context to its own compilation object even though there
> isn't any code, but to maintain consistency with our documentation
> model.
> 
> Correctly re-introduce the documentation into the libcamera::ipa::ipu3
> namespace during the move.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

However, it only has my SoB ...  You'll need to add yours too ;-)

I think I'm reviewing a patch I wrote here, but it still looks correct
to me ;-) so ...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/ipa/ipu3/ipa_context.cpp | 136 +++++++++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3.cpp        | 124 --------------------------------
>  src/ipa/ipu3/meson.build     |   1 +
>  3 files changed, 137 insertions(+), 124 deletions(-)
>  create mode 100644 src/ipa/ipu3/ipa_context.cpp
> 
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> new file mode 100644
> index 00000000..3e154e60
> --- /dev/null
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * ipa_context.cpp - IPU3 IPA Context
> + */
> +
> +#include "ipa_context.h"
> +
> +namespace libcamera::ipa::ipu3 {
> +
> +/**
> + * \file ipa_context.h
> + * \brief Context and state information shared between the algorithms
> + */
> +
> +/**
> + * \struct IPASessionConfiguration
> + * \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 IPAFrameContext
> + * \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 should only be written by its owner.
> + */
> +
> +/**
> + * \struct IPAContext
> + * \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 IPASessionConfiguration::grid
> + * \brief Grid configuration of the IPA
> + *
> + * \var IPASessionConfiguration::grid::bdsGrid
> + * \brief Bayer Down Scaler grid plane config used by the kernel
> + *
> + * \var IPASessionConfiguration::grid::bdsOutputSize
> + * \brief BDS output size configured by the pipeline handler
> + *
> + * \var IPASessionConfiguration::grid::stride
> + * \brief Number of cells on one line including the ImgU padding
> + */
> +
> +/**
> + * \struct IPASessionConfiguration::agc
> + * \brief AGC parameters configuration of the IPA
> + *
> + * \var IPASessionConfiguration::agc::minShutterSpeed
> + * \brief Minimum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::maxShutterSpeed
> + * \brief Maximum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::minAnalogueGain
> + * \brief Minimum analogue gain supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::maxAnalogueGain
> + * \brief Maximum analogue gain supported with the configured sensor
> + */
> +
> +/**
> + * \struct IPAFrameContext::agc
> + * \brief Context for the Automatic Gain Control algorithm
> + *
> + * The exposure and gain determined are expected to be applied to the sensor
> + * at the earliest opportunity.
> + *
> + * \var IPAFrameContext::agc::exposure
> + * \brief Exposure time expressed as a number of lines
> + *
> + * \var IPAFrameContext::agc::gain
> + * \brief Analogue gain multiplier
> + *
> + * The gain should be adapted to the sensor specific gain code before applying.
> + */
> +
> +/**
> + * \struct IPAFrameContext::awb
> + * \brief Context for the Automatic White Balance algorithm
> + *
> + * \struct IPAFrameContext::awb::gains
> + * \brief White balance gains
> + *
> + * \var IPAFrameContext::awb::gains::red
> + * \brief White balance gain for R channel
> + *
> + * \var IPAFrameContext::awb::gains::green
> + * \brief White balance gain for G channel
> + *
> + * \var IPAFrameContext::awb::gains::blue
> + * \brief White balance gain for B channel
> + */
> +
> +/**
> + * \struct IPAFrameContext::toneMapping
> + * \brief Context for ToneMapping and Gamma control
> + *
> + * \var IPAFrameContext::toneMapping::gamma
> + * \brief Gamma value for the LUT
> + *
> + * \var IPAFrameContext::toneMapping::gammaCorrection
> + * \brief Per-pixel tone mapping implemented as a LUT
> + *
> + * The LUT structure is defined by the IPU3 kernel interface. See
> + * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> + */
> +
> +} /* namespace libcamera::ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index f7632bc0..065febf8 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -37,130 +37,6 @@
>  #include "algorithms/tone_mapping.h"
>  #include "libipa/camera_sensor_helper.h"
>  
> -/**
> - * \file ipa_context.h
> - * \brief Context and state information shared between the algorithms
> - */
> -
> -/**
> - * \struct IPASessionConfiguration
> - * \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 IPAFrameContext
> - * \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 should only be written by its owner.
> - */
> -
> -/**
> - * \struct IPAContext
> - * \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 IPASessionConfiguration::grid
> - * \brief Grid configuration of the IPA
> - *
> - * \var IPASessionConfiguration::grid::bdsGrid
> - * \brief Bayer Down Scaler grid plane config used by the kernel
> - *
> - * \var IPASessionConfiguration::grid::bdsOutputSize
> - * \brief BDS output size configured by the pipeline handler
> - *
> - * \var IPASessionConfiguration::grid::stride
> - * \brief Number of cells on one line including the ImgU padding
> - */
> -
> -/**
> - * \struct IPASessionConfiguration::agc
> - * \brief AGC parameters configuration of the IPA
> - *
> - * \var IPASessionConfiguration::agc::minShutterSpeed
> - * \brief Minimum shutter speed supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::grid::maxShutterSpeed
> - * \brief Maximum shutter speed supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::grid::minAnalogueGain
> - * \brief Minimum analogue gain supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::grid::maxAnalogueGain
> - * \brief Maximum analogue gain supported with the configured sensor
> - */
> -
> -/**
> - * \struct IPAFrameContext::agc
> - * \brief Context for the Automatic Gain Control algorithm
> - *
> - * The exposure and gain determined are expected to be applied to the sensor
> - * at the earliest opportunity.
> - *
> - * \var IPAFrameContext::agc::exposure
> - * \brief Exposure time expressed as a number of lines
> - *
> - * \var IPAFrameContext::agc::gain
> - * \brief Analogue gain multiplier
> - *
> - * The gain should be adapted to the sensor specific gain code before applying.
> - */
> -
> -/**
> - * \struct IPAFrameContext::awb
> - * \brief Context for the Automatic White Balance algorithm
> - *
> - * \struct IPAFrameContext::awb::gains
> - * \brief White balance gains
> - *
> - * \var IPAFrameContext::awb::gains::red
> - * \brief White balance gain for R channel
> - *
> - * \var IPAFrameContext::awb::gains::green
> - * \brief White balance gain for G channel
> - *
> - * \var IPAFrameContext::awb::gains::blue
> - * \brief White balance gain for B channel
> - */
> -
> -/**
> - * \struct IPAFrameContext::toneMapping
> - * \brief Context for ToneMapping and Gamma control
> - *
> - * \var IPAFrameContext::toneMapping::gamma
> - * \brief Gamma value for the LUT
> - *
> - * \var IPAFrameContext::toneMapping::gammaCorrection
> - * \brief Per-pixel tone mapping implemented as a LUT
> - *
> - * The LUT structure is defined by the IPU3 kernel interface. See
> - * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> - */
> -
>  /* Minimum grid width, expressed as a number of cells */
>  static constexpr uint32_t kMinGridWidth = 16;
>  /* Maximum grid width, expressed as a number of cells */
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index 39320980..3194111a 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -5,6 +5,7 @@ subdir('algorithms')
>  ipa_name = 'ipa_ipu3'
>  
>  ipu3_ipa_sources = files([
> +    'ipa_context.cpp',
>      'ipu3.cpp',
>  ])
>  
> -- 
> 2.32.0
>

Hi Jean-Michel and Kieran,

Thank you for the patch.

On Fri, Oct 22, 2021 at 05:12:14PM +0200, Jean-Michel Hautbois wrote:

No From: line from Kieran here ?

> The IPU3 IPA core is growing with additional documentation. The
> ipa_context documentation is stored here, but it pushes the IPU3
> documentation and implementation further from the head of the file.
> 
> Furthermore, the ipa_context documentation is outside of the ipa::ipu3
> namespace and isn't identified correctly by Doxygen.
> 
> Move the ipa_context to its own compilation object even though there
> isn't any code, but to maintain consistency with our documentation
> model.
> 
> Correctly re-introduce the documentation into the libcamera::ipa::ipu3
> namespace during the move.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/ipa/ipu3/ipa_context.cpp | 136 +++++++++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3.cpp        | 124 --------------------------------
>  src/ipa/ipu3/meson.build     |   1 +
>  3 files changed, 137 insertions(+), 124 deletions(-)
>  create mode 100644 src/ipa/ipu3/ipa_context.cpp
> 
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> new file mode 100644
> index 00000000..3e154e60
> --- /dev/null
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * ipa_context.cpp - IPU3 IPA Context
> + */
> +
> +#include "ipa_context.h"
> +
> +namespace libcamera::ipa::ipu3 {
> +
> +/**
> + * \file ipa_context.h
> + * \brief Context and state information shared between the algorithms
> + */

We usually put the \file block before the namespace.

> +
> +/**
> + * \struct IPASessionConfiguration
> + * \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 IPAFrameContext
> + * \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 should only be written by its owner.
> + */
> +
> +/**
> + * \struct IPAContext
> + * \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 IPASessionConfiguration::grid
> + * \brief Grid configuration of the IPA
> + *
> + * \var IPASessionConfiguration::grid::bdsGrid
> + * \brief Bayer Down Scaler grid plane config used by the kernel
> + *
> + * \var IPASessionConfiguration::grid::bdsOutputSize
> + * \brief BDS output size configured by the pipeline handler
> + *
> + * \var IPASessionConfiguration::grid::stride
> + * \brief Number of cells on one line including the ImgU padding
> + */
> +
> +/**
> + * \struct IPASessionConfiguration::agc
> + * \brief AGC parameters configuration of the IPA
> + *
> + * \var IPASessionConfiguration::agc::minShutterSpeed
> + * \brief Minimum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::maxShutterSpeed
> + * \brief Maximum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::minAnalogueGain
> + * \brief Minimum analogue gain supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::grid::maxAnalogueGain
> + * \brief Maximum analogue gain supported with the configured sensor
> + */
> +
> +/**
> + * \struct IPAFrameContext::agc
> + * \brief Context for the Automatic Gain Control algorithm
> + *
> + * The exposure and gain determined are expected to be applied to the sensor
> + * at the earliest opportunity.
> + *
> + * \var IPAFrameContext::agc::exposure
> + * \brief Exposure time expressed as a number of lines
> + *
> + * \var IPAFrameContext::agc::gain
> + * \brief Analogue gain multiplier
> + *
> + * The gain should be adapted to the sensor specific gain code before applying.
> + */
> +
> +/**
> + * \struct IPAFrameContext::awb
> + * \brief Context for the Automatic White Balance algorithm
> + *
> + * \struct IPAFrameContext::awb::gains
> + * \brief White balance gains
> + *
> + * \var IPAFrameContext::awb::gains::red
> + * \brief White balance gain for R channel
> + *
> + * \var IPAFrameContext::awb::gains::green
> + * \brief White balance gain for G channel
> + *
> + * \var IPAFrameContext::awb::gains::blue
> + * \brief White balance gain for B channel
> + */
> +
> +/**
> + * \struct IPAFrameContext::toneMapping
> + * \brief Context for ToneMapping and Gamma control
> + *
> + * \var IPAFrameContext::toneMapping::gamma
> + * \brief Gamma value for the LUT
> + *
> + * \var IPAFrameContext::toneMapping::gammaCorrection
> + * \brief Per-pixel tone mapping implemented as a LUT
> + *
> + * The LUT structure is defined by the IPU3 kernel interface. See
> + * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> + */
> +
> +} /* namespace libcamera::ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index f7632bc0..065febf8 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -37,130 +37,6 @@
>  #include "algorithms/tone_mapping.h"
>  #include "libipa/camera_sensor_helper.h"
>  
> -/**
> - * \file ipa_context.h
> - * \brief Context and state information shared between the algorithms
> - */
> -
> -/**
> - * \struct IPASessionConfiguration
> - * \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 IPAFrameContext
> - * \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 should only be written by its owner.
> - */
> -
> -/**
> - * \struct IPAContext
> - * \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 IPASessionConfiguration::grid
> - * \brief Grid configuration of the IPA
> - *
> - * \var IPASessionConfiguration::grid::bdsGrid
> - * \brief Bayer Down Scaler grid plane config used by the kernel
> - *
> - * \var IPASessionConfiguration::grid::bdsOutputSize
> - * \brief BDS output size configured by the pipeline handler
> - *
> - * \var IPASessionConfiguration::grid::stride
> - * \brief Number of cells on one line including the ImgU padding
> - */
> -
> -/**
> - * \struct IPASessionConfiguration::agc
> - * \brief AGC parameters configuration of the IPA
> - *
> - * \var IPASessionConfiguration::agc::minShutterSpeed
> - * \brief Minimum shutter speed supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::grid::maxShutterSpeed
> - * \brief Maximum shutter speed supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::grid::minAnalogueGain
> - * \brief Minimum analogue gain supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::grid::maxAnalogueGain
> - * \brief Maximum analogue gain supported with the configured sensor
> - */
> -
> -/**
> - * \struct IPAFrameContext::agc
> - * \brief Context for the Automatic Gain Control algorithm
> - *
> - * The exposure and gain determined are expected to be applied to the sensor
> - * at the earliest opportunity.
> - *
> - * \var IPAFrameContext::agc::exposure
> - * \brief Exposure time expressed as a number of lines
> - *
> - * \var IPAFrameContext::agc::gain
> - * \brief Analogue gain multiplier
> - *
> - * The gain should be adapted to the sensor specific gain code before applying.
> - */
> -
> -/**
> - * \struct IPAFrameContext::awb
> - * \brief Context for the Automatic White Balance algorithm
> - *
> - * \struct IPAFrameContext::awb::gains
> - * \brief White balance gains
> - *
> - * \var IPAFrameContext::awb::gains::red
> - * \brief White balance gain for R channel
> - *
> - * \var IPAFrameContext::awb::gains::green
> - * \brief White balance gain for G channel
> - *
> - * \var IPAFrameContext::awb::gains::blue
> - * \brief White balance gain for B channel
> - */
> -
> -/**
> - * \struct IPAFrameContext::toneMapping
> - * \brief Context for ToneMapping and Gamma control
> - *
> - * \var IPAFrameContext::toneMapping::gamma
> - * \brief Gamma value for the LUT
> - *
> - * \var IPAFrameContext::toneMapping::gammaCorrection
> - * \brief Per-pixel tone mapping implemented as a LUT
> - *
> - * The LUT structure is defined by the IPU3 kernel interface. See
> - * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> - */
> -
>  /* Minimum grid width, expressed as a number of cells */
>  static constexpr uint32_t kMinGridWidth = 16;
>  /* Maximum grid width, expressed as a number of cells */
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index 39320980..3194111a 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -5,6 +5,7 @@ subdir('algorithms')
>  ipa_name = 'ipa_ipu3'
>  
>  ipu3_ipa_sources = files([
> +    'ipa_context.cpp',
>      'ipu3.cpp',
>  ])
>

Hi Kieran,

On 25/10/2021 23:37, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-22 16:12:14)
> 
> This doesn't have a 'From' (me) line here, so I assume the commit author
> has been set to you.

Mmmh, I don't know why, I will change the author before v4.

> 
> 
>> The IPU3 IPA core is growing with additional documentation. The
>> ipa_context documentation is stored here, but it pushes the IPU3
>> documentation and implementation further from the head of the file.
>>
>> Furthermore, the ipa_context documentation is outside of the ipa::ipu3
>> namespace and isn't identified correctly by Doxygen.
>>
>> Move the ipa_context to its own compilation object even though there
>> isn't any code, but to maintain consistency with our documentation
>> model.
>>
>> Correctly re-introduce the documentation into the libcamera::ipa::ipu3
>> namespace during the move.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> However, it only has my SoB ...  You'll need to add yours too ;-)
> 
> I think I'm reviewing a patch I wrote here, but it still looks correct
> to me ;-) so ...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

And:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
>> ---
>>   src/ipa/ipu3/ipa_context.cpp | 136 +++++++++++++++++++++++++++++++++++
>>   src/ipa/ipu3/ipu3.cpp        | 124 --------------------------------
>>   src/ipa/ipu3/meson.build     |   1 +
>>   3 files changed, 137 insertions(+), 124 deletions(-)
>>   create mode 100644 src/ipa/ipu3/ipa_context.cpp
>>
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> new file mode 100644
>> index 00000000..3e154e60
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -0,0 +1,136 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Google Inc.
>> + *
>> + * ipa_context.cpp - IPU3 IPA Context
>> + */
>> +
>> +#include "ipa_context.h"
>> +
>> +namespace libcamera::ipa::ipu3 {
>> +
>> +/**
>> + * \file ipa_context.h
>> + * \brief Context and state information shared between the algorithms
>> + */
>> +
>> +/**
>> + * \struct IPASessionConfiguration
>> + * \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 IPAFrameContext
>> + * \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 should only be written by its owner.
>> + */
>> +
>> +/**
>> + * \struct IPAContext
>> + * \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 IPASessionConfiguration::grid
>> + * \brief Grid configuration of the IPA
>> + *
>> + * \var IPASessionConfiguration::grid::bdsGrid
>> + * \brief Bayer Down Scaler grid plane config used by the kernel
>> + *
>> + * \var IPASessionConfiguration::grid::bdsOutputSize
>> + * \brief BDS output size configured by the pipeline handler
>> + *
>> + * \var IPASessionConfiguration::grid::stride
>> + * \brief Number of cells on one line including the ImgU padding
>> + */
>> +
>> +/**
>> + * \struct IPASessionConfiguration::agc
>> + * \brief AGC parameters configuration of the IPA
>> + *
>> + * \var IPASessionConfiguration::agc::minShutterSpeed
>> + * \brief Minimum shutter speed supported with the configured sensor
>> + *
>> + * \var IPASessionConfiguration::grid::maxShutterSpeed
>> + * \brief Maximum shutter speed supported with the configured sensor
>> + *
>> + * \var IPASessionConfiguration::grid::minAnalogueGain
>> + * \brief Minimum analogue gain supported with the configured sensor
>> + *
>> + * \var IPASessionConfiguration::grid::maxAnalogueGain
>> + * \brief Maximum analogue gain supported with the configured sensor
>> + */
>> +
>> +/**
>> + * \struct IPAFrameContext::agc
>> + * \brief Context for the Automatic Gain Control algorithm
>> + *
>> + * The exposure and gain determined are expected to be applied to the sensor
>> + * at the earliest opportunity.
>> + *
>> + * \var IPAFrameContext::agc::exposure
>> + * \brief Exposure time expressed as a number of lines
>> + *
>> + * \var IPAFrameContext::agc::gain
>> + * \brief Analogue gain multiplier
>> + *
>> + * The gain should be adapted to the sensor specific gain code before applying.
>> + */
>> +
>> +/**
>> + * \struct IPAFrameContext::awb
>> + * \brief Context for the Automatic White Balance algorithm
>> + *
>> + * \struct IPAFrameContext::awb::gains
>> + * \brief White balance gains
>> + *
>> + * \var IPAFrameContext::awb::gains::red
>> + * \brief White balance gain for R channel
>> + *
>> + * \var IPAFrameContext::awb::gains::green
>> + * \brief White balance gain for G channel
>> + *
>> + * \var IPAFrameContext::awb::gains::blue
>> + * \brief White balance gain for B channel
>> + */
>> +
>> +/**
>> + * \struct IPAFrameContext::toneMapping
>> + * \brief Context for ToneMapping and Gamma control
>> + *
>> + * \var IPAFrameContext::toneMapping::gamma
>> + * \brief Gamma value for the LUT
>> + *
>> + * \var IPAFrameContext::toneMapping::gammaCorrection
>> + * \brief Per-pixel tone mapping implemented as a LUT
>> + *
>> + * The LUT structure is defined by the IPU3 kernel interface. See
>> + * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>> + */
>> +
>> +} /* namespace libcamera::ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index f7632bc0..065febf8 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -37,130 +37,6 @@
>>   #include "algorithms/tone_mapping.h"
>>   #include "libipa/camera_sensor_helper.h"
>>   
>> -/**
>> - * \file ipa_context.h
>> - * \brief Context and state information shared between the algorithms
>> - */
>> -
>> -/**
>> - * \struct IPASessionConfiguration
>> - * \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 IPAFrameContext
>> - * \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 should only be written by its owner.
>> - */
>> -
>> -/**
>> - * \struct IPAContext
>> - * \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 IPASessionConfiguration::grid
>> - * \brief Grid configuration of the IPA
>> - *
>> - * \var IPASessionConfiguration::grid::bdsGrid
>> - * \brief Bayer Down Scaler grid plane config used by the kernel
>> - *
>> - * \var IPASessionConfiguration::grid::bdsOutputSize
>> - * \brief BDS output size configured by the pipeline handler
>> - *
>> - * \var IPASessionConfiguration::grid::stride
>> - * \brief Number of cells on one line including the ImgU padding
>> - */
>> -
>> -/**
>> - * \struct IPASessionConfiguration::agc
>> - * \brief AGC parameters configuration of the IPA
>> - *
>> - * \var IPASessionConfiguration::agc::minShutterSpeed
>> - * \brief Minimum shutter speed supported with the configured sensor
>> - *
>> - * \var IPASessionConfiguration::grid::maxShutterSpeed
>> - * \brief Maximum shutter speed supported with the configured sensor
>> - *
>> - * \var IPASessionConfiguration::grid::minAnalogueGain
>> - * \brief Minimum analogue gain supported with the configured sensor
>> - *
>> - * \var IPASessionConfiguration::grid::maxAnalogueGain
>> - * \brief Maximum analogue gain supported with the configured sensor
>> - */
>> -
>> -/**
>> - * \struct IPAFrameContext::agc
>> - * \brief Context for the Automatic Gain Control algorithm
>> - *
>> - * The exposure and gain determined are expected to be applied to the sensor
>> - * at the earliest opportunity.
>> - *
>> - * \var IPAFrameContext::agc::exposure
>> - * \brief Exposure time expressed as a number of lines
>> - *
>> - * \var IPAFrameContext::agc::gain
>> - * \brief Analogue gain multiplier
>> - *
>> - * The gain should be adapted to the sensor specific gain code before applying.
>> - */
>> -
>> -/**
>> - * \struct IPAFrameContext::awb
>> - * \brief Context for the Automatic White Balance algorithm
>> - *
>> - * \struct IPAFrameContext::awb::gains
>> - * \brief White balance gains
>> - *
>> - * \var IPAFrameContext::awb::gains::red
>> - * \brief White balance gain for R channel
>> - *
>> - * \var IPAFrameContext::awb::gains::green
>> - * \brief White balance gain for G channel
>> - *
>> - * \var IPAFrameContext::awb::gains::blue
>> - * \brief White balance gain for B channel
>> - */
>> -
>> -/**
>> - * \struct IPAFrameContext::toneMapping
>> - * \brief Context for ToneMapping and Gamma control
>> - *
>> - * \var IPAFrameContext::toneMapping::gamma
>> - * \brief Gamma value for the LUT
>> - *
>> - * \var IPAFrameContext::toneMapping::gammaCorrection
>> - * \brief Per-pixel tone mapping implemented as a LUT
>> - *
>> - * The LUT structure is defined by the IPU3 kernel interface. See
>> - * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
>> - */
>> -
>>   /* Minimum grid width, expressed as a number of cells */
>>   static constexpr uint32_t kMinGridWidth = 16;
>>   /* Maximum grid width, expressed as a number of cells */
>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>> index 39320980..3194111a 100644
>> --- a/src/ipa/ipu3/meson.build
>> +++ b/src/ipa/ipu3/meson.build
>> @@ -5,6 +5,7 @@ subdir('algorithms')
>>   ipa_name = 'ipa_ipu3'
>>   
>>   ipu3_ipa_sources = files([
>> +    'ipa_context.cpp',
>>       'ipu3.cpp',
>>   ])
>>   
>> -- 
>> 2.32.0
>>