[libcamera-devel] [PATCH v3 2/4] src: ipa: raspberrypi: agc: Make AGC handle Pause/Resume for itself

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Thu, Nov 26, 2020 at 02:23:19PM +0000, David Plowman wrote:
> AGC, when paused, sets the last exposure/gain it wrote to be its
> "fixed" values and will therefore continue to return them. When
> resumed, we clear them so that both will float again.
> 
> This approach is better because AGC can be paused and we can
> subsequently change (for example) the exposure and the gain won't
> float again.

I think we'll need to revisit this based on the outcome of the
discussion on v2, but that's not a blocker.

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

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++
>  src/ipa/raspberrypi/controller/rpi/agc.hpp |  4 ++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 30a1c1c1..9da18c31 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -184,6 +184,26 @@ void Agc::Read(boost::property_tree::ptree const &params)
>  	exposure_mode_ = &config_.exposure_modes[exposure_mode_name_];
>  	constraint_mode_name_ = config_.default_constraint_mode;
>  	constraint_mode_ = &config_.constraint_modes[constraint_mode_name_];
> +	// Set up the "last shutter/gain" values, in case AGC starts "disabled".
> +	status_.shutter_time = config_.default_exposure_time;
> +	status_.analogue_gain = config_.default_analogue_gain;
> +}
> +
> +bool Agc::IsPaused() const
> +{
> +	return false;
> +}
> +
> +void Agc::Pause()
> +{
> +	fixed_shutter_ = status_.shutter_time;
> +	fixed_analogue_gain_ = status_.analogue_gain;
> +}
> +
> +void Agc::Resume()
> +{
> +	fixed_shutter_ = 0;
> +	fixed_analogue_gain_ = 0;
>  }
>  
>  void Agc::SetEv(double ev)
> @@ -199,11 +219,15 @@ void Agc::SetFlickerPeriod(double flicker_period)
>  void Agc::SetFixedShutter(double fixed_shutter)
>  {
>  	fixed_shutter_ = fixed_shutter;
> +	// Set this in case someone calls Pause() straight after.
> +	status_.shutter_time = fixed_shutter;
>  }
>  
>  void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
>  {
>  	fixed_analogue_gain_ = fixed_analogue_gain;
> +	// Set this in case someone calls Pause() straight after.
> +	status_.analogue_gain = fixed_analogue_gain;
>  }
>  
>  void Agc::SetMeteringMode(std::string const &metering_mode_name)
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 47ebb324..95db1812 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -70,6 +70,10 @@ public:
>  	Agc(Controller *controller);
>  	char const *Name() const override;
>  	void Read(boost::property_tree::ptree const &params) override;
> +	// AGC handles "pausing" for itself.
> +	bool IsPaused() const override;
> +	void Pause() override;
> +	void Resume() override;
>  	void SetEv(double ev) override;
>  	void SetFlickerPeriod(double flicker_period) override;
>  	void SetFixedShutter(double fixed_shutter) override; // microseconds

-- 
Regards,

Laurent Pinchart