[{"id":2267,"web_url":"https://patchwork.libcamera.org/comment/2267/","msgid":"<20190714110014.GL31102@wyvern>","date":"2019-07-14T11:00:14","subject":"Re: [libcamera-devel] [PATCH v2 14/16] libcamera: stream: Map\n\texternal buffers to indexes","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo and Laurent.\n\nThanks for your work.\n\nOn 2019-07-13 20:23:49 +0300, Laurent Pinchart wrote:\n> From: Jacopo Mondi \n> \n> Add and use an operation to assign to Buffer representing external\n> memory locations an index at queueRequest() time. The index is used to\n> identify the memory buffer to be queued to the video device once the\n> buffer will be queued in a Request.\n> \n> In order to minimize relocations in the V4L2 backend, this method\n> provides a best-effort caching mechanisms that attempts to reuse\n> BufferMemory previously mapped to the buffer's dmabuf file descriptors,\n> if any.\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/buffer.h | 2 +\n> include/libcamera/stream.h | 6 +++\n> src/libcamera/camera.cpp | 20 +++++++-\n> src/libcamera/stream.cpp | 96 ++++++++++++++++++++++++++++++++++++++\n> 4 files changed, 123 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index fc5c7d4c41b6..7b657509ab5f 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -30,6 +30,8 @@ public:\n> \tunsigned int length() const { return length_; }\n> \n> private:\n> +\tfriend class Stream;\n> +\n> \tint mmap();\n> \tint munmap();\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 1883d9e9b9c5..2e619cdf0e89 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -85,12 +85,18 @@ public:\n> protected:\n> \tfriend class Camera;\n> \n> +\tint mapBuffer(const Buffer *buffer);\n> +\tvoid unmapBuffer(const Buffer *buffer);\n> +\n> \tvoid createBuffers(MemoryType memory, unsigned int count);\n> \tvoid destroyBuffers();\n> \n> \tBufferPool bufferPool_;\n> \tStreamConfiguration configuration_;\n> \tMemoryType memoryType_;\n> +\n> +private:\n> +\tstd::vector, unsigned int>> bufferCache_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index db15fd46cd51..f2deb38da367 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -800,6 +800,7 @@ Request *Camera::createRequest(uint64_t cookie)\n> * \\retval -ENODEV The camera has been disconnected from the system\n> * \\retval -EACCES The camera is not running so requests can't be queued\n> * \\retval -EINVAL The request is invalid\n> + * \\retval -ENOMEM No buffer memory was available to handle the request\n> */\n> int Camera::queueRequest(Request *request)\n> {\n> @@ -818,6 +819,16 @@ int Camera::queueRequest(Request *request)\n> \t\t\treturn -EINVAL;\n> \t\t}\n> \n> +\t\tif (stream->memoryType() == ExternalMemory) {\n> +\t\t\tint index = stream->mapBuffer(buffer);\n> +\t\t\tif (index < 0) {\n> +\t\t\t\tLOG(Camera, Error) << \"No buffer memory available\";\n> +\t\t\t\treturn -ENOMEM;\n> +\t\t\t}\n> +\n> +\t\t\tbuffer->index_ = index;\n> +\t\t}\n> +\n> \t\tbuffer->mem_ = &stream->buffers()[buffer->index_];\n> \t}\n> \n> @@ -901,7 +912,14 @@ int Camera::stop()\n> */\n> void Camera::requestComplete(Request *request)\n> {\n> -\trequestCompleted.emit(request, request->bufferMap_);\n> +\tfor (auto it : request->buffers()) {\n> +\t\tStream *stream = it.first;\n> +\t\tBuffer *buffer = it.second;\n> +\t\tif (stream->memoryType() == ExternalMemory)\n> +\t\t\tstream->unmapBuffer(buffer);\n> +\t}\n> +\n> +\trequestCompleted.emit(request, request->buffers());\n> \tdelete request;\n> }\n> \n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index e6aa1b643a37..729d36b31712 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -13,6 +13,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"log.h\"\n> \n> /**\n> @@ -476,6 +478,8 @@ std::unique_ptr Stream::createBuffer(unsigned int index)\n> * will return a null pointer when called on streams using the ExternalMemory\n> * type.\n> *\n> + * \\sa Stream::mapBuffer()\n> + *\n> * \\return A newly created Buffer on success or nullptr otherwise\n> */\n> std::unique_ptr Stream::createBuffer(const std::array &fds)\n> @@ -521,6 +525,86 @@ std::unique_ptr Stream::createBuffer(const std::array &fds)\n> * \\return The memory type used by the stream\n> */\n> \n> +/**\n> + * \\brief Map a Buffer to a buffer memory index\n> + * \\param[in] buffer The buffer to map to a buffer memory index\n> + *\n> + * Streams configured to use externally allocated memory need to maintain a\n> + * best-effort association between the memory area the \\a buffer represents\n> + * and the associated buffer memory in the Stream's pool.\n> + *\n> + * The buffer memory to use, once the \\a buffer reaches the video device,\n> + * is selected using the index assigned to the \\a buffer and to minimize\n> + * relocations in the V4L2 back-end, this operation provides a best-effort\n> + * caching mechanism that associates to the dmabuf file descriptors contained\n> + * in the \\a buffer the index of the buffer memory that was lastly queued with\n> + * those file descriptors set.\n> + *\n> + * If the Stream uses internally allocated memory, the index of the memory\n> + * buffer to use will match the one request at Stream::createBuffer(unsigned int)\n> + * time, and no mapping is thus required.\n> + *\n> + * \\return The buffer memory index for the buffer on success, or a negative\n> + * error code otherwise\n> + * \\retval -ENOMEM No buffer memory was available to map the buffer\n> + */\n> +int Stream::mapBuffer(const Buffer *buffer)\n> +{\n> +\tASSERT(memoryType_ == ExternalMemory);\n> +\n> +\tif (bufferCache_.empty())\n> +\t\treturn -ENOMEM;\n> +\n> +\tconst std::array &dmabufs = buffer->dmabufs();\n> +\n> +\t/*\n> +\t * Try to find a previously mapped buffer in the cache. If we miss, use\n> +\t * the oldest entry in the cache.\n> +\t */\n> +\tauto map = std::find_if(bufferCache_.begin(), bufferCache_.end(),\n> +\t\t\t\t[&](std::pair, unsigned int> &entry) {\n> +\t\t\t\t\treturn entry.first == dmabufs;\n> +\t\t\t\t});\n> +\tif (map == bufferCache_.end())\n> +\t\tmap = bufferCache_.begin();\n> +\n> +\t/*\n> +\t * Update the dmabuf file descriptors of the entry. We can't assume that\n> +\t * identical file descriptor numbers refer to the same dmabuf object as\n> +\t * it may have been closed and its file descriptor reused. We thus need\n> +\t * to update the plane's internally cached mmap()ed memory.\n> +\t */\n> +\tunsigned int index = map->second;\n> +\tBufferMemory *mem = &bufferPool_.buffers()[index];\n> +\tmem->planes().clear();\n> +\n> +\tfor (unsigned int i = 0; i < dmabufs.size(); ++i) {\n> +\t\tif (dmabufs[i] == -1)\n> +\t\t\tbreak;\n> +\n> +\t\tmem->planes().emplace_back();\n> +\t\tmem->planes().back().setDmabuf(dmabufs[i], 0);\n> +\t}\n> +\n> +\t/* Remove the buffer from the cache and return its index. */\n> +\tbufferCache_.erase(map);\n> +\treturn index;\n> +}\n> +\n> +/**\n> + * \\brief Unmap a Buffer from its buffer memory\n> + * \\param[in] buffer The buffer to unmap\n> + *\n> + * This method releases the buffer memory entry that was mapped by mapBuffer(),\n> + * making it available for new mappings.\n> + */\n> +void Stream::unmapBuffer(const Buffer *buffer)\n> +{\n> +\tASSERT(memoryType_ == ExternalMemory);\n> +\n> +\tbufferCache_.emplace_back(buffer->dmabufs(), buffer->index());\n> +}\n> +\n> /**\n> * \\brief Create buffers for the stream\n> * \\param count The number of buffers to create\n> @@ -536,6 +620,18 @@ void Stream::createBuffers(MemoryType memory, unsigned int count)\n> \n> \tmemoryType_ = memory;\n> \tbufferPool_.createBuffers(count);\n> +\n> +\t/* Streams with internal memory usage do not need buffer mapping. */\n> +\tif (memoryType_ == InternalMemory)\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Prepare for buffer mapping by adding all buffer memory entries to the\n> +\t * cache.\n> +\t */\n> +\tbufferCache_.clear();\n> +\tfor (unsigned int i = 0; i < bufferPool_.count(); ++i)\n> +\t\tbufferCache_.emplace_back(std::array{ -1, -1, -1 }, i);\n> }\n> \n> /**\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-pf1-x444.google.com (mail-pf1-x444.google.com\n\t[IPv6:2607:f8b0:4864:20::444])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5C9C61572\n\tfor ;\n\tSun, 14 Jul 2019 13:00:20 +0200 (CEST)","by mail-pf1-x444.google.com with SMTP id q10so6144223pff.9\n\tfor ;\n\tSun, 14 Jul 2019 04:00:20 -0700 (PDT)","from localhost (softbank126159224182.bbtec.net. [126.159.224.182])\n\tby smtp.gmail.com with ESMTPSA id\n\t34sm14071090pgl.15.2019.07.14.04.00.18\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSun, 14 Jul 2019 04:00:18 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=tdxhpvaGYJME9ENDkiyWL9WLDrX5W/+TvEu1zJfk41g=;\n\tb=MpclwRVcoH5e9UjnZAZ9d/828xpi8hK5P8YwZUvh1Aat5RD68BgXMogu1wmyX77EiP\n\tcu74BjXqEpHXZVrcuZ4+x7IVAXSYZATZm3xMVeTdfdSiaEd6A+w54wOmm8pwK5NEG5hJ\n\tmbjLqWGVhlzfcEYvOENUlEqVITJGHKmJzNYFGvwvq7GiSj4XeRap40LIjYeuctiIvXpK\n\tvPL7AooXtUYkX1BiE5EyUMdo4nLMdjAn/eMFXZiljIAZvKmW/zQgBBeC04fFaMLQ6FHk\n\tZOTwWZ9hu4l4uXp8GqnRJgqo+dqbn/59O45KiCiqhbC+xoggIV8g9tKsNIRgOTeojwQn\n\tkRBw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=tdxhpvaGYJME9ENDkiyWL9WLDrX5W/+TvEu1zJfk41g=;\n\tb=YINhYbWh5Tv7PKVGV3NGP2+2WGidytIQqsjPLYmZi8ZdcYs1yg8QymvHfjlPcjMHCm\n\tArbuL5rup3AGBIuoi08M8pIcTB8zTjmAXG2rtcE10696NtWSVhftpNkwaBJd63AQWZ+p\n\tqrIgIFsUS042U4D1EzcV/WUtqRsG4ZL/fui714Tfa7hPT3PuujAySHNDkj2TsaUlh5jc\n\tLvqmpukt5N3I5lp9iaM15Gk/MjnwOBZpCockH3iQRFtLIAJcjlzOHHewb2U6egbBmvJu\n\tw4cDsGyLkuS4lNrLUecRzxvV1GhZXLqm+GSaOqEWr9BhYGVYdqMVq9NBLsKwj4cWwpmH\n\tdGCw==","X-Gm-Message-State":"APjAAAUDFI3Tl19CyxJ+/gp4xxa/Hz8OBxdqLppb7mhDjerq7N180D/f\n\tIqGcmPHWuVwPoM/5mL07im4=","X-Google-Smtp-Source":"APXvYqy9fB39qwTtrwIkSh/uctPbin8hdccNqaiE7/U5tffRjHVsnyrX6bxsedtmXhyh+s5QLs+PIw==","X-Received":"by 2002:a65:44cb:: with SMTP id\n\tg11mr10827800pgs.288.1563102019565; \n\tSun, 14 Jul 2019 04:00:19 -0700 (PDT)","Date":"Sun, 14 Jul 2019 20:00:14 +0900","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190714110014.GL31102@wyvern>","References":"<20190713172351.25452-1-laurent.pinchart@ideasonboard.com>\n\t<20190713172351.25452-15-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190713172351.25452-15-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 14/16] libcamera: stream: Map\n\texternal buffers to indexes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 14 Jul 2019 11:00:21 -0000"}}]