[libcamera-devel,v2.1] ipa: raspberrypi: Use direct return value for configure()
diff mbox series

Message ID 20210309023907.83798-1-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2.1] ipa: raspberrypi: Use direct return value for configure()
Related show

Commit Message

Paul Elder March 9, 2021, 2:39 a.m. UTC 
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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

Comments

Naushir Patuck March 9, 2021, 7:28 a.m. UTC | #1 
Hi Paul,

Thank you for your work.

On Tue, 9 Mar 2021 at 02:39, Paul Elder <paul.elder@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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

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