From patchwork Tue Dec 1 09:59:42 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 10525 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 3E9E3BE177 for ; Tue, 1 Dec 2020 09:59:57 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C69EE634A1; Tue, 1 Dec 2020 10:59:56 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Ji0p6Zjo"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 7DF7D6032B for ; Tue, 1 Dec 2020 10:59:55 +0100 (CET) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id CF072538; Tue, 1 Dec 2020 10:59:54 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1606816795; bh=xfAEkFKUZrOV1uL+f5kJ4Ism49BJpN2arCWIhBFzKOc=; h=From:To:Cc:Subject:Date:From; b=Ji0p6Zjow74KN5H2psGFb3xTuaCsbhx4hXTDx5RgorvF7KqxOWbIv6ciCBdO2BLvk qI2EFCjzy4MHjqzw2D3VgRg4aopBp/gaSGhspY2VZHGzY5/uCCpyR/4iBNRe2kokLJ BQApfJBiPL5wRCfSmdOfDkie0k4aUAeswOnZIQ6Y= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Tue, 1 Dec 2020 11:59:42 +0200 Message-Id: <20201201095942.7280-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH] libcamera: ipa_interface: Make configure() return an int X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 Reviewed-by: Naushir Patuck Tested-by: Naushir Patuck --- .../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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &ipaConfig, - IPAOperationData *result) override; + int configure(const CameraSensorInfo &sensorInfo, + const std::map &streamConfig, + const std::map &entityControls, + const IPAOperationData &ipaConfig, + IPAOperationData *result) override; void mapBuffers(const std::vector &buffers) override; void unmapBuffers(const std::vector &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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &ipaConfig, - IPAOperationData *result) = 0; + virtual int configure(const CameraSensorInfo &sensorInfo, + const std::map &streamConfig, + const std::map &entityControls, + const IPAOperationData &ipaConfig, + IPAOperationData *result) = 0; virtual void mapBuffers(const std::vector &buffers) = 0; virtual void unmapBuffers(const std::vector &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(_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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &data, - IPAOperationData *response) override; + int configure(const CameraSensorInfo &sensorInfo, + const std::map &streamConfig, + const std::map &entityControls, + const IPAOperationData &data, + IPAOperationData *response) override; void mapBuffers(const std::vector &buffers) override; void unmapBuffers(const std::vector &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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &ipaConfig, - IPAOperationData *result) +int IPARPi::configure(const CameraSensorInfo &sensorInfo, + [[maybe_unused]] const std::map &streamConfig, + const std::map &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 &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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &ipaConfig, - IPAOperationData *response) override; + int configure(const CameraSensorInfo &info, + const std::map &streamConfig, + const std::map &entityControls, + const IPAOperationData &ipaConfig, + IPAOperationData *response) override; void mapBuffers(const std::vector &buffers) override; void unmapBuffers(const std::vector &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 &streamConfig, - const std::map &entityControls, - [[maybe_unused]] const IPAOperationData &ipaConfig, - [[maybe_unused]] IPAOperationData *result) +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, + [[maybe_unused]] const std::map &streamConfig, + const std::map &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 &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 &streamConfig, - [[maybe_unused]] const std::map &entityControls, - [[maybe_unused]] const IPAOperationData &ipaConfig, - [[maybe_unused]] IPAOperationData *result) override {} + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo, + [[maybe_unused]] const std::map &streamConfig, + [[maybe_unused]] const std::map &entityControls, + [[maybe_unused]] const IPAOperationData &ipaConfig, + [[maybe_unused]] IPAOperationData *result) override { return 0; } void mapBuffers([[maybe_unused]] const std::vector &buffers) override {} void unmapBuffers([[maybe_unused]] const std::vector &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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &ipaConfig, - IPAOperationData *result) +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo, + const std::map &streamConfig, + const std::map &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 &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 &streamConfig, - [[maybe_unused]] const std::map &entityControls, - [[maybe_unused]] const IPAOperationData &ipaConfig, - [[maybe_unused]] IPAOperationData *result) override {} + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo, + [[maybe_unused]] const std::map &streamConfig, + [[maybe_unused]] const std::map &entityControls, + [[maybe_unused]] const IPAOperationData &ipaConfig, + [[maybe_unused]] IPAOperationData *result) override { return 0; } void mapBuffers([[maybe_unused]] const std::vector &buffers) override {} void unmapBuffers([[maybe_unused]] const std::vector &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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &ipaConfig, - IPAOperationData *result) override; + int configure(const CameraSensorInfo &sensorInfo, + const std::map &streamConfig, + const std::map &entityControls, + const IPAOperationData &ipaConfig, + IPAOperationData *result) override; void mapBuffers(const std::vector &buffers) override; void unmapBuffers(const std::vector &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 &streamConfig, - const std::map &entityControls, - const IPAOperationData &ipaConfig, - IPAOperationData *result) +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo, + const std::map &streamConfig, + const std::map &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 &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 &streamConfig, - const std::map &entityControls, - [[maybe_unused]] const IPAOperationData &ipaConfig, - [[maybe_unused]] IPAOperationData *result) override + int configure(const CameraSensorInfo &sensorInfo, + const std::map &streamConfig, + const std::map &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 &buffers) override