[{"id":20757,"web_url":"https://patchwork.libcamera.org/comment/20757/","msgid":"<163646599181.1606134.2579414771855623957@Monstersaurus>","date":"2021-11-09T13:53:11","subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-10-28 12:15:16)\n> Add an optional synchronization file descriptor to the FrameBuffer constuctor.\n> \n> The fence is handled by the FrameBuffer::Private class by constructing\n> a Fence class instance to wrap the file descriptor. Once the Request the\n> FrameBuffer is part of has completed, the fence file descriptor will read\n> as -1 if successfully handled by the library; otherwise the file\n> descriptor value is kept and applications are responsible for closing\n> it.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/framebuffer.h | 5 ++-\n> include/libcamera/internal/framebuffer.h | 7 +++-\n> src/libcamera/framebuffer.cpp | 46 +++++++++++++++++++++---\n> 3 files changed, 52 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 7f2f176af691..ac96790b76c0 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -57,7 +57,8 @@ public:\n> unsigned int length;\n> };\n> \n> - FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> + FrameBuffer(const std::vector &planes, unsigned int cookie = 0,\n> + int fence = -1);\n\nHrm, -1 seems a bit odd here, Perhaps we should be passing in a\nFileDescriptor.\n\n\ndoes this impact on the recent work to have an external\nFrameBufferAllocator at all?\n\n> \n> const std::vector &planes() const { return planes_; }\n> Request *request() const;\n> @@ -66,6 +67,8 @@ public:\n> unsigned int cookie() const { return cookie_; }\n> void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> \n> + int fence() const;\n> +\n> void 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 cd33c295466e..db1a4bd72354 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -11,6 +11,8 @@\n> \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> class FrameBuffer::Private : public Extensible::Private\n> @@ -18,14 +20,17 @@ class FrameBuffer::Private : public Extensible::Private\n> LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> \n> public:\n> - Private();\n> + Private(int fence);\n> \n> void setRequest(Request *request) { request_ = request; }\n> bool isContiguous() const { return isContiguous_; }\n> + const Fence &fence() const { return fence_; }\n> + Fence &fence() { return fence_; }\n> \n> private:\n> Request *request_;\n> bool isContiguous_;\n> + Fence fence_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index d44a98babd05..c3e03896b184 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -111,8 +111,12 @@ LOG_DEFINE_CATEGORY(Buffer)\n> * pipeline handlers.\n> */\n> \n> -FrameBuffer::Private::Private()\n> - : request_(nullptr), isContiguous_(true)\n> +/**\n> + * \\brief Construct a FrameBuffer::Private instance\n> + * \\param[in] fence The synchronization fence file descriptor\n> + */\n> +FrameBuffer::Private::Private(int fence)\n> + : request_(nullptr), isContiguous_(true), fence_(fence)\n> {\n> }\n> \n> @@ -137,6 +141,18 @@ FrameBuffer::Private::Private()\n> * \\return True if the planes are stored contiguously in memory, false otherwise\n> */\n> \n> +/**\n> + * \\fn const FrameBuffer::Private::fence() const\n> + * \\brief Return a const reference to the Fence\n> + * \\return A const reference to the frame buffer fence\n> + */\n> +\n> +/**\n> + * \\fn FrameBuffer::Private::fence()\n> + * \\brief Return a reference to the Fence\n> + * \\return A reference to the frame buffer fence\n> + */\n> +\n> /**\n> * \\class FrameBuffer\n> * \\brief Frame buffer data and its associated dynamic metadata\n> @@ -211,9 +227,11 @@ FrameBuffer::Private::Private()\n> * \\brief Construct a FrameBuffer with an array of planes\n> * \\param[in] planes The frame memory planes\n> * \\param[in] cookie Cookie\n> + * \\param[in] fence Synchronization fence\n> */\n> -FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> - : Extensible(std::make_unique()), planes_(planes),\n> +FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie,\n> + int fence)\n> + : Extensible(std::make_unique(fence)), planes_(planes),\n> cookie_(cookie)\n> {\n> metadata_.planes_.resize(planes_.size());\n> @@ -305,6 +323,26 @@ Request *FrameBuffer::request() const\n> * libcamera core never modifies the buffer cookie.\n> */\n> \n> +/**\n> + * \\fn FrameBuffer::fence()\n> + * \\brief Return the synchronization fence file descriptor\n> + *\n> + * The fence file descriptor is set by applications at construction time.\n> + *\n> + * Once the Request is queued to the Camera the fence is handled by the\n> + * library and if successfully notified the fence will read as -1 after the\n> + * Request has completed.\n\nAre fences always kept with the FrameBuffer allocations? Or is it a new\nfence for everytime the FrameBuffer is queued?\n\n\n> + *\n> + * If waiting for fence fails, the fence is instead kept in the FrameBuffer\n> + * after the Request has completed and it is responsibility of the\n> + * application to correctly close it.\n\nBy simply closing the fd? Would this be automatic if it was constructed\ninto a FileDescriptor?\n\nI don't think I've understood the underlying lifetime of the fence\nmechanisms enough yet ...\n\nI'll keep going and see if I get enlightenment from the following\npatches.\n\n> + */\n> +int FrameBuffer::fence() const\n> +{\n> + const Fence &fence = _d()->fence();\n> + return fence.fd();\n> +}\n> +\n> /**\n> * \\fn FrameBuffer::cancel()\n> * \\brief Marks the buffer as cancelled\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 4289BBDB1C\n\tfor ;\n\tTue, 9 Nov 2021 13:53:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 536066035D;\n\tTue, 9 Nov 2021 14:53:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C548600BF\n\tfor ;\n\tTue, 9 Nov 2021 14:53:14 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC901556;\n\tTue, 9 Nov 2021 14:53:13 +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=\"hewJJ3Qd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636465994;\n\tbh=j1SUek+w1xP2ioyHDbqEg90xRD24XjqZlZmJESiEFeg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=hewJJ3QdcP2r7ErsolLym7pUKprX/zz7mDjALddXtwr36RX3bfjYM0+P4xttsPZ76\n\tYGtgHgm0jfW2KPqtKkckhz0itRTyHaEy3xQXIF0wu91V4HZ6kHJNnCD2ZYL6FcyL8R\n\tbJ0Qk2oyRfipEZADSmPa1NCsRnus4wDBY3ul9RAY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-7-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-7-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 13:53:11 +0000","Message-ID":"<163646599181.1606134.2579414771855623957@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20768,"web_url":"https://patchwork.libcamera.org/comment/20768/","msgid":"<20211109173637.whne3us5hqr2nijk@uno.localdomain>","date":"2021-11-09T17:36:37","subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Tue, Nov 09, 2021 at 01:53:11PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-10-28 12:15:16)\n> > Add an optional synchronization file descriptor to the FrameBuffer constuctor.\n> >\n> > The fence is handled by the FrameBuffer::Private class by constructing\n> > a Fence class instance to wrap the file descriptor. Once the Request the\n> > FrameBuffer is part of has completed, the fence file descriptor will read\n> > as -1 if successfully handled by the library; otherwise the file\n> > descriptor value is kept and applications are responsible for closing\n> > it.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/framebuffer.h | 5 ++-\n> > include/libcamera/internal/framebuffer.h | 7 +++-\n> > src/libcamera/framebuffer.cpp | 46 +++++++++++++++++++++---\n> > 3 files changed, 52 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index 7f2f176af691..ac96790b76c0 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -57,7 +57,8 @@ public:\n> > unsigned int length;\n> > };\n> >\n> > - FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > + FrameBuffer(const std::vector &planes, unsigned int cookie = 0,\n> > + int fence = -1);\n>\n> Hrm, -1 seems a bit odd here, Perhaps we should be passing in a\n> FileDescriptor.\n>\n\nThat would also make clear that apps are not meant to touch the Fence\nonce it has been added to the FrameBuffer\n\n>\n> does this impact on the recent work to have an external\n> FrameBufferAllocator at all?\n>\n\nNot sure. I guess its interface should be expanded to accept an\noptional fence. Being it Android-only, the FrameBufferAllocator can\naccept a raw int and wrap it in a FileDescriptor before creating the\nFrameBuffer.\n\nThanks\n j\n\n> >\n> > const std::vector &planes() const { return planes_; }\n> > Request *request() const;\n> > @@ -66,6 +67,8 @@ public:\n> > unsigned int cookie() const { return cookie_; }\n> > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> >\n> > + int fence() const;\n> > +\n> > void 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 cd33c295466e..db1a4bd72354 100644\n> > --- a/include/libcamera/internal/framebuffer.h\n> > +++ b/include/libcamera/internal/framebuffer.h\n> > @@ -11,6 +11,8 @@\n> >\n> > #include \n> >\n> > +#include \n> > +\n> > namespace libcamera {\n> >\n> > class FrameBuffer::Private : public Extensible::Private\n> > @@ -18,14 +20,17 @@ class FrameBuffer::Private : public Extensible::Private\n> > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> >\n> > public:\n> > - Private();\n> > + Private(int fence);\n> >\n> > void setRequest(Request *request) { request_ = request; }\n> > bool isContiguous() const { return isContiguous_; }\n> > + const Fence &fence() const { return fence_; }\n> > + Fence &fence() { return fence_; }\n> >\n> > private:\n> > Request *request_;\n> > bool isContiguous_;\n> > + Fence fence_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index d44a98babd05..c3e03896b184 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -111,8 +111,12 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > * pipeline handlers.\n> > */\n> >\n> > -FrameBuffer::Private::Private()\n> > - : request_(nullptr), isContiguous_(true)\n> > +/**\n> > + * \\brief Construct a FrameBuffer::Private instance\n> > + * \\param[in] fence The synchronization fence file descriptor\n> > + */\n> > +FrameBuffer::Private::Private(int fence)\n> > + : request_(nullptr), isContiguous_(true), fence_(fence)\n> > {\n> > }\n> >\n> > @@ -137,6 +141,18 @@ FrameBuffer::Private::Private()\n> > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > */\n> >\n> > +/**\n> > + * \\fn const FrameBuffer::Private::fence() const\n> > + * \\brief Return a const reference to the Fence\n> > + * \\return A const reference to the frame buffer fence\n> > + */\n> > +\n> > +/**\n> > + * \\fn FrameBuffer::Private::fence()\n> > + * \\brief Return a reference to the Fence\n> > + * \\return A reference to the frame buffer fence\n> > + */\n> > +\n> > /**\n> > * \\class FrameBuffer\n> > * \\brief Frame buffer data and its associated dynamic metadata\n> > @@ -211,9 +227,11 @@ FrameBuffer::Private::Private()\n> > * \\brief Construct a FrameBuffer with an array of planes\n> > * \\param[in] planes The frame memory planes\n> > * \\param[in] cookie Cookie\n> > + * \\param[in] fence Synchronization fence\n> > */\n> > -FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > - : Extensible(std::make_unique()), planes_(planes),\n> > +FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie,\n> > + int fence)\n> > + : Extensible(std::make_unique(fence)), planes_(planes),\n> > cookie_(cookie)\n> > {\n> > metadata_.planes_.resize(planes_.size());\n> > @@ -305,6 +323,26 @@ Request *FrameBuffer::request() const\n> > * libcamera core never modifies the buffer cookie.\n> > */\n> >\n> > +/**\n> > + * \\fn FrameBuffer::fence()\n> > + * \\brief Return the synchronization fence file descriptor\n> > + *\n> > + * The fence file descriptor is set by applications at construction time.\n> > + *\n> > + * Once the Request is queued to the Camera the fence is handled by the\n> > + * library and if successfully notified the fence will read as -1 after the\n> > + * Request has completed.\n>\n> Are fences always kept with the FrameBuffer allocations? Or is it a new\n> fence for everytime the FrameBuffer is queued?\n>\n>\n> > + *\n> > + * If waiting for fence fails, the fence is instead kept in the FrameBuffer\n> > + * after the Request has completed and it is responsibility of the\n> > + * application to correctly close it.\n>\n> By simply closing the fd? Would this be automatic if it was constructed\n> into a FileDescriptor?\n>\n> I don't think I've understood the underlying lifetime of the fence\n> mechanisms enough yet ...\n>\n> I'll keep going and see if I get enlightenment from the following\n> patches.\n>\n> > + */\n> > +int FrameBuffer::fence() const\n> > +{\n> > + const Fence &fence = _d()->fence();\n> > + return fence.fd();\n> > +}\n> > +\n> > /**\n> > * \\fn FrameBuffer::cancel()\n> > * \\brief Marks the buffer as cancelled\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 92811C3252\n\tfor ;\n\tTue, 9 Nov 2021 17:35:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7D856034E;\n\tTue, 9 Nov 2021 18:35:45 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F610600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:35:44 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 1884960002;\n\tTue, 9 Nov 2021 17:35:43 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 18:36:37 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211109173637.whne3us5hqr2nijk@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-7-jacopo@jmondi.org>\n\t<163646599181.1606134.2579414771855623957@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163646599181.1606134.2579414771855623957@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","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\" "}},{"id":20918,"web_url":"https://patchwork.libcamera.org/comment/20918/","msgid":"","date":"2021-11-12T15:38:07","subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","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 Tue, Nov 09, 2021 at 06:36:37PM +0100, Jacopo Mondi wrote:\n> On Tue, Nov 09, 2021 at 01:53:11PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-10-28 12:15:16)\n> > > Add an optional synchronization file descriptor to the FrameBuffer constuctor.\n\ns/constuctor/constructor/\n\n> > >\n> > > The fence is handled by the FrameBuffer::Private class by constructing\n> > > a Fence class instance to wrap the file descriptor. Once the Request the\n> > > FrameBuffer is part of has completed, the fence file descriptor will read\n> > > as -1 if successfully handled by the library; otherwise the file\n> > > descriptor value is kept and applications are responsible for closing\n> > > it.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/framebuffer.h | 5 ++-\n> > > include/libcamera/internal/framebuffer.h | 7 +++-\n> > > src/libcamera/framebuffer.cpp | 46 +++++++++++++++++++++---\n> > > 3 files changed, 52 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index 7f2f176af691..ac96790b76c0 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -57,7 +57,8 @@ public:\n> > > unsigned int length;\n> > > };\n> > >\n> > > - FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > > + FrameBuffer(const std::vector &planes, unsigned int cookie = 0,\n> > > + int fence = -1);\n> >\n> > Hrm, -1 seems a bit odd here, Perhaps we should be passing in a\n> > FileDescriptor.\n> \n> That would also make clear that apps are not meant to touch the Fence\n> once it has been added to the FrameBuffer\n\nOr even passing a Fence ? We'd need a default Fence constructor for an\ninvalid fence.\n\nI think the fence should be associated with the frame buffer when adding\nit to a request. In the use case where FrameBuffer instances are created\nat init time and reused, we'll have a new fence every time a frame\nbuffer is queued in a request.\n\nWe need to figure out what to do with fences that fail time out, we need\na way to extract them from the frame buffer when the request completes\n(in order to pass them back to the Android camera service), without\nclosing the fd at that point, but we also need to make sure that the fd\nwill get closed automatically if the application doesn't handle the\nfence at completion time to avoid leaks.\n\n> > does this impact on the recent work to have an external\n> > FrameBufferAllocator at all?\n> \n> Not sure. I guess its interface should be expanded to accept an\n> optional fence. Being it Android-only, the FrameBufferAllocator can\n> accept a raw int and wrap it in a FileDescriptor before creating the\n> FrameBuffer.\n> \n> > >\n> > > const std::vector &planes() const { return planes_; }\n> > > Request *request() const;\n> > > @@ -66,6 +67,8 @@ public:\n> > > unsigned int cookie() const { return cookie_; }\n> > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > >\n> > > + int fence() const;\n> > > +\n> > > void 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 cd33c295466e..db1a4bd72354 100644\n> > > --- a/include/libcamera/internal/framebuffer.h\n> > > +++ b/include/libcamera/internal/framebuffer.h\n> > > @@ -11,6 +11,8 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > namespace libcamera {\n> > >\n> > > class FrameBuffer::Private : public Extensible::Private\n> > > @@ -18,14 +20,17 @@ class FrameBuffer::Private : public Extensible::Private\n> > > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > >\n> > > public:\n> > > - Private();\n> > > + Private(int fence);\n> > >\n> > > void setRequest(Request *request) { request_ = request; }\n> > > bool isContiguous() const { return isContiguous_; }\n> > > + const Fence &fence() const { return fence_; }\n> > > + Fence &fence() { return fence_; }\n> > >\n> > > private:\n> > > Request *request_;\n> > > bool isContiguous_;\n> > > + Fence fence_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index d44a98babd05..c3e03896b184 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -111,8 +111,12 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > > * pipeline handlers.\n> > > */\n> > >\n> > > -FrameBuffer::Private::Private()\n> > > - : request_(nullptr), isContiguous_(true)\n> > > +/**\n> > > + * \\brief Construct a FrameBuffer::Private instance\n> > > + * \\param[in] fence The synchronization fence file descriptor\n> > > + */\n> > > +FrameBuffer::Private::Private(int fence)\n> > > + : request_(nullptr), isContiguous_(true), fence_(fence)\n> > > {\n> > > }\n> > >\n> > > @@ -137,6 +141,18 @@ FrameBuffer::Private::Private()\n> > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn const FrameBuffer::Private::fence() const\n> > > + * \\brief Return a const reference to the Fence\n> > > + * \\return A const reference to the frame buffer fence\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FrameBuffer::Private::fence()\n> > > + * \\brief Return a reference to the Fence\n> > > + * \\return A reference to the frame buffer fence\n> > > + */\n> > > +\n> > > /**\n> > > * \\class FrameBuffer\n> > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > @@ -211,9 +227,11 @@ FrameBuffer::Private::Private()\n> > > * \\brief Construct a FrameBuffer with an array of planes\n> > > * \\param[in] planes The frame memory planes\n> > > * \\param[in] cookie Cookie\n> > > + * \\param[in] fence Synchronization fence\n> > > */\n> > > -FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > - : Extensible(std::make_unique()), planes_(planes),\n> > > +FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie,\n> > > + int fence)\n> > > + : Extensible(std::make_unique(fence)), planes_(planes),\n> > > cookie_(cookie)\n> > > {\n> > > metadata_.planes_.resize(planes_.size());\n> > > @@ -305,6 +323,26 @@ Request *FrameBuffer::request() const\n> > > * libcamera core never modifies the buffer cookie.\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn FrameBuffer::fence()\n> > > + * \\brief Return the synchronization fence file descriptor\n> > > + *\n> > > + * The fence file descriptor is set by applications at construction time.\n> > > + *\n> > > + * Once the Request is queued to the Camera the fence is handled by the\n> > > + * library and if successfully notified the fence will read as -1 after the\n> > > + * Request has completed.\n> >\n> > Are fences always kept with the FrameBuffer allocations? Or is it a new\n> > fence for everytime the FrameBuffer is queued?\n\nSee above ;-)\n\n> > > + *\n> > > + * If waiting for fence fails, the fence is instead kept in the FrameBuffer\n> > > + * after the Request has completed and it is responsibility of the\n> > > + * application to correctly close it.\n> >\n> > By simply closing the fd? Would this be automatic if it was constructed\n> > into a FileDescriptor?\n> >\n> > I don't think I've understood the underlying lifetime of the fence\n> > mechanisms enough yet ...\n\nA bit more documentation would indeed be useful.\n\n> > I'll keep going and see if I get enlightenment from the following\n> > patches.\n> >\n> > > + */\n> > > +int FrameBuffer::fence() const\n> > > +{\n> > > + const Fence &fence = _d()->fence();\n> > > + return fence.fd();\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 E2E90BDB1C\n\tfor ;\n\tFri, 12 Nov 2021 15:38:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27E936036B;\n\tFri, 12 Nov 2021 16:38:31 +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 EF2A56032C\n\tfor ;\n\tFri, 12 Nov 2021 16:38:29 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89948D6E;\n\tFri, 12 Nov 2021 16:38:29 +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=\"EymLUg7x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636731509;\n\tbh=bxqyJYg2OrPlrLpmDoA70J9hWUDuwUtuVE1LPKWh8cg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EymLUg7xPq8fOAwy4fAfxeh5T0ClZD1BoRI1Q53/m2zJ9lUZ0/buCV9lRnKBsGZyk\n\tJptas8vvbASvmKRYaxJrtj/7KK2vtZkptcHDVjQAuUOOqZvP1pBlo2h6aRHsnfm3bo\n\t4+g0+q/sX89agQCkRvMNPNbqzv56tcABPRlrMSrg=","Date":"Fri, 12 Nov 2021 17:38:07 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-7-jacopo@jmondi.org>\n\t<163646599181.1606134.2579414771855623957@Monstersaurus>\n\t<20211109173637.whne3us5hqr2nijk@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211109173637.whne3us5hqr2nijk@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","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\" "}},{"id":20959,"web_url":"https://patchwork.libcamera.org/comment/20959/","msgid":"<20211115203225.5xf5kxxcqbhi64xx@uno.localdomain>","date":"2021-11-15T20:32:25","subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Fri, Nov 12, 2021 at 05:38:07PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Nov 09, 2021 at 06:36:37PM +0100, Jacopo Mondi wrote:\n> > On Tue, Nov 09, 2021 at 01:53:11PM +0000, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi (2021-10-28 12:15:16)\n> > > > Add an optional synchronization file descriptor to the FrameBuffer constuctor.\n>\n> s/constuctor/constructor/\n>\n> > > >\n> > > > The fence is handled by the FrameBuffer::Private class by constructing\n> > > > a Fence class instance to wrap the file descriptor. Once the Request the\n> > > > FrameBuffer is part of has completed, the fence file descriptor will read\n> > > > as -1 if successfully handled by the library; otherwise the file\n> > > > descriptor value is kept and applications are responsible for closing\n> > > > it.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/framebuffer.h | 5 ++-\n> > > > include/libcamera/internal/framebuffer.h | 7 +++-\n> > > > src/libcamera/framebuffer.cpp | 46 +++++++++++++++++++++---\n> > > > 3 files changed, 52 insertions(+), 6 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > index 7f2f176af691..ac96790b76c0 100644\n> > > > --- a/include/libcamera/framebuffer.h\n> > > > +++ b/include/libcamera/framebuffer.h\n> > > > @@ -57,7 +57,8 @@ public:\n> > > > unsigned int length;\n> > > > };\n> > > >\n> > > > - FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > > > + FrameBuffer(const std::vector &planes, unsigned int cookie = 0,\n> > > > + int fence = -1);\n> > >\n> > > Hrm, -1 seems a bit odd here, Perhaps we should be passing in a\n> > > FileDescriptor.\n> >\n> > That would also make clear that apps are not meant to touch the Fence\n> > once it has been added to the FrameBuffer\n>\n> Or even passing a Fence ? We'd need a default Fence constructor for an\n> invalid fence.\n>\n> I think the fence should be associated with the frame buffer when adding\n> it to a request. In the use case where FrameBuffer instances are created\n> at init time and reused, we'll have a new fence every time a frame\n> buffer is queued in a request.\n>\n> We need to figure out what to do with fences that fail time out, we need\n> a way to extract them from the frame buffer when the request completes\n> (in order to pass them back to the Android camera service), without\n> closing the fd at that point, but we also need to make sure that the fd\n> will get closed automatically if the application doesn't handle the\n> fence at completion time to avoid leaks.\n\nYes, and that's why I insisted on Fence having a move only semantic.\n\nThe key idea (which could be achieved in different ways than moving\nfences, by storing them as unique ptrs in example as you suggested)\nwas indeed to guarantee that:\n\n- Fences (and their underlying file descriptor) are only owned by a\n single component\n\n - Framebuffer creation -> frame buffer internal\n - Request::addBuffer() -> frame buffer internal\n - Camera::queueRequest() -> request internal, read as -1 from frame buffer\n - Request::complete() - either deleted if fence has been signaled or\n moved back to the frame buffer if timedout\n\nAll of this can be read as: if a completed buffer has a fence, then it\nis reponsibility of the app to handle it. Otherwise it is guaranteed\nthe core closed it.\n\nI thought it was a clear interaction pattern for apps, and I had\nintegers as representation towards apps as it read more natural \"if\n(buffer.fence != -1)\" but indeed it will require to rethink the\ninterface completely when we'll support other types of fences.\n\nI'll see if a similar semantic can be implemented by using\nunique_ptr<> only, but it felt so natural to me to use move semantic\nto -move back- a fence to a Framebuffer if it had timedout to give it\nback to applications. Probably we can keep the fence owned by the\nframebuffer for the whole Request lifecycle, and simply .release() it\nif it gets signalled before timeout.\n\n>\n> > > does this impact on the recent work to have an external\n> > > FrameBufferAllocator at all?\n> >\n> > Not sure. I guess its interface should be expanded to accept an\n> > optional fence. Being it Android-only, the FrameBufferAllocator can\n> > accept a raw int and wrap it in a FileDescriptor before creating the\n> > FrameBuffer.\n> >\n> > > >\n> > > > const std::vector &planes() const { return planes_; }\n> > > > Request *request() const;\n> > > > @@ -66,6 +67,8 @@ public:\n> > > > unsigned int cookie() const { return cookie_; }\n> > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > >\n> > > > + int fence() const;\n> > > > +\n> > > > void 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 cd33c295466e..db1a4bd72354 100644\n> > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > @@ -11,6 +11,8 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > +\n> > > > namespace libcamera {\n> > > >\n> > > > class FrameBuffer::Private : public Extensible::Private\n> > > > @@ -18,14 +20,17 @@ class FrameBuffer::Private : public Extensible::Private\n> > > > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > > >\n> > > > public:\n> > > > - Private();\n> > > > + Private(int fence);\n> > > >\n> > > > void setRequest(Request *request) { request_ = request; }\n> > > > bool isContiguous() const { return isContiguous_; }\n> > > > + const Fence &fence() const { return fence_; }\n> > > > + Fence &fence() { return fence_; }\n> > > >\n> > > > private:\n> > > > Request *request_;\n> > > > bool isContiguous_;\n> > > > + Fence fence_;\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > index d44a98babd05..c3e03896b184 100644\n> > > > --- a/src/libcamera/framebuffer.cpp\n> > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > @@ -111,8 +111,12 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > > > * pipeline handlers.\n> > > > */\n> > > >\n> > > > -FrameBuffer::Private::Private()\n> > > > - : request_(nullptr), isContiguous_(true)\n> > > > +/**\n> > > > + * \\brief Construct a FrameBuffer::Private instance\n> > > > + * \\param[in] fence The synchronization fence file descriptor\n> > > > + */\n> > > > +FrameBuffer::Private::Private(int fence)\n> > > > + : request_(nullptr), isContiguous_(true), fence_(fence)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -137,6 +141,18 @@ FrameBuffer::Private::Private()\n> > > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn const FrameBuffer::Private::fence() const\n> > > > + * \\brief Return a const reference to the Fence\n> > > > + * \\return A const reference to the frame buffer fence\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn FrameBuffer::Private::fence()\n> > > > + * \\brief Return a reference to the Fence\n> > > > + * \\return A reference to the frame buffer fence\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\class FrameBuffer\n> > > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > > @@ -211,9 +227,11 @@ FrameBuffer::Private::Private()\n> > > > * \\brief Construct a FrameBuffer with an array of planes\n> > > > * \\param[in] planes The frame memory planes\n> > > > * \\param[in] cookie Cookie\n> > > > + * \\param[in] fence Synchronization fence\n> > > > */\n> > > > -FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > > - : Extensible(std::make_unique()), planes_(planes),\n> > > > +FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie,\n> > > > + int fence)\n> > > > + : Extensible(std::make_unique(fence)), planes_(planes),\n> > > > cookie_(cookie)\n> > > > {\n> > > > metadata_.planes_.resize(planes_.size());\n> > > > @@ -305,6 +323,26 @@ Request *FrameBuffer::request() const\n> > > > * libcamera core never modifies the buffer cookie.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn FrameBuffer::fence()\n> > > > + * \\brief Return the synchronization fence file descriptor\n> > > > + *\n> > > > + * The fence file descriptor is set by applications at construction time.\n> > > > + *\n> > > > + * Once the Request is queued to the Camera the fence is handled by the\n> > > > + * library and if successfully notified the fence will read as -1 after the\n> > > > + * Request has completed.\n> > >\n> > > Are fences always kept with the FrameBuffer allocations? Or is it a new\n> > > fence for everytime the FrameBuffer is queued?\n>\n> See above ;-)\n>\n> > > > + *\n> > > > + * If waiting for fence fails, the fence is instead kept in the FrameBuffer\n> > > > + * after the Request has completed and it is responsibility of the\n> > > > + * application to correctly close it.\n> > >\n> > > By simply closing the fd? Would this be automatic if it was constructed\n> > > into a FileDescriptor?\n> > >\n> > > I don't think I've understood the underlying lifetime of the fence\n> > > mechanisms enough yet ...\n>\n> A bit more documentation would indeed be useful.\n>\n> > > I'll keep going and see if I get enlightenment from the following\n> > > patches.\n> > >\n> > > > + */\n> > > > +int FrameBuffer::fence() const\n> > > > +{\n> > > > + const Fence &fence = _d()->fence();\n> > > > + return fence.fd();\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\fn FrameBuffer::cancel()\n> > > > * \\brief Marks the buffer as cancelled\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 DFF47BF415\n\tfor ;\n\tMon, 15 Nov 2021 20:31:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11D3860378;\n\tMon, 15 Nov 2021 21:31:32 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B308C60233\n\tfor ;\n\tMon, 15 Nov 2021 21:31:30 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 1DEF640004;\n\tMon, 15 Nov 2021 20:31:29 +0000 (UTC)"],"Date":"Mon, 15 Nov 2021 21:32:25 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211115203225.5xf5kxxcqbhi64xx@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-7-jacopo@jmondi.org>\n\t<163646599181.1606134.2579414771855623957@Monstersaurus>\n\t<20211109173637.whne3us5hqr2nijk@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add\n\tsynchronization Fence","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\" "}}]