[{"id":3246,"web_url":"https://patchwork.libcamera.org/comment/3246/","msgid":"<20191214223701.GA4857@pendragon.ideasonboard.com>","date":"2019-12-14T22:37:01","subject":"Re: [libcamera-devel] [PATCH 08/30] libcamera: buffer: Switch from\n\tPlane to Dmabuf","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 Wed, Nov 27, 2019 at 12:35:58AM +0100, Niklas Söderlund wrote:\n> Dmabug and Plane serves similar functions in the two buffer interfaces\n\nIn retrospect naming the API dmabuf was a big mistake, with f being so\nclose to g on keyboards. Even Dvorak doesn't help.\n\n> Buffer and FrameBuffer and share many function signatures. Replace all\n> usages of Plane with Dmabuf to prepare for dropping the Buffer\n> interface.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/buffer.h | 6 +++---\n> src/cam/buffer_writer.cpp | 6 +++---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--\n> src/libcamera/stream.cpp | 3 +--\n> src/libcamera/v4l2_videodevice.cpp | 14 ++++++--------\n> src/qcam/main_window.cpp | 4 ++--\n> test/camera/buffer_import.cpp | 2 +-\n> 7 files changed, 18 insertions(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index fe5195327b540f5c..2dd5bcf3b49c4ee8 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -101,11 +101,11 @@ private:\n> class BufferMemory final\n> {\n> public:\n> -\tconst std::vector &planes() const { return planes_; }\n> -\tstd::vector &planes() { return planes_; }\n> +\tconst std::vector &planes() const { return planes_; }\n> +\tstd::vector &planes() { return planes_; }\n\nThe BufferMemory class documentation mentions the Plane class, it should\nbe replaced with Dmabuf there too. It's no big deal given that the\nBufferMemory class is dropped later, but you may still want to fix that.\n\n> \n> private:\n> -\tstd::vector planes_;\n> +\tstd::vector planes_;\n> };\n> \n> class BufferPool final\n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index c33e99c5f8173db8..5967efca07254666 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -43,9 +43,9 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> \t\treturn -errno;\n> \n> \tBufferMemory *mem = buffer->mem();\n> -\tfor (Plane &plane : mem->planes()) {\n> -\t\tvoid *data = plane.mem();\n> -\t\tunsigned int length = plane.length();\n> +\tfor (Dmabuf &dmabuf : mem->planes()) {\n> +\t\tvoid *data = dmabuf.mem();\n> +\t\tunsigned int length = dmabuf.length();\n> \n> \t\tret = ::write(fd, data, length);\n> \t\tif (ret < 0) {\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 00fa4d4f19cb4aa4..e8b6a278e97b0ba0 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -689,7 +689,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n> \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> \t\tstd::vector planes;\n> \t\tplanes.push_back({\n> -\t\t\t.fd = paramPool_.buffers()[i].planes()[0].dmabuf(),\n> +\t\t\t.fd = paramPool_.buffers()[i].planes()[0].fd(),\n\nI like how .fd = <...>.fd(), it's nicely consistent. It's one of those\nsmall details that show we're getting it right.\n\n> \t\t\t.length = paramPool_.buffers()[i].planes()[0].length(),\n> \t\t});\n> \n> @@ -701,7 +701,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n> \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> \t\tstd::vector planes;\n> \t\tplanes.push_back({\n> -\t\t\t.fd = statPool_.buffers()[i].planes()[0].dmabuf(),\n> +\t\t\t.fd = statPool_.buffers()[i].planes()[0].fd(),\n> \t\t\t.length = statPool_.buffers()[i].planes()[0].length(),\n> \t\t});\n> \n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 45f31ae1e2daeb53..e70a1e307ecaa5ba 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -577,8 +577,7 @@ int Stream::mapBuffer(const Buffer *buffer)\n> \t\tif (dmabufs[i] == -1)\n> \t\t\tbreak;\n> \n> -\t\tmem->planes().emplace_back();\n> -\t\tmem->planes().back().setDmabuf(dmabufs[i], 0);\n> +\t\tmem->planes().emplace_back(dmabufs[i], 0);\n\nThis, on the other hand, is less consistent, but the BufferMemory class\nis going away :-)\n\nWith the BufferMemory documentation updated if desired,\n\nReviewed-by: Laurent Pinchart \n\n> \t}\n> \n> \t/* Remove the buffer from the cache and return its index. */\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index dbb5c3982334e243..166b0abc1b101f88 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -921,9 +921,7 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,\n> \t\treturn ret;\n> \t}\n> \n> -\tbuffer->planes().emplace_back();\n> -\tPlane &plane = buffer->planes().back();\n> -\tplane.setDmabuf(expbuf.fd, length);\n> +\tbuffer->planes().emplace_back(expbuf.fd, length);\n> \t::close(expbuf.fd);\n> \n> \treturn 0;\n> @@ -986,19 +984,19 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> \n> \tbool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> \tBufferMemory *mem = &bufferPool_->buffers()[buf.index];\n> -\tconst std::vector &planes = mem->planes();\n> +\tconst std::vector &dmabufs = mem->planes();\n> \n> \tif (buf.memory == V4L2_MEMORY_DMABUF) {\n> \t\tif (multiPlanar) {\n> -\t\t\tfor (unsigned int p = 0; p < planes.size(); ++p)\n> -\t\t\t\tv4l2Planes[p].m.fd = planes[p].dmabuf();\n> +\t\t\tfor (unsigned int p = 0; p < dmabufs.size(); ++p)\n> +\t\t\t\tv4l2Planes[p].m.fd = dmabufs[p].fd();\n> \t\t} else {\n> -\t\t\tbuf.m.fd = planes[0].dmabuf();\n> +\t\t\tbuf.m.fd = dmabufs[0].fd();\n> \t\t}\n> \t}\n> \n> \tif (multiPlanar) {\n> -\t\tbuf.length = planes.size();\n> +\t\tbuf.length = dmabufs.size();\n> \t\tbuf.m.planes = v4l2Planes;\n> \t}\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 0c7ca61ac12ec41c..a828a176cbbf4854 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -300,8 +300,8 @@ int MainWindow::display(Buffer *buffer)\n> \tif (mem->planes().size() != 1)\n> \t\treturn -EINVAL;\n> \n> -\tPlane &plane = mem->planes().front();\n> -\tunsigned char *raw = static_cast(plane.mem());\n> +\tDmabuf &dmabuf = mem->planes().front();\n> +\tunsigned char *raw = static_cast(dmabuf.mem());\n> \tviewfinder_->display(raw, buffer->bytesused());\n> \n> \treturn 0;\n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 3efe02704c02f691..8b0d1c167bd2bbe8 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -178,7 +178,7 @@ private:\n> \n> \t\tuint64_t cookie = index;\n> \t\tBufferMemory &mem = pool_.buffers()[index];\n> -\t\tint dmabuf = mem.planes()[0].dmabuf();\n> +\t\tint dmabuf = mem.planes()[0].fd();\n> \n> \t\trequestReady.emit(cookie, dmabuf);\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 B290B601E5\n\tfor ;\n\tSat, 14 Dec 2019 23:37:11 +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 25AE42D1;\n\tSat, 14 Dec 2019 23:37:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576363031;\n\tbh=QvuVnf5J02wGssCZvZFjmAnuekEpG4CI3l40NUhslTM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HOKCay08jxj1hjipMNEreGWy6mYWJ0U3W3zTXs23UJZyBggFiQv4u97Vx/Ku/73y4\n\t2kupn5C6rxx73oGAEilQKdw+D3b0QK0KUJZoW1GegCeU47dvxRR1zwL12PnAfg8f7T\n\tnvEFM46XblTRO1pkl2/iUs8MnWz/ez9wHkfFIuoc=","Date":"Sun, 15 Dec 2019 00:37:01 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191214223701.GA4857@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-9-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":"<20191126233620.1695316-9-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 08/30] libcamera: buffer: Switch from\n\tPlane to Dmabuf","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, 14 Dec 2019 22:37:11 -0000"}}]