ipa: Switch to FrameBuffer interface

Switch the IPA interfaces and implementations to use the Framebuffer
interface.

- The IPA interface is switched to use the simpler FrameBuffer::Plane
  container when carrying dmabuf descriptions (fd and length) over the
  pipeline/IPA boundary.

- The RkISP1 IPA implementation takes advantage of the new simpler and
  safer (better control over file descriptors) FrameBuffer interface.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

7 files changed, 76 insertions, 43 deletions

diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
index 0ea53d12..229d1124 100644
--- a/include/ipa/ipa_interface.h
+++ b/include/ipa/ipa_interface.h
@@ -105,7 +105,7 @@ struct IPAStream {
 
 struct IPABuffer {
 	unsigned int id;
-	BufferMemory memory;
+	std::vector<FrameBuffer::Plane> planes;
 };
 
 struct IPAOperationData {
diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
index 6a389dfa..3628a785 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.cpp
+++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
@@ -144,17 +144,14 @@ void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
 	for (unsigned int i = 0; i < num_buffers; ++i) {
 		const struct ipa_buffer &_buffer = _buffers[i];
 		IPABuffer &buffer = buffers[i];
-		std::vector<Plane> &planes = buffer.memory.planes();
+		std::vector<FrameBuffer::Plane> &planes = buffer.planes;
 
 		buffer.id = _buffer.id;
 
 		planes.resize(_buffer.num_planes);
 		for (unsigned int j = 0; j < _buffer.num_planes; ++j) {
-			if (_buffer.planes[j].dmabuf != -1)
-				planes[j].setDmabuf(_buffer.planes[j].dmabuf,
-						    _buffer.planes[j].length);
-			/** \todo Create a Dmabuf class to implement RAII. */
-			::close(_buffer.planes[j].dmabuf);
+			planes[j].fd = FileDescriptor(_buffer.planes[j].dmabuf);
+			planes[j].length = _buffer.planes[j].length;
 		}
 	}
 
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 744a16ae..74b29220 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -10,6 +10,7 @@
 #include <queue>
 #include <stdint.h>
 #include <string.h>
+#include <sys/mman.h>
 
 #include <linux/rkisp1-config.h>
 
@@ -48,7 +49,8 @@ private:
 	void setControls(unsigned int frame);
 	void metadataReady(unsigned int frame, unsigned int aeState);
 
-	std::map<unsigned int, BufferMemory> bufferInfo_;
+	std::map<unsigned int, FrameBuffer> buffers_;
+	std::map<unsigned int, void *> buffersMemory_;
 
 	ControlInfoMap ctrls_;
 
@@ -102,15 +104,39 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
 void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
 {
 	for (const IPABuffer &buffer : buffers) {
-		bufferInfo_[buffer.id] = buffer.memory;
-		bufferInfo_[buffer.id].planes()[0].mem();
+		auto elem = buffers_.emplace(buffer.id, buffer.planes);
+		const FrameBuffer &fb = elem.first->second;
+
+		/*
+		 * \todo Provide a helper to mmap() buffers (possibly exposed
+		 * to applications).
+		 */
+		buffersMemory_[buffer.id] = mmap(NULL,
+						 fb.planes()[0].length,
+						 PROT_READ | PROT_WRITE,
+						 MAP_SHARED,
+						 fb.planes()[0].fd.fd(),
+						 0);
+
+		if (buffersMemory_[buffer.id] == MAP_FAILED) {
+			int ret = -errno;
+			LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: "
+					      << strerror(-ret);
+		}
 	}
 }
 
 void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
 {
-	for (unsigned int id : ids)
-		bufferInfo_.erase(id);
+	for (unsigned int id : ids) {
+		const auto fb = buffers_.find(id);
+		if (fb == buffers_.end())
+			continue;
+
+		munmap(buffersMemory_[id], fb->second.planes()[0].length);
+		buffersMemory_.erase(id);
+		buffers_.erase(id);
+	}
 }
 
 void IPARkISP1::processEvent(const IPAOperationData &event)
@@ -121,7 +147,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)
 		unsigned int bufferId = event.data[1];
 
 		const rkisp1_stat_buffer *stats =
-			static_cast<rkisp1_stat_buffer *>(bufferInfo_[bufferId].planes()[0].mem());
+			static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]);
 
 		updateStatistics(frame, stats);
 		break;
@@ -131,7 +157,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)
 		unsigned int bufferId = event.data[1];
 
 		rkisp1_isp_params_cfg *params =
-			static_cast<rkisp1_isp_params_cfg *>(bufferInfo_[bufferId].planes()[0].mem());
+			static_cast<rkisp1_isp_params_cfg *>(buffersMemory_[bufferId]);
 
 		queueRequest(frame, params, event.controls[0]);
 		break;
diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
index 9603fdac..946a2fd8 100644
--- a/src/libcamera/ipa_context_wrapper.cpp
+++ b/src/libcamera/ipa_context_wrapper.cpp
@@ -149,15 +149,15 @@ void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
 	for (unsigned int i = 0; i < buffers.size(); ++i) {
 		struct ipa_buffer &c_buffer = c_buffers[i];
 		const IPABuffer &buffer = buffers[i];
-		const std::vector<Plane> &planes = buffer.memory.planes();
+		const std::vector<FrameBuffer::Plane> &planes = buffer.planes;
 
 		c_buffer.id = buffer.id;
 		c_buffer.num_planes = planes.size();
 
 		for (unsigned int j = 0; j < planes.size(); ++j) {
-			const Plane &plane = planes[j];
-			c_buffer.planes[j].dmabuf = plane.dmabuf();
-			c_buffer.planes[j].length = plane.length();
+			const FrameBuffer::Plane &plane = planes[j];
+			c_buffer.planes[j].dmabuf = plane.fd.fd();
+			c_buffer.planes[j].length = plane.length;
 		}
 	}
 
diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
index ee3e3622..2f86087e 100644
--- a/src/libcamera/ipa_interface.cpp
+++ b/src/libcamera/ipa_interface.cpp
@@ -334,11 +334,10 @@ namespace libcamera {
  */
 
 /**
- * \var IPABuffer::memory
- * \brief The buffer memory description
+ * \var IPABuffer::planes
+ * \brief The buffer planes description
  *
- * The memory field stores the dmabuf handle and size for each plane of the
- * buffer.
+ * Stores the dmabuf handle and length for each plane of the buffer.
  */
 
 /**
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 7e41222e..d7ee95de 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -687,14 +687,22 @@ 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,
-					      .memory = paramPool_.buffers()[i] });
+					      .planes = { plane } });
 		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,
-					      .memory = statPool_.buffers()[i] });
+					      .planes = { plane } });
 		statBuffers_.push(new Buffer(i));
 	}
 
diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
index a1e34ad5..e711e4fe 100644
--- a/test/ipa/ipa_wrappers_test.cpp
+++ b/test/ipa/ipa_wrappers_test.cpp
@@ -115,28 +115,28 @@ public:
 			return report(Op_mapBuffers, TestFail);
 		}
 
-		if (buffers[0].memory.planes().size() != 3 ||
-		    buffers[1].memory.planes().size() != 3) {
+		if (buffers[0].planes.size() != 3 ||
+		    buffers[1].planes.size() != 3) {
 			cerr << "mapBuffers(): Invalid number of planes" << endl;
 			return report(Op_mapBuffers, TestFail);
 		}
 
-		if (buffers[0].memory.planes()[0].length() != 4096 ||
-		    buffers[0].memory.planes()[1].length() != 0 ||
-		    buffers[0].memory.planes()[2].length() != 0 ||
-		    buffers[0].memory.planes()[0].length() != 4096 ||
-		    buffers[1].memory.planes()[1].length() != 4096 ||
-		    buffers[1].memory.planes()[2].length() != 0) {
+		if (buffers[0].planes[0].length != 4096 ||
+		    buffers[0].planes[1].length != 0 ||
+		    buffers[0].planes[2].length != 0 ||
+		    buffers[0].planes[0].length != 4096 ||
+		    buffers[1].planes[1].length != 4096 ||
+		    buffers[1].planes[2].length != 0) {
 			cerr << "mapBuffers(): Invalid length" << endl;
 			return report(Op_mapBuffers, TestFail);
 		}
 
-		if (buffers[0].memory.planes()[0].dmabuf() == -1 ||
-		    buffers[0].memory.planes()[1].dmabuf() != -1 ||
-		    buffers[0].memory.planes()[2].dmabuf() != -1 ||
-		    buffers[0].memory.planes()[0].dmabuf() == -1 ||
-		    buffers[1].memory.planes()[1].dmabuf() == -1 ||
-		    buffers[1].memory.planes()[2].dmabuf() != -1) {
+		if (buffers[0].planes[0].fd.fd() == -1 ||
+		    buffers[0].planes[1].fd.fd() != -1 ||
+		    buffers[0].planes[2].fd.fd() != -1 ||
+		    buffers[0].planes[0].fd.fd() == -1 ||
+		    buffers[1].planes[1].fd.fd() == -1 ||
+		    buffers[1].planes[2].fd.fd() != -1) {
 			cerr << "mapBuffers(): Invalid dmabuf" << endl;
 			return report(Op_mapBuffers, TestFail);
 		}
@@ -287,13 +287,16 @@ protected:
 
 		/* Test mapBuffers(). */
 		std::vector<IPABuffer> buffers(2);
-		buffers[0].memory.planes().resize(3);
+		buffers[0].planes.resize(3);
 		buffers[0].id = 10;
-		buffers[0].memory.planes()[0].setDmabuf(fd_, 4096);
+		buffers[0].planes[0].fd = FileDescriptor(fd_);
+		buffers[0].planes[0].length = 4096;
 		buffers[1].id = 11;
-		buffers[1].memory.planes().resize(3);
-		buffers[1].memory.planes()[0].setDmabuf(fd_, 4096);
-		buffers[1].memory.planes()[1].setDmabuf(fd_, 4096);
+		buffers[1].planes.resize(3);
+		buffers[1].planes[0].fd = FileDescriptor(fd_);
+		buffers[1].planes[0].length = 4096;
+		buffers[1].planes[1].fd = FileDescriptor(fd_);
+		buffers[1].planes[1].length = 4096;
 
 		ret = INVOKE(mapBuffers, buffers);
 		if (ret == TestFail)