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

[Naushir Patuck]

HI Jacopo,

Thank you for the review.

On Fri, 27 Nov 2020 at 13:46, Jacopo Mondi <jacopo at jmondi.org> wrote:

> 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.
>

You are right, it is a bit pointless here.  I will remove and submit an
update after waiting a bit for any more feedback.

Regards,
Naush



>
> > +             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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201127/d14894bb/attachment.htm>