[libcamera-devel] [PATCH v2 3/4] src: ipa: raspberrypi: Improve behaviour when AE disabled

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Thu, Nov 26, 2020 at 12:32:02PM +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 at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..e437c626 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -495,6 +495,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>  		case controls::AE_ENABLE: {
>  			RPiController::Algorithm *agc = controller_.GetAlgorithm("agc");
>  			ASSERT(agc);
> +

It's customary, when including unrelated minor changes such as this, to
mention it in the commit message, to let reviewers know the change
didn't get included by mistake.

>  			if (ctrl.second.get<bool>() == false)
>  				agc->Pause();
>  			else
> @@ -512,10 +513,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();
> -

Only AWB now uses the pause mechanism. I wonder if it will survive, or
be dropped there too for the same reason as for the AGC :-)

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

>  			libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());
>  			break;
>  		}
> @@ -526,10 +523,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;

-- 
Regards,

Laurent Pinchart