[{"id":11571,"web_url":"https://patchwork.libcamera.org/comment/11571/","msgid":"<20200724165840.GM5921@pendragon.ideasonboard.com>","date":"2020-07-24T16:58:40","subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Mon, Jul 20, 2020 at 11:42:25PM +0100, Kieran Bingham wrote:\n> Provide a MappedFrameBuffer helper class which will map\n> all of the Planes within a FrameBuffer and provide CPU addressable\n> pointers for those planes.\n> \n> Mappings are removed upon destruction.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> \n> This introduces an automatically mapping/unmapping MappedFrameBuffer\n> object, with a move constructor (essential to prevent un-desirable\n> unmapping).\n> \n> \n> \n> include/libcamera/buffer.h | 23 ++++++++++++++++++++\n\nShould this be an internal helper ? The main foreseen flow is for\napplications to create frame buffers from dmabuf fds they recently\noutside of libcamera, so I would expect them to handle memory mapping.\nI'd thus rather start with an internal helper, which we could graduate\nto a public API later if needed/desired.\n\n> src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 66 insertions(+)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 6bb2e4f8558f..881d736da2db 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -71,6 +71,29 @@ private:\n> \tunsigned int cookie_;\n> };\n> \n> +class MappedFrameBuffer {\n> +public:\n> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> +\t~MappedFrameBuffer();\n> +\n> +\t/* Move constructor only, copying is not permitted. */\n\nWe usually don't put comments in the header files. This should go to the\n\\class documentation block below.\n\n> +\tMappedFrameBuffer(MappedFrameBuffer &&rhs);\n\nDo you actually need this ? If it's only for the purpose of preventing\ncopy-construction, you can simply write\n\n\tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n\tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n\nThe C++ rules governing implicitly-defined and defaulted copy and move\nconstructors and assignment operators are not the easiest to follow.\nIt's usually best to be explicit. If you need move semantics,\n\n\tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n\tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n\n\tMappedFrameBuffer(MappedFrameBuffer &&other);\n\tMappedFrameBuffer operator=(MappedFrameBuffer &&other);\n\n(either neither or both of the move constructor and the move assignment\noperator should be defined) otherwise you can leave the latter two out.\n\n> +\n> +\tstruct MappedPlane {\n> +\t\tvoid *address;\n> +\t\tsize_t length;\n> +\t};\n\nWould it make sense to use Span to replace MappedPlane ?\n\n> +\n> +\tbool isValid() const { return valid_; }\n> +\tint error() const { return error_; }\n> +\tconst std::vector &maps() const { return maps_; }\n> +\n> +private:\n> +\tint error_;\n> +\tbool valid_;\n> +\tstd::vector maps_;\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_BUFFER_H__ */\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 1a1d4bac7aed..28b43d937f57 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -290,4 +290,47 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n> \treturn 0;\n> }\n> \n\nMissing\n\n/**\n * \\class MappedFrameBuffer\n * ...\n */\n\n> +/**\n> + * \\brief Map all planes of a FrameBuffer\n\nAs this is a constructor, I think it should be documented as \"Construct\n...\" to follow the usual pattern.\n\n> + * \\param[in] src Buffer to be mapped\n\ns/src/buffer/\n\n> + * \\param[in] flags Protection flags to apply to map\n> + *\n> + * Construct an object to map a frame buffer for CPU access.\n> + * The flags are passed directly to mmap and should be either PROT_READ,\n> + * PROT_WRITE, or a bitwise-or combination of both.\n> + */\n> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> +\t: error_(0)\n> +{\n> +\tmaps_.reserve(buffer->planes().size());\n> +\n> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> +\t\tvoid *address = mmap(nullptr, plane.length, flags,\n> +\t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n> +\n> +\t\tif (address == MAP_FAILED) {\n> +\t\t\terror_ = -errno;\n> +\t\t\tLOG(Buffer, Error) << \"Failed to mmap plane\";\n> +\t\t\tcontinue;\n\nShouldn't you break ?\n\n> +\t\t}\n> +\n> +\t\tmaps_.push_back({address, plane.length});\n> +\t}\n> +\n> +\tvalid_ = buffer->planes().size() == maps_.size();\n> +}\n> +\n> +MappedFrameBuffer::~MappedFrameBuffer()\n> +{\n> +\tfor (MappedPlane map : maps_)\n\n\tfor (MappedPlane &map : maps_)\n\n> +\t\tmunmap(map.address, map.length);\n> +}\n> +\n> +MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)\n> +{\n> +\terror_ = rhs.error_;\n> +\tvalid_ = rhs.valid_;\n> +\tmaps_ = std::move(rhs.maps_);\n\nThis is the default move constructor, so you could simply write\n\n\tMappedFrameBuffer(MappedFrameBuffer &&other) = default;\n\nin the class definition. However, I think you should keep this, and add\n\n\trhs.error_ = 0;\n\trhs.valid_ = false;\n\nNote that you can implement the move constructor in terms of the move\nassignment operator:\n\n\t*this = std::move(other);\n\n> +}\n> +\n> } /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 83C00BD86F\n\tfor ;\n\tFri, 24 Jul 2020 16:58:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E06DB61218;\n\tFri, 24 Jul 2020 18:58:48 +0200 (CEST)","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 76E06605B2\n\tfor ;\n\tFri, 24 Jul 2020 18:58:47 +0200 (CEST)","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 D211D538;\n\tFri, 24 Jul 2020 18:58:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BZ+r1B14\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595609927;\n\tbh=yfL2vKkXYnLQzKEuGibW7RO1JfaV4PyoUshJvRA7btI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BZ+r1B14I4QigkBH3eHjl17wFcgBhumwBmXsW1XBaoZwN5n665Y1QeCgEBkL78JMf\n\tKr1t7ObEjUvNRlQki3KxdokttHlVgE1amaMv/bP7RSZFHz6oxw2L2NPOWqCgbyZJST\n\tz6WqnhVMAhwrH8NmQrrvLx5HetPRv+IG0RNpkAsE=","Date":"Fri, 24 Jul 2020 19:58:40 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200724165840.GM5921@pendragon.ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200720224232.153717-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11712,"web_url":"https://patchwork.libcamera.org/comment/11712/","msgid":"","date":"2020-07-29T14:09:59","subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/07/2020 17:58, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Mon, Jul 20, 2020 at 11:42:25PM +0100, Kieran Bingham wrote:\n>> Provide a MappedFrameBuffer helper class which will map\n>> all of the Planes within a FrameBuffer and provide CPU addressable\n>> pointers for those planes.\n>>\n>> Mappings are removed upon destruction.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>>\n>> This introduces an automatically mapping/unmapping MappedFrameBuffer\n>> object, with a move constructor (essential to prevent un-desirable\n>> unmapping).\n>>\n>>\n>>\n>> include/libcamera/buffer.h | 23 ++++++++++++++++++++\n> \n> Should this be an internal helper ? The main foreseen flow is for\n> applications to create frame buffers from dmabuf fds they recently\n> outside of libcamera, so I would expect them to handle memory mapping.\n> I'd thus rather start with an internal helper, which we could graduate\n> to a public API later if needed/desired.\n\nI saw it as beneficial to be in the public-api, because FrameBuffer is\nin the public API.\n\nIf an application has created a FrameBuffer of their own, by the\ndefinition of it, it can be used to construct a MappedFrameBuffer.\n\nWe've already had a support-request in the past where someone had not\nmapped the FrameBuffer because it wasn't obvious, so I thought providing\ncommon helpers to map the Common FrameBuffer object was useful.\n\nI can move it to private API all the same though.\n\nAs you say, it could be made public later.\n\nThis implies that we would create\n\n\tinclude/libcamera/internal/buffer.h\nas well as\n\tinclude/libcamera/buffer.h\n\nWhere both would be implemented in:\n\n\tsrc/libcamera/buffer.cpp\n\nIs that acceptable? or should this become:\n\n\tinclude/libcamera/internal/mapped_buffer.h\n\tsrc/libcamera/mapped_buffer.cpp\n\n\n--\nKieran\n\n\n>> src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 66 insertions(+)\n>>\n>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>> index 6bb2e4f8558f..881d736da2db 100644\n>> --- a/include/libcamera/buffer.h\n>> +++ b/include/libcamera/buffer.h\n>> @@ -71,6 +71,29 @@ private:\n>> \tunsigned int cookie_;\n>> };\n>> \n>> +class MappedFrameBuffer {\n>> +public:\n>> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n>> +\t~MappedFrameBuffer();\n>> +\n>> +\t/* Move constructor only, copying is not permitted. */\n> \n> We usually don't put comments in the header files. This should go to the\n> \\class documentation block below.\n> \n>> +\tMappedFrameBuffer(MappedFrameBuffer &&rhs);\n> \n> Do you actually need this ? If it's only for the purpose of preventing\n> copy-construction, you can simply write\n\nIt was to self implement the Move constructor (and disable the copy\nconstructors yes).\n\nI think I had originally already put in the update to the rhs, which is\nwhy I implemented it rather than using the default, but yet that change\ndidn't seem to make it into this patch ... :S\n\n\n\n> \tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n> \tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n> \n> The C++ rules governing implicitly-defined and defaulted copy and move\n> constructors and assignment operators are not the easiest to follow.\n> It's usually best to be explicit. If you need move semantics,\n> \n> \tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n> \tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n> \n> \tMappedFrameBuffer(MappedFrameBuffer &&other);\n> \tMappedFrameBuffer operator=(MappedFrameBuffer &&other);\n> \n> (either neither or both of the move constructor and the move assignment\n> operator should be defined) otherwise you can leave the latter two out.\n> \n>> +\n>> +\tstruct MappedPlane {\n>> +\t\tvoid *address;\n>> +\t\tsize_t length;\n>> +\t};\n> \n> Would it make sense to use Span to replace MappedPlane ?\n\nAha, I like that.\n\nI like the type naming though, so should it be\n\nusing MappedPlane = Span;\n\nOr would you prefer to use the Span directly?\n\n> \n>> +\n>> +\tbool isValid() const { return valid_; }\n>> +\tint error() const { return error_; }\n>> +\tconst std::vector &maps() const { return maps_; }\n>> +\n>> +private:\n>> +\tint error_;\n>> +\tbool valid_;\n>> +\tstd::vector maps_;\n>> +};\n>> +\n>> } /* namespace libcamera */\n>> \n>> #endif /* __LIBCAMERA_BUFFER_H__ */\n>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n>> index 1a1d4bac7aed..28b43d937f57 100644\n>> --- a/src/libcamera/buffer.cpp\n>> +++ b/src/libcamera/buffer.cpp\n>> @@ -290,4 +290,47 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n>> \treturn 0;\n>> }\n>> \n> \n> Missing\n> \n> /**\n> * \\class MappedFrameBuffer\n> * ...\n> */\n\n\nSure.\n\n> \n>> +/**\n>> + * \\brief Map all planes of a FrameBuffer\n> \n> As this is a constructor, I think it should be documented as \"Construct\n> ...\" to follow the usual pattern.\n> \n>> + * \\param[in] src Buffer to be mapped\n> \n> s/src/buffer/\n> \n>> + * \\param[in] flags Protection flags to apply to map\n>> + *\n>> + * Construct an object to map a frame buffer for CPU access.\n>> + * The flags are passed directly to mmap and should be either PROT_READ,\n>> + * PROT_WRITE, or a bitwise-or combination of both.\n>> + */\n>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n>> +\t: error_(0)\n>> +{\n>> +\tmaps_.reserve(buffer->planes().size());\n>> +\n>> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n>> +\t\tvoid *address = mmap(nullptr, plane.length, flags,\n>> +\t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n>> +\n>> +\t\tif (address == MAP_FAILED) {\n>> +\t\t\terror_ = -errno;\n>> +\t\t\tLOG(Buffer, Error) << \"Failed to mmap plane\";\n>> +\t\t\tcontinue;\n> \n> Shouldn't you break ?\n\nIt will still fail the valid_ conditional below, but yes - I think break\nwould be better.\n\n\n> \n>> +\t\t}\n>> +\n>> +\t\tmaps_.push_back({address, plane.length});\n>> +\t}\n>> +\n>> +\tvalid_ = buffer->planes().size() == maps_.size();\n>> +}\n>> +\n>> +MappedFrameBuffer::~MappedFrameBuffer()\n>> +{\n>> +\tfor (MappedPlane map : maps_)\n> \n> \tfor (MappedPlane &map : maps_)\n\nGood spot.\n\n\n\n>> +\t\tmunmap(map.address, map.length);\n>> +}\n>> +\n>> +MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)\n>> +{\n>> +\terror_ = rhs.error_;\n>> +\tvalid_ = rhs.valid_;\n>> +\tmaps_ = std::move(rhs.maps_);\n> \n> This is the default move constructor, so you could simply write\n> \n> \tMappedFrameBuffer(MappedFrameBuffer &&other) = default;\n> \n> in the class definition. However, I think you should keep this, and add\n> \n> \trhs.error_ = 0;\n> \trhs.valid_ = false;\n> \n> Note that you can implement the move constructor in terms of the move\n> assignment operator:\n> \n> \t*this = std::move(other);\n\n\nOk, so I think we need to keep this and have it as:\n\nMappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)\n{\n\t*this = std::move(other);\n \trhs.error_ = 0;\n \trhs.valid_ = false;\n}\n\nMappedFrameBuffer operator=(MappedFrameBuffer &&rhs)\n{\n\t*this = std::move(other);\n \trhs.error_ = 0;\n \trhs.valid_ = false;\n}\n\nI was sure I already had the resetting of rhs, but I guess I dropped it\nsomewhere.\n\nIt's a shame to have to duplicate for the assignment operator, but I\ndon't think that's too bad.\n\n--\nKieran\n\n\n\n> \n>> +}\n>> +\n>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D18DDBD86F\n\tfor ;\n\tWed, 29 Jul 2020 14:10:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50699617AF;\n\tWed, 29 Jul 2020 16:10:05 +0200 (CEST)","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 BA2626039D\n\tfor ;\n\tWed, 29 Jul 2020 16:10:03 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40B8531F;\n\tWed, 29 Jul 2020 16:10:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UcQOWRWx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596031803;\n\tbh=Il6lOQsCsEsyns72hl0WT1stFGf9mMx+xR0YanQC4Xg=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=UcQOWRWxV5bLr/hoLq+FbrDwOn5zbH/lnn+DIaW1IXg1ZUmwtqMUxagDNbVb1VtyN\n\tY4tYKDO9ShomDoroP6Xu9C/je72Rnw7KFdRuMJUJsvQ8gdT+pNS7QOVgVqxKcspBsh\n\tKZoeuieZN3VmpWnQIV7OfGDAx3U66hWZAKhvZ13Y=","To":"Laurent Pinchart ","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-2-kieran.bingham@ideasonboard.com>\n\t<20200724165840.GM5921@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 29 Jul 2020 15:09:59 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200724165840.GM5921@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11713,"web_url":"https://patchwork.libcamera.org/comment/11713/","msgid":"<20200729141842.GC6183@pendragon.ideasonboard.com>","date":"2020-07-29T14:18:42","subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 29, 2020 at 03:09:59PM +0100, Kieran Bingham wrote:\n> On 24/07/2020 17:58, Laurent Pinchart wrote:\n> > On Mon, Jul 20, 2020 at 11:42:25PM +0100, Kieran Bingham wrote:\n> >> Provide a MappedFrameBuffer helper class which will map\n> >> all of the Planes within a FrameBuffer and provide CPU addressable\n> >> pointers for those planes.\n> >>\n> >> Mappings are removed upon destruction.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >>\n> >> This introduces an automatically mapping/unmapping MappedFrameBuffer\n> >> object, with a move constructor (essential to prevent un-desirable\n> >> unmapping).\n> >>\n> >> include/libcamera/buffer.h | 23 ++++++++++++++++++++\n> > \n> > Should this be an internal helper ? The main foreseen flow is for\n> > applications to create frame buffers from dmabuf fds they recently\n> > outside of libcamera, so I would expect them to handle memory mapping.\n> > I'd thus rather start with an internal helper, which we could graduate\n> > to a public API later if needed/desired.\n> \n> I saw it as beneficial to be in the public-api, because FrameBuffer is\n> in the public API.\n> \n> If an application has created a FrameBuffer of their own, by the\n> definition of it, it can be used to construct a MappedFrameBuffer.\n> \n> We've already had a support-request in the past where someone had not\n> mapped the FrameBuffer because it wasn't obvious, so I thought providing\n> common helpers to map the Common FrameBuffer object was useful.\n> \n> I can move it to private API all the same though.\n> \n> As you say, it could be made public later.\n> \n> This implies that we would create\n> \n> \tinclude/libcamera/internal/buffer.h\n> as well as\n> \tinclude/libcamera/buffer.h\n> \n> Where both would be implemented in:\n> \n> \tsrc/libcamera/buffer.cpp\n> \n> Is that acceptable? or should this become:\n> \n> \tinclude/libcamera/internal/mapped_buffer.h\n> \tsrc/libcamera/mapped_buffer.cpp\n\nBoth options are fine with me.\n\n> >> src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++\n> >> 2 files changed, 66 insertions(+)\n> >>\n> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> >> index 6bb2e4f8558f..881d736da2db 100644\n> >> --- a/include/libcamera/buffer.h\n> >> +++ b/include/libcamera/buffer.h\n> >> @@ -71,6 +71,29 @@ private:\n> >> \tunsigned int cookie_;\n> >> };\n> >> \n> >> +class MappedFrameBuffer {\n> >> +public:\n> >> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> >> +\t~MappedFrameBuffer();\n> >> +\n> >> +\t/* Move constructor only, copying is not permitted. */\n> > \n> > We usually don't put comments in the header files. This should go to the\n> > \\class documentation block below.\n> > \n> >> +\tMappedFrameBuffer(MappedFrameBuffer &&rhs);\n> > \n> > Do you actually need this ? If it's only for the purpose of preventing\n> > copy-construction, you can simply write\n> \n> It was to self implement the Move constructor (and disable the copy\n> constructors yes).\n> \n> I think I had originally already put in the update to the rhs, which is\n> why I implemented it rather than using the default, but yet that change\n> didn't seem to make it into this patch ... :S\n> \n> > \tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n> > \tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n> > \n> > The C++ rules governing implicitly-defined and defaulted copy and move\n> > constructors and assignment operators are not the easiest to follow.\n> > It's usually best to be explicit. If you need move semantics,\n> > \n> > \tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n> > \tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n> > \n> > \tMappedFrameBuffer(MappedFrameBuffer &&other);\n> > \tMappedFrameBuffer operator=(MappedFrameBuffer &&other);\n> > \n> > (either neither or both of the move constructor and the move assignment\n> > operator should be defined) otherwise you can leave the latter two out.\n> > \n> >> +\n> >> +\tstruct MappedPlane {\n> >> +\t\tvoid *address;\n> >> +\t\tsize_t length;\n> >> +\t};\n> > \n> > Would it make sense to use Span to replace MappedPlane ?\n> \n> Aha, I like that.\n> \n> I like the type naming though, so should it be\n> \n> using MappedPlane = Span;\n> \n> Or would you prefer to use the Span directly?\n\nI don't mind much. Aliases require looking up what they correspond to,\nbut it can also clarify the code as the name better matches the purpose.\nUp to you.\n\n> >> +\n> >> +\tbool isValid() const { return valid_; }\n> >> +\tint error() const { return error_; }\n> >> +\tconst std::vector &maps() const { return maps_; }\n> >> +\n> >> +private:\n> >> +\tint error_;\n> >> +\tbool valid_;\n> >> +\tstd::vector maps_;\n> >> +};\n> >> +\n> >> } /* namespace libcamera */\n> >> \n> >> #endif /* __LIBCAMERA_BUFFER_H__ */\n> >> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> >> index 1a1d4bac7aed..28b43d937f57 100644\n> >> --- a/src/libcamera/buffer.cpp\n> >> +++ b/src/libcamera/buffer.cpp\n> >> @@ -290,4 +290,47 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n> >> \treturn 0;\n> >> }\n> > \n> > Missing\n> > \n> > /**\n> > * \\class MappedFrameBuffer\n> > * ...\n> > */\n> \n> Sure.\n> \n> >> +/**\n> >> + * \\brief Map all planes of a FrameBuffer\n> > \n> > As this is a constructor, I think it should be documented as \"Construct\n> > ...\" to follow the usual pattern.\n> > \n> >> + * \\param[in] src Buffer to be mapped\n> > \n> > s/src/buffer/\n> > \n> >> + * \\param[in] flags Protection flags to apply to map\n> >> + *\n> >> + * Construct an object to map a frame buffer for CPU access.\n> >> + * The flags are passed directly to mmap and should be either PROT_READ,\n> >> + * PROT_WRITE, or a bitwise-or combination of both.\n> >> + */\n> >> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> >> +\t: error_(0)\n> >> +{\n> >> +\tmaps_.reserve(buffer->planes().size());\n> >> +\n> >> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> >> +\t\tvoid *address = mmap(nullptr, plane.length, flags,\n> >> +\t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n> >> +\n> >> +\t\tif (address == MAP_FAILED) {\n> >> +\t\t\terror_ = -errno;\n> >> +\t\t\tLOG(Buffer, Error) << \"Failed to mmap plane\";\n> >> +\t\t\tcontinue;\n> > \n> > Shouldn't you break ?\n> \n> It will still fail the valid_ conditional below, but yes - I think break\n> would be better.\n> \n> >> +\t\t}\n> >> +\n> >> +\t\tmaps_.push_back({address, plane.length});\n> >> +\t}\n> >> +\n> >> +\tvalid_ = buffer->planes().size() == maps_.size();\n> >> +}\n> >> +\n> >> +MappedFrameBuffer::~MappedFrameBuffer()\n> >> +{\n> >> +\tfor (MappedPlane map : maps_)\n> > \n> > \tfor (MappedPlane &map : maps_)\n> \n> Good spot.\n> \n> >> +\t\tmunmap(map.address, map.length);\n> >> +}\n> >> +\n> >> +MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)\n> >> +{\n> >> +\terror_ = rhs.error_;\n> >> +\tvalid_ = rhs.valid_;\n> >> +\tmaps_ = std::move(rhs.maps_);\n> > \n> > This is the default move constructor, so you could simply write\n> > \n> > \tMappedFrameBuffer(MappedFrameBuffer &&other) = default;\n> > \n> > in the class definition. However, I think you should keep this, and add\n> > \n> > \trhs.error_ = 0;\n> > \trhs.valid_ = false;\n> > \n> > Note that you can implement the move constructor in terms of the move\n> > assignment operator:\n> > \n> > \t*this = std::move(other);\n> \n> Ok, so I think we need to keep this and have it as:\n> \n> MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)\n> {\n> \t*this = std::move(other);\n> \trhs.error_ = 0;\n> \trhs.valid_ = false;\n> }\n> \n> MappedFrameBuffer operator=(MappedFrameBuffer &&rhs)\n> {\n> \t*this = std::move(other);\n\nWon't this cause a recursive call to the move assignement operator ?\n\n> \trhs.error_ = 0;\n> \trhs.valid_ = false;\n> }\n> \n> I was sure I already had the resetting of rhs, but I guess I dropped it\n> somewhere.\n> \n> It's a shame to have to duplicate for the assignment operator, but I\n> don't think that's too bad.\n> \n> >> +}\n> >> +\n> >> } /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 63727BD878\n\tfor ;\n\tWed, 29 Jul 2020 14:18:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCBB9613C6;\n\tWed, 29 Jul 2020 16:18:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9679A6039D\n\tfor ;\n\tWed, 29 Jul 2020 16:18:51 +0200 (CEST)","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 1838331F;\n\tWed, 29 Jul 2020 16:18:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"T5obmxvN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596032331;\n\tbh=I5pw5VuZuJcl33O6SEyV085SVA/PSJ+6J3ohqb1q9os=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T5obmxvN9lDSN0b0omXgog2MnWFgssZhgGRpxQQ9I62xrOn2Hwasfy5ysFl0NYx0N\n\tzsMQNCTk3MltABQ/mUTx8XaSA1WS9hOCPlySb528/M8nFiqnrbnLisND3wyjVncWDP\n\tCdIM65IcvXZXIrWVvqCHgyRhjpyDqLFk7e1wkgQY=","Date":"Wed, 29 Jul 2020 17:18:42 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200729141842.GC6183@pendragon.ideasonboard.com>","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-2-kieran.bingham@ideasonboard.com>\n\t<20200724165840.GM5921@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11716,"web_url":"https://patchwork.libcamera.org/comment/11716/","msgid":"<4505240e-db28-6257-0f23-52a2fbdac4fd@ideasonboard.com>","date":"2020-07-29T14:30:54","subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 29/07/2020 15:18, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Jul 29, 2020 at 03:09:59PM +0100, Kieran Bingham wrote:\n>> On 24/07/2020 17:58, Laurent Pinchart wrote:\n>>> On Mon, Jul 20, 2020 at 11:42:25PM +0100, Kieran Bingham wrote:\n>>>> Provide a MappedFrameBuffer helper class which will map\n>>>> all of the Planes within a FrameBuffer and provide CPU addressable\n>>>> pointers for those planes.\n>>>>\n>>>> Mappings are removed upon destruction.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>>\n>>>> This introduces an automatically mapping/unmapping MappedFrameBuffer\n>>>> object, with a move constructor (essential to prevent un-desirable\n>>>> unmapping).\n>>>>\n>>>> include/libcamera/buffer.h | 23 ++++++++++++++++++++\n>>>\n>>> Should this be an internal helper ? The main foreseen flow is for\n>>> applications to create frame buffers from dmabuf fds they recently\n>>> outside of libcamera, so I would expect them to handle memory mapping.\n>>> I'd thus rather start with an internal helper, which we could graduate\n>>> to a public API later if needed/desired.\n>>\n>> I saw it as beneficial to be in the public-api, because FrameBuffer is\n>> in the public API.\n>>\n>> If an application has created a FrameBuffer of their own, by the\n>> definition of it, it can be used to construct a MappedFrameBuffer.\n>>\n>> We've already had a support-request in the past where someone had not\n>> mapped the FrameBuffer because it wasn't obvious, so I thought providing\n>> common helpers to map the Common FrameBuffer object was useful.\n>>\n>> I can move it to private API all the same though.\n>>\n>> As you say, it could be made public later.\n>>\n>> This implies that we would create\n>>\n>> \tinclude/libcamera/internal/buffer.h\n>> as well as\n>> \tinclude/libcamera/buffer.h\n>>\n>> Where both would be implemented in:\n>>\n>> \tsrc/libcamera/buffer.cpp\n>>\n>> Is that acceptable? or should this become:\n>>\n>> \tinclude/libcamera/internal/mapped_buffer.h\n>> \tsrc/libcamera/mapped_buffer.cpp\n> \n> Both options are fine with me.\n\nI'll take the easy option, otherwise I'll convince myself buffer.cpp\nwill need a whole bunch of refactoring and renaming.\n\n\n> \n>>>> src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++\n>>>> 2 files changed, 66 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>>>> index 6bb2e4f8558f..881d736da2db 100644\n>>>> --- a/include/libcamera/buffer.h\n>>>> +++ b/include/libcamera/buffer.h\n>>>> @@ -71,6 +71,29 @@ private:\n>>>> \tunsigned int cookie_;\n>>>> };\n>>>> \n>>>> +class MappedFrameBuffer {\n>>>> +public:\n>>>> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n>>>> +\t~MappedFrameBuffer();\n>>>> +\n>>>> +\t/* Move constructor only, copying is not permitted. */\n>>>\n>>> We usually don't put comments in the header files. This should go to the\n>>> \\class documentation block below.\n>>>\n>>>> +\tMappedFrameBuffer(MappedFrameBuffer &&rhs);\n>>>\n>>> Do you actually need this ? If it's only for the purpose of preventing\n>>> copy-construction, you can simply write\n>>\n>> It was to self implement the Move constructor (and disable the copy\n>> constructors yes).\n>>\n>> I think I had originally already put in the update to the rhs, which is\n>> why I implemented it rather than using the default, but yet that change\n>> didn't seem to make it into this patch ... :S\n>>\n>>> \tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n>>> \tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n>>>\n>>> The C++ rules governing implicitly-defined and defaulted copy and move\n>>> constructors and assignment operators are not the easiest to follow.\n>>> It's usually best to be explicit. If you need move semantics,\n>>>\n>>> \tMappedFrameBuffer(const MappedFrameBuffer &other) = delete;\n>>> \tMappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;\n>>>\n>>> \tMappedFrameBuffer(MappedFrameBuffer &&other);\n>>> \tMappedFrameBuffer operator=(MappedFrameBuffer &&other);\n>>>\n>>> (either neither or both of the move constructor and the move assignment\n>>> operator should be defined) otherwise you can leave the latter two out.\n>>>\n>>>> +\n>>>> +\tstruct MappedPlane {\n>>>> +\t\tvoid *address;\n>>>> +\t\tsize_t length;\n>>>> +\t};\n>>>\n>>> Would it make sense to use Span to replace MappedPlane ?\n>>\n>> Aha, I like that.\n>>\n>> I like the type naming though, so should it be\n>>\n>> using MappedPlane = Span;\n>>\n>> Or would you prefer to use the Span directly?\n> \n> I don't mind much. Aliases require looking up what they correspond to,\n> but it can also clarify the code as the name better matches the purpose.\n> Up to you.\n> \n>>>> +\n>>>> +\tbool isValid() const { return valid_; }\n>>>> +\tint error() const { return error_; }\n>>>> +\tconst std::vector &maps() const { return maps_; }\n>>>> +\n>>>> +private:\n>>>> +\tint error_;\n>>>> +\tbool valid_;\n>>>> +\tstd::vector maps_;\n>>>> +};\n>>>> +\n>>>> } /* namespace libcamera */\n>>>> \n>>>> #endif /* __LIBCAMERA_BUFFER_H__ */\n>>>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n>>>> index 1a1d4bac7aed..28b43d937f57 100644\n>>>> --- a/src/libcamera/buffer.cpp\n>>>> +++ b/src/libcamera/buffer.cpp\n>>>> @@ -290,4 +290,47 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n>>>> \treturn 0;\n>>>> }\n>>>\n>>> Missing\n>>>\n>>> /**\n>>> * \\class MappedFrameBuffer\n>>> * ...\n>>> */\n>>\n>> Sure.\n>>\n>>>> +/**\n>>>> + * \\brief Map all planes of a FrameBuffer\n>>>\n>>> As this is a constructor, I think it should be documented as \"Construct\n>>> ...\" to follow the usual pattern.\n>>>\n>>>> + * \\param[in] src Buffer to be mapped\n>>>\n>>> s/src/buffer/\n>>>\n>>>> + * \\param[in] flags Protection flags to apply to map\n>>>> + *\n>>>> + * Construct an object to map a frame buffer for CPU access.\n>>>> + * The flags are passed directly to mmap and should be either PROT_READ,\n>>>> + * PROT_WRITE, or a bitwise-or combination of both.\n>>>> + */\n>>>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n>>>> +\t: error_(0)\n>>>> +{\n>>>> +\tmaps_.reserve(buffer->planes().size());\n>>>> +\n>>>> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n>>>> +\t\tvoid *address = mmap(nullptr, plane.length, flags,\n>>>> +\t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n>>>> +\n>>>> +\t\tif (address == MAP_FAILED) {\n>>>> +\t\t\terror_ = -errno;\n>>>> +\t\t\tLOG(Buffer, Error) << \"Failed to mmap plane\";\n>>>> +\t\t\tcontinue;\n>>>\n>>> Shouldn't you break ?\n>>\n>> It will still fail the valid_ conditional below, but yes - I think break\n>> would be better.\n>>\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tmaps_.push_back({address, plane.length});\n>>>> +\t}\n>>>> +\n>>>> +\tvalid_ = buffer->planes().size() == maps_.size();\n>>>> +}\n>>>> +\n>>>> +MappedFrameBuffer::~MappedFrameBuffer()\n>>>> +{\n>>>> +\tfor (MappedPlane map : maps_)\n>>>\n>>> \tfor (MappedPlane &map : maps_)\n>>\n>> Good spot.\n>>\n>>>> +\t\tmunmap(map.address, map.length);\n>>>> +}\n>>>> +\n>>>> +MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)\n>>>> +{\n>>>> +\terror_ = rhs.error_;\n>>>> +\tvalid_ = rhs.valid_;\n>>>> +\tmaps_ = std::move(rhs.maps_);\n>>>\n>>> This is the default move constructor, so you could simply write\n>>>\n>>> \tMappedFrameBuffer(MappedFrameBuffer &&other) = default;\n>>>\n>>> in the class definition. However, I think you should keep this, and add\n>>>\n>>> \trhs.error_ = 0;\n>>> \trhs.valid_ = false;\n>>>\n>>> Note that you can implement the move constructor in terms of the move\n>>> assignment operator:\n>>>\n>>> \t*this = std::move(other);\n>>\n>> Ok, so I think we need to keep this and have it as:\n>>\n>> MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)\n>> {\n>> \t*this = std::move(other);\n>> \trhs.error_ = 0;\n>> \trhs.valid_ = false;\n>> }\n>>\n>> MappedFrameBuffer operator=(MappedFrameBuffer &&rhs)\n>> {\n>> \t*this = std::move(other);\n> \n> Won't this cause a recursive call to the move assignement operator ?\n\n\nHaha, maybe, I'd have discovered that when I actually got round to\ncompiling ;-)\n\nI see now that you mean the 'move constructor' should just call the move\nassignement operator ;-)\n\n\n\nThanks,\n\n\n\n> \n>> \trhs.error_ = 0;\n>> \trhs.valid_ = false;\n>> }\n>>\n>> I was sure I already had the resetting of rhs, but I guess I dropped it\n>> somewhere.\n>>\n>> It's a shame to have to duplicate for the assignment operator, but I\n>> don't think that's too bad.\n>>\n>>>> +}\n>>>> +\n>>>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9E663BD878\n\tfor ;\n\tWed, 29 Jul 2020 14:30:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08681617AF;\n\tWed, 29 Jul 2020 16:30:59 +0200 (CEST)","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 8A7D96039D\n\tfor ;\n\tWed, 29 Jul 2020 16:30:57 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EEEF131F;\n\tWed, 29 Jul 2020 16:30:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vvE5lHlZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596033057;\n\tbh=gJ4PwrU8n/1YYsSs3SkaVxUaheKz8ptOgwTA4gGojD4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=vvE5lHlZ9LOelibZLiyJ2CjSVdQPmAPYHptBIupBwhRxo29faBuaX0Usp1hsbuGlU\n\tWUBeIPdjqmgSQJvWR7MtQCdCM7njokOkVxdZIyk9N325+9WG4VnLtme4M2BUTaSfpw\n\tCD78VIi3H73LcD3n7oheZtMf+AF98qeuBDWJ8oyI=","To":"Laurent Pinchart ","References":"<20200720224232.153717-1-kieran.bingham@ideasonboard.com>\n\t<20200720224232.153717-2-kieran.bingham@ideasonboard.com>\n\t<20200724165840.GM5921@pendragon.ideasonboard.com>\n\t\n\t<20200729141842.GC6183@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<4505240e-db28-6257-0f23-52a2fbdac4fd@ideasonboard.com>","Date":"Wed, 29 Jul 2020 15:30:54 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200729141842.GC6183@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a\n\tMappedFrameBuffer","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]