[libcamera-devel,v2] ipa: rpi: Add hardware line rate constraints

19360 diff mbox series

Advertise hardware constraints on the pixel processing rate through the
Controller::HardwareConfig structure. When calculating the minimum line
length during a configure() operation, ensure that we don't exceed this
constraint.

If we do exceed the hardware constraints, increase the modes's minimum
line length so the pixel processing rate falls below the hardware limit.
If this is not possible, throw a loud error message in the logs.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp       | 27 +++++++++++++++++++++++++++
 src/ipa/rpi/controller/controller.cpp | 20 ++++++++++++++++++++
 src/ipa/rpi/controller/controller.h   |  2 ++
 3 files changed, 49 insertions(+)

Hi Naush,

Thank you for the patch.

On Thu, Jan 04, 2024 at 11:38:55AM +0000, Naushir Patuck via libcamera-devel wrote:
> Advertise hardware constraints on the pixel processing rate through the
> Controller::HardwareConfig structure. When calculating the minimum line
> length during a configure() operation, ensure that we don't exceed this
> constraint.
> 
> If we do exceed the hardware constraints, increase the modes's minimum
> line length so the pixel processing rate falls below the hardware limit.
> If this is not possible, throw a loud error message in the logs.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>

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

> ---
>  src/ipa/rpi/common/ipa_base.cpp       | 27 +++++++++++++++++++++++++++
>  src/ipa/rpi/controller/controller.cpp | 20 ++++++++++++++++++++
>  src/ipa/rpi/controller/controller.h   |  2 ++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 6ac9d5de2f88..6ec9157561cf 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -531,6 +531,33 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)
>  	mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);
>  	mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);
>  
> +	/*
> +	 * Ensure that the maximum pixel processing rate does not exceed the ISP
> +	 * hardware capabilities. If it does, try adjusting the minimum line
> +	 * length to compensate if possible.
> +	 */
> +	Duration minPixelTime = controller_.getHardwareConfig().minPixelProcessingTime;
> +	Duration pixelTime = mode_.minLineLength / mode_.width;
> +	if (minPixelTime && pixelTime < minPixelTime) {
> +		Duration adjustedLineLength = minPixelTime * mode_.width;
> +		if (adjustedLineLength <= mode_.maxLineLength) {
> +			LOG(IPARPI, Info)
> +				<< "Adjusting mode minimum line length from " << mode_.minLineLength
> +				<< " to " << adjustedLineLength << " because of ISP constraints.";
> +			mode_.minLineLength = adjustedLineLength;
> +		} else {
> +			LOG(IPARPI, Error)
> +				<< "Sensor minimum line length of " << pixelTime * mode_.width
> +				<< " (" << 1us / pixelTime << " MPix/s)"
> +				<< " is below the minimum allowable ISP limit of "
> +				<< adjustedLineLength
> +				<< " (" << 1us / minPixelTime << " MPix/s) ";
> +			LOG(IPARPI, Error)
> +				<< "THIS WILL CAUSE IMAGE CORRUPTION!!! "
> +				<< "Please update the camera sensor driver to allow more horizontal blanking control.";
> +		}
> +	}
> +
>  	/*
>  	 * Set the frame length limits for the mode to ensure exposure and
>  	 * framerate calculations are clipped appropriately.
> diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> index e62becd87e85..5ca98b989740 100644
> --- a/src/ipa/rpi/controller/controller.cpp
> +++ b/src/ipa/rpi/controller/controller.cpp
> @@ -17,6 +17,7 @@
>  
>  using namespace RPiController;
>  using namespace libcamera;
> +using namespace std::literals::chrono_literals;
>  
>  LOG_DEFINE_CATEGORY(RPiController)
>  
> @@ -37,6 +38,7 @@ static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
>  			.numGammaPoints = 33,
>  			.pipelineWidth = 13,
>  			.statsInline = false,
> +			.minPixelProcessingTime = 0s,
>  		}
>  	},
>  	{
> @@ -51,6 +53,24 @@ static const std::map<std::string, Controller::HardwareConfig> HardwareConfigMap
>  			.numGammaPoints = 64,
>  			.pipelineWidth = 16,
>  			.statsInline = true,
> +
> +			/*
> +			 * The constraint below is on the rate of pixels going
> +			 * from CSI2 peripheral to ISP-FE (400Mpix/s, plus tiny
> +			 * overheads per scanline, for which 380Mpix/s is a
> +			 * conservative bound).
> +			 *
> +			 * There is a 64kbit data FIFO before the bottleneck,
> +			 * which means that in all reasonable cases the
> +			 * constraint applies at a timescale >= 1 scanline, so
> +			 * adding horizontal blanking can prevent loss.
> +			 *
> +			 * If the backlog were to grow beyond 64kbit during a
> +			 * single scanline, there could still be loss. This
> +			 * could happen using 4 lanes at 1.5Gbps at 10bpp with
> +			 * frames wider than ~16,000 pixels.
> +			 */
> +			.minPixelProcessingTime = 1.0us / 380,
>  		}
>  	},
>  };
> diff --git a/src/ipa/rpi/controller/controller.h b/src/ipa/rpi/controller/controller.h
> index 6e5f595284fd..170aea740789 100644
> --- a/src/ipa/rpi/controller/controller.h
> +++ b/src/ipa/rpi/controller/controller.h
> @@ -15,6 +15,7 @@
>  #include <vector>
>  #include <string>
>  
> +#include <libcamera/base/utils.h>
>  #include "libcamera/internal/yaml_parser.h"
>  
>  #include "camera_mode.h"
> @@ -47,6 +48,7 @@ public:
>  		unsigned int numGammaPoints;
>  		unsigned int pipelineWidth;
>  		bool statsInline;
> +		libcamera::utils::Duration minPixelProcessingTime;
>  	};
>  
>  	Controller();