[{"id":1378,"web_url":"https://patchwork.libcamera.org/comment/1378/","msgid":"<20190416001629.GD16492@bigcity.dyn.berto.se>","date":"2019-04-16T00:16:29","subject":"Re: [libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store\n\tRequest reference in Buffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-04-16 01:18:59 +0200, Jacopo Mondi wrote:\n> Add to the Buffer class methods to set and retrieve a reference to the\n> Request instance this buffer is part of.\n> \n> As Buffers might outlive the Request they are associated with, the\n> reference is only temporary valid during the buffer completion interval\n> (since when the buffer gets queued to Camera for processing, until it\n> gets marked as completed).\n> \n> To show this change purpose, use the new Buffer::request() method\n> in IPU3 pipeline handler, as it will soon be moved to support multiple\n> streams where buffers might complete in an order different from the request\n> queuing one.\n\nYou know it, this should IMHO be done in a separate patch or merged with\nsome other patch adding multiple stream support to the IPU3 ;-)\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/buffer.h | 6 ++++\n> src/libcamera/buffer.cpp | 45 +++++++++++++++++++++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> src/libcamera/request.cpp | 4 +++\n> 4 files changed, 55 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 0c844d126a27..a2daaaf346dc 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -13,6 +13,7 @@\n> namespace libcamera {\n> \n> class BufferPool;\n> +class Request;\n> \n> class Plane final\n> {\n> @@ -52,14 +53,18 @@ public:\n> \tunsigned int sequence() const { return sequence_; }\n> \tStatus status() const { return status_; }\n> \tstd::vector &planes() { return planes_; }\n> +\tRequest *request() const { return request_; }\n> \n> private:\n> \tfriend class BufferPool;\n> \tfriend class PipelineHandler;\n> +\tfriend class Request;\n> \tfriend class V4L2Device;\n> \n> \tvoid cancel();\n> \n> +\tvoid setRequest(Request *req) { request_ = req; }\n> +\n> \tunsigned int index_;\n> \tunsigned int bytesused_;\n> \tuint64_t timestamp_;\n> @@ -67,6 +72,7 @@ private:\n> \tStatus status_;\n> \n> \tstd::vector planes_;\n> +\tRequest *request_;\n> };\n> \n> class BufferPool final\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index e2d1cf04411e..790c6dcffe3a 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -196,7 +196,7 @@ void *Plane::mem()\n> */\n> \n> Buffer::Buffer()\n> -\t: index_(-1)\n> +\t: index_(-1), request_(nullptr)\n> {\n> }\n> \n> @@ -248,6 +248,26 @@ Buffer::Buffer()\n> * \\return The buffer status\n> */\n> \n> +/**\n> + * \\fn Buffer::request()\n> + * \\brief Retrieve the request this buffer belongs to\n> + *\n> + * The intended callers of this method are buffer completion slots\n> + * implemented in CameraData sub-classes which needs to associated a request\n> + * to the just completed buffer, before calling Request::completeBuffer().\n\nThe intended caller of this method is buffer completion handlers. Buffer \ncompletion handlers often need to associate a buffer to the request it \nis associated with.\n\n> + *\n> + * It is up to the caller of this function to deal with the case the buffer has\n> + * been already marked as complete, and the reference to the Request has been\n> + * invalidated and set to nullptr.\n\nI would drop this.\n\n> + *\n> + * See also Buffer::setRequest() for a more detailed explanation of the\n> + * validity interval of the Request reference contained in a Buffer.\n\nSee Buffer::setRequest() for a detailed explanation of the lifetime \ncycle for the buffer to request association.\n\n\n> + *\n> + * \\return The Request this Buffer belongs to or nullptr if the Buffer has\n> + * not been queued to the Camera for processing yet or it has completed already.\n\nThe Request the Buffer belongs to, or nullptr if the buffer is either \ncompleted or not associate with a request\n\n> + * \\sa Buffer::setRequest()\n> + */\n> +\n> /**\n> * \\brief Mark a buffer as cancel by setting its status to BufferCancelled\n> */\n> @@ -259,6 +279,29 @@ void Buffer::cancel()\n> \tstatus_ = BufferCancelled;\n> }\n> \n> +/**\n> + * \\fn Buffer::setRequest()\n> + * \\brief Set the request this buffer belongs to\n> + *\n> + * Buffers are associated to Streams in a Request, which is then sent to the\n> + * Camera for processing. This method stores in the Buffer a pointer to the\n> + * Request this Buffer is part of, for later retrieval through the\n> + * Buffer::request() method.\n> + *\n> + * Buffers are associated to requests at Request::prepare() time and said\n> + * association is valid until the buffer does not complete at\n> + * Request::completeBuffer() time. Before and after the buffer completion\n> + * interval (the time between when the request is queued to the Camera, and\n> + * the buffer is marked as 'complete' by pipeline handlers) the reference to\n> + * Request is set to nullptr.\n\nIMHO most of this should be rephrases and moved to the documentation for \nRequest::prepare(). What should be documented here is how this call \nmodifies the state of the Buffer object.\n\n> + *\n> + * The intended caller of this methods is the Request class at\n> + * Request::prepare() time, when it stores a reference to itself through a\n> + * call to this method, and at Request::completeBuffer() time, where it\n> + * invalidates the request_ reference by calling this method with a nullptr as\n> + * argument.\n> + */\n\nI'm not saying it's wrong, but is it strictly necessary to document \nintended callers for an internal API? I agree this is most needed for \ninterfaces deigned to be called at specific points in either \napplications or pipeline handles (and later 3A).\n\n> +\n> /**\n> * \\class BufferPool\n> * \\brief A pool of buffers\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 7d865fa329ea..ce680835cec2 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -630,7 +630,7 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> */\n> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n> \n> \tpipe_->completeBuffer(camera_, request, buffer);\n> \tpipe_->completeRequest(camera_, request);\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 7fa034e6c747..6b175e125596 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -130,6 +130,8 @@ int Request::prepare()\n> {\n> \tfor (auto const &pair : bufferMap_) {\n> \t\tBuffer *buffer = pair.second;\n> +\n> +\t\tbuffer->setRequest(this);\n> \t\tpending_.insert(buffer);\n> \t}\n> \n> @@ -166,6 +168,8 @@ bool Request::completeBuffer(Buffer *buffer)\n> \tint ret = pending_.erase(buffer);\n> \tASSERT(ret == 1);\n> \n> +\tbuffer->setRequest(nullptr);\n> +\n> \treturn empty();\n> }\n> \n> -- \n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86D2460DBE\n\tfor ;\n\tTue, 16 Apr 2019 02:16:31 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id h16so17344045ljg.11\n\tfor ;\n\tMon, 15 Apr 2019 17:16:31 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tu19sm10263681lfg.74.2019.04.15.17.16.29\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 15 Apr 2019 17:16:29 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=/4DWQjK2GUZteevq+e3rfFcRP5pdSoUdU2f8lfBcdhY=;\n\tb=JS6meaBX8ZGf4Z8AWzAkbJvmx6hs8li6UhatA2qGL0ascKKRQ3nJagJjkRuK/qF0VO\n\tFXtMLPluRkS2Vy2GDXhQ+37nVMLjJaRK1y5k20OvFLuKtOEIJfPc91tfSpbFaMg71qx1\n\t3hxXliPUlWRZKsLXL6KTskuYPpu5IVUz+FFp2eyeGGurQpJem3mNkFRQ5VZIIsP7R2Sl\n\tUeTaEhG46tpgATlTcK9AfHO98V0so9aIpB9SQeqA5WQGRkECweYP21B6poWQOo8thHIF\n\tnAEMD+1hwl/NszNVqwVHnZShHsc8smcdHxSzUrGR/8az2K8JMg24WTUXGaDTh7j9tAnK\n\t4QTg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=/4DWQjK2GUZteevq+e3rfFcRP5pdSoUdU2f8lfBcdhY=;\n\tb=PCdZcyQl91z4fvjVWJRPT/Igjvg1xD7wqUxyGk+CLuWyIvAHvTl8JEsOu02u4MOyEA\n\tKJdn0ehHOcS/I0UJxMhGnf5j7ml7SSWk/PrUGwYMGsr11uiWBUNF32g3de3nlvyLpIL2\n\tB3hXShwI/mw0anJ4ccqjfS8gKiLCs54wMo7yoSLyM4+occtmoUIUx08QOe6ev/1BbHFR\n\t4Oa8Ng/CqfnTeMhGVD8WpLpOkbyU/s0sMitkbe0NqDJnU5dta7yJ4lvKhZNaNA5KWd8F\n\toJjRLXU+9iomfolLHSpk71re6cCyi17cZaftTOwXI15Vws6kvDzxQbe6dEK7JhJ6vP/r\n\tp1pQ==","X-Gm-Message-State":"APjAAAWOxlZ5WPDrujq6VJYpAiEJpCalxqF4UHWGLuRK7wb33rkr1i5e\n\tDQ24NCKlzo5fJEOHjyqRkTa0sQ==","X-Google-Smtp-Source":"APXvYqzNOREmmECTtzurbnFk5tzxrUOB+4HnzUnj650DnunfMG3dCqh4IHSN1lTwDABEAjpibjlpDg==","X-Received":"by 2002:a2e:219:: with SMTP id 25mr41066750ljc.34.1555373790739; \n\tMon, 15 Apr 2019 17:16:30 -0700 (PDT)","Date":"Tue, 16 Apr 2019 02:16:29 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416001629.GD16492@bigcity.dyn.berto.se>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190415231859.9747-8-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store\n\tRequest reference in Buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 16 Apr 2019 00:16:31 -0000"}},{"id":1384,"web_url":"https://patchwork.libcamera.org/comment/1384/","msgid":"<20190416113559.GF4832@pendragon.ideasonboard.com>","date":"2019-04-16T11:35:59","subject":"Re: [libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store\n\tRequest reference in Buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Apr 16, 2019 at 02:16:29AM +0200, Niklas Söderlund wrote:\n> On 2019-04-16 01:18:59 +0200, Jacopo Mondi wrote:\n> > Add to the Buffer class methods to set and retrieve a reference to the\n> > Request instance this buffer is part of.\n> > \n> > As Buffers might outlive the Request they are associated with, the\n> > reference is only temporary valid during the buffer completion interval\n> > (since when the buffer gets queued to Camera for processing, until it\n> > gets marked as completed).\n> > \n> > To show this change purpose, use the new Buffer::request() method\n> > in IPU3 pipeline handler, as it will soon be moved to support multiple\n> > streams where buffers might complete in an order different from the request\n> > queuing one.\n> \n> You know it, this should IMHO be done in a separate patch or merged with\n> some other patch adding multiple stream support to the IPU3 ;-)\n> \n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/buffer.h | 6 ++++\n> > src/libcamera/buffer.cpp | 45 +++++++++++++++++++++++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > src/libcamera/request.cpp | 4 +++\n> > 4 files changed, 55 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 0c844d126a27..a2daaaf346dc 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -13,6 +13,7 @@\n> > namespace libcamera {\n> > \n> > class BufferPool;\n> > +class Request;\n> > \n> > class Plane final\n> > {\n> > @@ -52,14 +53,18 @@ public:\n> > \tunsigned int sequence() const { return sequence_; }\n> > \tStatus status() const { return status_; }\n> > \tstd::vector &planes() { return planes_; }\n> > +\tRequest *request() const { return request_; }\n> > \n> > private:\n> > \tfriend class BufferPool;\n> > \tfriend class PipelineHandler;\n> > +\tfriend class Request;\n> > \tfriend class V4L2Device;\n> > \n> > \tvoid cancel();\n> > \n> > +\tvoid setRequest(Request *req) { request_ = req; }\n\ns/req/request/\n\nI wonder if we need this method, or if the Request class could access\nthe request_ field directly as it's a friend. I suppose than an accessor\nis cleaner.\n\n> > +\n> > \tunsigned int index_;\n> > \tunsigned int bytesused_;\n> > \tuint64_t timestamp_;\n> > @@ -67,6 +72,7 @@ private:\n> > \tStatus status_;\n> > \n> > \tstd::vector planes_;\n> > +\tRequest *request_;\n> > };\n> > \n> > class BufferPool final\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index e2d1cf04411e..790c6dcffe3a 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -196,7 +196,7 @@ void *Plane::mem()\n> > */\n> > \n> > Buffer::Buffer()\n> > -\t: index_(-1)\n> > +\t: index_(-1), request_(nullptr)\n> > {\n> > }\n> > \n> > @@ -248,6 +248,26 @@ Buffer::Buffer()\n> > * \\return The buffer status\n> > */\n> > \n> > +/**\n> > + * \\fn Buffer::request()\n> > + * \\brief Retrieve the request this buffer belongs to\n> > + *\n> > + * The intended callers of this method are buffer completion slots\n> > + * implemented in CameraData sub-classes which needs to associated a request\n> > + * to the just completed buffer, before calling Request::completeBuffer().\n> \n> The intended caller of this method is buffer completion handlers. Buffer \n\n\"handlers\" is plural, so \"The intended callers of this method are\".\n\n> completion handlers often need to associate a buffer to the request it \n> is associated with.\n\ns/is associated with/has been queued to/ to avoid repeating \"associate\"?\n\n> > + *\n> > + * It is up to the caller of this function to deal with the case the buffer has\n> > + * been already marked as complete, and the reference to the Request has been\n> > + * invalidated and set to nullptr.\n> \n> I would drop this.\n\nI agree, but I would also add\n\n * The returned request pointer is guaranteed to be valid from the time the\n * request is prepared with Request::prepare() to the time the buffer is marked\n * as complete with Request::completeBuffer().\n\nMaybe with an additional \"See the documentation of those two methods for\nmore information\".\n\n> > + *\n> > + * See also Buffer::setRequest() for a more detailed explanation of the\n> > + * validity interval of the Request reference contained in a Buffer.\n\nAh, the information is in setRequest() :-) I would move it here, as\nsetRequest() has a single intended caller, while request() is called by\npipeline handlers, so it's easier for pipeline handlers developers to\nnot have to read the setRequest() documentation.\n\n> See Buffer::setRequest() for a detailed explanation of the lifetime \n> cycle for the buffer to request association.\n\nDoxygen has a \\sa tag for the \"See also\".\n\n> > + *\n> > + * \\return The Request this Buffer belongs to or nullptr if the Buffer has\n> > + * not been queued to the Camera for processing yet or it has completed already.\n> \n> The Request the Buffer belongs to, or nullptr if the buffer is either \n> completed or not associate with a request\n\ns/associate/associated/\n\n> > + * \\sa Buffer::setRequest()\n> > + */\n> > +\n> > /**\n> > * \\brief Mark a buffer as cancel by setting its status to BufferCancelled\n> > */\n> > @@ -259,6 +279,29 @@ void Buffer::cancel()\n> > \tstatus_ = BufferCancelled;\n> > }\n> > \n> > +/**\n> > + * \\fn Buffer::setRequest()\n> > + * \\brief Set the request this buffer belongs to\n> > + *\n> > + * Buffers are associated to Streams in a Request, which is then sent to the\n> > + * Camera for processing. This method stores in the Buffer a pointer to the\n> > + * Request this Buffer is part of, for later retrieval through the\n> > + * Buffer::request() method.\n> > + *\n> > + * Buffers are associated to requests at Request::prepare() time and said\n> > + * association is valid until the buffer does not complete at\n> > + * Request::completeBuffer() time. Before and after the buffer completion\n> > + * interval (the time between when the request is queued to the Camera, and\n> > + * the buffer is marked as 'complete' by pipeline handlers) the reference to\n> > + * Request is set to nullptr.\n> \n> IMHO most of this should be rephrases and moved to the documentation for \n> Request::prepare(). What should be documented here is how this call \n> modifies the state of the Buffer object.\n> \n> > + *\n> > + * The intended caller of this methods is the Request class at\n> > + * Request::prepare() time, when it stores a reference to itself through a\n> > + * call to this method, and at Request::completeBuffer() time, where it\n> > + * invalidates the request_ reference by calling this method with a nullptr as\n> > + * argument.\n> > + */\n> \n> I'm not saying it's wrong, but is it strictly necessary to document \n> intended callers for an internal API? I agree this is most needed for \n> interfaces deigned to be called at specific points in either \n> applications or pipeline handles (and later 3A).\n\nIt's not strictly necessary, but it can avoid abuses for methods that\nare meant to be called from a very specific place. I would however\nsimplify this paragraph by just stating\n\n * The intended callers are the Request::prepare() and Request::completeBuffer()\n * methods.\n\nWith the life cycle explanation moved to the Request documentation, as\nproposed above.\n\n> > +\n> > /**\n> > * \\class BufferPool\n> > * \\brief A pool of buffers\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 7d865fa329ea..ce680835cec2 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -630,7 +630,7 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> > */\n> > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> > {\n> > -\tRequest *request = queuedRequests_.front();\n> > +\tRequest *request = buffer->request();\n> > \n> > \tpipe_->completeBuffer(camera_, request, buffer);\n> > \tpipe_->completeRequest(camera_, request);\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 7fa034e6c747..6b175e125596 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -130,6 +130,8 @@ int Request::prepare()\n> > {\n> > \tfor (auto const &pair : bufferMap_) {\n> > \t\tBuffer *buffer = pair.second;\n> > +\n> > +\t\tbuffer->setRequest(this);\n> > \t\tpending_.insert(buffer);\n> > \t}\n> > \n> > @@ -166,6 +168,8 @@ bool Request::completeBuffer(Buffer *buffer)\n> > \tint ret = pending_.erase(buffer);\n> > \tASSERT(ret == 1);\n> > \n> > +\tbuffer->setRequest(nullptr);\n> > +\n> > \treturn empty();\n> > }\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D823D60DB4\n\tfor ;\n\tTue, 16 Apr 2019 13:36:08 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49D36E2;\n\tTue, 16 Apr 2019 13:36:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555414568;\n\tbh=ryAoFDgh6avs1PL7dSlneVMHY/+pvO2fmtfg01RfcdI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cE62vYKXbd2t3cLnwGREUVMqYtnuhfKSGe6etgC8sIUre5eASBajX64SOk/cWHFPi\n\tzVGynQSfC5FBNy71qYyR/v0MEY7mavNtQ5PNjZo+1lZ4fq7Aw4ztZ9WhP7E8VCJ0Ix\n\tBqIfP5oj7oZegkgWBB5iDUbpMXhNt2lRCXWsZwZE=","Date":"Tue, 16 Apr 2019 14:35:59 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190416113559.GF4832@pendragon.ideasonboard.com>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-8-jacopo@jmondi.org>\n\t<20190416001629.GD16492@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190416001629.GD16492@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store\n\tRequest reference in Buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 16 Apr 2019 11:36:09 -0000"}},{"id":1386,"web_url":"https://patchwork.libcamera.org/comment/1386/","msgid":"<20190416132417.krhj2ht2jwunut4r@uno.localdomain>","date":"2019-04-16T13:24:17","subject":"Re: [libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store\n\tRequest reference in Buffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 16, 2019 at 02:35:59PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 16, 2019 at 02:16:29AM +0200, Niklas Söderlund wrote:\n> > On 2019-04-16 01:18:59 +0200, Jacopo Mondi wrote:\n> > > Add to the Buffer class methods to set and retrieve a reference to the\n> > > Request instance this buffer is part of.\n> > >\n> > > As Buffers might outlive the Request they are associated with, the\n> > > reference is only temporary valid during the buffer completion interval\n> > > (since when the buffer gets queued to Camera for processing, until it\n> > > gets marked as completed).\n> > >\n> > > To show this change purpose, use the new Buffer::request() method\n> > > in IPU3 pipeline handler, as it will soon be moved to support multiple\n> > > streams where buffers might complete in an order different from the request\n> > > queuing one.\n> >\n> > You know it, this should IMHO be done in a separate patch or merged with\n> > some other patch adding multiple stream support to the IPU3 ;-)\n> >\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/buffer.h | 6 ++++\n> > > src/libcamera/buffer.cpp | 45 +++++++++++++++++++++++++++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > src/libcamera/request.cpp | 4 +++\n> > > 4 files changed, 55 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > > index 0c844d126a27..a2daaaf346dc 100644\n> > > --- a/include/libcamera/buffer.h\n> > > +++ b/include/libcamera/buffer.h\n> > > @@ -13,6 +13,7 @@\n> > > namespace libcamera {\n> > >\n> > > class BufferPool;\n> > > +class Request;\n> > >\n> > > class Plane final\n> > > {\n> > > @@ -52,14 +53,18 @@ public:\n> > > \tunsigned int sequence() const { return sequence_; }\n> > > \tStatus status() const { return status_; }\n> > > \tstd::vector &planes() { return planes_; }\n> > > +\tRequest *request() const { return request_; }\n> > >\n> > > private:\n> > > \tfriend class BufferPool;\n> > > \tfriend class PipelineHandler;\n> > > +\tfriend class Request;\n> > > \tfriend class V4L2Device;\n> > >\n> > > \tvoid cancel();\n> > >\n> > > +\tvoid setRequest(Request *req) { request_ = req; }\n>\n> s/req/request/\n>\n> I wonder if we need this method, or if the Request class could access\n> the request_ field directly as it's a friend. I suppose than an accessor\n> is cleaner.\n>\n\nTrue indeed, but I would prefer going through an accessor method if\npossible...\n\n> > > +\n> > > \tunsigned int index_;\n> > > \tunsigned int bytesused_;\n> > > \tuint64_t timestamp_;\n> > > @@ -67,6 +72,7 @@ private:\n> > > \tStatus status_;\n> > >\n> > > \tstd::vector planes_;\n> > > +\tRequest *request_;\n> > > };\n> > >\n> > > class BufferPool final\n> > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > index e2d1cf04411e..790c6dcffe3a 100644\n> > > --- a/src/libcamera/buffer.cpp\n> > > +++ b/src/libcamera/buffer.cpp\n> > > @@ -196,7 +196,7 @@ void *Plane::mem()\n> > > */\n> > >\n> > > Buffer::Buffer()\n> > > -\t: index_(-1)\n> > > +\t: index_(-1), request_(nullptr)\n> > > {\n> > > }\n> > >\n> > > @@ -248,6 +248,26 @@ Buffer::Buffer()\n> > > * \\return The buffer status\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn Buffer::request()\n> > > + * \\brief Retrieve the request this buffer belongs to\n> > > + *\n> > > + * The intended callers of this method are buffer completion slots\n> > > + * implemented in CameraData sub-classes which needs to associated a request\n> > > + * to the just completed buffer, before calling Request::completeBuffer().\n> >\n> > The intended caller of this method is buffer completion handlers. Buffer\n>\n> \"handlers\" is plural, so \"The intended callers of this method are\".\n>\n> > completion handlers often need to associate a buffer to the request it\n> > is associated with.\n\nI don't like the \"often need\"\n\n>\n> s/is associated with/has been queued to/ to avoid repeating \"associate\"?\n\n + * The intended callers of this method are buffer completion\n + * handlers that needs to associated a buffer to the request it has\n + * been queued to.\n\nI find the original version more informative, but ok...\n\n\n>\n> > > + *\n> > > + * It is up to the caller of this function to deal with the case the buffer has\n> > > + * been already marked as complete, and the reference to the Request has been\n> > > + * invalidated and set to nullptr.\n> >\n> > I would drop this.\n>\n> I agree, but I would also add\n>\n> * The returned request pointer is guaranteed to be valid from the time the\n> * request is prepared with Request::prepare() to the time the buffer is marked\n> * as complete with Request::completeBuffer().\n>\n> Maybe with an additional \"See the documentation of those two methods for\n> more information\".\n\nThis I agree.\n\n>\n> > > + *\n> > > + * See also Buffer::setRequest() for a more detailed explanation of the\n> > > + * validity interval of the Request reference contained in a Buffer.\n>\n> Ah, the information is in setRequest() :-) I would move it here, as\n> setRequest() has a single intended caller, while request() is called by\n> pipeline handlers, so it's easier for pipeline handlers developers to\n> not have to read the setRequest() documentation.\n>\n\nRight, I'll move the lifecycle description here\n\n> > See Buffer::setRequest() for a detailed explanation of the lifetime\n> > cycle for the buffer to request association.\n>\n> Doxygen has a \\sa tag for the \"See also\".\n>\n> > > + *\n> > > + * \\return The Request this Buffer belongs to or nullptr if the Buffer has\n> > > + * not been queued to the Camera for processing yet or it has completed already.\n> >\n> > The Request the Buffer belongs to, or nullptr if the buffer is either\n> > completed or not associate with a request\n>\n> s/associate/associated/\n\nAck, I'll take it in.\n\n>\n> > > + * \\sa Buffer::setRequest()\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Mark a buffer as cancel by setting its status to BufferCancelled\n> > > */\n> > > @@ -259,6 +279,29 @@ void Buffer::cancel()\n> > > \tstatus_ = BufferCancelled;\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn Buffer::setRequest()\n> > > + * \\brief Set the request this buffer belongs to\n> > > + *\n> > > + * Buffers are associated to Streams in a Request, which is then sent to the\n> > > + * Camera for processing. This method stores in the Buffer a pointer to the\n> > > + * Request this Buffer is part of, for later retrieval through the\n> > > + * Buffer::request() method.\n> > > + *\n> > > + * Buffers are associated to requests at Request::prepare() time and said\n> > > + * association is valid until the buffer does not complete at\n> > > + * Request::completeBuffer() time. Before and after the buffer completion\n> > > + * interval (the time between when the request is queued to the Camera, and\n> > > + * the buffer is marked as 'complete' by pipeline handlers) the reference to\n> > > + * Request is set to nullptr.\n> >\n> > IMHO most of this should be rephrases and moved to the documentation for\n> > Request::prepare(). What should be documented here is how this call\n> > modifies the state of the Buffer object.\n> >\n> > > + *\n> > > + * The intended caller of this methods is the Request class at\n> > > + * Request::prepare() time, when it stores a reference to itself through a\n> > > + * call to this method, and at Request::completeBuffer() time, where it\n> > > + * invalidates the request_ reference by calling this method with a nullptr as\n> > > + * argument.\n> > > + */\n> >\n> > I'm not saying it's wrong, but is it strictly necessary to document\n> > intended callers for an internal API? I agree this is most needed for\n> > interfaces deigned to be called at specific points in either\n> > applications or pipeline handles (and later 3A).\n>\n> It's not strictly necessary, but it can avoid abuses for methods that\n> are meant to be called from a very specific place. I would however\n> simplify this paragraph by just stating\n>\n> * The intended callers are the Request::prepare() and Request::completeBuffer()\n> * methods.\n>\n> With the life cycle explanation moved to the Request documentation, as\n> proposed above.\n>\n\nAck!\n\nThanks\n j\n\n> > > +\n> > > /**\n> > > * \\class BufferPool\n> > > * \\brief A pool of buffers\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 7d865fa329ea..ce680835cec2 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -630,7 +630,7 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> > > */\n> > > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> > > {\n> > > -\tRequest *request = queuedRequests_.front();\n> > > +\tRequest *request = buffer->request();\n> > >\n> > > \tpipe_->completeBuffer(camera_, request, buffer);\n> > > \tpipe_->completeRequest(camera_, request);\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index 7fa034e6c747..6b175e125596 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -130,6 +130,8 @@ int Request::prepare()\n> > > {\n> > > \tfor (auto const &pair : bufferMap_) {\n> > > \t\tBuffer *buffer = pair.second;\n> > > +\n> > > +\t\tbuffer->setRequest(this);\n> > > \t\tpending_.insert(buffer);\n> > > \t}\n> > >\n> > > @@ -166,6 +168,8 @@ bool Request::completeBuffer(Buffer *buffer)\n> > > \tint ret = pending_.erase(buffer);\n> > > \tASSERT(ret == 1);\n> > >\n> > > +\tbuffer->setRequest(nullptr);\n> > > +\n> > > \treturn empty();\n> > > }\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B2EA60DB4\n\tfor ;\n\tTue, 16 Apr 2019 15:23:26 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 79517200014;\n\tTue, 16 Apr 2019 13:23:25 +0000 (UTC)"],"Date":"Tue, 16 Apr 2019 15:24:17 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190416132417.krhj2ht2jwunut4r@uno.localdomain>","References":"<20190415231859.9747-1-jacopo@jmondi.org>\n\t<20190415231859.9747-8-jacopo@jmondi.org>\n\t<20190416001629.GD16492@bigcity.dyn.berto.se>\n\t<20190416113559.GF4832@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"phxwshswf3hldddv\"","Content-Disposition":"inline","In-Reply-To":"<20190416113559.GF4832@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store\n\tRequest reference in Buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 16 Apr 2019 13:23:26 -0000"}}]