[{"id":12722,"web_url":"https://patchwork.libcamera.org/comment/12722/","msgid":"<20200924105505.jvxhf6rp3xnui3tb@uno.localdomain>","date":"2020-09-24T10:55:05","subject":"Re: [libcamera-devel] [PATCH 15/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":"On Tue, Sep 22, 2020 at 10:35:14PM +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 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 | 115 ++++++++++++\n> src/libcamera/ipa_ipc_unixsocket.cpp | 175 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 293 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> \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..de8af35b\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> @@ -0,0 +1,115 @@\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> +inline void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq)\n> +{\n> +\tuint8_t cmd_arr[] = {static_cast(cmd & 0xff),\n> +\t\t\t static_cast((cmd >> 8 ) & 0xff),\n> +\t\t\t static_cast((cmd >> 16) & 0xff),\n> +\t\t\t static_cast((cmd >> 24) & 0xff)};\n> +\tuint8_t seq_arr[] = {static_cast(seq & 0xff),\n> +\t\t\t static_cast((seq >> 8 ) & 0xff),\n> +\t\t\t static_cast((seq >> 16) & 0xff),\n> +\t\t\t static_cast((seq >> 24) & 0xff)};\n\nI think you can omit the 0xff as the cast to uint8_t implies it\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> +inline std::tuple readHeader(IPCUnixSocket::Payload &payload)\n> +{\n> +\tuint32_t cmd = (payload.data[0] & 0xff) |\n> +\t\t ((payload.data[1] & 0xff) << 8) |\n> +\t\t ((payload.data[2] & 0xff) << 16) |\n> +\t\t ((payload.data[3] & 0xff) << 24);\n> +\tuint32_t seq = (payload.data[4] & 0xff) |\n> +\t\t ((payload.data[5] & 0xff) << 8) |\n> +\t\t ((payload.data[6] & 0xff) << 16) |\n> +\t\t ((payload.data[7] & 0xff) << 24);\n> +\n> +\treturn {cmd, seq};\n> +}\n> +\n> +inline void eraseHeader(IPCUnixSocket::Payload &payload)\n> +{\n> +\tpayload.data.erase(payload.data.begin(), payload.data.begin() + 8);\n> +}\n\nI would not place these in the header file but rather as static\nfunctions in an anonymous namespace in the cpp file. Or are they used\noutside from this module ? It seems so:\n\nsrc/libcamera/ipa_ipc_unixsocket.cpp: std::tie(cmd, seq) = readHeader(message);\ntest/ipc/unixsocket_ipc.cpp: std::tie(cmd, seq) = readHeader(message);\nutils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl: std::tie(_cmd, _seq) = readHeader(_message);\n\nSo why are they defined in this header which implements the IPAIPC\ninterface ? What if another IPC mechanism is used ?\n\n(I -think- also this causes the function to be defined in every module\nthat includes this header, wasting a bit of space, but that's a\nsecondary issue).\n\n> +\n> +inline void writeUInt32(IPCUnixSocket::Payload &payload, uint32_t val, uint32_t pos)\n> +{\n> +\tif (pos + 4 > payload.data.size())\n> +\t\treturn;\n> +\n> +\tuint8_t arr[] = {static_cast(val & 0xff),\n> +\t\t\t static_cast(val & (0xff << 8)),\n> +\t\t\t static_cast(val & (0xff << 16)),\n> +\t\t\t static_cast(val & (0xff << 24))};\n> +\tstd::copy(arr, arr + 4, payload.data.begin() + pos);\n> +}\n> +\n> +inline uint32_t readUInt32(IPCUnixSocket::Payload &payload, uint32_t pos)\n> +{\n> +\tif (pos + 4 > payload.data.size())\n> +\t\treturn 0;\n> +\n> +\treturn payload.data[pos] & (payload.data[pos + 1] << 8) &\n> +\t (payload.data[pos + 2] << 16) & (payload.data[pos + 3] << 24);\n> +}\n\nI don't see these used.\n\nAlso, double empty line\n\n> +\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> +\tbool isValid() const { return valid_; }\n\nthat's defined in the base class, right ?\n\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> +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\nI'll for now skip the review of the implementation.\n\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..c1997e96\n> --- /dev/null\n> +++ b/src/libcamera/ipa_ipc_unixsocket.cpp\n> @@ -0,0 +1,175 @@\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> +#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> +\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> +\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> +\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> +}\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> +\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> +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 e3eade57..24f32815 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -25,6 +25,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 B6795C3B5C\n\tfor ;\n\tThu, 24 Sep 2020 10:51:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19B2562FF8;\n\tThu, 24 Sep 2020 12:51:15 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6978460363\n\tfor ;\n\tThu, 24 Sep 2020 12:51:13 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id CF445100013;\n\tThu, 24 Sep 2020 10:51:12 +0000 (UTC)"],"Date":"Thu, 24 Sep 2020 12:55:05 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20200924105505.jvxhf6rp3xnui3tb@uno.localdomain>","References":"<20200922133537.258098-1-paul.elder@ideasonboard.com>\n\t<20200922133537.258098-16-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200922133537.258098-16-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 15/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\" "}},{"id":12757,"web_url":"https://patchwork.libcamera.org/comment/12757/","msgid":"<20200925025727.GP45948@pyrite.rasen.tech>","date":"2020-09-25T02:57:27","subject":"Re: [libcamera-devel] [PATCH 15/38] 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":"On Thu, Sep 24, 2020 at 12:55:05PM +0200, Jacopo Mondi wrote:\n> On Tue, Sep 22, 2020 at 10:35:14PM +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 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 | 115 ++++++++++++\n> > src/libcamera/ipa_ipc_unixsocket.cpp | 175 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 293 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> > \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..de8af35b\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> > @@ -0,0 +1,115 @@\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> > +inline void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq)\n> > +{\n> > +\tuint8_t cmd_arr[] = {static_cast(cmd & 0xff),\n> > +\t\t\t static_cast((cmd >> 8 ) & 0xff),\n> > +\t\t\t static_cast((cmd >> 16) & 0xff),\n> > +\t\t\t static_cast((cmd >> 24) & 0xff)};\n> > +\tuint8_t seq_arr[] = {static_cast(seq & 0xff),\n> > +\t\t\t static_cast((seq >> 8 ) & 0xff),\n> > +\t\t\t static_cast((seq >> 16) & 0xff),\n> > +\t\t\t static_cast((seq >> 24) & 0xff)};\n> \n> I think you can omit the 0xff as the cast to uint8_t implies it\n\nack\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> > +inline std::tuple readHeader(IPCUnixSocket::Payload &payload)\n> > +{\n> > +\tuint32_t cmd = (payload.data[0] & 0xff) |\n> > +\t\t ((payload.data[1] & 0xff) << 8) |\n> > +\t\t ((payload.data[2] & 0xff) << 16) |\n> > +\t\t ((payload.data[3] & 0xff) << 24);\n> > +\tuint32_t seq = (payload.data[4] & 0xff) |\n> > +\t\t ((payload.data[5] & 0xff) << 8) |\n> > +\t\t ((payload.data[6] & 0xff) << 16) |\n> > +\t\t ((payload.data[7] & 0xff) << 24);\n> > +\n> > +\treturn {cmd, seq};\n> > +}\n> > +\n> > +inline void eraseHeader(IPCUnixSocket::Payload &payload)\n> > +{\n> > +\tpayload.data.erase(payload.data.begin(), payload.data.begin() + 8);\n> > +}\n> \n> I would not place these in the header file but rather as static\n> functions in an anonymous namespace in the cpp file. Or are they used\n> outside from this module ? It seems so:\n> \n> src/libcamera/ipa_ipc_unixsocket.cpp: std::tie(cmd, seq) = readHeader(message);\n> test/ipc/unixsocket_ipc.cpp: std::tie(cmd, seq) = readHeader(message);\n> utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl: std::tie(_cmd, _seq) = readHeader(_message);\n> \n> So why are they defined in this header which implements the IPAIPC\n> interface ? What if another IPC mechanism is used ?\n\nThis header is only for IPAIPCUnixSocket, not the base IPAIPC, so it\nshould be fine to keep them here. test/ipc/unixsocket_ipc.cpp tests\nIPAIPCUnixSocket.\n\nAs for the proxy worker, as I mentioned before, technically it's a\nlayering violation and we should really have an IPAIPCWorker\n(IPAIPCUnixSocketWorker) in between IPAIPC (IPAIPCUnixSocket)\nand IPAProxy{pipeline_name}Worker. But we don't have any other IPC\nmechanisms yet, so it should be fine for now. When we do, then the proxy\nworker will no longer use these IPC-specific header manipulation\nfunctions, and they will be used in the IPAIPCUnixSocketWorker.\n\n> (I -think- also this causes the function to be defined in every module\n> that includes this header, wasting a bit of space, but that's a\n> secondary issue).\n\nThese are the only files that include this header:\n\nsrc/libcamera/ipa_ipc_unixsocket.cpp\ntest/ipc/unixsocket_ipc.cpp\nutils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\nutils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\nutils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n\nThe first two are fine. The last one is fine for now. The other two\nreally only include this header for choosing the IPAIPC that they want\nto use, which for now should be fine. When we add other IPC mechanisms\nwe can rename or namespace these.\n\n> > +\n> > +inline void writeUInt32(IPCUnixSocket::Payload &payload, uint32_t val, uint32_t pos)\n> > +{\n> > +\tif (pos + 4 > payload.data.size())\n> > +\t\treturn;\n> > +\n> > +\tuint8_t arr[] = {static_cast(val & 0xff),\n> > +\t\t\t static_cast(val & (0xff << 8)),\n> > +\t\t\t static_cast(val & (0xff << 16)),\n> > +\t\t\t static_cast(val & (0xff << 24))};\n> > +\tstd::copy(arr, arr + 4, payload.data.begin() + pos);\n> > +}\n> > +\n> > +inline uint32_t readUInt32(IPCUnixSocket::Payload &payload, uint32_t pos)\n> > +{\n> > +\tif (pos + 4 > payload.data.size())\n> > +\t\treturn 0;\n> > +\n> > +\treturn payload.data[pos] & (payload.data[pos + 1] << 8) &\n> > +\t (payload.data[pos + 2] << 16) & (payload.data[pos + 3] << 24);\n> > +}\n> \n> I don't see these used.\n\nAh yes, these are leftovers from the de/serialization work...\n\n> Also, double empty line\n> \n> > +\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> > +\tbool isValid() const { return valid_; }\n> \n> that's defined in the base class, right ?\n\nAh yeah it is.\n\n\nThanks,\n\nPaul\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> > +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> \n> I'll for now skip the review of the implementation.\n> \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..c1997e96\n> > --- /dev/null\n> > +++ b/src/libcamera/ipa_ipc_unixsocket.cpp\n> > @@ -0,0 +1,175 @@\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> > +#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> > +\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> > +\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> > +\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> > +}\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> > +\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> > +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 e3eade57..24f32815 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -25,6 +25,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 E4A25C3B5B\n\tfor ;\n\tFri, 25 Sep 2020 02:57:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DF5A63005;\n\tFri, 25 Sep 2020 04:57:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0067660576\n\tfor ;\n\tFri, 25 Sep 2020 04:57:36 +0200 (CEST)","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 0B9D52D7;\n\tFri, 25 Sep 2020 04:57:34 +0200 (CEST)"],"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=\"E58wezyH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601002656;\n\tbh=D2O1/jXWmW7ilVs9yntcl73KRGL3s6jLpw8lL8YoGXU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E58wezyHJerOfBqA0ASDWKjqzU7kexeXBFv96m+X6fi3VA6m8rRv9TIwcRZy11K8q\n\t56+LzYPBlCshdUvlX5WePPcAGCkpf6ygm1uukPGLBiE7lZX5icVhRdvduaLcrvaLTN\n\tO0f1YYZOhM4rl7goq0LWbjHHkhwoDIt26cWyNT8M=","Date":"Fri, 25 Sep 2020 11:57:27 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20200925025727.GP45948@pyrite.rasen.tech>","References":"<20200922133537.258098-1-paul.elder@ideasonboard.com>\n\t<20200922133537.258098-16-paul.elder@ideasonboard.com>\n\t<20200924105505.jvxhf6rp3xnui3tb@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200924105505.jvxhf6rp3xnui3tb@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 15/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\" "}},{"id":12760,"web_url":"https://patchwork.libcamera.org/comment/12760/","msgid":"<20200925080611.4uf6nawovmria5rq@uno.localdomain>","date":"2020-09-25T08:06:11","subject":"Re: [libcamera-devel] [PATCH 15/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":"Paul,\n\nOn Fri, Sep 25, 2020 at 11:57:27AM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, Sep 24, 2020 at 12:55:05PM +0200, Jacopo Mondi wrote:\n> > On Tue, Sep 22, 2020 at 10:35:14PM +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 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 | 115 ++++++++++++\n> > > src/libcamera/ipa_ipc_unixsocket.cpp | 175 ++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 293 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> > > \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..de8af35b\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h\n> > > @@ -0,0 +1,115 @@\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> > > +inline void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq)\n> > > +{\n> > > +\tuint8_t cmd_arr[] = {static_cast(cmd & 0xff),\n> > > +\t\t\t static_cast((cmd >> 8 ) & 0xff),\n> > > +\t\t\t static_cast((cmd >> 16) & 0xff),\n> > > +\t\t\t static_cast((cmd >> 24) & 0xff)};\n> > > +\tuint8_t seq_arr[] = {static_cast(seq & 0xff),\n> > > +\t\t\t static_cast((seq >> 8 ) & 0xff),\n> > > +\t\t\t static_cast((seq >> 16) & 0xff),\n> > > +\t\t\t static_cast((seq >> 24) & 0xff)};\n> >\n> > I think you can omit the 0xff as the cast to uint8_t implies it\n>\n> ack\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> > > +inline std::tuple readHeader(IPCUnixSocket::Payload &payload)\n> > > +{\n> > > +\tuint32_t cmd = (payload.data[0] & 0xff) |\n> > > +\t\t ((payload.data[1] & 0xff) << 8) |\n> > > +\t\t ((payload.data[2] & 0xff) << 16) |\n> > > +\t\t ((payload.data[3] & 0xff) << 24);\n> > > +\tuint32_t seq = (payload.data[4] & 0xff) |\n> > > +\t\t ((payload.data[5] & 0xff) << 8) |\n> > > +\t\t ((payload.data[6] & 0xff) << 16) |\n> > > +\t\t ((payload.data[7] & 0xff) << 24);\n> > > +\n\nAlso\n\npayload.data is a vector of uint8_t, no need for the 0xff\nYou can save all ()\n\n\n> > > +\treturn {cmd, seq};\n> > > +}\n> > > +\n> > > +inline void eraseHeader(IPCUnixSocket::Payload &payload)\n> > > +{\n> > > +\tpayload.data.erase(payload.data.begin(), payload.data.begin() + 8);\n> > > +}\n> >\n> > I would not place these in the header file but rather as static\n> > functions in an anonymous namespace in the cpp file. Or are they used\n> > outside from this module ? It seems so:\n> >\n> > src/libcamera/ipa_ipc_unixsocket.cpp: std::tie(cmd, seq) = readHeader(message);\n> > test/ipc/unixsocket_ipc.cpp: std::tie(cmd, seq) = readHeader(message);\n> > utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl: std::tie(_cmd, _seq) = readHeader(_message);\n> >\n> > So why are they defined in this header which implements the IPAIPC\n> > interface ? What if another IPC mechanism is used ?\n>\n> This header is only for IPAIPCUnixSocket, not the base IPAIPC, so it\n> should be fine to keep them here. test/ipc/unixsocket_ipc.cpp tests\n> IPAIPCUnixSocket.\n>\n> As for the proxy worker, as I mentioned before, technically it's a\n> layering violation and we should really have an IPAIPCWorker\n> (IPAIPCUnixSocketWorker) in between IPAIPC (IPAIPCUnixSocket)\n> and IPAProxy{pipeline_name}Worker. But we don't have any other IPC\n> mechanisms yet, so it should be fine for now. When we do, then the proxy\n> worker will no longer use these IPC-specific header manipulation\n> functions, and they will be used in the IPAIPCUnixSocketWorker.\n>\n\nSo all the generated workers uses the UnixIPC mechanism\nunconditionally, right ?\n\nAnyway it feels really sloppy to pollute the global namespace with two\nfunctions defined in the unix socket IPC. Do you expect other IPC\nmechanism to have different read/writeHeader ? In this case you should\nmake these static class members (you can use inline in the function\ndefinition in the .cpp file if you want to keep them inlined).\nWill these methods be generic to all the IPC mechanisms instead? Move\nthem to the base virtual class. I don't see a real reason to keep them\nglobally visible.\n\nI'm still not familiar on the Proxy/ProxyWorker architecture, so I\nmight be missing some parts.\n\n> > (I -think- also this causes the function to be defined in every module\n> > that includes this header, wasting a bit of space, but that's a\n> > secondary issue).\n>\n> These are the only files that include this header:\n>\n> src/libcamera/ipa_ipc_unixsocket.cpp\n> test/ipc/unixsocket_ipc.cpp\n> utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n>\n> The first two are fine. The last one is fine for now. The other two\n> really only include this header for choosing the IPAIPC that they want\n> to use, which for now should be fine. When we add other IPC mechanisms\n> we can rename or namespace these.\n>\n> > > +\n> > > +inline void writeUInt32(IPCUnixSocket::Payload &payload, uint32_t val, uint32_t pos)\n> > > +{\n> > > +\tif (pos + 4 > payload.data.size())\n> > > +\t\treturn;\n> > > +\n> > > +\tuint8_t arr[] = {static_cast(val & 0xff),\n> > > +\t\t\t static_cast(val & (0xff << 8)),\n> > > +\t\t\t static_cast(val & (0xff << 16)),\n> > > +\t\t\t static_cast(val & (0xff << 24))};\n> > > +\tstd::copy(arr, arr + 4, payload.data.begin() + pos);\n> > > +}\n> > > +\n> > > +inline uint32_t readUInt32(IPCUnixSocket::Payload &payload, uint32_t pos)\n> > > +{\n> > > +\tif (pos + 4 > payload.data.size())\n> > > +\t\treturn 0;\n> > > +\n> > > +\treturn payload.data[pos] & (payload.data[pos + 1] << 8) &\n> > > +\t (payload.data[pos + 2] << 16) & (payload.data[pos + 3] << 24);\n> > > +}\n> >\n> > I don't see these used.\n>\n> Ah yes, these are leftovers from the de/serialization work...\n>\n> > Also, double empty line\n> >\n> > > +\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> > > +\tbool isValid() const { return valid_; }\n> >\n> > that's defined in the base class, right ?\n>\n> Ah yeah it is.\n>\n>\n> Thanks,\n>\n> Paul\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> > > +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> >\n> > I'll for now skip the review of the implementation.\n> >\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..c1997e96\n> > > --- /dev/null\n> > > +++ b/src/libcamera/ipa_ipc_unixsocket.cpp\n> > > @@ -0,0 +1,175 @@\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> > > +#include \"libcamera/internal/log.h\"\nAnyway I see all the generated workers calling readHeader() and writeHeader(),\nas a result of the ipa_proxy_worker template using them.\n\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> > > +\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> > > +\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> > > +\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> > > +}\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> > > +\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> > > +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 e3eade57..24f32815 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -25,6 +25,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 11931C3B5B\n\tfor ;\n\tFri, 25 Sep 2020 08:02:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72F7963019;\n\tFri, 25 Sep 2020 10:02:19 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BA5360576\n\tfor ;\n\tFri, 25 Sep 2020 10:02:18 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id BD1E824000B;\n\tFri, 25 Sep 2020 08:02:17 +0000 (UTC)"],"Date":"Fri, 25 Sep 2020 10:06:11 +0200","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20200925080611.4uf6nawovmria5rq@uno.localdomain>","References":"<20200922133537.258098-1-paul.elder@ideasonboard.com>\n\t<20200922133537.258098-16-paul.elder@ideasonboard.com>\n\t<20200924105505.jvxhf6rp3xnui3tb@uno.localdomain>\n\t<20200925025727.GP45948@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925025727.GP45948@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 15/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\" "}}]