[{"id":14358,"web_url":"https://patchwork.libcamera.org/comment/14358/","msgid":"","date":"2020-12-27T11:49:06","subject":"Re: [libcamera-devel] [PATCH v6 8/9] libcamera: Add IPCPipe\n\timplementation based on unix socket","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for your nice work.\n\nOn 2020-12-24 17:15:33 +0900, Paul Elder wrote:\n> Add an implementation of IPCPipe using unix socket.\n> \n> Signed-off-by: Paul Elder \n\nReviewed-by: Niklas Söderlund \n\n> \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 | 50 ++++++\n> src/libcamera/ipc_pipe_unixsocket.cpp | 147 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 200 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 b18b8e9c..36a0cef3 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..fea3179c\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> @@ -0,0 +1,50 @@\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(uint32_t cmd,\n> +\t\t const IPCMessage &in,\n> +\t\t IPCMessage *out = nullptr) override;\n> +\n> +\tint sendAsync(uint32_t cmd, 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, IPCUnixSocket::Payload *response, uint32_t seq);\n> +\n> +\tuint32_t seq_;\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..43c20e6b\n> --- /dev/null\n> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> @@ -0,0 +1,147 @@\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(), seq_(0)\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(uint32_t cmd,\n> +\t\t\t\tconst IPCMessage &in, IPCMessage *out)\n> +{\n> +\tIPCUnixSocket::Payload message, response;\n> +\n> +\tconst IPCMessage::Header header{ cmd, ++seq_ };\n> +\tmessage = in.payload(&header);\n> +\n> +\tint ret = call(message, &response, seq_);\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(uint32_t cmd, const IPCMessage &data)\n> +{\n> +\tconst IPCMessage::Header header{ cmd, 0 };\n> +\tIPCUnixSocket::Payload message = data.payload(&header);\n> +\n> +\tint ret = socket_->send(message);\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 message;\n> +\tint ret = socket->receive(&message);\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(message);\n> +\n> +\tauto callData = callData_.find(ipcMessage.header().cookie);\n> +\tif (callData != callData_.end()) {\n> +\t\t*callData->second.response = std::move(message);\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.header().cmd, ipcMessage);\n> +\n> +\treturn;\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> +\tcallData_[cookie].response = response;\n> +\tcallData_[cookie].done = false;\n> +\n> +\tret = socket_->send(message);\n> +\tif (ret) {\n> +\t\tcallData_.erase(seq_);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\ttimeout.start(2000);\n> +\twhile (!callData_[cookie].done) {\n> +\t\tif (!timeout.isRunning()) {\n> +\t\t\tLOG(IPCPipe, Error) << \"Call timeout!\";\n> +\t\t\tcallData_.erase(cookie);\n> +\t\t\treturn -ETIMEDOUT;\n> +\t\t}\n> +\n> +\t\tThread::current()->eventDispatcher()->processEvents();\n> +\t}\n> +\n> +\tcallData_.erase(cookie);\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index b7cfe0c2..cd3cf9e3 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -31,6 +31,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',\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 0724CC0F1A\n\tfor ;\n\tSun, 27 Dec 2020 11:49:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 722606200D;\n\tSun, 27 Dec 2020 12:49:13 +0100 (CET)","from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D958615D2\n\tfor ;\n\tSun, 27 Dec 2020 12:49:12 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id l11so18255707lfg.0\n\tfor ;\n\tSun, 27 Dec 2020 03:49:12 -0800 (PST)","from localhost ([185.224.57.161]) by smtp.gmail.com with ESMTPSA id\n\tg4sm4557803lfc.85.2020.12.27.03.49.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 27 Dec 2020 03:49:11 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"FXkLUPR+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=arhTSTXjd22KLI+HuAouf4G9WqyfefgpOfbH9M6ydbo=;\n\tb=FXkLUPR+7qwORBirJGTgZUPBI+xJn4SdpYhx81Gwv7UQmUZjKkFPs/WGBdR/nK4cRu\n\tG0s842b7AQqZu+mqPmSB8C9r+h6CzvpBWt7yBUZHV+ThBzayHWNIoLsZzIwiQbnMm7c1\n\tKj/OQmaCuOqzre5vZTDEJVCLiWDnJLFnkJbX/bT6MXh4DiBv7+KFsI/recwqXF+KS+UZ\n\tQO7QeB0LKc0Xu278z/vKi6iW9fdLbQWrSDxrh+l2B9fGCKZmOc+X0OE/2ZlmkinoGfGL\n\tifBNLjPTpxxjtWxHSBfr+PxGRf/XFtUJYAzWwiuDXHQxJwocjz4oHRJZzEckdrJfRx/2\n\txamw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=arhTSTXjd22KLI+HuAouf4G9WqyfefgpOfbH9M6ydbo=;\n\tb=M7TQDFDjXN8+l1+uQtva7bImkWYMMXLR2HCri7FB60qKht3cMsMd6Z++mfZU811l5C\n\tet6sDBcA7ER6DciYnjNR1BYsZJCqeTWx31bHNfUWDSpcNJrGLqclCFTnwW+EBpK5Q2Gd\n\tUFiwPkVonw1J160gjlCAoiQXlQnNNBPImxbeTb1y4jl6MJxUvc4NxJGtXRn0HS/dptuZ\n\t6jqC7xecLbLd6dJa1p3bGlIgz+rlJ5ik+YZQNrn1G0+RpwoUXg9HNiHR5pthwsjG9OJ3\n\tnJbxO4ag6p6M3O0Nvpj0L7J/seSGtmnpUweaPwfVvhhWbcNN6R1BfskqQ1sCt5FMqscV\n\tz2vg==","X-Gm-Message-State":"AOAM532L2JIzVHJgEsFeWj9QKH91WwQrBJ5lduVXO44e50JgQOUmF3a6\n\tKIHH41O8YKm28q1GjHoeGKW28wM6i9e8bg==","X-Google-Smtp-Source":"ABdhPJxV/GvuFTljBlKr6721G3shYTHbVPyJSHwe5wSN91AkStr6WaF41w2N3m8iyQ3occKnRP6WGQ==","X-Received":"by 2002:a2e:9d8e:: with SMTP id c14mr18897185ljj.7.1609069751919;\n\tSun, 27 Dec 2020 03:49:11 -0800 (PST)","Date":"Sun, 27 Dec 2020 12:49:06 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Paul Elder ","Message-ID":"","References":"<20201224081534.41601-1-paul.elder@ideasonboard.com>\n\t<20201224081534.41601-9-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201224081534.41601-9-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 8/9] 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14897,"web_url":"https://patchwork.libcamera.org/comment/14897/","msgid":"","date":"2021-02-02T02:29:38","subject":"Re: [libcamera-devel] [PATCH v6 8/9] 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, Dec 24, 2020 at 05:15:33PM +0900, Paul Elder wrote:\n> Add an implementation of IPCPipe using unix socket.\n> \n> Signed-off-by: Paul Elder \n> \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 | 50 ++++++\n> src/libcamera/ipc_pipe_unixsocket.cpp | 147 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 200 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 b18b8e9c..36a0cef3 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..fea3179c\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> @@ -0,0 +1,50 @@\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(uint32_t cmd,\n> +\t\t const IPCMessage &in,\n> +\t\t IPCMessage *out = nullptr) override;\n> +\n> +\tint sendAsync(uint32_t cmd, 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, IPCUnixSocket::Payload *response, uint32_t seq);\n\nLine wrap ?\n\n> +\n> +\tuint32_t seq_;\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..43c20e6b\n> --- /dev/null\n> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> @@ -0,0 +1,147 @@\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(), seq_(0)\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\nI've recently learnt that you can also write\n\nIPCPipeUnixSocket::~IPCPipeUnixSocket() = default;\n\n(which doesn't mean you have to change this)\n\n> +\n> +int IPCPipeUnixSocket::sendSync(uint32_t cmd,\n> +\t\t\t\tconst IPCMessage &in, IPCMessage *out)\n> +{\n> +\tIPCUnixSocket::Payload message, response;\n\ns/message/request/ ?\n\n> +\n> +\tconst IPCMessage::Header header{ cmd, ++seq_ };\n> +\tmessage = in.payload(&header);\n\n\tIPCUnixSocket::Payload message = in.payload(&header);\n\tIPCUnixSocket::Payload response;\n\n> +\n> +\tint ret = call(message, &response, seq_);\n\nOr maybe\n\n\tint ret = call(in.payload(&header), &response, seq_);\n\nwhich would be nice to turn into\n\n\tint ret = call(in.payload(), &response, seq_);\n\nfollowing my comments on the previous patch. However, we have here an\nissue as we need to set the sequence number, and in is const. Maybe the\npayload() function could take a sequence number instead of a header\npointer ?\n\nOr maybe this is too much churn and we can keep it as-is until we decide\nto refactor the code. Feel free to clean up what you think makes sense\nto clean up already, and leave the rest out.\n\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(uint32_t cmd, const IPCMessage &data)\n> +{\n> +\tconst IPCMessage::Header header{ cmd, 0 };\n> +\tIPCUnixSocket::Payload message = data.payload(&header);\n\nHere, it would be really nice if we didn't have to pass cmd to\nsendAsync(), and data already had cmd set in the header.\n\ns/message/request/ ?\n\nand s/data/msg/ (or message)\n\n> +\n> +\tint ret = socket_->send(message);\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 message;\n\ns/message/payload/ ?\n\n> +\tint ret = socket->receive(&message);\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(message);\n\n\tif (message.data.size() < sizeof(IPCMessage::Header)) {\n\t\tLOG(...) << ...;\n\t\treturn;\n\t}\n\n\tIPCMessage::Header *header =\n\t\tstatic_cast(messaga.data.data());\n\n> +\n> +\tauto callData = callData_.find(ipcMessage.header().cookie);\n\n\tauto callData = callData_.find(header->cookie);\n\n> +\tif (callData != callData_.end()) {\n> +\t\t*callData->second.response = std::move(message);\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.header().cmd, ipcMessage);\n\nCould we drop the cmd argument, as it's available through the message ?\n\n\trecv.emit(IPCMessage(message));\n\n> +\n> +\treturn;\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> +\tcallData_[cookie].response = response;\n> +\tcallData_[cookie].done = false;\n\nThis could be made more efficient by avoiding the lookups.\n\n\tconst auto [iter, success] = callData_.insert({ cookie, { response, false }});\n\n(you may need a constructor for the CallData structure)\n\n> +\n> +\tret = socket_->send(message);\n> +\tif (ret) {\n> +\t\tcallData_.erase(seq_);\n\ns/seq_/cookie/ ?\n\nBut in any case,\n\n\t\tcallData_.erase(iter);\n\n> +\t\treturn ret;\n> +\t}\n> +\n\n\t/* \\todo Make this less dangerous, see IPCPipe::sendSync() */\n\n> +\ttimeout.start(2000);\n> +\twhile (!callData_[cookie].done) {\n\n\twhile (!iter->second.done) {\n\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(cookie);\n\n\tcallData_.erase(iter);\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index b7cfe0c2..cd3cf9e3 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -31,6 +31,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 1553FBD808\n\tfor ;\n\tTue, 2 Feb 2021 02:30:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 943CF6841C;\n\tTue, 2 Feb 2021 03:30:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDBC160106\n\tfor ;\n\tTue, 2 Feb 2021 03:30:00 +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 4BC91556;\n\tTue, 2 Feb 2021 03:30:00 +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=\"SQBS3kp2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612233000;\n\tbh=adYMr4tA0qasnLT44VrBZD0qibHf1dUzcZeyKtXzGU0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SQBS3kp2sIJzmi06ahc4oe/Ws9E2pslrVMcVkUIiRS8kXG2p6KxrLOc3sxzp5QPuq\n\tLrFr7tHvtOt5e1V7H+IBcNLF38foZlyiy232dN+eRPwBTUw7c0/qNQqJZiX/nginfK\n\tmy10jdpxiIF255I/DJnPeJYp7fGQmKFJLFuO43XQ=","Date":"Tue, 2 Feb 2021 04:29:38 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20201224081534.41601-1-paul.elder@ideasonboard.com>\n\t<20201224081534.41601-9-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201224081534.41601-9-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 8/9] 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]