[{"id":3339,"web_url":"https://patchwork.libcamera.org/comment/3339/","msgid":"<20200107105356.y6ktknay3xjeo2ve@uno.localdomain>","date":"2020-01-07T10:53:56","subject":"Re: [libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch\n\tfrom Plane to FrameBuffer::Plane","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Dec 30, 2019 at 01:04:49PM +0100, Niklas Söderlund wrote:\n> It is not libcameras responsibility to handle memory mappings. Switch\n> from the soon to be removed Plane class which deals with memory\n> mappings to FrameBuffer::Plane which just describes it. This makes the\n\nmapping\n\n> transition to the full FrameBuffer easier.\n>\n> As the full FrameBuffer interface have not yet spread to all parts of\n\ns/have/has\n\n> libcamera core it is hard to create efficient caching of memory mappings\n> in the qcam application. This will be fixed in a later patch, for now\n> the dmabuf is mapped and unmapped each time it is seen by the\n> application.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/buffer.h | 6 +++---\n> src/cam/buffer_writer.cpp | 10 +++++++---\n> src/libcamera/buffer.cpp | 4 ++--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++----\n> src/libcamera/stream.cpp | 6 ++++--\n> src/libcamera/v4l2_videodevice.cpp | 13 +++++++------\n> src/qcam/main_window.cpp | 14 ++++++++++----\n> test/camera/buffer_import.cpp | 2 +-\n> 8 files changed, 38 insertions(+), 25 deletions(-)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 862031123b4b510c..71633492e4752efb 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -89,11 +89,11 @@ private:\n> class BufferMemory final\n> {\n> public:\n> -\tconst std::vector &planes() const { return planes_; }\n> -\tstd::vector &planes() { return planes_; }\n> +\tconst std::vector &planes() const { return planes_; }\n> +\tstd::vector &planes() { return planes_; }\n>\n> private:\n> -\tstd::vector planes_;\n> +\tstd::vector planes_;\n> };\n>\n> class BufferPool final\n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index c33e99c5f8173db8..3e84068e66bb4dd7 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \"buffer_writer.h\"\n> @@ -43,9 +44,10 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> \t\treturn -errno;\n>\n> \tBufferMemory *mem = buffer->mem();\n> -\tfor (Plane &plane : mem->planes()) {\n> -\t\tvoid *data = plane.mem();\n> -\t\tunsigned int length = plane.length();\n> +\tfor (const FrameBuffer::Plane &plane : mem->planes()) {\n> +\t\tvoid *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> +\t\t\t\t plane.fd.fd(), 0);\n> +\t\tunsigned int length = plane.length;\n>\n> \t\tret = ::write(fd, data, length);\n> \t\tif (ret < 0) {\n> @@ -59,6 +61,8 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> \t\t\t\t << length << std::endl;\n> \t\t\tbreak;\n> \t\t}\n> +\n> +\t\tmunmap(data, length);\n> \t}\n>\n> \tclose(fd);\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 7c78b9b8f1e90aa5..fde0b33511c64c9e 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -246,13 +246,13 @@ void *Plane::mem()\n> /**\n> * \\fn BufferMemory::planes() const\n> * \\brief Retrieve the planes within the buffer\n> - * \\return A const reference to a vector holding all Planes within the buffer\n> + * \\return A const reference to a vector holding all planes within the buffer\n> */\n>\n> /**\n> * \\fn BufferMemory::planes()\n> * \\brief Retrieve the planes within the buffer\n> - * \\return A reference to a vector holding all Planes within the buffer\n> + * \\return A reference to a vector holding all planes within the buffer\n> */\n>\n> /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index d7ee95ded0f76027..bb652d0da9c6df52 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -688,8 +688,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\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> +\t\tplane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());\n\nIsn't paramPool_.buffers()[i].planes()[0].fd a FileDescriptor already ?\nYou have a copy constructor for it..\n\nAlthough, this will later go away so...\n\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 .planes = { plane } });\n> @@ -698,8 +698,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\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> +\t\tplane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());\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 .planes = { plane } });\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 45f31ae1e2daeb53..a6adc0de5da40063 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -577,8 +577,10 @@ int Stream::mapBuffer(const Buffer *buffer)\n> \t\tif (dmabufs[i] == -1)\n> \t\t\tbreak;\n>\n> -\t\tmem->planes().emplace_back();\n> -\t\tmem->planes().back().setDmabuf(dmabufs[i], 0);\n> +\t\tFrameBuffer::Plane plane;\n> +\t\tplane.fd = FileDescriptor(dmabufs[i]);\n> +\t\tplane.length = 0;\n> +\t\tmem->planes().emplace_back(plane);\n\nEmplacing an already constructed object calls the copy constructor.\nYou could just push_back here I think.\n\n> \t}\n>\n> \t/* Remove the buffer from the cache and return its index. */\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 13e023237dab0daf..f3f5303b7f470f63 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -921,9 +921,10 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,\n> \t\treturn ret;\n> \t}\n>\n> -\tbuffer->planes().emplace_back();\n> -\tPlane &plane = buffer->planes().back();\n> -\tplane.setDmabuf(expbuf.fd, length);\n> +\tFrameBuffer::Plane plane;\n> +\tplane.fd = FileDescriptor(expbuf.fd);\n> +\tplane.length = length;\n> +\tbuffer->planes().emplace_back(plane);\n\nSame.\n\nOr you could provide a constructor for FrameBuffer::Plane that accepts\nand unsigned int fd and an unisgned in length, then you can actually\nemplace.\n\nNits apart\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t::close(expbuf.fd);\n>\n> \treturn 0;\n> @@ -986,14 +987,14 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n>\n> \tbool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> \tBufferMemory *mem = &bufferPool_->buffers()[buf.index];\n> -\tconst std::vector &planes = mem->planes();\n> +\tconst std::vector &planes = mem->planes();\n>\n> \tif (buf.memory == V4L2_MEMORY_DMABUF) {\n> \t\tif (multiPlanar) {\n> \t\t\tfor (unsigned int p = 0; p < planes.size(); ++p)\n> -\t\t\t\tv4l2Planes[p].m.fd = planes[p].dmabuf();\n> +\t\t\t\tv4l2Planes[p].m.fd = planes[p].fd.fd();\n> \t\t} else {\n> -\t\t\tbuf.m.fd = planes[0].dmabuf();\n> +\t\t\tbuf.m.fd = planes[0].fd.fd();\n> \t\t}\n> \t}\n>\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 0c7ca61ac12ec41c..11fb67a509e973f5 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> #include \n> #include \n> +#include \n>\n> #include \n> #include \n> @@ -296,13 +297,18 @@ void MainWindow::requestComplete(Request *request)\n>\n> int MainWindow::display(Buffer *buffer)\n> {\n> -\tBufferMemory *mem = buffer->mem();\n> -\tif (mem->planes().size() != 1)\n> +\tif (buffer->mem()->planes().size() != 1)\n> \t\treturn -EINVAL;\n>\n> -\tPlane &plane = mem->planes().front();\n> -\tunsigned char *raw = static_cast(plane.mem());\n> +\t/* \\todo: Once the FrameBuffer is done cache mapped memory. */\n> +\tconst FrameBuffer::Plane &plane = buffer->mem()->planes().front();\n> +\tvoid *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> +\t\t\t plane.fd.fd(), 0);\n> +\n> +\tunsigned char *raw = static_cast(memory);\n> \tviewfinder_->display(raw, buffer->bytesused());\n>\n> +\tmunmap(memory, plane.length);\n> +\n> \treturn 0;\n> }\n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 3efe02704c02f691..171540edd96f9fca 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -178,7 +178,7 @@ private:\n>\n> \t\tuint64_t cookie = index;\n> \t\tBufferMemory &mem = pool_.buffers()[index];\n> -\t\tint dmabuf = mem.planes()[0].dmabuf();\n> +\t\tint dmabuf = mem.planes()[0].fd.fd();\n>\n> \t\trequestReady.emit(cookie, dmabuf);\n>\n> --\n> 2.24.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D7BB60460\n\tfor ;\n\tTue, 7 Jan 2020 11:51:32 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 99EE9100008;\n\tTue, 7 Jan 2020 10:51:31 +0000 (UTC)"],"Date":"Tue, 7 Jan 2020 11:53:56 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200107105356.y6ktknay3xjeo2ve@uno.localdomain>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"znewholwu46w25a5\"","Content-Disposition":"inline","In-Reply-To":"<20191230120510.938333-5-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch\n\tfrom Plane to FrameBuffer::Plane","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, 07 Jan 2020 10:51:32 -0000"}},{"id":3353,"web_url":"https://patchwork.libcamera.org/comment/3353/","msgid":"<20200107165629.GK4871@pendragon.ideasonboard.com>","date":"2020-01-07T16:56:29","subject":"Re: [libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch\n\tfrom Plane to FrameBuffer::Plane","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 Tue, Jan 07, 2020 at 11:53:56AM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 30, 2019 at 01:04:49PM +0100, Niklas Söderlund wrote:\n> > It is not libcameras responsibility to handle memory mappings. Switch\n\ns/libcameras/libcamera's/\n\n> > from the soon to be removed Plane class which deals with memory\n> > mappings to FrameBuffer::Plane which just describes it. This makes the\n> \n> mapping\n\nBoth \"mapping\" and \"mappings\" work here.\n\n> > transition to the full FrameBuffer easier.\n> >\n> > As the full FrameBuffer interface have not yet spread to all parts of\n> \n> s/have/has\n> \n> > libcamera core it is hard to create efficient caching of memory mappings\n> > in the qcam application. This will be fixed in a later patch, for now\n> > the dmabuf is mapped and unmapped each time it is seen by the\n> > application.\n\nOuch :-) But it's ok for now if it gets fixed later.\n\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/buffer.h | 6 +++---\n> > src/cam/buffer_writer.cpp | 10 +++++++---\n> > src/libcamera/buffer.cpp | 4 ++--\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++----\n> > src/libcamera/stream.cpp | 6 ++++--\n> > src/libcamera/v4l2_videodevice.cpp | 13 +++++++------\n> > src/qcam/main_window.cpp | 14 ++++++++++----\n> > test/camera/buffer_import.cpp | 2 +-\n> > 8 files changed, 38 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 862031123b4b510c..71633492e4752efb 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -89,11 +89,11 @@ private:\n> > class BufferMemory final\n> > {\n> > public:\n> > -\tconst std::vector &planes() const { return planes_; }\n> > -\tstd::vector &planes() { return planes_; }\n> > +\tconst std::vector &planes() const { return planes_; }\n> > +\tstd::vector &planes() { return planes_; }\n> >\n> > private:\n> > -\tstd::vector planes_;\n> > +\tstd::vector planes_;\n> > };\n> >\n> > class BufferPool final\n> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > index c33e99c5f8173db8..3e84068e66bb4dd7 100644\n> > --- a/src/cam/buffer_writer.cpp\n> > +++ b/src/cam/buffer_writer.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \"buffer_writer.h\"\n> > @@ -43,9 +44,10 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> > \t\treturn -errno;\n> >\n> > \tBufferMemory *mem = buffer->mem();\n> > -\tfor (Plane &plane : mem->planes()) {\n> > -\t\tvoid *data = plane.mem();\n> > -\t\tunsigned int length = plane.length();\n> > +\tfor (const FrameBuffer::Plane &plane : mem->planes()) {\n\nCan you add a\n\n\t/* \\todo Cache memory mappings. */\n\nand add a patch that fixes this ?\n\n> > +\t\tvoid *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> > +\t\t\t\t plane.fd.fd(), 0);\n> > +\t\tunsigned int length = plane.length;\n> >\n> > \t\tret = ::write(fd, data, length);\n> > \t\tif (ret < 0) {\n> > @@ -59,6 +61,8 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> > \t\t\t\t << length << std::endl;\n> > \t\t\tbreak;\n> > \t\t}\n> > +\n> > +\t\tmunmap(data, length);\n> > \t}\n> >\n> > \tclose(fd);\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 7c78b9b8f1e90aa5..fde0b33511c64c9e 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -246,13 +246,13 @@ void *Plane::mem()\n> > /**\n> > * \\fn BufferMemory::planes() const\n> > * \\brief Retrieve the planes within the buffer\n> > - * \\return A const reference to a vector holding all Planes within the buffer\n> > + * \\return A const reference to a vector holding all planes within the buffer\n> > */\n> >\n> > /**\n> > * \\fn BufferMemory::planes()\n> > * \\brief Retrieve the planes within the buffer\n> > - * \\return A reference to a vector holding all Planes within the buffer\n> > + * \\return A reference to a vector holding all planes within the buffer\n> > */\n> >\n> > /**\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index d7ee95ded0f76027..bb652d0da9c6df52 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -688,8 +688,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\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> > +\t\tplane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());\n> \n> Isn't paramPool_.buffers()[i].planes()[0].fd a FileDescriptor already ?\n> You have a copy constructor for it..\n> \n> Although, this will later go away so...\n> \n> > +\t\tplane.length = paramPool_.buffers()[i].planes()[0].length;\n\nI think we can even simplify it further.\n\n\t\tplane = paramPool_.buffers()[i].planes()[0];\n\n> >\n> > \t\tdata->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,\n> > \t\t\t\t\t .planes = { plane } });\n\nOr even\n\n \t\tdata->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,\n \t\t\t\t\t .planes = paramPool_.buffers()[i].planes() });\n\nAs Jacopo said the code will indeed go away later, but if it's not too\npainful to fix, let's keep a good history.\n\n> > @@ -698,8 +698,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\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> > +\t\tplane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());\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 .planes = { plane } });\n\nSame here.\n\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index 45f31ae1e2daeb53..a6adc0de5da40063 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -577,8 +577,10 @@ int Stream::mapBuffer(const Buffer *buffer)\n> > \t\tif (dmabufs[i] == -1)\n> > \t\t\tbreak;\n> >\n> > -\t\tmem->planes().emplace_back();\n> > -\t\tmem->planes().back().setDmabuf(dmabufs[i], 0);\n> > +\t\tFrameBuffer::Plane plane;\n> > +\t\tplane.fd = FileDescriptor(dmabufs[i]);\n> > +\t\tplane.length = 0;\n> > +\t\tmem->planes().emplace_back(plane);\n> \n> Emplacing an already constructed object calls the copy constructor.\n> You could just push_back here I think.\n\npush_back is worse is it calls the default constructor followed by the\ncopy assignment operator.\n\n> > \t}\n> >\n> > \t/* Remove the buffer from the cache and return its index. */\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 13e023237dab0daf..f3f5303b7f470f63 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -921,9 +921,10 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\tbuffer->planes().emplace_back();\n> > -\tPlane &plane = buffer->planes().back();\n> > -\tplane.setDmabuf(expbuf.fd, length);\n> > +\tFrameBuffer::Plane plane;\n> > +\tplane.fd = FileDescriptor(expbuf.fd);\n> > +\tplane.length = length;\n> > +\tbuffer->planes().emplace_back(plane);\n> \n> Same.\n> \n> Or you could provide a constructor for FrameBuffer::Plane that accepts\n> and unsigned int fd and an unisgned in length, then you can actually\n> emplace.\n\nIt could make sense to do so, but I would then provide a const FileDescriptor\n&, not an int fd.\n\n> Nits apart\n> Reviewed-by: Jacopo Mondi \n> \n> > \t::close(expbuf.fd);\n> >\n> > \treturn 0;\n> > @@ -986,14 +987,14 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> >\n> > \tbool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > \tBufferMemory *mem = &bufferPool_->buffers()[buf.index];\n> > -\tconst std::vector &planes = mem->planes();\n> > +\tconst std::vector &planes = mem->planes();\n> >\n> > \tif (buf.memory == V4L2_MEMORY_DMABUF) {\n> > \t\tif (multiPlanar) {\n> > \t\t\tfor (unsigned int p = 0; p < planes.size(); ++p)\n> > -\t\t\t\tv4l2Planes[p].m.fd = planes[p].dmabuf();\n> > +\t\t\t\tv4l2Planes[p].m.fd = planes[p].fd.fd();\n> > \t\t} else {\n> > -\t\t\tbuf.m.fd = planes[0].dmabuf();\n> > +\t\t\tbuf.m.fd = planes[0].fd.fd();\n> > \t\t}\n> > \t}\n> >\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 0c7ca61ac12ec41c..11fb67a509e973f5 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> > #include \n> > @@ -296,13 +297,18 @@ void MainWindow::requestComplete(Request *request)\n> >\n> > int MainWindow::display(Buffer *buffer)\n> > {\n> > -\tBufferMemory *mem = buffer->mem();\n> > -\tif (mem->planes().size() != 1)\n> > +\tif (buffer->mem()->planes().size() != 1)\n> > \t\treturn -EINVAL;\n> >\n> > -\tPlane &plane = mem->planes().front();\n> > -\tunsigned char *raw = static_cast(plane.mem());\n> > +\t/* \\todo: Once the FrameBuffer is done cache mapped memory. */\n\ns/todo:/todo/\n\nReviewed-by: Laurent Pinchart \n\n> > +\tconst FrameBuffer::Plane &plane = buffer->mem()->planes().front();\n> > +\tvoid *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> > +\t\t\t plane.fd.fd(), 0);\n> > +\n> > +\tunsigned char *raw = static_cast(memory);\n> > \tviewfinder_->display(raw, buffer->bytesused());\n> >\n> > +\tmunmap(memory, plane.length);\n> > +\n> > \treturn 0;\n> > }\n> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > index 3efe02704c02f691..171540edd96f9fca 100644\n> > --- a/test/camera/buffer_import.cpp\n> > +++ b/test/camera/buffer_import.cpp\n> > @@ -178,7 +178,7 @@ private:\n> >\n> > \t\tuint64_t cookie = index;\n> > \t\tBufferMemory &mem = pool_.buffers()[index];\n> > -\t\tint dmabuf = mem.planes()[0].dmabuf();\n> > +\t\tint dmabuf = mem.planes()[0].fd.fd();\n> >\n> > \t\trequestReady.emit(cookie, dmabuf);\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 690366063B\n\tfor ;\n\tTue, 7 Jan 2020 17:56:41 +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 627A5A46;\n\tTue, 7 Jan 2020 17:56:40 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578416201;\n\tbh=yG4PxHCViMqCQKw54GJh5F5wn2IE6/o/wwsTP5qvOOY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nwXGKr3Pllnz5ZiqSG/LvNqy5DewfBWvA02DJdp2X4mkeHLhvusnUjQh2OXpRWsKq\n\tNoFokPDPsRX+r6tOgNeeE5ya33SSUrruPF3VcwW9R7dODbiUDWMWkRbIDebRseVdS0\n\tz5YRljq6Xr8Fyw03NRdiT1ZredirDkOj2pA79zGc=","Date":"Tue, 7 Jan 2020 18:56:29 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200107165629.GK4871@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-5-niklas.soderlund@ragnatech.se>\n\t<20200107105356.y6ktknay3xjeo2ve@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200107105356.y6ktknay3xjeo2ve@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch\n\tfrom Plane to FrameBuffer::Plane","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, 07 Jan 2020 16:56:41 -0000"}},{"id":3375,"web_url":"https://patchwork.libcamera.org/comment/3375/","msgid":"<20200107232642.GA10933@pendragon.ideasonboard.com>","date":"2020-01-07T23:26:42","subject":"Re: [libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch\n\tfrom Plane to FrameBuffer::Plane","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jan 07, 2020 at 06:56:29PM +0200, Laurent Pinchart wrote:\n> On Tue, Jan 07, 2020 at 11:53:56AM +0100, Jacopo Mondi wrote:\n> > On Mon, Dec 30, 2019 at 01:04:49PM +0100, Niklas Söderlund wrote:\n> > > It is not libcameras responsibility to handle memory mappings. Switch\n> \n> s/libcameras/libcamera's/\n> \n> > > from the soon to be removed Plane class which deals with memory\n> > > mappings to FrameBuffer::Plane which just describes it. This makes the\n> > \n> > mapping\n> \n> Both \"mapping\" and \"mappings\" work here.\n> \n> > > transition to the full FrameBuffer easier.\n> > >\n> > > As the full FrameBuffer interface have not yet spread to all parts of\n> > \n> > s/have/has\n> > \n> > > libcamera core it is hard to create efficient caching of memory mappings\n> > > in the qcam application. This will be fixed in a later patch, for now\n> > > the dmabuf is mapped and unmapped each time it is seen by the\n> > > application.\n> \n> Ouch :-) But it's ok for now if it gets fixed later.\n> \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > include/libcamera/buffer.h | 6 +++---\n> > > src/cam/buffer_writer.cpp | 10 +++++++---\n> > > src/libcamera/buffer.cpp | 4 ++--\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 ++++----\n> > > src/libcamera/stream.cpp | 6 ++++--\n> > > src/libcamera/v4l2_videodevice.cpp | 13 +++++++------\n> > > src/qcam/main_window.cpp | 14 ++++++++++----\n> > > test/camera/buffer_import.cpp | 2 +-\n> > > 8 files changed, 38 insertions(+), 25 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > > index 862031123b4b510c..71633492e4752efb 100644\n> > > --- a/include/libcamera/buffer.h\n> > > +++ b/include/libcamera/buffer.h\n> > > @@ -89,11 +89,11 @@ private:\n> > > class BufferMemory final\n> > > {\n> > > public:\n> > > -\tconst std::vector &planes() const { return planes_; }\n> > > -\tstd::vector &planes() { return planes_; }\n> > > +\tconst std::vector &planes() const { return planes_; }\n> > > +\tstd::vector &planes() { return planes_; }\n> > >\n> > > private:\n> > > -\tstd::vector planes_;\n> > > +\tstd::vector planes_;\n> > > };\n> > >\n> > > class BufferPool final\n> > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > > index c33e99c5f8173db8..3e84068e66bb4dd7 100644\n> > > --- a/src/cam/buffer_writer.cpp\n> > > +++ b/src/cam/buffer_writer.cpp\n> > > @@ -10,6 +10,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \"buffer_writer.h\"\n> > > @@ -43,9 +44,10 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> > > \t\treturn -errno;\n> > >\n> > > \tBufferMemory *mem = buffer->mem();\n> > > -\tfor (Plane &plane : mem->planes()) {\n> > > -\t\tvoid *data = plane.mem();\n> > > -\t\tunsigned int length = plane.length();\n> > > +\tfor (const FrameBuffer::Plane &plane : mem->planes()) {\n> \n> Can you add a\n> \n> \t/* \\todo Cache memory mappings. */\n> \n> and add a patch that fixes this ?\n> \n> > > +\t\tvoid *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> > > +\t\t\t\t plane.fd.fd(), 0);\n> > > +\t\tunsigned int length = plane.length;\n> > >\n> > > \t\tret = ::write(fd, data, length);\n> > > \t\tif (ret < 0) {\n> > > @@ -59,6 +61,8 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> > > \t\t\t\t << length << std::endl;\n> > > \t\t\tbreak;\n> > > \t\t}\n> > > +\n> > > +\t\tmunmap(data, length);\n> > > \t}\n> > >\n> > > \tclose(fd);\n> > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > index 7c78b9b8f1e90aa5..fde0b33511c64c9e 100644\n> > > --- a/src/libcamera/buffer.cpp\n> > > +++ b/src/libcamera/buffer.cpp\n> > > @@ -246,13 +246,13 @@ void *Plane::mem()\n> > > /**\n> > > * \\fn BufferMemory::planes() const\n> > > * \\brief Retrieve the planes within the buffer\n> > > - * \\return A const reference to a vector holding all Planes within the buffer\n> > > + * \\return A const reference to a vector holding all planes within the buffer\n> > > */\n> > >\n> > > /**\n> > > * \\fn BufferMemory::planes()\n> > > * \\brief Retrieve the planes within the buffer\n> > > - * \\return A reference to a vector holding all Planes within the buffer\n> > > + * \\return A reference to a vector holding all planes within the buffer\n> > > */\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index d7ee95ded0f76027..bb652d0da9c6df52 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -688,8 +688,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\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> > > +\t\tplane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());\n> > \n> > Isn't paramPool_.buffers()[i].planes()[0].fd a FileDescriptor already ?\n> > You have a copy constructor for it..\n> > \n> > Although, this will later go away so...\n> > \n> > > +\t\tplane.length = paramPool_.buffers()[i].planes()[0].length;\n> \n> I think we can even simplify it further.\n> \n> \t\tplane = paramPool_.buffers()[i].planes()[0];\n> \n> > >\n> > > \t\tdata->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,\n> > > \t\t\t\t\t .planes = { plane } });\n> \n> Or even\n> \n> \t\tdata->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,\n> \t\t\t\t\t .planes = paramPool_.buffers()[i].planes() });\n> \n> As Jacopo said the code will indeed go away later, but if it's not too\n> painful to fix, let's keep a good history.\n> \n> > > @@ -698,8 +698,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\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> > > +\t\tplane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());\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 .planes = { plane } });\n> \n> Same here.\n> \n> > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > > index 45f31ae1e2daeb53..a6adc0de5da40063 100644\n> > > --- a/src/libcamera/stream.cpp\n> > > +++ b/src/libcamera/stream.cpp\n> > > @@ -577,8 +577,10 @@ int Stream::mapBuffer(const Buffer *buffer)\n> > > \t\tif (dmabufs[i] == -1)\n> > > \t\t\tbreak;\n> > >\n> > > -\t\tmem->planes().emplace_back();\n> > > -\t\tmem->planes().back().setDmabuf(dmabufs[i], 0);\n> > > +\t\tFrameBuffer::Plane plane;\n> > > +\t\tplane.fd = FileDescriptor(dmabufs[i]);\n> > > +\t\tplane.length = 0;\n> > > +\t\tmem->planes().emplace_back(plane);\n> > \n> > Emplacing an already constructed object calls the copy constructor.\n> > You could just push_back here I think.\n> \n> push_back is worse is it calls the default constructor followed by the\n> copy assignment operator.\n\nI have to apologize for this, I was wrong. push_back() doesn't call the\ndefault constructor, so Jacopo was right.\n\n> > > \t}\n> > >\n> > > \t/* Remove the buffer from the cache and return its index. */\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 13e023237dab0daf..f3f5303b7f470f63 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -921,9 +921,10 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > -\tbuffer->planes().emplace_back();\n> > > -\tPlane &plane = buffer->planes().back();\n> > > -\tplane.setDmabuf(expbuf.fd, length);\n> > > +\tFrameBuffer::Plane plane;\n> > > +\tplane.fd = FileDescriptor(expbuf.fd);\n> > > +\tplane.length = length;\n> > > +\tbuffer->planes().emplace_back(plane);\n> > \n> > Same.\n> > \n> > Or you could provide a constructor for FrameBuffer::Plane that accepts\n> > and unsigned int fd and an unisgned in length, then you can actually\n> > emplace.\n> \n> It could make sense to do so, but I would then provide a const FileDescriptor\n> &, not an int fd.\n> \n> > Nits apart\n> > Reviewed-by: Jacopo Mondi \n> > \n> > > \t::close(expbuf.fd);\n> > >\n> > > \treturn 0;\n> > > @@ -986,14 +987,14 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> > >\n> > > \tbool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > > \tBufferMemory *mem = &bufferPool_->buffers()[buf.index];\n> > > -\tconst std::vector &planes = mem->planes();\n> > > +\tconst std::vector &planes = mem->planes();\n> > >\n> > > \tif (buf.memory == V4L2_MEMORY_DMABUF) {\n> > > \t\tif (multiPlanar) {\n> > > \t\t\tfor (unsigned int p = 0; p < planes.size(); ++p)\n> > > -\t\t\t\tv4l2Planes[p].m.fd = planes[p].dmabuf();\n> > > +\t\t\t\tv4l2Planes[p].m.fd = planes[p].fd.fd();\n> > > \t\t} else {\n> > > -\t\t\tbuf.m.fd = planes[0].dmabuf();\n> > > +\t\t\tbuf.m.fd = planes[0].fd.fd();\n> > > \t\t}\n> > > \t}\n> > >\n> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > index 0c7ca61ac12ec41c..11fb67a509e973f5 100644\n> > > --- a/src/qcam/main_window.cpp\n> > > +++ b/src/qcam/main_window.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > > #include \n> > > @@ -296,13 +297,18 @@ void MainWindow::requestComplete(Request *request)\n> > >\n> > > int MainWindow::display(Buffer *buffer)\n> > > {\n> > > -\tBufferMemory *mem = buffer->mem();\n> > > -\tif (mem->planes().size() != 1)\n> > > +\tif (buffer->mem()->planes().size() != 1)\n> > > \t\treturn -EINVAL;\n> > >\n> > > -\tPlane &plane = mem->planes().front();\n> > > -\tunsigned char *raw = static_cast(plane.mem());\n> > > +\t/* \\todo: Once the FrameBuffer is done cache mapped memory. */\n> \n> s/todo:/todo/\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > > +\tconst FrameBuffer::Plane &plane = buffer->mem()->planes().front();\n> > > +\tvoid *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n> > > +\t\t\t plane.fd.fd(), 0);\n> > > +\n> > > +\tunsigned char *raw = static_cast(memory);\n> > > \tviewfinder_->display(raw, buffer->bytesused());\n> > >\n> > > +\tmunmap(memory, plane.length);\n> > > +\n> > > \treturn 0;\n> > > }\n> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > > index 3efe02704c02f691..171540edd96f9fca 100644\n> > > --- a/test/camera/buffer_import.cpp\n> > > +++ b/test/camera/buffer_import.cpp\n> > > @@ -178,7 +178,7 @@ private:\n> > >\n> > > \t\tuint64_t cookie = index;\n> > > \t\tBufferMemory &mem = pool_.buffers()[index];\n> > > -\t\tint dmabuf = mem.planes()[0].dmabuf();\n> > > +\t\tint dmabuf = mem.planes()[0].fd.fd();\n> > >\n> > > \t\trequestReady.emit(cookie, dmabuf);\n> > >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1CAF60612\n\tfor ;\n\tWed, 8 Jan 2020 00:26:54 +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 EEABF30F;\n\tWed, 8 Jan 2020 00:26:53 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578439614;\n\tbh=833oXq8hBSeFvBk9S3y9PYwataxeS4v9E9PP5L533eQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zv3+593oK2ywGyo6AMDOUyCd84voKyeoVUlf2AQAY7OYhPVSqk45pp+cUJokkgdEB\n\tehsZE9JdhzagtmEr6gbW1I8XdPhSyRrMXG91DJybCIfb0BRlxyJwU0JjczWtaMCeg3\n\t9ErQYyKvUEnrMKxSPXZ3Hhxu8b9LTyqKkaBTWh8s=","Date":"Wed, 8 Jan 2020 01:26:42 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200107232642.GA10933@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-5-niklas.soderlund@ragnatech.se>\n\t<20200107105356.y6ktknay3xjeo2ve@uno.localdomain>\n\t<20200107165629.GK4871@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200107165629.GK4871@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch\n\tfrom Plane to FrameBuffer::Plane","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, 07 Jan 2020 23:26:55 -0000"}}]