[{"id":34176,"web_url":"https://patchwork.libcamera.org/comment/34176/","msgid":"<174696016162.4078945.4469923532661672470@ping.linuxembedded.co.uk>","date":"2025-05-11T10:42:41","subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2025-05-10 15:12:19)\n> Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for\n> more then 1 pipeline handler.\n\nI can see this being useful in other pipelines too - as I think we can\nanticipate multiple pipelines that can share a common IPA (certainly the\nSoftIPA/GPU-IPA) but also potentially other derivatives of the RKISP1\npipeline handler.\n\n> Currently createIPA() / IPAManager::module() assume that there is a 1:1\n> relationship between pipeline handlers and IPAs and IPA matching is done\n> based on matching the pipe to ipaModuleInfo.pipelineName[].\n> \n> One way to allow using a single IPA with multiple pipelines would be to\n> allow the IPA to declare itself compatible with more then one pipeline,\n> turning ipaModuleInfo.pipelineName[] into e.g. a vector. But the way\n> ipaModuleInfo is loaded as an ELF symbol requires it to be a simple flat\n> C-struct.\n> \n> Instead add an optional ipaName argument to createIPA() which when set\n> switches things over to matching ipaModuleInfo.name[] allowing pipelines\n> to request a specific shared IPA module this way.\n\nI think that's reasonable. The Pipeline handler knows more about the\nsystem than the IPA ... so it's more reasonable for the PH to say \"I can\nuse this\" rather than the IPA to say \"I support X PipelineHandlers\"\n\n> Signed-off-by: Hans de Goede \n> ---\n> include/libcamera/internal/ipa_manager.h | 7 ++++---\n> include/libcamera/internal/ipa_module.h | 4 ++--\n> src/libcamera/ipa_manager.cpp | 6 ++++--\n> src/libcamera/ipa_module.cpp | 19 +++++++++++++------\n> 4 files changed, 23 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index a0d448cf..af784c9c 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -34,11 +34,12 @@ public:\n> template\n> static std::unique_ptr createIPA(PipelineHandler *pipe,\n> uint32_t minVersion,\n> - uint32_t maxVersion)\n> + uint32_t maxVersion,\n> + const char *ipaName = NULL)\n> {\n> CameraManager *cm = pipe->cameraManager();\n> IPAManager *self = cm->_d()->ipaManager();\n> - IPAModule *m = self->module(pipe, minVersion, maxVersion);\n> + IPAModule *m = self->module(pipe, minVersion, maxVersion, ipaName);\n> if (!m)\n> return nullptr;\n> \n> @@ -64,7 +65,7 @@ private:\n> unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n> \n> IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n> - uint32_t maxVersion);\n> + uint32_t maxVersion, const char *ipaName);\n> \n> bool isSignatureValid(IPAModule *ipa) const;\n> \n> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h\n> index 15f19492..e7b00fdb 100644\n> --- a/include/libcamera/internal/ipa_module.h\n> +++ b/include/libcamera/internal/ipa_module.h\n> @@ -36,8 +36,8 @@ public:\n> \n> IPAInterface *createInterface();\n> \n> - bool match(PipelineHandler *pipe,\n> - uint32_t minVersion, uint32_t maxVersion) const;\n> + bool match(PipelineHandler *pipe, uint32_t minVersion,\n> + uint32_t maxVersion, const char *ipaName = NULL) const;\n> \n> protected:\n> std::string logPrefix() const override;\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 830750dc..2ef8b98e 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -240,12 +240,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> * \\param[in] pipe The pipeline handler\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n\nI wondered about saying \"Defaults to the pipeline handler name\" ... but\neither way.\n\n> */\n> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n> - uint32_t maxVersion)\n> + uint32_t maxVersion, const char *ipaName)\n> {\n> for (const auto &module : modules_) {\n> - if (module->match(pipe, minVersion, maxVersion))\n> + if (module->match(pipe, minVersion, maxVersion, ipaName))\n> return module.get();\n> }\n> \n> @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n> * \\param[in] pipe The pipeline handler that wants a matching IPA proxy\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> *\n> * \\return A newly created IPA proxy, or nullptr if no matching IPA module is\n> * found or if the IPA proxy fails to initialize\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index e6ea61e4..b7004b1c 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface()\n> * \\param[in] pipe Pipeline handler to match with\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> *\n> * This function checks if this IPA module matches the \\a pipe pipeline handler,\n> - * and the input version range.\n> + * and the input version range. If \\a ipaName is non-null then the IPA module\n> + * name is matched against \\a ipaName instead of matching \\a pipe.\n> *\n> * \\return True if the pipeline handler matches the IPA module, or false otherwise\n> */\n> -bool IPAModule::match(PipelineHandler *pipe,\n> - uint32_t minVersion, uint32_t maxVersion) const\n> +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion,\n> + uint32_t maxVersion, const char *ipaName) const\n> {\n> - return info_.pipelineVersion >= minVersion &&\n> - info_.pipelineVersion <= maxVersion &&\n> - !strcmp(info_.pipelineName, pipe->name());\n> + if (info_.pipelineVersion < minVersion ||\n> + info_.pipelineVersion > maxVersion)\n> + return false;\n> +\n> + if (ipaName)\n> + return !strcmp(info_.name, ipaName);\n> + else\n> + return !strcmp(info_.pipelineName, pipe->name());\n\nThe code looks fine so:\n\nReviewed-by: Kieran Bingham \n\n> }\n> \n> std::string IPAModule::logPrefix() const\n> -- \n> 2.49.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 BCF0CC3200\n\tfor ;\n\tSun, 11 May 2025 10:42:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1721F68B55;\n\tSun, 11 May 2025 12:42:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85A2A68B51\n\tfor ;\n\tSun, 11 May 2025 12:42:44 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3E8EC63B;\n\tSun, 11 May 2025 12:42:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kH7Npd/N\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746960150;\n\tbh=9UxObX3/jjett+U0TT3vgb0k+HQ8LQNNM4VBjKgOsU4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=kH7Npd/NY1Li4QsjPsUCjrAJOUyAvVvNtORqjzDn+LwQOI9DM5HRJozne2fpsNQ/G\n\ttD6YgzR3eek7AxM9qDUG0nE+3p2V6hFGT/rJ8upA93DwF/jR+pF349JJlGzamwRPaQ\n\tqxhy7QJnl5gvHG3c2wCXFMb5pD4ihZJgGaWgsliU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250510141220.54872-8-hdegoede@redhat.com>","References":"<20250510141220.54872-1-hdegoede@redhat.com>\n\t<20250510141220.54872-8-hdegoede@redhat.com>","Subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","From":"Kieran Bingham ","Cc":"Milan Zamazal , Hans de Goede ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Sun, 11 May 2025 11:42:41 +0100","Message-ID":"<174696016162.4078945.4469923532661672470@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34180,"web_url":"https://patchwork.libcamera.org/comment/34180/","msgid":"<1101a528-0620-4165-a94b-c30d556de72e@redhat.com>","date":"2025-05-11T13:49:13","subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nOn 11-May-25 12:42 PM, Kieran Bingham wrote:\n> Quoting Hans de Goede (2025-05-10 15:12:19)\n>> Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for\n>> more then 1 pipeline handler.\n> \n> I can see this being useful in other pipelines too - as I think we can\n> anticipate multiple pipelines that can share a common IPA (certainly the\n> SoftIPA/GPU-IPA) but also potentially other derivatives of the RKISP1\n> pipeline handler.\n> \n>> Currently createIPA() / IPAManager::module() assume that there is a 1:1\n>> relationship between pipeline handlers and IPAs and IPA matching is done\n>> based on matching the pipe to ipaModuleInfo.pipelineName[].\n>>\n>> One way to allow using a single IPA with multiple pipelines would be to\n>> allow the IPA to declare itself compatible with more then one pipeline,\n>> turning ipaModuleInfo.pipelineName[] into e.g. a vector. But the way\n>> ipaModuleInfo is loaded as an ELF symbol requires it to be a simple flat\n>> C-struct.\n>>\n>> Instead add an optional ipaName argument to createIPA() which when set\n>> switches things over to matching ipaModuleInfo.name[] allowing pipelines\n>> to request a specific shared IPA module this way.\n> \n> I think that's reasonable. The Pipeline handler knows more about the\n> system than the IPA ... so it's more reasonable for the PH to say \"I can\n> use this\" rather than the IPA to say \"I support X PipelineHandlers\"\n> \n>> Signed-off-by: Hans de Goede \n>> ---\n>> include/libcamera/internal/ipa_manager.h | 7 ++++---\n>> include/libcamera/internal/ipa_module.h | 4 ++--\n>> src/libcamera/ipa_manager.cpp | 6 ++++--\n>> src/libcamera/ipa_module.cpp | 19 +++++++++++++------\n>> 4 files changed, 23 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n>> index a0d448cf..af784c9c 100644\n>> --- a/include/libcamera/internal/ipa_manager.h\n>> +++ b/include/libcamera/internal/ipa_manager.h\n>> @@ -34,11 +34,12 @@ public:\n>> template\n>> static std::unique_ptr createIPA(PipelineHandler *pipe,\n>> uint32_t minVersion,\n>> - uint32_t maxVersion)\n>> + uint32_t maxVersion,\n>> + const char *ipaName = NULL)\n>> {\n>> CameraManager *cm = pipe->cameraManager();\n>> IPAManager *self = cm->_d()->ipaManager();\n>> - IPAModule *m = self->module(pipe, minVersion, maxVersion);\n>> + IPAModule *m = self->module(pipe, minVersion, maxVersion, ipaName);\n>> if (!m)\n>> return nullptr;\n>> \n>> @@ -64,7 +65,7 @@ private:\n>> unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n>> \n>> IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n>> - uint32_t maxVersion);\n>> + uint32_t maxVersion, const char *ipaName);\n>> \n>> bool isSignatureValid(IPAModule *ipa) const;\n>> \n>> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h\n>> index 15f19492..e7b00fdb 100644\n>> --- a/include/libcamera/internal/ipa_module.h\n>> +++ b/include/libcamera/internal/ipa_module.h\n>> @@ -36,8 +36,8 @@ public:\n>> \n>> IPAInterface *createInterface();\n>> \n>> - bool match(PipelineHandler *pipe,\n>> - uint32_t minVersion, uint32_t maxVersion) const;\n>> + bool match(PipelineHandler *pipe, uint32_t minVersion,\n>> + uint32_t maxVersion, const char *ipaName = NULL) const;\n>> \n>> protected:\n>> std::string logPrefix() const override;\n>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n>> index 830750dc..2ef8b98e 100644\n>> --- a/src/libcamera/ipa_manager.cpp\n>> +++ b/src/libcamera/ipa_manager.cpp\n>> @@ -240,12 +240,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n>> * \\param[in] pipe The pipeline handler\n>> * \\param[in] minVersion Minimum acceptable version of IPA module\n>> * \\param[in] maxVersion Maximum acceptable version of IPA module\n>> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> \n> I wondered about saying \"Defaults to the pipeline handler name\" ... but\n> either way.\n\nThat is not true/correct though, after this patch there are 2 different\nmatching methods using 2 different fields of ipaModuleInfo:\n\n1. ipaName==null, match ipaModuleInfo.pipelineName[] against pipe->name()\n2. ipaName!=null, match ipaModuleInfo.name[] against ipaName\n\nSo claiming that ipaName defaults to pipe->name() when not set is\nincorrect, when ipaName is not set, ipaName and ipaModuleInfo.name[]\nare both not used at all.\n\nMaybe the help text for IPAModule::match() needs to be updated a bit\nmore in this patch to make this more explicit ?\n\nRegards,\n\nHans\n\n\n\n\n\n\n\n>> */\n>> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n>> - uint32_t maxVersion)\n>> + uint32_t maxVersion, const char *ipaName)\n>> {\n>> for (const auto &module : modules_) {\n>> - if (module->match(pipe, minVersion, maxVersion))\n>> + if (module->match(pipe, minVersion, maxVersion, ipaName))\n>> return module.get();\n>> }\n>> \n>> @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n>> * \\param[in] pipe The pipeline handler that wants a matching IPA proxy\n>> * \\param[in] minVersion Minimum acceptable version of IPA module\n>> * \\param[in] maxVersion Maximum acceptable version of IPA module\n>> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n>> *\n>> * \\return A newly created IPA proxy, or nullptr if no matching IPA module is\n>> * found or if the IPA proxy fails to initialize\n>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n>> index e6ea61e4..b7004b1c 100644\n>> --- a/src/libcamera/ipa_module.cpp\n>> +++ b/src/libcamera/ipa_module.cpp\n>> @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface()\n>> * \\param[in] pipe Pipeline handler to match with\n>> * \\param[in] minVersion Minimum acceptable version of IPA module\n>> * \\param[in] maxVersion Maximum acceptable version of IPA module\n>> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n>> *\n>> * This function checks if this IPA module matches the \\a pipe pipeline handler,\n>> - * and the input version range.\n>> + * and the input version range. If \\a ipaName is non-null then the IPA module\n>> + * name is matched against \\a ipaName instead of matching \\a pipe.\n>> *\n>> * \\return True if the pipeline handler matches the IPA module, or false otherwise\n>> */\n>> -bool IPAModule::match(PipelineHandler *pipe,\n>> - uint32_t minVersion, uint32_t maxVersion) const\n>> +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion,\n>> + uint32_t maxVersion, const char *ipaName) const\n>> {\n>> - return info_.pipelineVersion >= minVersion &&\n>> - info_.pipelineVersion <= maxVersion &&\n>> - !strcmp(info_.pipelineName, pipe->name());\n>> + if (info_.pipelineVersion < minVersion ||\n>> + info_.pipelineVersion > maxVersion)\n>> + return false;\n>> +\n>> + if (ipaName)\n>> + return !strcmp(info_.name, ipaName);\n>> + else\n>> + return !strcmp(info_.pipelineName, pipe->name());\n> \n> The code looks fine so:\n> \n> Reviewed-by: Kieran Bingham \n> \n>> }\n>> \n>> std::string IPAModule::logPrefix() const\n>> -- \n>> 2.49.0\n>>\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 082A1C3226\n\tfor ;\n\tSun, 11 May 2025 13:49:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A6ED68B63;\n\tSun, 11 May 2025 15:49:22 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D73D868B51\n\tfor ;\n\tSun, 11 May 2025 15:49:20 +0200 (CEST)","from mail-ed1-f69.google.com (mail-ed1-f69.google.com\n\t[209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-283-_kdP086AMSah2w13bxnSmA-1; Sun, 11 May 2025 09:49:17 -0400","by mail-ed1-f69.google.com with SMTP id\n\t4fb4d7f45d1cf-5fc812d9cf8so3485520a12.2\n\tfor ;\n\tSun, 11 May 2025 06:49:17 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5fc9d70ec1esm4269195a12.78.2025.05.11.06.49.14\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 11 May 2025 06:49:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"DxmFmSI3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1746971359;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=9CXEgxdG3f/EX7vFLwei+JcZ6v6bMzIvVULFNo3N/6Q=;\n\tb=DxmFmSI3Pg68JD7wdQzrUyqeHvaSNWHKxOYghy0M4qbBu91Uqvv/F3ic7Fv2QJtiRTon9u\n\tdreUlctTveO/6UKpwAINLkHe6yL5+2S3EKwZzjFjMe+9x6z95TZ6Ai/5S3z4YqQq/8YT8v\n\t+Nc9HdVkZtM7WmPTDGKQ3W1/mM+dVrc=","X-MC-Unique":"_kdP086AMSah2w13bxnSmA-1","X-Mimecast-MFC-AGG-ID":"_kdP086AMSah2w13bxnSmA_1746971356","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1746971356; x=1747576156;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=9CXEgxdG3f/EX7vFLwei+JcZ6v6bMzIvVULFNo3N/6Q=;\n\tb=qdyci6dfc+I+M5rJdFae9leaza/a5yeIFfVLnDOtBYKc8gkJHgOutM6aoZTfFs1mtw\n\tvXUQcMp03RTz73XZT/fqqgP/cTVMKnBFdGE5U2JkjRI2YdNXcYAZgJgIGuRvGCl2rjYe\n\tcpcpeCMuEdGHIqwf8YkTyOQnoCW9/LUtYrSL38o0w3FmxCoSuWF05n6BVLtORPjmN21k\n\ternugVmz6vpz+JNaMQ67dCkysqjyC+FlOX5FzMCpLI8XtzJz0AQZnGy35W0+7BEGLxh8\n\tk7ERs0PCdZv0fwyiW+Qu1tQRvtNTOfgA9ieMn95+WDpshN1TZPugrSaI8h+k88u5WOA3\n\t+GlQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUPzKabCMsxEG9g8fITBvHYsX3FjiCZW/l6QFxHmGii8TrZHhLjgvlv7HsJn70uWZCHVbc7Ps+6Y8npynj6+18=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yx1z0g0BF2N5GDSF41sgoHUzrMrpbfnTfNgMulmzZmD/tXJex4O\n\tjKU5qzdx86+jf9lLxwQHj/93RGmG6aB7Yfan1YB0JgxFkYAZlUvegawdvQUbAdp+AmaBNDGqMtY\n\tUV8MvAMSH4lUeLvqb5aZghKsZhfQRdKfye+933nVwOXz6Po1P9ilNN+4CZ4OKes8SHLB/Ylo=","X-Gm-Gg":"ASbGncsQ8xBam+lTyn+IFPIGBxqmCS/JLxiTMiw5a86uAK2igukiJNLKzp2XtvhQmMR\n\tq9StVWTc+Ya+a0w/jYGnBv9pFTlIfhpeEJwEnZwW3vW9zecSch5h3egNXP3fLp6f6Zb6d7MDBLV\n\tBA69nB3k0RD+/VePHwTslhMyefDGebCFRG3/6CqKzCrxY4W8Yo5HOKqwbn3oLD5tOPeNr9mUxfr\n\t/P+JEZkBM1KaUrFpNzKEnR82rArHGlbZwqDPquLgX/rPZaiQXMXt/RWzSsknFWPUOaZB59syEFY\n\tT9F6yirlJ9w7j9mK7QmG2b8Z6Tdcl//MD3PkGhm7Rj9Qas3E24jqR/O4xLrWt5D3PTui6rswCcg\n\tNsmKkWYrEPD/Dm2GUe75qW5DGGnnPu8/RJw7kZe7WDOaDOzwa0Sp+3EWSSBpbmQ==","X-Received":["by 2002:a05:6402:1ecb:b0:5fc:bb1d:71e4 with SMTP id\n\t4fb4d7f45d1cf-5fcbb1d76efmr5198804a12.6.1746971355994; \n\tSun, 11 May 2025 06:49:15 -0700 (PDT)","by 2002:a05:6402:1ecb:b0:5fc:bb1d:71e4 with SMTP id\n\t4fb4d7f45d1cf-5fcbb1d76efmr5198774a12.6.1746971355434; \n\tSun, 11 May 2025 06:49:15 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFpUAkBurIYneruzgF9IpBMoI10udDaMuD+tctIWmnjDxlRLJ63e9y6wkPxm8zinZYjYi11LA==","Message-ID":"<1101a528-0620-4165-a94b-c30d556de72e@redhat.com>","Date":"Sun, 11 May 2025 15:49:13 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ","References":"<20250510141220.54872-1-hdegoede@redhat.com>\n\t<20250510141220.54872-8-hdegoede@redhat.com>\n\t<174696016162.4078945.4469923532661672470@ping.linuxembedded.co.uk>","From":"Hans de Goede ","In-Reply-To":"<174696016162.4078945.4469923532661672470@ping.linuxembedded.co.uk>","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"0-0xi2I_ZCYE5VV9-DQX_9QGyFVO15yHy_hHE4SBT_E_1746971356","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34182,"web_url":"https://patchwork.libcamera.org/comment/34182/","msgid":"<174697627558.4078945.6126221948320130247@ping.linuxembedded.co.uk>","date":"2025-05-11T15:11:15","subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2025-05-11 14:49:13)\n> Hi Kieran,\n> \n> Thank you for the review.\n> \n> On 11-May-25 12:42 PM, Kieran Bingham wrote:\n> > Quoting Hans de Goede (2025-05-10 15:12:19)\n> >> Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for\n> >> more then 1 pipeline handler.\n> > \n> > I can see this being useful in other pipelines too - as I think we can\n> > anticipate multiple pipelines that can share a common IPA (certainly the\n> > SoftIPA/GPU-IPA) but also potentially other derivatives of the RKISP1\n> > pipeline handler.\n> > \n> >> Currently createIPA() / IPAManager::module() assume that there is a 1:1\n> >> relationship between pipeline handlers and IPAs and IPA matching is done\n> >> based on matching the pipe to ipaModuleInfo.pipelineName[].\n> >>\n> >> One way to allow using a single IPA with multiple pipelines would be to\n> >> allow the IPA to declare itself compatible with more then one pipeline,\n> >> turning ipaModuleInfo.pipelineName[] into e.g. a vector. But the way\n> >> ipaModuleInfo is loaded as an ELF symbol requires it to be a simple flat\n> >> C-struct.\n> >>\n> >> Instead add an optional ipaName argument to createIPA() which when set\n> >> switches things over to matching ipaModuleInfo.name[] allowing pipelines\n> >> to request a specific shared IPA module this way.\n> > \n> > I think that's reasonable. The Pipeline handler knows more about the\n> > system than the IPA ... so it's more reasonable for the PH to say \"I can\n> > use this\" rather than the IPA to say \"I support X PipelineHandlers\"\n> > \n> >> Signed-off-by: Hans de Goede \n> >> ---\n> >> include/libcamera/internal/ipa_manager.h | 7 ++++---\n> >> include/libcamera/internal/ipa_module.h | 4 ++--\n> >> src/libcamera/ipa_manager.cpp | 6 ++++--\n> >> src/libcamera/ipa_module.cpp | 19 +++++++++++++------\n> >> 4 files changed, 23 insertions(+), 13 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> >> index a0d448cf..af784c9c 100644\n> >> --- a/include/libcamera/internal/ipa_manager.h\n> >> +++ b/include/libcamera/internal/ipa_manager.h\n> >> @@ -34,11 +34,12 @@ public:\n> >> template\n> >> static std::unique_ptr createIPA(PipelineHandler *pipe,\n> >> uint32_t minVersion,\n> >> - uint32_t maxVersion)\n> >> + uint32_t maxVersion,\n> >> + const char *ipaName = NULL)\n> >> {\n> >> CameraManager *cm = pipe->cameraManager();\n> >> IPAManager *self = cm->_d()->ipaManager();\n> >> - IPAModule *m = self->module(pipe, minVersion, maxVersion);\n> >> + IPAModule *m = self->module(pipe, minVersion, maxVersion, ipaName);\n> >> if (!m)\n> >> return nullptr;\n> >> \n> >> @@ -64,7 +65,7 @@ private:\n> >> unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n> >> \n> >> IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n> >> - uint32_t maxVersion);\n> >> + uint32_t maxVersion, const char *ipaName);\n> >> \n> >> bool isSignatureValid(IPAModule *ipa) const;\n> >> \n> >> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h\n> >> index 15f19492..e7b00fdb 100644\n> >> --- a/include/libcamera/internal/ipa_module.h\n> >> +++ b/include/libcamera/internal/ipa_module.h\n> >> @@ -36,8 +36,8 @@ public:\n> >> \n> >> IPAInterface *createInterface();\n> >> \n> >> - bool match(PipelineHandler *pipe,\n> >> - uint32_t minVersion, uint32_t maxVersion) const;\n> >> + bool match(PipelineHandler *pipe, uint32_t minVersion,\n> >> + uint32_t maxVersion, const char *ipaName = NULL) const;\n> >> \n> >> protected:\n> >> std::string logPrefix() const override;\n> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> >> index 830750dc..2ef8b98e 100644\n> >> --- a/src/libcamera/ipa_manager.cpp\n> >> +++ b/src/libcamera/ipa_manager.cpp\n> >> @@ -240,12 +240,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> >> * \\param[in] pipe The pipeline handler\n> >> * \\param[in] minVersion Minimum acceptable version of IPA module\n> >> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> >> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> > \n> > I wondered about saying \"Defaults to the pipeline handler name\" ... but\n> > either way.\n> \n> That is not true/correct though, after this patch there are 2 different\n> matching methods using 2 different fields of ipaModuleInfo:\n> \n> 1. ipaName==null, match ipaModuleInfo.pipelineName[] against pipe->name()\n> 2. ipaName!=null, match ipaModuleInfo.name[] against ipaName\n> \n> So claiming that ipaName defaults to pipe->name() when not set is\n> incorrect, when ipaName is not set, ipaName and ipaModuleInfo.name[]\n> are both not used at all.\n> \n> Maybe the help text for IPAModule::match() needs to be updated a bit\n> more in this patch to make this more explicit ?\n\nOhh - I hadn't realised that extra bit there - so maybe the text here\ncould reference that - but I'm fine with what you have already too.\n\n\n--\nKieran","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 60DE7C3200\n\tfor ;\n\tSun, 11 May 2025 15:11:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 159D468B5D;\n\tSun, 11 May 2025 17:11:20 +0200 (CEST)","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 196AE68B51\n\tfor ;\n\tSun, 11 May 2025 17:11:18 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC133B63;\n\tSun, 11 May 2025 17:11:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Wojq2IsB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746976263;\n\tbh=yfYKCA4jnt9qOvsv+45u0ZtYDG51g4zS0dxnsheBIn0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Wojq2IsBmwj7AQzQsHTcoNWD13WcOOv7YDXTfq668gDMjlsEMcBw/ihLtKUlE72r+\n\tsYWx+EtnsFq5cHlvyIWTsptwStQRsNL/0hIggiqgXBAyH93PFQUAbYneO2doFx14m8\n\tSYQZbRm1qBpzgOE3xJCemhLMhqoB1AMDRrkIPlW4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<1101a528-0620-4165-a94b-c30d556de72e@redhat.com>","References":"<20250510141220.54872-1-hdegoede@redhat.com>\n\t<20250510141220.54872-8-hdegoede@redhat.com>\n\t<174696016162.4078945.4469923532661672470@ping.linuxembedded.co.uk>\n\t<1101a528-0620-4165-a94b-c30d556de72e@redhat.com>","Subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","From":"Kieran Bingham ","Cc":"Milan Zamazal ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Sun, 11 May 2025 16:11:15 +0100","Message-ID":"<174697627558.4078945.6126221948320130247@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34224,"web_url":"https://patchwork.libcamera.org/comment/34224/","msgid":"<8534d8ip86.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-05-13T14:01:45","subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow\n\tmatching by IPA name instead of by pipeline","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Hans,\n\nHans de Goede writes:\n\n> Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for\n> more then 1 pipeline handler.\n\ns/then/than/\n\n> Currently createIPA() / IPAManager::module() assume that there is a 1:1\n> relationship between pipeline handlers and IPAs and IPA matching is done\n> based on matching the pipe to ipaModuleInfo.pipelineName[].\n>\n> One way to allow using a single IPA with multiple pipelines would be to\n> allow the IPA to declare itself compatible with more then one pipeline,\n\ns/then/than/\n\n> turning ipaModuleInfo.pipelineName[] into e.g. a vector. But the way\n> ipaModuleInfo is loaded as an ELF symbol requires it to be a simple flat\n> C-struct.\n>\n> Instead add an optional ipaName argument to createIPA() which when set\n> switches things over to matching ipaModuleInfo.name[] allowing pipelines\n> to request a specific shared IPA module this way.\n>\n> Signed-off-by: Hans de Goede \n> ---\n> include/libcamera/internal/ipa_manager.h | 7 ++++---\n> include/libcamera/internal/ipa_module.h | 4 ++--\n> src/libcamera/ipa_manager.cpp | 6 ++++--\n> src/libcamera/ipa_module.cpp | 19 +++++++++++++------\n> 4 files changed, 23 insertions(+), 13 deletions(-)\n>\n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index a0d448cf..af784c9c 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -34,11 +34,12 @@ public:\n> \ttemplate\n> \tstatic std::unique_ptr createIPA(PipelineHandler *pipe,\n> \t\t\t\t\t uint32_t minVersion,\n> -\t\t\t\t\t uint32_t maxVersion)\n> +\t\t\t\t\t uint32_t maxVersion,\n> +\t\t\t\t\t const char *ipaName = NULL)\n\ns/NULL/nullptr/\n\n> \t{\n> \t\tCameraManager *cm = pipe->cameraManager();\n> \t\tIPAManager *self = cm->_d()->ipaManager();\n> -\t\tIPAModule *m = self->module(pipe, minVersion, maxVersion);\n> +\t\tIPAModule *m = self->module(pipe, minVersion, maxVersion, ipaName);\n> \t\tif (!m)\n> \t\t\treturn nullptr;\n> \n> @@ -64,7 +65,7 @@ private:\n> \tunsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n> \n> \tIPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n> -\t\t\t uint32_t maxVersion);\n> +\t\t\t uint32_t maxVersion, const char *ipaName);\n\nWouldn't it be nicer to introduce\n\n IPAModule *module(const char *ipaName, uint32_t minVersion, uint32_t maxVersion);\n\nand the same for IPAModule::match? It's more lines of code but less\nconfusion.\n\n> \tbool isSignatureValid(IPAModule *ipa) const;\n> \n> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h\n> index 15f19492..e7b00fdb 100644\n> --- a/include/libcamera/internal/ipa_module.h\n> +++ b/include/libcamera/internal/ipa_module.h\n> @@ -36,8 +36,8 @@ public:\n> \n> \tIPAInterface *createInterface();\n> \n> -\tbool match(PipelineHandler *pipe,\n> -\t\t uint32_t minVersion, uint32_t maxVersion) const;\n> +\tbool match(PipelineHandler *pipe, uint32_t minVersion,\n> +\t\t uint32_t maxVersion, const char *ipaName = NULL) const;\n\ns/NULL/nullptr/\n\n> protected:\n> \tstd::string logPrefix() const override;\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 830750dc..2ef8b98e 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -240,12 +240,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> * \\param[in] pipe The pipeline handler\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> */\n> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n> -\t\t\t uint32_t maxVersion)\n> +\t\t\t uint32_t maxVersion, const char *ipaName)\n> {\n> \tfor (const auto &module : modules_) {\n> -\t\tif (module->match(pipe, minVersion, maxVersion))\n> +\t\tif (module->match(pipe, minVersion, maxVersion, ipaName))\n> \t\t\treturn module.get();\n> \t}\n> \n> @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n> * \\param[in] pipe The pipeline handler that wants a matching IPA proxy\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> *\n> * \\return A newly created IPA proxy, or nullptr if no matching IPA module is\n> * found or if the IPA proxy fails to initialize\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index e6ea61e4..b7004b1c 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface()\n> * \\param[in] pipe Pipeline handler to match with\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> *\n> * This function checks if this IPA module matches the \\a pipe pipeline handler,\n> - * and the input version range.\n> + * and the input version range. If \\a ipaName is non-null then the IPA module\n> + * name is matched against \\a ipaName instead of matching \\a pipe.\n> *\n> * \\return True if the pipeline handler matches the IPA module, or false otherwise\n> */\n> -bool IPAModule::match(PipelineHandler *pipe,\n> -\t\t uint32_t minVersion, uint32_t maxVersion) const\n> +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion,\n> +\t\t uint32_t maxVersion, const char *ipaName) const\n> {\n> -\treturn info_.pipelineVersion >= minVersion &&\n> -\t info_.pipelineVersion <= maxVersion &&\n> -\t !strcmp(info_.pipelineName, pipe->name());\n> +\tif (info_.pipelineVersion < minVersion ||\n> +\t info_.pipelineVersion > maxVersion)\n> +\t\treturn false;\n> +\n> +\tif (ipaName)\n> +\t\treturn !strcmp(info_.name, ipaName);\n> +\telse\n> +\t\treturn !strcmp(info_.pipelineName, pipe->name());\n> }\n> \n> std::string IPAModule::logPrefix() const","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 A83C2C3200\n\tfor ;\n\tTue, 13 May 2025 14:01:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B3A568B61;\n\tTue, 13 May 2025 16:01:52 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C39468B40\n\tfor ;\n\tTue, 13 May 2025 16:01:51 +0200 (CEST)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-588-Q3nbXurdP2qrbN_B6-0JLQ-1; Tue, 13 May 2025 10:01:49 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-442d472cf84so26325125e9.2\n\tfor ;\n\tTue, 13 May 2025 07:01:48 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-442cd3b7d2bsm214533185e9.36.2025.05.13.07.01.45\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 13 May 2025 07:01:45 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"XO9TsowV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1747144910;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=xn5A9QrjvXrRzulv/FudNoe3emu5pKLEQKn+ePJYir0=;\n\tb=XO9TsowVD+oAQzKZ8T/AoEspUHwyvTJ8Exl2NxkwYWSarO5kYcq6lE8rYxfirdTfG5vQw1\n\tDlJtuo/2w65FJ/hLAbozpQsHHxpnK4T1QL/fJMW0g7JJYL0ZxVv35Zu/VZjNJ/HJNUYATR\n\t2Dj0zFuZjUtQxNnUgclRJr/lorbN2P0=","X-MC-Unique":"Q3nbXurdP2qrbN_B6-0JLQ-1","X-Mimecast-MFC-AGG-ID":"Q3nbXurdP2qrbN_B6-0JLQ_1747144908","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1747144907; x=1747749707;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=xn5A9QrjvXrRzulv/FudNoe3emu5pKLEQKn+ePJYir0=;\n\tb=DrjPbFipppH+iF6xmCSi4Jo1t6O2GC+12x7hHuEdTZYsOUjmlsypqUJEph7QfJju3S\n\tAbziI87VcW4NKKur2BTS5bt2Txm5VKBRbUdg6jThhAkZTgvF4vY1TZc7JfzIzucYG3kL\n\tDsETVvpkJOym0wcLOWv+QTPFYHPUncQ4qRlvgYR8hDE3OtLq0CZYlMFFkR3H93ETkl7x\n\toRx3RGQVSVc0mzLz77CmuTmjSJvfIFiOFAJaldznoOoL5KpRIiiXNCuocLLNYr0CcGew\n\t9ITlmxUQamViW1gzIFKtp3cloNMIwP+BMyVGLSh9uNDE1Mtd1Zwad1UfYXjGS5L7dV+g\n\tzqYA==","X-Gm-Message-State":"AOJu0Yy2u095FIpUSpNV+WQpVwAmANL6gD/bqw9T2teLcxqSDksDhD9r\n\tWgF4YjuVsFpfSNDdkgRmvvsfesCHDVvpXQGi3ujieNNt1rS2AJCtHyi4g3t77aXXJdOoxGfcxwG\n\t0n083AHPflc/EWOJyS/+PBHDwQ5DlQf2pH9UjzkVkBmlu7hM5AB9WVsnzn9mzIAKr+Y+/dBpyjT\n\tI5vj4V6sBeouIqDj4AQ0FUe6LeE5NlNf5smOJqpiR6m+vh3ITWMsRiuwU=","X-Gm-Gg":"ASbGnctibtAtyAordnQZK2o7xRiEvYgsfrW6Doktp00NWJEXM766ed0NZuzOdGgvhWM\n\twNM8VW1eJZ6MSQoMPl3/dHGXDXwospBqkh0J7uTX752WkFhQK+lYgncwIeaACM45S1pX9K3G/G8\n\t6PjL1AWccA9mzzKVG3hF9Ja3Uf4XzP+Yi1YRJRa0FyfnMjQdyQcDkHKkCU7g3HTcBE8qAL5aWVx\n\tMqtPIbDuQFDKmLVTFm13qxHxcHw1BHBGIdjwGm5pZS43cylhORkzLy0WjAgl5sAEAJNMReCIpkf\n\tomUjE+2iR0gTpJlLbKQ7lws5VgXxoHsvCcQS","X-Received":["by 2002:a05:600c:c0d2:20b0:43c:fa3f:8e5d with SMTP id\n\t5b1f17b1804b1-442d77fa124mr91852015e9.2.1747144907168; \n\tTue, 13 May 2025 07:01:47 -0700 (PDT)","by 2002:a05:600c:c0d2:20b0:43c:fa3f:8e5d with SMTP id\n\t5b1f17b1804b1-442d77fa124mr91851235e9.2.1747144906184; \n\tTue, 13 May 2025 07:01:46 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IG2zRKH2PbWIO3qr7vvghpID39mDYazaQsP0fs0pC2arBc8T3BXPYTn/KsY9jxIv7jiiRB5lw==","From":"Milan Zamazal ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow\n\tmatching by IPA name instead of by pipeline","In-Reply-To":"<20250510141220.54872-8-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Sat, 10 May 2025 16:12:19 +0200\")","References":"<20250510141220.54872-1-hdegoede@redhat.com>\n\t<20250510141220.54872-8-hdegoede@redhat.com>","Date":"Tue, 13 May 2025 16:01:45 +0200","Message-ID":"<8534d8ip86.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"lLC_KEk09gMSKEgMf2L2-5cnFiCnGDk0VtR8d8cJzhc_1747144908","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34231,"web_url":"https://patchwork.libcamera.org/comment/34231/","msgid":"<55b5b6fe-256d-4f34-97d9-5a93de5a38ad@ideasonboard.com>","date":"2025-05-13T16:50:10","subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 05. 11. 12:42 keltezéssel, Kieran Bingham írta:\n> Quoting Hans de Goede (2025-05-10 15:12:19)\n>> Parts of the software ISP and the ipa_soft_simple.so IPA may be useful for\n>> more then 1 pipeline handler.\n> \n> I can see this being useful in other pipelines too - as I think we can\n> anticipate multiple pipelines that can share a common IPA (certainly the\n> SoftIPA/GPU-IPA) but also potentially other derivatives of the RKISP1\n> pipeline handler.\n> \n>> Currently createIPA() / IPAManager::module() assume that there is a 1:1\n>> relationship between pipeline handlers and IPAs and IPA matching is done\n>> based on matching the pipe to ipaModuleInfo.pipelineName[].\n>>\n>> One way to allow using a single IPA with multiple pipelines would be to\n>> allow the IPA to declare itself compatible with more then one pipeline,\n>> turning ipaModuleInfo.pipelineName[] into e.g. a vector. But the way\n>> ipaModuleInfo is loaded as an ELF symbol requires it to be a simple flat\n>> C-struct.\n>>\n>> Instead add an optional ipaName argument to createIPA() which when set\n>> switches things over to matching ipaModuleInfo.name[] allowing pipelines\n>> to request a specific shared IPA module this way.\n> \n> I think that's reasonable. The Pipeline handler knows more about the\n> system than the IPA ... so it's more reasonable for the PH to say \"I can\n> use this\" rather than the IPA to say \"I support X PipelineHandlers\"\n\nMaybe we can go a step further and explicitly require the IPA name\nfrom pipeline handlers? And remove `IPAModuleInfo::pipelineName`?\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>> Signed-off-by: Hans de Goede \n>> ---\n>> include/libcamera/internal/ipa_manager.h | 7 ++++---\n>> include/libcamera/internal/ipa_module.h | 4 ++--\n>> src/libcamera/ipa_manager.cpp | 6 ++++--\n>> src/libcamera/ipa_module.cpp | 19 +++++++++++++------\n>> 4 files changed, 23 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n>> index a0d448cf..af784c9c 100644\n>> --- a/include/libcamera/internal/ipa_manager.h\n>> +++ b/include/libcamera/internal/ipa_manager.h\n>> @@ -34,11 +34,12 @@ public:\n>> template\n>> static std::unique_ptr createIPA(PipelineHandler *pipe,\n>> uint32_t minVersion,\n>> - uint32_t maxVersion)\n>> + uint32_t maxVersion,\n>> + const char *ipaName = NULL)\n>> {\n>> CameraManager *cm = pipe->cameraManager();\n>> IPAManager *self = cm->_d()->ipaManager();\n>> - IPAModule *m = self->module(pipe, minVersion, maxVersion);\n>> + IPAModule *m = self->module(pipe, minVersion, maxVersion, ipaName);\n>> if (!m)\n>> return nullptr;\n>> \n>> @@ -64,7 +65,7 @@ private:\n>> unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n>> \n>> IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n>> - uint32_t maxVersion);\n>> + uint32_t maxVersion, const char *ipaName);\n>> \n>> bool isSignatureValid(IPAModule *ipa) const;\n>> \n>> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h\n>> index 15f19492..e7b00fdb 100644\n>> --- a/include/libcamera/internal/ipa_module.h\n>> +++ b/include/libcamera/internal/ipa_module.h\n>> @@ -36,8 +36,8 @@ public:\n>> \n>> IPAInterface *createInterface();\n>> \n>> - bool match(PipelineHandler *pipe,\n>> - uint32_t minVersion, uint32_t maxVersion) const;\n>> + bool match(PipelineHandler *pipe, uint32_t minVersion,\n>> + uint32_t maxVersion, const char *ipaName = NULL) const;\n>> \n>> protected:\n>> std::string logPrefix() const override;\n>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n>> index 830750dc..2ef8b98e 100644\n>> --- a/src/libcamera/ipa_manager.cpp\n>> +++ b/src/libcamera/ipa_manager.cpp\n>> @@ -240,12 +240,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n>> * \\param[in] pipe The pipeline handler\n>> * \\param[in] minVersion Minimum acceptable version of IPA module\n>> * \\param[in] maxVersion Maximum acceptable version of IPA module\n>> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n> \n> I wondered about saying \"Defaults to the pipeline handler name\" ... but\n> either way.\n> \n>> */\n>> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n>> - uint32_t maxVersion)\n>> + uint32_t maxVersion, const char *ipaName)\n>> {\n>> for (const auto &module : modules_) {\n>> - if (module->match(pipe, minVersion, maxVersion))\n>> + if (module->match(pipe, minVersion, maxVersion, ipaName))\n>> return module.get();\n>> }\n>> \n>> @@ -258,6 +259,7 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n>> * \\param[in] pipe The pipeline handler that wants a matching IPA proxy\n>> * \\param[in] minVersion Minimum acceptable version of IPA module\n>> * \\param[in] maxVersion Maximum acceptable version of IPA module\n>> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n>> *\n>> * \\return A newly created IPA proxy, or nullptr if no matching IPA module is\n>> * found or if the IPA proxy fails to initialize\n>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n>> index e6ea61e4..b7004b1c 100644\n>> --- a/src/libcamera/ipa_module.cpp\n>> +++ b/src/libcamera/ipa_module.cpp\n>> @@ -466,18 +466,25 @@ IPAInterface *IPAModule::createInterface()\n>> * \\param[in] pipe Pipeline handler to match with\n>> * \\param[in] minVersion Minimum acceptable version of IPA module\n>> * \\param[in] maxVersion Maximum acceptable version of IPA module\n>> + * \\param[in] ipaName If set match IPA module by this name instead of by pipe\n>> *\n>> * This function checks if this IPA module matches the \\a pipe pipeline handler,\n>> - * and the input version range.\n>> + * and the input version range. If \\a ipaName is non-null then the IPA module\n>> + * name is matched against \\a ipaName instead of matching \\a pipe.\n>> *\n>> * \\return True if the pipeline handler matches the IPA module, or false otherwise\n>> */\n>> -bool IPAModule::match(PipelineHandler *pipe,\n>> - uint32_t minVersion, uint32_t maxVersion) const\n>> +bool IPAModule::match(PipelineHandler *pipe, uint32_t minVersion,\n>> + uint32_t maxVersion, const char *ipaName) const\n>> {\n>> - return info_.pipelineVersion >= minVersion &&\n>> - info_.pipelineVersion <= maxVersion &&\n>> - !strcmp(info_.pipelineName, pipe->name());\n>> + if (info_.pipelineVersion < minVersion ||\n>> + info_.pipelineVersion > maxVersion)\n>> + return false;\n>> +\n>> + if (ipaName)\n>> + return !strcmp(info_.name, ipaName);\n>> + else\n>> + return !strcmp(info_.pipelineName, pipe->name());\n> \n> The code looks fine so:\n> \n> Reviewed-by: Kieran Bingham \n> \n>> }\n>> \n>> std::string IPAModule::logPrefix() const\n>> -- \n>> 2.49.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 DC263C3220\n\tfor ;\n\tTue, 13 May 2025 16:50:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D332368B61;\n\tTue, 13 May 2025 18:50:16 +0200 (CEST)","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 A158F68B40\n\tfor ;\n\tTue, 13 May 2025 18:50:14 +0200 (CEST)","from [192.168.33.2] (185.221.142.248.nat.pool.zt.hu\n\t[185.221.142.248])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 72B187E4;\n\tTue, 13 May 2025 18:49:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lH4bhHYt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747154998;\n\tbh=nksvLYkFV2E4pksI54Uacoq4puPpuki4Ni+MZAFHL6U=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=lH4bhHYtY8LW63tFg6QApMqwx+pnx+ECKRYId2ma54hnwfcxaLxR88OVMKR7G2GxT\n\tqQXTRK5mkKNQ/g4UmlZgxqC9E/t2ZQKh32sDBoYjGa3+lmz1d19Qhz/OeNstTddoUg\n\taaVqJN/3Xe2JcikFoj3PrleUJG+7zdsBHncTrDHY=","Message-ID":"<55b5b6fe-256d-4f34-97d9-5a93de5a38ad@ideasonboard.com>","Date":"Tue, 13 May 2025 18:50:10 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 7/8] libcamera: ipa_manager: createIPA: Allow matching\n\tby IPA name instead of by pipeline","To":"Kieran Bingham ,\n\tHans de Goede , libcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ","References":"<20250510141220.54872-1-hdegoede@redhat.com>\n\t<20250510141220.54872-8-hdegoede@redhat.com>\n\t<174696016162.4078945.4469923532661672470@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174696016162.4078945.4469923532661672470@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]