[libcamera-devel] [PATCH v4 09/32] ipa: Switch to FrameBuffer interface
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jan 12 02:01:49 CET 2020
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 at ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
* Changes since v2
- Update how FrameBuffers are handled in IPARkISP1::mapBuffers() and
IPARkISP1::unmapBuffers() to match the changes to FrameBuffers.
- Add todo about helper to mmap() in RkISP1 IPA.
- Dropped left over paragraph in commit message from RFCv1 which should
have been dropped in v1.
---
include/ipa/ipa_interface.h | 2 +-
src/ipa/libipa/ipa_interface_wrapper.cpp | 9 ++----
src/ipa/rkisp1/rkisp1.cpp | 40 +++++++++++++++++++----
src/libcamera/ipa_context_wrapper.cpp | 8 ++---
src/libcamera/ipa_interface.cpp | 7 ++--
src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++--
test/ipa/ipa_wrappers_test.cpp | 41 +++++++++++++-----------
7 files changed, 76 insertions(+), 43 deletions(-)
diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
index 0ea53d120fe10acf..229d1124b19ec1c9 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 6a389dfa714ab535..3628a785dc3e63f4 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 744a16ae5b9aa420..74b2922004bed556 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 9603fdac87150aa0..946a2fd8cab1782a 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 ee3e3622f39ae85f..2f86087ee4aa414f 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 7e41222e3d01a475..d7ee95ded0f76027 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 a1e34ad52317f47b..e711e4fe318d8179 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)
--
2.24.1
More information about the libcamera-devel
mailing list