{"id":2355,"url":"https://patchwork.libcamera.org/api/1.1/patches/2355/?format=json","web_url":"https://patchwork.libcamera.org/patch/2355/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20191126233620.1695316-8-niklas.soderlund@ragnatech.se>","date":"2019-11-26T23:35:57","name":"[libcamera-devel,07/30] ipa: Switch to FrameBuffer interface","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"290eb38a591c1e5a922f772f4ab3b0154267c997","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/1.1/people/5/?format=json","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/2355/mbox/","series":[{"id":579,"url":"https://patchwork.libcamera.org/api/1.1/series/579/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=579","date":"2019-11-26T23:35:50","name":"libcamera: Rework buffer API","version":1,"mbox":"https://patchwork.libcamera.org/series/579/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/2355/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/2355/checks/","tags":{},"headers":{"Return-Path":"","Received":["from bin-mail-out-06.binero.net (bin-mail-out-06.binero.net\n\t[195.74.38.229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CB7B61C7D\n\tfor ;\n\tWed, 27 Nov 2019 00:39:30 +0100 (CET)","from bismarck.berto.se (p54ac5865.dip0.t-ipconnect.de\n\t[84.172.88.101]) by bin-vsp-out-02.atm.binero.net (Halon) with ESMTPA\n\tid fcbca656-10a5-11ea-a0b9-005056917f90;\n\tWed, 27 Nov 2019 00:39:28 +0100 (CET)"],"X-Halon-ID":"fcbca656-10a5-11ea-a0b9-005056917f90","Authorized-sender":"niklas@soderlund.pp.se","From":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","To":"libcamera-devel@lists.libcamera.org","Date":"Wed, 27 Nov 2019 00:35:57 +0100","Message-Id":"<20191126233620.1695316-8-niklas.soderlund@ragnatech.se>","X-Mailer":"git-send-email 2.24.0","In-Reply-To":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH 07/30] ipa: Switch to FrameBuffer interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 26 Nov 2019 23:39:31 -0000"},"content":"Switch the IPA interfaces and implementations to use the Framebuffer\ninterface.\n\n- The IPA interface is switch to use the simpler FrameBuffer::Plane\n container when carrying dmabuf descriptions (fd and length) over the\n pipeline/IPA boundary.\n\n- The RkISP1 IPA implementation takes advantage of the new simpler and\n safer (better control over file descriptors) FrameBuffer interface.\n\nThe biggest change can be observed in the test case where it is\ndemonstrated that FrameBuffer objects only carry the necessary number of\nplanes and not a fixed number. As a consequence of this the test needs\nto verify the file descriptors it injects and not depend on the library\nto handle uninitialized fields.\n\nSigned-off-by: Niklas Söderlund \n---\n include/ipa/ipa_interface.h | 2 +-\n src/ipa/libipa/ipa_interface_wrapper.cpp | 7 ++---\n src/ipa/rkisp1/rkisp1.cpp | 14 +++++----\n src/libcamera/ipa_context_wrapper.cpp | 8 ++---\n src/libcamera/ipa_interface.cpp | 7 ++---\n src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++--\n test/ipa/ipa_wrappers_test.cpp | 38 ++++++++++++------------\n 7 files changed, 52 insertions(+), 40 deletions(-)","diff":"diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\nindex 0ea53d120fe10acf..229d1124b19ec1c9 100644\n--- a/include/ipa/ipa_interface.h\n+++ b/include/ipa/ipa_interface.h\n@@ -105,7 +105,7 @@ struct IPAStream {\n \n struct IPABuffer {\n \tunsigned int id;\n-\tBufferMemory memory;\n+\tstd::vector planes;\n };\n \n struct IPAOperationData {\ndiff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\nindex 6a389dfa714ab535..a1aa88979d73446f 100644\n--- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n+++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n@@ -144,15 +144,14 @@ void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n \tfor (unsigned int i = 0; i < num_buffers; ++i) {\n \t\tconst struct ipa_buffer &_buffer = _buffers[i];\n \t\tIPABuffer &buffer = buffers[i];\n-\t\tstd::vector &planes = buffer.memory.planes();\n+\t\tstd::vector &planes = buffer.planes;\n \n \t\tbuffer.id = _buffer.id;\n \n \t\tplanes.resize(_buffer.num_planes);\n \t\tfor (unsigned int j = 0; j < _buffer.num_planes; ++j) {\n-\t\t\tif (_buffer.planes[j].dmabuf != -1)\n-\t\t\t\tplanes[j].setDmabuf(_buffer.planes[j].dmabuf,\n-\t\t\t\t\t\t _buffer.planes[j].length);\n+\t\t\tplanes[j].fd = _buffer.planes[j].dmabuf;\n+\t\t\tplanes[j].length = _buffer.planes[j].length;\n \t\t\t/** \\todo Create a Dmabuf class to implement RAII. */\n \t\t\t::close(_buffer.planes[j].dmabuf);\n \t\t}\ndiff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\nindex 744a16ae5b9aa420..7f29d0351ba58e6d 100644\n--- a/src/ipa/rkisp1/rkisp1.cpp\n+++ b/src/ipa/rkisp1/rkisp1.cpp\n@@ -48,7 +48,7 @@ private:\n \tvoid setControls(unsigned int frame);\n \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n \n-\tstd::map bufferInfo_;\n+\tstd::map bufferInfo_;\n \n \tControlInfoMap ctrls_;\n \n@@ -102,15 +102,17 @@ void IPARkISP1::configure(const std::map &streamConfig,\n void IPARkISP1::mapBuffers(const std::vector &buffers)\n {\n \tfor (const IPABuffer &buffer : buffers) {\n-\t\tbufferInfo_[buffer.id] = buffer.memory;\n-\t\tbufferInfo_[buffer.id].planes()[0].mem();\n+\t\tbufferInfo_[buffer.id] = new FrameBuffer(buffer.planes);\n+\t\tbufferInfo_[buffer.id]->dmabufs()[0]->mem();\n \t}\n }\n \n void IPARkISP1::unmapBuffers(const std::vector &ids)\n {\n-\tfor (unsigned int id : ids)\n+\tfor (unsigned int id : ids) {\n+\t\tdelete bufferInfo_[id];\n \t\tbufferInfo_.erase(id);\n+\t}\n }\n \n void IPARkISP1::processEvent(const IPAOperationData &event)\n@@ -121,7 +123,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)\n \t\tunsigned int bufferId = event.data[1];\n \n \t\tconst rkisp1_stat_buffer *stats =\n-\t\t\tstatic_cast(bufferInfo_[bufferId].planes()[0].mem());\n+\t\t\tstatic_cast(bufferInfo_[bufferId]->dmabufs()[0]->mem());\n \n \t\tupdateStatistics(frame, stats);\n \t\tbreak;\n@@ -131,7 +133,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)\n \t\tunsigned int bufferId = event.data[1];\n \n \t\trkisp1_isp_params_cfg *params =\n-\t\t\tstatic_cast(bufferInfo_[bufferId].planes()[0].mem());\n+\t\t\tstatic_cast(bufferInfo_[bufferId]->dmabufs()[0]->mem());\n \n \t\tqueueRequest(frame, params, event.controls[0]);\n \t\tbreak;\ndiff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\nindex 9603fdac87150aa0..978ead927d196f02 100644\n--- a/src/libcamera/ipa_context_wrapper.cpp\n+++ b/src/libcamera/ipa_context_wrapper.cpp\n@@ -149,15 +149,15 @@ void IPAContextWrapper::mapBuffers(const std::vector &buffers)\n \tfor (unsigned int i = 0; i < buffers.size(); ++i) {\n \t\tstruct ipa_buffer &c_buffer = c_buffers[i];\n \t\tconst IPABuffer &buffer = buffers[i];\n-\t\tconst std::vector &planes = buffer.memory.planes();\n+\t\tconst std::vector &planes = buffer.planes;\n \n \t\tc_buffer.id = buffer.id;\n \t\tc_buffer.num_planes = planes.size();\n \n \t\tfor (unsigned int j = 0; j < planes.size(); ++j) {\n-\t\t\tconst Plane &plane = planes[j];\n-\t\t\tc_buffer.planes[j].dmabuf = plane.dmabuf();\n-\t\t\tc_buffer.planes[j].length = plane.length();\n+\t\t\tconst FrameBuffer::Plane &plane = planes[j];\n+\t\t\tc_buffer.planes[j].dmabuf = plane.fd;\n+\t\t\tc_buffer.planes[j].length = plane.length;\n \t\t}\n \t}\n \ndiff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\nindex ee3e3622f39ae85f..2f86087ee4aa414f 100644\n--- a/src/libcamera/ipa_interface.cpp\n+++ b/src/libcamera/ipa_interface.cpp\n@@ -334,11 +334,10 @@ namespace libcamera {\n */\n \n /**\n- * \\var IPABuffer::memory\n- * \\brief The buffer memory description\n+ * \\var IPABuffer::planes\n+ * \\brief The buffer planes description\n *\n- * The memory field stores the dmabuf handle and size for each plane of the\n- * buffer.\n+ * Stores the dmabuf handle and length for each plane of the buffer.\n */\n \n /**\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\nindex f75b25c6787e40df..00fa4d4f19cb4aa4 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n@@ -687,14 +687,26 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n \t}\n \n \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n+\t\tstd::vector planes;\n+\t\tplanes.push_back({\n+\t\t\t.fd = paramPool_.buffers()[i].planes()[0].dmabuf(),\n+\t\t\t.length = paramPool_.buffers()[i].planes()[0].length(),\n+\t\t});\n+\n \t\tdata->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,\n-\t\t\t\t\t .memory = paramPool_.buffers()[i] });\n+\t\t\t\t\t .planes = planes });\n \t\tparamBuffers_.push(new Buffer(i));\n \t}\n \n \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n+\t\tstd::vector planes;\n+\t\tplanes.push_back({\n+\t\t\t.fd = statPool_.buffers()[i].planes()[0].dmabuf(),\n+\t\t\t.length = statPool_.buffers()[i].planes()[0].length(),\n+\t\t});\n+\n \t\tdata->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,\n-\t\t\t\t\t .memory = statPool_.buffers()[i] });\n+\t\t\t\t\t .planes = planes });\n \t\tstatBuffers_.push(new Buffer(i));\n \t}\n \ndiff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\nindex a1e34ad52317f47b..022db05172fcd548 100644\n--- a/test/ipa/ipa_wrappers_test.cpp\n+++ b/test/ipa/ipa_wrappers_test.cpp\n@@ -115,28 +115,28 @@ public:\n \t\t\treturn report(Op_mapBuffers, TestFail);\n \t\t}\n \n-\t\tif (buffers[0].memory.planes().size() != 3 ||\n-\t\t buffers[1].memory.planes().size() != 3) {\n+\t\tif (buffers[0].planes.size() != 3 ||\n+\t\t buffers[1].planes.size() != 3) {\n \t\t\tcerr << \"mapBuffers(): Invalid number of planes\" << endl;\n \t\t\treturn report(Op_mapBuffers, TestFail);\n \t\t}\n \n-\t\tif (buffers[0].memory.planes()[0].length() != 4096 ||\n-\t\t buffers[0].memory.planes()[1].length() != 0 ||\n-\t\t buffers[0].memory.planes()[2].length() != 0 ||\n-\t\t buffers[0].memory.planes()[0].length() != 4096 ||\n-\t\t buffers[1].memory.planes()[1].length() != 4096 ||\n-\t\t buffers[1].memory.planes()[2].length() != 0) {\n+\t\tif (buffers[0].planes[0].length != 4096 ||\n+\t\t buffers[0].planes[1].length != 0 ||\n+\t\t buffers[0].planes[2].length != 0 ||\n+\t\t buffers[0].planes[0].length != 4096 ||\n+\t\t buffers[1].planes[1].length != 4096 ||\n+\t\t buffers[1].planes[2].length != 0) {\n \t\t\tcerr << \"mapBuffers(): Invalid length\" << endl;\n \t\t\treturn report(Op_mapBuffers, TestFail);\n \t\t}\n \n-\t\tif (buffers[0].memory.planes()[0].dmabuf() == -1 ||\n-\t\t buffers[0].memory.planes()[1].dmabuf() != -1 ||\n-\t\t buffers[0].memory.planes()[2].dmabuf() != -1 ||\n-\t\t buffers[0].memory.planes()[0].dmabuf() == -1 ||\n-\t\t buffers[1].memory.planes()[1].dmabuf() == -1 ||\n-\t\t buffers[1].memory.planes()[2].dmabuf() != -1) {\n+\t\tif (buffers[0].planes[0].fd == 0 ||\n+\t\t buffers[0].planes[1].fd != 0 ||\n+\t\t buffers[0].planes[2].fd != 0 ||\n+\t\t buffers[0].planes[0].fd == 0 ||\n+\t\t buffers[1].planes[1].fd == 0 ||\n+\t\t buffers[1].planes[2].fd != 0) {\n \t\t\tcerr << \"mapBuffers(): Invalid dmabuf\" << endl;\n \t\t\treturn report(Op_mapBuffers, TestFail);\n \t\t}\n@@ -287,13 +287,13 @@ protected:\n \n \t\t/* Test mapBuffers(). */\n \t\tstd::vector buffers(2);\n-\t\tbuffers[0].memory.planes().resize(3);\n+\t\tbuffers[0].planes.resize(3);\n \t\tbuffers[0].id = 10;\n-\t\tbuffers[0].memory.planes()[0].setDmabuf(fd_, 4096);\n+\t\tbuffers[0].planes[0] = { .fd = fd_, .length = 4096 };\n \t\tbuffers[1].id = 11;\n-\t\tbuffers[1].memory.planes().resize(3);\n-\t\tbuffers[1].memory.planes()[0].setDmabuf(fd_, 4096);\n-\t\tbuffers[1].memory.planes()[1].setDmabuf(fd_, 4096);\n+\t\tbuffers[1].planes.resize(3);\n+\t\tbuffers[1].planes[0] = { .fd = fd_, .length = 4096 };\n+\t\tbuffers[1].planes[1] = { .fd = fd_, .length = 4096 };\n \n \t\tret = INVOKE(mapBuffers, buffers);\n \t\tif (ret == TestFail)\n","prefixes":["libcamera-devel","07/30"]}