[libcamera-devel] [PATCH 3/4] ipa: rkisp1: Fail hard on empty CameraSensorInfo

[Kieran Bingham]

Quoting Jacopo Mondi via libcamera-devel (2022-11-04 10:50:03)
> The RkISP1 pipeline and IPA module allows for the CameraSensorInfo to be
> empty, probably to accommodate some sensor used in a test platform that
> does not provide the mandatory libcamera requirements.
> 
> As the \todo item in the IPA reports, there is a possibility that the
> received CameraSensorInfo is empty and it should be checked before
> accessing it, but currently such requirement is not enforced in the
> code.
> 
> This allows to assume all the test platforms in use have now
> successfully moved their sensor driver to comply with the minimum
> requirements and provide a populated CameraSensorInfo to the IPA.
> 
> As the safety check is not enforced, and as we don't want to allow
> faulty sensors to send empty CameraSensorInfo to the IPA, remove the
> \todo item in the IPA and fail hard in the pipeline handler if the
> sensor does not comply with libcamera requirements.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/ipa/rkisp1/rkisp1.cpp                | 6 ------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++---
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index e6e58feee6b0..e1ede62ead3a 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -198,12 +198,6 @@ void IPARkISP1::stop()
>         context_.frameContexts.clear();
>  }
>  
> -/**
> - * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> - * if the connected sensor does not provide enough information to properly
> - * assemble one. Make sure the reported sensor information are relevant
> - * before accessing them.
> - */
>  int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>                          [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig)
>  {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e345718bd305..023d1fd4744f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -718,9 +718,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  
>         ret = data->sensor_->sensorInfo(&ipaConfig.sensorInfo);
>         if (ret) {
> -               /* \todo Turn this into a hard failure. */
> -               LOG(RkISP1, Warning) << "Camera sensor information not available";
> -               ipaConfig.sensorInfo = {};
> +               LOG(RkISP1, Error) << "Camera sensor information not available";

I wonder if reporting the sensor name would help here, but as it wasn't
there before it's hardly a blocker.

I found myself hesitating on this patch at first, but I can't come up
with a reason why we shouldn't be more safe/pedantic here so:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +               return ret;
>         }
>  
>         ipaConfig.sensorControls = data->sensor_->controls();
> -- 
> 2.38.1
>