[libcamera-devel,v2,2/4] ipa: raspberrypi: Use the new sensor limits fields in CameraMode
diff mbox series

Message ID 20230327093439.24670-3-naush@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi: Sensor limits
Related show

Commit Message

Naushir Patuck March 27, 2023, 9:34 a.m. UTC 
Use the new analogue gain and shutter speed limit fields in the ipa
code when reporting back the control value limits and calculating the
analogue gain code to use. This also replaces the now unused (and
removed) maxSensorGainCode_ field.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 37 +++++++++--------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

Comments

David Plowman March 27, 2023, 1:23 p.m. UTC | #1 
Hi Naush

Thanks for the patch.

On Mon, 27 Mar 2023 at 10:34, Naushir Patuck via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Use the new analogue gain and shutter speed limit fields in the ipa
> code when reporting back the control value limits and calculating the
> analogue gain code to use. This also replaces the now unused (and
> removed) maxSensorGainCode_ field.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

LGTM.

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 37 +++++++++--------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3f1afb846420..c10e57e07ab0 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -224,9 +224,6 @@ private:
>         Duration minFrameDuration_;
>         Duration maxFrameDuration_;
>
> -       /* Maximum gain code for the sensor. */
> -       uint32_t maxSensorGainCode_;
> -
>         /* Track the frame length times over FrameLengthsQueueSize frames. */
>         std::deque<Duration> frameLengths_;
>         Duration lastTimeout_;
> @@ -455,8 +452,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>                 }
>         }
>
> -       maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();
> -
>         /* Setup a metadata ControlList to output metadata. */
>         libcameraMetadata_ = ControlList(controls::controls);
>
> @@ -517,26 +512,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>          * based on the current sensor mode.
>          */
>         ControlInfoMap::Map ctrlMap = ipaControls;
> -       const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> -       const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
>         ctrlMap[&controls::FrameDurationLimits] =
> -               ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> +               ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()),
> +                           static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()));
>
>         ctrlMap[&controls::AnalogueGain] =
> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
> -
> -       /*
> -        * Calculate the max exposure limit from the frame duration limit as V4L2
> -        * will limit the maximum control value based on the current VBLANK value.
> -        */
> -       Duration maxShutter = Duration::max();
> -       helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);
> -       const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> +               ControlInfo(static_cast<float>(mode_.minAnalogueGain),
> +                           static_cast<float>(mode_.maxAnalogueGain));
>
>         ctrlMap[&controls::ExposureTime] =
> -               ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()),
> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
> +               ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),
> +                           static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));
>
>         /* Declare Autofocus controls, only if we have a controllable lens */
>         if (lensPresent_)
> @@ -1496,9 +1482,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>
>  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)
>  {
> -       const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> -       const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> -
>         /*
>          * This will only be applied once AGC recalculations occur.
>          * The values may be clamped based on the sensor mode capabilities as well.
> @@ -1506,9 +1489,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
>         minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;
>         maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;
>         minFrameDuration_ = std::clamp(minFrameDuration_,
> -                                      minSensorFrameDuration, maxSensorFrameDuration);
> +                                      mode_.minFrameDuration, mode_.maxFrameDuration);
>         maxFrameDuration_ = std::clamp(maxFrameDuration_,
> -                                      minSensorFrameDuration, maxSensorFrameDuration);
> +                                      mode_.minFrameDuration, mode_.maxFrameDuration);
>         maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
>
>         /* Return the validated limits via metadata. */
> @@ -1531,6 +1514,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  {
> +       const int32_t minGainCode = helper_->gainCode(mode_.minAnalogueGain);
> +       const int32_t maxGainCode = helper_->gainCode(mode_.maxAnalogueGain);
>         int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);
>
>         /*
> @@ -1538,7 +1523,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>          * DelayedControls. The AGC will correctly handle a lower gain returned
>          * by the sensor, provided it knows the actual gain used.
>          */
> -       gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_);
> +       gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode);
>
>         /* getBlanking might clip exposure time to the fps limits. */
>         Duration exposure = agcStatus->shutterTime;
> --
> 2.34.1
>
Jacopo Mondi March 27, 2023, 1:52 p.m. UTC | #2 
Hi Naush

On Mon, Mar 27, 2023 at 10:34:37AM +0100, Naushir Patuck via libcamera-devel wrote:
> Use the new analogue gain and shutter speed limit fields in the ipa
> code when reporting back the control value limits and calculating the
> analogue gain code to use. This also replaces the now unused (and
> removed) maxSensorGainCode_ field.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  src/ipa/raspberrypi/raspberrypi.cpp | 37 +++++++++--------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3f1afb846420..c10e57e07ab0 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -224,9 +224,6 @@ private:
>  	Duration minFrameDuration_;
>  	Duration maxFrameDuration_;
>
> -	/* Maximum gain code for the sensor. */
> -	uint32_t maxSensorGainCode_;
> -
>  	/* Track the frame length times over FrameLengthsQueueSize frames. */
>  	std::deque<Duration> frameLengths_;
>  	Duration lastTimeout_;
> @@ -455,8 +452,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>  		}
>  	}
>
> -	maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();
> -
>  	/* Setup a metadata ControlList to output metadata. */
>  	libcameraMetadata_ = ControlList(controls::controls);
>
> @@ -517,26 +512,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>  	 * based on the current sensor mode.
>  	 */
>  	ControlInfoMap::Map ctrlMap = ipaControls;
> -	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> -	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
>  	ctrlMap[&controls::FrameDurationLimits] =
> -		ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> -			    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> +		ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()),
> +			    static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()));
>
>  	ctrlMap[&controls::AnalogueGain] =
> -		ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
> -
> -	/*
> -	 * Calculate the max exposure limit from the frame duration limit as V4L2
> -	 * will limit the maximum control value based on the current VBLANK value.
> -	 */
> -	Duration maxShutter = Duration::max();
> -	helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);
> -	const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> +		ControlInfo(static_cast<float>(mode_.minAnalogueGain),
> +			    static_cast<float>(mode_.maxAnalogueGain));
>
>  	ctrlMap[&controls::ExposureTime] =
> -		ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()),
> -			    static_cast<int32_t>(maxShutter.get<std::micro>()));
> +		ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),
> +			    static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));
>
>  	/* Declare Autofocus controls, only if we have a controllable lens */
>  	if (lensPresent_)
> @@ -1496,9 +1482,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>
>  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)
>  {
> -	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> -	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> -
>  	/*
>  	 * This will only be applied once AGC recalculations occur.
>  	 * The values may be clamped based on the sensor mode capabilities as well.
> @@ -1506,9 +1489,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
>  	minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;
>  	maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;
>  	minFrameDuration_ = std::clamp(minFrameDuration_,
> -				       minSensorFrameDuration, maxSensorFrameDuration);
> +				       mode_.minFrameDuration, mode_.maxFrameDuration);
>  	maxFrameDuration_ = std::clamp(maxFrameDuration_,
> -				       minSensorFrameDuration, maxSensorFrameDuration);
> +				       mode_.minFrameDuration, mode_.maxFrameDuration);
>  	maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
>
>  	/* Return the validated limits via metadata. */
> @@ -1531,6 +1514,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  {
> +	const int32_t minGainCode = helper_->gainCode(mode_.minAnalogueGain);
> +	const int32_t maxGainCode = helper_->gainCode(mode_.maxAnalogueGain);
>  	int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);
>
>  	/*
> @@ -1538,7 +1523,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  	 * DelayedControls. The AGC will correctly handle a lower gain returned
>  	 * by the sensor, provided it knows the actual gain used.
>  	 */
> -	gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_);
> +	gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode);
>
>  	/* getBlanking might clip exposure time to the fps limits. */
>  	Duration exposure = agcStatus->shutterTime;
> --
> 2.34.1
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 3f1afb846420..c10e57e07ab0 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -224,9 +224,6 @@  private:
 	Duration minFrameDuration_;
 	Duration maxFrameDuration_;
 
-	/* Maximum gain code for the sensor. */
-	uint32_t maxSensorGainCode_;
-
 	/* Track the frame length times over FrameLengthsQueueSize frames. */
 	std::deque<Duration> frameLengths_;
 	Duration lastTimeout_;
@@ -455,8 +452,6 @@  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
 		}
 	}
 
-	maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();
-
 	/* Setup a metadata ControlList to output metadata. */
 	libcameraMetadata_ = ControlList(controls::controls);
 
@@ -517,26 +512,17 @@  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
 	 * based on the current sensor mode.
 	 */
 	ControlInfoMap::Map ctrlMap = ipaControls;
-	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
-	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
 	ctrlMap[&controls::FrameDurationLimits] =
-		ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
-			    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
+		ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()),
+			    static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()));
 
 	ctrlMap[&controls::AnalogueGain] =
-		ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
-
-	/*
-	 * Calculate the max exposure limit from the frame duration limit as V4L2
-	 * will limit the maximum control value based on the current VBLANK value.
-	 */
-	Duration maxShutter = Duration::max();
-	helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);
-	const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
+		ControlInfo(static_cast<float>(mode_.minAnalogueGain),
+			    static_cast<float>(mode_.maxAnalogueGain));
 
 	ctrlMap[&controls::ExposureTime] =
-		ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()),
-			    static_cast<int32_t>(maxShutter.get<std::micro>()));
+		ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),
+			    static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));
 
 	/* Declare Autofocus controls, only if we have a controllable lens */
 	if (lensPresent_)
@@ -1496,9 +1482,6 @@  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 
 void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)
 {
-	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
-	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
-
 	/*
 	 * This will only be applied once AGC recalculations occur.
 	 * The values may be clamped based on the sensor mode capabilities as well.
@@ -1506,9 +1489,9 @@  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
 	minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;
 	maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;
 	minFrameDuration_ = std::clamp(minFrameDuration_,
-				       minSensorFrameDuration, maxSensorFrameDuration);
+				       mode_.minFrameDuration, mode_.maxFrameDuration);
 	maxFrameDuration_ = std::clamp(maxFrameDuration_,
-				       minSensorFrameDuration, maxSensorFrameDuration);
+				       mode_.minFrameDuration, mode_.maxFrameDuration);
 	maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
 
 	/* Return the validated limits via metadata. */
@@ -1531,6 +1514,8 @@  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
 
 void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 {
+	const int32_t minGainCode = helper_->gainCode(mode_.minAnalogueGain);
+	const int32_t maxGainCode = helper_->gainCode(mode_.maxAnalogueGain);
 	int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);
 
 	/*
@@ -1538,7 +1523,7 @@  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 	 * DelayedControls. The AGC will correctly handle a lower gain returned
 	 * by the sensor, provided it knows the actual gain used.
 	 */
-	gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_);
+	gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode);
 
 	/* getBlanking might clip exposure time to the fps limits. */
 	Duration exposure = agcStatus->shutterTime;