[libcamera-devel] [PATCH 15/38] libcamera: Add IPAIPC implementation based on unix socket
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Fri Sep 25 04:57:27 CEST 2020
On Thu, Sep 24, 2020 at 12:55:05PM +0200, Jacopo Mondi wrote:
> 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
ack
> > + 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 ?
This header is only for IPAIPCUnixSocket, not the base IPAIPC, so it
should be fine to keep them here. test/ipc/unixsocket_ipc.cpp tests
IPAIPCUnixSocket.
As for the proxy worker, as I mentioned before, technically it's a
layering violation and we should really have an IPAIPCWorker
(IPAIPCUnixSocketWorker) in between IPAIPC (IPAIPCUnixSocket)
and IPAProxy{pipeline_name}Worker. But we don't have any other IPC
mechanisms yet, so it should be fine for now. When we do, then the proxy
worker will no longer use these IPC-specific header manipulation
functions, and they will be used in the IPAIPCUnixSocketWorker.
> (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).
These are the only files that include this header:
src/libcamera/ipa_ipc_unixsocket.cpp
test/ipc/unixsocket_ipc.cpp
utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
The first two are fine. The last one is fine for now. The other two
really only include this header for choosing the IPAIPC that they want
to use, which for now should be fine. When we add other IPC mechanisms
we can rename or namespace these.
> > +
> > +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.
Ah yes, these are leftovers from the de/serialization work...
> 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 ?
Ah yeah it is.
Thanks,
Paul
> > +
> > + 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