[{"id":20754,"web_url":"https://patchwork.libcamera.org/comment/20754/","msgid":"<163646421293.1606134.1729310028789817202@Monstersaurus>","date":"2021-11-09T13:23:32","subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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:15)\n> Prepare the Request::Private class to handle fences by providing a\n> storage vector and interface functions to handle the number of pending\n> and expired fences.\n> \n> The intended user of the interface is the PipelineHandler class\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/request.h | 21 +++++++\n> src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++-\n> 2 files changed, 109 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index df0cc014067e..2be4874756de 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -7,8 +7,12 @@\n> #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n> #define __LIBCAMERA_INTERNAL_REQUEST_H__\n> \n> +#include \n> +\n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> class Camera;\n> @@ -24,9 +28,26 @@ public:\n> \n> Camera *camera() const { return camera_; }\n> \n> + unsigned int pendingFences() const { return pendingFences_; }\n> + unsigned int expiredFences() const { return expiredFences_; }\n> +\n> + void reuse();\n> +\n> + std::vector &fences() { return fences_; }\n> + void addFence(Fence &&fence);\n> + void clearFences();\n> +\n> + void fenceExpired();\n> + void fenceCompleted();\n> +\n> private:\n> Camera *camera_;\n> bool cancelled_;\n> +\n> + unsigned int pendingFences_ = 0;\n> + unsigned int expiredFences_ = 0;\n> +\n> + std::vector fences_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 33fee1ac05ba..e88eee1fac36 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -63,6 +63,92 @@ Request::Private::~Private()\n> * request hasn't been queued\n> */\n> \n> +/**\n> + * \\fn Request::Private::pendingFences()\n> + * \\brief Retrieve the number of pending fences\n> + *\n> + * A Fence is pending if has not yet been signalled or it has not expired yet.\n> + *\n> + * \\return The number of pending fences\n> + */\n> +\n> +/**\n> + * \\fn Request::Private::expiredFences()\n> + * \\brief Retrieve the number of expired fences\n> + * \\return The number of expired fences\n> + */\n> +\n> +/**\n> + * \\brief Reset the request for reuse\n> + */\n> +void Request::Private::reuse()\n> +{\n> + cancelled_ = false;\n> +\n> + fences_.clear();\n> + pendingFences_ = 0;\n> + expiredFences_ = 0;\n> +}\n> +\n> +/**\n> + * \\fn Request::Private::fences()\n> + * \\brief Retrieve a reference to the vector of pending fences\n> + * \\return A reference to the vector of pending fences\n> + */\n> +\n> +/**\n> + * \\brief Move a Fence into the Request\n> + *\n> + * Move a Fence into the Request and increase the pending fences\n> + * counter. The Fence is now stored into the Request and the original\n> + * one passed in as parameter is reset.\n> + *\n> + * Once the Fence completes, either because it is signalled or because\n> + * it has expired, the caller shall notify the Request about this by\n> + * calling fenceCompleted() or fenceExpired();\n> + */\n> +void Request::Private::addFence(Fence &&fence)\n> +{\n> + fences_.push_back(std::move(fence));\n> + pendingFences_++;\n\nHrm ... I was expecting signals to be connected here ... but I guess\nthat happens elsewhere...\n\n\n> +}\n> +\n> +/**\n> + * \\brief Release all Fences stored in the request\n> + *\n> + * Clear the vector of fences in the Request by releasing all of them.\n> + * All Fences are closed and their file descriptors reset to 0.\n> + */\n> +void Request::Private::clearFences()\n> +{\n> + ASSERT(!pendingFences_);\n> + fences_.clear();\n> +}\n> +\n> +/**\n> + * \\brief Notify that a Fence has been signalled\n> + *\n> + * Notify to the Request that a Fence has completed. This function decrease the\n> + * number of pending Fences in the request.\n> + */\n> +void Request::Private::fenceCompleted()\n> +{\n> + pendingFences_--;\n> +}\n> +\n> +/**\n> + * \\brief Notify that a Fence has expired\n> + *\n> + * Notify to the Request that a Fence has expired. This function decrease the\n> + * number of pending Fences in the request and increase the number of expired\n> + * ones.\n> + */\n> +void Request::Private::fenceExpired()\n> +{\n> + expiredFences_++;\n> + pendingFences_--;\n\nAre all these event driven counters thread safe? They may be, just seems\na potential risk area...\n\n\n> +}\n> +\n> /**\n> * \\enum Request::Status\n> * Request completion status\n> @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)\n> \n> sequence_ = 0;\n> status_ = RequestPending;\n> - _d()->cancelled_ = false;\n> \n> controls_->clear();\n> metadata_->clear();\n> +\n> + _d()->reuse();\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 C0779BF415\n\tfor ;\n\tTue, 9 Nov 2021 13:23:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 288276034E;\n\tTue, 9 Nov 2021 14:23:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70D68600BF\n\tfor ;\n\tTue, 9 Nov 2021 14:23:35 +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 0981CDEE;\n\tTue, 9 Nov 2021 14:23:35 +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=\"UVkNq5/p\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636464215;\n\tbh=iOWPkufdHF2aQM8hsUwAfSkmBM2GNrSOdGRp7NvRpEE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=UVkNq5/p1ZMW913l570NLggoFkKT27fQ1OH1HK4tFRYtNyPJjq1xiUDzDprI84S7m\n\tYjdWbGBSBlaOp2N8G74uvedKC9AG99fCEk627yZhWzVHorXsNsxo95696hmyAN6UDL\n\ta+LE/7MIw5zEZvhXF7WvOzkmPohUD3SqwFC2VOEI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-6-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-6-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 13:23:32 +0000","Message-ID":"<163646421293.1606134.1729310028789817202@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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":20767,"web_url":"https://patchwork.libcamera.org/comment/20767/","msgid":"<20211109173446.vbhdogutcenrysfs@uno.localdomain>","date":"2021-11-09T17:34:46","subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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:23:32PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-10-28 12:15:15)\n> > Prepare the Request::Private class to handle fences by providing a\n> > storage vector and interface functions to handle the number of pending\n> > and expired fences.\n> >\n> > The intended user of the interface is the PipelineHandler class\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/request.h | 21 +++++++\n> > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++-\n> > 2 files changed, 109 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > index df0cc014067e..2be4874756de 100644\n> > --- a/include/libcamera/internal/request.h\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -7,8 +7,12 @@\n> > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n> > #define __LIBCAMERA_INTERNAL_REQUEST_H__\n> >\n> > +#include \n> > +\n> > #include \n> >\n> > +#include \n> > +\n> > namespace libcamera {\n> >\n> > class Camera;\n> > @@ -24,9 +28,26 @@ public:\n> >\n> > Camera *camera() const { return camera_; }\n> >\n> > + unsigned int pendingFences() const { return pendingFences_; }\n> > + unsigned int expiredFences() const { return expiredFences_; }\n> > +\n> > + void reuse();\n> > +\n> > + std::vector &fences() { return fences_; }\n> > + void addFence(Fence &&fence);\n> > + void clearFences();\n> > +\n> > + void fenceExpired();\n> > + void fenceCompleted();\n> > +\n> > private:\n> > Camera *camera_;\n> > bool cancelled_;\n> > +\n> > + unsigned int pendingFences_ = 0;\n> > + unsigned int expiredFences_ = 0;\n> > +\n> > + std::vector fences_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 33fee1ac05ba..e88eee1fac36 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -63,6 +63,92 @@ Request::Private::~Private()\n> > * request hasn't been queued\n> > */\n> >\n> > +/**\n> > + * \\fn Request::Private::pendingFences()\n> > + * \\brief Retrieve the number of pending fences\n> > + *\n> > + * A Fence is pending if has not yet been signalled or it has not expired yet.\n> > + *\n> > + * \\return The number of pending fences\n> > + */\n> > +\n> > +/**\n> > + * \\fn Request::Private::expiredFences()\n> > + * \\brief Retrieve the number of expired fences\n> > + * \\return The number of expired fences\n> > + */\n> > +\n> > +/**\n> > + * \\brief Reset the request for reuse\n> > + */\n> > +void Request::Private::reuse()\n> > +{\n> > + cancelled_ = false;\n> > +\n> > + fences_.clear();\n> > + pendingFences_ = 0;\n> > + expiredFences_ = 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn Request::Private::fences()\n> > + * \\brief Retrieve a reference to the vector of pending fences\n> > + * \\return A reference to the vector of pending fences\n> > + */\n> > +\n> > +/**\n> > + * \\brief Move a Fence into the Request\n> > + *\n> > + * Move a Fence into the Request and increase the pending fences\n> > + * counter. The Fence is now stored into the Request and the original\n> > + * one passed in as parameter is reset.\n> > + *\n> > + * Once the Fence completes, either because it is signalled or because\n> > + * it has expired, the caller shall notify the Request about this by\n> > + * calling fenceCompleted() or fenceExpired();\n> > + */\n> > +void Request::Private::addFence(Fence &&fence)\n> > +{\n> > + fences_.push_back(std::move(fence));\n> > + pendingFences_++;\n>\n> Hrm ... I was expecting signals to be connected here ... but I guess\n> that happens elsewhere...\n>\n>\n> > +}\n> > +\n> > +/**\n> > + * \\brief Release all Fences stored in the request\n> > + *\n> > + * Clear the vector of fences in the Request by releasing all of them.\n> > + * All Fences are closed and their file descriptors reset to 0.\n> > + */\n> > +void Request::Private::clearFences()\n> > +{\n> > + ASSERT(!pendingFences_);\n> > + fences_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Notify that a Fence has been signalled\n> > + *\n> > + * Notify to the Request that a Fence has completed. This function decrease the\n> > + * number of pending Fences in the request.\n> > + */\n> > +void Request::Private::fenceCompleted()\n> > +{\n> > + pendingFences_--;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Notify that a Fence has expired\n> > + *\n> > + * Notify to the Request that a Fence has expired. This function decrease the\n> > + * number of pending Fences in the request and increase the number of expired\n> > + * ones.\n> > + */\n> > +void Request::Private::fenceExpired()\n> > +{\n> > + expiredFences_++;\n> > + pendingFences_--;\n>\n> Are all these event driven counters thread safe? They may be, just seems\n> a potential risk area...\n\nMy understanding is that slots are synchronous unless otherwise\nspecified.\n\nAs the Request::Private::fence*() functions are called from the\nPipelineHandler::fenceCompleted() slot connected to Fence::complete\nsignal what happens is that\n- Fence::activated() stops the timer\n- Fence::timedout() disables the event notifier\n\nso a single Fence::complete is emitted per Fence.\n\nFrom there we call the PipelineHandler::fenceCompleted() slot which is\nrun synchronously, hence I think we're safe.\n\nDoes this seems safe to you as well ?\n\nThanks\n j\n\n>\n>\n> > +}\n> > +\n> > /**\n> > * \\enum Request::Status\n> > * Request completion status\n> > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)\n> >\n> > sequence_ = 0;\n> > status_ = RequestPending;\n> > - _d()->cancelled_ = false;\n> >\n> > controls_->clear();\n> > metadata_->clear();\n> > +\n> > + _d()->reuse();\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 169CABDB1C\n\tfor ;\n\tTue, 9 Nov 2021 17:33:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62E496034E;\n\tTue, 9 Nov 2021 18:33:54 +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 B2FF4600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:33:53 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 3B5C540002;\n\tTue, 9 Nov 2021 17:33:53 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 18:34:46 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211109173446.vbhdogutcenrysfs@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-6-jacopo@jmondi.org>\n\t<163646421293.1606134.1729310028789817202@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163646421293.1606134.1729310028789817202@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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":20782,"web_url":"https://patchwork.libcamera.org/comment/20782/","msgid":"","date":"2021-11-10T09:21:44","subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 10/28/21 4:45 PM, Jacopo Mondi wrote:\n> Prepare the Request::Private class to handle fences by providing a\n> storage vector and interface functions to handle the number of pending\n> and expired fences.\n>\n> The intended user of the interface is the PipelineHandler class\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/request.h | 21 +++++++\n> src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++-\n> 2 files changed, 109 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index df0cc014067e..2be4874756de 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -7,8 +7,12 @@\n> #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n> #define __LIBCAMERA_INTERNAL_REQUEST_H__\n> \n> +#include \n> +\n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> class Camera;\n> @@ -24,9 +28,26 @@ public:\n> \n> \tCamera *camera() const { return camera_; }\n> \n> +\tunsigned int pendingFences() const { return pendingFences_; }\n> +\tunsigned int expiredFences() const { return expiredFences_; }\n> +\n> +\tvoid reuse();\n> +\n> +\tstd::vector &fences() { return fences_; }\n> +\tvoid addFence(Fence &&fence);\n> +\tvoid clearFences();\n> +\n> +\tvoid fenceExpired();\n> +\tvoid fenceCompleted();\n> +\n> private:\n> \tCamera *camera_;\n> \tbool cancelled_;\n> +\n> +\tunsigned int pendingFences_ = 0;\n> +\tunsigned int expiredFences_ = 0;\n> +\n> +\tstd::vector fences_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 33fee1ac05ba..e88eee1fac36 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -63,6 +63,92 @@ Request::Private::~Private()\n> * request hasn't been queued\n> */\n> \n> +/**\n> + * \\fn Request::Private::pendingFences()\n> + * \\brief Retrieve the number of pending fences\n> + *\n> + * A Fence is pending if has not yet been signalled or it has not expired yet.\n> + *\n> + * \\return The number of pending fences\n> + */\n> +\n> +/**\n> + * \\fn Request::Private::expiredFences()\n> + * \\brief Retrieve the number of expired fences\n> + * \\return The number of expired fences\n> + */\n> +\n> +/**\n> + * \\brief Reset the request for reuse\n> + */\n> +void Request::Private::reuse()\n> +{\n> +\tcancelled_ = false;\n> +\n> +\tfences_.clear();\n> +\tpendingFences_ = 0;\n> +\texpiredFences_ = 0;\n> +}\n> +\n> +/**\n> + * \\fn Request::Private::fences()\n> + * \\brief Retrieve a reference to the vector of pending fences\n> + * \\return A reference to the vector of pending fences\n> + */\n> +\n> +/**\n> + * \\brief Move a Fence into the Request\n> + *\n> + * Move a Fence into the Request and increase the pending fences\n> + * counter. The Fence is now stored into the Request and the original\n> + * one passed in as parameter is reset.\n> + *\n> + * Once the Fence completes, either because it is signalled or because\n> + * it has expired, the caller shall notify the Request about this by\n> + * calling fenceCompleted() or fenceExpired();\n> + */\n> +void Request::Private::addFence(Fence &&fence)\n> +{\n> +\tfences_.push_back(std::move(fence));\n\nDrive-by comment, please let me know if I am wrong:\n\nAs per my understanding and your clarification, the fence is moved to a \nnew Fence and pushed into fences_. I believe this new Fence will be \ndisabled (by-default behaviour as stated in Fence class). So I don't \nthink any of fenceCompleted() or fenceExpired() getting called, which \nseems problematic.\n\n> +\tpendingFences_++;\n> +}\n> +\n> +/**\n> + * \\brief Release all Fences stored in the request\n> + *\n> + * Clear the vector of fences in the Request by releasing all of them.\n> + * All Fences are closed and their file descriptors reset to 0.\n> + */\n> +void Request::Private::clearFences()\n> +{\n> +\tASSERT(!pendingFences_);\n> +\tfences_.clear();\n> +}\n> +\n> +/**\n> + * \\brief Notify that a Fence has been signalled\n> + *\n> + * Notify to the Request that a Fence has completed. This function decrease the\n> + * number of pending Fences in the request.\n> + */\n> +void Request::Private::fenceCompleted()\n> +{\n> +\tpendingFences_--;\n> +}\n> +\n> +/**\n> + * \\brief Notify that a Fence has expired\n> + *\n> + * Notify to the Request that a Fence has expired. This function decrease the\n> + * number of pending Fences in the request and increase the number of expired\n> + * ones.\n> + */\n> +void Request::Private::fenceExpired()\n> +{\n> +\texpiredFences_++;\n> +\tpendingFences_--;\n> +}\n> +\n> /**\n> * \\enum Request::Status\n> * Request completion status\n> @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)\n> \n> \tsequence_ = 0;\n> \tstatus_ = RequestPending;\n> -\t_d()->cancelled_ = false;\n> \n> \tcontrols_->clear();\n> \tmetadata_->clear();\n> +\n> +\t_d()->reuse();\n> }\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 363BBBF415\n\tfor ;\n\tWed, 10 Nov 2021 09:21:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E0476035A;\n\tWed, 10 Nov 2021 10:21:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 96A1760128\n\tfor ;\n\tWed, 10 Nov 2021 10:21:49 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D16EED8B;\n\tWed, 10 Nov 2021 10:21:48 +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=\"jA/yZ5WM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636536109;\n\tbh=aAhXSKqc1LLSUDMXeSjNKUPmJ6+II69LeAe5hGdt0Hg=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=jA/yZ5WM1paG4a5l7QlrLUPQbthxsJK1q0S8JvGKGKQlfNQu7AxVt2l+/I9Fu/pwc\n\teWLB8UheDspYMmjk9Rp/EF9+uareqd5iytSE3HopH5on0wdOGjUGSnSFznwWHt4S2r\n\tOtxTX4VoZuKxXjJh3VGv/X3AJHaIMk+27mRFCJbA=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-6-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 10 Nov 2021 14:51:44 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211028111520.256612-6-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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":20783,"web_url":"https://patchwork.libcamera.org/comment/20783/","msgid":"<20211110092827.tovodowtm6fmayrn@uno.localdomain>","date":"2021-11-10T09:28:27","subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Wed, Nov 10, 2021 at 02:51:44PM +0530, Umang Jain wrote:\n> Hi Jacopo,\n>\n> On 10/28/21 4:45 PM, Jacopo Mondi wrote:\n> > Prepare the Request::Private class to handle fences by providing a\n> > storage vector and interface functions to handle the number of pending\n> > and expired fences.\n> >\n> > The intended user of the interface is the PipelineHandler class\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/request.h | 21 +++++++\n> > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++-\n> > 2 files changed, 109 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > index df0cc014067e..2be4874756de 100644\n> > --- a/include/libcamera/internal/request.h\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -7,8 +7,12 @@\n> > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n> > #define __LIBCAMERA_INTERNAL_REQUEST_H__\n> > +#include \n> > +\n> > #include \n> > +#include \n> > +\n> > namespace libcamera {\n> > class Camera;\n> > @@ -24,9 +28,26 @@ public:\n> > \tCamera *camera() const { return camera_; }\n> > +\tunsigned int pendingFences() const { return pendingFences_; }\n> > +\tunsigned int expiredFences() const { return expiredFences_; }\n> > +\n> > +\tvoid reuse();\n> > +\n> > +\tstd::vector &fences() { return fences_; }\n> > +\tvoid addFence(Fence &&fence);\n> > +\tvoid clearFences();\n> > +\n> > +\tvoid fenceExpired();\n> > +\tvoid fenceCompleted();\n> > +\n> > private:\n> > \tCamera *camera_;\n> > \tbool cancelled_;\n> > +\n> > +\tunsigned int pendingFences_ = 0;\n> > +\tunsigned int expiredFences_ = 0;\n> > +\n> > +\tstd::vector fences_;\n> > };\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 33fee1ac05ba..e88eee1fac36 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -63,6 +63,92 @@ Request::Private::~Private()\n> > * request hasn't been queued\n> > */\n> > +/**\n> > + * \\fn Request::Private::pendingFences()\n> > + * \\brief Retrieve the number of pending fences\n> > + *\n> > + * A Fence is pending if has not yet been signalled or it has not expired yet.\n> > + *\n> > + * \\return The number of pending fences\n> > + */\n> > +\n> > +/**\n> > + * \\fn Request::Private::expiredFences()\n> > + * \\brief Retrieve the number of expired fences\n> > + * \\return The number of expired fences\n> > + */\n> > +\n> > +/**\n> > + * \\brief Reset the request for reuse\n> > + */\n> > +void Request::Private::reuse()\n> > +{\n> > +\tcancelled_ = false;\n> > +\n> > +\tfences_.clear();\n> > +\tpendingFences_ = 0;\n> > +\texpiredFences_ = 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn Request::Private::fences()\n> > + * \\brief Retrieve a reference to the vector of pending fences\n> > + * \\return A reference to the vector of pending fences\n> > + */\n> > +\n> > +/**\n> > + * \\brief Move a Fence into the Request\n> > + *\n> > + * Move a Fence into the Request and increase the pending fences\n> > + * counter. The Fence is now stored into the Request and the original\n> > + * one passed in as parameter is reset.\n> > + *\n> > + * Once the Fence completes, either because it is signalled or because\n> > + * it has expired, the caller shall notify the Request about this by\n> > + * calling fenceCompleted() or fenceExpired();\n> > + */\n> > +void Request::Private::addFence(Fence &&fence)\n> > +{\n> > +\tfences_.push_back(std::move(fence));\n>\n> Drive-by comment, please let me know if I am wrong:\n>\n> As per my understanding and your clarification, the fence is moved to a new\n> Fence and pushed into fences_. I believe this new Fence will be disabled\n> (by-default behaviour as stated in Fence class). So I don't think any of\n> fenceCompleted() or fenceExpired() getting called, which seems problematic.\n>\n\nNo, that's by design :)\n\nFences will be activated only when the request is queued to the camera.\n\n> > +\tpendingFences_++;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Release all Fences stored in the request\n> > + *\n> > + * Clear the vector of fences in the Request by releasing all of them.\n> > + * All Fences are closed and their file descriptors reset to 0.\n> > + */\n> > +void Request::Private::clearFences()\n> > +{\n> > +\tASSERT(!pendingFences_);\n> > +\tfences_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Notify that a Fence has been signalled\n> > + *\n> > + * Notify to the Request that a Fence has completed. This function decrease the\n> > + * number of pending Fences in the request.\n> > + */\n> > +void Request::Private::fenceCompleted()\n> > +{\n> > +\tpendingFences_--;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Notify that a Fence has expired\n> > + *\n> > + * Notify to the Request that a Fence has expired. This function decrease the\n> > + * number of pending Fences in the request and increase the number of expired\n> > + * ones.\n> > + */\n> > +void Request::Private::fenceExpired()\n> > +{\n> > +\texpiredFences_++;\n> > +\tpendingFences_--;\n> > +}\n> > +\n> > /**\n> > * \\enum Request::Status\n> > * Request completion status\n> > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)\n> > \tsequence_ = 0;\n> > \tstatus_ = RequestPending;\n> > -\t_d()->cancelled_ = false;\n> > \tcontrols_->clear();\n> > \tmetadata_->clear();\n> > +\n> > +\t_d()->reuse();\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 CF8C4BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 09:27:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 491236034A;\n\tWed, 10 Nov 2021 10:27:36 +0100 (CET)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5247860128\n\tfor ;\n\tWed, 10 Nov 2021 10:27:34 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id D58BC200008;\n\tWed, 10 Nov 2021 09:27:33 +0000 (UTC)"],"Date":"Wed, 10 Nov 2021 10:28:27 +0100","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20211110092827.tovodowtm6fmayrn@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-6-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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":20796,"web_url":"https://patchwork.libcamera.org/comment/20796/","msgid":"<163654184603.1896795.15850817086408488829@Monstersaurus>","date":"2021-11-10T10:57:26","subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-11-09 17:34:46)\n> Hi Kieran,\n> \n> On Tue, Nov 09, 2021 at 01:23:32PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-10-28 12:15:15)\n> > > Prepare the Request::Private class to handle fences by providing a\n> > > storage vector and interface functions to handle the number of pending\n> > > and expired fences.\n> > >\n> > > The intended user of the interface is the PipelineHandler class\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/request.h | 21 +++++++\n> > > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++-\n> > > 2 files changed, 109 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > index df0cc014067e..2be4874756de 100644\n> > > --- a/include/libcamera/internal/request.h\n> > > +++ b/include/libcamera/internal/request.h\n> > > @@ -7,8 +7,12 @@\n> > > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n> > > #define __LIBCAMERA_INTERNAL_REQUEST_H__\n> > >\n> > > +#include \n> > > +\n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > namespace libcamera {\n> > >\n> > > class Camera;\n> > > @@ -24,9 +28,26 @@ public:\n> > >\n> > > Camera *camera() const { return camera_; }\n> > >\n> > > + unsigned int pendingFences() const { return pendingFences_; }\n> > > + unsigned int expiredFences() const { return expiredFences_; }\n> > > +\n> > > + void reuse();\n> > > +\n> > > + std::vector &fences() { return fences_; }\n> > > + void addFence(Fence &&fence);\n> > > + void clearFences();\n> > > +\n> > > + void fenceExpired();\n> > > + void fenceCompleted();\n> > > +\n> > > private:\n> > > Camera *camera_;\n> > > bool cancelled_;\n> > > +\n> > > + unsigned int pendingFences_ = 0;\n> > > + unsigned int expiredFences_ = 0;\n> > > +\n> > > + std::vector fences_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index 33fee1ac05ba..e88eee1fac36 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -63,6 +63,92 @@ Request::Private::~Private()\n> > > * request hasn't been queued\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn Request::Private::pendingFences()\n> > > + * \\brief Retrieve the number of pending fences\n> > > + *\n> > > + * A Fence is pending if has not yet been signalled or it has not expired yet.\n> > > + *\n> > > + * \\return The number of pending fences\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Request::Private::expiredFences()\n> > > + * \\brief Retrieve the number of expired fences\n> > > + * \\return The number of expired fences\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Reset the request for reuse\n> > > + */\n> > > +void Request::Private::reuse()\n> > > +{\n> > > + cancelled_ = false;\n> > > +\n> > > + fences_.clear();\n> > > + pendingFences_ = 0;\n> > > + expiredFences_ = 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn Request::Private::fences()\n> > > + * \\brief Retrieve a reference to the vector of pending fences\n> > > + * \\return A reference to the vector of pending fences\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Move a Fence into the Request\n> > > + *\n> > > + * Move a Fence into the Request and increase the pending fences\n> > > + * counter. The Fence is now stored into the Request and the original\n> > > + * one passed in as parameter is reset.\n> > > + *\n> > > + * Once the Fence completes, either because it is signalled or because\n> > > + * it has expired, the caller shall notify the Request about this by\n> > > + * calling fenceCompleted() or fenceExpired();\n> > > + */\n> > > +void Request::Private::addFence(Fence &&fence)\n> > > +{\n> > > + fences_.push_back(std::move(fence));\n> > > + pendingFences_++;\n> >\n> > Hrm ... I was expecting signals to be connected here ... but I guess\n> > that happens elsewhere...\n> >\n> >\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Release all Fences stored in the request\n> > > + *\n> > > + * Clear the vector of fences in the Request by releasing all of them.\n> > > + * All Fences are closed and their file descriptors reset to 0.\n> > > + */\n> > > +void Request::Private::clearFences()\n> > > +{\n> > > + ASSERT(!pendingFences_);\n> > > + fences_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Notify that a Fence has been signalled\n> > > + *\n> > > + * Notify to the Request that a Fence has completed. This function decrease the\n> > > + * number of pending Fences in the request.\n> > > + */\n> > > +void Request::Private::fenceCompleted()\n> > > +{\n> > > + pendingFences_--;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Notify that a Fence has expired\n> > > + *\n> > > + * Notify to the Request that a Fence has expired. This function decrease the\n> > > + * number of pending Fences in the request and increase the number of expired\n> > > + * ones.\n> > > + */\n> > > +void Request::Private::fenceExpired()\n> > > +{\n> > > + expiredFences_++;\n> > > + pendingFences_--;\n> >\n> > Are all these event driven counters thread safe? They may be, just seems\n> > a potential risk area...\n> \n> My understanding is that slots are synchronous unless otherwise\n> specified.\n> \n> As the Request::Private::fence*() functions are called from the\n> PipelineHandler::fenceCompleted() slot connected to Fence::complete\n> signal what happens is that\n> - Fence::activated() stops the timer\n> - Fence::timedout() disables the event notifier\n> \n> so a single Fence::complete is emitted per Fence.\n> \n> From there we call the PipelineHandler::fenceCompleted() slot which is\n> run synchronously, hence I think we're safe.\n> \n> Does this seems safe to you as well ?\n\nCan multiple Fences complete at the same time? or are they 'safe'\nbecause they would all be handled/monitored from the same event loop,\nthus only one can happen...\n\nI think it's fine. They all generate events from the EventNotifier, and\nI think I expect them to all be in the same thread....\n\nReviewed-by: Kieran Bingham \n\n> Thanks\n> j\n> \n> >\n> >\n> > > +}\n> > > +\n> > > /**\n> > > * \\enum Request::Status\n> > > * Request completion status\n> > > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)\n> > >\n> > > sequence_ = 0;\n> > > status_ = RequestPending;\n> > > - _d()->cancelled_ = false;\n> > >\n> > > controls_->clear();\n> > > metadata_->clear();\n> > > +\n> > > + _d()->reuse();\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 BA4B8BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 10:57:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06D756034E;\n\tWed, 10 Nov 2021 11:57:30 +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 8C51960128\n\tfor ;\n\tWed, 10 Nov 2021 11:57:28 +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 2A0C7D8B;\n\tWed, 10 Nov 2021 11:57:28 +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=\"glvpaeTC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636541848;\n\tbh=tkRpHhfa6/nGKU2t8SYUPPITF5r66km8cfU6X0yzbv0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=glvpaeTCHuZQn3ufeBOC0L8HvfdszCzTm+l03LTp2AJSKub5HmJ1kXAJvewA5pigG\n\tmrXQJONjP328/ZB98gOQcRBepNUmOwt2LuBNYNB56r1ck2VdyMz/JawYwpoNyQN/MF\n\tq/cK/IqnbdnG2BWWd9wImW9JdcYYjgtFGRuCU9ks=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211109173446.vbhdogutcenrysfs@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-6-jacopo@jmondi.org>\n\t<163646421293.1606134.1729310028789817202@Monstersaurus>\n\t<20211109173446.vbhdogutcenrysfs@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ","Date":"Wed, 10 Nov 2021 10:57:26 +0000","Message-ID":"<163654184603.1896795.15850817086408488829@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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":20799,"web_url":"https://patchwork.libcamera.org/comment/20799/","msgid":"","date":"2021-11-10T12:45:32","subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 11/10/21 2:58 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Wed, Nov 10, 2021 at 02:51:44PM +0530, Umang Jain wrote:\n>> Hi Jacopo,\n>>\n>> On 10/28/21 4:45 PM, Jacopo Mondi wrote:\n>>> Prepare the Request::Private class to handle fences by providing a\n>>> storage vector and interface functions to handle the number of pending\n>>> and expired fences.\n>>>\n>>> The intended user of the interface is the PipelineHandler class\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> include/libcamera/internal/request.h | 21 +++++++\n>>> src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++-\n>>> 2 files changed, 109 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n>>> index df0cc014067e..2be4874756de 100644\n>>> --- a/include/libcamera/internal/request.h\n>>> +++ b/include/libcamera/internal/request.h\n>>> @@ -7,8 +7,12 @@\n>>> #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n>>> #define __LIBCAMERA_INTERNAL_REQUEST_H__\n>>> +#include \n>>> +\n>>> #include \n>>> +#include \n>>> +\n>>> namespace libcamera {\n>>> class Camera;\n>>> @@ -24,9 +28,26 @@ public:\n>>> \tCamera *camera() const { return camera_; }\n>>> +\tunsigned int pendingFences() const { return pendingFences_; }\n>>> +\tunsigned int expiredFences() const { return expiredFences_; }\n>>> +\n>>> +\tvoid reuse();\n>>> +\n>>> +\tstd::vector &fences() { return fences_; }\n>>> +\tvoid addFence(Fence &&fence);\n>>> +\tvoid clearFences();\n>>> +\n>>> +\tvoid fenceExpired();\n>>> +\tvoid fenceCompleted();\n>>> +\n>>> private:\n>>> \tCamera *camera_;\n>>> \tbool cancelled_;\n>>> +\n>>> +\tunsigned int pendingFences_ = 0;\n>>> +\tunsigned int expiredFences_ = 0;\n>>> +\n>>> +\tstd::vector fences_;\n>>> };\n>>> } /* namespace libcamera */\n>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>>> index 33fee1ac05ba..e88eee1fac36 100644\n>>> --- a/src/libcamera/request.cpp\n>>> +++ b/src/libcamera/request.cpp\n>>> @@ -63,6 +63,92 @@ Request::Private::~Private()\n>>> * request hasn't been queued\n>>> */\n>>> +/**\n>>> + * \\fn Request::Private::pendingFences()\n>>> + * \\brief Retrieve the number of pending fences\n>>> + *\n>>> + * A Fence is pending if has not yet been signalled or it has not expired yet.\n>>> + *\n>>> + * \\return The number of pending fences\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn Request::Private::expiredFences()\n>>> + * \\brief Retrieve the number of expired fences\n>>> + * \\return The number of expired fences\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Reset the request for reuse\n>>> + */\n>>> +void Request::Private::reuse()\n>>> +{\n>>> +\tcancelled_ = false;\n>>> +\n>>> +\tfences_.clear();\n>>> +\tpendingFences_ = 0;\n>>> +\texpiredFences_ = 0;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\fn Request::Private::fences()\n>>> + * \\brief Retrieve a reference to the vector of pending fences\n>>> + * \\return A reference to the vector of pending fences\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Move a Fence into the Request\n>>> + *\n>>> + * Move a Fence into the Request and increase the pending fences\n>>> + * counter. The Fence is now stored into the Request and the original\n>>> + * one passed in as parameter is reset.\n>>> + *\n>>> + * Once the Fence completes, either because it is signalled or because\n>>> + * it has expired, the caller shall notify the Request about this by\n>>> + * calling fenceCompleted() or fenceExpired();\n>>> + */\n>>> +void Request::Private::addFence(Fence &&fence)\n>>> +{\n>>> +\tfences_.push_back(std::move(fence));\n>> Drive-by comment, please let me know if I am wrong:\n>>\n>> As per my understanding and your clarification, the fence is moved to a new\n>> Fence and pushed into fences_. I believe this new Fence will be disabled\n>> (by-default behaviour as stated in Fence class). So I don't think any of\n>> fenceCompleted() or fenceExpired() getting called, which seems problematic.\n>>\n> No, that's by design :)\n>\n> Fences will be activated only when the request is queued to the camera.\n\n\nOk, I see in the below patch that you enable one-by-one.\n\nI think I got confused by your reply and the statement point from the \ncover letter :\n\n```\n\n- Fences are attacched to a FrameBuffer and their value is valid until the\n Request is queued to the Camera\n\n```\n\n>\n>>> +\tpendingFences_++;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Release all Fences stored in the request\n>>> + *\n>>> + * Clear the vector of fences in the Request by releasing all of them.\n>>> + * All Fences are closed and their file descriptors reset to 0.\n>>> + */\n>>> +void Request::Private::clearFences()\n>>> +{\n>>> +\tASSERT(!pendingFences_);\n>>> +\tfences_.clear();\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Notify that a Fence has been signalled\n>>> + *\n>>> + * Notify to the Request that a Fence has completed. This function decrease the\n>>> + * number of pending Fences in the request.\n>>> + */\n>>> +void Request::Private::fenceCompleted()\n>>> +{\n>>> +\tpendingFences_--;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Notify that a Fence has expired\n>>> + *\n>>> + * Notify to the Request that a Fence has expired. This function decrease the\n>>> + * number of pending Fences in the request and increase the number of expired\n>>> + * ones.\n>>> + */\n>>> +void Request::Private::fenceExpired()\n>>> +{\n>>> +\texpiredFences_++;\n>>> +\tpendingFences_--;\n>>> +}\n>>> +\n>>> /**\n>>> * \\enum Request::Status\n>>> * Request completion status\n>>> @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)\n>>> \tsequence_ = 0;\n>>> \tstatus_ = RequestPending;\n>>> -\t_d()->cancelled_ = false;\n>>> \tcontrols_->clear();\n>>> \tmetadata_->clear();\n>>> +\n>>> +\t_d()->reuse();\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 BD728BF415\n\tfor ;\n\tWed, 10 Nov 2021 12:45:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 791F96035D;\n\tWed, 10 Nov 2021 13:45:40 +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 9EDBA6033C\n\tfor ;\n\tWed, 10 Nov 2021 13:45:38 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2ABD556;\n\tWed, 10 Nov 2021 13:45:37 +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=\"a/tLG7H1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636548338;\n\tbh=E/oddeTs4vOvcw04fWOt9lTLYb/29rdg9r5UhINrzcQ=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=a/tLG7H1xUDXe21XHGZ4uxCj37sOm/bGIkoqbLvK92xl4F2P0aVYhnUHN8Jfawt/e\n\tUD/nsViTCmkBAtL55RNYA9lQ5NJNcRcs3uTuAoWFH+BNRc/eT/MBQw4cQPY71REbb8\n\tA60wnpYd6Wk9IS4M6pLoy96XrCaM0+ln6mjVlX64=","To":"Jacopo Mondi ","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-6-jacopo@jmondi.org>\n\t\n\t<20211110092827.tovodowtm6fmayrn@uno.localdomain>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 10 Nov 2021 18:15:32 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211110092827.tovodowtm6fmayrn@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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":20917,"web_url":"https://patchwork.libcamera.org/comment/20917/","msgid":"","date":"2021-11-12T15:27:45","subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Nov 10, 2021 at 10:57:26AM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-11-09 17:34:46)\n> > On Tue, Nov 09, 2021 at 01:23:32PM +0000, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi (2021-10-28 12:15:15)\n> > > > Prepare the Request::Private class to handle fences by providing a\n> > > > storage vector and interface functions to handle the number of pending\n> > > > and expired fences.\n> > > >\n> > > > The intended user of the interface is the PipelineHandler class\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/internal/request.h | 21 +++++++\n> > > > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++-\n> > > > 2 files changed, 109 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > index df0cc014067e..2be4874756de 100644\n> > > > --- a/include/libcamera/internal/request.h\n> > > > +++ b/include/libcamera/internal/request.h\n> > > > @@ -7,8 +7,12 @@\n> > > > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n> > > > #define __LIBCAMERA_INTERNAL_REQUEST_H__\n> > > >\n> > > > +#include \n> > > > +\n> > > > #include \n> > > >\n> > > > +#include \n> > > > +\n> > > > namespace libcamera {\n> > > >\n> > > > class Camera;\n> > > > @@ -24,9 +28,26 @@ public:\n> > > >\n> > > > Camera *camera() const { return camera_; }\n> > > >\n> > > > + unsigned int pendingFences() const { return pendingFences_; }\n> > > > + unsigned int expiredFences() const { return expiredFences_; }\n> > > > +\n> > > > + void reuse();\n> > > > +\n> > > > + std::vector &fences() { return fences_; }\n> > > > + void addFence(Fence &&fence);\n> > > > + void clearFences();\n> > > > +\n> > > > + void fenceExpired();\n> > > > + void fenceCompleted();\n> > > > +\n> > > > private:\n> > > > Camera *camera_;\n> > > > bool cancelled_;\n> > > > +\n> > > > + unsigned int pendingFences_ = 0;\n> > > > + unsigned int expiredFences_ = 0;\n\nBy moving timeout handling to the request or pipeline handler, you could\ndrop the expiredFences_ counter and checked the pendingFences_ counter\nwhen the global timer expires.\n\n> > > > +\n> > > > + std::vector fences_;\n\nDo we need to move the fences to the request ? It seems that fences_\nisn't used internally, and fences() is only used in\nPipelineHandler::queueRequest() where we could have a vector of Fence\npointers instead as a local variable (or even enable the fences in the\nfirst loop in that function). That would simplify the API.\n\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > index 33fee1ac05ba..e88eee1fac36 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -63,6 +63,92 @@ Request::Private::~Private()\n> > > > * request hasn't been queued\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn Request::Private::pendingFences()\n> > > > + * \\brief Retrieve the number of pending fences\n> > > > + *\n> > > > + * A Fence is pending if has not yet been signalled or it has not expired yet.\n> > > > + *\n> > > > + * \\return The number of pending fences\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Request::Private::expiredFences()\n> > > > + * \\brief Retrieve the number of expired fences\n> > > > + * \\return The number of expired fences\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Reset the request for reuse\n> > > > + */\n> > > > +void Request::Private::reuse()\n> > > > +{\n> > > > + cancelled_ = false;\n> > > > +\n> > > > + fences_.clear();\n> > > > + pendingFences_ = 0;\n> > > > + expiredFences_ = 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\fn Request::Private::fences()\n> > > > + * \\brief Retrieve a reference to the vector of pending fences\n> > > > + * \\return A reference to the vector of pending fences\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Move a Fence into the Request\n> > > > + *\n> > > > + * Move a Fence into the Request and increase the pending fences\n> > > > + * counter. The Fence is now stored into the Request and the original\n> > > > + * one passed in as parameter is reset.\n> > > > + *\n> > > > + * Once the Fence completes, either because it is signalled or because\n> > > > + * it has expired, the caller shall notify the Request about this by\n> > > > + * calling fenceCompleted() or fenceExpired();\n> > > > + */\n> > > > +void Request::Private::addFence(Fence &&fence)\n> > > > +{\n> > > > + fences_.push_back(std::move(fence));\n> > > > + pendingFences_++;\n> > >\n> > > Hrm ... I was expecting signals to be connected here ... but I guess\n> > > that happens elsewhere...\n> > >\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Release all Fences stored in the request\n> > > > + *\n> > > > + * Clear the vector of fences in the Request by releasing all of them.\n> > > > + * All Fences are closed and their file descriptors reset to 0.\n> > > > + */\n> > > > +void Request::Private::clearFences()\n> > > > +{\n> > > > + ASSERT(!pendingFences_);\n> > > > + fences_.clear();\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Notify that a Fence has been signalled\n> > > > + *\n> > > > + * Notify to the Request that a Fence has completed. This function decrease the\n> > > > + * number of pending Fences in the request.\n> > > > + */\n> > > > +void Request::Private::fenceCompleted()\n> > > > +{\n> > > > + pendingFences_--;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Notify that a Fence has expired\n> > > > + *\n> > > > + * Notify to the Request that a Fence has expired. This function decrease the\n> > > > + * number of pending Fences in the request and increase the number of expired\n> > > > + * ones.\n> > > > + */\n> > > > +void Request::Private::fenceExpired()\n> > > > +{\n> > > > + expiredFences_++;\n> > > > + pendingFences_--;\n> > >\n> > > Are all these event driven counters thread safe? They may be, just seems\n> > > a potential risk area...\n> > \n> > My understanding is that slots are synchronous unless otherwise\n> > specified.\n> > \n> > As the Request::Private::fence*() functions are called from the\n> > PipelineHandler::fenceCompleted() slot connected to Fence::complete\n> > signal what happens is that\n> > - Fence::activated() stops the timer\n> > - Fence::timedout() disables the event notifier\n> > \n> > so a single Fence::complete is emitted per Fence.\n> > \n> > From there we call the PipelineHandler::fenceCompleted() slot which is\n> > run synchronously, hence I think we're safe.\n> > \n> > Does this seems safe to you as well ?\n> \n> Can multiple Fences complete at the same time? or are they 'safe'\n> because they would all be handled/monitored from the same event loop,\n> thus only one can happen...\n\nEverything is running in a single thread on the pipeline handler side,\nwith a single event loop, so it should be safe (there's a single thread\ncovering all pipeline handlers at the moment, which may be turned into a\nper-pipeline thread in the future, but that won't make any difference\nhere).\n\n> I think it's fine. They all generate events from the EventNotifier, and\n> I think I expect them to all be in the same thread....\n> \n> Reviewed-by: Kieran Bingham \n> \n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\enum Request::Status\n> > > > * Request completion status\n> > > > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)\n> > > >\n> > > > sequence_ = 0;\n> > > > status_ = RequestPending;\n> > > > - _d()->cancelled_ = false;\n> > > >\n> > > > controls_->clear();\n> > > > metadata_->clear();\n> > > > +\n> > > > + _d()->reuse();\n> > > > }\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 34971BF415\n\tfor ;\n\tFri, 12 Nov 2021 15:28:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C3CF6036B;\n\tFri, 12 Nov 2021 16:28:08 +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 B994D6032C\n\tfor ;\n\tFri, 12 Nov 2021 16:28:07 +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 3CC0C74C;\n\tFri, 12 Nov 2021 16:28:07 +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=\"LpllC2jd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636730887;\n\tbh=BRbhp7lexnjb/TfmVY/T/AiQAG6pxTGt19JLVdPPTcw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LpllC2jdZ5FjAsk2JgVgx5KUa7FqaPxIx/9skkQDvou1DYqaIwCRt7eNY4inE+z5M\n\tgaux/NJseCGuuZskbMOnAJTLJUkPjDvM70Tv8npJHE/6jIm8wIL3r8sOb72biA0psN\n\tvVFCsA5lc+lvyka/w8XMusPJWN7iXG5lsL9D4/pI=","Date":"Fri, 12 Nov 2021 17:27:45 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-6-jacopo@jmondi.org>\n\t<163646421293.1606134.1729310028789817202@Monstersaurus>\n\t<20211109173446.vbhdogutcenrysfs@uno.localdomain>\n\t<163654184603.1896795.15850817086408488829@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163654184603.1896795.15850817086408488829@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 05/10] libcamera: request: Add support\n\tfor fences","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\" "}}]