[libcamera-devel] [PATCH v3 10/33] libcamera: buffer: Switch from Plane to FrameBuffer::Plane

Niklas Söderlund niklas.soderlund at ragnatech.se
Fri Jan 10 20:37:45 CET 2020


It is not libcamera's responsibility to handle memory mappings. Switch
from the soon to be removed Plane class which deals with memory
mappings to FrameBuffer::Plane which just describes it. This makes the
transition to the full FrameBuffer easier.

As the full FrameBuffer interface has not yet spread to all parts of
libcamera core it is hard to create efficient caching of memory mappings
in the qcam application. This will be fixed in a later patch, for now
the dmabuf is mapped and unmapped each time it is seen by the
application.

Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
* Changes since v2
- Add todo about adding caching for cam mmap() call
- Use {param,stat}Pool_.buffers()[i].planes() directly instead of
  creating a new plane.
- Use push_back() instead of emplace_back() for already constructed
  objects.
- s/todo:/todo/
---
 include/libcamera/buffer.h               |  6 +++---
 src/cam/buffer_writer.cpp                | 11 ++++++++---
 src/libcamera/buffer.cpp                 |  4 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 ++----------
 src/libcamera/stream.cpp                 |  6 ++++--
 src/libcamera/v4l2_videodevice.cpp       | 13 +++++++------
 src/qcam/main_window.cpp                 | 14 ++++++++++----
 src/v4l2/v4l2_camera.cpp                 |  2 +-
 test/camera/buffer_import.cpp            |  2 +-
 9 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
index f5522edaf68bbf61..98245e304009ccc1 100644
--- a/include/libcamera/buffer.h
+++ b/include/libcamera/buffer.h
@@ -93,11 +93,11 @@ private:
 class BufferMemory final
 {
 public:
-	const std::vector<Plane> &planes() const { return planes_; }
-	std::vector<Plane> &planes() { return planes_; }
+	const std::vector<FrameBuffer::Plane> &planes() const { return planes_; }
+	std::vector<FrameBuffer::Plane> &planes() { return planes_; }
 
 private:
-	std::vector<Plane> planes_;
+	std::vector<FrameBuffer::Plane> planes_;
 };
 
 class BufferPool final
diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
index c33e99c5f8173db8..765a176238e58dff 100644
--- a/src/cam/buffer_writer.cpp
+++ b/src/cam/buffer_writer.cpp
@@ -10,6 +10,7 @@
 #include <iostream>
 #include <sstream>
 #include <string.h>
+#include <sys/mman.h>
 #include <unistd.h>
 
 #include "buffer_writer.h"
@@ -43,9 +44,11 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
 		return -errno;
 
 	BufferMemory *mem = buffer->mem();
-	for (Plane &plane : mem->planes()) {
-		void *data = plane.mem();
-		unsigned int length = plane.length();
+	for (const FrameBuffer::Plane &plane : mem->planes()) {
+		/* \todo Once the FrameBuffer is done cache mapped memory. */
+		void *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
+				  plane.fd.fd(), 0);
+		unsigned int length = plane.length;
 
 		ret = ::write(fd, data, length);
 		if (ret < 0) {
@@ -59,6 +62,8 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
 				  << length << std::endl;
 			break;
 		}
+
+		munmap(data, length);
 	}
 
 	close(fd);
diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
index 3e2cc9b94595795e..de92a729e56d265f 100644
--- a/src/libcamera/buffer.cpp
+++ b/src/libcamera/buffer.cpp
@@ -257,13 +257,13 @@ void *Plane::mem()
 /**
  * \fn BufferMemory::planes() const
  * \brief Retrieve the planes within the buffer
- * \return A const reference to a vector holding all Planes within the buffer
+ * \return A const reference to a vector holding all planes within the buffer
  */
 
 /**
  * \fn BufferMemory::planes()
  * \brief Retrieve the planes within the buffer
- * \return A reference to a vector holding all Planes within the buffer
+ * \return A reference to a vector holding all planes within the buffer
  */
 
 /**
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index d7ee95ded0f76027..1e44571589a6003d 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -687,22 +687,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
 	}
 
 	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
-		FrameBuffer::Plane plane;
-		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());
-		plane.length = paramPool_.buffers()[i].planes()[0].length();
-
 		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
-					      .planes = { plane } });
+					      .planes = paramPool_.buffers()[i].planes() });
 		paramBuffers_.push(new Buffer(i));
 	}
 
 	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
-		FrameBuffer::Plane plane;
-		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());
-		plane.length = statPool_.buffers()[i].planes()[0].length();
-
 		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
-					      .planes = { plane } });
+					      .planes = statPool_.buffers()[i].planes() });
 		statBuffers_.push(new Buffer(i));
 	}
 
diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 45f31ae1e2daeb53..16a323f135b2022a 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -577,8 +577,10 @@ int Stream::mapBuffer(const Buffer *buffer)
 		if (dmabufs[i] == -1)
 			break;
 
-		mem->planes().emplace_back();
-		mem->planes().back().setDmabuf(dmabufs[i], 0);
+		FrameBuffer::Plane plane;
+		plane.fd = FileDescriptor(dmabufs[i]);
+		plane.length = 0;
+		mem->planes().push_back(plane);
 	}
 
 	/* Remove the buffer from the cache and return its index. */
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 033f7cc0d809ea8a..1dc9e19350f825a9 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -922,9 +922,10 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
 		return ret;
 	}
 
-	buffer->planes().emplace_back();
-	Plane &plane = buffer->planes().back();
-	plane.setDmabuf(expbuf.fd, length);
+	FrameBuffer::Plane plane;
+	plane.fd = FileDescriptor(expbuf.fd);
+	plane.length = length;
+	buffer->planes().push_back(plane);
 	::close(expbuf.fd);
 
 	return 0;
@@ -987,14 +988,14 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
 
 	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
 	BufferMemory *mem = &bufferPool_->buffers()[buf.index];
-	const std::vector<Plane> &planes = mem->planes();
+	const std::vector<FrameBuffer::Plane> &planes = mem->planes();
 
 	if (buf.memory == V4L2_MEMORY_DMABUF) {
 		if (multiPlanar) {
 			for (unsigned int p = 0; p < planes.size(); ++p)
-				v4l2Planes[p].m.fd = planes[p].dmabuf();
+				v4l2Planes[p].m.fd = planes[p].fd.fd();
 		} else {
-			buf.m.fd = planes[0].dmabuf();
+			buf.m.fd = planes[0].fd.fd();
 		}
 	}
 
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index 0c7ca61ac12ec41c..0a353e8ba247caf9 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -8,6 +8,7 @@
 #include <iomanip>
 #include <iostream>
 #include <string>
+#include <sys/mman.h>
 
 #include <QCoreApplication>
 #include <QInputDialog>
@@ -296,13 +297,18 @@ void MainWindow::requestComplete(Request *request)
 
 int MainWindow::display(Buffer *buffer)
 {
-	BufferMemory *mem = buffer->mem();
-	if (mem->planes().size() != 1)
+	if (buffer->mem()->planes().size() != 1)
 		return -EINVAL;
 
-	Plane &plane = mem->planes().front();
-	unsigned char *raw = static_cast<unsigned char *>(plane.mem());
+	/* \todo Once the FrameBuffer is done cache mapped memory. */
+	const FrameBuffer::Plane &plane = buffer->mem()->planes().front();
+	void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
+			    plane.fd.fd(), 0);
+
+	unsigned char *raw = static_cast<unsigned char *>(memory);
 	viewfinder_->display(raw, buffer->bytesused());
 
+	munmap(memory, plane.length);
+
 	return 0;
 }
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
index ba3783876101b284..359e7d0b656061df 100644
--- a/src/v4l2/v4l2_camera.cpp
+++ b/src/v4l2/v4l2_camera.cpp
@@ -135,7 +135,7 @@ void V4L2Camera::freeBuffers()
 int V4L2Camera::getBufferFd(unsigned int index)
 {
 	Stream *stream = *camera_->streams().begin();
-	return stream->buffers()[index].planes()[0].dmabuf();
+	return stream->buffers()[index].planes()[0].fd.fd();
 }
 
 int V4L2Camera::streamOn()
diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
index 3efe02704c02f691..171540edd96f9fca 100644
--- a/test/camera/buffer_import.cpp
+++ b/test/camera/buffer_import.cpp
@@ -178,7 +178,7 @@ private:
 
 		uint64_t cookie = index;
 		BufferMemory &mem = pool_.buffers()[index];
-		int dmabuf = mem.planes()[0].dmabuf();
+		int dmabuf = mem.planes()[0].fd.fd();
 
 		requestReady.emit(cookie, dmabuf);
 
-- 
2.24.1



More information about the libcamera-devel mailing list