[libcamera-devel] [PATCH v1 2/9] ipa: raspberrypi: Add minimum and maximum line length fields to CameraMode

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Oct 03, 2022 at 09:39:28AM +0100, Naushir Patuck via libcamera-devel wrote:
> Add fields for minimum and maximum line length (in units of pixels) to the

I think the unit is seconds, not pixels.

> CameraMode structure. This replaces the existing lineLength field.
> 
> Any use of the existing lineLength field is replaced by the new minLineLength
> field, as logically we always want to use the fastest sensor readout by default.

I wonder if that's always the case, but I think it's fine here.

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp           |  8 ++++----
>  src/ipa/raspberrypi/controller/camera_mode.h |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp          | 13 +++++++------
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index cac8f39ee763..42251ba29682 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -64,13 +64,13 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats,
>  uint32_t CamHelper::exposureLines(const Duration exposure) const
>  {
>  	assert(initialized_);
> -	return exposure / mode_.lineLength;
> +	return exposure / mode_.minLineLength;
>  }
>  
>  Duration CamHelper::exposure(uint32_t exposureLines) const
>  {
>  	assert(initialized_);
> -	return exposureLines * mode_.lineLength;
> +	return exposureLines * mode_.minLineLength;
>  }
>  
>  uint32_t CamHelper::getVBlanking(Duration &exposure,
> @@ -86,8 +86,8 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,
>  	 * minFrameDuration and maxFrameDuration are clamped by the caller
>  	 * based on the limits for the active sensor mode.
>  	 */
> -	frameLengthMin = minFrameDuration / mode_.lineLength;
> -	frameLengthMax = maxFrameDuration / mode_.lineLength;
> +	frameLengthMin = minFrameDuration / mode_.minLineLength;
> +	frameLengthMax = maxFrameDuration / mode_.minLineLength;
>  
>  	/*
>  	 * Limit the exposure to the maximum frame duration requested, and
> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h
> index a6ccf8c1c600..b7e73aae4698 100644
> --- a/src/ipa/raspberrypi/controller/camera_mode.h
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> @@ -32,7 +32,7 @@ struct CameraMode {
>  	/* scaling of the noise compared to the native sensor mode */
>  	double noiseFactor;
>  	/* line time */
> -	libcamera::utils::Duration lineLength;
> +	libcamera::utils::Duration minLineLength, maxLineLength;

	libcamera::utils::Duration minLineLength;
	libcamera::utils::Duration maxLineLength;

would be more readable.

>  	/* any camera transform *not* reflected already in the camera tuning */
>  	libcamera::Transform transform;
>  	/* minimum and maximum fame lengths in units of lines */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 358a119da222..67326bcf4a14 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -314,7 +314,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  	}
>  
>  	startConfig->dropFrameCount = dropFrameCount_;
> -	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
> +	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;
>  	startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();
>  
>  	firstStart_ = false;
> @@ -356,7 +356,8 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
>  	 * Calculate the line length as the ratio between the line length in
>  	 * pixels and the pixel rate.
>  	 */
> -	mode_.lineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);
> +	mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);
> +	mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);
>  
>  	/*
>  	 * Set the frame length limits for the mode to ensure exposure and
> @@ -458,8 +459,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>  	 * based on the current sensor mode.
>  	 */
>  	ControlInfoMap::Map ctrlMap = ipaControls;
> -	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength;
> -	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
> +	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> +	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;
>  	ctrlMap[&controls::FrameDurationLimits] =
>  		ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
>  			    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> @@ -1150,8 +1151,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  
>  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)
>  {
> -	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength;
> -	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
> +	const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> +	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;
>  
>  	/*
>  	 * This will only be applied once AGC recalculations occur.

-- 
Regards,

Laurent Pinchart