[{"id":27938,"web_url":"https://patchwork.libcamera.org/comment/27938/","msgid":"","date":"2023-10-12T07:21:06","subject":"Re: [libcamera-devel] [PATCH 01/20] pipeline: rpi: Add RequiresMmap\n\tflag to RPi::Stream","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, Oct 06, 2023 at 02:19:41PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Add a new RequiresMmap flag to the RPi::Stream class indicating that\n> buffers handled by the stream must be mmapped after allocation and\n> cached internally.\n>\n> Add a new member function getBuffer(id) which can be used to obtain the\n> mapped buffers for a given buffer id.\n>\n> Add a new member function acquireBuffer() which can be used to obtain\n> any mapped buffer that has not already been acquired by the caller.\n>\n> As a drive-by, add the header to rpi_stream.cpp as it is\n> needed for the std::find_if() function.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/rpi/common/pipeline_base.cpp | 2 +-\n> .../pipeline/rpi/common/rpi_stream.cpp | 50 +++++++++++++++++--\n> .../pipeline/rpi/common/rpi_stream.h | 32 +++++++++++-\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 6 +--\n> 4 files changed, 81 insertions(+), 9 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 fad710a63c46..825eae80d014 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -888,7 +888,7 @@ void PipelineHandlerBase::mapBuffers(Camera *camera, const BufferMap &buffers, u\n> \t */\n> \tfor (auto const &it : buffers) {\n> \t\tbufferIds.push_back(IPABuffer(mask | it.first,\n> -\t\t\t\t\t it.second->planes()));\n> +\t\t\t\t\t it.second.buffer->planes()));\n> \t\tdata->bufferIds_.insert(mask | it.first);\n> \t}\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> index 7319f510a371..83c2205f7ca0 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> @@ -6,6 +6,10 @@\n> */\n> #include \"rpi_stream.h\"\n>\n> +#include \n> +#include \n> +#include \n> +\n> #include \n>\n> /* Maximum number of buffer slots to allocate in the V4L2 device driver. */\n> @@ -17,8 +21,13 @@ LOG_DEFINE_CATEGORY(RPISTREAM)\n>\n> namespace RPi {\n>\n> +const BufferObject Stream::errorBufferObject{ nullptr, false };\n> +\n> void Stream::setFlags(StreamFlags flags)\n> {\n> +\t/* We don't want dynamic mmapping. */\n> +\tASSERT(!(flags & StreamFlag::RequiresMmap));\n> +\n> \tflags_ |= flags;\n>\n> \t/* Import streams cannot be external. */\n> @@ -27,6 +36,9 @@ void Stream::setFlags(StreamFlags flags)\n>\n> void Stream::clearFlags(StreamFlags flags)\n> {\n> +\t/* We don't want dynamic mmapping. */\n> +\tASSERT(!(flags & StreamFlag::RequiresMmap));\n> +\n> \tflags_ &= ~flags;\n> }\n>\n> @@ -56,7 +68,7 @@ void Stream::resetBuffers()\n> void Stream::setExportedBuffers(std::vector> *buffers)\n> {\n> \tfor (auto const &buffer : *buffers)\n> -\t\tbufferMap_.emplace(++id_, buffer.get());\n> +\t\tbufferEmplace(++id_, buffer.get());\n> }\n>\n> const BufferMap &Stream::getBuffers() const\n> @@ -71,7 +83,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const\n>\n> \t/* Find the buffer in the map, and return the buffer id. */\n> \tauto it = std::find_if(bufferMap_.begin(), bufferMap_.end(),\n> -\t\t\t [&buffer](auto const &p) { return p.second == buffer; });\n> +\t\t\t [&buffer](auto const &p) { return p.second.buffer == buffer; });\n>\n> \tif (it == bufferMap_.end())\n> \t\treturn 0;\n> @@ -81,7 +93,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const\n>\n> void Stream::setExportedBuffer(FrameBuffer *buffer)\n> {\n> -\tbufferMap_.emplace(++id_, buffer);\n> +\tbufferEmplace(++id_, buffer);\n> }\n>\n> int Stream::prepareBuffers(unsigned int count)\n> @@ -180,6 +192,27 @@ void Stream::returnBuffer(FrameBuffer *buffer)\n> \t}\n> }\n>\n> +const BufferObject &Stream::getBuffer(unsigned int id)\n> +{\n> +\tauto const &it = bufferMap_.find(id);\n> +\tif (it == bufferMap_.end())\n> +\t\treturn errorBufferObject;\n> +\n> +\treturn it->second;\n> +}\n> +\n> +const BufferObject &Stream::acquireBuffer()\n> +{\n> +\t/* No id provided, so pick up the next available buffer if possible. */\n> +\tif (availableBuffers_.empty())\n> +\t\treturn errorBufferObject;\n> +\n> +\tunsigned int id = getBufferId(availableBuffers_.front());\n> +\tavailableBuffers_.pop();\n> +\n> +\treturn getBuffer(id);\n> +}\n> +\n> int Stream::queueAllBuffers()\n> {\n> \tint ret;\n> @@ -204,6 +237,17 @@ void Stream::releaseBuffers()\n> \tclearBuffers();\n> }\n>\n> +void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer)\n> +{\n> +\tif (flags_ & StreamFlag::RequiresMmap) {\n> +\t\tbufferMap_.emplace(std::piecewise_construct, std::forward_as_tuple(id),\n> +\t\t\t\t std::forward_as_tuple(buffer, true));\n> +\t} else {\n> +\t\tbufferMap_.emplace(std::piecewise_construct, std::forward_as_tuple(id),\n> +\t\t\t\t std::forward_as_tuple(buffer, false));\n> +\t}\n\nnit: drop {}\n\ncan be done when applying\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +}\n> +\n> void Stream::clearBuffers()\n> {\n> \tavailableBuffers_ = std::queue{};\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> index 889b499782a4..861e9c8e7dab 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> @@ -7,22 +7,23 @@\n>\n> #pragma once\n>\n> +#include \n> #include \n> #include \n> #include \n> #include \n>\n> #include \n> +\n> #include \n>\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n>\n> namespace libcamera {\n>\n> namespace RPi {\n>\n> -using BufferMap = std::unordered_map;\n> -\n> enum BufferMask {\n> \tMaskID\t\t\t= 0x00ffff,\n> \tMaskStats\t\t= 0x010000,\n> @@ -30,6 +31,21 @@ enum BufferMask {\n> \tMaskBayerData\t\t= 0x040000,\n> };\n>\n> +struct BufferObject {\n> +\tBufferObject(FrameBuffer *b, bool requiresMmap)\n> +\t\t: buffer(b), mapped(std::nullopt)\n> +\t{\n> +\t\tif (requiresMmap)\n> +\t\t\tmapped = std::make_optional\n> +\t\t\t\t\t(b, MappedFrameBuffer::MapFlag::ReadWrite);\n> +\t}\n> +\n> +\tFrameBuffer *buffer;\n> +\tstd::optional mapped;\n> +};\n> +\n> +using BufferMap = std::unordered_map;\n> +\n> /*\n> * Device stream abstraction for either an internal or external stream.\n> * Used for both Unicam and the ISP.\n> @@ -49,6 +65,11 @@ public:\n> \t\t * buffers might be provided by (and returned to) the application.\n> \t\t */\n> \t\tExternal\t= (1 << 1),\n> +\t\t/*\n> +\t\t * Indicates that the stream buffers need to be mmaped and returned\n> +\t\t * to the pipeline handler when requested.\n> +\t\t */\n> +\t\tRequiresMmap\t= (1 << 2),\n> \t};\n>\n> \tusing StreamFlags = Flags;\n> @@ -82,10 +103,17 @@ public:\n> \tint queueBuffer(FrameBuffer *buffer);\n> \tvoid returnBuffer(FrameBuffer *buffer);\n>\n> +\tconst BufferObject &getBuffer(unsigned int id);\n> +\tconst BufferObject &acquireBuffer();\n> +\n> \tint queueAllBuffers();\n> \tvoid releaseBuffers();\n>\n> +\t/* For error handling. */\n> +\tstatic const BufferObject errorBufferObject;\n> +\n> private:\n> +\tvoid bufferEmplace(unsigned int id, FrameBuffer *buffer);\n> \tvoid clearBuffers();\n> \tint queueToDevice(FrameBuffer *buffer);\n>\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index c189abaabc87..bc90d6324777 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -825,7 +825,7 @@ void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)\n> \tif (!isRunning())\n> \t\treturn;\n>\n> -\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);\n> +\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID).buffer;\n>\n> \thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n>\n> @@ -842,7 +842,7 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> \tif (!isRunning())\n> \t\treturn;\n>\n> -\tbuffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);\n> +\tbuffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID).buffer;\n> \tLOG(RPI, Debug) << \"Input re-queue to ISP, buffer id \" << (bayer & RPi::MaskID)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n>\n> @@ -850,7 +850,7 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)\n> \tispOutputCount_ = 0;\n>\n> \tif (sensorMetadata_ && embeddedId) {\n> -\t\tbuffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);\n> +\t\tbuffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID).buffer;\n> \t\thandleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);\n> \t}\n>\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 35557C3272\n\tfor ;\n\tThu, 12 Oct 2023 07:21:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76FEF6297F;\n\tThu, 12 Oct 2023 09:21:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6091662962\n\tfor ;\n\tThu, 12 Oct 2023 09:21:09 +0200 (CEST)","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 3F94BF0C;\n\tThu, 12 Oct 2023 09:21:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697095271;\n\tbh=QOMNwBNNzPrlMSF4cVsHuS2qNaoqEz+0Kgcou+krlSs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Wz7EVjmOGD1RNaprv6eM9nOY8173zXkD2NY4SemZx1IfUC1dyQIM8u5LCMvRN7Vnk\n\trQjKgRJakYCrqX6ll0wD8YTnHRbeXP4Z+PGqXdnd2+McnE57BUapPq1/qAN0XVVwZ3\n\t8UxLqJvkPk7ULbNUyIJT5LRC+IS1qPjddBc7DWBhLfH6ghvDuHIWCPLAYbvT3aR1Be\n\tURj3naJhIG90hv4mgVms5ZQBizi3luSQKQ+8vy/4Zuqv3PLJdWF1qtsHc8K5f2nQJF\n\tafU6mcH/jHnWkuvfP5LZZoFjp8iwwbOOTt0yJKuynpmqpgEcSiXRm97h7H7X7shsa+\n\t3IV5KuDiHVnug==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697095266;\n\tbh=QOMNwBNNzPrlMSF4cVsHuS2qNaoqEz+0Kgcou+krlSs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sybHGnc6nOZ5fleJ0eBEa8mGL/rXM+ildus/vyM1YZSv6R01qj4hN6Qd16MN53VqW\n\toxd7SVp4BEbShIHfaNB+fc9btVNDPkaDL34OOhsXLHgUt7sEKqy5nigekg44wGIliH\n\tZFpy1+4/MKnsVICiPMgqKgQYaRyeVIfFoLbFNhHg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"sybHGnc6\"; dkim-atps=neutral","Date":"Thu, 12 Oct 2023 09:21:06 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20231006132000.23504-1-naush@raspberrypi.com>\n\t<20231006132000.23504-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231006132000.23504-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 01/20] pipeline: rpi: Add RequiresMmap\n\tflag to RPi::Stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]