[{"id":21074,"web_url":"https://patchwork.libcamera.org/comment/21074/","msgid":"","date":"2021-11-21T18:16:19","subject":"Re: [libcamera-devel] [PATCH v2 04/12] 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 Sat, Nov 20, 2021 at 12:13:05PM +0100, Jacopo Mondi wrote:\n> Add an overloaded version of Request::addBuffer() that allows\n> application to specify a Fence instance to be associated with the\n> Framebuffer.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/request.h | 5 ++++\n> src/libcamera/request.cpp | 58 ++++++++++++++++++++++++++++---------\n> 2 files changed, 50 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index f0c5163d987e..37aebc41834e 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -22,6 +22,7 @@ namespace libcamera {\n> \n> class Camera;\n> class CameraControlValidator;\n> +class Fence;\n> class FrameBuffer;\n> class Stream;\n> \n> @@ -52,6 +53,8 @@ public:\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);\n> \tFrameBuffer *findBuffer(const Stream *stream) const;\n> \n> \tuint32_t sequence() const;\n> @@ -65,6 +68,8 @@ public:\n> private:\n> \tLIBCAMERA_DISABLE_COPY(Request)\n> \n> +\tint _addBuffer(const Stream *stream, FrameBuffer *buffer);\n> +\n> \tControlList *controls_;\n> \tControlList *metadata_;\n> \tBufferMap bufferMap_;\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 90c436648405..1d47698a6263 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -19,6 +19,7 @@\n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/fence.h\"\n> #include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/tracepoints.h\"\n> \n> @@ -316,6 +317,26 @@ void Request::reuse(ReuseFlag flags)\n> * \\return The map of Stream to FrameBuffer\n> */\n> \n> +int Request::_addBuffer(const Stream *stream, FrameBuffer *buffer)\n> +{\n> +\tif (!stream) {\n> +\t\tLOG(Request, Error) << \"Invalid stream reference\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tauto it = bufferMap_.find(stream);\n> +\tif (it != bufferMap_.end()) {\n> +\t\tLOG(Request, Error) << \"FrameBuffer already set for stream\";\n> +\t\treturn -EEXIST;\n> +\t}\n> +\n> +\tbuffer->_d()->setRequest(this);\n> +\t_d()->pending_.insert(buffer);\n> +\tbufferMap_[stream] = buffer;\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Add a FrameBuffer with its associated Stream to the Request\n> * \\param[in] stream The stream the buffer belongs to\n> @@ -334,22 +355,33 @@ void Request::reuse(ReuseFlag flags)\n> */\n> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> {\n> -\tif (!stream) {\n> -\t\tLOG(Request, Error) << \"Invalid stream reference\";\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tint ret = _addBuffer(stream, buffer);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Drop the buffer Fence, if any.\n> +\t *\n> +\t * Buffers can be re-used and might still have a Fence associated from\n> +\t * a previous run if the Fence has failed. Drop it here.\n> +\t */\n> +\tbuffer->_d()->closeFence();\n\nI think it would be better to ASSERT(!buffer->fence()) instead.\nApplications must handle fence timeouts by extracting the fence from the\nFrameBuffer at request completion time. If we silently ignore the\nfailures to do so, it will cause hard to debug problem.\n\nWe could alternatively print an Error message and return an error, to\navoid crashes. That would probably be better, as wait timeouts should be\nrare, so they may not be caught early during development.\n\nWith such a change, we could probably merge the two addBuffer()\nfunctions in one, with a default value for the fence argument.\n\n> \n> -\tauto it = bufferMap_.find(stream);\n> -\tif (it != bufferMap_.end()) {\n> -\t\tLOG(Request, Error) << \"FrameBuffer already set for stream\";\n> -\t\treturn -EEXIST;\n> -\t}\n> +\treturn 0;\n> +}\n> \n> -\tbuffer->_d()->setRequest(this);\n> -\t_d()->pending_.insert(buffer);\n> -\tbufferMap_[stream] = buffer;\n> +/**\n> + * \\brief Add a FrameBuffer with its associated Stream and Fence\n> + * \\copydoc Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> + * \\param[in] fence The synchronization fence associated with \\a buffer\n> + */\n> +int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n> +\t\t std::unique_ptr &&fence)\n> +{\n> +\tif (fence->isValid())\n> +\t\tbuffer->_d()->setFence(std::move(fence));\n> \n> -\treturn 0;\n> +\treturn _addBuffer(stream, buffer);\n> }\n> \n> /**\n> -- \n> 2.33.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 EA949BDB13\n\tfor ;\n\tSun, 21 Nov 2021 18:16:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C5BE6033C;\n\tSun, 21 Nov 2021 19:16:44 +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 75AB960233\n\tfor ;\n\tSun, 21 Nov 2021 19:16:42 +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 E03889DD;\n\tSun, 21 Nov 2021 19:16:41 +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=\"JEbKgSGd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637518602;\n\tbh=nr3uX08czA//IztmIkkFOqDYTfGkcgLPlzNVXK3JTyM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JEbKgSGdSfn2iaOI6GPFEYSmL62NGwdgCZG3T2FWSe/ITI594PriZVX2plbAhX6lx\n\tWTi2/Fl6AaHVPy49BQJg04Mk+5UnCedAEPURxf3ODwGNzFm2s0s8L2hCRgoeapVppL\n\tH9kZZq5y2uktTCcDNowQjeOBZnn6PQxX7TNxjxOs=","Date":"Sun, 21 Nov 2021 20:16:19 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211120111313.106621-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 04/12] 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\" "}}]