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

Kieran Bingham kieran.bingham at ideasonboard.com
Mon Dec 7 16:14:16 CET 2020


Hi Naush, Paul,

On 07/12/2020 13:11, Naushir Patuck wrote:
> Hi Paul,
> 
> Thank you for your review.
> 
> On Fri, 4 Dec 2020 at 10:39, <paul.elder at ideasonboard.com
> <mailto:paul.elder at ideasonboard.com>> wrote:
> 
>     Hi Naush,
> 
>     On Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:
>     > Hi Naush,
>     >
>     > Thank you for the patch.
>     >
>     > On Fri, Nov 27, 2020 at 02:56:12PM +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 present, 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
>     <mailto:naush at raspberrypi.com>>
>     > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org
>     <mailto:jacopo at jmondi.org>>
>     >
>     > Wouldn't it be simpler to modify the configure() function to return an
>     > int ? How about the following ? If it works for you I'll submit a
>     proper
>     > patch.
> 
>     From the perspective of the IPC changes, Naush's solution is more easily
>     translatable. When the IPA interface will become customizable, there
>     isn't a way to specify which output parameters should be return values
>     vs return parameters. Thus keeping them all as return parameters is the
>     better solution here.
> 
>     So imo this is fine to merge as-is.
> 
>     Reviewed-by: Paul Elder <paul.elder at ideasonboard.com
>     <mailto:paul.elder at ideasonboard.com>>
> 
> 
> In which case, I think this is ready to be merged :)

Pushed.

Regards
--
Kieran


> Regards,
> Naush
>  
> 
> 
>     > Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com
>     <mailto:laurent.pinchart at ideasonboard.com>>
>     > Date:   Tue Dec 1 03:54:18 2020 +0200
>     >
>     >     libcamera: ipa_interface: Make configure() return an int
>     >
>     >     It's useful for the IPAInterface::configure() function to be
>     able to
>     >     return a status. Make it return an int, to avoid forcing IPAs
>     to return
>     >     a status encoded in the IPAOperationData in a custom way.
>     >
>     >     Signed-off-by: Laurent Pinchart
>     <laurent.pinchart at ideasonboard.com
>     <mailto:laurent.pinchart at ideasonboard.com>>
>     >
>     > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
>     b/include/libcamera/internal/ipa_context_wrapper.h
>     > index 8f767e844221..a00b5e7b92eb 100644
>     > --- a/include/libcamera/internal/ipa_context_wrapper.h
>     > +++ b/include/libcamera/internal/ipa_context_wrapper.h
>     > @@ -22,11 +22,11 @@ public:
>     >       int init(const IPASettings &settings) override;
>     >       int start() override;
>     >       void stop() override;
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *result) override;
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *result) override;
>     >
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     > diff --git a/include/libcamera/ipa/ipa_interface.h
>     b/include/libcamera/ipa/ipa_interface.h
>     > index 322b7079070e..1d8cf8dc1c56 100644
>     > --- a/include/libcamera/ipa/ipa_interface.h
>     > +++ b/include/libcamera/ipa/ipa_interface.h
>     > @@ -95,12 +95,12 @@ struct ipa_context_ops {
>     >       void (*register_callbacks)(struct ipa_context *ctx,
>     >                                  const struct ipa_callback_ops
>     *callbacks,
>     >                                  void *cb_ctx);
>     > -     void (*configure)(struct ipa_context *ctx,
>     > -                       const struct ipa_sensor_info *sensor_info,
>     > -                       const struct ipa_stream *streams,
>     > -                       unsigned int num_streams,
>     > -                       const struct ipa_control_info_map *maps,
>     > -                       unsigned int num_maps);
>     > +     int (*configure)(struct ipa_context *ctx,
>     > +                      const struct ipa_sensor_info *sensor_info,
>     > +                      const struct ipa_stream *streams,
>     > +                      unsigned int num_streams,
>     > +                      const struct ipa_control_info_map *maps,
>     > +                      unsigned int num_maps);
>     >       void (*map_buffers)(struct ipa_context *ctx,
>     >                           const struct ipa_buffer *buffers,
>     >                           size_t num_buffers);
>     > @@ -156,11 +156,11 @@ public:
>     >       virtual int start() = 0;
>     >       virtual void stop() = 0;
>     >
>     > -     virtual void configure(const CameraSensorInfo &sensorInfo,
>     > -                            const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                            const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                            const IPAOperationData &ipaConfig,
>     > -                            IPAOperationData *result) = 0;
>     > +     virtual int configure(const CameraSensorInfo &sensorInfo,
>     > +                           const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                           const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                           const IPAOperationData &ipaConfig,
>     > +                           IPAOperationData *result) = 0;
>     >
>     >       virtual void mapBuffers(const std::vector<IPABuffer>
>     &buffers) = 0;
>     >       virtual void unmapBuffers(const std::vector<unsigned int>
>     &ids) = 0;
>     > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
>     b/src/ipa/libipa/ipa_interface_wrapper.cpp
>     > index cee532e3a747..74d7bc6e1c9b 100644
>     > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
>     > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
>     > @@ -115,12 +115,12 @@ void
>     IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
>     >       ctx->cb_ctx_ = cb_ctx;
>     >  }
>     >
>     > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>     > -                                 const struct ipa_sensor_info
>     *sensor_info,
>     > -                                 const struct ipa_stream *streams,
>     > -                                 unsigned int num_streams,
>     > -                                 const struct
>     ipa_control_info_map *maps,
>     > -                                 unsigned int num_maps)
>     > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>     > +                                const struct ipa_sensor_info
>     *sensor_info,
>     > +                                const struct ipa_stream *streams,
>     > +                                unsigned int num_streams,
>     > +                                const struct ipa_control_info_map
>     *maps,
>     > +                                unsigned int num_maps)
>     >  {
>     >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
>     *>(_ctx);
>     >
>     > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct
>     ipa_context *_ctx,
>     >
>     >       /* \todo Translate the ipaConfig and result. */
>     >       IPAOperationData ipaConfig;
>     > -     ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
>     ipaConfig,
>     > -                          nullptr);
>     > +     return ctx->ipa_->configure(sensorInfo, ipaStreams,
>     entityControls,
>     > +                                 ipaConfig, nullptr);
>     >  }
>     >
>     >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
>     > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h
>     b/src/ipa/libipa/ipa_interface_wrapper.h
>     > index a1c701599b56..acd3160039b1 100644
>     > --- a/src/ipa/libipa/ipa_interface_wrapper.h
>     > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
>     > @@ -30,12 +30,12 @@ private:
>     >       static void register_callbacks(struct ipa_context *ctx,
>     >                                      const struct ipa_callback_ops
>     *callbacks,
>     >                                      void *cb_ctx);
>     > -     static void configure(struct ipa_context *ctx,
>     > -                           const struct ipa_sensor_info *sensor_info,
>     > -                           const struct ipa_stream *streams,
>     > -                           unsigned int num_streams,
>     > -                           const struct ipa_control_info_map *maps,
>     > -                           unsigned int num_maps);
>     > +     static int configure(struct ipa_context *ctx,
>     > +                          const struct ipa_sensor_info *sensor_info,
>     > +                          const struct ipa_stream *streams,
>     > +                          unsigned int num_streams,
>     > +                          const struct ipa_control_info_map *maps,
>     > +                          unsigned int num_maps);
>     >       static void map_buffers(struct ipa_context *ctx,
>     >                               const struct ipa_buffer *c_buffers,
>     >                               size_t num_buffers);
>     > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>     b/src/ipa/raspberrypi/raspberrypi.cpp
>     > index 9853a343c892..75f7af3430ef 100644
>     > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     > @@ -81,11 +81,11 @@ public:
>     >       int start() override { return 0; }
>     >       void stop() override {}
>     >
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &data,
>     > -                    IPAOperationData *response) override;
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &data,
>     > +                   IPAOperationData *response) override;
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     >       void processEvent(const IPAOperationData &event) override;
>     > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo
>     &sensorInfo)
>     >       mode_.line_length = 1e9 * sensorInfo.lineLength /
>     sensorInfo.pixelRate;
>     >  }
>     >
>     > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *result)
>     > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *result)
>     >  {
>     >       if (entityControls.empty())
>     > -             return;
>     > +             return -EINVAL;
>     >
>     >       result->operation = 0;
>     >
>     > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo
>     &sensorInfo,
>     >       }
>     >
>     >       lastMode_ = mode_;
>     > +     return 0;
>     >  }
>     >
>     >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
>     > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>     > index 07d7f1b2ddd8..78d78c5ac521 100644
>     > --- a/src/ipa/rkisp1/rkisp1.cpp
>     > +++ b/src/ipa/rkisp1/rkisp1.cpp
>     > @@ -40,11 +40,11 @@ public:
>     >       int start() override { return 0; }
>     >       void stop() override {}
>     >
>     > -     void configure(const CameraSensorInfo &info,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *response) override;
>     > +     int configure(const CameraSensorInfo &info,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *response) override;
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     >       void processEvent(const IPAOperationData &event) override;
>     > @@ -79,27 +79,27 @@ private:
>     >   * assemble one. Make sure the reported sensor information are
>     relevant
>     >   * before accessing them.
>     >   */
>     > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo
>     &info,
>     > -                       [[maybe_unused]] const std::map<unsigned
>     int, IPAStream> &streamConfig,
>     > -                       const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                       [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                       [[maybe_unused]] IPAOperationData *result)
>     > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo
>     &info,
>     > +                      [[maybe_unused]] const std::map<unsigned
>     int, IPAStream> &streamConfig,
>     > +                      const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                      [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                      [[maybe_unused]] IPAOperationData *result)
>     >  {
>     >       if (entityControls.empty())
>     > -             return;
>     > +             return -EINVAL;
>     >
>     >       ctrls_ = entityControls.at(0);
>     >
>     >       const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>     >       if (itExp == ctrls_.end()) {
>     >               LOG(IPARkISP1, Error) << "Can't find exposure control";
>     > -             return;
>     > +             return -EINVAL;
>     >       }
>     >
>     >       const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>     >       if (itGain == ctrls_.end()) {
>     >               LOG(IPARkISP1, Error) << "Can't find gain control";
>     > -             return;
>     > +             return -EINVAL;
>     >       }
>     >
>     >       autoExposure_ = true;
>     > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]]
>     const CameraSensorInfo &info,
>     >               << " Gain: " << minGain_ << "-" << maxGain_;
>     >
>     >       setControls(0);
>     > +     return 0;
>     >  }
>     >
>     >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>     > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
>     > index cf8411359e40..79c8c2fb8927 100644
>     > --- a/src/ipa/vimc/vimc.cpp
>     > +++ b/src/ipa/vimc/vimc.cpp
>     > @@ -37,11 +37,11 @@ public:
>     >       int start() override;
>     >       void stop() override;
>     >
>     > -     void configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > -                    [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                    [[maybe_unused]] IPAOperationData *result)
>     override {}
>     > +     int configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > +                   [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                   [[maybe_unused]] IPAOperationData *result)
>     override { return 0; }
>     >       void mapBuffers([[maybe_unused]] const
>     std::vector<IPABuffer> &buffers) override {}
>     >       void unmapBuffers([[maybe_unused]] const
>     std::vector<unsigned int> &ids) override {}
>     >       void processEvent([[maybe_unused]] const IPAOperationData
>     &event) override {}
>     > diff --git a/src/libcamera/ipa_context_wrapper.cpp
>     b/src/libcamera/ipa_context_wrapper.cpp
>     > index 231300ce0bec..f63ad830c003 100644
>     > --- a/src/libcamera/ipa_context_wrapper.cpp
>     > +++ b/src/libcamera/ipa_context_wrapper.cpp
>     > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
>     >       ctx_->ops->stop(ctx_);
>     >  }
>     >
>     > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>     > -                               const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                               const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                               const IPAOperationData &ipaConfig,
>     > -                               IPAOperationData *result)
>     > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>     > +                              const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                              const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                              const IPAOperationData &ipaConfig,
>     > +                              IPAOperationData *result)
>     >  {
>     >       if (intf_)
>     >               return intf_->configure(sensorInfo, streamConfig,
>     >                                       entityControls, ipaConfig,
>     result);
>     >
>     >       if (!ctx_)
>     > -             return;
>     > +             return 0;
>     >
>     >       serializer_.reset();
>     >
>     > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const
>     CameraSensorInfo &sensorInfo,
>     >       }
>     >
>     >       /* \todo Translate the ipaConfig and reponse */
>     > -     ctx_->ops->configure(ctx_, &sensor_info, c_streams,
>     streamConfig.size(),
>     > -                          c_info_maps, entityControls.size());
>     > +     return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
>     > +                                 streamConfig.size(), c_info_maps,
>     > +                                 entityControls.size());
>     >  }
>     >
>     >  void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer>
>     &buffers)
>     > diff --git a/src/libcamera/ipa_interface.cpp
>     b/src/libcamera/ipa_interface.cpp
>     > index 23fc56d7d48e..516a8ecd4b53 100644
>     > --- a/src/libcamera/ipa_interface.cpp
>     > +++ b/src/libcamera/ipa_interface.cpp
>     > @@ -347,6 +347,8 @@
>     >   * \param[in] num_maps The number of entries in the \a maps array
>     >   *
>     >   * \sa libcamera::IPAInterface::configure()
>     > + *
>     > + * \return 0 on success or a negative error code on failure
>     >   */
>     >
>     >  /**
>     > @@ -573,6 +575,8 @@ namespace libcamera {
>     >   * pipeline handler to the IPA and back. The pipeline handler may
>     set the \a
>     >   * result parameter to null if the IPA protocol doesn't need to
>     pass a result
>     >   * back through the configure() function.
>     > + *
>     > + * \return 0 on success or a negative error code on failure
>     >   */
>     >
>     >  /**
>     > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
>     b/src/libcamera/proxy/ipa_proxy_linux.cpp
>     > index b78a0e4535f5..ed250ce79c17 100644
>     > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
>     > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
>     > @@ -32,11 +32,11 @@ public:
>     >       }
>     >       int start() override { return 0; }
>     >       void stop() override {}
>     > -     void configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > -                    [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                    [[maybe_unused]] IPAOperationData *result)
>     override {}
>     > +     int configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > +                   [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                   [[maybe_unused]] IPAOperationData *result)
>     override { return 0; }
>     >       void mapBuffers([[maybe_unused]] const
>     std::vector<IPABuffer> &buffers) override {}
>     >       void unmapBuffers([[maybe_unused]] const
>     std::vector<unsigned int> &ids) override {}
>     >       void processEvent([[maybe_unused]] const IPAOperationData
>     &event) override {}
>     > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
>     b/src/libcamera/proxy/ipa_proxy_thread.cpp
>     > index eead2883708d..fd91726c4840 100644
>     > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
>     > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
>     > @@ -29,11 +29,11 @@ public:
>     >       int start() override;
>     >       void stop() override;
>     >
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *result) override;
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *result) override;
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     >       void processEvent(const IPAOperationData &event) override;
>     > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
>     >       thread_.wait();
>     >  }
>     >
>     > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>     > -                            const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                            const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                            const IPAOperationData &ipaConfig,
>     > -                            IPAOperationData *result)
>     > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>     > +                           const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                           const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                           const IPAOperationData &ipaConfig,
>     > +                           IPAOperationData *result)
>     >  {
>     > -     ipa_->configure(sensorInfo, streamConfig, entityControls,
>     ipaConfig,
>     > -                     result);
>     > +     return ipa_->configure(sensorInfo, streamConfig, entityControls,
>     > +                            ipaConfig, result);
>     >  }
>     >
>     >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer>
>     &buffers)
>     > diff --git a/test/ipa/ipa_wrappers_test.cpp
>     b/test/ipa/ipa_wrappers_test.cpp
>     > index 59d991cbbf6a..ad0fb0386865 100644
>     > --- a/test/ipa/ipa_wrappers_test.cpp
>     > +++ b/test/ipa/ipa_wrappers_test.cpp
>     > @@ -67,55 +67,63 @@ public:
>     >               report(Op_stop, TestPass);
>     >       }
>     >
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                    [[maybe_unused]] IPAOperationData *result)
>     override
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                   [[maybe_unused]] IPAOperationData *result)
>     override
>     >       {
>     >               /* Verify sensorInfo. */
>     >               if (sensorInfo.outputSize.width != 2560 ||
>     >                   sensorInfo.outputSize.height != 1940) {
>     >                       cerr << "configure(): Invalid sensor info size "
>     >                            << sensorInfo.outputSize.toString();
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               /* Verify streamConfig. */
>     >               if (streamConfig.size() != 2) {
>     >                       cerr << "configure(): Invalid number of
>     streams "
>     >                            << streamConfig.size() << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               auto iter = streamConfig.find(1);
>     >               if (iter == streamConfig.end()) {
>     >                       cerr << "configure(): No configuration for
>     stream 1" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >               const IPAStream *stream = &iter->second;
>     >               if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
>     >                   stream->size != Size{ 1024, 768 }) {
>     >                       cerr << "configure(): Invalid configuration
>     for stream 1" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               iter = streamConfig.find(2);
>     >               if (iter == streamConfig.end()) {
>     >                       cerr << "configure(): No configuration for
>     stream 2" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >               stream = &iter->second;
>     >               if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
>     >                   stream->size != Size{ 800, 600 }) {
>     >                       cerr << "configure(): Invalid configuration
>     for stream 2" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               /* Verify entityControls. */
>     >               auto ctrlIter = entityControls.find(42);
>     >               if (ctrlIter == entityControls.end()) {
>     >                       cerr << "configure(): Controls not found" <<
>     endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               const ControlInfoMap &infoMap = ctrlIter->second;
>     > @@ -124,10 +132,12 @@ public:
>     >                   infoMap.count(V4L2_CID_CONTRAST) != 1 ||
>     >                   infoMap.count(V4L2_CID_SATURATION) != 1) {
>     >                       cerr << "configure(): Invalid control IDs"
>     << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               report(Op_configure, TestPass);
>     > +             return 0;
>     >       }
>     >
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override
>     >
>     > > ---
>     > >  include/libcamera/ipa/raspberrypi.h                |  1 +
>     > >  src/ipa/raspberrypi/raspberrypi.cpp                | 12
>     +++++++++++-
>     > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
>     > >  3 files changed, 17 insertions(+), 1 deletion(-)
>     > >
>     > > 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..57dd9c61 100644
>     > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     > > @@ -197,8 +197,11 @@ void IPARPi::configure(const
>     CameraSensorInfo &sensorInfo,
>     > >                    const IPAOperationData &ipaConfig,
>     > >                    IPAOperationData *result)
>     > >  {
>     > > -   if (entityControls.empty())
>     > > +   if (entityControls.size() != 2) {
>     > > +           LOG(IPARPI, Error) << "No ISP or sensor controls
>     found.";
>     > > +           result->operation = RPi::IPA_CONFIG_FAILED;
>     > >             return;
>     > > +   }
>     > > 
>     > >     result->operation = 0;
>     > > 
>     > > @@ -217,6 +220,13 @@ void IPARPi::configure(const
>     CameraSensorInfo &sensorInfo,
>     > >     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) {
>     > >             /*
> 
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran


More information about the libcamera-devel mailing list