[{"id":26281,"web_url":"https://patchwork.libcamera.org/comment/26281/","msgid":"<167421272601.42371.14947020293466641333@Monstersaurus>","date":"2023-01-20T11:05:26","subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48)\n> Add the ability to read the platform configuration parameters from a config\n> file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment\n> variable. Use the PipelineHandler::configurationFile() helper to determine the\n> full path of the file.\n\nI think this needs an addition to Documentation/environment_variables.rst\n \n> Provide an example configuration file named example.yaml. Currently two\n> parameters are available through the json file:\n> \n> \"min_unicam_buffers\"\n> The minimum number of internal Unicam buffers to allocate.\n> \n> \"min_total_unicam_buffers\"\n> The minimum number of internal + external Unicam buffers that must be allocated.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++\n> .../pipeline/raspberrypi/data/meson.build | 8 ++++\n> .../pipeline/raspberrypi/meson.build | 2 +\n> .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++\n> 4 files changed, 75 insertions(+)\n> create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml\n> create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> new file mode 100644\n> index 000000000000..001a906af528\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> @@ -0,0 +1,20 @@\n> +{\n> + \"version\": 1.0,\n> + \"target\": \"bcm2835\",\n> +\n> + \"pipeline_handler\":\n> + {\n> + # The minimum number of internal buffers to be allocated for\n> + # Unicam. This value must be greater than 0, but less than or\n> + # equal to min_total_unicam_buffers.\n> + \"min_unicam_buffers\": 2,\n\nI anticipate we'll want somewhere specific to document every\nconfiguration file option. I expect this is ok for the moment, but we\nneed to do better at documenting the public API.\n\n\n\n\n> +\n> + # The minimum total (internal + external) buffer count used for\n> + # Unicam. The number of internal buffers allocated for Unicam is\n> + # given by:\n> + #\n> + # internal buffer count = max(min_unicam_buffers,\n> + # min_total_unicam_buffers - external buffer count)\n> + \"min_total_unicam_buffers\": 4\n\nReading above as min_unicam_buffers must be less than or equal to\nmin_total_unicam_buffers really makes me wonder why this isn't set up so\nit's an additional buffer count rather than a total buffer count.\n\nAlso we now have different documentation here in an example file, vs\nwhat's in the code...\n\nWhat will be the definitive source of information ?\n\nI expect it will be the code - so hopefully we can generate\npublic API documentation from that ?\n\n> + }\n> +}\n> diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> new file mode 100644\n> index 000000000000..1c70433bbcbc\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> @@ -0,0 +1,8 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +conf_files = files([\n> + 'example.yaml',\n> +])\n> +\n> +install_data(conf_files,\n> + install_dir : pipeline_data_dir / 'raspberrypi')\n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index 6064a3f00122..59e8fb18c555 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -6,3 +6,5 @@ libcamera_sources += files([\n> 'raspberrypi.cpp',\n> 'rpi_stream.cpp',\n> ])\n> +\n> +subdir('data')\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 6ed4cc2c7ba7..2b396f1db9b6 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> \n> @@ -39,6 +40,7 @@\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> +#include \"libcamera/internal/yaml_parser.h\"\n> \n> #include \"delayed_controls.h\"\n> #include \"dma_heaps.h\"\n> @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> */\n> stream->setExternalBuffer(buffer);\n> }\n> +\n> /*\n> * If no buffer is provided by the request for this stream, we\n> * queue a nullptr to the stream to signify that it must use an\n> @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline()\n> .minTotalUnicamBuffers = 4,\n> };\n> \n> + char const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> + if (!configFromEnv || *configFromEnv == '\\0')\n> + return 0;\n\nThis seems like the sort of thing users might want to set for the whole\nsystem and make sure it stays configured. Which will warrant development\nof a 'libcamera configuration file' - which would then have the ability\nto set a default file here perhaps?\n\nHow do you expect users to currently handle this ? I guess your\nlibcamera-apps will set this env var perhaps? But will that then cause\nusers to have a different experience vs libcamera-apps and say any\ngeneric app or the gstreamer pipeline ?\n\n\n> +\n> + std::string filename = std::string(configFromEnv);\n> + File file(filename);\n> +\n> + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> + LOG(RPI, Error) << \"Failed to open configuration file '\" << filename << \"'\";\n> + return -EIO;\n> + }\n> +\n> + LOG(RPI, Info) << \"Using configuration file '\" << filename << \"'\";\n> +\n> + std::unique_ptr root = YamlParser::parse(file);\n> + if (!root) {\n> + LOG(RPI, Error) << \"Failed to parse configuration file, using defaults\";\n> + return -EINVAL;\n> + }\n> +\n> + std::optional ver = (*root)[\"version\"].get();\n> + if (!ver || *ver != 1.0) {\n> + LOG(RPI, Error) << \"Unexpected configuration file version reported\";\n> + return -EINVAL;\n> + }\n> +\n> + const YamlObject &phConfig = (*root)[\"pipeline_handler\"];\n> + config_.minUnicamBuffers =\n> + phConfig[\"min_unicam_buffers\"].get(config_.minUnicamBuffers);\n> + config_.minTotalUnicamBuffers =\n> + phConfig[\"min_total_unicam_buffers\"].get(config_.minTotalUnicamBuffers);\n> +\n> + if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {\n> + LOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers\";\n> + return -EINVAL;\n> + }\n> +\n> + if (config_.minTotalUnicamBuffers < 1) {\n> + LOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= 1\";\n> + return -EINVAL;\n> + }\n> +\n> return 0;\n> }\n> \n> -- \n> 2.25.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 8E0A3C3240\n\tfor ;\n\tFri, 20 Jan 2023 11:05:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE9AF625E4;\n\tFri, 20 Jan 2023 12:05:30 +0100 (CET)","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 73FF961EFD\n\tfor ;\n\tFri, 20 Jan 2023 12:05:29 +0100 (CET)","from pendragon.ideasonboard.com\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 016A6514;\n\tFri, 20 Jan 2023 12:05:28 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674212731;\n\tbh=+xlkTdkiXXd25WYAO/skISENrK2Zee77U5G8sBYr8Kw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=N9TzBSckVdNtL64bvcj8b5SRRFqAFMaayMI43kPZoYVGvJMhQFoGDtk9BP85FiBty\n\tQVEkgDPSaJD01ejZ3RxBn2ghRsbqegJbgVC12sXiBU6/pV3Yi9kjsqeBn0gcLqmTA3\n\tKE3kOwur2AI38ZfE6l+sbxLBmQq9l8aZAx/HVI2iB+Dcpvu+4biCWcdJNJLqUAGwO+\n\th+v/pcWCgruwaldXDJxEsA8RHUmmUzNrmcB9EZMgaxLd7Wd0siGm5t7ZDIDsA8ZRPJ\n\t+uUR+ry/RTGgdl+RhJDTnXA2mBr6mHEyGceHxPKJCzExkcG4P6xRtIlLBdrvKCA6xW\n\tUSiXHIccboUIQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674212729;\n\tbh=+xlkTdkiXXd25WYAO/skISENrK2Zee77U5G8sBYr8Kw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=JO80HLx/YeP/Jnz8skvQKiDpepRcOUUOpEZfGhT1HhV7mCLqNM6QbgnufsjEj/7wS\n\t81PdTDAGZMNfVcSCtJRaVN6Mg0nMeFhVnXc4jBV+BjgzYIwt6StEZbhuSqsYGcQjhE\n\tYbxYe9Rp8mpWLCHmezOfxtu7cAmcAy11gNOEo+Ek="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JO80HLx/\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230118085953.7027-8-naush@raspberrypi.com>","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-8-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 20 Jan 2023 11:05:26 +0000","Message-ID":"<167421272601.42371.14947020293466641333@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26291,"web_url":"https://patchwork.libcamera.org/comment/26291/","msgid":"","date":"2023-01-20T11:52:58","subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your review!\n\nOn Fri, 20 Jan 2023 at 11:05, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48)\n> > Add the ability to read the platform configuration parameters from a\n> config\n> > file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE\n> environment\n> > variable. Use the PipelineHandler::configurationFile() helper to\n> determine the\n> > full path of the file.\n>\n> I think this needs an addition to Documentation/environment_variables.rst\n>\n\nAck.\n\n\n>\n> > Provide an example configuration file named example.yaml. Currently two\n> > parameters are available through the json file:\n> >\n> > \"min_unicam_buffers\"\n> > The minimum number of internal Unicam buffers to allocate.\n> >\n> > \"min_total_unicam_buffers\"\n> > The minimum number of internal + external Unicam buffers that must be\n> allocated.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++\n> > .../pipeline/raspberrypi/data/meson.build | 8 ++++\n> > .../pipeline/raspberrypi/meson.build | 2 +\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++\n> > 4 files changed, 75 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > new file mode 100644\n> > index 000000000000..001a906af528\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > @@ -0,0 +1,20 @@\n> > +{\n> > + \"version\": 1.0,\n> > + \"target\": \"bcm2835\",\n> > +\n> > + \"pipeline_handler\":\n> > + {\n> > + # The minimum number of internal buffers to be\n> allocated for\n> > + # Unicam. This value must be greater than 0, but less\n> than or\n> > + # equal to min_total_unicam_buffers.\n> > + \"min_unicam_buffers\": 2,\n>\n> I anticipate we'll want somewhere specific to document every\n> configuration file option. I expect this is ok for the moment, but we\n> need to do better at documenting the public API.\n>\n>\n>\n>\n> > +\n> > + # The minimum total (internal + external) buffer count\n> used for\n> > + # Unicam. The number of internal buffers allocated for\n> Unicam is\n> > + # given by:\n> > + #\n> > + # internal buffer count = max(min_unicam_buffers,\n> > + # min_total_unicam_buffers - external buffer\n> count)\n> > + \"min_total_unicam_buffers\": 4\n>\n> Reading above as min_unicam_buffers must be less than or equal to\n> min_total_unicam_buffers really makes me wonder why this isn't set up so\n> it's an additional buffer count rather than a total buffer count.\n>\n\nmin_total_unicam_buffers is the minimum amount of internal + application\nallocated Unicam buffers, i.e. we want to have at least these many total\nbuffers available.\nminUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we\nhave to allocate these many for internal use.\n\nSo in that sense, it does act as an additional count... maybe? Really,\nthese params were defined to mirror what the existing buffer allocating\nlogic did.\n\n\n> Also we now have different documentation here in an example file, vs\n> what's in the code...\n>\n> What will be the definitive source of information ?\n>\n> I expect it will be the code - so hopefully we can generate\n> public API documentation from that ?\n>\n> > + }\n> > +}\n> > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build\n> b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> > new file mode 100644\n> > index 000000000000..1c70433bbcbc\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> > @@ -0,0 +1,8 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +conf_files = files([\n> > + 'example.yaml',\n> > +])\n> > +\n> > +install_data(conf_files,\n> > + install_dir : pipeline_data_dir / 'raspberrypi')\n> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build\n> b/src/libcamera/pipeline/raspberrypi/meson.build\n> > index 6064a3f00122..59e8fb18c555 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > @@ -6,3 +6,5 @@ libcamera_sources += files([\n> > 'raspberrypi.cpp',\n> > 'rpi_stream.cpp',\n> > ])\n> > +\n> > +subdir('data')\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 6ed4cc2c7ba7..2b396f1db9b6 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -14,6 +14,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -39,6 +40,7 @@\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> >\n> > #include \"delayed_controls.h\"\n> > #include \"dma_heaps.h\"\n> > @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> *camera, Request *request)\n> > */\n> > stream->setExternalBuffer(buffer);\n> > }\n> > +\n> > /*\n> > * If no buffer is provided by the request for this\n> stream, we\n> > * queue a nullptr to the stream to signify that it must\n> use an\n> > @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline()\n> > .minTotalUnicamBuffers = 4,\n> > };\n> >\n> > + char const *configFromEnv =\n> utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> > + if (!configFromEnv || *configFromEnv == '\\0')\n> > + return 0;\n>\n> This seems like the sort of thing users might want to set for the whole\n> system and make sure it stays configured. Which will warrant development\n> of a 'libcamera configuration file' - which would then have the ability\n> to set a default file here perhaps?\n>\n> How do you expect users to currently handle this ? I guess your\n> libcamera-apps will set this env var perhaps? But will that then cause\n> users to have a different experience vs libcamera-apps and say any\n> generic app or the gstreamer pipeline ?\n>\n\nCorrect, we will explicitly set the env variable (if needed) in\nlibcamera-apps/picamera2.\nOther apps who don't set the env variable will get the default config,\nwhich will guarantee to work for all cases at the expense of probably over\nallocating buffers.\n\nRegards,\nNaush\n\n\n>\n>\n> > +\n> > + std::string filename = std::string(configFromEnv);\n> > + File file(filename);\n> > +\n> > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > + LOG(RPI, Error) << \"Failed to open configuration file '\"\n> << filename << \"'\";\n> > + return -EIO;\n> > + }\n> > +\n> > + LOG(RPI, Info) << \"Using configuration file '\" << filename <<\n> \"'\";\n> > +\n> > + std::unique_ptr root = YamlParser::parse(file);\n> > + if (!root) {\n> > + LOG(RPI, Error) << \"Failed to parse configuration file,\n> using defaults\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + std::optional ver = (*root)[\"version\"].get();\n> > + if (!ver || *ver != 1.0) {\n> > + LOG(RPI, Error) << \"Unexpected configuration file\n> version reported\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + const YamlObject &phConfig = (*root)[\"pipeline_handler\"];\n> > + config_.minUnicamBuffers =\n> > + phConfig[\"min_unicam_buffers\"].get int>(config_.minUnicamBuffers);\n> > + config_.minTotalUnicamBuffers =\n> > + phConfig[\"min_total_unicam_buffers\"].get int>(config_.minTotalUnicamBuffers);\n> > +\n> > + if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {\n> > + LOG(RPI, Error) << \"Invalid configuration:\n> min_total_unicam_buffers must be >= min_unicam_buffers\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (config_.minTotalUnicamBuffers < 1) {\n> > + LOG(RPI, Error) << \"Invalid configuration:\n> min_total_unicam_buffers must be >= 1\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > --\n> > 2.25.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 0F448BE175\n\tfor ;\n\tFri, 20 Jan 2023 11:53:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F733625E4;\n\tFri, 20 Jan 2023 12:53:16 +0100 (CET)","from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com\n\t[IPv6:2607:f8b0:4864:20::b2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A34F61EFD\n\tfor ;\n\tFri, 20 Jan 2023 12:53:15 +0100 (CET)","by mail-yb1-xb2d.google.com with SMTP id 66so6364985yba.4\n\tfor ;\n\tFri, 20 Jan 2023 03:53:15 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674215596;\n\tbh=rWlAySxxiH/BPYErGhaObQvmWXkqLK2SVqzzWbn24D0=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=SeEZC+2uoEuoXsvISsi2hAlbKmV9BnMVIOxmUJUh7myiU/TCw5uki1ynNs+Lle0SM\n\t6bu1Ow6nBfZxPvXtaRIQdT0OfvO+ShthaRsVVxUAHtJVMLXzO8RHPUNktYa42w4amS\n\trDp7A96uFU+FlXlL38D8mg488kpsMMWD8AdQht1BaOlNSsPGQs8pC4cupoHdEVKWfV\n\tWBpVhoRs9cgpsi687UjtmNqxJj3ME0sRbLFvXCKWUtPAT4DEPdvH4oSYIBIXdJxkZJ\n\thwAx1B84S4R0pQbBfMXfuEHprpGb1ckAG05ZM+YJL6dCh/CF12XK4g0TPovX/YygyR\n\ttT3COLrKhhL8w==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=8cADsM+ZVKjInnT5krnXOp36GKtqn2MpOjlmrKN4bKA=;\n\tb=PC2wmF14yOvGeA021vsbrkfYvKb12wKbLUpJh8J6xiAZYztctKlsEqgrg8WAtDxo2E\n\tDFocD387NqgDC9LiC1rolLnbsN9R/556Zic97dLKDCzjEkz4LYw1W04ssQMDIbPYX65g\n\tWW+D2J2JS7XapLy2r+deToAeSeolS610GYPSCQF4PrUJ0sE/7AGk+XZCxXjDRrl1KeYm\n\tuWzTQR4AYi9a45Ac4iso6wSStS4tat5XDnG9y20U7t9ATfu51Cb8JMlgeDyDspUTfibi\n\tCyzB3Eopd+FGxQSmKOTqElNAMoHxCAe7cRbAtY3RqyFSchNSb9ieMRKPtPLN751wThuW\n\tydRA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"PC2wmF14\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=8cADsM+ZVKjInnT5krnXOp36GKtqn2MpOjlmrKN4bKA=;\n\tb=RTbCFqD9b6PP6oZTM9x+pYR2TwxgAS08iF96dCf/nrB65u644Fkcq0v95n4F7ccsiV\n\tRQHlz5VqNalGXzJ6ZrR6VvqU6NYfv3tDiFi9Z7Lcu5Z9GZkn8pF0uI9Tkx0ogt9EE1u5\n\tA/0A77aFpoNddRtb4WfQlKFMGr/HZsQVdX6OAu3TZXzylgITfiZqPk/M4ifxNiTiJitT\n\tLEtX7oGmqfZ+hKSCRrlmnyjGlo6DSNka1XB2JhRAueu5Ya+0xNQ2UO61N9L2BnXWxg/h\n\tKJQRn4uIIvn+ryE1MN/eI4/DjNlP/8iXCL46J4jhTh9ADngcpPHXTF4ST8sVTlGMV8P1\n\t6j3Q==","X-Gm-Message-State":"AFqh2kq4ZhboktLDp7UMB0Cqy/IlgWJ7bN1+hp+DAutCzUVopdnmFZgE\n\ton6iNqQoPef8eEZj2Tzhi3P6hrSg1q1ARMkPk6Gou3iChSf9B3AI","X-Google-Smtp-Source":"AMrXdXvu3tHtNaVg9FVLVmAzAju8Dzwlg0+2QtqLlttwf5kdyZER9Ph4tr1k6ddPkH8NE9SHHjZiHjnUnWKRrHGB6/g=","X-Received":"by 2002:a25:bf8e:0:b0:7d7:ec44:7cdc with SMTP id\n\tl14-20020a25bf8e000000b007d7ec447cdcmr1624454ybk.598.1674215594144;\n\tFri, 20 Jan 2023 03:53:14 -0800 (PST)","MIME-Version":"1.0","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-8-naush@raspberrypi.com>\n\t<167421272601.42371.14947020293466641333@Monstersaurus>","In-Reply-To":"<167421272601.42371.14947020293466641333@Monstersaurus>","Date":"Fri, 20 Jan 2023 11:52:58 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000c44da605f2b0b125\"","Subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26314,"web_url":"https://patchwork.libcamera.org/comment/26314/","msgid":"","date":"2023-01-22T22:32:29","subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Jan 20, 2023 at 11:52:58AM +0000, Naushir Patuck via libcamera-devel wrote:\n> On Fri, 20 Jan 2023 at 11:05, Kieran Bingham wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48)\n> > > Add the ability to read the platform configuration parameters from a config\n> > > file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment\n> > > variable. Use the PipelineHandler::configurationFile() helper to determine the\n> > > full path of the file.\n> >\n> > I think this needs an addition to Documentation/environment_variables.rst\n> \n> Ack.\n> \n> > > Provide an example configuration file named example.yaml. Currently two\n> > > parameters are available through the json file:\n> > >\n> > > \"min_unicam_buffers\"\n> > > The minimum number of internal Unicam buffers to allocate.\n> > >\n> > > \"min_total_unicam_buffers\"\n> > > The minimum number of internal + external Unicam buffers that must be allocated.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++\n> > > .../pipeline/raspberrypi/data/meson.build | 8 ++++\n> > > .../pipeline/raspberrypi/meson.build | 2 +\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++\n> > > 4 files changed, 75 insertions(+)\n> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > > new file mode 100644\n> > > index 000000000000..001a906af528\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > > @@ -0,0 +1,20 @@\n> > > +{\n> > > + \"version\": 1.0,\n> > > + \"target\": \"bcm2835\",\n> > > +\n> > > + \"pipeline_handler\":\n> > > + {\n> > > + # The minimum number of internal buffers to be allocated for\n> > > + # Unicam. This value must be greater than 0, but less than or\n> > > + # equal to min_total_unicam_buffers.\n> > > + \"min_unicam_buffers\": 2,\n> >\n> > I anticipate we'll want somewhere specific to document every\n> > configuration file option. I expect this is ok for the moment, but we\n> > need to do better at documenting the public API.\n\nI'd appreciate if the documentation in this file could already be\nimproved, to explain the effects of the configuration parameters.\n\n> > > +\n> > > + # The minimum total (internal + external) buffer count used for\n> > > + # Unicam. The number of internal buffers allocated for Unicam is\n> > > + # given by:\n> > > + #\n> > > + # internal buffer count = max(min_unicam_buffers,\n> > > + # min_total_unicam_buffers - external buffer count)\n> > > + \"min_total_unicam_buffers\": 4\n> >\n> > Reading above as min_unicam_buffers must be less than or equal to\n> > min_total_unicam_buffers really makes me wonder why this isn't set up so\n> > it's an additional buffer count rather than a total buffer count.\n> \n> min_total_unicam_buffers is the minimum amount of internal + application\n> allocated Unicam buffers, i.e. we want to have at least these many total\n> buffers available.\n\nWhat happens if the application allocates buffers for the raw stream but\nqueues requests slowly ? The pipeline handler will have less than\nmin_total_unicam_buffers available at a time then. Is the parameter\nill-defined in such cases ?\n\n> minUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we\n> have to allocate these many for internal use.\n> \n> So in that sense, it does act as an additional count... maybe? Really,\n> these params were defined to mirror what the existing buffer allocating\n> logic did.\n> \n> > Also we now have different documentation here in an example file, vs\n> > what's in the code...\n> >\n> > What will be the definitive source of information ?\n> >\n> > I expect it will be the code - so hopefully we can generate\n> > public API documentation from that ?\n\nAs this will be documentation for system integrators, I'd rather have it\nin the example.yaml file, or possibly in .rst form somewhere in\nDocumentation/.\n\n> > > + }\n> > > +}\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> > > new file mode 100644\n> > > index 000000000000..1c70433bbcbc\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> > > @@ -0,0 +1,8 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +conf_files = files([\n> > > + 'example.yaml',\n> > > +])\n> > > +\n> > > +install_data(conf_files,\n> > > + install_dir : pipeline_data_dir / 'raspberrypi')\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > index 6064a3f00122..59e8fb18c555 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > @@ -6,3 +6,5 @@ libcamera_sources += files([\n> > > 'raspberrypi.cpp',\n> > > 'rpi_stream.cpp',\n> > > ])\n> > > +\n> > > +subdir('data')\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 6ed4cc2c7ba7..2b396f1db9b6 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -14,6 +14,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -39,6 +40,7 @@\n> > > #include \"libcamera/internal/media_device.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > >\n> > > #include \"delayed_controls.h\"\n> > > #include \"dma_heaps.h\"\n> > > @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > */\n> > > stream->setExternalBuffer(buffer);\n> > > }\n> > > +\n> > > /*\n> > > * If no buffer is provided by the request for this stream, we\n> > > * queue a nullptr to the stream to signify that it must use an\n> > > @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline()\n> > > .minTotalUnicamBuffers = 4,\n> > > };\n> > >\n> > > + char const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> > > + if (!configFromEnv || *configFromEnv == '\\0')\n> > > + return 0;\n> >\n> > This seems like the sort of thing users might want to set for the whole\n> > system and make sure it stays configured. Which will warrant development\n> > of a 'libcamera configuration file' - which would then have the ability\n> > to set a default file here perhaps?\n> >\n> > How do you expect users to currently handle this ?\n\nI don't like this question, as I don't expect \"users\" in the traditional\nsense of the term to set these parameters. End-users of camera\napplications shouldn't need to know about any of this. These parameters\nshould be for \"system integrators\" instead. Keeping this in mind, I\nwould expect those parameters to be system-wide in the long term, even\nif in the short term solution is different.\n\nOf course, the boundary between \"users\" and \"system integrators\" is\nquite fuzzy, especially on Raspberry Pi systems, but if anyone disagrees\nconceptually, please let me know.\n\n> > I guess your\n> > libcamera-apps will set this env var perhaps? But will that then cause\n> > users to have a different experience vs libcamera-apps and say any\n> > generic app or the gstreamer pipeline ?\n> \n> Correct, we will explicitly set the env variable (if needed) in\n> libcamera-apps/picamera2.\n> Other apps who don't set the env variable will get the default config,\n> which will guarantee to work for all cases at the expense of probably over\n> allocating buffers.\n> \n> > > +\n> > > + std::string filename = std::string(configFromEnv);\n> > > + File file(filename);\n> > > +\n> > > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > + LOG(RPI, Error) << \"Failed to open configuration file '\" << filename << \"'\";\n> > > + return -EIO;\n> > > + }\n> > > +\n> > > + LOG(RPI, Info) << \"Using configuration file '\" << filename << \"'\";\n> > > +\n> > > + std::unique_ptr root = YamlParser::parse(file);\n> > > + if (!root) {\n> > > + LOG(RPI, Error) << \"Failed to parse configuration file, using defaults\";\n> > > + return -EINVAL;\n\nIs this a valid use case or not ? If it's valid, you shouldn't return\n-EINVAL as the caller will otherwise fail to register the camera, and I\nwould demote the message from Error to Warning. If it's no valid, then\nyou should drop the \"using defaults\" from the error message.\n\n> > > + }\n> > > +\n> > > + std::optional ver = (*root)[\"version\"].get();\n> > > + if (!ver || *ver != 1.0) {\n> > > + LOG(RPI, Error) << \"Unexpected configuration file version reported\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + const YamlObject &phConfig = (*root)[\"pipeline_handler\"];\n> > > + config_.minUnicamBuffers =\n> > > + phConfig[\"min_unicam_buffers\"].get(config_.minUnicamBuffers);\n> > > + config_.minTotalUnicamBuffers =\n> > > + phConfig[\"min_total_unicam_buffers\"].get(config_.minTotalUnicamBuffers);\n> > > +\n> > > + if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {\n> > > + LOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + if (config_.minTotalUnicamBuffers < 1) {\n> > > + LOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= 1\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > return 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 6A087BDB1D\n\tfor ;\n\tSun, 22 Jan 2023 22:32:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB124625E4;\n\tSun, 22 Jan 2023 23:32:33 +0100 (CET)","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 54E6F625DD\n\tfor ;\n\tSun, 22 Jan 2023 23:32:32 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 737CA471;\n\tSun, 22 Jan 2023 23:32:31 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674426753;\n\tbh=lqW8NYpyDLYH13JNaHDiodBud1BEDKPueuz5Y0zXPjI=;\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=H1F0SWeMxQM6JdXiptEf/Gj9xJeeXLK2jHNog08KyEit2fGJFGwloqeA1JQRdC304\n\tM07AvNhq1yaBbGx/cDXHX+tf8Ij343mKlNqxKt3pfftQQwMeh6G6O71XJHSLOShOP3\n\tburHPtGKsu3C99W1VYAdAT88GjCAmjptbpFmAA5M1XSgBNdvERwpgYoSa4D7Iec9fH\n\t/yLENWdX1xjmJCxa1zGPqaWvSa13fpeVwJ1Eb0pKt9SB3XdqsLBmLHKecxFNSLAa+F\n\t1yylVBUosH2m3N8e3DR8tOtz+TYU2yAjshgi4HZIAy61Hyzi8U9RgzvlWZM4tZtKDT\n\t8CO1fj1dHvQUw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674426751;\n\tbh=lqW8NYpyDLYH13JNaHDiodBud1BEDKPueuz5Y0zXPjI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WN+4/c4je6nrNyyuA9UrnZr5H9fCUXjeEU2VFgdvFw9Ilh0DgHDxrtvh6GctTXRAM\n\t620FQtc5n6RuMa1e2P+Ansi0qyZacLHX4vDzwihN+mbAZ0PetTeV7P2jH/NCElD+LV\n\tu8TXWVh2WZDDLIf/3WB0cWEGSFzH3ScD/HN3j5bs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WN+4/c4j\"; dkim-atps=neutral","Date":"Mon, 23 Jan 2023 00:32:29 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-8-naush@raspberrypi.com>\n\t<167421272601.42371.14947020293466641333@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26350,"web_url":"https://patchwork.libcamera.org/comment/26350/","msgid":"","date":"2023-01-25T13:00:02","subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"This is true. Apart from the buffer allocation optimisations, the goal of this\nseries was to provide our \"power users\" a way to control some of these intricate\nbits.Hi Laurent,\n\nThank you for your feedback.\n\nOn Sun, 22 Jan 2023 at 22:32, Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> On Fri, Jan 20, 2023 at 11:52:58AM +0000, Naushir Patuck via libcamera-devel wrote:\n> > On Fri, 20 Jan 2023 at 11:05, Kieran Bingham wrote:\n> > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48)\n> > > > Add the ability to read the platform configuration parameters from a config\n> > > > file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment\n> > > > variable. Use the PipelineHandler::configurationFile() helper to determine the\n> > > > full path of the file.\n> > >\n> > > I think this needs an addition to Documentation/environment_variables.rst\n> >\n> > Ack.\n> >\n> > > > Provide an example configuration file named example.yaml. Currently two\n> > > > parameters are available through the json file:\n> > > >\n> > > > \"min_unicam_buffers\"\n> > > > The minimum number of internal Unicam buffers to allocate.\n> > > >\n> > > > \"min_total_unicam_buffers\"\n> > > > The minimum number of internal + external Unicam buffers that must be allocated.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++\n> > > > .../pipeline/raspberrypi/data/meson.build | 8 ++++\n> > > > .../pipeline/raspberrypi/meson.build | 2 +\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++\n> > > > 4 files changed, 75 insertions(+)\n> > > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > > > new file mode 100644\n> > > > index 000000000000..001a906af528\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml\n> > > > @@ -0,0 +1,20 @@\n> > > > +{\n> > > > + \"version\": 1.0,\n> > > > + \"target\": \"bcm2835\",\n> > > > +\n> > > > + \"pipeline_handler\":\n> > > > + {\n> > > > + # The minimum number of internal buffers to be allocated for\n> > > > + # Unicam. This value must be greater than 0, but less than or\n> > > > + # equal to min_total_unicam_buffers.\n> > > > + \"min_unicam_buffers\": 2,\n> > >\n> > > I anticipate we'll want somewhere specific to document every\n> > > configuration file option. I expect this is ok for the moment, but we\n> > > need to do better at documenting the public API.\n>\n> I'd appreciate if the documentation in this file could already be\n> improved, to explain the effects of the configuration parameters.\n\nSure, I can add some more details to the config files.\n\n>\n> > > > +\n> > > > + # The minimum total (internal + external) buffer count used for\n> > > > + # Unicam. The number of internal buffers allocated for Unicam is\n> > > > + # given by:\n> > > > + #\n> > > > + # internal buffer count = max(min_unicam_buffers,\n> > > > + # min_total_unicam_buffers - external buffer count)\n> > > > + \"min_total_unicam_buffers\": 4\n> > >\n> > > Reading above as min_unicam_buffers must be less than or equal to\n> > > min_total_unicam_buffers really makes me wonder why this isn't set up so\n> > > it's an additional buffer count rather than a total buffer count.\n> >\n> > min_total_unicam_buffers is the minimum amount of internal + application\n> > allocated Unicam buffers, i.e. we want to have at least these many total\n> > buffers available.\n>\n> What happens if the application allocates buffers for the raw stream but\n> queues requests slowly ? The pipeline handler will have less than\n> min_total_unicam_buffers available at a time then. Is the parameter\n> ill-defined in such cases ?\n\nmin_total_unicam_buffers refers to the total number of buffers allocated, not\nnecessarily the number of buffers in active use. Maybe this needs to be made\nmore explicit in the above comments as well?\n\n>\n> > minUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we\n> > have to allocate these many for internal use.\n> >\n> > So in that sense, it does act as an additional count... maybe? Really,\n> > these params were defined to mirror what the existing buffer allocating\n> > logic did.\n> >\n> > > Also we now have different documentation here in an example file, vs\n> > > what's in the code...\n> > >\n> > > What will be the definitive source of information ?\n> > >\n> > > I expect it will be the code - so hopefully we can generate\n> > > public API documentation from that ?\n>\n> As this will be documentation for system integrators, I'd rather have it\n> in the example.yaml file, or possibly in .rst form somewhere in\n> Documentation/.\n>\n> > > > + }\n> > > > +}\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> > > > new file mode 100644\n> > > > index 000000000000..1c70433bbcbc\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build\n> > > > @@ -0,0 +1,8 @@\n> > > > +# SPDX-License-Identifier: CC0-1.0\n> > > > +\n> > > > +conf_files = files([\n> > > > + 'example.yaml',\n> > > > +])\n> > > > +\n> > > > +install_data(conf_files,\n> > > > + install_dir : pipeline_data_dir / 'raspberrypi')\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > > index 6064a3f00122..59e8fb18c555 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > > @@ -6,3 +6,5 @@ libcamera_sources += files([\n> > > > 'raspberrypi.cpp',\n> > > > 'rpi_stream.cpp',\n> > > > ])\n> > > > +\n> > > > +subdir('data')\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 6ed4cc2c7ba7..2b396f1db9b6 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -14,6 +14,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -39,6 +40,7 @@\n> > > > #include \"libcamera/internal/media_device.h\"\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > >\n> > > > #include \"delayed_controls.h\"\n> > > > #include \"dma_heaps.h\"\n> > > > @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > > */\n> > > > stream->setExternalBuffer(buffer);\n> > > > }\n> > > > +\n> > > > /*\n> > > > * If no buffer is provided by the request for this stream, we\n> > > > * queue a nullptr to the stream to signify that it must use an\n> > > > @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline()\n> > > > .minTotalUnicamBuffers = 4,\n> > > > };\n> > > >\n> > > > + char const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> > > > + if (!configFromEnv || *configFromEnv == '\\0')\n> > > > + return 0;\n> > >\n> > > This seems like the sort of thing users might want to set for the whole\n> > > system and make sure it stays configured. Which will warrant development\n> > > of a 'libcamera configuration file' - which would then have the ability\n> > > to set a default file here perhaps?\n> > >\n> > > How do you expect users to currently handle this ?\n>\n> I don't like this question, as I don't expect \"users\" in the traditional\n> sense of the term to set these parameters. End-users of camera\n> applications shouldn't need to know about any of this. These parameters\n> should be for \"system integrators\" instead. Keeping this in mind, I\n> would expect those parameters to be system-wide in the long term, even\n> if in the short term solution is different.\n>\n> Of course, the boundary between \"users\" and \"system integrators\" is\n> quite fuzzy, especially on Raspberry Pi systems, but if anyone disagrees\n> conceptually, please let me know.\n\nThis is true. Apart from the buffer allocation optimisations, the goal of this\nseries was to provide our \"power users\" a way to control some of these intricate\nbits.\n\n>\n> > > I guess your\n> > > libcamera-apps will set this env var perhaps? But will that then cause\n> > > users to have a different experience vs libcamera-apps and say any\n> > > generic app or the gstreamer pipeline ?\n> >\n> > Correct, we will explicitly set the env variable (if needed) in\n> > libcamera-apps/picamera2.\n> > Other apps who don't set the env variable will get the default config,\n> > which will guarantee to work for all cases at the expense of probably over\n> > allocating buffers.\n> >\n> > > > +\n> > > > + std::string filename = std::string(configFromEnv);\n> > > > + File file(filename);\n> > > > +\n> > > > + if (!file.open(File::OpenModeFlag::ReadOnly)) {\n> > > > + LOG(RPI, Error) << \"Failed to open configuration file '\" << filename << \"'\";\n> > > > + return -EIO;\n> > > > + }\n> > > > +\n> > > > + LOG(RPI, Info) << \"Using configuration file '\" << filename << \"'\";\n> > > > +\n> > > > + std::unique_ptr root = YamlParser::parse(file);\n> > > > + if (!root) {\n> > > > + LOG(RPI, Error) << \"Failed to parse configuration file, using defaults\";\n> > > > + return -EINVAL;\n>\n> Is this a valid use case or not ? If it's valid, you shouldn't return\n> -EINVAL as the caller will otherwise fail to register the camera, and I\n> would demote the message from Error to Warning. If it's no valid, then\n> you should drop the \"using defaults\" from the error message.\n\nThis should be a valid condition. I'll make the correction as suggested.\n\nRegards,\nNaush\n\n>\n> > > > + }\n> > > > +\n> > > > + std::optional ver = (*root)[\"version\"].get();\n> > > > + if (!ver || *ver != 1.0) {\n> > > > + LOG(RPI, Error) << \"Unexpected configuration file version reported\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + const YamlObject &phConfig = (*root)[\"pipeline_handler\"];\n> > > > + config_.minUnicamBuffers =\n> > > > + phConfig[\"min_unicam_buffers\"].get(config_.minUnicamBuffers);\n> > > > + config_.minTotalUnicamBuffers =\n> > > > + phConfig[\"min_total_unicam_buffers\"].get(config_.minTotalUnicamBuffers);\n> > > > +\n> > > > + if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {\n> > > > + LOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + if (config_.minTotalUnicamBuffers < 1) {\n> > > > + LOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= 1\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 CF219BEFBE\n\tfor ;\n\tWed, 25 Jan 2023 13:00:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43764625E4;\n\tWed, 25 Jan 2023 14:00:21 +0100 (CET)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A17D2603C0\n\tfor ;\n\tWed, 25 Jan 2023 14:00:19 +0100 (CET)","by mail-yb1-xb36.google.com with SMTP id t16so18193392ybk.2\n\tfor ;\n\tWed, 25 Jan 2023 05:00:19 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674651621;\n\tbh=gUoS6Bk72TTvXNHJ72FsUUn1kIuJyEcpCzIx5xjxwEU=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OwWbfc4J4wJcP5FZsmxiRBZbn60nLeKmWnHbi43GgD6dwPwk+eN5rWVcBFFVcIfHK\n\tK0HtqrI/XDfWSuhvNuD4JuH+pJASaUoF7OQbogAKozOlgIQVm8bw0fpH65isELFnFK\n\tAT2C5282VGftSCWUYVPS3zVYgw7rI6OyGZrAHbGNJ6qpdUpCXbY0PftQNHIkAAc8zm\n\t+eH606zJobC9ZKCByJao5SBR24scoFgPdye4cFoaTxbwd3NSuSDx2O3aNxH+aGjx+J\n\tmmXYyOQ+3BKp12eoLunQHUmhEwf8aNPtEcRbQgTJcqj9wC6WqFrzKm5k7x+jNlC++Y\n\tugrgkRN9J+fRA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=K1Xk2NfAlDUHAqOoVtJuthYl+T4qDgg4hfx4rmoTZD0=;\n\tb=sELlauR7FMQx+Iu7ytzzNiG3Q/GFAdKPEJXgR5nhR2u2p3c/oqJDFFUxqWnv/WgRH8\n\t/PrGaxFubiRNmlsRs967RiSdHFoHIlwzFrb/ZLwkDNaRwHKX8UaX2Fykj5QqDTzrpDEB\n\tSK/a/UklRQ8p+TvL62R34GqFlbEBFX7uaQB/R5FcEwdiupvSLWB1Bdk1r44jKHtothB0\n\tFm9mAPv1DDhb9QvSfu9QOLYeJRK5KONJZBQ8ywD2a1WOwjRxhUe8Ye0N4bq496w7kjLq\n\to1wnS3NNE7+wOomi2wGpKwOqnf5YU+Aw1AAIM6mlLzrq7Bi1o7gLRNlmGqnvd1QoMr/k\n\tOp/w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sELlauR7\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=K1Xk2NfAlDUHAqOoVtJuthYl+T4qDgg4hfx4rmoTZD0=;\n\tb=jhI1L3UBdzDZOT64oRdlaXWbMZOd1UXaYhwjgNvHAuHyS6O97wCdrnYgWV+DfeKelB\n\tDfabufkeJz8oRxE8b96KxGq6z3Iy+yu1JlTM+nYSsoLIx4rHAv1cSnxkT4cUQT73CmEB\n\tC9EmzmZ9Kt3xJmD5ipYahrW/RHddVt6jk9jDOCx3zipFyivwVmN2Pn7BOdICl9v2RSMH\n\tbXtrKMAXgYLXPpuz7B0BfC2/YHFmMGRMPCfnl77mbxMKBVXX8HzBnJSTdiqSYcm3ya9P\n\tD2sYIchLASKBvk42+8exThw2CS8wwhRUsbwENKaEW9Ep4MgWsjuNdvL55LGlNLm7i/LV\n\teJ+A==","X-Gm-Message-State":"AO0yUKXY92TX8SxuKCGiR4yDIzjaDDzac8N0p0EvVkxViTU0vzYh7S6M\n\th29CvCFKT+lHmo9/PRZIrbSr/x3re1skf1CsRX8bDA==","X-Google-Smtp-Source":"AK7set9ecgpRRj630NW9KPMp+hVDz07iPBD/HrwHn8jPqS2iP4ycWR/P6478IDjj/gqgQuF7KQv77dgeo7wkejM15ZM=","X-Received":"by 2002:a25:e746:0:b0:80b:6935:4db5 with SMTP id\n\te67-20020a25e746000000b0080b69354db5mr604644ybh.181.1674651618353;\n\tWed, 25 Jan 2023 05:00:18 -0800 (PST)","MIME-Version":"1.0","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-8-naush@raspberrypi.com>\n\t<167421272601.42371.14947020293466641333@Monstersaurus>\n\t\n\t","In-Reply-To":"","Date":"Wed, 25 Jan 2023 13:00:02 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read\n\tconfig parameters from a file","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]