[{"id":25970,"web_url":"https://patchwork.libcamera.org/comment/25970/","msgid":"","date":"2022-12-02T12:38:46","subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Read the platform configuration parameters from default.json config file. Use\n> the PipelineHandler::configurationFile() helper to determine the full path of\n> the file. The default.json filename can be overridden by the user setting the\n> LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of the\n> custom file.\n\nHow do you envision supporting different configurations for different\ncameras (for instance for the cameras connected to the two Unicam\ninstances on the Raspberry Pi CM) ?\n\n> Add config parameter validation to ensure bad parameters cannot cause the\n> pipeline handler to stop working.\n> \n> Currently three 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> \"num_output0_buffers\"\n> Number of internal ISP Output0 buffers to allocate.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/data/default.json | 20 ++++++\n> .../pipeline/raspberrypi/data/meson.build | 8 +++\n> .../pipeline/raspberrypi/meson.build | 2 +\n> .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++\n> 4 files changed, 91 insertions(+)\n> create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json\n> create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n> new file mode 100644\n> index 000000000000..d709e31850af\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\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 Unicam.\n> + # This value must be greater than 0, but less than or equal to min_total_unicam_buffers.\n\nCould you please wrap lines to 80 columns where possible ?\n\nComments are not allowed by JSON. This works only because we use a YAML\nperson that doesn't enforce strict JSON compliance. I would thus name\nthe file with a .yaml extension, and possibly convert it to the native\nYAML syntax. You can then add an SPDX license header.\n\n> + \"min_unicam_buffers\": 2,\n> +\n> + # The minimum total (internal + external) buffer count used for Unicam.\n> + # The number of internal buffers allocated for Unicam is given by:\n> + # internal buffer count = max(min_unicam_buffers,\n> + # min_total_unicam_buffers - external buffer count)\n> + \"min_total_unicam_buffers\": 4,\n> + \n> + # The number of internal buffers used for ISP Output0. This must be set to 1.\n> + \"num_output0_buffers\": 1\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..232f8b43c5fd\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> + 'default.json',\n> +])\n> +\n> +install_data(conf_files,\n> + install_dir : libcamera_datadir / 'pipeline/raspberrypi')\n\nShould we add a pipeline_data_dir variable, the same way we have\nipa_data_dir ?\n\n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index f1a2f5ee72c2..48235f887501 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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> @@ -40,6 +41,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 \"dma_heaps.h\"\n> #include \"rpi_stream.h\"\n> @@ -313,6 +315,7 @@ public:\n> \t};\n> \n> \tConfig config_;\n> +\tunsigned int numUnicamBuffers;\n> \n> private:\n> \tvoid checkRequestCompleted();\n> @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \t\t\t */\n> \t\t\tstream->setExternalBuffer(buffer);\n> \t\t}\n> +\n> +\t\tif (!buffer) {\n> +\t\t\tif (stream == &data->isp_[Isp::Output0] && !data->config_.numOutput0Buffers) {\n> +\t\t\t\tLOG(RPI, Error) << \"Output0 buffer must be provided in the request.\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tif (stream == &data->unicam_[Unicam::Image] && !data->numUnicamBuffers) {\n> +\t\t\t\tLOG(RPI, Error) << \"Unicam Image buffer must be provided in the request.\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t}\n\nThis seems to belong to a separate patch. Same for the change in\nPipelineHandlerRPi::prepareBuffers().\n\n> +\n> \t\t/*\n> \t\t * If no buffer is provided by the request for this stream, we\n> \t\t * queue a nullptr to the stream to signify that it must use an\n> @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> {\n> \tRPiCameraData::Config &config = data->config_;\n> +\tstd::string filename;\n> \n> \tconfig = {\n> \t\t.minUnicamBuffers = 2,\n> @@ -1426,6 +1443,49 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> \t\t.numOutput0Buffers = 1,\n> \t};\n> \n> +\tchar const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> +\tif (!configFromEnv || *configFromEnv == '\\0')\n> +\t\tfilename = configurationFile(\"raspberrypi\", \"default.json\");\n\nIdeally the first parameter should be deduced from the PipelineHandler\nsubclass automatically, but that can be done later.\n\n> +\telse\n> +\t\tfilename = std::string(configFromEnv);\n> +\n> +\tif (filename.empty())\n> +\t\treturn -EINVAL;\n\nDo we need to require a default configuration file, given that config is\ninitialized to default values above ? I'm even tempted to consider\ndefault.json as an example (possibly renamed to config.json.example)\nonly and not try to load it at runtime.\n\n> +\n> +\tFile file(filename);\n> +\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> +\t\tLOG(RPI, Error) << \"Failed to open configuration file '\" << filename << \"'\";\n> +\t\treturn -EIO;\n> +\t}\n> +\n> +\tLOG(RPI, Info) << \"Using configuration file '\" << filename << \"'\";\n> +\n> +\tstd::unique_ptr root = YamlParser::parse(file);\n> +\tif (!root) {\n> +\t\tLOG(RPI, Error) << \"Failed to parse configuration file, using defaults!\";\n\nNo need to shout, you can drop the exclamation mark at the end of all\ncomments.\n\n> +\t\treturn -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> +\t}\n> +\n> +\tASSERT((*root)[\"version\"].get() == 1.0);\n\nI think this deserves similar treatment as parsing failures.\n\n> +\n> +\tconst YamlObject &phConfig = (*root)[\"pipeline_handler\"];\n> +\tconfig.minUnicamBuffers =\n> +\t\tphConfig[\"min_unicam_buffers\"].get(config.minUnicamBuffers);\n> +\tconfig.minTotalUnicamBuffers =\n> +\t\tphConfig[\"min_total_unicam_buffers\"].get(config.minTotalUnicamBuffers);\n> +\tconfig.numOutput0Buffers =\n> +\t\tphConfig[\"num_output0_buffers\"].get(config.numOutput0Buffers);\n> +\n> +\tif (config.minTotalUnicamBuffers < config.minUnicamBuffers) {\n> +\t\tLOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers!\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (config.minTotalUnicamBuffers < 1) {\n> +\t\tLOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= 1!\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t\t\t */\n> \t\t\tnumBuffers = std::max(data->config_.minUnicamBuffers,\n> \t\t\t\t\t\t minBuffers - numRawBuffers);\n> +\t\t\tdata->numUnicamBuffers = numBuffers;\n> \t\t} else if (stream == &data->isp_[Isp::Input]) {\n> \t\t\t/*\n> \t\t\t * ISP input buffers are imported from Unicam, so follow","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 A1BABBE08B\n\tfor ;\n\tFri, 2 Dec 2022 12:38:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32E6963336;\n\tFri, 2 Dec 2022 13:38:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A18C60483\n\tfor ;\n\tFri, 2 Dec 2022 13:38:49 +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 BEBBE151A;\n\tFri, 2 Dec 2022 13:38:48 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669984731;\n\tbh=1EVFhGu0Gq4ENYG34ubDqYIJV8/CWlpJs4Zbin9ubRI=;\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=c1huXzuHbPJ5O0lYlS1WPkN9ImWCP0rEiafoBB4KuoMxmIHFwKN/KRgtD+CaD0WZZ\n\tjUp3y66nMpV5wtqA42rzBMoBX2vlakymsq2FuzRE4MqLCUVVceOOcZN0eNzNlmeCfk\n\tuJD3B14GF1WwpQg/azVi+KoEpYJNgl/A/v0W2uvDsf3aIpM712fS6T/WAaNoqokGZR\n\tghu2o3fPMM53gdGMgX6NGFso+Uy4VSM83MnjFBIKntaVE8vKW6ixjoVSAFKiVYb3sL\n\t4vh2N8ZXv07+Xd4faNDhwbPDkkvzu0V2G2DaGY5QFQUmDlHovLAO5YlT9rAPzYlgfg\n\tztR8mr8C00Rvw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669984729;\n\tbh=1EVFhGu0Gq4ENYG34ubDqYIJV8/CWlpJs4Zbin9ubRI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v1TYF0h/MiTnVr67rBfIDcNfUTJGCDyu+zRP0BG9sCcPgLkDROZsPYRVnn94XeSNF\n\tpBTj1hNuDqSXaIGO0gbQQ1R3hnMkzFVpx1bORbnLZ5G7kHxRCuq4QMEut7QOVZHUBg\n\tUo2ODvxD/mm2ThJH61II5oQvnCOoZCd+JYoCcuss="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"v1TYF0h/\"; dkim-atps=neutral","Date":"Fri, 2 Dec 2022 14:38:46 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221129134534.2933-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":25973,"web_url":"https://patchwork.libcamera.org/comment/25973/","msgid":"","date":"2022-12-02T13:31:08","subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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 Laurent,\n\nThank you for your feedback!\n\nOn Fri, 2 Dec 2022 at 12:38, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > Read the platform configuration parameters from default.json config\n> file. Use\n> > the PipelineHandler::configurationFile() helper to determine the full\n> path of\n> > the file. The default.json filename can be overridden by the user\n> setting the\n> > LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of\n> the\n> > custom file.\n>\n> How do you envision supporting different configurations for different\n> cameras (for instance for the cameras connected to the two Unicam\n> instances on the Raspberry Pi CM) ?\n>\n\nFollowing on from the comments in patch 2, as long as the 2 cameras are\nrunning in 2 separate libcamera processes, we can use\nthe LIBCAMERA_RPI_CONFIG_FILE\nenv variable to set different configurations. Longer term, it would be\nnice to\nhave an API to pass this (as well as a custom IPA turning file) from the\napplication.\n\n\n>\n> > Add config parameter validation to ensure bad parameters cannot cause the\n> > pipeline handler to stop working.\n> >\n> > Currently three 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> > \"num_output0_buffers\"\n> > Number of internal ISP Output0 buffers to allocate.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/data/default.json | 20 ++++++\n> > .../pipeline/raspberrypi/data/meson.build | 8 +++\n> > .../pipeline/raspberrypi/meson.build | 2 +\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++\n> > 4 files changed, 91 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json\n> b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > new file mode 100644\n> > index 000000000000..d709e31850af\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\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 Unicam.\n> > + # This value must be greater than 0, but less than or\n> equal to min_total_unicam_buffers.\n>\n> Could you please wrap lines to 80 columns where possible ?\n>\n> Comments are not allowed by JSON. This works only because we use a YAML\n> person that doesn't enforce strict JSON compliance. I would thus name\n> the file with a .yaml extension, and possibly convert it to the native\n> YAML syntax. You can then add an SPDX license header.\n>\n\nYou are right.\n\n@David Plowman Do you have a preference\nhere? Should we keep \"json\" syntax\nto match our tuning files? If so, I can rename this to .yaml only. Or\nshould we replace\nthis with full yaml syntax?\n\n\n>\n> > + \"min_unicam_buffers\": 2,\n> > +\n> > + # The minimum total (internal + external) buffer count\n> used for Unicam.\n> > + # The number of internal buffers allocated for Unicam\n> is given by:\n> > + # internal buffer count = max(min_unicam_buffers,\n> > + # min_total_unicam_buffers\n> - external buffer count)\n> > + \"min_total_unicam_buffers\": 4,\n> > +\n> > + # The number of internal buffers used for ISP Output0.\n> This must be set to 1.\n> > + \"num_output0_buffers\": 1\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..232f8b43c5fd\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> > + 'default.json',\n> > +])\n> > +\n> > +install_data(conf_files,\n> > + install_dir : libcamera_datadir / 'pipeline/raspberrypi')\n>\n> Should we add a pipeline_data_dir variable, the same way we have\n> ipa_data_dir ?\n>\n\nGood idea.\n\n\n>\n> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build\n> b/src/libcamera/pipeline/raspberrypi/meson.build\n> > index f1a2f5ee72c2..48235f887501 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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> > @@ -40,6 +41,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 \"dma_heaps.h\"\n> > #include \"rpi_stream.h\"\n> > @@ -313,6 +315,7 @@ public:\n> > };\n> >\n> > Config config_;\n> > + unsigned int numUnicamBuffers;\n> >\n> > private:\n> > void checkRequestCompleted();\n> > @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> *camera, Request *request)\n> > */\n> > stream->setExternalBuffer(buffer);\n> > }\n> > +\n> > + if (!buffer) {\n> > + if (stream == &data->isp_[Isp::Output0] &&\n> !data->config_.numOutput0Buffers) {\n> > + LOG(RPI, Error) << \"Output0 buffer must be\n> provided in the request.\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (stream == &data->unicam_[Unicam::Image] &&\n> !data->numUnicamBuffers) {\n> > + LOG(RPI, Error) << \"Unicam Image buffer\n> must be provided in the request.\";\n> > + return -EINVAL;\n> > + }\n> > + }\n>\n> This seems to belong to a separate patch. Same for the change in\n> PipelineHandlerRPi::prepareBuffers().\n>\n\nThe block above is to validate the Request has the stream buffers required\nbased on the config\nparams read from the file in this commit. If you prefer, I can move this\nand the lower bit of\nvalidation to a separate commit.\n\n\n\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> > @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> > int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > {\n> > RPiCameraData::Config &config = data->config_;\n> > + std::string filename;\n> >\n> > config = {\n> > .minUnicamBuffers = 2,\n> > @@ -1426,6 +1443,49 @@ int\n> PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > .numOutput0Buffers = 1,\n> > };\n> >\n> > + char const *configFromEnv =\n> utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> > + if (!configFromEnv || *configFromEnv == '\\0')\n> > + filename = configurationFile(\"raspberrypi\",\n> \"default.json\");\n>\n> Ideally the first parameter should be deduced from the PipelineHandler\n> subclass automatically, but that can be done later.\n>\n\nI did consider this, and we have PipelineHandler::name() that I thought we\ncould use.\nHowever, the name field returned is based on the factory creation, and the\nstrings are\ngenerated as \"PipelineHandlerRPi\", \"PipelineHandlerIPU3\", etc. Having the\nconfig\nsubdirectory named like that seemed a bit ugly to me.\n\n\n>\n> > + else\n> > + filename = std::string(configFromEnv);\n> > +\n> > + if (filename.empty())\n> > + return -EINVAL;\n>\n> Do we need to require a default configuration file, given that config is\n> initialized to default values above ? I'm even tempted to consider\n> default.json as an example (possibly renamed to config.json.example)\n> only and not try to load it at runtime.\n>\n\nI was a bit unsure of this myself. Happy to remove (or rename) default.json\nand use the struct for the default configuration if others think that is\nbetter.\n\nRegards,\nNaush\n\n\n>\n> > +\n> > + File file(filename);\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> > + std::unique_ptr root = YamlParser::parse(file);\n> > + if (!root) {\n> > + LOG(RPI, Error) << \"Failed to parse configuration file,\n> using defaults!\";\n>\n> No need to shout, you can drop the exclamation mark at the end of all\n> comments.\n>\n\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>\n> > + }\n> > +\n> > + ASSERT((*root)[\"version\"].get() == 1.0);\n>\n> I think this deserves similar treatment as parsing failures.\n>\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> > + config.numOutput0Buffers =\n> > + phConfig[\"num_output0_buffers\"].get int>(config.numOutput0Buffers);\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> > @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera\n> *camera)\n> > */\n> > numBuffers =\n> std::max(data->config_.minUnicamBuffers,\n> > minBuffers -\n> numRawBuffers);\n> > + data->numUnicamBuffers = numBuffers;\n> > } else if (stream == &data->isp_[Isp::Input]) {\n> > /*\n> > * ISP input buffers are imported from Unicam, so\n> follow\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 72FCFBE08B\n\tfor ;\n\tFri, 2 Dec 2022 13:31:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E598B63336;\n\tFri, 2 Dec 2022 14:31:26 +0100 (CET)","from mail-io1-xd2b.google.com (mail-io1-xd2b.google.com\n\t[IPv6:2607:f8b0:4864:20::d2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1AC160483\n\tfor ;\n\tFri, 2 Dec 2022 14:31:24 +0100 (CET)","by mail-io1-xd2b.google.com with SMTP id n188so3050868iof.8\n\tfor ;\n\tFri, 02 Dec 2022 05:31:24 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669987886;\n\tbh=Aa1ecv0SZm/O4k3h5+t9D2v8AKSooCX8dtitLPXM2WM=;\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=zbZnBJAEIpOuccUDT3KGHpEAovcl8P/lGdway2Jmo7EoSGkZmcAvcy9ZCqlOFZmhE\n\tvX6GJptXh2LvAcAJ9ci+VWbS0GIQ0vSpFcMlLCMTb9ACokJworSj/ykEZjycIcJQqm\n\t0AXMPmheNE70v79dSlX8E8MNNqGlnqqH0CW66TlXh/HcdIyd/aVBIyVNJwdislkIVd\n\tHBXU8vzOmdMmOm0wg3m8bnqec5fUuJe8bdyXL43tsMUX9gcr7+vbfDzETl04oGMIvZ\n\tcq/CnjvnFCzk396iugkdUa8CNcHxGuWgFU9UAEEGjTXlGa5/tA5q4VloK6Nr/3mJeU\n\t0DJwT/YqQL+Mw==","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=DlYSa/u5xOfYib8SQsYsdPmFGS2E+zJXI3zeMAXugDY=;\n\tb=CdejfjEB3PIGoysT0JKV/uunVONq/jVH17f/1g0paeH6x6Kw/FWqi2wmsUc8yc6r7W\n\tQ8OqodweZaCZ6rSABKh5VbABnme0A9gyiaSvv0Dg6Uut275QGaQgHZmevIEGUEh9rbaU\n\tXudJERcoj9NSHmm1ghTr/Tjeh+CrpHD5pMJvSrZWyBZBmIU7r4gDU6LXsh4LrTbHkLWH\n\tfZ8zMBswUCEuKQCmVboRcn251lgpM23+QEstgErzMyGCMdg0oqVw4KYFkKbPkmgJSXfA\n\tilqbh9ul694GimrDnIwkkZSiq/NMw1He1NC/Ra7SQbquAqNPHxXlxUIgCzZ+sfU1y24X\n\t3D0g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"CdejfjEB\"; 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=DlYSa/u5xOfYib8SQsYsdPmFGS2E+zJXI3zeMAXugDY=;\n\tb=7N45RsW6WygijwVd/Sv0OkHTqehb1RHPFbOmYcnE/Jx0cg9Q0xfzC1HvIbuIQ9ZgNl\n\tBaiwodfgBExJ9soeJkZ8nOyCF96OqPcg26OGq+we1wqij/hpddXjVP5tXhjge9u8WCIw\n\t8d2f6njuWL9cZm/hXTpTmlG29j34BQ74gjmAD9iCxMSylk5CPgOcqNPBoUY37RyZ6zgF\n\tlwRBdBgrdIi6gjjhoeu9wnr7CkzknNVyqVgrmzE8UBy9M2+5TdmdWFAxk4cHgf1uJoPH\n\tC0n7hhqMd/fjOTIqMRsl6KIRx7QmdcaTarM1bF0e3u3pemFWCODszi965PGAHtq1f3qc\n\tsuvg==","X-Gm-Message-State":"ANoB5plZvyUXMpascAGCVG9sLlokiaTua7VbTW3afvKc5Bh38T3snXzC\n\tbWVkVzWQgY12EA5d8MH2ndLqigwIMUY7TTsxK51oMQ==","X-Google-Smtp-Source":"AA0mqf7Y1ubVv7jm6bYaTzxh41midYbYYYgXMvGSEvztUysJjzCdt81vqXBrr5rbkAIPHM79XTW8dVUsH/tXbOsHwqQ=","X-Received":"by 2002:a05:6638:3786:b0:363:b82d:d510 with SMTP id\n\tw6-20020a056638378600b00363b82dd510mr32637513jal.112.1669987883309;\n\tFri, 02 Dec 2022 05:31:23 -0800 (PST)","MIME-Version":"1.0","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Fri, 2 Dec 2022 13:31:08 +0000","Message-ID":"","To":"Laurent Pinchart , \n\tDavid Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000009074f305eed85a6c\"","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":25976,"web_url":"https://patchwork.libcamera.org/comment/25976/","msgid":"","date":"2022-12-02T13:43:54","subject":"Re: [libcamera-devel] [PATCH v2 04/10] pipeline: raspberrypi: Read\n\tconfig parameters from a file","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush, everyone\n\nOn Fri, 2 Dec 2022 at 13:31, Naushir Patuck wrote:\n>\n> Hi Laurent,\n>\n> Thank you for your feedback!\n>\n> On Fri, 2 Dec 2022 at 12:38, Laurent Pinchart wrote:\n>>\n>> Hi Naush,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via libcamera-devel wrote:\n>> > Read the platform configuration parameters from default.json config file. Use\n>> > the PipelineHandler::configurationFile() helper to determine the full path of\n>> > the file. The default.json filename can be overridden by the user setting the\n>> > LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of the\n>> > custom file.\n>>\n>> How do you envision supporting different configurations for different\n>> cameras (for instance for the cameras connected to the two Unicam\n>> instances on the Raspberry Pi CM) ?\n>\n>\n> Following on from the comments in patch 2, as long as the 2 cameras are\n> running in 2 separate libcamera processes, we can use the LIBCAMERA_RPI_CONFIG_FILE\n> env variable to set different configurations. Longer term, it would be nice to\n> have an API to pass this (as well as a custom IPA turning file) from the application.\n>\n>>\n>>\n>> > Add config parameter validation to ensure bad parameters cannot cause the\n>> > pipeline handler to stop working.\n>> >\n>> > Currently three 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>> > \"num_output0_buffers\"\n>> > Number of internal ISP Output0 buffers to allocate.\n>> >\n>> > Signed-off-by: Naushir Patuck \n>> > Reviewed-by: David Plowman \n>> > ---\n>> > .../pipeline/raspberrypi/data/default.json | 20 ++++++\n>> > .../pipeline/raspberrypi/data/meson.build | 8 +++\n>> > .../pipeline/raspberrypi/meson.build | 2 +\n>> > .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++\n>> > 4 files changed, 91 insertions(+)\n>> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json\n>> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n>> >\n>> > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n>> > new file mode 100644\n>> > index 000000000000..d709e31850af\n>> > --- /dev/null\n>> > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\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 Unicam.\n>> > + # This value must be greater than 0, but less than or equal to min_total_unicam_buffers.\n>>\n>> Could you please wrap lines to 80 columns where possible ?\n>>\n>> Comments are not allowed by JSON. This works only because we use a YAML\n>> person that doesn't enforce strict JSON compliance. I would thus name\n>> the file with a .yaml extension, and possibly convert it to the native\n>> YAML syntax. You can then add an SPDX license header.\n>\n>\n> You are right.\n>\n> @David Plowman Do you have a preference here? Should we keep \"json\" syntax\n> to match our tuning files? If so, I can rename this to .yaml only. Or should we replace\n> this with full yaml syntax?\n\nI must confess I can't get too worked up about this either way. I\ndon't really see people editing these files like they might our tuning\nfiles (they'll all just use our standard libcamera-apps/Picamera2\none), so I wouldn't be averse to going full YAML. I certainly like\nhaving comments.\n\nDavid\n\n>\n>>\n>>\n>> > + \"min_unicam_buffers\": 2,\n>> > +\n>> > + # The minimum total (internal + external) buffer count used for Unicam.\n>> > + # The number of internal buffers allocated for Unicam is given by:\n>> > + # internal buffer count = max(min_unicam_buffers,\n>> > + # min_total_unicam_buffers - external buffer count)\n>> > + \"min_total_unicam_buffers\": 4,\n>> > +\n>> > + # The number of internal buffers used for ISP Output0. This must be set to 1.\n>> > + \"num_output0_buffers\": 1\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..232f8b43c5fd\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>> > + 'default.json',\n>> > +])\n>> > +\n>> > +install_data(conf_files,\n>> > + install_dir : libcamera_datadir / 'pipeline/raspberrypi')\n>>\n>> Should we add a pipeline_data_dir variable, the same way we have\n>> ipa_data_dir ?\n>\n>\n> Good idea.\n>\n>>\n>>\n>> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n>> > index f1a2f5ee72c2..48235f887501 100644\n>> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n>> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n>> > @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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>> > @@ -40,6 +41,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 \"dma_heaps.h\"\n>> > #include \"rpi_stream.h\"\n>> > @@ -313,6 +315,7 @@ public:\n>> > };\n>> >\n>> > Config config_;\n>> > + unsigned int numUnicamBuffers;\n>> >\n>> > private:\n>> > void checkRequestCompleted();\n>> > @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n>> > */\n>> > stream->setExternalBuffer(buffer);\n>> > }\n>> > +\n>> > + if (!buffer) {\n>> > + if (stream == &data->isp_[Isp::Output0] && !data->config_.numOutput0Buffers) {\n>> > + LOG(RPI, Error) << \"Output0 buffer must be provided in the request.\";\n>> > + return -EINVAL;\n>> > + }\n>> > +\n>> > + if (stream == &data->unicam_[Unicam::Image] && !data->numUnicamBuffers) {\n>> > + LOG(RPI, Error) << \"Unicam Image buffer must be provided in the request.\";\n>> > + return -EINVAL;\n>> > + }\n>> > + }\n>>\n>> This seems to belong to a separate patch. Same for the change in\n>> PipelineHandlerRPi::prepareBuffers().\n>\n>\n> The block above is to validate the Request has the stream buffers required based on the config\n> params read from the file in this commit. If you prefer, I can move this and the lower bit of\n> validation to a separate commit.\n>\n>\n>>\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>> > @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>> > int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n>> > {\n>> > RPiCameraData::Config &config = data->config_;\n>> > + std::string filename;\n>> >\n>> > config = {\n>> > .minUnicamBuffers = 2,\n>> > @@ -1426,6 +1443,49 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n>> > .numOutput0Buffers = 1,\n>> > };\n>> >\n>> > + char const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n>> > + if (!configFromEnv || *configFromEnv == '\\0')\n>> > + filename = configurationFile(\"raspberrypi\", \"default.json\");\n>>\n>> Ideally the first parameter should be deduced from the PipelineHandler\n>> subclass automatically, but that can be done later.\n>\n>\n> I did consider this, and we have PipelineHandler::name() that I thought we could use.\n> However, the name field returned is based on the factory creation, and the strings are\n> generated as \"PipelineHandlerRPi\", \"PipelineHandlerIPU3\", etc. Having the config\n> subdirectory named like that seemed a bit ugly to me.\n>\n>>\n>>\n>> > + else\n>> > + filename = std::string(configFromEnv);\n>> > +\n>> > + if (filename.empty())\n>> > + return -EINVAL;\n>>\n>> Do we need to require a default configuration file, given that config is\n>> initialized to default values above ? I'm even tempted to consider\n>> default.json as an example (possibly renamed to config.json.example)\n>> only and not try to load it at runtime.\n>\n>\n> I was a bit unsure of this myself. Happy to remove (or rename) default.json\n> and use the struct for the default configuration if others think that is better.\n>\n> Regards,\n> Naush\n>\n>>\n>>\n>> > +\n>> > + File file(filename);\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>>\n>> No need to shout, you can drop the exclamation mark at the end of all\n>> comments.\n>>\n>>\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>>\n>> > + }\n>> > +\n>> > + ASSERT((*root)[\"version\"].get() == 1.0);\n>>\n>> I think this deserves similar treatment as parsing failures.\n>>\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>> > + config.numOutput0Buffers =\n>> > + phConfig[\"num_output0_buffers\"].get(config.numOutput0Buffers);\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>> > @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>> > */\n>> > numBuffers = std::max(data->config_.minUnicamBuffers,\n>> > minBuffers - numRawBuffers);\n>> > + data->numUnicamBuffers = numBuffers;\n>> > } else if (stream == &data->isp_[Isp::Input]) {\n>> > /*\n>> > * ISP input buffers are imported from Unicam, so follow\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 EA428BDE6B\n\tfor ;\n\tFri, 2 Dec 2022 13:44:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 548C86333F;\n\tFri, 2 Dec 2022 14:44:09 +0100 (CET)","from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com\n\t[IPv6:2607:f8b0:4864:20::102a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A78560483\n\tfor ;\n\tFri, 2 Dec 2022 14:44:07 +0100 (CET)","by mail-pj1-x102a.google.com with SMTP id q15so3910577pja.0\n\tfor ;\n\tFri, 02 Dec 2022 05:44:07 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669988649;\n\tbh=yz4jon/bXn77F3tgjZMYw5TrApyNV2vsAU1Z1Bdl4/k=;\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=IXntnvlYidrtYEyTbs60Wn6rLoDhVOaxtoDL8fcyTh9rWtsMT2gUi9juYAIrDh3Q6\n\tpUzc0rwucdPJKNpExstrdtb7GyF9M7AmDDIWrLqw7LSmFbhwMdj82HaTsKP7ita7gw\n\tyxAI5shcurvNodgcf8Xl9+fNXVzVLST30aMDQfjb1FVqXOL551vZIlIva6C2/BrjBX\n\tUIK2CzFMLVof3+kN/jWSUZ8fJQEB/Sd+fUOzO75ZuOOiC2+KO88Krh2HqbRvEdHYo1\n\tno0NHbZsdznX2mayiyXTMbBLe2G+Z2c0DHiNnq9+uLcdhGL+81mc8DSwS5HnwR1XKQ\n\t+HdH3v3C2Yf0A==","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=Bqguym0MhH/TbxL0V56qeGH5dBkiqOBqA9KuBb6DyqI=;\n\tb=OZNXS9Rr7aNA+MNy+0VX4/hNlugocqZb7CGMI63wrqLvQ5LLrHNjHSZpK7Tu0aiQCL\n\tUsa5y6WtXpm4g4JwhMGxRJSlsbkoHbXGAjhb5piu61SloeEsF5f2Hf5NwKB9W0brKW56\n\t8XhOveK33RXVTeTuR0/Sr7J2bDWsSP1ycBg7AK3I403kIp4ef5i1bvSw7MXnzaTIC3Hv\n\tyWS+kaVC1uDVQj01yAu4IWxcjDcc7JXeBPYgm2tyq+pivIiffMJ2Yiu1FxT5ETFDHGTh\n\tNY/9Ihx+qUKuFLVQ3SrviG7deJqwtTGtzf34cX9+LBkOhDuVyXJFyT+Z+BFHP7HqWY2h\n\tPXiA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"OZNXS9Rr\"; 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=Bqguym0MhH/TbxL0V56qeGH5dBkiqOBqA9KuBb6DyqI=;\n\tb=4nyvRzcgCWSNajSDgQl8Op2Q8NkxVidf9ahUoqvMkuyP0HISZ4bJb2RtIXjvVuQwMz\n\tDG1SftwbRjVcmef3v5VcT36r+WZ9uixXfahQAhs12DIhfZWmlYeIdvyPz8Dp+ad6DvDk\n\tGkyl5ngV0BKJVEF4ko4gBjU8W2f52VpU4nFAvHAbP1Xh29b1RKQvcwJ+kztDTcmZ+TE+\n\tXkaB3Km/KArrUS4mbFDXiUwl+xHBvKpIRdcJN89VTYB9kQEx03OonTxthkRZC4/zNY7Q\n\tv2TTpwf/baJCFPZFKST3IWQhxg7m1tOy2pCC70eQ8GjB8aZ72Ez2RahOjqrvv+0mJ/pO\n\tCkPw==","X-Gm-Message-State":"ANoB5pkq6wAGKaJUPB3amPf8JKEknpYidLD8FkibQHZyep2rJhEOdAs8\n\tAIDwudWhF3VRcMSyjRput3xu6L9xmaE3OOI7gBFlgubLeskiSw==","X-Google-Smtp-Source":"AA0mqf4nGmADIKzkEGmTizIebVEURNybE1lJv3LPVWl5CFR2k05TEk2Srpl/4/3hp201m2jQyKzFDfm146WepQ/evEc=","X-Received":"by 2002:a17:902:d4c8:b0:186:9d71:228c with SMTP id\n\to8-20020a170902d4c800b001869d71228cmr54610879plg.109.1669988645302;\n\tFri, 02 Dec 2022 05:44:05 -0800 (PST)","MIME-Version":"1.0","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-5-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","Date":"Fri, 2 Dec 2022 13:43:54 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25989,"web_url":"https://patchwork.libcamera.org/comment/25989/","msgid":"","date":"2022-12-02T16:19:03","subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":"Hi Naush,\n\nOn Fri, Dec 02, 2022 at 01:31:08PM +0000, Naushir Patuck wrote:\n> On Fri, 2 Dec 2022 at 12:38, Laurent Pinchart wrote:\n> > On Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > > Read the platform configuration parameters from default.json config file. Use\n> > > the PipelineHandler::configurationFile() helper to determine the full path of\n> > > the file. The default.json filename can be overridden by the user setting the\n> > > LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of the\n> > > custom file.\n> >\n> > How do you envision supporting different configurations for different\n> > cameras (for instance for the cameras connected to the two Unicam\n> > instances on the Raspberry Pi CM) ?\n> \n> Following on from the comments in patch 2, as long as the 2 cameras are\n> running in 2 separate libcamera processes, we can use the LIBCAMERA_RPI_CONFIG_FILE\n> env variable to set different configurations. Longer term, it would be nice to\n> have an API to pass this (as well as a custom IPA turning file) from the\n> application.\n\nOK, works for me. I'm sure we'll improve all that in the future, it's\ngood enough for now.\n\n> > > Add config parameter validation to ensure bad parameters cannot cause the\n> > > pipeline handler to stop working.\n> > >\n> > > Currently three 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> > > \"num_output0_buffers\"\n> > > Number of internal ISP Output0 buffers to allocate.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > .../pipeline/raspberrypi/data/default.json | 20 ++++++\n> > > .../pipeline/raspberrypi/data/meson.build | 8 +++\n> > > .../pipeline/raspberrypi/meson.build | 2 +\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++\n> > > 4 files changed, 91 insertions(+)\n> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json\n> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > new file mode 100644\n> > > index 000000000000..d709e31850af\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\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 Unicam.\n> > > + # This value must be greater than 0, but less than or equal to min_total_unicam_buffers.\n> >\n> > Could you please wrap lines to 80 columns where possible ?\n> >\n> > Comments are not allowed by JSON. This works only because we use a YAML\n> > person that doesn't enforce strict JSON compliance. I would thus name\n> > the file with a .yaml extension, and possibly convert it to the native\n> > YAML syntax. You can then add an SPDX license header.\n> \n> You are right.\n> \n> @David Plowman Do you have a preference\n> here? Should we keep \"json\" syntax\n> to match our tuning files? If so, I can rename this to .yaml only. Or\n> should we replace\n> this with full yaml syntax?\n> \n> > > + \"min_unicam_buffers\": 2,\n> > > +\n> > > + # The minimum total (internal + external) buffer count used for Unicam.\n> > > + # The number of internal buffers allocated for Unicam is given by:\n> > > + # internal buffer count = max(min_unicam_buffers,\n> > > + # min_total_unicam_buffers - external buffer count)\n> > > + \"min_total_unicam_buffers\": 4,\n> > > +\n> > > + # The number of internal buffers used for ISP Output0. This must be set to 1.\n> > > + \"num_output0_buffers\": 1\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..232f8b43c5fd\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> > > + 'default.json',\n> > > +])\n> > > +\n> > > +install_data(conf_files,\n> > > + install_dir : libcamera_datadir / 'pipeline/raspberrypi')\n> >\n> > Should we add a pipeline_data_dir variable, the same way we have\n> > ipa_data_dir ?\n> \n> Good idea.\n> \n> > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > index f1a2f5ee72c2..48235f887501 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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> > > @@ -40,6 +41,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 \"dma_heaps.h\"\n> > > #include \"rpi_stream.h\"\n> > > @@ -313,6 +315,7 @@ public:\n> > > };\n> > >\n> > > Config config_;\n> > > + unsigned int numUnicamBuffers;\n> > >\n> > > private:\n> > > void checkRequestCompleted();\n> > > @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > */\n> > > stream->setExternalBuffer(buffer);\n> > > }\n> > > +\n> > > + if (!buffer) {\n> > > + if (stream == &data->isp_[Isp::Output0] && !data->config_.numOutput0Buffers) {\n> > > + LOG(RPI, Error) << \"Output0 buffer must be provided in the request.\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + if (stream == &data->unicam_[Unicam::Image] && !data->numUnicamBuffers) {\n> > > + LOG(RPI, Error) << \"Unicam Image buffer must be provided in the request.\";\n> > > + return -EINVAL;\n> > > + }\n> > > + }\n> >\n> > This seems to belong to a separate patch. Same for the change in\n> > PipelineHandlerRPi::prepareBuffers().\n> \n> The block above is to validate the Request has the stream buffers required based on the config\n> params read from the file in this commit. If you prefer, I can move this and the lower bit of\n> validation to a separate commit.\n\nI would prefer that, with an explanation of the validation in the commit\nmessage. You can move this change before this patch, as\nConfig::numOutput0Buffers already exists.\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> > > @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > > {\n> > > RPiCameraData::Config &config = data->config_;\n> > > + std::string filename;\n> > >\n> > > config = {\n> > > .minUnicamBuffers = 2,\n> > > @@ -1426,6 +1443,49 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > > .numOutput0Buffers = 1,\n> > > };\n> > >\n> > > + char const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> > > + if (!configFromEnv || *configFromEnv == '\\0')\n> > > + filename = configurationFile(\"raspberrypi\", \"default.json\");\n> >\n> > Ideally the first parameter should be deduced from the PipelineHandler\n> > subclass automatically, but that can be done later.\n> \n> I did consider this, and we have PipelineHandler::name() that I thought we could use.\n> However, the name field returned is based on the factory creation, and the strings are\n> generated as \"PipelineHandlerRPi\", \"PipelineHandlerIPU3\", etc. Having the config\n> subdirectory named like that seemed a bit ugly to me.\n\nI agree. We'll improve this later.\n\n> > > + else\n> > > + filename = std::string(configFromEnv);\n> > > +\n> > > + if (filename.empty())\n> > > + return -EINVAL;\n> >\n> > Do we need to require a default configuration file, given that config is\n> > initialized to default values above ? I'm even tempted to consider\n> > default.json as an example (possibly renamed to config.json.example)\n> > only and not try to load it at runtime.\n> \n> I was a bit unsure of this myself. Happy to remove (or rename) default.json\n> and use the struct for the default configuration if others think that is\n> better.\n\nI think I would prefer renaming (or removing) it. When using the default\nconfiguration, parsing the file is wasting CPU cycles.\n\n> > > +\n> > > + File file(filename);\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> >\n> > No need to shout, you can drop the exclamation mark at the end of all\n> > comments.\n> >\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> >\n> > > + }\n> > > +\n> > > + ASSERT((*root)[\"version\"].get() == 1.0);\n> >\n> > I think this deserves similar treatment as parsing failures.\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> > > + config.numOutput0Buffers =\n> > > + phConfig[\"num_output0_buffers\"].get(config.numOutput0Buffers);\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> > > @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > */\n> > > numBuffers = std::max(data->config_.minUnicamBuffers,\n> > > minBuffers - numRawBuffers);\n> > > + data->numUnicamBuffers = numBuffers;\n> > > } else if (stream == &data->isp_[Isp::Input]) {\n> > > /*\n> > > * ISP input buffers are imported from Unicam, so follow","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 C6ACCBDE6B\n\tfor ;\n\tFri, 2 Dec 2022 16:19:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3596E63336;\n\tFri, 2 Dec 2022 17:19:06 +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 5FD7860483\n\tfor ;\n\tFri, 2 Dec 2022 17:19:05 +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 A9C296E0;\n\tFri, 2 Dec 2022 17:19:04 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669997946;\n\tbh=j4Hv61/E8cNTuew96u0WVRNv9Nu7ImaWWh/121QGpCk=;\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=mHFzNOuKg8eyeAnAugGR2zxN96qSGGmkTxgkOHO0Rwquit6cumHuyHPGpx+bdkSpO\n\tYFwEvleqSfpT3Qo+ljkSlUe5Td38+enhIjbdpgiX6/mbM21sZX5ViQyO96TpGnsDUW\n\tUJsP3U8S6XGmvRANbGi4pgSOcalf4YZQOg8aDT1kOBBljjtQ/Atnu66H3v9mAIXZy0\n\t/qBbyDimwSQ7elA28XuNpvksnBQwDH1ZctYY0MS3y59kP5VkCseGzM2fL/vVXCivAg\n\tmAGsu2cKcdUHSyYfcSlfMgp6TPg+H/J/UBTjbaVMwsBzyKGubg4Q+je4BzZmo0Lrv/\n\to5SlMmBq8/8gw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669997944;\n\tbh=j4Hv61/E8cNTuew96u0WVRNv9Nu7ImaWWh/121QGpCk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=koO/oagljXgOkekk6+AJ3awLh0U+jnahXfH248+SjsSwuyBcTKwKreAISiPbfckam\n\t69LjWloJ5LVC491EqM3CNK4p7ji3o55BkLQomUafIsYYvr6228oBTIfrsoFHasNvz6\n\tzL/J9ldLO73pKEGINe3MB6Q+pa7NoxCz6tSsGkrM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"koO/oagl\"; dkim-atps=neutral","Date":"Fri, 2 Dec 2022 18:19:03 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-5-naush@raspberrypi.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 v2 04/10] 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":25994,"web_url":"https://patchwork.libcamera.org/comment/25994/","msgid":"","date":"2022-12-02T20:44:53","subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":"Hi Naush,\n\nIt's me again :-) Please see below for another comment.\n\nOn Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Read the platform configuration parameters from default.json config file. Use\n> the PipelineHandler::configurationFile() helper to determine the full path of\n> the file. The default.json filename can be overridden by the user setting the\n> LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of the\n> custom file.\n> \n> Add config parameter validation to ensure bad parameters cannot cause the\n> pipeline handler to stop working.\n> \n> Currently three 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> \"num_output0_buffers\"\n> Number of internal ISP Output0 buffers to allocate.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/data/default.json | 20 ++++++\n> .../pipeline/raspberrypi/data/meson.build | 8 +++\n> .../pipeline/raspberrypi/meson.build | 2 +\n> .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++\n> 4 files changed, 91 insertions(+)\n> create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json\n> create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n> new file mode 100644\n> index 000000000000..d709e31850af\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\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 Unicam.\n> + # This value must be greater than 0, but less than or equal to min_total_unicam_buffers.\n> + \"min_unicam_buffers\": 2,\n> +\n> + # The minimum total (internal + external) buffer count used for Unicam.\n> + # The number of internal buffers allocated for Unicam is given by:\n> + # internal buffer count = max(min_unicam_buffers,\n> + # min_total_unicam_buffers - external buffer count)\n> + \"min_total_unicam_buffers\": 4,\n> + \n> + # The number of internal buffers used for ISP Output0. This must be set to 1.\n> + \"num_output0_buffers\": 1\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..232f8b43c5fd\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> + 'default.json',\n> +])\n> +\n> +install_data(conf_files,\n> + install_dir : libcamera_datadir / 'pipeline/raspberrypi')\n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index f1a2f5ee72c2..48235f887501 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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> @@ -40,6 +41,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 \"dma_heaps.h\"\n> #include \"rpi_stream.h\"\n> @@ -313,6 +315,7 @@ public:\n> \t};\n> \n> \tConfig config_;\n> +\tunsigned int numUnicamBuffers;\n> \n> private:\n> \tvoid checkRequestCompleted();\n> @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \t\t\t */\n> \t\t\tstream->setExternalBuffer(buffer);\n> \t\t}\n> +\n> +\t\tif (!buffer) {\n> +\t\t\tif (stream == &data->isp_[Isp::Output0] && !data->config_.numOutput0Buffers) {\n> +\t\t\t\tLOG(RPI, Error) << \"Output0 buffer must be provided in the request.\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\n> +\t\t\tif (stream == &data->unicam_[Unicam::Image] && !data->numUnicamBuffers) {\n> +\t\t\t\tLOG(RPI, Error) << \"Unicam Image buffer must be provided in the request.\";\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n\nThe numOutput0Buffers and minUnicamBuffers configuration parameters seem\nto me to be a contract between the application and libcamera, where\nsetting them to 0 requires the application to always pass buffers for\nthe corresponding streams. As we discussed previously, while I'm fine\nwith the concept of a configuration file to tune the behaviour of the\ncamera, I'm bothered by conveing information about such an API contract\nthrough a file.\n\nCould we instead add a hint to the StreamConfiguration class to let\napplications tell about their side of the contract ? Something along the\nlines of\n\ndiff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\nindex 29235ddf0d8a..616f6dd174f1 100644\n--- a/include/libcamera/stream.h\n+++ b/include/libcamera/stream.h\n@@ -13,6 +13,8 @@\n #include \n #include \n\n+#include \n+\n #include \n #include \n #include \n@@ -39,6 +41,13 @@ private:\n };\n\n struct StreamConfiguration {\n+\tenum class Hint {\n+\t\tNone = 0,\n+\t\tMandatoryBuffer = (1 << 0),\n+\t};\n+\n+\tusing Hints = Flags;\n+\n \tStreamConfiguration();\n \tStreamConfiguration(const StreamFormats &formats);\n\n@@ -50,6 +59,7 @@ struct StreamConfiguration {\n \tunsigned int bufferCount;\n\n \tstd::optional colorSpace;\n+\tHints hints;\n\n \tStream *stream() const { return stream_; }\n \tvoid setStream(Stream *stream) { stream_ = stream; }\n\n(names to be bikeshedded of course)\n\n> +\t\t}\n> +\n> \t\t/*\n> \t\t * If no buffer is provided by the request for this stream, we\n> \t\t * queue a nullptr to the stream to signify that it must use an\n> @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> {\n> \tRPiCameraData::Config &config = data->config_;\n> +\tstd::string filename;\n> \n> \tconfig = {\n> \t\t.minUnicamBuffers = 2,\n> @@ -1426,6 +1443,49 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> \t\t.numOutput0Buffers = 1,\n> \t};\n> \n> +\tchar const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> +\tif (!configFromEnv || *configFromEnv == '\\0')\n> +\t\tfilename = configurationFile(\"raspberrypi\", \"default.json\");\n> +\telse\n> +\t\tfilename = std::string(configFromEnv);\n> +\n> +\tif (filename.empty())\n> +\t\treturn -EINVAL;\n> +\n> +\tFile file(filename);\n> +\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> +\t\tLOG(RPI, Error) << \"Failed to open configuration file '\" << filename << \"'\";\n> +\t\treturn -EIO;\n> +\t}\n> +\n> +\tLOG(RPI, Info) << \"Using configuration file '\" << filename << \"'\";\n> +\n> +\tstd::unique_ptr root = YamlParser::parse(file);\n> +\tif (!root) {\n> +\t\tLOG(RPI, Error) << \"Failed to parse configuration file, using defaults!\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tASSERT((*root)[\"version\"].get() == 1.0);\n> +\n> +\tconst YamlObject &phConfig = (*root)[\"pipeline_handler\"];\n> +\tconfig.minUnicamBuffers =\n> +\t\tphConfig[\"min_unicam_buffers\"].get(config.minUnicamBuffers);\n> +\tconfig.minTotalUnicamBuffers =\n> +\t\tphConfig[\"min_total_unicam_buffers\"].get(config.minTotalUnicamBuffers);\n> +\tconfig.numOutput0Buffers =\n> +\t\tphConfig[\"num_output0_buffers\"].get(config.numOutput0Buffers);\n> +\n> +\tif (config.minTotalUnicamBuffers < config.minUnicamBuffers) {\n> +\t\tLOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers!\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (config.minTotalUnicamBuffers < 1) {\n> +\t\tLOG(RPI, Error) << \"Invalid configuration: min_total_unicam_buffers must be >= 1!\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t\t\t */\n> \t\t\tnumBuffers = std::max(data->config_.minUnicamBuffers,\n> \t\t\t\t\t\t minBuffers - numRawBuffers);\n> +\t\t\tdata->numUnicamBuffers = numBuffers;\n> \t\t} else if (stream == &data->isp_[Isp::Input]) {\n> \t\t\t/*\n> \t\t\t * ISP input buffers are imported from Unicam, so follow","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 79355BDE6B\n\tfor ;\n\tFri, 2 Dec 2022 20:44:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C80E863336;\n\tFri, 2 Dec 2022 21:44:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D616C60483\n\tfor ;\n\tFri, 2 Dec 2022 21:44:55 +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 47E12151A;\n\tFri, 2 Dec 2022 21:44:55 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670013897;\n\tbh=xOJh/cYiNUaxshpLC2Xrrrw16O/8Q5WJD66IygJSCkY=;\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=BftdSZlewFEK3zLds2OgymiHBxPDZQ3PZFRkKvp+TiN/N4G3ZUNVl5gpVzlF24Nbm\n\tulygX08ae7HQ/LsTaugVu5X7+QCzZJghN7mYOtERirmdjQZJAUYP1dMjjFPsolLRgd\n\to99cx4/F1/F9YTqPXzM0gLZMXVcglkl3FYpBif/9HwRh3LeZCYMTjNIHKQRmn2B2F2\n\tSkfWCicySks1u0wPIXdXe9nPkRzaASYduFFpsVduy5P9u6Fpp9irfjqXM16VSnZ8hb\n\tYwfDnfqwwVoC91Nz37p0tApGGnRAldnR5MERFS1wS/q2CzcE+1MdKMALS0qufu79qj\n\tPFiFPRuMaeb/A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670013895;\n\tbh=xOJh/cYiNUaxshpLC2Xrrrw16O/8Q5WJD66IygJSCkY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MJ57o6jOTWJJZGsxi22Kg1aF/iqnASVtnWviRPlehDftA4vFekAl55CyEP6ATJQyz\n\tNKraX6U18tsUHGou7eUpshz/FiIQUKxswYn7vP9Hzt8MqHa6KXz5QL1Q/ZH5rc2rw3\n\t5off02VROdFLKlukD6OYJ1QBCClqaYfuQGp6Va5U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MJ57o6jO\"; dkim-atps=neutral","Date":"Fri, 2 Dec 2022 22:44:53 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221129134534.2933-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":25999,"web_url":"https://patchwork.libcamera.org/comment/25999/","msgid":"","date":"2022-12-05T09:20:18","subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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 Laurent,\n\nOn Fri, 2 Dec 2022 at 20:44, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> It's me again :-) Please see below for another comment.\n>\n> On Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > Read the platform configuration parameters from default.json config\n> file. Use\n> > the PipelineHandler::configurationFile() helper to determine the full\n> path of\n> > the file. The default.json filename can be overridden by the user\n> setting the\n> > LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of\n> the\n> > custom file.\n> >\n> > Add config parameter validation to ensure bad parameters cannot cause the\n> > pipeline handler to stop working.\n> >\n> > Currently three 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> > \"num_output0_buffers\"\n> > Number of internal ISP Output0 buffers to allocate.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/data/default.json | 20 ++++++\n> > .../pipeline/raspberrypi/data/meson.build | 8 +++\n> > .../pipeline/raspberrypi/meson.build | 2 +\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++\n> > 4 files changed, 91 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json\n> b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > new file mode 100644\n> > index 000000000000..d709e31850af\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\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 Unicam.\n> > + # 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> > + # The minimum total (internal + external) buffer count\n> used for Unicam.\n> > + # The number of internal buffers allocated for Unicam\n> is given by:\n> > + # internal buffer count = max(min_unicam_buffers,\n> > + # min_total_unicam_buffers\n> - external buffer count)\n> > + \"min_total_unicam_buffers\": 4,\n> > +\n> > + # The number of internal buffers used for ISP Output0.\n> This must be set to 1.\n> > + \"num_output0_buffers\": 1\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..232f8b43c5fd\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> > + 'default.json',\n> > +])\n> > +\n> > +install_data(conf_files,\n> > + install_dir : libcamera_datadir / 'pipeline/raspberrypi')\n> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build\n> b/src/libcamera/pipeline/raspberrypi/meson.build\n> > index f1a2f5ee72c2..48235f887501 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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> > @@ -40,6 +41,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 \"dma_heaps.h\"\n> > #include \"rpi_stream.h\"\n> > @@ -313,6 +315,7 @@ public:\n> > };\n> >\n> > Config config_;\n> > + unsigned int numUnicamBuffers;\n> >\n> > private:\n> > void checkRequestCompleted();\n> > @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> *camera, Request *request)\n> > */\n> > stream->setExternalBuffer(buffer);\n> > }\n> > +\n> > + if (!buffer) {\n> > + if (stream == &data->isp_[Isp::Output0] &&\n> !data->config_.numOutput0Buffers) {\n> > + LOG(RPI, Error) << \"Output0 buffer must be\n> provided in the request.\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + if (stream == &data->unicam_[Unicam::Image] &&\n> !data->numUnicamBuffers) {\n> > + LOG(RPI, Error) << \"Unicam Image buffer\n> must be provided in the request.\";\n> > + return -EINVAL;\n> > + }\n>\n> The numOutput0Buffers and minUnicamBuffers configuration parameters seem\n> to me to be a contract between the application and libcamera, where\n> setting them to 0 requires the application to always pass buffers for\n> the corresponding streams. As we discussed previously, while I'm fine\n> with the concept of a configuration file to tune the behaviour of the\n> camera, I'm bothered by conveing information about such an API contract\n> through a file.\n>\n> Could we instead add a hint to the StreamConfiguration class to let\n> applications tell about their side of the contract ? Something along the\n> lines of\n>\n\nSure, I'll prototype something along these lines for the next revision of\nthe\nseries.\n\nRegards,\nNaush\n\n\n>\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 29235ddf0d8a..616f6dd174f1 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -13,6 +13,8 @@\n> #include \n> #include \n>\n> +#include \n> +\n> #include \n> #include \n> #include \n> @@ -39,6 +41,13 @@ private:\n> };\n>\n> struct StreamConfiguration {\n> + enum class Hint {\n> + None = 0,\n> + MandatoryBuffer = (1 << 0),\n> + };\n> +\n> + using Hints = Flags;\n> +\n> StreamConfiguration();\n> StreamConfiguration(const StreamFormats &formats);\n>\n> @@ -50,6 +59,7 @@ struct StreamConfiguration {\n> unsigned int bufferCount;\n>\n> std::optional colorSpace;\n> + Hints hints;\n>\n> Stream *stream() const { return stream_; }\n> void setStream(Stream *stream) { stream_ = stream; }\n>\n> (names to be bikeshedded of course)\n>\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> > @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> > int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > {\n> > RPiCameraData::Config &config = data->config_;\n> > + std::string filename;\n> >\n> > config = {\n> > .minUnicamBuffers = 2,\n> > @@ -1426,6 +1443,49 @@ int\n> PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > .numOutput0Buffers = 1,\n> > };\n> >\n> > + char const *configFromEnv =\n> utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> > + if (!configFromEnv || *configFromEnv == '\\0')\n> > + filename = configurationFile(\"raspberrypi\",\n> \"default.json\");\n> > + else\n> > + filename = std::string(configFromEnv);\n> > +\n> > + if (filename.empty())\n> > + return -EINVAL;\n> > +\n> > + File file(filename);\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> > + 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> > + ASSERT((*root)[\"version\"].get() == 1.0);\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> > + config.numOutput0Buffers =\n> > + phConfig[\"num_output0_buffers\"].get int>(config.numOutput0Buffers);\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> > @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera\n> *camera)\n> > */\n> > numBuffers =\n> std::max(data->config_.minUnicamBuffers,\n> > minBuffers -\n> numRawBuffers);\n> > + data->numUnicamBuffers = numBuffers;\n> > } else if (stream == &data->isp_[Isp::Input]) {\n> > /*\n> > * ISP input buffers are imported from Unicam, so\n> follow\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 D3E6DBDE6B\n\tfor ;\n\tMon, 5 Dec 2022 09:20:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4653C6333B;\n\tMon, 5 Dec 2022 10:20:36 +0100 (CET)","from mail-il1-x136.google.com (mail-il1-x136.google.com\n\t[IPv6:2607:f8b0:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F4BC63335\n\tfor ;\n\tMon, 5 Dec 2022 10:20:33 +0100 (CET)","by mail-il1-x136.google.com with SMTP id d14so4822817ilq.11\n\tfor ;\n\tMon, 05 Dec 2022 01:20:33 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670232036;\n\tbh=LImfuJUvzX3TKhQJbf9JeUEuE13J6US0BgMFHQgY9Xs=;\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=JLylgzf64Xemj6nmD1qZKqQsK37U1m173CHb16142CTOBb4WFWxqCKcKRZB+ToKKg\n\ty4N8guqEoMF0q4YYmjMikAdr76kkX374ZbCaJvHuvoLRL0AvNRDTHOSR6yVbEiWoaO\n\t+MNg5jR9DPqLfUSI/AA0qxfHVLcgOQmSCPfCd6l4JFza1FcGl2QdQtaLA+eepNTOR9\n\tLw/8GAjxpzpd9WBHBpx3O+ifiQRwpS4uBUvNbpN/gMH+PLSW/hW9pKaSwXT54drM4p\n\toNPV0dgTRk8oeWLQxUnqx+/TaigBw/fhTz1AnIDSM3nSy259iCBAe0p4M6m1T/psDE\n\tEyer6IWsRGr7w==","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=GUnh0b/JymVDizdzfkjhDuT0BCYksMTZAqP09n9XEcM=;\n\tb=WU//4IbnPlc8HaOIz/vGgcNn8q7NeCYHlldM10syk1A0l+aDaXQG3SBw7peIuj2N0f\n\tl0FIjt8p1KRUNmAINGp6MdjbN3UEApIV1UzOQ6PgizcPwTh7S+wuHDDEA8Yp9NtwKMKh\n\tu+0CVuBw07xjiojcOZ3irn6wv3Bxrspvx9ZzxQ9cgXOyF5EOH2NrOJQfUna7m94mUF1n\n\txxJbzZNcuLbhbr0SZULonhROu5XZOLSmIWMmmnqyRmY0Z0kpJEwoDW91fj1nag2p8mQd\n\tPqrvZrzrVGi5Q5V5WJUbcNT5dnaDqEPu1qiKzUjPZMeumrjzTNrRPwHR7O4IBIieuwfT\n\t/V6Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"WU//4Ibn\"; 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=GUnh0b/JymVDizdzfkjhDuT0BCYksMTZAqP09n9XEcM=;\n\tb=T7BUmXIycjMheDF5M5DesRsk8C08HWf8P29+THh1BDb1tpnqq5GNa0Nn1Khcp70Zjm\n\tjLZZ3kxmt71KjAArFcAAU3tjoplxMk5ki7khWNsn68ye0LMykWNqKU1KdzE4vtVKFnJy\n\tSiORG0nBl9UIi4IS8q4D830nX/FUS+gCAlgPZVDEgLA6IwT6lq+fDmd3MWJYJvKK//4W\n\t41/1rLsOQejHy00MG6kZvtexg0mvvh/6sLIh7dWcOdkATO2EMQedMjR+2dOG1bprSTZF\n\t+0kZN9q1eU5lHQlvK5CRlwDWaiEWG0PMT3KpX2hgN4MnAZEKfYUIxeGbdOJww/iKV0bS\n\tws6g==","X-Gm-Message-State":"ANoB5pnSUiD9jxvoCFP+KbUysDRH87U1iQb09tfi5X4qaJlBtR1c3zRW\n\tt+lZCLI7RNkZh7LvmTPViU92RR3aDK9KI+FgC7T2NAY+DU/3kg==","X-Google-Smtp-Source":"AA0mqf74ZJgTSsZh7ONgqUlvzjxTEuV9KZpGM5UbTsaKAYstDGYgK4XFmMb9Fy87ForZ6pJ5oFWfC3gOb+40ZZb+jwo=","X-Received":"by 2002:a92:1912:0:b0:302:5c57:c19d with SMTP id\n\t18-20020a921912000000b003025c57c19dmr29248860ilz.226.1670232032085;\n\tMon, 05 Dec 2022 01:20:32 -0800 (PST)","MIME-Version":"1.0","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 5 Dec 2022 09:20:18 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000f7227d05ef113255\"","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":26001,"web_url":"https://patchwork.libcamera.org/comment/26001/","msgid":"","date":"2022-12-05T09:36:46","subject":"Re: [libcamera-devel] [PATCH v2 04/10] 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":"Hi Naush,\n\nOn Mon, Dec 05, 2022 at 09:20:18AM +0000, Naushir Patuck wrote:\n> On Fri, 2 Dec 2022 at 20:44, Laurent Pinchart wrote:\n> > It's me again :-) Please see below for another comment.\n> >\n> > On Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via\n> > libcamera-devel wrote:\n> > > Read the platform configuration parameters from default.json config file. Use\n> > > the PipelineHandler::configurationFile() helper to determine the full path of\n> > > the file. The default.json filename can be overridden by the user setting the\n> > > LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of the\n> > > custom file.\n> > >\n> > > Add config parameter validation to ensure bad parameters cannot cause the\n> > > pipeline handler to stop working.\n> > >\n> > > Currently three 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> > > \"num_output0_buffers\"\n> > > Number of internal ISP Output0 buffers to allocate.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > .../pipeline/raspberrypi/data/default.json | 20 ++++++\n> > > .../pipeline/raspberrypi/data/meson.build | 8 +++\n> > > .../pipeline/raspberrypi/meson.build | 2 +\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++\n> > > 4 files changed, 91 insertions(+)\n> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json\n> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json\n> > > new file mode 100644\n> > > index 000000000000..d709e31850af\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json\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 Unicam.\n> > > + # This value must be greater than 0, but less than or equal to min_total_unicam_buffers.\n> > > + \"min_unicam_buffers\": 2,\n> > > +\n> > > + # The minimum total (internal + external) buffer count used for Unicam.\n> > > + # The number of internal buffers allocated for Unicam is given by:\n> > > + # internal buffer count = max(min_unicam_buffers,\n> > > + # min_total_unicam_buffers - external buffer count)\n> > > + \"min_total_unicam_buffers\": 4,\n> > > +\n> > > + # The number of internal buffers used for ISP Output0. This must be set to 1.\n> > > + \"num_output0_buffers\": 1\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..232f8b43c5fd\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> > > + 'default.json',\n> > > +])\n> > > +\n> > > +install_data(conf_files,\n> > > + install_dir : libcamera_datadir / 'pipeline/raspberrypi')\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > index f1a2f5ee72c2..48235f887501 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > > @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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> > > @@ -40,6 +41,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 \"dma_heaps.h\"\n> > > #include \"rpi_stream.h\"\n> > > @@ -313,6 +315,7 @@ public:\n> > > };\n> > >\n> > > Config config_;\n> > > + unsigned int numUnicamBuffers;\n> > >\n> > > private:\n> > > void checkRequestCompleted();\n> > > @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > */\n> > > stream->setExternalBuffer(buffer);\n> > > }\n> > > +\n> > > + if (!buffer) {\n> > > + if (stream == &data->isp_[Isp::Output0] && !data->config_.numOutput0Buffers) {\n> > > + LOG(RPI, Error) << \"Output0 buffer must be provided in the request.\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + if (stream == &data->unicam_[Unicam::Image] && !data->numUnicamBuffers) {\n> > > + LOG(RPI, Error) << \"Unicam Image buffer must be provided in the request.\";\n> > > + return -EINVAL;\n> > > + }\n> >\n> > The numOutput0Buffers and minUnicamBuffers configuration parameters seem\n> > to me to be a contract between the application and libcamera, where\n> > setting them to 0 requires the application to always pass buffers for\n> > the corresponding streams. As we discussed previously, while I'm fine\n> > with the concept of a configuration file to tune the behaviour of the\n> > camera, I'm bothered by conveing information about such an API contract\n> > through a file.\n> >\n> > Could we instead add a hint to the StreamConfiguration class to let\n> > applications tell about their side of the contract ? Something along the\n> > lines of\n> \n> Sure, I'll prototype something along these lines for the next revision of the\n> series.\n\nThanks a lot, that's very appreciated.\n\n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index 29235ddf0d8a..616f6dd174f1 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -13,6 +13,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > #include \n> > #include \n> > #include \n> > @@ -39,6 +41,13 @@ private:\n> > };\n> >\n> > struct StreamConfiguration {\n> > + enum class Hint {\n> > + None = 0,\n> > + MandatoryBuffer = (1 << 0),\n> > + };\n> > +\n> > + using Hints = Flags;\n> > +\n> > StreamConfiguration();\n> > StreamConfiguration(const StreamFormats &formats);\n> >\n> > @@ -50,6 +59,7 @@ struct StreamConfiguration {\n> > unsigned int bufferCount;\n> >\n> > std::optional colorSpace;\n> > + Hints hints;\n> >\n> > Stream *stream() const { return stream_; }\n> > void setStream(Stream *stream) { stream_ = stream; }\n> >\n> > (names to be bikeshedded of course)\n> >\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> > > @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > > {\n> > > RPiCameraData::Config &config = data->config_;\n> > > + std::string filename;\n> > >\n> > > config = {\n> > > .minUnicamBuffers = 2,\n> > > @@ -1426,6 +1443,49 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > > .numOutput0Buffers = 1,\n> > > };\n> > >\n> > > + char const *configFromEnv = utils::secure_getenv(\"LIBCAMERA_RPI_CONFIG_FILE\");\n> > > + if (!configFromEnv || *configFromEnv == '\\0')\n> > > + filename = configurationFile(\"raspberrypi\", \"default.json\");\n> > > + else\n> > > + filename = std::string(configFromEnv);\n> > > +\n> > > + if (filename.empty())\n> > > + return -EINVAL;\n> > > +\n> > > + File file(filename);\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> > > + ASSERT((*root)[\"version\"].get() == 1.0);\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> > > + config.numOutput0Buffers =\n> > > + phConfig[\"num_output0_buffers\"].get(config.numOutput0Buffers);\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> > > @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > */\n> > > numBuffers = std::max(data->config_.minUnicamBuffers,\n> > > minBuffers - numRawBuffers);\n> > > + data->numUnicamBuffers = numBuffers;\n> > > } else if (stream == &data->isp_[Isp::Input]) {\n> > > /*\n> > > * ISP input buffers are imported from Unicam, so follow","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 A26CABDE6B\n\tfor ;\n\tMon, 5 Dec 2022 09:36:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B07E6333B;\n\tMon, 5 Dec 2022 10:36:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7338363335\n\tfor ;\n\tMon, 5 Dec 2022 10:36:48 +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 EC770327;\n\tMon, 5 Dec 2022 10:36:47 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670233010;\n\tbh=UWfvTzF7+RvAtXdUmGogsa9bY4wXVsl1oPfq5Jkwl5o=;\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=hYxNmdGAgT70oF+FlM1mckt38nORgowNd7jNzBEpZt1nIYxjZD0L9ffKGmMi/meT4\n\tlAtRlgXUO+5z1TB23Weg9zQDFIxy1VnC2lgG5DtOPTKCKhv+BcfdXSVd4MA3NFFEem\n\tEzoStC6YR8J4P3IdGDDUl5BkIuC2iCW9Zy5ehsNsMKOcm4HxEpq5K1Dj3vvui2cupe\n\tN7n54ChcNwC4NCA2PEhTqUDulNKaNiVTZyxLDP7EALh4vgIhylu8m3H4E87nFlCpGK\n\tj9Y2DHXrdWB6tntV3MjYstIgtlIMjwNmi5sFZlpYtSC/ebqzLAgQ4Z/SShylD635Yb\n\t/Re4U/SwCKYHg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670233008;\n\tbh=UWfvTzF7+RvAtXdUmGogsa9bY4wXVsl1oPfq5Jkwl5o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H/Ezn16BIQGzAtKUF9Gk+eYOkT1zIEeSxiBvgzlEwY5n4dO9edInLSjKQYLSNte8u\n\tyjLrttU3CvxKHG+46WZEQcIXDOUqSjPPdYJOWeURi4nthQkxk2NbW5zYaS+43jRIYs\n\tkks5dRH8WtzSw2JKtIl9YzlyRtzwB5YT576orGt4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"H/Ezn16B\"; dkim-atps=neutral","Date":"Mon, 5 Dec 2022 11:36:46 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-5-naush@raspberrypi.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 v2 04/10] 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\" "}}]