[{"id":13555,"web_url":"https://patchwork.libcamera.org/comment/13555/","msgid":"<20201029125726.2lerjzzchzlahzgp@uno.localdomain>","date":"2020-10-29T12:57:26","subject":"Re: [libcamera-devel] [PATCH v3 12/38] libcamera: Add IPAIPC\n\timplementation based on unix socket","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Oct 02, 2020 at 11:31:28PM +0900, Paul Elder wrote:\n> Add an implementation of IPAIPC using unix socket.\n\nSpell UNIX with capital letter ?\n\n>\n> Signed-off-by: Paul Elder \n>\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/ipa_ipc_unixsocket.h | 62 +++++\n> src/libcamera/ipa_ipc_unixsocket.cpp | 211 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 276 insertions(+)\n> create mode 100644 include/libcamera/internal/ipa_ipc_unixsocket.h\n> create mode 100644 src/libcamera/ipa_ipc_unixsocket.cpp\n>\n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 598eca9b..71509ff7 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/ipa_ipc_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/ipa_ipc_unixsocket.cpp \\\n\nI wonder if we shouldn't make a src/libcamera/ipc/\n\n> \t\t\t @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> \t\t\t @TOP_SRCDIR@/src/libcamera/proxy/ \\\n> \t\t\t @TOP_BUILDDIR@/include/libcamera/ipa/ \\\n> diff --git a/include/libcamera/internal/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> new file mode 100644\n> index 00000000..11f05b12\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> @@ -0,0 +1,62 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipa_ipc_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> +\n> +#include \n> +\n> +#include \"libcamera/internal/ipa_ipc.h\"\n> +#include \"libcamera/internal/ipa_module.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +\n> +namespace libcamera {\n> +\n> +class Process;\n> +\n> +class IPAIPCUnixSocket : public IPAIPC\n> +{\n> +public:\n> +\tIPAIPCUnixSocket(const char *ipa_module_path, const char *ipa_proxy_worker_path);\n> +\t~IPAIPCUnixSocket();\n> +\n> +\tint sendSync(uint32_t cmd,\n> +\t\t const std::vector &data_in,\n> +\t\t const std::vector &fds_in,\n> +\t\t std::vector *data_out = nullptr,\n> +\t\t std::vector *fds_out = nullptr) override;\n> +\n> +\tint sendAsync(uint32_t cmd,\n> +\t\t const std::vector &data_in,\n> +\t\t const std::vector &fds_in) override;\n> +\n> +\tstatic void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq);\n> +\tstatic std::tuple readHeader(IPCUnixSocket::Payload &payload);\n> +\tstatic void eraseHeader(IPCUnixSocket::Payload &payload);\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, IPCUnixSocket::Payload *response, uint32_t seq);\n> +\n> +\tuint32_t seq_;\n> +\n> +\tProcess *proc_;\n> +\n> +\tIPCUnixSocket *socket_;\n> +\n> +\tstd::map callData_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */\n> diff --git a/src/libcamera/ipa_ipc_unixsocket.cpp b/src/libcamera/ipa_ipc_unixsocket.cpp\n> new file mode 100644\n> index 00000000..6dc92768\n> --- /dev/null\n> +++ b/src/libcamera/ipa_ipc_unixsocket.cpp\n> @@ -0,0 +1,211 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipa_ipc_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket\n> + */\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n\nThis one should go first\n\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/process.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/ipa_ipc.h\"\n> +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPAIPC)\n> +\n> +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipa_module_path,\n> +\t\t\t\t const char *ipa_proxy_worker_path)\n\nI get this complaint from doxygen (on ipa_ipc.c actually)\n\n/home/jmondi/project/libcamera/libcamera.git/src/libcamera/ipa_ipc.cpp:38: warning: argument 'ipa_module_path' of command @param is not found in the argument list of libcamera::IPAIPC::IPAIPC(const char *ipa_module_path][[maybe_unused], const char *ipa_proxy_worker_path][[maybe_unused])\n/home/jmondi/project/libcamera/libcamera.git/src/libcamera/ipa_ipc.cpp:38: warning: argument 'ipa_proxy_worker_path' of command @param is not found in the argument list of libcamera::IPAIPC::IPAIPC(const char *ipa_module_path][[maybe_unused], const char *ipa_proxy_worker_path][[maybe_unused])\n\nYou don't ? (doxygen 1.8.20 here)\n\n> +\t: IPAIPC(ipa_module_path, ipa_proxy_worker_path), seq_(0),\n> +\t proc_(nullptr), socket_(nullptr)\n> +{\n> +\tstd::vector fds;\n> +\tstd::vector args;\n> +\targs.push_back(ipa_module_path);\n> +\n> +\tsocket_ = new IPCUnixSocket();\n> +\tint fd = socket_->create();\n> +\tif (fd < 0) {\n> +\t\tLOG(IPAIPC, Error)\n> +\t\t\t<< \"Failed to create socket\";\n\nFits on one line ?\n\n> +\t\treturn;\n> +\t}\n> +\tsocket_->readyRead.connect(this, &IPAIPCUnixSocket::readyRead);\n> +\targs.push_back(std::to_string(fd));\n> +\tfds.push_back(fd);\n> +\n> +\tproc_ = new Process();\n> +\tint ret = proc_->start(ipa_proxy_worker_path, args, fds);\n\nWhat defines the proxy_worker interface ? ie that it wants 'fd' as\nfirst argument ? (not that it is wrong but just to know where it is\ndefined)\n\n> +\tif (ret) {\n> +\t\tLOG(IPAIPC, Error)\n> +\t\t\t<< \"Failed to start proxy worker process\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tvalid_ = true;\n> +}\n> +\n> +IPAIPCUnixSocket::~IPAIPCUnixSocket()\n> +{\n> +\tdelete proc_;\n> +\tdelete socket_;\n\nwe can save this by making proc_ and socket_ unique_ptr<>\n\n> +}\n> +\n> +int IPAIPCUnixSocket::sendSync(uint32_t cmd,\n> +\t\t\t const std::vector &data_in,\n> +\t\t\t const std::vector &fds_in,\n> +\t\t\t std::vector *data_out,\n> +\t\t\t std::vector *fds_out)\n> +{\n> +\tIPCUnixSocket::Payload message, response;\n\nIt would be easy to pre-reserve message.data\n8 bytes for the header + data_in.size()\n\nThis would avoid at least two potential relocations\n\nSame for fds, not possible for response unless we over-allocate.\n\n> +\tint ret;\n> +\n> +\t/* It's fine if seq_ overflows; that'll just be the new epoch. */\n> +\tseq_++;\n> +\twriteHeader(message, cmd, seq_);\n> +\tmessage.data.insert(message.data.end(), data_in.begin(), data_in.end());\n> +\n> +\tmessage.fds = const_cast &>(fds_in);\n> +\n> +\tret = call(message, &response, seq_);\n> +\tif (ret) {\n> +\t\tLOG(IPAIPC, Error) << \"Failed to call sync\";\n> +\t\tcallData_.erase(seq_);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (data_out)\n> +\t\tdata_out->insert(data_out->end(), response.data.begin(), response.data.end());\n> +\n> +\tif (fds_out)\n> +\t\tfds_out->insert(fds_out->end(), response.fds.begin(), response.fds.end());\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPAIPCUnixSocket::sendAsync(uint32_t cmd,\n> +\t\t\t\tconst std::vector &data_in,\n> +\t\t\t\tconst std::vector &fds_in)\n> +{\n> +\tIPCUnixSocket::Payload message;\n> +\tint ret;\n> +\n> +\twriteHeader(message, cmd, 0);\n> +\tmessage.data.insert(message.data.end(), data_in.begin(), data_in.end());\n> +\n> +\tmessage.fds = const_cast &>(fds_in);\n> +\n> +\tret = socket_->send(message);\n> +\tif (ret) {\n> +\t\tLOG(IPAIPC, Error) << \"Failed to call async\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +\n\nDouble empty line ?\n\n> +void IPAIPCUnixSocket::writeHeader(IPCUnixSocket::Payload &payload,\n> +\t\t\t\t uint32_t cmd, uint32_t seq)\n> +{\n> +\tuint8_t cmd_arr[] = {static_cast(cmd),\n> +\t\tstatic_cast((cmd >> 8)),\n> +\t\tstatic_cast((cmd >> 16)),\n> +\t\tstatic_cast((cmd >> 24))};\n> +\tuint8_t seq_arr[] = {static_cast(seq),\n> +\t\tstatic_cast((seq >> 8)),\n> +\t\tstatic_cast((seq >> 16)),\n> +\t\tstatic_cast((seq >> 24))};\n\nDo we care about endianesa between IPA and pipeline handlers ? Unless we run IPA\non a different CPU it should not matter, righ ?\n\nBecause otherwise you can take a uint8_t pointer to cmd and seq and just:\n\n \tuint8_t *p = reinterpret_cast(&cmd);\n\tpayload.data.insert(payload.data.begin(), p, p + 4);\n \tp = reinterpret_cast(&seq);\n\tpayload.data.insert(payload.data.begin(), p, p + 4);\n\nand make this an inline function\n\n> +\tpayload.data.insert(payload.data.begin(), cmd_arr, cmd_arr+4);\n> +\tpayload.data.insert(payload.data.begin() + 4, seq_arr, seq_arr+4);\n> +}\n> +\n> +std::tuple\n> +IPAIPCUnixSocket::readHeader(IPCUnixSocket::Payload &payload)\n> +{\n> +\tuint32_t cmd = payload.data[0] |\n> +\t\t(payload.data[1] << 8) |\n> +\t\t(payload.data[2] << 16) |\n> +\t\t(payload.data[3] << 24);\n> +\tuint32_t seq = payload.data[4] |\n> +\t\t(payload.data[5] << 8) |\n> +\t\t(payload.data[6] << 16) |\n> +\t\t(payload.data[7] << 24);\n> +\n> +\treturn {cmd, seq};\n\nsame here by reintrpreting payload.data[0] and payload.data[4] as\nuint32_t\n\n> +}\n> +\n> +void IPAIPCUnixSocket::eraseHeader(IPCUnixSocket::Payload &payload)\n> +{\n> +\tpayload.data.erase(payload.data.begin(), payload.data.begin() + 8);\n> +}\n> +\n> +void IPAIPCUnixSocket::readyRead(IPCUnixSocket *socket)\n> +{\n> +\tIPCUnixSocket::Payload message;\n> +\tint ret = socket->receive(&message);\n> +\tif (ret) {\n> +\t\tLOG(IPAIPC, Error) << \"Receive message failed\" << ret;\n> +\t\treturn;\n> +\t}\n> +\n> +\tuint32_t cmd, seq;\n> +\tstd::tie(cmd, seq) = readHeader(message);\n> +\n> +\tauto callData = callData_.find(seq);\n> +\tif (callData != callData_.end()) {\n> +\t\teraseHeader(message);\n> +\t\t/* Is there any way to avoid this copy? */\n> +\t\t*callData->second.response = message;\n> +\t\tcallData->second.done = true;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * Received unexpected data, this means it's a call from the IPA.\n> +\t * We can't return anything to the IPA (gotta keep them under *our*\n> +\t * control, plus returning would require blocking the caller, and we\n> +\t * can't afford to do that). Let the proxy do switch-case on cmd.\n> +\t */\n> +\trecvIPC.emit(message.data, message.fds);\n> +\n> +\treturn;\n> +}\n> +\n> +int IPAIPCUnixSocket::call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq)\n> +{\n> +\tTimer timeout;\n> +\tint ret;\n> +\n> +\tcallData_[seq].response = response;\n> +\tcallData_[seq].done = false;\n> +\n> +\tret = socket_->send(message);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\ttimeout.start(200);\n> +\twhile (!callData_[seq].done) {\n> +\t\tif (!timeout.isRunning()) {\n> +\t\t\tLOG(IPAIPC, Error) << \"Call timeout!\";\n> +\t\t\tcallData_.erase(seq);\n> +\t\t\treturn -ETIMEDOUT;\n> +\t\t}\n> +\n> +\t\tThread::current()->eventDispatcher()->processEvents();\n\nI'll more carefully review this last part later, but I would have a\nquestion, possibly for Laurent: do we have or need to implement a\ntimed wait condition support in the thread library, taking a condition\nvariable and a timeout and implementing this part ? It should be\npaired with a condition signaling couter-part. This would avoid having\nlibrary classes poking the eventDispatcher themselves.\n\nThanks\n j\n\n> +\t}\n> +\n> +\tcallData_.erase(seq);\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index e0a2ab47..59417403 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -26,6 +26,7 @@ libcamera_sources = files([\n> 'ipa_controls.cpp',\n> 'ipa_data_serializer.cpp',\n> 'ipa_ipc.cpp',\n> + 'ipa_ipc_unixsocket.cpp',\n> 'ipa_interface.cpp',\n> 'ipa_manager.cpp',\n> 'ipa_module.cpp',\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 4B1CDBDB1E\n\tfor ;\n\tThu, 29 Oct 2020 12:57:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE084628AF;\n\tThu, 29 Oct 2020 13:57:28 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4238E61563\n\tfor ;\n\tThu, 29 Oct 2020 13:57:27 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id B3D46100010;\n\tThu, 29 Oct 2020 12:57:26 +0000 (UTC)"],"Date":"Thu, 29 Oct 2020 13:57:26 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20201029125726.2lerjzzchzlahzgp@uno.localdomain>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-13-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002143154.468162-13-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 12/38] libcamera: Add IPAIPC\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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]