[{"id":37323,"web_url":"https://patchwork.libcamera.org/comment/37323/","msgid":"<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>","date":"2025-12-12T10:07:38","subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote:\n> In order to clear the V4L2VideoDevice cache, we must call\n> V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this\n> without releasing the allocated buffers, we have to rework the internal\n> buffer allocation logic.\n>\n> Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers()\n> from within the RPi::Stream class. Instead, move these calls to the\n> PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear\n> the V4L2VideoDevice cache unconditionally on stop without de-allocating\n> the internal buffers.\n>\n> The loigc Stream::clearBuffers() can be then moved into releaseBuffers()\n\nlogic\n\n> which will actually de-allocate the internal buffers when required.\n>\n> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++-\n> .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++-------------\n> .../pipeline/rpi/common/rpi_stream.h | 1 -\n> 3 files changed, 14 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 2b61b5d241c5..aa0af367d301 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera)\n> \tdata->state_ = CameraData::State::Stopped;\n> \tdata->platformStop();\n>\n> -\tfor (auto const stream : data->streams_)\n> +\tfor (auto const stream : data->streams_) {\n> \t\tstream->dev()->streamOff();\n> +\t\tstream->dev()->releaseBuffers();\n> +\t}\n>\n> \t/* Disable SOF event generation. */\n> \tdata->frontendDevice()->setFrameStartEnabled(false);\n> @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> \tint ret;\n>\n> \tfor (auto const stream : data->streams_) {\n> +\t\tret = stream->dev()->importBuffers(VIDEO_MAX_FRAME);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n\nI still have troubles understanding the buffer allocation policy of\nyour pipeline, but that's certainly me.\n\nIt seems to me you now call V4L2VideoDevice::importBuffers() on all\nyour streams (and you have one stream for each video device in the\nmedia graph, it seems to me)\n\nWhile before this change, Stream::prepareBuffers() (where\nimportBuffers() was used to be called) was only called on some devices\nand with a variable number of buffers to import (see\nPipelineHandlerPiSP::prepareBuffers()).\n\nIs this intended, or have I missed something ?\n\n\n> \t\tif (stream->getFlags() & StreamFlag::External)\n> \t\t\tcontinue;\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> index e73f4b7d31af..f420400dfe18 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> @@ -12,9 +12,6 @@\n>\n> #include \n>\n> -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> -static constexpr unsigned int maxV4L2BufferCount = 32;\n> -\n> namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(RPISTREAM)\n> @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer)\n>\n> int Stream::allocateBuffers(unsigned int count)\n> {\n> -\tint ret;\n> +\tint ret = 0;\n>\n> \tif (!(flags_ & StreamFlag::ImportOnly)) {\n> \t\t/* Export some frame buffers for internal use. */\n> @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count)\n> \t\tresetBuffers();\n> \t}\n>\n> -\treturn dev_->importBuffers(maxV4L2BufferCount);\n> +\treturn ret;\n> }\n>\n> int Stream::queueBuffer(FrameBuffer *buffer)\n> @@ -243,8 +240,11 @@ int Stream::queueAllBuffers()\n>\n> void Stream::releaseBuffers()\n> {\n> -\tdev_->releaseBuffers();\n> -\tclearBuffers();\n> +\tavailableBuffers_ = std::queue{};\n> +\trequestBuffers_ = std::queue{};\n> +\tinternalBuffers_.clear();\n> +\tbufferMap_.clear();\n> +\tid_ = 0;\n> }\n>\n> void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> \t\t\t\t std::forward_as_tuple(buffer, false));\n> }\n>\n> -void Stream::clearBuffers()\n> -{\n> -\tavailableBuffers_ = std::queue{};\n> -\trequestBuffers_ = std::queue{};\n> -\tinternalBuffers_.clear();\n> -\tbufferMap_.clear();\n> -\tid_ = 0;\n> -}\n> -\n> int Stream::queueToDevice(FrameBuffer *buffer)\n> {\n> \tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> index c267447e5ab5..300a352a7d39 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> @@ -140,7 +140,6 @@ public:\n>\n> private:\n> \tvoid bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> -\tvoid clearBuffers();\n> \tint queueToDevice(FrameBuffer *buffer);\n>\n> \tStreamFlags flags_;\n> --\n> 2.51.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 B0C46C3257\n\tfor ;\n\tFri, 12 Dec 2025 10:07:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E206961635;\n\tFri, 12 Dec 2025 11:07:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65AD66069A\n\tfor ;\n\tFri, 12 Dec 2025 11:07:42 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1642766;\n\tFri, 12 Dec 2025 11:07:39 +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=\"RSM0hgB8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765534059;\n\tbh=4Lrd9hHUH7tPVjWNc3Ox+ZUALp4BVnooG/ZFQebbrps=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RSM0hgB8RETMTLPsJ28+zbU8iSI5VMVQjtltp9jOvGuZ01yvC1j5jVWi9t0U8MY93\n\tWJthF/hSmnlo8xTDza6qZCi1XsyyDmQB6/tt97SSe8N/SZvu8BTjxjyyEQY1GGCNsS\n\tg4EJgs8SKRRp/JpErK/w6b4hhwC2hlcnB9ar4dMg=","Date":"Fri, 12 Dec 2025 11:07:38 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","Message-ID":"<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>","References":"<20251210131302.81887-1-naush@raspberrypi.com>\n\t<20251210131302.81887-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251210131302.81887-3-naush@raspberrypi.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":37326,"web_url":"https://patchwork.libcamera.org/comment/37326/","msgid":"","date":"2025-12-12T12:25:03","subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo!\n\nOn Fri, 12 Dec 2025 at 10:07, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote:\n> > In order to clear the V4L2VideoDevice cache, we must call\n> > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this\n> > without releasing the allocated buffers, we have to rework the internal\n> > buffer allocation logic.\n> >\n> > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers()\n> > from within the RPi::Stream class. Instead, move these calls to the\n> > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear\n> > the V4L2VideoDevice cache unconditionally on stop without de-allocating\n> > the internal buffers.\n> >\n> > The loigc Stream::clearBuffers() can be then moved into releaseBuffers()\n>\n> logic\n>\n> > which will actually de-allocate the internal buffers when required.\n> >\n> > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++-\n> > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++-------------\n> > .../pipeline/rpi/common/rpi_stream.h | 1 -\n> > 3 files changed, 14 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 2b61b5d241c5..aa0af367d301 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera)\n> > data->state_ = CameraData::State::Stopped;\n> > data->platformStop();\n> >\n> > - for (auto const stream : data->streams_)\n> > + for (auto const stream : data->streams_) {\n> > stream->dev()->streamOff();\n> > + stream->dev()->releaseBuffers();\n> > + }\n> >\n> > /* Disable SOF event generation. */\n> > data->frontendDevice()->setFrameStartEnabled(false);\n> > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> > int ret;\n> >\n> > for (auto const stream : data->streams_) {\n> > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME);\n> > + if (ret < 0)\n> > + return ret;\n> > +\n>\n> I still have troubles understanding the buffer allocation policy of\n> your pipeline, but that's certainly me.\n>\n> It seems to me you now call V4L2VideoDevice::importBuffers() on all\n> your streams (and you have one stream for each video device in the\n> media graph, it seems to me)\n>\n> While before this change, Stream::prepareBuffers() (where\n> importBuffers() was used to be called) was only called on some devices\n> and with a variable number of buffers to import (see\n> PipelineHandlerPiSP::prepareBuffers()).\n>\n> Is this intended, or have I missed something ?\n\nI think the logic remains the same. On both cases, we call\nV4L2VideoDevice::importBuffers() on whichever device nodes (or\nstreams) are enabled/active. The difference in behavior with this\npatch is that we now separate the calls to exportBuffers and\nimportBuffers, and this allows us to keep the internal buffers still\nallocated when clearing the v4l2 cache. Does that make sense?\n\nNaush\n\n\n>\n>\n> > if (stream->getFlags() & StreamFlag::External)\n> > continue;\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > index e73f4b7d31af..f420400dfe18 100644\n> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > @@ -12,9 +12,6 @@\n> >\n> > #include \n> >\n> > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> > -static constexpr unsigned int maxV4L2BufferCount = 32;\n> > -\n> > namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(RPISTREAM)\n> > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer)\n> >\n> > int Stream::allocateBuffers(unsigned int count)\n> > {\n> > - int ret;\n> > + int ret = 0;\n> >\n> > if (!(flags_ & StreamFlag::ImportOnly)) {\n> > /* Export some frame buffers for internal use. */\n> > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count)\n> > resetBuffers();\n> > }\n> >\n> > - return dev_->importBuffers(maxV4L2BufferCount);\n> > + return ret;\n> > }\n> >\n> > int Stream::queueBuffer(FrameBuffer *buffer)\n> > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers()\n> >\n> > void Stream::releaseBuffers()\n> > {\n> > - dev_->releaseBuffers();\n> > - clearBuffers();\n> > + availableBuffers_ = std::queue{};\n> > + requestBuffers_ = std::queue{};\n> > + internalBuffers_.clear();\n> > + bufferMap_.clear();\n> > + id_ = 0;\n> > }\n> >\n> > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > std::forward_as_tuple(buffer, false));\n> > }\n> >\n> > -void Stream::clearBuffers()\n> > -{\n> > - availableBuffers_ = std::queue{};\n> > - requestBuffers_ = std::queue{};\n> > - internalBuffers_.clear();\n> > - bufferMap_.clear();\n> > - id_ = 0;\n> > -}\n> > -\n> > int Stream::queueToDevice(FrameBuffer *buffer)\n> > {\n> > LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > index c267447e5ab5..300a352a7d39 100644\n> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > @@ -140,7 +140,6 @@ public:\n> >\n> > private:\n> > void bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> > - void clearBuffers();\n> > int queueToDevice(FrameBuffer *buffer);\n> >\n> > StreamFlags flags_;\n> > --\n> > 2.51.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 2F9D7BD1F1\n\tfor ;\n\tFri, 12 Dec 2025 12:25:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D8206165D;\n\tFri, 12 Dec 2025 13:25:43 +0100 (CET)","from mail-vs1-xe33.google.com (mail-vs1-xe33.google.com\n\t[IPv6:2607:f8b0:4864:20::e33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BAD4D6142F\n\tfor ;\n\tFri, 12 Dec 2025 13:25:40 +0100 (CET)","by mail-vs1-xe33.google.com with SMTP id\n\tada2fe7eead31-5dfb3407d0fso71114137.1\n\tfor ;\n\tFri, 12 Dec 2025 04:25:40 -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=\"bBUb0wvZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1765542339; x=1766147139;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=X86uP2ycqskargbtP1YZ2O6G4p/UWiWfZojNRglzVC8=;\n\tb=bBUb0wvZf1bQRQRgoxGWTqbMWagD9vfzuh6X8BQO63UPBIdNIWuOaQSRj0uWCSW76+\n\ttDDFoi67Apct20IRFwoPZHIO8bdlk/bOLevgDRz6BBrDFovKYjA05J5LyBt+XHI4PoAX\n\t21jLQNX6wiFsMYU11RjDy6bObqs9sthuhk2juWmXib+UBQyF3LIdIV/8CNSOOTu83cLs\n\te5McoqUTKsIB7saQhqnI+L07C5LavjVzDsxawwg69veOtDSmsytP331BiO7NgIjGLEBD\n\tVuPBpJAtp24GBkitZzsih9SjcS7zlqJamcAAZE1gX2KU0M4ZfPnQErW+RZc0vc8jOnpW\n\tVYkw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1765542339; x=1766147139;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=X86uP2ycqskargbtP1YZ2O6G4p/UWiWfZojNRglzVC8=;\n\tb=O/luqdZHZQdFjvVedeOff84V4N73gzw5gFtbSINJjYK4CsmOxrmJ4n/2vxek4UD3h1\n\tQenvB3ZE1ny4LRONGcbc9eZfGRNNNseDwKdQFh7vrr5JS8TaWCGApx21uBF/ubmVQROD\n\tREANW+yaIogyQg6ag3oTnPSFGmX0WlW4cK74LxsHO1x8QQvhEwZc+3cX8M0/Rght7N1x\n\tWJAgK2wbVuAtwZm1N30VK74UnjStpPVV/0/iKSvG2udC541TS1/XoyLyIXVEL89t9cLs\n\t47OtCYYOkVyw21tD8CaVwja7tLZTZFok4ozD8G9jkTZKdwOo80N+ICX5e6sHXjLHiDir\n\tgKlQ==","X-Gm-Message-State":"AOJu0YxL0SHo9d5ANtkcPyIbhVod/ykipd/pA0oLFoYp4fG6187PjeOD\n\tspR9LyWJAP3QmbV5NkRk6oYJzYFfI6xmAZdglD22Jg2HlvgTuwZq2eX6VNubhGPeK3eA0nJfKF2\n\tPg2+5KH35tsh+I5GOJE/LpfzTxlPGK+LPWwIctJygcP6Q4RNGci0mXik=","X-Gm-Gg":"AY/fxX7kB/b29/oB4jlUpOVzg10A/fYD2KED9erT80XbA7Lt9+0Q60lMn+PHVLLAgvH\n\tfTiHkJezV6yB13akS4Ni4WfYRLvJyq9LlmxXwfMtrFNO8cCGXRq6y5mbuo2M4feRs/pXuePgMAF\n\tDvg2tMEivsgXnjXsAcRLXeE5uavmJWBdeyRQ+3oGxvHLEC1dqVt9BsU09KZTzFEYeruJVsbICkn\n\tMrm5uvGy+A/KcO4m/8xOOx+7k0HnVC7p6R5+Nv21zymDSO0Zldhiu6vwxCSR1aJZBUQBZ3o4wyL\n\tRz2KDcAzMwZ56WxKqKjcyF+AH/0=","X-Google-Smtp-Source":"AGHT+IFyLY4LGJLHSUY8EG60tTViaqJZ6R0ENJ54lfhsVuqzFcZLSg41Kgyo/GE5DFmfVPhF3W8do8zyZ0VXJG+QMJ4=","X-Received":"by 2002:a05:6102:1919:b0:5df:a914:bbe7 with SMTP id\n\tada2fe7eead31-5e827471530mr188299137.2.1765542339446; Fri, 12 Dec 2025\n\t04:25:39 -0800 (PST)","MIME-Version":"1.0","References":"<20251210131302.81887-1-naush@raspberrypi.com>\n\t<20251210131302.81887-3-naush@raspberrypi.com>\n\t<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>","In-Reply-To":"<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>","From":"Naushir Patuck ","Date":"Fri, 12 Dec 2025 12:25:03 +0000","X-Gm-Features":"AQt7F2qlgz2FgGtaXzaQUELrzAQIsm6KTiudepdiTqR9zSBET5oxKOG3CUKu8XQ","Message-ID":"","Subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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":37327,"web_url":"https://patchwork.libcamera.org/comment/37327/","msgid":"","date":"2025-12-12T12:35:43","subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote:\n> Hi Jacopo!\n>\n> On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi\n> wrote:\n> >\n> > Hi Naush\n> >\n> > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote:\n> > > In order to clear the V4L2VideoDevice cache, we must call\n> > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this\n> > > without releasing the allocated buffers, we have to rework the internal\n> > > buffer allocation logic.\n> > >\n> > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers()\n> > > from within the RPi::Stream class. Instead, move these calls to the\n> > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear\n> > > the V4L2VideoDevice cache unconditionally on stop without de-allocating\n> > > the internal buffers.\n> > >\n> > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers()\n> >\n> > logic\n> >\n> > > which will actually de-allocate the internal buffers when required.\n> > >\n> > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++-\n> > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++-------------\n> > > .../pipeline/rpi/common/rpi_stream.h | 1 -\n> > > 3 files changed, 14 insertions(+), 18 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > index 2b61b5d241c5..aa0af367d301 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera)\n> > > data->state_ = CameraData::State::Stopped;\n> > > data->platformStop();\n> > >\n> > > - for (auto const stream : data->streams_)\n> > > + for (auto const stream : data->streams_) {\n> > > stream->dev()->streamOff();\n> > > + stream->dev()->releaseBuffers();\n> > > + }\n> > >\n> > > /* Disable SOF event generation. */\n> > > data->frontendDevice()->setFrameStartEnabled(false);\n> > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> > > int ret;\n> > >\n> > > for (auto const stream : data->streams_) {\n> > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME);\n> > > + if (ret < 0)\n> > > + return ret;\n> > > +\n> >\n> > I still have troubles understanding the buffer allocation policy of\n> > your pipeline, but that's certainly me.\n> >\n> > It seems to me you now call V4L2VideoDevice::importBuffers() on all\n> > your streams (and you have one stream for each video device in the\n> > media graph, it seems to me)\n> >\n> > While before this change, Stream::prepareBuffers() (where\n> > importBuffers() was used to be called) was only called on some devices\n> > and with a variable number of buffers to import (see\n> > PipelineHandlerPiSP::prepareBuffers()).\n> >\n> > Is this intended, or have I missed something ?\n>\n> I think the logic remains the same. On both cases, we call\n> V4L2VideoDevice::importBuffers() on whichever device nodes (or\n> streams) are enabled/active. The difference in behavior with this\n\nOk, but why I see PipelineHandlerPiSP::prepareBuffers() cycling\nthrough streams and\n1) Skipping some of them becaue they are not active\n2) Setting a different number of buffers depending on the stream\n\nwhile I see PipelineHandlerBase::queueAllBuffers() importing the same\nnumber of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ?\n\n> patch is that we now separate the calls to exportBuffers and\n> importBuffers, and this allows us to keep the internal buffers still\n> allocated when clearing the v4l2 cache. Does that make sense?\n>\n> Naush\n>\n>\n> >\n> >\n> > > if (stream->getFlags() & StreamFlag::External)\n> > > continue;\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > index e73f4b7d31af..f420400dfe18 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > @@ -12,9 +12,6 @@\n> > >\n> > > #include \n> > >\n> > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> > > -static constexpr unsigned int maxV4L2BufferCount = 32;\n> > > -\n> > > namespace libcamera {\n> > >\n> > > LOG_DEFINE_CATEGORY(RPISTREAM)\n> > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer)\n> > >\n> > > int Stream::allocateBuffers(unsigned int count)\n> > > {\n> > > - int ret;\n> > > + int ret = 0;\n> > >\n> > > if (!(flags_ & StreamFlag::ImportOnly)) {\n> > > /* Export some frame buffers for internal use. */\n> > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count)\n> > > resetBuffers();\n> > > }\n> > >\n> > > - return dev_->importBuffers(maxV4L2BufferCount);\n> > > + return ret;\n> > > }\n> > >\n> > > int Stream::queueBuffer(FrameBuffer *buffer)\n> > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers()\n> > >\n> > > void Stream::releaseBuffers()\n> > > {\n> > > - dev_->releaseBuffers();\n> > > - clearBuffers();\n> > > + availableBuffers_ = std::queue{};\n> > > + requestBuffers_ = std::queue{};\n> > > + internalBuffers_.clear();\n> > > + bufferMap_.clear();\n> > > + id_ = 0;\n> > > }\n> > >\n> > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > std::forward_as_tuple(buffer, false));\n> > > }\n> > >\n> > > -void Stream::clearBuffers()\n> > > -{\n> > > - availableBuffers_ = std::queue{};\n> > > - requestBuffers_ = std::queue{};\n> > > - internalBuffers_.clear();\n> > > - bufferMap_.clear();\n> > > - id_ = 0;\n> > > -}\n> > > -\n> > > int Stream::queueToDevice(FrameBuffer *buffer)\n> > > {\n> > > LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > index c267447e5ab5..300a352a7d39 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > @@ -140,7 +140,6 @@ public:\n> > >\n> > > private:\n> > > void bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> > > - void clearBuffers();\n> > > int queueToDevice(FrameBuffer *buffer);\n> > >\n> > > StreamFlags flags_;\n> > > --\n> > > 2.51.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 545EBC3257\n\tfor ;\n\tFri, 12 Dec 2025 12:35:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 776566165F;\n\tFri, 12 Dec 2025 13:35:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A7DC6142F\n\tfor ;\n\tFri, 12 Dec 2025 13:35:48 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47149176B;\n\tFri, 12 Dec 2025 13:35:43 +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=\"VaHni9yb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765542944;\n\tbh=AhqOYoL0/JauwfqYZvrskjrdGst5l8zIR0sGBqqRS8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VaHni9yb/CMKdy0X786FogGnMTQcVmylVmQjmxahr7hbJD8Lj1fo3T+FXX0SdV+oi\n\tPz/T2UjyTy0Fa+TWsoEvS0/tDeSr5fS6KbhPJP2IQOxS7NE+0YANBrM0ditYijDLzu\n\t8Oj+EHkq8YXeGqVQVNvsQf1thIt37wIbvSIMFdgM=","Date":"Fri, 12 Dec 2025 13:35:43 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","Message-ID":"","References":"<20251210131302.81887-1-naush@raspberrypi.com>\n\t<20251210131302.81887-3-naush@raspberrypi.com>\n\t<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":37328,"web_url":"https://patchwork.libcamera.org/comment/37328/","msgid":"","date":"2025-12-12T12:47:55","subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Fri, 12 Dec 2025 at 12:35, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo!\n> >\n> > On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Naush\n> > >\n> > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote:\n> > > > In order to clear the V4L2VideoDevice cache, we must call\n> > > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this\n> > > > without releasing the allocated buffers, we have to rework the internal\n> > > > buffer allocation logic.\n> > > >\n> > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers()\n> > > > from within the RPi::Stream class. Instead, move these calls to the\n> > > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear\n> > > > the V4L2VideoDevice cache unconditionally on stop without de-allocating\n> > > > the internal buffers.\n> > > >\n> > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers()\n> > >\n> > > logic\n> > >\n> > > > which will actually de-allocate the internal buffers when required.\n> > > >\n> > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++-\n> > > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++-------------\n> > > > .../pipeline/rpi/common/rpi_stream.h | 1 -\n> > > > 3 files changed, 14 insertions(+), 18 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > index 2b61b5d241c5..aa0af367d301 100644\n> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera)\n> > > > data->state_ = CameraData::State::Stopped;\n> > > > data->platformStop();\n> > > >\n> > > > - for (auto const stream : data->streams_)\n> > > > + for (auto const stream : data->streams_) {\n> > > > stream->dev()->streamOff();\n> > > > + stream->dev()->releaseBuffers();\n> > > > + }\n> > > >\n> > > > /* Disable SOF event generation. */\n> > > > data->frontendDevice()->setFrameStartEnabled(false);\n> > > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> > > > int ret;\n> > > >\n> > > > for (auto const stream : data->streams_) {\n> > > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME);\n> > > > + if (ret < 0)\n> > > > + return ret;\n> > > > +\n> > >\n> > > I still have troubles understanding the buffer allocation policy of\n> > > your pipeline, but that's certainly me.\n> > >\n> > > It seems to me you now call V4L2VideoDevice::importBuffers() on all\n> > > your streams (and you have one stream for each video device in the\n> > > media graph, it seems to me)\n> > >\n> > > While before this change, Stream::prepareBuffers() (where\n> > > importBuffers() was used to be called) was only called on some devices\n> > > and with a variable number of buffers to import (see\n> > > PipelineHandlerPiSP::prepareBuffers()).\n> > >\n> > > Is this intended, or have I missed something ?\n> >\n> > I think the logic remains the same. On both cases, we call\n> > V4L2VideoDevice::importBuffers() on whichever device nodes (or\n> > streams) are enabled/active. The difference in behavior with this\n>\n> Ok, but why I see PipelineHandlerPiSP::prepareBuffers() cycling\n> through streams and\n> 1) Skipping some of them becaue they are not active\n> 2) Setting a different number of buffers depending on the stream\n>\n> while I see PipelineHandlerBase::queueAllBuffers() importing the same\n> number of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ?\n\nI think they are still equivalent.\n\nIn the old (existing) code:\n\n- in pipeilne->prepareBuffers() we cycle through all data->streams_\nand choose the number of buffers to allocate for streams that need\nspecial handling.\n- Still in the pipeilne->prepareBuffers() loop, we call\nstream->prepareBuffers(count) for all streams, which does the\nallocation with exportBuffers(count) and then calls importBuffers(32).\n\nIn the new code:\n\n- pipeilne->prepareBuffers() we cycle through all data->streams_ and\nchoose the number of buffers to allocate in\n- Still in the pipeilne->prepareBuffers() loop, call\nstream->allocateBuffers(count), which does the allocation with\nexportBuffers(count).\n- in pipeline->queueAllBuffers() during startup we cycle through all\ndata->streams_ and calls importBuffers(32).\n\nThe 32 the semi-arbitrary constant in both cases. A long time ago we\ndiscussed what number to use, and we settled on VIDEO_MAX_FRAME.\n\nThis is how the logic is *supposed* to be, but maybe I have messed up\nsomewhere? :)\n\n>\n> > patch is that we now separate the calls to exportBuffers and\n> > importBuffers, and this allows us to keep the internal buffers still\n> > allocated when clearing the v4l2 cache. Does that make sense?\n> >\n> > Naush\n> >\n> >\n> > >\n> > >\n> > > > if (stream->getFlags() & StreamFlag::External)\n> > > > continue;\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > index e73f4b7d31af..f420400dfe18 100644\n> > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > @@ -12,9 +12,6 @@\n> > > >\n> > > > #include \n> > > >\n> > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> > > > -static constexpr unsigned int maxV4L2BufferCount = 32;\n> > > > -\n> > > > namespace libcamera {\n> > > >\n> > > > LOG_DEFINE_CATEGORY(RPISTREAM)\n> > > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer)\n> > > >\n> > > > int Stream::allocateBuffers(unsigned int count)\n> > > > {\n> > > > - int ret;\n> > > > + int ret = 0;\n> > > >\n> > > > if (!(flags_ & StreamFlag::ImportOnly)) {\n> > > > /* Export some frame buffers for internal use. */\n> > > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count)\n> > > > resetBuffers();\n> > > > }\n> > > >\n> > > > - return dev_->importBuffers(maxV4L2BufferCount);\n> > > > + return ret;\n> > > > }\n> > > >\n> > > > int Stream::queueBuffer(FrameBuffer *buffer)\n> > > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers()\n> > > >\n> > > > void Stream::releaseBuffers()\n> > > > {\n> > > > - dev_->releaseBuffers();\n> > > > - clearBuffers();\n> > > > + availableBuffers_ = std::queue{};\n> > > > + requestBuffers_ = std::queue{};\n> > > > + internalBuffers_.clear();\n> > > > + bufferMap_.clear();\n> > > > + id_ = 0;\n> > > > }\n> > > >\n> > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > > std::forward_as_tuple(buffer, false));\n> > > > }\n> > > >\n> > > > -void Stream::clearBuffers()\n> > > > -{\n> > > > - availableBuffers_ = std::queue{};\n> > > > - requestBuffers_ = std::queue{};\n> > > > - internalBuffers_.clear();\n> > > > - bufferMap_.clear();\n> > > > - id_ = 0;\n> > > > -}\n> > > > -\n> > > > int Stream::queueToDevice(FrameBuffer *buffer)\n> > > > {\n> > > > LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > index c267447e5ab5..300a352a7d39 100644\n> > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > @@ -140,7 +140,6 @@ public:\n> > > >\n> > > > private:\n> > > > void bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> > > > - void clearBuffers();\n> > > > int queueToDevice(FrameBuffer *buffer);\n> > > >\n> > > > StreamFlags flags_;\n> > > > --\n> > > > 2.51.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 F23DBBD1F1\n\tfor ;\n\tFri, 12 Dec 2025 12:48:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B0746169C;\n\tFri, 12 Dec 2025 13:48:35 +0100 (CET)","from mail-vk1-xa2a.google.com (mail-vk1-xa2a.google.com\n\t[IPv6:2607:f8b0:4864:20::a2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE4846142F\n\tfor ;\n\tFri, 12 Dec 2025 13:48:32 +0100 (CET)","by mail-vk1-xa2a.google.com with SMTP id\n\t71dfb90a1353d-5595f3acf27so19959e0c.0\n\tfor ;\n\tFri, 12 Dec 2025 04:48:32 -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=\"NNQYVGSd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1765543712; x=1766148512;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=U9MJjzSY4KF444qebYxhyZ878LqY6UB1mNVwPkr4w1I=;\n\tb=NNQYVGSdCEBG8PN9RXjlaiEyV1fqd+ukpNhXPAzvcx+mq+eSFNsEr2HNynm6p1MTkZ\n\t4HI7uv3+LzCiMYF4y2u6/cSpeJ6A3GDtk7EJDP0wrn6xfodDIC5/KTqviXtMEkmeyz11\n\tSNP3K/Kd3X7CxzQ6k1gYD+PSyqZ74R9xT27wJ+Y5loEAsSqeKK3rW3s9nzyJGX4fZrcX\n\thDo81+EilzOFPbAraVRoKkIz4EGCCyIl2FaDKqwtrK/QqjscKqp3/0kBcSXn678xmppc\n\tw/Gatje43UbhdEk9vxDh6v9pztxZp5Zdsq6bc+y8H/7eNefZ+bWgiV/NcXmFWbXZ1y/g\n\txoiw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1765543712; x=1766148512;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=U9MJjzSY4KF444qebYxhyZ878LqY6UB1mNVwPkr4w1I=;\n\tb=gqtQpUndTwLYM5o/Oh8OJqWQEMyAdhUiRyt2DiuZ/Qyhj8Py8PyVsXUhPL6UmTPadO\n\tipvHabW0qeZIpr/iT1oMtQ8u2V2IXuSuF+pO1dA4INPoZZMGYq6cr1kuu9QrWJ2VanES\n\tJ3iUtiHVEOa0n7vEoKbL3qlVVw5Z0tdYolYf3qYwbd7vTDfA66oetE1yEUs9xB30zZHg\n\tiuv1AVp+hnmWdPoeuRKOIBWXorgcI9Epjivh4rJovTxsOwLtN3VJ3IXQU5Y6vChKbMUu\n\tGY/IQ5JnkvwQ2EC6xdmykQ4I236s2Yg+qxvfxeQrBIOv0OmCRLfcao9sJ2tJ/d2WrOtc\n\tiMDg==","X-Gm-Message-State":"AOJu0Yz2oEYribNdUTceK6hDEV0pC3qz/vPeZobii+izO3J2Fb/cdZt2\n\td30d3m7sk5hQuqmwqV8e6vhkcFZ2c5zazUEE93BqnC2cZTN/zMOvr7XAuNuMZn7A+VTgsmozELb\n\tZHDqYcXcHsKldJXu6WyseKm0T+vKIuZ8g+Hi9PkcHpA6/zXhmuI6OHfQ=","X-Gm-Gg":"AY/fxX4jzPXavGu/YhNmdboz+TzjYen3moCDJRET/GvqRI+GsT5gDpQvw7xkKRNmKZh\n\tf60BaJBZF4Qh/sfzqjpZF9xbsEG1PXuBJKPQuZU+mICkEdbEGBhjWAq9u0ASUeARtn6hiEOdFif\n\tLNqA3FWtWZn8caxMFAI2vEj5qkfriOEIFSpiV2rW0wt360L5Ye+f/9gU61lNw4PU6Medtgkjp+l\n\txo1LXELfx774fy67qqRu+1PPPs+i3i4o8/HVUoW6Pr5yr1FMiXa16AV9NvV5P/1m6DL+V9Kov46\n\tAHAPANkUBUegKb4nV3aDzfmWxcg=","X-Google-Smtp-Source":"AGHT+IFsbsKZAy/VcBWPrdXfbLHbAwcxSAFtAp1PjD5mcghnDIhMu8eNUKsnfd8wq0MCRkTyIrXWMfAIcvnuwX52ny8=","X-Received":"by 2002:ac5:c252:0:b0:55b:1668:8a76 with SMTP id\n\t71dfb90a1353d-55fed64dbf5mr192110e0c.2.1765543711651; Fri, 12 Dec 2025\n\t04:48:31 -0800 (PST)","MIME-Version":"1.0","References":"<20251210131302.81887-1-naush@raspberrypi.com>\n\t<20251210131302.81887-3-naush@raspberrypi.com>\n\t<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 12 Dec 2025 12:47:55 +0000","X-Gm-Features":"AQt7F2pYSQwAUlXHnAWJU_2ShqUu8IGSbfgZdOy228Z9Mo0gYnGnSSKTXoAYgSo","Message-ID":"","Subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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":37329,"web_url":"https://patchwork.libcamera.org/comment/37329/","msgid":"","date":"2025-12-12T12:58:35","subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Fri, Dec 12, 2025 at 12:47:55PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> On Fri, 12 Dec 2025 at 12:35, Jacopo Mondi\n> wrote:\n> >\n> > Hi Naush\n> >\n> > On Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote:\n> > > Hi Jacopo!\n> > >\n> > > On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi Naush\n> > > >\n> > > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote:\n> > > > > In order to clear the V4L2VideoDevice cache, we must call\n> > > > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this\n> > > > > without releasing the allocated buffers, we have to rework the internal\n> > > > > buffer allocation logic.\n> > > > >\n> > > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers()\n> > > > > from within the RPi::Stream class. Instead, move these calls to the\n> > > > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear\n> > > > > the V4L2VideoDevice cache unconditionally on stop without de-allocating\n> > > > > the internal buffers.\n> > > > >\n> > > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers()\n> > > >\n> > > > logic\n> > > >\n> > > > > which will actually de-allocate the internal buffers when required.\n> > > > >\n> > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++-\n> > > > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++-------------\n> > > > > .../pipeline/rpi/common/rpi_stream.h | 1 -\n> > > > > 3 files changed, 14 insertions(+), 18 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > index 2b61b5d241c5..aa0af367d301 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera)\n> > > > > data->state_ = CameraData::State::Stopped;\n> > > > > data->platformStop();\n> > > > >\n> > > > > - for (auto const stream : data->streams_)\n> > > > > + for (auto const stream : data->streams_) {\n> > > > > stream->dev()->streamOff();\n> > > > > + stream->dev()->releaseBuffers();\n> > > > > + }\n> > > > >\n> > > > > /* Disable SOF event generation. */\n> > > > > data->frontendDevice()->setFrameStartEnabled(false);\n> > > > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> > > > > int ret;\n> > > > >\n> > > > > for (auto const stream : data->streams_) {\n> > > > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME);\n> > > > > + if (ret < 0)\n> > > > > + return ret;\n> > > > > +\n> > > >\n> > > > I still have troubles understanding the buffer allocation policy of\n> > > > your pipeline, but that's certainly me.\n> > > >\n> > > > It seems to me you now call V4L2VideoDevice::importBuffers() on all\n> > > > your streams (and you have one stream for each video device in the\n> > > > media graph, it seems to me)\n> > > >\n> > > > While before this change, Stream::prepareBuffers() (where\n> > > > importBuffers() was used to be called) was only called on some devices\n> > > > and with a variable number of buffers to import (see\n> > > > PipelineHandlerPiSP::prepareBuffers()).\n> > > >\n> > > > Is this intended, or have I missed something ?\n> > >\n> > > I think the logic remains the same. On both cases, we call\n> > > V4L2VideoDevice::importBuffers() on whichever device nodes (or\n> > > streams) are enabled/active. The difference in behavior with this\n> >\n> > Ok, but why I see PipelineHandlerPiSP::prepareBuffers() cycling\n> > through streams and\n> > 1) Skipping some of them becaue they are not active\n> > 2) Setting a different number of buffers depending on the stream\n> >\n> > while I see PipelineHandlerBase::queueAllBuffers() importing the same\n> > number of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ?\n>\n> I think they are still equivalent.\n>\n> In the old (existing) code:\n>\n> - in pipeilne->prepareBuffers() we cycle through all data->streams_\n> and choose the number of buffers to allocate for streams that need\n> special handling.\n> - Still in the pipeilne->prepareBuffers() loop, we call\n> stream->prepareBuffers(count) for all streams, which does the\n> allocation with exportBuffers(count) and then calls importBuffers(32).\n>\n> In the new code:\n>\n> - pipeilne->prepareBuffers() we cycle through all data->streams_ and\n> choose the number of buffers to allocate in\n> - Still in the pipeilne->prepareBuffers() loop, call\n> stream->allocateBuffers(count), which does the allocation with\n> exportBuffers(count).\n> - in pipeline->queueAllBuffers() during startup we cycle through all\n> data->streams_ and calls importBuffers(32).\n>\n> The 32 the semi-arbitrary constant in both cases. A long time ago we\n> discussed what number to use, and we settled on VIDEO_MAX_FRAME.\n>\n> This is how the logic is *supposed* to be, but maybe I have messed up\n> somewhere? :)\n\nNo I don't thinks so, and you're right about that the buffer count is\nonly used by exportBuffers() and not importBuffers() so that doesn't\nchange.\n\nSo my last remaining question (sorry, I don't want to bother too much,\njust making sure this is all intended) is that previously the call to\nStream::prepareBuffers() was skipped for some streams (in example\nStitchOutput and TdnOutput in PipelineHandlerPiSP::prepareBuffers())\nwhile it now happens for all of them.\n\nI don't think it's a big deal though.\n\n>\n> >\n> > > patch is that we now separate the calls to exportBuffers and\n> > > importBuffers, and this allows us to keep the internal buffers still\n> > > allocated when clearing the v4l2 cache. Does that make sense?\n> > >\n> > > Naush\n> > >\n> > >\n> > > >\n> > > >\n> > > > > if (stream->getFlags() & StreamFlag::External)\n> > > > > continue;\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > > index e73f4b7d31af..f420400dfe18 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > > @@ -12,9 +12,6 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> > > > > -static constexpr unsigned int maxV4L2BufferCount = 32;\n> > > > > -\n> > > > > namespace libcamera {\n> > > > >\n> > > > > LOG_DEFINE_CATEGORY(RPISTREAM)\n> > > > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer)\n> > > > >\n> > > > > int Stream::allocateBuffers(unsigned int count)\n> > > > > {\n> > > > > - int ret;\n> > > > > + int ret = 0;\n> > > > >\n> > > > > if (!(flags_ & StreamFlag::ImportOnly)) {\n> > > > > /* Export some frame buffers for internal use. */\n> > > > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count)\n> > > > > resetBuffers();\n> > > > > }\n> > > > >\n> > > > > - return dev_->importBuffers(maxV4L2BufferCount);\n> > > > > + return ret;\n> > > > > }\n> > > > >\n> > > > > int Stream::queueBuffer(FrameBuffer *buffer)\n> > > > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers()\n> > > > >\n> > > > > void Stream::releaseBuffers()\n> > > > > {\n> > > > > - dev_->releaseBuffers();\n> > > > > - clearBuffers();\n> > > > > + availableBuffers_ = std::queue{};\n> > > > > + requestBuffers_ = std::queue{};\n> > > > > + internalBuffers_.clear();\n> > > > > + bufferMap_.clear();\n> > > > > + id_ = 0;\n> > > > > }\n> > > > >\n> > > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > > > std::forward_as_tuple(buffer, false));\n> > > > > }\n> > > > >\n> > > > > -void Stream::clearBuffers()\n> > > > > -{\n> > > > > - availableBuffers_ = std::queue{};\n> > > > > - requestBuffers_ = std::queue{};\n> > > > > - internalBuffers_.clear();\n> > > > > - bufferMap_.clear();\n> > > > > - id_ = 0;\n> > > > > -}\n> > > > > -\n> > > > > int Stream::queueToDevice(FrameBuffer *buffer)\n> > > > > {\n> > > > > LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > > index c267447e5ab5..300a352a7d39 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > > @@ -140,7 +140,6 @@ public:\n> > > > >\n> > > > > private:\n> > > > > void bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> > > > > - void clearBuffers();\n> > > > > int queueToDevice(FrameBuffer *buffer);\n> > > > >\n> > > > > StreamFlags flags_;\n> > > > > --\n> > > > > 2.51.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 DA667C3257\n\tfor ;\n\tFri, 12 Dec 2025 12:58:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7790616AB;\n\tFri, 12 Dec 2025 13:58:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C09496142F\n\tfor ;\n\tFri, 12 Dec 2025 13:58:40 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DCDE2520;\n\tFri, 12 Dec 2025 13:58: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=\"AduyQEve\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765544317;\n\tbh=LLHLyyj0F51cAdWJd0Ekb9s/vj4/3aHE57VIq6pB/OA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AduyQEverBkVmapRaqbAFZXS591zQ9PXeZ1Og9IXniSZodWRj0foQfnGw4lhWez89\n\t0oHkPOtgbFcx7NwmGngf7JLY44xpH+Q3PBEWhFUu3K4066PzQpGUr6E6FVTeZNus4x\n\tN817WNWWTzHCxKgFBCOG44yX5WE7jb72Qor7J2hw=","Date":"Fri, 12 Dec 2025 13:58:35 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","Message-ID":"","References":"<20251210131302.81887-1-naush@raspberrypi.com>\n\t<20251210131302.81887-3-naush@raspberrypi.com>\n\t<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":37330,"web_url":"https://patchwork.libcamera.org/comment/37330/","msgid":"","date":"2025-12-12T13:06:53","subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hey Jacopo,\n\nOn Fri, 12 Dec 2025 at 12:58, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Fri, Dec 12, 2025 at 12:47:55PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > On Fri, 12 Dec 2025 at 12:35, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Naush\n> > >\n> > > On Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote:\n> > > > Hi Jacopo!\n> > > >\n> > > > On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi\n> > > > wrote:\n> > > > >\n> > > > > Hi Naush\n> > > > >\n> > > > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote:\n> > > > > > In order to clear the V4L2VideoDevice cache, we must call\n> > > > > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this\n> > > > > > without releasing the allocated buffers, we have to rework the internal\n> > > > > > buffer allocation logic.\n> > > > > >\n> > > > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers()\n> > > > > > from within the RPi::Stream class. Instead, move these calls to the\n> > > > > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear\n> > > > > > the V4L2VideoDevice cache unconditionally on stop without de-allocating\n> > > > > > the internal buffers.\n> > > > > >\n> > > > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers()\n> > > > >\n> > > > > logic\n> > > > >\n> > > > > > which will actually de-allocate the internal buffers when required.\n> > > > > >\n> > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > ---\n> > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++-\n> > > > > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++-------------\n> > > > > > .../pipeline/rpi/common/rpi_stream.h | 1 -\n> > > > > > 3 files changed, 14 insertions(+), 18 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > index 2b61b5d241c5..aa0af367d301 100644\n> > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera)\n> > > > > > data->state_ = CameraData::State::Stopped;\n> > > > > > data->platformStop();\n> > > > > >\n> > > > > > - for (auto const stream : data->streams_)\n> > > > > > + for (auto const stream : data->streams_) {\n> > > > > > stream->dev()->streamOff();\n> > > > > > + stream->dev()->releaseBuffers();\n> > > > > > + }\n> > > > > >\n> > > > > > /* Disable SOF event generation. */\n> > > > > > data->frontendDevice()->setFrameStartEnabled(false);\n> > > > > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> > > > > > int ret;\n> > > > > >\n> > > > > > for (auto const stream : data->streams_) {\n> > > > > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME);\n> > > > > > + if (ret < 0)\n> > > > > > + return ret;\n> > > > > > +\n> > > > >\n> > > > > I still have troubles understanding the buffer allocation policy of\n> > > > > your pipeline, but that's certainly me.\n> > > > >\n> > > > > It seems to me you now call V4L2VideoDevice::importBuffers() on all\n> > > > > your streams (and you have one stream for each video device in the\n> > > > > media graph, it seems to me)\n> > > > >\n> > > > > While before this change, Stream::prepareBuffers() (where\n> > > > > importBuffers() was used to be called) was only called on some devices\n> > > > > and with a variable number of buffers to import (see\n> > > > > PipelineHandlerPiSP::prepareBuffers()).\n> > > > >\n> > > > > Is this intended, or have I missed something ?\n> > > >\n> > > > I think the logic remains the same. On both cases, we call\n> > > > V4L2VideoDevice::importBuffers() on whichever device nodes (or\n> > > > streams) are enabled/active. The difference in behavior with this\n> > >\n> > > Ok, but why I see PipelineHandlerPiSP::prepareBuffers() cycling\n> > > through streams and\n> > > 1) Skipping some of them becaue they are not active\n> > > 2) Setting a different number of buffers depending on the stream\n> > >\n> > > while I see PipelineHandlerBase::queueAllBuffers() importing the same\n> > > number of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ?\n> >\n> > I think they are still equivalent.\n> >\n> > In the old (existing) code:\n> >\n> > - in pipeilne->prepareBuffers() we cycle through all data->streams_\n> > and choose the number of buffers to allocate for streams that need\n> > special handling.\n> > - Still in the pipeilne->prepareBuffers() loop, we call\n> > stream->prepareBuffers(count) for all streams, which does the\n> > allocation with exportBuffers(count) and then calls importBuffers(32).\n> >\n> > In the new code:\n> >\n> > - pipeilne->prepareBuffers() we cycle through all data->streams_ and\n> > choose the number of buffers to allocate in\n> > - Still in the pipeilne->prepareBuffers() loop, call\n> > stream->allocateBuffers(count), which does the allocation with\n> > exportBuffers(count).\n> > - in pipeline->queueAllBuffers() during startup we cycle through all\n> > data->streams_ and calls importBuffers(32).\n> >\n> > The 32 the semi-arbitrary constant in both cases. A long time ago we\n> > discussed what number to use, and we settled on VIDEO_MAX_FRAME.\n> >\n> > This is how the logic is *supposed* to be, but maybe I have messed up\n> > somewhere? :)\n>\n> No I don't thinks so, and you're right about that the buffer count is\n> only used by exportBuffers() and not importBuffers() so that doesn't\n> change.\n>\n> So my last remaining question (sorry, I don't want to bother too much,\n> just making sure this is all intended) is that previously the call to\n> Stream::prepareBuffers() was skipped for some streams (in example\n> StitchOutput and TdnOutput in PipelineHandlerPiSP::prepareBuffers())\n> while it now happens for all of them.\n\nOh good point! In the current logic in prepareBuffers() we have:\n\n} else if (stream == &data->isp_[Isp::TdnOutput] && data->config_.disableTdn) {\n /* TDN is explicitly disabled. */\n continue;\n} else if (stream == &data->isp_[Isp::StitchOutput] &&\ndata->config_.disableHdr) {\n /* Stitch/HDR is explicitly disabled. */\n continue;\n}\n\nwhich skips the exportBuffers() and importBuffers() call. In the new\nlogic we call importBuffers() unconditionally when these features\nmight be disabled.\n\n> I don't think it's a big deal though.\n\nYes, it's not a big deal as we don't allocate buffers, just reserve\nv4l2 slots that will never be used. But I can change it and make the\nimportBuffers() call conditional like above in v2.\n\nRegards,\nNaush\n\n\n>\n> >\n> > >\n> > > > patch is that we now separate the calls to exportBuffers and\n> > > > importBuffers, and this allows us to keep the internal buffers still\n> > > > allocated when clearing the v4l2 cache. Does that make sense?\n> > > >\n> > > > Naush\n> > > >\n> > > >\n> > > > >\n> > > > >\n> > > > > > if (stream->getFlags() & StreamFlag::External)\n> > > > > > continue;\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > > > index e73f4b7d31af..f420400dfe18 100644\n> > > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > > > > > @@ -12,9 +12,6 @@\n> > > > > >\n> > > > > > #include \n> > > > > >\n> > > > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> > > > > > -static constexpr unsigned int maxV4L2BufferCount = 32;\n> > > > > > -\n> > > > > > namespace libcamera {\n> > > > > >\n> > > > > > LOG_DEFINE_CATEGORY(RPISTREAM)\n> > > > > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer)\n> > > > > >\n> > > > > > int Stream::allocateBuffers(unsigned int count)\n> > > > > > {\n> > > > > > - int ret;\n> > > > > > + int ret = 0;\n> > > > > >\n> > > > > > if (!(flags_ & StreamFlag::ImportOnly)) {\n> > > > > > /* Export some frame buffers for internal use. */\n> > > > > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count)\n> > > > > > resetBuffers();\n> > > > > > }\n> > > > > >\n> > > > > > - return dev_->importBuffers(maxV4L2BufferCount);\n> > > > > > + return ret;\n> > > > > > }\n> > > > > >\n> > > > > > int Stream::queueBuffer(FrameBuffer *buffer)\n> > > > > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers()\n> > > > > >\n> > > > > > void Stream::releaseBuffers()\n> > > > > > {\n> > > > > > - dev_->releaseBuffers();\n> > > > > > - clearBuffers();\n> > > > > > + availableBuffers_ = std::queue{};\n> > > > > > + requestBuffers_ = std::queue{};\n> > > > > > + internalBuffers_.clear();\n> > > > > > + bufferMap_.clear();\n> > > > > > + id_ = 0;\n> > > > > > }\n> > > > > >\n> > > > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > > > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> > > > > > std::forward_as_tuple(buffer, false));\n> > > > > > }\n> > > > > >\n> > > > > > -void Stream::clearBuffers()\n> > > > > > -{\n> > > > > > - availableBuffers_ = std::queue{};\n> > > > > > - requestBuffers_ = std::queue{};\n> > > > > > - internalBuffers_.clear();\n> > > > > > - bufferMap_.clear();\n> > > > > > - id_ = 0;\n> > > > > > -}\n> > > > > > -\n> > > > > > int Stream::queueToDevice(FrameBuffer *buffer)\n> > > > > > {\n> > > > > > LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > > > index c267447e5ab5..300a352a7d39 100644\n> > > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > > > > > @@ -140,7 +140,6 @@ public:\n> > > > > >\n> > > > > > private:\n> > > > > > void bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> > > > > > - void clearBuffers();\n> > > > > > int queueToDevice(FrameBuffer *buffer);\n> > > > > >\n> > > > > > StreamFlags flags_;\n> > > > > > --\n> > > > > > 2.51.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 7B448BD1F1\n\tfor ;\n\tFri, 12 Dec 2025 13:07:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67F4A617F3;\n\tFri, 12 Dec 2025 14:07:33 +0100 (CET)","from mail-vs1-xe2e.google.com (mail-vs1-xe2e.google.com\n\t[IPv6:2607:f8b0:4864:20::e2e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13BBD6142F\n\tfor ;\n\tFri, 12 Dec 2025 14:07:31 +0100 (CET)","by mail-vs1-xe2e.google.com with SMTP id\n\tada2fe7eead31-5e563b35adeso69319137.3\n\tfor ;\n\tFri, 12 Dec 2025 05:07:30 -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=\"PBphqKGy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1765544850; x=1766149650;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=00FIiZeAF6MUFg4d5KQDpzwvk019RwlDihBV42U3jdA=;\n\tb=PBphqKGycBeJaHtz9DY3qeCXmOOABTx9ps/jyekACLbrnKLUU0T/55G8KaUL95tajQ\n\tkgFEUosVBmuvbA8mvtrN8Ly3vNPU7mam6f0Iq3gmSQasdxzNZIvCqaz9lTBnHtmtK+i+\n\tQA+ZxCJaucTCrlQ1+HfTywvbHStGxLdf/+Yk4234Wj0cJKrHT5F1WWFPEXG48ixj2IhQ\n\tVYGo8XaFuOHhebrjvzYiLzgOLRTFblBiTO/EUPzTOyBCDoXRWRL7nPWYa/OAztQUXc2S\n\tjerHGNHpi4uIBXI2ITg+fJTeKwQb5aYxesg2ZL21VC9RWEAXEPpJ8z38CR/scIqR22KK\n\twmZg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1765544850; x=1766149650;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=00FIiZeAF6MUFg4d5KQDpzwvk019RwlDihBV42U3jdA=;\n\tb=TB3RqjvjIsRIZxYLC85oMyLA2XeAbtBpwP9hNe5V/8Iw3LiT9/bRaHqkvdSqKOMgO3\n\toN0gp+XqUpHHnZJimy7wV9r4pnSBowO+BruQ72ngapbYbVPkJEksMpydMd+1xXrOE5Sj\n\t0/m5pxu023vysfKmuLKv61w/wTiqhf6KO/CNjAZ3CPSQIM5T/1Kqc1iLrVZHKaB4Ok6j\n\ttU3cLr5Wf4f2ZrSHSdMi7gsbPSqzUQZPGZz6KKAN+t0GRB1V/F5KqsSQQ0hrsC6V9JJu\n\tIjGq4UeG5CaYfJALBHD2G35ScBsBadBD/FmoQ0hpPLFeNrdAsB+U704B70kPtLy9Vkw/\n\t5yhw==","X-Gm-Message-State":"AOJu0Yw9ckZzUunXcKGSLboz06hqA4OXmgWfxu3f/zs1gGJQSsLLK/3Z\n\tTysB6xcJLWBh0LsFbtn/fO9qAmT+VcQQNg+y1Pm4V6/C7zwPCuoHgCXC6b18qe/Ws15abq8DQQl\n\tpa2gZ1ZQtNJcn7ccV5mwv/qkNIxfd1/LpQ7wqJPwy/BRxWT9+1nyFF1I=","X-Gm-Gg":"AY/fxX4oQ+HtUniRiFa01n2ir1I1sjISeHHuwXYfGGwZKT5B910fzX/D7qaKifUyHxd\n\tOgCCi4BEUYlHb8/Kdkam16viP3e4+1qfsD0ODdpWH/gShZTh7ZE+r4nisxzjxQLxBaotkszgk+z\n\tZNUzrsDfhfEvCxahBw1xkl3cgQYSMxiZkp3cUgZv57MeHXGhBgnR6IhJRi7YHlK9L5yPUBFPWUU\n\trao9xJKpmTa+DjXfUEpVWDd0WutXKVB0H2x+F1QS5rgQUQhdSePy52OjP0nyR55GkfxigePlF1n\n\tFmPslHpJ3hCgJhx5DEKnafernXQ=","X-Google-Smtp-Source":"AGHT+IEQ1+GoyB2Fd1H63FSRp/b0x986dkBg1uX9asfEj5PIRtaq+Ca0RYNJaGazimo6557kxfr+GOtntEQPcHU+DwQ=","X-Received":"by 2002:a67:c590:0:b0:5df:bde2:676d with SMTP id\n\tada2fe7eead31-5e82783582emr222564137.6.1765544849739; Fri, 12 Dec 2025\n\t05:07:29 -0800 (PST)","MIME-Version":"1.0","References":"<20251210131302.81887-1-naush@raspberrypi.com>\n\t<20251210131302.81887-3-naush@raspberrypi.com>\n\t<2edisg3x2pxip4tquc4p5ggfl4ijoemuy54q4ffxjrkylmnxda@iv7xcw7j4hx6>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 12 Dec 2025 13:06:53 +0000","X-Gm-Features":"AQt7F2onl6IzEKEb6NtzxXXBtRVrG5NoQRnoPUKAyLEeBKBOfl2kEpg58bDzddo","Message-ID":"","Subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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":37371,"web_url":"https://patchwork.libcamera.org/comment/37371/","msgid":"","date":"2025-12-15T10:38:31","subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","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. 12. 10. 14:09 keltezéssel, Naushir Patuck írta:\n> In order to clear the V4L2VideoDevice cache, we must call\n> V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this\n> without releasing the allocated buffers, we have to rework the internal\n> buffer allocation logic.\n> \n> Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers()\n> from within the RPi::Stream class. Instead, move these calls to the\n> PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear\n> the V4L2VideoDevice cache unconditionally on stop without de-allocating\n> the internal buffers.\n> \n> The loigc Stream::clearBuffers() can be then moved into releaseBuffers()\n> which will actually de-allocate the internal buffers when required.\n> \n> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265\n> Signed-off-by: Naushir Patuck \n> ---\n\nThe `CaptureStartStop` tests in lc-compliance now seem to finish mostly\nwithout errors.\n\nAnything involving `StreamRole::Raw` streams still fails because it is\nexpected that calling `validate()` on configuration returned from\n`Camera::generateConfiguration()` should return `Valid` if it is\nnot modified. But it returns `Adjusted` (at least on the cases I tested).\n\nTested-by: Barnabás Pőcze # rpi4\n\n\n> .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++-\n> .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++-------------\n> .../pipeline/rpi/common/rpi_stream.h | 1 -\n> 3 files changed, 14 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 2b61b5d241c5..aa0af367d301 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera)\n> \tdata->state_ = CameraData::State::Stopped;\n> \tdata->platformStop();\n> \n> -\tfor (auto const stream : data->streams_)\n> +\tfor (auto const stream : data->streams_) {\n> \t\tstream->dev()->streamOff();\n> +\t\tstream->dev()->releaseBuffers();\n> +\t}\n> \n> \t/* Disable SOF event generation. */\n> \tdata->frontendDevice()->setFrameStartEnabled(false);\n> @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)\n> \tint ret;\n> \n> \tfor (auto const stream : data->streams_) {\n> +\t\tret = stream->dev()->importBuffers(VIDEO_MAX_FRAME);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> \t\tif (stream->getFlags() & StreamFlag::External)\n> \t\t\tcontinue;\n> \n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> index e73f4b7d31af..f420400dfe18 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> @@ -12,9 +12,6 @@\n> \n> #include \n> \n> -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> -static constexpr unsigned int maxV4L2BufferCount = 32;\n> -\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RPISTREAM)\n> @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer)\n> \n> int Stream::allocateBuffers(unsigned int count)\n> {\n> -\tint ret;\n> +\tint ret = 0;\n> \n> \tif (!(flags_ & StreamFlag::ImportOnly)) {\n> \t\t/* Export some frame buffers for internal use. */\n> @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count)\n> \t\tresetBuffers();\n> \t}\n> \n> -\treturn dev_->importBuffers(maxV4L2BufferCount);\n> +\treturn ret;\n> }\n> \n> int Stream::queueBuffer(FrameBuffer *buffer)\n> @@ -243,8 +240,11 @@ int Stream::queueAllBuffers()\n> \n> void Stream::releaseBuffers()\n> {\n> -\tdev_->releaseBuffers();\n> -\tclearBuffers();\n> +\tavailableBuffers_ = std::queue{};\n> +\trequestBuffers_ = std::queue{};\n> +\tinternalBuffers_.clear();\n> +\tbufferMap_.clear();\n> +\tid_ = 0;\n> }\n> \n> void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> \t\t\t\t std::forward_as_tuple(buffer, false));\n> }\n> \n> -void Stream::clearBuffers()\n> -{\n> -\tavailableBuffers_ = std::queue{};\n> -\trequestBuffers_ = std::queue{};\n> -\tinternalBuffers_.clear();\n> -\tbufferMap_.clear();\n> -\tid_ = 0;\n> -}\n> -\n> int Stream::queueToDevice(FrameBuffer *buffer)\n> {\n> \tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> index c267447e5ab5..300a352a7d39 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> @@ -140,7 +140,6 @@ public:\n> \n> private:\n> \tvoid bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> -\tvoid clearBuffers();\n> \tint queueToDevice(FrameBuffer *buffer);\n> \n> \tStreamFlags flags_;","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 9E537C3257\n\tfor ;\n\tMon, 15 Dec 2025 10:38:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DBCE61958;\n\tMon, 15 Dec 2025 11:38:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19E77606D5\n\tfor ;\n\tMon, 15 Dec 2025 11:38:34 +0100 (CET)","from [192.168.33.22] (185.221.143.114.nat.pool.zt.hu\n\t[185.221.143.114])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53A35ED;\n\tMon, 15 Dec 2025 11:38:29 +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=\"ppxpuq2R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765795109;\n\tbh=9H1ci7G7GHMyY0I4q8ASyfjVyBnBRqbf5DLd6BZolcs=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ppxpuq2RRKG0YeqhgRmFfW4F3WKgmMM1ePKtA221GpiPmo2+RxpHLUlwCrfdJ1q9F\n\tSSNf9ZmLbfqdOQR7gv5FVhJetnVPMATqlQaabgR3wPXZOlUp12s2RPK+XK04BLTdEv\n\tdWqRQuRTrNDSy9F8yJv+IK5yPvTX7yxX0XXaV75A=","Message-ID":"","Date":"Mon, 15 Dec 2025 11:38:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/2] pipeline: rpi: Rework internal buffer allocations","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251210131302.81887-1-naush@raspberrypi.com>\n\t<20251210131302.81887-3-naush@raspberrypi.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251210131302.81887-3-naush@raspberrypi.com>","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\" "}}]