[libcamera-devel] libcamera: ipa_interface: Make configure() return an int
diff mbox series

Message ID 20201201095942.7280-1-laurent.pinchart@ideasonboard.com
State Rejected
Headers show
Series
  • [libcamera-devel] libcamera: ipa_interface: Make configure() return an int
Related show

Commit Message

Laurent Pinchart Dec. 1, 2020, 9:59 a.m. UTC 
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@ideasonboard.com>
---
 .../libcamera/internal/ipa_context_wrapper.h  | 10 +++---
 include/libcamera/ipa/ipa_interface.h         | 22 ++++++------
 src/ipa/libipa/ipa_interface_wrapper.cpp      | 16 ++++-----
 src/ipa/libipa/ipa_interface_wrapper.h        | 12 +++----
 src/ipa/raspberrypi/raspberrypi.cpp           | 23 +++++++------
 src/ipa/rkisp1/rkisp1.cpp                     | 27 ++++++++-------
 src/ipa/vimc/vimc.cpp                         | 10 +++---
 src/libcamera/ipa_context_wrapper.cpp         | 17 +++++-----
 src/libcamera/ipa_interface.cpp               |  4 +++
 src/libcamera/proxy/ipa_proxy_linux.cpp       | 10 +++---
 src/libcamera/proxy/ipa_proxy_thread.cpp      | 24 ++++++-------
 test/ipa/ipa_wrappers_test.cpp                | 34 ++++++++++++-------
 12 files changed, 113 insertions(+), 96 deletions(-)

Comments

Naushir Patuck Dec. 1, 2020, 6:47 p.m. UTC | #1 
Hi Laurent,

Thank you for the patch.  This does exactly what I need!

On Tue, 1 Dec 2020 at 09:59, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> 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@ideasonboard.com>
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Tested-by: Naushir Patuck <naush@raspberrypi.com>

---
>  .../libcamera/internal/ipa_context_wrapper.h  | 10 +++---
>  include/libcamera/ipa/ipa_interface.h         | 22 ++++++------
>  src/ipa/libipa/ipa_interface_wrapper.cpp      | 16 ++++-----
>  src/ipa/libipa/ipa_interface_wrapper.h        | 12 +++----
>  src/ipa/raspberrypi/raspberrypi.cpp           | 23 +++++++------
>  src/ipa/rkisp1/rkisp1.cpp                     | 27 ++++++++-------
>  src/ipa/vimc/vimc.cpp                         | 10 +++---
>  src/libcamera/ipa_context_wrapper.cpp         | 17 +++++-----
>  src/libcamera/ipa_interface.cpp               |  4 +++
>  src/libcamera/proxy/ipa_proxy_linux.cpp       | 10 +++---
>  src/libcamera/proxy/ipa_proxy_thread.cpp      | 24 ++++++-------
>  test/ipa/ipa_wrappers_test.cpp                | 34 ++++++++++++-------
>  12 files changed, 113 insertions(+), 96 deletions(-)
>
> 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
> --
> Regards,
>
> Laurent Pinchart
>
>
Naushir Patuck Dec. 4, 2020, 10:17 a.m. UTC | #2 
Hi Laurent,

On Tue, 1 Dec 2020 at 18:47, Naushir Patuck <naush@raspberrypi.com> wrote:

> Hi Laurent,
>
> Thank you for the patch.  This does exactly what I need!
>
> On Tue, 1 Dec 2020 at 09:59, Laurent Pinchart <
> laurent.pinchart@ideasonboard.com> wrote:
>
>> 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@ideasonboard.com>
>>
>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: Naushir Patuck <naush@raspberrypi.com>
>

Did you reach any conclusion on whether this is the preferred approach to
use for this?

Naush



> ---
>>  .../libcamera/internal/ipa_context_wrapper.h  | 10 +++---
>>  include/libcamera/ipa/ipa_interface.h         | 22 ++++++------
>>  src/ipa/libipa/ipa_interface_wrapper.cpp      | 16 ++++-----
>>  src/ipa/libipa/ipa_interface_wrapper.h        | 12 +++----
>>  src/ipa/raspberrypi/raspberrypi.cpp           | 23 +++++++------
>>  src/ipa/rkisp1/rkisp1.cpp                     | 27 ++++++++-------
>>  src/ipa/vimc/vimc.cpp                         | 10 +++---
>>  src/libcamera/ipa_context_wrapper.cpp         | 17 +++++-----
>>  src/libcamera/ipa_interface.cpp               |  4 +++
>>  src/libcamera/proxy/ipa_proxy_linux.cpp       | 10 +++---
>>  src/libcamera/proxy/ipa_proxy_thread.cpp      | 24 ++++++-------
>>  test/ipa/ipa_wrappers_test.cpp                | 34 ++++++++++++-------
>>  12 files changed, 113 insertions(+), 96 deletions(-)
>>
>> 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
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>>
Paul Elder Dec. 7, 2020, 12:43 a.m. UTC | #3 
Hi Naush,

On Fri, Dec 04, 2020 at 10:17:44AM +0000, Naushir Patuck wrote:
> Hi Laurent,
> 
> On Tue, 1 Dec 2020 at 18:47, Naushir Patuck <naush@raspberrypi.com> wrote:
> 
>     Hi Laurent,
> 
>     Thank you for the patch.  This does exactly what I need!
> 
>     On Tue, 1 Dec 2020 at 09:59, Laurent Pinchart <
>     laurent.pinchart@ideasonboard.com> wrote:
> 
>         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@ideasonboard.com>
> 
> 
>     Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>     Tested-by: Naushir Patuck <naush@raspberrypi.com>
> 
> 
> Did you reach any conclusion on whether this is the preferred approach to use
> for this?

Yeah, your approach is better, and I've sent my rb tag for it.


Paul

>  
> 
>         ---
>          .../libcamera/internal/ipa_context_wrapper.h  | 10 +++---
>          include/libcamera/ipa/ipa_interface.h         | 22 ++++++------
>          src/ipa/libipa/ipa_interface_wrapper.cpp      | 16 ++++-----
>          src/ipa/libipa/ipa_interface_wrapper.h        | 12 +++----
>          src/ipa/raspberrypi/raspberrypi.cpp           | 23 +++++++------
>          src/ipa/rkisp1/rkisp1.cpp                     | 27 ++++++++-------
>          src/ipa/vimc/vimc.cpp                         | 10 +++---
>          src/libcamera/ipa_context_wrapper.cpp         | 17 +++++-----
>          src/libcamera/ipa_interface.cpp               |  4 +++
>          src/libcamera/proxy/ipa_proxy_linux.cpp       | 10 +++---
>          src/libcamera/proxy/ipa_proxy_thread.cpp      | 24 ++++++-------
>          test/ipa/ipa_wrappers_test.cpp                | 34 ++++++++++++-------
>          12 files changed, 113 insertions(+), 96 deletions(-)
> 
>         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
>         --
>         Regards,
> 
>         Laurent Pinchart
> 
>

Patch
diff mbox series 

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