[{"id":15115,"web_url":"https://patchwork.libcamera.org/comment/15115/","msgid":"","date":"2021-02-12T01:26:33","subject":"Re: [libcamera-devel] [PATCH v7 07/10] libcamera: Add IPCPipe","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:02PM +0900, Paul Elder wrote:\n> Create a virtual IPCPipe class that models an IPC/RPC system. IPA proxies\n> and proxy workers will call into the IPCPipe, rather than implementing\n> the IPC themselves.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> Changes in v7:\n> - add an IPCMessage construct that takes just a command code\n> - remove 'c' from cheader, cdata, and cfds, and just overload them\n> - implement new sendSync/sendAsync API, so that they only deal with\n> IPCMessages\n> - so the caller is responsible for populating the command code and the\n> sequence number\n> - cosmetic changes, reword some docs, add some todos\n> \n> No change in v6\n> \n> Changes in v5:\n> - fix style\n> - rename ipa_ipc.[(cpp)h] to ipc_pipe.[(cpp),h]\n> - rename IPAIPC to IPCPipe\n> - add IPCMessage to use in the IPCPipe\n> \n> Change in v4:\n> - change snake_case to camelCase\n> - remove ipaModulePath and ipaProxyWorkerPath from parameters in the\n> IPAIPC base contructor\n> - include signal.h\n> \n> Changes in v3:\n> - expand documentation a bit\n> - move [[maybe_unused]] to after the parameters in the IPAIPC\n> constructor, to appease gcc <= 9.1\n> \n> Changes in v2:\n> - add documentation\n> ---\n> include/libcamera/internal/ipc_pipe.h | 69 ++++++++\n> src/libcamera/ipc_pipe.cpp | 220 ++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 290 insertions(+)\n> create mode 100644 include/libcamera/internal/ipc_pipe.h\n> create mode 100644 src/libcamera/ipc_pipe.cpp\n> \n> diff --git a/include/libcamera/internal/ipc_pipe.h b/include/libcamera/internal/ipc_pipe.h\n> new file mode 100644\n> index 00000000..c9a84b78\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipc_pipe.h\n> @@ -0,0 +1,69 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipc_pipe.h - Image Processing Algorithm IPC module for IPA proxies\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class IPCMessage\n> +{\n> +public:\n> +\tstruct Header {\n> +\t\tuint32_t cmd;\n> +\t\tuint32_t cookie;\n> +\t};\n> +\n> +\tIPCMessage();\n> +\tIPCMessage(uint32_t cmd);\n> +\tIPCMessage(const Header &header);\n> +\tIPCMessage(const IPCUnixSocket::Payload &payload);\n> +\n> +\tIPCUnixSocket::Payload payload() const;\n> +\n> +\tHeader &header() { return header_; }\n> +\tstd::vector &data() { return data_; }\n> +\tstd::vector &fds() { return fds_; }\n> +\n> +\tconst Header &header() const { return header_; }\n> +\tconst std::vector &data() const { return data_; }\n> +\tconst std::vector &fds() const { return fds_; }\n> +\n> +private:\n> +\tHeader header_;\n> +\n> +\tstd::vector data_;\n> +\tstd::vector fds_;\n> +};\n> +\n> +class IPCPipe\n> +{\n> +public:\n> +\tIPCPipe();\n> +\tvirtual ~IPCPipe();\n> +\n> +\tbool isConnected() const { return connected_; }\n> +\n> +\tvirtual int sendSync(const IPCMessage &in,\n> +\t\t\t IPCMessage *out) = 0;\n> +\n> +\tvirtual int sendAsync(const IPCMessage &data) = 0;\n> +\n> +\tSignal recv;\n> +\n> +protected:\n> +\tbool connected_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */\n> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp\n> new file mode 100644\n> index 00000000..77b5ef2b\n> --- /dev/null\n> +++ b/src/libcamera/ipc_pipe.cpp\n> @@ -0,0 +1,220 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipc_pipe.cpp - Image Processing Algorithm IPC module for IPA proxies\n> + */\n> +\n> +#include \"libcamera/internal/ipc_pipe.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file ipc_pipe.h\n> + * \\brief IPC mechanism for IPA isolation\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPCPipe)\n> +\n> +/**\n> + * \\struct IPCMessage::Header\n> + * \\brief Container for an IPCMessage header\n> + *\n> + * Holds a cmd code for the IPC message, and a cookie.\n> + */\n> +\n> +/**\n> + * \\var IPCMessage::Header::cmd\n> + * \\brief Type of IPCMessage\n> + *\n> + * Typically used to carry a command code for an RPC.\n> + */\n> +\n> +/**\n> + * \\var IPCMessage::Header::cookie\n> + * \\brief Cookie to identify the message and a corresponding reply.\n> + *\n> + * Populated and used by IPCPipe implementations for matching calls with\n> + * replies.\n> + */\n> +\n> +/**\n> + * \\class IPCMessage\n> + * \\brief IPC message to be passed through IPC message pipe\n> + */\n> +\n> +/**\n> + * \\brief Construct an empty IPCMessage instance\n> + */\n> +IPCMessage::IPCMessage()\n> +\t: header_(Header{ 0, 0 })\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct an IPCMessage instance with a given command code\n> + * \\param[in] cmd The command code\n> + */\n> +IPCMessage::IPCMessage(uint32_t cmd)\n> +\t: header_(Header{ cmd, 0 })\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct an IPCMessage instance with a given header\n> + * \\param[in] header The header that the constructed IPCMessage will contain\n> + */\n> +IPCMessage::IPCMessage(const Header &header)\n> +\t: header_(header)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct an IPCMessage instance from an IPC payload\n> + * \\param[in] payload The IPCUnixSocket payload to construct from\n> + *\n> + * This essentially converts an IPCUnixSocket payload into an IPCMessage.\n> + * The header is extracted from the payload into the IPCMessage's header field.\n> + */\n> +IPCMessage::IPCMessage(const IPCUnixSocket::Payload &payload)\n> +{\n> +\tmemcpy(&header_, payload.data.data(), sizeof(header_));\n> +\tdata_ = std::vector(payload.data.begin() + sizeof(header_),\n> +\t\t\t\t payload.data.end());\n> +\tfds_ = payload.fds;\n> +}\n> +\n> +/**\n> + * \\brief Create an IPCUnixSocket payload from the IPCMessage\n> + *\n> + * This essentially converts the IPCMessage into an IPCUnixSocket payload.\n> + *\n> + * \\todo Resolve the layering violation (add other converters later?)\n> + */\n> +IPCUnixSocket::Payload IPCMessage::payload() const\n> +{\n> +\tIPCUnixSocket::Payload payload;\n> +\n> +\tpayload.data.resize(sizeof(Header) + data_.size());\n> +\tpayload.fds.reserve(fds_.size());\n> +\n> +\tmemcpy(payload.data.data(), &header_, sizeof(Header));\n> +\n> +\t/* \\todo Make this work without copy */\n> +\tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n> +\tpayload.fds = fds_;\n> +\n> +\treturn payload;\n> +}\n> +\n> +/**\n> + * \\fn IPCMessage::header()\n> + * \\brief Returns a reference to the header\n> + */\n> +\n> +/**\n> + * \\fn IPCMessage::data()\n> + * \\brief Returns a reference to the byte vector containing data\n> + */\n> +\n> +/**\n> + * \\fn IPCMessage::fds()\n> + * \\brief Returns a reference to the vector containing file descriptors\n> + */\n> +\n> +/**\n> + * \\fn IPCMessage::header() const\n> + * \\brief Returns a const reference to the header\n> + */\n> +\n> +/**\n> + * \\fn IPCMessage::data() const\n> + * \\brief Returns a const reference to the byte vector containing data\n> + */\n> +\n> +/**\n> + * \\fn IPCMessage::fds() const\n> + * \\brief Returns a const reference to the vector containing file descriptors\n> + */\n> +\n> +/**\n> + * \\class IPCPipe\n> + * \\brief IPC message pipe for IPA isolation\n> + *\n> + * Virtual class to model an IPC message pipe for use by IPA proxies for IPA\n> + * isolation. sendSync() and sendAsync() must be implemented, and the recvMessage\n> + * signal must be emitted whenever new data is available.\n> + */\n> +\n> +/**\n> + * \\brief Construct an IPCPipe instance\n> + */\n> +IPCPipe::IPCPipe()\n> +\t: connected_(false)\n> +{\n> +}\n> +\n> +IPCPipe::~IPCPipe()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn IPCPipe::isConnected()\n> + * \\brief Check if the IPCPipe instance is connected\n> + *\n> + * An IPCPipe instance is connected if IPC is successfully set up.\n> + *\n> + * \\return True if the IPCPipe is connected, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn IPCPipe::sendSync()\n> + * \\brief Send a message over IPC synchronously\n> + * \\param[in] in Data to send, as an IPCMessage\n\nYou can drop \", as an IPCMessage\", doxygen shows the type of parameters.\nSame for the out parameter, and the other functions below.\n\n> + * \\param[in] out IPCMessage instance in which to receive data, if applicable\n> + *\n> + * This function will not return until a response is received. The event loop\n> + * will still continue to execute, however.\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + *\n> + * \\todo Determine if the event loop should limit the types of messages it\n> + * processes, to avoid reintrancy in the caller, and carefully document what\n> + * the caller needs to implement to make this safe.\n> + */\n> +\n> +/**\n> + * \\fn IPCPipe::sendAsync()\n> + * \\brief Send a message over IPC asynchronously\n> + * \\param[in] data Data to send, as an IPCMessage\n> + *\n> + * This function will return immediately after sending the message.\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\var IPCPipe::recv\n> + * \\brief Signal to be emitted when a message is received over IPC\n> + *\n> + * When data is received over IPC, this signal shall be emitted. Users must\n\ns/data/a message/\n\n> + * connect to this to receive messages.\n> + *\n> + * The parameters of the signal are an IPCMessage.\n\nDoesn't doxygen also show this ?\n\n> + */\n> +\n> +/**\n> + * \\var IPCPipe::connected_\n> + * \\brief Flag to indicate if the IPCPipe instance is connected\n> + *\n> + * An IPCPipe instance is connected if IPC is successfully set up.\n> + *\n> + * This flag can be read via IPCPipe::isConnected().\n> + *\n> + * Implementations of the IPCPipe class should set this flag upon successful\n> + * connection.\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 7d8ea00a..e736c41e 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -31,6 +31,7 @@ libcamera_sources = files([\n> 'ipa_manager.cpp',\n> 'ipa_module.cpp',\n> 'ipa_proxy.cpp',\n> + 'ipc_pipe.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 689D6BD162\n\tfor ;\n\tFri, 12 Feb 2021 01:27:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF24863752;\n\tFri, 12 Feb 2021 02:27:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 900E3602FC\n\tfor ;\n\tFri, 12 Feb 2021 02:26:59 +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 D274A8B5;\n\tFri, 12 Feb 2021 02:26:58 +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=\"ClLG93qA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613093219;\n\tbh=XZY902MHQ6yE7wTyAawvF4v2IhYjuOC9qeHoO9OOojc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ClLG93qA7XDd88ud2ywK3/Wih/1OMgKj1K9+hOwy3ncWlLnv9wDAinCnONAgyP1wD\n\t/MNX51VYbBbpgZV7c8K3Pubyb6s11PmZzOjkxTdNLFveTW+aSnD2ckCz6HmjuV4qqp\n\tO74hXIZiBEgynVZaM8syB7bosesQ0TlG1QOZqIy8=","Date":"Fri, 12 Feb 2021 03:26:33 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210211071805.34963-1-paul.elder@ideasonboard.com>\n\t<20210211071805.34963-8-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210211071805.34963-8-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 07/10] libcamera: Add IPCPipe","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\" "}}]