[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Handle failures during IPA configuration

[Jacopo Mondi]

Hello Naush,

On Fri, Nov 27, 2020 at 01:30:31PM +0000, Naushir Patuck wrote:
> If the IPA fails during configuration, return an error flag to the
> pipeline handler and fail the use case gracefully.
>
> At preset, the IPA configuration can fail for the following reasons:
> - The sensor is not recognised, and fails to open a CamHelper object.
> - The pipeline handler did not pass in controls for the ISP and sensor.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

Looks good to me one minor nit apart. See below.

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> ---
>  include/libcamera/ipa/raspberrypi.h              |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp              | 16 ++++++++++++++--
>  .../pipeline/raspberrypi/raspberrypi.cpp         |  5 +++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 2b55dbfc..86c97ffa 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -21,6 +21,7 @@ enum ConfigParameters {
>  	IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>  	IPA_CONFIG_SENSOR = (1 << 2),
>  	IPA_CONFIG_DROP_FRAMES = (1 << 3),
> +	IPA_CONFIG_FAILED = (1 << 4),
>  };
>
>  enum Operations {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..7542dd26 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -197,8 +197,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		       const IPAOperationData &ipaConfig,
>  		       IPAOperationData *result)
>  {
> -	if (entityControls.empty())
> +	std::string cameraName(sensorInfo.model);
> +
> +	if (entityControls.size() != 2) {
> +		LOG(IPARPI, Error) << "No ISP or sensor controls found for "
> +				   << cameraName;

This will print the sensor name while the error is global to the
camera, as the ISP controls might be missing as well.

> +		result->operation = RPi::IPA_CONFIG_FAILED;
>  		return;
> +	}
>
>  	result->operation = 0;
>
> @@ -213,10 +219,16 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	 * that the kernel driver doesn't. We only do this the first time; we don't need
>  	 * to re-parse the metadata after a simple mode-switch for no reason.
>  	 */
> -	std::string cameraName(sensorInfo.model);
>  	if (!helper_) {
>  		helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
>
> +		if (!helper_) {
> +			LOG(IPARPI, Error) << "Could not create camera helper for "
> +					   << cameraName;
> +			result->operation = RPi::IPA_CONFIG_FAILED;
> +			return;
> +		}
> +
>  		/*
>  		 * Pass out the sensor config to the pipeline handler in order
>  		 * to setup the staggered writer class.
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 6fcdf557..76252806 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
>  			&result);
>
> +	if (result.operation & RPi::IPA_CONFIG_FAILED) {
> +		LOG(RPI, Error) << "IPA configuration failed!";
> +		return -EPIPE;
> +	}
> +
>  	unsigned int resultIdx = 0;
>  	if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
>  		/*
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel