[libcamera-devel,v3,3/4] src: ipa: raspberrypi: Improve behaviour when AE disabled
diff mbox series

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

Commit Message

David Plowman Nov. 26, 2020, 2:23 p.m. UTC 
AE/AGC "disabled" is now handled better by the algorithm for itself,
so it no longer needs to be "resumed" before setting fixed shutter or
gain values.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 8 --------
 1 file changed, 8 deletions(-)

Comments

Naushir Patuck Nov. 26, 2020, 4:44 p.m. UTC | #1 
Hi David,

Thank you for the patch.

On Thu, 26 Nov 2020 at 14:23, David Plowman <david.plowman@raspberrypi.com>
wrote:

> AE/AGC "disabled" is now handled better by the algorithm for itself,
> so it no longer needs to be "resumed" before setting fixed shutter or
> gain values.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

Reviewed-by Naushir Patuck <naush@raspberrypi.com>


> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..29d48b1b 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -512,10 +512,6 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         /* This expects units of micro-seconds. */
>                         agc->SetFixedShutter(ctrl.second.get<int32_t>());
>
> -                       /* For the manual values to take effect, AGC must
> be unpaused. */
> -                       if (agc->IsPaused())
> -                               agc->Resume();
> -
>                         libcameraMetadata_.set(controls::ExposureTime,
> ctrl.second.get<int32_t>());
>                         break;
>                 }
> @@ -526,10 +522,6 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         ASSERT(agc);
>
> agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>
> -                       /* For the manual values to take effect, AGC must
> be unpaused. */
> -                       if (agc->IsPaused())
> -                               agc->Resume();
> -
>                         libcameraMetadata_.set(controls::AnalogueGain,
>                                                ctrl.second.get<float>());
>                         break;
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Kieran Bingham Nov. 30, 2020, 11:01 a.m. UTC | #2 
Hi David,

On 26/11/2020 14:23, David Plowman wrote:
> AE/AGC "disabled" is now handled better by the algorithm for itself,
> so it no longer needs to be "resumed" before setting fixed shutter or
> gain values.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

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

Also, this was given in v2, so collecting in this version.

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

> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..29d48b1b 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -512,10 +512,6 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			/* This expects units of micro-seconds. */
>  			agc->SetFixedShutter(ctrl.second.get<int32_t>());
>  
> -			/* For the manual values to take effect, AGC must be unpaused. */
> -			if (agc->IsPaused())
> -				agc->Resume();
> -
>  			libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
>  			break;
>  		}
> @@ -526,10 +522,6 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			ASSERT(agc);
>  			agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>  
> -			/* For the manual values to take effect, AGC must be unpaused. */
> -			if (agc->IsPaused())
> -				agc->Resume();
> -
>  			libcameraMetadata_.set(controls::AnalogueGain,
>  					       ctrl.second.get<float>());
>  			break;
>
Laurent Pinchart Nov. 30, 2020, 6:55 p.m. UTC | #3 
Hi David,

Thank you for the patch.

On Thu, Nov 26, 2020 at 02:23:20PM +0000, David Plowman wrote:
> AE/AGC "disabled" is now handled better by the algorithm for itself,
> so it no longer needs to be "resumed" before setting fixed shutter or
> gain values.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

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

Btw, when I give a Reviewed-by tag conditioned by small issues being
addressed, feel free to add it to the next version. I trust that you can
address removal of a blank line without me needing to review that again
:-)

> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..29d48b1b 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -512,10 +512,6 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			/* This expects units of micro-seconds. */
>  			agc->SetFixedShutter(ctrl.second.get<int32_t>());
>  
> -			/* For the manual values to take effect, AGC must be unpaused. */
> -			if (agc->IsPaused())
> -				agc->Resume();
> -
>  			libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
>  			break;
>  		}
> @@ -526,10 +522,6 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			ASSERT(agc);
>  			agc->SetFixedAnalogueGain(ctrl.second.get<float>());
>  
> -			/* For the manual values to take effect, AGC must be unpaused. */
> -			if (agc->IsPaused())
> -				agc->Resume();
> -
>  			libcameraMetadata_.set(controls::AnalogueGain,
>  					       ctrl.second.get<float>());
>  			break;

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 9853a343..29d48b1b 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -512,10 +512,6 @@  void IPARPi::queueRequest(const ControlList &controls)
 			/* This expects units of micro-seconds. */
 			agc->SetFixedShutter(ctrl.second.get<int32_t>());
 
-			/* For the manual values to take effect, AGC must be unpaused. */
-			if (agc->IsPaused())
-				agc->Resume();
-
 			libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
 			break;
 		}
@@ -526,10 +522,6 @@  void IPARPi::queueRequest(const ControlList &controls)
 			ASSERT(agc);
 			agc->SetFixedAnalogueGain(ctrl.second.get<float>());
 
-			/* For the manual values to take effect, AGC must be unpaused. */
-			if (agc->IsPaused())
-				agc->Resume();
-
 			libcameraMetadata_.set(controls::AnalogueGain,
 					       ctrl.second.get<float>());
 			break;