From patchwork Fri Jan 10 19:37:44 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Niklas_S=C3=B6derlund?= X-Patchwork-Id: 2556 Return-Path: Received: from vsp-unauthed02.binero.net (vsp-unauthed02.binero.net [195.74.38.227]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A64E60689 for ; Fri, 10 Jan 2020 20:38:42 +0100 (CET) X-Halon-ID: cc89039e-33e0-11ea-b6d8-005056917f90 Authorized-sender: niklas@soderlund.pp.se Received: from bismarck.berto.se (p54ac5d7b.dip0.t-ipconnect.de [84.172.93.123]) by bin-vsp-out-02.atm.binero.net (Halon) with ESMTPA id cc89039e-33e0-11ea-b6d8-005056917f90; Fri, 10 Jan 2020 20:38:38 +0100 (CET) From: =?utf-8?q?Niklas_S=C3=B6derlund?= To: libcamera-devel@lists.libcamera.org Date: Fri, 10 Jan 2020 20:37:44 +0100 Message-Id: <20200110193808.2266294-10-niklas.soderlund@ragnatech.se> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200110193808.2266294-1-niklas.soderlund@ragnatech.se> References: <20200110193808.2266294-1-niklas.soderlund@ragnatech.se> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v3 09/33] ipa: Switch to FrameBuffer interface X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Jan 2020 19:38:42 -0000 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 Reviewed-by: Laurent Pinchart --- * 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 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 &planes = buffer.memory.planes(); + std::vector &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 #include #include +#include #include @@ -48,7 +49,8 @@ private: void setControls(unsigned int frame); void metadataReady(unsigned int frame, unsigned int aeState); - std::map bufferInfo_; + std::map buffers_; + std::map buffersMemory_; ControlInfoMap ctrls_; @@ -102,15 +104,39 @@ void IPARkISP1::configure(const std::map &streamConfig, void IPARkISP1::mapBuffers(const std::vector &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 &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(bufferInfo_[bufferId].planes()[0].mem()); + static_cast(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(bufferInfo_[bufferId].planes()[0].mem()); + static_cast(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 &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 &planes = buffer.memory.planes(); + const std::vector &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 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)