[{"id":13547,"web_url":"https://patchwork.libcamera.org/comment/13547/","msgid":"<20201029085352.wtxlfqppjfaqvqnr@uno.localdomain>","date":"2020-10-29T08:53:52","subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Oct 02, 2020 at 11:31:27PM +0900, Paul Elder wrote:\n> Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies\n> and proxy workers will call into the IPAIPC, rather than implementing\n> the IPC themselves.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v3:\n> - expand documentation a bit\n> - move [[maybe_unused]] to after the parameters in the IPAIPC\n> constructor, to appease gcc <= 9.1\n>\n> Changes in v2:\n> - add documentation\n> ---\n> include/libcamera/internal/ipa_ipc.h | 41 ++++++++++\n> src/libcamera/ipa_ipc.cpp | 114 +++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 156 insertions(+)\n> create mode 100644 include/libcamera/internal/ipa_ipc.h\n> create mode 100644 src/libcamera/ipa_ipc.cpp\n>\n> diff --git a/include/libcamera/internal/ipa_ipc.h b/include/libcamera/internal/ipa_ipc.h\n> new file mode 100644\n> index 00000000..09de36a5\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipa_ipc.h\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipa_ipc.h - Image Processing Algorithm IPC module for IPA proxies\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class IPAIPC\n> +{\n> +public:\n> +\tIPAIPC(const char *ipa_module_path [[maybe_unused]],\n> +\t const char *ipa_proxy_worker_path [[maybe_unused]]);\n> +\tvirtual ~IPAIPC();\n> +\n> +\tbool isValid() const { return valid_; }\n\nWe usually have getters with the same name as the variable they \"get\".\nI'm fine with 'isValid()' but I felt like pointing this out.\n\n> +\n> +\tvirtual int 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 = nullptr,\n> +\t\t\t std::vector *fds_out = nullptr) = 0;\n> +\n> +\tvirtual int sendAsync(uint32_t cmd,\n> +\t\t\t const std::vector &data_in,\n> +\t\t\t const std::vector &fds_in) = 0;\n> +\n> +\tSignal &, std::vector &> recvIPC;\n> +\n> +protected:\n> +\tbool valid_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */\n> diff --git a/src/libcamera/ipa_ipc.cpp b/src/libcamera/ipa_ipc.cpp\n> new file mode 100644\n> index 00000000..b963351f\n> --- /dev/null\n> +++ b/src/libcamera/ipa_ipc.cpp\n> @@ -0,0 +1,114 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipa_ipc.cpp - Image Processing Algorithm IPC module for IPA proxies\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\nThe inclusion of the header corresponding to the cpp file is usually\nplaced first, to ensure it is self-contained and does not depend on\nother include directives that might be placed before it.\n\n> +\n> +/**\n> + * \\file ipa_ipc.h\n> + * \\brief IPC mechanism for IPA isolation\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPAIPC)\n> +\n> +/**\n> + * \\class IPAIPC\n> + * \\brief IPC mechanism for IPA isolation\n> + *\n> + * Virtual class to model an IPC mechanism for use by IPA proxies for IPA\n> + * isolation. sendSync() and sendAsync() must be implemented, and the recvIPC\n> + * signal must be emitted whenever new data is available.\n> + */\n> +\n> +/**\n> + * \\brief Construct an IPAIPC instance\n> + * \\param[in] ipa_module_path Path to the IPA module shared object\n> + * \\param[in] ipa_proxy_worker_path Path to the IPA proxy worker shared object\n> + */\n> +IPAIPC::IPAIPC([[maybe_unused]] const char *ipa_module_path,\n> +\t [[maybe_unused]] const char *ipa_proxy_worker_path)\n\nnit: parameters alignment\n\nIf these fields are not used by the base virtual class, do we need\nthem in the constructor ?\n\n> +\t: valid_(false)\n> +{\n> +}\n> +\n> +IPAIPC::~IPAIPC()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn IPAIPC::isValid()\n> + * \\brief Check if the IPAIPC instance is valid\n> + *\n> + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> + * isolation, and IPC is successfully set up.\n> + *\n> + * \\return True if the IPAIPC is valid, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn IPAIPC::sendSync()\n> + * \\brief Send a message over IPC synchronously\n> + * \\param[in] cmd Command ID\n> + * \\param[in] data_in Data to send, as a byte vector\n> + * \\param[in] fds_in Fds to send, in a vector\n> + * \\param[in] data_out Byte vector in which to receive data, if applicable\n> + * \\param[in] fds_out Fds vector in which to receive fds, if applicable\n\nCould you spell 'file descriptors' entirely ?\n * \\param[in] fds_out Vector in which to receive file descriptors, if applicable\n\nSame for other locations.\n\nI a bit wonder why fds should be kept separate and not serialized\nin the data_in vector. Does it depend on the fact that fds as\nserialized separately by the IPADataSerializer module ?\n\nA final minor note on this: are _in and _out the best names we can have ?\nShould this be expressed as '_msg' and '_reply' ?\n\n> + *\n> + * This function will not return until a response is received. The event loop\n> + * will still continue to execute, however.\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn IPAIPC::sendAsync()\n> + * \\brief Send a message over IPC asynchronously\n> + * \\param[in] cmd Command ID\n> + * \\param[in] data_in Data to send, as a byte vector\n> + * \\param[in] fds_in Fds to send, in a vector\n> + *\n> + * This function will return immediately after sending the message.\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\var IPAIPC::recvIPC\n> + * \\brief Signal to be emitted when data is received over IPC\n> + *\n> + * When data is received over IPC, this signal shall be emitted. Users must\n> + * connect to this to receive new data.\n> + *\n> + * The parameters of the signal are a vector of bytes and a vector of file\n> + * descriptors.\n\nI assume this completes the sendAsync() method, right ? Can we state\nthat explicitely, if that's the case ?\n\nMostly minor comments, please add\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> + */\n> +\n> +/**\n> + * \\var IPAIPC::valid_\n> + * \\brief Flag to indicate if the IPAIPC instance is valid\n> + *\n> + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> + * isolation, and IPC is successfully set up.\n> + *\n> + * This flag can be read via IPAIPC::isValid().\n> + *\n> + * Implementations of the IPAIPC class should set this flag upon successful\n> + * construction.\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 190d7490..e0a2ab47 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -25,6 +25,7 @@ libcamera_sources = files([\n> 'ipa_context_wrapper.cpp',\n> 'ipa_controls.cpp',\n> 'ipa_data_serializer.cpp',\n> + 'ipa_ipc.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 621E3BDB1E\n\tfor ;\n\tThu, 29 Oct 2020 08:53:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D414162825;\n\tThu, 29 Oct 2020 09:53:54 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A39C162053\n\tfor ;\n\tThu, 29 Oct 2020 09:53:53 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 165651BF216;\n\tThu, 29 Oct 2020 08:53:52 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 29 Oct 2020 09:53:52 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20201029085352.wtxlfqppjfaqvqnr@uno.localdomain>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-12-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002143154.468162-12-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","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":13548,"web_url":"https://patchwork.libcamera.org/comment/13548/","msgid":"<20201029102844.GA4944@pyrite.rasen.tech>","date":"2020-10-29T10:28:44","subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the review.\n\nOn Thu, Oct 29, 2020 at 09:53:52AM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Oct 02, 2020 at 11:31:27PM +0900, Paul Elder wrote:\n> > Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies\n> > and proxy workers will call into the IPAIPC, rather than implementing\n> > the IPC themselves.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v3:\n> > - expand documentation a bit\n> > - move [[maybe_unused]] to after the parameters in the IPAIPC\n> > constructor, to appease gcc <= 9.1\n> >\n> > Changes in v2:\n> > - add documentation\n> > ---\n> > include/libcamera/internal/ipa_ipc.h | 41 ++++++++++\n> > src/libcamera/ipa_ipc.cpp | 114 +++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 3 files changed, 156 insertions(+)\n> > create mode 100644 include/libcamera/internal/ipa_ipc.h\n> > create mode 100644 src/libcamera/ipa_ipc.cpp\n> >\n> > diff --git a/include/libcamera/internal/ipa_ipc.h b/include/libcamera/internal/ipa_ipc.h\n> > new file mode 100644\n> > index 00000000..09de36a5\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/ipa_ipc.h\n> > @@ -0,0 +1,41 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * ipa_ipc.h - Image Processing Algorithm IPC module for IPA proxies\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> > +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class IPAIPC\n> > +{\n> > +public:\n> > +\tIPAIPC(const char *ipa_module_path [[maybe_unused]],\n> > +\t const char *ipa_proxy_worker_path [[maybe_unused]]);\n> > +\tvirtual ~IPAIPC();\n> > +\n> > +\tbool isValid() const { return valid_; }\n> \n> We usually have getters with the same name as the variable they \"get\".\n> I'm fine with 'isValid()' but I felt like pointing this out.\n\nOh, okay. IPAModule has this too though.\n\n> > +\n> > +\tvirtual int 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 = nullptr,\n> > +\t\t\t std::vector *fds_out = nullptr) = 0;\n> > +\n> > +\tvirtual int sendAsync(uint32_t cmd,\n> > +\t\t\t const std::vector &data_in,\n> > +\t\t\t const std::vector &fds_in) = 0;\n> > +\n> > +\tSignal &, std::vector &> recvIPC;\n> > +\n> > +protected:\n> > +\tbool valid_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */\n> > diff --git a/src/libcamera/ipa_ipc.cpp b/src/libcamera/ipa_ipc.cpp\n> > new file mode 100644\n> > index 00000000..b963351f\n> > --- /dev/null\n> > +++ b/src/libcamera/ipa_ipc.cpp\n> > @@ -0,0 +1,114 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * ipa_ipc.cpp - Image Processing Algorithm IPC module for IPA proxies\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> \n> The inclusion of the header corresponding to the cpp file is usually\n> placed first, to ensure it is self-contained and does not depend on\n> other include directives that might be placed before it.\n\nAh, okay.\n\n> > +\n> > +/**\n> > + * \\file ipa_ipc.h\n> > + * \\brief IPC mechanism for IPA isolation\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(IPAIPC)\n> > +\n> > +/**\n> > + * \\class IPAIPC\n> > + * \\brief IPC mechanism for IPA isolation\n> > + *\n> > + * Virtual class to model an IPC mechanism for use by IPA proxies for IPA\n> > + * isolation. sendSync() and sendAsync() must be implemented, and the recvIPC\n> > + * signal must be emitted whenever new data is available.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct an IPAIPC instance\n> > + * \\param[in] ipa_module_path Path to the IPA module shared object\n> > + * \\param[in] ipa_proxy_worker_path Path to the IPA proxy worker shared object\n> > + */\n> > +IPAIPC::IPAIPC([[maybe_unused]] const char *ipa_module_path,\n> > +\t [[maybe_unused]] const char *ipa_proxy_worker_path)\n> \n> nit: parameters alignment\n\nThis is aligned correctly. I guess patches mess up tabs?\n\n> If these fields are not used by the base virtual class, do we need\n> them in the constructor ?\n\nI imagined that they would be required by all subclasses of IPAIPC, so I\nwanted to put them here.\n\n> > +\t: valid_(false)\n> > +{\n> > +}\n> > +\n> > +IPAIPC::~IPAIPC()\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn IPAIPC::isValid()\n> > + * \\brief Check if the IPAIPC instance is valid\n> > + *\n> > + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> > + * isolation, and IPC is successfully set up.\n> > + *\n> > + * \\return True if the IPAIPC is valid, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAIPC::sendSync()\n> > + * \\brief Send a message over IPC synchronously\n> > + * \\param[in] cmd Command ID\n> > + * \\param[in] data_in Data to send, as a byte vector\n> > + * \\param[in] fds_in Fds to send, in a vector\n> > + * \\param[in] data_out Byte vector in which to receive data, if applicable\n> > + * \\param[in] fds_out Fds vector in which to receive fds, if applicable\n> \n> Could you spell 'file descriptors' entirely ?\n> * \\param[in] fds_out Vector in which to receive file descriptors, if applicable\n> \n> Same for other locations.\n\nOkay.\n\n> I a bit wonder why fds should be kept separate and not serialized\n> in the data_in vector. Does it depend on the fact that fds as\n> serialized separately by the IPADataSerializer module ?\n\nBecause if you send the fds as data they won't get translated :)\n\n> A final minor note on this: are _in and _out the best names we can have ?\n> Should this be expressed as '_msg' and '_reply' ?\n\nThat'll work too. I just thought of it as input and output data.\n\n> > + *\n> > + * This function will not return until a response is received. The event loop\n> > + * will still continue to execute, however.\n> > + *\n> > + * \\return Zero on success, negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn IPAIPC::sendAsync()\n> > + * \\brief Send a message over IPC asynchronously\n> > + * \\param[in] cmd Command ID\n> > + * \\param[in] data_in Data to send, as a byte vector\n> > + * \\param[in] fds_in Fds to send, in a vector\n> > + *\n> > + * This function will return immediately after sending the message.\n> > + *\n> > + * \\return Zero on success, negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAIPC::recvIPC\n> > + * \\brief Signal to be emitted when data is received over IPC\n> > + *\n> > + * When data is received over IPC, this signal shall be emitted. Users must\n> > + * connect to this to receive new data.\n> > + *\n> > + * The parameters of the signal are a vector of bytes and a vector of file\n> > + * descriptors.\n> \n> I assume this completes the sendAsync() method, right ? Can we state\n> that explicitely, if that's the case ?\n\nNot necessarily. One sendAsync() could have multiple responses.\n\n> Mostly minor comments, please add\n> Reviewed-by: Jacopo Mondi \n\n\nThank you,\n\nPaul\n\n> > + */\n> > +\n> > +/**\n> > + * \\var IPAIPC::valid_\n> > + * \\brief Flag to indicate if the IPAIPC instance is valid\n> > + *\n> > + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> > + * isolation, and IPC is successfully set up.\n> > + *\n> > + * This flag can be read via IPAIPC::isValid().\n> > + *\n> > + * Implementations of the IPAIPC class should set this flag upon successful\n> > + * construction.\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 190d7490..e0a2ab47 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -25,6 +25,7 @@ libcamera_sources = files([\n> > 'ipa_context_wrapper.cpp',\n> > 'ipa_controls.cpp',\n> > 'ipa_data_serializer.cpp',\n> > + 'ipa_ipc.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 B9FD8BDB1E\n\tfor ;\n\tThu, 29 Oct 2020 10:28:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2431462895;\n\tThu, 29 Oct 2020 11:28:54 +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 07CCB627D7\n\tfor ;\n\tThu, 29 Oct 2020 11:28:52 +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 6F3A750E;\n\tThu, 29 Oct 2020 11:28:50 +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=\"O5HHqyKf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603967331;\n\tbh=BWjKK6Q5vSBb7UdGjVZ17+UbxMv/U0rFHib7RoUiIm0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O5HHqyKf2zJAXM9nfcuQ4YBGinWptmbqQowq7WtUBZERRB6mufI3/aiWA9FhJNIW9\n\tTHQXrKeAl+it2Ct5gCeqPu5hKlXAxPUbm4YGc637nRzdOcEwbCe3siRfQQa8Xc72yE\n\tS3OvKfinpm19P98F4uHEU9TpPG1SpGRE8sZOToxI=","Date":"Thu, 29 Oct 2020 19:28:44 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20201029102844.GA4944@pyrite.rasen.tech>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-12-paul.elder@ideasonboard.com>\n\t<20201029085352.wtxlfqppjfaqvqnr@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201029085352.wtxlfqppjfaqvqnr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","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":13553,"web_url":"https://patchwork.libcamera.org/comment/13553/","msgid":"<20201029122345.xhknnri3wb6pursd@uno.localdomain>","date":"2020-10-29T12:23:45","subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Thu, Oct 29, 2020 at 07:28:44PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> Thank you for the review.\n>\n> On Thu, Oct 29, 2020 at 09:53:52AM +0100, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Fri, Oct 02, 2020 at 11:31:27PM +0900, Paul Elder wrote:\n> > > Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies\n> > > and proxy workers will call into the IPAIPC, rather than implementing\n> > > the IPC themselves.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v3:\n> > > - expand documentation a bit\n> > > - move [[maybe_unused]] to after the parameters in the IPAIPC\n> > > constructor, to appease gcc <= 9.1\n> > >\n> > > Changes in v2:\n> > > - add documentation\n> > > ---\n> > > include/libcamera/internal/ipa_ipc.h | 41 ++++++++++\n> > > src/libcamera/ipa_ipc.cpp | 114 +++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 3 files changed, 156 insertions(+)\n> > > create mode 100644 include/libcamera/internal/ipa_ipc.h\n> > > create mode 100644 src/libcamera/ipa_ipc.cpp\n> > >\n> > > diff --git a/include/libcamera/internal/ipa_ipc.h b/include/libcamera/internal/ipa_ipc.h\n> > > new file mode 100644\n> > > index 00000000..09de36a5\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/ipa_ipc.h\n> > > @@ -0,0 +1,41 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * ipa_ipc.h - Image Processing Algorithm IPC module for IPA proxies\n> > > + */\n> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> > > +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class IPAIPC\n> > > +{\n> > > +public:\n> > > +\tIPAIPC(const char *ipa_module_path [[maybe_unused]],\n> > > +\t const char *ipa_proxy_worker_path [[maybe_unused]]);\n> > > +\tvirtual ~IPAIPC();\n> > > +\n> > > +\tbool isValid() const { return valid_; }\n> >\n> > We usually have getters with the same name as the variable they \"get\".\n> > I'm fine with 'isValid()' but I felt like pointing this out.\n>\n> Oh, okay. IPAModule has this too though.\n>\n> > > +\n> > > +\tvirtual int 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 = nullptr,\n> > > +\t\t\t std::vector *fds_out = nullptr) = 0;\n> > > +\n> > > +\tvirtual int sendAsync(uint32_t cmd,\n> > > +\t\t\t const std::vector &data_in,\n> > > +\t\t\t const std::vector &fds_in) = 0;\n> > > +\n> > > +\tSignal &, std::vector &> recvIPC;\n> > > +\n> > > +protected:\n> > > +\tbool valid_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */\n> > > diff --git a/src/libcamera/ipa_ipc.cpp b/src/libcamera/ipa_ipc.cpp\n> > > new file mode 100644\n> > > index 00000000..b963351f\n> > > --- /dev/null\n> > > +++ b/src/libcamera/ipa_ipc.cpp\n> > > @@ -0,0 +1,114 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * ipa_ipc.cpp - Image Processing Algorithm IPC module for IPA proxies\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> >\n> > The inclusion of the header corresponding to the cpp file is usually\n> > placed first, to ensure it is self-contained and does not depend on\n> > other include directives that might be placed before it.\n>\n> Ah, okay.\n>\n> > > +\n> > > +/**\n> > > + * \\file ipa_ipc.h\n> > > + * \\brief IPC mechanism for IPA isolation\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(IPAIPC)\n> > > +\n> > > +/**\n> > > + * \\class IPAIPC\n> > > + * \\brief IPC mechanism for IPA isolation\n> > > + *\n> > > + * Virtual class to model an IPC mechanism for use by IPA proxies for IPA\n> > > + * isolation. sendSync() and sendAsync() must be implemented, and the recvIPC\n> > > + * signal must be emitted whenever new data is available.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct an IPAIPC instance\n> > > + * \\param[in] ipa_module_path Path to the IPA module shared object\n> > > + * \\param[in] ipa_proxy_worker_path Path to the IPA proxy worker shared object\n> > > + */\n> > > +IPAIPC::IPAIPC([[maybe_unused]] const char *ipa_module_path,\n> > > +\t [[maybe_unused]] const char *ipa_proxy_worker_path)\n> >\n> > nit: parameters alignment\n>\n> This is aligned correctly. I guess patches mess up tabs?\n>\n> > If these fields are not used by the base virtual class, do we need\n> > them in the constructor ?\n>\n> I imagined that they would be required by all subclasses of IPAIPC, so I\n> wanted to put them here.\n>\n\nI would avoid that and only them in the dervied classes that need\nthem, or store them in the base class for derived to access them alternatively\n\n> > > +\t: valid_(false)\n> > > +{\n> > > +}\n> > > +\n> > > +IPAIPC::~IPAIPC()\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn IPAIPC::isValid()\n> > > + * \\brief Check if the IPAIPC instance is valid\n> > > + *\n> > > + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> > > + * isolation, and IPC is successfully set up.\n> > > + *\n> > > + * \\return True if the IPAIPC is valid, false otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAIPC::sendSync()\n> > > + * \\brief Send a message over IPC synchronously\n> > > + * \\param[in] cmd Command ID\n> > > + * \\param[in] data_in Data to send, as a byte vector\n> > > + * \\param[in] fds_in Fds to send, in a vector\n> > > + * \\param[in] data_out Byte vector in which to receive data, if applicable\n> > > + * \\param[in] fds_out Fds vector in which to receive fds, if applicable\n> >\n> > Could you spell 'file descriptors' entirely ?\n> > * \\param[in] fds_out Vector in which to receive file descriptors, if applicable\n> >\n> > Same for other locations.\n>\n> Okay.\n>\n> > I a bit wonder why fds should be kept separate and not serialized\n> > in the data_in vector. Does it depend on the fact that fds as\n> > serialized separately by the IPADataSerializer module ?\n>\n> Because if you send the fds as data they won't get translated :)\n>\n\nThis answer sound a bit like \"because yes\".\n\nMy question was more on the serialization part, where fds are always\ntreated as 'special' while in theory there is nothing preventing\nplacing them in the data buffer, as long as the serialization format\nallows to identify them.\n\nThis would anyway require re-thinking how the whole serialization\nprocess work, and I concur that's probably not worth it, but I still\nfail to see the technical reason why they're kept separate. An fd is a\nuin32_t like all other fields that could be part of a structure or class,\nafter all.\n\n> > A final minor note on this: are _in and _out the best names we can have ?\n> > Should this be expressed as '_msg' and '_reply' ?\n>\n> That'll work too. I just thought of it as input and output data.\n>\n\ninput output are fine too.\n\n> > > + *\n> > > + * This function will not return until a response is received. The event loop\n> > > + * will still continue to execute, however.\n> > > + *\n> > > + * \\return Zero on success, negative error code otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAIPC::sendAsync()\n> > > + * \\brief Send a message over IPC asynchronously\n> > > + * \\param[in] cmd Command ID\n> > > + * \\param[in] data_in Data to send, as a byte vector\n> > > + * \\param[in] fds_in Fds to send, in a vector\n> > > + *\n> > > + * This function will return immediately after sending the message.\n> > > + *\n> > > + * \\return Zero on success, negative error code otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAIPC::recvIPC\n> > > + * \\brief Signal to be emitted when data is received over IPC\n> > > + *\n> > > + * When data is received over IPC, this signal shall be emitted. Users must\n> > > + * connect to this to receive new data.\n> > > + *\n> > > + * The parameters of the signal are a vector of bytes and a vector of file\n> > > + * descriptors.\n> >\n> > I assume this completes the sendAsync() method, right ? Can we state\n> > that explicitely, if that's the case ?\n>\n> Not necessarily. One sendAsync() could have multiple responses.\n\nWhat I meant was: \"Is the recvIPA signal meant to be used to signal\na response to a asynchronous method call\" or are there other intended\nusages I am missing ?\n\n\n>\n> > Mostly minor comments, please add\n> > Reviewed-by: Jacopo Mondi \n>\n>\n> Thank you,\n>\n> Paul\n>\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAIPC::valid_\n> > > + * \\brief Flag to indicate if the IPAIPC instance is valid\n> > > + *\n> > > + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> > > + * isolation, and IPC is successfully set up.\n> > > + *\n> > > + * This flag can be read via IPAIPC::isValid().\n> > > + *\n> > > + * Implementations of the IPAIPC class should set this flag upon successful\n> > > + * construction.\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 190d7490..e0a2ab47 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -25,6 +25,7 @@ libcamera_sources = files([\n> > > 'ipa_context_wrapper.cpp',\n> > > 'ipa_controls.cpp',\n> > > 'ipa_data_serializer.cpp',\n> > > + 'ipa_ipc.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 0F1D1BDB1E\n\tfor ;\n\tThu, 29 Oct 2020 12:23:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94F86628AF;\n\tThu, 29 Oct 2020 13:23:46 +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 19E0E61563\n\tfor ;\n\tThu, 29 Oct 2020 13:23:46 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 8402B40002;\n\tThu, 29 Oct 2020 12:23:45 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 29 Oct 2020 13:23:45 +0100","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20201029122345.xhknnri3wb6pursd@uno.localdomain>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-12-paul.elder@ideasonboard.com>\n\t<20201029085352.wtxlfqppjfaqvqnr@uno.localdomain>\n\t<20201029102844.GA4944@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201029102844.GA4944@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","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":13554,"web_url":"https://patchwork.libcamera.org/comment/13554/","msgid":"<20201029123420.dzqdrunbcqrworcs@uno.localdomain>","date":"2020-10-29T12:34:20","subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again,\n\nOn Thu, Oct 29, 2020 at 07:28:44PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> Thank you for the review.\n>\n> On Thu, Oct 29, 2020 at 09:53:52AM +0100, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Fri, Oct 02, 2020 at 11:31:27PM +0900, Paul Elder wrote:\n> > > Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies\n> > > and proxy workers will call into the IPAIPC, rather than implementing\n> > > the IPC themselves.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v3:\n> > > - expand documentation a bit\n> > > - move [[maybe_unused]] to after the parameters in the IPAIPC\n> > > constructor, to appease gcc <= 9.1\n> > >\n> > > Changes in v2:\n> > > - add documentation\n> > > ---\n> > > include/libcamera/internal/ipa_ipc.h | 41 ++++++++++\n> > > src/libcamera/ipa_ipc.cpp | 114 +++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 3 files changed, 156 insertions(+)\n> > > create mode 100644 include/libcamera/internal/ipa_ipc.h\n> > > create mode 100644 src/libcamera/ipa_ipc.cpp\n> > >\n> > > diff --git a/include/libcamera/internal/ipa_ipc.h b/include/libcamera/internal/ipa_ipc.h\n> > > new file mode 100644\n> > > index 00000000..09de36a5\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/ipa_ipc.h\n> > > @@ -0,0 +1,41 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * ipa_ipc.h - Image Processing Algorithm IPC module for IPA proxies\n> > > + */\n> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> > > +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class IPAIPC\n> > > +{\n> > > +public:\n> > > +\tIPAIPC(const char *ipa_module_path [[maybe_unused]],\n> > > +\t const char *ipa_proxy_worker_path [[maybe_unused]]);\n> > > +\tvirtual ~IPAIPC();\n> > > +\n> > > +\tbool isValid() const { return valid_; }\n> >\n> > We usually have getters with the same name as the variable they \"get\".\n> > I'm fine with 'isValid()' but I felt like pointing this out.\n>\n> Oh, okay. IPAModule has this too though.\n>\n> > > +\n> > > +\tvirtual int 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 = nullptr,\n> > > +\t\t\t std::vector *fds_out = nullptr) = 0;\n> > > +\n> > > +\tvirtual int sendAsync(uint32_t cmd,\n> > > +\t\t\t const std::vector &data_in,\n> > > +\t\t\t const std::vector &fds_in) = 0;\n> > > +\n> > > +\tSignal &, std::vector &> recvIPC;\n> > > +\n> > > +protected:\n> > > +\tbool valid_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */\n> > > diff --git a/src/libcamera/ipa_ipc.cpp b/src/libcamera/ipa_ipc.cpp\n> > > new file mode 100644\n> > > index 00000000..b963351f\n> > > --- /dev/null\n> > > +++ b/src/libcamera/ipa_ipc.cpp\n> > > @@ -0,0 +1,114 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * ipa_ipc.cpp - Image Processing Algorithm IPC module for IPA proxies\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> >\n> > The inclusion of the header corresponding to the cpp file is usually\n> > placed first, to ensure it is self-contained and does not depend on\n> > other include directives that might be placed before it.\n>\n> Ah, okay.\n>\n> > > +\n> > > +/**\n> > > + * \\file ipa_ipc.h\n> > > + * \\brief IPC mechanism for IPA isolation\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(IPAIPC)\n> > > +\n> > > +/**\n> > > + * \\class IPAIPC\n> > > + * \\brief IPC mechanism for IPA isolation\n> > > + *\n> > > + * Virtual class to model an IPC mechanism for use by IPA proxies for IPA\n> > > + * isolation. sendSync() and sendAsync() must be implemented, and the recvIPC\n> > > + * signal must be emitted whenever new data is available.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct an IPAIPC instance\n> > > + * \\param[in] ipa_module_path Path to the IPA module shared object\n> > > + * \\param[in] ipa_proxy_worker_path Path to the IPA proxy worker shared object\n> > > + */\n> > > +IPAIPC::IPAIPC([[maybe_unused]] const char *ipa_module_path,\n> > > +\t [[maybe_unused]] const char *ipa_proxy_worker_path)\n> >\n> > nit: parameters alignment\n>\n> This is aligned correctly. I guess patches mess up tabs?\n>\n> > If these fields are not used by the base virtual class, do we need\n> > them in the constructor ?\n>\n> I imagined that they would be required by all subclasses of IPAIPC, so I\n> wanted to put them here.\n>\n> > > +\t: valid_(false)\n> > > +{\n> > > +}\n> > > +\n> > > +IPAIPC::~IPAIPC()\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn IPAIPC::isValid()\n> > > + * \\brief Check if the IPAIPC instance is valid\n> > > + *\n> > > + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> > > + * isolation, and IPC is successfully set up.\n> > > + *\n> > > + * \\return True if the IPAIPC is valid, false otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAIPC::sendSync()\n> > > + * \\brief Send a message over IPC synchronously\n> > > + * \\param[in] cmd Command ID\n> > > + * \\param[in] data_in Data to send, as a byte vector\n> > > + * \\param[in] fds_in Fds to send, in a vector\n> > > + * \\param[in] data_out Byte vector in which to receive data, if applicable\n> > > + * \\param[in] fds_out Fds vector in which to receive fds, if applicable\n> >\n> > Could you spell 'file descriptors' entirely ?\n> > * \\param[in] fds_out Vector in which to receive file descriptors, if applicable\n> >\n> > Same for other locations.\n>\n> Okay.\n>\n> > I a bit wonder why fds should be kept separate and not serialized\n> > in the data_in vector. Does it depend on the fact that fds as\n> > serialized separately by the IPADataSerializer module ?\n>\n> Because if you send the fds as data they won't get translated :)\n>\n> > A final minor note on this: are _in and _out the best names we can have ?\n> > Should this be expressed as '_msg' and '_reply' ?\n>\n> That'll work too. I just thought of it as input and output data.\n>\n> > > + *\n> > > + * This function will not return until a response is received. The event loop\n> > > + * will still continue to execute, however.\n> > > + *\n> > > + * \\return Zero on success, negative error code otherwise\n\nAlso, for both these methods, the return code reflects the message\nsending operation result, not the result of the operation itself. I\nthink it would help documenting it.\n\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn IPAIPC::sendAsync()\n> > > + * \\brief Send a message over IPC asynchronously\n> > > + * \\param[in] cmd Command ID\n> > > + * \\param[in] data_in Data to send, as a byte vector\n> > > + * \\param[in] fds_in Fds to send, in a vector\n> > > + *\n> > > + * This function will return immediately after sending the message.\n> > > + *\n> > > + * \\return Zero on success, negative error code otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAIPC::recvIPC\n> > > + * \\brief Signal to be emitted when data is received over IPC\n> > > + *\n> > > + * When data is received over IPC, this signal shall be emitted. Users must\n> > > + * connect to this to receive new data.\n> > > + *\n> > > + * The parameters of the signal are a vector of bytes and a vector of file\n> > > + * descriptors.\n> >\n> > I assume this completes the sendAsync() method, right ? Can we state\n> > that explicitely, if that's the case ?\n>\n> Not necessarily. One sendAsync() could have multiple responses.\n>\n> > Mostly minor comments, please add\n> > Reviewed-by: Jacopo Mondi \n>\n>\n> Thank you,\n>\n> Paul\n>\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var IPAIPC::valid_\n> > > + * \\brief Flag to indicate if the IPAIPC instance is valid\n> > > + *\n> > > + * An IPAIPC instance is valid if the IPA interface is successfully created in\n> > > + * isolation, and IPC is successfully set up.\n> > > + *\n> > > + * This flag can be read via IPAIPC::isValid().\n> > > + *\n> > > + * Implementations of the IPAIPC class should set this flag upon successful\n> > > + * construction.\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 190d7490..e0a2ab47 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -25,6 +25,7 @@ libcamera_sources = files([\n> > > 'ipa_context_wrapper.cpp',\n> > > 'ipa_controls.cpp',\n> > > 'ipa_data_serializer.cpp',\n> > > + 'ipa_ipc.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 04C6EBDB9B\n\tfor ;\n\tThu, 29 Oct 2020 12:34:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81FCF628AF;\n\tThu, 29 Oct 2020 13:34:22 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B7FB61563\n\tfor ;\n\tThu, 29 Oct 2020 13:34:21 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id E9546FF804;\n\tThu, 29 Oct 2020 12:34:20 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 29 Oct 2020 13:34:20 +0100","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20201029123420.dzqdrunbcqrworcs@uno.localdomain>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-12-paul.elder@ideasonboard.com>\n\t<20201029085352.wtxlfqppjfaqvqnr@uno.localdomain>\n\t<20201029102844.GA4944@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201029102844.GA4944@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC","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\" "}}]