[{"id":23469,"web_url":"https://patchwork.libcamera.org/comment/23469/","msgid":"<Yqy8PgMWCWLXL8H1@pendragon.ideasonboard.com>","date":"2022-06-17T17:39:10","subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rkisp1: Add Yaml tuning\n\tfile support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Florian,\n\nThank you for the patch.\n\nOn Fri, Jun 17, 2022 at 11:23:13AM +0200, Florian Sylvestre wrote:\n> Retrieve root node in Yaml tuning file and provide to\n> each algorithm this YamlObject to allow them to grab their default\n> parameters by calling init() function.\n> \n> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  src/ipa/rkisp1/rkisp1.cpp | 34 ++++++++++++++++++++++++++++++++++\n>  1 file changed, 34 insertions(+)\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 61a3bab9..5eb23669 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -14,6 +14,7 @@\n>  #include <linux/rkisp1-config.h>\n>  #include <linux/v4l2-controls.h>\n>  \n> +#include <libcamera/base/file.h>\n>  #include <libcamera/base/log.h>\n>  \n>  #include <libcamera/control_ids.h>\n> @@ -24,6 +25,7 @@\n>  #include <libcamera/request.h>\n>  \n>  #include \"libcamera/internal/mapped_framebuffer.h\"\n> +#include \"libcamera/internal/yaml_parser.h\"\n>  \n>  #include \"algorithms/agc.h\"\n>  #include \"algorithms/algorithm.h\"\n> @@ -61,6 +63,7 @@ public:\n>  private:\n>  \tvoid setControls(unsigned int frame);\n>  \tvoid prepareMetadata(unsigned int frame, unsigned int aeState);\n> +\tint parseConfigurationFile(File &file);\n>  \n>  \tstd::map<unsigned int, FrameBuffer> buffers_;\n>  \tstd::map<unsigned int, MappedFrameBuffer> mappedBuffers_;\n> @@ -126,6 +129,37 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n>  \talgorithms_.push_back(std::make_unique<algorithms::Awb>());\n>  \talgorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());\n>  \n> +\t/* Load the tuning file for this sensor. */\n> +\tFile file(settings.configurationFile.c_str());\n> +\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> +\t\tint ret = file.error();\n> +\t\tLOG(IPARkISP1, Error)\n> +\t\t\t<< \"Failed to open configuration file \"\n> +\t\t\t<< settings.configurationFile << \": \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn parseConfigurationFile(file);\n> +}\n> +\n> +int IPARkISP1::parseConfigurationFile(File &file)\n> +{\n> +\tstd::unique_ptr<libcamera::YamlObject> root = YamlParser::parse(file);\n> +\tif (!root)\n> +\t\treturn -EINVAL;\n> +\n> +\tconst YamlObject &algoObject = (*root)[\"algorithms\"];\n> +\n> +\tif (!algoObject[\"algorithms\"].isDictionary())\n> +\t\treturn -EINVAL;\n\nI'm surprised that we go past this, as the tuning data file now has a\nlist for the algorithms element, not a dictionary. I'll test this.\n\nThis makes me realize that my suggestion of using a list will also cause\nother issues. I'll experiment a bit. Do you mind if I take over and send\na v4, in case that's easier than explaining my findings and asking you\nto implement them ?\n\n> +\n> +\t/* Allow each algo to get parameters from configuration file. */\n> +\tfor (auto const &algo : algorithms_) {\n> +\t\tint ret = algo->init(context_, algoObject);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n>  \treturn 0;\n>  }\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A5017BD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 17 Jun 2022 17:39:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE25B65634;\n\tFri, 17 Jun 2022 19:39:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0E4665632\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 17 Jun 2022 19:39:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DD467982;\n\tFri, 17 Jun 2022 19:39:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655487563;\n\tbh=WJwYIues3D4XhmQyMHcNfNvMpRlgKC+NaiwD+J0s+60=;\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=Na7Vh6isEn8h9zvt2VyJxz9NIeVQ/KuNqhLB/pTEU1IQ2sZeEy+hYqhgPNJmUtcIL\n\tG+F3Cv/Un/c5YKQ1i+A1xus4M4OsWND80AhSkrmyj19IEUh/eLj/+n2eduPqm3THIB\n\thnlkrtzX7wjFNsEbbDxeXl15HwxBcb3WiEjInbgiOoN3mmElqITcW6Ow2PMYlR+/ux\n\tKbrkr+yt1CNDkRxvycukCg3yEgfEpJuQPloPwUEZ4qhoyTCkqdr98sirsIOb3dYt/F\n\tPjITyl5A4U775GW/CJmU9dYsfUGkBkcEuYamTnoJ/C4bOmc7L9by3kF0SfIgZM0aGv\n\t5FZfk6C/lD6Uw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655487562;\n\tbh=WJwYIues3D4XhmQyMHcNfNvMpRlgKC+NaiwD+J0s+60=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GUUoyqZGgxoEYFhUAQc+OUiW7ud4DP/h3KqtWhmzQvAv+viylpPudtqzxyCe8WGLO\n\thQfGBzjX3kWrH+IUC2aYXzeGBrqBH5zragPTISRVUJ2gHvif31YDykn4lMKy2TbxJr\n\tJRyZbrSNsoy+6uj/dtSTC2h4a0z/uE+HVxIOzCnM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GUUoyqZG\"; dkim-atps=neutral","Date":"Fri, 17 Jun 2022 20:39:10 +0300","To":"Florian Sylvestre <fsylvestre@baylibre.com>","Message-ID":"<Yqy8PgMWCWLXL8H1@pendragon.ideasonboard.com>","References":"<20220617092315.120781-1-fsylvestre@baylibre.com>\n\t<20220617092315.120781-4-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220617092315.120781-4-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rkisp1: Add Yaml tuning\n\tfile support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":23473,"web_url":"https://patchwork.libcamera.org/comment/23473/","msgid":"<795fa8af-1d9b-cdde-e7dc-3bd29acf1457@ideasonboard.com>","date":"2022-06-20T05:20:07","subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rkisp1: Add Yaml tuning\n\tfile support","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 19/06/2022 17:47, Laurent Pinchart via libcamera-devel wrote:\n> Hi Florian,\n> \n> Another question.\n> \n> On Fri, Jun 17, 2022 at 08:39:10PM +0300, Laurent Pinchart via libcamera-devel wrote:\n>> On Fri, Jun 17, 2022 at 11:23:13AM +0200, Florian Sylvestre wrote:\n>>> Retrieve root node in Yaml tuning file and provide to\n>>> each algorithm this YamlObject to allow them to grab their default\n>>> parameters by calling init() function.\n>>>\n>>> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n>>> ---\n>>>   src/ipa/rkisp1/rkisp1.cpp | 34 ++++++++++++++++++++++++++++++++++\n>>>   1 file changed, 34 insertions(+)\n>>>\n>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>> index 61a3bab9..5eb23669 100644\n>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>> @@ -14,6 +14,7 @@\n>>>   #include <linux/rkisp1-config.h>\n>>>   #include <linux/v4l2-controls.h>\n>>>   \n>>> +#include <libcamera/base/file.h>\n>>>   #include <libcamera/base/log.h>\n>>>   \n>>>   #include <libcamera/control_ids.h>\n>>> @@ -24,6 +25,7 @@\n>>>   #include <libcamera/request.h>\n>>>   \n>>>   #include \"libcamera/internal/mapped_framebuffer.h\"\n>>> +#include \"libcamera/internal/yaml_parser.h\"\n>>>   \n>>>   #include \"algorithms/agc.h\"\n>>>   #include \"algorithms/algorithm.h\"\n>>> @@ -61,6 +63,7 @@ public:\n>>>   private:\n>>>   \tvoid setControls(unsigned int frame);\n>>>   \tvoid prepareMetadata(unsigned int frame, unsigned int aeState);\n>>> +\tint parseConfigurationFile(File &file);\n>>>   \n>>>   \tstd::map<unsigned int, FrameBuffer> buffers_;\n>>>   \tstd::map<unsigned int, MappedFrameBuffer> mappedBuffers_;\n>>> @@ -126,6 +129,37 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n>>>   \talgorithms_.push_back(std::make_unique<algorithms::Awb>());\n>>>   \talgorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());\n>>>   \n>>> +\t/* Load the tuning file for this sensor. */\n>>> +\tFile file(settings.configurationFile.c_str());\n>>> +\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n>>> +\t\tint ret = file.error();\n>>> +\t\tLOG(IPARkISP1, Error)\n>>> +\t\t\t<< \"Failed to open configuration file \"\n>>> +\t\t\t<< settings.configurationFile << \": \" << strerror(-ret);\n> \n> Let's add quotes here, as otherwise when the file doesn't exist the\n> error message isn't very clear:\n> \n> [0:41:48.257336778] [318] ERROR IPARkISP1 rkisp1.cpp:136 Failed to open configuration file : No such file or directory\n> \n> \t\tLOG(IPARkISP1, Error)\n> \t\t\t<< \"Failed to open configuration file '\"\n> \t\t\t<< settings.configurationFile << \"': \"\n> \t\t\t<< strerror(-ret);\n> \n>>> +\t\treturn ret;\n> \n> This introduces a regression as all platforms that currently work with\n> the rkisp1 pipeline handler will fail here.\n> \n> I wonder how we should handle this, both short term and long term. In\n> the short term we could treat this as a warning and use defaults for the\n> algorithms (or just disable them). In the long term, do we want to make\n> the tuning file mandatory ?\n> \n>>> +\t}\n>>> +\n>>> +\treturn parseConfigurationFile(file);\n>>> +}\n>>> +\n>>> +int IPARkISP1::parseConfigurationFile(File &file)\n>>> +{\n>>> +\tstd::unique_ptr<libcamera::YamlObject> root = YamlParser::parse(file);\n>>> +\tif (!root)\n>>> +\t\treturn -EINVAL;\n>>> +\n>>> +\tconst YamlObject &algoObject = (*root)[\"algorithms\"];\n>>> +\n>>> +\tif (!algoObject[\"algorithms\"].isDictionary())\n>>> +\t\treturn -EINVAL;\n>>\n>> I'm surprised that we go past this, as the tuning data file now has a\n>> list for the algorithms element, not a dictionary. I'll test this.\n>>\n>> This makes me realize that my suggestion of using a list will also cause\n>> other issues. I'll experiment a bit. Do you mind if I take over and send\n>> a v4, in case that's easier than explaining my findings and asking you\n>> to implement them ?\n\nMy 2 cents: I don't think we want tuning files to be mandatory, as \nhaving them means you tuned the sensor you use in the first place. It \nmight be the case most of the time, but the algorithms should be able to \ndo something relatively correct without those too.\n\n>>\n>>> +\n>>> +\t/* Allow each algo to get parameters from configuration file. */\n>>> +\tfor (auto const &algo : algorithms_) {\n>>> +\t\tint ret = algo->init(context_, algoObject);\n>>> +\t\tif (ret)\n>>> +\t\t\treturn ret;\n>>> +\t}\n>>> +\n>>>   \treturn 0;\n>>>   }\n>>>   \n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 1B6B4BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 20 Jun 2022 05:20:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5978765637;\n\tMon, 20 Jun 2022 07:20:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7F1C6048F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jun 2022 07:20:10 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:dd5b:d9bb:a45f:5e1b] (unknown\n\t[IPv6:2a01:e0a:169:7140:dd5b:d9bb:a45f:5e1b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E26F8F;\n\tMon, 20 Jun 2022 07:20:10 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655702412;\n\tbh=k9W4DTyq57sktynZvguwz0a3/cQ1Kp/514sEvubC5tk=;\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:\n\tFrom;\n\tb=R7AOMTh/Qvr3uOEOFJ3Fro/5/LdqL/NvYYgBWbFXP4F4kZepwMKAv0LU0fUFueaeK\n\tUKltZYp9dNWb8nHzULAh7A5DOHeYWjN3BDSORx7qWsT+QogArCT9U5JjHjoC/abBPa\n\twKuNwxvIkkiwE1jlh9bqKafqnV+iKNFJp/CrlQ9dwoOn3ysqkJqyBvvN47vHlWBlRU\n\tISZ3Qyg1VSKkZQO8V2S5QfLJJvToau8yPMZtlR8ck5rBaj2cq4a5Zq+6pJLKvJc8Cv\n\tgrv3A0fCQE2a1bS31Pu5WZXqAIRNU1+y3m8Al7g6M1a1pEeBpYkdVCejkBm+Cq2gjM\n\t1iRai/EkEHRFg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655702410;\n\tbh=k9W4DTyq57sktynZvguwz0a3/cQ1Kp/514sEvubC5tk=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Xlc5JTx7SRc+8PFJKvWCZr2iHM1FTFaFu5Up7oOjrh1ULlSGx+mtkQNrcEGs0YDj3\n\tM+utOU3TkE5TU+PMNiytgGEc0vw+pTNJ2dFb2NT82IIQanNpCXkW3S8XIGanErLeK1\n\tt4YXm+6w3mnRmGVED258tUelv10czXnhlK83wHSU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Xlc5JTx7\"; dkim-atps=neutral","Message-ID":"<795fa8af-1d9b-cdde-e7dc-3bd29acf1457@ideasonboard.com>","Date":"Mon, 20 Jun 2022 07:20:07 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tFlorian Sylvestre <fsylvestre@baylibre.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220617092315.120781-1-fsylvestre@baylibre.com>\n\t<20220617092315.120781-4-fsylvestre@baylibre.com>\n\t<Yqy8PgMWCWLXL8H1@pendragon.ideasonboard.com>\n\t<Yq9FHrQj0K/MWX8N@pendragon.ideasonboard.com>","In-Reply-To":"<Yq9FHrQj0K/MWX8N@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rkisp1: Add Yaml tuning\n\tfile support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jean-Michel Hautbois via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":23474,"web_url":"https://patchwork.libcamera.org/comment/23474/","msgid":"<YrAu+trrjKk9b4ww@pendragon.ideasonboard.com>","date":"2022-06-20T08:25:30","subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rkisp1: Add Yaml tuning\n\tfile support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Mon, Jun 20, 2022 at 07:20:07AM +0200, Jean-Michel Hautbois wrote:\n> On 19/06/2022 17:47, Laurent Pinchart via libcamera-devel wrote:\n> > On Fri, Jun 17, 2022 at 08:39:10PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> >> On Fri, Jun 17, 2022 at 11:23:13AM +0200, Florian Sylvestre wrote:\n> >>> Retrieve root node in Yaml tuning file and provide to\n> >>> each algorithm this YamlObject to allow them to grab their default\n> >>> parameters by calling init() function.\n> >>>\n> >>> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> >>> ---\n> >>>   src/ipa/rkisp1/rkisp1.cpp | 34 ++++++++++++++++++++++++++++++++++\n> >>>   1 file changed, 34 insertions(+)\n> >>>\n> >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >>> index 61a3bab9..5eb23669 100644\n> >>> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >>> @@ -14,6 +14,7 @@\n> >>>   #include <linux/rkisp1-config.h>\n> >>>   #include <linux/v4l2-controls.h>\n> >>>   \n> >>> +#include <libcamera/base/file.h>\n> >>>   #include <libcamera/base/log.h>\n> >>>   \n> >>>   #include <libcamera/control_ids.h>\n> >>> @@ -24,6 +25,7 @@\n> >>>   #include <libcamera/request.h>\n> >>>   \n> >>>   #include \"libcamera/internal/mapped_framebuffer.h\"\n> >>> +#include \"libcamera/internal/yaml_parser.h\"\n> >>>   \n> >>>   #include \"algorithms/agc.h\"\n> >>>   #include \"algorithms/algorithm.h\"\n> >>> @@ -61,6 +63,7 @@ public:\n> >>>   private:\n> >>>   \tvoid setControls(unsigned int frame);\n> >>>   \tvoid prepareMetadata(unsigned int frame, unsigned int aeState);\n> >>> +\tint parseConfigurationFile(File &file);\n> >>>   \n> >>>   \tstd::map<unsigned int, FrameBuffer> buffers_;\n> >>>   \tstd::map<unsigned int, MappedFrameBuffer> mappedBuffers_;\n> >>> @@ -126,6 +129,37 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)\n> >>>   \talgorithms_.push_back(std::make_unique<algorithms::Awb>());\n> >>>   \talgorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());\n> >>>   \n> >>> +\t/* Load the tuning file for this sensor. */\n> >>> +\tFile file(settings.configurationFile.c_str());\n> >>> +\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> >>> +\t\tint ret = file.error();\n> >>> +\t\tLOG(IPARkISP1, Error)\n> >>> +\t\t\t<< \"Failed to open configuration file \"\n> >>> +\t\t\t<< settings.configurationFile << \": \" << strerror(-ret);\n> > \n> > Let's add quotes here, as otherwise when the file doesn't exist the\n> > error message isn't very clear:\n> > \n> > [0:41:48.257336778] [318] ERROR IPARkISP1 rkisp1.cpp:136 Failed to open configuration file : No such file or directory\n> > \n> > \t\tLOG(IPARkISP1, Error)\n> > \t\t\t<< \"Failed to open configuration file '\"\n> > \t\t\t<< settings.configurationFile << \"': \"\n> > \t\t\t<< strerror(-ret);\n> > \n> >>> +\t\treturn ret;\n> > \n> > This introduces a regression as all platforms that currently work with\n> > the rkisp1 pipeline handler will fail here.\n> > \n> > I wonder how we should handle this, both short term and long term. In\n> > the short term we could treat this as a warning and use defaults for the\n> > algorithms (or just disable them). In the long term, do we want to make\n> > the tuning file mandatory ?\n> > \n> >>> +\t}\n> >>> +\n> >>> +\treturn parseConfigurationFile(file);\n> >>> +}\n> >>> +\n> >>> +int IPARkISP1::parseConfigurationFile(File &file)\n> >>> +{\n> >>> +\tstd::unique_ptr<libcamera::YamlObject> root = YamlParser::parse(file);\n> >>> +\tif (!root)\n> >>> +\t\treturn -EINVAL;\n> >>> +\n> >>> +\tconst YamlObject &algoObject = (*root)[\"algorithms\"];\n> >>> +\n> >>> +\tif (!algoObject[\"algorithms\"].isDictionary())\n> >>> +\t\treturn -EINVAL;\n> >>\n> >> I'm surprised that we go past this, as the tuning data file now has a\n> >> list for the algorithms element, not a dictionary. I'll test this.\n> >>\n> >> This makes me realize that my suggestion of using a list will also cause\n> >> other issues. I'll experiment a bit. Do you mind if I take over and send\n> >> a v4, in case that's easier than explaining my findings and asking you\n> >> to implement them ?\n> \n> My 2 cents: I don't think we want tuning files to be mandatory, as \n> having them means you tuned the sensor you use in the first place. It \n> might be the case most of the time, but the algorithms should be able to \n> do something relatively correct without those too.\n\nI went with an approach influenced by Raspberry Pi, which as an\n\"uncalibrated.json\" file. v4 includes an \"uncalibrated.yaml\" file that\nthe pipeline handler selects if no other tuning file can be found. Let's\ndiscuss whether that is a good or bad idea in the review of v4.\n\n> >>> +\n> >>> +\t/* Allow each algo to get parameters from configuration file. */\n> >>> +\tfor (auto const &algo : algorithms_) {\n> >>> +\t\tint ret = algo->init(context_, algoObject);\n> >>> +\t\tif (ret)\n> >>> +\t\t\treturn ret;\n> >>> +\t}\n> >>> +\n> >>>   \treturn 0;\n> >>>   }\n> >>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 86E9ABE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 20 Jun 2022 08:25:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB75B65635;\n\tMon, 20 Jun 2022 10:25:47 +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 DBBC760473\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jun 2022 10:25:45 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 264E4892;\n\tMon, 20 Jun 2022 10:25:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655713547;\n\tbh=i0d77eyTrQFRnPhd+8SqrNp3Kjy5plUC89cz55hPRY8=;\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=217TtTEhrwKUVMIIKMq28NrYhiM1ZUDzn4R60TsQSWRNXJFJkgFUxMNqpIySUtHKt\n\tO/VUTbQNJKG45M4pK+l/KECj5VKUBwVCKvMPdXmH3hMLOvrBMuDcOpwgpnr6gmWpII\n\tiks9mNv+zm8+4EdOwGexA9l8khi3PbZKEmtqETnSEscD8ARK5ANv8vqnX2dKRra4Tw\n\t7XgF4z0hjeO/vSxiewm68IsZ0xOeWYnDTBDJ6jjtEgYEIWoNijdIbu9FzO8jA49/OZ\n\tSruS0QQ8tK1i745P/1nqqysHpt5SevS/Bq35DsKyv6o5iRzg7HIMSdxy51hutspBpd\n\tgAPpUVthtSPxg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655713545;\n\tbh=i0d77eyTrQFRnPhd+8SqrNp3Kjy5plUC89cz55hPRY8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cMu5qopj7qzBMQUEp7Hm4s8Kwe6o8tOf/v+7z/dIE/x0mB3+pKMvI2mjMDTpg+69E\n\tgG1zGrrwSpVuimXraZAzxMCMnSixC0x8aAcDX0TcJ8f80plyPR0tbm3IGgo84HgtR3\n\tvIXqXTEubZk7VISZudd+JlOEua5eaUD5o/8566D0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cMu5qopj\"; dkim-atps=neutral","Date":"Mon, 20 Jun 2022 11:25:30 +0300","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<YrAu+trrjKk9b4ww@pendragon.ideasonboard.com>","References":"<20220617092315.120781-1-fsylvestre@baylibre.com>\n\t<20220617092315.120781-4-fsylvestre@baylibre.com>\n\t<Yqy8PgMWCWLXL8H1@pendragon.ideasonboard.com>\n\t<Yq9FHrQj0K/MWX8N@pendragon.ideasonboard.com>\n\t<795fa8af-1d9b-cdde-e7dc-3bd29acf1457@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<795fa8af-1d9b-cdde-e7dc-3bd29acf1457@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rkisp1: Add Yaml tuning\n\tfile support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]