<div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div>Thank you for your work.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 9 Mar 2021 at 02:39, Paul Elder <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Now that we support returning int directly in addition to other output<br>
parameters, improve the configure() function in the raspberrypi IPA<br>
interface.<br>
<br>
Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
---<br>
Changes in v2.1:<br>
- remove init() changes, keep configure changes<br>
---<br>
include/libcamera/ipa/raspberrypi.mojom | 2 +-<br>
src/ipa/raspberrypi/raspberrypi.cpp | 34 ++++++++-----------<br>
.../pipeline/raspberrypi/raspberrypi.cpp | 5 ++-<br>
3 files changed, 18 insertions(+), 23 deletions(-)<br>
<br>
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
index c3d614b8..eb427697 100644<br>
--- a/include/libcamera/ipa/raspberrypi.mojom<br>
+++ b/include/libcamera/ipa/raspberrypi.mojom<br>
@@ -77,7 +77,7 @@ interface IPARPiInterface {<br>
map<uint32, IPAStream> streamConfig,<br>
map<uint32, ControlInfoMap> entityControls,<br>
ConfigInput ipaConfig)<br>
- => (ConfigOutput results, int32 ret);<br>
+ => (int32 ret, ConfigOutput results);<br>
<br>
/**<br>
* \fn mapBuffers()<br>
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
index 85a2b846..7904225a 100644<br>
--- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
+++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
@@ -84,11 +84,11 @@ public:<br>
ipa::RPi::StartControls *result) override;<br>
void stop() override {}<br>
<br>
- void configure(const CameraSensorInfo &sensorInfo,<br>
- const std::map<unsigned int, IPAStream> &streamConfig,<br>
- const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
- const ipa::RPi::ConfigInput &data,<br>
- ipa::RPi::ConfigOutput *response, int32_t *ret) override;<br>
+ int configure(const CameraSensorInfo &sensorInfo,<br>
+ const std::map<unsigned int, IPAStream> &streamConfig,<br>
+ const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
+ const ipa::RPi::ConfigInput &data,<br>
+ ipa::RPi::ConfigOutput *response) override;<br></blockquote><div><br></div><div>Should the return value here be an int32_t type to be consistent with the mojom definition?</div><div>Apart from that,<br></div><div><br></div><div>Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
void mapBuffers(const std::vector<IPABuffer> &buffers) override;<br>
void unmapBuffers(const std::vector<unsigned int> &ids) override;<br>
void signalStatReady(const uint32_t bufferId) override;<br>
@@ -290,16 +290,15 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)<br>
mode_.max_frame_length = sensorInfo.maxFrameLength;<br>
}<br>
<br>
-void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
- [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,<br>
- const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
- const ipa::RPi::ConfigInput &ipaConfig,<br>
- ipa::RPi::ConfigOutput *result, int32_t *ret)<br>
+int IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
+ [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,<br>
+ const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
+ const ipa::RPi::ConfigInput &ipaConfig,<br>
+ ipa::RPi::ConfigOutput *result)<br>
{<br>
if (entityControls.size() != 2) {<br>
LOG(IPARPI, Error) << "No ISP or sensor controls found.";<br>
- *ret = -1;<br>
- return;<br>
+ return -1;<br>
}<br>
<br>
result->params = 0;<br>
@@ -309,14 +308,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
<br>
if (!validateSensorControls()) {<br>
LOG(IPARPI, Error) << "Sensor control validation failed.";<br>
- *ret = -1;<br>
- return;<br>
+ return -1;<br>
}<br>
<br>
if (!validateIspControls()) {<br>
LOG(IPARPI, Error) << "ISP control validation failed.";<br>
- *ret = -1;<br>
- return;<br>
+ return -1;<br>
}<br>
<br>
/* Setup a metadata ControlList to output metadata. */<br>
@@ -334,8 +331,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
if (!helper_) {<br>
LOG(IPARPI, Error) << "Could not create camera helper for "<br>
<< cameraName;<br>
- *ret = -1;<br>
- return;<br>
+ return -1;<br>
}<br>
<br>
/*<br>
@@ -403,7 +399,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
<br>
lastMode_ = mode_;<br>
<br>
- *ret = 0;<br>
+ return 0;<br>
}<br>
<br>
void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)<br>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
index 6387fae5..0f01ce8f 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
@@ -1250,9 +1250,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
/* Ready the IPA - it must know about the sensor resolution. */<br>
ipa::RPi::ConfigOutput result;<br>
<br>
- ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,<br>
- &result, &ret);<br>
-<br>
+ ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,<br>
+ &result);<br>
if (ret < 0) {<br>
LOG(RPI, Error) << "IPA configuration failed!";<br>
return -EPIPE;<br>
-- <br>
2.27.0<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>