[{"id":21482,"web_url":"https://patchwork.libcamera.org/comment/21482/","msgid":"","date":"2021-12-01T01:44:45","subject":"Re: [libcamera-devel] [PATCH v3 04/11] libcamera: framebuffer: Add\n\tFence to FrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Dec 01, 2021 at 12:36:27AM +0100, Jacopo Mondi wrote:\n> Add to the FrameBuffer::Private class a unique pointer to a\n> synchronization Fence.\n> \n> The Fence will be used to signal the availability of the Framebuffer for\n> incoming data transfer.\n> \n> The Fence will be associated to a FrameBuffer at Request::addBuffer()\n> time, and if correctly signalled, reset by the core at Request queue\n> time.\n> \n> If a FrameBuffer completes with errors, due to a Fence wait failure, the\n> Fence will still be owned by the FrameBuffer and it is application\n> responsibility to correctly reset it before reusing the buffer.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/framebuffer.h | 4 ++\n> include/libcamera/internal/framebuffer.h | 7 +++\n> src/libcamera/framebuffer.cpp | 66 ++++++++++++++++++++++++\n> 3 files changed, 77 insertions(+)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 357bbe189551..2f2ecbda6cb4 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -18,6 +18,7 @@\n> \n> namespace libcamera {\n> \n> +class Fence;\n> class Request;\n> \n> struct FrameMetadata {\n> @@ -65,6 +66,9 @@ public:\n> \tunsigned int cookie() const { return cookie_; }\n> \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> \n> +\tFence *fence() const;\n\nDo we need this function in the public API, or could it be moved to the\nPrivate class ? It seems to be used internally only.\n\n> +\tstd::unique_ptr resetFence();\n\nFollowing the UniqueFD and std::unique_ptr<> naming convention, I think\nthis should be releaseFence() (sorry for proposing an incorrect name in\nthe previous version).\n\n> +\n> \tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> \n> private:\n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index 908b478985e7..e2609300d209 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -7,12 +7,16 @@\n> \n> #pragma once\n> \n> +#include \n> +\n> #include \n> \n> #include \n> \n> namespace libcamera {\n> \n> +class Fence;\n> +\n> class FrameBuffer::Private : public Extensible::Private\n> {\n> \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> @@ -23,7 +27,10 @@ public:\n> \tvoid setRequest(Request *request) { request_ = request; }\n> \tbool isContiguous() const { return isContiguous_; }\n> \n> +\tvoid setFence(std::unique_ptr fence);\n> +\n> private:\n> +\tstd::unique_ptr fence_;\n> \tRequest *request_;\n> \tbool isContiguous_;\n> };\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 701212f3ae21..2e6f07fb0e56 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -13,6 +13,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> /**\n> * \\file libcamera/framebuffer.h\n> * \\brief Frame buffer handling\n> @@ -129,6 +131,30 @@ FrameBuffer::Private::Private()\n> * handlers, it is called by the pipeline handlers themselves.\n> */\n> \n> +/**\n> + * \\brief Move a \\a fence in this buffer\n> + * \\param[in] fence The synchronization fence\n> + *\n> + * This function associates a Fence with this Framebuffer. The intended caller\n> + * is the Request::addBuffer() function.\n> + *\n> + * Once a FrameBuffer is associated with a Fence, the FrameBuffer will only be\n> + * made available to the hardware device once the synchronization Fence has been\n> + * correctly signalled.\n> + *\n> + * \\sa Request::prepare()\n> + *\n> + * If the FrameBuffer completes successfully the core resets the Fence and the\n> + * Buffer can be reused immediately. If handling of the Fence fails during the\n> + * request preparation, the Fence is not reset and is left in the FrameBuffer.\n> + * It is applications responsibility to correctly reset the fence and handle it\n\ns/applications/the application's/\n\n> + * opportunely before using the buffer again.\n> + */\n> +void FrameBuffer::Private::setFence(std::unique_ptr fence)\n> +{\n> +\tfence_ = std::move(fence);\n\nThis could also be an inline function in the Private class, up to you.\n\n> +}\n> +\n> /**\n> * \\fn FrameBuffer::Private::isContiguous()\n> * \\brief Check if the frame buffer stores planes contiguously in memory\n> @@ -329,6 +355,46 @@ Request *FrameBuffer::request() const\n> * libcamera core never modifies the buffer cookie.\n> */\n> \n> +/**\n> + * \\brief Retrieve a const pointer to the Fence\n> + *\n> + * This function does only return a reference to the the fence and does not\n> + * change its ownership. The fence is stored in the FrameBuffer and can only be\n> + * reset with FrameBuffer::resetFence() in case the buffer has completed with\n> + * error due to a Fence wait failure.\n> + *\n> + * If buffer with a Fence completes with errors due to a failure in handling\n> + * the fence, applications are responsible for resetting the Fence before\n> + * calling Request::addBuffer() again.\n> + *\n> + * \\sa Request::addBuffer()\n> + *\n> + * \\return A const pointer to the Fence if any, nullptr otherwise\n> + */\n> +Fence *FrameBuffer::fence() const\n> +{\n> +\treturn _d()->fence_.get();\n> +}\n> +\n> +/**\n> + * \\brief Extract the Fence associated with this Framebuffer\n> + *\n> + * This function moves the buffer's fence ownership to the caller.\n> + * After the fence has been reset, calling this function always return\n> + * nullptr.\n> + *\n> + * If buffer with a Fence completes with errors due to a failure in handling\n> + * the fence, applications are responsible for resetting the Fence before\n> + * calling Request::addBuffer() again.\n> + *\n> + * \\return A unique pointer to the Fence if set, or nullptr if the fence has\n> + * been reset already\n> + */\n> +std::unique_ptr FrameBuffer::resetFence()\n> +{\n> +\treturn std::move(_d()->fence_);\n> +}\n> +\n> /**\n> * \\fn FrameBuffer::cancel()\n> * \\brief Marks the buffer as cancelled","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 B5E3ABF415\n\tfor ;\n\tWed, 1 Dec 2021 01:45:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CFC660710;\n\tWed, 1 Dec 2021 02:45:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F323960592\n\tfor ;\n\tWed, 1 Dec 2021 02:45:10 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65E688AE;\n\tWed, 1 Dec 2021 02:45:10 +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=\"HoBb2HwC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638323110;\n\tbh=miGyMsyl1SVlKP1AwTcfifJpLyZjkm/tVROVIPhLkyk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HoBb2HwCS0V7OAptkhvALiXIn2iSBFy9cfLKq1MvUmZJtQokZqwsNhKwex0Ckbn17\n\t58tDIgKtxpAu6u6y5qziCkXjstzo7N3JhtLivc3dXUNfRk8FcHWkbNrwHB49q5LCq/\n\tzXM0mhT6keJpVrE8O8G8ZPMj98QfsvnOrfKwpQLA=","Date":"Wed, 1 Dec 2021 03:44:45 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211130233634.34173-1-jacopo@jmondi.org>\n\t<20211130233634.34173-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211130233634.34173-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 04/11] libcamera: framebuffer: Add\n\tFence to FrameBuffer","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]