[{"id":21759,"web_url":"https://patchwork.libcamera.org/comment/21759/","msgid":"","date":"2021-12-10T21:13:54","subject":"Re: [libcamera-devel] [PATCH v5 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 Fri, Dec 10, 2021 at 09:52:33PM +0100, Jacopo Mondi wrote:\n> Add an optional fence parameter to Request::addBuffer() to allow\n> associating a Fence with a FrameBuffer.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/request.h | 4 +++-\n> src/libcamera/request.cpp | 35 ++++++++++++++++++++++++++++++++++-\n> 2 files changed, 37 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..016db9d62340 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 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,30 @@ 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 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> + * 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\nI think you forgot to drop this paragraph when adding the next one.\n\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> + * \\sa FrameBuffer::releaseFence()\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\n * \\retval -EEXIST The request already contains a buffer for the stream,\n * or the buffer still references a fence\n\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 +344,18 @@ 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> +\tif (buffer->_d()->fence()) {\n> +\t\tLOG(Request, Error) << \"Failed to add buffer with a valid fence\";\n\nMaybe\n\n\t\tLOG(Request, Error) << \"Can't add buffer that still references a fence\";\n\nto match the documentation ?\n\n\n> +\t\treturn -EEXIST;\n> +\t}\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 98280BF415\n\tfor ;\n\tFri, 10 Dec 2021 21:14:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08A4F6087E;\n\tFri, 10 Dec 2021 22:14:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1DC060868\n\tfor ;\n\tFri, 10 Dec 2021 22:14:24 +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 15619F84;\n\tFri, 10 Dec 2021 22:14:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dyLIaWl1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639170864;\n\tbh=KkbwoL/WEcb0agwlxz2+wS0HqZlEZxLIlluhAzdYYyA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dyLIaWl1lXwGBrifaNCQBl/x3q+CQElM6UK4XXyPtWFciqjoLAXL1Lnq910gJjouj\n\trVDGpF00UZEzVBvDXkUYTkHKLUJG/xwLAdIY2v7hpdQbxF/7GQhvH4Dr68mGR/S6hY\n\tODoXVt9MJ4zNFTavejK3WzV4jhXorPsK/jD+vN8k=","Date":"Fri, 10 Dec 2021 23:13:54 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211210205239.354901-1-jacopo@jmondi.org>\n\t<20211210205239.354901-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211210205239.354901-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v5 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\" "}}]