[{"id":36594,"web_url":"https://patchwork.libcamera.org/comment/36594/","msgid":"","date":"2025-11-01T17:22:21","subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote:\n> Most pipeline handlers release buffers when a camera is stopped. However,\n> the raspberry pi pipelines do not do that. Hence they are incompatible\n\n\nThe RPi pipeline is the only one that, apart from UVC, that implements\nPipelineHandler::releaseDevice(), called in the Camera::release()\npath.\n\nThe RPi implementation of ::releaseDevice() calls \"data->freeBuffer()\"\nwhich ends up calling the RPiStream Stream::releaseBuffers()\nwhich first releases buffers from the video device and then clears the\nFrameBuffers created around them.\n\nNow if I get this right, this also clears bufferMap_ which is\npopulated through Stream::setExportedBuffers() in the\nPipelineHandlerBase::exportFrameBuffers() call.\n\nTo make this short, my understanding is that rpi releases all\nallocated buffers on the video device at configure() and release() time.\n\nI don't think we specify anywhere how pipelines should behave, but\nthis could certainly save allocations/deallocation at every start/stop\ncycle.\n\n\n> with the current model and all \"CaptureStartStop\" tests fail because\n> lc-compliance allocates buffers before starting and releases them after\n> stopping. Fix this by allocating buffers after the camera is configured,\n> and do not release them after stopping.\n\nSo we now have two models at play: one where buffers allocated on the\nvideo device through FrameBufferAllocator are released at stop() time\nand one where they are released at configure()/release() time.\n\nI wonder, but that's a general question, not specifically on this\npatch which does anyway implement the behaviour I'm about to describe:\nwhat happens if a FrameBuffers created with FrameBufferAllocator\noutlives the buffers allocated on the video device it wraps ?\n\nWith this patch applied, FrameBuffers in the frame buffer allocator\nare released only at ~Capture(), while when running this on, say,\nrkisp1, the memory buffers on the video device are released at every stop().\n\nI feel like I'm missing something obvious, otherwise things should go\nbooom quite fast on pipelines !rpi with this change in..\n\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++-----\n> src/apps/lc-compliance/helpers/capture.h | 1 +\n> 2 files changed, 23 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index d076e964a..df0ad00ac 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr camera)\n> Capture::~Capture()\n> {\n> \tstop();\n> +\tunconfigure();\n\na little bikeshedding to make this review more fun: I would call this\nfreeBuffers() or something similar.\n\nThanks\n j\n\n> }\n>\n> void Capture::configure(libcamera::Span roles)\n> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span roles)\n> \tASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n> \tASSERT_EQ(camera_->configure(config.get()), 0);\n>\n> +\tconst auto bufferCount = config->at(0).bufferCount;\n> +\n> +\tfor (const auto &cfg : *config) {\n> +\t\tStream *stream = cfg.stream();\n> +\n> +\t\tASSERT_GE(allocator_.allocate(stream), 0)\n> +\t\t\t<< \"Failed to allocate buffers\";\n> +\n> +\t\tASSERT_EQ(allocator_.buffers(stream).size(), bufferCount)\n> +\t\t\t<< \"Mismatching buffer count\";\n> +\t}\n> +\n> +\tASSERT_TRUE(allocator_.allocated());\n> +\n> \tconfig_ = std::move(config);\n> }\n>\n> @@ -112,7 +127,7 @@ void Capture::start()\n> {\n> \tassert(config_);\n> \tassert(!config_->empty());\n> -\tassert(!allocator_.allocated());\n> +\tassert(allocator_.allocated());\n> \tassert(requests_.empty());\n>\n> \tconst auto bufferCount = config_->at(0).bufferCount;\n> @@ -132,9 +147,6 @@ void Capture::start()\n> \tfor (const auto &cfg : *config_) {\n> \t\tStream *stream = cfg.stream();\n>\n> -\t\tint count = allocator_.allocate(stream);\n> -\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> -\n> \t\tconst auto &buffers = allocator_.buffers(stream);\n> \t\tASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n>\n> @@ -144,8 +156,6 @@ void Capture::start()\n> \t\t}\n> \t}\n>\n> -\tASSERT_TRUE(allocator_.allocated());\n> -\n> \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n>\n> \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> @@ -161,6 +171,12 @@ void Capture::stop()\n> \tcamera_->requestCompleted.disconnect(this);\n>\n> \trequests_.clear();\n> +}\n> +\n> +void Capture::unconfigure()\n> +{\n> +\tif (!config_ || !allocator_.allocated())\n> +\t\treturn;\n>\n> \tfor (const auto &cfg : *config_) {\n> \t\tEXPECT_EQ(allocator_.free(cfg.stream()), 0)\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index ea01c11c1..41ce4e5a6 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -28,6 +28,7 @@ private:\n>\n> \tvoid start();\n> \tvoid stop();\n> +\tvoid unconfigure();\n>\n> \tint queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> --\n> 2.50.0\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 B553EBDE4C\n\tfor ;\n\tSat, 1 Nov 2025 17:22:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0948D6086F;\n\tSat, 1 Nov 2025 18:22:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B3CC606D5\n\tfor ;\n\tSat, 1 Nov 2025 18:22:24 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:6462:5de2:153:f9b8:5024:faa2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C351E14A6;\n\tSat, 1 Nov 2025 18:20:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Nb2jEHXu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762017632;\n\tbh=edl8KuIaq1DnmJRHd94sWAw0HHfT2foCCdGHyMz3gJM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Nb2jEHXu+chyCmSwbIO80guoHBdNnbsmz5yDgISa6pJ4C3LfnWK8QDhhuCP7kvq/8\n\tu1/WfnKm0+PybffTRlplCSDgs4HLGhSKYrRP9DWLVLF7czeU7v83uRbHh7ybc24d56\n\tQ7FKOFAnhXexh387BfvlTkG/H1zI5Yd7HpYherSQ=","Date":"Sat, 1 Nov 2025 18:22:21 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","Message-ID":"","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<20250624122830.726987-2-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250624122830.726987-2-barnabas.pocze@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":36652,"web_url":"https://patchwork.libcamera.org/comment/36652/","msgid":"<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>","date":"2025-11-03T12:37:39","subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote:\n>> Most pipeline handlers release buffers when a camera is stopped. However,\n>> the raspberry pi pipelines do not do that. Hence they are incompatible\n> \n> \n> The RPi pipeline is the only one that, apart from UVC, that implements\n> PipelineHandler::releaseDevice(), called in the Camera::release()\n> path.\n> \n> The RPi implementation of ::releaseDevice() calls \"data->freeBuffer()\"\n> which ends up calling the RPiStream Stream::releaseBuffers()\n> which first releases buffers from the video device and then clears the\n> FrameBuffers created around them.\n> \n> Now if I get this right, this also clears bufferMap_ which is\n> populated through Stream::setExportedBuffers() in the\n> PipelineHandlerBase::exportFrameBuffers() call.\n> \n> To make this short, my understanding is that rpi releases all\n> allocated buffers on the video device at configure() and release() time.\n\nThat is my understanding as well.\n\n\n> \n> I don't think we specify anywhere how pipelines should behave, but\n> this could certainly save allocations/deallocation at every start/stop\n> cycle.\n> \n> \n>> with the current model and all \"CaptureStartStop\" tests fail because\n>> lc-compliance allocates buffers before starting and releases them after\n>> stopping. Fix this by allocating buffers after the camera is configured,\n>> and do not release them after stopping.\n> \n> So we now have two models at play: one where buffers allocated on the\n> video device through FrameBufferAllocator are released at stop() time\n> and one where they are released at configure()/release() time.\n> \n> I wonder, but that's a general question, not specifically on this\n> patch which does anyway implement the behaviour I'm about to describe:\n> what happens if a FrameBuffers created with FrameBufferAllocator\n> outlives the buffers allocated on the video device it wraps ?\n\nI don't think that can happen. A `FrameBuffer` object has a `SharedFD`\nin each of its `Plane`s, keeping the kernel resource alive. This matches\nthe documentation of `V4L2VideoDevice::releaseBuffers()`:\n\n [...] Buffer exported by exportBuffers(), if any, are not affected.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> With this patch applied, FrameBuffers in the frame buffer allocator\n> are released only at ~Capture(), while when running this on, say,\n> rkisp1, the memory buffers on the video device are released at every stop().\n> \n> I feel like I'm missing something obvious, otherwise things should go\n> booom quite fast on pipelines !rpi with this change in..\n> \n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++-----\n>> src/apps/lc-compliance/helpers/capture.h | 1 +\n>> 2 files changed, 23 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n>> index d076e964a..df0ad00ac 100644\n>> --- a/src/apps/lc-compliance/helpers/capture.cpp\n>> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n>> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr camera)\n>> Capture::~Capture()\n>> {\n>> \tstop();\n>> +\tunconfigure();\n> \n> a little bikeshedding to make this review more fun: I would call this\n> freeBuffers() or something similar.\n> \n> Thanks\n> j\n> \n>> }\n>>\n>> void Capture::configure(libcamera::Span roles)\n>> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span roles)\n>> \tASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n>> \tASSERT_EQ(camera_->configure(config.get()), 0);\n>>\n>> +\tconst auto bufferCount = config->at(0).bufferCount;\n>> +\n>> +\tfor (const auto &cfg : *config) {\n>> +\t\tStream *stream = cfg.stream();\n>> +\n>> +\t\tASSERT_GE(allocator_.allocate(stream), 0)\n>> +\t\t\t<< \"Failed to allocate buffers\";\n>> +\n>> +\t\tASSERT_EQ(allocator_.buffers(stream).size(), bufferCount)\n>> +\t\t\t<< \"Mismatching buffer count\";\n>> +\t}\n>> +\n>> +\tASSERT_TRUE(allocator_.allocated());\n>> +\n>> \tconfig_ = std::move(config);\n>> }\n>>\n>> @@ -112,7 +127,7 @@ void Capture::start()\n>> {\n>> \tassert(config_);\n>> \tassert(!config_->empty());\n>> -\tassert(!allocator_.allocated());\n>> +\tassert(allocator_.allocated());\n>> \tassert(requests_.empty());\n>>\n>> \tconst auto bufferCount = config_->at(0).bufferCount;\n>> @@ -132,9 +147,6 @@ void Capture::start()\n>> \tfor (const auto &cfg : *config_) {\n>> \t\tStream *stream = cfg.stream();\n>>\n>> -\t\tint count = allocator_.allocate(stream);\n>> -\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n>> -\n>> \t\tconst auto &buffers = allocator_.buffers(stream);\n>> \t\tASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n>>\n>> @@ -144,8 +156,6 @@ void Capture::start()\n>> \t\t}\n>> \t}\n>>\n>> -\tASSERT_TRUE(allocator_.allocated());\n>> -\n>> \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n>>\n>> \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n>> @@ -161,6 +171,12 @@ void Capture::stop()\n>> \tcamera_->requestCompleted.disconnect(this);\n>>\n>> \trequests_.clear();\n>> +}\n>> +\n>> +void Capture::unconfigure()\n>> +{\n>> +\tif (!config_ || !allocator_.allocated())\n>> +\t\treturn;\n>>\n>> \tfor (const auto &cfg : *config_) {\n>> \t\tEXPECT_EQ(allocator_.free(cfg.stream()), 0)\n>> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n>> index ea01c11c1..41ce4e5a6 100644\n>> --- a/src/apps/lc-compliance/helpers/capture.h\n>> +++ b/src/apps/lc-compliance/helpers/capture.h\n>> @@ -28,6 +28,7 @@ private:\n>>\n>> \tvoid start();\n>> \tvoid stop();\n>> +\tvoid unconfigure();\n>>\n>> \tint queueRequest(libcamera::Request *request);\n>> \tvoid requestComplete(libcamera::Request *request);\n>> --\n>> 2.50.0\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 17FE4BDE4C\n\tfor ;\n\tMon, 3 Nov 2025 12:37:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01EFA609D8;\n\tMon, 3 Nov 2025 13:37: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 113E0606A0\n\tfor ;\n\tMon, 3 Nov 2025 13:37:43 +0100 (CET)","from [192.168.33.39] (185.221.140.239.nat.pool.zt.hu\n\t[185.221.140.239])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1247299F;\n\tMon, 3 Nov 2025 13:35:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XjZHkF/s\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762173350;\n\tbh=jf6uDEBgB8A0oDQYukuu39dVzlBpksJHRrlmmQbJ+1Y=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=XjZHkF/sOvMAcP96VCqgnEnrNqYi4/L4CoukiE36cBpT8wTxkMhzctuO1HKOTaV02\n\tHV7z1HPRRLAKlg2d0hK5qUj4VHqRxP0rdbnFnOh+XBwtK/6iVLXnVuYKwYTHoqbPEA\n\t6+BrXMQMuEhEwRgal62e4ybEKLkHVfEXHoDOqZs8=","Message-ID":"<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>","Date":"Mon, 3 Nov 2025 13:37:39 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<20250624122830.726987-2-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36653,"web_url":"https://patchwork.libcamera.org/comment/36653/","msgid":"","date":"2025-11-03T12:46:29","subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Barnabás and Jacopo,\n\nOn Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze\n wrote:\n>\n> Hi\n>\n> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote:\n> >> Most pipeline handlers release buffers when a camera is stopped. However,\n> >> the raspberry pi pipelines do not do that. Hence they are incompatible\n> >\n> >\n> > The RPi pipeline is the only one that, apart from UVC, that implements\n> > PipelineHandler::releaseDevice(), called in the Camera::release()\n> > path.\n> >\n> > The RPi implementation of ::releaseDevice() calls \"data->freeBuffer()\"\n> > which ends up calling the RPiStream Stream::releaseBuffers()\n> > which first releases buffers from the video device and then clears the\n> > FrameBuffers created around them.\n> >\n> > Now if I get this right, this also clears bufferMap_ which is\n> > populated through Stream::setExportedBuffers() in the\n> > PipelineHandlerBase::exportFrameBuffers() call.\n> >\n> > To make this short, my understanding is that rpi releases all\n> > allocated buffers on the video device at configure() and release() time.\n>\n> That is my understanding as well.\n\nThis is correct and a bit of a pain point in our popeline handler. To\nsummarise, our PH does not free internal buffer allocations on the\nstop() call, but instead waits until configure() and\nfrees/re-allocates buffers if the configuration\n(resolution/pixelformat) changes. This is to avoid fragmentation on\nearlier Pi platforms that can only use a limited CMA carveout for\nframebuffers where we can very quickly run out of free contiguous\nspace if we run many start/stop cycles.\n\nMany moons ago I suggested a parameter in stop() to allow the\napplication to either request holding on to resources (like we do in\nour PH) or free them completely. That did not go anywhere, but\nperhaps it's time to revisit a change like that?\n\nNaush\n\n>\n>\n>\n> >\n> > I don't think we specify anywhere how pipelines should behave, but\n> > this could certainly save allocations/deallocation at every start/stop\n> > cycle.\n> >\n> >\n> >> with the current model and all \"CaptureStartStop\" tests fail because\n> >> lc-compliance allocates buffers before starting and releases them after\n> >> stopping. Fix this by allocating buffers after the camera is configured,\n> >> and do not release them after stopping.\n> >\n> > So we now have two models at play: one where buffers allocated on the\n> > video device through FrameBufferAllocator are released at stop() time\n> > and one where they are released at configure()/release() time.\n> >\n> > I wonder, but that's a general question, not specifically on this\n> > patch which does anyway implement the behaviour I'm about to describe:\n> > what happens if a FrameBuffers created with FrameBufferAllocator\n> > outlives the buffers allocated on the video device it wraps ?\n>\n> I don't think that can happen. A `FrameBuffer` object has a `SharedFD`\n> in each of its `Plane`s, keeping the kernel resource alive. This matches\n> the documentation of `V4L2VideoDevice::releaseBuffers()`:\n>\n> [...] Buffer exported by exportBuffers(), if any, are not affected.\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> >\n> > With this patch applied, FrameBuffers in the frame buffer allocator\n> > are released only at ~Capture(), while when running this on, say,\n> > rkisp1, the memory buffers on the video device are released at every stop().\n> >\n> > I feel like I'm missing something obvious, otherwise things should go\n> > booom quite fast on pipelines !rpi with this change in..\n> >\n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++-----\n> >> src/apps/lc-compliance/helpers/capture.h | 1 +\n> >> 2 files changed, 23 insertions(+), 6 deletions(-)\n> >>\n> >> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> >> index d076e964a..df0ad00ac 100644\n> >> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> >> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> >> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr camera)\n> >> Capture::~Capture()\n> >> {\n> >> stop();\n> >> + unconfigure();\n> >\n> > a little bikeshedding to make this review more fun: I would call this\n> > freeBuffers() or something similar.\n> >\n> > Thanks\n> > j\n> >\n> >> }\n> >>\n> >> void Capture::configure(libcamera::Span roles)\n> >> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span roles)\n> >> ASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n> >> ASSERT_EQ(camera_->configure(config.get()), 0);\n> >>\n> >> + const auto bufferCount = config->at(0).bufferCount;\n> >> +\n> >> + for (const auto &cfg : *config) {\n> >> + Stream *stream = cfg.stream();\n> >> +\n> >> + ASSERT_GE(allocator_.allocate(stream), 0)\n> >> + << \"Failed to allocate buffers\";\n> >> +\n> >> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount)\n> >> + << \"Mismatching buffer count\";\n> >> + }\n> >> +\n> >> + ASSERT_TRUE(allocator_.allocated());\n> >> +\n> >> config_ = std::move(config);\n> >> }\n> >>\n> >> @@ -112,7 +127,7 @@ void Capture::start()\n> >> {\n> >> assert(config_);\n> >> assert(!config_->empty());\n> >> - assert(!allocator_.allocated());\n> >> + assert(allocator_.allocated());\n> >> assert(requests_.empty());\n> >>\n> >> const auto bufferCount = config_->at(0).bufferCount;\n> >> @@ -132,9 +147,6 @@ void Capture::start()\n> >> for (const auto &cfg : *config_) {\n> >> Stream *stream = cfg.stream();\n> >>\n> >> - int count = allocator_.allocate(stream);\n> >> - ASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> >> -\n> >> const auto &buffers = allocator_.buffers(stream);\n> >> ASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n> >>\n> >> @@ -144,8 +156,6 @@ void Capture::start()\n> >> }\n> >> }\n> >>\n> >> - ASSERT_TRUE(allocator_.allocated());\n> >> -\n> >> camera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >>\n> >> ASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> >> @@ -161,6 +171,12 @@ void Capture::stop()\n> >> camera_->requestCompleted.disconnect(this);\n> >>\n> >> requests_.clear();\n> >> +}\n> >> +\n> >> +void Capture::unconfigure()\n> >> +{\n> >> + if (!config_ || !allocator_.allocated())\n> >> + return;\n> >>\n> >> for (const auto &cfg : *config_) {\n> >> EXPECT_EQ(allocator_.free(cfg.stream()), 0)\n> >> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> >> index ea01c11c1..41ce4e5a6 100644\n> >> --- a/src/apps/lc-compliance/helpers/capture.h\n> >> +++ b/src/apps/lc-compliance/helpers/capture.h\n> >> @@ -28,6 +28,7 @@ private:\n> >>\n> >> void start();\n> >> void stop();\n> >> + void unconfigure();\n> >>\n> >> int queueRequest(libcamera::Request *request);\n> >> void requestComplete(libcamera::Request *request);\n> >> --\n> >> 2.50.0\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 BB04EC3241\n\tfor ;\n\tMon, 3 Nov 2025 12:47:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AEDE60A8B;\n\tMon, 3 Nov 2025 13:47:08 +0100 (CET)","from mail-vs1-xe36.google.com (mail-vs1-xe36.google.com\n\t[IPv6:2607:f8b0:4864:20::e36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11290606A0\n\tfor ;\n\tMon, 3 Nov 2025 13:47:07 +0100 (CET)","by mail-vs1-xe36.google.com with SMTP id\n\tada2fe7eead31-5dbceacaf48so37112137.0\n\tfor ;\n\tMon, 03 Nov 2025 04:47:06 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"VvU5OlXO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1762174026; x=1762778826;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=x/RCFcRWlUEWlDWBtYLNVFQ12ITAab/hsQGC1MhyCIg=;\n\tb=VvU5OlXOAYLrVHjv9SYB1xpq8bxl8mCmpl7O9kzEGuMTdQW/92RxCeIk1Q5c+3g0PK\n\tNU+0riL7DYjGB4zTplPBqKVbHbdM6xIug6J2Db72QnCzexCmycq2GjT2Y6LPTM2sFJ4q\n\tv90RJFjQHX2BcQFhcmztSm+R0s8VzUaGZbxVCcAqOFKTVOx+PtLd5hw9smwXq81fulFk\n\twq9qnb+d7LJcutBvIaX+2Ku5Hsw4jcpQZU27MGp04pUB1/bhMA53KwAtmPmXYjIwjKgY\n\tns9x61BrHtTz4ZgyfyAeLgaUja3fEuL8sD7AJO4Q77VN4azSCdinFLebu7V1Kx5t7vq+\n\tFuOg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1762174026; x=1762778826;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=x/RCFcRWlUEWlDWBtYLNVFQ12ITAab/hsQGC1MhyCIg=;\n\tb=gpp0q6WAWiZb2WTMnnkbuQJAyWon9zV568c9m46r5OJvuQXqFDC3SJpHQBafCOtp/e\n\tPXi4G4HDNrwCiPwVhzQisxRQSGosOkbKesZOdiqX9mzKOeCzpAMLVOtnQnz/qu2+txiU\n\tJevmnI8V6oEgieO/29ZUXrP7Xk5Qcmej0NgQyM1OiJBd9sSQxWunOBiup02iIjp1MIWo\n\tVURuAorLFpmcjyuNTQpqrXqOwkSs2L/bQUv4SlNx4TbBTrqyA4GWlpbhSI3v7jIzXMKB\n\t3uSUgIp24mJx40PqDNBZjWjXd4qC2fAKxrTpRE55CL+ZZR+cSV90jGKN8dgNc0DR1TlG\n\t8Jkw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVr7Igw1SAD+12ESlC/P9/uFQLYE94HP409v+I4b4ve/0HXE+nrIAeBclbjwyNpy9/dRtb5kTeGzRuBFAdy/Yg=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yw55YK6o6TTy4/f2qDKIn7W15voBPlpjqJhacBXXBwQPXgk5+yt\n\tkKedcIEKFAhYCjJT0NrpUZJhZDr6iOIPrnPX8ZOqhFpvxMqZ7Vy+A7kJvvOmUlxOLsJ40d+qQG3\n\tCA2cjOMHkDQdlDjpEnZCXCSnvJq8oysh2Fx5iEjaxM8m/eGB28sTf","X-Gm-Gg":"ASbGncvSZlmEpI8FPCInD1HElYGezsszfH8hB9HKZCM7HOYWGBNvsOxHu04jNNPZ9NA\n\txxTqObWEOub9JwKodPFrhndeCfRoxFnfzMqZvsxRwFWQa2ZKu3fk4eHaZmiAnIDgXt2m7afNa6M\n\tRoCERikTV4ydi8KpH2bcLjf8NY/A/3zUMGYk10A1pUFSL0jgpv2SH2GP4ImXvlcWVOWd6xpmQsv\n\tmQesBH6Uw9VXuvSE4qns6dNUxWZrRGeYxztOP5jKDHuZq6I3wf7TscYCuHIxa5SCCBcvuxyF9h/\n\tSvIhtSv0jHFUGCzV","X-Google-Smtp-Source":"AGHT+IFDVaQY9VBBgwv/RjgB6BVLbTyGizmyVr6JMiVgxY0vADbDqBO1v8pvgRseVaOs5j523IDrhM/T6zUPW5f2TUQ=","X-Received":"by 2002:a05:6102:dd2:b0:5db:dad4:861 with SMTP id\n\tada2fe7eead31-5dbdad4372dmr399649137.6.1762174025550; Mon, 03 Nov 2025\n\t04:47:05 -0800 (PST)","MIME-Version":"1.0","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<20250624122830.726987-2-barnabas.pocze@ideasonboard.com>\n\t\n\t<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>","In-Reply-To":"<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 3 Nov 2025 12:46:29 +0000","X-Gm-Features":"AWmQ_blVHtEFBwgmNrjD1IKOQlJGFdRZq7B1dh9uRk2uOTZ-fka7VWRCpy1pNsA","Message-ID":"","Subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":36657,"web_url":"https://patchwork.libcamera.org/comment/36657/","msgid":"<10ab6e11-5aaf-40e7-bd66-5c64a29e0493@ideasonboard.com>","date":"2025-11-03T13:16:04","subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 11. 03. 13:46 keltezéssel, Naushir Patuck írta:\n> Hi Barnabás and Jacopo,\n> \n> On Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze\n> wrote:\n>>\n>> Hi\n>>\n>> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta:\n>>> Hi Barnabás\n>>>\n>>> On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote:\n>>>> Most pipeline handlers release buffers when a camera is stopped. However,\n>>>> the raspberry pi pipelines do not do that. Hence they are incompatible\n>>>\n>>>\n>>> The RPi pipeline is the only one that, apart from UVC, that implements\n>>> PipelineHandler::releaseDevice(), called in the Camera::release()\n>>> path.\n>>>\n>>> The RPi implementation of ::releaseDevice() calls \"data->freeBuffer()\"\n>>> which ends up calling the RPiStream Stream::releaseBuffers()\n>>> which first releases buffers from the video device and then clears the\n>>> FrameBuffers created around them.\n>>>\n>>> Now if I get this right, this also clears bufferMap_ which is\n>>> populated through Stream::setExportedBuffers() in the\n>>> PipelineHandlerBase::exportFrameBuffers() call.\n>>>\n>>> To make this short, my understanding is that rpi releases all\n>>> allocated buffers on the video device at configure() and release() time.\n>>\n>> That is my understanding as well.\n> \n> This is correct and a bit of a pain point in our popeline handler. To\n> summarise, our PH does not free internal buffer allocations on the\n> stop() call, but instead waits until configure() and\n> frees/re-allocates buffers if the configuration\n> (resolution/pixelformat) changes. This is to avoid fragmentation on\n> earlier Pi platforms that can only use a limited CMA carveout for\n> framebuffers where we can very quickly run out of free contiguous\n> space if we run many start/stop cycles.\n> \n> Many moons ago I suggested a parameter in stop() to allow the\n> application to either request holding on to resources (like we do in\n> our PH) or free them completely. That did not go anywhere, but\n> perhaps it's time to revisit a change like that?\n\nAs for the \"user facing\" buffers (i.e. ones from `Request::buffers()`), I think\na pipeline handler must remove any references to them at `stop()` time because\nit should have no control over their lifetimes. If they wish, users can keep them\nalive simply by not destroying them (e.g. by calling `FrameBufferAllocator::free()`),\nwhich should avoid fragmentation.\n\nThe above includes calling `V4L2VideoDevice::releaseBuffers()` (or similar) to\nremove any references from `V4L2VideoDevice::cache_` so as not to keep them\nalive longer than the user wishes.\n\nI believe this last part is one thing that the rpi pipeline is missing.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> Naush\n> \n>>\n>>\n>>\n>>>\n>>> I don't think we specify anywhere how pipelines should behave, but\n>>> this could certainly save allocations/deallocation at every start/stop\n>>> cycle.\n>>>\n>>>\n>>>> with the current model and all \"CaptureStartStop\" tests fail because\n>>>> lc-compliance allocates buffers before starting and releases them after\n>>>> stopping. Fix this by allocating buffers after the camera is configured,\n>>>> and do not release them after stopping.\n>>>\n>>> So we now have two models at play: one where buffers allocated on the\n>>> video device through FrameBufferAllocator are released at stop() time\n>>> and one where they are released at configure()/release() time.\n>>>\n>>> I wonder, but that's a general question, not specifically on this\n>>> patch which does anyway implement the behaviour I'm about to describe:\n>>> what happens if a FrameBuffers created with FrameBufferAllocator\n>>> outlives the buffers allocated on the video device it wraps ?\n>>\n>> I don't think that can happen. A `FrameBuffer` object has a `SharedFD`\n>> in each of its `Plane`s, keeping the kernel resource alive. This matches\n>> the documentation of `V4L2VideoDevice::releaseBuffers()`:\n>>\n>> [...] Buffer exported by exportBuffers(), if any, are not affected.\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>>\n>>> With this patch applied, FrameBuffers in the frame buffer allocator\n>>> are released only at ~Capture(), while when running this on, say,\n>>> rkisp1, the memory buffers on the video device are released at every stop().\n>>>\n>>> I feel like I'm missing something obvious, otherwise things should go\n>>> booom quite fast on pipelines !rpi with this change in..\n>>>\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++-----\n>>>> src/apps/lc-compliance/helpers/capture.h | 1 +\n>>>> 2 files changed, 23 insertions(+), 6 deletions(-)\n>>>>\n>>>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n>>>> index d076e964a..df0ad00ac 100644\n>>>> --- a/src/apps/lc-compliance/helpers/capture.cpp\n>>>> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n>>>> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr camera)\n>>>> Capture::~Capture()\n>>>> {\n>>>> stop();\n>>>> + unconfigure();\n>>>\n>>> a little bikeshedding to make this review more fun: I would call this\n>>> freeBuffers() or something similar.\n>>>\n>>> Thanks\n>>> j\n>>>\n>>>> }\n>>>>\n>>>> void Capture::configure(libcamera::Span roles)\n>>>> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span roles)\n>>>> ASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n>>>> ASSERT_EQ(camera_->configure(config.get()), 0);\n>>>>\n>>>> + const auto bufferCount = config->at(0).bufferCount;\n>>>> +\n>>>> + for (const auto &cfg : *config) {\n>>>> + Stream *stream = cfg.stream();\n>>>> +\n>>>> + ASSERT_GE(allocator_.allocate(stream), 0)\n>>>> + << \"Failed to allocate buffers\";\n>>>> +\n>>>> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount)\n>>>> + << \"Mismatching buffer count\";\n>>>> + }\n>>>> +\n>>>> + ASSERT_TRUE(allocator_.allocated());\n>>>> +\n>>>> config_ = std::move(config);\n>>>> }\n>>>>\n>>>> @@ -112,7 +127,7 @@ void Capture::start()\n>>>> {\n>>>> assert(config_);\n>>>> assert(!config_->empty());\n>>>> - assert(!allocator_.allocated());\n>>>> + assert(allocator_.allocated());\n>>>> assert(requests_.empty());\n>>>>\n>>>> const auto bufferCount = config_->at(0).bufferCount;\n>>>> @@ -132,9 +147,6 @@ void Capture::start()\n>>>> for (const auto &cfg : *config_) {\n>>>> Stream *stream = cfg.stream();\n>>>>\n>>>> - int count = allocator_.allocate(stream);\n>>>> - ASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n>>>> -\n>>>> const auto &buffers = allocator_.buffers(stream);\n>>>> ASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n>>>>\n>>>> @@ -144,8 +156,6 @@ void Capture::start()\n>>>> }\n>>>> }\n>>>>\n>>>> - ASSERT_TRUE(allocator_.allocated());\n>>>> -\n>>>> camera_->requestCompleted.connect(this, &Capture::requestComplete);\n>>>>\n>>>> ASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n>>>> @@ -161,6 +171,12 @@ void Capture::stop()\n>>>> camera_->requestCompleted.disconnect(this);\n>>>>\n>>>> requests_.clear();\n>>>> +}\n>>>> +\n>>>> +void Capture::unconfigure()\n>>>> +{\n>>>> + if (!config_ || !allocator_.allocated())\n>>>> + return;\n>>>>\n>>>> for (const auto &cfg : *config_) {\n>>>> EXPECT_EQ(allocator_.free(cfg.stream()), 0)\n>>>> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n>>>> index ea01c11c1..41ce4e5a6 100644\n>>>> --- a/src/apps/lc-compliance/helpers/capture.h\n>>>> +++ b/src/apps/lc-compliance/helpers/capture.h\n>>>> @@ -28,6 +28,7 @@ private:\n>>>>\n>>>> void start();\n>>>> void stop();\n>>>> + void unconfigure();\n>>>>\n>>>> int queueRequest(libcamera::Request *request);\n>>>> void requestComplete(libcamera::Request *request);\n>>>> --\n>>>> 2.50.0\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 797E6BDE4C\n\tfor ;\n\tMon, 3 Nov 2025 13:16:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30055609D8;\n\tMon, 3 Nov 2025 14:16:10 +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 76134606A0\n\tfor ;\n\tMon, 3 Nov 2025 14:16:08 +0100 (CET)","from [192.168.33.39] (185.221.140.239.nat.pool.zt.hu\n\t[185.221.140.239])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8518E8BE;\n\tMon, 3 Nov 2025 14:14:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UeQctf+V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762175655;\n\tbh=qyhoZqcyxa0D2sduR7BOUfte/PULOrE00XQdO4nR+SM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=UeQctf+V9oMTdn6fAwofO0CaWxvjkviDDY5AkWgT0+/pIHzVTFoXhbLLuL0HNmBJB\n\tC+lWjb27d6JV4JrbOnL/yGSN4Ym/NS4Lb2GDfmSWRinRO2lfMj8PTbQWIMZSYOFJ88\n\tZzBmqQGdpZU4c1Vx3p6BRXRfrCXB3Mpn1tbE7OZs=","Message-ID":"<10ab6e11-5aaf-40e7-bd66-5c64a29e0493@ideasonboard.com>","Date":"Mon, 3 Nov 2025 14:16:04 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","To":"Naushir Patuck ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<20250624122830.726987-2-barnabas.pocze@ideasonboard.com>\n\t\n\t<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36659,"web_url":"https://patchwork.libcamera.org/comment/36659/","msgid":"","date":"2025-11-03T13:28:48","subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Barnabás,\n\nOn Mon, 3 Nov 2025 at 13:16, Barnabás Pőcze\n wrote:\n>\n> Hi\n>\n> 2025. 11. 03. 13:46 keltezéssel, Naushir Patuck írta:\n> > Hi Barnabás and Jacopo,\n> >\n> > On Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze\n> > wrote:\n> >>\n> >> Hi\n> >>\n> >> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta:\n> >>> Hi Barnabás\n> >>>\n> >>> On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote:\n> >>>> Most pipeline handlers release buffers when a camera is stopped. However,\n> >>>> the raspberry pi pipelines do not do that. Hence they are incompatible\n> >>>\n> >>>\n> >>> The RPi pipeline is the only one that, apart from UVC, that implements\n> >>> PipelineHandler::releaseDevice(), called in the Camera::release()\n> >>> path.\n> >>>\n> >>> The RPi implementation of ::releaseDevice() calls \"data->freeBuffer()\"\n> >>> which ends up calling the RPiStream Stream::releaseBuffers()\n> >>> which first releases buffers from the video device and then clears the\n> >>> FrameBuffers created around them.\n> >>>\n> >>> Now if I get this right, this also clears bufferMap_ which is\n> >>> populated through Stream::setExportedBuffers() in the\n> >>> PipelineHandlerBase::exportFrameBuffers() call.\n> >>>\n> >>> To make this short, my understanding is that rpi releases all\n> >>> allocated buffers on the video device at configure() and release() time.\n> >>\n> >> That is my understanding as well.\n> >\n> > This is correct and a bit of a pain point in our popeline handler. To\n> > summarise, our PH does not free internal buffer allocations on the\n> > stop() call, but instead waits until configure() and\n> > frees/re-allocates buffers if the configuration\n> > (resolution/pixelformat) changes. This is to avoid fragmentation on\n> > earlier Pi platforms that can only use a limited CMA carveout for\n> > framebuffers where we can very quickly run out of free contiguous\n> > space if we run many start/stop cycles.\n> >\n> > Many moons ago I suggested a parameter in stop() to allow the\n> > application to either request holding on to resources (like we do in\n> > our PH) or free them completely. That did not go anywhere, but\n> > perhaps it's time to revisit a change like that?\n>\n> As for the \"user facing\" buffers (i.e. ones from `Request::buffers()`), I think\n> a pipeline handler must remove any references to them at `stop()` time because\n> it should have no control over their lifetimes. If they wish, users can keep them\n> alive simply by not destroying them (e.g. by calling `FrameBufferAllocator::free()`),\n> which should avoid fragmentation.\n>\n> The above includes calling `V4L2VideoDevice::releaseBuffers()` (or similar) to\n> remove any references from `V4L2VideoDevice::cache_` so as not to keep them\n> alive longer than the user wishes.\n>\n> I believe this last part is one thing that the rpi pipeline is missing.\n\nWon't calling V4L2VideoDevice::releaseBuffers() also release buffers\nallocated through VIDIOC_REQBUFS? This is crucial, as we have a mix\nof buffers in use, some allocated this way for internal pipeline\nhandler only use, and some allocated through (typically) the CMA heap\nor system heap for framebuffers exported to the application. We could\n(perhap should?) switch our pipeline handler to never allocate through\nV4L2VideoDevice::allocateBuffers() and have allocations managed by the\npipeline handler exclusively.\n\nIt's been a while since I've looked into this so apologies if the\nabove is wrong!\n\nNaush\n\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> >\n> > Naush\n> >\n> >>\n> >>\n> >>\n> >>>\n> >>> I don't think we specify anywhere how pipelines should behave, but\n> >>> this could certainly save allocations/deallocation at every start/stop\n> >>> cycle.\n> >>>\n> >>>\n> >>>> with the current model and all \"CaptureStartStop\" tests fail because\n> >>>> lc-compliance allocates buffers before starting and releases them after\n> >>>> stopping. Fix this by allocating buffers after the camera is configured,\n> >>>> and do not release them after stopping.\n> >>>\n> >>> So we now have two models at play: one where buffers allocated on the\n> >>> video device through FrameBufferAllocator are released at stop() time\n> >>> and one where they are released at configure()/release() time.\n> >>>\n> >>> I wonder, but that's a general question, not specifically on this\n> >>> patch which does anyway implement the behaviour I'm about to describe:\n> >>> what happens if a FrameBuffers created with FrameBufferAllocator\n> >>> outlives the buffers allocated on the video device it wraps ?\n> >>\n> >> I don't think that can happen. A `FrameBuffer` object has a `SharedFD`\n> >> in each of its `Plane`s, keeping the kernel resource alive. This matches\n> >> the documentation of `V4L2VideoDevice::releaseBuffers()`:\n> >>\n> >> [...] Buffer exported by exportBuffers(), if any, are not affected.\n> >>\n> >>\n> >> Regards,\n> >> Barnabás Pőcze\n> >>\n> >>>\n> >>> With this patch applied, FrameBuffers in the frame buffer allocator\n> >>> are released only at ~Capture(), while when running this on, say,\n> >>> rkisp1, the memory buffers on the video device are released at every stop().\n> >>>\n> >>> I feel like I'm missing something obvious, otherwise things should go\n> >>> booom quite fast on pipelines !rpi with this change in..\n> >>>\n> >>>>\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++-----\n> >>>> src/apps/lc-compliance/helpers/capture.h | 1 +\n> >>>> 2 files changed, 23 insertions(+), 6 deletions(-)\n> >>>>\n> >>>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> >>>> index d076e964a..df0ad00ac 100644\n> >>>> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> >>>> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> >>>> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr camera)\n> >>>> Capture::~Capture()\n> >>>> {\n> >>>> stop();\n> >>>> + unconfigure();\n> >>>\n> >>> a little bikeshedding to make this review more fun: I would call this\n> >>> freeBuffers() or something similar.\n> >>>\n> >>> Thanks\n> >>> j\n> >>>\n> >>>> }\n> >>>>\n> >>>> void Capture::configure(libcamera::Span roles)\n> >>>> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span roles)\n> >>>> ASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n> >>>> ASSERT_EQ(camera_->configure(config.get()), 0);\n> >>>>\n> >>>> + const auto bufferCount = config->at(0).bufferCount;\n> >>>> +\n> >>>> + for (const auto &cfg : *config) {\n> >>>> + Stream *stream = cfg.stream();\n> >>>> +\n> >>>> + ASSERT_GE(allocator_.allocate(stream), 0)\n> >>>> + << \"Failed to allocate buffers\";\n> >>>> +\n> >>>> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount)\n> >>>> + << \"Mismatching buffer count\";\n> >>>> + }\n> >>>> +\n> >>>> + ASSERT_TRUE(allocator_.allocated());\n> >>>> +\n> >>>> config_ = std::move(config);\n> >>>> }\n> >>>>\n> >>>> @@ -112,7 +127,7 @@ void Capture::start()\n> >>>> {\n> >>>> assert(config_);\n> >>>> assert(!config_->empty());\n> >>>> - assert(!allocator_.allocated());\n> >>>> + assert(allocator_.allocated());\n> >>>> assert(requests_.empty());\n> >>>>\n> >>>> const auto bufferCount = config_->at(0).bufferCount;\n> >>>> @@ -132,9 +147,6 @@ void Capture::start()\n> >>>> for (const auto &cfg : *config_) {\n> >>>> Stream *stream = cfg.stream();\n> >>>>\n> >>>> - int count = allocator_.allocate(stream);\n> >>>> - ASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> >>>> -\n> >>>> const auto &buffers = allocator_.buffers(stream);\n> >>>> ASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n> >>>>\n> >>>> @@ -144,8 +156,6 @@ void Capture::start()\n> >>>> }\n> >>>> }\n> >>>>\n> >>>> - ASSERT_TRUE(allocator_.allocated());\n> >>>> -\n> >>>> camera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >>>>\n> >>>> ASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> >>>> @@ -161,6 +171,12 @@ void Capture::stop()\n> >>>> camera_->requestCompleted.disconnect(this);\n> >>>>\n> >>>> requests_.clear();\n> >>>> +}\n> >>>> +\n> >>>> +void Capture::unconfigure()\n> >>>> +{\n> >>>> + if (!config_ || !allocator_.allocated())\n> >>>> + return;\n> >>>>\n> >>>> for (const auto &cfg : *config_) {\n> >>>> EXPECT_EQ(allocator_.free(cfg.stream()), 0)\n> >>>> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> >>>> index ea01c11c1..41ce4e5a6 100644\n> >>>> --- a/src/apps/lc-compliance/helpers/capture.h\n> >>>> +++ b/src/apps/lc-compliance/helpers/capture.h\n> >>>> @@ -28,6 +28,7 @@ private:\n> >>>>\n> >>>> void start();\n> >>>> void stop();\n> >>>> + void unconfigure();\n> >>>>\n> >>>> int queueRequest(libcamera::Request *request);\n> >>>> void requestComplete(libcamera::Request *request);\n> >>>> --\n> >>>> 2.50.0\n> >>>>\n> >>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BEA84BDE4C\n\tfor ;\n\tMon, 3 Nov 2025 13:29:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAC0A60A80;\n\tMon, 3 Nov 2025 14:29:27 +0100 (CET)","from mail-ua1-x92c.google.com (mail-ua1-x92c.google.com\n\t[IPv6:2607:f8b0:4864:20::92c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E308D606A0\n\tfor ;\n\tMon, 3 Nov 2025 14:29:25 +0100 (CET)","by mail-ua1-x92c.google.com with SMTP id\n\ta1e0cc1a2514c-935181d4aedso53315241.3\n\tfor ;\n\tMon, 03 Nov 2025 05:29:25 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"pa3WOY82\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1762176564; x=1762781364;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=fJFlMVJlVITxeWUC5+Xje7AojjrouPTOZhWH53SJfvQ=;\n\tb=pa3WOY82WCVzQXAbo1t4iCjwO5i2+bp6llhr145mEi+qkgMrs4MHkDnNBjkYg7s9Sj\n\trwwz3m3lr0rrhV3by0tp8mdJ3LYGgDkBHkGF5SeHgxLyY9VI6A85+NA++X6comiUF1Rw\n\t+n8l7kZtXFBak41OqmtqWcABSb84aotCvXc7CrJ7aSIEZM8nBPVRki4gDrlAJ8n0Hz4c\n\tv3lUaCBzWxQPnVW+NI7NNiPJKP2DkdOt6LParJgWwCqsVMeNLhAkFjOu+F7jwagLSyqD\n\tAe4x2oJgFiGnGZf+4UlkckoCZw4eKGxHMPGbvc9s4zFbbMGxbkMZM0mQHXfyDnDcFaMa\n\tRFyQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1762176564; x=1762781364;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=fJFlMVJlVITxeWUC5+Xje7AojjrouPTOZhWH53SJfvQ=;\n\tb=SJyPwlre6K/leoFmcoqtfQASzOaMi1PoluROLRPTrt4HENT6U+KTd34aBEJT8rSSMr\n\tDZPri4c6xeHDk58WgPgv512hmjrDlghLfLcepelEPcXK7KtfElvOeXJiyzdGgkcJ08y1\n\tRcd/ojXRQCpaKQfDAfQlbAJdr28hMZCxILD748FIFrxtHlRkUtJYXqg7/ZKCf2Zf9bcH\n\t8HOPROywzm8rAOQ6NWLTT9ktAzBGTpaEPjduaZPJuaUHiBSxwe2bfvfpf729W5RiUSNJ\n\twLuaaCoGf+e0gm1Lqgjj2yGEo+7t6MseCymkVf9GMwcQlLxX1lqxuOZmbrRuL9L4+cNN\n\tVD6A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXUGq9nJuIXL4xpwvgRphTdh/cvhe/GD2D2FpKhWe/ZnxjorQUokK88dKml4N4jQLNYa7gm62CiemTE+7xLqGo=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yz2ot5c/rs1nbEJMQc/4d4vKxkrUxF3SFJ3T3eQiELjA087U1ZW\n\t5eR5P6GE643f1wK4NHfNh0kcNK9fqiaasohs6CLHWVIsk3fEcHI/5wcZ9Wjb1OV9cz4MmSdXlBs\n\tHGW/rCuC2Qhp0H7bdH2eWE2FahlgsD/NfoFgMgpCzBw==","X-Gm-Gg":"ASbGncu4oF7MqwnZTmjxOrn2qe8USGQvqYFvxklfLLop890cS8DAjjYUlhlF2LdaUgd\n\tOxcsywl5sA38agRRYoKLfnFxsyAriG7gse86IY5+WhwMY+vNGRkjfSfCMNovfTc8QiyCdZ0X4vu\n\tcz7WobMPsyrFdrLSJTZufsCKMGebFR7sxXzk9yMmN0wbEeD4jljsAy95k3vEGZNU4drnsQR3gLK\n\tMjcIk5qd4mb5u7ez4RcqZ0p0GB1a0MtVyiVqVp6aasuLoBHeqVFYzjik8yRpqhG+NFq/Z2L5wfs\n\t0x85zp2jYKo31Lbkz26chDMtsKg=","X-Google-Smtp-Source":"AGHT+IG+1S2UeRt4ChDOulo9/X06H2X2soPpQKo+v+CHbutVWw7O2pY/hn/pedpifJOH13vUKG115hJjrAcyV7Tuk6c=","X-Received":"by 2002:a05:6102:5106:b0:5d5:eabc:815b with SMTP id\n\tada2fe7eead31-5dbb12f7852mr1669381137.4.1762176564174;\n\tMon, 03 Nov 2025 05:29:24 -0800 (PST)","MIME-Version":"1.0","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<20250624122830.726987-2-barnabas.pocze@ideasonboard.com>\n\t\n\t<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>\n\t\n\t<10ab6e11-5aaf-40e7-bd66-5c64a29e0493@ideasonboard.com>","In-Reply-To":"<10ab6e11-5aaf-40e7-bd66-5c64a29e0493@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 3 Nov 2025 13:28:48 +0000","X-Gm-Features":"AWmQ_bmPeABG18BUVG5kwt_H2ehYfHtqfHZT8SnYFQM9ouxARqOr20MRWwWkCek","Message-ID":"","Subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":36674,"web_url":"https://patchwork.libcamera.org/comment/36674/","msgid":"<99562773-b786-4587-826f-992353f1f0f3@ideasonboard.com>","date":"2025-11-04T12:56:27","subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 11. 03. 14:28 keltezéssel, Naushir Patuck írta:\n> Hi Barnabás,\n> \n> On Mon, 3 Nov 2025 at 13:16, Barnabás Pőcze\n> wrote:\n>>\n>> Hi\n>>\n>> 2025. 11. 03. 13:46 keltezéssel, Naushir Patuck írta:\n>>> Hi Barnabás and Jacopo,\n>>>\n>>> On Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze\n>>> wrote:\n>>>>\n>>>> Hi\n>>>>\n>>>> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta:\n>>>>> Hi Barnabás\n>>>>>\n>>>>> On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote:\n>>>>>> Most pipeline handlers release buffers when a camera is stopped. However,\n>>>>>> the raspberry pi pipelines do not do that. Hence they are incompatible\n>>>>>\n>>>>>\n>>>>> The RPi pipeline is the only one that, apart from UVC, that implements\n>>>>> PipelineHandler::releaseDevice(), called in the Camera::release()\n>>>>> path.\n>>>>>\n>>>>> The RPi implementation of ::releaseDevice() calls \"data->freeBuffer()\"\n>>>>> which ends up calling the RPiStream Stream::releaseBuffers()\n>>>>> which first releases buffers from the video device and then clears the\n>>>>> FrameBuffers created around them.\n>>>>>\n>>>>> Now if I get this right, this also clears bufferMap_ which is\n>>>>> populated through Stream::setExportedBuffers() in the\n>>>>> PipelineHandlerBase::exportFrameBuffers() call.\n>>>>>\n>>>>> To make this short, my understanding is that rpi releases all\n>>>>> allocated buffers on the video device at configure() and release() time.\n>>>>\n>>>> That is my understanding as well.\n>>>\n>>> This is correct and a bit of a pain point in our popeline handler. To\n>>> summarise, our PH does not free internal buffer allocations on the\n>>> stop() call, but instead waits until configure() and\n>>> frees/re-allocates buffers if the configuration\n>>> (resolution/pixelformat) changes. This is to avoid fragmentation on\n>>> earlier Pi platforms that can only use a limited CMA carveout for\n>>> framebuffers where we can very quickly run out of free contiguous\n>>> space if we run many start/stop cycles.\n>>>\n>>> Many moons ago I suggested a parameter in stop() to allow the\n>>> application to either request holding on to resources (like we do in\n>>> our PH) or free them completely. That did not go anywhere, but\n>>> perhaps it's time to revisit a change like that?\n>>\n>> As for the \"user facing\" buffers (i.e. ones from `Request::buffers()`), I think\n>> a pipeline handler must remove any references to them at `stop()` time because\n>> it should have no control over their lifetimes. If they wish, users can keep them\n>> alive simply by not destroying them (e.g. by calling `FrameBufferAllocator::free()`),\n>> which should avoid fragmentation.\n>>\n>> The above includes calling `V4L2VideoDevice::releaseBuffers()` (or similar) to\n>> remove any references from `V4L2VideoDevice::cache_` so as not to keep them\n>> alive longer than the user wishes.\n>>\n>> I believe this last part is one thing that the rpi pipeline is missing.\n> \n> Won't calling V4L2VideoDevice::releaseBuffers() also release buffers\n> allocated through VIDIOC_REQBUFS? This is crucial, as we have a mix\n> of buffers in use, some allocated this way for internal pipeline\n> handler only use, and some allocated through (typically) the CMA heap\n> or system heap for framebuffers exported to the application. We could\n> (perhap should?) switch our pipeline handler to never allocate through\n> V4L2VideoDevice::allocateBuffers() and have allocations managed by the\n> pipeline handler exclusively.\n\nSorry, I'm not familiar with what exactly the rpi parts do. But based on my\nunderstanding, it depends. My interpretation of https://docs.kernel.org/userspace-api/media/v4l/vidioc-reqbufs.html\nsays that VIDIOC_REQBUFS only really allocates memory if V4L2_MEMORY_MMAP.\n\nSo my understanding is that if `V4L2VideoDevice::allocateBuffers()` is not\nused, which does not seem to be the case for rpi, then there will only\nbe real allocations when it does `V4L2VideoDevice::exportBuffers()`, and\nthose buffers will be alive until the planes' `SharedFD`s are destroyed.\n\nSo importBuffers()/releaseBuffers() can be used at start/stop time, and they\nwon't do any buffer allocation. At least if I understand it correctly.\n\nRegards,\nBarnabás Pőcze\n\n> \n> It's been a while since I've looked into this so apologies if the\n> above is wrong!\n> \n> Naush\n> \n> \n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>>\n>>> Naush\n>>>\n>>>>\n>>>>\n>>>>\n>>>>>\n>>>>> I don't think we specify anywhere how pipelines should behave, but\n>>>>> this could certainly save allocations/deallocation at every start/stop\n>>>>> cycle.\n>>>>>\n>>>>>\n>>>>>> with the current model and all \"CaptureStartStop\" tests fail because\n>>>>>> lc-compliance allocates buffers before starting and releases them after\n>>>>>> stopping. Fix this by allocating buffers after the camera is configured,\n>>>>>> and do not release them after stopping.\n>>>>>\n>>>>> So we now have two models at play: one where buffers allocated on the\n>>>>> video device through FrameBufferAllocator are released at stop() time\n>>>>> and one where they are released at configure()/release() time.\n>>>>>\n>>>>> I wonder, but that's a general question, not specifically on this\n>>>>> patch which does anyway implement the behaviour I'm about to describe:\n>>>>> what happens if a FrameBuffers created with FrameBufferAllocator\n>>>>> outlives the buffers allocated on the video device it wraps ?\n>>>>\n>>>> I don't think that can happen. A `FrameBuffer` object has a `SharedFD`\n>>>> in each of its `Plane`s, keeping the kernel resource alive. This matches\n>>>> the documentation of `V4L2VideoDevice::releaseBuffers()`:\n>>>>\n>>>> [...] Buffer exported by exportBuffers(), if any, are not affected.\n>>>>\n>>>>\n>>>> Regards,\n>>>> Barnabás Pőcze\n>>>>\n>>>>>\n>>>>> With this patch applied, FrameBuffers in the frame buffer allocator\n>>>>> are released only at ~Capture(), while when running this on, say,\n>>>>> rkisp1, the memory buffers on the video device are released at every stop().\n>>>>>\n>>>>> I feel like I'm missing something obvious, otherwise things should go\n>>>>> booom quite fast on pipelines !rpi with this change in..\n>>>>>\n>>>>>>\n>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>> ---\n>>>>>> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++-----\n>>>>>> src/apps/lc-compliance/helpers/capture.h | 1 +\n>>>>>> 2 files changed, 23 insertions(+), 6 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n>>>>>> index d076e964a..df0ad00ac 100644\n>>>>>> --- a/src/apps/lc-compliance/helpers/capture.cpp\n>>>>>> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n>>>>>> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr camera)\n>>>>>> Capture::~Capture()\n>>>>>> {\n>>>>>> stop();\n>>>>>> + unconfigure();\n>>>>>\n>>>>> a little bikeshedding to make this review more fun: I would call this\n>>>>> freeBuffers() or something similar.\n>>>>>\n>>>>> Thanks\n>>>>> j\n>>>>>\n>>>>>> }\n>>>>>>\n>>>>>> void Capture::configure(libcamera::Span roles)\n>>>>>> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span roles)\n>>>>>> ASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n>>>>>> ASSERT_EQ(camera_->configure(config.get()), 0);\n>>>>>>\n>>>>>> + const auto bufferCount = config->at(0).bufferCount;\n>>>>>> +\n>>>>>> + for (const auto &cfg : *config) {\n>>>>>> + Stream *stream = cfg.stream();\n>>>>>> +\n>>>>>> + ASSERT_GE(allocator_.allocate(stream), 0)\n>>>>>> + << \"Failed to allocate buffers\";\n>>>>>> +\n>>>>>> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount)\n>>>>>> + << \"Mismatching buffer count\";\n>>>>>> + }\n>>>>>> +\n>>>>>> + ASSERT_TRUE(allocator_.allocated());\n>>>>>> +\n>>>>>> config_ = std::move(config);\n>>>>>> }\n>>>>>>\n>>>>>> @@ -112,7 +127,7 @@ void Capture::start()\n>>>>>> {\n>>>>>> assert(config_);\n>>>>>> assert(!config_->empty());\n>>>>>> - assert(!allocator_.allocated());\n>>>>>> + assert(allocator_.allocated());\n>>>>>> assert(requests_.empty());\n>>>>>>\n>>>>>> const auto bufferCount = config_->at(0).bufferCount;\n>>>>>> @@ -132,9 +147,6 @@ void Capture::start()\n>>>>>> for (const auto &cfg : *config_) {\n>>>>>> Stream *stream = cfg.stream();\n>>>>>>\n>>>>>> - int count = allocator_.allocate(stream);\n>>>>>> - ASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n>>>>>> -\n>>>>>> const auto &buffers = allocator_.buffers(stream);\n>>>>>> ASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n>>>>>>\n>>>>>> @@ -144,8 +156,6 @@ void Capture::start()\n>>>>>> }\n>>>>>> }\n>>>>>>\n>>>>>> - ASSERT_TRUE(allocator_.allocated());\n>>>>>> -\n>>>>>> camera_->requestCompleted.connect(this, &Capture::requestComplete);\n>>>>>>\n>>>>>> ASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n>>>>>> @@ -161,6 +171,12 @@ void Capture::stop()\n>>>>>> camera_->requestCompleted.disconnect(this);\n>>>>>>\n>>>>>> requests_.clear();\n>>>>>> +}\n>>>>>> +\n>>>>>> +void Capture::unconfigure()\n>>>>>> +{\n>>>>>> + if (!config_ || !allocator_.allocated())\n>>>>>> + return;\n>>>>>>\n>>>>>> for (const auto &cfg : *config_) {\n>>>>>> EXPECT_EQ(allocator_.free(cfg.stream()), 0)\n>>>>>> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n>>>>>> index ea01c11c1..41ce4e5a6 100644\n>>>>>> --- a/src/apps/lc-compliance/helpers/capture.h\n>>>>>> +++ b/src/apps/lc-compliance/helpers/capture.h\n>>>>>> @@ -28,6 +28,7 @@ private:\n>>>>>>\n>>>>>> void start();\n>>>>>> void stop();\n>>>>>> + void unconfigure();\n>>>>>>\n>>>>>> int queueRequest(libcamera::Request *request);\n>>>>>> void requestComplete(libcamera::Request *request);\n>>>>>> --\n>>>>>> 2.50.0\n>>>>>>\n>>>>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 937EEC3241\n\tfor ;\n\tTue, 4 Nov 2025 12:56:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B115460A80;\n\tTue, 4 Nov 2025 13:56:31 +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 B481E6069A\n\tfor ;\n\tTue, 4 Nov 2025 13:56:30 +0100 (CET)","from [192.168.33.40] (185.221.140.239.nat.pool.zt.hu\n\t[185.221.140.239])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D1AF1C6F;\n\tTue, 4 Nov 2025 13:54:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tstQtJkA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762260877;\n\tbh=OfueVoKGseg521edfcsWFIHNJPHJCYmDhN/EL7bvjoE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=tstQtJkAM4htcEWrXfXTNWZGkgtEuMGFyVAgXlpsnSTaReCmF7RVpSwRKdWp1JalV\n\thz2HakOQ++dPdJUZE72CvbbfwTNyzE+PkuDzEQM9KeKL4dLKGo7VOwFZ0GfB/5aY4w\n\tv9/f4iXt2o8gVXTwQhAfgD6KMG5r6nTVjnEftZTE=","Message-ID":"<99562773-b786-4587-826f-992353f1f0f3@ideasonboard.com>","Date":"Tue, 4 Nov 2025 13:56:27 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","To":"Naushir Patuck ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<20250624122830.726987-2-barnabas.pocze@ideasonboard.com>\n\t\n\t<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>\n\t\n\t<10ab6e11-5aaf-40e7-bd66-5c64a29e0493@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36678,"web_url":"https://patchwork.libcamera.org/comment/36678/","msgid":"","date":"2025-11-04T14:30:32","subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Mon, Nov 03, 2025 at 01:37:39PM +0100, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote:\n> > > Most pipeline handlers release buffers when a camera is stopped. However,\n> > > the raspberry pi pipelines do not do that. Hence they are incompatible\n> >\n> >\n> > The RPi pipeline is the only one that, apart from UVC, that implements\n> > PipelineHandler::releaseDevice(), called in the Camera::release()\n> > path.\n> >\n> > The RPi implementation of ::releaseDevice() calls \"data->freeBuffer()\"\n> > which ends up calling the RPiStream Stream::releaseBuffers()\n> > which first releases buffers from the video device and then clears the\n> > FrameBuffers created around them.\n> >\n> > Now if I get this right, this also clears bufferMap_ which is\n> > populated through Stream::setExportedBuffers() in the\n> > PipelineHandlerBase::exportFrameBuffers() call.\n> >\n> > To make this short, my understanding is that rpi releases all\n> > allocated buffers on the video device at configure() and release() time.\n>\n> That is my understanding as well.\n>\n>\n> >\n> > I don't think we specify anywhere how pipelines should behave, but\n> > this could certainly save allocations/deallocation at every start/stop\n> > cycle.\n> >\n> >\n> > > with the current model and all \"CaptureStartStop\" tests fail because\n> > > lc-compliance allocates buffers before starting and releases them after\n> > > stopping. Fix this by allocating buffers after the camera is configured,\n> > > and do not release them after stopping.\n> >\n> > So we now have two models at play: one where buffers allocated on the\n> > video device through FrameBufferAllocator are released at stop() time\n> > and one where they are released at configure()/release() time.\n> >\n> > I wonder, but that's a general question, not specifically on this\n> > patch which does anyway implement the behaviour I'm about to describe:\n> > what happens if a FrameBuffers created with FrameBufferAllocator\n> > outlives the buffers allocated on the video device it wraps ?\n>\n> I don't think that can happen. A `FrameBuffer` object has a `SharedFD`\n> in each of its `Plane`s, keeping the kernel resource alive. This matches\n> the documentation of `V4L2VideoDevice::releaseBuffers()`:\n>\n> [...] Buffer exported by exportBuffers(), if any, are not affected.\n\nMy question was a bit of nonsense, as to implement exportBuffers() we\nuse V4L2 buffer orphaning. According to the V4L2 REQBUFS ioctl\ndocumentation\n\nIf ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS`` is set, then these buffers are\norphaned and will be freed when they are unmapped or when the exported DMABUF\nfds are closed. A ``count`` value of zero frees or orphans all buffers, after\naborting or finishing any DMA in progress, an implicit\n:ref:`VIDIOC_STREAMOFF `.\n\nso, yeah, there is no risk of an \"exported\" buffer to outlive the\nmemory associated with it in the video device, because that memory has\nbeen orphaned and its lifetime is now bound to the one of the dma-buf\nassociated with it.\n\nThis flips the way I interpreted this patch. I thought this was about\navoiding an early release of the exported buffers, but this is\nactually about avoiding allocation from the FrameBufferAllocator\nat every start() as on RPi it results in\n\nERROR V4L2 v4l2_videodevice.cpp:1434 /dev/video24[19:cap]: Buffers already allocated\n\ntriggered by the fact that the V4L2VideoDevice cache has not been\nreset by a call to V4L2VideoDevice::releaseBuffers(), which doesn't\nhappen at stop() as it happens for all other pipelines,\n\nAs said, I think the what the RPi pipeline does is legit, and we make\nno restrictions on how pipelines should handle internal buffers\nallocations but at the same time it requires a bit of a more\n\"controlled\" handling of exported buffers as they can't be released\nfreely at every start/stop sequence but only after the video device\nwhere they have been exported from has been reset with\nV4L2VideoDevice::releaseBuffers().\n\nSo yeah, unless we formalize more strict requirements on how the\npipeline handlers should handle their buffers, I guess this patch is\nrequired to support RPi. This also show that FrameBufferAllocator is\nan hack but we knew that and we all hope we can get rid of it sooner\nor later once Linux gets a unified memory allocator.\n\nTo summarize:\nReviewed-by: Jacopo Mondi \n\nThanks!\n j\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> >\n> > With this patch applied, FrameBuffers in the frame buffer allocator\n> > are released only at ~Capture(), while when running this on, say,\n> > rkisp1, the memory buffers on the video device are released at every stop().\n> >\n> > I feel like I'm missing something obvious, otherwise things should go\n> > booom quite fast on pipelines !rpi with this change in..\n> >\n> > >\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++-----\n> > > src/apps/lc-compliance/helpers/capture.h | 1 +\n> > > 2 files changed, 23 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > index d076e964a..df0ad00ac 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr camera)\n> > > Capture::~Capture()\n> > > {\n> > > \tstop();\n> > > +\tunconfigure();\n> >\n> > a little bikeshedding to make this review more fun: I would call this\n> > freeBuffers() or something similar.\n> >\n> > Thanks\n> > j\n> >\n> > > }\n> > >\n> > > void Capture::configure(libcamera::Span roles)\n> > > @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span roles)\n> > > \tASSERT_EQ(config->validate(), CameraConfiguration::Valid);\n> > > \tASSERT_EQ(camera_->configure(config.get()), 0);\n> > >\n> > > +\tconst auto bufferCount = config->at(0).bufferCount;\n> > > +\n> > > +\tfor (const auto &cfg : *config) {\n> > > +\t\tStream *stream = cfg.stream();\n> > > +\n> > > +\t\tASSERT_GE(allocator_.allocate(stream), 0)\n> > > +\t\t\t<< \"Failed to allocate buffers\";\n> > > +\n> > > +\t\tASSERT_EQ(allocator_.buffers(stream).size(), bufferCount)\n> > > +\t\t\t<< \"Mismatching buffer count\";\n> > > +\t}\n> > > +\n> > > +\tASSERT_TRUE(allocator_.allocated());\n> > > +\n> > > \tconfig_ = std::move(config);\n> > > }\n> > >\n> > > @@ -112,7 +127,7 @@ void Capture::start()\n> > > {\n> > > \tassert(config_);\n> > > \tassert(!config_->empty());\n> > > -\tassert(!allocator_.allocated());\n> > > +\tassert(allocator_.allocated());\n> > > \tassert(requests_.empty());\n> > >\n> > > \tconst auto bufferCount = config_->at(0).bufferCount;\n> > > @@ -132,9 +147,6 @@ void Capture::start()\n> > > \tfor (const auto &cfg : *config_) {\n> > > \t\tStream *stream = cfg.stream();\n> > >\n> > > -\t\tint count = allocator_.allocate(stream);\n> > > -\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > -\n> > > \t\tconst auto &buffers = allocator_.buffers(stream);\n> > > \t\tASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n> > >\n> > > @@ -144,8 +156,6 @@ void Capture::start()\n> > > \t\t}\n> > > \t}\n> > >\n> > > -\tASSERT_TRUE(allocator_.allocated());\n> > > -\n> > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > >\n> > > \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> > > @@ -161,6 +171,12 @@ void Capture::stop()\n> > > \tcamera_->requestCompleted.disconnect(this);\n> > >\n> > > \trequests_.clear();\n> > > +}\n> > > +\n> > > +void Capture::unconfigure()\n> > > +{\n> > > +\tif (!config_ || !allocator_.allocated())\n> > > +\t\treturn;\n> > >\n> > > \tfor (const auto &cfg : *config_) {\n> > > \t\tEXPECT_EQ(allocator_.free(cfg.stream()), 0)\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > > index ea01c11c1..41ce4e5a6 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.h\n> > > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > > @@ -28,6 +28,7 @@ private:\n> > >\n> > > \tvoid start();\n> > > \tvoid stop();\n> > > +\tvoid unconfigure();\n> > >\n> > > \tint queueRequest(libcamera::Request *request);\n> > > \tvoid requestComplete(libcamera::Request *request);\n> > > --\n> > > 2.50.0\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 6D3E7BDE4C\n\tfor ;\n\tTue, 4 Nov 2025 14:30:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8273C60856;\n\tTue, 4 Nov 2025 15:30: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 843F46069A\n\tfor ;\n\tTue, 4 Nov 2025 15:30:36 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A5EC9EB7;\n\tTue, 4 Nov 2025 15:28:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"W5FXsDh7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762266522;\n\tbh=Oyml7KcRBRmSP0WnsLBa58JJGFdWw9DL1QXF4f4wuRI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W5FXsDh7q0AeJ2xFGRDlWP6hCZPTHGtrVR0gsVPmWOh6k7zmlchr0rq+5gFkKy11y\n\ty4MFsRnE8AgNprqE4d1C4MfRanXMG/RYkPzgKVqsq1RsKJFQLVv8e8nRLDC+NLeSIO\n\tZUgWdioQHENmVT0vYoPX9MnijR/XgF//Mr/VTbi8=","Date":"Tue, 4 Nov 2025 15:30:32 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] apps: lc-compliance: Allocate buffers when\n\tconfiguring","Message-ID":"","References":"<20250624122830.726987-1-barnabas.pocze@ideasonboard.com>\n\t<20250624122830.726987-2-barnabas.pocze@ideasonboard.com>\n\t\n\t<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<52b16dc4-c0c2-48a6-9c89-30a38bb7f4df@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\" "}}]