[{"id":15116,"web_url":"https://patchwork.libcamera.org/comment/15116/","msgid":"","date":"2021-02-12T01:47:30","subject":"Re: [libcamera-devel] [PATCH v7 08/10] libcamera: Add IPCPipe\n\timplementation based on unix socket","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Feb 11, 2021 at 04:18:03PM +0900, Paul Elder wrote:\n> Add an implementation of IPCPipe using unix socket.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> Changes in v7:\n> - implement new sendSync/sendAsync that only deals with IPCMessages\n> - remove sequence number (the called of sendSync/sendAsync deals with it\n> now)\n> - add underflow check on receive\n> - cosmetic changes\n> \n> Changes in v6:\n> - remove explicit nullptr intializations for unique_ptr members\n> - move callData_.erase() to the call() error path\n> \n> Changes in v5:\n> - rename IPAIPCUnixSocket to IPCPipeUnixSocket\n> - rename ipa_ipc_unisocket.[(cpp),h] ipc_pipe_unixsocket.[(cpp),h]\n> \n> Changes in v4:\n> - change snake_case to camelCase\n> - change proc_ and socket_ to unique pointers\n> - move inclusion of corresponding header to first in the include list\n> - reserve message data and fds size (for sending)\n> \n> Changes in v3:\n> - remove unused writeUInt32() and readUInt32()\n> - remove redundant definition of IPAIPCUnixSocket::isValid()\n> - remove & 0xff in writeHeader()\n> - make readHeader, writeHeader, and eraseHeader static class functions\n> of IPAIPCUnixSocket instead of globals\n> \n> Changes in v2:\n> - specify in doxygen to skip generating documentation for\n> IPAIPCUnixSocket\n> ---\n> Documentation/Doxyfile.in | 2 +\n> .../libcamera/internal/ipc_pipe_unixsocket.h | 49 ++++++\n> src/libcamera/ipc_pipe_unixsocket.cpp | 144 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 196 insertions(+)\n> create mode 100644 include/libcamera/internal/ipc_pipe_unixsocket.h\n> create mode 100644 src/libcamera/ipc_pipe_unixsocket.cpp\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 5b64f705..e0de6c6d 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -837,8 +837,10 @@ RECURSIVE = YES\n> EXCLUDE = @TOP_SRCDIR@/include/libcamera/span.h \\\n> \t\t\t @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \\\n> \t\t\t @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \\\n> +\t\t\t @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \\\n> +\t\t\t @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/proxy/ \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> new file mode 100644\n> index 00000000..4ffdddcc\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> @@ -0,0 +1,49 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipc_pipe_unixsocket.h - Image Processing Algorithm IPC module using unix socket\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__\n> +#define __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/ipc_pipe.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +\n> +namespace libcamera {\n> +\n> +class Process;\n> +\n> +class IPCPipeUnixSocket : public IPCPipe\n> +{\n> +public:\n> +\tIPCPipeUnixSocket(const char *ipaModulePath, const char *ipaProxyWorkerPath);\n> +\t~IPCPipeUnixSocket();\n> +\n> +\tint sendSync(const IPCMessage &in,\n> +\t\t IPCMessage *out = nullptr) override;\n> +\n> +\tint sendAsync(const IPCMessage &data) override;\n> +\n> +private:\n> +\tstruct CallData {\n> +\t\tIPCUnixSocket::Payload *response;\n> +\t\tbool done;\n> +\t};\n> +\n> +\tvoid readyRead(IPCUnixSocket *socket);\n> +\tint call(const IPCUnixSocket::Payload &message,\n> +\t\t IPCUnixSocket::Payload *response, uint32_t seq);\n> +\n> +\tstd::unique_ptr proc_;\n> +\tstd::unique_ptr socket_;\n> +\tstd::map callData_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */\n> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> new file mode 100644\n> index 00000000..2bdce29e\n> --- /dev/null\n> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> @@ -0,0 +1,144 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipc_pipe_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket\n> + */\n> +\n> +#include \"libcamera/internal/ipc_pipe_unixsocket.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/event_dispatcher.h\"\n> +#include \"libcamera/internal/ipc_pipe.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/process.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +#include \"libcamera/internal/timer.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPCPipe)\n> +\n> +IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,\n> +\t\t\t\t const char *ipaProxyWorkerPath)\n> +\t: IPCPipe()\n> +{\n> +\tstd::vector fds;\n> +\tstd::vector args;\n> +\targs.push_back(ipaModulePath);\n> +\n> +\tsocket_ = std::make_unique();\n> +\tint fd = socket_->create();\n> +\tif (fd < 0) {\n> +\t\tLOG(IPCPipe, Error) << \"Failed to create socket\";\n> +\t\treturn;\n> +\t}\n> +\tsocket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);\n> +\targs.push_back(std::to_string(fd));\n> +\tfds.push_back(fd);\n> +\n> +\tproc_ = std::make_unique();\n> +\tint ret = proc_->start(ipaProxyWorkerPath, args, fds);\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error)\n> +\t\t\t<< \"Failed to start proxy worker process\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tconnected_ = true;\n> +}\n> +\n> +IPCPipeUnixSocket::~IPCPipeUnixSocket()\n> +{\n> +}\n> +\n> +int IPCPipeUnixSocket::sendSync(const IPCMessage &in, IPCMessage *out)\n> +{\n> +\tIPCUnixSocket::Payload response;\n> +\n> +\tint ret = call(in.payload(), &response, in.header().cookie);\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error) << \"Failed to call sync\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (out)\n> +\t\t*out = IPCMessage(response);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)\n> +{\n> +\tint ret = socket_->send(data.payload());\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error) << \"Failed to call async\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IPCPipeUnixSocket::readyRead(IPCUnixSocket *socket)\n> +{\n> +\tIPCUnixSocket::Payload payload;\n> +\tint ret = socket->receive(&payload);\n> +\tif (ret) {\n> +\t\tLOG(IPCPipe, Error) << \"Receive message failed\" << ret;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* \\todo Use span to avoid the double copy when callData is found. */\n> +\tIPCMessage ipcMessage(payload);\n\nThis line should move after the size check, otherwise it's a bit\npointless.\n\n> +\tif (payload.data.size() < sizeof(IPCMessage::Header)) {\n> +\t\tLOG(IPCPipe, Error) << \"Not enough data received\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tauto callData = callData_.find(ipcMessage.header().cookie);\n> +\tif (callData != callData_.end()) {\n> +\t\t*callData->second.response = std::move(payload);\n> +\t\tcallData->second.done = true;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* Received unexpected data, this means it's a call from the IPA. */\n> +\trecv.emit(ipcMessage);\n> +\n> +\treturn;\n\nYou can drop the return.\n\n> +}\n> +\n> +int IPCPipeUnixSocket::call(const IPCUnixSocket::Payload &message,\n> +\t\t\t IPCUnixSocket::Payload *response, uint32_t cookie)\n> +{\n> +\tTimer timeout;\n> +\tint ret;\n> +\n> +\tconst auto [iter, success] = callData_.insert({ cookie, { response, false } });\n> +\n> +\tret = socket_->send(message);\n> +\tif (ret) {\n> +\t\tcallData_.erase(iter);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* \\todo Make this less dangerous, see IPCPipe::sendSync() */\n> +\ttimeout.start(2000);\n> +\twhile (!iter->second.done) {\n> +\t\tif (!timeout.isRunning()) {\n> +\t\t\tLOG(IPCPipe, Error) << \"Call timeout!\";\n> +\t\t\tcallData_.erase(cookie);\n\n\t\t\tcallData_.erase(iter);\n\n> +\t\t\treturn -ETIMEDOUT;\n> +\t\t}\n> +\n> +\t\tThread::current()->eventDispatcher()->processEvents();\n> +\t}\n> +\n> +\tcallData_.erase(iter);\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index e736c41e..efd85849 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -32,6 +32,7 @@ libcamera_sources = files([\n> 'ipa_module.cpp',\n> 'ipa_proxy.cpp',\n> 'ipc_pipe.cpp',\n> + 'ipc_pipe_unixsocket.cpp',\n> 'ipc_unixsocket.cpp',\n> 'log.cpp',\n> 'media_device.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 364B4BD160\n\tfor ;\n\tFri, 12 Feb 2021 01:47:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C394E63752;\n\tFri, 12 Feb 2021 02:47:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7544B602FC\n\tfor ;\n\tFri, 12 Feb 2021 02:47:56 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C84188B5;\n\tFri, 12 Feb 2021 02:47:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XSHS3Z40\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613094476;\n\tbh=5PU+tYI9igBfEQoMRbW79Rai5+aC5QqiKK/vOAbVHLU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XSHS3Z408xAtRefbWnHBcAwsi2GBXZwvZx80sEn1Zm+G7qtJNSa44EYcoNuSZ4X8g\n\tHV4LPsssGeHbeFHRotxH+sCEHTgSggMGsfIGaU9iic/Dymx39VA73lQUwgixFdj04C\n\t2/jferJ5gsbBB4+LwtNSP1fsJZorVeX56j/3ahdE=","Date":"Fri, 12 Feb 2021 03:47:30 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210211071805.34963-1-paul.elder@ideasonboard.com>\n\t<20210211071805.34963-9-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210211071805.34963-9-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 08/10] libcamera: Add IPCPipe\n\timplementation based on unix socket","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]