[{"id":26240,"web_url":"https://patchwork.libcamera.org/comment/26240/","msgid":"<167388951896.42371.15280676491639567975@Monstersaurus>","date":"2023-01-16T17:18:38","subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:40)\n> Add a new helper function PipelineHandler::configurationFile() that returns\n> the full path of a named configuration file. This configuration file may be read\n> by pipeline handlers for platform specific configuration parameters on\n> initialisation.\n> \n> The mechanism for searching for the configuration file is similar to the IPA\n> configuration file:\n> \n> - In the source tree if libcamera is not installed\n> - Otherwise in standard system locations (etc and share directories).\n> \n> When stored in the source tree, configuration files shall be located in a 'data'\n> subdirectory of their respective pipeline handler directory.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> include/libcamera/internal/pipeline_handler.h | 3 +\n> src/libcamera/pipeline_handler.cpp | 60 +++++++++++++++++++\n> 2 files changed, 63 insertions(+)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index ec4f662d7399..4c4dfe62a680 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -65,6 +65,9 @@ public:\n> bool completeBuffer(Request *request, FrameBuffer *buffer);\n> void completeRequest(Request *request);\n> \n> + std::string configurationFile(const std::string &subdir,\n> + const std::string &name) const;\n> +\n> const char *name() const { return name_; }\n> \n> protected:\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index cfade4908118..a515ad5ecffb 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -8,6 +8,7 @@\n> #include \"libcamera/internal/pipeline_handler.h\"\n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -534,6 +535,65 @@ void PipelineHandler::completeRequest(Request *request)\n> }\n> }\n> \n> +/**\n> + * \\brief Retrieve the absolute path to a platform configuration file\n> + * \\param[in] subdir The pipeline handler specific subdirectory name\n> + * \\param[in] name The configuration file name\n> + *\n> + * This function locates a named platform configuration file and returns\n> + * its absolute path to the pipeline handler. It searches the following\n> + * directories, in order:\n> + *\n> + * - If libcamera is not installed, the src/libcamera/pipeline//data/\n> + * directory within the source tree ; otherwise\n> + * - The system data (share/libcamera/pipeline/) directory.\n> + *\n> + * The system directories are not searched if libcamera is not installed.\n> + *\n> + * \\return The full path to the pipeline handler configuration file, or an empty\n> + * string if no configuration file can be found\n> + */\n> +std::string PipelineHandler::configurationFile(const std::string &subdir,\n> + const std::string &name) const\n> +{\n> + std::string confPath;\n> + struct stat statbuf;\n> + int ret;\n> +\n> + std::string root = utils::libcameraSourcePath();\n> + if (!root.empty()) {\n> + /*\n> + * When libcamera is used before it is installed, load\n> + * configuration files from the source directory. The\n> + * configuration files are then located in the 'data'\n> + * subdirectory of the corresponding pipeline handler.\n> + */\n> + std::string confDir = root + \"src/libcamera/pipeline/\";\n> + confPath = confDir + subdir + \"/data/\" + name;\n> +\n> + LOG(Pipeline, Info)\n> + << \"libcamera is not installed. Loading platform configuration file from '\"\n> + << confPath << \"'\";\n> +\n> + ret = stat(confPath.c_str(), &statbuf);\n> + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n\nWill this stop configuration files being a symlink to an alternative\nfile? (S_IFREG vs S_IFLINK) ? Does that even matter?\n\nWe could do:\n\n\tif (File::exists(confPath))\n\t\treturn confPath;\n\nThat only checks that the 'name' is not a directory... So ... it's\ndifferent again, though I would expect the \n\n\n> + return confPath;\n> + } else {\n> + /* Else look in the system locations. */\n> + confPath = std::string(LIBCAMERA_DATA_DIR)\n> + + \"/pipeline/\" + subdir + '/' + name;\n> + ret = stat(confPath.c_str(), &statbuf);\n> + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n> + return confPath;\n> + }\n> +\n> + LOG(Pipeline, Error)\n> + << \"Configuration file '\" << confPath\n> + << \"' not found for pipeline handler '\" << PipelineHandler::name() << \"'\";\n\nThis sounds like it suddenly makes configuration files mandatory for all\npipelines ?\n\n(or perhaps only if a pipeline looks for one ?)\n\n> +\n> + return std::string();\n> +}\n> +\n> /**\n> * \\brief Register a camera to the camera manager and pipeline handler\n> * \\param[in] camera The camera to be added\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3FBDFC3240\n\tfor ;\n\tMon, 16 Jan 2023 17:18:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8731B625E4;\n\tMon, 16 Jan 2023 18:18:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58FFD61F01\n\tfor ;\n\tMon, 16 Jan 2023 18:18:41 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D1DA1802;\n\tMon, 16 Jan 2023 18:18:40 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673889522;\n\tbh=mgC3F91QEb9J4ZddB97vKEwqgw7rNqqH6MR9sJI6ZY8=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=SM0GXgBu5MRExXuVlQG4vH/DsxPm+4FQHIURLYLUY6NZ9BdQV35G8puXlCJtZSkg2\n\tk6C4TpqaQge13iCXEeUqou/1YU7c/RZZnchy2/V2SWXmDB2LyWWdtw/8NxJVMqvoL0\n\tyD4lUzfTm9nhJ7h8I3L0OI4PxVxX9jwBco8oU2haFbfWxAmU97WLDUeG6bwv+FCaQn\n\t4OMZQgHC3TCKiOxTEBNInDw5DEYI4N5Pg3emJj07zEjV8+H0AC6yBvYM4gwo361HII\n\tXOfCnnOr5SuzuTcMrPkjrsgzOFZEcpVpUTAtmMOsrHe9paylx+/z0ZBU8FR2FRbPFq\n\tUSatMZkcVGOkw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673889521;\n\tbh=mgC3F91QEb9J4ZddB97vKEwqgw7rNqqH6MR9sJI6ZY8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=j1xtqr7Lptq1rJ1irQ3s0m4v+kqCQlSzelsV7Gt2W4ytPFSNy/YhJMg3I0z2B+ctH\n\t907xeTJCZ6b99bh4pNQMJ24YRSpIRe3p2lNtbyDGxqsocIUbXZ+AWw+nbsBYeJX6Bs\n\tU5/dBEauD5nahv+aZqnCNxy+um7XYolGeLeXBDWA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"j1xtqr7L\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221209090050.19441-3-naush@raspberrypi.com>","References":"<20221209090050.19441-1-naush@raspberrypi.com>\n\t<20221209090050.19441-3-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 16 Jan 2023 17:18:38 +0000","Message-ID":"<167388951896.42371.15280676491639567975@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26246,"web_url":"https://patchwork.libcamera.org/comment/26246/","msgid":"","date":"2023-01-17T08:50:46","subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for the feedback!\n\nOn Mon, 16 Jan 2023 at 17:18, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:40)\n> > Add a new helper function PipelineHandler::configurationFile() that\n> returns\n> > the full path of a named configuration file. This configuration file may\n> be read\n> > by pipeline handlers for platform specific configuration parameters on\n> > initialisation.\n> >\n> > The mechanism for searching for the configuration file is similar to the\n> IPA\n> > configuration file:\n> >\n> > - In the source tree if libcamera is not installed\n> > - Otherwise in standard system locations (etc and share directories).\n> >\n> > When stored in the source tree, configuration files shall be located in\n> a 'data'\n> > subdirectory of their respective pipeline handler directory.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 3 +\n> > src/libcamera/pipeline_handler.cpp | 60 +++++++++++++++++++\n> > 2 files changed, 63 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h\n> b/include/libcamera/internal/pipeline_handler.h\n> > index ec4f662d7399..4c4dfe62a680 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -65,6 +65,9 @@ public:\n> > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > void completeRequest(Request *request);\n> >\n> > + std::string configurationFile(const std::string &subdir,\n> > + const std::string &name) const;\n> > +\n> > const char *name() const { return name_; }\n> >\n> > protected:\n> > diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> > index cfade4908118..a515ad5ecffb 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -534,6 +535,65 @@ void PipelineHandler::completeRequest(Request\n> *request)\n> > }\n> > }\n> >\n> > +/**\n> > + * \\brief Retrieve the absolute path to a platform configuration file\n> > + * \\param[in] subdir The pipeline handler specific subdirectory name\n> > + * \\param[in] name The configuration file name\n> > + *\n> > + * This function locates a named platform configuration file and returns\n> > + * its absolute path to the pipeline handler. It searches the following\n> > + * directories, in order:\n> > + *\n> > + * - If libcamera is not installed, the\n> src/libcamera/pipeline//data/\n> > + * directory within the source tree ; otherwise\n> > + * - The system data (share/libcamera/pipeline/) directory.\n> > + *\n> > + * The system directories are not searched if libcamera is not\n> installed.\n> > + *\n> > + * \\return The full path to the pipeline handler configuration file, or\n> an empty\n> > + * string if no configuration file can be found\n> > + */\n> > +std::string PipelineHandler::configurationFile(const std::string\n> &subdir,\n> > + const std::string &name)\n> const\n> > +{\n> > + std::string confPath;\n> > + struct stat statbuf;\n> > + int ret;\n> > +\n> > + std::string root = utils::libcameraSourcePath();\n> > + if (!root.empty()) {\n> > + /*\n> > + * When libcamera is used before it is installed, load\n> > + * configuration files from the source directory. The\n> > + * configuration files are then located in the 'data'\n> > + * subdirectory of the corresponding pipeline handler.\n> > + */\n> > + std::string confDir = root + \"src/libcamera/pipeline/\";\n> > + confPath = confDir + subdir + \"/data/\" + name;\n> > +\n> > + LOG(Pipeline, Info)\n> > + << \"libcamera is not installed. Loading platform\n> configuration file from '\"\n> > + << confPath << \"'\";\n> > +\n> > + ret = stat(confPath.c_str(), &statbuf);\n> > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n>\n> Will this stop configuration files being a symlink to an alternative\n> file? (S_IFREG vs S_IFLINK) ? Does that even matter?\n>\n> We could do:\n>\n> if (File::exists(confPath))\n> return confPath;\n>\n> That only checks that the 'name' is not a directory... So ... it's\n> different again, though I would expect the\n\n\nI was copying the implementation of IPAProxy::configurationFile()\nfor this function to retain consistency, but could change if other folks\nagree?\n\n\n>\n>\n>\n> > + return confPath;\n> > + } else {\n> > + /* Else look in the system locations. */\n> > + confPath = std::string(LIBCAMERA_DATA_DIR)\n> > + + \"/pipeline/\" + subdir + '/' + name;\n> > + ret = stat(confPath.c_str(), &statbuf);\n> > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n> > + return confPath;\n> > + }\n> > +\n> > + LOG(Pipeline, Error)\n> > + << \"Configuration file '\" << confPath\n> > + << \"' not found for pipeline handler '\" <<\n> PipelineHandler::name() << \"'\";\n>\n> This sounds like it suddenly makes configuration files mandatory for all\n> pipelines ?\n>\n> (or perhaps only if a pipeline looks for one ?)\n>\n\nCorrect, the config file is only mandatory if a pipeline handler explicitly\nlooks for one.\n\nRegards,\nNaush\n\n\n\n>\n> > +\n> > + return std::string();\n> > +}\n> > +\n> > /**\n> > * \\brief Register a camera to the camera manager and pipeline handler\n> > * \\param[in] camera The camera to be added\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0FEF2C3240\n\tfor ;\n\tTue, 17 Jan 2023 08:51:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 84361625D8;\n\tTue, 17 Jan 2023 09:51:05 +0100 (CET)","from mail-yw1-x112d.google.com (mail-yw1-x112d.google.com\n\t[IPv6:2607:f8b0:4864:20::112d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 787B961EFE\n\tfor ;\n\tTue, 17 Jan 2023 09:51:03 +0100 (CET)","by mail-yw1-x112d.google.com with SMTP id\n\t00721157ae682-4d19b2686a9so285632017b3.6\n\tfor ;\n\tTue, 17 Jan 2023 00:51:03 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673945465;\n\tbh=zbGozgqHE2S5uBP+1a5zr8XM8oUwXAIuppFYmBR6qK8=;\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=hoh80tG2cL11JsXYeAFb9CdpKq6VjkR0qfbPpn8Kx8kwVuSTzzIDdNaDolBIfOMO8\n\tGBoCentWsMwpxgoAiwJFVnmlgrkgSDPLena/Jgdx2EhNKkE8ZoEIP9NSMtmw/Oowi7\n\tcQ8dxiQm360hQHB6oy3OjuC5nlwUU/5t6RONMPRDsOr7j+dpSs0f9bR1WAyXD33i2g\n\toCz83RGB3WqtgKGIhVZ9XiiofbwMmZugZxSCz3ursJLTnTYQMOC5jk4zPj36YjwHy6\n\thM2A08vfrM6bW9nkwdk0tt7kuKc1URk2nQaqFJ6glUIpD8ueZ3Lo2+loUUbgDeVX7D\n\t5XI/23Ee8VMuA==","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=hVNevEnKhtYtz1hfPLRKPXH+eFbD/QiWJmWF5MM79LQ=;\n\tb=t4/s9GwHKJogoHxQwbclpAmhswVJVbOWSZbAWIuV674oPAJjJIPTFvHXjE9oiv9SrK\n\tl51+xygRAyHag2EarhovDRJffRhPG4y9prv8WtPdhI2Pa0NWb6G5pCmpfsoLoARhlLHX\n\tXjnPaN6NytXW0qofi0NeL2bmW7eq4+xmfXfhO+2J8QJw/7bA1lLIwKjFfTriACIvPp/n\n\tToA3KnA6ySpUc05KqATvgdbDyhG756xn9tUrPOyBjybnpy5imNGOs95gzLlUuoxQQ+as\n\tkceS+L7kMf4ALSzxL92vTUVdLR4ae1oq3X/DlWfu93h3xvbhV79geZf9kmGcDDIe+Wbv\n\tM3og=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"t4/s9GwH\"; 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=hVNevEnKhtYtz1hfPLRKPXH+eFbD/QiWJmWF5MM79LQ=;\n\tb=SJr5HRQmVEovdQtuV6tDfH6zkBX3D30K9RW+nnWVRL8siUdHgJcX8wGggVsOaeBN3x\n\t7dALgz6+WCE2gqDq2QwMdZkpbt3yEebnXICKvghaO7YU2FE73gTXFU3SNocVzn2D7tAz\n\ta9Xl2/GuW8/nSNf2TeBFO2++6GTAv3mq6pAPf/1IjNEMe/eHsXq9+7CALDIWYfVyka1r\n\tA6qZ4sa9wiJy2fH2oNfgFtrsziogyAbxDd4JCb9O4fMjyblI52zTiD3H298CCLVsoLGX\n\tVaAKlT5QsFjMLbuEVvVzceAiirEsjur6Myt0ZfqZNsE5wL7urapilp/4w5WffwryJPWc\n\tOMEA==","X-Gm-Message-State":"AFqh2kp3fuA2NHfh2zY790aSuTuuK1M3VzUmCCsMtPjHVssAt/d8xVBz\n\txngqLVXBTKMzYRvIrjbIrIhf6CqT9bk+7zerW5MnCW46yvTxUBtwZow=","X-Google-Smtp-Source":"AMrXdXvNYwTECLdTVShK9PIPJ+yI1yeauI7NXQBJz+zg8A9+mz1fBO33eoGyXbHnIUkh4J/RmZamyfaCzBKkxbgfteU=","X-Received":"by 2002:a81:7096:0:b0:3ec:2e89:409c with SMTP id\n\tl144-20020a817096000000b003ec2e89409cmr297412ywc.20.1673945462097;\n\tTue, 17 Jan 2023 00:51:02 -0800 (PST)","MIME-Version":"1.0","References":"<20221209090050.19441-1-naush@raspberrypi.com>\n\t<20221209090050.19441-3-naush@raspberrypi.com>\n\t<167388951896.42371.15280676491639567975@Monstersaurus>","In-Reply-To":"<167388951896.42371.15280676491639567975@Monstersaurus>","Date":"Tue, 17 Jan 2023 08:50:46 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000a4606705f271cc59\"","Subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","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":26249,"web_url":"https://patchwork.libcamera.org/comment/26249/","msgid":"<167394844788.42371.16246058578541702614@Monstersaurus>","date":"2023-01-17T09:40:47","subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2023-01-17 08:50:46)\n> Hi Kieran,\n> \n> Thank you for the feedback!\n> \n> On Mon, 16 Jan 2023 at 17:18, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:40)\n> > > Add a new helper function PipelineHandler::configurationFile() that\n> > returns\n> > > the full path of a named configuration file. This configuration file may\n> > be read\n> > > by pipeline handlers for platform specific configuration parameters on\n> > > initialisation.\n> > >\n> > > The mechanism for searching for the configuration file is similar to the\n> > IPA\n> > > configuration file:\n> > >\n> > > - In the source tree if libcamera is not installed\n> > > - Otherwise in standard system locations (etc and share directories).\n> > >\n> > > When stored in the source tree, configuration files shall be located in\n> > a 'data'\n> > > subdirectory of their respective pipeline handler directory.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 3 +\n> > > src/libcamera/pipeline_handler.cpp | 60 +++++++++++++++++++\n> > > 2 files changed, 63 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h\n> > b/include/libcamera/internal/pipeline_handler.h\n> > > index ec4f662d7399..4c4dfe62a680 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -65,6 +65,9 @@ public:\n> > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > void completeRequest(Request *request);\n> > >\n> > > + std::string configurationFile(const std::string &subdir,\n> > > + const std::string &name) const;\n> > > +\n> > > const char *name() const { return name_; }\n> > >\n> > > protected:\n> > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > b/src/libcamera/pipeline_handler.cpp\n> > > index cfade4908118..a515ad5ecffb 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -534,6 +535,65 @@ void PipelineHandler::completeRequest(Request\n> > *request)\n> > > }\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Retrieve the absolute path to a platform configuration file\n> > > + * \\param[in] subdir The pipeline handler specific subdirectory name\n> > > + * \\param[in] name The configuration file name\n> > > + *\n> > > + * This function locates a named platform configuration file and returns\n> > > + * its absolute path to the pipeline handler. It searches the following\n> > > + * directories, in order:\n> > > + *\n> > > + * - If libcamera is not installed, the\n> > src/libcamera/pipeline//data/\n> > > + * directory within the source tree ; otherwise\n> > > + * - The system data (share/libcamera/pipeline/) directory.\n> > > + *\n> > > + * The system directories are not searched if libcamera is not\n> > installed.\n> > > + *\n> > > + * \\return The full path to the pipeline handler configuration file, or\n> > an empty\n> > > + * string if no configuration file can be found\n> > > + */\n> > > +std::string PipelineHandler::configurationFile(const std::string\n> > &subdir,\n> > > + const std::string &name)\n> > const\n> > > +{\n> > > + std::string confPath;\n> > > + struct stat statbuf;\n> > > + int ret;\n> > > +\n> > > + std::string root = utils::libcameraSourcePath();\n> > > + if (!root.empty()) {\n> > > + /*\n> > > + * When libcamera is used before it is installed, load\n> > > + * configuration files from the source directory. The\n> > > + * configuration files are then located in the 'data'\n> > > + * subdirectory of the corresponding pipeline handler.\n> > > + */\n> > > + std::string confDir = root + \"src/libcamera/pipeline/\";\n> > > + confPath = confDir + subdir + \"/data/\" + name;\n> > > +\n> > > + LOG(Pipeline, Info)\n> > > + << \"libcamera is not installed. Loading platform\n> > configuration file from '\"\n> > > + << confPath << \"'\";\n> > > +\n> > > + ret = stat(confPath.c_str(), &statbuf);\n> > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n> >\n> > Will this stop configuration files being a symlink to an alternative\n> > file? (S_IFREG vs S_IFLINK) ? Does that even matter?\n> >\n> > We could do:\n> >\n> > if (File::exists(confPath))\n> > return confPath;\n> >\n> > That only checks that the 'name' is not a directory... So ... it's\n> > different again, though I would expect the\n\n\"I would expect the user of the confPath to use the File class, so it\nshould be appropriate\" ... was supposed to be written here ;)\n\n> \n> \n> I was copying the implementation of IPAProxy::configurationFile()\n> for this function to retain consistency, but could change if other folks\n> agree?\n\nIf it's better we can fix the configurationFile location too. But both\nseem functional anyway.\n--\nKieran\n\n\n> \n> \n> >\n> >\n> >\n> > > + return confPath;\n> > > + } else {\n> > > + /* Else look in the system locations. */\n> > > + confPath = std::string(LIBCAMERA_DATA_DIR)\n> > > + + \"/pipeline/\" + subdir + '/' + name;\n> > > + ret = stat(confPath.c_str(), &statbuf);\n> > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n> > > + return confPath;\n> > > + }\n> > > +\n> > > + LOG(Pipeline, Error)\n> > > + << \"Configuration file '\" << confPath\n> > > + << \"' not found for pipeline handler '\" <<\n> > PipelineHandler::name() << \"'\";\n> >\n> > This sounds like it suddenly makes configuration files mandatory for all\n> > pipelines ?\n> >\n> > (or perhaps only if a pipeline looks for one ?)\n> >\n> \n> Correct, the config file is only mandatory if a pipeline handler explicitly\n> looks for one.\n> \n\nWould you expect pipelines to have a default configuration for all\noptions?\n\nAnyway, while it reports an error, it's up to the pipeline handler to\ndecide how to respond if there's no file.\n\n--\nKieran\n\n\n> Regards,\n> Naush\n> \n> \n> \n> >\n> > > +\n> > > + return std::string();\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Register a camera to the camera manager and pipeline handler\n> > > * \\param[in] camera The camera to be added\n> > > --\n> > > 2.25.1\n> > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6CE1ABD87C\n\tfor ;\n\tTue, 17 Jan 2023 09:40:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A656F625E4;\n\tTue, 17 Jan 2023 10:40:52 +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 1CC9961EFE\n\tfor ;\n\tTue, 17 Jan 2023 10:40:51 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A8EFB10C;\n\tTue, 17 Jan 2023 10:40:50 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673948452;\n\tbh=JjOSW+uJSHQhg8fBUCexIOjjogHnql8/6PtsyYmK9/k=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3EQikvDDGbBOVEjjHRc1HLZcW3Jd2SElTtlUIuc5nW1UIKuhHxuodHb/p2hjp5Qwf\n\tp8jz5KQcxkChb8S+zDu6Y84c12yqkDqkdbNCy1a79B6RCULqWdhXYR7fKaP2ZBWpg8\n\timN3BjPDI31A0937fc7PoVP/OhJ/C26lYCMt8KwhKkhCAMkAY/EVAfmKMkzQwPJrCu\n\tAtJzyGiVgKI7GraM4EIrS7pvV4AUHFHQG2xbJsY4M3J0oI+D26BKwZ6c9n5yMmlAiZ\n\tFgFzEYpP7qxMtRTG51Mih7BovXKyJlYBM9Lii2os02WmWoTGdY8KXbVJ9T521h8fvB\n\twcXSb5W3Xn0fA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673948450;\n\tbh=JjOSW+uJSHQhg8fBUCexIOjjogHnql8/6PtsyYmK9/k=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=H+JX7yT7okH/8fuvkag2w6CfiTQOyii2d87nHmbJRm0yR2V1HMlExCQvWfTFBcpTf\n\tW0VGhL3EiLh0SlcktEh/G41muAK1iMx1OWSOlEdr6q9zPND7FYabOv3A3PACiG0xOW\n\tCEBAoVMLA1f0yfut6YSsElcTyVsJmn0txjQCoEso="],"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+JX7yT7\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221209090050.19441-1-naush@raspberrypi.com>\n\t<20221209090050.19441-3-naush@raspberrypi.com>\n\t<167388951896.42371.15280676491639567975@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Tue, 17 Jan 2023 09:40:47 +0000","Message-ID":"<167394844788.42371.16246058578541702614@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26250,"web_url":"https://patchwork.libcamera.org/comment/26250/","msgid":"","date":"2023-01-17T09:44:12","subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Tue, 17 Jan 2023 at 09:40, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck (2023-01-17 08:50:46)\n> > Hi Kieran,\n> >\n> > Thank you for the feedback!\n> >\n> > On Mon, 16 Jan 2023 at 17:18, Kieran Bingham <\n> > kieran.bingham@ideasonboard.com> wrote:\n> >\n> > > Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:40)\n> > > > Add a new helper function PipelineHandler::configurationFile() that\n> > > returns\n> > > > the full path of a named configuration file. This configuration file\n> may\n> > > be read\n> > > > by pipeline handlers for platform specific configuration parameters\n> on\n> > > > initialisation.\n> > > >\n> > > > The mechanism for searching for the configuration file is similar to\n> the\n> > > IPA\n> > > > configuration file:\n> > > >\n> > > > - In the source tree if libcamera is not installed\n> > > > - Otherwise in standard system locations (etc and share directories).\n> > > >\n> > > > When stored in the source tree, configuration files shall be located\n> in\n> > > a 'data'\n> > > > subdirectory of their respective pipeline handler directory.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > include/libcamera/internal/pipeline_handler.h | 3 +\n> > > > src/libcamera/pipeline_handler.cpp | 60\n> +++++++++++++++++++\n> > > > 2 files changed, 63 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/pipeline_handler.h\n> > > b/include/libcamera/internal/pipeline_handler.h\n> > > > index ec4f662d7399..4c4dfe62a680 100644\n> > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > @@ -65,6 +65,9 @@ public:\n> > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > void completeRequest(Request *request);\n> > > >\n> > > > + std::string configurationFile(const std::string &subdir,\n> > > > + const std::string &name) const;\n> > > > +\n> > > > const char *name() const { return name_; }\n> > > >\n> > > > protected:\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > b/src/libcamera/pipeline_handler.cpp\n> > > > index cfade4908118..a515ad5ecffb 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -8,6 +8,7 @@\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > >\n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \n> > > > @@ -534,6 +535,65 @@ void PipelineHandler::completeRequest(Request\n> > > *request)\n> > > > }\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Retrieve the absolute path to a platform configuration\n> file\n> > > > + * \\param[in] subdir The pipeline handler specific subdirectory name\n> > > > + * \\param[in] name The configuration file name\n> > > > + *\n> > > > + * This function locates a named platform configuration file and\n> returns\n> > > > + * its absolute path to the pipeline handler. It searches the\n> following\n> > > > + * directories, in order:\n> > > > + *\n> > > > + * - If libcamera is not installed, the\n> > > src/libcamera/pipeline//data/\n> > > > + * directory within the source tree ; otherwise\n> > > > + * - The system data (share/libcamera/pipeline/) directory.\n> > > > + *\n> > > > + * The system directories are not searched if libcamera is not\n> > > installed.\n> > > > + *\n> > > > + * \\return The full path to the pipeline handler configuration\n> file, or\n> > > an empty\n> > > > + * string if no configuration file can be found\n> > > > + */\n> > > > +std::string PipelineHandler::configurationFile(const std::string\n> > > &subdir,\n> > > > + const std::string\n> &name)\n> > > const\n> > > > +{\n> > > > + std::string confPath;\n> > > > + struct stat statbuf;\n> > > > + int ret;\n> > > > +\n> > > > + std::string root = utils::libcameraSourcePath();\n> > > > + if (!root.empty()) {\n> > > > + /*\n> > > > + * When libcamera is used before it is installed,\n> load\n> > > > + * configuration files from the source directory. The\n> > > > + * configuration files are then located in the 'data'\n> > > > + * subdirectory of the corresponding pipeline\n> handler.\n> > > > + */\n> > > > + std::string confDir = root +\n> \"src/libcamera/pipeline/\";\n> > > > + confPath = confDir + subdir + \"/data/\" + name;\n> > > > +\n> > > > + LOG(Pipeline, Info)\n> > > > + << \"libcamera is not installed. Loading\n> platform\n> > > configuration file from '\"\n> > > > + << confPath << \"'\";\n> > > > +\n> > > > + ret = stat(confPath.c_str(), &statbuf);\n> > > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) ==\n> S_IFREG)\n> > >\n> > > Will this stop configuration files being a symlink to an alternative\n> > > file? (S_IFREG vs S_IFLINK) ? Does that even matter?\n> > >\n> > > We could do:\n> > >\n> > > if (File::exists(confPath))\n> > > return confPath;\n> > >\n> > > That only checks that the 'name' is not a directory... So ... it's\n> > > different again, though I would expect the\n>\n> \"I would expect the user of the confPath to use the File class, so it\n> should be appropriate\" ... was supposed to be written here ;)\n>\n> >\n> >\n> > I was copying the implementation of IPAProxy::configurationFile()\n> > for this function to retain consistency, but could change if other folks\n> > agree?\n>\n> If it's better we can fix the configurationFile location too. But both\n> seem functional anyway.\n> --\n> Kieran\n>\n>\n> >\n> >\n> > >\n> > >\n> > >\n> > > > + return confPath;\n> > > > + } else {\n> > > > + /* Else look in the system locations. */\n> > > > + confPath = std::string(LIBCAMERA_DATA_DIR)\n> > > > + + \"/pipeline/\" + subdir + '/' + name;\n> > > > + ret = stat(confPath.c_str(), &statbuf);\n> > > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) ==\n> S_IFREG)\n> > > > + return confPath;\n> > > > + }\n> > > > +\n> > > > + LOG(Pipeline, Error)\n> > > > + << \"Configuration file '\" << confPath\n> > > > + << \"' not found for pipeline handler '\" <<\n> > > PipelineHandler::name() << \"'\";\n> > >\n> > > This sounds like it suddenly makes configuration files mandatory for\n> all\n> > > pipelines ?\n> > >\n> > > (or perhaps only if a pipeline looks for one ?)\n> > >\n> >\n> > Correct, the config file is only mandatory if a pipeline handler\n> explicitly\n> > looks for one.\n> >\n>\n> Would you expect pipelines to have a default configuration for all\n> options?\n>\n> Anyway, while it reports an error, it's up to the pipeline handler to\n> decide how to respond if there's no file.\n>\n\nIn an earlier version of the patch, we did have a default.yaml file that\nloaded\na default configuration if no other file was provided. However, Laurent\nsuggested\n(and I agree) that we remove that and hard-code the default configuration\nin the\nsource itself.\n\nAs you said, it's up to the individual pipeline handlers to decide the\nbehavior.\n\nNaush\n\n\n\n>\n> --\n> Kieran\n>\n>\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > >\n> > > > +\n> > > > + return std::string();\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Register a camera to the camera manager and pipeline\n> handler\n> > > > * \\param[in] camera The camera to be added\n> > > > --\n> > > > 2.25.1\n> > > >\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 40AC0C3240\n\tfor ;\n\tTue, 17 Jan 2023 09:44:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8AB27625E6;\n\tTue, 17 Jan 2023 10:44:31 +0100 (CET)","from mail-yw1-x112a.google.com (mail-yw1-x112a.google.com\n\t[IPv6:2607:f8b0:4864:20::112a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0404961EFE\n\tfor ;\n\tTue, 17 Jan 2023 10:44:29 +0100 (CET)","by mail-yw1-x112a.google.com with SMTP id\n\t00721157ae682-4c24993965eso408295967b3.12\n\tfor ;\n\tTue, 17 Jan 2023 01:44:28 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673948671;\n\tbh=2c7PGdzRmcfH+ywpHPS+RW4kZI8Ti67tFOP8B2PwNbw=;\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=mYzCsS7tnQMw/VWHxMRdNxg4h5X/2qm/4XtP88GK8+3QGlGNz7d3Y07vaFVo8ZuiT\n\tIu0TsymAiUuGKA2gHk9LQqe/6jzKI94uAF10npVk2gKfh9x0aSMc5lFnF1bmc0RBPX\n\thSo/ReqsYWUV9dUftcNWMtmZZ2W7Vokdr6glTI0XXjCd+EKpCM63rIkMvJRlfBavbo\n\tHZgmNsQEnld3sa6A3L79jkXtw6zZs9trI3TFccJ8aSs6yp7J6h4iNg0SZhSaNzIwRD\n\tHsrpmEDREfH2vq9heC5UtiT+Vv51/PekwPa8kH2Ez+qQp1C7sXJq3QzK6paf10n4Ac\n\tZBGY7Siv70WRQ==","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=yFvP9Pc5/nJgqgu1nuj8qBYzbxoQ03NkIV8yadUbOKU=;\n\tb=Rk0UNGpEyN/yOn98m49pMTrWrsA7vKabV4bVQ/zJBbI7huqXMjIbpHNGd3XioY30wp\n\tlgs8ZYFt1fJUXEX3hNXbRWF/7TztVpXiIq6GHJa/SoluvSLF+K2nupA1qbuD81VEWeoR\n\tFUwkcvf3V9AEGMqEOLRHOEJLOuuSVBh2FmvC6j0+AWBbRALGoB6lNKMD+0CaehwN3rUI\n\tNVYOGVFw+EarPNwPkEWsOLv4Emts9QMkKjn7qzMfNlJUKxn5nRp+mLFAejsMj8Ru6nwv\n\tuPTMGUMr+KFjHhsV8TVEa9FQEwu6DPD0WcAh3SsKiOJxrZ1pcX1vZ8FVg8t7Uci9023s\n\tZ25w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Rk0UNGpE\"; 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=yFvP9Pc5/nJgqgu1nuj8qBYzbxoQ03NkIV8yadUbOKU=;\n\tb=ZZmrPqiBjD1Rct0HbOCwdEaTDrUICGt3dWbUDet0Z5EocrNypNEAhV7NswJh7n8TMk\n\tcST8ADlqkPLIQWj4kdp+IQxiuQbpAi/WrPRIYhIiZ6uFwRVqq1oSfDXn+3IepFKzPjc0\n\tQL+RqG2b8aCIxbtgkIWFrW+AXleoz8wU5yGRbnVVW2nr8T+V/uskGLB8IZd7gwQXAseH\n\tqdGqA0fREm9bda6yZtLLm0qLMtsuIf0yx4zBGEIRPfyT2gqyXmfs/4/KJoLtftSdg30I\n\tW0siRuHdulzNHIHB5TI/uGPEuJ0lg3NIS62J+kzMknLZVUGGothSIpjNf9oKOp1LzZgE\n\thXYA==","X-Gm-Message-State":"AFqh2koVI2CrtegZ+L+kk0+3hzGS4k4lgkJyxWUcARYmccc2pTdIij8o\n\te1KLtlk6CH+l/r4T6Vk8r/iTpr/HvPGslbHHurrCRDEz4QUUwo3czNs=","X-Google-Smtp-Source":"AMrXdXtE8wbBW2p+U0lNtbUN3DrL1XhkwwIVGVlSJ2g7Dx7ABwbgyE3yPIyrIXpE4H3OteN/3F/w0Qhl7YrdG2J9Hfw=","X-Received":"by 2002:a81:6b02:0:b0:3f4:6ecb:471d with SMTP id\n\tg2-20020a816b02000000b003f46ecb471dmr326750ywc.231.1673948667666;\n\tTue, 17 Jan 2023 01:44:27 -0800 (PST)","MIME-Version":"1.0","References":"<20221209090050.19441-1-naush@raspberrypi.com>\n\t<20221209090050.19441-3-naush@raspberrypi.com>\n\t<167388951896.42371.15280676491639567975@Monstersaurus>\n\t\n\t<167394844788.42371.16246058578541702614@Monstersaurus>","In-Reply-To":"<167394844788.42371.16246058578541702614@Monstersaurus>","Date":"Tue, 17 Jan 2023 09:44:12 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000b5816905f2728b7f\"","Subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","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":26251,"web_url":"https://patchwork.libcamera.org/comment/26251/","msgid":"<167394940800.3184615.17362721039831144590@Monstersaurus>","date":"2023-01-17T09:56:48","subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2023-01-17 09:44:12)\n> On Tue, 17 Jan 2023 at 09:40, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Naushir Patuck (2023-01-17 08:50:46)\n> > > Hi Kieran,\n> > >\n> > > Thank you for the feedback!\n> > >\n> > > On Mon, 16 Jan 2023 at 17:18, Kieran Bingham <\n> > > kieran.bingham@ideasonboard.com> wrote:\n> > >\n> > > > Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:40)\n> > > > > Add a new helper function PipelineHandler::configurationFile() that\n> > > > returns\n> > > > > the full path of a named configuration file. This configuration file\n> > may\n> > > > be read\n> > > > > by pipeline handlers for platform specific configuration parameters\n> > on\n> > > > > initialisation.\n> > > > >\n> > > > > The mechanism for searching for the configuration file is similar to\n> > the\n> > > > IPA\n> > > > > configuration file:\n> > > > >\n> > > > > - In the source tree if libcamera is not installed\n> > > > > - Otherwise in standard system locations (etc and share directories).\n> > > > >\n> > > > > When stored in the source tree, configuration files shall be located\n> > in\n> > > > a 'data'\n> > > > > subdirectory of their respective pipeline handler directory.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Reviewed-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/internal/pipeline_handler.h | 3 +\n> > > > > src/libcamera/pipeline_handler.cpp | 60\n> > +++++++++++++++++++\n> > > > > 2 files changed, 63 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/pipeline_handler.h\n> > > > b/include/libcamera/internal/pipeline_handler.h\n> > > > > index ec4f662d7399..4c4dfe62a680 100644\n> > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > @@ -65,6 +65,9 @@ public:\n> > > > > bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > > void completeRequest(Request *request);\n> > > > >\n> > > > > + std::string configurationFile(const std::string &subdir,\n> > > > > + const std::string &name) const;\n> > > > > +\n> > > > > const char *name() const { return name_; }\n> > > > >\n> > > > > protected:\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > > b/src/libcamera/pipeline_handler.cpp\n> > > > > index cfade4908118..a515ad5ecffb 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -8,6 +8,7 @@\n> > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > @@ -534,6 +535,65 @@ void PipelineHandler::completeRequest(Request\n> > > > *request)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Retrieve the absolute path to a platform configuration\n> > file\n> > > > > + * \\param[in] subdir The pipeline handler specific subdirectory name\n> > > > > + * \\param[in] name The configuration file name\n> > > > > + *\n> > > > > + * This function locates a named platform configuration file and\n> > returns\n> > > > > + * its absolute path to the pipeline handler. It searches the\n> > following\n> > > > > + * directories, in order:\n> > > > > + *\n> > > > > + * - If libcamera is not installed, the\n> > > > src/libcamera/pipeline//data/\n> > > > > + * directory within the source tree ; otherwise\n> > > > > + * - The system data (share/libcamera/pipeline/) directory.\n> > > > > + *\n> > > > > + * The system directories are not searched if libcamera is not\n> > > > installed.\n> > > > > + *\n> > > > > + * \\return The full path to the pipeline handler configuration\n> > file, or\n> > > > an empty\n> > > > > + * string if no configuration file can be found\n> > > > > + */\n> > > > > +std::string PipelineHandler::configurationFile(const std::string\n> > > > &subdir,\n> > > > > + const std::string\n> > &name)\n> > > > const\n> > > > > +{\n> > > > > + std::string confPath;\n> > > > > + struct stat statbuf;\n> > > > > + int ret;\n> > > > > +\n> > > > > + std::string root = utils::libcameraSourcePath();\n> > > > > + if (!root.empty()) {\n> > > > > + /*\n> > > > > + * When libcamera is used before it is installed,\n> > load\n> > > > > + * configuration files from the source directory. The\n> > > > > + * configuration files are then located in the 'data'\n> > > > > + * subdirectory of the corresponding pipeline\n> > handler.\n> > > > > + */\n> > > > > + std::string confDir = root +\n> > \"src/libcamera/pipeline/\";\n> > > > > + confPath = confDir + subdir + \"/data/\" + name;\n> > > > > +\n> > > > > + LOG(Pipeline, Info)\n> > > > > + << \"libcamera is not installed. Loading\n> > platform\n> > > > configuration file from '\"\n> > > > > + << confPath << \"'\";\n> > > > > +\n> > > > > + ret = stat(confPath.c_str(), &statbuf);\n> > > > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) ==\n> > S_IFREG)\n> > > >\n> > > > Will this stop configuration files being a symlink to an alternative\n> > > > file? (S_IFREG vs S_IFLINK) ? Does that even matter?\n> > > >\n> > > > We could do:\n> > > >\n> > > > if (File::exists(confPath))\n> > > > return confPath;\n> > > >\n> > > > That only checks that the 'name' is not a directory... So ... it's\n> > > > different again, though I would expect the\n> >\n> > \"I would expect the user of the confPath to use the File class, so it\n> > should be appropriate\" ... was supposed to be written here ;)\n> >\n> > >\n> > >\n> > > I was copying the implementation of IPAProxy::configurationFile()\n> > > for this function to retain consistency, but could change if other folks\n> > > agree?\n> >\n> > If it's better we can fix the configurationFile location too. But both\n> > seem functional anyway.\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > >\n> > > >\n> > > >\n> > > >\n> > > > > + return confPath;\n> > > > > + } else {\n> > > > > + /* Else look in the system locations. */\n> > > > > + confPath = std::string(LIBCAMERA_DATA_DIR)\n> > > > > + + \"/pipeline/\" + subdir + '/' + name;\n> > > > > + ret = stat(confPath.c_str(), &statbuf);\n> > > > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) ==\n> > S_IFREG)\n> > > > > + return confPath;\n> > > > > + }\n> > > > > +\n> > > > > + LOG(Pipeline, Error)\n> > > > > + << \"Configuration file '\" << confPath\n> > > > > + << \"' not found for pipeline handler '\" <<\n> > > > PipelineHandler::name() << \"'\";\n> > > >\n> > > > This sounds like it suddenly makes configuration files mandatory for\n> > all\n> > > > pipelines ?\n> > > >\n> > > > (or perhaps only if a pipeline looks for one ?)\n> > > >\n> > >\n> > > Correct, the config file is only mandatory if a pipeline handler\n> > explicitly\n> > > looks for one.\n> > >\n> >\n> > Would you expect pipelines to have a default configuration for all\n> > options?\n> >\n> > Anyway, while it reports an error, it's up to the pipeline handler to\n> > decide how to respond if there's no file.\n> >\n> \n> In an earlier version of the patch, we did have a default.yaml file that\n> loaded\n> a default configuration if no other file was provided. However, Laurent\n> suggested\n> (and I agree) that we remove that and hard-code the default configuration\n> in the\n> source itself.\n\nAbsolutely, yes I think the defaults should be handled by the pipeline\nhandler itself.\n--\nKieran\n\n\n> As you said, it's up to the individual pipeline handlers to decide the\n> behavior.\n> \n> Naush\n> \n> \n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > >\n> > > > > +\n> > > > > + return std::string();\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Register a camera to the camera manager and pipeline\n> > handler\n> > > > > * \\param[in] camera The camera to be added\n> > > > > --\n> > > > > 2.25.1\n> > > > >\n> > > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7E5EDBD87C\n\tfor ;\n\tTue, 17 Jan 2023 09:56:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA1F4625D8;\n\tTue, 17 Jan 2023 10:56:53 +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 2D9B461EFE\n\tfor ;\n\tTue, 17 Jan 2023 10:56:51 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A80E510C;\n\tTue, 17 Jan 2023 10:56:50 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1673949413;\n\tbh=EVumALC0kp5yesOOcuulFKL67ohn/sZr8+aVRNSQM5o=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=MCqi4eLNCbQefFiHblw6cx+eUTEFsRlYm9uur7GO6AfInP2W6T1JNIRFTpp+qc92v\n\tl5rRZlHeFbp64t407s8c+DcI413xtah/cM4fceq/Z5vvb3Vp5D3ca9fauQkJ2ENi+a\n\twvClagH8eZjOPAo31KBH2+vdQDNIgOx56RrWDc9+LvBWyXgFVUQmN5N6sGvGhcVC6D\n\tt3l9Jnf11/BzkmdF8EpYpm6NMr2JQefA/buXMKTQAaFK5Uox62Ve3ITq5yrHS8RL9w\n\tCFmFXjxsh2S3RJhFlMQLY33CVhbrMhaB0FfcI/7TvdHM6VVPik1oqPDeJOndebQ1qR\n\tkp+2g32FK0HwA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1673949410;\n\tbh=EVumALC0kp5yesOOcuulFKL67ohn/sZr8+aVRNSQM5o=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=mvG8LWgUP+/BGO8DZV7d7VVN6uvdqV4D92nzb7pt33p+/+KbxlibkpeotjbOzqtFd\n\tPwwxp+OE0dQa96KYR663YaF7r+2ZQRJ0qzmV0hAU5c4fZl6ih9HhBGZhZHXhh8ksdG\n\tibaWehoo/zUYrdySt3Xiz5v/CKsD3CBbiGnOSVZ4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mvG8LWgU\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221209090050.19441-1-naush@raspberrypi.com>\n\t<20221209090050.19441-3-naush@raspberrypi.com>\n\t<167388951896.42371.15280676491639567975@Monstersaurus>\n\t\n\t<167394844788.42371.16246058578541702614@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Tue, 17 Jan 2023 09:56:48 +0000","Message-ID":"<167394940800.3184615.17362721039831144590@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a\n\tplatform configuration file helper","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]