[{"id":13752,"web_url":"https://patchwork.libcamera.org/comment/13752/","msgid":"<20201117154228.kksyzrlxo2zx3sed@uno.localdomain>","date":"2020-11-17T15:42:28","subject":"Re: [libcamera-devel] [PATCH v4 10/37] 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, Nov 06, 2020 at 07:36:40PM +0900, Paul Elder wrote:\n> Add an implementation of IPAIPC using unix socket.\n>\n> Signed-off-by: Paul Elder \n>\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/ipa_ipc_unixsocket.h | 62 +++++\n> src/libcamera/ipa_ipc_unixsocket.cpp | 213 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 278 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 a6754a47..20fa1349 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> \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/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> new file mode 100644\n> index 00000000..f7248ca0\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 *ipaModulePath, const char *ipaProxyWorkerPath);\n> +\t~IPAIPCUnixSocket();\n> +\n> +\tint sendSync(uint32_t cmd,\n> +\t\t const std::vector &dataIn,\n> +\t\t const std::vector &fdsIn,\n> +\t\t std::vector *dataOut = nullptr,\n> +\t\t std::vector *fdsOut = nullptr) override;\n> +\n> +\tint sendAsync(uint32_t cmd,\n> +\t\t const std::vector &dataIn,\n> +\t\t const std::vector &fdsIn) 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\nThere's one thing I don't fully get yet.\n\nsendSycn/Async are methods of the interface, whatever IPC mechanism is\nused, the generated classes that call those methods are guaranteed to\nbe independent from it.\n\nwriteHeader and readHeader are specific to the IPCUnixSocket\nimplementation, and I see the generated worker using them and using\nexplicit calls to, for example, socket_.send() which is not IPC\nmechanism agnostic.\n\nIs this by-design ? The workers are meant to know which IPC mechamism\nis in use ?\n\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> +\tstd::unique_ptr proc_;\n> +\n> +\tstd::unique_ptr 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..eebb39fd\n> --- /dev/null\n> +++ b/src/libcamera/ipa_ipc_unixsocket.cpp\n> @@ -0,0 +1,213 @@\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 \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/ipa_ipc.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> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPAIPC)\n> +\n> +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipaModulePath,\n> +\t\t\t\t const char *ipaProxyWorkerPath)\n> +\t: IPAIPC(), seq_(0),\n> +\t proc_(nullptr), socket_(nullptr)\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(IPAIPC, Error) << \"Failed to create socket\";\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_ = std::make_unique();\n> +\tint ret = proc_->start(ipaProxyWorkerPath, args, fds);\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> +}\n> +\n> +int IPAIPCUnixSocket::sendSync(uint32_t cmd,\n> +\t\t\t const std::vector &dataIn,\n> +\t\t\t const std::vector &fdsIn,\n> +\t\t\t std::vector *dataOut,\n> +\t\t\t std::vector *fdsOut)\n> +{\n> +\tIPCUnixSocket::Payload message, response;\n> +\tint ret;\n> +\n> +\tmessage.data.reserve(8 + dataIn.size());\n> +\tmessage.fds.reserve(fdsIn.size());\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(), dataIn.begin(), dataIn.end());\n> +\n> +\tmessage.fds = const_cast &>(fdsIn);\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 (dataOut)\n> +\t\tdataOut->insert(dataOut->end(), response.data.begin(), response.data.end());\n> +\n> +\tif (fdsOut)\n> +\t\tfdsOut->insert(fdsOut->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 &dataIn,\n> +\t\t\t\tconst std::vector &fdsIn)\n> +{\n> +\tIPCUnixSocket::Payload message;\n> +\tint ret;\n> +\n> +\tmessage.data.reserve(8 + dataIn.size());\n> +\tmessage.fds.reserve(fdsIn.size());\n> +\n> +\twriteHeader(message, cmd, 0);\n> +\tmessage.data.insert(message.data.end(), dataIn.begin(), dataIn.end());\n> +\n> +\tmessage.fds = const_cast &>(fdsIn);\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> +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> +\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> +}\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> +\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 85f3a202..d6bd9a05 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 7FB3BBE081\n\tfor ;\n\tTue, 17 Nov 2020 15:42:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13B166331E;\n\tTue, 17 Nov 2020 16:42:28 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 645126033B\n\tfor ;\n\tTue, 17 Nov 2020 16:42:26 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id E522B40016;\n\tTue, 17 Nov 2020 15:42:25 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 17 Nov 2020 16:42:28 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20201117154228.kksyzrlxo2zx3sed@uno.localdomain>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-11-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-11-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 10/37] 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\" "}},{"id":13797,"web_url":"https://patchwork.libcamera.org/comment/13797/","msgid":"<20201119060527.GA1795@pyrite.rasen.tech>","date":"2020-11-19T06:05:27","subject":"Re: [libcamera-devel] [PATCH v4 10/37] libcamera: Add IPAIPC\n\timplementation based on unix socket","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Nov 18, 2020 at 10:37:54AM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Wed, Nov 18, 2020 at 06:25:50PM +0900, paul.elder@ideasonboard.com wrote:\n> > Hi Jacopo,\n> >\n> > On Tue, Nov 17, 2020 at 04:42:28PM +0100, Jacopo Mondi wrote:\n> > > Hi Paul,\n> > >\n> > > On Fri, Nov 06, 2020 at 07:36:40PM +0900, Paul Elder wrote:\n> > > > Add an implementation of IPAIPC using unix socket.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\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/ipa_ipc_unixsocket.h | 62 +++++\n> > > > src/libcamera/ipa_ipc_unixsocket.cpp | 213 ++++++++++++++++++\n> > > > src/libcamera/meson.build | 1 +\n> > > > 4 files changed, 278 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 a6754a47..20fa1349 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> > > > \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/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> > > > new file mode 100644\n> > > > index 00000000..f7248ca0\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 *ipaModulePath, const char *ipaProxyWorkerPath);\n> > > > +\t~IPAIPCUnixSocket();\n> > > > +\n> > > > +\tint sendSync(uint32_t cmd,\n> > > > +\t\t const std::vector &dataIn,\n> > > > +\t\t const std::vector &fdsIn,\n> > > > +\t\t std::vector *dataOut = nullptr,\n> > > > +\t\t std::vector *fdsOut = nullptr) override;\n> > > > +\n> > > > +\tint sendAsync(uint32_t cmd,\n> > > > +\t\t const std::vector &dataIn,\n> > > > +\t\t const std::vector &fdsIn) 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> > > There's one thing I don't fully get yet.\n> > >\n> > > sendSycn/Async are methods of the interface, whatever IPC mechanism is\n> > > used, the generated classes that call those methods are guaranteed to\n> > > be independent from it.\n> > >\n> > > writeHeader and readHeader are specific to the IPCUnixSocket\n> > > implementation, and I see the generated worker using them and using\n> > > explicit calls to, for example, socket_.send() which is not IPC\n> > > mechanism agnostic.\n> > >\n> > > Is this by-design ? The workers are meant to know which IPC mechamism\n> > > is in use ?\n> >\n> > Yeah... the worker is not /supposed/ to know which IPC mechanism is in\n> > use. I only realized after I finished everything, and Laurent agreed\n> > that we could wait until we get a new IPC mechanism to fix it. Ideally\n> > there should be an IPAIPC worker of some sort, another layer in between\n> > the main IPAIPC and the proxy worker.\n> \n> Oh I see.. as long as you and Laurent have validated this I'm fine.\n> \n> I now wonder if writeHeader/readHeader are not best added to the\n> IPCUnixSocket component and left out from the IPAIPCUnixSocket.\n> \n> The reason is that workers do not use the IPAIPC interface if not for\n> these functions, but they use IPCUnixSocket to communicate back with\n> the Proxy. Removing any dependency from IPAIPC from worker will make\n> it easier to isolate in a new interface the interactions with the IPC\n> mechanism in future. The IPAIPCUnixSocket will then use them from the\n> IPCUnixSocket library. If another IPC mechanism is in use, maybe a\n> different header format will be required, and IPAIPC and Workers will\n> use it from the new IPCSomething library.\n> \n> After all, writeHeader/readHeader have nothing IPA specific, they just\n> (de)serialize two integers in a buffer.\n\nWhen it comes time to supporting swapping out IPC mechanisms, we'll need\nan IPAIPC worker to receive the IPC calls and unwrap the serialized\ndata, and call into the Proxy worker.\n\nSo like... the IPAIPC (for UnixSocket) worker would have readyRead, and\nthen unpack the header (and other IPC-specific things), and then call\ninto a catch-all in the proxy worker while passing the serialized data,\nwhich would switch-case and deserialize and call the IPA.\n\nSo the current proxy worker still has to be torn in half anyway.\n\nBut I think you're right, neither the proxy worker nor the future IPAIPC\nworker actually need the IPAIPCUnixSocket. The IPAIPC worker would use\nIPCUnixSocket, and the proxy worker... nothing.\n\n\nThanks,\n\nPaul\n\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> > > > +\tstd::unique_ptr proc_;\n> > > > +\n> > > > +\tstd::unique_ptr 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..eebb39fd\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/ipa_ipc_unixsocket.cpp\n> > > > @@ -0,0 +1,213 @@\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 \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/ipa_ipc.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> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(IPAIPC)\n> > > > +\n> > > > +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipaModulePath,\n> > > > +\t\t\t\t const char *ipaProxyWorkerPath)\n> > > > +\t: IPAIPC(), seq_(0),\n> > > > +\t proc_(nullptr), socket_(nullptr)\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(IPAIPC, Error) << \"Failed to create socket\";\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_ = std::make_unique();\n> > > > +\tint ret = proc_->start(ipaProxyWorkerPath, args, fds);\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> > > > +}\n> > > > +\n> > > > +int IPAIPCUnixSocket::sendSync(uint32_t cmd,\n> > > > +\t\t\t const std::vector &dataIn,\n> > > > +\t\t\t const std::vector &fdsIn,\n> > > > +\t\t\t std::vector *dataOut,\n> > > > +\t\t\t std::vector *fdsOut)\n> > > > +{\n> > > > +\tIPCUnixSocket::Payload message, response;\n> > > > +\tint ret;\n> > > > +\n> > > > +\tmessage.data.reserve(8 + dataIn.size());\n> > > > +\tmessage.fds.reserve(fdsIn.size());\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(), dataIn.begin(), dataIn.end());\n> > > > +\n> > > > +\tmessage.fds = const_cast &>(fdsIn);\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 (dataOut)\n> > > > +\t\tdataOut->insert(dataOut->end(), response.data.begin(), response.data.end());\n> > > > +\n> > > > +\tif (fdsOut)\n> > > > +\t\tfdsOut->insert(fdsOut->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 &dataIn,\n> > > > +\t\t\t\tconst std::vector &fdsIn)\n> > > > +{\n> > > > +\tIPCUnixSocket::Payload message;\n> > > > +\tint ret;\n> > > > +\n> > > > +\tmessage.data.reserve(8 + dataIn.size());\n> > > > +\tmessage.fds.reserve(fdsIn.size());\n> > > > +\n> > > > +\twriteHeader(message, cmd, 0);\n> > > > +\tmessage.data.insert(message.data.end(), dataIn.begin(), dataIn.end());\n> > > > +\n> > > > +\tmessage.fds = const_cast &>(fdsIn);\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> > > > +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> > > > +\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> > > > +}\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> > > > +\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 85f3a202..d6bd9a05 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 123FFBE08A\n\tfor ;\n\tThu, 19 Nov 2020 06:05:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8066C6159F;\n\tThu, 19 Nov 2020 07:05:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C3ED260336\n\tfor ;\n\tThu, 19 Nov 2020 07:05:34 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1CBD7292;\n\tThu, 19 Nov 2020 07:05:32 +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=\"f7Gl1N5M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605765934;\n\tbh=bMIS/+Cs1hcWzzIHvhAgn0DUijja7/Nor5zjtN19boc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=f7Gl1N5MZebupHXeRvL8a7c/vI/15Z5/FPncKysD3+s4ZfH2uRSvIRgnaRc53WjYV\n\tjgmv4q5MYSNFQG6a3xPVqYazsluESQf/D85y4XBJ7WYVWWqf8wSumULgEW8/l8fOLV\n\tbw1Ti4z4xnF4fM5K66QVx8G6jkLNzUK2vE4f8OWg=","Date":"Thu, 19 Nov 2020 15:05:27 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20201119060527.GA1795@pyrite.rasen.tech>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-11-paul.elder@ideasonboard.com>\n\t<20201117154228.kksyzrlxo2zx3sed@uno.localdomain>\n\t<20201118092550.GE1856@pyrite.rasen.tech>\n\t<20201118093754.mitl77nkffad7u2j@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201118093754.mitl77nkffad7u2j@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 10/37] 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\" "}},{"id":13816,"web_url":"https://patchwork.libcamera.org/comment/13816/","msgid":"<20201120145015.GH4563@pendragon.ideasonboard.com>","date":"2020-11-20T14:50:15","subject":"Re: [libcamera-devel] [PATCH v4 10/37] libcamera: Add IPAIPC\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, Nov 19, 2020 at 03:05:27PM +0900, paul.elder@ideasonboard.com wrote:\n> On Wed, Nov 18, 2020 at 10:37:54AM +0100, Jacopo Mondi wrote:\n> > On Wed, Nov 18, 2020 at 06:25:50PM +0900, paul.elder@ideasonboard.com wrote:\n> > > On Tue, Nov 17, 2020 at 04:42:28PM +0100, Jacopo Mondi wrote:\n> > > > On Fri, Nov 06, 2020 at 07:36:40PM +0900, Paul Elder wrote:\n> > > > > Add an implementation of IPAIPC using unix socket.\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > >\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/ipa_ipc_unixsocket.h | 62 +++++\n> > > > > src/libcamera/ipa_ipc_unixsocket.cpp | 213 ++++++++++++++++++\n> > > > > src/libcamera/meson.build | 1 +\n> > > > > 4 files changed, 278 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 a6754a47..20fa1349 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> > > > > \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/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> > > > > new file mode 100644\n> > > > > index 00000000..f7248ca0\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\nYou need , and too.\n\n> > > > > +\n> > > > > +#include \n\nThis is not needed.\n\n> > > > > +\n> > > > > +#include \"libcamera/internal/ipa_ipc.h\"\n> > > > > +#include \"libcamera/internal/ipa_module.h\"\n\nNeither is ipa_module.h (you will need it in the .cpp file though).\n\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 *ipaModulePath, const char *ipaProxyWorkerPath);\n> > > > > +\t~IPAIPCUnixSocket();\n> > > > > +\n> > > > > +\tint sendSync(uint32_t cmd,\n> > > > > +\t\t const std::vector &dataIn,\n> > > > > +\t\t const std::vector &fdsIn,\n> > > > > +\t\t std::vector *dataOut = nullptr,\n> > > > > +\t\t std::vector *fdsOut = nullptr) override;\n> > > > > +\n> > > > > +\tint sendAsync(uint32_t cmd,\n> > > > > +\t\t const std::vector &dataIn,\n> > > > > +\t\t const std::vector &fdsIn) 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> > > > There's one thing I don't fully get yet.\n> > > >\n> > > > sendSycn/Async are methods of the interface, whatever IPC mechanism is\n> > > > used, the generated classes that call those methods are guaranteed to\n> > > > be independent from it.\n> > > >\n> > > > writeHeader and readHeader are specific to the IPCUnixSocket\n> > > > implementation, and I see the generated worker using them and using\n> > > > explicit calls to, for example, socket_.send() which is not IPC\n> > > > mechanism agnostic.\n> > > >\n> > > > Is this by-design ? The workers are meant to know which IPC mechamism\n> > > > is in use ?\n> > >\n> > > Yeah... the worker is not /supposed/ to know which IPC mechanism is in\n> > > use. I only realized after I finished everything, and Laurent agreed\n> > > that we could wait until we get a new IPC mechanism to fix it. Ideally\n> > > there should be an IPAIPC worker of some sort, another layer in between\n> > > the main IPAIPC and the proxy worker.\n> > \n> > Oh I see.. as long as you and Laurent have validated this I'm fine.\n> > \n> > I now wonder if writeHeader/readHeader are not best added to the\n> > IPCUnixSocket component and left out from the IPAIPCUnixSocket.\n> > \n> > The reason is that workers do not use the IPAIPC interface if not for\n> > these functions, but they use IPCUnixSocket to communicate back with\n> > the Proxy. Removing any dependency from IPAIPC from worker will make\n> > it easier to isolate in a new interface the interactions with the IPC\n> > mechanism in future. The IPAIPCUnixSocket will then use them from the\n> > IPCUnixSocket library. If another IPC mechanism is in use, maybe a\n> > different header format will be required, and IPAIPC and Workers will\n> > use it from the new IPCSomething library.\n> > \n> > After all, writeHeader/readHeader have nothing IPA specific, they just\n> > (de)serialize two integers in a buffer.\n> \n> When it comes time to supporting swapping out IPC mechanisms, we'll need\n> an IPAIPC worker to receive the IPC calls and unwrap the serialized\n> data, and call into the Proxy worker.\n> \n> So like... the IPAIPC (for UnixSocket) worker would have readyRead, and\n> then unpack the header (and other IPC-specific things), and then call\n> into a catch-all in the proxy worker while passing the serialized data,\n> which would switch-case and deserialize and call the IPA.\n> \n> So the current proxy worker still has to be torn in half anyway.\n> \n> But I think you're right, neither the proxy worker nor the future IPAIPC\n> worker actually need the IPAIPCUnixSocket. The IPAIPC worker would use\n> IPCUnixSocket, and the proxy worker... nothing.\n\nFollowing the review comments on \"[PATCH v4 09/37] libcamera: Add\nIPAIPC\", and the proposed introduction of an IPCMessage class, how about\nmoving those three methods to IPCMessage ? They should actually then\nbecome internal, as IPCMessage would be constructed from a payload. The\nIPCUnixSocket::Payload is specified to IPCUnixSocket so that's a bit of\nan issue, but we could move it out to ipa_ipc.h and name it IPCPayload.\nIPCMessage would be constructed from an IPCPayload on the receive path,\nand would create a payload on the send path (with an\nIPCMessage::payload()) method.\n\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> > > > > +\tstd::unique_ptr proc_;\n> > > > > +\n> > > > > +\tstd::unique_ptr socket_;\n> > > > > +\n> > > > > +\tstd::map callData_;\n\ns/struct //\n\nYou could drop blank lines.\n\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..eebb39fd\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/ipa_ipc_unixsocket.cpp\n> > > > > @@ -0,0 +1,213 @@\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 \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/ipa_ipc.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> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DECLARE_CATEGORY(IPAIPC)\n> > > > > +\n> > > > > +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipaModulePath,\n> > > > > +\t\t\t\t const char *ipaProxyWorkerPath)\n> > > > > +\t: IPAIPC(), seq_(0),\n> > > > > +\t proc_(nullptr), socket_(nullptr)\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(IPAIPC, Error) << \"Failed to create socket\";\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_ = std::make_unique();\n> > > > > +\tint ret = proc_->start(ipaProxyWorkerPath, args, fds);\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> > > > > +}\n\nYou can drop the destructor if it's empty.\n\n> > > > > +\n> > > > > +int IPAIPCUnixSocket::sendSync(uint32_t cmd,\n> > > > > +\t\t\t const std::vector &dataIn,\n> > > > > +\t\t\t const std::vector &fdsIn,\n> > > > > +\t\t\t std::vector *dataOut,\n> > > > > +\t\t\t std::vector *fdsOut)\n> > > > > +{\n> > > > > +\tIPCUnixSocket::Payload message, response;\n> > > > > +\tint ret;\n> > > > > +\n> > > > > +\tmessage.data.reserve(8 + dataIn.size());\n> > > > > +\tmessage.fds.reserve(fdsIn.size());\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(), dataIn.begin(), dataIn.end());\n> > > > > +\n> > > > > +\tmessage.fds = const_cast &>(fdsIn);\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 (dataOut)\n> > > > > +\t\tdataOut->insert(dataOut->end(), response.data.begin(), response.data.end());\n> > > > > +\n> > > > > +\tif (fdsOut)\n> > > > > +\t\tfdsOut->insert(fdsOut->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 &dataIn,\n> > > > > +\t\t\t\tconst std::vector &fdsIn)\n> > > > > +{\n> > > > > +\tIPCUnixSocket::Payload message;\n> > > > > +\tint ret;\n> > > > > +\n> > > > > +\tmessage.data.reserve(8 + dataIn.size());\n> > > > > +\tmessage.fds.reserve(fdsIn.size());\n> > > > > +\n> > > > > +\twriteHeader(message, cmd, 0);\n> > > > > +\tmessage.data.insert(message.data.end(), dataIn.begin(), dataIn.end());\n> > > > > +\n> > > > > +\tmessage.fds = const_cast &>(fdsIn);\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\nThere's similar code in the two functions, merging sendSync() and\nsendAsync() with flags to select sync or async behaviour would I think\nlower code duplication.\n\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> > > > > +\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\nHow about creating a structure to define the header ?\n\n\tstruct IPCMessageHeader {\n\t\tuint32_t cmd;\n\t\tuint32_t seq;\n\t};\n\n\tIPCMessageHeader *header;\n\tpayload.data.insert(payload.data.begin(), sizeof(*header), 0);\n\tnew (payload.data.data()) IPCMessageHeader{ cmd, seq };\n\nThe last line is referred as \"placement new\"\n(https://en.cppreference.com/w/cpp/language/new#Placement_new) and\nconstructs an object in allocated storage.\n\nI'm tempted to rename cmd to type, name or id, as it identifies the type\nof message. I would also rename seq to cookie (or requestId or ...), as\nit's meant to match requests with replies, and the protocol thus doesn't\nneed to depend on the value being a sequence number (the implementation\ncan of course use a sequence number).\n\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> > > > > +}\n\nWe could do something similar here.\n\n> > > > > +\n> > > > > +void IPAIPCUnixSocket::eraseHeader(IPCUnixSocket::Payload &payload)\n> > > > > +{\n> > > > > +\tpayload.data.erase(payload.data.begin(), payload.data.begin() + 8);\n> > > > > +}\n\nCould we avoid the need to call eraseHeader() by using Span and/or\niterators appropriately ? Dropping 8 bytes at the beginning of the\nmessage is a fairly expensive operation.\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\nEvents sent by the IPA have a sequence number hardcoded to 0 in the\nproxy worker. This means that when the sequence number wraps around in\nIPAIPCUnixSocket::sendSync(), there's a risk an event would be\nincorrectly interpreted as a response.\n\nYou could consider 0 as a reserved value for sequence numbers (and thus\nskip it in IPAIPCUnixSocket::sendSync()), or add flags to the message\nheader to indicate that the message is a response. I think flags would\nmake sense, we may need more flags later.\n\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\n\t= std::move(message);\n\n(making sure that Payload has appropriate move constructor and\nassignment operator, either explicit, or implicit).\n\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\nI'd drop the last sentence, and possibly even the last two sentences, as\nthere's nothing requiring this class to be used only by proxies.\n\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\nLine wrap ?\n\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\nThe event loop wasn't developed to be reentrant. I think this is a very\nvalid use case, which means we'll likely need to fix an issue or two in\nthe event dispatcher to support this properly.\n\nIt also means that the pipeline handler could receive another event\nwhile waiting for the call to complete. This part worries me a bit as it\nwill be hard to implement it properly in pipeline handlers. See\nhttps://docs.google.com/document/d/1dixzFzZQW8e3ldjdM8Adbo8klXDDE4pVekwo5aLgUsE/edit#\nfor some background information.\n\nThe IPA protocols we are envisioning should not mix sync and async\nmessages : everything is synchronous until we start the IPA, after which\nmessages are asynchronous until we stop it. This should give us some\nsafety. I'm not sure if we'll easily be able to enforce that though.\n\nThere are other options, such as split the camera manager thread to one\nthread per pipeline handler, and only processing IPC events here (we\nwould need to extend the event dispatcher API to make this possible).\nThe drawback is that pipeline handlers that handle multiple cameras\nwould all of a sudden block event processing for all the cameras when a\nsync call is made, which isn't nice at all. Moving to one thread per\ncamera would help, but then pipeline handlers would suddenly become\nmulti-threaded when they support multiple cameras, which is a completely\ndifferent can of worms.\n\nIdeas are welcome :-)\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 85f3a202..d6bd9a05 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',","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 F0238BE08A\n\tfor ;\n\tFri, 20 Nov 2020 14:50:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 793CA625AC;\n\tFri, 20 Nov 2020 15:50:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0BEA6220D\n\tfor ;\n\tFri, 20 Nov 2020 15:50:23 +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 19B9C2A3;\n\tFri, 20 Nov 2020 15:50:22 +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=\"wpuifeYs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605883823;\n\tbh=we2i8DbI3kIa33dycEhYGfZrDQl3/x76ct/gQS6lAAU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wpuifeYsE0Lj0+G4icqVfY2EKMjyHz0Vlgm6ppTJvVPG8UuX8OBKGxUVR4k/ESojj\n\tjO10pS7hueQIqE81WsGEJ4/wB8njLJZetof8W027k13W6bhGUSsJTJHSlucVn6Avuw\n\td9A8nhqKuEgaDYu3AwK6WBWDr7Y27gvkshh15it4=","Date":"Fri, 20 Nov 2020 16:50:15 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"<20201120145015.GH4563@pendragon.ideasonboard.com>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-11-paul.elder@ideasonboard.com>\n\t<20201117154228.kksyzrlxo2zx3sed@uno.localdomain>\n\t<20201118092550.GE1856@pyrite.rasen.tech>\n\t<20201118093754.mitl77nkffad7u2j@uno.localdomain>\n\t<20201119060527.GA1795@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201119060527.GA1795@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v4 10/37] 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\" "}}]