[libcamera-devel] [PATCH 2/3] libcamera: ipa: Pass a set of controls and return results from ipa::start()

David Plowman david.plowman at raspberrypi.com
Tue Nov 17 17:34:54 CET 2020


Hi Naush

Thanks for the patch. Only some minor nitpicks...

On Thu, 12 Nov 2020 at 08:59, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> This change allows controls passed into pipeline_hanlder::start to be
> forwarded onto ipa::start(). We also add a return channel if the
> pipeline handle must action any of these controls, e.g. setting the
> analogue gain or shutter speed in the sensor device.

s/hanlder/handler/   (though strictly the class is PipelineHandler)

Is IPAContextWrapper::start more accurate, instead of ipa::start?

s/pipeline handle/pipeline handler/

(sorry!)

>
> todo: The IPA interface wrapper needs a translation for passing
> IPAOperationData into start() and configure()
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
>  include/libcamera/ipa/ipa_interface.h              |  3 ++-
>  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
>  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
>  src/ipa/vimc/vimc.cpp                              |  6 ++++--
>  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
>  src/libcamera/ipa_interface.cpp                    |  7 +++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
>  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
>  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
>  test/ipa/ipa_interface_test.cpp                    |  3 ++-
>  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
>  15 files changed, 49 insertions(+), 22 deletions(-)
>
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> index 8f767e84..1878a615 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -20,7 +20,8 @@ public:
>         ~IPAContextWrapper();
>
>         int init(const IPASettings &settings) override;
> -       int start() override;
> +       int start(const IPAOperationData &ipaConfig,
> +                 IPAOperationData *result) override;
>         void stop() override;
>         void configure(const CameraSensorInfo &sensorInfo,
>                        const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index 322b7079..b44e2538 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -153,7 +153,8 @@ public:
>         virtual ~IPAInterface() = default;
>
>         virtual int init(const IPASettings &settings) = 0;
> -       virtual int start() = 0;
> +       virtual int start(const IPAOperationData &ipaConfig,
> +                         IPAOperationData *result) = 0;
>         virtual void stop() = 0;
>
>         virtual void configure(const CameraSensorInfo &sensorInfo,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index cee532e3..fb2da9d3 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
>  {
>         IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
>
> -       return ctx->ipa_->start();
> +       /* \todo Translate the ipaConfig and result. */
> +       IPAOperationData ipaConfig;

In other places (though not always) you've tended to add "= {}" to the
ipaConfig object. As far as I can see, it's only the "operation" field
that might be uninitialised. Does this matter?

> +       return ctx->ipa_->start(ipaConfig, nullptr);
>  }
>
>  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index f338ff8b..7a07b477 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -77,7 +77,8 @@ public:
>         }
>
>         int init(const IPASettings &settings) override;
> -       int start() override { return 0; }
> +       int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +                 [[maybe_unused]] IPAOperationData *result) override { return 0; }
>         void stop() override {}
>
>         void configure(const CameraSensorInfo &sensorInfo,
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 07d7f1b2..0b8a9096 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -37,7 +37,8 @@ public:
>         {
>                 return 0;
>         }
> -       int start() override { return 0; }
> +       int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +                 [[maybe_unused]] IPAOperationData *result) override { return 0; }
>         void stop() override {}
>
>         void configure(const CameraSensorInfo &info,
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cf841135..ed26331d 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -34,7 +34,8 @@ public:
>
>         int init(const IPASettings &settings) override;
>
> -       int start() override;
> +       int start(const IPAOperationData &ipaConfig,
> +                 IPAOperationData *result) override;
>         void stop() override;
>
>         void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
>         return 0;
>  }
>
> -int IPAVimc::start()
> +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +                  [[maybe_unused]] IPAOperationData *result)
>  {
>         trace(IPAOperationStart);
>
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 231300ce..de96a606 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings &settings)
>         return 0;
>  }
>
> -int IPAContextWrapper::start()
> +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
> +                            IPAOperationData *result)
>  {
>         if (intf_)
> -               return intf_->start();
> +               return intf_->start(ipaConfig, result);
>
>         if (!ctx_)
>                 return 0;
>
> +       /* \todo Translate the ipaConfig and reponse */
>         return ctx_->ops->start(ctx_);
>  }

s/reponse/response/

>
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 23fc56d7..282c3c0f 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -536,10 +536,17 @@ namespace libcamera {
>  /**
>   * \fn IPAInterface::start()
>   * \brief Start the IPA
> + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> + * \param[out] result Pipeline-handler-specific configuration result
>   *
>   * This method informs the IPA module that the camera is about to be started.
>   * The IPA module shall prepare any resources it needs to operate.
>   *
> + * The \a ipaConfig and \a result parameters carry custom data passed by the
> + * 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 otherwise
>   */
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index ddb30e49..a8e4997a 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>         }
>
>         /* Start the IPA. */
> -       ret = data->ipa_->start();
> +       IPAOperationData ipaConfig, result;

Another IPAOperationData with undefined "operation" field. Does it matter?

> +       ipaConfig.controls.emplace_back(*controls);
> +       ret = data->ipa_->start(ipaConfig, &result);
>         if (ret) {
>                 LOG(RPI, Error)
>                         << "Failed to start IPA for " << camera->id();
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2e8d2930..a6c82a48 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>         if (ret)
>                 return ret;
>
> -       ret = data->ipa_->start();
> +       IPAOperationData ipaConfig = {};
> +       ret = data->ipa_->start(ipaConfig, nullptr);
>         if (ret) {
>                 freeBuffers(camera);
>                 LOG(RkISP1, Error)
> @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>         std::map<unsigned int, const ControlInfoMap &> entityControls;
>         entityControls.emplace(0, data->sensor_->controls());
>
> -       IPAOperationData ipaConfig;
> +       ipaConfig = {};
>         data->ipa_->configure(sensorInfo, streamConfig, entityControls,
>                               ipaConfig, nullptr);
>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index d81b8598..b4773cf5 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *con
>         if (ret < 0)
>                 return ret;
>
> -       ret = data->ipa_->start();
> +       IPAOperationData ipaConfig = {};
> +       ret = data->ipa_->start(ipaConfig, nullptr);
>         if (ret) {
>                 data->video_->releaseBuffers();
>                 return ret;
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index b78a0e45..619976e5 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -30,7 +30,8 @@ public:
>         {
>                 return 0;
>         }
> -       int start() override { return 0; }
> +       int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +                 [[maybe_unused]] IPAOperationData *result) override { return 0; }
>         void stop() override {}
>         void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
>                        [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index eead2883..9a81b6e7 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -26,7 +26,8 @@ public:
>         IPAProxyThread(IPAModule *ipam);
>
>         int init(const IPASettings &settings) override;
> -       int start() override;
> +       int start(const IPAOperationData &ipaConfig,
> +                 IPAOperationData *result) override;
>         void stop() override;
>
>         void configure(const CameraSensorInfo &sensorInfo,
> @@ -50,9 +51,9 @@ private:
>                         ipa_ = ipa;
>                 }
>
> -               int start()
> +               int start(const IPAOperationData &ipaConfig, IPAOperationData *result)
>                 {
> -                       return ipa_->start();
> +                       return ipa_->start(ipaConfig, result);
>                 }
>
>                 void stop()
> @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings &settings)
>         return 0;
>  }
>
> -int IPAProxyThread::start()
> +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
> +                         IPAOperationData *result)
>  {
>         running_ = true;
>         thread_.start();
>
> -       return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> +       return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking,
> +                                  ipaConfig, result);
>  }
>
>  void IPAProxyThread::stop()
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 67488409..6222938f 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -120,7 +120,8 @@ protected:
>                 }
>
>                 /* Test start of IPA module. */
> -               ipa_->start();
> +               IPAOperationData ipaConfig;

Another uninitialised "operation" field. Does it matter? Should we
make them all look the same?

Other than these nitpicks:

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
Tested-by: David Plowman <david.plowman at raspberrypi.com>

Best regards
David

> +               ipa_->start(ipaConfig, nullptr);
>                 timer.start(1000);
>                 while (timer.isRunning() && trace_ != IPAOperationStart)
>                         dispatcher->processEvents();
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index 59d991cb..7b8ae77b 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -56,7 +56,8 @@ public:
>                 return 0;
>         }
>
> -       int start() override
> +       int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +                 [[maybe_unused]] IPAOperationData *result) override
>         {
>                 report(Op_start, TestPass);
>                 return 0;
> @@ -376,7 +377,7 @@ protected:
>                         return TestFail;
>                 }
>
> -               ret = INVOKE(start);
> +               ret = INVOKE(start, ipaConfig, nullptr);
>                 if (ret == TestFail) {
>                         cerr << "Failed to run start()";
>                         return TestFail;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel


More information about the libcamera-devel mailing list