[{"id":36481,"web_url":"https://patchwork.libcamera.org/comment/36481/","msgid":"<50d98175-170d-435a-bd44-42b74615ecdc@protonmail.com>","date":"2025-10-27T12:02:20","subject":"Re: [PATCH 4/4] sensor: camera_sensor_legacy: Get module identifier\n\tfrom control","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n2025. 10. 23. 12:56 keltezéssel, Isaac Scott írta:\n> It is possible that the base tuning file of a camera sensor may not be\n> applciable to all camera modules including that camera sensor. This is\n> especially applicable in the case of lenses, which can cause lens\n> shading corrections to be unapplicable in the base tuning file, or CCMs\n> configured for the base sensor being incorrect for a given module.\n> \n> Read the module identifier string from the\n> V4L2_CID_CAMERA_MODULE_IDENTIFIER control, and use it as the primary\n> tuning file to search for when initialising the IPA.\n> \n> Signed-off-by: Isaac Scott \n> ---\n> include/libcamera/internal/camera_sensor.h | 2 ++\n> include/libcamera/internal/v4l2_subdevice.h | 2 ++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 ++++--\n> src/libcamera/sensor/camera_sensor.cpp | 10 ++++++\n> src/libcamera/sensor/camera_sensor_legacy.cpp | 5 +++\n> src/libcamera/sensor/camera_sensor_raw.cpp | 4 +++\n> src/libcamera/v4l2_subdevice.cpp | 34 +++++++++++++++++++\n> 7 files changed, 65 insertions(+), 3 deletions(-)\n> \n> [...]\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 31a2ac72a..bc4004e0e 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -7,9 +7,11 @@\n> \n> #include \"libcamera/internal/v4l2_subdevice.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -1743,6 +1745,38 @@ const std::string &V4L2Subdevice::model()\n> \treturn model_;\n> }\n> \n> +/**\n> + * \\brief Retrieve the module identifier of the device\n> + *\n> + * Where a module uses a sensor within a specific camera module, it may require a\n> + * specific tuning file for that particular module. This information is exposed\n> + * through a V4L2 control.\n> + *\n> + * \\return The module identifier of the device\n> + */\n> +const std::optional V4L2Subdevice::module()\n\nI am a bit confused here. This function never returns an empty optional\nas far as I can see.\n\nI think I mentioned it earlier, but I think something like an `std::optional>`\nwould be preferable:\n\n if the outer optional has a value, return it\n if not:\n initialize the inner optional to empty\n if no V4L2_CID_CAMERA_MODULE_IDENTIFIER, return\n if V4L2_CID_CAMERA_MODULE_IDENTIFIER retrieval fails, return\n set the inner optional to the returned string\n return the inner optional\n\nor something similar.\n\nIn any case I think there are still two questions:\n * does this value need to be cached here?\n (CameraSensor* classes already do that)\n * should this be moved into the CameraSensor (base) class maybe?\n (since this feels very sensor specific, but maybe I'm missing something)\n\n\nRegards,\nBarnabás Pőcze\n\n> +{\n> +\tif (module_.has_value())\n> +\t\treturn module_;\n> +\n> +\t/* Check if the control exists for a camera module identifier */\n> +\tif (!controlInfo(V4L2_CID_CAMERA_MODULE_IDENTIFIER))\n> +\t\treturn \"\";\n> +\n> +\t/*\n> +\t * Get the camera module identifier string of the module, if available\n> +\t */\n> +\tauto sensorInfo = getControls(Span({ V4L2_CID_CAMERA_MODULE_IDENTIFIER }));\n> +\tif (sensorInfo.empty())\n> +\t\treturn \"\";\n> +\n> +\tmodule_.emplace(sensorInfo.begin()->second.get());\n> +\tLOG(V4L2, Debug)\n> +\t\t<< \"Camera module identifier: \" << module_.value();\n> +\n> +\treturn module_;\n> +}\n> +\n> /**\n> * \\fn V4L2Subdevice::caps()\n> * \\brief Retrieve the subdevice V4L2 capabilities\n> --\n> 2.43.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 E574DBE080\n\tfor ;\n\tMon, 27 Oct 2025 12:02:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15C4C6074D;\n\tMon, 27 Oct 2025 13:02:28 +0100 (CET)","from mail-24418.protonmail.ch (mail-24418.protonmail.ch\n\t[109.224.244.18])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E29886069A\n\tfor ;\n\tMon, 27 Oct 2025 13:02:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"wQ6554T9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1761566545; x=1761825745;\n\tbh=s7EzJ1F14kiw+IzBvt8Xt/wK7dS0Qzit1Og8rEq8NbU=;\n\th=Date:To:From:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=wQ6554T9UhyCnAZPugQsYrKbXQi0uUXcgIG6Wf/nHNo1HFizZse8HS9zrHmBirj2L\n\t9YCqRE7KQBQhSis0nmSmGwO5ucKhty3O5Dk2ClnOjKPKIYh8duBqoo+lZr49YsDQTn\n\tLcNMHNDfyU0tMGrvHWFk7AD5Z0hPYIaUM89nKWGz413uoyXE/mkzqZtayQd2DfsMpC\n\tyQ5nve7Y3j+N1j5i7F6UhTbLqzfy7mq7SwSZ6hAswAUlAWTLJ46yy8nHB1AmSSDq2z\n\tbZc24IA5PjzzPvAOmQW1J15ffM4FsCMHyueC6TbS3G8Eg8LljfJUg7e8l8JhSZL+Tc\n\tFU9KK4xekWLmw==","Date":"Mon, 27 Oct 2025 12:02:20 +0000","To":"Isaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","From":"=?utf-8?q?P=C5=91cze_Barnab=C3=A1s?= ","Subject":"Re: [PATCH 4/4] sensor: camera_sensor_legacy: Get module identifier\n\tfrom control","Message-ID":"<50d98175-170d-435a-bd44-42b74615ecdc@protonmail.com>","In-Reply-To":"<20251023105651.78395-5-isaac.scott@ideasonboard.com>","References":"<20251023105651.78395-1-isaac.scott@ideasonboard.com>\n\t<20251023105651.78395-5-isaac.scott@ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"b5d6be00ec9d83220a5d73f084c98507136b4f03","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":36485,"web_url":"https://patchwork.libcamera.org/comment/36485/","msgid":"","date":"2025-10-27T13:55:13","subject":"Re: [PATCH 3/4] ipa_proxy: Rework configurationFile to take variable\n\tnumbers of paths","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 10. 23. 12:56 keltezéssel, Isaac Scott írta:\n> At present, it is only possible to attempt to find a tuning file at two\n> paths. These are often the sensor model name, and 'uncalibrated.yaml'.\n> \n> If the camera sensor has a CAMERA_MODULE_IDENTIFIER control, we should\n> first check for a camera module-specific tuning file, with the camera\n> sensor model name as its fallback tuning file path, ideally followed by\n> 'uncalibrated.yaml', which is not possible if the configurationFile()\n> function only takes two potential paths.\n> \n> To handle this problem effectively, adapt the configurationFile()\n> function for all pipelines and unit tests to take an initializer_list of\n> potential paths.\n> \n> Signed-off-by: Isaac Scott \n> ---\n> include/libcamera/internal/ipa_proxy.h | 7 ++-\n> src/libcamera/ipa_proxy.cpp | 48 +++++++++++++-------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 4 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 2 +-\n> src/libcamera/software_isp/software_isp.cpp | 2 +-\n> test/ipa/ipa_interface_test.cpp | 2 +-\n> 7 files changed, 40 insertions(+), 27 deletions(-)\n> \n> [...]\n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> index a3ccfa603..8c150eae6 100644\n> --- a/src/libcamera/ipa_proxy.cpp\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -167,33 +167,47 @@ IPAProxy::~IPAProxy()\n> * \\return The full path to the IPA configuration file, or an empty string if\n> * no configuration file can be found\n> */\n> -std::string IPAProxy::configurationFile(const std::string &name,\n> -\t\t\t\t\tconst std::string &fallbackName) const\n> +std::string IPAProxy::configurationFile(std::initializer_list names) const\n> {\n> \t/*\n> \t * The IPA module name can be used as-is to build directory names as it\n> \t * has been validated when loading the module.\n> \t */\n> \tconst std::string ipaName = ipam_->info().name;\n> -\tstd::string confPath = ipaConfigurationFile(ipaName, name, configPaths_);\n> -\tif (!confPath.empty()) {\n> -\t\tLOG(IPAProxy, Info) << \"Using tuning file \" << confPath;\n> -\t\treturn confPath;\n> +\n> +\t/*\n> +\t * Start with any user override through the module-specific environment\n> +\t * variable. Use the name of the IPA module up to the first '/' to\n> +\t * construct the variable name.\n> +\t */\n> +\tstd::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));\n> +\tstd::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),\n> +\t\t [](unsigned char c) { return std::toupper(c); });\n> +\tipaEnvName = \"LIBCAMERA_\" + ipaEnvName + \"_TUNING_FILE\";\n> +\n> +\tchar const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());\n> +\tif (configFromEnv && *configFromEnv != '\\0') {\n> +\t\tLOG(IPAProxy, Info) << \"Using tuning file \" << configFromEnv;\n> +\t\treturn { std::string(configFromEnv) };\n> \t}\n> \n> -\tif (fallbackName.empty()) {\n> -\t\tLOG(IPAProxy, Error)\n> -\t\t\t<< \"Configuration file '\" << name\n> -\t\t\t<< \"' not found for IPA module '\" << ipaName << \"'\";\n> -\t\treturn std::string();\n> +\n> +\tfor (auto name : names) {\n> +\t\tstd::string confPath = ipaConfigurationFile(ipaName, std::string(name), configPaths_);\n\nThe environment variable handling was moved here, so I think it can be removed\nfrom `ipaConfigurationFile()`.\n\n\n> +\t\tif (!confPath.empty()) {\n> +\t\t\tLOG(IPAProxy, Info) << \"Using tuning file \" << confPath;\n> +\t\t\treturn confPath;\n> +\t\t}\n> +\n> +\t\tif (&name == names.end()) {\n\nThis will never be true. I think the below message should be moved to after the loop.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +\t\t\tLOG(IPAProxy, Error)\n> +\t\t\t\t<< \"Configuration file '\" << name\n> +\t\t\t\t<< \"' not found for IPA module '\" << ipaName << \"'\";\n> +\t\t\treturn std::string();\n> +\t\t}\n> \t}\n> \n> -\tconfPath = ipaConfigurationFile(ipaName, fallbackName, configPaths_);\n> -\tLOG(IPAProxy, Warning)\n> -\t\t<< \"Configuration file '\" << name\n> -\t\t<< \"' not found for IPA module '\" << ipaName\n> -\t\t<< \"', falling back to '\" << confPath << \"'\";\n> -\treturn confPath;\n> +\treturn std::string();\n> }\n> \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 BFA16C3259\n\tfor ;\n\tMon, 27 Oct 2025 13:55:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0BC960765;\n\tMon, 27 Oct 2025 14:55:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BEF946069A\n\tfor ;\n\tMon, 27 Oct 2025 14:55:16 +0100 (CET)","from [192.168.33.25] (185.182.215.162.nat.pool.zt.hu\n\t[185.182.215.162])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 878651661;\n\tMon, 27 Oct 2025 14:53:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ni6fORZH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761573208;\n\tbh=rmCpjMkXy5OFe0Ca6UMyCf1K+8i9sna1VCn/P4UdwCw=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ni6fORZHRCmCItS3g3YuCCkbIA/i6WsdoQ/dk8gtqkSiG8wcpIOeGiGScBslQftVm\n\tCj3/0BbCT/kh5Zr3GkHWSIL/KAZRQj5GBmYT9a6+cJfTqOacdRHqJytXDKr1VC15K9\n\tTSaCGqhz12iwQajWPHQ2j3mPty/FH2IVKCCEOfoI=","Message-ID":"","Date":"Mon, 27 Oct 2025 14:55:13 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/4] ipa_proxy: Rework configurationFile to take variable\n\tnumbers of paths","To":"Isaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251023105651.78395-1-isaac.scott@ideasonboard.com>\n\t\n\t<20251023105651.78395-4-isaac.scott@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251023105651.78395-4-isaac.scott@ideasonboard.com>","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\" "}}]