[{"id":21667,"web_url":"https://patchwork.libcamera.org/comment/21667/","msgid":"","date":"2021-12-08T11:27:03","subject":"Re: [libcamera-devel] [PATCH v4 05/11] libcamera: request: Add\n\tFence to Request::addBuffer()","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 03:29:30PM +0100, Jacopo Mondi wrote:\n> Add an optional synchronization fence to Request::addBuffer() to allow\n\ns/synchronization fence/fence/ (same in the patch)\n\n> associating a Fence with a FrameBuffer part of a Request.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/request.h | 4 +++-\n> src/libcamera/request.cpp | 26 +++++++++++++++++++++++++-\n> 2 files changed, 28 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 8c78970d88ab..1eb537e9b09b 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -51,7 +52,8 @@ public:\n> \tControlList &controls() { return *controls_; }\n> \tControlList &metadata() { return *metadata_; }\n> \tconst BufferMap &buffers() const { return bufferMap_; }\n> -\tint addBuffer(const Stream *stream, FrameBuffer *buffer);\n> +\tint addBuffer(const Stream *stream, FrameBuffer *buffer,\n> +\t\t std::unique_ptr fence = nullptr);\n> \tFrameBuffer *findBuffer(const Stream *stream) const;\n> \n> \tuint32_t sequence() const;\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 692202473360..5ddb4b0592b3 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -14,6 +14,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -294,6 +295,7 @@ void Request::reuse(ReuseFlag flags)\n> * \\brief Add a FrameBuffer with its associated Stream to the Request\n> * \\param[in] stream The stream the buffer belongs to\n> * \\param[in] buffer The FrameBuffer to add to the request\n> + * \\param[in] fence The optional synchronization Fence\n> *\n> * A reference to the buffer is stored in the request. The caller is responsible\n> * for ensuring that the buffer will remain valid until the request complete\n> @@ -302,11 +304,24 @@ void Request::reuse(ReuseFlag flags)\n> * A request can only contain one buffer per stream. If a buffer has already\n> * been added to the request for the same stream, this function returns -EEXIST.\n> *\n> + * A synchronization Fence can be optionally associated with the \\a buffer.\n> + *\n> + * When a valid fence is provided to this function, \\a fence is moved to \\a\n> + * buffer and this Request will only be queued to the device once the\n> + * synchronization fences of all its buffers have been correctly signalled.\n> + *\n> + * If the \\a fence associated with \\a buffer fails, the application is\n> + * responsible for resetting it before associating this buffer with a new\n> + * Request by calling this function again.\n\n\"fails\" is a bit vague. How about the following ?\n\n * If the \\a fence associated with \\a buffer isn't signalled, the request will\n * fail after a timeout. The buffer will still contain the fence, which\n * applications must retrieve with FrameBuffer::releaseFence() before the buffer\n * can be reused in another request. Attempting to add a buffer that still\n * contains a fence to a request will result in this function returning -EEXIST.\n\n> + *\n> + * \\sa FrameBuffer::resetFence()\n> + *\n> * \\return 0 on success or a negative error code otherwise\n> * \\retval -EEXIST The request already contains a buffer for the stream\n> * \\retval -EINVAL The buffer does not reference a valid Stream\n> */\n> -int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> +int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n> +\t\t std::unique_ptr fence)\n> {\n> \tif (!stream) {\n> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n> @@ -323,6 +338,15 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> \t_d()->pending_.insert(buffer);\n> \tbufferMap_[stream] = buffer;\n> \n> +\t/*\n> +\t * Make sure the fence has been extracted from the buffer\n> +\t * to avoid waiting on a stale fence.\n> +\t */\n> +\tASSERT(!buffer->_d()->fence());\n\nI wonder if that's not a bit too harsh. Maybe we should log an Error\nmessage and return an error (something different than EEXIST and EINVAL\ncould be nice, although this really means that there's an issue on the\napplication side, so we could possibly reuse EEXIST) ?\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\tif (fence && fence->isValid())\n> +\t\tbuffer->_d()->setFence(std::move(fence));\n> +\n> \treturn 0;\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CFB03BDB13\n\tfor ;\n\tWed, 8 Dec 2021 11:27:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 045E86086A;\n\tWed, 8 Dec 2021 12:27:36 +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 3B7BA60225\n\tfor ;\n\tWed, 8 Dec 2021 12:27:34 +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 90F918BB;\n\tWed, 8 Dec 2021 12:27:33 +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=\"hFT3yBPW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638962853;\n\tbh=PDAP3qHNmAdvMFg/qUZI+xQ63hZTlJA0c0McUmtc/L4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hFT3yBPW1CTv8ff34YxdJNq+XSjzH4sFLFdU73o+DQ2zGMJ6+kpmZW5fhdRO2rrN0\n\t5tJvmnjozZOIlgAdbeMwp9S7EYYWFguH7+6ALyg5bWW1mECIagSrYRYFRWqCfvw5mr\n\tQaY2O+/SJz87Bpx+7/2XhgKomtUSDY7UUzdMJfZE=","Date":"Wed, 8 Dec 2021 13:27:03 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211201142936.107405-1-jacopo@jmondi.org>\n\t<20211201142936.107405-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211201142936.107405-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 05/11] libcamera: request: Add\n\tFence to Request::addBuffer()","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\" "}}]