[{"id":4196,"web_url":"https://patchwork.libcamera.org/comment/4196/","msgid":"<20200323122606.GF4768@pendragon.ideasonboard.com>","date":"2020-03-23T12:26:06","subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Mar 16, 2020 at 03:41:45AM +0100, Niklas Söderlund wrote:\n> Instead of unconditionally cycling buffers between the CIO2 and IMGU\n> pick a buffer when a request is queued to the pipeline. This is needed\n> if operations are the be applied to the buffer coming from CIO2 with\n\ns/the be/to be/\n\n> parameters coming from a Request.\n> \n> The approach to pick a CIO2 buffer when a request is queued is similar\n> to other pipelines, where parameters and statistic buffers are picked\n> this way.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++-----\n> 1 file changed, 58 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -119,6 +120,10 @@ public:\n> \tint allocateBuffers();\n> \tvoid freeBuffers();\n> \n> +\tFrameBuffer *getBuffer();\n> +\tvoid putBuffer(FrameBuffer *buffer);\n> +\tint queueBuffer(FrameBuffer *buffer);\n> +\n> \tint start();\n> \tint stop();\n> \n> @@ -130,6 +135,7 @@ public:\n> \n> private:\n> \tstd::vector> buffers_;\n> +\tstd::queue availableBuffers_;\n> };\n> \n> class IPU3Stream : public Stream\n> @@ -157,6 +163,8 @@ public:\n> \tvoid imguInputBufferReady(FrameBuffer *buffer);\n> \tvoid cio2BufferReady(FrameBuffer *buffer);\n> \n> +\tstd::map cio2ToRequest_;\n\nWe have a Request pointer in FrameBuffer to track this. It's a private\nfield, pipeline handlers can't set it, but I think it's a good time to\nfix that issue properly.\n\n> +\n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> \n> @@ -717,11 +725,20 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \n> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> {\n> +\tIPU3CameraData *data = cameraData(camera);\n> +\tFrameBuffer *buffer;\n> \tint error = 0;\n> \n> +\t/* Get a CIO2 buffer and track it it's used for this request. */\n> +\tbuffer = data->cio2_.getBuffer();\n> +\tif (!buffer)\n> +\t\treturn -EINVAL;\n\nWe can return an error for now, but we'll really have to handle this\nbetter. If the application queues more request than the pipeline handler\ninternal depth, they need to be put in a queue somewhere. I wonder if we\nshould try address this already.\n\n> +\tdata->cio2ToRequest_[buffer] = request;\n> +\tdata->cio2_.queueBuffer(buffer);\n> +\n> \tfor (auto it : request->buffers()) {\n> \t\tIPU3Stream *stream = static_cast(it.first);\n> -\t\tFrameBuffer *buffer = it.second;\n> +\t\tbuffer = it.second;\n> \n> \t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> \t\tif (ret < 0)\n> @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> \t\treturn;\n> \n> -\tcio2_.output_->queueBuffer(buffer);\n> +\t/* Remove association between CIO2 buffer an Request. */\n> +\tcio2ToRequest_.erase(buffer);\n> +\tcio2_.putBuffer(buffer);\n> }\n> \n> /**\n> @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size,\n> int CIO2Device::allocateBuffers()\n> {\n> \tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> -\tif (ret < 0)\n> +\tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to allocate CIO2 buffers\";\n\nThere's already a message printed by the V4L2VideoDevice class, do we\nneed another one here ?\n\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfor (std::unique_ptr &buffer : buffers_)\n> +\t\tavailableBuffers_.push(buffer.get());\n> \n> \treturn ret;\n> }\n> \n> void CIO2Device::freeBuffers()\n> {\n> +\twhile (!availableBuffers_.empty())\n> +\t\tavailableBuffers_.pop();\n> +\n\nHow about\n\n\tavailableBuffers_ = {};\n\n?\n\n> \tbuffers_.clear();\n> \n> \tif (output_->releaseBuffers())\n> \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> }\n> \n> +FrameBuffer *CIO2Device::getBuffer()\n> +{\n> +\tif (availableBuffers_.empty()) {\n> +\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tFrameBuffer *buffer = availableBuffers_.front();\n> +\n> +\tavailableBuffers_.pop();\n> +\n> +\treturn buffer;\n> +}\n> +\n> +void CIO2Device::putBuffer(FrameBuffer *buffer)\n> +{\n> +\tavailableBuffers_.push(buffer);\n> +}\n> +\n> +int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> +{\n> +\tint ret = output_->queueBuffer(buffer);\n> +\tif (ret)\n> +\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> +\n> +\treturn ret;\n\nDo we need this wrapper, can't we call\ncio2_.output_->queueBuffer(buffer) in the caller like we used to ?\n\n> +}\n> +\n> int CIO2Device::start()\n> {\n> -\tfor (const std::unique_ptr &buffer : buffers_) {\n> -\t\tint ret = output_->queueBuffer(buffer.get());\n> -\t\tif (ret) {\n> -\t\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> -\t\t\treturn ret;\n> -\t\t}\n> -\t}\n> -\n> \treturn output_->streamOn();\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 9E86F6041A\n\tfor ;\n\tMon, 23 Mar 2020 13:26:16 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D4F16308;\n\tMon, 23 Mar 2020 13:26:15 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584966376;\n\tbh=TfHKoJ5RKMz/k02+DUCvvOypyf3LybuHVn4oRJ7D5fs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oqg5WMY7j6aDpovO3dB2tVGMKzqBC5hhbzr38Qx20FK5OBhZGfi+dUsG0uABTxRhq\n\tFFzJBwYAJdvvlqrJEJ108fn8TRZ2LwOznx72HdZivV1GzaQ7O6Lpkiq0UTtjvqx/F2\n\taG9eXOI3nOjzZ3Q7Ez7JhVPMMSLhA16L0RiFF4kg=","Date":"Mon, 23 Mar 2020 14:26:06 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200323122606.GF4768@pendragon.ideasonboard.com>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200316024146.2474424-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 23 Mar 2020 12:26:16 -0000"}},{"id":4255,"web_url":"https://patchwork.libcamera.org/comment/4255/","msgid":"<20200324123332.GB625480@oden.dyn.berto.se>","date":"2020-03-24T12:33:32","subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-03-23 14:26:06 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 16, 2020 at 03:41:45AM +0100, Niklas Söderlund wrote:\n> > Instead of unconditionally cycling buffers between the CIO2 and IMGU\n> > pick a buffer when a request is queued to the pipeline. This is needed\n> > if operations are the be applied to the buffer coming from CIO2 with\n> \n> s/the be/to be/\n> \n> > parameters coming from a Request.\n> > \n> > The approach to pick a CIO2 buffer when a request is queued is similar\n> > to other pipelines, where parameters and statistic buffers are picked\n> > this way.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++-----\n> > 1 file changed, 58 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -119,6 +120,10 @@ public:\n> > \tint allocateBuffers();\n> > \tvoid freeBuffers();\n> > \n> > +\tFrameBuffer *getBuffer();\n> > +\tvoid putBuffer(FrameBuffer *buffer);\n> > +\tint queueBuffer(FrameBuffer *buffer);\n> > +\n> > \tint start();\n> > \tint stop();\n> > \n> > @@ -130,6 +135,7 @@ public:\n> > \n> > private:\n> > \tstd::vector> buffers_;\n> > +\tstd::queue availableBuffers_;\n> > };\n> > \n> > class IPU3Stream : public Stream\n> > @@ -157,6 +163,8 @@ public:\n> > \tvoid imguInputBufferReady(FrameBuffer *buffer);\n> > \tvoid cio2BufferReady(FrameBuffer *buffer);\n> > \n> > +\tstd::map cio2ToRequest_;\n> \n> We have a Request pointer in FrameBuffer to track this. It's a private\n> field, pipeline handlers can't set it, but I think it's a good time to\n> fix that issue properly.\n\nGood idea, I will try and improve on this for v1.\n\n> \n> > +\n> > \tCIO2Device cio2_;\n> > \tImgUDevice *imgu_;\n> > \n> > @@ -717,11 +725,20 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > \n> > int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > {\n> > +\tIPU3CameraData *data = cameraData(camera);\n> > +\tFrameBuffer *buffer;\n> > \tint error = 0;\n> > \n> > +\t/* Get a CIO2 buffer and track it it's used for this request. */\n> > +\tbuffer = data->cio2_.getBuffer();\n> > +\tif (!buffer)\n> > +\t\treturn -EINVAL;\n> \n> We can return an error for now, but we'll really have to handle this\n> better. If the application queues more request than the pipeline handler\n> internal depth, they need to be put in a queue somewhere. I wonder if we\n> should try address this already.\n\nI'm all for starting work on this topic now. But do you think it really \nshould be solved in individual pipeline handlers and not on a framework \nlevel?\n\nWhen I thought about this in the past I always imaged the pipeline to \nreport its min and max pipeline depth and then having the framework \ndoing the heavy lifting of keeping and flushing a queue. If so I think \nit the behavior here is correct, but if you think pipelines shall deal \nwith this it should be reworked.\n\n> \n> > +\tdata->cio2ToRequest_[buffer] = request;\n> > +\tdata->cio2_.queueBuffer(buffer);\n> > +\n> > \tfor (auto it : request->buffers()) {\n> > \t\tIPU3Stream *stream = static_cast(it.first);\n> > -\t\tFrameBuffer *buffer = it.second;\n> > +\t\tbuffer = it.second;\n> > \n> > \t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> > \t\tif (ret < 0)\n> > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > \t\treturn;\n> > \n> > -\tcio2_.output_->queueBuffer(buffer);\n> > +\t/* Remove association between CIO2 buffer an Request. */\n> > +\tcio2ToRequest_.erase(buffer);\n> > +\tcio2_.putBuffer(buffer);\n> > }\n> > \n> > /**\n> > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size,\n> > int CIO2Device::allocateBuffers()\n> > {\n> > \tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > -\tif (ret < 0)\n> > +\tif (ret < 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to allocate CIO2 buffers\";\n> \n> There's already a message printed by the V4L2VideoDevice class, do we\n> need another one here ?\n\nI'm all for dropping it ;-)\n\n> \n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfor (std::unique_ptr &buffer : buffers_)\n> > +\t\tavailableBuffers_.push(buffer.get());\n> > \n> > \treturn ret;\n> > }\n> > \n> > void CIO2Device::freeBuffers()\n> > {\n> > +\twhile (!availableBuffers_.empty())\n> > +\t\tavailableBuffers_.pop();\n> > +\n> \n> How about\n> \n> \tavailableBuffers_ = {};\n\nNice idea.\n\n> \n> ?\n> \n> > \tbuffers_.clear();\n> > \n> > \tif (output_->releaseBuffers())\n> > \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> > }\n> > \n> > +FrameBuffer *CIO2Device::getBuffer()\n> > +{\n> > +\tif (availableBuffers_.empty()) {\n> > +\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tFrameBuffer *buffer = availableBuffers_.front();\n> > +\n> > +\tavailableBuffers_.pop();\n> > +\n> > +\treturn buffer;\n> > +}\n> > +\n> > +void CIO2Device::putBuffer(FrameBuffer *buffer)\n> > +{\n> > +\tavailableBuffers_.push(buffer);\n> > +}\n> > +\n> > +int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> > +{\n> > +\tint ret = output_->queueBuffer(buffer);\n> > +\tif (ret)\n> > +\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> > +\n> > +\treturn ret;\n> \n> Do we need this wrapper, can't we call\n> cio2_.output_->queueBuffer(buffer) in the caller like we used to ?\n\nNot really, I added it as I think this driver is quiet messy and \nbreaking things apart could be a good thing. Will drop for v1 and then \nwe can refactor on top to make it more readable.\n\n> \n> > +}\n> > +\n> > int CIO2Device::start()\n> > {\n> > -\tfor (const std::unique_ptr &buffer : buffers_) {\n> > -\t\tint ret = output_->queueBuffer(buffer.get());\n> > -\t\tif (ret) {\n> > -\t\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> > -\t\t\treturn ret;\n> > -\t\t}\n> > -\t}\n> > -\n> > \treturn output_->streamOn();\n> > }\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2EDAC60412\n\tfor ;\n\tTue, 24 Mar 2020 13:33:35 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id c5so7576560lfp.5\n\tfor ;\n\tTue, 24 Mar 2020 05:33:35 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\td23sm344061lji.62.2020.03.24.05.33.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 24 Mar 2020 05:33:33 -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\tbh=MnxRIykTiTY2DGT0GTAB89Zw+8Pl3rE+c6TF+sUfm14=;\n\tb=0dDvB7hNjmhxZe/Ho+L052q8IQ+IygQm/2MUUNjLVtNwZ67QuKP3OG3UuCH0+Q71aC\n\tJC7D9o7BHuF9bun5UhCVhYFReNniLnzgPBaHAh85Qi77T7ax+aw/wR8xCLTxILIxHnFf\n\twTzvbuq52+ZmzWZcl3TVOT9Bzc3B4NerDyL2rk4O8mzcB3S8StUSX+8slOpb2k4GKG5F\n\tfeqgKqKkbN1Tj1T6ZprAfQ0wnhJPoST93L1s2YWz/bmr4Iycyqd21X3b/aX10+WIl0dN\n\tLc6zj9vmHhGHMWsdx22BrNTfiSuUReFhmrtPm3nhSw3PdS4JVEvUo5ndclJ/MxVFEgUa\n\tpSBQ==","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;\n\tbh=MnxRIykTiTY2DGT0GTAB89Zw+8Pl3rE+c6TF+sUfm14=;\n\tb=IlHLtewW7TDHILarb1+PHzjDPhyk/bLR+ENSiPtChfAid20jHgxOkzSkQUZzSjpGGY\n\tuGpLpzxGefBkSgHdUb2T2l0k3X9llT/FwxTq4CcgzvEGA9pLaWnF2QlgsfJsCgLrt9+p\n\tG37CYQW6P8GQguhgpqUa0LM/IZFarXU2TZPpqZ3Eh1WZwxLbRd4ysvUuiKMchnywfSe4\n\tf3On1RsG6Ru1RTVqXhSTKx+ARUHdtPWbPoL704FIs4zs2sE1hF+7VkF8qOQO57E0uO79\n\toNXB4JnCzXP1UJD3Kiq7WNvgMZpvLXhFHP2Xa843KsmUvRVvITScqIjP1QjQjlru2rUX\n\tNvLQ==","X-Gm-Message-State":"ANhLgQ13BcwEk7ZonvRB404pZXwF7ZnhxwnWxNkGkaMdPRnyjLzKRkAF\n\tpVGPye5bWAVHovltJ1JzXSqXLA==","X-Google-Smtp-Source":"ADFU+vt3bSE/6ny61okIctszIblz3PntseD8e06wV/y5HIlcnLKU9O+H9t04GtSFzXYABzckLfhsyQ==","X-Received":"by 2002:ac2:5f75:: with SMTP id\n\tc21mr5832733lfc.194.1585053214319; \n\tTue, 24 Mar 2020 05:33:34 -0700 (PDT)","Date":"Tue, 24 Mar 2020 13:33:32 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200324123332.GB625480@oden.dyn.berto.se>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-6-niklas.soderlund@ragnatech.se>\n\t<20200323122606.GF4768@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200323122606.GF4768@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 24 Mar 2020 12:33:35 -0000"}},{"id":4260,"web_url":"https://patchwork.libcamera.org/comment/4260/","msgid":"<20200324130216.GE24971@pendragon.ideasonboard.com>","date":"2020-03-24T13:02:16","subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Mar 24, 2020 at 01:33:32PM +0100, Niklas Söderlund wrote:\n> On 2020-03-23 14:26:06 +0200, Laurent Pinchart wrote:\n> > On Mon, Mar 16, 2020 at 03:41:45AM +0100, Niklas Söderlund wrote:\n> > > Instead of unconditionally cycling buffers between the CIO2 and IMGU\n> > > pick a buffer when a request is queued to the pipeline. This is needed\n> > > if operations are the be applied to the buffer coming from CIO2 with\n> > \n> > s/the be/to be/\n> > \n> > > parameters coming from a Request.\n> > > \n> > > The approach to pick a CIO2 buffer when a request is queued is similar\n> > > to other pipelines, where parameters and statistic buffers are picked\n> > > this way.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++-----\n> > > 1 file changed, 58 insertions(+), 11 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \n> > > @@ -119,6 +120,10 @@ public:\n> > > \tint allocateBuffers();\n> > > \tvoid freeBuffers();\n> > > \n> > > +\tFrameBuffer *getBuffer();\n> > > +\tvoid putBuffer(FrameBuffer *buffer);\n> > > +\tint queueBuffer(FrameBuffer *buffer);\n> > > +\n> > > \tint start();\n> > > \tint stop();\n> > > \n> > > @@ -130,6 +135,7 @@ public:\n> > > \n> > > private:\n> > > \tstd::vector> buffers_;\n> > > +\tstd::queue availableBuffers_;\n> > > };\n> > > \n> > > class IPU3Stream : public Stream\n> > > @@ -157,6 +163,8 @@ public:\n> > > \tvoid imguInputBufferReady(FrameBuffer *buffer);\n> > > \tvoid cio2BufferReady(FrameBuffer *buffer);\n> > > \n> > > +\tstd::map cio2ToRequest_;\n> > \n> > We have a Request pointer in FrameBuffer to track this. It's a private\n> > field, pipeline handlers can't set it, but I think it's a good time to\n> > fix that issue properly.\n> \n> Good idea, I will try and improve on this for v1.\n> \n> > > +\n> > > \tCIO2Device cio2_;\n> > > \tImgUDevice *imgu_;\n> > > \n> > > @@ -717,11 +725,20 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > \n> > > int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > > {\n> > > +\tIPU3CameraData *data = cameraData(camera);\n> > > +\tFrameBuffer *buffer;\n> > > \tint error = 0;\n> > > \n> > > +\t/* Get a CIO2 buffer and track it it's used for this request. */\n> > > +\tbuffer = data->cio2_.getBuffer();\n> > > +\tif (!buffer)\n> > > +\t\treturn -EINVAL;\n> > \n> > We can return an error for now, but we'll really have to handle this\n> > better. If the application queues more request than the pipeline handler\n> > internal depth, they need to be put in a queue somewhere. I wonder if we\n> > should try address this already.\n> \n> I'm all for starting work on this topic now. But do you think it really \n> should be solved in individual pipeline handlers and not on a framework \n> level?\n> \n> When I thought about this in the past I always imaged the pipeline to \n> report its min and max pipeline depth and then having the framework \n> doing the heavy lifting of keeping and flushing a queue. If so I think \n> it the behavior here is correct, but if you think pipelines shall deal \n> with this it should be reworked.\n\nI think the interface between the camera and pipeline handler should\nremain as simple with possible, with high-level entry points. We can\nthen implement helpers, in the form of classes deriving from\nPipelineHandler, or classes they implement helper objects (similar in\nconcept to the V4L2BufferCache for instance) to avoid code duplication.\nIt's a bit early to tell what form this will take, we probably need to\nlook at two examples at least.\n\n> > > +\tdata->cio2ToRequest_[buffer] = request;\n> > > +\tdata->cio2_.queueBuffer(buffer);\n> > > +\n> > > \tfor (auto it : request->buffers()) {\n> > > \t\tIPU3Stream *stream = static_cast(it.first);\n> > > -\t\tFrameBuffer *buffer = it.second;\n> > > +\t\tbuffer = it.second;\n> > > \n> > > \t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> > > \t\tif (ret < 0)\n> > > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n> > > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > > \t\treturn;\n> > > \n> > > -\tcio2_.output_->queueBuffer(buffer);\n> > > +\t/* Remove association between CIO2 buffer an Request. */\n> > > +\tcio2ToRequest_.erase(buffer);\n> > > +\tcio2_.putBuffer(buffer);\n> > > }\n> > > \n> > > /**\n> > > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size,\n> > > int CIO2Device::allocateBuffers()\n> > > {\n> > > \tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > > -\tif (ret < 0)\n> > > +\tif (ret < 0) {\n> > > \t\tLOG(IPU3, Error) << \"Failed to allocate CIO2 buffers\";\n> > \n> > There's already a message printed by the V4L2VideoDevice class, do we\n> > need another one here ?\n> \n> I'm all for dropping it ;-)\n> \n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tfor (std::unique_ptr &buffer : buffers_)\n> > > +\t\tavailableBuffers_.push(buffer.get());\n> > > \n> > > \treturn ret;\n> > > }\n> > > \n> > > void CIO2Device::freeBuffers()\n> > > {\n> > > +\twhile (!availableBuffers_.empty())\n> > > +\t\tavailableBuffers_.pop();\n> > > +\n> > \n> > How about\n> > \n> > \tavailableBuffers_ = {};\n> \n> Nice idea.\n> \n> > ?\n> > \n> > > \tbuffers_.clear();\n> > > \n> > > \tif (output_->releaseBuffers())\n> > > \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> > > }\n> > > \n> > > +FrameBuffer *CIO2Device::getBuffer()\n> > > +{\n> > > +\tif (availableBuffers_.empty()) {\n> > > +\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\tFrameBuffer *buffer = availableBuffers_.front();\n> > > +\n> > > +\tavailableBuffers_.pop();\n> > > +\n> > > +\treturn buffer;\n> > > +}\n> > > +\n> > > +void CIO2Device::putBuffer(FrameBuffer *buffer)\n> > > +{\n> > > +\tavailableBuffers_.push(buffer);\n> > > +}\n> > > +\n> > > +int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> > > +{\n> > > +\tint ret = output_->queueBuffer(buffer);\n> > > +\tif (ret)\n> > > +\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> > > +\n> > > +\treturn ret;\n> > \n> > Do we need this wrapper, can't we call\n> > cio2_.output_->queueBuffer(buffer) in the caller like we used to ?\n> \n> Not really, I added it as I think this driver is quiet messy and \n> breaking things apart could be a good thing. Will drop for v1 and then \n> we can refactor on top to make it more readable.\n> \n> > > +}\n> > > +\n> > > int CIO2Device::start()\n> > > {\n> > > -\tfor (const std::unique_ptr &buffer : buffers_) {\n> > > -\t\tint ret = output_->queueBuffer(buffer.get());\n> > > -\t\tif (ret) {\n> > > -\t\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> > > -\t\t\treturn ret;\n> > > -\t\t}\n> > > -\t}\n> > > -\n> > > \treturn output_->streamOn();\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 E4C8F60412\n\tfor ;\n\tTue, 24 Mar 2020 14:02:18 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B8A6308;\n\tTue, 24 Mar 2020 14:02:18 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585054938;\n\tbh=5/1APkBpr4gA4YqosOlgF3d0lNlDuyifiZUdHJvnpM8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WuuoO8kVeI2ZnMP2rfydc0QEwW221YsFAFDpvpgGCydGrueUghsaEy5ck+neHN+DO\n\tRt6E9sDoss25F8C6z51KjJCcsEdZoIswni54BaLHfnJYSif+OS4enPBkIEi7kBIfiZ\n\tyQsYEXyMJd5xDWzFrU2tPBc39TeBbFEXycuKFHok=","Date":"Tue, 24 Mar 2020 15:02:16 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200324130216.GE24971@pendragon.ideasonboard.com>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-6-niklas.soderlund@ragnatech.se>\n\t<20200323122606.GF4768@pendragon.ideasonboard.com>\n\t<20200324123332.GB625480@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200324123332.GB625480@oden.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 24 Mar 2020 13:02:19 -0000"}},{"id":4279,"web_url":"https://patchwork.libcamera.org/comment/4279/","msgid":"<20200325111339.mtdsr4ct6mujf2ru@uno.localdomain>","date":"2020-03-25T11:13:39","subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Mar 16, 2020 at 03:41:45AM +0100, Niklas Söderlund wrote:\n> Instead of unconditionally cycling buffers between the CIO2 and IMGU\n> pick a buffer when a request is queued to the pipeline. This is needed\n> if operations are the be applied to the buffer coming from CIO2 with\n> parameters coming from a Request.\n>\n> The approach to pick a CIO2 buffer when a request is queued is similar\n> to other pipelines, where parameters and statistic buffers are picked\n> this way.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++-----\n> 1 file changed, 58 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -119,6 +120,10 @@ public:\n> \tint allocateBuffers();\n> \tvoid freeBuffers();\n>\n> +\tFrameBuffer *getBuffer();\n> +\tvoid putBuffer(FrameBuffer *buffer);\n> +\tint queueBuffer(FrameBuffer *buffer);\n> +\n> \tint start();\n> \tint stop();\n>\n> @@ -130,6 +135,7 @@ public:\n>\n> private:\n> \tstd::vector> buffers_;\n> +\tstd::queue availableBuffers_;\n> };\n>\n> class IPU3Stream : public Stream\n> @@ -157,6 +163,8 @@ public:\n> \tvoid imguInputBufferReady(FrameBuffer *buffer);\n> \tvoid cio2BufferReady(FrameBuffer *buffer);\n>\n> +\tstd::map cio2ToRequest_;\n> +\n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n>\n> @@ -717,11 +725,20 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>\n> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> {\n> +\tIPU3CameraData *data = cameraData(camera);\n> +\tFrameBuffer *buffer;\n> \tint error = 0;\n>\n> +\t/* Get a CIO2 buffer and track it it's used for this request. */\n> +\tbuffer = data->cio2_.getBuffer();\n> +\tif (!buffer)\n> +\t\treturn -EINVAL;\n\nvery minor nit: empty line\n\nI don't have much to add to Laurent's review, I'm wondering if we need\nsome kind of locking when accessing the queue though.\n\n> +\tdata->cio2ToRequest_[buffer] = request;\n> +\tdata->cio2_.queueBuffer(buffer);\n> +\n> \tfor (auto it : request->buffers()) {\n> \t\tIPU3Stream *stream = static_cast(it.first);\n> -\t\tFrameBuffer *buffer = it.second;\n> +\t\tbuffer = it.second;\n>\n> \t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> \t\tif (ret < 0)\n> @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> \t\treturn;\n>\n> -\tcio2_.output_->queueBuffer(buffer);\n> +\t/* Remove association between CIO2 buffer an Request. */\n> +\tcio2ToRequest_.erase(buffer);\n> +\tcio2_.putBuffer(buffer);\n> }\n>\n> /**\n> @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size,\n> int CIO2Device::allocateBuffers()\n> {\n> \tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> -\tif (ret < 0)\n> +\tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to allocate CIO2 buffers\";\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfor (std::unique_ptr &buffer : buffers_)\n> +\t\tavailableBuffers_.push(buffer.get());\n>\n> \treturn ret;\n> }\n>\n> void CIO2Device::freeBuffers()\n> {\n> +\twhile (!availableBuffers_.empty())\n> +\t\tavailableBuffers_.pop();\n> +\n> \tbuffers_.clear();\n>\n> \tif (output_->releaseBuffers())\n> \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> }\n>\n> +FrameBuffer *CIO2Device::getBuffer()\n> +{\n> +\tif (availableBuffers_.empty()) {\n> +\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tFrameBuffer *buffer = availableBuffers_.front();\n> +\n> +\tavailableBuffers_.pop();\n\nOT:\nIs it me or I'm rightfully puzzled by the fact std::queue::pop() does\nnot return an reference to the popped element?\n\nAnyway, looks good to me, will wait for v1 for a tag.\n\nThanks\n j\n\n> +\n> +\treturn buffer;\n> +}\n> +\n> +void CIO2Device::putBuffer(FrameBuffer *buffer)\n> +{\n> +\tavailableBuffers_.push(buffer);\n> +}\n> +\n> +int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> +{\n> +\tint ret = output_->queueBuffer(buffer);\n> +\tif (ret)\n> +\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> +\n> +\treturn ret;\n> +}\n> +\n> int CIO2Device::start()\n> {\n> -\tfor (const std::unique_ptr &buffer : buffers_) {\n> -\t\tint ret = output_->queueBuffer(buffer.get());\n> -\t\tif (ret) {\n> -\t\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> -\t\t\treturn ret;\n> -\t\t}\n> -\t}\n> -\n> \treturn output_->streamOn();\n> }\n>\n> --\n> 2.25.1\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 relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D08562BD6\n\tfor ;\n\tWed, 25 Mar 2020 12:10:41 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id D1CF1240004;\n\tWed, 25 Mar 2020 11:10:40 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 25 Mar 2020 12:13:39 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200325111339.mtdsr4ct6mujf2ru@uno.localdomain>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200316024146.2474424-6-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 25 Mar 2020 11:10:41 -0000"}},{"id":4280,"web_url":"https://patchwork.libcamera.org/comment/4280/","msgid":"<20200325111246.GB19171@pendragon.ideasonboard.com>","date":"2020-03-25T11:12:46","subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Mar 25, 2020 at 12:13:39PM +0100, Jacopo Mondi wrote:\n> On Mon, Mar 16, 2020 at 03:41:45AM +0100, Niklas Söderlund wrote:\n> > Instead of unconditionally cycling buffers between the CIO2 and IMGU\n> > pick a buffer when a request is queued to the pipeline. This is needed\n> > if operations are the be applied to the buffer coming from CIO2 with\n> > parameters coming from a Request.\n> >\n> > The approach to pick a CIO2 buffer when a request is queued is similar\n> > to other pipelines, where parameters and statistic buffers are picked\n> > this way.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++-----\n> > 1 file changed, 58 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -119,6 +120,10 @@ public:\n> > \tint allocateBuffers();\n> > \tvoid freeBuffers();\n> >\n> > +\tFrameBuffer *getBuffer();\n> > +\tvoid putBuffer(FrameBuffer *buffer);\n> > +\tint queueBuffer(FrameBuffer *buffer);\n> > +\n> > \tint start();\n> > \tint stop();\n> >\n> > @@ -130,6 +135,7 @@ public:\n> >\n> > private:\n> > \tstd::vector> buffers_;\n> > +\tstd::queue availableBuffers_;\n> > };\n> >\n> > class IPU3Stream : public Stream\n> > @@ -157,6 +163,8 @@ public:\n> > \tvoid imguInputBufferReady(FrameBuffer *buffer);\n> > \tvoid cio2BufferReady(FrameBuffer *buffer);\n> >\n> > +\tstd::map cio2ToRequest_;\n> > +\n> > \tCIO2Device cio2_;\n> > \tImgUDevice *imgu_;\n> >\n> > @@ -717,11 +725,20 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> >\n> > int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > {\n> > +\tIPU3CameraData *data = cameraData(camera);\n> > +\tFrameBuffer *buffer;\n> > \tint error = 0;\n> >\n> > +\t/* Get a CIO2 buffer and track it it's used for this request. */\n> > +\tbuffer = data->cio2_.getBuffer();\n> > +\tif (!buffer)\n> > +\t\treturn -EINVAL;\n> \n> very minor nit: empty line\n> \n> I don't have much to add to Laurent's review, I'm wondering if we need\n> some kind of locking when accessing the queue though.\n\nEverything runs in a single thread, so there's no need to.\n\n> > +\tdata->cio2ToRequest_[buffer] = request;\n> > +\tdata->cio2_.queueBuffer(buffer);\n> > +\n> > \tfor (auto it : request->buffers()) {\n> > \t\tIPU3Stream *stream = static_cast(it.first);\n> > -\t\tFrameBuffer *buffer = it.second;\n> > +\t\tbuffer = it.second;\n> >\n> > \t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> > \t\tif (ret < 0)\n> > @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > \t\treturn;\n> >\n> > -\tcio2_.output_->queueBuffer(buffer);\n> > +\t/* Remove association between CIO2 buffer an Request. */\n> > +\tcio2ToRequest_.erase(buffer);\n> > +\tcio2_.putBuffer(buffer);\n> > }\n> >\n> > /**\n> > @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size,\n> > int CIO2Device::allocateBuffers()\n> > {\n> > \tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > -\tif (ret < 0)\n> > +\tif (ret < 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to allocate CIO2 buffers\";\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfor (std::unique_ptr &buffer : buffers_)\n> > +\t\tavailableBuffers_.push(buffer.get());\n> >\n> > \treturn ret;\n> > }\n> >\n> > void CIO2Device::freeBuffers()\n> > {\n> > +\twhile (!availableBuffers_.empty())\n> > +\t\tavailableBuffers_.pop();\n> > +\n> > \tbuffers_.clear();\n> >\n> > \tif (output_->releaseBuffers())\n> > \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> > }\n> >\n> > +FrameBuffer *CIO2Device::getBuffer()\n> > +{\n> > +\tif (availableBuffers_.empty()) {\n> > +\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tFrameBuffer *buffer = availableBuffers_.front();\n> > +\n> > +\tavailableBuffers_.pop();\n> \n> OT:\n> Is it me or I'm rightfully puzzled by the fact std::queue::pop() does\n> not return an reference to the popped element?\n\nIt would need to be a copy, not a reference, which would be expensive\nfor queues that store large elements.\n\n> Anyway, looks good to me, will wait for v1 for a tag.\n> \n> > +\n> > +\treturn buffer;\n> > +}\n> > +\n> > +void CIO2Device::putBuffer(FrameBuffer *buffer)\n> > +{\n> > +\tavailableBuffers_.push(buffer);\n> > +}\n> > +\n> > +int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> > +{\n> > +\tint ret = output_->queueBuffer(buffer);\n> > +\tif (ret)\n> > +\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > int CIO2Device::start()\n> > {\n> > -\tfor (const std::unique_ptr &buffer : buffers_) {\n> > -\t\tint ret = output_->queueBuffer(buffer.get());\n> > -\t\tif (ret) {\n> > -\t\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> > -\t\t\treturn ret;\n> > -\t\t}\n> > -\t}\n> > -\n> > \treturn output_->streamOn();\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 55B7162BD6\n\tfor ;\n\tWed, 25 Mar 2020 12:12:50 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6BD5E80C;\n\tWed, 25 Mar 2020 12:12:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gyNpFY64\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585134770;\n\tbh=RppvajZtU2/YCK22K1kNDVE6pzE6MBWyURgC5ljpl6g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gyNpFY64jyjgPTwKnH9sKVn4q5lPKWffxA2NT2M0P0z33xP3Cb5eYdkcWraWz55zV\n\tp9ms3rV64MOpL67PNrYzA+H0ZU7aQvEDd0IipKGCXPkfnguo2EHc3PqAmuaKXk5JDg\n\txC3fLd6nL88qowNFOOG4H4eOJaGhusJYzR2nMJXk=","Date":"Wed, 25 Mar 2020 13:12:46 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200325111246.GB19171@pendragon.ideasonboard.com>","References":"<20200316024146.2474424-1-niklas.soderlund@ragnatech.se>\n\t<20200316024146.2474424-6-niklas.soderlund@ragnatech.se>\n\t<20200325111339.mtdsr4ct6mujf2ru@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200325111339.mtdsr4ct6mujf2ru@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not\n\tunconditionally queue buffers to CIO2","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 25 Mar 2020 11:12:50 -0000"}}]