[libcamera-devel] [PATCH v3 09/33] ipa: Switch to FrameBuffer interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jan 11 01:05:01 CET 2020
Hi Niklas,
Thank you for the patch.
On Fri, Jan 10, 2020 at 08:37:44PM +0100, Niklas Söderlund wrote:
> 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)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list