[{"id":1405,"web_url":"https://patchwork.libcamera.org/comment/1405/","msgid":"<20190416201434.GD28515@bigcity.dyn.berto.se>","date":"2019-04-16T20:14:34","subject":"Re: [libcamera-devel] [PATCH v6 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 15:42:10 +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> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/buffer.h | 6 +++++\n> src/libcamera/buffer.cpp | 34 +++++++++++++++++++++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> src/libcamera/request.cpp | 4 ++++\n> 4 files changed, 44 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 0c844d126a27..8f9b42e39339 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 *request) { request_ = request; }\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..550091c998a6 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,25 @@ 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 handlers that\n\ns/callers/caller/\n\n> + * needs to associated a buffer to the request it has been queued to.\n\ns/it has been queued to/belongs to/\n\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.\n\nA Buffer is associated to a request by Request::prepare() and the \nassociation is valid until the buffer completes.\n\n> + * 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> + * the request is set to nullptr.\n\nI would drop this.\n\n> + *\n> + * \\return The Request the Buffer belongs to, or nullptr if the buffer is\n> + * either completed or not associated with a request\n> + * \\sa Buffer::setRequest()\n> + */\n> +\n> /**\n> * \\brief Mark a buffer as cancel by setting its status to BufferCancelled\n> */\n> @@ -259,6 +278,19 @@ 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\nI would drop this. Or if you really want rewrite it to drop the talk of \nstreams, it's only confusing. All setRequest() does is associate a \nbuffer to a request. How it all fits together should be described at a \nhigher level and not in a function description.\n\n> + *\n> + * The intended callers are the Request::prepare() and Request::completeBuffer()\n> + * methods.\n\ns/the//\ns/methods//\n\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 f96e8763bce9..6f5747fb1505 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -633,7 +633,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 1946845b91f4..9ae62f632308 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -144,6 +144,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> @@ -180,6 +182,8 @@ bool Request::completeBuffer(Buffer *buffer)\n> \tint ret = pending_.erase(buffer);\n> \tASSERT(ret == 1);\n> \n> +\tbuffer->setRequest(nullptr);\n> +\n> \treturn !hasPendingBuffers();\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-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D465360004\n\tfor ;\n\tTue, 16 Apr 2019 22:14:35 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id i68so6096919lfi.10\n\tfor ;\n\tTue, 16 Apr 2019 13:14:35 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tp18sm10105687ljp.1.2019.04.16.13.14.34\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 16 Apr 2019 13:14:34 -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=FJeElcpSsLPg6VtphGj3bgaeLE84GJcf94lhRIwCVeQ=;\n\tb=UuebLfwbDvSUvEcyZl7mHFFLMYtl728e81StKITcXm/lxXGSaR2wd+OTABZ5ezSsBI\n\tg/qbO6u6nuv2SHzqKn022g1nQa7dp158GHWqbXZMHIiNsNL/aMJXSi2ICowVKourvVAs\n\tHTlyL1G4M1D/3mM9N5a2PB2C3nMRFcQTXjyV33jIMO+CNY32qybGEgmjo4V7b19kF0SW\n\tJoybf6y9Eohe1cCpp83b2urLQS+Ronr+wA9K7OeiFU9pHSTQjm6CdPUK7Reqx2e+Rm8t\n\tAExkj4hfhUQgF5gEatrmuZ/YzOYoa82+U+DSmsF98W74rGxhmYSzQXH2OcNRw3huuqdD\n\ttldg==","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=FJeElcpSsLPg6VtphGj3bgaeLE84GJcf94lhRIwCVeQ=;\n\tb=dWZ+VcnMPtFMWHBJyMKgoWRTYQJ9zqumvMGHkVwFJTjnllukSIy2+czi1/pAclRZF/\n\tJdjpDPJhu3AoY/2vrTr1BEQbcXVa6LGCG0NPcyRiSiLCWhpJuoQ2wLu3q4Cs3uVT3iZZ\n\tEOQAg3Q/2QyqIBhxwGSypXIR6Xx+FMXfmOOBoD9vo319JfhijhYPuBArXxp80Q3YyJ7B\n\tPR1/vIg6E1BnV9B5gYQXCeXU+sL4DY673vtaiDTSejx8WfIia17c1G/DdH4y7n8cEPMw\n\t2tnuMO1MgIohH96bT46KvkBg2fATSoKCx8M4cHT1MUPrOziQWh/cmeBem3fLKuzoOC70\n\tIaCw==","X-Gm-Message-State":"APjAAAW5ai3d2OclKQjzPts6PR7NvkS5zuuv6ZyH7ejBGUnChVrfqhsk\n\tZgA7fDrou49nhhV1bGGqQjTXv5P0AWA=","X-Google-Smtp-Source":"APXvYqyEd5ckUMARnqCK5T3TtzcJHkqusYklWO5+f10WsbOps5xorvanKTfVpEFpFoQxUODbkXvPcw==","X-Received":"by 2002:ac2:554a:: with SMTP id\n\tl10mr12305850lfk.45.1555445675230; \n\tTue, 16 Apr 2019 13:14:35 -0700 (PDT)","Date":"Tue, 16 Apr 2019 22:14:34 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416201434.GD28515@bigcity.dyn.berto.se>","References":"<20190416134210.21097-1-jacopo@jmondi.org>\n\t<20190416134210.21097-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":"<20190416134210.21097-8-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v6 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 20:14:36 -0000"}},{"id":1414,"web_url":"https://patchwork.libcamera.org/comment/1414/","msgid":"<20190416222943.GL4822@pendragon.ideasonboard.com>","date":"2019-04-16T22:29:43","subject":"Re: [libcamera-devel] [PATCH v6 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 10:14:34PM +0200, Niklas Söderlund wrote:\n> On 2019-04-16 15:42:10 +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\ns/this buffer/the buffer/\n\n> > \n> > As Buffers might outlive the Request they are associated with, the\n\ns/Buffers/buffers/ (let's keep capitalized class names invariable)\n\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\ns/since/from/\n\n> > gets marked as completed).\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/buffer.h | 6 +++++\n> > src/libcamera/buffer.cpp | 34 +++++++++++++++++++++++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > src/libcamera/request.cpp | 4 ++++\n> > 4 files changed, 44 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 0c844d126a27..8f9b42e39339 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 *request) { request_ = request; }\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..550091c998a6 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,25 @@ 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 handlers that\n> \n> s/callers/caller/\n\n\"callers ... are\" or \"caller ... is\". As there are multiple completion\nhandlers, I think the plural is the correct form.\n\n> > + * needs to associated a buffer to the request it has been queued to.\n\ns/needs to associated/need to associate/\n\n> s/it has been queued to/belongs to/\n\ns/belongs/it belongs/ :-)\n\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.\n> \n> A Buffer is associated to a request by Request::prepare() and the \n> association is valid until the buffer completes.\n> \n> > + * 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> > + * the request is set to nullptr.\n> \n> I would drop this.\n\nI would replace it with \"The returned request pointer is valid only\nduring that interval.\" as I think it's important to document the\nlifetime of the pointer.\n\n> > + *\n> > + * \\return The Request the Buffer belongs to, or nullptr if the buffer is\n> > + * either completed or not associated with a request\n> > + * \\sa Buffer::setRequest()\n> > + */\n> > +\n> > /**\n> > * \\brief Mark a buffer as cancel by setting its status to BufferCancelled\n> > */\n> > @@ -259,6 +278,19 @@ 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> I would drop this. Or if you really want rewrite it to drop the talk of \n> streams, it's only confusing. All setRequest() does is associate a \n> buffer to a request. How it all fits together should be described at a \n> higher level and not in a function description.\n> \n> > + *\n> > + * The intended callers are the Request::prepare() and Request::completeBuffer()\n> > + * methods.\n> \n> s/the//\n> s/methods//\n> \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 f96e8763bce9..6f5747fb1505 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -633,7 +633,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 1946845b91f4..9ae62f632308 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -144,6 +144,8 @@ int Request::prepare()\n> > \n> > \tfor (auto const &pair : bufferMap_) {\n> > \t\tBuffer *buffer = pair.second;\n> > +\n\nYou can remove this blank line.\n\n> > +\t\tbuffer->setRequest(this);\n> > \t\tpending_.insert(buffer);\n> > \t}\n> > \n> > @@ -180,6 +182,8 @@ bool Request::completeBuffer(Buffer *buffer)\n> > \tint ret = pending_.erase(buffer);\n> > \tASSERT(ret == 1);\n> > \n> > +\tbuffer->setRequest(nullptr);\n> > +\n> > \treturn !hasPendingBuffers();\n> > }\n> >","headers":{"Return-Path":"","Received":["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 2DE4660004\n\tfor ;\n\tWed, 17 Apr 2019 00:29:53 +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 71AE0E2;\n\tWed, 17 Apr 2019 00:29:52 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555453792;\n\tbh=7fbToasP4gFhJzmYXf+kOY3YCFrT+z11fq6R0Ud4MHE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OydEavd/SsuyLOvk3bg4wQEvMFDln+h4gSd+w97CcQK0kOsgK+Rg25DMEF7GNujDa\n\trL5dqpC9e9tRTG5nfd94LVN1UDUVkYTIcsV7u8pSHlAw6Nt+97NBsYRg+zVhbqiVqh\n\t8FfT02wdIoFt0o8zopybkqB7eJ15jKosl/j9E8to=","Date":"Wed, 17 Apr 2019 01:29:43 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190416222943.GL4822@pendragon.ideasonboard.com>","References":"<20190416134210.21097-1-jacopo@jmondi.org>\n\t<20190416134210.21097-8-jacopo@jmondi.org>\n\t<20190416201434.GD28515@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":"<20190416201434.GD28515@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 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 22:29:53 -0000"}}]