[libcamera-devel] [PATCH v6 8/9] libcamera: Add IPCPipe implementation based on unix socket
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 2 03:29:38 CET 2021
Hi Paul,
Thank you for the patch.
On Thu, Dec 24, 2020 at 05:15:33PM +0900, Paul Elder wrote:
> Add an implementation of IPCPipe using unix socket.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v6:
> - remove explicit nullptr intializations for unique_ptr members
> - move callData_.erase() to the call() error path
>
> Changes in v5:
> - rename IPAIPCUnixSocket to IPCPipeUnixSocket
> - rename ipa_ipc_unisocket.[(cpp),h] ipc_pipe_unixsocket.[(cpp),h]
>
> Changes in v4:
> - change snake_case to camelCase
> - change proc_ and socket_ to unique pointers
> - move inclusion of corresponding header to first in the include list
> - reserve message data and fds size (for sending)
>
> 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/ipc_pipe_unixsocket.h | 50 ++++++
> src/libcamera/ipc_pipe_unixsocket.cpp | 147 ++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 4 files changed, 200 insertions(+)
> create mode 100644 include/libcamera/internal/ipc_pipe_unixsocket.h
> create mode 100644 src/libcamera/ipc_pipe_unixsocket.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index b18b8e9c..36a0cef3 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/ipc_pipe_unixsocket.h \
> @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> + @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> @TOP_SRCDIR@/src/libcamera/pipeline/ \
> @TOP_SRCDIR@/src/libcamera/proxy/ \
> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h
> new file mode 100644
> index 00000000..fea3179c
> --- /dev/null
> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipc_pipe_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 <map>
> +#include <memory>
> +#include <vector>
> +
> +#include "libcamera/internal/ipc_pipe.h"
> +#include "libcamera/internal/ipc_unixsocket.h"
> +
> +namespace libcamera {
> +
> +class Process;
> +
> +class IPCPipeUnixSocket : public IPCPipe
> +{
> +public:
> + IPCPipeUnixSocket(const char *ipaModulePath, const char *ipaProxyWorkerPath);
> + ~IPCPipeUnixSocket();
> +
> + int sendSync(uint32_t cmd,
> + const IPCMessage &in,
> + IPCMessage *out = nullptr) override;
> +
> + int sendAsync(uint32_t cmd, const IPCMessage &data) 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);
Line wrap ?
> +
> + uint32_t seq_;
> + std::unique_ptr<Process> proc_;
> + std::unique_ptr<IPCUnixSocket> socket_;
> + std::map<uint32_t, CallData> callData_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */
> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp
> new file mode 100644
> index 00000000..43c20e6b
> --- /dev/null
> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipc_pipe_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket
> + */
> +
> +#include "libcamera/internal/ipc_pipe_unixsocket.h"
> +
> +#include <vector>
> +
> +#include "libcamera/internal/event_dispatcher.h"
> +#include "libcamera/internal/ipc_pipe.h"
> +#include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/process.h"
> +#include "libcamera/internal/thread.h"
> +#include "libcamera/internal/timer.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPCPipe)
> +
> +IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,
> + const char *ipaProxyWorkerPath)
> + : IPCPipe(), seq_(0)
> +{
> + std::vector<int> fds;
> + std::vector<std::string> args;
> + args.push_back(ipaModulePath);
> +
> + socket_ = std::make_unique<IPCUnixSocket>();
> + int fd = socket_->create();
> + if (fd < 0) {
> + LOG(IPCPipe, Error) << "Failed to create socket";
> + return;
> + }
> + socket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);
> + args.push_back(std::to_string(fd));
> + fds.push_back(fd);
> +
> + proc_ = std::make_unique<Process>();
> + int ret = proc_->start(ipaProxyWorkerPath, args, fds);
> + if (ret) {
> + LOG(IPCPipe, Error)
> + << "Failed to start proxy worker process";
> + return;
> + }
> +
> + connected_ = true;
> +}
> +
> +IPCPipeUnixSocket::~IPCPipeUnixSocket()
> +{
> +}
I've recently learnt that you can also write
IPCPipeUnixSocket::~IPCPipeUnixSocket() = default;
(which doesn't mean you have to change this)
> +
> +int IPCPipeUnixSocket::sendSync(uint32_t cmd,
> + const IPCMessage &in, IPCMessage *out)
> +{
> + IPCUnixSocket::Payload message, response;
s/message/request/ ?
> +
> + const IPCMessage::Header header{ cmd, ++seq_ };
> + message = in.payload(&header);
IPCUnixSocket::Payload message = in.payload(&header);
IPCUnixSocket::Payload response;
> +
> + int ret = call(message, &response, seq_);
Or maybe
int ret = call(in.payload(&header), &response, seq_);
which would be nice to turn into
int ret = call(in.payload(), &response, seq_);
following my comments on the previous patch. However, we have here an
issue as we need to set the sequence number, and in is const. Maybe the
payload() function could take a sequence number instead of a header
pointer ?
Or maybe this is too much churn and we can keep it as-is until we decide
to refactor the code. Feel free to clean up what you think makes sense
to clean up already, and leave the rest out.
> + if (ret) {
> + LOG(IPCPipe, Error) << "Failed to call sync";
> + return ret;
> + }
> +
> + if (out)
> + *out = IPCMessage(response);
> +
> + return 0;
> +}
> +
> +int IPCPipeUnixSocket::sendAsync(uint32_t cmd, const IPCMessage &data)
> +{
> + const IPCMessage::Header header{ cmd, 0 };
> + IPCUnixSocket::Payload message = data.payload(&header);
Here, it would be really nice if we didn't have to pass cmd to
sendAsync(), and data already had cmd set in the header.
s/message/request/ ?
and s/data/msg/ (or message)
> +
> + int ret = socket_->send(message);
> + if (ret) {
> + LOG(IPCPipe, Error) << "Failed to call async";
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +void IPCPipeUnixSocket::readyRead(IPCUnixSocket *socket)
> +{
> + IPCUnixSocket::Payload message;
s/message/payload/ ?
> + int ret = socket->receive(&message);
> + if (ret) {
> + LOG(IPCPipe, Error) << "Receive message failed" << ret;
> + return;
> + }
> +
> + /* \todo Use span to avoid the double copy when callData is found. */
> + IPCMessage ipcMessage(message);
if (message.data.size() < sizeof(IPCMessage::Header)) {
LOG(...) << ...;
return;
}
IPCMessage::Header *header =
static_cast<IPCMessage::Header *>(messaga.data.data());
> +
> + auto callData = callData_.find(ipcMessage.header().cookie);
auto callData = callData_.find(header->cookie);
> + if (callData != callData_.end()) {
> + *callData->second.response = std::move(message);
> + callData->second.done = true;
> + return;
> + }
> +
> + /* Received unexpected data, this means it's a call from the IPA. */
> + recv.emit(ipcMessage.header().cmd, ipcMessage);
Could we drop the cmd argument, as it's available through the message ?
recv.emit(IPCMessage(message));
> +
> + return;
> +}
> +
> +int IPCPipeUnixSocket::call(const IPCUnixSocket::Payload &message,
> + IPCUnixSocket::Payload *response, uint32_t cookie)
> +{
> + Timer timeout;
> + int ret;
> +
> + callData_[cookie].response = response;
> + callData_[cookie].done = false;
This could be made more efficient by avoiding the lookups.
const auto [iter, success] = callData_.insert({ cookie, { response, false }});
(you may need a constructor for the CallData structure)
> +
> + ret = socket_->send(message);
> + if (ret) {
> + callData_.erase(seq_);
s/seq_/cookie/ ?
But in any case,
callData_.erase(iter);
> + return ret;
> + }
> +
/* \todo Make this less dangerous, see IPCPipe::sendSync() */
> + timeout.start(2000);
> + while (!callData_[cookie].done) {
while (!iter->second.done) {
> + if (!timeout.isRunning()) {
> + LOG(IPCPipe, Error) << "Call timeout!";
> + callData_.erase(cookie);
callData_.erase(iter);
> + return -ETIMEDOUT;
> + }
> +
> + Thread::current()->eventDispatcher()->processEvents();
> + }
> +
> + callData_.erase(cookie);
callData_.erase(iter);
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> +
> + return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index b7cfe0c2..cd3cf9e3 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -31,6 +31,7 @@ libcamera_sources = files([
> 'ipa_module.cpp',
> 'ipa_proxy.cpp',
> 'ipc_pipe.cpp',
> + 'ipc_pipe_unixsocket.cpp',
> 'ipc_unixsocket.cpp',
> 'log.cpp',
> 'media_device.cpp',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list