[{"id":13682,"web_url":"https://patchwork.libcamera.org/comment/13682/","msgid":"<20201112110539.GD1480295@oden.dyn.berto.se>","date":"2020-11-12T11:05:39","subject":"Re: [libcamera-devel] [PATCH v4 09/37] libcamera: Add IPAIPC","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for your work.\n\nOn 2020-11-06 19:36:39 +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> Reviewed-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\n> \n> ---\n> Change in v4:\n> - change snake_case to camelCase\n> - remove ipaModulePath and ipaProxyWorkerPath from parameters in the\n> IPAIPC base contructor\n> - include signal.h\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 | 42 ++++++++++\n> src/libcamera/ipa_ipc.cpp | 110 +++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 153 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..f83a90a3\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipa_ipc.h\n> @@ -0,0 +1,42 @@\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> +#include \n> +\n> +namespace libcamera {\n> +\n> +class IPAIPC\n> +{\n> +public:\n> +\tIPAIPC();\n> +\tvirtual ~IPAIPC();\n> +\n> +\tbool isValid() const { return valid_; }\n> +\n> +\tvirtual int sendSync(uint32_t cmd,\n> +\t\t\t const std::vector &dataIn,\n> +\t\t\t const std::vector &fdsIn,\n> +\t\t\t std::vector *dataOut = nullptr,\n> +\t\t\t std::vector *fdsOut = nullptr) = 0;\n> +\n> +\tvirtual int sendAsync(uint32_t cmd,\n> +\t\t\t const std::vector &dataIn,\n> +\t\t\t const std::vector &fdsIn) = 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..964d4ac1\n> --- /dev/null\n> +++ b/src/libcamera/ipa_ipc.cpp\n> @@ -0,0 +1,110 @@\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> +/**\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> + */\n> +IPAIPC::IPAIPC() : 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] dataIn Data to send, as a byte vector\n> + * \\param[in] fdsIn Fds to send, in a vector\n> + * \\param[in] dataOut Byte vector in which to receive data, if applicable\n> + * \\param[in] fdsOut Fds vector in which to receive fds, if applicable\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] dataIn Data to send, as a byte vector\n> + * \\param[in] fdsIn 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> +\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 01bcffd4..85f3a202 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 9CA84BE082\n\tfor ;\n\tThu, 12 Nov 2020 11:05:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F77463162;\n\tThu, 12 Nov 2020 12:05:42 +0100 (CET)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A78046305E\n\tfor ;\n\tThu, 12 Nov 2020 12:05:41 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id j205so7745920lfj.6\n\tfor ;\n\tThu, 12 Nov 2020 03:05:41 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tb20sm405521lfc.89.2020.11.12.03.05.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 12 Nov 2020 03:05:40 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"D6d6GxcR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=E35QqowK7alioV3PGUDjjlxF1vTAkO20LVHSd0Vemso=;\n\tb=D6d6GxcRYCQfgzAcdCZMfVdAnvrxU2H3fK+cLurcbqilmL5B5cku9RfGk3L+uNa7Qo\n\ty1SdIX6C1UcYsvsfoU5Exh67M50f0jmdA6XTv0CfQzNbXCjUPfxBgxR1ZxoQKdP5EC9q\n\tJDS6KmX6pBhYuQ+LyDlnHaPBv0goRVoVBppHGXv1d0MqLHyzD+VwOhL08Yl+OO9YlvYv\n\tP06HGj90vDkIzN0eQJs3MxNqx7uB/WY+cnyYlW+O0Vo3BVwwzkZWYSctnYU17b0dZq4u\n\tdnCUFUnTX7NUyVUuElBhyRx485wxn4SOThySwqaB/JP9VXMzECFznyFIqwtz/FvvNWLC\n\tfqXw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=E35QqowK7alioV3PGUDjjlxF1vTAkO20LVHSd0Vemso=;\n\tb=MnNaZjMvCtQGy1iGuo4wbBc6EuX/t+1YwmyDYyOw1V4u0d70iQlKSgO05pHUXYW7kg\n\tYrYbcyoLmxCJbyv39x6V/bMeuyUGeHf8Uy/99TrHuuNT53ksmeX/L+WXxKzYCxC3uKoM\n\t0YktBxnNC5jd2CMDLncVLZZfF5smVlgDqSidUaubcN5Is81SfS126BruH0evGSY4r0Oe\n\tx9eh6hDt+IRvm75vgvnaND56qMCP7VdQDzPFvzX0YCLTo6D3gHiA+J5x2LUvLz/en67l\n\tazaDxXs1tqGzqWYaL5FDJn/rZY0AHDGG1LruSPKhjgzLMmwovRKe2ymzHLBhscMCOd3H\n\tnrlA==","X-Gm-Message-State":"AOAM530r04pTULlDSCjLV1dAJQKSGvvTdHitt0o4k03vKQ91z7im12Ea\n\tZl8Th7g9BzPd2T1WaxMFDx40Bw==","X-Google-Smtp-Source":"ABdhPJz9KomnPrb9dvBV4Q76At8JKCoNxiZpbpyLNYRJsRmUEzQ/KuxT9Uwd7PEr1k/jS9/tvJkg7w==","X-Received":"by 2002:ac2:518f:: with SMTP id u15mr5227205lfi.6.1605179140974; \n\tThu, 12 Nov 2020 03:05:40 -0800 (PST)","Date":"Thu, 12 Nov 2020 12:05:39 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Paul Elder ","Message-ID":"<20201112110539.GD1480295@oden.dyn.berto.se>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-10-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-10-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 09/37] 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13751,"web_url":"https://patchwork.libcamera.org/comment/13751/","msgid":"<20201117145929.obmfpbs4gklmbhxj@uno.localdomain>","date":"2020-11-17T14:59:29","subject":"Re: [libcamera-devel] [PATCH v4 09/37] 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, Nov 06, 2020 at 07:36:39PM +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> Reviewed-by: Jacopo Mondi \n>\n> ---\n> Change in v4:\n> - change snake_case to camelCase\n> - remove ipaModulePath and ipaProxyWorkerPath from parameters in the\n> IPAIPC base contructor\n> - include signal.h\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 | 42 ++++++++++\n> src/libcamera/ipa_ipc.cpp | 110 +++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 153 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..f83a90a3\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipa_ipc.h\n> @@ -0,0 +1,42 @@\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> +#include \n> +\n> +namespace libcamera {\n> +\n> +class IPAIPC\n> +{\n> +public:\n> +\tIPAIPC();\n> +\tvirtual ~IPAIPC();\n> +\n> +\tbool isValid() const { return valid_; }\n> +\n> +\tvirtual int sendSync(uint32_t cmd,\n> +\t\t\t const std::vector &dataIn,\n> +\t\t\t const std::vector &fdsIn,\n> +\t\t\t std::vector *dataOut = nullptr,\n> +\t\t\t std::vector *fdsOut = nullptr) = 0;\n> +\n> +\tvirtual int sendAsync(uint32_t cmd,\n> +\t\t\t const std::vector &dataIn,\n> +\t\t\t const std::vector &fdsIn) = 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..964d4ac1\n> --- /dev/null\n> +++ b/src/libcamera/ipa_ipc.cpp\n> @@ -0,0 +1,110 @@\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> +/**\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> + */\n> +IPAIPC::IPAIPC() : valid_(false)\n\nNit: we usually break variable initialization to a new line\n\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] dataIn Data to send, as a byte vector\n> + * \\param[in] fdsIn Fds to send, in a vector\n> + * \\param[in] dataOut Byte vector in which to receive data, if applicable\n> + * \\param[in] fdsOut Fds vector in which to receive fds, if applicable\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] dataIn Data to send, as a byte vector\n> + * \\param[in] fdsIn 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> +\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 01bcffd4..85f3a202 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>","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 30120BE082\n\tfor ;\n\tTue, 17 Nov 2020 14:59:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA85A6331E;\n\tTue, 17 Nov 2020 15:59:28 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E4CB6033B\n\tfor ;\n\tTue, 17 Nov 2020 15:59:27 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 0871F20018;\n\tTue, 17 Nov 2020 14:59:26 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 17 Nov 2020 15:59:29 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20201117145929.obmfpbs4gklmbhxj@uno.localdomain>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-10-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-10-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 09/37] 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":13813,"web_url":"https://patchwork.libcamera.org/comment/13813/","msgid":"<20201120124214.GG4563@pendragon.ideasonboard.com>","date":"2020-11-20T12:42:14","subject":"Re: [libcamera-devel] [PATCH v4 09/37] libcamera: Add IPAIPC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Nov 06, 2020 at 07:36:39PM +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> Reviewed-by: Jacopo Mondi \n> \n> ---\n> Change in v4:\n> - change snake_case to camelCase\n> - remove ipaModulePath and ipaProxyWorkerPath from parameters in the\n> IPAIPC base contructor\n> - include signal.h\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 | 42 ++++++++++\n> src/libcamera/ipa_ipc.cpp | 110 +++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 153 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..f83a90a3\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipa_ipc.h\n> @@ -0,0 +1,42 @@\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> +#include \n> +\n> +namespace libcamera {\n> +\n> +class IPAIPC\n> +{\n> +public:\n> +\tIPAIPC();\n> +\tvirtual ~IPAIPC();\n> +\n> +\tbool isValid() const { return valid_; }\n> +\n> +\tvirtual int sendSync(uint32_t cmd,\n> +\t\t\t const std::vector &dataIn,\n> +\t\t\t const std::vector &fdsIn,\n> +\t\t\t std::vector *dataOut = nullptr,\n> +\t\t\t std::vector *fdsOut = nullptr) = 0;\n\nHow about grouping the command, data and fds in an IPAIPCMessage class ?\nThis would become\n\n\tvirtual int sendSync(const IPAIPCMessage &msg,\n\t\t\t IPAIPCMessage *response = nullptr) = 0;\n\nwhich should be easier to read. Same for sendAsync() and recvIPC.\n\nI'm even tempted to merge sendSync() and sendAsync() into a single\nfunction with an additional flag argument, with\n\nclass IPCMessage {\npublic:\n\tenum Flag {\n\t\tSync = 1 << 0,\n\t};\n\n\t...\n};\n\n> +\n> +\tvirtual int sendAsync(uint32_t cmd,\n> +\t\t\t const std::vector &dataIn,\n> +\t\t\t const std::vector &fdsIn) = 0;\n> +\n> +\tSignal &, std::vector &> recvIPC;\n\nThis can be called recv or receive.\n\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..964d4ac1\n> --- /dev/null\n> +++ b/src/libcamera/ipa_ipc.cpp\n> @@ -0,0 +1,110 @@\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\nI wonder if we should drop the IPA prefix in various places, as the IPC\nmechanism could be used for other purposes too (although I don't have a\nuse case in mind at the moment). This class could be called IPCBase, or\nIPCPipe, or IPCMessagePipe for instance (or ipc::MessagePipe, or...).\n\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\nYou can drop all the headers above.\n\n> +#include \"libcamera/internal/ipa_ipc.h\"\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\nDo you think \"message pipe\" would be a more accurate description than\n\"mechanism\" ? The component modelled by this base class is meant to send\nand receive messages over a pipe, so I think it's a good match.\n\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> + */\n> +IPAIPC::IPAIPC() : 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\nIf we're talking about a pipe, would isConnected() be a better name ?\nI'd also avoid talking about isolation or IPA interface here, to avoid\ncoupling the class with IPA modules.\n\n> +\n> +/**\n> + * \\fn IPAIPC::sendSync()\n> + * \\brief Send a message over IPC synchronously\n\nThere we go, we're sending messages already :-)\n\n> + * \\param[in] cmd Command ID\n> + * \\param[in] dataIn Data to send, as a byte vector\n> + * \\param[in] fdsIn Fds to send, in a vector\n> + * \\param[in] dataOut Byte vector in which to receive data, if applicable\n> + * \\param[in] fdsOut Fds vector in which to receive fds, if applicable\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] dataIn Data to send, as a byte vector\n> + * \\param[in] fdsIn 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> +\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 01bcffd4..85f3a202 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\nAnother close call for alphabetical order :-)\n\n> 'ipa_manager.cpp',\n> 'ipa_module.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2BB51BE176\n\tfor ;\n\tFri, 20 Nov 2020 12:42:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A1440615B5;\n\tFri, 20 Nov 2020 13:42:23 +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 C004C61564\n\tfor ;\n\tFri, 20 Nov 2020 13:42:21 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2BEC22A3;\n\tFri, 20 Nov 2020 13:42:21 +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=\"syEkV9KF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605876141;\n\tbh=38KXKAf26N+jrBORL20kdksvrz/hfzkCFQC/fmYAnOs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=syEkV9KF2MAuf+L88emswoZ9mjgENFGwBvXPf9Jl31xUe/mV1pMnTQ11yLE2s1cpj\n\tGALVU4XzrhGwyVO6HD9LlmivS3iLN2P71LMEP+/tGy7MFoXJNBG3MDg6htLqcW12g7\n\tazreBnLmH8eN57+MEbDnmplLEEaFzwlts/WxlVLE=","Date":"Fri, 20 Nov 2020 14:42:14 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201120124214.GG4563@pendragon.ideasonboard.com>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-10-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-10-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 09/37] 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\" "}}]