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

[Naushir Patuck]

Hi Dave,

Thanks for the review.

On Fri, 27 Nov 2020 at 14:21, Dave Stevenson <dave.stevenson at raspberrypi.com>
wrote:

> Hi Naush
>
> On Fri, 27 Nov 2020 at 13:52, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > 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:
>
> s/preset/present
>

Ack.  Will post an update shortly.

Regards,
Naush


>
> >> > - 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
> >
> > _______________________________________________
> > 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/d8b0221e/attachment.htm>