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

[Dave Stevenson]

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

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