[{"id":21970,"web_url":"https://patchwork.libcamera.org/comment/21970/","msgid":"<20220107145818.glzz4u67irmnge7s@uno.localdomain>","date":"2022-01-07T14:58:18","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Jan 07, 2022 at 12:55:05PM +0000, Kieran Bingham wrote:\n> Provide a call allowing requests to be prepared and associated with the\n> pipeline handler after being constructed by the camera.\n>\n> This provides an opportunity for the Pipeline Handler to connect any\n> signals it may be interested in receiveing for the request such as\n> getting notifications when the request is ready for processing when\n> using a fence.\n>\n> While here, update the existing usage of the d pointer in\n> Camera::createRequest() to match the style of other functions.\n>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nNice, this sounds better than disconnecting after the signal has been\nemitted.\n\n> ---\n>  include/libcamera/internal/pipeline_handler.h |  1 +\n>  src/libcamera/camera.cpp                      | 14 ++++++++++---\n>  src/libcamera/pipeline_handler.cpp            | 20 ++++++++++++++++---\n>  3 files changed, 29 insertions(+), 6 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index e5b8ffb4db3d..ef7e1390560f 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -59,6 +59,7 @@ public:\n>  \tvoid stop(Camera *camera);\n>  \tbool hasPendingRequests(const Camera *camera) const;\n>\n> +\tvoid prepareRequest(Request *request);\n>  \tvoid queueRequest(Request *request);\n>\n>  \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 86d84ac0a77d..1a00b34d9d9d 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1074,12 +1074,20 @@ int Camera::configure(CameraConfiguration *config)\n>   */\n>  std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)\n>  {\n> -\tint ret = _d()->isAccessAllowed(Private::CameraConfigured,\n> -\t\t\t\t\tPrivate::CameraRunning);\n> +\tPrivate *const d = _d();\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraConfigured,\n> +\t\t\t\t     Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn nullptr;\n>\n> -\treturn std::make_unique<Request>(this, cookie);\n> +\tstd::unique_ptr<Request> request = std::make_unique<Request>(this, cookie);\n> +\n> +\t/* Associate the request with the pipeline handler */\n\nIt doesn't really associate it, but rather initialize it\n\nI would also be tempted to suggest to rename the function to\nPipelineHandler::createRequest() to make sure it is immediately clear\nwhere the function is meant to be called from and when, as all the\nother 'prepare' actions happen at request queue time, not at request\ncreation time.\n\n> +\tif (request)\n> +\t\td->pipe_->prepareRequest(request.get());\n> +\n> +\treturn request;\n>  }\n>\n>  /**\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 03e4b9e66aa6..18b59ef27fb6 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -337,6 +337,23 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n>  \treturn !camera->_d()->queuedRequests_.empty();\n>  }\n>\n> +/**\n> + * \\fn PipelineHandler::prepareRequest()\n> + * \\brief Prepare a request for use by the Pipeline Handler\n> + * \\param[in] request The request to prepare\n> + */\n> +void PipelineHandler::prepareRequest(Request *request)\n> +{\n> +\t/*\n> +\t * Connect the request prepared signal to the pipeline handler\n> +\t * to manage fence based preparations allowing the request\n\nI would drop this statement. Fences handling is only potentially one\nthe preparation steps we might want to handle in Request::prepare()\n\n> +\t * to inform us when it is ready to be processed by hardware.\n> +\t */\n> +\trequest->_d()->prepared.connect(this, [this]() {\n> +\t\t\t\t\t\tdoQueueRequests();\n> +\t\t\t\t\t});\n> +}\n> +\n>  /**\n>   * \\fn PipelineHandler::queueRequest()\n>   * \\brief Queue a request\n> @@ -366,9 +383,6 @@ void PipelineHandler::queueRequest(Request *request)\n>\n>  \twaitingRequests_.push(request);\n>\n> -\trequest->_d()->prepared.connect(this, [this]() {\n> -\t\t\t\t\t\tdoQueueRequests();\n> -\t\t\t\t\t});\n>  \trequest->_d()->prepare(300ms);\n>  }\n>\n> --\n> 2.32.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D9025BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  7 Jan 2022 14:57:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C775608E6;\n\tFri,  7 Jan 2022 15:57:21 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CF75360868\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  7 Jan 2022 15:57:19 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 4F81240005;\n\tFri,  7 Jan 2022 14:57:19 +0000 (UTC)"],"Date":"Fri, 7 Jan 2022 15:58:18 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20220107145818.glzz4u67irmnge7s@uno.localdomain>","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220107125506.1477795-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21972,"web_url":"https://patchwork.libcamera.org/comment/21972/","msgid":"<164156806812.1224575.4952431468893409126@Monstersaurus>","date":"2022-01-07T15:07:48","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-01-07 14:58:18)\n> Hi Kieran,\n> \n> On Fri, Jan 07, 2022 at 12:55:05PM +0000, Kieran Bingham wrote:\n> > Provide a call allowing requests to be prepared and associated with the\n> > pipeline handler after being constructed by the camera.\n> >\n> > This provides an opportunity for the Pipeline Handler to connect any\n> > signals it may be interested in receiveing for the request such as\n> > getting notifications when the request is ready for processing when\n> > using a fence.\n> >\n> > While here, update the existing usage of the d pointer in\n> > Camera::createRequest() to match the style of other functions.\n> >\n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> Nice, this sounds better than disconnecting after the signal has been\n> emitted.\n\nThat's what I thought, I couldn't bear to send a patch that disconnected\nthe signal on every request to have it reconnect again.\n\n> \n> > ---\n> >  include/libcamera/internal/pipeline_handler.h |  1 +\n> >  src/libcamera/camera.cpp                      | 14 ++++++++++---\n> >  src/libcamera/pipeline_handler.cpp            | 20 ++++++++++++++++---\n> >  3 files changed, 29 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index e5b8ffb4db3d..ef7e1390560f 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -59,6 +59,7 @@ public:\n> >       void stop(Camera *camera);\n> >       bool hasPendingRequests(const Camera *camera) const;\n> >\n> > +     void prepareRequest(Request *request);\n> >       void queueRequest(Request *request);\n> >\n> >       bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 86d84ac0a77d..1a00b34d9d9d 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1074,12 +1074,20 @@ int Camera::configure(CameraConfiguration *config)\n> >   */\n> >  std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)\n> >  {\n> > -     int ret = _d()->isAccessAllowed(Private::CameraConfigured,\n> > -                                     Private::CameraRunning);\n> > +     Private *const d = _d();\n> > +\n> > +     int ret = d->isAccessAllowed(Private::CameraConfigured,\n> > +                                  Private::CameraRunning);\n> >       if (ret < 0)\n> >               return nullptr;\n> >\n> > -     return std::make_unique<Request>(this, cookie);\n> > +     std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie);\n> > +\n> > +     /* Associate the request with the pipeline handler */\n> \n> It doesn't really associate it, but rather initialize it\n\n\"Intialise the request with the pipeline handler\" doesn't sound right\nthough... ?\n\n> \n> I would also be tempted to suggest to rename the function to\n> PipelineHandler::createRequest() to make sure it is immediately clear\n> where the function is meant to be called from and when, as all the\n\nThat would bother me as the PipelineHandler is not createing the\nrequest...\n\nWhat about \n\tPipelineHandler::registerRequest()\n\nTo state that the request is 'registered' with a single pipeline\nhandler?\n\n> other 'prepare' actions happen at request queue time, not at request\n> creation time.\n> \n> > +     if (request)\n> > +             d->pipe_->prepareRequest(request.get());\n> > +\n> > +     return request;\n> >  }\n> >\n> >  /**\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 03e4b9e66aa6..18b59ef27fb6 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -337,6 +337,23 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> >       return !camera->_d()->queuedRequests_.empty();\n> >  }\n> >\n> > +/**\n> > + * \\fn PipelineHandler::prepareRequest()\n> > + * \\brief Prepare a request for use by the Pipeline Handler\n> > + * \\param[in] request The request to prepare\n> > + */\n> > +void PipelineHandler::prepareRequest(Request *request)\n> > +{\n> > +     /*\n> > +      * Connect the request prepared signal to the pipeline handler\n> > +      * to manage fence based preparations allowing the request\n> \n> I would drop this statement. Fences handling is only potentially one\n> the preparation steps we might want to handle in Request::prepare()\n\nI can drop it, but the reason for having the comment here is to delcare\nwhat this connection is for. This function (which could be renamed to\nregisterRequest()) could do other things when registering the request\ntoo.\n\nIs this better?\n\n\t/*\n\t * Connect the request prepared signal to the pipeline handler\n\t * to notify the pipeline handler when a request is ready to be\n\t * processed.\n\t */\n\n> > +      * to inform us when it is ready to be processed by hardware.\n> > +      */\n> > +     request->_d()->prepared.connect(this, [this]() {\n> > +                                             doQueueRequests();\n> > +                                     });\n> > +}\n> > +\n> >  /**\n> >   * \\fn PipelineHandler::queueRequest()\n> >   * \\brief Queue a request\n> > @@ -366,9 +383,6 @@ void PipelineHandler::queueRequest(Request *request)\n> >\n> >       waitingRequests_.push(request);\n> >\n> > -     request->_d()->prepared.connect(this, [this]() {\n> > -                                             doQueueRequests();\n> > -                                     });\n> >       request->_d()->prepare(300ms);\n> >  }\n> >\n> > --\n> > 2.32.0\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 40F02BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  7 Jan 2022 15:07:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8242E60921;\n\tFri,  7 Jan 2022 16:07:51 +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 C58F560868\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  7 Jan 2022 16:07:50 +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 563FE8D7;\n\tFri,  7 Jan 2022 16:07:50 +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=\"dr79o1aE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641568070;\n\tbh=z+/g/f0a2k2tpTBnfrjvAoJiUTP7+894K6UpjR8vwWs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=dr79o1aEQbW7zRskBSdAcp5bJSlFnwL8EkDUxttr/AaXu2JFS2yMnD09Omft/fpt8\n\tXvRCfm2QeutwfwIO5n57GDEQlicOOU+F2swsyzY4HFPz10iUPHkv9byoeSaoufgb/+\n\tQL1Yvir2+Ur/IKXm9glPGqeL2lUOaQhVNUp4dCT8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220107145818.glzz4u67irmnge7s@uno.localdomain>","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-2-kieran.bingham@ideasonboard.com>\n\t<20220107145818.glzz4u67irmnge7s@uno.localdomain>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Date":"Fri, 07 Jan 2022 15:07:48 +0000","Message-ID":"<164156806812.1224575.4952431468893409126@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21974,"web_url":"https://patchwork.libcamera.org/comment/21974/","msgid":"<CAHW6GYKbhTEx1L2VSxaa5yFEeE=fN0cDRYvCFgZ1Ra6tgG73Tw@mail.gmail.com>","date":"2022-01-07T15:30:49","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nThanks for looking into this. I've tried it on the example in the\noriginal bug report and can confirm that it is fixed and now works\nproperly:\n\nTested-by: David Plowman <david.plowman@raspberrypi.com>\n\nDavid\n\nOn Fri, 7 Jan 2022 at 15:07, Kieran Bingham\n<kieran.bingham@ideasonboard.com> wrote:\n>\n> Quoting Jacopo Mondi (2022-01-07 14:58:18)\n> > Hi Kieran,\n> >\n> > On Fri, Jan 07, 2022 at 12:55:05PM +0000, Kieran Bingham wrote:\n> > > Provide a call allowing requests to be prepared and associated with the\n> > > pipeline handler after being constructed by the camera.\n> > >\n> > > This provides an opportunity for the Pipeline Handler to connect any\n> > > signals it may be interested in receiveing for the request such as\n> > > getting notifications when the request is ready for processing when\n> > > using a fence.\n> > >\n> > > While here, update the existing usage of the d pointer in\n> > > Camera::createRequest() to match the style of other functions.\n> > >\n> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >\n> > Nice, this sounds better than disconnecting after the signal has been\n> > emitted.\n>\n> That's what I thought, I couldn't bear to send a patch that disconnected\n> the signal on every request to have it reconnect again.\n>\n> >\n> > > ---\n> > >  include/libcamera/internal/pipeline_handler.h |  1 +\n> > >  src/libcamera/camera.cpp                      | 14 ++++++++++---\n> > >  src/libcamera/pipeline_handler.cpp            | 20 ++++++++++++++++---\n> > >  3 files changed, 29 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index e5b8ffb4db3d..ef7e1390560f 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -59,6 +59,7 @@ public:\n> > >       void stop(Camera *camera);\n> > >       bool hasPendingRequests(const Camera *camera) const;\n> > >\n> > > +     void prepareRequest(Request *request);\n> > >       void queueRequest(Request *request);\n> > >\n> > >       bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 86d84ac0a77d..1a00b34d9d9d 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1074,12 +1074,20 @@ int Camera::configure(CameraConfiguration *config)\n> > >   */\n> > >  std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)\n> > >  {\n> > > -     int ret = _d()->isAccessAllowed(Private::CameraConfigured,\n> > > -                                     Private::CameraRunning);\n> > > +     Private *const d = _d();\n> > > +\n> > > +     int ret = d->isAccessAllowed(Private::CameraConfigured,\n> > > +                                  Private::CameraRunning);\n> > >       if (ret < 0)\n> > >               return nullptr;\n> > >\n> > > -     return std::make_unique<Request>(this, cookie);\n> > > +     std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie);\n> > > +\n> > > +     /* Associate the request with the pipeline handler */\n> >\n> > It doesn't really associate it, but rather initialize it\n>\n> \"Intialise the request with the pipeline handler\" doesn't sound right\n> though... ?\n>\n> >\n> > I would also be tempted to suggest to rename the function to\n> > PipelineHandler::createRequest() to make sure it is immediately clear\n> > where the function is meant to be called from and when, as all the\n>\n> That would bother me as the PipelineHandler is not createing the\n> request...\n>\n> What about\n>         PipelineHandler::registerRequest()\n>\n> To state that the request is 'registered' with a single pipeline\n> handler?\n>\n> > other 'prepare' actions happen at request queue time, not at request\n> > creation time.\n> >\n> > > +     if (request)\n> > > +             d->pipe_->prepareRequest(request.get());\n> > > +\n> > > +     return request;\n> > >  }\n> > >\n> > >  /**\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 03e4b9e66aa6..18b59ef27fb6 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -337,6 +337,23 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> > >       return !camera->_d()->queuedRequests_.empty();\n> > >  }\n> > >\n> > > +/**\n> > > + * \\fn PipelineHandler::prepareRequest()\n> > > + * \\brief Prepare a request for use by the Pipeline Handler\n> > > + * \\param[in] request The request to prepare\n> > > + */\n> > > +void PipelineHandler::prepareRequest(Request *request)\n> > > +{\n> > > +     /*\n> > > +      * Connect the request prepared signal to the pipeline handler\n> > > +      * to manage fence based preparations allowing the request\n> >\n> > I would drop this statement. Fences handling is only potentially one\n> > the preparation steps we might want to handle in Request::prepare()\n>\n> I can drop it, but the reason for having the comment here is to delcare\n> what this connection is for. This function (which could be renamed to\n> registerRequest()) could do other things when registering the request\n> too.\n>\n> Is this better?\n>\n>         /*\n>          * Connect the request prepared signal to the pipeline handler\n>          * to notify the pipeline handler when a request is ready to be\n>          * processed.\n>          */\n>\n> > > +      * to inform us when it is ready to be processed by hardware.\n> > > +      */\n> > > +     request->_d()->prepared.connect(this, [this]() {\n> > > +                                             doQueueRequests();\n> > > +                                     });\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\fn PipelineHandler::queueRequest()\n> > >   * \\brief Queue a request\n> > > @@ -366,9 +383,6 @@ void PipelineHandler::queueRequest(Request *request)\n> > >\n> > >       waitingRequests_.push(request);\n> > >\n> > > -     request->_d()->prepared.connect(this, [this]() {\n> > > -                                             doQueueRequests();\n> > > -                                     });\n> > >       request->_d()->prepare(300ms);\n> > >  }\n> > >\n> > > --\n> > > 2.32.0\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DA813BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  7 Jan 2022 15:31:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64A226091E;\n\tFri,  7 Jan 2022 16:31:02 +0100 (CET)","from mail-wr1-x430.google.com (mail-wr1-x430.google.com\n\t[IPv6:2a00:1450:4864:20::430])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13CEC60868\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  7 Jan 2022 16:31:01 +0100 (CET)","by mail-wr1-x430.google.com with SMTP id t28so4964407wrb.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 07 Jan 2022 07:31:01 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"t+bxf0HC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=BchlO/K1gezGlYjeqcADMOTwtlvNjsuFcwPPSFx1p0A=;\n\tb=t+bxf0HCGGiH65NAanuEY7zGC4b2FxtnkxfazsPJWGmydYugJtI6uQG00zo1szTj2E\n\tzXIh1ueV5at6JG6YNub4Q/HGg4PBsiPwuoKovBiaEETFJO+4u07txNNob17/qUYV0uDk\n\t/GI55V0OOIj17rLB58l58A9kjsThJ+DTJfTSFp77JYpyYE7QlC97nUGaEzG9+9IMfw0S\n\tgDbOixzvTXkyqm+tnAokyv/fo12OSpzAvGsyD/poVBHiV47Ka2V2sBf018+z6w5/iPLn\n\tlYU+R1TY93fkA1hT9f14Qvmcr4sz/HvLRxvSk/Ahzu6vof/du0JU2RBs36D0Ppw+lxjz\n\tcgPw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=BchlO/K1gezGlYjeqcADMOTwtlvNjsuFcwPPSFx1p0A=;\n\tb=w+zJ+jxGQr0564UNZUO0/fu7hOb+4SPuWQbdNeZCVe4wVvUQI5Pjcc+KTWg2sbXKtM\n\t2Of8vwlRyBvlc8YnsCxzIkHAIsg9vinI8uXuYrCLoMPCkq+DIokgg9ksSsI0mMxXElVt\n\t3RCcsIzhBxPUG1EWViUwJmAZ89y2fdZAZS0VM+0EfTX0TCZxf2mADPo08rrl6kYamqlq\n\tyZuRSI0LsFnrTmLp7NVtckdDUuu/QhjYUOLV/ja2nx/fmoWbTmQTVmKZxYq3lY0HIAur\n\taLVow7pcuBkpx/voJbuuGvazhjrFVtmYJOeHTItgyfhenVj61+JNKOSl28VokpfEwzfl\n\tyNuw==","X-Gm-Message-State":"AOAM531l/EjLxWzntEXkHWijYclZRpr+iAnnsnp/oVzI3/FRcPzjo3KY\n\tzI4RGEXliKCTXTgE8Ec/rFRCfB24xxn0TN4xSKOEqg==","X-Google-Smtp-Source":"ABdhPJyuUDC2P1nNBQItn5//VyZd8GDKVbr4hE6PI3ds8RMvXwHi9Ln/oVrJ6NxB0xRUej7Y9rU7Mcw53t9VDWzib3E=","X-Received":"by 2002:adf:e70e:: with SMTP id\n\tc14mr2549181wrm.334.1641569460511; \n\tFri, 07 Jan 2022 07:31:00 -0800 (PST)","MIME-Version":"1.0","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-2-kieran.bingham@ideasonboard.com>\n\t<20220107145818.glzz4u67irmnge7s@uno.localdomain>\n\t<164156806812.1224575.4952431468893409126@Monstersaurus>","In-Reply-To":"<164156806812.1224575.4952431468893409126@Monstersaurus>","From":"David Plowman <david.plowman@raspberrypi.com>","Date":"Fri, 7 Jan 2022 15:30:49 +0000","Message-ID":"<CAHW6GYKbhTEx1L2VSxaa5yFEeE=fN0cDRYvCFgZ1Ra6tgG73Tw@mail.gmail.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21978,"web_url":"https://patchwork.libcamera.org/comment/21978/","msgid":"<20220107155313.ibkhyvvwps45tsa6@uno.localdomain>","date":"2022-01-07T15:53:13","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Jan 07, 2022 at 03:07:48PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2022-01-07 14:58:18)\n> > Hi Kieran,\n> >\n> > On Fri, Jan 07, 2022 at 12:55:05PM +0000, Kieran Bingham wrote:\n> > > Provide a call allowing requests to be prepared and associated with the\n> > > pipeline handler after being constructed by the camera.\n> > >\n> > > This provides an opportunity for the Pipeline Handler to connect any\n> > > signals it may be interested in receiveing for the request such as\n> > > getting notifications when the request is ready for processing when\n> > > using a fence.\n> > >\n> > > While here, update the existing usage of the d pointer in\n> > > Camera::createRequest() to match the style of other functions.\n> > >\n> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >\n> > Nice, this sounds better than disconnecting after the signal has been\n> > emitted.\n>\n> That's what I thought, I couldn't bear to send a patch that disconnected\n> the signal on every request to have it reconnect again.\n>\n> >\n> > > ---\n> > >  include/libcamera/internal/pipeline_handler.h |  1 +\n> > >  src/libcamera/camera.cpp                      | 14 ++++++++++---\n> > >  src/libcamera/pipeline_handler.cpp            | 20 ++++++++++++++++---\n> > >  3 files changed, 29 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index e5b8ffb4db3d..ef7e1390560f 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -59,6 +59,7 @@ public:\n> > >       void stop(Camera *camera);\n> > >       bool hasPendingRequests(const Camera *camera) const;\n> > >\n> > > +     void prepareRequest(Request *request);\n> > >       void queueRequest(Request *request);\n> > >\n> > >       bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 86d84ac0a77d..1a00b34d9d9d 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1074,12 +1074,20 @@ int Camera::configure(CameraConfiguration *config)\n> > >   */\n> > >  std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)\n> > >  {\n> > > -     int ret = _d()->isAccessAllowed(Private::CameraConfigured,\n> > > -                                     Private::CameraRunning);\n> > > +     Private *const d = _d();\n> > > +\n> > > +     int ret = d->isAccessAllowed(Private::CameraConfigured,\n> > > +                                  Private::CameraRunning);\n> > >       if (ret < 0)\n> > >               return nullptr;\n> > >\n> > > -     return std::make_unique<Request>(this, cookie);\n> > > +     std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie);\n> > > +\n> > > +     /* Associate the request with the pipeline handler */\n> >\n> > It doesn't really associate it, but rather initialize it\n>\n> \"Intialise the request with the pipeline handler\" doesn't sound right\n> though... ?\n>\n> >\n> > I would also be tempted to suggest to rename the function to\n> > PipelineHandler::createRequest() to make sure it is immediately clear\n> > where the function is meant to be called from and when, as all the\n>\n> That would bother me as the PipelineHandler is not createing the\n> request...\n\nNo it's not, but it's in the creation call path.\n\n>\n> What about\n> \tPipelineHandler::registerRequest()\n>\n> To state that the request is 'registered' with a single pipeline\n> handler?\n>\n\nWorks for met\n\n> > other 'prepare' actions happen at request queue time, not at request\n> > creation time.\n> >\n> > > +     if (request)\n> > > +             d->pipe_->prepareRequest(request.get());\n> > > +\n> > > +     return request;\n> > >  }\n> > >\n> > >  /**\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 03e4b9e66aa6..18b59ef27fb6 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -337,6 +337,23 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> > >       return !camera->_d()->queuedRequests_.empty();\n> > >  }\n> > >\n> > > +/**\n> > > + * \\fn PipelineHandler::prepareRequest()\n> > > + * \\brief Prepare a request for use by the Pipeline Handler\n> > > + * \\param[in] request The request to prepare\n> > > + */\n> > > +void PipelineHandler::prepareRequest(Request *request)\n> > > +{\n> > > +     /*\n> > > +      * Connect the request prepared signal to the pipeline handler\n> > > +      * to manage fence based preparations allowing the request\n> >\n> > I would drop this statement. Fences handling is only potentially one\n> > the preparation steps we might want to handle in Request::prepare()\n>\n> I can drop it, but the reason for having the comment here is to delcare\n> what this connection is for. This function (which could be renamed to\n> registerRequest()) could do other things when registering the request\n> too.\n\nSorry, I wasn't clear, I was suggestin to just drop\n\n +      * to manage fence based preparations allowing the request\n\n>\n> Is this better?\n>\n> \t/*\n> \t * Connect the request prepared signal to the pipeline handler\n> \t * to notify the pipeline handler when a request is ready to be\n> \t * processed.\n> \t */\n\nWith one of \"the pipeline handler\" dropped, yes :)\n\n>\n> > > +      * to inform us when it is ready to be processed by hardware.\n> > > +      */\n> > > +     request->_d()->prepared.connect(this, [this]() {\n> > > +                                             doQueueRequests();\n> > > +                                     });\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\fn PipelineHandler::queueRequest()\n> > >   * \\brief Queue a request\n> > > @@ -366,9 +383,6 @@ void PipelineHandler::queueRequest(Request *request)\n> > >\n> > >       waitingRequests_.push(request);\n> > >\n> > > -     request->_d()->prepared.connect(this, [this]() {\n> > > -                                             doQueueRequests();\n> > > -                                     });\n> > >       request->_d()->prepare(300ms);\n> > >  }\n> > >\n> > > --\n> > > 2.32.0\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E15AEBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  7 Jan 2022 15:52:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 420A560921;\n\tFri,  7 Jan 2022 16:52:16 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 028D960868\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  7 Jan 2022 16:52:14 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 7EE78FF80A;\n\tFri,  7 Jan 2022 15:52:14 +0000 (UTC)"],"Date":"Fri, 7 Jan 2022 16:53:13 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20220107155313.ibkhyvvwps45tsa6@uno.localdomain>","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-2-kieran.bingham@ideasonboard.com>\n\t<20220107145818.glzz4u67irmnge7s@uno.localdomain>\n\t<164156806812.1224575.4952431468893409126@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164156806812.1224575.4952431468893409126@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21986,"web_url":"https://patchwork.libcamera.org/comment/21986/","msgid":"<Ydo6detp5Q+dZFHC@pendragon.ideasonboard.com>","date":"2022-01-09T01:29:25","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Jan 07, 2022 at 04:53:13PM +0100, Jacopo Mondi wrote:\n> On Fri, Jan 07, 2022 at 03:07:48PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2022-01-07 14:58:18)\n> > > On Fri, Jan 07, 2022 at 12:55:05PM +0000, Kieran Bingham wrote:\n> > > > Provide a call allowing requests to be prepared and associated with the\n> > > > pipeline handler after being constructed by the camera.\n> > > >\n> > > > This provides an opportunity for the Pipeline Handler to connect any\n> > > > signals it may be interested in receiveing for the request such as\n\ns/receiveing/receiving/\n\n> > > > getting notifications when the request is ready for processing when\n> > > > using a fence.\n> > > >\n> > > > While here, update the existing usage of the d pointer in\n> > > > Camera::createRequest() to match the style of other functions.\n> > > >\n> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > >\n> > > Nice, this sounds better than disconnecting after the signal has been\n> > > emitted.\n> >\n> > That's what I thought, I couldn't bear to send a patch that disconnected\n> > the signal on every request to have it reconnect again.\n\nAvoiding constant connections and disconnections is certainly nice (but\nnote that we will connect and disconnect the EventNotifier::activated\nsignal for each fence every time a request is queued, we can't do much\nabout that). I'm not thrilled about connecting the Request::prepared\nsignal when creating the request though, it feels wrong somehow, but as\nit's internal I can live with it.\n\n> > > > ---\n> > > >  include/libcamera/internal/pipeline_handler.h |  1 +\n> > > >  src/libcamera/camera.cpp                      | 14 ++++++++++---\n> > > >  src/libcamera/pipeline_handler.cpp            | 20 ++++++++++++++++---\n> > > >  3 files changed, 29 insertions(+), 6 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > index e5b8ffb4db3d..ef7e1390560f 100644\n> > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > @@ -59,6 +59,7 @@ public:\n> > > >       void stop(Camera *camera);\n> > > >       bool hasPendingRequests(const Camera *camera) const;\n> > > >\n> > > > +     void prepareRequest(Request *request);\n\nI don't like the name, given that the request has a prepared signal that\nis emitted when the request is queued, which is completely unrelated to\nthis \"prepare\" operation. associateRequest() could be better, I'm sure\nthere are even better names.\n\n> > > >       void queueRequest(Request *request);\n> > > >\n> > > >       bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 86d84ac0a77d..1a00b34d9d9d 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -1074,12 +1074,20 @@ int Camera::configure(CameraConfiguration *config)\n> > > >   */\n> > > >  std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)\n> > > >  {\n> > > > -     int ret = _d()->isAccessAllowed(Private::CameraConfigured,\n> > > > -                                     Private::CameraRunning);\n> > > > +     Private *const d = _d();\n> > > > +\n> > > > +     int ret = d->isAccessAllowed(Private::CameraConfigured,\n> > > > +                                  Private::CameraRunning);\n> > > >       if (ret < 0)\n> > > >               return nullptr;\n> > > >\n> > > > -     return std::make_unique<Request>(this, cookie);\n> > > > +     std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie);\n> > > > +\n> > > > +     /* Associate the request with the pipeline handler */\n> > >\n> > > It doesn't really associate it, but rather initialize it\n> >\n> > \"Intialise the request with the pipeline handler\" doesn't sound right\n> > though... ?\n> >\n> > > I would also be tempted to suggest to rename the function to\n> > > PipelineHandler::createRequest() to make sure it is immediately clear\n> > > where the function is meant to be called from and when, as all the\n> >\n> > That would bother me as the PipelineHandler is not createing the\n> > request...\n> \n> No it's not, but it's in the creation call path.\n> \n> > What about\n> > \tPipelineHandler::registerRequest()\n\nAh, there we go :-)\n\n> > To state that the request is 'registered' with a single pipeline\n> > handler?\n> \n> Works for met\n> \n> > > other 'prepare' actions happen at request queue time, not at request\n> > > creation time.\n> > >\n> > > > +     if (request)\n> > > > +             d->pipe_->prepareRequest(request.get());\n> > > > +\n> > > > +     return request;\n> > > >  }\n> > > >\n> > > >  /**\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > index 03e4b9e66aa6..18b59ef27fb6 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -337,6 +337,23 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> > > >       return !camera->_d()->queuedRequests_.empty();\n> > > >  }\n> > > >\n> > > > +/**\n> > > > + * \\fn PipelineHandler::prepareRequest()\n> > > > + * \\brief Prepare a request for use by the Pipeline Handler\n> > > > + * \\param[in] request The request to prepare\n> > > > + */\n> > > > +void PipelineHandler::prepareRequest(Request *request)\n> > > > +{\n> > > > +     /*\n> > > > +      * Connect the request prepared signal to the pipeline handler\n> > > > +      * to manage fence based preparations allowing the request\n> > >\n> > > I would drop this statement. Fences handling is only potentially one\n> > > the preparation steps we might want to handle in Request::prepare()\n> >\n> > I can drop it, but the reason for having the comment here is to delcare\n> > what this connection is for. This function (which could be renamed to\n> > registerRequest()) could do other things when registering the request\n> > too.\n> \n> Sorry, I wasn't clear, I was suggestin to just drop\n> \n>  +      * to manage fence based preparations allowing the request\n> \n> > Is this better?\n> >\n> > \t/*\n> > \t * Connect the request prepared signal to the pipeline handler\n> > \t * to notify the pipeline handler when a request is ready to be\n> > \t * processed.\n> > \t */\n> \n> With one of \"the pipeline handler\" dropped, yes :)\n> \n> > > > +      * to inform us when it is ready to be processed by hardware.\n> > > > +      */\n> > > > +     request->_d()->prepared.connect(this, [this]() {\n> > > > +                                             doQueueRequests();\n> > > > +                                     });\n> > > > +}\n> > > > +\n> > > >  /**\n> > > >   * \\fn PipelineHandler::queueRequest()\n> > > >   * \\brief Queue a request\n> > > > @@ -366,9 +383,6 @@ void PipelineHandler::queueRequest(Request *request)\n> > > >\n> > > >       waitingRequests_.push(request);\n> > > >\n> > > > -     request->_d()->prepared.connect(this, [this]() {\n> > > > -                                             doQueueRequests();\n> > > > -                                     });\n> > > >       request->_d()->prepare(300ms);\n> > > >  }\n> > > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7AA81BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  9 Jan 2022 01:29:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDDC260921;\n\tSun,  9 Jan 2022 02:29:35 +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 DE73B6017E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  9 Jan 2022 02:29:33 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E516A1B;\n\tSun,  9 Jan 2022 02:29:33 +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=\"h2KVI/i6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641691773;\n\tbh=ibl+E3AKyuXKJ1x87Yr2mIAl0WFnzBLQ0u0ZvktVcfc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h2KVI/i6ftNnzlZcZSqwxxRBvCMzx5OMCkbrpGoNn9rG3ICfxZq9egD0ADK05omRc\n\tPaT42e5Lkr+X+yzhO63M27/vl99txgldMrqc6YobKeQN20R20d6IQqoZzXthCXRSi+\n\tbX5dIP3YEKn3pvmLq2L+P1lZcLsHrns26/+3BDbI=","Date":"Sun, 9 Jan 2022 03:29:25 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<Ydo6detp5Q+dZFHC@pendragon.ideasonboard.com>","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-2-kieran.bingham@ideasonboard.com>\n\t<20220107145818.glzz4u67irmnge7s@uno.localdomain>\n\t<164156806812.1224575.4952431468893409126@Monstersaurus>\n\t<20220107155313.ibkhyvvwps45tsa6@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220107155313.ibkhyvvwps45tsa6@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler:\n\tPrepare requests","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]