[{"id":26950,"web_url":"https://patchwork.libcamera.org/comment/26950/","msgid":"","date":"2023-04-27T07:53:14","subject":"Re: [libcamera-devel] [PATCH 01/13] meson: ipa: Add mapping for\n\tpipeline handler to mojom interface file","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Wed, Apr 26, 2023 at 02:10:45PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Allow an arbitrary mapping between the pipeline handler and IPA mojom\n> interface file in the build system. This removes the 1:1 mapping of\n> pipeline handler name to mojom filename, and allows more flexibility to\n> pipeline developers.\n>\n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> Documentation/guides/ipa.rst | 19 ++++++++--------\n> include/libcamera/ipa/meson.build | 36 ++++++++++++++++++++-----------\n> 2 files changed, 33 insertions(+), 22 deletions(-)\n>\n> diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst\n> index fc0317451e24..89839408672a 100644\n> --- a/Documentation/guides/ipa.rst\n> +++ b/Documentation/guides/ipa.rst\n> @@ -269,35 +269,36 @@ The following is an example of an event interface definition:\n> Compiling the IPA interface\n> ---------------------------\n>\n> -After the IPA interface is defined in include/libcamera/ipa/{pipeline_name}.mojom,\n> +After the IPA interface is defined in include/libcamera/ipa/{interface_name}.mojom,\n> an entry for it must be added in meson so that it can be compiled. The filename\n> -must be added to the ipa_mojom_files object in include/libcamera/ipa/meson.build.\n> +must be added to the pipeline_ipa_mojom_mapping object in include/libcamera/ipa/meson.build.\n> +This object maps the pipeline handler name with an ipa interface file.\n>\n> For example, adding the raspberrypi.mojom file to meson:\n>\n> .. code-block:: none\n>\n> - ipa_mojom_files = [\n> - 'raspberrypi.mojom',\n> + pipeline_ipa_mojom_mapping = [\n> + 'raspberrypi': 'raspberrypi.mojom',\n> ]\n>\n> This will cause the mojo data definition file to be compiled. Specifically, it\n> generates five files:\n>\n> - a header describing the custom data structures, and the complete IPA\n> - interface (at {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_interface.h)\n> + interface (at {$build_dir}/include/libcamera/ipa/{interface}_ipa_interface.h)\n>\n> - a serializer implementing de/serialization for the custom data structures (at\n> - {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_serializer.h)\n> + {$build_dir}/include/libcamera/ipa/{interface}_ipa_serializer.h)\n>\n> - a proxy header describing a specialized IPA proxy (at\n> - {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_proxy.h)\n> + {$build_dir}/include/libcamera/ipa/{interface}_ipa_proxy.h)\n>\n> - a proxy source implementing the IPA proxy (at\n> - {$build_dir}/src/libcamera/proxy/{pipeline}_ipa_proxy.cpp)\n> + {$build_dir}/src/libcamera/proxy/{interface}_ipa_proxy.cpp)\n>\n> - a proxy worker source implementing the other end of the IPA proxy (at\n> - {$build_dir}/src/libcamera/proxy/worker/{pipeline}_ipa_proxy_worker.cpp)\n> + {$build_dir}/src/libcamera/proxy/worker/{interface}_ipa_proxy_worker.cpp)\n>\n> The IPA proxy serves as the layer between the pipeline handler and the IPA, and\n> handles threading vs isolation transparently. The pipeline handler and the IPA\n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index 442ca3dd7e1c..67c31cb04ccf 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -60,13 +60,15 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h',\n> './' +'@INPUT@'\n> ])\n>\n> -ipa_mojom_files = [\n> - 'ipu3.mojom',\n> - 'raspberrypi.mojom',\n> - 'rkisp1.mojom',\n> - 'vimc.mojom',\n> -]\n> -\n> +# Mapping from pipeline handler name to mojom file\n> +pipeline_ipa_mojom_mapping = {\n> + 'ipu3': 'ipu3.mojom',\n> + 'rkisp1': 'rkisp1.mojom',\n> + 'raspberrypi': 'raspberrypi.mojom',\n> + 'vimc': 'vimc.mojom',\n> +}\n> +\n> +ipa_mojom_files = []\n> ipa_mojoms = []\n>\n> #\n> @@ -75,14 +77,22 @@ ipa_mojoms = []\n>\n> # TODO Define per-pipeline ControlInfoMap with yaml?\n>\n> -foreach file : ipa_mojom_files\n> +foreach pipeline, file : pipeline_ipa_mojom_mapping\n> +\n> name = file.split('.')[0]\n>\n> - if name not in pipelines\n> + # Ensure we do not build duplicate mojom modules\n> + if (file in ipa_mojom_files)\n> + continue\n> + endif\n> +\n> + ipa_mojom_files += file\n> +\n> + if pipeline not in pipelines\n> continue\n> endif\n>\n> - # {pipeline}.mojom-module\n> + # {interface}.mojom-module\n> mojom = custom_target(name + '_mojom_module',\n> input : file,\n> output : file + '-module',\n> @@ -94,7 +104,7 @@ foreach file : ipa_mojom_files\n> '--mojoms', '@INPUT@'\n> ])\n>\n> - # {pipeline}_ipa_interface.h\n> + # {interface}_ipa_interface.h\n> header = custom_target(name + '_ipa_interface_h',\n> input : mojom,\n> output : name + '_ipa_interface.h',\n> @@ -110,7 +120,7 @@ foreach file : ipa_mojom_files\n> './' +'@INPUT@'\n> ])\n>\n> - # {pipeline}_ipa_serializer.h\n> + # {interface}_ipa_serializer.h\n> serializer = custom_target(name + '_ipa_serializer_h',\n> input : mojom,\n> output : name + '_ipa_serializer.h',\n> @@ -124,7 +134,7 @@ foreach file : ipa_mojom_files\n> './' +'@INPUT@'\n> ])\n>\n> - # {pipeline}_ipa_proxy.h\n> + # {interface}_ipa_proxy.h\n> proxy_header = custom_target(name + '_proxy_h',\n> input : mojom,\n> output : name + '_ipa_proxy.h',\n> --\n> 2.34.1\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 60C44BDCBD\n\tfor ;\n\tThu, 27 Apr 2023 07:53:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9446627E0;\n\tThu, 27 Apr 2023 09:53:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FAAC627D1\n\tfor ;\n\tThu, 27 Apr 2023 09:53:18 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:1cf0:b3bc:c785:4625])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55C9EC7E;\n\tThu, 27 Apr 2023 09:53:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682581999;\n\tbh=uchdZRRjo8toaOnWFJvuFz9wGA3Uz9fMUVXul0/q1V8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=cjiX1y8OJkEu9IlMvF7jFnT1Osxi9vtFz3HHbomCDAp0iSwKkZaWPgt7w1xVRxGEp\n\ttj7FywKCKuk4rLSC6zz3awrKM04GqNigqSKoZ45H36JFZ1SXAht/kBryFlP4iARHBt\n\tszoBVUBSoUEb0yQb66luTUxp4Xpr9mNzwuKGajaNTd8C4IGRBogYOHOwqLneYRwoxA\n\tfrWXKYZKDkAMvySayDzVh8wUxWjOVZbfDnwmdD0iYaGQnqUT7TuaJp+wdqsPMXc0vq\n\tL5jviuR0eYU72x4IRQWlwcfOCQ2zPmdXiBlxCr7cXOGm1zqS8LuOoCWesMYpiL0ZVt\n\tifsg/pQ0w5kpQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682581986;\n\tbh=uchdZRRjo8toaOnWFJvuFz9wGA3Uz9fMUVXul0/q1V8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RlB15TGI48qBjv2FbrTMLqY3IIMDx7VgAD91l6WXQJYec+G/CaXWK5PTJw1qFwCeo\n\tGiMABdFPpKIbfXpmpDMFY8OCP5rJaRleocaL4F4VNqfhpQM+ZqqQ11MHtn3yTbKyyX\n\tAY7BBa6hRe2b5i3BkBTknxS4GUcftUPn+vWhR2Fw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RlB15TGI\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 09:53:14 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 01/13] meson: ipa: Add mapping for\n\tpipeline handler to mojom interface file","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26963,"web_url":"https://patchwork.libcamera.org/comment/26963/","msgid":"<20230427121910.GK2326@pendragon.ideasonboard.com>","date":"2023-04-27T12:19:10","subject":"Re: [libcamera-devel] [PATCH 01/13] meson: ipa: Add mapping for\n\tpipeline handler to mojom interface file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Wed, Apr 26, 2023 at 02:10:45PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Allow an arbitrary mapping between the pipeline handler and IPA mojom\n> interface file in the build system. This removes the 1:1 mapping of\n> pipeline handler name to mojom filename, and allows more flexibility to\n> pipeline developers.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> Documentation/guides/ipa.rst | 19 ++++++++--------\n> include/libcamera/ipa/meson.build | 36 ++++++++++++++++++++-----------\n> 2 files changed, 33 insertions(+), 22 deletions(-)\n> \n> diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst\n> index fc0317451e24..89839408672a 100644\n> --- a/Documentation/guides/ipa.rst\n> +++ b/Documentation/guides/ipa.rst\n> @@ -269,35 +269,36 @@ The following is an example of an event interface definition:\n> Compiling the IPA interface\n> ---------------------------\n> \n> -After the IPA interface is defined in include/libcamera/ipa/{pipeline_name}.mojom,\n> +After the IPA interface is defined in include/libcamera/ipa/{interface_name}.mojom,\n\nThere are three references to {pipeline_name} above that need to be\naddressed.\n\nJust replacing {pipeline_name} with {interface_name} there would be\nconfusing I think, the text should be extended to explain that each\npipeline handler maps to an interface name, as to describe the rules\nthat govern the choice of the interface name (both the hard\nrequirements, and the recommended naming scheme).\n\nI think this change is interesting. I envision it could enable us to\nshare the same IPA modules between different pipeline handlers. For\ninstance, the rkisp1 pipeline handler supports both the RK3399 and the\ni.MX8MP as they integrate the same ISP. However, the i.MX8MP has a more\ncomplex camera pipeline, which may call for a dedicated pipeline handler\nin the future. As the ISP is the same as for RK3399, I think we could\nuse the same IPA module for both pipeline handlers, and would thus need\nthe same IPA interface.\n\nCould you capture this in the documentation when explaining the concept\nof IPA interface ? Otherwise, decoupling the pipeline handler name and\nthe IPA interface name without explaining why would lead to confusion.\n\n> an entry for it must be added in meson so that it can be compiled. The filename\n> -must be added to the ipa_mojom_files object in include/libcamera/ipa/meson.build.\n> +must be added to the pipeline_ipa_mojom_mapping object in include/libcamera/ipa/meson.build.\n> +This object maps the pipeline handler name with an ipa interface file.\n> \n> For example, adding the raspberrypi.mojom file to meson:\n> \n> .. code-block:: none\n> \n> - ipa_mojom_files = [\n> - 'raspberrypi.mojom',\n> + pipeline_ipa_mojom_mapping = [\n> + 'raspberrypi': 'raspberrypi.mojom',\n> ]\n> \n> This will cause the mojo data definition file to be compiled. Specifically, it\n> generates five files:\n> \n> - a header describing the custom data structures, and the complete IPA\n> - interface (at {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_interface.h)\n> + interface (at {$build_dir}/include/libcamera/ipa/{interface}_ipa_interface.h)\n> \n> - a serializer implementing de/serialization for the custom data structures (at\n> - {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_serializer.h)\n> + {$build_dir}/include/libcamera/ipa/{interface}_ipa_serializer.h)\n> \n> - a proxy header describing a specialized IPA proxy (at\n> - {$build_dir}/include/libcamera/ipa/{pipeline}_ipa_proxy.h)\n> + {$build_dir}/include/libcamera/ipa/{interface}_ipa_proxy.h)\n> \n> - a proxy source implementing the IPA proxy (at\n> - {$build_dir}/src/libcamera/proxy/{pipeline}_ipa_proxy.cpp)\n> + {$build_dir}/src/libcamera/proxy/{interface}_ipa_proxy.cpp)\n> \n> - a proxy worker source implementing the other end of the IPA proxy (at\n> - {$build_dir}/src/libcamera/proxy/worker/{pipeline}_ipa_proxy_worker.cpp)\n> + {$build_dir}/src/libcamera/proxy/worker/{interface}_ipa_proxy_worker.cpp)\n> \n> The IPA proxy serves as the layer between the pipeline handler and the IPA, and\n> handles threading vs isolation transparently. The pipeline handler and the IPA\n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index 442ca3dd7e1c..67c31cb04ccf 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -60,13 +60,15 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h',\n> './' +'@INPUT@'\n> ])\n> \n> -ipa_mojom_files = [\n> - 'ipu3.mojom',\n> - 'raspberrypi.mojom',\n> - 'rkisp1.mojom',\n> - 'vimc.mojom',\n> -]\n> -\n> +# Mapping from pipeline handler name to mojom file\n> +pipeline_ipa_mojom_mapping = {\n> + 'ipu3': 'ipu3.mojom',\n> + 'rkisp1': 'rkisp1.mojom',\n> + 'raspberrypi': 'raspberrypi.mojom',\n> + 'vimc': 'vimc.mojom',\n> +}\n> +\n> +ipa_mojom_files = []\n> ipa_mojoms = []\n> \n> #\n> @@ -75,14 +77,22 @@ ipa_mojoms = []\n> \n> # TODO Define per-pipeline ControlInfoMap with yaml?\n> \n> -foreach file : ipa_mojom_files\n> +foreach pipeline, file : pipeline_ipa_mojom_mapping\n> +\n> name = file.split('.')[0]\n> \n> - if name not in pipelines\n> + # Ensure we do not build duplicate mojom modules\n\nIt sounds like that would be a major bug in meson.build, so it should be\nreported as an error if it happens. The risk sounds low though, so I'd\nalso be fine dropping the check.\n\n> + if (file in ipa_mojom_files)\n\nNo need for parentheses.\n\n> + continue\n> + endif\n> +\n> + ipa_mojom_files += file\n> +\n> + if pipeline not in pipelines\n> continue\n> endif\n> \n> - # {pipeline}.mojom-module\n> + # {interface}.mojom-module\n> mojom = custom_target(name + '_mojom_module',\n> input : file,\n> output : file + '-module',\n> @@ -94,7 +104,7 @@ foreach file : ipa_mojom_files\n> '--mojoms', '@INPUT@'\n> ])\n> \n> - # {pipeline}_ipa_interface.h\n> + # {interface}_ipa_interface.h\n> header = custom_target(name + '_ipa_interface_h',\n> input : mojom,\n> output : name + '_ipa_interface.h',\n> @@ -110,7 +120,7 @@ foreach file : ipa_mojom_files\n> './' +'@INPUT@'\n> ])\n> \n> - # {pipeline}_ipa_serializer.h\n> + # {interface}_ipa_serializer.h\n> serializer = custom_target(name + '_ipa_serializer_h',\n> input : mojom,\n> output : name + '_ipa_serializer.h',\n> @@ -124,7 +134,7 @@ foreach file : ipa_mojom_files\n> './' +'@INPUT@'\n> ])\n> \n> - # {pipeline}_ipa_proxy.h\n> + # {interface}_ipa_proxy.h\n> proxy_header = custom_target(name + '_proxy_h',\n> input : mojom,\n> output : name + '_ipa_proxy.h',","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 EF1D4C0DA4\n\tfor ;\n\tThu, 27 Apr 2023 12:19:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50F24627D6;\n\tThu, 27 Apr 2023 14:19:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AB75627B7\n\tfor ;\n\tThu, 27 Apr 2023 14:18:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 82A049DE;\n\tThu, 27 Apr 2023 14:18:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682597941;\n\tbh=W5CTTF+LAEuqSiL7pXld5A1Cn/nBAw8EOwqvxyxfQr4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=a0WGEAsPsxFE1frDz/zAIXnfdDkCjjNo2DLoCqEKEn1aC76wt0kQ+mfqBVPa/4I7S\n\tCGT99GqjaU136Ff1iv2SMeM0w6WthXjjlUuWajFODXJJnkofJ1n7uMY7AXZKSEe6ER\n\tZYhh8zJWHXpBh4Jq32ONPLWT8pEAsodUsvgD1K00j1XwM2M2EV5IBNkYtJF724SIPN\n\tz7p81FHwge7qWN+nSUY/oDTBMYIsK4wUvWuzyIbncD3Yb/ohg+nZFa5TKrtvn2Aoq3\n\tvOPXxyLxcMpFxsR6hW/4YXPnE3BgI4HzSI/94jCXUpuD9gtUJb8+0jbAqVP2iadCpz\n\tioA6Oe7FFbImg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682597927;\n\tbh=W5CTTF+LAEuqSiL7pXld5A1Cn/nBAw8EOwqvxyxfQr4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bHS3EOVXgs2mSBQrXZD/MUJrvOvp2LvJsRlLLR15pzma/TvmZEyy9Upv2kGHZcYq1\n\tlYwR66lO5FB6aT9UBYuQTAdV3MAneBmAnLs3Qtej8zba59IW7BR1LAFvJVE7UtrnM6\n\tyXU718ZznGllBziPycKM3GguuOMZKa6QAnG1LsWY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bHS3EOVX\"; dkim-atps=neutral","Date":"Thu, 27 Apr 2023 15:19:10 +0300","To":"Naushir Patuck ","Message-ID":"<20230427121910.GK2326@pendragon.ideasonboard.com>","References":"<20230426131057.21550-1-naush@raspberrypi.com>\n\t<20230426131057.21550-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230426131057.21550-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 01/13] meson: ipa: Add mapping for\n\tpipeline handler to mojom interface file","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]