[{"id":26276,"web_url":"https://patchwork.libcamera.org/comment/26276/","msgid":"<167421186370.42371.15907607251381425817@Monstersaurus>","date":"2023-01-20T10:51:03","subject":"Re: [libcamera-devel] [PATCH v5 03/12] pipeline: raspberrypi: Add a\n\tpipeline config structure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44)\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> 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 | 49 ++++++++++++++++---\n> 1 file changed, 42 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8569df17976a..6bf9a669c679 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -199,6 +199,7 @@ public:\n> \n> int loadIPA(ipa::RPi::IPAInitResult *result);\n> int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> + int configurePipeline();\n> \n> void enumerateVideoDevices(MediaLink *link);\n> \n> @@ -295,6 +296,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 stream.\n\nAre there any limits that should be conveyed by this documentation?\nLike should minTotalUnicamBuffers always be recommended to at least 2?\n\nCan minTotalUnicamBuffers be less than minUnicamBuffers? If not - should\nthis convey instead the quantity of buffers to allocate in addition to\nminUnicamBuffers instead?\n\n> + */\n> + unsigned int minTotalUnicamBuffers;\n> + };\n> +\n> + Config config_;\n> +\n> private:\n> void checkRequestCompleted();\n> void fillRequestMetadata(const ControlList &bufferControls,\n> @@ -1378,6 +1394,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 = data->configurePipeline();\n\nGlancing above - would the number of supported streams ever be\nconfigurable, requiring this to load earlier ?\n\nOf course such a change could load earlier as part of that so this is\nfine by me.\n\nReviewed-by: Kieran Bingham \n\n\n> + if (ret) {\n> + LOG(RPI, Error) << \"Unable to load pipeline handler configuration\";\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> @@ -1440,25 +1462,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 of internal buffers\n> + * 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> /*\n> @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> return 0;\n> }\n> \n> +int RPiCameraData::configurePipeline()\n> +{\n> + config_ = {\n> + .minUnicamBuffers = 2,\n> + .minTotalUnicamBuffers = 4,\n> + };\n> +\n> + return 0;\n> +}\n> +\n> /*\n> * enumerateVideoDevices() iterates over the Media Controller topology, starting\n> * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores\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 E36F4C3240\n\tfor ;\n\tFri, 20 Jan 2023 10:51:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E94F625E4;\n\tFri, 20 Jan 2023 11:51:07 +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 1C0736045E\n\tfor ;\n\tFri, 20 Jan 2023 11:51:06 +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 9755E514;\n\tFri, 20 Jan 2023 11:51:05 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674211867;\n\tbh=cNvApZqs9LH2zMzgB4/mbt/TPE79RaaJAvLXPlolIbU=;\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=BfAdgmwvWaKuMKpM3StYxHFAovLSLyZYOAGMJbHvnj3UMoFEqIX9In+RMO9bwByAI\n\tQJvP6DyabkZoqDuQHceqKwdUAESgxKSzwV21UquuTjFLNdsuotclL49odP9ABSeDjZ\n\td0knwhuWesjjKqcfNb7i/3dZh+EGioqlKN7hUeMrtqAdrgTiifhGfy5ozeKZQGXRrG\n\tNlnF7Wo+xR2P8Y/P7ss4HRs5j3YD55P0rTUq8+PaGr1CwjGFMvQ6S7JidZxzrLPKqo\n\tKGA/B5T04rgyBWag9YoLbJvC12srtK0HeVk8fLNvodTD8jdDgp+mwI2SSSPqCPiev1\n\tByii9TcJyHC3g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674211865;\n\tbh=cNvApZqs9LH2zMzgB4/mbt/TPE79RaaJAvLXPlolIbU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=szQ/hXCekWkhzkQLJqwwXyrvowvtUV9Mfr9Rfkoldcg1sC3uY5ISBiW/NBU6Sd5JX\n\tWGeUneKEjA5u0V3Spwhaw+FUZ+pNpsFfH2DSqSBDtxYbrS+WdjvBIS6rfFFCuPyL97\n\tLhZC5mC+p/ncBVz6puLBK8YFLkQTvclwIxcw0Ha4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"szQ/hXCe\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230118085953.7027-4-naush@raspberrypi.com>","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-4-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 20 Jan 2023 10:51:03 +0000","Message-ID":"<167421186370.42371.15907607251381425817@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26289,"web_url":"https://patchwork.libcamera.org/comment/26289/","msgid":"","date":"2023-01-20T11:37:13","subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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 Kieran,\n\nThank you for your feedback!\n\nOn Fri, 20 Jan 2023 at 10:51, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44)\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> > 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 | 49 ++++++++++++++++---\n> > 1 file changed, 42 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 8569df17976a..6bf9a669c679 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -199,6 +199,7 @@ public:\n> >\n> > int loadIPA(ipa::RPi::IPAInitResult *result);\n> > int configureIPA(const CameraConfiguration *config,\n> ipa::RPi::IPAConfigResult *result);\n> > + int configurePipeline();\n> >\n> > void enumerateVideoDevices(MediaLink *link);\n> >\n> > @@ -295,6 +296,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\n> allocated 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 stream.\n>\n> Are there any limits that should be conveyed by this documentation?\n> Like should minTotalUnicamBuffers always be recommended to at least 2?\n>\n\nminTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >=\nminUnicamBuffers\n\nThis is tested when we read the config file\nin RPiCameraData::configurePipeline(), but\nperhaps a little comment in the config files might be needed?\n\n\n>\n> Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should\n> this convey instead the quantity of buffers to allocate in addition to\n> minUnicamBuffers instead?\n>\n> > + */\n> > + unsigned int minTotalUnicamBuffers;\n> > + };\n> > +\n> > + Config config_;\n> > +\n> > private:\n> > void checkRequestCompleted();\n> > void fillRequestMetadata(const ControlList &bufferControls,\n> > @@ -1378,6 +1394,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 = data->configurePipeline();\n>\n> Glancing above - would the number of supported streams ever be\n> configurable, requiring this to load earlier ?\n>\n\nNo, I think that would be fixed at compile time.\n\nRegards,\nNaush\n\n\n>\n> Of course such a change could load earlier as part of that so this is\n> fine by me.\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > + if (ret) {\n> > + LOG(RPI, Error) << \"Unable to load pipeline handler\n> configuration\";\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> > @@ -1440,25 +1462,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\n> stream, allocate\n> > * additional buffers to make up the minimum,\n> but ensure\n> > - * we have at least 2 sets of internal buffers\n> to use to\n> > - * minimise frame drops.\n> > + * we have at least minUnicamBuffers of internal\n> buffers\n> > + * 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,\n> so 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> > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config, ipa::RPi::IPA\n> > return 0;\n> > }\n> >\n> > +int RPiCameraData::configurePipeline()\n> > +{\n> > + config_ = {\n> > + .minUnicamBuffers = 2,\n> > + .minTotalUnicamBuffers = 4,\n> > + };\n> > +\n> > + return 0;\n> > +}\n> > +\n> > /*\n> > * enumerateVideoDevices() iterates over the Media Controller topology,\n> starting\n> > * at the sensor and finishing at Unicam. For each sensor,\n> RPiCameraData stores\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 EDD65BE175\n\tfor ;\n\tFri, 20 Jan 2023 11:37:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CF23625E4;\n\tFri, 20 Jan 2023 12:37:32 +0100 (CET)","from mail-yb1-xb2e.google.com (mail-yb1-xb2e.google.com\n\t[IPv6:2607:f8b0:4864:20::b2e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B18B361EFD\n\tfor ;\n\tFri, 20 Jan 2023 12:37:30 +0100 (CET)","by mail-yb1-xb2e.google.com with SMTP id d62so6287039ybh.8\n\tfor ;\n\tFri, 20 Jan 2023 03:37:30 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674214652;\n\tbh=ziAJtf4v0htoteU8eHRlhaLLjuvGSrpY5YZgzoYaW4M=;\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=TRc1wFrA5udnUIVU6Z4FO4F52IrjfDejmC4ufnX5QNaiMw+BJjXelVbJ4i/5GV4yv\n\t3XuUewzKyYnYfKmiJxuhaseh8nW8hw0vvalITjMaV3NAwhKAznwi3T1J47s/Po8TuV\n\tnVEf+GNTSP58G1LG5aL9n3my2dVXgLmkcAoX658IR/oXBFaJnjYJeI3/RZxMyEnJc6\n\t/FpnIbPITp4XBI3LXYlNL3BspZ5pUvasUBVMS9/HW71HawuPy0xg7HC7JGhEoRz6ch\n\t82rUu9y+sI4QWhBDfoda4CUuEWii2c5x2DEx0LO0fJekmvMz3KVWIqf3G/xkHB9e94\n\tSNH8k8WyNn6fg==","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=LYtXzmSzumSZqd9mgDsMeCUx8atylxFyFgySB+ftpFE=;\n\tb=V5qOfi2QCjxYwDB4mgJVohIy8cfS54NFZhbUo2cam7FSJuTGNYfrJN/URoE3P/e0TN\n\tnqPlS0BkUG71LSQGabNAfvfWP0oR2n9po+YFwitK5iJIlHAUU9Ca7gDPFZqjIGHpVjkX\n\tu7O6R24uylgBVk5aoZZuEndLJf40cW6KiTK/m1ObogIdxfl8gOOhaNAFsGjNcyVB5O/d\n\tt9R+eiKCyWGkhfgipUL2YU8MoTKZwPc1Kz8xQGVaKK8dsLwYdJvoU8puEfx6nB4rexKU\n\tSN8zpUFY4huFmJqQ2BOJ+H0zyt/gdKL51/RfdWAt/4TkNaqTgpZPTay98MrioASo/xTC\n\tTS3A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"V5qOfi2Q\"; 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=LYtXzmSzumSZqd9mgDsMeCUx8atylxFyFgySB+ftpFE=;\n\tb=eqRKnise58urigX5tAjYB5zEHUMmwAz0j0z8DxmdILnWoNow5pT4os9hTHJC8sxigX\n\tm0E8BTPOikn6efW07L3YD8mAGvMF99XTZp8avp8FDzaQU6gOfTqqt/echt4hBkjRNAUN\n\ty9STTnMRIKX296ejEiBtxTCGl+wDVkbSshyQftzQFL8DOhReUtLRecY/0lVPo0vx4oEU\n\t9PG3LMYjcEcUssaa6xQNd4xQ3PC9Aa4MQWRQlHN2xTd5VBrkpWDOAzSg13vy8IExKvnI\n\tdkRqFJSu+Syhfog/VqWces1MsNe6InsHVoz2q5CTnI5HfIno+VoAQAFy9C4DIDAK/+g0\n\tmYlA==","X-Gm-Message-State":"AFqh2kr3HzSsWF7I66B0v2OUzw+Vr2Ao00jcEg1qTm0NJo0LXL1AX9eu\n\tXJx1sB8W4/D1EUzNBgQUuNsO/NUODZNmIM+YeBjOQXiVhHtPId6Df/c=","X-Google-Smtp-Source":"AMrXdXsrVYpae1Yk024Qokr5Li1HghFx8oQWOekd9dHJUlmRR7qMgimN439c6YQds1amRQgjXyl0dxtw286k3xVhNI0=","X-Received":"by 2002:a25:7583:0:b0:783:bc5e:3d67 with SMTP id\n\tq125-20020a257583000000b00783bc5e3d67mr1222645ybc.524.1674214649381;\n\tFri, 20 Jan 2023 03:37:29 -0800 (PST)","MIME-Version":"1.0","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-4-naush@raspberrypi.com>\n\t<167421186370.42371.15907607251381425817@Monstersaurus>","In-Reply-To":"<167421186370.42371.15907607251381425817@Monstersaurus>","Date":"Fri, 20 Jan 2023 11:37:13 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"00000000000074652505f2b07942\"","Subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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":26295,"web_url":"https://patchwork.libcamera.org/comment/26295/","msgid":"<167422367353.42371.17274854320325252024@Monstersaurus>","date":"2023-01-20T14:07:53","subject":"Re: [libcamera-devel] [PATCH v5 03/12] pipeline: raspberrypi: Add a\n\tpipeline config structure","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-20 11:37:13)\n> Hi Kieran,\n> \n> Thank you for your feedback!\n> \n> On Fri, 20 Jan 2023 at 10:51, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44)\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> > > 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 | 49 ++++++++++++++++---\n> > > 1 file changed, 42 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 8569df17976a..6bf9a669c679 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -199,6 +199,7 @@ public:\n> > >\n> > > int loadIPA(ipa::RPi::IPAInitResult *result);\n> > > int configureIPA(const CameraConfiguration *config,\n> > ipa::RPi::IPAConfigResult *result);\n> > > + int configurePipeline();\n> > >\n> > > void enumerateVideoDevices(MediaLink *link);\n> > >\n> > > @@ -295,6 +296,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\n> > allocated 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 stream.\n> >\n> > Are there any limits that should be conveyed by this documentation?\n> > Like should minTotalUnicamBuffers always be recommended to at least 2?\n> >\n> \n> minTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >=\n> minUnicamBuffers\n> \n> This is tested when we read the config file\n> in RPiCameraData::configurePipeline(), but\n> perhaps a little comment in the config files might be needed?\n\nIt needs to be whereever the option is documented such that the user\n(or system integrator?) will read that documentation. At the moment it\nseems we only have that in the config files, but we can't generate\ndocumentation from the config files... (or can we?)\n\nEssentially - I belive this information needs to find its way into:\n\thttps://libcamera.org/api-html/\n\nI plan to split this into 'public' api documentation for libcamera, and\ninternal documentation.\n\nBut configuration files are essentially a public API.\n\n\n\n\n> > Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should\n> > this convey instead the quantity of buffers to allocate in addition to\n> > minUnicamBuffers instead?\n> >\n> > > + */\n> > > + unsigned int minTotalUnicamBuffers;\n> > > + };\n> > > +\n> > > + Config config_;\n> > > +\n> > > private:\n> > > void checkRequestCompleted();\n> > > void fillRequestMetadata(const ControlList &bufferControls,\n> > > @@ -1378,6 +1394,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 = data->configurePipeline();\n> >\n> > Glancing above - would the number of supported streams ever be\n> > configurable, requiring this to load earlier ?\n> >\n> \n> No, I think that would be fixed at compile time.\n\nOk - that's fine.\n\n> \n> Regards,\n> Naush\n> \n> \n> >\n> > Of course such a change could load earlier as part of that so this is\n> > fine by me.\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > > + if (ret) {\n> > > + LOG(RPI, Error) << \"Unable to load pipeline handler\n> > configuration\";\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> > > @@ -1440,25 +1462,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\n> > stream, allocate\n> > > * additional buffers to make up the minimum,\n> > but ensure\n> > > - * we have at least 2 sets of internal buffers\n> > to use to\n> > > - * minimise frame drops.\n> > > + * we have at least minUnicamBuffers of internal\n> > buffers\n> > > + * 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,\n> > so 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> > > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config, ipa::RPi::IPA\n> > > return 0;\n> > > }\n> > >\n> > > +int RPiCameraData::configurePipeline()\n> > > +{\n> > > + config_ = {\n> > > + .minUnicamBuffers = 2,\n> > > + .minTotalUnicamBuffers = 4,\n> > > + };\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > /*\n> > > * enumerateVideoDevices() iterates over the Media Controller topology,\n> > starting\n> > > * at the sensor and finishing at Unicam. For each sensor,\n> > RPiCameraData stores\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 E36C9C3240\n\tfor ;\n\tFri, 20 Jan 2023 14:07:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70C4B61EFD;\n\tFri, 20 Jan 2023 15:07:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF89561EFD\n\tfor ;\n\tFri, 20 Jan 2023 15:07:56 +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 53376514;\n\tFri, 20 Jan 2023 15:07:56 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674223678;\n\tbh=a/PjWij6XK7rmARAWMFRuEI8yqJ/OApsK0RdwQPzjqQ=;\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=3ENIYqScwL3VaqLmdAE7hk4Cl5h6H4i86coRkr3pCQUeLnVbA+s3QWKsQEQdOxE7X\n\tEUuVCuTCInLdeGhgcqGFnDogRQe91IZ+0xwDLFY4dscIAErqT7pTITJjt0CB/9GFaa\n\tH1coL/wKM7mNtGOTnXQwwlYbgmOduCxRniAPzqj9dzf9GmpHLwtaO9QKC2eq8GU8r6\n\tvpnSOsC26Ye0O3NZg3h3ZIGWaDCYn1yC/lgrQkvQonYRzi3qVeGjlFiYTlpKYXaBu5\n\tQoW1ENb8r/mY7K46J6UfTMfRHGFXZ6xQCGLdWNGWTrrIe++tcvEBoGIBVO+toDyiWs\n\tkZXhisWgqOvTQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674223676;\n\tbh=a/PjWij6XK7rmARAWMFRuEI8yqJ/OApsK0RdwQPzjqQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ezoa33Ynu97FDKr8yaMIT8JNcbPqUh96J5xJhfM5zxS/0TvAiJ26spgRDNIbWpbLU\n\tvv6cI6xONSN+VK6GSF1z+JcJiXg+IYc4ntYI4iX6/6gWJb2TFMwHeawmGpC8HeIPYq\n\th7CE0P4vcHvxz8wB8H3qK6+U7YmB7ETJVBWmGDWU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ezoa33Yn\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-4-naush@raspberrypi.com>\n\t<167421186370.42371.15907607251381425817@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Fri, 20 Jan 2023 14:07:53 +0000","Message-ID":"<167422367353.42371.17274854320325252024@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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":"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":26308,"web_url":"https://patchwork.libcamera.org/comment/26308/","msgid":"","date":"2023-01-22T19:36:40","subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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":"On Fri, Jan 20, 2023 at 02:07:53PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Quoting Naushir Patuck (2023-01-20 11:37:13)\n> > On Fri, 20 Jan 2023 at 10:51, Kieran Bingham wrote:\n> > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44)\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> > > > 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 | 49 ++++++++++++++++---\n> > > > 1 file changed, 42 insertions(+), 7 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 8569df17976a..6bf9a669c679 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -199,6 +199,7 @@ public:\n> > > >\n> > > > int loadIPA(ipa::RPi::IPAInitResult *result);\n> > > > int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> > > > + int configurePipeline();\n> > > >\n> > > > void enumerateVideoDevices(MediaLink *link);\n> > > >\n> > > > @@ -295,6 +296,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 stream.\n> > >\n> > > Are there any limits that should be conveyed by this documentation?\n> > > Like should minTotalUnicamBuffers always be recommended to at least 2?\n> > >\n> > \n> > minTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >=\n> > minUnicamBuffers\n> > \n> > This is tested when we read the config file\n> > in RPiCameraData::configurePipeline(), but\n> > perhaps a little comment in the config files might be needed?\n> \n> It needs to be whereever the option is documented such that the user\n> (or system integrator?) will read that documentation. At the moment it\n> seems we only have that in the config files, but we can't generate\n> documentation from the config files... (or can we?)\n> \n> Essentially - I belive this information needs to find its way into:\n> \thttps://libcamera.org/api-html/\n> \n> I plan to split this into 'public' api documentation for libcamera, and\n> internal documentation.\n> \n> But configuration files are essentially a public API.\n\nI'm not sure about that, I think it will end up in a different\ndocumentation. End-users are not expected to read the API reference\ndocumentation, and neither are system integrator.\n\nWhat is quite sure is that it should be documented somewhere, and having\nthe information in the comments above is a good idea. We can later move\nit somewhere else when the time will be right.\n\n> > > Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should\n> > > this convey instead the quantity of buffers to allocate in addition to\n> > > minUnicamBuffers instead?\n> > >\n> > > > + */\n> > > > + unsigned int minTotalUnicamBuffers;\n> > > > + };\n> > > > +\n> > > > + Config config_;\n> > > > +\n> > > > private:\n> > > > void checkRequestCompleted();\n> > > > void fillRequestMetadata(const ControlList &bufferControls,\n> > > > @@ -1378,6 +1394,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 = data->configurePipeline();\n> > >\n> > > Glancing above - would the number of supported streams ever be\n> > > configurable, requiring this to load earlier ?\n> > >\n> > \n> > No, I think that would be fixed at compile time.\n> \n> Ok - that's fine.\n> \n> > > Of course such a change could load earlier as part of that so this is\n> > > fine by me.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > + if (ret) {\n> > > > + LOG(RPI, Error) << \"Unable to load pipeline handler configuration\";\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> > > > @@ -1440,25 +1462,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 of internal buffers\n> > > > + * 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> > > > /*\n> > > > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +int RPiCameraData::configurePipeline()\n> > > > +{\n> > > > + config_ = {\n> > > > + .minUnicamBuffers = 2,\n> > > > + .minTotalUnicamBuffers = 4,\n> > > > + };\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > /*\n> > > > * enumerateVideoDevices() iterates over the Media Controller topology, starting\n> > > > * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores","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 75E8FBDB1D\n\tfor ;\n\tSun, 22 Jan 2023 19:36:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA258625E4;\n\tSun, 22 Jan 2023 20:36:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE9FC625DD\n\tfor ;\n\tSun, 22 Jan 2023 20:36:42 +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 10152308;\n\tSun, 22 Jan 2023 20:36:41 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674416204;\n\tbh=jntYwnb5d/70sR/+G+aKxMJ8M6+4HiRirKshpWfO6qc=;\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=PSfU/BlBnJ4xaFPtLXZQG45NTELac/ILJ1/GjdEPQN3+mJBQI0Tm4fneIp6hLYB1R\n\tB+ROHlq2HVJg6/I1gFUe4dyOSmZYX+vz3L6culAfgmPyOKpxQ8IABWLqfA2eCydeGZ\n\tj5Ij1RvMZSidB+GNjh8ZaWOnidp8Yj4bIcQtI2reo97xbqB1LP0w8L6hnRvOR5uPT/\n\tJq7VLF3BdVIDBDfvktcwxzCIAFZtUX/rs+35K9MBkw5qxdeRv+YeZieQy92c4rZgZ2\n\ty/1aBba0sbdVGvaQPQlt9yjXtqgVOuRUW+0TLR16ErfKxVkdqFrloXPmtSfdzOHRiX\n\tdzAb7GSrYFDGA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674416202;\n\tbh=jntYwnb5d/70sR/+G+aKxMJ8M6+4HiRirKshpWfO6qc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jtYpQWmEXbmUHuHjWmaf9ti/syTew35EzljMNXrg+Y3Hg8psYvqn8uVOkWhOqKcev\n\tZOoyssf2YSWdd0Urk8eicKLscBngBuMu1cHGpjTVFqA+sXsi9rWao/7YJS7nNdwgHk\n\toOb8jw5lRi//Z3oH/yWMhnjzg4yR8TpOH0tHscqc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jtYpQWmE\"; dkim-atps=neutral","Date":"Sun, 22 Jan 2023 21:36:40 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-4-naush@raspberrypi.com>\n\t<167421186370.42371.15907607251381425817@Monstersaurus>\n\t\n\t<167422367353.42371.17274854320325252024@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<167422367353.42371.17274854320325252024@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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":26309,"web_url":"https://patchwork.libcamera.org/comment/26309/","msgid":"","date":"2023-01-22T19:49:38","subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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 Wed, Jan 18, 2023 at 08:59:44AM +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> 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 | 49 ++++++++++++++++---\n> 1 file changed, 42 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8569df17976a..6bf9a669c679 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -199,6 +199,7 @@ public:\n> \n> \tint loadIPA(ipa::RPi::IPAInitResult *result);\n> \tint configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> +\tint configurePipeline();\n\nI think loadPipelineConfiguration() would be more explicit. Would it be\ntoo long though ?\n\n> \n> \tvoid enumerateVideoDevices(MediaLink *link);\n> \n> @@ -295,6 +296,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 stream.\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> @@ -1378,6 +1394,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 = data->configurePipeline();\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error) << \"Unable to load pipeline handler configuration\";\n\nAnd to match the function name, s/handler //\n\nThis patch otherwise looks good to me.\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> @@ -1440,25 +1462,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 of internal buffers\n> +\t\t\t * 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/*\n> @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> \treturn 0;\n> }\n> \n> +int RPiCameraData::configurePipeline()\n> +{\n> +\tconfig_ = {\n> +\t\t.minUnicamBuffers = 2,\n> +\t\t.minTotalUnicamBuffers = 4,\n> +\t};\n> +\n> +\treturn 0;\n> +}\n> +\n> /*\n> * enumerateVideoDevices() iterates over the Media Controller topology, starting\n> * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores","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 2BAA3BEFBE\n\tfor ;\n\tSun, 22 Jan 2023 19:49:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A40A625E4;\n\tSun, 22 Jan 2023 20:49:42 +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 042FD625DD\n\tfor ;\n\tSun, 22 Jan 2023 20:49:41 +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 6D08A308;\n\tSun, 22 Jan 2023 20:49:40 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1674416982;\n\tbh=cEQW1bMIbwj1ZH6DUyHsBkbH8ZLQxGtTcPCQgt9+u4Y=;\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=IH+6GgaNGFvJnI0875dM/I+dEGra/D7pl2xgaVvR7weKxJe2MCCxV8Rw8ah072gif\n\tn3oMu/yuX8LbZJy3rlQ4IaInKURBQhvzdcbOAEb0vsq11IbsPTqv3AAHUoqP9dwvQ5\n\tas1BOG2PMIIO7tVbsrDKi5uw/54vZHavdMLThL9X8ee1X5ttEFtMXDFziRIc+Ffeuo\n\tfYLEUSe5l9oNCvtsSwDb/ZvzmuBGJTh+y5TyIDXP0mSz92BQfIigQTrgERL5qykfr4\n\t8DCa5q0GF5BpfPBgi6pA3A6lMeG5a+aPJQcjS0kZKqVhPELplWbIRd0GDMJTOqDxQb\n\tjmjZzo/zPoLmg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1674416980;\n\tbh=cEQW1bMIbwj1ZH6DUyHsBkbH8ZLQxGtTcPCQgt9+u4Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jmNmNbJZoVfuq3d8GWktqIntJRRaNlmdC4ZMr3Skt7qEO/w5O8JTJZ0Tt11A7h1Zi\n\tLs65WnyhLFTdX+rgT2zKH0KXCpBFy+8buwdZWbdMcNj7KnjsSqCanxEnA98oQ/nROb\n\tYArAGIFHl4F3Zhcsfx4A8TWWKB0dvhkWx2ioi+v8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jmNmNbJZ\"; dkim-atps=neutral","Date":"Sun, 22 Jan 2023 21:49:38 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230118085953.7027-1-naush@raspberrypi.com>\n\t<20230118085953.7027-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230118085953.7027-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 03/12] 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\" "}}]