[{"id":13920,"web_url":"https://patchwork.libcamera.org/comment/13920/","msgid":"<20201126143304.GN3905@pendragon.ideasonboard.com>","date":"2020-11-26T14:33:04","subject":"Re: [libcamera-devel] [PATCH v4 24/37] ipa: Add core.mojom","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:54PM +0900, Paul Elder wrote:\n> Add a base mojom file to contain empty mojom definitions of libcamera\n> objects, as well as documentation for the IPA interfaces that need to be\n> defined in the mojom files.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v4:\n> - move docs to IPA guide\n> \n> Changes in v3:\n> - add doc that structs need to be defined\n> - add doc to recommend namespacing\n> - change indentation\n> - add requirement that base controls *must* be defined in\n> libcamera::{pipeline_name}::Controls\n> \n> New in v2\n> ---\n> include/libcamera/ipa/core.mojom | 18 ++++++++++++++++++\n> include/libcamera/ipa/meson.build | 18 +++++++++++++++---\n> src/libcamera/proxy/meson.build | 2 +-\n> src/libcamera/proxy/worker/meson.build | 2 +-\n> 4 files changed, 35 insertions(+), 5 deletions(-)\n> create mode 100644 include/libcamera/ipa/core.mojom\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> new file mode 100644\n> index 00000000..ed26aaad\n> --- /dev/null\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -0,0 +1,18 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +\n> +/*\n> + * Any libcamera objects that are used by any interfaces must be declared\n> + * here, and a de/serializer be implemented in IPADataSerializer. In addition,\n\ns/de\\/serializer/(de)serializer/ ?\ns/in IPADataSerializer/as a template specialization of IPADataSerializer/ ?\n\n> + * the corresponding header file (or forward-declarations) must be placed in\n\ns/placed/included/\n\n> + * {pipeline_name}.h.\n> + *\n> + * For libcamera types, the [hasFd] attribute is needed to notify the compiler\n> + * that the struct embeds a FileDescriptor.\n> + */\n> +struct CameraSensorInfo {};\n> +struct ControlInfoMap {};\n> +struct ControlList {};\n> +struct FileDescriptor {};\n> +[hasFd] struct IPABuffer {};\n> +struct IPASettings {};\n> +struct IPAStream {};\n\nI think the last three structures can be defined fully in core.mojom.\nThe comment above will need to be updated, to reflect the fact that\nspecializations of IPADataSerializer are only needed for opaque\nstructures, not structures defined in mojom.\n\n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index d7a2b6b2..b7caec95 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -13,6 +13,17 @@ install_headers(libcamera_ipa_headers,\n> # Prepare IPA/IPC generation components\n> #\n> \n> +core_mojom_file = 'core.mojom'\n> +ipa_mojom_core = custom_target(core_mojom_file.split('.')[0] + '_mojom_module',\n> + input : core_mojom_file,\n> + output : core_mojom_file + '-module',\n> + command : [\n> + mojom_parser,\n> + '--output-root', meson.build_root(),\n> + '--input-root', meson.source_root(),\n> + '--mojoms', '@INPUT@'\n> + ])\n\nIt would be nice if the mojo parser was able to handle imports\nautomatically :-S\n\n> +\n> ipa_mojom_files = []\n> \n> ipa_mojoms = []\n> @@ -30,6 +41,7 @@ foreach file : ipa_mojom_files\n> mojom = custom_target(file.split('.')[0] + '_mojom_module',\n> input : file,\n> output : file + '-module',\n> + depends : ipa_mojom_core,\n> command : [\n> mojom_parser,\n> '--output-root', meson.build_root(),\n> @@ -41,7 +53,7 @@ foreach file : ipa_mojom_files\n> header = custom_target(name + '_ipa_interface_h',\n> input : mojom,\n> output : name + '_ipa_interface.h',\n> - depends : mojom_templates,\n> + depends : [mojom_templates, ipa_mojom_core],\n\nDo we need this, given that input is set to mojom, which depends on\nipa_mojom_core already ? The dependency is for the parsing step, not the\ngeneration step. I think all the dependencies below can be dropped.\n\nReviewed-by: Laurent Pinchart \n\n> command : [\n> mojom_generator, 'generate',\n> '-g', 'libcamera',\n> @@ -55,7 +67,7 @@ foreach file : ipa_mojom_files\n> serializer = custom_target(name + '_ipa_serializer_h',\n> input : mojom,\n> output : name + '_ipa_serializer.h',\n> - depends : mojom_templates,\n> + depends : [mojom_templates, ipa_mojom_core],\n> command : [\n> mojom_generator, 'generate',\n> '-g', 'libcamera',\n> @@ -69,7 +81,7 @@ foreach file : ipa_mojom_files\n> proxy_header = custom_target(name + '_proxy_h',\n> input : mojom,\n> output : 'ipa_proxy_' + name + '.h',\n> - depends : mojom_templates,\n> + depends : [mojom_templates, ipa_mojom_core],\n> command : [\n> mojom_generator, 'generate',\n> '-g', 'libcamera',\n> diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build\n> index ac68ad08..f27b453a 100644\n> --- a/src/libcamera/proxy/meson.build\n> +++ b/src/libcamera/proxy/meson.build\n> @@ -10,7 +10,7 @@ foreach mojom : ipa_mojoms\n> proxy = custom_target(mojom['name'] + '_proxy_cpp',\n> input : mojom['mojom'],\n> output : 'ipa_proxy_' + mojom['name'] + '.cpp',\n> - depends : [mojom_templates],\n> + depends : [mojom_templates, ipa_mojom_core],\n> command : [\n> mojom_generator, 'generate',\n> '-g', 'libcamera',\n> diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build\n> index 8e0b978a..628bb050 100644\n> --- a/src/libcamera/proxy/worker/meson.build\n> +++ b/src/libcamera/proxy/worker/meson.build\n> @@ -11,7 +11,7 @@ foreach mojom : ipa_mojoms\n> worker = custom_target(mojom['name'] + '_proxy_worker',\n> input : mojom['mojom'],\n> output : 'ipa_proxy_' + mojom['name'] + '_worker.cpp',\n> - depends : [mojom_templates],\n> + depends : [mojom_templates, ipa_mojom_core],\n> command : [\n> mojom_generator, 'generate',\n> '-g', 'libcamera',","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 C00FDBE176\n\tfor ;\n\tThu, 26 Nov 2020 14:33:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B3B663470;\n\tThu, 26 Nov 2020 15:33:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18BD9633F6\n\tfor ;\n\tThu, 26 Nov 2020 15:33:13 +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 8E604A1B;\n\tThu, 26 Nov 2020 15:33:12 +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=\"L6F0UOYB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606401192;\n\tbh=Gue/pB5gdRBWl3ZSZ4W7Blk4AHzIXkgXPUPLnTzIb5o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=L6F0UOYBeHQYHqnQTVu6lRKDI0kQBGPPwqm76U7BogCR95EVZwvwj10nKQwz1WIcp\n\tsVaGgkDe3496mP0uErbUsRX4rqIVbyJMiTA+H9bHX41L4JyE12jPx3Kg6pj1smUmPz\n\tkLBFI5nhGQ/W7cL7hvPbgdct8nzgcHSwIivyEIpY=","Date":"Thu, 26 Nov 2020 16:33:04 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201126143304.GN3905@pendragon.ideasonboard.com>","References":"<20201106103707.49660-1-paul.elder@ideasonboard.com>\n\t<20201106103707.49660-25-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201106103707.49660-25-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 24/37] ipa: Add core.mojom","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\" "}}]