[{"id":18626,"web_url":"https://patchwork.libcamera.org/comment/18626/","msgid":"","date":"2021-08-09T14:28:01","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Give\n\tMappedFrameBuffer its own implementation","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, Aug 09, 2021 at 02:29:28PM +0100, Kieran Bingham wrote:\n> The MappedFrameBuffer is a convenience feature which sits on top of the\n> FrameBuffer and facilitates mapping it to CPU accessible memory with\n> mmap.\n> \n> This implementation is internal and currently sits in the same internal\n> files as the internal FrameBuffer, thus exposing those internals to\n> users of the MappedFramebuffer implementation.\n> \n> Move the MappedFrameBuffer and MappedBuffer implementation to its own\n> implementation files, and fix the sources throughout to use that\n> accordingly.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/framebuffer.h | 36 ----\n> .../libcamera/internal/mapped_framebuffer.h | 52 ++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/android/camera_device.h | 2 -\n> src/android/camera_stream.cpp | 2 +\n> src/android/camera_stream.h | 2 -\n> src/android/jpeg/encoder_libjpeg.cpp | 1 +\n> src/android/jpeg/encoder_libjpeg.h | 1 -\n> src/android/jpeg/post_processor_jpeg.h | 2 -\n> src/android/jpeg/thumbnailer.cpp | 2 +\n> src/android/jpeg/thumbnailer.h | 2 +-\n> src/android/mm/generic_camera_buffer.cpp | 3 +-\n> src/android/post_processor.h | 2 -\n> src/android/yuv/post_processor_yuv.cpp | 1 +\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> src/libcamera/framebuffer.cpp | 145 ---------------\n> src/libcamera/mapped_framebuffer.cpp | 171 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> test/mapped-buffer.cpp | 2 +-\n> 20 files changed, 237 insertions(+), 195 deletions(-)\n> create mode 100644 include/libcamera/internal/mapped_framebuffer.h\n> create mode 100644 src/libcamera/mapped_framebuffer.cpp\n> \n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index 8c187adf70c7..1352578a6cfb 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -7,46 +7,10 @@\n> #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n> #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n> \n> -#include \n> -#include \n> -\n> -#include \n\nThis header should be kept for Extensible::Private, used by\nFrameBuffer::Private.\n\n> -#include \n> -\n> #include \n> \n> namespace libcamera {\n> \n> -class MappedBuffer\n> -{\n> -public:\n> -\tusing Plane = Span;\n> -\n> -\t~MappedBuffer();\n> -\n> -\tMappedBuffer(MappedBuffer &&other);\n> -\tMappedBuffer &operator=(MappedBuffer &&other);\n> -\n> -\tbool isValid() const { return error_ == 0; }\n> -\tint error() const { return error_; }\n> -\tconst std::vector &maps() const { return maps_; }\n> -\n> -protected:\n> -\tMappedBuffer();\n> -\n> -\tint error_;\n> -\tstd::vector maps_;\n> -\n> -private:\n> -\tLIBCAMERA_DISABLE_COPY(MappedBuffer)\n> -};\n> -\n> -class MappedFrameBuffer : public MappedBuffer\n> -{\n> -public:\n> -\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> -};\n> -\n> class FrameBuffer::Private : public Extensible::Private\n> {\n> \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> new file mode 100644\n> index 000000000000..41e587364260\n> --- /dev/null\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -0,0 +1,52 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * mapped_framebuffer.h - Frame buffer memory mapping support\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class MappedBuffer\n> +{\n> +public:\n> +\tusing Plane = Span;\n> +\n> +\t~MappedBuffer();\n> +\n> +\tMappedBuffer(MappedBuffer &&other);\n> +\tMappedBuffer &operator=(MappedBuffer &&other);\n> +\n> +\tbool isValid() const { return error_ == 0; }\n> +\tint error() const { return error_; }\n> +\tconst std::vector &maps() const { return maps_; }\n> +\n> +protected:\n> +\tMappedBuffer();\n> +\n> +\tint error_;\n> +\tstd::vector maps_;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(MappedBuffer)\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_MAPPED_FRAMEBUFFER_H__ */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index dac1a2d36fa8..665fd6de4ed3 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([\n> 'ipa_module.h',\n> 'ipa_proxy.h',\n> 'ipc_unixsocket.h',\n> + 'mapped_framebuffer.h',\n> 'media_device.h',\n> 'media_object.h',\n> 'pipeline_handler.h',\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 089a6204605e..dd9aebba7553 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -24,8 +24,6 @@\n> #include \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> #include \"camera_capabilities.h\"\n> #include \"camera_metadata.h\"\n> #include \"camera_stream.h\"\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index bf4a7b41a70a..61b441830e18 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \"camera_stream.h\"\n> \n> +#include \n> +\n> #include \"camera_buffer.h\"\n> #include \"camera_device.h\"\n> #include \"camera_metadata.h\"\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 629d9e00e08d..2dab6c3a37d1 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -19,8 +19,6 @@\n> #include \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> class CameraDevice;\n> class CameraMetadata;\n> class PostProcessor;\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index e6358ca9466f..372018d2207f 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -23,6 +23,7 @@\n> #include \n> \n> #include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> using namespace libcamera;\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 14bf89223982..61fbd1a69278 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -10,7 +10,6 @@\n> #include \"encoder.h\"\n> \n> #include \"libcamera/internal/formats.h\"\n> -#include \"libcamera/internal/framebuffer.h\"\n> \n> #include \n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index 5c399be9eb6a..6fd3102229ab 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -13,8 +13,6 @@\n> \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> class CameraDevice;\n> \n> class PostProcessorJpeg : public PostProcessor\n> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> index 5cb00744a688..535e2cece914 100644\n> --- a/src/android/jpeg/thumbnailer.cpp\n> +++ b/src/android/jpeg/thumbnailer.cpp\n> @@ -11,6 +11,8 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> using namespace libcamera;\n> \n> LOG_DEFINE_CATEGORY(Thumbnailer)\n> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> index 68cbf74329a9..4d086c4943b0 100644\n> --- a/src/android/jpeg/thumbnailer.h\n> +++ b/src/android/jpeg/thumbnailer.h\n> @@ -7,10 +7,10 @@\n> #ifndef __ANDROID_JPEG_THUMBNAILER_H__\n> #define __ANDROID_JPEG_THUMBNAILER_H__\n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/formats.h\"\n> -#include \"libcamera/internal/framebuffer.h\"\n> \n> class Thumbnailer\n> {\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 2a4b77ea5236..b3af194c21dd 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -7,11 +7,12 @@\n> \n> #include \"../camera_buffer.h\"\n> \n> +#include \n> #include \n> \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> using namespace libcamera;\n> \n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index 689f85d9d3b8..ab2b2c606fd0 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -10,8 +10,6 @@\n> #include \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> #include \"camera_buffer.h\"\n> \n> class CameraMetadata;\n> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> index 772e805b32cc..509d4244d614 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> \n> #include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> using namespace libcamera;\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36e50f..2647bf2f3b96 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -20,7 +20,7 @@\n> #include \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> #include \"ipu3_agc.h\"\n> #include \"ipu3_awb.h\"\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 48e11c699f29..76f67dd4567a 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -28,7 +28,7 @@\n> #include \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> #include \"agc_algorithm.hpp\"\n> #include \"agc_status.h\"\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index a59e93fbdf91..3d98affb20f9 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -251,149 +251,4 @@ Request *FrameBuffer::request() const\n> * indicate that the metadata is invalid.\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 the CPU.\n> - *\n> - * This class is not meant to be constructed directly, but instead derived\n> - * classes should be used to implement the correct mapping of a source 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> - * \\typedef MappedBuffer::Plane\n> - * \\brief A mapped region of memory accessible to the CPU\n> - *\n> - * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n> - * region.\n> - */\n> -\n> -/**\n> - * \\brief Construct an empty MappedBuffer\n> - */\n> -MappedBuffer::MappedBuffer()\n> -\t: error_(0)\n> -{\n> -}\n> -\n> -/**\n> - * \\brief Move constructor, construct the MappedBuffer with the contents 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 the new\n> - * MappedBuffer and invalidates the \\a other.\n> - *\n> - * No mappings are unmapped or destroyed in this process.\n> - */\n> -MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> -{\n> -\t*this = std::move(other);\n> -}\n> -\n> -/**\n> - * \\brief Move assignment operator, replace the mappings with those of \\a other\n> -* \\param[in] other The other MappedBuffer\n> - *\n> - * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> - * MappedBuffer and invalidates the \\a other.\n> - *\n> - * No mappings are unmapped or destroyed in this process.\n> - */\n> -MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> -{\n> -\terror_ = other.error_;\n> -\tmaps_ = std::move(other.maps_);\n> -\tother.error_ = -ENOENT;\n> -\n> -\treturn *this;\n> -}\n> -\n> -MappedBuffer::~MappedBuffer()\n> -{\n> -\tfor (Plane &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 The map error code\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::error_\n> - * \\brief Stores the error value if present\n> - *\n> - * MappedBuffer derived classes 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 mapped planes\n> - *\n> - * MappedBuffer derived classes 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 Map a FrameBuffer using the MappedBuffer interface\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> -\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> -\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> new file mode 100644\n> index 000000000000..0e30fc542154\n> --- /dev/null\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -0,0 +1,171 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * mapped_framebuffer.cpp - Mapped Framebuffer support\n> + */\n> +\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n\nI think you can drop string.h and unistd.h.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +#include \n> +\n> +/**\n> + * \\file libcamera/internal/mapped_framebuffer.h\n> + * \\brief Frame buffer memory mapping support\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Buffer)\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 the CPU.\n> + *\n> + * This class is not meant to be constructed directly, but instead derived\n> + * classes should be used to implement the correct mapping of a source 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> + * \\typedef MappedBuffer::Plane\n> + * \\brief A mapped region of memory accessible to the CPU\n> + *\n> + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n> + * region.\n> + */\n> +\n> +/**\n> + * \\brief Construct an empty MappedBuffer\n> + */\n> +MappedBuffer::MappedBuffer()\n> +\t: error_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Move constructor, construct the MappedBuffer with the contents 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 the new\n> + * MappedBuffer and invalidates the \\a other.\n> + *\n> + * No mappings are unmapped or destroyed in this process.\n> + */\n> +MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> +{\n> +\t*this = std::move(other);\n> +}\n> +\n> +/**\n> + * \\brief Move assignment operator, replace the mappings with those of \\a other\n> +* \\param[in] other The other MappedBuffer\n> + *\n> + * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> + * MappedBuffer and invalidates the \\a other.\n> + *\n> + * No mappings are unmapped or destroyed in this process.\n> + */\n> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> +{\n> +\terror_ = other.error_;\n> +\tmaps_ = std::move(other.maps_);\n> +\tother.error_ = -ENOENT;\n> +\n> +\treturn *this;\n> +}\n> +\n> +MappedBuffer::~MappedBuffer()\n> +{\n> +\tfor (Plane &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 The map error code\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::error_\n> + * \\brief Stores the error value if present\n> + *\n> + * MappedBuffer derived classes 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 mapped planes\n> + *\n> + * MappedBuffer derived classes 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 Map a FrameBuffer using the MappedBuffer interface\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> +\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> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 4f08580157f9..e9230b983aeb 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -28,6 +28,7 @@ libcamera_sources = files([\n> 'ipc_pipe.cpp',\n> 'ipc_pipe_unixsocket.cpp',\n> 'ipc_unixsocket.cpp',\n> + 'mapped_framebuffer.cpp',\n> 'media_device.cpp',\n> 'media_object.cpp',\n> 'pipeline_handler.cpp',\n> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> index c9479194cb68..a3d1511b74ce 100644\n> --- a/test/mapped-buffer.cpp\n> +++ b/test/mapped-buffer.cpp\n> @@ -9,7 +9,7 @@\n> \n> #include \n> \n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> \n> #include \"camera_test.h\"\n> #include \"test.h\"","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 128D3BD87D\n\tfor ;\n\tMon, 9 Aug 2021 14:28:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 704EF6884F;\n\tMon, 9 Aug 2021 16:28:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B91060269\n\tfor ;\n\tMon, 9 Aug 2021 16:28:03 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 132B8DD;\n\tMon, 9 Aug 2021 16:28:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"I3hgo1lO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628519283;\n\tbh=6UWHWxpda6g74fXWQrQ7jAVoAZUlVN1KWINvrDYxApY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I3hgo1lOla+ZTuusO+uXkgPO/irFSgxH+6uzKTp5ycwHDniOY8O+sqxi7z8LrRPI+\n\tm50wdyDxOR+he+o09HJhTuBWMQKHPyUxQwZ7slDJSyyVxSdAwbxUktC13Auj/Ptc67\n\tfVn89tfA6L4Xqt0kfgO479N/syvs3kScK3E/fSls=","Date":"Mon, 9 Aug 2021 17:28:01 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210809132929.1824114-1-kieran.bingham@ideasonboard.com>\n\t<20210809132929.1824114-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210809132929.1824114-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Give\n\tMappedFrameBuffer its own implementation","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18637,"web_url":"https://patchwork.libcamera.org/comment/18637/","msgid":"<84a2d1b4-21c9-e2ca-b69f-a378b5d397f3@ideasonboard.com>","date":"2021-08-09T16:40:42","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Give\n\tMappedFrameBuffer its own implementation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 09/08/2021 15:28, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Mon, Aug 09, 2021 at 02:29:28PM +0100, Kieran Bingham wrote:\n>> The MappedFrameBuffer is a convenience feature which sits on top of the\n>> FrameBuffer and facilitates mapping it to CPU accessible memory with\n>> mmap.\n>>\n>> This implementation is internal and currently sits in the same internal\n>> files as the internal FrameBuffer, thus exposing those internals to\n>> users of the MappedFramebuffer implementation.\n>>\n>> Move the MappedFrameBuffer and MappedBuffer implementation to its own\n>> implementation files, and fix the sources throughout to use that\n>> accordingly.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/internal/framebuffer.h | 36 ----\n\n\n\n>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n>> new file mode 100644\n>> index 000000000000..0e30fc542154\n>> --- /dev/null\n>> +++ b/src/libcamera/mapped_framebuffer.cpp\n>> @@ -0,0 +1,171 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * mapped_framebuffer.cpp - Mapped Framebuffer support\n>> + */\n>> +\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n> \n> I think you can drop string.h and unistd.h.\n> \n\n\niwyu says:\n\n../src/libcamera/mapped_framebuffer.cpp should add these lines:\n#include // for max\n#include // for operator<<\n#include // for move\n#include \"libcamera/file_descriptor.h\" // for FileDescriptor\n#include \"libcamera/framebuffer.h\" // for FrameBuffer,\nFrameBuffer::Plane\n\n../src/libcamera/mapped_framebuffer.cpp should remove these lines:\n- #include // lines 11-11\n- #include // lines 13-13\n\n\nand also:\n\n../src/libcamera/framebuffer.cpp should remove these lines:\n- #include // lines 11-11\n- #include // lines 12-12\n- #include // lines 13-13\n- #include // lines 14-14\n\n\nwhile we're moving code on these files.\n\nI will remove those extra unused headers during this patch.\n\n\nI don't know if we want to go as far as including headers for\n/everything/ we use yet ... All of the above suggestions are already\nbrought in by the other headers, and are not required to compile of\ncourse...\n\n\n\n> Reviewed-by: Laurent Pinchart \n\nThanks\n\n\n> \n>> +\n>> +#include ","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 DD569BD87D\n\tfor ;\n\tMon, 9 Aug 2021 16:40:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 422CE6884F;\n\tMon, 9 Aug 2021 18:40:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA26960269\n\tfor ;\n\tMon, 9 Aug 2021 18:40:44 +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 4EA7549A;\n\tMon, 9 Aug 2021 18:40:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qdIn5zXl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628527244;\n\tbh=UBj+wk56JJkzxi/WlTss21GCJ+zCgAoE5FJjWWK6wYI=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=qdIn5zXlVCGn/6nl/43MJQ19hgS9akUaxLI7A0klIHwZ/9agPHYuRkpMYViipV1qq\n\tGzAuwWl69XMyXONzomzW2Tx56+Gd8LmZTfaGh1AH8XHExLfNor6jf8UB0TAqsNI5mM\n\tNgRwpmtJrjOhUnkl1C8vKjnC4RCqQiYe4qlyBNuw=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210809132929.1824114-1-kieran.bingham@ideasonboard.com>\n\t<20210809132929.1824114-3-kieran.bingham@ideasonboard.com>\n\t","Message-ID":"<84a2d1b4-21c9-e2ca-b69f-a378b5d397f3@ideasonboard.com>","Date":"Mon, 9 Aug 2021 17:40:42 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Give\n\tMappedFrameBuffer its own implementation","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18651,"web_url":"https://patchwork.libcamera.org/comment/18651/","msgid":"","date":"2021-08-10T02:45:22","subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Give\n\tMappedFrameBuffer its own implementation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran, thank you for the patch.\n\nOn Tue, Aug 10, 2021 at 1:40 AM Kieran Bingham\n wrote:\n>\n> On 09/08/2021 15:28, Laurent Pinchart wrote:\n> > Hi Kieran,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Aug 09, 2021 at 02:29:28PM +0100, Kieran Bingham wrote:\n> >> The MappedFrameBuffer is a convenience feature which sits on top of the\n> >> FrameBuffer and facilitates mapping it to CPU accessible memory with\n> >> mmap.\n> >>\n> >> This implementation is internal and currently sits in the same internal\n> >> files as the internal FrameBuffer, thus exposing those internals to\n> >> users of the MappedFramebuffer implementation.\n> >>\n> >> Move the MappedFrameBuffer and MappedBuffer implementation to its own\n> >> implementation files, and fix the sources throughout to use that\n> >> accordingly.\n> >>\n> >> Signed-off-by: Kieran Bingham \n\nReviewed-by: Hirokazu Honda \n\n> >> ---\n> >> include/libcamera/internal/framebuffer.h | 36 ----\n>\n> \n>\n> >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> >> new file mode 100644\n> >> index 000000000000..0e30fc542154\n> >> --- /dev/null\n> >> +++ b/src/libcamera/mapped_framebuffer.cpp\n> >> @@ -0,0 +1,171 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Google Inc.\n> >> + *\n> >> + * mapped_framebuffer.cpp - Mapped Framebuffer support\n> >> + */\n> >> +\n> >> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >\n> > I think you can drop string.h and unistd.h.\n> >\n>\n>\n> iwyu says:\n>\n> ../src/libcamera/mapped_framebuffer.cpp should add these lines:\n> #include // for max\n> #include // for operator<<\n> #include // for move\n> #include \"libcamera/file_descriptor.h\" // for FileDescriptor\n> #include \"libcamera/framebuffer.h\" // for FrameBuffer,\n> FrameBuffer::Plane\n>\n> ../src/libcamera/mapped_framebuffer.cpp should remove these lines:\n> - #include // lines 11-11\n> - #include // lines 13-13\n>\n>\n> and also:\n>\n> ../src/libcamera/framebuffer.cpp should remove these lines:\n> - #include // lines 11-11\n> - #include // lines 12-12\n> - #include // lines 13-13\n> - #include // lines 14-14\n>\n>\n> while we're moving code on these files.\n>\n> I will remove those extra unused headers during this patch.\n>\n>\n> I don't know if we want to go as far as including headers for\n> /everything/ we use yet ... All of the above suggestions are already\n> brought in by the other headers, and are not required to compile of\n> course...\n>\n>\n>\n> > Reviewed-by: Laurent Pinchart \n>\n> Thanks\n>\n>\n> >\n> >> +\n> >> +#include ","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 9392FBD87D\n\tfor ;\n\tTue, 10 Aug 2021 02:45:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D14A687FA;\n\tTue, 10 Aug 2021 04:45:35 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FEA6687EB\n\tfor ;\n\tTue, 10 Aug 2021 04:45:34 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id hs10so32735781ejc.0\n\tfor ;\n\tMon, 09 Aug 2021 19:45:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"AqIuSGJ6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=i9Ij3snM6qmptJiri06GzShwd3Qe5oQvseVLfnh69E4=;\n\tb=AqIuSGJ6sccD3kFj+ePh6RkpL1sPBT2bcF5tzudH34cVLhYP/qjTk2Mn2X+Lw0nx+R\n\tbwXSQohTDoY4gL0nwquCLaQVw09kL0VJ+2Ehh9R3guuLa8P31PdHcvD8Vbcmxp6cE9m/\n\tWHU17xsko8D9EXi3pzCebOmCi0ePtcTGxKemg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=i9Ij3snM6qmptJiri06GzShwd3Qe5oQvseVLfnh69E4=;\n\tb=obGnItlnIMbPNiTbHNXyVxjqbLh0iXb7SbCTJuTmXpNwzVrkpMSsVQ5xu5PniP6ySc\n\t9q0atqh9tG/uyMcwclYLBpQoQa+Lu+aUYjgkXi3oxgN/ZyNlS4Wk26vw1N7+2W6qFZHp\n\tl+Bl22Hk9Q3P/8bc7nCg/QxGICClldPCcPnmAKKCJz4oiRzVlVxF9nSS1je8aBXZMSgf\n\ti0ilIg76dk9HSe6oeFENhLL+guJRifP5N9DUX+PmTLFFyW0opVwguiUdvWZ+l5+b/V6h\n\t6w65UPLDPtzhwUHBxcA7Z9fW1V6GDez3U1t4WMBAackVU6YfzK79OwU22n5ZPkjDkIC/\n\tCjIA==","X-Gm-Message-State":"AOAM531uJUknadAbMAkklTCiTAoq1S/jm+lYK2j6P1pzfZIXZr/7T4A9\n\tBaSei4UdtCskRSCSVX+rngxQ7/DRU3gSLinPMtnvsbTY8QI=","X-Google-Smtp-Source":"ABdhPJxhI0T64JxMu8Dnk9T4RvVNETiwRRzy7cqeS/foXByeaFZLOKPNt/0XaeP8VkWN0VhoA9mf/uaxi3v+qsCx1Gc=","X-Received":"by 2002:a17:906:90d9:: with SMTP id\n\tv25mr12657431ejw.221.1628563533578; \n\tMon, 09 Aug 2021 19:45:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20210809132929.1824114-1-kieran.bingham@ideasonboard.com>\n\t<20210809132929.1824114-3-kieran.bingham@ideasonboard.com>\n\t\n\t<84a2d1b4-21c9-e2ca-b69f-a378b5d397f3@ideasonboard.com>","In-Reply-To":"<84a2d1b4-21c9-e2ca-b69f-a378b5d397f3@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 10 Aug 2021 11:45:22 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] libcamera: Give\n\tMappedFrameBuffer its own implementation","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]