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

Message ID 20201126142321.5563-3-david.plowman@raspberrypi.com
State Accepted
Commit 4db11b57258a1ea3a0e58f1e05d1854de0510d67
Headers show
Series
  • Raspberry Pi AGC improvements
Related show

Commit Message

David Plowman Nov. 26, 2020, 2:23 p.m. UTC 
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.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 24 ++++++++++++++++++++++
 src/ipa/raspberrypi/controller/rpi/agc.hpp |  4 ++++
 2 files changed, 28 insertions(+)

Comments

Kieran Bingham Nov. 30, 2020, 11:06 a.m. UTC | #1 
Hi David,

On 26/11/2020 14:23, 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.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Pulling my tag forwards here too:

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

Naush, Can you review this one directly please?

This is the only one in this series that hasn't got your tag.

--
Kieran



> ---
>  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
>
Naushir Patuck Nov. 30, 2020, 12:14 p.m. UTC | #2 
Hi David,

Thank you for the patch.


On Thu, 26 Nov 2020, 2:23 pm David Plowman, <david.plowman@raspberrypi.com>
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.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

Reviewed-by: Naushir Patuck <naush@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
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Nov. 30, 2020, 6:53 p.m. UTC | #3 
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@ideasonboard.com>

> Signed-off-by: David Plowman <david.plowman@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

Patch
diff mbox series 

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