[{"id":20761,"web_url":"https://patchwork.libcamera.org/comment/20761/","msgid":"<163647406399.1606134.9236088055713064085@Monstersaurus>","date":"2021-11-09T16:07:43","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle 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:19)\n> Before queueing a request to the device, any synchronization fence from\n> the Request framebuffers has to be waited on.\n> \n> Start by moving the fences to the Request, in order to reset the ones\n> in the FrameBuffers and then enable all of them one by one.\n\n\nAha, ok so now I'm starting to understand the lifetime of the fence.\n\nIf a Buffer is added with a fence to protect it until it's ready, do we\nintroduce any kind of race when enabling if the buffer fence has\ncompleted /before/ we enable it?\n\nI.e. could we end up sitting on a fence waiting for a completion that\nhas already occured, and then timeout - and cause the request to be\nincorrectly cancelled?\n\n\n> When a fence completes, make sure all fences in the Request have been\n> waited on or have expired and only after that actually queue the Request\n> to the device.\n> \n> If any fence in the Request has expired cancel the request and do not\n> queue it to the device.\n\nSounds good...\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/pipeline_handler.h | 2 +\n> src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++-\n> 2 files changed, 77 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 5c3c0bc5a8b3..5b98011ac4f6 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -29,6 +29,7 @@ class CameraConfiguration;\n> class CameraManager;\n> class DeviceEnumerator;\n> class DeviceMatch;\n> +class Fence;\n> class FrameBuffer;\n> class MediaDevice;\n> class PipelineHandler;\n> @@ -79,6 +80,7 @@ private:\n> void mediaDeviceDisconnected(MediaDevice *media);\n> virtual void disconnect();\n> \n> + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence);\n> void doQueueRequest();\n> \n> std::vector> mediaDevices_;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 0f9fec1b618f..39cb680e5386 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -18,6 +18,7 @@\n> #include \n> \n> #include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/request.h\"\n> @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> LIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> + Request::Private *data = request->_d();\n> +\n> {\n> MutexLocker lock(waitingRequestsMutex_);\n> waitingRequests_.push_back(request);\n> }\n> \n> + for (FrameBuffer *buffer : request->pending_) {\n> + if (buffer->fence() == -1)\n> + continue;\n> +\n> + /*\n> + * Move the Fence into the Request::Private list of\n> + * fences. This resets the file descriptor fence in the\n> + * FrameBuffer to -1.\n> + */\n> + data->addFence(std::move(buffer->_d()->fence()));\n> +\n> + Fence *fence = &data->fences().back();\n> + fence->complete.connect(this,\n> + [this, request, buffer, fence]() {\n> + fenceCompleted(request, buffer, fence);\n> + });\n> + }\n\nI can't help but wonder if this should be handled in the Request, and\nnot by moving the fences to the camera data....\n\nThe request could signal that it is ready for consumption, without\nhaving to move the ownership of the fence from the buffer to the\ncamera...\n\nThen the PipelineHandler wouldn't have to be digging around in the\ninternals of the Request buffers.\n\nOf course it already has access to those, but I'm not 100% convinced,\nthis is a good reason to dig in there, and not expose an \"I am ready\"\nsignal from the request. But maybe that adds more layering on the requst\nthat is undesirable?\n\n\nThat's a fair bit of redesign though - so if you say \"No, it's better\nthis way\" I'll just move on ;-)\n\n\n\n> +\n> + /* If no fences to wait on, we can queue the request immediately. */\n> + if (!data->pendingFences()) {\n> + doQueueRequest();\n> +\n> + return;\n> + }\n> +\n> + /*\n> + * Now that we have added all fences, enable them one by one.\n> + * Enabling fences while adding them to the Request would race on the\n> + * number of pending fences.\n> + */\n> + for (Fence &fence : data->fences())\n> + fence.enable();\n> +}\n> +\n> +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer,\n> + Fence *fence)\n> +{\n> + Request::Private *data = request->_d();\n> +\n> + if (fence->expired()) {\n> + /*\n> + * Move the fence back to the framebuffer, so that it doesn't\n> + * get closed and the file descriptor number is made\n> + * available again to applications.\n> + */\n> + FrameBuffer::Private *bufferData = buffer->_d();\n> + bufferData->fence() = std::move(*fence);\n> +\n> + data->fenceExpired();\n> + } else {\n> + data->fenceCompleted();\n> + }\n> +\n> + if (data->pendingFences())\n> + return;\n> +\n> + data->clearFences();\n> doQueueRequest();\n> }\n> \n> @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest()\n> return;\n> \n> request = waitingRequests_.front();\n> + if (request->_d()->pendingFences())\n> + return;\n> +\n> waitingRequests_.pop_front();\n> }\n> \n> - /* Queue Request to the pipeline handler. */\n> Camera *camera = request->_d()->camera();\n> Camera::Private *camData = camera->_d();\n> \n> request->sequence_ = camData->requestSequence_++;\n> - camData->queuedRequests_.push_back(request);\n> \n> + /* Cancel the request if one of the fences has failed. */\n> + if (request->_d()->expiredFences()) {\n> + request->cancel();\n> + completeRequest(request);\n> +\n> + doQueueRequest();\n\nThis makes me really think this would be better handled in a loop rather\nthan recursion... :-S\n\n> +\n> + return;\n> + }\n> +\n> + /* Queue Request to the pipeline handler. */\n> + camData->queuedRequests_.push_back(request);\n> int ret = queueRequestDevice(camera, request);\n> if (ret) {\n> request->cancel();\n> completeRequest(request);\n> }\n> \n> - /* Try to queue the next Request in the queue, if ready. */\n> doQueueRequest();\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 7C8CCBDB1C\n\tfor ;\n\tTue, 9 Nov 2021 16:07:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4E9060234;\n\tTue, 9 Nov 2021 17:07:48 +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 979D2600BF\n\tfor ;\n\tTue, 9 Nov 2021 17:07:46 +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 28DE79FF;\n\tTue, 9 Nov 2021 17:07:46 +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=\"wDpap3yR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636474066;\n\tbh=T7zv0DytUZ8+WwFbNcB+D/fExEg9IhDZ9Q+eMIOwbHc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=wDpap3yRQ6A455Z060cRMR7somCB6J27rdkx+v6CULJK3qa7S46WXYmXtPIJpUJzk\n\tnrJ3b2pOluakiP2iB6M5B94WYq3T0CgUzac8Ttjh+w8Y6zpKynOX9tLxaK+b7vDM+B\n\tRAW1cTEfW5iHbC4TE58j02my2ZYP91fyqM0eiCVs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-10-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-10-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 16:07:43 +0000","Message-ID":"<163647406399.1606134.9236088055713064085@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle 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":20773,"web_url":"https://patchwork.libcamera.org/comment/20773/","msgid":"<20211109175724.s62teskaxf6o3rxv@uno.localdomain>","date":"2021-11-09T17:57:24","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle 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 04:07:43PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-10-28 12:15:19)\n> > Before queueing a request to the device, any synchronization fence from\n> > the Request framebuffers has to be waited on.\n> >\n> > Start by moving the fences to the Request, in order to reset the ones\n> > in the FrameBuffers and then enable all of them one by one.\n>\n>\n> Aha, ok so now I'm starting to understand the lifetime of the fence.\n>\n> If a Buffer is added with a fence to protect it until it's ready, do we\n> introduce any kind of race when enabling if the buffer fence has\n> completed /before/ we enable it?\n>\n> I.e. could we end up sitting on a fence waiting for a completion that\n> has already occured, and then timeout - and cause the request to be\n> incorrectly cancelled?\n>\n\ndepends :) For sofware fences I think if a fence gets signalled before\nwe get to poll() on it, I think poll() would immediately return\nsignalling the fence is read.\n\nI assume hw fences should work the same, but I can't tell for sure\n\n>\n> > When a fence completes, make sure all fences in the Request have been\n> > waited on or have expired and only after that actually queue the Request\n> > to the device.\n> >\n> > If any fence in the Request has expired cancel the request and do not\n> > queue it to the device.\n>\n> Sounds good...\n>\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 2 +\n> > src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++-\n> > 2 files changed, 77 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 5c3c0bc5a8b3..5b98011ac4f6 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -29,6 +29,7 @@ class CameraConfiguration;\n> > class CameraManager;\n> > class DeviceEnumerator;\n> > class DeviceMatch;\n> > +class Fence;\n> > class FrameBuffer;\n> > class MediaDevice;\n> > class PipelineHandler;\n> > @@ -79,6 +80,7 @@ private:\n> > void mediaDeviceDisconnected(MediaDevice *media);\n> > virtual void disconnect();\n> >\n> > + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence);\n> > void doQueueRequest();\n> >\n> > std::vector> mediaDevices_;\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 0f9fec1b618f..39cb680e5386 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -18,6 +18,7 @@\n> > #include \n> >\n> > #include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/framebuffer.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/request.h\"\n> > @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request)\n> > {\n> > LIBCAMERA_TRACEPOINT(request_queue, request);\n> >\n> > + Request::Private *data = request->_d();\n> > +\n> > {\n> > MutexLocker lock(waitingRequestsMutex_);\n> > waitingRequests_.push_back(request);\n> > }\n> >\n> > + for (FrameBuffer *buffer : request->pending_) {\n> > + if (buffer->fence() == -1)\n> > + continue;\n> > +\n> > + /*\n> > + * Move the Fence into the Request::Private list of\n> > + * fences. This resets the file descriptor fence in the\n> > + * FrameBuffer to -1.\n> > + */\n> > + data->addFence(std::move(buffer->_d()->fence()));\n> > +\n> > + Fence *fence = &data->fences().back();\n> > + fence->complete.connect(this,\n> > + [this, request, buffer, fence]() {\n> > + fenceCompleted(request, buffer, fence);\n> > + });\n> > + }\n>\n> I can't help but wonder if this should be handled in the Request, and\n> not by moving the fences to the camera data....\n\nThey get moved to Request::Private\n\n Request::Private *data = request->_d();\n\n for (buffer : request->pending)\n data->addFence(std::move(buffer->_d()->fence()));\n\n>\n> The request could signal that it is ready for consumption, without\n> having to move the ownership of the fence from the buffer to the\n> camera...\n\nHowever I got your point, the Request could be auto-counting, ie we\nconnect the Fence::complete signal to a slot in Request::Private and\nbasically move the fence counting implemented in\nPipelineHandler::fenceCompleted() there. I initially wanted this but I\ngot discouraged by the need to have the Request then queue itself when\nit's ready. Now that I look at it again, we could add to\nRequest::Private a 'done' (or 'ready' or whatever) signal to which we\nconnect a slot in PipelineHandler. This would also simplify the\nRequest::Private::fence*() interface that can be made private to\nRequest::Private.\n\n>\n> Then the PipelineHandler wouldn't have to be digging around in the\n> internals of the Request buffers.\n\n\nI'll experiment with that, but please note that fences will anyway\nhave to be moved to Request::Private here anyhow, as doing so at\nRequest::addBuffer() time would invalidate the fence as seen from\napplication much earlier than Camera::queueRequest() time, something I\nwould rather not do.\n\n>\n> Of course it already has access to those, but I'm not 100% convinced,\n> this is a good reason to dig in there, and not expose an \"I am ready\"\n> signal from the request. But maybe that adds more layering on the requst\n> that is undesirable?\n\nLet's see how it looks like. I didn't want the request to call\nPipelineHandler::doQueueRequest() when it's ready, but maybe with a\nSignal it gets nicer.\n\n>\n>\n> That's a fair bit of redesign though - so if you say \"No, it's better\n> this way\" I'll just move on ;-)\n\nOuch, I already acknoleded you comment. Can I take it back and say\n\"No, it's better this way\" ? :D\n>\n>\n>\n> > +\n> > + /* If no fences to wait on, we can queue the request immediately. */\n> > + if (!data->pendingFences()) {\n> > + doQueueRequest();\n> > +\n> > + return;\n> > + }\n> > +\n> > + /*\n> > + * Now that we have added all fences, enable them one by one.\n> > + * Enabling fences while adding them to the Request would race on the\n> > + * number of pending fences.\n> > + */\n> > + for (Fence &fence : data->fences())\n> > + fence.enable();\n> > +}\n> > +\n> > +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer,\n> > + Fence *fence)\n> > +{\n> > + Request::Private *data = request->_d();\n> > +\n> > + if (fence->expired()) {\n> > + /*\n> > + * Move the fence back to the framebuffer, so that it doesn't\n> > + * get closed and the file descriptor number is made\n> > + * available again to applications.\n> > + */\n> > + FrameBuffer::Private *bufferData = buffer->_d();\n> > + bufferData->fence() = std::move(*fence);\n> > +\n> > + data->fenceExpired();\n> > + } else {\n> > + data->fenceCompleted();\n> > + }\n> > +\n> > + if (data->pendingFences())\n> > + return;\n> > +\n> > + data->clearFences();\n> > doQueueRequest();\n> > }\n> >\n> > @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest()\n> > return;\n> >\n> > request = waitingRequests_.front();\n> > + if (request->_d()->pendingFences())\n> > + return;\n> > +\n> > waitingRequests_.pop_front();\n> > }\n> >\n> > - /* Queue Request to the pipeline handler. */\n> > Camera *camera = request->_d()->camera();\n> > Camera::Private *camData = camera->_d();\n> >\n> > request->sequence_ = camData->requestSequence_++;\n> > - camData->queuedRequests_.push_back(request);\n> >\n> > + /* Cancel the request if one of the fences has failed. */\n> > + if (request->_d()->expiredFences()) {\n> > + request->cancel();\n> > + completeRequest(request);\n> > +\n> > + doQueueRequest();\n>\n> This makes me really think this would be better handled in a loop rather\n> than recursion... :-S\n>\n\nStill not convinced, mostly for a style point of view of having one\nmore indentation layer.\n\nThanks\n j\n\n> > +\n> > + return;\n> > + }\n> > +\n> > + /* Queue Request to the pipeline handler. */\n> > + camData->queuedRequests_.push_back(request);\n> > int ret = queueRequestDevice(camera, request);\n> > if (ret) {\n> > request->cancel();\n> > completeRequest(request);\n> > }\n> >\n> > - /* Try to queue the next Request in the queue, if ready. */\n> > doQueueRequest();\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 D5387BF415\n\tfor ;\n\tTue, 9 Nov 2021 17:56:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DD236034E;\n\tTue, 9 Nov 2021 18:56:34 +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 CFE5A600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:56:31 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 48EB96000F;\n\tTue, 9 Nov 2021 17:56:31 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 18:57:24 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211109175724.s62teskaxf6o3rxv@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-10-jacopo@jmondi.org>\n\t<163647406399.1606134.9236088055713064085@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163647406399.1606134.9236088055713064085@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle 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":20805,"web_url":"https://patchwork.libcamera.org/comment/20805/","msgid":"<9077be91-d17c-8c5e-7d88-4b5a7446bd5c@ideasonboard.com>","date":"2021-11-10T13:09:15","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle fences","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch\n\nOn 10/28/21 4:45 PM, Jacopo Mondi wrote:\n> Before queueing a request to the device, any synchronization fence from\n> the Request framebuffers has to be waited on.\n>\n> Start by moving the fences to the Request, in order to reset the ones\n> in the FrameBuffers and then enable all of them one by one.\n>\n> When a fence completes, make sure all fences in the Request have been\n> waited on or have expired and only after that actually queue the Request\n> to the device.\n\n\nHere it's said:\n\n     waited on OR expired  => queue the Request to device\n\n>\n> If any fence in the Request has expired cancel the request and do not\n> queue it to the device.\n\n\nAnd here it's said:\n\n     If fence expired => Cancel the request and do not queue.\n\nAm I missing something?\n\nThe patch looks good to me overall.\n\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/pipeline_handler.h | 2 +\n> src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++-\n> 2 files changed, 77 insertions(+), 3 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 5c3c0bc5a8b3..5b98011ac4f6 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -29,6 +29,7 @@ class CameraConfiguration;\n> class CameraManager;\n> class DeviceEnumerator;\n> class DeviceMatch;\n> +class Fence;\n> class FrameBuffer;\n> class MediaDevice;\n> class PipelineHandler;\n> @@ -79,6 +80,7 @@ private:\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> \tvirtual void disconnect();\n> \n> +\tvoid fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence);\n> \tvoid doQueueRequest();\n> \n> \tstd::vector> mediaDevices_;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 0f9fec1b618f..39cb680e5386 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -18,6 +18,7 @@\n> #include \n> \n> #include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/request.h\"\n> @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> +\tRequest::Private *data = request->_d();\n> +\n> \t{\n> \t\tMutexLocker lock(waitingRequestsMutex_);\n> \t\twaitingRequests_.push_back(request);\n> \t}\n> \n> +\tfor (FrameBuffer *buffer : request->pending_) {\n> +\t\tif (buffer->fence() == -1)\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * Move the Fence into the Request::Private list of\n> +\t\t * fences. This resets the file descriptor fence in the\n> +\t\t * FrameBuffer to -1.\n> +\t\t */\n> +\t\tdata->addFence(std::move(buffer->_d()->fence()));\n> +\n> +\t\tFence *fence = &data->fences().back();\n> +\t\tfence->complete.connect(this,\n> +\t\t\t\t\t[this, request, buffer, fence]() {\n> +\t\t\t\t\t\tfenceCompleted(request, buffer, fence);\n> +\t\t\t\t\t});\n> +\t}\n> +\n> +\t/* If no fences to wait on, we can queue the request immediately. */\n> +\tif (!data->pendingFences()) {\n> +\t\tdoQueueRequest();\n> +\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * Now that we have added all fences, enable them one by one.\n> +\t * Enabling fences while adding them to the Request would race on the\n> +\t * number of pending fences.\n> +\t */\n> +\tfor (Fence &fence : data->fences())\n> +\t\tfence.enable();\n> +}\n> +\n> +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer,\n> +\t\t\t\t Fence *fence)\n> +{\n> +\tRequest::Private *data = request->_d();\n> +\n> +\tif (fence->expired()) {\n> +\t\t/*\n> +\t\t * Move the fence back to the framebuffer, so that it doesn't\n> +\t\t * get closed and the file descriptor number is made\n> +\t\t * available again to applications.\n> +\t\t */\n> +\t\tFrameBuffer::Private *bufferData = buffer->_d();\n> +\t\tbufferData->fence() = std::move(*fence);\n> +\n> +\t\tdata->fenceExpired();\n> +\t} else {\n> +\t\tdata->fenceCompleted();\n> +\t}\n> +\n> +\tif (data->pendingFences())\n> +\t\treturn;\n> +\n> +\tdata->clearFences();\n> \tdoQueueRequest();\n> }\n> \n> @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest()\n> \t\t\treturn;\n> \n> \t\trequest = waitingRequests_.front();\n> +\t\tif (request->_d()->pendingFences())\n> +\t\t\treturn;\n> +\n> \t\twaitingRequests_.pop_front();\n> \t}\n> \n> -\t/* Queue Request to the pipeline handler. */\n> \tCamera *camera = request->_d()->camera();\n> \tCamera::Private *camData = camera->_d();\n> \n> \trequest->sequence_ = camData->requestSequence_++;\n> -\tcamData->queuedRequests_.push_back(request);\n> \n> +\t/* Cancel the request if one of the fences has failed. */\n> +\tif (request->_d()->expiredFences()) {\n> +\t\trequest->cancel();\n> +\t\tcompleteRequest(request);\n> +\n> +\t\tdoQueueRequest();\n\n\nA bit-off to read doQueueRequest() in this codepath here, but okay I guess\n\n> +\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* Queue Request to the pipeline handler. */\n> +\tcamData->queuedRequests_.push_back(request);\n> \tint ret = queueRequestDevice(camera, request);\n> \tif (ret) {\n> \t\trequest->cancel();\n> \t\tcompleteRequest(request);\n> \t}\n> \n> -\t/* Try to queue the next Request in the queue, if ready. */\n> \tdoQueueRequest();\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 35C88BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 13:09:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6A6A6035D;\n\tWed, 10 Nov 2021 14:09:20 +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 7C0E86033C\n\tfor ;\n\tWed, 10 Nov 2021 14:09:19 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93BF555C;\n\tWed, 10 Nov 2021 14:09:18 +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=\"ZSkROPBu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636549759;\n\tbh=QNtR70Eu78uH9Zwmzj8Ly7cHtlm+vhSs7KDnbvXmLww=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ZSkROPBujQnDpdCj3w8/XknXufptiKLZcwK6Y4N7hhQeAelFfK+gIK/i7JgG9nQ9y\n\tl+N3oFC6mfYNbCZ1GL1sGlzNx3DWP8DMvEZBfDzV5XVK/0FOjuILfclSWa0Aj/5GFp\n\tUV7jKQ/RRLl6MBw3NTY88Og1knw3USGX0OCL3qWY=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-10-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"<9077be91-d17c-8c5e-7d88-4b5a7446bd5c@ideasonboard.com>","Date":"Wed, 10 Nov 2021 18:39:15 +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-10-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle 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":20923,"web_url":"https://patchwork.libcamera.org/comment/20923/","msgid":"","date":"2021-11-12T16:02:57","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle fences","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:57:24PM +0100, Jacopo Mondi wrote:\n> On Tue, Nov 09, 2021 at 04:07:43PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-10-28 12:15:19)\n> > > Before queueing a request to the device, any synchronization fence from\n> > > the Request framebuffers has to be waited on.\n> > >\n> > > Start by moving the fences to the Request, in order to reset the ones\n> > > in the FrameBuffers and then enable all of them one by one.\n> >\n> > Aha, ok so now I'm starting to understand the lifetime of the fence.\n> >\n> > If a Buffer is added with a fence to protect it until it's ready, do we\n> > introduce any kind of race when enabling if the buffer fence has\n> > completed /before/ we enable it?\n> >\n> > I.e. could we end up sitting on a fence waiting for a completion that\n> > has already occured, and then timeout - and cause the request to be\n> > incorrectly cancelled?\n> \n> depends :) For sofware fences I think if a fence gets signalled before\n> we get to poll() on it, I think poll() would immediately return\n> signalling the fence is read.\n> \n> I assume hw fences should work the same, but I can't tell for sure\n\nThat's how the fence API is meant to behave, yes.\n\n> > > When a fence completes, make sure all fences in the Request have been\n> > > waited on or have expired and only after that actually queue the Request\n> > > to the device.\n> > >\n> > > If any fence in the Request has expired cancel the request and do not\n> > > queue it to the device.\n> >\n> > Sounds good...\n> >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 2 +\n> > > src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++-\n> > > 2 files changed, 77 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 5c3c0bc5a8b3..5b98011ac4f6 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -29,6 +29,7 @@ class CameraConfiguration;\n> > > class CameraManager;\n> > > class DeviceEnumerator;\n> > > class DeviceMatch;\n> > > +class Fence;\n> > > class FrameBuffer;\n> > > class MediaDevice;\n> > > class PipelineHandler;\n> > > @@ -79,6 +80,7 @@ private:\n> > > void mediaDeviceDisconnected(MediaDevice *media);\n> > > virtual void disconnect();\n> > >\n> > > + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence);\n> > > void doQueueRequest();\n> > >\n> > > std::vector> mediaDevices_;\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 0f9fec1b618f..39cb680e5386 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -18,6 +18,7 @@\n> > > #include \n> > >\n> > > #include \"libcamera/internal/camera.h\"\n> > > +#include \"libcamera/internal/framebuffer.h\"\n> > > #include \"libcamera/internal/device_enumerator.h\"\n> > > #include \"libcamera/internal/media_device.h\"\n> > > #include \"libcamera/internal/request.h\"\n> > > @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request)\n> > > {\n> > > LIBCAMERA_TRACEPOINT(request_queue, request);\n> > >\n> > > + Request::Private *data = request->_d();\n> > > +\n> > > {\n> > > MutexLocker lock(waitingRequestsMutex_);\n> > > waitingRequests_.push_back(request);\n> > > }\n> > >\n> > > + for (FrameBuffer *buffer : request->pending_) {\n> > > + if (buffer->fence() == -1)\n> > > + continue;\n> > > +\n> > > + /*\n> > > + * Move the Fence into the Request::Private list of\n> > > + * fences. This resets the file descriptor fence in the\n> > > + * FrameBuffer to -1.\n> > > + */\n> > > + data->addFence(std::move(buffer->_d()->fence()));\n> > > +\n> > > + Fence *fence = &data->fences().back();\n> > > + fence->complete.connect(this,\n> > > + [this, request, buffer, fence]() {\n> > > + fenceCompleted(request, buffer, fence);\n> > > + });\n> > > + }\n> >\n> > I can't help but wonder if this should be handled in the Request, and\n> > not by moving the fences to the camera data....\n> \n> They get moved to Request::Private\n> \n> Request::Private *data = request->_d();\n> \n> for (buffer : request->pending)\n> data->addFence(std::move(buffer->_d()->fence()));\n> \n> > The request could signal that it is ready for consumption, without\n> > having to move the ownership of the fence from the buffer to the\n> > camera...\n> \n> However I got your point, the Request could be auto-counting, ie we\n> connect the Fence::complete signal to a slot in Request::Private and\n> basically move the fence counting implemented in\n> PipelineHandler::fenceCompleted() there. I initially wanted this but I\n> got discouraged by the need to have the Request then queue itself when\n> it's ready. Now that I look at it again, we could add to\n> Request::Private a 'done' (or 'ready' or whatever) signal to which we\n> connect a slot in PipelineHandler. This would also simplify the\n> Request::Private::fence*() interface that can be made private to\n> Request::Private.\n> \n> > Then the PipelineHandler wouldn't have to be digging around in the\n> > internals of the Request buffers.\n> \n> I'll experiment with that, but please note that fences will anyway\n> have to be moved to Request::Private here anyhow, as doing so at\n> Request::addBuffer() time would invalidate the fence as seen from\n> application much earlier than Camera::queueRequest() time, something I\n> would rather not do.\n> \n> > Of course it already has access to those, but I'm not 100% convinced,\n> > this is a good reason to dig in there, and not expose an \"I am ready\"\n> > signal from the request. But maybe that adds more layering on the requst\n> > that is undesirable?\n> \n> Let's see how it looks like. I didn't want the request to call\n> PipelineHandler::doQueueRequest() when it's ready, but maybe with a\n> Signal it gets nicer.\n> \n> > That's a fair bit of redesign though - so if you say \"No, it's better\n> > this way\" I'll just move on ;-)\n> \n> Ouch, I already acknoleded you comment. Can I take it back and say\n> \"No, it's better this way\" ? :D\n>\n> > > +\n> > > + /* If no fences to wait on, we can queue the request immediately. */\n> > > + if (!data->pendingFences()) {\n> > > + doQueueRequest();\n> > > +\n> > > + return;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Now that we have added all fences, enable them one by one.\n> > > + * Enabling fences while adding them to the Request would race on the\n> > > + * number of pending fences.\n> > > + */\n> > > + for (Fence &fence : data->fences())\n> > > + fence.enable();\n> > > +}\n> > > +\n> > > +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer,\n> > > + Fence *fence)\n> > > +{\n> > > + Request::Private *data = request->_d();\n> > > +\n> > > + if (fence->expired()) {\n> > > + /*\n> > > + * Move the fence back to the framebuffer, so that it doesn't\n> > > + * get closed and the file descriptor number is made\n> > > + * available again to applications.\n> > > + */\n> > > + FrameBuffer::Private *bufferData = buffer->_d();\n> > > + bufferData->fence() = std::move(*fence);\n> > > +\n> > > + data->fenceExpired();\n> > > + } else {\n> > > + data->fenceCompleted();\n> > > + }\n> > > +\n> > > + if (data->pendingFences())\n> > > + return;\n> > > +\n> > > + data->clearFences();\n> > > doQueueRequest();\n> > > }\n> > >\n> > > @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest()\n> > > return;\n> > >\n> > > request = waitingRequests_.front();\n> > > + if (request->_d()->pendingFences())\n> > > + return;\n> > > +\n> > > waitingRequests_.pop_front();\n> > > }\n> > >\n> > > - /* Queue Request to the pipeline handler. */\n> > > Camera *camera = request->_d()->camera();\n> > > Camera::Private *camData = camera->_d();\n> > >\n> > > request->sequence_ = camData->requestSequence_++;\n> > > - camData->queuedRequests_.push_back(request);\n> > >\n> > > + /* Cancel the request if one of the fences has failed. */\n> > > + if (request->_d()->expiredFences()) {\n> > > + request->cancel();\n> > > + completeRequest(request);\n> > > +\n> > > + doQueueRequest();\n> >\n> > This makes me really think this would be better handled in a loop rather\n> > than recursion... :-S\n> \n> Still not convinced, mostly for a style point of view of having one\n> more indentation layer.\n\nYou can split the function in two, doQueueRequest() and\ndoQueueRequests(), with the latter implementing the loop and calling the\nformer.\n\n> > > +\n> > > + return;\n> > > + }\n> > > +\n> > > + /* Queue Request to the pipeline handler. */\n> > > + camData->queuedRequests_.push_back(request);\n> > > int ret = queueRequestDevice(camera, request);\n> > > if (ret) {\n> > > request->cancel();\n> > > completeRequest(request);\n> > > }\n> > >\n> > > - /* Try to queue the next Request in the queue, if ready. */\n> > > doQueueRequest();\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 E5040BDB1C\n\tfor ;\n\tFri, 12 Nov 2021 16:03:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F6676036B;\n\tFri, 12 Nov 2021 17:03:22 +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 31AB36032C\n\tfor ;\n\tFri, 12 Nov 2021 17:03:20 +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 BA5EE268;\n\tFri, 12 Nov 2021 17:03:19 +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=\"Idnme5oN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636732999;\n\tbh=WHwCqNyoo4Zp/70ZlMRlUzL/h3mI4XZGzbpKku8dXqU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Idnme5oN6/WpYD0c+QwllP5zZgK85MQOo68yUtoxfUrqFf8l1Cfe+piy3eI3eb9Xf\n\tJvZwqkWtuMVsKzQJOcnj0fhDe3uLn0jqyrGOo/7GQMS8aTGy1OqjXfw4jNjK97DDaw\n\tIb9CIgvJgvZr7oKa8P0gPyr9Uu8JSqblGGsdF4i8=","Date":"Fri, 12 Nov 2021 18:02:57 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-10-jacopo@jmondi.org>\n\t<163647406399.1606134.9236088055713064085@Monstersaurus>\n\t<20211109175724.s62teskaxf6o3rxv@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211109175724.s62teskaxf6o3rxv@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler:\n\tHandle 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\" "}}]