[libcamera-devel] [PATCH v3 12/38] libcamera: Add IPAIPC implementation based on unix socket
Jacopo Mondi
jacopo at jmondi.org
Thu Oct 29 13:57:26 CET 2020
Hi Paul,
On Fri, Oct 02, 2020 at 11:31:28PM +0900, Paul Elder wrote:
> Add an implementation of IPAIPC using unix socket.
Spell UNIX with capital letter ?
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v3:
> - remove unused writeUInt32() and readUInt32()
> - remove redundant definition of IPAIPCUnixSocket::isValid()
> - remove & 0xff in writeHeader()
> - make readHeader, writeHeader, and eraseHeader static class functions
> of IPAIPCUnixSocket instead of globals
>
> Changes in v2:
> - specify in doxygen to skip generating documentation for
> IPAIPCUnixSocket
> ---
> Documentation/Doxyfile.in | 2 +
> .../libcamera/internal/ipa_ipc_unixsocket.h | 62 +++++
> src/libcamera/ipa_ipc_unixsocket.cpp | 211 ++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 4 files changed, 276 insertions(+)
> create mode 100644 include/libcamera/internal/ipa_ipc_unixsocket.h
> create mode 100644 src/libcamera/ipa_ipc_unixsocket.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 598eca9b..71509ff7 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -837,8 +837,10 @@ RECURSIVE = YES
> EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \
> @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> + @TOP_SRCDIR@/include/libcamera/internal/ipa_ipc_unixsocket.h \
> @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> + @TOP_SRCDIR@/src/libcamera/ipa_ipc_unixsocket.cpp \
I wonder if we shouldn't make a src/libcamera/ipc/
> @TOP_SRCDIR@/src/libcamera/pipeline/ \
> @TOP_SRCDIR@/src/libcamera/proxy/ \
> @TOP_BUILDDIR@/include/libcamera/ipa/ \
> diff --git a/include/libcamera/internal/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h
> new file mode 100644
> index 00000000..11f05b12
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc_unixsocket.h - Image Processing Algorithm IPC module using unix socket
> + */
> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> +#define __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> +
> +#include <vector>
> +
> +#include <libcamera/span.h>
> +
> +#include "libcamera/internal/ipa_ipc.h"
> +#include "libcamera/internal/ipa_module.h"
> +#include "libcamera/internal/ipc_unixsocket.h"
> +
> +namespace libcamera {
> +
> +class Process;
> +
> +class IPAIPCUnixSocket : public IPAIPC
> +{
> +public:
> + IPAIPCUnixSocket(const char *ipa_module_path, const char *ipa_proxy_worker_path);
> + ~IPAIPCUnixSocket();
> +
> + int sendSync(uint32_t cmd,
> + const std::vector<uint8_t> &data_in,
> + const std::vector<int32_t> &fds_in,
> + std::vector<uint8_t> *data_out = nullptr,
> + std::vector<int32_t> *fds_out = nullptr) override;
> +
> + int sendAsync(uint32_t cmd,
> + const std::vector<uint8_t> &data_in,
> + const std::vector<int32_t> &fds_in) override;
> +
> + static void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq);
> + static std::tuple<uint32_t, uint32_t> readHeader(IPCUnixSocket::Payload &payload);
> + static void eraseHeader(IPCUnixSocket::Payload &payload);
> +
> +private:
> + struct CallData {
> + IPCUnixSocket::Payload *response;
> + bool done;
> + };
> +
> + void readyRead(IPCUnixSocket *socket);
> + int call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq);
> +
> + uint32_t seq_;
> +
> + Process *proc_;
> +
> + IPCUnixSocket *socket_;
> +
> + std::map<uint32_t, struct CallData> callData_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */
> diff --git a/src/libcamera/ipa_ipc_unixsocket.cpp b/src/libcamera/ipa_ipc_unixsocket.cpp
> new file mode 100644
> index 00000000..6dc92768
> --- /dev/null
> +++ b/src/libcamera/ipa_ipc_unixsocket.cpp
> @@ -0,0 +1,211 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket
> + */
> +
> +#include <vector>
> +
> +#include "libcamera/internal/ipc_unixsocket.h"
This one should go first
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/process.h"
> +#include "libcamera/internal/thread.h"
> +
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include "libcamera/internal/ipa_ipc.h"
> +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAIPC)
> +
> +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipa_module_path,
> + const char *ipa_proxy_worker_path)
I get this complaint from doxygen (on ipa_ipc.c actually)
/home/jmondi/project/libcamera/libcamera.git/src/libcamera/ipa_ipc.cpp:38: warning: argument 'ipa_module_path' of command @param is not found in the argument list of libcamera::IPAIPC::IPAIPC(const char *ipa_module_path][[maybe_unused], const char *ipa_proxy_worker_path][[maybe_unused])
/home/jmondi/project/libcamera/libcamera.git/src/libcamera/ipa_ipc.cpp:38: warning: argument 'ipa_proxy_worker_path' of command @param is not found in the argument list of libcamera::IPAIPC::IPAIPC(const char *ipa_module_path][[maybe_unused], const char *ipa_proxy_worker_path][[maybe_unused])
You don't ? (doxygen 1.8.20 here)
> + : IPAIPC(ipa_module_path, ipa_proxy_worker_path), seq_(0),
> + proc_(nullptr), socket_(nullptr)
> +{
> + std::vector<int> fds;
> + std::vector<std::string> args;
> + args.push_back(ipa_module_path);
> +
> + socket_ = new IPCUnixSocket();
> + int fd = socket_->create();
> + if (fd < 0) {
> + LOG(IPAIPC, Error)
> + << "Failed to create socket";
Fits on one line ?
> + return;
> + }
> + socket_->readyRead.connect(this, &IPAIPCUnixSocket::readyRead);
> + args.push_back(std::to_string(fd));
> + fds.push_back(fd);
> +
> + proc_ = new Process();
> + int ret = proc_->start(ipa_proxy_worker_path, args, fds);
What defines the proxy_worker interface ? ie that it wants 'fd' as
first argument ? (not that it is wrong but just to know where it is
defined)
> + if (ret) {
> + LOG(IPAIPC, Error)
> + << "Failed to start proxy worker process";
> + return;
> + }
> +
> + valid_ = true;
> +}
> +
> +IPAIPCUnixSocket::~IPAIPCUnixSocket()
> +{
> + delete proc_;
> + delete socket_;
we can save this by making proc_ and socket_ unique_ptr<>
> +}
> +
> +int IPAIPCUnixSocket::sendSync(uint32_t cmd,
> + const std::vector<uint8_t> &data_in,
> + const std::vector<int32_t> &fds_in,
> + std::vector<uint8_t> *data_out,
> + std::vector<int32_t> *fds_out)
> +{
> + IPCUnixSocket::Payload message, response;
It would be easy to pre-reserve message.data
8 bytes for the header + data_in.size()
This would avoid at least two potential relocations
Same for fds, not possible for response unless we over-allocate.
> + int ret;
> +
> + /* It's fine if seq_ overflows; that'll just be the new epoch. */
> + seq_++;
> + writeHeader(message, cmd, seq_);
> + message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> +
> + message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> +
> + ret = call(message, &response, seq_);
> + if (ret) {
> + LOG(IPAIPC, Error) << "Failed to call sync";
> + callData_.erase(seq_);
> + return ret;
> + }
> +
> + if (data_out)
> + data_out->insert(data_out->end(), response.data.begin(), response.data.end());
> +
> + if (fds_out)
> + fds_out->insert(fds_out->end(), response.fds.begin(), response.fds.end());
> +
> + return 0;
> +}
> +
> +int IPAIPCUnixSocket::sendAsync(uint32_t cmd,
> + const std::vector<uint8_t> &data_in,
> + const std::vector<int32_t> &fds_in)
> +{
> + IPCUnixSocket::Payload message;
> + int ret;
> +
> + writeHeader(message, cmd, 0);
> + message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> +
> + message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> +
> + ret = socket_->send(message);
> + if (ret) {
> + LOG(IPAIPC, Error) << "Failed to call async";
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +
Double empty line ?
> +void IPAIPCUnixSocket::writeHeader(IPCUnixSocket::Payload &payload,
> + uint32_t cmd, uint32_t seq)
> +{
> + uint8_t cmd_arr[] = {static_cast<uint8_t>(cmd),
> + static_cast<uint8_t>((cmd >> 8)),
> + static_cast<uint8_t>((cmd >> 16)),
> + static_cast<uint8_t>((cmd >> 24))};
> + uint8_t seq_arr[] = {static_cast<uint8_t>(seq),
> + static_cast<uint8_t>((seq >> 8)),
> + static_cast<uint8_t>((seq >> 16)),
> + static_cast<uint8_t>((seq >> 24))};
Do we care about endianesa between IPA and pipeline handlers ? Unless we run IPA
on a different CPU it should not matter, righ ?
Because otherwise you can take a uint8_t pointer to cmd and seq and just:
uint8_t *p = reinterpret_cast<uint8_t *>(&cmd);
payload.data.insert(payload.data.begin(), p, p + 4);
p = reinterpret_cast<uint8_t *>(&seq);
payload.data.insert(payload.data.begin(), p, p + 4);
and make this an inline function
> + payload.data.insert(payload.data.begin(), cmd_arr, cmd_arr+4);
> + payload.data.insert(payload.data.begin() + 4, seq_arr, seq_arr+4);
> +}
> +
> +std::tuple<uint32_t, uint32_t>
> +IPAIPCUnixSocket::readHeader(IPCUnixSocket::Payload &payload)
> +{
> + uint32_t cmd = payload.data[0] |
> + (payload.data[1] << 8) |
> + (payload.data[2] << 16) |
> + (payload.data[3] << 24);
> + uint32_t seq = payload.data[4] |
> + (payload.data[5] << 8) |
> + (payload.data[6] << 16) |
> + (payload.data[7] << 24);
> +
> + return {cmd, seq};
same here by reintrpreting payload.data[0] and payload.data[4] as
uint32_t
> +}
> +
> +void IPAIPCUnixSocket::eraseHeader(IPCUnixSocket::Payload &payload)
> +{
> + payload.data.erase(payload.data.begin(), payload.data.begin() + 8);
> +}
> +
> +void IPAIPCUnixSocket::readyRead(IPCUnixSocket *socket)
> +{
> + IPCUnixSocket::Payload message;
> + int ret = socket->receive(&message);
> + if (ret) {
> + LOG(IPAIPC, Error) << "Receive message failed" << ret;
> + return;
> + }
> +
> + uint32_t cmd, seq;
> + std::tie(cmd, seq) = readHeader(message);
> +
> + auto callData = callData_.find(seq);
> + if (callData != callData_.end()) {
> + eraseHeader(message);
> + /* Is there any way to avoid this copy? */
> + *callData->second.response = message;
> + callData->second.done = true;
> + return;
> + }
> +
> + /*
> + * Received unexpected data, this means it's a call from the IPA.
> + * We can't return anything to the IPA (gotta keep them under *our*
> + * control, plus returning would require blocking the caller, and we
> + * can't afford to do that). Let the proxy do switch-case on cmd.
> + */
> + recvIPC.emit(message.data, message.fds);
> +
> + return;
> +}
> +
> +int IPAIPCUnixSocket::call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq)
> +{
> + Timer timeout;
> + int ret;
> +
> + callData_[seq].response = response;
> + callData_[seq].done = false;
> +
> + ret = socket_->send(message);
> + if (ret)
> + return ret;
> +
> + timeout.start(200);
> + while (!callData_[seq].done) {
> + if (!timeout.isRunning()) {
> + LOG(IPAIPC, Error) << "Call timeout!";
> + callData_.erase(seq);
> + return -ETIMEDOUT;
> + }
> +
> + Thread::current()->eventDispatcher()->processEvents();
I'll more carefully review this last part later, but I would have a
question, possibly for Laurent: do we have or need to implement a
timed wait condition support in the thread library, taking a condition
variable and a timeout and implementing this part ? It should be
paired with a condition signaling couter-part. This would avoid having
library classes poking the eventDispatcher themselves.
Thanks
j
> + }
> +
> + callData_.erase(seq);
> +
> + return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e0a2ab47..59417403 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -26,6 +26,7 @@ libcamera_sources = files([
> 'ipa_controls.cpp',
> 'ipa_data_serializer.cpp',
> 'ipa_ipc.cpp',
> + 'ipa_ipc_unixsocket.cpp',
> 'ipa_interface.cpp',
> 'ipa_manager.cpp',
> 'ipa_module.cpp',
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list