[{"id":3410,"web_url":"https://patchwork.libcamera.org/comment/3410/","msgid":"<20200111010044.GM4859@pendragon.ideasonboard.com>","date":"2020-01-11T01:00:44","subject":"Re: [libcamera-devel] [PATCH v3 23/33] libcamera: pipeline: ipu3:\n\tSwitch to FrameBuffer interface for cio2 and stat","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 Fri, Jan 10, 2020 at 08:37:58PM +0100, Niklas Söderlund wrote:\n> The V4L2VideoDevice class can now operate using a FrameBuffer interface,\n> switch the IPU3 CIO2 and statistics buffer to use it. We can not convert\n> the application facing buffers yet.\n\ns/application facing/application-facing/\n\nand please keep my Reviewed-by.\n\n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Laurent Pinchart \n> ---\n> * Changes since v2\n> - Fix spelling in commit message\n> - Update a comment in the code path touch by this patch\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 107 ++++++++++++---------------\n> 1 file changed, 46 insertions(+), 61 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 34fc792977d151be..7c4539d488bd2d26 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -45,6 +45,7 @@ public:\n> \t\tunsigned int pad;\n> \t\tstd::string name;\n> \t\tBufferPool *pool;\n> +\t\tstd::vector> buffers;\n> \t};\n> \n> \tImgUDevice()\n> @@ -70,7 +71,6 @@ public:\n> \tint configureOutput(ImgUOutput *output,\n> \t\t\t const StreamConfiguration &cfg);\n> \n> -\tint importInputBuffers(BufferPool *pool);\n> \tint importOutputBuffers(ImgUOutput *output, BufferPool *pool);\n> \tint exportOutputBuffers(ImgUOutput *output, BufferPool *pool);\n> \tvoid freeBuffers();\n> @@ -95,7 +95,6 @@ public:\n> \t/* \\todo Add param video device for 3A tuning */\n> \n> \tBufferPool vfPool_;\n> -\tBufferPool statPool_;\n> \tBufferPool outPool_;\n> };\n> \n> @@ -120,10 +119,10 @@ public:\n> \tint configure(const Size &size,\n> \t\t V4L2DeviceFormat *outputFormat);\n> \n> -\tBufferPool *exportBuffers();\n> +\tint allocateBuffers();\n> \tvoid freeBuffers();\n> \n> -\tint start(std::vector> *buffers);\n> +\tint start();\n> \tint stop();\n> \n> \tstatic int mediaBusToFormat(unsigned int code);\n> @@ -132,7 +131,8 @@ public:\n> \tV4L2Subdevice *csi2_;\n> \tCameraSensor *sensor_;\n> \n> -\tBufferPool pool_;\n> +private:\n> +\tstd::vector> buffers_;\n> };\n> \n> class IPU3Stream : public Stream\n> @@ -157,16 +157,14 @@ public:\n> \t}\n> \n> \tvoid imguOutputBufferReady(Buffer *buffer);\n> -\tvoid imguInputBufferReady(Buffer *buffer);\n> -\tvoid cio2BufferReady(Buffer *buffer);\n> +\tvoid imguInputBufferReady(FrameBuffer *buffer);\n> +\tvoid cio2BufferReady(FrameBuffer *buffer);\n> \n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> \n> \tIPU3Stream outStream_;\n> \tIPU3Stream vfStream_;\n> -\n> -\tstd::vector> rawBuffers_;\n> };\n> \n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -638,24 +636,28 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> \tint ret;\n> \n> \t/* Share buffers between CIO2 output and ImgU input. */\n> -\tBufferPool *pool = cio2->exportBuffers();\n> -\tif (!pool)\n> -\t\treturn -ENOMEM;\n> +\tret = cio2->allocateBuffers();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> \n> -\tret = imgu->importInputBuffers(pool);\n> -\tif (ret)\n> +\tbufferCount = ret;\n> +\n> +\tret = imgu->input_->importBuffers(bufferCount);\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n> \t\tgoto error;\n> +\t}\n> \n> \t/*\n> -\t * Use for the stat's internal pool the same number of buffer as\n> -\t * for the input pool.\n> +\t * Use for the stat's internal pool the same number of buffers as for\n> +\t * the input pool.\n> \t * \\todo To be revised when we'll actually use the stat node.\n> \t */\n> -\tbufferCount = pool->count();\n> -\timgu->stat_.pool->createBuffers(bufferCount);\n> -\tret = imgu->exportOutputBuffers(&imgu->stat_, imgu->stat_.pool);\n> -\tif (ret)\n> +\tret = imgu->stat_.dev->exportBuffers(bufferCount, &imgu->stat_.buffers);\n> +\tif (ret < 0) {\n> +\t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n> \t\tgoto error;\n> +\t}\n> \n> \t/* Allocate buffers for each active stream. */\n> \tfor (Stream *s : streams) {\n> @@ -722,7 +724,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> \t * Start the ImgU video devices, buffers will be queued to the\n> \t * ImgU output and viewfinder when requests will be queued.\n> \t */\n> -\tret = cio2->start(&data->rawBuffers_);\n> +\tret = cio2->start();\n> \tif (ret)\n> \t\tgoto error;\n> \n> @@ -738,7 +740,6 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> error:\n> \tLOG(IPU3, Error) << \"Failed to start camera \" << camera->name();\n> \n> -\tdata->rawBuffers_.clear();\n> \treturn ret;\n> }\n> \n> @@ -752,8 +753,6 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tif (ret)\n> \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n> \t\t\t\t << camera->name();\n> -\n> -\tdata->rawBuffers_.clear();\n> }\n> \n> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> @@ -885,9 +884,9 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t * associated ImgU input where they get processed and\n> \t\t * returned through the ImgU main and secondary outputs.\n> \t\t */\n> -\t\tdata->cio2_.output_->bufferReady.connect(data.get(),\n> +\t\tdata->cio2_.output_->frameBufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::cio2BufferReady);\n> -\t\tdata->imgu_->input_->bufferReady.connect(data.get(),\n> +\t\tdata->imgu_->input_->frameBufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::imguInputBufferReady);\n> \t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> @@ -925,7 +924,7 @@ int PipelineHandlerIPU3::registerCameras()\n> * Buffers completed from the ImgU input are immediately queued back to the\n> * CIO2 unit to continue frame capture.\n> */\n> -void IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> +void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n> {\n> \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> @@ -959,7 +958,7 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> * Buffers completed from the CIO2 are immediately queued to the ImgU unit\n> * for further processing.\n> */\n> -void IPU3CameraData::cio2BufferReady(Buffer *buffer)\n> +void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> {\n> \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> @@ -1034,7 +1033,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> \n> \tstat_.pad = PAD_STAT;\n> \tstat_.name = \"stat\";\n> -\tstat_.pool = &statPool_;\n> \n> \treturn 0;\n> }\n> @@ -1134,22 +1132,6 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief Import buffers from \\a pool into the ImgU input\n> - * \\param[in] pool The buffer pool to import\n> - * \\return 0 on success or a negative error code otherwise\n> - */\n> -int ImgUDevice::importInputBuffers(BufferPool *pool)\n> -{\n> -\tint ret = input_->importBuffers(pool);\n> -\tif (ret) {\n> -\t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n> -\t\treturn ret;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> /**\n> * \\brief Export buffers from \\a output to the provided \\a pool\n> * \\param[in] output The ImgU output device\n> @@ -1448,37 +1430,40 @@ int CIO2Device::configure(const Size &size,\n> }\n> \n> /**\n> - * \\brief Allocate CIO2 memory buffers and export them in a BufferPool\n> + * \\brief Allocate frame buffers for the CIO2 output\n> *\n> - * Allocate memory buffers in the CIO2 video device and export them to\n> - * a buffer pool that can be imported by another device.\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 The buffer pool with export buffers on success or nullptr otherwise\n> + * \\return Number of buffers allocated or negative error code\n> */\n> -BufferPool *CIO2Device::exportBuffers()\n> +int CIO2Device::allocateBuffers()\n> {\n> -\tpool_.createBuffers(CIO2_BUFFER_COUNT);\n> -\n> -\tint ret = output_->exportBuffers(&pool_);\n> -\tif (ret) {\n> +\tint ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> +\tif (ret < 0)\n> \t\tLOG(IPU3, Error) << \"Failed to export CIO2 buffers\";\n> -\t\treturn nullptr;\n> -\t}\n> \n> -\treturn &pool_;\n> +\treturn ret;\n> }\n> \n> void CIO2Device::freeBuffers()\n> {\n> +\tbuffers_.clear();\n> +\n> \tif (output_->releaseBuffers())\n> \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> }\n> \n> -int CIO2Device::start(std::vector> *buffers)\n> +int CIO2Device::start()\n> {\n> -\t*buffers = output_->queueAllBuffers();\n> -\tif (buffers->empty())\n> -\t\treturn -EINVAL;\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> }","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 51C08606AC\n\tfor ;\n\tSat, 11 Jan 2020 02:00:58 +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 CCE6E2F9;\n\tSat, 11 Jan 2020 02:00:57 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578704458;\n\tbh=a8TvGADIxJ0KYCBBM9A7RmepIPTQxZZC60+PHC8UKVM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vA7zvr07R4SKLUVs0JONsjsp7X4Hc+fWD0jaNCMY9FZdfvgKiLakhxsvgQ92EaG1Z\n\tMRmdsXRt1NcE1Tq0i10RtJM9nUjGejA6dv1iLHeeVRy7PPZqugxdB5c3yBL7dwVe5c\n\tY2Qsf910g+nlmGIRS69Eq4cLIi0CWHL/A8LRyXhA=","Date":"Sat, 11 Jan 2020 03:00:44 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200111010044.GM4859@pendragon.ideasonboard.com>","References":"<20200110193808.2266294-1-niklas.soderlund@ragnatech.se>\n\t<20200110193808.2266294-24-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":"<20200110193808.2266294-24-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 23/33] libcamera: pipeline: ipu3:\n\tSwitch to FrameBuffer interface for cio2 and stat","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, 11 Jan 2020 01:00:58 -0000"}}]