[{"id":4967,"web_url":"https://patchwork.libcamera.org/comment/4967/","msgid":"<20200602123225.7rgsgoxxtksqckjo@uno.localdomain>","date":"2020-06-02T12:32:25","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Jun 02, 2020 at 03:39:08AM +0200, Niklas Söderlund wrote:\n> The allocation and freeing of buffers for the CIO2 is handled in the\n> IPU3 pipeline handlers start() and stop() functions. These functions\n> also calls CIO2Device start() and stop() at the appropriate times so\n> move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce\n> the complexity of the exposed interface.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 53 +++++++++++-----------------\n> src/libcamera/pipeline/ipu3/cio2.h | 2 --\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +-----\n> 3 files changed, 21 insertions(+), 44 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 63a46959f3cac06a..895848d2b3a1fba9 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -199,44 +199,12 @@ CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> \treturn cfg;\n> }\n>\n> -/**\n> - * \\brief Allocate frame buffers for the CIO2 output\n> - *\n> - * Allocate frame buffers in the CIO2 video device to be used to capture frames\n> - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_\n> - * vector.\n> - *\n> - * \\return Number of buffers allocated or negative error code\n> - */\n> -int CIO2Device::allocateBuffers()\n> -{\n> -\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> -\tfor (std::unique_ptr &buffer : buffers_)\n> -\t\tavailableBuffers_.push(buffer.get());\n> -\n> -\treturn ret;\n> -}\n> -\n> int CIO2Device::exportBuffers(unsigned int count,\n> \t\t\t std::vector> *buffers)\n> {\n> \treturn output_->exportBuffers(count, buffers);\n> }\n>\n> -void CIO2Device::freeBuffers()\n> -{\n> -\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> -\tavailableBuffers_ = std::queue{};\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> @@ -258,12 +226,31 @@ void CIO2Device::putBuffer(FrameBuffer *buffer)\n>\n> int CIO2Device::start()\n> {\n> +\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tfor (std::unique_ptr &buffer : buffers_)\n> +\t\tavailableBuffers_.push(buffer.get());\n> +\n> \treturn output_->streamOn();\n> }\n>\n> int CIO2Device::stop()\n> {\n> -\treturn output_->streamOff();\n> +\tint ret;\n> +\n> +\tret = output_->streamOff();\n> +\n> +\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> +\tavailableBuffers_ = std::queue{};\n\nI'm surprised std::queue does not have a reset method or something\nsimilar.\n\n> +\n> +\tbuffers_.clear();\n> +\n> +\tif (output_->releaseBuffers())\n> +\t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> +\n> +\treturn ret;\n> }\n>\n> int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index 465c0778f27e4066..032b91c082889a63 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -44,10 +44,8 @@ public:\n> \tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> \t\t\t\t\t\t const Size desiredSize = {}) const;\n>\n> -\tint allocateBuffers();\n> \tint exportBuffers(unsigned int count,\n> \t\t\t std::vector> *buffers);\n> -\tvoid freeBuffers();\n>\n> \tFrameBuffer *getBuffer();\n> \tvoid putBuffer(FrameBuffer *buffer);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2d636f0944587a17..636c1c54627b5777 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -653,24 +653,17 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> \tunsigned int bufferCount = 0;\n> \tint ret;\n>\n> -\tret = cio2->allocateBuffers();\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> \tbufferCount = std::max(data->outStream_.configuration().bufferCount, bufferCount);\n> \tbufferCount = std::max(data->vfStream_.configuration().bufferCount, bufferCount);\n> \tbufferCount = std::max(data->rawStream_.configuration().bufferCount, bufferCount);\n>\n> \tret = imgu->allocateBuffers(data, bufferCount);\n> -\tif (ret < 0) {\n> -\t\tcio2->freeBuffers();\n> +\tif (ret < 0)\n> \t\treturn ret;\n> -\t}\n>\n> \treturn 0;\n> }\n> @@ -679,7 +672,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n>\n> -\tdata->cio2_.freeBuffers();\n> \tdata->imgu_->freeBuffers(data);\n>\n> \treturn 0;\n\nLooks good to me\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> --\n> 2.26.2\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 relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5269E61012\n\tfor ;\n\tTue, 2 Jun 2020 14:29:04 +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 709E2200004;\n\tTue, 2 Jun 2020 12:29:03 +0000 (UTC)"],"Date":"Tue, 2 Jun 2020 14:32:25 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200602123225.7rgsgoxxtksqckjo@uno.localdomain>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-10-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":"<20200602013909.3170593-10-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","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, 02 Jun 2020 12:29:04 -0000"}},{"id":5073,"web_url":"https://patchwork.libcamera.org/comment/5073/","msgid":"<20200605221503.GL26752@pendragon.ideasonboard.com>","date":"2020-06-05T22:15:03","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","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, Jun 02, 2020 at 02:32:25PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 02, 2020 at 03:39:08AM +0200, Niklas Söderlund wrote:\n> > The allocation and freeing of buffers for the CIO2 is handled in the\n> > IPU3 pipeline handlers start() and stop() functions. These functions\n> > also calls CIO2Device start() and stop() at the appropriate times so\n> > move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce\n> > the complexity of the exposed interface.\n\nGiven the overall structure of the libcamera internal APIs I think\nthat's fine, but I think we'll likely have to reintroduce intermediate\nsteps before start() and after stop() to handle buffer allocation, as\nthose are expensive operations. This would then get in the way. It's OK\nfor now though.\n\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 53 +++++++++++-----------------\n> > src/libcamera/pipeline/ipu3/cio2.h | 2 --\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +-----\n> > 3 files changed, 21 insertions(+), 44 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 63a46959f3cac06a..895848d2b3a1fba9 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -199,44 +199,12 @@ CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> > \treturn cfg;\n> > }\n> >\n> > -/**\n> > - * \\brief Allocate frame buffers for the CIO2 output\n> > - *\n> > - * Allocate frame buffers in the CIO2 video device to be used to capture frames\n> > - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_\n> > - * vector.\n> > - *\n> > - * \\return Number of buffers allocated or negative error code\n> > - */\n> > -int CIO2Device::allocateBuffers()\n> > -{\n> > -\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > -\n> > -\tfor (std::unique_ptr &buffer : buffers_)\n> > -\t\tavailableBuffers_.push(buffer.get());\n> > -\n> > -\treturn ret;\n> > -}\n> > -\n> > int CIO2Device::exportBuffers(unsigned int count,\n> > \t\t\t std::vector> *buffers)\n> > {\n> > \treturn output_->exportBuffers(count, buffers);\n> > }\n> >\n> > -void CIO2Device::freeBuffers()\n> > -{\n> > -\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> > -\tavailableBuffers_ = std::queue{};\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> > @@ -258,12 +226,31 @@ void CIO2Device::putBuffer(FrameBuffer *buffer)\n> >\n> > int CIO2Device::start()\n> > {\n> > +\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tfor (std::unique_ptr &buffer : buffers_)\n> > +\t\tavailableBuffers_.push(buffer.get());\n> > +\n> > \treturn output_->streamOn();\n\nAren't you leaking the buffers if this call fails ?\n\n> > }\n> >\n> > int CIO2Device::stop()\n> > {\n> > -\treturn output_->streamOff();\n> > +\tint ret;\n> > +\n> > +\tret = output_->streamOff();\n\nYou can merge the two lines.\n\n> > +\n> > +\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> > +\tavailableBuffers_ = std::queue{};\n> \n> I'm surprised std::queue does not have a reset method or something\n> similar.\n> \n> > +\n> > +\tbuffers_.clear();\n> > +\n> > +\tif (output_->releaseBuffers())\n> > +\t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> > +\n> > +\treturn ret;\n> > }\n> >\n> > int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > index 465c0778f27e4066..032b91c082889a63 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > @@ -44,10 +44,8 @@ public:\n> > \tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> > \t\t\t\t\t\t const Size desiredSize = {}) const;\n> >\n> > -\tint allocateBuffers();\n> > \tint exportBuffers(unsigned int count,\n> > \t\t\t std::vector> *buffers);\n> > -\tvoid freeBuffers();\n> >\n> > \tFrameBuffer *getBuffer();\n> > \tvoid putBuffer(FrameBuffer *buffer);\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 2d636f0944587a17..636c1c54627b5777 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -653,24 +653,17 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tCIO2Device *cio2 = &data->cio2_;\n> > \tImgUDevice *imgu = data->imgu_;\n> > \tunsigned int bufferCount = 0;\n> > \tint ret;\n> >\n> > -\tret = cio2->allocateBuffers();\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > -\n> > \tbufferCount = std::max(data->outStream_.configuration().bufferCount, bufferCount);\n> > \tbufferCount = std::max(data->vfStream_.configuration().bufferCount, bufferCount);\n> > \tbufferCount = std::max(data->rawStream_.configuration().bufferCount, bufferCount);\n> >\n> > \tret = imgu->allocateBuffers(data, bufferCount);\n> > -\tif (ret < 0) {\n> > -\t\tcio2->freeBuffers();\n> > +\tif (ret < 0)\n> > \t\treturn ret;\n> > -\t}\n> >\n> > \treturn 0;\n> > }\n> > @@ -679,7 +672,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> >\n> > -\tdata->cio2_.freeBuffers();\n\nThe CIO2 buffers are now freed when stopping the CIO2, which happens\nbefore stopping the IMGU. Is there a risk they could still be in use by\nthe IMGU hardware ? V4L2 buffer orphaning may help us here, but handling\nthis issue explicitly may be better.\n\n> > \tdata->imgu_->freeBuffers(data);\n> >\n> > \treturn 0;\n> \n> Looks good to me\n> Reviewed-by: Jacopo Mondi ","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 08294603C8\n\tfor ;\n\tSat, 6 Jun 2020 00:15:23 +0200 (CEST)","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 77A6527C;\n\tSat, 6 Jun 2020 00:15:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pdm1nrMj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591395322;\n\tbh=ecDpFOBmNw2qG8n2TS0hj9NUEyD12/Z2wDTeK+yZ5y8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pdm1nrMjGzdKKHxVMiBBxD7rvA0AG/zrXBg3luRACn51m62wvl2p5Kk/JjhfHKWWU\n\t6dG7KMJzTIvQF1Xqm8/IEXnFB/QKWKDS8JuRO2tup0m2qzIcX5+8vXMqLTJv7iKLUq\n\tejSBhFnEywnXx7Y6gQb30k56zSMu51ZTTcTQUAjE=","Date":"Sat, 6 Jun 2020 01:15:03 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200605221503.GL26752@pendragon.ideasonboard.com>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-10-niklas.soderlund@ragnatech.se>\n\t<20200602123225.7rgsgoxxtksqckjo@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200602123225.7rgsgoxxtksqckjo@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","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":"Fri, 05 Jun 2020 22:15:23 -0000"}},{"id":5091,"web_url":"https://patchwork.libcamera.org/comment/5091/","msgid":"<20200606140841.GA402502@oden.dyn.berto.se>","date":"2020-06-06T14:08:41","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","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-06-06 01:15:03 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Tue, Jun 02, 2020 at 02:32:25PM +0200, Jacopo Mondi wrote:\n> > On Tue, Jun 02, 2020 at 03:39:08AM +0200, Niklas Söderlund wrote:\n> > > The allocation and freeing of buffers for the CIO2 is handled in the\n> > > IPU3 pipeline handlers start() and stop() functions. These functions\n> > > also calls CIO2Device start() and stop() at the appropriate times so\n> > > move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce\n> > > the complexity of the exposed interface.\n> \n> Given the overall structure of the libcamera internal APIs I think\n> that's fine, but I think we'll likely have to reintroduce intermediate\n> steps before start() and after stop() to handle buffer allocation, as\n> those are expensive operations. This would then get in the way. It's OK\n> for now though.\n> \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/ipu3/cio2.cpp | 53 +++++++++++-----------------\n> > > src/libcamera/pipeline/ipu3/cio2.h | 2 --\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +-----\n> > > 3 files changed, 21 insertions(+), 44 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > index 63a46959f3cac06a..895848d2b3a1fba9 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > @@ -199,44 +199,12 @@ CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> > > \treturn cfg;\n> > > }\n> > >\n> > > -/**\n> > > - * \\brief Allocate frame buffers for the CIO2 output\n> > > - *\n> > > - * Allocate frame buffers in the CIO2 video device to be used to capture frames\n> > > - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_\n> > > - * vector.\n> > > - *\n> > > - * \\return Number of buffers allocated or negative error code\n> > > - */\n> > > -int CIO2Device::allocateBuffers()\n> > > -{\n> > > -\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > > -\tif (ret < 0)\n> > > -\t\treturn ret;\n> > > -\n> > > -\tfor (std::unique_ptr &buffer : buffers_)\n> > > -\t\tavailableBuffers_.push(buffer.get());\n> > > -\n> > > -\treturn ret;\n> > > -}\n> > > -\n> > > int CIO2Device::exportBuffers(unsigned int count,\n> > > \t\t\t std::vector> *buffers)\n> > > {\n> > > \treturn output_->exportBuffers(count, buffers);\n> > > }\n> > >\n> > > -void CIO2Device::freeBuffers()\n> > > -{\n> > > -\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> > > -\tavailableBuffers_ = std::queue{};\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> > > @@ -258,12 +226,31 @@ void CIO2Device::putBuffer(FrameBuffer *buffer)\n> > >\n> > > int CIO2Device::start()\n> > > {\n> > > +\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tfor (std::unique_ptr &buffer : buffers_)\n> > > +\t\tavailableBuffers_.push(buffer.get());\n> > > +\n> > > \treturn output_->streamOn();\n> \n> Aren't you leaking the buffers if this call fails ?\n> \n> > > }\n> > >\n> > > int CIO2Device::stop()\n> > > {\n> > > -\treturn output_->streamOff();\n> > > +\tint ret;\n> > > +\n> > > +\tret = output_->streamOff();\n> \n> You can merge the two lines.\n> \n> > > +\n> > > +\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> > > +\tavailableBuffers_ = std::queue{};\n> > \n> > I'm surprised std::queue does not have a reset method or something\n> > similar.\n> > \n> > > +\n> > > +\tbuffers_.clear();\n> > > +\n> > > +\tif (output_->releaseBuffers())\n> > > +\t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> > > +\n> > > +\treturn ret;\n> > > }\n> > >\n> > > int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > > index 465c0778f27e4066..032b91c082889a63 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > > @@ -44,10 +44,8 @@ public:\n> > > \tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> > > \t\t\t\t\t\t const Size desiredSize = {}) const;\n> > >\n> > > -\tint allocateBuffers();\n> > > \tint exportBuffers(unsigned int count,\n> > > \t\t\t std::vector> *buffers);\n> > > -\tvoid freeBuffers();\n> > >\n> > > \tFrameBuffer *getBuffer();\n> > > \tvoid putBuffer(FrameBuffer *buffer);\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 2d636f0944587a17..636c1c54627b5777 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -653,24 +653,17 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > > {\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > -\tCIO2Device *cio2 = &data->cio2_;\n> > > \tImgUDevice *imgu = data->imgu_;\n> > > \tunsigned int bufferCount = 0;\n> > > \tint ret;\n> > >\n> > > -\tret = cio2->allocateBuffers();\n> > > -\tif (ret < 0)\n> > > -\t\treturn ret;\n> > > -\n> > > \tbufferCount = std::max(data->outStream_.configuration().bufferCount, bufferCount);\n> > > \tbufferCount = std::max(data->vfStream_.configuration().bufferCount, bufferCount);\n> > > \tbufferCount = std::max(data->rawStream_.configuration().bufferCount, bufferCount);\n> > >\n> > > \tret = imgu->allocateBuffers(data, bufferCount);\n> > > -\tif (ret < 0) {\n> > > -\t\tcio2->freeBuffers();\n> > > +\tif (ret < 0)\n> > > \t\treturn ret;\n> > > -\t}\n> > >\n> > > \treturn 0;\n> > > }\n> > > @@ -679,7 +672,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > > {\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > >\n> > > -\tdata->cio2_.freeBuffers();\n> \n> The CIO2 buffers are now freed when stopping the CIO2, which happens\n> before stopping the IMGU. Is there a risk they could still be in use by\n> the IMGU hardware ? V4L2 buffer orphaning may help us here, but handling\n> this issue explicitly may be better.\n\nYes the buffer may be used by the ImgU when the buffer is freed but as \nyou point out buffer orphaning takes care of this. We already depend on \nbuffer orphaning working properly and 10/10 in this series makes us even \nmore depending on it. In this case we could make the stop sequencing \nmore complex to eliminate the need to depend on buffer orphaning here \nbut IMHO it's not worth it.\n\n> \n> > > \tdata->imgu_->freeBuffers(data);\n> > >\n> > > \treturn 0;\n> > \n> > Looks good to me\n> > Reviewed-by: Jacopo Mondi \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7825A61167\n\tfor ;\n\tSat, 6 Jun 2020 16:08:43 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id j18so2135475lji.2\n\tfor ;\n\tSat, 06 Jun 2020 07:08:43 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\t2sm1905001lji.100.2020.06.06.07.08.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 06 Jun 2020 07:08:41 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"uul9I7DX\"; \n\tdkim-atps=neutral","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=HdNI2tr5IGaYF2/x/4PkHMJZDpECQaljXIxUfWQd9wo=;\n\tb=uul9I7DXKDYi3u2z0LsPGTEdKcfdWAo/eOR4iTmebz4mpnsMo+vBrDHmgkKSLu84Ox\n\tChDhx0YeflYakeR8d23KLOak/AP3qAgxZ1D4o4Rv2eJ+d+KdnMev9ISB2Qc7JHYJWoWM\n\tCwpl+g+fIfs8TFp/PiRRQJppy6Cb3pPJ6Vutk1EtHJe4mPHXmainaYpSlM5dEnbNIqqV\n\tUmjrN2ymaG/v3h9CcXBy0U21Tv9JLk+TwSey+pZnHKHGUSztYztdnMDa1+e+e2Kr+lL7\n\t86O6rEjEBRxYMljYWIzQSJlUbh633FKBfBuCULyXHWE7bsaXgEIlAmtT9pPWBmZKoC9W\n\tortA==","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=HdNI2tr5IGaYF2/x/4PkHMJZDpECQaljXIxUfWQd9wo=;\n\tb=dO2zVp6RT3cCAdBeis7HXPUrE6miqWaKAd+8+AQCUYDEPhQ8TarK6dBQhD5MAsoEvr\n\tcWU5VJY0j90nFX0d/pQSBKG7lcIl4QVvFLoSyXeZ3pRxxGEhTXKVosFzMv6CPivQLOEY\n\tiO+ew6BgbTZxXcG5ccTBzdFNkfihEM5nzyIBnrKgRbTyokKYAKPoYu3Wn3JPlOMYwchp\n\ttqcjvxY6W7ucbsjv7+dlsMEIbZNKV+EaqcX7GqeIxnsyqh0+zY644LmGhHJZxXxhe9Ai\n\t+SOY5T7Y0xHMH/SGSLt4rtacARq6qA1iLVCWKd0NKYUWDOpVnHcwARr7i+svjfXS95tR\n\tmzZw==","X-Gm-Message-State":"AOAM530/xCCiNhvalq8wUHZI/Sp+oa0RuJ8HLsguLKhtZknzV/YFVAnf\n\t+jd2slWH2KathBRFR1EFUCQvcA==","X-Google-Smtp-Source":"ABdhPJzIKueEOKXYtYMLIuPmO27qSydgvn9pZ9wopGCRnIb1/w4YDhItQkZyMmBcxCN4iMadVtXj9A==","X-Received":"by 2002:a2e:9ad8:: with SMTP id\n\tp24mr3343498ljj.179.1591452522715; \n\tSat, 06 Jun 2020 07:08:42 -0700 (PDT)","Date":"Sat, 6 Jun 2020 16:08:41 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200606140841.GA402502@oden.dyn.berto.se>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-10-niklas.soderlund@ragnatech.se>\n\t<20200602123225.7rgsgoxxtksqckjo@uno.localdomain>\n\t<20200605221503.GL26752@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":"<20200605221503.GL26752@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","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":"Sat, 06 Jun 2020 14:08:43 -0000"}},{"id":5094,"web_url":"https://patchwork.libcamera.org/comment/5094/","msgid":"<20200606170450.GC7339@pendragon.ideasonboard.com>","date":"2020-06-06T17:04:50","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sat, Jun 06, 2020 at 04:08:41PM +0200, Niklas Söderlund wrote:\n> On 2020-06-06 01:15:03 +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 02, 2020 at 02:32:25PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Jun 02, 2020 at 03:39:08AM +0200, Niklas Söderlund wrote:\n> > > > The allocation and freeing of buffers for the CIO2 is handled in the\n> > > > IPU3 pipeline handlers start() and stop() functions. These functions\n> > > > also calls CIO2Device start() and stop() at the appropriate times so\n> > > > move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce\n> > > > the complexity of the exposed interface.\n> > \n> > Given the overall structure of the libcamera internal APIs I think\n> > that's fine, but I think we'll likely have to reintroduce intermediate\n> > steps before start() and after stop() to handle buffer allocation, as\n> > those are expensive operations. This would then get in the way. It's OK\n> > for now though.\n> > \n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/cio2.cpp | 53 +++++++++++-----------------\n> > > > src/libcamera/pipeline/ipu3/cio2.h | 2 --\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +-----\n> > > > 3 files changed, 21 insertions(+), 44 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > index 63a46959f3cac06a..895848d2b3a1fba9 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > @@ -199,44 +199,12 @@ CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> > > > \treturn cfg;\n> > > > }\n> > > >\n> > > > -/**\n> > > > - * \\brief Allocate frame buffers for the CIO2 output\n> > > > - *\n> > > > - * Allocate frame buffers in the CIO2 video device to be used to capture frames\n> > > > - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_\n> > > > - * vector.\n> > > > - *\n> > > > - * \\return Number of buffers allocated or negative error code\n> > > > - */\n> > > > -int CIO2Device::allocateBuffers()\n> > > > -{\n> > > > -\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > > > -\tif (ret < 0)\n> > > > -\t\treturn ret;\n> > > > -\n> > > > -\tfor (std::unique_ptr &buffer : buffers_)\n> > > > -\t\tavailableBuffers_.push(buffer.get());\n> > > > -\n> > > > -\treturn ret;\n> > > > -}\n> > > > -\n> > > > int CIO2Device::exportBuffers(unsigned int count,\n> > > > \t\t\t std::vector> *buffers)\n> > > > {\n> > > > \treturn output_->exportBuffers(count, buffers);\n> > > > }\n> > > >\n> > > > -void CIO2Device::freeBuffers()\n> > > > -{\n> > > > -\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> > > > -\tavailableBuffers_ = std::queue{};\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> > > > @@ -258,12 +226,31 @@ void CIO2Device::putBuffer(FrameBuffer *buffer)\n> > > >\n> > > > int CIO2Device::start()\n> > > > {\n> > > > +\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> > > > +\tif (ret < 0)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\tfor (std::unique_ptr &buffer : buffers_)\n> > > > +\t\tavailableBuffers_.push(buffer.get());\n> > > > +\n> > > > \treturn output_->streamOn();\n> > \n> > Aren't you leaking the buffers if this call fails ?\n> > \n> > > > }\n> > > >\n> > > > int CIO2Device::stop()\n> > > > {\n> > > > -\treturn output_->streamOff();\n> > > > +\tint ret;\n> > > > +\n> > > > +\tret = output_->streamOff();\n> > \n> > You can merge the two lines.\n> > \n> > > > +\n> > > > +\t/* The default std::queue constructor is explicit with gcc 5 and 6. */\n> > > > +\tavailableBuffers_ = std::queue{};\n> > > \n> > > I'm surprised std::queue does not have a reset method or something\n> > > similar.\n> > > \n> > > > +\n> > > > +\tbuffers_.clear();\n> > > > +\n> > > > +\tif (output_->releaseBuffers())\n> > > > +\t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> > > > +\n> > > > +\treturn ret;\n> > > > }\n> > > >\n> > > > int CIO2Device::queueBuffer(FrameBuffer *buffer)\n> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > > > index 465c0778f27e4066..032b91c082889a63 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > > > @@ -44,10 +44,8 @@ public:\n> > > > \tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> > > > \t\t\t\t\t\t const Size desiredSize = {}) const;\n> > > >\n> > > > -\tint allocateBuffers();\n> > > > \tint exportBuffers(unsigned int count,\n> > > > \t\t\t std::vector> *buffers);\n> > > > -\tvoid freeBuffers();\n> > > >\n> > > > \tFrameBuffer *getBuffer();\n> > > > \tvoid putBuffer(FrameBuffer *buffer);\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 2d636f0944587a17..636c1c54627b5777 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -653,24 +653,17 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > > > {\n> > > > \tIPU3CameraData *data = cameraData(camera);\n> > > > -\tCIO2Device *cio2 = &data->cio2_;\n> > > > \tImgUDevice *imgu = data->imgu_;\n> > > > \tunsigned int bufferCount = 0;\n> > > > \tint ret;\n> > > >\n> > > > -\tret = cio2->allocateBuffers();\n> > > > -\tif (ret < 0)\n> > > > -\t\treturn ret;\n> > > > -\n> > > > \tbufferCount = std::max(data->outStream_.configuration().bufferCount, bufferCount);\n> > > > \tbufferCount = std::max(data->vfStream_.configuration().bufferCount, bufferCount);\n> > > > \tbufferCount = std::max(data->rawStream_.configuration().bufferCount, bufferCount);\n> > > >\n> > > > \tret = imgu->allocateBuffers(data, bufferCount);\n> > > > -\tif (ret < 0) {\n> > > > -\t\tcio2->freeBuffers();\n> > > > +\tif (ret < 0)\n> > > > \t\treturn ret;\n> > > > -\t}\n> > > >\n> > > > \treturn 0;\n> > > > }\n> > > > @@ -679,7 +672,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > > > {\n> > > > \tIPU3CameraData *data = cameraData(camera);\n> > > >\n> > > > -\tdata->cio2_.freeBuffers();\n> > \n> > The CIO2 buffers are now freed when stopping the CIO2, which happens\n> > before stopping the IMGU. Is there a risk they could still be in use by\n> > the IMGU hardware ? V4L2 buffer orphaning may help us here, but handling\n> > this issue explicitly may be better.\n> \n> Yes the buffer may be used by the ImgU when the buffer is freed but as \n> you point out buffer orphaning takes care of this. We already depend on \n> buffer orphaning working properly and 10/10 in this series makes us even \n> more depending on it. In this case we could make the stop sequencing \n> more complex to eliminate the need to depend on buffer orphaning here \n> but IMHO it's not worth it.\n\nCould we solve this by stopping the ImgU first ?\n\n> > > > \tdata->imgu_->freeBuffers(data);\n> > > >\n> > > > \treturn 0;\n> > > \n> > > Looks good to me\n> > > Reviewed-by: Jacopo Mondi ","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 AF98061167\n\tfor ;\n\tSat, 6 Jun 2020 19:05:09 +0200 (CEST)","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 45DC530D;\n\tSat, 6 Jun 2020 19:05:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JK6D1dby\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591463109;\n\tbh=Ab+CtgfZWJ2FsQthLdvIt3qEu0UDy+MqKRrsKIrgWdc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JK6D1dbyNkKWFRyxnorUYQI31uVWe+mrBaaG2H7V4NVFh6/JogEXoHCLSZ8xuYV/W\n\tGsTbv6k7Y0ft/J7LNKHYymaSnb7ZBfLC9uSBzQjm3LAXH18IiGEoyievnx1CZD3/FI\n\txLHnYLci+jd/cLrHXW93aPF1QHgU9uJXj650SZhY=","Date":"Sat, 6 Jun 2020 20:04:50 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200606170450.GC7339@pendragon.ideasonboard.com>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-10-niklas.soderlund@ragnatech.se>\n\t<20200602123225.7rgsgoxxtksqckjo@uno.localdomain>\n\t<20200605221503.GL26752@pendragon.ideasonboard.com>\n\t<20200606140841.GA402502@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":"<20200606140841.GA402502@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide\n\tbuffer allocation and freeing from users","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":"Sat, 06 Jun 2020 17:05:09 -0000"}}]