[{"id":3335,"web_url":"https://patchwork.libcamera.org/comment/3335/","msgid":"<20200107030108.GH22189@pendragon.ideasonboard.com>","date":"2020-01-07T03:01:08","subject":"Re: [libcamera-devel] [PATCH v2 03/25] ipa: Switch to FrameBuffer\n\tinterface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Dec 30, 2019 at 01:04:48PM +0100, Niklas Söderlund wrote:\n> Switch the IPA interfaces and implementations to use the Framebuffer\n> interface.\n> \n> - The IPA interface is switch to use the simpler FrameBuffer::Plane\n\ns/switch/switched/\n\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> \n> The biggest change can be observed in the test case where it is\n> demonstrated that FrameBuffer objects only carry the necessary number of\n> planes and not a fixed number. As a consequence of this the test needs\n> to verify the file descriptors it injects and not depend on the library\n> to handle uninitialized fields.\n\nReally ? It seems you set the number of planes to 3 and only initialize\nsome of them. Did I miss something ?\n\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/ipa/ipa_interface.h              |  2 +-\n>  src/ipa/libipa/ipa_interface_wrapper.cpp |  9 ++----\n>  src/ipa/rkisp1/rkisp1.cpp                | 30 +++++++++++++----\n>  src/libcamera/ipa_context_wrapper.cpp    |  8 ++---\n>  src/libcamera/ipa_interface.cpp          |  7 ++--\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++--\n>  test/ipa/ipa_wrappers_test.cpp           | 41 +++++++++++++-----------\n>  7 files changed, 66 insertions(+), 43 deletions(-)\n> \n> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> index 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<FrameBuffer::Plane> planes;\n\nI wonder if we have a file descriptor lifetime management issue here.\nThe IPA API doesn't define ownership for file descriptors :-S I think we\nneed to address this first (to decide if the mapBuffers() method should\nbe given ownership of the passed file descriptors or if it should borrow\nthem), and then revisit this patch (with Jacopo's comments on 02/25\ntaken into account). Should this then become a vector of unique_ptr, or\na vector of const references ?\n\n>  };\n>  \n>  struct IPAOperationData {\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> index 6a389dfa714ab535..3628a785dc3e63f4 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -144,17 +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<Plane> &planes = buffer.memory.planes();\n> +\t\tstd::vector<FrameBuffer::Plane> &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\t/** \\todo Create a Dmabuf class to implement RAII. */\n> -\t\t\t::close(_buffer.planes[j].dmabuf);\n> +\t\t\tplanes[j].fd = FileDescriptor(_buffer.planes[j].dmabuf);\n> +\t\t\tplanes[j].length = _buffer.planes[j].length;\n>  \t\t}\n>  \t}\n>  \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 744a16ae5b9aa420..63776563b149f7ed 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -10,6 +10,7 @@\n>  #include <queue>\n>  #include <stdint.h>\n>  #include <string.h>\n> +#include <sys/mman.h>\n>  \n>  #include <linux/rkisp1-config.h>\n>  \n> @@ -48,7 +49,8 @@ private:\n>  \tvoid setControls(unsigned int frame);\n>  \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n>  \n> -\tstd::map<unsigned int, BufferMemory> bufferInfo_;\n> +\tstd::map<unsigned int, FrameBuffer> buffers_;\n> +\tstd::map<unsigned int, void *> buffersMemory_;\n>  \n>  \tControlInfoMap ctrls_;\n>  \n> @@ -102,15 +104,29 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n>  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &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\tbuffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n> +\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n> +\t\t\t\t\t\t buffers_[buffer.id].planes()[0].length,\n> +\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n> +\t\t\t\t\t\t MAP_SHARED,\n> +\t\t\t\t\t\t buffers_[buffer.id].planes()[0].fd.fd(),\n> +\t\t\t\t\t\t 0);\n> +\n> +\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n> +\t\t\tint ret = -errno;\n> +\t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n> +\t\t\t\t\t      << strerror(-ret);\n> +\t\t}\n>  \t}\n>  }\n>  \n>  void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)\n>  {\n> -\tfor (unsigned int id : ids)\n> -\t\tbufferInfo_.erase(id);\n> +\tfor (unsigned int id : ids) {\n> +\t\tmunmap(buffersMemory_[id], buffers_[id].planes()[0].length);\n> +\t\tbuffersMemory_.erase(id);\n> +\t\tbuffers_.erase(id);\n> +\t}\n>  }\n>  \n>  void IPARkISP1::processEvent(const IPAOperationData &event)\n> @@ -121,7 +137,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<rkisp1_stat_buffer *>(bufferInfo_[bufferId].planes()[0].mem());\n> +\t\t\tstatic_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]);\n>  \n>  \t\tupdateStatistics(frame, stats);\n>  \t\tbreak;\n> @@ -131,7 +147,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<rkisp1_isp_params_cfg *>(bufferInfo_[bufferId].planes()[0].mem());\n> +\t\t\tstatic_cast<rkisp1_isp_params_cfg *>(buffersMemory_[bufferId]);\n>  \n>  \t\tqueueRequest(frame, params, event.controls[0]);\n>  \t\tbreak;\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> index 9603fdac87150aa0..946a2fd8cab1782a 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<IPABuffer> &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<Plane> &planes = buffer.memory.planes();\n> +\t\tconst std::vector<FrameBuffer::Plane> &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.fd();\n> +\t\t\tc_buffer.planes[j].length = plane.length;\n>  \t\t}\n>  \t}\n>  \n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 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>  /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 7e41222e3d01a475..d7ee95ded0f76027 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -687,14 +687,22 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n>  \t}\n>  \n>  \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> +\t\tFrameBuffer::Plane plane;\n> +\t\tplane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());\n> +\t\tplane.length = paramPool_.buffers()[i].planes()[0].length();\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 = { plane } });\n>  \t\tparamBuffers_.push(new Buffer(i));\n>  \t}\n>  \n>  \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> +\t\tFrameBuffer::Plane plane;\n> +\t\tplane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());\n> +\t\tplane.length = statPool_.buffers()[i].planes()[0].length();\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 = { plane } });\n>  \t\tstatBuffers_.push(new Buffer(i));\n>  \t}\n>  \n> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> index a1e34ad52317f47b..e711e4fe318d8179 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.fd() == -1 ||\n> +\t\t    buffers[0].planes[1].fd.fd() != -1 ||\n> +\t\t    buffers[0].planes[2].fd.fd() != -1 ||\n> +\t\t    buffers[0].planes[0].fd.fd() == -1 ||\n> +\t\t    buffers[1].planes[1].fd.fd() == -1 ||\n> +\t\t    buffers[1].planes[2].fd.fd() != -1) {\n>  \t\t\tcerr << \"mapBuffers(): Invalid dmabuf\" << endl;\n>  \t\t\treturn report(Op_mapBuffers, TestFail);\n>  \t\t}\n> @@ -287,13 +287,16 @@ protected:\n>  \n>  \t\t/* Test mapBuffers(). */\n>  \t\tstd::vector<IPABuffer> 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 = FileDescriptor(fd_);\n> +\t\tbuffers[0].planes[0].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 = FileDescriptor(fd_);\n> +\t\tbuffers[1].planes[0].length = 4096;\n> +\t\tbuffers[1].planes[1].fd = FileDescriptor(fd_);\n> +\t\tbuffers[1].planes[1].length = 4096;\n>  \n>  \t\tret = INVOKE(mapBuffers, buffers);\n>  \t\tif (ret == TestFail)","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C317A6045E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  7 Jan 2020 04:01:51 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 17C2252F;\n\tTue,  7 Jan 2020 04:01:51 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578366111;\n\tbh=Lbsshl8Bx1j+hHK1/lCcSP3vOlpnA0WrGKvzRc6fIJw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lp4bQ1kbPhkOqzqRfkoi4PzG0ggMbbuc7nHYzg2U3uUwYyKp2uhvdlSNldifjH8bf\n\tsBxJ5jh+WkYBBVracKsGZDhtrZsihzWJ1rcFlzC/UV6EmOgIJpQ7bWPJFJ95l+IqN9\n\tGfaK3AgZrHsiaXWHxhzSjxX8+DjfiqtZjMD4pMxk=","Date":"Tue, 7 Jan 2020 05:01:08 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200107030108.GH22189@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191230120510.938333-4-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 03/25] ipa: Switch to FrameBuffer\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 07 Jan 2020 03:01:52 -0000"}},{"id":3388,"web_url":"https://patchwork.libcamera.org/comment/3388/","msgid":"<20200108030437.GD10933@pendragon.ideasonboard.com>","date":"2020-01-08T03:04:37","subject":"Re: [libcamera-devel] [PATCH v2 03/25] ipa: Switch to FrameBuffer\n\tinterface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOne more thing.\n\nOn Tue, Jan 07, 2020 at 05:01:08AM +0200, Laurent Pinchart wrote:\n> On Mon, Dec 30, 2019 at 01:04:48PM +0100, Niklas Söderlund wrote:\n> > Switch the IPA interfaces and implementations to use the Framebuffer\n> > interface.\n> > \n> > - The IPA interface is switch to use the simpler FrameBuffer::Plane\n> \n> s/switch/switched/\n> \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> > \n> > The biggest change can be observed in the test case where it is\n> > demonstrated that FrameBuffer objects only carry the necessary number of\n> > planes and not a fixed number. As a consequence of this the test needs\n> > to verify the file descriptors it injects and not depend on the library\n> > to handle uninitialized fields.\n> \n> Really ? It seems you set the number of planes to 3 and only initialize\n> some of them. Did I miss something ?\n> \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/ipa/ipa_interface.h              |  2 +-\n> >  src/ipa/libipa/ipa_interface_wrapper.cpp |  9 ++----\n> >  src/ipa/rkisp1/rkisp1.cpp                | 30 +++++++++++++----\n> >  src/libcamera/ipa_context_wrapper.cpp    |  8 ++---\n> >  src/libcamera/ipa_interface.cpp          |  7 ++--\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++--\n> >  test/ipa/ipa_wrappers_test.cpp           | 41 +++++++++++++-----------\n> >  7 files changed, 66 insertions(+), 43 deletions(-)\n> > \n> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h\n> > index 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<FrameBuffer::Plane> planes;\n> \n> I wonder if we have a file descriptor lifetime management issue here.\n> The IPA API doesn't define ownership for file descriptors :-S I think we\n> need to address this first (to decide if the mapBuffers() method should\n> be given ownership of the passed file descriptors or if it should borrow\n> them), and then revisit this patch (with Jacopo's comments on 02/25\n> taken into account). Should this then become a vector of unique_ptr, or\n> a vector of const references ?\n> \n> >  };\n> >  \n> >  struct IPAOperationData {\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > index 6a389dfa714ab535..3628a785dc3e63f4 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > @@ -144,17 +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<Plane> &planes = buffer.memory.planes();\n> > +\t\tstd::vector<FrameBuffer::Plane> &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\t/** \\todo Create a Dmabuf class to implement RAII. */\n> > -\t\t\t::close(_buffer.planes[j].dmabuf);\n> > +\t\t\tplanes[j].fd = FileDescriptor(_buffer.planes[j].dmabuf);\n> > +\t\t\tplanes[j].length = _buffer.planes[j].length;\n> >  \t\t}\n> >  \t}\n> >  \n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 744a16ae5b9aa420..63776563b149f7ed 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -10,6 +10,7 @@\n> >  #include <queue>\n> >  #include <stdint.h>\n> >  #include <string.h>\n> > +#include <sys/mman.h>\n> >  \n> >  #include <linux/rkisp1-config.h>\n> >  \n> > @@ -48,7 +49,8 @@ private:\n> >  \tvoid setControls(unsigned int frame);\n> >  \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n> >  \n> > -\tstd::map<unsigned int, BufferMemory> bufferInfo_;\n> > +\tstd::map<unsigned int, FrameBuffer> buffers_;\n> > +\tstd::map<unsigned int, void *> buffersMemory_;\n> >  \n> >  \tControlInfoMap ctrls_;\n> >  \n> > @@ -102,15 +104,29 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,\n> >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &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\tbuffers_.emplace(buffer.id, FrameBuffer(buffer.planes));\n\n /* \\todo Provide a helper to mmap() buffers (possibly exposed to applications) */\n\n> > +\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n> > +\t\t\t\t\t\t buffers_[buffer.id].planes()[0].length,\n> > +\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n> > +\t\t\t\t\t\t MAP_SHARED,\n> > +\t\t\t\t\t\t buffers_[buffer.id].planes()[0].fd.fd(),\n> > +\t\t\t\t\t\t 0);\n> > +\n> > +\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n> > +\t\t\tint ret = -errno;\n> > +\t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n> > +\t\t\t\t\t      << strerror(-ret);\n> > +\t\t}\n> >  \t}\n> >  }\n> >  \n> >  void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)\n> >  {\n> > -\tfor (unsigned int id : ids)\n> > -\t\tbufferInfo_.erase(id);\n> > +\tfor (unsigned int id : ids) {\n> > +\t\tmunmap(buffersMemory_[id], buffers_[id].planes()[0].length);\n> > +\t\tbuffersMemory_.erase(id);\n> > +\t\tbuffers_.erase(id);\n> > +\t}\n> >  }\n> >  \n> >  void IPARkISP1::processEvent(const IPAOperationData &event)\n> > @@ -121,7 +137,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<rkisp1_stat_buffer *>(bufferInfo_[bufferId].planes()[0].mem());\n> > +\t\t\tstatic_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]);\n> >  \n> >  \t\tupdateStatistics(frame, stats);\n> >  \t\tbreak;\n> > @@ -131,7 +147,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<rkisp1_isp_params_cfg *>(bufferInfo_[bufferId].planes()[0].mem());\n> > +\t\t\tstatic_cast<rkisp1_isp_params_cfg *>(buffersMemory_[bufferId]);\n> >  \n> >  \t\tqueueRequest(frame, params, event.controls[0]);\n> >  \t\tbreak;\n> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> > index 9603fdac87150aa0..946a2fd8cab1782a 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<IPABuffer> &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<Plane> &planes = buffer.memory.planes();\n> > +\t\tconst std::vector<FrameBuffer::Plane> &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.fd();\n> > +\t\t\tc_buffer.planes[j].length = plane.length;\n> >  \t\t}\n> >  \t}\n> >  \n> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> > index 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> >  /**\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 7e41222e3d01a475..d7ee95ded0f76027 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -687,14 +687,22 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n> >  \t}\n> >  \n> >  \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> > +\t\tFrameBuffer::Plane plane;\n> > +\t\tplane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());\n> > +\t\tplane.length = paramPool_.buffers()[i].planes()[0].length();\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 = { plane } });\n> >  \t\tparamBuffers_.push(new Buffer(i));\n> >  \t}\n> >  \n> >  \tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> > +\t\tFrameBuffer::Plane plane;\n> > +\t\tplane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());\n> > +\t\tplane.length = statPool_.buffers()[i].planes()[0].length();\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 = { plane } });\n> >  \t\tstatBuffers_.push(new Buffer(i));\n> >  \t}\n> >  \n> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> > index a1e34ad52317f47b..e711e4fe318d8179 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.fd() == -1 ||\n> > +\t\t    buffers[0].planes[1].fd.fd() != -1 ||\n> > +\t\t    buffers[0].planes[2].fd.fd() != -1 ||\n> > +\t\t    buffers[0].planes[0].fd.fd() == -1 ||\n> > +\t\t    buffers[1].planes[1].fd.fd() == -1 ||\n> > +\t\t    buffers[1].planes[2].fd.fd() != -1) {\n> >  \t\t\tcerr << \"mapBuffers(): Invalid dmabuf\" << endl;\n> >  \t\t\treturn report(Op_mapBuffers, TestFail);\n> >  \t\t}\n> > @@ -287,13 +287,16 @@ protected:\n> >  \n> >  \t\t/* Test mapBuffers(). */\n> >  \t\tstd::vector<IPABuffer> 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 = FileDescriptor(fd_);\n> > +\t\tbuffers[0].planes[0].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 = FileDescriptor(fd_);\n> > +\t\tbuffers[1].planes[0].length = 4096;\n> > +\t\tbuffers[1].planes[1].fd = FileDescriptor(fd_);\n> > +\t\tbuffers[1].planes[1].length = 4096;\n> >  \n> >  \t\tret = INVOKE(mapBuffers, buffers);\n> >  \t\tif (ret == TestFail)","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 885B96045D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Jan 2020 04:04:48 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 17EDD52F;\n\tWed,  8 Jan 2020 04:04:48 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578452688;\n\tbh=fHYOKtnuu9jZWGqkkwN+APVWcHpTwejaYr3yp5JSU6k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sEvOIggVo1ZtguG7ZD8Z0OLXDMLG5SWS5ItQDUZZfuKVc7gZsHZ4IkPsOEXPZ8b02\n\tgKEn0SHuAZEZpmIkhZLkopyqhqQ2YuNUmpqSb8oZM0doy1iELGf0xK7PEpwtl2esE7\n\tgTBemK6X4h7E3ibJ/tq7KWAEjkWASfosBndrdUPo=","Date":"Wed, 8 Jan 2020 05:04:37 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200108030437.GD10933@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-4-niklas.soderlund@ragnatech.se>\n\t<20200107030108.GH22189@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200107030108.GH22189@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 03/25] ipa: Switch to FrameBuffer\n\tinterface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 08 Jan 2020 03:04:48 -0000"}}]