[libcamera-devel] [PATCH v3 3/5] ipa: ipu3: Use a local parameter object and prepare for 3A algorithms

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Mar 29, 2021 at 09:18:24PM +0200, Jean-Michel Hautbois wrote:
> The IPA will locally modify the parameters before they are passed down
> to the imgU. Use a local parameter object to give a reference to those

s/imgU/ImgU/

> algorithms.
> 
> AGC algorithm calculates a brightness level at min and max

"The AGC algoritm will calculate ..." as there's no AGC yet in this
commit.

> exposure/gains.

"exposure and gain"

If you want to start a new paragraph, you need to insert a blank line.
Otherwise (and I think that's the right option in this case), you
shouldn't break in the middle of a line.

> Starting at the minimum value for both of them shortens the time to
> converge (as the first frames will already have the correct level).
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp    | 11 +++++++++--
>  src/ipa/ipu3/meson.build |  6 +++++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 34a907f2..07dbc24a 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -61,6 +61,9 @@ private:
>  	uint32_t gain_;
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
> +
> +	/* Local parameter storage */
> +	ipu3_uapi_params params_;

As this isn't used in this patch, you can move it to 4/5. Don't forget
to update the subject line to remove the first part.

>  };
>  
>  int IPAIPU3::start()
> @@ -92,11 +95,15 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>  
>  	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
>  	maxExposure_ = itExp->second.max().get<int32_t>();
> -	exposure_ = maxExposure_;
> +	exposure_ = minExposure_;
>  
>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>  	maxGain_ = itGain->second.max().get<int32_t>();
> -	gain_ = maxGain_;
> +	gain_ = minGain_;
> +
> +	params_ = {};
> +
> +	setControls(0);
>  }
>  
>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index a241f617..52d98c8e 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -2,8 +2,12 @@
>  
>  ipa_name = 'ipa_ipu3'
>  
> +ipu3_ipa_sources = files([
> +  'ipu3.cpp',

4 spaces.

> +])
> +
>  mod = shared_module(ipa_name,
> -                    ['ipu3.cpp', libcamera_generated_ipa_headers],
> +                    [ipu3_ipa_sources, libcamera_generated_ipa_headers],
>                      name_prefix : '',
>                      include_directories : [ipa_includes, libipa_includes],
>                      dependencies : libcamera_dep,

Changes in this file aren'trelated to the commit message though. You can
move them to 4/5.

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

-- 
Regards,

Laurent Pinchart