[libcamera-devel] [PATCH] libcamera: ipa_interface: Make configure() return an int
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 1 10:59:42 CET 2020
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 at 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(-)
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
More information about the libcamera-devel
mailing list