[{"id":11851,"web_url":"https://patchwork.libcamera.org/comment/11851/","msgid":"<97706fe0-f83d-bed9-2b82-87821e6b6577@ideasonboard.com>","date":"2020-08-04T22:17:01","subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 04/08/2020 22:47, 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> The MappedFrameBuffer implements the interface of the MappedBuffer\n> allowing other buffer types to be constructed of the same form, with a\n> common interface and cleanup.\n> \n> This allows MappedBuffer instances to be created from Camera3Buffer types.\n> \n> Mappings are removed upon destruction.\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> v3\n> - Documentation fixups\n> ---\n> include/libcamera/internal/buffer.h | 47 ++++++++\n> src/libcamera/buffer.cpp | 162 +++++++++++++++++++++++++++-\n> 2 files changed, 208 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/internal/buffer.h\n> \n> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h\n> new file mode 100644\n> index 000000000000..e86a003cd8ba\n> --- /dev/null\n> +++ b/include/libcamera/internal/buffer.h\n> @@ -0,0 +1,47 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * buffer.h - Internal buffer handling\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__\n> +#define __LIBCAMERA_INTERNAL_BUFFER_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +using MappedPlane = Span;\n> +\n> +class MappedBuffer\n> +{\n> +public:\n> +\tMappedBuffer();\n> +\t~MappedBuffer();\n> +\n> +\tMappedBuffer(MappedBuffer &&rhs);\n> +\tMappedBuffer &operator=(MappedBuffer &&rhs);\n> +\n> +\tbool isValid() const { return valid_; }\n> +\tint error() const { return error_; }\n> +\tconst std::vector &maps() const { return maps_; }\n> +\n> +protected:\n> +\tint error_;\n> +\tbool valid_;\n> +\tstd::vector maps_;\n> +};\n> +\n> +class MappedFrameBuffer : public MappedBuffer\n> +{\n> +public:\n> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 8278f8a92af4..5f7dff60a48b 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \n> +#include \"libcamera/internal/buffer.h\"\n> \n> #include \n> #include \n> @@ -15,7 +16,8 @@\n> #include \"libcamera/internal/log.h\"\n> \n> /**\n> - * \\file buffer.h\n> + * \\file libcamera/buffer.h\n> + * \\file libcamera/internal/buffer.h\n> * \\brief Buffer handling\n> */\n> \n> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\class MappedBuffer\n> + * \\brief Provide an interface to support managing memory mapped buffers\n> + *\n> + * The MappedBuffer interface provides access to a set of MappedPlanes which\n> + * are available for access by a CPU.\n> + *\n> + * The MappedBuffer interface does not implement any valid constructor but\n> + * defines the move operators and destructors for the derived implementations,\n> + * which are able to construct according to their derived types and given\n> + * flags.\n> + *\n> + * This allows treating CPU accessible memory through a generic interface\n> + * regardless of whether it originates from a libcamera FrameBuffer or other\n> + * source.\n> + */\n> +\n> +/**\n> + * \\brief Construct an empty MappedBuffer\n> + *\n> + * A default constructor is required to allow subclassing the MappedBuffer\n> + * class. Construct an initialised, but invalid MappedBuffer.\n> + */\n> +MappedBuffer::MappedBuffer()\n> +\t: error_(0), valid_(false)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a MappedBuffer by taking the \\a rhs mappings\n> + * \\param[in] rhs The other MappedBuffer\n> + *\n> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n> + * destroyed in this process.\n> + */\n> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)\n> +{\n> +\t*this = std::move(rhs);\n> +}\n> +\n> +/**\n> + * \\brief Move assingment operator, move a MappedBuffer by taking the \\a rhs mappings\n> + * \\param[in] rhs The other MappedBuffer\n> + *\n> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n> + * destroyed in this process.\n> + */\n> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)\n> +{\n> +\terror_ = rhs.error_;\n> +\tvalid_ = rhs.valid_;\n> +\tmaps_ = std::move(rhs.maps_);\n> +\trhs.valid_ = false;\n> +\trhs.error_ = 0;\n> +\n> +\treturn *this;\n> +}\n> +\n> +MappedBuffer::~MappedBuffer()\n> +{\n> +\tfor (MappedPlane map : maps_)\n\nThis should be:\n\tfor (MappedPlane &map : maps_)\n\nAs has already been highlighted in an earlier review.\n\nNow updated locally.\n\n\n\n> +\t\tmunmap(map.data(), map.size());\n> +}\n> +\n> +/**\n> + * \\fn MappedBuffer::isValid()\n> + * \\brief Check if the MappedBuffer instance is valid\n> + * \\return True if the MappedBuffer has valid mappings, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn MappedBuffer::error()\n> + * \\brief Retrieve the map error status\n> + *\n> + * This function retrieves the error status from the MappedBuffer.\n> + * The error status is a negative number as defined by errno.h. If\n> + * no error occurred, this function returns 0.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn MappedBuffer::maps()\n> + * \\brief Retrieve the mapped planes\n> + *\n> + * This function retrieves the successfully mapped planes stored as a vector\n> + * of Span to provide access to the mapped memory.\n> + *\n> + * \\return A vector of the mapped planes.\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::valid_\n> + * \\brief Stores the status of the mapping\n> + *\n> + * MappedBuffer implementations shall set this to represent if the mapping\n> + * was successfully completed without errors.\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::error_\n> + * \\brief Stores the error value if present\n> + *\n> + * MappedBuffer implementations shall set this to a negative value as defined\n> + * by errno.h if an error occured during the mapping process\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::maps_\n> + * \\brief Stores the internal\n> + *\n> + * MappedBuffer implementations shall store the mappings they create in this\n> + * vector which is parsed during destruct to unmap any memory mappings which\n> + * completed successfully.\n> + */\n> +\n> +/**\n> + * \\class MappedFrameBuffer\n> + * \\brief Maps a FrameBuffer using the MappedBuffer interface\n> + *\n> + * The MappedFrameBuffer interface maps a FrameBuffer instance\n> + *\n> + * The MappedBuffer interface does not implement any constructor but defines\n> + * the move operators and destructors for the derived implementations, which\n> + * are able to construct according to their derived types and given flags.\n> + */\n> +\n> +/**\n> + * \\brief Map all planes of a FrameBuffer\n> + * \\param[in] buffer FrameBuffer to be mapped\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> +{\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\tbreak;\n> +\t\t}\n> +\n> +\t\tmaps_.emplace_back(static_cast(address), plane.length);\n> +\t}\n> +\n> +\tvalid_ = buffer->planes().size() == maps_.size();\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 29E07BD86F\n\tfor ;\n\tTue, 4 Aug 2020 22:17:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2B5C6054A;\n\tWed, 5 Aug 2020 00:17: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 7379360392\n\tfor ;\n\tWed, 5 Aug 2020 00:17:04 +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 DD3E327B;\n\tWed, 5 Aug 2020 00:17:03 +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=\"UFxk+4b9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596579424;\n\tbh=Xk6llq9rVtnK9cQpk204nQ/pfkAbnpDezCsZSsl8OOA=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=UFxk+4b9T95U6JqprBRsE9sskmCobfyNXewmNk1ESImrlNj4VE7IDS564s+aS/PMY\n\tQAwq3Hid3jHx+2rZlS4/n0OdbMF/CWij2BVHN6cFmkSO97aJGgmWk+fgdGPSSFLh+L\n\t70zdeD2Wg9gOzxUyCIC792eyQ/JtJieY3ZHl5w1I=","To":"Kieran Bingham ,\n\tLibCamera Devel ","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-4-kieran.bingham@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":"<97706fe0-f83d-bed9-2b82-87821e6b6577@ideasonboard.com>","Date":"Tue, 4 Aug 2020 23:17:01 +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":"<20200804214711.177645-4-kieran.bingham@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11855,"web_url":"https://patchwork.libcamera.org/comment/11855/","msgid":"<20200805002904.GV6075@pendragon.ideasonboard.com>","date":"2020-08-05T00:29:04","subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","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 Tue, Aug 04, 2020 at 10:47:01PM +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> The MappedFrameBuffer implements the interface of the MappedBuffer\n> allowing other buffer types to be constructed of the same form, with a\n> common interface and cleanup.\n> \n> This allows MappedBuffer instances to be created from Camera3Buffer types.\n> \n> Mappings are removed upon destruction.\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> v3\n> - Documentation fixups\n> ---\n> include/libcamera/internal/buffer.h | 47 ++++++++\n> src/libcamera/buffer.cpp | 162 +++++++++++++++++++++++++++-\n> 2 files changed, 208 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/internal/buffer.h\n> \n> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h\n> new file mode 100644\n> index 000000000000..e86a003cd8ba\n> --- /dev/null\n> +++ b/include/libcamera/internal/buffer.h\n> @@ -0,0 +1,47 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * buffer.h - Internal buffer handling\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__\n> +#define __LIBCAMERA_INTERNAL_BUFFER_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +using MappedPlane = Span;\n> +\n> +class MappedBuffer\n> +{\n> +public:\n\nHow about moving MappedPlane here, and naming it Plane ?\n\n\tusing Plane = Span;\n\nThis makes it clear that Plane is meant to be used with MappedBuffer.\n\n> +\tMappedBuffer();\n\nDo you want to allow construction of the base class, or only of derived\nclasses ? In the latter case, you should move this constructor to the\nprotected section below.\n\n> +\t~MappedBuffer();\n> +\n> +\tMappedBuffer(MappedBuffer &&rhs);\n> +\tMappedBuffer &operator=(MappedBuffer &&rhs);\n\nThe parameter is customarily called other for the copy and move\nconstructors and assignment operators. rhs is usually used in\nconjunction with lhs for non-member operators.\n\n> +\n> +\tbool isValid() const { return valid_; }\n> +\tint error() const { return error_; }\n> +\tconst std::vector &maps() const { return maps_; }\n> +\n> +protected:\n> +\tint error_;\n> +\tbool valid_;\n\nDo you need both ? Could isValid() return error_ == 0 ?\n\n> +\tstd::vector maps_;\n> +};\n> +\n> +class MappedFrameBuffer : public MappedBuffer\n> +{\n> +public:\n> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 8278f8a92af4..5f7dff60a48b 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \n> +#include \"libcamera/internal/buffer.h\"\n> \n> #include \n> #include \n> @@ -15,7 +16,8 @@\n> #include \"libcamera/internal/log.h\"\n> \n> /**\n> - * \\file buffer.h\n> + * \\file libcamera/buffer.h\n> + * \\file libcamera/internal/buffer.h\n> * \\brief Buffer handling\n> */\n> \n> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\class MappedBuffer\n> + * \\brief Provide an interface to support managing memory mapped buffers\n> + *\n> + * The MappedBuffer interface provides access to a set of MappedPlanes which\n> + * are available for access by a CPU.\n\ns/a CPU/the CPU/ ?\n\nYou're clearly thinking as a kernel developer :-)\n\n> + *\n> + * The MappedBuffer interface does not implement any valid constructor but\n> + * defines the move operators and destructors for the derived implementations,\n> + * which are able to construct according to their derived types and given\n> + * flags.\n\nThat's a bit of implementation details. Maybe \"This class is not meant\nto be constructed directly, and thus has no public default constructor.\nDerived classes shall be used instead.\" ?\n\n> + *\n> + * This allows treating CPU accessible memory through a generic interface\n> + * regardless of whether it originates from a libcamera FrameBuffer or other\n> + * source.\n> + */\n> +\n> +/**\n> + * \\brief Construct an empty MappedBuffer\n> + *\n> + * A default constructor is required to allow subclassing the MappedBuffer\n> + * class. Construct an initialised, but invalid MappedBuffer.\n\nYou can drop the first sentence, you're explaining C++ :-)\n\n> + */\n> +MappedBuffer::MappedBuffer()\n> +\t: error_(0), valid_(false)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a MappedBuffer by taking the \\a rhs mappings\n\nConstruct the MappedBuffer with the contents of \\a other using move\nsemantics\n\n(This is the usual way to document move constructors in\nhttps://en.cppreference.com/, which we're mimicking in libcamera.)\n\n> + * \\param[in] rhs The other MappedBuffer\n> + *\n> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n> + * destroyed in this process.\n> + */\n> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)\n> +{\n> +\t*this = std::move(rhs);\n> +}\n> +\n> +/**\n> + * \\brief Move assingment operator, move a MappedBuffer by taking the \\a rhs mappings\n\ns/assingment/assignement/\n\nBut for the same reason as above,\n\nReplace the contents with those of \\a other using move semantics\n\n> + * \\param[in] rhs The other MappedBuffer\n> + *\n> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n\ns/the \\a rhs/\\a other/\ns/ new// (this is not a newly constructed instance)\n\n> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n\ns/the \\a rhs/\\a other/\n\n> + * destroyed in this process.\n> + */\n> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)\n> +{\n> +\terror_ = rhs.error_;\n> +\tvalid_ = rhs.valid_;\n> +\tmaps_ = std::move(rhs.maps_);\n> +\trhs.valid_ = false;\n> +\trhs.error_ = 0;\n> +\n> +\treturn *this;\n> +}\n> +\n> +MappedBuffer::~MappedBuffer()\n> +{\n> +\tfor (MappedPlane map : maps_)\n> +\t\tmunmap(map.data(), map.size());\n> +}\n> +\n> +/**\n> + * \\fn MappedBuffer::isValid()\n> + * \\brief Check if the MappedBuffer instance is valid\n> + * \\return True if the MappedBuffer has valid mappings, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn MappedBuffer::error()\n> + * \\brief Retrieve the map error status\n> + *\n> + * This function retrieves the error status from the MappedBuffer.\n> + * The error status is a negative number as defined by errno.h. If\n> + * no error occurred, this function returns 0.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn MappedBuffer::maps()\n> + * \\brief Retrieve the mapped planes\n> + *\n> + * This function retrieves the successfully mapped planes stored as a vector\n> + * of Span to provide access to the mapped memory.\n> + *\n> + * \\return A vector of the mapped planes.\n\ns/.$//\n\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::valid_\n> + * \\brief Stores the status of the mapping\n> + *\n> + * MappedBuffer implementations shall set this to represent if the mapping\n\ns/implementations/derived classes/ ? Same below.\n\n> + * was successfully completed without errors.\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::error_\n> + * \\brief Stores the error value if present\n> + *\n> + * MappedBuffer implementations shall set this to a negative value as defined\n> + * by errno.h if an error occured during the mapping process\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::maps_\n> + * \\brief Stores the internal\n> + *\n> + * MappedBuffer implementations shall store the mappings they create in this\n> + * vector which is parsed during destruct to unmap any memory mappings which\n> + * completed successfully.\n> + */\n> +\n> +/**\n> + * \\class MappedFrameBuffer\n> + * \\brief Maps a FrameBuffer using the MappedBuffer interface\n\ns/Maps/Map/\n\n> + *\n> + * The MappedFrameBuffer interface maps a FrameBuffer instance\n\ns/$/./\n\nOr maybe drop the sentence as it duplicates the brief.\n\n> + *\n> + * The MappedBuffer interface does not implement any constructor but defines\n> + * the move operators and destructors for the derived implementations, which\n> + * are able to construct according to their derived types and given flags.\n\nIs this a leftover from a copy & paste ? I think ou can drop this\nparagraph.\n\n> + */\n> +\n> +/**\n> + * \\brief Map all planes of a FrameBuffer\n> + * \\param[in] buffer FrameBuffer to be mapped\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> +{\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\nYou can drop the blank line.\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\tbreak;\n> +\t\t}\n> +\n> +\t\tmaps_.emplace_back(static_cast(address), plane.length);\n> +\t}\n> +\n> +\tvalid_ = buffer->planes().size() == maps_.size();\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 93B5FBD86F\n\tfor ;\n\tWed, 5 Aug 2020 00:29:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1133460547;\n\tWed, 5 Aug 2020 02:29:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2474160392\n\tfor ;\n\tWed, 5 Aug 2020 02:29:20 +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 A77A227B;\n\tWed, 5 Aug 2020 02:29:16 +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=\"tLH/+8SH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596587356;\n\tbh=k19u6Sjj0D08GjqEzgxZ22lbQagwYH3Jd+0tH66g9IM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tLH/+8SHY5XWevxVOfJvWfSwW6VBx+Ud9rjIdJUooe+SBb40qhOKt+GeqUHV0f5td\n\toZWF0LWFdddyNnaNZYIs64zXPMi19ZPJ2rggdEiKE2jd8s6m9R0/+uQ6EPnoS6CrgY\n\t74Yl1HFTQU3fFpTkfGXxILBcPvd6ZfJA983ok1g0=","Date":"Wed, 5 Aug 2020 03:29:04 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200805002904.GV6075@pendragon.ideasonboard.com>","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804214711.177645-4-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","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":11874,"web_url":"https://patchwork.libcamera.org/comment/11874/","msgid":"<3daba6c4-9d14-df5b-d716-5f955b49cc17@ideasonboard.com>","date":"2020-08-05T11:27:01","subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 05/08/2020 01:29, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Tue, Aug 04, 2020 at 10:47:01PM +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>> The MappedFrameBuffer implements the interface of the MappedBuffer\n>> allowing other buffer types to be constructed of the same form, with a\n>> common interface and cleanup.\n>>\n>> This allows MappedBuffer instances to be created from Camera3Buffer types.\n>>\n>> Mappings are removed upon destruction.\n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> v3\n>> - Documentation fixups\n>> ---\n>> include/libcamera/internal/buffer.h | 47 ++++++++\n>> src/libcamera/buffer.cpp | 162 +++++++++++++++++++++++++++-\n>> 2 files changed, 208 insertions(+), 1 deletion(-)\n>> create mode 100644 include/libcamera/internal/buffer.h\n>>\n>> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h\n>> new file mode 100644\n>> index 000000000000..e86a003cd8ba\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/buffer.h\n>> @@ -0,0 +1,47 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * buffer.h - Internal buffer handling\n>> + */\n>> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__\n>> +#define __LIBCAMERA_INTERNAL_BUFFER_H__\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +using MappedPlane = Span;\n>> +\n>> +class MappedBuffer\n>> +{\n>> +public:\n> \n> How about moving MappedPlane here, and naming it Plane ?\n> \n> \tusing Plane = Span;\n> \n> This makes it clear that Plane is meant to be used with MappedBuffer.\n\nDefining it in the class namespace certainly makes sense. I was a bit\nweary of using the name Plane, as it could conflict with our other Plane\ntypes, but as it's scoped inside the MappedBuffer, I think that's OK,\nand after all - it is a Plane.\n\n\n>> +\tMappedBuffer();\n> \n> Do you want to allow construction of the base class, or only of derived\n> classes ? In the latter case, you should move this constructor to the\n> protected section below.\n\nHrm, I thought I needed this public to support STL construction of\nvectors or emplace or something.\n\nLooks like it's working in protected, so I'll move it there.\n\n\n>> +\t~MappedBuffer();\n>> +\n>> +\tMappedBuffer(MappedBuffer &&rhs);\n>> +\tMappedBuffer &operator=(MappedBuffer &&rhs);\n> \n> The parameter is customarily called other for the copy and move\n> constructors and assignment operators. rhs is usually used in\n> conjunction with lhs for non-member operators.\n\nRenamed.\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>> +protected:\n>> +\tint error_;\n>> +\tbool valid_;\n> \n> Do you need both ? Could isValid() return error_ == 0 ?\n\n\nI can do that, I will initialise error_ to -ENOENT in\nMappedBuffer::MappedBuffer().\n\n\n\n>> +\tstd::vector maps_;\n>> +};\n>> +\n>> +class MappedFrameBuffer : public MappedBuffer\n>> +{\n>> +public:\n>> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */\n>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n>> index 8278f8a92af4..5f7dff60a48b 100644\n>> --- a/src/libcamera/buffer.cpp\n>> +++ b/src/libcamera/buffer.cpp\n>> @@ -6,6 +6,7 @@\n>> */\n>> \n>> #include \n>> +#include \"libcamera/internal/buffer.h\"\n>> \n>> #include \n>> #include \n>> @@ -15,7 +16,8 @@\n>> #include \"libcamera/internal/log.h\"\n>> \n>> /**\n>> - * \\file buffer.h\n>> + * \\file libcamera/buffer.h\n>> + * \\file libcamera/internal/buffer.h\n>> * \\brief Buffer handling\n>> */\n>> \n>> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n>> \treturn 0;\n>> }\n>> \n>> +/**\n>> + * \\class MappedBuffer\n>> + * \\brief Provide an interface to support managing memory mapped buffers\n>> + *\n>> + * The MappedBuffer interface provides access to a set of MappedPlanes which\n>> + * are available for access by a CPU.\n> \n> s/a CPU/the CPU/ ?\n> \n> You're clearly thinking as a kernel developer :-)\n\nWell, sometimes ;-) I mean 'the' cpu is 'a' cpu ;-)\n\nAlso, I guess it depends on how you name the multiple cores/threads from\nuserspace.\n\n/sys/bus/cpu/devices/cpuN implies that each available core is a 'CPU',\nand it would be accessible from each of those CPU's...\n\n\nOf course, Mapping on 'my' CPU wouldn't be available for access on\n'your' CPU which is also 'a' CPU... so I think 'the CPU' is good.\n\n\n>> + *\n>> + * The MappedBuffer interface does not implement any valid constructor but\n>> + * defines the move operators and destructors for the derived implementations,\n>> + * which are able to construct according to their derived types and given\n>> + * flags.\n> \n> That's a bit of implementation details. Maybe \"This class is not meant\n> to be constructed directly, and thus has no public default constructor.\n> Derived classes shall be used instead.\" ?\n> \n\n\nReplaced with:\n\n\n * The MappedBuffer interface provides access to a set of MappedPlanes which\n * are available for access by the CPU.\n *\n * The MappedBuffer is not meant to be constructed directly, but instead\nderived\n * classes should be used to implement the correct mapping of a source\nbuffer.\n *\n * This allows treating CPU accessible memory through a generic interface\n * regardless of whether it originates from a libcamera FrameBuffer or other\n * source.\n\n\n>> + *\n>> + * This allows treating CPU accessible memory through a generic interface\n>> + * regardless of whether it originates from a libcamera FrameBuffer or other\n>> + * source.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct an empty MappedBuffer\n>> + *\n>> + * A default constructor is required to allow subclassing the MappedBuffer\n>> + * class. Construct an initialised, but invalid MappedBuffer.\n> \n> You can drop the first sentence, you're explaining C++ :-)\n\nWell someone's got to explain it to me ... it may as well be me ;-)\n\n(dropped).\n\n>> + */\n>> +MappedBuffer::MappedBuffer()\n>> +\t: error_(0), valid_(false)\n\n\nActually, now that this is protected, all this really does is initialise\nthe base class, and that's now only : error_(0).\n\nI've considered making this error_(-ENOENT), so it starts off invalid,\nbut the pattern used in both existing constructors for this now set\nerror_ = 0; and set the error if an error occurs...\n\nSo I think this is better left as error_(0), and update the function doc\nto just:\n\n/**\n * \\brief Construct an empty MappedBuffer\n *\n * Base class initialisation for MappedBuffer.\n */\n\n\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Construct a MappedBuffer by taking the \\a rhs mappings\n> \n> Construct the MappedBuffer with the contents of \\a other using move\n> semantics\n> \n> (This is the usual way to document move constructors in\n> https://en.cppreference.com/, which we're mimicking in libcamera.)\n\nIt's new to me that we are 'mimicking' en.cppreference.com.\nMaybe that needs to be documented somehow.\n\nI think I based my original text templates on the file_descriptor\nconstructors you authored, which differ slightly:\n\n* \\brief Move constructor, create a FileDescriptor by taking over \\a other\n\n...\n\n\n\n>> + * \\param[in] rhs The other MappedBuffer\n>> + *\n>> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n>> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n>> + * destroyed in this process.\n>> + */\n>> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)\n>> +{\n>> +\t*this = std::move(rhs);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Move assingment operator, move a MappedBuffer by taking the \\a rhs mappings\n> \n> s/assingment/assignement/\n> \n> But for the same reason as above,\n> \n> Replace the contents with those of \\a other using move semantics\n\nAgain, this was based upon text from file_descriptor:\n\n * \\brief Move assignment operator, replace the wrapped file descriptor\n * by taking over \\a other\n\nI think I like keeping the 'name of the constructor' to make it clear.\n\nIt's not quickly clear to me looking at\nMappedBuffer::MappedBuffer(MappedBuffer &&rhs) 'which' type of\nconstructor it is, so I think that's important.\n\n\n>> + * \\param[in] rhs The other MappedBuffer\n>> + *\n>> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n> \n> s/the \\a rhs/\\a other/\n> s/ new// (this is not a newly constructed instance)\n\nlikewise here, the wording was based on text you already wrote.\n\nYou wrote:\n\n * Moving a FileDescriptor moves the reference to the wrapped descriptor\n * owned by \\a other to the new FileDescriptor.\n\n\nThis is exhausting. Trying to emulate your text so you don't rewrite it,\nbut then you rewrite it anyway ;-)\n\n\n\n>> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n> \n> s/the \\a rhs/\\a other/\n\n\n\nOk, so the new text for both of those is:\n\n\n\n/**\n * \\brief Move constructor, construct the MappedBuffer with the contents\nof \\a\n * other using move semantics\n * \\param[in] other The other MappedBuffer\n *\n * Moving a MappedBuffer moves the mappings contained in the \\a other to\nthe new\n * MappedBuffer and invalidates the \\a other.\n *\n * No mappings are unmapped or destroyed in this process.\n */\n\n\n/**\n * \\brief Move assignment operator, replace the mappings with those of\n\\a other\n * mappings\n * \\param[in] other The other MappedBuffer\n *\n * Moving a MappedBuffer moves the mappings contained in the \\a other to\nthe new\n * MappedBuffer and invalidates the \\a other.\n *\n * No mappings are unmapped or destroyed in this process.\n */\n\n\n\n>> + * destroyed in this process.\n>> + */\n>> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)\n>> +{\n>> +\terror_ = rhs.error_;\n>> +\tvalid_ = rhs.valid_;\n>> +\tmaps_ = std::move(rhs.maps_);\n>> +\trhs.valid_ = false;\n>> +\trhs.error_ = 0;\n>> +\n>> +\treturn *this;\n>> +}\n>> +\n>> +MappedBuffer::~MappedBuffer()\n>> +{\n>> +\tfor (MappedPlane map : maps_)\n>> +\t\tmunmap(map.data(), map.size());\n>> +}\n>> +\n>> +/**\n>> + * \\fn MappedBuffer::isValid()\n>> + * \\brief Check if the MappedBuffer instance is valid\n>> + * \\return True if the MappedBuffer has valid mappings, false otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn MappedBuffer::error()\n>> + * \\brief Retrieve the map error status\n>> + *\n>> + * This function retrieves the error status from the MappedBuffer.\n>> + * The error status is a negative number as defined by errno.h. If\n>> + * no error occurred, this function returns 0.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn MappedBuffer::maps()\n>> + * \\brief Retrieve the mapped planes\n>> + *\n>> + * This function retrieves the successfully mapped planes stored as a vector\n>> + * of Span to provide access to the mapped memory.\n>> + *\n>> + * \\return A vector of the mapped planes.\n> \n> s/.$//\n> \n\nRemoved.\n\n>> + */\n>> +\n>> +/**\n>> + * \\var MappedBuffer::valid_\n>> + * \\brief Stores the status of the mapping\n>> + *\n>> + * MappedBuffer implementations shall set this to represent if the mapping\n> \n> s/implementations/derived classes/ ? Same below.\n> \n\nvalid_ removed.\n\n\n>> + * was successfully completed without errors.\n>> + */\n>> +\n>> +/**\n>> + * \\var MappedBuffer::error_\n>> + * \\brief Stores the error value if present\n>> + *\n>> + * MappedBuffer implementations shall set this to a negative value as defined\n>> + * by errno.h if an error occured during the mapping process\n>> + */\n>> +\n>> +/**\n>> + * \\var MappedBuffer::maps_\n>> + * \\brief Stores the internal\n>> + *\n>> + * MappedBuffer implementations shall store the mappings they create in this\n>> + * vector which is parsed during destruct to unmap any memory mappings which\n>> + * completed successfully.\n>> + */\n>> +\n>> +/**\n>> + * \\class MappedFrameBuffer\n>> + * \\brief Maps a FrameBuffer using the MappedBuffer interface\n> \n> s/Maps/Map/\n> \n>> + *\n>> + * The MappedFrameBuffer interface maps a FrameBuffer instance\n> \n> s/$/./\n> \n> Or maybe drop the sentence as it duplicates the brief.\n> \n>> + *\n>> + * The MappedBuffer interface does not implement any constructor but defines\n>> + * the move operators and destructors for the derived implementations, which\n>> + * are able to construct according to their derived types and given flags.\n> \n> Is this a leftover from a copy & paste ? I think ou can drop this\n> paragraph.\n\n\nDropping it leaves just the brief. Maybe that's fine. But it seems a bit\nbrief ;-)\n\nI thought the point of this section is to expand on the purpose of the\nclass as an overview, so I thought that paragraph was relevant as a summary.\n\n\n>> + */\n>> +\n>> +/**\n>> + * \\brief Map all planes of a FrameBuffer\n>> + * \\param[in] buffer FrameBuffer to be mapped\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>> +{\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> \n> You can drop the blank line.\n\nI liked that one, but dropped.\n\n\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\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tmaps_.emplace_back(static_cast(address), plane.length);\n>> +\t}\n>> +\n>> +\tvalid_ = buffer->planes().size() == maps_.size();\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 2893EBD86F\n\tfor ;\n\tWed, 5 Aug 2020 11:27:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB47460595;\n\tWed, 5 Aug 2020 13:27:06 +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 D186660492\n\tfor ;\n\tWed, 5 Aug 2020 13:27:04 +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 265102C0;\n\tWed, 5 Aug 2020 13:27:04 +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=\"vM91wBIt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596626824;\n\tbh=4lZZ0uraobNa7heoEDanKqE+XrNKO+D0cD+a6qcz8Eg=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=vM91wBItDaFVh8QZ+CtamTzbhEV18WIHcckFLXtzndpAp3/IDS12sf9Xz0lpgDqDA\n\tBIPGQP57bGKki74aADa4dZu4SUQXiXSL6Xe+erAL6PlAqOsc6XSoXewmBURqNV4uwc\n\teZCizxdztwEL1X+Ai8MlfxCHDnSkipzn1T7j8IlE=","To":"Laurent Pinchart ","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-4-kieran.bingham@ideasonboard.com>\n\t<20200805002904.GV6075@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":"<3daba6c4-9d14-df5b-d716-5f955b49cc17@ideasonboard.com>","Date":"Wed, 5 Aug 2020 12:27:01 +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":"<20200805002904.GV6075@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","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":11875,"web_url":"https://patchwork.libcamera.org/comment/11875/","msgid":"<72e0fff9-8f8a-e1b9-4a5f-0ff761aa0c7d@ideasonboard.com>","date":"2020-08-05T11:30:47","subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 05/08/2020 12:27, Kieran Bingham wrote:\n> Hi Laurent,\n> \n> On 05/08/2020 01:29, Laurent Pinchart wrote:\n>> Hi Kieran,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Aug 04, 2020 at 10:47:01PM +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>>> The MappedFrameBuffer implements the interface of the MappedBuffer\n>>> allowing other buffer types to be constructed of the same form, with a\n>>> common interface and cleanup.\n>>>\n>>> This allows MappedBuffer instances to be created from Camera3Buffer types.\n>>>\n>>> Mappings are removed upon destruction.\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>>\n>>> ---\n>>> v3\n>>> - Documentation fixups\n>>> ---\n>>> include/libcamera/internal/buffer.h | 47 ++++++++\n>>> src/libcamera/buffer.cpp | 162 +++++++++++++++++++++++++++-\n>>> 2 files changed, 208 insertions(+), 1 deletion(-)\n>>> create mode 100644 include/libcamera/internal/buffer.h\n>>>\n>>> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h\n>>> new file mode 100644\n>>> index 000000000000..e86a003cd8ba\n>>> --- /dev/null\n>>> +++ b/include/libcamera/internal/buffer.h\n>>> @@ -0,0 +1,47 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2020, Google Inc.\n>>> + *\n>>> + * buffer.h - Internal buffer handling\n>>> + */\n>>> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__\n>>> +#define __LIBCAMERA_INTERNAL_BUFFER_H__\n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +using MappedPlane = Span;\n>>> +\n>>> +class MappedBuffer\n>>> +{\n>>> +public:\n>>\n>> How about moving MappedPlane here, and naming it Plane ?\n>>\n>> \tusing Plane = Span;\n>>\n>> This makes it clear that Plane is meant to be used with MappedBuffer.\n> \n> Defining it in the class namespace certainly makes sense. I was a bit\n> weary of using the name Plane, as it could conflict with our other Plane\n> types, but as it's scoped inside the MappedBuffer, I think that's OK,\n> and after all - it is a Plane.\n> \n> \n>>> +\tMappedBuffer();\n>>\n>> Do you want to allow construction of the base class, or only of derived\n>> classes ? In the latter case, you should move this constructor to the\n>> protected section below.\n> \n> Hrm, I thought I needed this public to support STL construction of\n> vectors or emplace or something.\n> \n> Looks like it's working in protected, so I'll move it there.\n> \n> \n>>> +\t~MappedBuffer();\n>>> +\n>>> +\tMappedBuffer(MappedBuffer &&rhs);\n>>> +\tMappedBuffer &operator=(MappedBuffer &&rhs);\n>>\n>> The parameter is customarily called other for the copy and move\n>> constructors and assignment operators. rhs is usually used in\n>> conjunction with lhs for non-member operators.\n> \n> Renamed.\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>>> +protected:\n>>> +\tint error_;\n>>> +\tbool valid_;\n>>\n>> Do you need both ? Could isValid() return error_ == 0 ?\n> \n> \n> I can do that, I will initialise error_ to -ENOENT in\n> MappedBuffer::MappedBuffer().\n> \n> \n> \n>>> +\tstd::vector maps_;\n>>> +};\n>>> +\n>>> +class MappedFrameBuffer : public MappedBuffer\n>>> +{\n>>> +public:\n>>> +\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n>>> +};\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */\n>>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n>>> index 8278f8a92af4..5f7dff60a48b 100644\n>>> --- a/src/libcamera/buffer.cpp\n>>> +++ b/src/libcamera/buffer.cpp\n>>> @@ -6,6 +6,7 @@\n>>> */\n>>> \n>>> #include \n>>> +#include \"libcamera/internal/buffer.h\"\n>>> \n>>> #include \n>>> #include \n>>> @@ -15,7 +16,8 @@\n>>> #include \"libcamera/internal/log.h\"\n>>> \n>>> /**\n>>> - * \\file buffer.h\n>>> + * \\file libcamera/buffer.h\n>>> + * \\file libcamera/internal/buffer.h\n>>> * \\brief Buffer handling\n>>> */\n>>> \n>>> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)\n>>> \treturn 0;\n>>> }\n>>> \n>>> +/**\n>>> + * \\class MappedBuffer\n>>> + * \\brief Provide an interface to support managing memory mapped buffers\n>>> + *\n>>> + * The MappedBuffer interface provides access to a set of MappedPlanes which\n>>> + * are available for access by a CPU.\n>>\n>> s/a CPU/the CPU/ ?\n>>\n>> You're clearly thinking as a kernel developer :-)\n> \n> Well, sometimes ;-) I mean 'the' cpu is 'a' cpu ;-)\n> \n> Also, I guess it depends on how you name the multiple cores/threads from\n> userspace.\n> \n> /sys/bus/cpu/devices/cpuN implies that each available core is a 'CPU',\n> and it would be accessible from each of those CPU's...\n> \n> \n> Of course, Mapping on 'my' CPU wouldn't be available for access on\n> 'your' CPU which is also 'a' CPU... so I think 'the CPU' is good.\n> \n> \n>>> + *\n>>> + * The MappedBuffer interface does not implement any valid constructor but\n>>> + * defines the move operators and destructors for the derived implementations,\n>>> + * which are able to construct according to their derived types and given\n>>> + * flags.\n>>\n>> That's a bit of implementation details. Maybe \"This class is not meant\n>> to be constructed directly, and thus has no public default constructor.\n>> Derived classes shall be used instead.\" ?\n>>\n> \n> \n> Replaced with:\n> \n> \n> * The MappedBuffer interface provides access to a set of MappedPlanes which\n> * are available for access by the CPU.\n> *\n> * The MappedBuffer is not meant to be constructed directly, but instead\n> derived\n> * classes should be used to implement the correct mapping of a source\n> buffer.\n> *\n> * This allows treating CPU accessible memory through a generic interface\n> * regardless of whether it originates from a libcamera FrameBuffer or other\n> * source.\n> \n> \n>>> + *\n>>> + * This allows treating CPU accessible memory through a generic interface\n>>> + * regardless of whether it originates from a libcamera FrameBuffer or other\n>>> + * source.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct an empty MappedBuffer\n>>> + *\n>>> + * A default constructor is required to allow subclassing the MappedBuffer\n>>> + * class. Construct an initialised, but invalid MappedBuffer.\n>>\n>> You can drop the first sentence, you're explaining C++ :-)\n> \n> Well someone's got to explain it to me ... it may as well be me ;-)\n> \n> (dropped).\n> \n>>> + */\n>>> +MappedBuffer::MappedBuffer()\n>>> +\t: error_(0), valid_(false)\n> \n> \n> Actually, now that this is protected, all this really does is initialise\n> the base class, and that's now only : error_(0).\n> \n> I've considered making this error_(-ENOENT), so it starts off invalid,\n> but the pattern used in both existing constructors for this now set\n> error_ = 0; and set the error if an error occurs...\n> \n> So I think this is better left as error_(0), and update the function doc\n> to just:\n> \n> /**\n> * \\brief Construct an empty MappedBuffer\n> *\n> * Base class initialisation for MappedBuffer.\n> */\n> \n> \n>>> +{\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Construct a MappedBuffer by taking the \\a rhs mappings\n>>\n>> Construct the MappedBuffer with the contents of \\a other using move\n>> semantics\n>>\n>> (This is the usual way to document move constructors in\n>> https://en.cppreference.com/, which we're mimicking in libcamera.)\n> \n> It's new to me that we are 'mimicking' en.cppreference.com.\n> Maybe that needs to be documented somehow.\n> \n> I think I based my original text templates on the file_descriptor\n> constructors you authored, which differ slightly:\n> \n> * \\brief Move constructor, create a FileDescriptor by taking over \\a other\n> \n> ...\n> \n> \n> \n>>> + * \\param[in] rhs The other MappedBuffer\n>>> + *\n>>> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n>>> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n>>> + * destroyed in this process.\n>>> + */\n>>> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)\n>>> +{\n>>> +\t*this = std::move(rhs);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Move assingment operator, move a MappedBuffer by taking the \\a rhs mappings\n>>\n>> s/assingment/assignement/\n>>\n>> But for the same reason as above,\n>>\n>> Replace the contents with those of \\a other using move semantics\n> \n> Again, this was based upon text from file_descriptor:\n> \n> * \\brief Move assignment operator, replace the wrapped file descriptor\n> * by taking over \\a other\n> \n> I think I like keeping the 'name of the constructor' to make it clear.\n> \n> It's not quickly clear to me looking at\n> MappedBuffer::MappedBuffer(MappedBuffer &&rhs) 'which' type of\n> constructor it is, so I think that's important.\n> \n> \n>>> + * \\param[in] rhs The other MappedBuffer\n>>> + *\n>>> + * Moving a MappedBuffer moves the mappings contained in the \\a rhs to the new\n>>\n>> s/the \\a rhs/\\a other/\n>> s/ new// (this is not a newly constructed instance)\n> \n> likewise here, the wording was based on text you already wrote.\n> \n> You wrote:\n> \n> * Moving a FileDescriptor moves the reference to the wrapped descriptor\n> * owned by \\a other to the new FileDescriptor.\n> \n> \n> This is exhausting. Trying to emulate your text so you don't rewrite it,\n> but then you rewrite it anyway ;-)\n> \n> \n> \n>>> + * MappedBuffer and invalidates the \\a rhs. No mappings are unmapped or\n>>\n>> s/the \\a rhs/\\a other/\n> \n> \n> \n> Ok, so the new text for both of those is:\n> \n> \n> \n> /**\n> * \\brief Move constructor, construct the MappedBuffer with the contents\n> of \\a\n> * other using move semantics\n> * \\param[in] other The other MappedBuffer\n> *\n> * Moving a MappedBuffer moves the mappings contained in the \\a other to\n> the new\n> * MappedBuffer and invalidates the \\a other.\n> *\n> * No mappings are unmapped or destroyed in this process.\n> */\n> \n> \n> /**\n> * \\brief Move assignment operator, replace the mappings with those of\n> \\a other\n> * mappings\n> * \\param[in] other The other MappedBuffer\n> *\n> * Moving a MappedBuffer moves the mappings contained in the \\a other to\n> the new\n> * MappedBuffer and invalidates the \\a other.\n> *\n> * No mappings are unmapped or destroyed in this process.\n> */\n> \n> \n> \n>>> + * destroyed in this process.\n>>> + */\n>>> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)\n>>> +{\n>>> +\terror_ = rhs.error_;\n>>> +\tvalid_ = rhs.valid_;\n>>> +\tmaps_ = std::move(rhs.maps_);\n>>> +\trhs.valid_ = false;\n>>> +\trhs.error_ = 0;\n>>> +\n>>> +\treturn *this;\n>>> +}\n>>> +\n>>> +MappedBuffer::~MappedBuffer()\n>>> +{\n>>> +\tfor (MappedPlane map : maps_)\n>>> +\t\tmunmap(map.data(), map.size());\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\fn MappedBuffer::isValid()\n>>> + * \\brief Check if the MappedBuffer instance is valid\n>>> + * \\return True if the MappedBuffer has valid mappings, false otherwise\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn MappedBuffer::error()\n>>> + * \\brief Retrieve the map error status\n>>> + *\n>>> + * This function retrieves the error status from the MappedBuffer.\n>>> + * The error status is a negative number as defined by errno.h. If\n>>> + * no error occurred, this function returns 0.\n>>> + *\n>>> + * \\return 0 on success or a negative error code otherwise\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn MappedBuffer::maps()\n>>> + * \\brief Retrieve the mapped planes\n>>> + *\n>>> + * This function retrieves the successfully mapped planes stored as a vector\n>>> + * of Span to provide access to the mapped memory.\n>>> + *\n>>> + * \\return A vector of the mapped planes.\n>>\n>> s/.$//\n>>\n> \n> Removed.\n> \n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var MappedBuffer::valid_\n>>> + * \\brief Stores the status of the mapping\n>>> + *\n>>> + * MappedBuffer implementations shall set this to represent if the mapping\n>>\n>> s/implementations/derived classes/ ? Same below.\n>>\n> \n> valid_ removed.\n> \n> \n>>> + * was successfully completed without errors.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var MappedBuffer::error_\n>>> + * \\brief Stores the error value if present\n>>> + *\n>>> + * MappedBuffer implementations shall set this to a negative value as defined\n>>> + * by errno.h if an error occured during the mapping process\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var MappedBuffer::maps_\n>>> + * \\brief Stores the internal\n>>> + *\n>>> + * MappedBuffer implementations shall store the mappings they create in this\n>>> + * vector which is parsed during destruct to unmap any memory mappings which\n>>> + * completed successfully.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\class MappedFrameBuffer\n>>> + * \\brief Maps a FrameBuffer using the MappedBuffer interface\n>>\n>> s/Maps/Map/\n>>\n>>> + *\n>>> + * The MappedFrameBuffer interface maps a FrameBuffer instance\n>>\n>> s/$/./\n>>\n>> Or maybe drop the sentence as it duplicates the brief.\n>>\n>>> + *\n>>> + * The MappedBuffer interface does not implement any constructor but defines\n>>> + * the move operators and destructors for the derived implementations, which\n>>> + * are able to construct according to their derived types and given flags.\n>>\n>> Is this a leftover from a copy & paste ? I think ou can drop this\n>> paragraph.\n> \n> \n> Dropping it leaves just the brief. Maybe that's fine. But it seems a bit\n> brief ;-)\n> \n> I thought the point of this section is to expand on the purpose of the\n> class as an overview, so I thought that paragraph was relevant as a summary.\n\nAha, I just realised this is \\class MappedFrameBuffer not \\class\nMappedBuffer ;-)\n\nSo given the lightweight intention of that class, indeed a brief is\nprobably sufficient.\n\n\n> \n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Map all planes of a FrameBuffer\n>>> + * \\param[in] buffer FrameBuffer to be mapped\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>>> +{\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>>\n>> You can drop the blank line.\n> \n> I liked that one, but dropped.\n> \n> \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\tbreak;\n>>> +\t\t}\n>>> +\n>>> +\t\tmaps_.emplace_back(static_cast(address), plane.length);\n>>> +\t}\n>>> +\n>>> +\tvalid_ = buffer->planes().size() == maps_.size();\n>>> +}\n>>> +\n>>> } /* namespace libcamera */\n>>\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 E3E2DBD87A\n\tfor ;\n\tWed, 5 Aug 2020 11:30:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7684D60564;\n\tWed, 5 Aug 2020 13:30:51 +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 DA1C060492\n\tfor ;\n\tWed, 5 Aug 2020 13:30:50 +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 E82822C0;\n\tWed, 5 Aug 2020 13:30:49 +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=\"OLmOBSbe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596627050;\n\tbh=Oi6CvUyvFcQlo/Yty3lncQ9NARC6AdoUe9hA9W9YB5Q=;\n\th=Reply-To:Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=OLmOBSbeCp9uHlPinQOyjigI/O69f5sPxU1mLmQ8+mJDwMQ8sxdeS34sriIhmBkHd\n\tc6QfOG2IFXVX4Xs3cM8RxXWmd8/ZJbI2s5O4cEI9gLlfRziJ2tkCZ2tD47YwqjZAk/\n\tyQDdnwPEdA48IVUUI1z1UgyLXWBpP1ouSr5QFFaQ=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20200804214711.177645-1-kieran.bingham@ideasonboard.com>\n\t<20200804214711.177645-4-kieran.bingham@ideasonboard.com>\n\t<20200805002904.GV6075@pendragon.ideasonboard.com>\n\t<3daba6c4-9d14-df5b-d716-5f955b49cc17@ideasonboard.com>","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":"<72e0fff9-8f8a-e1b9-4a5f-0ff761aa0c7d@ideasonboard.com>","Date":"Wed, 5 Aug 2020 12:30:47 +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":"<3daba6c4-9d14-df5b-d716-5f955b49cc17@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] libcamera: buffer: Create a\n\tMappedBuffer","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\" "}}]