[{"id":20758,"web_url":"https://patchwork.libcamera.org/comment/20758/","msgid":"<163646877135.1606134.15749875474658569164@Monstersaurus>","date":"2021-11-09T14:39:31","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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:17)\n> In order to prepare to handle synchronization fences at Request\n> queueing time, split the PipelineHandler::queueRequest() function in\n> two, by creating a list of waiting requests and introducing a new\n> doQueueDevice() function that queues Requests to the device in the\n> order the pipeline has received them.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/pipeline_handler.h | 7 ++++\n> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> 2 files changed, 39 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 41cba44d990f..afb7990ea21b 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -7,7 +7,9 @@\n> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> \n> +#include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -76,9 +78,14 @@ private:\n> void mediaDeviceDisconnected(MediaDevice *media);\n> virtual void disconnect();\n> \n> + void doQueueRequest();\n> +\n> std::vector> mediaDevices_;\n> std::vector> cameras_;\n> \n> + std::mutex waitingRequestsMutex_;\n> + std::list waitingRequests_;\n> +\n> const char *name_;\n> \n> friend class PipelineHandlerFactory;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index cada864687ff..38edd00cebad 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> LIBCAMERA_TRACEPOINT(request_queue, request);\n\nShould we add a new TRACEPOINT for when the request gets queued to the\ndevice now?\n\n> \n> + {\n> + MutexLocker lock(waitingRequestsMutex_);\n> + waitingRequests_.push_back(request);\n> + }\n> +\n> + doQueueRequest();\n> +}\n> +\n> +/**\n> + * \\brief Queue a request to the device\n> + */\n> +void PipelineHandler::doQueueRequest()\n> +{\n> + Request *request = nullptr;\n> + {\n> + MutexLocker lock(waitingRequestsMutex_);\n> +\n> + if (!waitingRequests_.size())\n> + return;\n> +\n> + request = waitingRequests_.front();\n> + waitingRequests_.pop_front();\n> + }\n> +\n> + /* Queue Request to the pipeline handler. */\n> Camera *camera = request->_d()->camera();\n> - Camera::Private *data = camera->_d();\n> - data->queuedRequests_.push_back(request);\n> + Camera::Private *camData = camera->_d();\n\nIs this rename data -> camData required? it would simplify the diff if\nit was done separately. Otherwise - perhaps mention in the\ncommit-message.\n\n\n> \n> - request->sequence_ = data->requestSequence_++;\n> + request->sequence_ = camData->requestSequence_++;\n> + camData->queuedRequests_.push_back(request);\n> \n> int ret = queueRequestDevice(camera, request);\n\nSo we have a queue before we queue now... ;-)\n\nWe probably need to update/create some new block diagrams for the\nPipeline Handler documentation to show how the internal queuing works ...\n\nSo I think this is now\n\tqueueRequest(); // Add to the queue\n\t-> doQueueRequest(); // See if we're allowed to queue\n\t -> queueRequestDevice(); // Add to the kernel/hw queue ...\n\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\nHrm... I get a little scared/pessimistic when I see recursion. Is this\nbetter handled as a loop some how?\n\n\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 62F51BF415\n\tfor ;\n\tTue, 9 Nov 2021 14:39:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B55666034E;\n\tTue, 9 Nov 2021 15:39:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9D5B600BF\n\tfor ;\n\tTue, 9 Nov 2021 15:39:33 +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 7445ADEE;\n\tTue, 9 Nov 2021 15:39:33 +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=\"SNveLFCw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636468773;\n\tbh=2xZa9Nuc9pOxfEQPHMrxk2d8L2OrJRgjkcR219jRUQ4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=SNveLFCwPQjgJCTeEry4Z7lz5EXbIcJ9reza8eHgGM8Q7dgLUZc9SZaMdsLTX0cOb\n\tGMFxQLqdcIEFnZex3h5oYfkPgrD84FWcS38/kOqBBOJ4hktsCjW+dY88NM5/IDtujA\n\tfuXMVcZUYuEdsxf4DkKitrDRr5GGcyjNNqEiMQRo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-8-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 14:39:31 +0000","Message-ID":"<163646877135.1606134.15749875474658569164@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20769,"web_url":"https://patchwork.libcamera.org/comment/20769/","msgid":"<20211109174254.6nearm7nfhz7b3vt@uno.localdomain>","date":"2021-11-09T17:42:54","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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 02:39:31PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-10-28 12:15:17)\n> > In order to prepare to handle synchronization fences at Request\n> > queueing time, split the PipelineHandler::queueRequest() function in\n> > two, by creating a list of waiting requests and introducing a new\n> > doQueueDevice() function that queues Requests to the device in the\n> > order the pipeline has received them.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 7 ++++\n> > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> > 2 files changed, 39 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 41cba44d990f..afb7990ea21b 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -7,7 +7,9 @@\n> > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> >\n> > +#include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -76,9 +78,14 @@ private:\n> > void mediaDeviceDisconnected(MediaDevice *media);\n> > virtual void disconnect();\n> >\n> > + void doQueueRequest();\n> > +\n> > std::vector> mediaDevices_;\n> > std::vector> cameras_;\n> >\n> > + std::mutex waitingRequestsMutex_;\n> > + std::list waitingRequests_;\n> > +\n> > const char *name_;\n> >\n> > friend class PipelineHandlerFactory;\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index cada864687ff..38edd00cebad 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> > {\n> > LIBCAMERA_TRACEPOINT(request_queue, request);\n>\n> Should we add a new TRACEPOINT for when the request gets queued to the\n> device now?\n>\n\nDunno, this seems internal stuff, while I assumed tracepoints are\nmeant to track public API interactions ? Am I mistaken ?\n\n> >\n> > + {\n> > + MutexLocker lock(waitingRequestsMutex_);\n> > + waitingRequests_.push_back(request);\n> > + }\n> > +\n> > + doQueueRequest();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Queue a request to the device\n> > + */\n> > +void PipelineHandler::doQueueRequest()\n> > +{\n> > + Request *request = nullptr;\n> > + {\n> > + MutexLocker lock(waitingRequestsMutex_);\n> > +\n> > + if (!waitingRequests_.size())\n> > + return;\n> > +\n> > + request = waitingRequests_.front();\n> > + waitingRequests_.pop_front();\n> > + }\n> > +\n> > + /* Queue Request to the pipeline handler. */\n> > Camera *camera = request->_d()->camera();\n> > - Camera::Private *data = camera->_d();\n> > - data->queuedRequests_.push_back(request);\n> > + Camera::Private *camData = camera->_d();\n>\n> Is this rename data -> camData required? it would simplify the diff if\n> it was done separately. Otherwise - perhaps mention in the\n> commit-message.\n\nIt's probably a leftover, I'll keep using 'data'.\n\n>\n>\n> >\n> > - request->sequence_ = data->requestSequence_++;\n> > + request->sequence_ = camData->requestSequence_++;\n> > + camData->queuedRequests_.push_back(request);\n> >\n> > int ret = queueRequestDevice(camera, request);\n>\n> So we have a queue before we queue now... ;-)\n>\n> We probably need to update/create some new block diagrams for the\n> Pipeline Handler documentation to show how the internal queuing works ...\n>\n> So I think this is now\n> \tqueueRequest(); // Add to the queue\n> \t-> doQueueRequest(); // See if we're allowed to queue\n> \t -> queueRequestDevice(); // Add to the kernel/hw queue ...\n>\n\nit's more like\n\n queueRequest();\n if (ready)\n doQueueRequest()\n queueRequestDevice()\n else\n waitOnFences()\n if (fencesDone)\n doQueueRequest()\n queueRequestDevice()\n\n\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> Hrm... I get a little scared/pessimistic when I see recursion. Is this\n> better handled as a loop some how?\n>\n\nWhy so ? It's one more level of indendation and I'm not sure what\nbenefits it would bring :)\n>\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 2B985BDB1C\n\tfor ;\n\tTue, 9 Nov 2021 17:42:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 687E06034E;\n\tTue, 9 Nov 2021 18:42:02 +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 45248600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:42:01 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id C28646000E;\n\tTue, 9 Nov 2021 17:42:00 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 18:42:54 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211109174254.6nearm7nfhz7b3vt@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>\n\t<163646877135.1606134.15749875474658569164@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163646877135.1606134.15749875474658569164@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20784,"web_url":"https://patchwork.libcamera.org/comment/20784/","msgid":"<163653769093.1896795.2046669211809373213@Monstersaurus>","date":"2021-11-10T09:48:10","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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:42:54)\n> Hi Kieran\n> \n> On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-10-28 12:15:17)\n> > > In order to prepare to handle synchronization fences at Request\n> > > queueing time, split the PipelineHandler::queueRequest() function in\n> > > two, by creating a list of waiting requests and introducing a new\n> > > doQueueDevice() function that queues Requests to the device in the\n> > > order the pipeline has received them.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 7 ++++\n> > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> > > 2 files changed, 39 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 41cba44d990f..afb7990ea21b 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -7,7 +7,9 @@\n> > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > >\n> > > +#include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -76,9 +78,14 @@ private:\n> > > void mediaDeviceDisconnected(MediaDevice *media);\n> > > virtual void disconnect();\n> > >\n> > > + void doQueueRequest();\n> > > +\n> > > std::vector> mediaDevices_;\n> > > std::vector> cameras_;\n> > >\n> > > + std::mutex waitingRequestsMutex_;\n> > > + std::list waitingRequests_;\n> > > +\n> > > const char *name_;\n> > >\n> > > friend class PipelineHandlerFactory;\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index cada864687ff..38edd00cebad 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -10,6 +10,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> > > {\n> > > LIBCAMERA_TRACEPOINT(request_queue, request);\n> >\n> > Should we add a new TRACEPOINT for when the request gets queued to the\n> > device now?\n> >\n> \n> Dunno, this seems internal stuff, while I assumed tracepoints are\n> meant to track public API interactions ? Am I mistaken ?\n\nI would expect them to track any internal actions and events that a\ntracepoint helps to identify ...\n\nPerhaps one for Paul to chime in on...\n\nThe Tracepoint now tracks when the request is queued, but not when it\ngets sent to the hardware so I would expect that to be useful\ninformation for tracking the flow with the tracing mechanisms.\n\n> \n> > >\n> > > + {\n> > > + MutexLocker lock(waitingRequestsMutex_);\n> > > + waitingRequests_.push_back(request);\n> > > + }\n> > > +\n> > > + doQueueRequest();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Queue a request to the device\n> > > + */\n> > > +void PipelineHandler::doQueueRequest()\n> > > +{\n> > > + Request *request = nullptr;\n> > > + {\n> > > + MutexLocker lock(waitingRequestsMutex_);\n> > > +\n> > > + if (!waitingRequests_.size())\n> > > + return;\n> > > +\n> > > + request = waitingRequests_.front();\n> > > + waitingRequests_.pop_front();\n> > > + }\n> > > +\n> > > + /* Queue Request to the pipeline handler. */\n> > > Camera *camera = request->_d()->camera();\n> > > - Camera::Private *data = camera->_d();\n> > > - data->queuedRequests_.push_back(request);\n> > > + Camera::Private *camData = camera->_d();\n> >\n> > Is this rename data -> camData required? it would simplify the diff if\n> > it was done separately. Otherwise - perhaps mention in the\n> > commit-message.\n> \n> It's probably a leftover, I'll keep using 'data'.\n> \n> >\n> >\n> > >\n> > > - request->sequence_ = data->requestSequence_++;\n> > > + request->sequence_ = camData->requestSequence_++;\n> > > + camData->queuedRequests_.push_back(request);\n> > >\n> > > int ret = queueRequestDevice(camera, request);\n> >\n> > So we have a queue before we queue now... ;-)\n> >\n> > We probably need to update/create some new block diagrams for the\n> > Pipeline Handler documentation to show how the internal queuing works ...\n> >\n> > So I think this is now\n> > queueRequest(); // Add to the queue\n> > -> doQueueRequest(); // See if we're allowed to queue\n> > -> queueRequestDevice(); // Add to the kernel/hw queue ...\n> >\n> \n> it's more like\n> \n> queueRequest();\n> if (ready)\n> doQueueRequest()\n> queueRequestDevice()\n> else\n> waitOnFences()\n> if (fencesDone)\n> doQueueRequest()\n> queueRequestDevice()\n> \n> \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> > Hrm... I get a little scared/pessimistic when I see recursion. Is this\n> > better handled as a loop some how?\n> >\n> \n> Why so ? It's one more level of indendation and I'm not sure what\n> benefits it would bring :)\n\nOk, lets keep it your way then. I'd refactor it - but it's your code ;-)\n--\nKieran\n\n\n> >\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 9874FBF415\n\tfor ;\n\tWed, 10 Nov 2021 09:48:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0FB96035A;\n\tWed, 10 Nov 2021 10:48:15 +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 AD1B960128\n\tfor ;\n\tWed, 10 Nov 2021 10:48:13 +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 4265ED8B;\n\tWed, 10 Nov 2021 10:48:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hoGSkKfi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636537693;\n\tbh=iglO8RujdToQY3Wd7Vr7HnsY8qxG2Tsyf0sQa+2kCjA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hoGSkKfizPeessXUQ02jYLqAAAx/gG2kTJGiWocTTcLdaAETpusahbV03N+sQkUTn\n\tweZNimdrdhERE9WwCT7R6/vKv0sk/+axi4I/O1+IoxFWSs3V8kr9UKUVOraKYZpwpV\n\tKLjeg66RqGSpRGcWR0mXTvdTHrSI3e0nuhg6V/QE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211109174254.6nearm7nfhz7b3vt@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>\n\t<163646877135.1606134.15749875474658569164@Monstersaurus>\n\t<20211109174254.6nearm7nfhz7b3vt@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ,\n\tPaul Elder ","Date":"Wed, 10 Nov 2021 09:48:10 +0000","Message-ID":"<163653769093.1896795.2046669211809373213@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20786,"web_url":"https://patchwork.libcamera.org/comment/20786/","msgid":"<20211110100233.GB4088@pyrite.rasen.tech>","date":"2021-11-10T10:02:33","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Nov 10, 2021 at 09:48:10AM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-11-09 17:42:54)\n> > Hi Kieran\n> > \n> > On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi (2021-10-28 12:15:17)\n> > > > In order to prepare to handle synchronization fences at Request\n> > > > queueing time, split the PipelineHandler::queueRequest() function in\n> > > > two, by creating a list of waiting requests and introducing a new\n> > > > doQueueDevice() function that queues Requests to the device in the\n> > > > order the pipeline has received them.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/internal/pipeline_handler.h | 7 ++++\n> > > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> > > > 2 files changed, 39 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > index 41cba44d990f..afb7990ea21b 100644\n> > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > @@ -7,7 +7,9 @@\n> > > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > > >\n> > > > +#include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -76,9 +78,14 @@ private:\n> > > > void mediaDeviceDisconnected(MediaDevice *media);\n> > > > virtual void disconnect();\n> > > >\n> > > > + void doQueueRequest();\n> > > > +\n> > > > std::vector> mediaDevices_;\n> > > > std::vector> cameras_;\n> > > >\n> > > > + std::mutex waitingRequestsMutex_;\n> > > > + std::list waitingRequests_;\n> > > > +\n> > > > const char *name_;\n> > > >\n> > > > friend class PipelineHandlerFactory;\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > index cada864687ff..38edd00cebad 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -10,6 +10,7 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \n> > > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> > > > {\n> > > > LIBCAMERA_TRACEPOINT(request_queue, request);\n> > >\n> > > Should we add a new TRACEPOINT for when the request gets queued to the\n> > > device now?\n> > >\n> > \n> > Dunno, this seems internal stuff, while I assumed tracepoints are\n> > meant to track public API interactions ? Am I mistaken ?\n> \n> I would expect them to track any internal actions and events that a\n> tracepoint helps to identify ...\n> \n> Perhaps one for Paul to chime in on...\n\nChiming in here.\n\nTracepoints can go anywhere. I'd put them in places where we'd want\ninformation on the flow, but that are hit so frequently that putting\nthem in debug messages isn't practical.\n\n(Speaking of which, I think a bunch of per-request IPA messages should\nbe moved to tracepoints)\n\n> \n> The Tracepoint now tracks when the request is queued, but not when it\n> gets sent to the hardware so I would expect that to be useful\n> information for tracking the flow with the tracing mechanisms.\n\nYeah it definitely would.\n\n\nPaul\n\n> \n> > \n> > > >\n> > > > + {\n> > > > + MutexLocker lock(waitingRequestsMutex_);\n> > > > + waitingRequests_.push_back(request);\n> > > > + }\n> > > > +\n> > > > + doQueueRequest();\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Queue a request to the device\n> > > > + */\n> > > > +void PipelineHandler::doQueueRequest()\n> > > > +{\n> > > > + Request *request = nullptr;\n> > > > + {\n> > > > + MutexLocker lock(waitingRequestsMutex_);\n> > > > +\n> > > > + if (!waitingRequests_.size())\n> > > > + return;\n> > > > +\n> > > > + request = waitingRequests_.front();\n> > > > + waitingRequests_.pop_front();\n> > > > + }\n> > > > +\n> > > > + /* Queue Request to the pipeline handler. */\n> > > > Camera *camera = request->_d()->camera();\n> > > > - Camera::Private *data = camera->_d();\n> > > > - data->queuedRequests_.push_back(request);\n> > > > + Camera::Private *camData = camera->_d();\n> > >\n> > > Is this rename data -> camData required? it would simplify the diff if\n> > > it was done separately. Otherwise - perhaps mention in the\n> > > commit-message.\n> > \n> > It's probably a leftover, I'll keep using 'data'.\n> > \n> > >\n> > >\n> > > >\n> > > > - request->sequence_ = data->requestSequence_++;\n> > > > + request->sequence_ = camData->requestSequence_++;\n> > > > + camData->queuedRequests_.push_back(request);\n> > > >\n> > > > int ret = queueRequestDevice(camera, request);\n> > >\n> > > So we have a queue before we queue now... ;-)\n> > >\n> > > We probably need to update/create some new block diagrams for the\n> > > Pipeline Handler documentation to show how the internal queuing works ...\n> > >\n> > > So I think this is now\n> > > queueRequest(); // Add to the queue\n> > > -> doQueueRequest(); // See if we're allowed to queue\n> > > -> queueRequestDevice(); // Add to the kernel/hw queue ...\n> > >\n> > \n> > it's more like\n> > \n> > queueRequest();\n> > if (ready)\n> > doQueueRequest()\n> > queueRequestDevice()\n> > else\n> > waitOnFences()\n> > if (fencesDone)\n> > doQueueRequest()\n> > queueRequestDevice()\n> > \n> > \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> > > Hrm... I get a little scared/pessimistic when I see recursion. Is this\n> > > better handled as a loop some how?\n> > >\n> > \n> > Why so ? It's one more level of indendation and I'm not sure what\n> > benefits it would bring :)\n> \n> Ok, lets keep it your way then. I'd refactor it - but it's your code ;-)\n> --\n> Kieran\n> \n> \n> > >\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 E5D8EBF415\n\tfor ;\n\tWed, 10 Nov 2021 10:02:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 547D16035A;\n\tWed, 10 Nov 2021 11:02:43 +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 1523F60128\n\tfor ;\n\tWed, 10 Nov 2021 11:02:42 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33A7DD8B;\n\tWed, 10 Nov 2021 11:02:39 +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=\"wTtua7IP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636538561;\n\tbh=NUUuOq4di14DOKwxeILMrL7hKrht6O4i2h2JMYdQ4KY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wTtua7IPJubVjoJKT2RJUhFdTddi8Y6mVHl8Ev4F+U+fIcK2fHMo1ro24dRBS9do9\n\tRb13vh3/Pm+aAF9jrBI1dPSytUViYSPtVmMbli8uZvk9QXJhittgRCrN5qnSsmcJbT\n\t//Jlbqe2WLg/IVGIqLxmTQAhhI2qgeHzK67ulhUQ=","Date":"Wed, 10 Nov 2021 19:02:33 +0900","From":"paul.elder@ideasonboard.com","To":"Kieran Bingham ","Message-ID":"<20211110100233.GB4088@pyrite.rasen.tech>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>\n\t<163646877135.1606134.15749875474658569164@Monstersaurus>\n\t<20211109174254.6nearm7nfhz7b3vt@uno.localdomain>\n\t<163653769093.1896795.2046669211809373213@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<163653769093.1896795.2046669211809373213@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20801,"web_url":"https://patchwork.libcamera.org/comment/20801/","msgid":"","date":"2021-11-10T12:50:01","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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. One question below\n\nOn 10/28/21 4:45 PM, Jacopo Mondi wrote:\n> In order to prepare to handle synchronization fences at Request\n> queueing time, split the PipelineHandler::queueRequest() function in\n> two, by creating a list of waiting requests and introducing a new\n> doQueueDevice() function that queues Requests to the device in the\n> order the pipeline has received them.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/pipeline_handler.h | 7 ++++\n> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> 2 files changed, 39 insertions(+), 3 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 41cba44d990f..afb7990ea21b 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -7,7 +7,9 @@\n> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> \n> +#include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -76,9 +78,14 @@ private:\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> \tvirtual void disconnect();\n> \n> +\tvoid doQueueRequest();\n> +\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n> \n> +\tstd::mutex waitingRequestsMutex_;\n> +\tstd::list waitingRequests_;\n> +\n> \tconst char *name_;\n> \n> \tfriend class PipelineHandlerFactory;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index cada864687ff..38edd00cebad 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> +\t{\n> +\t\tMutexLocker lock(waitingRequestsMutex_);\n> +\t\twaitingRequests_.push_back(request);\n\n\nWhy do we need to lock this? Are there multiple threads accessing \nPipelineHandler::queueRequest()\n\nThe documentation of the function states\n\n\\context This function is called from the CameraManager thread.\n\nwhich I believe is all synchronous at this point.\n\n> +\t}\n> +\n> +\tdoQueueRequest();\n> +}\n> +\n> +/**\n> + * \\brief Queue a request to the device\n> + */\n> +void PipelineHandler::doQueueRequest()\n> +{\n> +\tRequest *request = nullptr;\n> +\t{\n> +\t\tMutexLocker lock(waitingRequestsMutex_);\n> +\n> +\t\tif (!waitingRequests_.size())\n> +\t\t\treturn;\n> +\n> +\t\trequest = waitingRequests_.front();\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 *data = camera->_d();\n> -\tdata->queuedRequests_.push_back(request);\n> +\tCamera::Private *camData = camera->_d();\n> \n> -\trequest->sequence_ = data->requestSequence_++;\n> +\trequest->sequence_ = camData->requestSequence_++;\n> +\tcamData->queuedRequests_.push_back(request);\n> \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> \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 BF5D4BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 12:50:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 219EA6035D;\n\tWed, 10 Nov 2021 13:50:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7AA36033C\n\tfor ;\n\tWed, 10 Nov 2021 13:50:05 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EE5B3556;\n\tWed, 10 Nov 2021 13:50:04 +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=\"FIvwgYte\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636548605;\n\tbh=AyHUm/Ja9eCuxQKjle0+1j5Rkxrnf4e3keDQLpPZVXU=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=FIvwgYteNoOcvq/Gx5ePMoJHBl5W2gqufplqvoiOtEw8p1/EWHGlqLIiPu+yQIqaz\n\t+i/Vk0ZUreKkfKVoPRDXX4wBKQpo0WQ78mNUO7CUS3kuQhNm/axnY+9QCfOB1Tzpe4\n\t3ghzyDOZ4H9nKjzm7IqWL0T3JsYJkqzPUwgQfvyw=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 10 Nov 2021 18:20:01 +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-8-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 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20804,"web_url":"https://patchwork.libcamera.org/comment/20804/","msgid":"<20211110130832.obwj7yt2ognrijwp@uno.localdomain>","date":"2021-11-10T13:08:32","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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 06:20:01PM +0530, Umang Jain wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch. One question below\n>\n> On 10/28/21 4:45 PM, Jacopo Mondi wrote:\n> > In order to prepare to handle synchronization fences at Request\n> > queueing time, split the PipelineHandler::queueRequest() function in\n> > two, by creating a list of waiting requests and introducing a new\n> > doQueueDevice() function that queues Requests to the device in the\n> > order the pipeline has received them.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 7 ++++\n> > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> > 2 files changed, 39 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 41cba44d990f..afb7990ea21b 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -7,7 +7,9 @@\n> > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > +#include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -76,9 +78,14 @@ private:\n> > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > \tvirtual void disconnect();\n> > +\tvoid doQueueRequest();\n> > +\n> > \tstd::vector> mediaDevices_;\n> > \tstd::vector> cameras_;\n> > +\tstd::mutex waitingRequestsMutex_;\n> > +\tstd::list waitingRequests_;\n> > +\n> > \tconst char *name_;\n> > \tfriend class PipelineHandlerFactory;\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index cada864687ff..38edd00cebad 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> > {\n> > \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> > +\t{\n> > +\t\tMutexLocker lock(waitingRequestsMutex_);\n> > +\t\twaitingRequests_.push_back(request);\n>\n>\n> Why do we need to lock this? Are there multiple threads accessing\n> PipelineHandler::queueRequest()\n>\n> The documentation of the function states\n>\n> \\context This function is called from the CameraManager thread.\n>\n> which I believe is all synchronous at this point.\n\nThe waitingRequests_ queue is accessed by the doQueueRequest()\nfunction which might get called in response to fence completion event.\n\nBut now that I've said so, the EventDispatcher that sends fence\ncompletions is the one running in the current thread, which is the\nsame as the CameraManager one, so it might be safe to remove the\nlocking maybe ?\n\nThanks\n j\n\n>\n> > +\t}\n> > +\n> > +\tdoQueueRequest();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Queue a request to the device\n> > + */\n> > +void PipelineHandler::doQueueRequest()\n> > +{\n> > +\tRequest *request = nullptr;\n> > +\t{\n> > +\t\tMutexLocker lock(waitingRequestsMutex_);\n> > +\n> > +\t\tif (!waitingRequests_.size())\n> > +\t\t\treturn;\n> > +\n> > +\t\trequest = waitingRequests_.front();\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 *data = camera->_d();\n> > -\tdata->queuedRequests_.push_back(request);\n> > +\tCamera::Private *camData = camera->_d();\n> > -\trequest->sequence_ = data->requestSequence_++;\n> > +\trequest->sequence_ = camData->requestSequence_++;\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 0CFC2BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 13:07:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 739F66035D;\n\tWed, 10 Nov 2021 14:07:39 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8EF556033C\n\tfor ;\n\tWed, 10 Nov 2021 14:07:38 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 2310DE0009;\n\tWed, 10 Nov 2021 13:07:37 +0000 (UTC)"],"Date":"Wed, 10 Nov 2021 14:08:32 +0100","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20211110130832.obwj7yt2ognrijwp@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-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 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20806,"web_url":"https://patchwork.libcamera.org/comment/20806/","msgid":"","date":"2021-11-10T13:17:01","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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 6:38 PM, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Wed, Nov 10, 2021 at 06:20:01PM +0530, Umang Jain wrote:\n>> Hi Jacopo,\n>>\n>> Thank you for the patch. One question below\n>>\n>> On 10/28/21 4:45 PM, Jacopo Mondi wrote:\n>>> In order to prepare to handle synchronization fences at Request\n>>> queueing time, split the PipelineHandler::queueRequest() function in\n>>> two, by creating a list of waiting requests and introducing a new\n>>> doQueueDevice() function that queues Requests to the device in the\n>>> order the pipeline has received them.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> include/libcamera/internal/pipeline_handler.h | 7 ++++\n>>> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n>>> 2 files changed, 39 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>> index 41cba44d990f..afb7990ea21b 100644\n>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>> @@ -7,7 +7,9 @@\n>>> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n>>> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n>>> +#include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -76,9 +78,14 @@ private:\n>>> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n>>> \tvirtual void disconnect();\n>>> +\tvoid doQueueRequest();\n>>> +\n>>> \tstd::vector> mediaDevices_;\n>>> \tstd::vector> cameras_;\n>>> +\tstd::mutex waitingRequestsMutex_;\n>>> +\tstd::list waitingRequests_;\n>>> +\n>>> \tconst char *name_;\n>>> \tfriend class PipelineHandlerFactory;\n>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>> index cada864687ff..38edd00cebad 100644\n>>> --- a/src/libcamera/pipeline_handler.cpp\n>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>> @@ -10,6 +10,7 @@\n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n>>> {\n>>> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n>>> +\t{\n>>> +\t\tMutexLocker lock(waitingRequestsMutex_);\n>>> +\t\twaitingRequests_.push_back(request);\n>>\n>> Why do we need to lock this? Are there multiple threads accessing\n>> PipelineHandler::queueRequest()\n>>\n>> The documentation of the function states\n>>\n>> \\context This function is called from the CameraManager thread.\n>>\n>> which I believe is all synchronous at this point.\n> The waitingRequests_ queue is accessed by the doQueueRequest()\n> function which might get called in response to fence completion event.\n>\n> But now that I've said so, the EventDispatcher that sends fence\n> completions is the one running in the current thread, which is the\n> same as the CameraManager one, so it might be safe to remove the\n> locking maybe ?\n\n\nThat's what I am contemplating too, the slot when triggered will run in \nthe same thread (which is the camera-manager one). I see no harm in \nremoving the lock but I maybe wrong too. Uptill now, in my limited \nreading around queueRequest and the PH class, I didn't see any separate \nthread angle coming up so... I'll keep an lookout.\n\n>\n> Thanks\n> j\n>\n>>> +\t}\n>>> +\n>>> +\tdoQueueRequest();\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Queue a request to the device\n>>> + */\n>>> +void PipelineHandler::doQueueRequest()\n>>> +{\n>>> +\tRequest *request = nullptr;\n>>> +\t{\n>>> +\t\tMutexLocker lock(waitingRequestsMutex_);\n>>> +\n>>> +\t\tif (!waitingRequests_.size())\n>>> +\t\t\treturn;\n>>> +\n>>> +\t\trequest = waitingRequests_.front();\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 *data = camera->_d();\n>>> -\tdata->queuedRequests_.push_back(request);\n>>> +\tCamera::Private *camData = camera->_d();\n>>> -\trequest->sequence_ = data->requestSequence_++;\n>>> +\trequest->sequence_ = camData->requestSequence_++;\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 76B75BF415\n\tfor ;\n\tWed, 10 Nov 2021 13:17:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1E1A6034E;\n\tWed, 10 Nov 2021 14:17:07 +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 ACAD96033C\n\tfor ;\n\tWed, 10 Nov 2021 14:17:06 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2A1A8B6;\n\tWed, 10 Nov 2021 14:17:05 +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=\"nKSNiC7A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636550226;\n\tbh=EP0ieSCQIHWdBqdaJWXdKGHtetCmwfGYYI66/e+p5tU=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=nKSNiC7AQ29Ha7HLKyrrwQOa9VNmQrlNLNhCAOuu1qf7rIWivnlFCfJMBzTjgod2R\n\t6zBZqlOCFHT+DGBqhFf1b6wWZhzT2UYO98T4yG/p0+XGB5eCxLmsbb5u1ABleOlfy0\n\tI5CqH6DCNjV2mjtIhwY7UXN/vELeFrX87zEWKZM4=","To":"Jacopo Mondi ","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>\n\t\n\t<20211110130832.obwj7yt2ognrijwp@uno.localdomain>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 10 Nov 2021 18:47:01 +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":"<20211110130832.obwj7yt2ognrijwp@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20919,"web_url":"https://patchwork.libcamera.org/comment/20919/","msgid":"","date":"2021-11-12T15:39:48","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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 06:47:01PM +0530, Umang Jain wrote:\n> On 11/10/21 6:38 PM, Jacopo Mondi wrote:\n> > On Wed, Nov 10, 2021 at 06:20:01PM +0530, Umang Jain wrote:\n> >> On 10/28/21 4:45 PM, Jacopo Mondi wrote:\n> >>> In order to prepare to handle synchronization fences at Request\n> >>> queueing time, split the PipelineHandler::queueRequest() function in\n> >>> two, by creating a list of waiting requests and introducing a new\n> >>> doQueueDevice() function that queues Requests to the device in the\n> >>> order the pipeline has received them.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>> ---\n> >>> include/libcamera/internal/pipeline_handler.h | 7 ++++\n> >>> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> >>> 2 files changed, 39 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >>> index 41cba44d990f..afb7990ea21b 100644\n> >>> --- a/include/libcamera/internal/pipeline_handler.h\n> >>> +++ b/include/libcamera/internal/pipeline_handler.h\n> >>> @@ -7,7 +7,9 @@\n> >>> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> >>> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> >>> +#include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -76,9 +78,14 @@ private:\n> >>> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> >>> \tvirtual void disconnect();\n> >>> +\tvoid doQueueRequest();\n> >>> +\n> >>> \tstd::vector> mediaDevices_;\n> >>> \tstd::vector> cameras_;\n> >>> +\tstd::mutex waitingRequestsMutex_;\n> >>> +\tstd::list waitingRequests_;\n> >>> +\n> >>> \tconst char *name_;\n> >>> \tfriend class PipelineHandlerFactory;\n> >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >>> index cada864687ff..38edd00cebad 100644\n> >>> --- a/src/libcamera/pipeline_handler.cpp\n> >>> +++ b/src/libcamera/pipeline_handler.cpp\n> >>> @@ -10,6 +10,7 @@\n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> >>> {\n> >>> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> >>> +\t{\n> >>> +\t\tMutexLocker lock(waitingRequestsMutex_);\n> >>> +\t\twaitingRequests_.push_back(request);\n> >>\n> >> Why do we need to lock this? Are there multiple threads accessing\n> >> PipelineHandler::queueRequest()\n> >>\n> >> The documentation of the function states\n> >>\n> >> \\context This function is called from the CameraManager thread.\n> >>\n> >> which I believe is all synchronous at this point.\n> >\n> > The waitingRequests_ queue is accessed by the doQueueRequest()\n> > function which might get called in response to fence completion event.\n> >\n> > But now that I've said so, the EventDispatcher that sends fence\n> > completions is the one running in the current thread, which is the\n> > same as the CameraManager one, so it might be safe to remove the\n> > locking maybe ?\n> \n> That's what I am contemplating too, the slot when triggered will run in \n> the same thread (which is the camera-manager one). I see no harm in \n> removing the lock but I maybe wrong too. Uptill now, in my limited \n> reading around queueRequest and the PH class, I didn't see any separate \n> thread angle coming up so... I'll keep an lookout.\n\nAs mentioned in a separate e-mail in this thread, pipeline handlers are\nsingle-threaded, so there's no need for a lock.\n\n> >>> +\t}\n> >>> +\n> >>> +\tdoQueueRequest();\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Queue a request to the device\n> >>> + */\n> >>> +void PipelineHandler::doQueueRequest()\n> >>> +{\n> >>> +\tRequest *request = nullptr;\n> >>> +\t{\n> >>> +\t\tMutexLocker lock(waitingRequestsMutex_);\n> >>> +\n> >>> +\t\tif (!waitingRequests_.size())\n> >>> +\t\t\treturn;\n> >>> +\n> >>> +\t\trequest = waitingRequests_.front();\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 *data = camera->_d();\n> >>> -\tdata->queuedRequests_.push_back(request);\n> >>> +\tCamera::Private *camData = camera->_d();\n> >>> -\trequest->sequence_ = data->requestSequence_++;\n> >>> +\trequest->sequence_ = camData->requestSequence_++;\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 4AEF2BDB1C\n\tfor ;\n\tFri, 12 Nov 2021 15:40:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA56B6036B;\n\tFri, 12 Nov 2021 16:40:12 +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 0559A6032C\n\tfor ;\n\tFri, 12 Nov 2021 16:40:11 +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 9A3F374C;\n\tFri, 12 Nov 2021 16:40:10 +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=\"HsQRVCbL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636731610;\n\tbh=IonsgVsqGZG0+ufruoJ1Ogbj+CdeGWEWhm+8JRYMuT8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HsQRVCbLB+iK0ma92Vn0EJkKJfgpUPa5hrvmOll9K7znoa5oSFBIUtaVtI9RbkmUb\n\tZmVTmvWq+TzzLt9Yy8jl3NsGpKt4MHJsBehCKInx6TrOfIeiD874ynx9WZuTv0w6pb\n\tq9LrLBnuAIN9qSZ2niiw7aYB7VppKbOR7Ri3NsXc=","Date":"Fri, 12 Nov 2021 17:39:48 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>\n\t\n\t<20211110130832.obwj7yt2ognrijwp@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20920,"web_url":"https://patchwork.libcamera.org/comment/20920/","msgid":"","date":"2021-11-12T15:47:20","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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 07:02:33PM +0900, paul.elder@ideasonboard.com wrote:\n> On Wed, Nov 10, 2021 at 09:48:10AM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-11-09 17:42:54)\n> > > On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote:\n> > > > Quoting Jacopo Mondi (2021-10-28 12:15:17)\n> > > > > In order to prepare to handle synchronization fences at Request\n> > > > > queueing time, split the PipelineHandler::queueRequest() function in\n> > > > > two, by creating a list of waiting requests and introducing a new\n> > > > > doQueueDevice() function that queues Requests to the device in the\n> > > > > order the pipeline has received them.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/internal/pipeline_handler.h | 7 ++++\n> > > > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> > > > > 2 files changed, 39 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > index 41cba44d990f..afb7990ea21b 100644\n> > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > @@ -7,7 +7,9 @@\n> > > > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > > > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -76,9 +78,14 @@ private:\n> > > > > void mediaDeviceDisconnected(MediaDevice *media);\n> > > > > virtual void disconnect();\n> > > > >\n> > > > > + void doQueueRequest();\n> > > > > +\n> > > > > std::vector> mediaDevices_;\n> > > > > std::vector> cameras_;\n> > > > >\n> > > > > + std::mutex waitingRequestsMutex_;\n> > > > > + std::list waitingRequests_;\n> > > > > +\n> > > > > const char *name_;\n> > > > >\n> > > > > friend class PipelineHandlerFactory;\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > index cada864687ff..38edd00cebad 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -10,6 +10,7 @@\n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> > > > > {\n> > > > > LIBCAMERA_TRACEPOINT(request_queue, request);\n> > > >\n> > > > Should we add a new TRACEPOINT for when the request gets queued to the\n> > > > device now?\n> > > \n> > > Dunno, this seems internal stuff, while I assumed tracepoints are\n> > > meant to track public API interactions ? Am I mistaken ?\n> > \n> > I would expect them to track any internal actions and events that a\n> > tracepoint helps to identify ...\n> > \n> > Perhaps one for Paul to chime in on...\n> \n> Chiming in here.\n> \n> Tracepoints can go anywhere. I'd put them in places where we'd want\n> information on the flow, but that are hit so frequently that putting\n> them in debug messages isn't practical.\n> \n> (Speaking of which, I think a bunch of per-request IPA messages should\n> be moved to tracepoints)\n> \n> > The Tracepoint now tracks when the request is queued, but not when it\n> > gets sent to the hardware so I would expect that to be useful\n> > information for tracking the flow with the tracing mechanisms.\n> \n> Yeah it definitely would.\n\nAgreed, knowing when libcamera receives the request and when it passes\nit to the pipeline handler are both useful pieces of information, now\nthat they're separate events.\n\n> > > > > + {\n> > > > > + MutexLocker lock(waitingRequestsMutex_);\n> > > > > + waitingRequests_.push_back(request);\n> > > > > + }\n> > > > > +\n> > > > > + doQueueRequest();\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Queue a request to the device\n> > > > > + */\n> > > > > +void PipelineHandler::doQueueRequest()\n> > > > > +{\n> > > > > + Request *request = nullptr;\n> > > > > + {\n> > > > > + MutexLocker lock(waitingRequestsMutex_);\n> > > > > +\n> > > > > + if (!waitingRequests_.size())\n\nempty()\n\n> > > > > + return;\n> > > > > +\n> > > > > + request = waitingRequests_.front();\n> > > > > + waitingRequests_.pop_front();\n> > > > > + }\n> > > > > +\n> > > > > + /* Queue Request to the pipeline handler. */\n> > > > > Camera *camera = request->_d()->camera();\n> > > > > - Camera::Private *data = camera->_d();\n> > > > > - data->queuedRequests_.push_back(request);\n> > > > > + Camera::Private *camData = camera->_d();\n> > > >\n> > > > Is this rename data -> camData required? it would simplify the diff if\n> > > > it was done separately. Otherwise - perhaps mention in the\n> > > > commit-message.\n> > > \n> > > It's probably a leftover, I'll keep using 'data'.\n> > > \n> > > > >\n> > > > > - request->sequence_ = data->requestSequence_++;\n> > > > > + request->sequence_ = camData->requestSequence_++;\n> > > > > + camData->queuedRequests_.push_back(request);\n> > > > >\n> > > > > int ret = queueRequestDevice(camera, request);\n> > > >\n> > > > So we have a queue before we queue now... ;-)\n> > > >\n> > > > We probably need to update/create some new block diagrams for the\n> > > > Pipeline Handler documentation to show how the internal queuing works ...\n> > > >\n> > > > So I think this is now\n> > > > queueRequest(); // Add to the queue\n> > > > -> doQueueRequest(); // See if we're allowed to queue\n> > > > -> queueRequestDevice(); // Add to the kernel/hw queue ...\n> > > >\n> > > \n> > > it's more like\n> > > \n> > > queueRequest();\n> > > if (ready)\n> > > doQueueRequest()\n> > > queueRequestDevice()\n> > > else\n> > > waitOnFences()\n> > > if (fencesDone)\n> > > doQueueRequest()\n> > > queueRequestDevice()\n> > > \n> > > \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> > > > Hrm... I get a little scared/pessimistic when I see recursion. Is this\n> > > > better handled as a loop some how?\n\nI'd like a loop better too, it's easier to understand the stop\ncondition.\n\n> > > Why so ? It's one more level of indendation and I'm not sure what\n> > > benefits it would bring :)\n> > \n> > Ok, lets keep it your way then. I'd refactor it - but it's your code ;-)\n> > \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 BF35DBF415\n\tfor ;\n\tFri, 12 Nov 2021 15:47:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FFF36036E;\n\tFri, 12 Nov 2021 16:47:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADCD76032C\n\tfor ;\n\tFri, 12 Nov 2021 16:47:42 +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 EED5074C;\n\tFri, 12 Nov 2021 16:47:41 +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=\"GmFcjXee\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636732062;\n\tbh=5jbyT2FZRYWKccqZzpnsv5SpOSiznbTZbbk0kNQgZR8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GmFcjXeeK3bW7SA701j7YzLPAzDdErJsCKP++u3KtYe7/a0USztjoI1MhqRPEsCmE\n\tTKPWs5PzZZwsJ86w05bZlExrQUIeVfCk4KFcEqQFreUGuP9c7FawOtMazSqOMd3xW9\n\tuhSCiQs0Pf4iUtDs2rqhuXUY5L3xM329KArJo8lY=","Date":"Fri, 12 Nov 2021 17:47:20 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>\n\t<163646877135.1606134.15749875474658569164@Monstersaurus>\n\t<20211109174254.6nearm7nfhz7b3vt@uno.localdomain>\n\t<163653769093.1896795.2046669211809373213@Monstersaurus>\n\t<20211110100233.GB4088@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211110100233.GB4088@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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":20921,"web_url":"https://patchwork.libcamera.org/comment/20921/","msgid":"","date":"2021-11-12T15:53:46","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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\nOne additional comment below.\n\nOn Thu, Oct 28, 2021 at 01:15:17PM +0200, Jacopo Mondi wrote:\n> In order to prepare to handle synchronization fences at Request\n> queueing time, split the PipelineHandler::queueRequest() function in\n> two, by creating a list of waiting requests and introducing a new\n> doQueueDevice() function that queues Requests to the device in the\n> order the pipeline has received them.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/pipeline_handler.h | 7 ++++\n> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++--\n> 2 files changed, 39 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 41cba44d990f..afb7990ea21b 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -7,7 +7,9 @@\n> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__\n> \n> +#include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -76,9 +78,14 @@ private:\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> \tvirtual void disconnect();\n> \n> +\tvoid doQueueRequest();\n> +\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n> \n> +\tstd::mutex waitingRequestsMutex_;\n> +\tstd::list waitingRequests_;\n\nShould this be a std::queue ?\n\n> +\n> \tconst char *name_;\n> \n> \tfriend class PipelineHandlerFactory;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index cada864687ff..38edd00cebad 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> +\t{\n> +\t\tMutexLocker lock(waitingRequestsMutex_);\n> +\t\twaitingRequests_.push_back(request);\n> +\t}\n> +\n> +\tdoQueueRequest();\n> +}\n> +\n> +/**\n> + * \\brief Queue a request to the device\n> + */\n> +void PipelineHandler::doQueueRequest()\n> +{\n> +\tRequest *request = nullptr;\n> +\t{\n> +\t\tMutexLocker lock(waitingRequestsMutex_);\n> +\n> +\t\tif (!waitingRequests_.size())\n> +\t\t\treturn;\n> +\n> +\t\trequest = waitingRequests_.front();\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 *data = camera->_d();\n> -\tdata->queuedRequests_.push_back(request);\n> +\tCamera::Private *camData = camera->_d();\n> \n> -\trequest->sequence_ = data->requestSequence_++;\n> +\trequest->sequence_ = camData->requestSequence_++;\n> +\tcamData->queuedRequests_.push_back(request);\n> \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> \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 6CDECBDB1C\n\tfor ;\n\tFri, 12 Nov 2021 15:54:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A92866036B;\n\tFri, 12 Nov 2021 16:54:09 +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 7769D6032C\n\tfor ;\n\tFri, 12 Nov 2021 16:54:08 +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 0A36974C;\n\tFri, 12 Nov 2021 16:54:08 +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=\"PZnXrl+6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636732448;\n\tbh=8/MxxXpmjKuA4QutOc3XFvMo4IsD5ePL5x1Y5Gq/RVA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PZnXrl+6S802mv13/TXZrxaIznH9/kF7om2G+a+w0sfAGw4l419Enir5ArI62TtN2\n\tzO2K7gyQafCgl2G3EXBpjU73GNC8k/ziytUIW9wOC0FjjMwMfS3HkWhp4BCgyHAL3h\n\tuhxEkJmQ5sLhI9AuK42qT5WwL1GUawS0W8tLmKtA=","Date":"Fri, 12 Nov 2021 17:53:46 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211028111520.256612-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler:\n\tSplit request queueing","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\" "}}]