[{"id":25968,"web_url":"https://patchwork.libcamera.org/comment/25968/","msgid":"","date":"2022-12-02T11:42:33","subject":"Re: [libcamera-devel] [PATCH v2 02/10] pipeline: raspberrypi: Add a\n\tpipeline config structure","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:26PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Add a configuration structure to store platform specific parameters used by\n> the pipeline handler. Currently, these only store Unicam buffer counts,\n> replacing the hardcoded static values in the source code.\n\nOne question here, does the configuration apply to the pipeline, or to\nthe camera ? If multiple sensors are connected to the same Unicam\ninstance (through an external multiplexers), will all these cameras\nalways have the same configuration, or are there use cases for\nper-camera configuration ? Looking at the series, the configuration is\nstored per-camera, but is the same for all cameras handled by the\npipeline handler.\n\nThe answer to that question matters for the implementation in the\nRaspberry Pi pipeline handler, but also in the common helpers in case\nother platforms would need per-camera configurations.\n\n> In subsequent commits, more parameters will be added to the configuration\n> structure, and parameters will be read in through a config file.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++++---\n> 1 file changed, 44 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0e0b71945640..4486d31ea78d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -294,6 +294,21 @@ public:\n> \t/* Have internal buffers been allocated? */\n> \tbool buffersAllocated_;\n> \n> +\tstruct Config {\n> +\t\t/*\n> +\t\t * The minimum number of internal buffers to be allocated for\n> +\t\t * the Unicam Image stream.\n> +\t\t */\n> +\t\tunsigned int minUnicamBuffers;\n> +\t\t/*\n> +\t\t * The minimum total (internal + external) buffer count used for\n> +\t\t * the Unicam Image steram.\n\ns/steram/stream/\n\n> +\t\t */\n> +\t\tunsigned int minTotalUnicamBuffers;\n> +\t};\n> +\n> +\tConfig config_;\n> +\n> private:\n> \tvoid checkRequestCompleted();\n> \tvoid fillRequestMetadata(const ControlList &bufferControls,\n> @@ -346,6 +361,7 @@ private:\n> \t}\n> \n> \tint registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);\n> +\tint configurePipelineHandler(RPiCameraData *data);\n\nWouldn't this be better placed in the RPiCameraData class ? I would also\nname the function loadPipelineHandlerConfiguration() (or similar) to\nmake its purpose clearer.\n\nI wonder if we shouldn't instead talk about \"pipeline configuration\"\ninstead of \"pipeline handler configuration\", especially if the\nconfiguration is specific to a camera. What does everybody think ?\n\n> \tint queueAllBuffers(Camera *camera);\n> \tint prepareBuffers(Camera *camera);\n> \tvoid mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);\n> @@ -1377,6 +1393,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> \tstreams.insert(&data->isp_[Isp::Output0]);\n> \tstreams.insert(&data->isp_[Isp::Output1]);\n> \n> +\tint ret = configurePipelineHandler(data.get());\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error) << \"Unable to configure the pipeline handler!\";\n\n\"Unable to load pipeline handler configuration\"\n\n> +\t\treturn ret;\n> +\t}\n> +\n> \t/* Create and register the camera. */\n> \tconst std::string &id = data->sensor_->id();\n> \tstd::shared_ptr camera =\n> @@ -1389,6 +1411,18 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> \treturn 0;\n> }\n> \n> +int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> +{\n> +\tRPiCameraData::Config &config = data->config_;\n> +\n> +\tconfig = {\n> +\t\t.minUnicamBuffers = 2,\n> +\t\t.minTotalUnicamBuffers = 4,\n> +\t};\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> @@ -1439,25 +1473,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \tfor (auto const stream : data->streams_) {\n> \t\tunsigned int numBuffers;\n> \t\t/*\n> -\t\t * For Unicam, allocate a minimum of 4 buffers as we want\n> -\t\t * to avoid any frame drops.\n> +\t\t * For Unicam, allocate a minimum number of buffers for internal\n> +\t\t * use as we want to avoid any frame drops.\n> \t\t */\n> -\t\tconstexpr unsigned int minBuffers = 4;\n> +\t\tconst unsigned int minBuffers = data->config_.minTotalUnicamBuffers;\n> \t\tif (stream == &data->unicam_[Unicam::Image]) {\n> \t\t\t/*\n> \t\t\t * If an application has configured a RAW stream, allocate\n> \t\t\t * additional buffers to make up the minimum, but ensure\n> -\t\t\t * we have at least 2 sets of internal buffers to use to\n> -\t\t\t * minimise frame drops.\n> +\t\t\t * we have at least minUnicamBuffers sets of internal\n\nI'd drop 'sets' here as it's not two buffer sets but two buffers.\n\n> +\t\t\t * buffers to use to minimise frame drops.\n> \t\t\t */\n> -\t\t\tnumBuffers = std::max(2, minBuffers - numRawBuffers);\n> +\t\t\tnumBuffers = std::max(data->config_.minUnicamBuffers,\n> +\t\t\t\t\t\t minBuffers - numRawBuffers);\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\n> \t\t\t * similar logic as above to count all the RAW buffers\n> \t\t\t * available.\n> \t\t\t */\n> -\t\t\tnumBuffers = numRawBuffers + std::max(2, minBuffers - numRawBuffers);\n> +\t\t\tnumBuffers = numRawBuffers +\n> +\t\t\t\t std::max(data->config_.minUnicamBuffers,\n> +\t\t\t\t\t\t minBuffers - numRawBuffers);\n> \n> \t\t} else if (stream == &data->unicam_[Unicam::Embedded]) {\n> \t\t\t/*","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 B146DBE08B\n\tfor ;\n\tFri, 2 Dec 2022 11:42:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D8B76333F;\n\tFri, 2 Dec 2022 12:42:38 +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 DA95260483\n\tfor ;\n\tFri, 2 Dec 2022 12:42:35 +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 3CF4D6E0;\n\tFri, 2 Dec 2022 12:42:35 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669981358;\n\tbh=hawsJnAteGjFzOSLz61zXm1dj8x1TI7pZ/Ni0VxuQz4=;\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=BcCyaPEFJKPYSH2IC5KwQlbOrbYPgeISgQPuqW/7+Axx9c81ZPQdOFR7KwVOJNJIs\n\ty3tASYkWOcoa9yx6u9RsnP1D7XmCgGu5INsr7lh7cbB+RklNdsH8K8kVcV2MCGVzZa\n\thN7bu2/IOvsIGetQrmBLGsriuBEpEvK5h7AT/14zU2BNYQkZf9NIpTYsKTdDqq+Tor\n\tIb8+HxVlTcZxX+eTOIyaTIgOqt8eViJRtnKGtKqyWEL/DqzvbqIo5S882GRgg8QmO8\n\tT1AfQsW5g2kD9zFqHlWfdfh8GGRnW9kzav8gT7+jiPI417JPmZNfTMOlD31uJ/ft8E\n\tWcKAZ/OSrF6Xg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669981355;\n\tbh=hawsJnAteGjFzOSLz61zXm1dj8x1TI7pZ/Ni0VxuQz4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mFFZ9kkf2Gj7bPZnvxnQjF2jJJgJrmPdbz1PTl2ajN07rGA1a2oiFKpfYvdgPhLtD\n\t+/6ApzzVHO07YZhVZFl7QGNw4i8KUdgzCZkut4Bv7UbuKX0T0IsgwWtRs2QJarqrs8\n\t+zzy/thw7kymvq4CnF192Rirn26Ck1hRG3VGWZ5E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mFFZ9kkf\"; dkim-atps=neutral","Date":"Fri, 2 Dec 2022 13:42:33 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221129134534.2933-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] pipeline: raspberrypi: Add a\n\tpipeline config structure","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":25971,"web_url":"https://patchwork.libcamera.org/comment/25971/","msgid":"","date":"2022-12-02T13:01:46","subject":"Re: [libcamera-devel] [PATCH v2 02/10] pipeline: raspberrypi: Add a\n\tpipeline config structure","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThanks for the feedback!\n\n\nOn Fri, 2 Dec 2022 at 11:42, 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:26PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > Add a configuration structure to store platform specific parameters used\n> by\n> > the pipeline handler. Currently, these only store Unicam buffer counts,\n> > replacing the hardcoded static values in the source code.\n>\n> One question here, does the configuration apply to the pipeline, or to\n> the camera ? If multiple sensors are connected to the same Unicam\n> instance (through an external multiplexers), will all these cameras\n> always have the same configuration, or are there use cases for\n> per-camera configuration ? Looking at the series, the configuration is\n> stored per-camera, but is the same for all cameras handled by the\n> pipeline handler.\n>\n\nThe intention is to allow us to run alternate configurations per-camera.\nWith this series, raspberrypi/default.json is loaded, unless the env\nvariable\nLIBCAMERA_RPI_CONFIG_FILE is set for the process. Admittedly this\nis not the neatest way of doing things. We did alway talk about a mechanism\nfor the application to set the IPA tuning file via some API. Whatever that\nAPI\nmay be, I expect the pipeline handler config to follow a similar mechanism.\n\n\n>\n> The answer to that question matters for the implementation in the\n> Raspberry Pi pipeline handler, but also in the common helpers in case\n> other platforms would need per-camera configurations.\n>\n\nWithout any sort of override mechanism, this is indeed a problem.\n\n\n>\n> > In subsequent commits, more parameters will be added to the configuration\n> > structure, and parameters will be read in through a config file.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++++---\n> > 1 file changed, 44 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 0e0b71945640..4486d31ea78d 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -294,6 +294,21 @@ public:\n> > /* Have internal buffers been allocated? */\n> > bool buffersAllocated_;\n> >\n> > + struct Config {\n> > + /*\n> > + * The minimum number of internal buffers to be allocated\n> for\n> > + * the Unicam Image stream.\n> > + */\n> > + unsigned int minUnicamBuffers;\n> > + /*\n> > + * The minimum total (internal + external) buffer count\n> used for\n> > + * the Unicam Image steram.\n>\n> s/steram/stream/\n>\n> > + */\n> > + unsigned int minTotalUnicamBuffers;\n> > + };\n> > +\n> > + Config config_;\n> > +\n> > private:\n> > void checkRequestCompleted();\n> > void fillRequestMetadata(const ControlList &bufferControls,\n> > @@ -346,6 +361,7 @@ private:\n> > }\n> >\n> > int registerCamera(MediaDevice *unicam, MediaDevice *isp,\n> MediaEntity *sensorEntity);\n> > + int configurePipelineHandler(RPiCameraData *data);\n>\n> Wouldn't this be better placed in the RPiCameraData class ? I would also\n> name the function loadPipelineHandlerConfiguration() (or similar) to\n> make its purpose clearer.\n>\n\nAck\n\n\n>\n> I wonder if we shouldn't instead talk about \"pipeline configuration\"\n> instead of \"pipeline handler configuration\", especially if the\n> configuration is specific to a camera. What does everybody think ?\n>\n\nYes, I agree with that. Pipeline configuration does sound more appropriate.\n\nRegards,\nNaush\n\n\n\n> > int queueAllBuffers(Camera *camera);\n> > int prepareBuffers(Camera *camera);\n> > void mapBuffers(Camera *camera, const RPi::BufferMap &buffers,\n> unsigned int mask);\n> > @@ -1377,6 +1393,12 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > streams.insert(&data->isp_[Isp::Output0]);\n> > streams.insert(&data->isp_[Isp::Output1]);\n> >\n> > + int ret = configurePipelineHandler(data.get());\n> > + if (ret) {\n> > + LOG(RPI, Error) << \"Unable to configure the pipeline\n> handler!\";\n>\n> \"Unable to load pipeline handler configuration\"\n>\n> > + return ret;\n> > + }\n> > +\n> > /* Create and register the camera. */\n> > const std::string &id = data->sensor_->id();\n> > std::shared_ptr camera =\n> > @@ -1389,6 +1411,18 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > return 0;\n> > }\n> >\n> > +int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > +{\n> > + RPiCameraData::Config &config = data->config_;\n> > +\n> > + config = {\n> > + .minUnicamBuffers = 2,\n> > + .minTotalUnicamBuffers = 4,\n> > + };\n> > +\n> > + return 0;\n> > +}\n> > +\n> > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > @@ -1439,25 +1473,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera\n> *camera)\n> > for (auto const stream : data->streams_) {\n> > unsigned int numBuffers;\n> > /*\n> > - * For Unicam, allocate a minimum of 4 buffers as we want\n> > - * to avoid any frame drops.\n> > + * For Unicam, allocate a minimum number of buffers for\n> internal\n> > + * use as we want to avoid any frame drops.\n> > */\n> > - constexpr unsigned int minBuffers = 4;\n> > + const unsigned int minBuffers =\n> data->config_.minTotalUnicamBuffers;\n> > if (stream == &data->unicam_[Unicam::Image]) {\n> > /*\n> > * If an application has configured a RAW stream,\n> allocate\n> > * additional buffers to make up the minimum, but\n> ensure\n> > - * we have at least 2 sets of internal buffers to\n> use to\n> > - * minimise frame drops.\n> > + * we have at least minUnicamBuffers sets of\n> internal\n>\n> I'd drop 'sets' here as it's not two buffer sets but two buffers.\n>\n> > + * buffers to use to minimise frame drops.\n> > */\n> > - numBuffers = std::max(2, minBuffers -\n> numRawBuffers);\n> > + numBuffers =\n> std::max(data->config_.minUnicamBuffers,\n> > + minBuffers -\n> numRawBuffers);\n> > } else if (stream == &data->isp_[Isp::Input]) {\n> > /*\n> > * ISP input buffers are imported from Unicam, so\n> follow\n> > * similar logic as above to count all the RAW\n> buffers\n> > * available.\n> > */\n> > - numBuffers = numRawBuffers + std::max(2,\n> minBuffers - numRawBuffers);\n> > + numBuffers = numRawBuffers +\n> > +\n> std::max(data->config_.minUnicamBuffers,\n> > + minBuffers -\n> numRawBuffers);\n> >\n> > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > /*\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0AB35BDE6B\n\tfor ;\n\tFri, 2 Dec 2022 13:02:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B1416333F;\n\tFri, 2 Dec 2022 14:02:04 +0100 (CET)","from mail-io1-xd35.google.com (mail-io1-xd35.google.com\n\t[IPv6:2607:f8b0:4864:20::d35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED00D60483\n\tfor ;\n\tFri, 2 Dec 2022 14:02:02 +0100 (CET)","by mail-io1-xd35.google.com with SMTP id d18so3007319iof.6\n\tfor ;\n\tFri, 02 Dec 2022 05:02:02 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669986124;\n\tbh=jfsoaYW5AbsJDFiTH3hy3qurGgIVXYzxQO3pI8Dn6nA=;\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=hSGnOG5cW7/D1g+d4gmEZwI5KreeQ+Q0ItTR6cXMGPJ9aF6EQEJnI2eXY3bfPPnqZ\n\tWtdanLEib78gGo1IT77T/9cEtqrYsdj41+K0C4EtuwVtZgDJaW0VjrTPUirxo7P1iK\n\tSxi/ngYmZTLwXviPY1BCmWt/nS0W18q1ET0q/eLyFnyMtUsm13WMCmvidniNB+oC6A\n\tkItRYHyK1aCeBbb7RhJj2meW2cEd+OGSabvYteNxtjiovDF4uLfwKrITq3Az9joCEj\n\tcObms86eODFpxyysT91/Y3xLjT4hwIG+mWW2kEZ6aDM9hr+Z/ZKaTHh40EhHsboru3\n\tN6S6sbtVRmlcQ==","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=+Kr6cwH6WvT2J+WCN9P28EqaRVcdjCB5ffp8JqvPxOk=;\n\tb=sWuLbp2fTipJIOak/PIg3+k3ldm3Q+Izdkkd9InH1yKyMWHnknLyW2JHHcNCl2ZG+O\n\tzzfg34taPxK9mCSIotubbPV3buzcSbqnuLURiXuVL/19MX9gQFVJLTAtjUZAu9QUlfon\n\tnzd9paysW4yXJSYPuKRBMss9j4hU+B8h0Q+7V9RJCzi5kRRXjYrhOBU9XUTk54T2HMta\n\tqhJKhbkv5iyDzjjyRQ7KKCc4Sy81QVRcrQBHA7jHUFPlDzjACtqoImwYOqa0yUIZsyBS\n\twup6m2rEE4gYxP7bnYmVQ51Z1Dm9S+bgqfP52ViM6Q8az7hPqGIXWgTXrpJwdWB2rFzh\n\t7lLw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sWuLbp2f\"; 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=+Kr6cwH6WvT2J+WCN9P28EqaRVcdjCB5ffp8JqvPxOk=;\n\tb=Zkj1LwetC1MVXMVX0RuSmBpGQIde/SwAL9U3CQQTDYq2XSXWbYGZzrYIkdTZirK70m\n\tcF0XM0pah0fGe0st7egKC7mUgTqsr6+eHljClUfOfMyxEgR1pl4CbnlzVrUSE1BMUBsD\n\tHP54/O8OlT0qYfx12vtHP3Z3lXulAQEcR93i6MCqEhoQfxJXdqoRSkEu/Ug/IRgLYOQq\n\tqkjREOdajHCjIMsgtf7awyF2bJwi4QnrscKKz54n8Pg33UOk258ToV2SFPQpqJgoELWH\n\tRMrC0ixL5fw893WBQW1+2SmBvX0HiiqFMS9S+jentRZBnVePOkSxt0CLIJOjhQqmb4g4\n\tVxYw==","X-Gm-Message-State":"ANoB5pn7rc17FVBEh+JduiMR3d/UYWFY6x/j4nNT87+zlbZwjJAfkMVN\n\tqscqozh87/qSvXo2+aGMhle6QU3msWhK7ZpDmSqv/WF6mTVowIgF","X-Google-Smtp-Source":"AA0mqf70eswVSCDhdogyGe2A9kReIzStzHJuEnFdfrOb+b41ZLrAWKdcJGAL32QSbzkkIgGRg03OssmOCHL+9XOt1lw=","X-Received":"by 2002:a05:6638:182:b0:389:d566:91d with SMTP id\n\ta2-20020a056638018200b00389d566091dmr13974954jaq.266.1669986121157;\n\tFri, 02 Dec 2022 05:02:01 -0800 (PST)","MIME-Version":"1.0","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Fri, 2 Dec 2022 13:01:46 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000088272d05eed7f159\"","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] pipeline: raspberrypi: Add a\n\tpipeline config structure","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":25988,"web_url":"https://patchwork.libcamera.org/comment/25988/","msgid":"","date":"2022-12-02T15:59:55","subject":"Re: [libcamera-devel] [PATCH v2 02/10] pipeline: raspberrypi: Add a\n\tpipeline config structure","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:01:46PM +0000, Naushir Patuck wrote:\n> On Fri, 2 Dec 2022 at 11:42, Laurent Pinchart wrote:\n> > On Tue, Nov 29, 2022 at 01:45:26PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > > Add a configuration structure to store platform specific parameters used by\n> > > the pipeline handler. Currently, these only store Unicam buffer counts,\n> > > replacing the hardcoded static values in the source code.\n> >\n> > One question here, does the configuration apply to the pipeline, or to\n> > the camera ? If multiple sensors are connected to the same Unicam\n> > instance (through an external multiplexers), will all these cameras\n> > always have the same configuration, or are there use cases for\n> > per-camera configuration ? Looking at the series, the configuration is\n> > stored per-camera, but is the same for all cameras handled by the\n> > pipeline handler.\n> \n> The intention is to allow us to run alternate configurations per-camera.\n> With this series, raspberrypi/default.json is loaded, unless the env variable\n> LIBCAMERA_RPI_CONFIG_FILE is set for the process. Admittedly this\n> is not the neatest way of doing things. We did alway talk about a mechanism\n> for the application to set the IPA tuning file via some API. Whatever that API\n> may be, I expect the pipeline handler config to follow a similar mechanism.\n\nOK, we can leave it as-is, implementing it per-camera in the pipeline\nhandler, but limiting the mechanism to a configuration file that will be\nthe same for all cameras for now.\n\n> > The answer to that question matters for the implementation in the\n> > Raspberry Pi pipeline handler, but also in the common helpers in case\n> > other platforms would need per-camera configurations.\n> \n> Without any sort of override mechanism, this is indeed a problem.\n> \n> > > In subsequent commits, more parameters will be added to the configuration\n> > > structure, and parameters will be read in through a config file.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++++---\n> > > 1 file changed, 44 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 0e0b71945640..4486d31ea78d 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -294,6 +294,21 @@ public:\n> > > /* Have internal buffers been allocated? */\n> > > bool buffersAllocated_;\n> > >\n> > > + struct Config {\n> > > + /*\n> > > + * The minimum number of internal buffers to be allocated for\n> > > + * the Unicam Image stream.\n> > > + */\n> > > + unsigned int minUnicamBuffers;\n> > > + /*\n> > > + * The minimum total (internal + external) buffer count used for\n> > > + * the Unicam Image steram.\n> >\n> > s/steram/stream/\n> >\n> > > + */\n> > > + unsigned int minTotalUnicamBuffers;\n> > > + };\n> > > +\n> > > + Config config_;\n> > > +\n> > > private:\n> > > void checkRequestCompleted();\n> > > void fillRequestMetadata(const ControlList &bufferControls,\n> > > @@ -346,6 +361,7 @@ private:\n> > > }\n> > >\n> > > int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);\n> > > + int configurePipelineHandler(RPiCameraData *data);\n> >\n> > Wouldn't this be better placed in the RPiCameraData class ? I would also\n> > name the function loadPipelineHandlerConfiguration() (or similar) to\n> > make its purpose clearer.\n> \n> Ack\n> \n> > I wonder if we shouldn't instead talk about \"pipeline configuration\"\n> > instead of \"pipeline handler configuration\", especially if the\n> > configuration is specific to a camera. What does everybody think ?\n> >\n> \n> Yes, I agree with that. Pipeline configuration does sound more appropriate.\n> \n> > > int queueAllBuffers(Camera *camera);\n> > > int prepareBuffers(Camera *camera);\n> > > void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);\n> > > @@ -1377,6 +1393,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > streams.insert(&data->isp_[Isp::Output0]);\n> > > streams.insert(&data->isp_[Isp::Output1]);\n> > >\n> > > + int ret = configurePipelineHandler(data.get());\n> > > + if (ret) {\n> > > + LOG(RPI, Error) << \"Unable to configure the pipeline handler!\";\n> > \"Unable to load pipeline handler configuration\"\n> >\n> > > + return ret;\n> > > + }\n> > > +\n> > > /* Create and register the camera. */\n> > > const std::string &id = data->sensor_->id();\n> > > std::shared_ptr camera =\n> > > @@ -1389,6 +1411,18 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > return 0;\n> > > }\n> > >\n> > > +int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)\n> > > +{\n> > > + RPiCameraData::Config &config = data->config_;\n> > > +\n> > > + config = {\n> > > + .minUnicamBuffers = 2,\n> > > + .minTotalUnicamBuffers = 4,\n> > > + };\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > {\n> > > RPiCameraData *data = cameraData(camera);\n> > > @@ -1439,25 +1473,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > for (auto const stream : data->streams_) {\n> > > unsigned int numBuffers;\n> > > /*\n> > > - * For Unicam, allocate a minimum of 4 buffers as we want\n> > > - * to avoid any frame drops.\n> > > + * For Unicam, allocate a minimum number of buffers for internal\n> > > + * use as we want to avoid any frame drops.\n> > > */\n> > > - constexpr unsigned int minBuffers = 4;\n> > > + const unsigned int minBuffers = data->config_.minTotalUnicamBuffers;\n> > > if (stream == &data->unicam_[Unicam::Image]) {\n> > > /*\n> > > * If an application has configured a RAW stream, allocate\n> > > * additional buffers to make up the minimum, but ensure\n> > > - * we have at least 2 sets of internal buffers to use to\n> > > - * minimise frame drops.\n> > > + * we have at least minUnicamBuffers sets of internal\n> >\n> > I'd drop 'sets' here as it's not two buffer sets but two buffers.\n> >\n> > > + * buffers to use to minimise frame drops.\n> > > */\n> > > - numBuffers = std::max(2, minBuffers - numRawBuffers);\n> > > + numBuffers = std::max(data->config_.minUnicamBuffers,\n> > > + minBuffers - numRawBuffers);\n> > > } else if (stream == &data->isp_[Isp::Input]) {\n> > > /*\n> > > * ISP input buffers are imported from Unicam, so follow\n> > > * similar logic as above to count all the RAW buffers\n> > > * available.\n> > > */\n> > > - numBuffers = numRawBuffers + std::max(2, minBuffers - numRawBuffers);\n> > > + numBuffers = numRawBuffers +\n> > > + std::max(data->config_.minUnicamBuffers,\n> > > + minBuffers - numRawBuffers);\n> > >\n> > > } else if (stream == &data->unicam_[Unicam::Embedded]) {\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 84295BE08B\n\tfor ;\n\tFri, 2 Dec 2022 15:59:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33F3C6333F;\n\tFri, 2 Dec 2022 16:59:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7AB4B60483\n\tfor ;\n\tFri, 2 Dec 2022 16:59:57 +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 F2C816E0;\n\tFri, 2 Dec 2022 16:59:56 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669996799;\n\tbh=iMJfSFpn/1GzlHMOVV/0+b+z33fE8fx5qfBH02PH7GA=;\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=AtLbdsMi0KfEgYEhN1VLMOVD+lgp6BM3MGsm6KJ1PrMhP1jpHDQxSEEoHqb4GbhFl\n\t73OptMGeufBfKXFWS4qg2KxRcpGyknTl61Sv2o9ucPc6Cfqx68S2WxOWphTxJsJhwD\n\t1+6uUm9RbynNj8Qwaf5KZincr4Lc02lNEPZZZllOSfDGnwTLOXlHyuIQYbGqKq16wV\n\tQDeJGQkTNklltybcot8yjx84Nbc4np+ytElmsz8CZ8adBbhpMHZ0SqJ3doPAE2DGWF\n\tv7Wp5czEWilNzwxcBP+z8Uv05mOK90V7aTLeRTUpIh+n9mZbclRMrWHAKuxUji7LAQ\n\t7iVkZBEjjm1zg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669996797;\n\tbh=iMJfSFpn/1GzlHMOVV/0+b+z33fE8fx5qfBH02PH7GA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PVi7HEzFdAO9ohNRe6ROTh2GK4ed5+quPRibAXwbS7vLu3s42vBYcrLvq6s95ffe5\n\t2NxRKPv/g/Y1eeoJxDsEHxF99JwQb9s1EOqA8Md7lrWLuqQzU2jEZ57X90F1/FVIm8\n\tkjFPGdttn/JTKRJ/CRfesRi4J9qQMagwpRYZRs30="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PVi7HEzF\"; dkim-atps=neutral","Date":"Fri, 2 Dec 2022 17:59:55 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20221129134534.2933-1-naush@raspberrypi.com>\n\t<20221129134534.2933-3-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 02/10] pipeline: raspberrypi: Add a\n\tpipeline config structure","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\" "}}]