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

[Laurent Pinchart]

On Fri, Nov 04, 2022 at 12:39:28PM +0000, Kieran Bingham via libcamera-devel wrote:
> 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>

The only case where I could imagine this happening is if the sensor
doesn't support raw formats. Supporting that would require lots of other
changes, so

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

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

-- 
Regards,

Laurent Pinchart