[libcamera-devel,v7,08/10] libcamera: Add IPCPipe implementation based on unix socket
diff mbox series

Message ID 20210211071805.34963-9-paul.elder@ideasonboard.com
State Changes Requested
Headers show
Series
  • IPA isolation: Part 1: Core components
Related show

Commit Message

Paul Elder Feb. 11, 2021, 7:18 a.m. UTC 
Add an implementation of IPCPipe using unix socket.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Changes in v7:
- implement new sendSync/sendAsync that only deals with IPCMessages
- remove sequence number (the called of sendSync/sendAsync deals with it
  now)
- add underflow check on receive
- cosmetic changes

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  |  49 ++++++
 src/libcamera/ipc_pipe_unixsocket.cpp         | 144 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 4 files changed, 196 insertions(+)
 create mode 100644 include/libcamera/internal/ipc_pipe_unixsocket.h
 create mode 100644 src/libcamera/ipc_pipe_unixsocket.cpp

Comments

Laurent Pinchart Feb. 12, 2021, 1:47 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Thu, Feb 11, 2021 at 04:18:03PM +0900, Paul Elder wrote:
> Add an implementation of IPCPipe using unix socket.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> ---
> Changes in v7:
> - implement new sendSync/sendAsync that only deals with IPCMessages
> - remove sequence number (the called of sendSync/sendAsync deals with it
>   now)
> - add underflow check on receive
> - cosmetic changes
> 
> 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  |  49 ++++++
>  src/libcamera/ipc_pipe_unixsocket.cpp         | 144 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  4 files changed, 196 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 5b64f705..e0de6c6d 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..4ffdddcc
> --- /dev/null
> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h
> @@ -0,0 +1,49 @@
> +/* 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(const IPCMessage &in,
> +		     IPCMessage *out = nullptr) override;
> +
> +	int sendAsync(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);
> +
> +	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..2bdce29e
> --- /dev/null
> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp
> @@ -0,0 +1,144 @@
> +/* 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()
> +{
> +	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()
> +{
> +}
> +
> +int IPCPipeUnixSocket::sendSync(const IPCMessage &in, IPCMessage *out)
> +{
> +	IPCUnixSocket::Payload response;
> +
> +	int ret = call(in.payload(), &response, in.header().cookie);
> +	if (ret) {
> +		LOG(IPCPipe, Error) << "Failed to call sync";
> +		return ret;
> +	}
> +
> +	if (out)
> +		*out = IPCMessage(response);
> +
> +	return 0;
> +}
> +
> +int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)
> +{
> +	int ret = socket_->send(data.payload());
> +	if (ret) {
> +		LOG(IPCPipe, Error) << "Failed to call async";
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void IPCPipeUnixSocket::readyRead(IPCUnixSocket *socket)
> +{
> +	IPCUnixSocket::Payload payload;
> +	int ret = socket->receive(&payload);
> +	if (ret) {
> +		LOG(IPCPipe, Error) << "Receive message failed" << ret;
> +		return;
> +	}
> +
> +	/* \todo Use span to avoid the double copy when callData is found. */
> +	IPCMessage ipcMessage(payload);

This line should move after the size check, otherwise it's a bit
pointless.

> +	if (payload.data.size() < sizeof(IPCMessage::Header)) {
> +		LOG(IPCPipe, Error) << "Not enough data received";
> +		return;
> +	}
> +
> +	auto callData = callData_.find(ipcMessage.header().cookie);
> +	if (callData != callData_.end()) {
> +		*callData->second.response = std::move(payload);
> +		callData->second.done = true;
> +		return;
> +	}
> +
> +	/* Received unexpected data, this means it's a call from the IPA. */
> +	recv.emit(ipcMessage);
> +
> +	return;

You can drop the return.

> +}
> +
> +int IPCPipeUnixSocket::call(const IPCUnixSocket::Payload &message,
> +			    IPCUnixSocket::Payload *response, uint32_t cookie)
> +{
> +	Timer timeout;
> +	int ret;
> +
> +	const auto [iter, success] = callData_.insert({ cookie, { response, false } });
> +
> +	ret = socket_->send(message);
> +	if (ret) {
> +		callData_.erase(iter);
> +		return ret;
> +	}
> +
> +	/* \todo Make this less dangerous, see IPCPipe::sendSync() */
> +	timeout.start(2000);
> +	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(iter);
> +
> +	return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e736c41e..efd85849 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -32,6 +32,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',

Patch
diff mbox series 

diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
index 5b64f705..e0de6c6d 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..4ffdddcc
--- /dev/null
+++ b/include/libcamera/internal/ipc_pipe_unixsocket.h
@@ -0,0 +1,49 @@ 
+/* 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(const IPCMessage &in,
+		     IPCMessage *out = nullptr) override;
+
+	int sendAsync(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);
+
+	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..2bdce29e
--- /dev/null
+++ b/src/libcamera/ipc_pipe_unixsocket.cpp
@@ -0,0 +1,144 @@ 
+/* 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()
+{
+	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()
+{
+}
+
+int IPCPipeUnixSocket::sendSync(const IPCMessage &in, IPCMessage *out)
+{
+	IPCUnixSocket::Payload response;
+
+	int ret = call(in.payload(), &response, in.header().cookie);
+	if (ret) {
+		LOG(IPCPipe, Error) << "Failed to call sync";
+		return ret;
+	}
+
+	if (out)
+		*out = IPCMessage(response);
+
+	return 0;
+}
+
+int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)
+{
+	int ret = socket_->send(data.payload());
+	if (ret) {
+		LOG(IPCPipe, Error) << "Failed to call async";
+		return ret;
+	}
+
+	return 0;
+}
+
+void IPCPipeUnixSocket::readyRead(IPCUnixSocket *socket)
+{
+	IPCUnixSocket::Payload payload;
+	int ret = socket->receive(&payload);
+	if (ret) {
+		LOG(IPCPipe, Error) << "Receive message failed" << ret;
+		return;
+	}
+
+	/* \todo Use span to avoid the double copy when callData is found. */
+	IPCMessage ipcMessage(payload);
+	if (payload.data.size() < sizeof(IPCMessage::Header)) {
+		LOG(IPCPipe, Error) << "Not enough data received";
+		return;
+	}
+
+	auto callData = callData_.find(ipcMessage.header().cookie);
+	if (callData != callData_.end()) {
+		*callData->second.response = std::move(payload);
+		callData->second.done = true;
+		return;
+	}
+
+	/* Received unexpected data, this means it's a call from the IPA. */
+	recv.emit(ipcMessage);
+
+	return;
+}
+
+int IPCPipeUnixSocket::call(const IPCUnixSocket::Payload &message,
+			    IPCUnixSocket::Payload *response, uint32_t cookie)
+{
+	Timer timeout;
+	int ret;
+
+	const auto [iter, success] = callData_.insert({ cookie, { response, false } });
+
+	ret = socket_->send(message);
+	if (ret) {
+		callData_.erase(iter);
+		return ret;
+	}
+
+	/* \todo Make this less dangerous, see IPCPipe::sendSync() */
+	timeout.start(2000);
+	while (!iter->second.done) {
+		if (!timeout.isRunning()) {
+			LOG(IPCPipe, Error) << "Call timeout!";
+			callData_.erase(cookie);
+			return -ETIMEDOUT;
+		}
+
+		Thread::current()->eventDispatcher()->processEvents();
+	}
+
+	callData_.erase(iter);
+
+	return 0;
+}
+
+} /* namespace libcamera */
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index e736c41e..efd85849 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -32,6 +32,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',