[{"id":32017,"web_url":"https://patchwork.libcamera.org/comment/32017/","msgid":"<87plnavmmn.fsf@redhat.com>","date":"2024-11-04T19:12:48","subject":"Re: [PATCH 7/8] libcamera: ipa_manager: createIPA: Allow passing an\n\tIPA name to match","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>\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 pipe->name().\n>\n> Add an optional ipaName argument to createIPA() which when set overrides\n> pipe->name(), allowing pipeline handlers to request a different IPA.\n\nThis deserves a bit more explanation. Looking at the followup patch,\nthe purpose seems to be to reuse a given IPA completely in a different\npipeline. Now the question would be, does the IPA belong to the\noriginal pipeline or to the new one, why should it be named after one or\nthe other? And what if some pipelines would like to reuse only parts of\nsoftware ISP and use different algorithms from its own IPA (which is not\nunlikely to happen in foreseeable future)? Is it about whole IPA\nsharing or more about sharing algorithm implementations? Does it matter\nwhether software ISP runs on CPU or GPU and can the new pipeline get\ndisturbed by contingent changes in the shared IPA?\n\nI understand the solution in this patch is the easiest one for the\npurpose of atomisp. I'm not sure whether the maintainers will like it;\nif they do I have nothing against it. But I think it's worth to think a\nbit about other possible ways of sharing pieces of software ISP among\npipelines. Maybe the conclusion would be it'd be nice and possible but\ntoo complicated at the moment -- then fine but let's know how and why.\n\n> Signed-off-by: Hans de Goede \n> ---\n> include/libcamera/internal/ipa_manager.h | 11 ++++++++---\n> include/libcamera/internal/ipa_module.h | 2 +-\n> src/libcamera/ipa_manager.cpp | 7 ++++---\n> src/libcamera/ipa_module.cpp | 6 +++---\n> 4 files changed, 16 insertions(+), 10 deletions(-)\n>\n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index 16dede0c..1bbb0011 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -33,11 +33,16 @@ 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\nnullptr\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> +\n> +\t\tif (!ipaName)\n> +\t\t\tipaName = pipe->name();\n> +\n> +\t\tIPAModule *m = self->module(ipaName, minVersion, maxVersion);\n> \t\tif (!m)\n> \t\t\treturn nullptr;\n> \n> @@ -62,7 +67,7 @@ private:\n> \t\t std::vector &files);\n> \tunsigned int addDir(const char *libDir, unsigned int maxDepth = 0);\n> \n> -\tIPAModule *module(PipelineHandler *pipe, uint32_t minVersion,\n> +\tIPAModule *module(const char *pipelineName, uint32_t minVersion,\n> \t\t\t uint32_t maxVersion);\n> \n> \tbool isSignatureValid(IPAModule *ipa) const;\n> diff --git a/include/libcamera/internal/ipa_module.h b/include/libcamera/internal/ipa_module.h\n> index 7c49d3f3..f732b0b0 100644\n> --- a/include/libcamera/internal/ipa_module.h\n> +++ b/include/libcamera/internal/ipa_module.h\n> @@ -36,7 +36,7 @@ public:\n> \n> \tIPAInterface *createInterface();\n> \n> -\tbool match(PipelineHandler *pipe,\n> +\tbool match(const char *pipelineName,\n> \t\t uint32_t minVersion, uint32_t maxVersion) const;\n> \n> protected:\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index cfc24d38..a5427ef3 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -243,15 +243,15 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> \n> /**\n> * \\brief Retrieve an IPA module that matches a given pipeline handler\n> - * \\param[in] pipe The pipeline handler\n> + * \\param[in] pipelineName The pipeline handler name\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> */\n> -IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n> +IPAModule *IPAManager::module(const char *pipelineName, uint32_t minVersion,\n> \t\t\t uint32_t maxVersion)\n> {\n> \tfor (IPAModule *module : modules_) {\n> -\t\tif (module->match(pipe, minVersion, maxVersion))\n> +\t\tif (module->match(pipelineName, minVersion, maxVersion))\n> \t\t\treturn module;\n> \t}\n> \n> @@ -264,6 +264,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 overrides pipe->name() for IPA module matching if set\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 9ca74be6..7392e356 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -463,7 +463,7 @@ IPAInterface *IPAModule::createInterface()\n> \n> /**\n> * \\brief Verify if the IPA module matches a given pipeline handler\n> - * \\param[in] pipe Pipeline handler to match with\n> + * \\param[in] pipelineName Pipeline handler name to match with\n> * \\param[in] minVersion Minimum acceptable version of IPA module\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> *\n> @@ -472,12 +472,12 @@ IPAInterface *IPAModule::createInterface()\n> *\n> * \\return True if the pipeline handler matches the IPA module, or false otherwise\n> */\n> -bool IPAModule::match(PipelineHandler *pipe,\n> +bool IPAModule::match(const char *pipelineName,\n> \t\t uint32_t minVersion, uint32_t maxVersion) const\n> {\n> \treturn info_.pipelineVersion >= minVersion &&\n> \t info_.pipelineVersion <= maxVersion &&\n> -\t !strcmp(info_.pipelineName, pipe->name());\n> +\t !strcmp(info_.pipelineName, pipelineName);\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 917C5BDC71\n\tfor ;\n\tMon, 4 Nov 2024 19:12:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2B5D65399;\n\tMon, 4 Nov 2024 20:12:57 +0100 (CET)","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 2310765399\n\tfor ;\n\tMon, 4 Nov 2024 20:12:56 +0100 (CET)","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-100-sSOpgxUOPv2RpoV9ibsz0g-1; Mon, 04 Nov 2024 14:12:53 -0500","by mail-ed1-f69.google.com with SMTP id\n\t4fb4d7f45d1cf-5cbad9b3ca2so3207711a12.3\n\tfor ;\n\tMon, 04 Nov 2024 11:12:53 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5cee6abfba2sm200122a12.36.2024.11.04.11.12.49\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 04 Nov 2024 11:12:50 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"cibWDODu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730747574;\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=cJtp/3D0iXasX2Br8wdo1ZlkPGKGMHm6+h8pONOXU64=;\n\tb=cibWDODuUaNtsMvVsXYNUPvhgGuU2sYs94lRMYmm+46ECL7bQmPABxO7ExWlPzrBlj4Hjd\n\tfz8cgstXjY9juHVC8fEZzE4gvJExD9jjxeYsYRIrWR0iBFD+v8Qyq7/X0P9F+QYTj8fMIL\n\tMRy1I/KfRdmu0dgYiUzDHnFBbVh4jvw=","X-MC-Unique":"sSOpgxUOPv2RpoV9ibsz0g-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730747571; x=1731352371;\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=cJtp/3D0iXasX2Br8wdo1ZlkPGKGMHm6+h8pONOXU64=;\n\tb=Ty2gdDN5ODZlFW28RYXkV9wz/gNKSV33zsUO/1rd+saL95gXGjYDhs05zT3e4NgKzR\n\tBCdagbgXVxcHmU9rMzoWeDUVQWm4JcD+I10UvnwULr9+JRyF9Vff+r73lOqHIVWhPSAQ\n\tlXO3c1I4M4uKofSzFEjNOuXE0yRFOKJMWapwGVxdJZgrRIrCg848vZlunLSXfOjjXvpX\n\tp0kJLI6mQPly9ycq850aNf6E1mh+sy677nGe5w2Bp0LBPvMbcAG5RCAqpMWcWaBu9gUX\n\t8un4BRR7xVXA0tNEMJSKZPxf0lAssWTHMrlswZdaB9Xn5MzfPcPooCrBuAw9ShD0xHb/\n\tQRig==","X-Gm-Message-State":"AOJu0Yzy98VWyj7QNroKB/DFIKNKvAcdM2qu1CS/JTWJ3Y8DzvOVWJjs\n\twD1HcuN+eNueMwIr5NiNqOdie0fhZC0dRRMLtt4Snm4krvuumeEi0mLCiJdOV34oA8L/0nsGyDd\n\tXI3VchN2gZnmzneq0gaIr54yMbyATriH28d9aGiv9nf0LMnD9L+YxR2FQijQntzUhEdhLNDX9lo\n\tJ8NKw=","X-Received":["by 2002:a05:6402:2786:b0:5cb:85f6:186d with SMTP id\n\t4fb4d7f45d1cf-5cbbfac4c5fmr27627079a12.36.1730747571282; \n\tMon, 04 Nov 2024 11:12:51 -0800 (PST)","by 2002:a05:6402:2786:b0:5cb:85f6:186d with SMTP id\n\t4fb4d7f45d1cf-5cbbfac4c5fmr27627065a12.36.1730747570811; \n\tMon, 04 Nov 2024 11:12:50 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHTp8T7G2TEZGflU13w7AXw+5fWXXCvQnfsvIU/Ugv0G4SkjvzzDXeB/VBEmGLPk+tRdIQIfA==","From":"Milan Zamazal ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard ","Subject":"Re: [PATCH 7/8] libcamera: ipa_manager: createIPA: Allow passing an\n\tIPA name to match","In-Reply-To":"<20241103152205.29219-8-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Sun, 3 Nov 2024 16:22:04 +0100\")","References":"<20241103152205.29219-1-hdegoede@redhat.com>\n\t<20241103152205.29219-8-hdegoede@redhat.com>","Date":"Mon, 04 Nov 2024 20:12:48 +0100","Message-ID":"<87plnavmmn.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","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\" "}}]