[{"id":16165,"web_url":"https://patchwork.libcamera.org/comment/16165/","msgid":"<20210410082911.dwggv56zr6v7sz3h@uno.localdomain>","date":"2021-04-10T08:29:11","subject":"Re: [libcamera-devel] [RFC PATCH 2/3] libcamera: pipeline: rkisp1:\n\tAllow bufferCount to be user-configurable","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas\n\nOn Fri, Apr 09, 2021 at 06:38:14PM -0300, Nícolas F. R. A. Prado wrote:\n> Allow the StreamConfiguration's bufferCount to be changed by the user.\n> This allows the user to, after increasing bufferCount, allocate more\n> buffers through FrameBufferAllocator.\n\nnit: I would avoid the \", allocate more\" as this is a consequence of\nbufferCount_ being increased, not something specifically about this\npatch.\n\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +++++++-----\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++--\n> 3 files changed, 11 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 549f4a4e61a8..c7b93b2804c7 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -792,7 +792,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t}\n>\n> \tif (data->mainPath_->isEnabled()) {\n> -\t\tret = mainPath_.start();\n> +\t\tret = mainPath_.start(data->mainPathStream_.configuration());\n> \t\tif (ret) {\n> \t\t\tparam_->streamOff();\n> \t\t\tstat_->streamOff();\n> @@ -803,7 +803,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t}\n>\n> \tif (data->selfPath_->isEnabled()) {\n> -\t\tret = selfPath_.start();\n> +\t\tret = selfPath_.start(data->selfPathStream_.configuration());\n\nThe patch is ok, however I would consider how all of this would look\nlike if we store in each 'path' (what an horrible name we chose!) the\nStreamConfiguration passed in at RkISP1Path::configure() time and\nre-use it here, instead of having to pass it in again at start() time.\n\n> \t\tif (ret) {\n> \t\t\tmainPath_.stop();\n> \t\t\tparam_->streamOff();\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 25f482eb8d8e..a03b5c23b87e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -61,7 +61,7 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)\n> \tStreamConfiguration cfg(formats);\n> \tcfg.pixelFormat = formats::NV12;\n> \tcfg.size = maxResolution;\n> -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> +\tcfg.bufferCount = RKISP1_MIN_BUFFER_COUNT;\n>\n> \treturn cfg;\n> }\n> @@ -77,7 +77,10 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)\n>\n> \tcfg->size.boundTo(maxResolution_);\n> \tcfg->size.expandTo(minResolution_);\n> -\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> +\tif (cfg->bufferCount < RKISP1_MIN_BUFFER_COUNT) {\n> +\t\tcfg->bufferCount = RKISP1_MIN_BUFFER_COUNT;\n> +\t\tstatus = CameraConfiguration::Adjusted;\n> +\t}\n>\n> \tV4L2DeviceFormat format;\n> \tformat.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);\n> @@ -164,15 +167,14 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> \treturn 0;\n> }\n>\n> -int RkISP1Path::start()\n> +int RkISP1Path::start(const StreamConfiguration &config)\n> {\n> \tint ret;\n>\n> \tif (running_)\n> \t\treturn -EBUSY;\n>\n> -\t/* \\todo Make buffer count user configurable. */\n> -\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> +\tret = video_->importBuffers(config.bufferCount);\n> \tif (ret)\n> \t\treturn ret;\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 3b3e37d258d0..13291da89dc5 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -49,14 +49,14 @@ public:\n> \t\treturn video_->exportBuffers(bufferCount, buffers);\n> \t}\n>\n> -\tint start();\n> +\tint start(const StreamConfiguration &config);\n> \tvoid stop();\n>\n> \tint queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); }\n> \tSignal &bufferReady() { return video_->bufferReady; }\n>\n> private:\n> -\tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> +\tstatic constexpr unsigned int RKISP1_MIN_BUFFER_COUNT = 4;\n>\n> \tconst char *name_;\n> \tbool running_;\n> --\n> 2.31.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 5A208BD1F6\n\tfor ;\n\tSat, 10 Apr 2021 08:28:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95E68687FB;\n\tSat, 10 Apr 2021 10:28:38 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B421687EC\n\tfor ;\n\tSat, 10 Apr 2021 10:28:36 +0200 (CEST)","from uno.localdomain (host-82-57-193-192.retail.telecomitalia.it\n\t[82.57.193.192]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 353B460006;\n\tSat, 10 Apr 2021 08:28:34 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Sat, 10 Apr 2021 10:29:11 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210410082911.dwggv56zr6v7sz3h@uno.localdomain>","References":"<20210409213815.356837-1-nfraprado@collabora.com>\n\t<20210409213815.356837-3-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210409213815.356837-3-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/3] libcamera: pipeline: rkisp1:\n\tAllow bufferCount to be user-configurable","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","Cc":"libcamera-devel@lists.libcamera.org,\n\tCollabora Kernel ML ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]