[libcamera-devel,01/11] ipa: ipu3: Document IPAIPU3 class interface

13829 diff mbox series

The IPU3 IPA is maturing to a modular and extensible system capable of
handling specific algorithms for the accelleration clusters on the ImgU.

Provide a top-level class documentation to provide an overview of the
IPA, detailing what events are used and what algorithms are currently
supported, as well as the limitations currently imposed.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

Hi Jean-Michel,

Thank you for the patch.

On Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:
> The IPU3 IPA is maturing to a modular and extensible system capable of
> handling specific algorithms for the accelleration clusters on the ImgU.

s/accelleration/acceleration/

Where does the term "acceleration cluster" come from ? It sounds weird
to me, it's the first time I come across it for ISPs. Have I missed
something ? Or should we say "processing blocks", "processing
operations" or something similar ?

> Provide a top-level class documentation to provide an overview of the
> IPA, detailing what events are used and what algorithms are currently
> supported, as well as the limitations currently imposed.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index c925cf64..90053069 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>  
>  namespace ipa::ipu3 {
>  
> +/**
> + * \brief The IPU3 IPA implementation
> + *
> + * The IPU3 Pipeline defines an IPU3 specific interface for communication

s/IPU3 specific/IPU3-specific/

As far as I know, a hyphen is used for compound adjectives (an
IPU3-specific interface) but not for predicates (the interface is IPU3
specific).

There are three other instances below.

> + * between the PipelineHandler, and the IPA module.

s/PipelineHandler,/PipelineHandler/

> + *
> + * We extend the IPAIPU3Interface to implement our algorithms and handle events
> + * from the IPU3 PipelineHandler to satisfy requests from the application.
> + *
> + * At initialisation time, a CameraSensorHelper is instantiated to support
> + * camera specific calculations, while the default controls are computed, and
> + * the algorithms are constructed and placed in an ordered list.
> + *
> + * The IPU3 ImgU operates with a grid layout to divide the overall frame into
> + * rectangular cells of pixels. When the IPA is configured, we determine the
> + * best grid for the statistics based on the pipeline handler Bayer Down Scaler
> + * output size.

On a side note, I wonder if the IPA should be involved to compute the
ImgU configuration. It's a topic for future research.

> + *
> + * Two main events are then handled to facilitate the operation of the IPU3 ImgU

I think we do a bit more than "facilitating" ImgU operation :-)

> + * by populating its parameter buffer, and adapting the settings of the sensor
> + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.
> + *
> + * When the event \a EventFillParams occurs we populate the ImgU parameter
> + * buffer with settings to configure the device in preparation for handling the
> + * frame queued in the Request.
> + *
> + * When the frame has completed processing, the ImgU will generate a statistics
> + * buffer which is given to the IPA as part of the \a EventStatReady event. At
> + * this event we run the algorithms to parse the statistics and cache any
> + * results for the next \a EventFillParams event.
> + *
> + * The individual algorithms are split into modular components that are called
> + * iteratively to allow them to process statistics from the ImgU in a defined
> + * order.
> + *
> + * The current implementation supports three core algorithms:
> + * - Automatic white balance (AWB)
> + * - Automatic gain and exposure control (AGC)
> + * - Tonemapping (Gamma)

s/Tonemapping/Tone mapping/

Same below.

> + *
> + * AWB is implemented using a Greyworld algorithm, and calculates the red and
> + * blue gains to apply to generate a neutral grey frame overall.
> + *
> + * AGC is handled by calculating a histogram of the green channel to estimate an
> + * analogue gain and shutter time which will provide a well exposed frame. An
> + * IIR filter is used to smooth the changes to the sensor to reduce perceivable

I'd say "low-pass filter" (of possibly "low-pass IIR filter") instead of
"IIR filter" here, as the infinite impulse response isn't what really
matters.

> + * steps.
> + *
> + * The Tonemapping algorithm provides a gamma correction table to improve the
> + * contrast of the scene.
> + *
> + * The IPU3 ImgU has further accellerator clusters to support image quality

s/accellerator/accelerator/

Same comment as for the commit message.

> + * improvements through bayer and temporal noise reductions, however those are
> + * not supported in the current implementation, and will use default settings as
> + * provided by the kernel driver.
> + *
> + * Demosaicing is operating on the default values and could be further optimised

I'm not totally sure what "values" mean here, did you mean "operating
with the default parameters" ?

> + * to provide improved sharpening coefficients, checker artifact removal, and
> + * false color correction.
> + *
> + * Additional image enhancements can be made by providing lens and sensor
> + * specific tuning to adapt for Black Level compensation (BLC), Lens shading
> + * correction (SHD) and Color correction (CCM)

s/$/./

> + */
>  class IPAIPU3 : public IPAIPU3Interface
>  {
>  public:

On Tue, Sep 14, 2021 at 05:54:01AM +0300, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:
> > The IPU3 IPA is maturing to a modular and extensible system capable of
> > handling specific algorithms for the accelleration clusters on the ImgU.
> 
> s/accelleration/acceleration/
> 
> Where does the term "acceleration cluster" come from ? It sounds weird
> to me, it's the first time I come across it for ISPs. Have I missed
> something ? Or should we say "processing blocks", "processing
> operations" or something similar ?
> 
> > Provide a top-level class documentation to provide an overview of the
> > IPA, detailing what events are used and what algorithms are currently
> > supported, as well as the limitations currently imposed.
> > 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index c925cf64..90053069 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
> >  
> >  namespace ipa::ipu3 {
> >  
> > +/**
> > + * \brief The IPU3 IPA implementation
> > + *
> > + * The IPU3 Pipeline defines an IPU3 specific interface for communication
> 
> s/IPU3 specific/IPU3-specific/
> 
> As far as I know, a hyphen is used for compound adjectives (an
> IPU3-specific interface) but not for predicates (the interface is IPU3
> specific).
> 
> There are three other instances below.
> 
> > + * between the PipelineHandler, and the IPA module.
> 
> s/PipelineHandler,/PipelineHandler/
> 
> > + *
> > + * We extend the IPAIPU3Interface to implement our algorithms and handle events
> > + * from the IPU3 PipelineHandler to satisfy requests from the application.
> > + *
> > + * At initialisation time, a CameraSensorHelper is instantiated to support
> > + * camera specific calculations, while the default controls are computed, and
> > + * the algorithms are constructed and placed in an ordered list.
> > + *
> > + * The IPU3 ImgU operates with a grid layout to divide the overall frame into
> > + * rectangular cells of pixels. When the IPA is configured, we determine the
> > + * best grid for the statistics based on the pipeline handler Bayer Down Scaler
> > + * output size.
> 
> On a side note, I wonder if the IPA should be involved to compute the
> ImgU configuration. It's a topic for future research.
> 
> > + *
> > + * Two main events are then handled to facilitate the operation of the IPU3 ImgU
> 
> I think we do a bit more than "facilitating" ImgU operation :-)
> 
> > + * by populating its parameter buffer, and adapting the settings of the sensor
> > + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.
> > + *
> > + * When the event \a EventFillParams occurs we populate the ImgU parameter
> > + * buffer with settings to configure the device in preparation for handling the
> > + * frame queued in the Request.
> > + *
> > + * When the frame has completed processing, the ImgU will generate a statistics
> > + * buffer which is given to the IPA as part of the \a EventStatReady event. At
> > + * this event we run the algorithms to parse the statistics and cache any
> > + * results for the next \a EventFillParams event.
> > + *
> > + * The individual algorithms are split into modular components that are called
> > + * iteratively to allow them to process statistics from the ImgU in a defined
> > + * order.
> > + *
> > + * The current implementation supports three core algorithms:
> > + * - Automatic white balance (AWB)
> > + * - Automatic gain and exposure control (AGC)
> > + * - Tonemapping (Gamma)
> 
> s/Tonemapping/Tone mapping/
> 
> Same below.
> 
> > + *
> > + * AWB is implemented using a Greyworld algorithm, and calculates the red and
> > + * blue gains to apply to generate a neutral grey frame overall.
> > + *
> > + * AGC is handled by calculating a histogram of the green channel to estimate an
> > + * analogue gain and shutter time which will provide a well exposed frame. An
> > + * IIR filter is used to smooth the changes to the sensor to reduce perceivable
> 
> I'd say "low-pass filter" (of possibly "low-pass IIR filter") instead of
> "IIR filter" here, as the infinite impulse response isn't what really
> matters.

By the way, are you aware that the output of the filter is actually
unused ?

> > + * steps.
> > + *
> > + * The Tonemapping algorithm provides a gamma correction table to improve the
> > + * contrast of the scene.
> > + *
> > + * The IPU3 ImgU has further accellerator clusters to support image quality
> 
> s/accellerator/accelerator/
> 
> Same comment as for the commit message.
> 
> > + * improvements through bayer and temporal noise reductions, however those are
> > + * not supported in the current implementation, and will use default settings as
> > + * provided by the kernel driver.
> > + *
> > + * Demosaicing is operating on the default values and could be further optimised
> 
> I'm not totally sure what "values" mean here, did you mean "operating
> with the default parameters" ?
> 
> > + * to provide improved sharpening coefficients, checker artifact removal, and
> > + * false color correction.
> > + *
> > + * Additional image enhancements can be made by providing lens and sensor
> > + * specific tuning to adapt for Black Level compensation (BLC), Lens shading
> > + * correction (SHD) and Color correction (CCM)
> 
> s/$/./
> 
> > + */
> >  class IPAIPU3 : public IPAIPU3Interface
> >  {
> >  public:

Hi Laurent,

As I am rebasing this series with the proper branch, I noticed a few 
comments I still not know how to address now) :

On 14/09/2021 04:54, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:
>> The IPU3 IPA is maturing to a modular and extensible system capable of
>> handling specific algorithms for the accelleration clusters on the ImgU.
> 
> s/accelleration/acceleration/
> 
> Where does the term "acceleration cluster" come from ? It sounds weird
> to me, it's the first time I come across it for ISPs. Have I missed
> something ? Or should we say "processing blocks", "processing
> operations" or something similar ?
> 
>> Provide a top-level class documentation to provide an overview of the
>> IPA, detailing what events are used and what algorithms are currently
>> supported, as well as the limitations currently imposed.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index c925cf64..90053069 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>>   
>>   namespace ipa::ipu3 {
>>   
>> +/**
>> + * \brief The IPU3 IPA implementation
>> + *
>> + * The IPU3 Pipeline defines an IPU3 specific interface for communication
> 
> s/IPU3 specific/IPU3-specific/
> 
> As far as I know, a hyphen is used for compound adjectives (an
> IPU3-specific interface) but not for predicates (the interface is IPU3
> specific).
> 
> There are three other instances below.
> 
>> + * between the PipelineHandler, and the IPA module.
> 
> s/PipelineHandler,/PipelineHandler/
> 
>> + *
>> + * We extend the IPAIPU3Interface to implement our algorithms and handle events
>> + * from the IPU3 PipelineHandler to satisfy requests from the application.
>> + *
>> + * At initialisation time, a CameraSensorHelper is instantiated to support
>> + * camera specific calculations, while the default controls are computed, and
>> + * the algorithms are constructed and placed in an ordered list.
>> + *
>> + * The IPU3 ImgU operates with a grid layout to divide the overall frame into
>> + * rectangular cells of pixels. When the IPA is configured, we determine the
>> + * best grid for the statistics based on the pipeline handler Bayer Down Scaler
>> + * output size.
> 
> On a side note, I wonder if the IPA should be involved to compute the
> ImgU configuration. It's a topic for future research.

Should I add a \todo ?

> 
>> + *
>> + * Two main events are then handled to facilitate the operation of the IPU3 ImgU
> 
> I think we do a bit more than "facilitating" ImgU operation :-)

This is Kieran's term, and I have changed it to:
s/facilitate the operation of /operate/

> 
>> + * by populating its parameter buffer, and adapting the settings of the sensor
>> + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.
>> + *
>> + * When the event \a EventFillParams occurs we populate the ImgU parameter
>> + * buffer with settings to configure the device in preparation for handling the
>> + * frame queued in the Request.
>> + *
>> + * When the frame has completed processing, the ImgU will generate a statistics
>> + * buffer which is given to the IPA as part of the \a EventStatReady event. At
>> + * this event we run the algorithms to parse the statistics and cache any
>> + * results for the next \a EventFillParams event.
>> + *
>> + * The individual algorithms are split into modular components that are called
>> + * iteratively to allow them to process statistics from the ImgU in a defined
>> + * order.
>> + *
>> + * The current implementation supports three core algorithms:
>> + * - Automatic white balance (AWB)
>> + * - Automatic gain and exposure control (AGC)
>> + * - Tonemapping (Gamma)
> 
> s/Tonemapping/Tone mapping/
> 
> Same below.
> 
>> + *
>> + * AWB is implemented using a Greyworld algorithm, and calculates the red and
>> + * blue gains to apply to generate a neutral grey frame overall.
>> + *
>> + * AGC is handled by calculating a histogram of the green channel to estimate an
>> + * analogue gain and shutter time which will provide a well exposed frame. An
>> + * IIR filter is used to smooth the changes to the sensor to reduce perceivable
> 
> I'd say "low-pass filter" (of possibly "low-pass IIR filter") instead of
> "IIR filter" here, as the infinite impulse response isn't what really
> matters.
> 
>> + * steps.
>> + *
>> + * The Tonemapping algorithm provides a gamma correction table to improve the
>> + * contrast of the scene.
>> + *
>> + * The IPU3 ImgU has further accellerator clusters to support image quality
> 
> s/accellerator/accelerator/
> 
> Same comment as for the commit message.
> 
>> + * improvements through bayer and temporal noise reductions, however those are
>> + * not supported in the current implementation, and will use default settings as
>> + * provided by the kernel driver.
>> + *
>> + * Demosaicing is operating on the default values and could be further optimised
> 
> I'm not totally sure what "values" mean here, did you mean "operating
> with the default parameters" ?
> 
>> + * to provide improved sharpening coefficients, checker artifact removal, and
>> + * false color correction.
>> + *
>> + * Additional image enhancements can be made by providing lens and sensor
>> + * specific tuning to adapt for Black Level compensation (BLC), Lens shading
>> + * correction (SHD) and Color correction (CCM)
> 
> s/$/./
> 
>> + */
>>   class IPAIPU3 : public IPAIPU3Interface
>>   {
>>   public:
>

Hi Jean-Michel,

On Fri, Oct 22, 2021 at 10:24:16AM +0200, Jean-Michel Hautbois wrote:
> Hi Laurent,
> 
> As I am rebasing this series with the proper branch, I noticed a few 
> comments I still not know how to address now) :
> 
> On 14/09/2021 04:54, Laurent Pinchart wrote:
> > On Mon, Sep 13, 2021 at 04:58:00PM +0200, Jean-Michel Hautbois wrote:
> >> The IPU3 IPA is maturing to a modular and extensible system capable of
> >> handling specific algorithms for the accelleration clusters on the ImgU.
> > 
> > s/accelleration/acceleration/
> > 
> > Where does the term "acceleration cluster" come from ? It sounds weird
> > to me, it's the first time I come across it for ISPs. Have I missed
> > something ? Or should we say "processing blocks", "processing
> > operations" or something similar ?
> > 
> >> Provide a top-level class documentation to provide an overview of the
> >> IPA, detailing what events are used and what algorithms are currently
> >> supported, as well as the limitations currently imposed.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>   src/ipa/ipu3/ipu3.cpp | 64 +++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 64 insertions(+)
> >>
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index c925cf64..90053069 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -145,6 +145,70 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
> >>   
> >>   namespace ipa::ipu3 {
> >>   
> >> +/**
> >> + * \brief The IPU3 IPA implementation
> >> + *
> >> + * The IPU3 Pipeline defines an IPU3 specific interface for communication
> > 
> > s/IPU3 specific/IPU3-specific/
> > 
> > As far as I know, a hyphen is used for compound adjectives (an
> > IPU3-specific interface) but not for predicates (the interface is IPU3
> > specific).
> > 
> > There are three other instances below.
> > 
> >> + * between the PipelineHandler, and the IPA module.
> > 
> > s/PipelineHandler,/PipelineHandler/
> > 
> >> + *
> >> + * We extend the IPAIPU3Interface to implement our algorithms and handle events
> >> + * from the IPU3 PipelineHandler to satisfy requests from the application.
> >> + *
> >> + * At initialisation time, a CameraSensorHelper is instantiated to support
> >> + * camera specific calculations, while the default controls are computed, and
> >> + * the algorithms are constructed and placed in an ordered list.
> >> + *
> >> + * The IPU3 ImgU operates with a grid layout to divide the overall frame into
> >> + * rectangular cells of pixels. When the IPA is configured, we determine the
> >> + * best grid for the statistics based on the pipeline handler Bayer Down Scaler
> >> + * output size.
> > 
> > On a side note, I wonder if the IPA should be involved to compute the
> > ImgU configuration. It's a topic for future research.
> 
> Should I add a \todo ?

No, I think you can leave it as-is, it's fine.

> >> + *
> >> + * Two main events are then handled to facilitate the operation of the IPU3 ImgU
> > 
> > I think we do a bit more than "facilitating" ImgU operation :-)
> 
> This is Kieran's term, and I have changed it to:
> s/facilitate the operation of /operate/

Sounds good to me.

> >> + * by populating its parameter buffer, and adapting the settings of the sensor
> >> + * attached to the IPU3 CIO2 through sensor specific V4L2 controls.
> >> + *
> >> + * When the event \a EventFillParams occurs we populate the ImgU parameter
> >> + * buffer with settings to configure the device in preparation for handling the
> >> + * frame queued in the Request.
> >> + *
> >> + * When the frame has completed processing, the ImgU will generate a statistics
> >> + * buffer which is given to the IPA as part of the \a EventStatReady event. At
> >> + * this event we run the algorithms to parse the statistics and cache any
> >> + * results for the next \a EventFillParams event.
> >> + *
> >> + * The individual algorithms are split into modular components that are called
> >> + * iteratively to allow them to process statistics from the ImgU in a defined
> >> + * order.
> >> + *
> >> + * The current implementation supports three core algorithms:
> >> + * - Automatic white balance (AWB)
> >> + * - Automatic gain and exposure control (AGC)
> >> + * - Tonemapping (Gamma)
> > 
> > s/Tonemapping/Tone mapping/
> > 
> > Same below.
> > 
> >> + *
> >> + * AWB is implemented using a Greyworld algorithm, and calculates the red and
> >> + * blue gains to apply to generate a neutral grey frame overall.
> >> + *
> >> + * AGC is handled by calculating a histogram of the green channel to estimate an
> >> + * analogue gain and shutter time which will provide a well exposed frame. An
> >> + * IIR filter is used to smooth the changes to the sensor to reduce perceivable
> > 
> > I'd say "low-pass filter" (of possibly "low-pass IIR filter") instead of
> > "IIR filter" here, as the infinite impulse response isn't what really
> > matters.
> > 
> >> + * steps.
> >> + *
> >> + * The Tonemapping algorithm provides a gamma correction table to improve the
> >> + * contrast of the scene.
> >> + *
> >> + * The IPU3 ImgU has further accellerator clusters to support image quality
> > 
> > s/accellerator/accelerator/
> > 
> > Same comment as for the commit message.
> > 
> >> + * improvements through bayer and temporal noise reductions, however those are
> >> + * not supported in the current implementation, and will use default settings as
> >> + * provided by the kernel driver.
> >> + *
> >> + * Demosaicing is operating on the default values and could be further optimised
> > 
> > I'm not totally sure what "values" mean here, did you mean "operating
> > with the default parameters" ?
> > 
> >> + * to provide improved sharpening coefficients, checker artifact removal, and
> >> + * false color correction.
> >> + *
> >> + * Additional image enhancements can be made by providing lens and sensor
> >> + * specific tuning to adapt for Black Level compensation (BLC), Lens shading
> >> + * correction (SHD) and Color correction (CCM)
> > 
> > s/$/./
> > 
> >> + */
> >>   class IPAIPU3 : public IPAIPU3Interface
> >>   {
> >>   public: