[libcamera-devel] [PATCH 15/38] libcamera: Add IPAIPC implementation based on unix socket
Jacopo Mondi
jacopo at jmondi.org
Thu Sep 24 12:55:05 CEST 2020
On Tue, Sep 22, 2020 at 10:35:14PM +0900, Paul Elder wrote:
> Add an implementation of IPAIPC using unix socket.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v2:
> - specify in doxygen to skip generating documentation for
> IPAIPCUnixSocket
> ---
> Documentation/Doxyfile.in | 2 +
> .../libcamera/internal/ipa_ipc_unixsocket.h | 115 ++++++++++++
> src/libcamera/ipa_ipc_unixsocket.cpp | 175 ++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 4 files changed, 293 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 \
> @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..de8af35b
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h
> @@ -0,0 +1,115 @@
> +/* 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;
> +
> +inline void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq)
> +{
> + uint8_t cmd_arr[] = {static_cast<uint8_t>(cmd & 0xff),
> + static_cast<uint8_t>((cmd >> 8 ) & 0xff),
> + static_cast<uint8_t>((cmd >> 16) & 0xff),
> + static_cast<uint8_t>((cmd >> 24) & 0xff)};
> + uint8_t seq_arr[] = {static_cast<uint8_t>(seq & 0xff),
> + static_cast<uint8_t>((seq >> 8 ) & 0xff),
> + static_cast<uint8_t>((seq >> 16) & 0xff),
> + static_cast<uint8_t>((seq >> 24) & 0xff)};
I think you can omit the 0xff as the cast to uint8_t implies it
> + payload.data.insert(payload.data.begin(), cmd_arr, cmd_arr+4);
> + payload.data.insert(payload.data.begin() + 4, seq_arr, seq_arr+4);
> +}
> +
> +inline std::tuple<uint32_t, uint32_t> readHeader(IPCUnixSocket::Payload &payload)
> +{
> + uint32_t cmd = (payload.data[0] & 0xff) |
> + ((payload.data[1] & 0xff) << 8) |
> + ((payload.data[2] & 0xff) << 16) |
> + ((payload.data[3] & 0xff) << 24);
> + uint32_t seq = (payload.data[4] & 0xff) |
> + ((payload.data[5] & 0xff) << 8) |
> + ((payload.data[6] & 0xff) << 16) |
> + ((payload.data[7] & 0xff) << 24);
> +
> + return {cmd, seq};
> +}
> +
> +inline void eraseHeader(IPCUnixSocket::Payload &payload)
> +{
> + payload.data.erase(payload.data.begin(), payload.data.begin() + 8);
> +}
I would not place these in the header file but rather as static
functions in an anonymous namespace in the cpp file. Or are they used
outside from this module ? It seems so:
src/libcamera/ipa_ipc_unixsocket.cpp: std::tie(cmd, seq) = readHeader(message);
test/ipc/unixsocket_ipc.cpp: std::tie(cmd, seq) = readHeader(message);
utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl: std::tie(_cmd, _seq) = readHeader(_message);
So why are they defined in this header which implements the IPAIPC
interface ? What if another IPC mechanism is used ?
(I -think- also this causes the function to be defined in every module
that includes this header, wasting a bit of space, but that's a
secondary issue).
> +
> +inline void writeUInt32(IPCUnixSocket::Payload &payload, uint32_t val, uint32_t pos)
> +{
> + if (pos + 4 > payload.data.size())
> + return;
> +
> + uint8_t arr[] = {static_cast<uint8_t>(val & 0xff),
> + static_cast<uint8_t>(val & (0xff << 8)),
> + static_cast<uint8_t>(val & (0xff << 16)),
> + static_cast<uint8_t>(val & (0xff << 24))};
> + std::copy(arr, arr + 4, payload.data.begin() + pos);
> +}
> +
> +inline uint32_t readUInt32(IPCUnixSocket::Payload &payload, uint32_t pos)
> +{
> + if (pos + 4 > payload.data.size())
> + return 0;
> +
> + return payload.data[pos] & (payload.data[pos + 1] << 8) &
> + (payload.data[pos + 2] << 16) & (payload.data[pos + 3] << 24);
> +}
I don't see these used.
Also, double empty line
> +
> +
> +class IPAIPCUnixSocket : public IPAIPC
> +{
> +public:
> + IPAIPCUnixSocket(const char *ipa_module_path, const char *ipa_proxy_worker_path);
> + ~IPAIPCUnixSocket();
> +
> + bool isValid() const { return valid_; }
that's defined in the base class, right ?
> +
> + 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;
> +
> +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 */
> +
I'll for now skip the review of the implementation.
> +#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..c1997e96
> --- /dev/null
> +++ b/src/libcamera/ipa_ipc_unixsocket.cpp
> @@ -0,0 +1,175 @@
> +/* 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"
> +#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)
> + : 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";
> + 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);
> + if (ret) {
> + LOG(IPAIPC, Error)
> + << "Failed to start proxy worker process";
> + return;
> + }
> +
> + valid_ = true;
> +}
> +
> +IPAIPCUnixSocket::~IPAIPCUnixSocket()
> +{
> + delete proc_;
> + delete socket_;
> +}
> +
> +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;
> + 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;
> +}
> +
> +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();
> + }
> +
> + callData_.erase(seq);
> +
> + return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e3eade57..24f32815 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -25,6 +25,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