[{"id":3145,"web_url":"https://patchwork.libcamera.org/comment/3145/","msgid":"<20191127115534.szobyp3tnaurfyzs@uno.localdomain>","date":"2019-11-27T11:55:34","subject":"Re: [libcamera-devel] [PATCH 05/30] libcamera: buffer: Add Dmabuf\n\tto describe a dma buffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Nov 27, 2019 at 12:35:55AM +0100, Niklas Söderlund wrote:\n> A FrameBuffer object that holds a frame captured from a sensor consists\n> of one or more plane(s). The memory of each plane can be accessed by\n> using a dma buffer. Add a class that describes a dmabuf to make it easy\n> for applications and IPAs to interact with memory.\n>\n\nI'm not sure I like this patch. I mean, abstracting access to the\nplanes memory to applications with an helper class is indeed a good\nidea, but I'm not sure I'll make it a \"Dmabuf\"..\n\nThe whole class and documentation is pretty dmabuf-centric, and while\nthis is the method used to export memory information through a file\ndescriptor, the class also provide a CPU accessible address which maps\nto the memory location that contains image data...\n\nTo me, this is just a plane, which an application can access\nthrough a dmabuf exported file descriptor or through a memory address.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/buffer.h | 19 +++++++\n> src/libcamera/buffer.cpp | 113 +++++++++++++++++++++++++++++++++++++\n> 2 files changed, 132 insertions(+)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 33793b4ccf881eda..3c430afbfe8e9a05 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -58,6 +58,25 @@ private:\n> \tint fd_;\n> };\n>\n> +class Dmabuf final\n> +{\n> +public:\n> +\tDmabuf(int fd, unsigned int length);\n> +\t~Dmabuf();\n> +\n> +\tint fd() const { return fd_.fd(); }\n> +\tunsigned int length() const { return length_; }\n> +\tvoid *mem();\n> +\n> +private:\n> +\tint mmap();\n> +\tint munmap();\n> +\n> +\tFileDescriptor fd_;\n> +\tunsigned int length_;\n> +\tvoid *mem_;\n\nThis really duplicates information we have in a Plane, with just an\nadditional memory mapped address, which is fine, but again to me this\nan abstraction of the memory area a Plan refers to.\n\n> +};\n> +\n> class Plane final\n> {\n> public:\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 0676586ae3be2a61..5516055b2ea885c2 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -177,6 +177,119 @@ FileDescriptor::~FileDescriptor()\n> * an image may or may not be contiguous.\n> */\n>\n> +/**\n> + * \\class Dmabuf\n> + * \\brief A memory region to store a single plane of a frame\n> + *\n> + * Planar pixel formats use multiple memory regions to store planes\n> + * corresponding to the different colour components of a frame. The Dmabuf class\n> + * tracks the specific details of a memory region used to store a single plane\n> + * for a given frame and provides the means to access the memory, both for the\n> + * application and for DMA. A Buffer then contains one or multiple planes\n> + * depending on its pixel format.\n\nI'm not sure I get what \"access to memory both for the application and\nfor DMA\" means. I think you're referring to dmabuf here, but \"DMA\naccess\" is confusing to me (yes, the numerical id is used by drivers\nto retrieve the memory location where to point their DMA engines to,\nbut the users of this class are applications which really should care\nonly about the fact that they have a mean to have a dmabuf-consumable\nrepresentation of the memory area they intend to share between\ndifferent subsystems).\n\nI would also drop from here any refernce to the image format and how\nthey map to one or multiple planes.\n\n> + *\n> + * To support DMA access, planes are associated with dmabuf objects represented\n\nHere as well, what do you mean with \"DMA access\" ?\n\n> + * by file handles. Each plane carries a dmabuf file handle and an offset within\n> + * the buffer. Those file handles may refer to the same dmabuf object, depending\n> + * on whether the devices accessing the memory regions composing the image\n> + * support non-contiguous DMA to planes ore require DMA-contiguous memory.\n\nI'm not following here either. We'll have one Dmabuf instance per\nplane, right ? Each Dmabuf instance should have it's own\ndmabuf-exported representation in form of a file handle and a CPU\naccessible memory pointer where data for that plane are mapped to.\nWhy what the file handles refers to is relevant, and why the DMA engine\ncapabilities are of any interest for applications ?\n\n> + *\n> + * To support CPU access, planes carry the CPU address of their backing memory.\n> + * Similarly to the dmabuf file handles, the CPU addresses for planes composing\n> + * an image may or may not be contiguous.\n\nI don't think it's relevant either.\n\n> + */\n> +\n> +/**\n> + * \\brief Set the dmabuf file handle backing the buffer\n> + * \\param[in] fd The dmabuf file handle\n> + * \\param[in] length The size of the memory region\n> + *\n> + * The \\a fd dmabuf file handle is duplicated and stored.\n> + */\n> +Dmabuf::Dmabuf(int fd, unsigned int length)\n> +\t: fd_(fd), length_(length), mem_(nullptr)\n> +{\n> +}\n> +\n> +Dmabuf::~Dmabuf()\n> +{\n> +\tmunmap();\n> +}\n> +\n> +/**\n> + * \\fn Dmabuf::fd()\n> + * \\brief Get the dmabuf file handle backing the buffer\n> + */\n> +\n> +/**\n> + * \\fn Dmabuf::length()\n> + * \\brief Retrieve the length of the memory region\n> + * \\return The length of the memory region\n> + */\n> +\n> +/**\n> + * \\fn Dmabuf::mem()\n> + * \\brief Retrieve the CPU accessible memory address of the Dmabuf\n> + * \\return The CPU accessible memory address on success or nullptr otherwise.\n> + */\n> +void *Dmabuf::mem()\n> +{\n> +\tif (!mem_)\n> +\t\tmmap();\n> +\n> +\treturn mem_;\n> +}\n> +\n> +/**\n> + * \\brief Map the plane memory data to a CPU accessible address\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int Dmabuf::mmap()\n> +{\n> +\tvoid *map;\n> +\n> +\tif (mem_)\n> +\t\treturn 0;\n> +\n> +\tmap = ::mmap(NULL, length_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_.fd(), 0);\n> +\tif (map == MAP_FAILED) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(Buffer, Error)\n> +\t\t\t<< \"Failed to mmap plane: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tmem_ = map;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Unmap any existing CPU accessible mapping\n> + *\n> + * Unmap the memory mapped by an earlier call to mmap().\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int Dmabuf::munmap()\n> +{\n> +\tint ret = 0;\n> +\n> +\tif (mem_)\n> +\t\tret = ::munmap(mem_, length_);\n> +\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Buffer, Warning)\n> +\t\t\t<< \"Failed to unmap plane: \" << strerror(-ret);\n> +\t} else {\n> +\t\tmem_ = 0;\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n\nAlso, looking at how it is used, the dmabufs_ vector of a FrameBuffer\nreally only uses informations copied from planes_.\n\nI would be tempted to ask why application cannot access Plane\ninstances in a frame buffer and get their dmabuf and memory\nrepresentation from there, which would make this class not required,\nbut I might be missing something...\n\nThanks\n j\n\n> Plane::Plane()\n> \t: fd_(-1), length_(0), mem_(0)\n> {\n> --\n> 2.24.0\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 relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26C4160C33\n\tfor ;\n\tWed, 27 Nov 2019 12:53:28 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 88BAB240005;\n\tWed, 27 Nov 2019 11:53:27 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Wed, 27 Nov 2019 12:55:34 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191127115534.szobyp3tnaurfyzs@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"nc64sqwa7hlvubgo\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-6-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 05/30] libcamera: buffer: Add Dmabuf\n\tto describe a dma buffer","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":"Wed, 27 Nov 2019 11:53:28 -0000"}},{"id":3239,"web_url":"https://patchwork.libcamera.org/comment/3239/","msgid":"<20191210010912.GK18060@pendragon.ideasonboard.com>","date":"2019-12-10T01:09:12","subject":"Re: [libcamera-devel] [PATCH 05/30] libcamera: buffer: Add Dmabuf\n\tto describe a dma buffer","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 Wed, Nov 27, 2019 at 12:55:34PM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 27, 2019 at 12:35:55AM +0100, Niklas Söderlund wrote:\n> > A FrameBuffer object that holds a frame captured from a sensor consists\n> > of one or more plane(s). The memory of each plane can be accessed by\n> > using a dma buffer. Add a class that describes a dmabuf to make it easy\n\ns/dma buffer/dmabuf/ (this is the standard term, dma buffer may refer to\nmany different things)\n\n> > for applications and IPAs to interact with memory.\n> \n> I'm not sure I like this patch. I mean, abstracting access to the\n> planes memory to applications with an helper class is indeed a good\n> idea, but I'm not sure I'll make it a \"Dmabuf\"..\n> \n> The whole class and documentation is pretty dmabuf-centric, and while\n> this is the method used to export memory information through a file\n> descriptor, the class also provide a CPU accessible address which maps\n> to the memory location that contains image data...\n> \n> To me, this is just a plane, which an application can access\n> through a dmabuf exported file descriptor or through a memory address.\n\nI share Jacopo's concern, I think that the Dmabuf and FrameBuffer::Plane\nclasses overlap too much to make me happy. The boundary between them is\nquite fuzzy, which leads me to believe that more work is needed in this\narea to either merge the two classes or clarify their purpose.\n\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/buffer.h | 19 +++++++\n> > src/libcamera/buffer.cpp | 113 +++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 132 insertions(+)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 33793b4ccf881eda..3c430afbfe8e9a05 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -58,6 +58,25 @@ private:\n> > \tint fd_;\n> > };\n> >\n> > +class Dmabuf final\n> > +{\n> > +public:\n> > +\tDmabuf(int fd, unsigned int length);\n> > +\t~Dmabuf();\n> > +\n> > +\tint fd() const { return fd_.fd(); }\n\nHow about returning a const FileDescriptor & instead ? This would give\ncallers an easy way to duplicate the FileDescriptor if needed.\n\n> > +\tunsigned int length() const { return length_; }\n> > +\tvoid *mem();\n> > +\n> > +private:\n> > +\tint mmap();\n> > +\tint munmap();\n> > +\n> > +\tFileDescriptor fd_;\n> > +\tunsigned int length_;\n> > +\tvoid *mem_;\n> \n> This really duplicates information we have in a Plane, with just an\n> additional memory mapped address, which is fine, but again to me this\n> an abstraction of the memory area a Plan refers to.\n\nThe FrameBuffer::Plane class is also missing an offset field, to\ndescribe at which offset in a dmabuf data is stored for a plane. This\nmay be applicable to the Dmabuf class too, although it seems that this\nproposed abstraction of the Dmabuf class aims at describing a memory\nregion, regardless of whether it stores a single plane or multiple\nplanes.\n\nData offset isn't supported by V4L2 at the moment, but I think it should\nbe part of our multi-plane API. This is how DRM/KMS describes a\nframebuffer, and extending V4L2 to use a similar mechanism has been\nproposed (the most recent proposal being presented by Boris during the\nELCE V4L2 workshop this year).\n\nStride information may also belong to FrameBuffer::Plane. Both data\noffset and stride could be added on top of this patch series, but the\ndesign of the FrameBuffer::Plane and Dmabuf classes should already take\nthis into account in order to define the boundaries of the two classes\nclearly.\n\n> > +};\n> > +\n> > class Plane final\n> > {\n> > public:\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 0676586ae3be2a61..5516055b2ea885c2 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -177,6 +177,119 @@ FileDescriptor::~FileDescriptor()\n> > * an image may or may not be contiguous.\n> > */\n> >\n> > +/**\n> > + * \\class Dmabuf\n> > + * \\brief A memory region to store a single plane of a frame\n> > + *\n> > + * Planar pixel formats use multiple memory regions to store planes\n> > + * corresponding to the different colour components of a frame. The Dmabuf class\n> > + * tracks the specific details of a memory region used to store a single plane\n> > + * for a given frame and provides the means to access the memory, both for the\n> > + * application and for DMA. A Buffer then contains one or multiple planes\n> > + * depending on its pixel format.\n> \n> I'm not sure I get what \"access to memory both for the application and\n> for DMA\" means. I think you're referring to dmabuf here, but \"DMA\n> access\" is confusing to me (yes, the numerical id is used by drivers\n> to retrieve the memory location where to point their DMA engines to,\n> but the users of this class are applications which really should care\n> only about the fact that they have a mean to have a dmabuf-consumable\n> representation of the memory area they intend to share between\n> different subsystems).\n> \n> I would also drop from here any refernce to the image format and how\n> they map to one or multiple planes.\n> \n> > + *\n> > + * To support DMA access, planes are associated with dmabuf objects represented\n> \n> Here as well, what do you mean with \"DMA access\" ?\n> \n> > + * by file handles. Each plane carries a dmabuf file handle and an offset within\n> > + * the buffer. Those file handles may refer to the same dmabuf object, depending\n> > + * on whether the devices accessing the memory regions composing the image\n> > + * support non-contiguous DMA to planes ore require DMA-contiguous memory.\n> \n> I'm not following here either. We'll have one Dmabuf instance per\n> plane, right ? Each Dmabuf instance should have it's own\n> dmabuf-exported representation in form of a file handle and a CPU\n> accessible memory pointer where data for that plane are mapped to.\n> Why what the file handles refers to is relevant, and why the DMA engine\n> capabilities are of any interest for applications ?\n> \n> > + *\n> > + * To support CPU access, planes carry the CPU address of their backing memory.\n> > + * Similarly to the dmabuf file handles, the CPU addresses for planes composing\n> > + * an image may or may not be contiguous.\n> \n> I don't think it's relevant either.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\brief Set the dmabuf file handle backing the buffer\n> > + * \\param[in] fd The dmabuf file handle\n> > + * \\param[in] length The size of the memory region\n> > + *\n> > + * The \\a fd dmabuf file handle is duplicated and stored.\n> > + */\n> > +Dmabuf::Dmabuf(int fd, unsigned int length)\n> > +\t: fd_(fd), length_(length), mem_(nullptr)\n> > +{\n> > +}\n> > +\n> > +Dmabuf::~Dmabuf()\n> > +{\n> > +\tmunmap();\n> > +}\n> > +\n> > +/**\n> > + * \\fn Dmabuf::fd()\n> > + * \\brief Get the dmabuf file handle backing the buffer\n> > + */\n> > +\n> > +/**\n> > + * \\fn Dmabuf::length()\n> > + * \\brief Retrieve the length of the memory region\n> > + * \\return The length of the memory region\n> > + */\n> > +\n> > +/**\n> > + * \\fn Dmabuf::mem()\n> > + * \\brief Retrieve the CPU accessible memory address of the Dmabuf\n> > + * \\return The CPU accessible memory address on success or nullptr otherwise.\n> > + */\n> > +void *Dmabuf::mem()\n> > +{\n> > +\tif (!mem_)\n> > +\t\tmmap();\n> > +\n> > +\treturn mem_;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Map the plane memory data to a CPU accessible address\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int Dmabuf::mmap()\n> > +{\n> > +\tvoid *map;\n> > +\n> > +\tif (mem_)\n> > +\t\treturn 0;\n> > +\n> > +\tmap = ::mmap(NULL, length_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_.fd(), 0);\n\nThis doesn't need to be implemented now, but please already think about\nthe fact that we will likely need to support read-only mappings in the\nfuture. We may also need to support different caching attributes for\nmappings (although I'm not sure what kernel API would be used for this).\nCould you try to think how it could affect the Dmabuf class API, and\npossibly already take this into account in the design (without a full\nimplementation yet of course) ?\n\n> > +\tif (map == MAP_FAILED) {\n> > +\t\tint ret = -errno;\n> > +\t\tLOG(Buffer, Error)\n> > +\t\t\t<< \"Failed to mmap plane: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tmem_ = map;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Unmap any existing CPU accessible mapping\n> > + *\n> > + * Unmap the memory mapped by an earlier call to mmap().\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int Dmabuf::munmap()\n> > +{\n> > +\tint ret = 0;\n> > +\n> > +\tif (mem_)\n> > +\t\tret = ::munmap(mem_, length_);\n> > +\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Buffer, Warning)\n> > +\t\t\t<< \"Failed to unmap plane: \" << strerror(-ret);\n> > +\t} else {\n> > +\t\tmem_ = 0;\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> \n> Also, looking at how it is used, the dmabufs_ vector of a FrameBuffer\n> really only uses informations copied from planes_.\n> \n> I would be tempted to ask why application cannot access Plane\n> instances in a frame buffer and get their dmabuf and memory\n> representation from there, which would make this class not required,\n> but I might be missing something...\n\nAs far as I understand, the Dmabuf class is only used by IPAs and by\napplications, in order to map buffers to the CPU. The IPA use case is\ncertainly something we need to support, but towards applications, I\nwonder if it's really the duty of the FrameBuffer class to provide CPU\nmappings. When importing dmabufs (which is expected to be the main use\ncase going forward) applications should already have a CPU mapping, and\nwhen exporting dmabufs we could expose the CPU mappings in the\nBufferAllocator class.\n\n> > Plane::Plane()\n> > \t: fd_(-1), length_(0), mem_(0)\n> > {","headers":{"Return-Path":"","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 44FAF60BDB\n\tfor ;\n\tTue, 10 Dec 2019 02:09:20 +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 B9410727;\n\tTue, 10 Dec 2019 02:09:19 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575940159;\n\tbh=Gd2c5rN4aIEyIhRPN+9LCAIRgpZ/9t0+m8Y3F+c+smg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eh37+BvFTsak8GSTQCvkHeucSpNp+IDpmsPN/cwPI/j5WUoGGr8KtD1Aq8dKrhucy\n\tdrbs6BsY2d/ppkLxftSxs3WGDNo68Fs/43+ZV7/uS1BrEFzxOyVtJ7/lzh4g2D80yW\n\t0ahgyW2WVGVY/ex0Fg1Pf/4XqUWHId6Y6fVWbP1Y=","Date":"Tue, 10 Dec 2019 03:09:12 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191210010912.GK18060@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-6-niklas.soderlund@ragnatech.se>\n\t<20191127115534.szobyp3tnaurfyzs@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191127115534.szobyp3tnaurfyzs@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 05/30] libcamera: buffer: Add Dmabuf\n\tto describe a dma buffer","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, 10 Dec 2019 01:09:20 -0000"}}]