[{"id":18017,"web_url":"https://patchwork.libcamera.org/comment/18017/","msgid":"<20210707143650.65v2fdhuikn72p4f@basti-TUXEDO-Book-XA1510>","date":"2021-07-07T14:36:50","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","submitter":{"id":78,"url":"https://patchwork.libcamera.org/api/people/78/","name":"Sebastian Fricke","email":"sebastian.fricke@posteo.net"},"content":"Hey David,\n\nOn 07.07.2021 14:40, David Plowman wrote:\n>The configuration (camera tuning) file used by the Raspberry Pi comes\n>by default from the sensor name. However, we now allow this to be\n>overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.\n\nNice I like this change, this is a great preparation for\ngeneralizations!\nThank you very much.\n\n>\n>Signed-off-by: David Plowman \nReviewed-by: Sebastian Fricke \n\nGreetings,\nSebastian\n\n>---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n>\n>diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>index 082eb1ee..a738770a 100644\n>--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>@@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> \tipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> \tipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n>\n>-\tIPASettings settings(ipa_->configurationFile(sensor_->model() + \".json\"),\n>-\t\t\t sensor_->model());\n>+\t/*\n>+\t * The configuration (tuning file) is made from the sensor name unless\n>+\t * the environment variable overrides it.\n>+\t */\n>+\tstd::string configurationFile;\n>+\tchar const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_TUNING_FILE\");\n>+\tif (!configFromEnv || *configFromEnv == '\\0')\n>+\t\tconfigurationFile = ipa_->configurationFile(sensor_->model() + \".json\");\n>+\telse\n>+\t\tconfigurationFile = std::string(configFromEnv);\n>+\n>+\tIPASettings settings(configurationFile, sensor_->model());\n>\n> \treturn ipa_->init(settings, sensorConfig);\n> }\n>-- \n>2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 721D1C3224\n\tfor ;\n\tWed, 7 Jul 2021 14:36:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0D6D60284;\n\tWed, 7 Jul 2021 16:36:53 +0200 (CEST)","from mout02.posteo.de (mout02.posteo.de [185.67.36.66])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ACCBA60284\n\tfor ;\n\tWed, 7 Jul 2021 16:36:52 +0200 (CEST)","from submission (posteo.de [89.146.220.130]) \n\tby mout02.posteo.de (Postfix) with ESMTPS id 014A92400FC\n\tfor ;\n\tWed, 7 Jul 2021 16:36:51 +0200 (CEST)","from customer (localhost [127.0.0.1])\n\tby submission (posteo.de) with ESMTPSA id 4GKhnv2dJHz9rxR;\n\tWed, 7 Jul 2021 16:36:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=posteo.net header.i=@posteo.net\n\theader.b=\"ZwZX/kiY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017;\n\tt=1625668612; bh=ERgWmTSWIFPCUQIIixY9r6tGgORNWEOnGtzLjLlhmms=;\n\th=Date:From:To:Cc:Subject:From;\n\tb=ZwZX/kiYnP1QcKHAvCkg2IR9518AzH7tiTfNmNWdLr14aoTYchmekU+YBdisRsLwE\n\tPGPzBcNkmNAqXRQsnDJMM8wO/qt2jf3mAdYUCYXpxKP9p4Bm4mfqSeDQs3s337WN97\n\tlKfQmcKsMtTcBlzq4BN/jiVXH6MfeCXBci4F9w0iyD5JHREQ+/t05v1norC6F2zzFf\n\tMQSQOC7+ZSRDdsc2HABIP9e/T7aWoDD1/Lra1+pF7fJVK0odV+KGTf+aqU55spOAKn\n\tYy8YW215RUmP8JaNdcTgDNn/GLNzrVQE2dRkYhfNsV5SNbOllyHbjKADCIrEj4g/VX\n\tIf44IGkBb1VQw==","Date":"Wed, 7 Jul 2021 14:36:50 +0000","From":"Sebastian Fricke ","To":"David Plowman ","Message-ID":"<20210707143650.65v2fdhuikn72p4f@basti-TUXEDO-Book-XA1510>","References":"<20210707134051.2740-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Disposition":"inline","In-Reply-To":"<20210707134051.2740-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18022,"web_url":"https://patchwork.libcamera.org/comment/18022/","msgid":"","date":"2021-07-08T09:25:07","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your work.\n\nOn Wed, 7 Jul 2021 at 14:40, David Plowman \nwrote:\n\n> The configuration (camera tuning) file used by the Raspberry Pi comes\n> by default from the sensor name. However, we now allow this to be\n> overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.\n>\n> Signed-off-by: David Plowman \n>\n\nReviewed-by: Naushir Patuck \n\n\n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 082eb1ee..a738770a 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> *sensorConfig)\n> ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> ipa_->setDelayedControls.connect(this,\n> &RPiCameraData::setDelayedControls);\n>\n> - IPASettings settings(ipa_->configurationFile(sensor_->model() +\n> \".json\"),\n> - sensor_->model());\n> + /*\n> + * The configuration (tuning file) is made from the sensor name\n> unless\n> + * the environment variable overrides it.\n> + */\n> + std::string configurationFile;\n> + char const *configFromEnv =\n> utils::secure_getenv(\"LIBCAMERA_RPI_TUNING_FILE\");\n> + if (!configFromEnv || *configFromEnv == '\\0')\n> + configurationFile =\n> ipa_->configurationFile(sensor_->model() + \".json\");\n> + else\n> + configurationFile = std::string(configFromEnv);\n> +\n> + IPASettings settings(configurationFile, sensor_->model());\n>\n> return ipa_->init(settings, sensorConfig);\n> }\n> --\n> 2.20.1\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 82685BD794\n\tfor ;\n\tThu, 8 Jul 2021 09:25:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E95B068513;\n\tThu, 8 Jul 2021 11:25:25 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C45BA68509\n\tfor ;\n\tThu, 8 Jul 2021 11:25:23 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id 8so1667916lfp.9\n\tfor ;\n\tThu, 08 Jul 2021 02:25:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"GFzyeNHG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=iAc+RDb6oDDnB75voK0hjGcGq04m9BFoDqJzqMzg+cw=;\n\tb=GFzyeNHG/XC5CUFvzFhwNq10OBVHYKUPF8V11Z9uU1v4KUQXxuwJQbXenqrQPVup65\n\tM3A8AXpkKg1qXWp8UwxKU+ZUSrMV/HKnMVwRBqraO9XRDSPHdHiwmm6L0aYR/pSLwZrS\n\tCLPNBv+WEFb7GP7vrlR49H02AxLxfF5u6o39bN4TFp5C9G2x7ACxaj2misnVY8G9WtuC\n\t5vD5A1o74K3ALn8LISjpf2oPzulyO7lH3ZZesnlegEnBvYor6FMk8/uTcHAQy3KlGuFe\n\tLfwb2y2MsH5vkQ8qWh3ZOSKYj/e6fnrgtBrAFWxziW8IvbffdvZ6uerg52AyXkss5oE9\n\tQw6Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=iAc+RDb6oDDnB75voK0hjGcGq04m9BFoDqJzqMzg+cw=;\n\tb=kx26HrRvbYoNYcWPlNyWoAQ0H2y9A4BARpuHB1gCo0L2xye8peej0Gzu/F1awjNJG9\n\tDa94Z110PNuOEb1J1A0sToDllAOo5ZGO4+vZ7jBOjo7DOan3IwgUKpLvywAvK9ulzaLQ\n\ta596IP4wUwrNnKbGBlHARdCM+SY/BXoO+lJA1A0sYCf2ulGmYX34inAvOXGVI6Nohee9\n\t6eQ/m8JIaz8U4FMb04/2iBD/wN6ar166ljjFI5TjAswcxbfw8Pts/SG7bAv0Avtoto6g\n\tPV29tlLry6dSV3QZLcM+PmNTXDQ1dS2G+3R5ZeLdHwaYl1KxzPDfbrL4trt6qi8Z7cuG\n\t08Rg==","X-Gm-Message-State":"AOAM5323mIAAlrKAIXa6fPkEyihEjVK7hQD34KiFO0ZW5HQItQMn9gXt\n\tBZB0dUQ8owHGnEn8kGud7BemVBpJN6hLXRPHeLqyog==","X-Google-Smtp-Source":"ABdhPJxbvXGJ3hXKz79oE8lpm998qDNJhtn3kQhG6aIAgJPZlgLQFdEShC+hPRfZnKcfXHrRHkX8aCRUZseKpU99ci8=","X-Received":"by 2002:a05:6512:308c:: with SMTP id\n\tz12mr19071908lfd.272.1625736323183; \n\tThu, 08 Jul 2021 02:25:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20210707134051.2740-1-david.plowman@raspberrypi.com>","In-Reply-To":"<20210707134051.2740-1-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 8 Jul 2021 10:25:07 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000000ad1c105c6993ce3\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18061,"web_url":"https://patchwork.libcamera.org/comment/18061/","msgid":"<4adc81f2-1dd5-68aa-66ec-54702b19c47a@ideasonboard.com>","date":"2021-07-09T15:04:22","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 07/07/2021 14:40, David Plowman wrote:\n> The configuration (camera tuning) file used by the Raspberry Pi comes\n> by default from the sensor name. However, we now allow this to be\n> overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.\n\nSome how I had it in my head that this would be an override occurring in\nthe IPA itself, but I think this is ok too.\n\n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 082eb1ee..a738770a 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> \tipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> \tipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> \n> -\tIPASettings settings(ipa_->configurationFile(sensor_->model() + \".json\"),\n> -\t\t\t sensor_->model());\n> +\t/*\n> +\t * The configuration (tuning file) is made from the sensor name unless\n> +\t * the environment variable overrides it.\n> +\t */\n> +\tstd::string configurationFile;\n> +\tchar const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_TUNING_FILE\");\n\nWe likely need to document this somewhere...\n\nWe have Documentation/environment_variables.rst\n\nBut that could be a follow up patch\n\n\n> +\tif (!configFromEnv || *configFromEnv == '\\0')\n> +\t\tconfigurationFile = ipa_->configurationFile(sensor_->model() + \".json\");\n\nI'm not a particular fan of the current ipa_->configurationFile\nimplementation, and I think I see that it should have changes in the future.\n\nIf this were to be generalised, then the override should go there - but\ngiven that this is quite specific to RPi needs right now - I think this\nis ok.\n\nHowever I could envisage that in the future this environment variable\ngets deprecated, but I don't yet know what would replace it - so I\nbelieve it's worth while to have this for now.\n\nReviewed-by: Kieran Bingham \n\n\n> +\telse\n> +\t\tconfigurationFile = std::string(configFromEnv);\n> +\n> +\tIPASettings settings(configurationFile, sensor_->model());\n> \n> \treturn ipa_->init(settings, sensorConfig);\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 D5785C3224\n\tfor ;\n\tFri, 9 Jul 2021 15:04:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45ACC6851B;\n\tFri, 9 Jul 2021 17:04:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74E20605AC\n\tfor ;\n\tFri, 9 Jul 2021 17:04:25 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1914AE7;\n\tFri, 9 Jul 2021 17:04:25 +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=\"Xfc27lZZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625843065;\n\tbh=9YtEuuMXi7K08cqo3buo6mPRRwWKq3ddjk6qK7FLMak=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=Xfc27lZZKFnEz3R/8ZRr/kqQ0i5mM9WP1uiikPoY39ZGHoyo/7P/iB2oAT6xKGr0f\n\tTiqTKmbRN5SGMWxKcuztjmlzKXgJvpRqiR59xEqbolNQm7J75OOEqGNJaNJv0nt315\n\tj2G+66yDS3paghWfb64tYUwJX90pcpHjOmyN4+lI=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210707134051.2740-1-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","Message-ID":"<4adc81f2-1dd5-68aa-66ec-54702b19c47a@ideasonboard.com>","Date":"Fri, 9 Jul 2021 16:04:22 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210707134051.2740-1-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","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":18084,"web_url":"https://patchwork.libcamera.org/comment/18084/","msgid":"","date":"2021-07-11T20:33:36","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote:\n> On 07/07/2021 14:40, David Plowman wrote:\n> > The configuration (camera tuning) file used by the Raspberry Pi comes\n> > by default from the sensor name. However, we now allow this to be\n> > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.\n> \n> Some how I had it in my head that this would be an override occurring in\n> the IPA itself, but I think this is ok too.\n\nI like it better in the pipeline handler, as the IPA module isolation\nmechanisms may limit the ability to pass environmen variables.\n\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--\n> > 1 file changed, 12 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 082eb1ee..a738770a 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > \tipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > \tipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> > \n> > -\tIPASettings settings(ipa_->configurationFile(sensor_->model() + \".json\"),\n> > -\t\t\t sensor_->model());\n> > +\t/*\n> > +\t * The configuration (tuning file) is made from the sensor name unless\n> > +\t * the environment variable overrides it.\n> > +\t */\n> > +\tstd::string configurationFile;\n> > +\tchar const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_TUNING_FILE\");\n> \n> We likely need to document this somewhere...\n> \n> We have Documentation/environment_variables.rst\n> \n> But that could be a follow up patch\n> \n> > +\tif (!configFromEnv || *configFromEnv == '\\0')\n> > +\t\tconfigurationFile = ipa_->configurationFile(sensor_->model() + \".json\");\n> \n> I'm not a particular fan of the current ipa_->configurationFile\n> implementation, and I think I see that it should have changes in the future.\n> \n> If this were to be generalised, then the override should go there - but\n> given that this is quite specific to RPi needs right now - I think this\n> is ok.\n\nI'd like to see this being generalized indeed. We'll have to figure out\nhow to specific a per-camera configuration file though.\n\n> However I could envisage that in the future this environment variable\n> gets deprecated, but I don't yet know what would replace it - so I\n> believe it's worth while to have this for now.\n\nThat's the important question: David, if we later create a more generic\nmechanism to select a different IPA configuration file, will it be fine\nto drop support for LIBCAMERA_RPI_TUNING_FILE ? If so,\n\nReviewed-by: Laurent Pinchart \n\n> Reviewed-by: Kieran Bingham \n> \n> > +\telse\n> > +\t\tconfigurationFile = std::string(configFromEnv);\n> > +\n> > +\tIPASettings settings(configurationFile, sensor_->model());\n> > \n> > \treturn ipa_->init(settings, sensorConfig);\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 33A69C3224\n\tfor ;\n\tSun, 11 Jul 2021 20:34:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FF456851D;\n\tSun, 11 Jul 2021 22:34:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 23E8B68519\n\tfor ;\n\tSun, 11 Jul 2021 22:34:23 +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 965CDCC;\n\tSun, 11 Jul 2021 22:34:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ba3rCRLo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626035662;\n\tbh=1QehiWPQjZMw2+ms0xJXCfOgnXfR0cCuecp6PY5MSqY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ba3rCRLoKIk1e6IXi/TaA1tLMqlnJcuUYfAWzHdVqCmvb9CeM2Dj465ikcIhrEBLz\n\tAaNSpQcboG0VP6aOOUeW7czSxrdrgMDYnYEP/Si48tLM75XWrfaKa7BYvPWQXzUycf\n\tJt1hcRS5RypjW1kC2r+EQPJOQgIwl4GOSgOnb+qE=","Date":"Sun, 11 Jul 2021 23:33:36 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210707134051.2740-1-david.plowman@raspberrypi.com>\n\t<4adc81f2-1dd5-68aa-66ec-54702b19c47a@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<4adc81f2-1dd5-68aa-66ec-54702b19c47a@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18119,"web_url":"https://patchwork.libcamera.org/comment/18119/","msgid":"","date":"2021-07-12T13:46:48","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nDavid's away this week, so I'll respond on his behalf.\n\nOn Sun, 11 Jul 2021 at 21:34, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hello,\n>\n> On Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote:\n> > On 07/07/2021 14:40, David Plowman wrote:\n> > > The configuration (camera tuning) file used by the Raspberry Pi comes\n> > > by default from the sensor name. However, we now allow this to be\n> > > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.\n> >\n> > Some how I had it in my head that this would be an override occurring in\n> > the IPA itself, but I think this is ok too.\n>\n> I like it better in the pipeline handler, as the IPA module isolation\n> mechanisms may limit the ability to pass environmen variables.\n>\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--\n> > > 1 file changed, 12 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 082eb1ee..a738770a 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1232,8 +1232,18 @@ int\n> RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > > ipa_->setDelayedControls.connect(this,\n> &RPiCameraData::setDelayedControls);\n> > >\n> > > - IPASettings settings(ipa_->configurationFile(sensor_->model() +\n> \".json\"),\n> > > - sensor_->model());\n> > > + /*\n> > > + * The configuration (tuning file) is made from the sensor name\n> unless\n> > > + * the environment variable overrides it.\n> > > + */\n> > > + std::string configurationFile;\n> > > + char const *configFromEnv =\n> utils::secure_getenv(\"LIBCAMERA_RPI_TUNING_FILE\");\n> >\n> > We likely need to document this somewhere...\n> >\n> > We have Documentation/environment_variables.rst\n> >\n> > But that could be a follow up patch\n> >\n> > > + if (!configFromEnv || *configFromEnv == '\\0')\n> > > + configurationFile =\n> ipa_->configurationFile(sensor_->model() + \".json\");\n> >\n> > I'm not a particular fan of the current ipa_->configurationFile\n> > implementation, and I think I see that it should have changes in the\n> future.\n> >\n> > If this were to be generalised, then the override should go there - but\n> > given that this is quite specific to RPi needs right now - I think this\n> > is ok.\n>\n> I'd like to see this being generalized indeed. We'll have to figure out\n> how to specific a per-camera configuration file though.\n>\n> > However I could envisage that in the future this environment variable\n> > gets deprecated, but I don't yet know what would replace it - so I\n> > believe it's worth while to have this for now.\n>\n> That's the important question: David, if we later create a more generic\n> mechanism to select a different IPA configuration file, will it be fine\n> to drop support for LIBCAMERA_RPI_TUNING_FILE ?\n\n\nYes, I think that should be fine. I assume any new mechanism would look\nlike a string parameter passed into the pipeline handler directly through\nthe application, hence able to be set on the command line.\n\nRegards,\nNaush\n\n\n\n> If so,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > Reviewed-by: Kieran Bingham \n> >\n> > > + else\n> > > + configurationFile = std::string(configFromEnv);\n> > > +\n> > > + IPASettings settings(configurationFile, sensor_->model());\n> > >\n> > > return ipa_->init(settings, sensorConfig);\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 534BFC3226\n\tfor ;\n\tMon, 12 Jul 2021 13:47:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1C5F68524;\n\tMon, 12 Jul 2021 15:47:06 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1727968513\n\tfor ;\n\tMon, 12 Jul 2021 15:47:05 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id u25so24301778ljj.11\n\tfor ;\n\tMon, 12 Jul 2021 06:47:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"SLWCYIdk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=arDzsHbNsuQbRVAZ4s9D7MFM7HL6Au1qiIL/gaB25so=;\n\tb=SLWCYIdkYUhHAG48Wj+mlVs7z+s8IBxSHxsjtwUIwhrhqf3jhyESB6qfnAqCwOM9yy\n\tYtq2nQTNHKrxXgLMMagLXs/RKWAkZDX8PWDyaAjrYFPgMOCVnxhiOQvO3+yTKld2AV6R\n\tYiECqEdEC+kr6afAxUANtXO0BbgKGPzCeyAxkreBo0ZikRManXSZauDM4FJZ8aNnqYc3\n\tWIRk0Qblhi/AHJ8/vKDVldmwBpvRNf/k0QAHwLu/XCjcOJwZ8T/KVTgc9f/8lwzA3ME0\n\t5DkGqwS2DSGxrdDgetkJaSd5Jp9gfsHPd/IvDSZpyy2+8GPW5yNFWjznMrzWJVjiAWf+\n\t/j1A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=arDzsHbNsuQbRVAZ4s9D7MFM7HL6Au1qiIL/gaB25so=;\n\tb=Y6+XjTHtTCI1CB/zfrlXPO8MsMdCNMIoco+280ZgguFqavfexydFRM7DFll2GM9f/8\n\t00P/+rPZZ6Co9zKpliF5YYexqTVH75hz0Oh2T4/9hFHIiS2FEXfgNgrntRTQVryl7tD1\n\telsxIsMnh8bjvfBv8ZhmS3Pd+cdKIa/hCEbHvvYcYB0qhXZVFu/tsq+ukov4vKsu6w6K\n\tQ5g5VJlwvAQdsWL5+NRcGtrvJUPvpXN3pN3CImIJJvvBP7CSuQK6KyAdXSX5L55iOYzW\n\tCJlMnBYeyKrIguk9dBIlX2ujoLAgzyZYEbqwXgqNIIBq+lZtnxtD+VoUJiGG09a4R2VA\n\ttRNQ==","X-Gm-Message-State":"AOAM531myR9+7OzMYMCKCuB/z4c2AfVN9XyKKOwz13Z7hhtOO91ZgX3+\n\tOoFMF+pUcrKM066I3kaCwCzZ8/0fHVqU8bdUd9dnw454u4c=","X-Google-Smtp-Source":"ABdhPJwME2Kj1YRWdPw0k1jYSRpEtEG/Afjff04J3hzDdqWVl0QaRxifGZjtPQM5MvXo8/EHCxRl00lgiZcFilfmt9Q=","X-Received":"by 2002:a2e:7617:: with SMTP id\n\tr23mr24815173ljc.169.1626097624308; \n\tMon, 12 Jul 2021 06:47:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20210707134051.2740-1-david.plowman@raspberrypi.com>\n\t<4adc81f2-1dd5-68aa-66ec-54702b19c47a@ideasonboard.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 12 Jul 2021 14:46:48 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000446e7905c6ed5b8c\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18143,"web_url":"https://patchwork.libcamera.org/comment/18143/","msgid":"","date":"2021-07-12T17:55:52","subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Jul 12, 2021 at 02:46:48PM +0100, Naushir Patuck wrote:\n> On Sun, 11 Jul 2021 at 21:34, Laurent Pinchart wrote:\n> > On Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote:\n> > > On 07/07/2021 14:40, David Plowman wrote:\n> > > > The configuration (camera tuning) file used by the Raspberry Pi comes\n> > > > by default from the sensor name. However, we now allow this to be\n> > > > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.\n> > >\n> > > Some how I had it in my head that this would be an override occurring in\n> > > the IPA itself, but I think this is ok too.\n> >\n> > I like it better in the pipeline handler, as the IPA module isolation\n> > mechanisms may limit the ability to pass environmen variables.\n> >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--\n> > > > 1 file changed, 12 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 082eb1ee..a738770a 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > > > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);\n> > > > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);\n> > > >\n> > > > - IPASettings settings(ipa_->configurationFile(sensor_->model() + \".json\"),\n> > > > - sensor_->model());\n> > > > + /*\n> > > > + * The configuration (tuning file) is made from the sensor name unless\n> > > > + * the environment variable overrides it.\n> > > > + */\n> > > > + std::string configurationFile;\n> > > > + char const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_TUNING_FILE\");\n> > >\n> > > We likely need to document this somewhere...\n> > >\n> > > We have Documentation/environment_variables.rst\n> > >\n> > > But that could be a follow up patch\n> > >\n> > > > + if (!configFromEnv || *configFromEnv == '\\0')\n> > > > + configurationFile = ipa_->configurationFile(sensor_->model() + \".json\");\n> > >\n> > > I'm not a particular fan of the current ipa_->configurationFile\n> > > implementation, and I think I see that it should have changes in the future.\n> > >\n> > > If this were to be generalised, then the override should go there - but\n> > > given that this is quite specific to RPi needs right now - I think this\n> > > is ok.\n> >\n> > I'd like to see this being generalized indeed. We'll have to figure out\n> > how to specific a per-camera configuration file though.\n> >\n> > > However I could envisage that in the future this environment variable\n> > > gets deprecated, but I don't yet know what would replace it - so I\n> > > believe it's worth while to have this for now.\n> >\n> > That's the important question: David, if we later create a more generic\n> > mechanism to select a different IPA configuration file, will it be fine\n> > to drop support for LIBCAMERA_RPI_TUNING_FILE ?\n> \n> Yes, I think that should be fine. I assume any new mechanism would look\n> like a string parameter passed into the pipeline handler directly through\n> the application, hence able to be set on the command line.\n\nI'm not sure yet what form that mechanism will take. An API that can be\ncalled by applications is certainly tempting. In any case, it will have\nto support per-camera configuration files, as a pipeline handler can\nexpose multiple cameras.\n\n> > If so,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > + else\n> > > > + configurationFile = std::string(configFromEnv);\n> > > > +\n> > > > + IPASettings settings(configurationFile, sensor_->model());\n> > > >\n> > > > return ipa_->init(settings, sensorConfig);\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 79393C3225\n\tfor ;\n\tMon, 12 Jul 2021 17:56:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9547B68524;\n\tMon, 12 Jul 2021 19:56:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A9FB68513\n\tfor ;\n\tMon, 12 Jul 2021 19:56:39 +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 11303CC;\n\tMon, 12 Jul 2021 19:56:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OGXO1KTl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626112599;\n\tbh=mgkmvBDCdd8cWe9OGsJdbhw5V/NTlGrlO4jACMs2v9M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OGXO1KTl5zR9KDLnIrDmKk9zCRp4NTr/EgMCrCUK9t5C5p5I7oo1Tpksuv2CCBvVY\n\tsQ25GH8RzhbCtu6ACUdHhW0ZywnWZjBKiCttd3E2QO5SvAYiUxC4zpPaWTVr4Nxz9S\n\tmh7CEJqdPKsycYW2QEqDeyb0U5S78ch1FLkFZ+mA=","Date":"Mon, 12 Jul 2021 20:55:52 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210707134051.2740-1-david.plowman@raspberrypi.com>\n\t<4adc81f2-1dd5-68aa-66ec-54702b19c47a@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the\n\ttuning file to be set by an environment variable","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]