[libcamera-devel] [PATCH v2.1] ipa: raspberrypi: Use direct return value for configure()

[Naushir Patuck]

Hi Paul,

Thank you for your work.

On Tue, 9 Mar 2021 at 02:39, Paul Elder <paul.elder at ideasonboard.com> wrote:

> Now that we support returning int directly in addition to other output
> parameters, improve the configure() function in the raspberrypi IPA
> interface.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> ---
> Changes in v2.1:
> - remove init() changes, keep configure changes
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 34 ++++++++-----------
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  5 ++-
>  3 files changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> index c3d614b8..eb427697 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -77,7 +77,7 @@ interface IPARPiInterface {
>                   map<uint32, IPAStream> streamConfig,
>                   map<uint32, ControlInfoMap> entityControls,
>                   ConfigInput ipaConfig)
> -               => (ConfigOutput results, int32 ret);
> +               => (int32 ret, ConfigOutput results);
>
>         /**
>          * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 85a2b846..7904225a 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -84,11 +84,11 @@ public:
>                    ipa::RPi::StartControls *result) override;
>         void stop() override {}
>
> -       void configure(const CameraSensorInfo &sensorInfo,
> -                      const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                      const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> -                      const ipa::RPi::ConfigInput &data,
> -                      ipa::RPi::ConfigOutput *response, int32_t *ret)
> override;
> +       int configure(const CameraSensorInfo &sensorInfo,
> +                     const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                     const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> +                     const ipa::RPi::ConfigInput &data,
> +                     ipa::RPi::ConfigOutput *response) override;
>

Should the return value here be an int32_t type to be consistent with the
mojom definition?
Apart from that,

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


>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>         void signalStatReady(const uint32_t bufferId) override;
> @@ -290,16 +290,15 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
>         mode_.max_frame_length = sensorInfo.maxFrameLength;
>  }
>
> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> -                      [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> -                      const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> -                      const ipa::RPi::ConfigInput &ipaConfig,
> -                      ipa::RPi::ConfigOutput *result, int32_t *ret)
> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> +                     [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> +                     const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> +                     const ipa::RPi::ConfigInput &ipaConfig,
> +                     ipa::RPi::ConfigOutput *result)
>  {
>         if (entityControls.size() != 2) {
>                 LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> -               *ret = -1;
> -               return;
> +               return -1;
>         }
>
>         result->params = 0;
> @@ -309,14 +308,12 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>
>         if (!validateSensorControls()) {
>                 LOG(IPARPI, Error) << "Sensor control validation failed.";
> -               *ret = -1;
> -               return;
> +               return -1;
>         }
>
>         if (!validateIspControls()) {
>                 LOG(IPARPI, Error) << "ISP control validation failed.";
> -               *ret = -1;
> -               return;
> +               return -1;
>         }
>
>         /* Setup a metadata ControlList to output metadata. */
> @@ -334,8 +331,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>                 if (!helper_) {
>                         LOG(IPARPI, Error) << "Could not create camera
> helper for "
>                                            << cameraName;
> -                       *ret = -1;
> -                       return;
> +                       return -1;
>                 }
>
>                 /*
> @@ -403,7 +399,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>
>         lastMode_ = mode_;
>
> -       *ret = 0;
> +       return 0;
>  }
>
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 6387fae5..0f01ce8f 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1250,9 +1250,8 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
>         /* Ready the IPA - it must know about the sensor resolution. */
>         ipa::RPi::ConfigOutput result;
>
> -       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> -                       &result, &ret);
> -
> +       ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> +                             &result);
>         if (ret < 0) {
>                 LOG(RPI, Error) << "IPA configuration failed!";
>                 return -EPIPE;
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210309/5d4f5473/attachment-0001.htm>