[libcamera-devel,v2,2/2] ipa: raspberrypi: Handle AEC/AGC flicker controls
diff mbox series

Message ID 20230126134559.3323-3-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Add flicker avoidance controls
Related show

Commit Message

David Plowman Jan. 26, 2023, 1:45 p.m. UTC 
We handle the flicker modes by passing the correct period to the
AEC/AGC algorithm which already contains the necessary code. The
"Auto" mode is currently unsupported.

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

Comments

Kieran Bingham Jan. 30, 2023, 1:01 p.m. UTC | #1 
Quoting David Plowman via libcamera-devel (2023-01-26 13:45:59)
> We handle the flicker modes by passing the correct period to the
> AEC/AGC algorithm which already contains the necessary code. The
> "Auto" mode is currently unsupported.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 5f7397e2..477d72ba 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -86,6 +86,8 @@ static const ControlInfoMap::Map ipaControls{
>         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>         { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>         { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> +       { &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },
> +       { &controls::AeFlickerPeriod, ControlInfo(0, 1000000) },
>         { &controls::AwbEnable, ControlInfo(false, true) },
>         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> @@ -218,6 +220,12 @@ private:
>  
>         /* Maximum gain code for the sensor. */
>         uint32_t maxSensorGainCode_;
> +
> +       /* The current state of flicker avoidance. */
> +       struct FlickerState {
> +               int32_t mode;
> +               Duration period;
> +       } flickerState_;
>  };
>  
>  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> @@ -946,6 +954,78 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         break;
>                 }
>  
> +               case controls::AE_FLICKER_MODE: {
> +                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +                               controller_.getAlgorithm("agc"));
> +                       if (!agc) {
> +                               LOG(IPARPI, Warning)
> +                                       << "Could not set AE_FLICKER_MODE - no AGC algorithm";

Hrm. I believe we use AE_FLICKER_MODE because we can't switch on
&controls::AeFlickerMode (it's difficult to get a constexpr of the id)

But users/developers who read this warning won't see
AE_FLICKER_MODE, they would be using the controls::AeFlickerMode name,
so it might be better to use that?

Otherwise, seems good to me.

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



> +                               break;
> +                       }
> +
> +                       int32_t mode = ctrl.second.get<int32_t>();
> +                       bool modeValid = true;
> +
> +                       switch (mode) {
> +                       case controls::FlickerOff: {
> +                               agc->setFlickerPeriod(0us);
> +
> +                               break;
> +                       }
> +
> +                       case controls::FlickerFreq50Hz: {
> +                               agc->setFlickerPeriod(10000 * 1.0us);
> +
> +                               break;
> +                       }
> +
> +                       case controls::FlickerFreq60Hz: {
> +                               agc->setFlickerPeriod(8333.333 * 1.0us);
> +
> +                               break;
> +                       }
> +
> +                       case controls::FlickerCustom: {
> +                               agc->setFlickerPeriod(flickerState_.period);
> +
> +                               break;
> +                       }
> +
> +                       default:
> +                               LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported";
> +                               modeValid = false;
> +
> +                               break;
> +                       }
> +
> +                       if (modeValid)
> +                               flickerState_.mode = mode;
> +
> +                       break;
> +               }
> +
> +               case controls::AE_FLICKER_PERIOD: {
> +                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +                               controller_.getAlgorithm("agc"));
> +                       if (!agc) {
> +                               LOG(IPARPI, Warning)
> +                                       << "Could not set AE_FLICKER_PERIOD - no AGC algorithm";
> +                               break;
> +                       }
> +
> +                       uint32_t period = ctrl.second.get<int32_t>();
> +                       flickerState_.period = period * 1.0us;
> +
> +                       /*
> +                        * We note that it makes no difference if the mode gets set to "custom"
> +                        * first, and the period updated after, or vice versa.
> +                        */
> +                       if (flickerState_.mode == controls::FlickerCustom)
> +                               agc->setFlickerPeriod(flickerState_.period);
> +
> +                       break;
> +               }
> +
>                 case controls::AWB_ENABLE: {
>                         RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>                                 controller_.getAlgorithm("awb"));
> -- 
> 2.30.2
>
Naushir Patuck March 13, 2023, 9:06 a.m. UTC | #2 
Hi David,

Thanks for the work!

On Thu, 26 Jan 2023 at 13:46, David Plowman via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> We handle the flicker modes by passing the correct period to the
> AEC/AGC algorithm which already contains the necessary code. The
> "Auto" mode is currently unsupported.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

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



> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 80 +++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 5f7397e2..477d72ba 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -86,6 +86,8 @@ static const ControlInfoMap::Map ipaControls{
>         { &controls::AeConstraintMode,
> ControlInfo(controls::AeConstraintModeValues) },
>         { &controls::AeExposureMode,
> ControlInfo(controls::AeExposureModeValues) },
>         { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> +       { &controls::AeFlickerMode,
> ControlInfo(controls::AeFlickerModeValues) },
> +       { &controls::AeFlickerPeriod, ControlInfo(0, 1000000) },
>         { &controls::AwbEnable, ControlInfo(false, true) },
>         { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> @@ -218,6 +220,12 @@ private:
>
>         /* Maximum gain code for the sensor. */
>         uint32_t maxSensorGainCode_;
> +
> +       /* The current state of flicker avoidance. */
> +       struct FlickerState {
> +               int32_t mode;
> +               Duration period;
> +       } flickerState_;
>  };
>
>  int IPARPi::init(const IPASettings &settings, bool lensPresent,
> IPAInitResult *result)
> @@ -946,6 +954,78 @@ void IPARPi::queueRequest(const ControlList &controls)
>                         break;
>                 }
>
> +               case controls::AE_FLICKER_MODE: {
> +                       RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> +                               controller_.getAlgorithm("agc"));
> +                       if (!agc) {
> +                               LOG(IPARPI, Warning)
> +                                       << "Could not set AE_FLICKER_MODE
> - no AGC algorithm";
> +                               break;
> +                       }
> +
> +                       int32_t mode = ctrl.second.get<int32_t>();
> +                       bool modeValid = true;
> +
> +                       switch (mode) {
> +                       case controls::FlickerOff: {
> +                               agc->setFlickerPeriod(0us);
> +
> +                               break;
> +                       }
> +
> +                       case controls::FlickerFreq50Hz: {
> +                               agc->setFlickerPeriod(10000 * 1.0us);
> +
> +                               break;
> +                       }
> +
> +                       case controls::FlickerFreq60Hz: {
> +                               agc->setFlickerPeriod(8333.333 * 1.0us);
> +
> +                               break;
> +                       }
> +
> +                       case controls::FlickerCustom: {
> +
>  agc->setFlickerPeriod(flickerState_.period);
> +
> +                               break;
> +                       }
> +
> +                       default:
> +                               LOG(IPARPI, Error) << "Flicker mode " <<
> mode << " is not supported";
> +                               modeValid = false;
> +
> +                               break;
> +                       }
> +
> +                       if (modeValid)
> +                               flickerState_.mode = mode;
> +
> +                       break;
> +               }
> +
> +               case controls::AE_FLICKER_PERIOD: {
> +                       RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> +                               controller_.getAlgorithm("agc"));
> +                       if (!agc) {
> +                               LOG(IPARPI, Warning)
> +                                       << "Could not set
> AE_FLICKER_PERIOD - no AGC algorithm";
> +                               break;
> +                       }
> +
> +                       uint32_t period = ctrl.second.get<int32_t>();
> +                       flickerState_.period = period * 1.0us;
> +
> +                       /*
> +                        * We note that it makes no difference if the mode
> gets set to "custom"
> +                        * first, and the period updated after, or vice
> versa.
> +                        */
> +                       if (flickerState_.mode == controls::FlickerCustom)
> +
>  agc->setFlickerPeriod(flickerState_.period);
> +
> +                       break;
> +               }
> +
>                 case controls::AWB_ENABLE: {
>                         RPiController::AwbAlgorithm *awb =
> dynamic_cast<RPiController::AwbAlgorithm *>(
>                                 controller_.getAlgorithm("awb"));
> --
> 2.30.2
>
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 5f7397e2..477d72ba 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -86,6 +86,8 @@  static const ControlInfoMap::Map ipaControls{
 	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
 	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
 	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
+	{ &controls::AeFlickerMode, ControlInfo(controls::AeFlickerModeValues) },
+	{ &controls::AeFlickerPeriod, ControlInfo(0, 1000000) },
 	{ &controls::AwbEnable, ControlInfo(false, true) },
 	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
 	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
@@ -218,6 +220,12 @@  private:
 
 	/* Maximum gain code for the sensor. */
 	uint32_t maxSensorGainCode_;
+
+	/* The current state of flicker avoidance. */
+	struct FlickerState {
+		int32_t mode;
+		Duration period;
+	} flickerState_;
 };
 
 int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
@@ -946,6 +954,78 @@  void IPARPi::queueRequest(const ControlList &controls)
 			break;
 		}
 
+		case controls::AE_FLICKER_MODE: {
+			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
+				controller_.getAlgorithm("agc"));
+			if (!agc) {
+				LOG(IPARPI, Warning)
+					<< "Could not set AE_FLICKER_MODE - no AGC algorithm";
+				break;
+			}
+
+			int32_t mode = ctrl.second.get<int32_t>();
+			bool modeValid = true;
+
+			switch (mode) {
+			case controls::FlickerOff: {
+				agc->setFlickerPeriod(0us);
+
+				break;
+			}
+
+			case controls::FlickerFreq50Hz: {
+				agc->setFlickerPeriod(10000 * 1.0us);
+
+				break;
+			}
+
+			case controls::FlickerFreq60Hz: {
+				agc->setFlickerPeriod(8333.333 * 1.0us);
+
+				break;
+			}
+
+			case controls::FlickerCustom: {
+				agc->setFlickerPeriod(flickerState_.period);
+
+				break;
+			}
+
+			default:
+				LOG(IPARPI, Error) << "Flicker mode " << mode << " is not supported";
+				modeValid = false;
+
+				break;
+			}
+
+			if (modeValid)
+				flickerState_.mode = mode;
+
+			break;
+		}
+
+		case controls::AE_FLICKER_PERIOD: {
+			RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
+				controller_.getAlgorithm("agc"));
+			if (!agc) {
+				LOG(IPARPI, Warning)
+					<< "Could not set AE_FLICKER_PERIOD - no AGC algorithm";
+				break;
+			}
+
+			uint32_t period = ctrl.second.get<int32_t>();
+			flickerState_.period = period * 1.0us;
+
+			/*
+			 * We note that it makes no difference if the mode gets set to "custom"
+			 * first, and the period updated after, or vice versa.
+			 */
+			if (flickerState_.mode == controls::FlickerCustom)
+				agc->setFlickerPeriod(flickerState_.period);
+
+			break;
+		}
+
 		case controls::AWB_ENABLE: {
 			RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
 				controller_.getAlgorithm("awb"));