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

[Laurent Pinchart]

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 at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Nick Hollinghurst <nick.hollinghurst at raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at 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();

-- 
Regards,

Laurent Pinchart