[{"id":34743,"web_url":"https://patchwork.libcamera.org/comment/34743/","msgid":"","date":"2025-06-30T15:18:12","subject":"Re: [PATCH v1 4/6] pipeline: rkisp1: Properly handle the bufferCount\n\tset in the stream configuration","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"On Mon, Jun 30, 2025 at 10:11:19AM +0200, Stefan Klug wrote:\n> The bufferCount is reset to a hardcoded value of 4 in RkISP1Path::validate().\n> Keep the default value of 4 but do not reset it, if it was changed.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v1:\n> - Removed todo comment that was solved by this change\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++-----\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +---\n> 3 files changed, 6 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index bd14ab237064..9d7b05490af6 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -790,6 +790,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\treturn nullptr;\n> \n> \t\tcfg.colorSpace = colorSpace;\n> +\t\tcfg.bufferCount = kPipelineDepth;\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n> \n> @@ -1123,14 +1124,14 @@ 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(maxQueuedRequestsDevice_);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t\tactions += [&]() { mainPath_.stop(); };\n> \t}\n> \n> \tif (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> -\t\tret = selfPath_.start();\n> +\t\tret = selfPath_.start(maxQueuedRequestsDevice_);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t}\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 64018dc5b2f4..8ea5500d4080 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -249,7 +249,6 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> \tStreamConfiguration cfg(formats);\n> \tcfg.pixelFormat = format;\n> \tcfg.size = streamSize;\n> -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> \n> \treturn cfg;\n> }\n> @@ -383,7 +382,6 @@ RkISP1Path::validate(const CameraSensor *sensor,\n> \n> \tcfg->size.boundTo(maxResolution);\n> \tcfg->size.expandTo(minResolution);\n> -\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> \n> \tV4L2DeviceFormat format;\n> \tformat.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);\n> @@ -480,15 +478,14 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> \treturn 0;\n> }\n> \n> -int RkISP1Path::start()\n> +int RkISP1Path::start(unsigned int bufferCount)\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(bufferCount);\n\nQuestion: how passing bufferCount=maxQueuedRequestsDevice_ makes\nthis user configurable? \n\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 430181d371a7..0b60c499ac64 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -58,7 +58,7 @@ public:\n> \t\treturn video_->exportBuffers(bufferCount, buffers);\n> \t}\n> \n> -\tint start();\n> +\tint start(unsigned int bufferCount);\n> \tvoid stop();\n> \n> \tint queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); }\n> @@ -69,8 +69,6 @@ private:\n> \tvoid populateFormats();\n> \tSize filterSensorResolution(const CameraSensor *sensor);\n> \n> -\tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> -\n> \tconst char *name_;\n> \tbool running_;\n> \n> -- \n> 2.48.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 82D17BDCBF\n\tfor ;\n\tMon, 30 Jun 2025 15:18:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3027268E04;\n\tMon, 30 Jun 2025 17:18:11 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8441661527\n\tfor ;\n\tMon, 30 Jun 2025 17:18:09 +0200 (CEST)","from [49.36.69.141] (helo=uajain)\n\tby fanzine2.igalia.com with esmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uWGGm-00AWzb-Fr; Mon, 30 Jun 2025 17:18:08 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"SSw2fWk5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:\n\tSubject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=1bF7ipWIEqBQtv5O0MsmNmMoqVTJhz3PIKj7xRpVbWw=;\n\tb=SSw2fWk57iOX7ET1xeNQY/vHx2\n\t48txYl+XyWwDcEHsLVTahPcosAlrCeKM9USYyx02UTr9zC7ND7xUp0Mnfjpm4m2d/qzTI+PjlHxDV\n\tLANevRA/7CiZpZZw4v8Isr/nP0gTvAPd96JY2vz+JrMcVnPqXerVzCWJpf0vMgxKRQbpgfsb+2Xr/\n\th3GKY1MHzKZI1a2+9DM6T3jxMrgcYc3wXEg0TpbA+EH9rQbo8uWNa//Y1/e0/ajvgkP565Pd3uwSj\n\tJvK/xmDjr3fSIr+k9NlN3IOJme+j67aYGRCdfrhO8Ytl6tpVEYqQeclBTxrgpLSVm7j9DcMX5gpvV\n\tq0OGqgfw==;","Date":"Mon, 30 Jun 2025 20:48:12 +0530","From":"Umang Jain ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 4/6] pipeline: rkisp1: Properly handle the bufferCount\n\tset in the stream configuration","Message-ID":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-5-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250630081126.2384387-5-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34766,"web_url":"https://patchwork.libcamera.org/comment/34766/","msgid":"<175137419195.2426344.17129278248452315898@localhost>","date":"2025-07-01T12:49:51","subject":"Re: [PATCH v1 4/6] pipeline: rkisp1: Properly handle the bufferCount\n\tset in the stream configuration","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nQuoting Umang Jain (2025-06-30 17:18:12)\n> On Mon, Jun 30, 2025 at 10:11:19AM +0200, Stefan Klug wrote:\n> > The bufferCount is reset to a hardcoded value of 4 in RkISP1Path::validate().\n> > Keep the default value of 4 but do not reset it, if it was changed.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > Changes in v1:\n> > - Removed todo comment that was solved by this change\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++--\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 ++-----\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +---\n> > 3 files changed, 6 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index bd14ab237064..9d7b05490af6 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -790,6 +790,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > return nullptr;\n> > \n> > cfg.colorSpace = colorSpace;\n> > + cfg.bufferCount = kPipelineDepth;\n> > config->addConfiguration(cfg);\n> > }\n> > \n> > @@ -1123,14 +1124,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > }\n> > \n> > if (data->mainPath_->isEnabled()) {\n> > - ret = mainPath_.start();\n> > + ret = mainPath_.start(maxQueuedRequestsDevice_);\n> > if (ret)\n> > return ret;\n> > actions += [&]() { mainPath_.stop(); };\n> > }\n> > \n> > if (hasSelfPath_ && data->selfPath_->isEnabled()) {\n> > - ret = selfPath_.start();\n> > + ret = selfPath_.start(maxQueuedRequestsDevice_);\n> > if (ret)\n> > return ret;\n> > }\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index 64018dc5b2f4..8ea5500d4080 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -249,7 +249,6 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> > StreamConfiguration cfg(formats);\n> > cfg.pixelFormat = format;\n> > cfg.size = streamSize;\n> > - cfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > \n> > return cfg;\n> > }\n> > @@ -383,7 +382,6 @@ RkISP1Path::validate(const CameraSensor *sensor,\n> > \n> > cfg->size.boundTo(maxResolution);\n> > cfg->size.expandTo(minResolution);\n> > - cfg->bufferCount = RKISP1_BUFFER_COUNT;\n> > \n> > V4L2DeviceFormat format;\n> > format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);\n> > @@ -480,15 +478,14 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> > return 0;\n> > }\n> > \n> > -int RkISP1Path::start()\n> > +int RkISP1Path::start(unsigned int bufferCount)\n> > {\n> > int ret;\n> > \n> > if (running_)\n> > return -EBUSY;\n> > \n> > - /* \\todo Make buffer count user configurable. */\n> > - ret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n> > + ret = video_->importBuffers(bufferCount);\n> \n> Question: how passing bufferCount=maxQueuedRequestsDevice_ makes\n> this user configurable? \n\nI'll improve patch message in the next iteration. The internal buffer\ncount is still fixed to 4 as before. By not modifying cfg->bufferCount,\nthe user can set that on the StreamConfiguration and the\nFrameBufferAllocator will allocate that many buffers. (The target of the\nseries was not mainly to increase the number of buffers inside libcamera\nbut the number of buffers circling).\n\nBest regards,\nStefan\n\n> \n> > if (ret)\n> > return ret;\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > index 430181d371a7..0b60c499ac64 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > @@ -58,7 +58,7 @@ public:\n> > return video_->exportBuffers(bufferCount, buffers);\n> > }\n> > \n> > - int start();\n> > + int start(unsigned int bufferCount);\n> > void stop();\n> > \n> > int queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); }\n> > @@ -69,8 +69,6 @@ private:\n> > void populateFormats();\n> > Size filterSensorResolution(const CameraSensor *sensor);\n> > \n> > - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> > -\n> > const char *name_;\n> > bool running_;\n> > \n> > -- \n> > 2.48.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 357BAC3237\n\tfor ;\n\tTue, 1 Jul 2025 12:49:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61F2768E26;\n\tTue, 1 Jul 2025 14:49:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3253368E1E\n\tfor ;\n\tTue, 1 Jul 2025 14:49:55 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:7cdc:2548:b8ee:3b98])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 9D9297E1;\n\tTue, 1 Jul 2025 14:49:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IdnAHKKN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751374172;\n\tbh=W+gIe2DzeO9dC+Dw7uuWtKJMLAhiYeKSVmkIpXS1Cjo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=IdnAHKKNnfo68v2gTCbY1fT8H6PRmJV+vNMvAAeqUTuOZj6BnshLLrj6JRu36Noch\n\t0KGnRJYKLTplQ1rWyQ6o5Ha+LA6B4V/3o1u6Ob1aLCrFOQyLzj2ZtLwEwvl8ysUjmo\n\tdSWiUBUPbEyJL/CzlM6oPGb0l4i9ZGhbVx2CmVOc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-5-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [PATCH v1 4/6] pipeline: rkisp1: Properly handle the bufferCount\n\tset in the stream configuration","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Umang Jain ","Date":"Tue, 01 Jul 2025 14:49:51 +0200","Message-ID":"<175137419195.2426344.17129278248452315898@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]