[{"id":18602,"web_url":"https://patchwork.libcamera.org/comment/18602/","msgid":"","date":"2021-08-06T15:19:00","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use\n\ttyped Flags","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 Fri, Aug 06, 2021 at 02:53:15PM +0100, Kieran Bingham wrote:\n> Remove the need for callers to reference PROT_READ/PROT_WRITE directly\n> from by instead exposing the Read/Write mapping options as\n> flags from the MappedFrameBuffer class itself.\n> \n> While here, introduce the header which is required for the\n> uint8_t as part of the Plane.\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> v2\n> - Use fully scoped enum class\n> - Swap MapMode and MapModes\n> - s/mmap_flags/mmapFlags/\n> - Remove and fix documentation regarding the modes\n> - add LIBCAMERA_FLAGS_ENABLE_OPERATORS()\n> \n> .../libcamera/internal/mapped_framebuffer.h | 16 +++++++--\n> src/android/jpeg/encoder_libjpeg.cpp | 2 +-\n> src/android/jpeg/thumbnailer.cpp | 2 +-\n> src/android/yuv/post_processor_yuv.cpp | 2 +-\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 3 +-\n> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++---\n> test/mapped-buffer.cpp | 6 ++--\n> 8 files changed, 52 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> index 41e587364260..e8234ebf76ff 100644\n> --- a/include/libcamera/internal/mapped_framebuffer.h\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -7,10 +7,11 @@\n> #ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> #define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> \n> -#include \n\nI trust that you've looked through the code base to ensure no other\nunnecessary inclusion of sys/mman.h is left.\n\n> +#include \n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -44,9 +45,20 @@ private:\n> class MappedFrameBuffer : public MappedBuffer\n> {\n> public:\n> -\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> +\tenum class MapMode {\n> +\t\tRead = 0 << 1,\n> +\t\tWrite = 1 << 1,\n> +\n\nI'd drop this blank line.\n\n> +\t\tReadWrite = Read | Write,\n> +\t};\n> +\n> +\tusing MapModes = Flags;\n> +\n> +\tMappedFrameBuffer(const FrameBuffer *buffer, MapModes flags);\n\nI'll be annoying again I'm afraid, but \"MapModes flags\" bothers me :-/\nLooking at the two example from File, we could have\n\n\tenum class MapModeFlag {\n\t\t...\n\t};\n\n\tusing MapMode = Flags;\n\n\tMappedFrameBuffer(const FrameBuffer *buffer, MapMode mode);\n\nor\n\tenum class MapFlag {\n\t\t...\n\t};\n\n\tusing MapFlags = Flags;\n\n\tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n\nOther options are possible (and it may make sense to standardize naming\nschemes once and for all), but naming the type \"modes\" and the argument\n\"flags\" lacks consistency.\n\nSorry for being nitpicking, it's the first usage of Flags<> outside of\nthe File class, so it's affected by the 0, 1, N generalization theorem.\nI'm also thinking that MappedBuffer may graduate as a public API, hence\nthe particular attention.\n\n> };\n> \n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode)\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index 372018d2207f..34472e8da6a2 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -182,7 +182,7 @@ void EncoderLibJpeg::compressNV(Span frame)\n> int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> \t\t\t Span exifData, unsigned int quality)\n> {\n> -\tMappedFrameBuffer frame(&source, PROT_READ);\n> +\tMappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);\n> \tif (!frame.isValid()) {\n> \t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> \t\t\t\t << strerror(frame.error());\n> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> index 535e2cece914..66b4e696b107 100644\n> --- a/src/android/jpeg/thumbnailer.cpp\n> +++ b/src/android/jpeg/thumbnailer.cpp\n> @@ -41,7 +41,7 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,\n> \t\t\t\t const Size &targetSize,\n> \t\t\t\t std::vector *destination)\n> {\n> -\tMappedFrameBuffer frame(&source, PROT_READ);\n> +\tMappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);\n> \tif (!frame.isValid()) {\n> \t\tLOG(Thumbnailer, Error)\n> \t\t\t<< \"Failed to map FrameBuffer : \"\n> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> index 509d4244d614..6d156c20646c 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -57,7 +57,7 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> \tif (!isValidBuffers(source, *destination))\n> \t\treturn -EINVAL;\n> \n> -\tconst MappedFrameBuffer sourceMapped(&source, PROT_READ);\n> +\tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapMode::Read);\n> \tif (!sourceMapped.isValid()) {\n> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> \t\treturn -EINVAL;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 2647bf2f3b96..c8fc26525707 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -211,7 +211,7 @@ void IPAIPU3::mapBuffers(const std::vector &buffers)\n> \tfor (const IPABuffer &buffer : buffers) {\n> \t\tconst FrameBuffer fb(buffer.planes);\n> \t\tbuffers_.emplace(buffer.id,\n> -\t\t\t\t MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));\n> +\t\t\t\t MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));\n> \t}\n> }\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 76f67dd4567a..54e22d91084b 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -411,7 +411,8 @@ void IPARPi::mapBuffers(const std::vector &buffers)\n> {\n> \tfor (const IPABuffer &buffer : buffers) {\n> \t\tconst FrameBuffer fb(buffer.planes);\n> -\t\tbuffers_.emplace(buffer.id, MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));\n> +\t\tbuffers_.emplace(buffer.id,\n> +\t\t\t\t MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));\n> \t}\n> }\n> \n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> index 0e30fc542154..207c80b5aba2 100644\n> --- a/src/libcamera/mapped_framebuffer.cpp\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -142,21 +142,45 @@ MappedBuffer::~MappedBuffer()\n> * \\brief Map a FrameBuffer using the MappedBuffer interface\n> */\n> \n> +/**\n> + * \\enum MappedFrameBuffer::MapMode\n> + * \\brief Specify the mapping mode for the FrameBuffer\n> + * \\var MappedFrameBuffer::Read\n> + * \\brief Create a Read-only mapping\n\nMaybe s/Read/read/ ? Same for write.\n\n> + * \\var MappedFrameBuffer::Write\n> + * \\brief Create a Write-only mapping\n> + * \\var MappedFrameBuffer::ReadWrite\n> + * \\brief Create a mapping with both read and write capabilities\n\nCapabilities sound a bit weird to me in this context, I would have\nwritten \"Create a mapping that can be both read and written\".\n\n> + */\n> +\n> +/**\n> + * \\typedef MappedFrameBuffer::MapModes\n> + * \\brief A bitwise combination of MappedFrameBuffer::MapMode values\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> + * Construct an object to map a frame buffer for CPU access. The mapping can be\n> + * made as Read only, Write only or support Read and Write operations by setting\n> + * the MapMode flags accordingly.\n> */\n> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags)\n> {\n> \tmaps_.reserve(buffer->planes().size());\n> \n> +\tint mmapFlags = 0;\n> +\n> +\tif (flags & MapMode::Read)\n> +\t\tmmapFlags |= PROT_READ;\n> +\n> +\tif (flags & MapMode::Write)\n> +\t\tmmapFlags |= PROT_WRITE;\n> +\n> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> -\t\tvoid *address = mmap(nullptr, plane.length, flags,\n> +\t\tvoid *address = mmap(nullptr, plane.length, mmapFlags,\n> \t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n> \t\tif (address == MAP_FAILED) {\n> \t\t\terror_ = -errno;\n> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> index a3d1511b74ce..63d20e7bc9c4 100644\n> --- a/test/mapped-buffer.cpp\n> +++ b/test/mapped-buffer.cpp\n> @@ -71,7 +71,7 @@ protected:\n> \t\tconst std::unique_ptr &buffer = allocator_->buffers(stream_).front();\n> \t\tstd::vector maps;\n> \n> -\t\tMappedFrameBuffer map(buffer.get(), PROT_READ);\n> +\t\tMappedFrameBuffer map(buffer.get(), MappedFrameBuffer::MapMode::Read);\n> \t\tif (!map.isValid()) {\n> \t\t\tcout << \"Failed to successfully map buffer\" << endl;\n> \t\t\treturn TestFail;\n> @@ -90,13 +90,13 @@ protected:\n> \t\t}\n> \n> \t\t/* Test for multiple successful maps on the same buffer. */\n> -\t\tMappedFrameBuffer write_map(buffer.get(), PROT_WRITE);\n> +\t\tMappedFrameBuffer write_map(buffer.get(), MappedFrameBuffer::MapMode::Write);\n> \t\tif (!write_map.isValid()) {\n> \t\t\tcout << \"Failed to map write buffer\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tMappedFrameBuffer rw_map(buffer.get(), PROT_READ | PROT_WRITE);\n> +\t\tMappedFrameBuffer rw_map(buffer.get(), MappedFrameBuffer::MapMode::ReadWrite);\n> \t\tif (!rw_map.isValid()) {\n> \t\t\tcout << \"Failed to map RW buffer\" << endl;\n> \t\t\treturn TestFail;","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 1F222C3235\n\tfor ;\n\tFri, 6 Aug 2021 15:19:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9560868815;\n\tFri, 6 Aug 2021 17:19:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71D9460266\n\tfor ;\n\tFri, 6 Aug 2021 17:19:15 +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 EA5354FB;\n\tFri, 6 Aug 2021 17:19:14 +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=\"tGLKAYwW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628263155;\n\tbh=KQ9yH9b3kOEQcbNWRfZ+2cuokv658ZApiw7Dk8pewlg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tGLKAYwW0uCa4RRCv3yCsxRKYOOzXccq7NdXiVMmCzR1vZ/Z8ER06dyJLplYuXIRS\n\tUcsi+8UaXalDb2HsVYTyspldrvfnGQ4hdtgP6Yacf8niRPnVpjc9QzXkUrqUTxf3Eb\n\t+aU1sLhLwL8bckJijjJpHXXRi6qCnvdtSJCPjKvg=","Date":"Fri, 6 Aug 2021 18:19:00 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t<20210806135315.743684-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210806135315.743684-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use\n\ttyped Flags","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":18621,"web_url":"https://patchwork.libcamera.org/comment/18621/","msgid":"","date":"2021-08-09T12:47:42","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use\n\ttyped Flags","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/08/2021 16:19, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 06, 2021 at 02:53:15PM +0100, Kieran Bingham wrote:\n>> Remove the need for callers to reference PROT_READ/PROT_WRITE directly\n>> from by instead exposing the Read/Write mapping options as\n>> flags from the MappedFrameBuffer class itself.\n>>\n>> While here, introduce the header which is required for the\n>> uint8_t as part of the Plane.\n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> v2\n>> - Use fully scoped enum class\n>> - Swap MapMode and MapModes\n>> - s/mmap_flags/mmapFlags/\n>> - Remove and fix documentation regarding the modes\n>> - add LIBCAMERA_FLAGS_ENABLE_OPERATORS()\n>>\n>> .../libcamera/internal/mapped_framebuffer.h | 16 +++++++--\n>> src/android/jpeg/encoder_libjpeg.cpp | 2 +-\n>> src/android/jpeg/thumbnailer.cpp | 2 +-\n>> src/android/yuv/post_processor_yuv.cpp | 2 +-\n>> src/ipa/ipu3/ipu3.cpp | 2 +-\n>> src/ipa/raspberrypi/raspberrypi.cpp | 3 +-\n>> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++---\n>> test/mapped-buffer.cpp | 6 ++--\n>> 8 files changed, 52 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n>> index 41e587364260..e8234ebf76ff 100644\n>> --- a/include/libcamera/internal/mapped_framebuffer.h\n>> +++ b/include/libcamera/internal/mapped_framebuffer.h\n>> @@ -7,10 +7,11 @@\n>> #ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>> #define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>> \n>> -#include \n> \n> I trust that you've looked through the code base to ensure no other\n> unnecessary inclusion of sys/mman.h is left.\n\nNo ;-)\n\nBut now I have at least removed them all and added back only the ones\nthat break compilation:\n\nWhich leaves:\n\n\tmodified: src/android/camera_device.cpp\n\tmodified: src/android/jpeg/encoder_libjpeg.cpp\n\tmodified: src/ipa/ipu3/ipu3.cpp\n\tmodified: src/libcamera/framebuffer.cpp\n\tmodified: src/libcamera/ipa_module.cpp\n\tmodified: src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n\tmodified: src/libcamera/v4l2_videodevice.cpp\n\tmodified: src/v4l2/v4l2_camera_proxy.h\n\tmodified: src/v4l2/v4l2_compat.cpp\n\tmodified: src/v4l2/v4l2_compat_manager.h\n\nthat can potentially have mman.h removed, but those are not all\nnecessarily caused by this series, nor no longer required if they simply\npass compilation due to other components including the requirements for\nthem ...\n\nEspecially given that I can remove all those references to mman.h before\nmy MappedFrameBuffer patch - and have a successful build just the same.\n\n\nIt might be better to sweep through things like that with a tool like\ninclude-what-you-use as a more general task, or inclusion in checkstyle.py.\n\n\n>> +#include \n>> #include \n>> \n>> #include \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -44,9 +45,20 @@ private:\n>> class MappedFrameBuffer : public MappedBuffer\n>> {\n>> public:\n>> -\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n>> +\tenum class MapMode {\n>> +\t\tRead = 0 << 1,\n>> +\t\tWrite = 1 << 1,\n>> +\n> \n> I'd drop this blank line.\n\n\nDropped.\n\n\n> \n>> +\t\tReadWrite = Read | Write,\n>> +\t};\n>> +\n>> +\tusing MapModes = Flags;\n>> +\n>> +\tMappedFrameBuffer(const FrameBuffer *buffer, MapModes flags);\n> \n> I'll be annoying again I'm afraid, but \"MapModes flags\" bothers me :-/\n> Looking at the two example from File, we could have\n> \n> \tenum class MapModeFlag {\n> \t\t...\n> \t};\n> \n> \tusing MapMode = Flags;\n> \n> \tMappedFrameBuffer(const FrameBuffer *buffer, MapMode mode);\n> \n> or\n> \tenum class MapFlag {\n> \t\t...\n> \t};\n> \n> \tusing MapFlags = Flags;\n> \n> \tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n> \n> Other options are possible (and it may make sense to standardize naming\n> schemes once and for all), but naming the type \"modes\" and the argument\n> \"flags\" lacks consistency.\n\nYes, I think I introduced 'Modes' because Read/Write/ReadWrite are more\nmodes to me than flags.\n\nBut as they are still MapFlags, I'm going to pick your second option.\n\nI sort of dislike the enum being singular - because that's where the\nflag(*s*) are stored.\n\n> \n> Sorry for being nitpicking, it's the first usage of Flags<> outside of\n> the File class, so it's affected by the 0, 1, N generalization theorem.\n> I'm also thinking that MappedBuffer may graduate as a public API, hence\n> the particular attention.\n> \n>> };\n>> \n>> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode)\n>> +\n>> } /* namespace libcamera */\n>> \n>> #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n>> index 372018d2207f..34472e8da6a2 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n>> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n>> @@ -182,7 +182,7 @@ void EncoderLibJpeg::compressNV(Span frame)\n>> int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n>> \t\t\t Span exifData, unsigned int quality)\n>> {\n>> -\tMappedFrameBuffer frame(&source, PROT_READ);\n>> +\tMappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);\n>> \tif (!frame.isValid()) {\n>> \t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n>> \t\t\t\t << strerror(frame.error());\n>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n>> index 535e2cece914..66b4e696b107 100644\n>> --- a/src/android/jpeg/thumbnailer.cpp\n>> +++ b/src/android/jpeg/thumbnailer.cpp\n>> @@ -41,7 +41,7 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,\n>> \t\t\t\t const Size &targetSize,\n>> \t\t\t\t std::vector *destination)\n>> {\n>> -\tMappedFrameBuffer frame(&source, PROT_READ);\n>> +\tMappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);\n>> \tif (!frame.isValid()) {\n>> \t\tLOG(Thumbnailer, Error)\n>> \t\t\t<< \"Failed to map FrameBuffer : \"\n>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n>> index 509d4244d614..6d156c20646c 100644\n>> --- a/src/android/yuv/post_processor_yuv.cpp\n>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>> @@ -57,7 +57,7 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n>> \tif (!isValidBuffers(source, *destination))\n>> \t\treturn -EINVAL;\n>> \n>> -\tconst MappedFrameBuffer sourceMapped(&source, PROT_READ);\n>> +\tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapMode::Read);\n>> \tif (!sourceMapped.isValid()) {\n>> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n>> \t\treturn -EINVAL;\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 2647bf2f3b96..c8fc26525707 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -211,7 +211,7 @@ void IPAIPU3::mapBuffers(const std::vector &buffers)\n>> \tfor (const IPABuffer &buffer : buffers) {\n>> \t\tconst FrameBuffer fb(buffer.planes);\n>> \t\tbuffers_.emplace(buffer.id,\n>> -\t\t\t\t MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));\n>> +\t\t\t\t MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));\n>> \t}\n>> }\n>> \n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 76f67dd4567a..54e22d91084b 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -411,7 +411,8 @@ void IPARPi::mapBuffers(const std::vector &buffers)\n>> {\n>> \tfor (const IPABuffer &buffer : buffers) {\n>> \t\tconst FrameBuffer fb(buffer.planes);\n>> -\t\tbuffers_.emplace(buffer.id, MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));\n>> +\t\tbuffers_.emplace(buffer.id,\n>> +\t\t\t\t MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));\n>> \t}\n>> }\n>> \n>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n>> index 0e30fc542154..207c80b5aba2 100644\n>> --- a/src/libcamera/mapped_framebuffer.cpp\n>> +++ b/src/libcamera/mapped_framebuffer.cpp\n>> @@ -142,21 +142,45 @@ MappedBuffer::~MappedBuffer()\n>> * \\brief Map a FrameBuffer using the MappedBuffer interface\n>> */\n>> \n>> +/**\n>> + * \\enum MappedFrameBuffer::MapMode\n>> + * \\brief Specify the mapping mode for the FrameBuffer\n>> + * \\var MappedFrameBuffer::Read\n>> + * \\brief Create a Read-only mapping\n> \n> Maybe s/Read/read/ ? Same for write.\n> \n>> + * \\var MappedFrameBuffer::Write\n>> + * \\brief Create a Write-only mapping\n>> + * \\var MappedFrameBuffer::ReadWrite\n>> + * \\brief Create a mapping with both read and write capabilities\n> \n> Capabilities sound a bit weird to me in this context, I would have\n> written \"Create a mapping that can be both read and written\".\n\nAll updated/taken.\n\n\n> \n>> + */\n>> +\n>> +/**\n>> + * \\typedef MappedFrameBuffer::MapModes\n>> + * \\brief A bitwise combination of MappedFrameBuffer::MapMode values\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>> + * Construct an object to map a frame buffer for CPU access. The mapping can be\n>> + * made as Read only, Write only or support Read and Write operations by setting\n>> + * the MapMode flags accordingly.\n>> */\n>> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags)\n>> {\n>> \tmaps_.reserve(buffer->planes().size());\n>> \n>> +\tint mmapFlags = 0;\n>> +\n>> +\tif (flags & MapMode::Read)\n>> +\t\tmmapFlags |= PROT_READ;\n>> +\n>> +\tif (flags & MapMode::Write)\n>> +\t\tmmapFlags |= PROT_WRITE;\n>> +\n>> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n>> -\t\tvoid *address = mmap(nullptr, plane.length, flags,\n>> +\t\tvoid *address = mmap(nullptr, plane.length, mmapFlags,\n>> \t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n>> \t\tif (address == MAP_FAILED) {\n>> \t\t\terror_ = -errno;\n>> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n>> index a3d1511b74ce..63d20e7bc9c4 100644\n>> --- a/test/mapped-buffer.cpp\n>> +++ b/test/mapped-buffer.cpp\n>> @@ -71,7 +71,7 @@ protected:\n>> \t\tconst std::unique_ptr &buffer = allocator_->buffers(stream_).front();\n>> \t\tstd::vector maps;\n>> \n>> -\t\tMappedFrameBuffer map(buffer.get(), PROT_READ);\n>> +\t\tMappedFrameBuffer map(buffer.get(), MappedFrameBuffer::MapMode::Read);\n>> \t\tif (!map.isValid()) {\n>> \t\t\tcout << \"Failed to successfully map buffer\" << endl;\n>> \t\t\treturn TestFail;\n>> @@ -90,13 +90,13 @@ protected:\n>> \t\t}\n>> \n>> \t\t/* Test for multiple successful maps on the same buffer. */\n>> -\t\tMappedFrameBuffer write_map(buffer.get(), PROT_WRITE);\n>> +\t\tMappedFrameBuffer write_map(buffer.get(), MappedFrameBuffer::MapMode::Write);\n>> \t\tif (!write_map.isValid()) {\n>> \t\t\tcout << \"Failed to map write buffer\" << endl;\n>> \t\t\treturn TestFail;\n>> \t\t}\n>> \n>> -\t\tMappedFrameBuffer rw_map(buffer.get(), PROT_READ | PROT_WRITE);\n>> +\t\tMappedFrameBuffer rw_map(buffer.get(), MappedFrameBuffer::MapMode::ReadWrite);\n>> \t\tif (!rw_map.isValid()) {\n>> \t\t\tcout << \"Failed to map RW buffer\" << endl;\n>> \t\t\treturn TestFail;\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 63B2FBD87D\n\tfor ;\n\tMon, 9 Aug 2021 12:47:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D685668826;\n\tMon, 9 Aug 2021 14:47:46 +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 50A5060269\n\tfor ;\n\tMon, 9 Aug 2021 14:47:45 +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 BA60C466;\n\tMon, 9 Aug 2021 14:47: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=\"mhQi2BPF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628513264;\n\tbh=FKRjA0hRSOPYpwdRARF+EnlkaiCJ2p6yz+8iTgxuUws=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=mhQi2BPF866prREdNA+s0zOiTR3k1NiaLdlVqPZb14UyI2uCaMMUu5n01/w3rgGDn\n\tDxrhwBSobffyxmV40gFX6l3PAchja8kyRRBy4+bpJIRB+UllfgxWswvq6J6K3xz3Da\n\tT3x1Q6y4v6RJP08HDTNpEMBuC4jByUiHFm1LlUtA=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t<20210806135315.743684-1-kieran.bingham@ideasonboard.com>\n\t","Message-ID":"","Date":"Mon, 9 Aug 2021 13:47: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 v2] libcamera: MappedFrameBuffer: Use\n\ttyped Flags","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":18628,"web_url":"https://patchwork.libcamera.org/comment/18628/","msgid":"","date":"2021-08-09T14:45:39","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use\n\ttyped Flags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Aug 09, 2021 at 01:47:42PM +0100, Kieran Bingham wrote:\n> On 06/08/2021 16:19, Laurent Pinchart wrote:\n> > On Fri, Aug 06, 2021 at 02:53:15PM +0100, Kieran Bingham wrote:\n> >> Remove the need for callers to reference PROT_READ/PROT_WRITE directly\n> >> from by instead exposing the Read/Write mapping options as\n> >> flags from the MappedFrameBuffer class itself.\n> >>\n> >> While here, introduce the header which is required for the\n> >> uint8_t as part of the Plane.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >>\n> >> ---\n> >> v2\n> >> - Use fully scoped enum class\n> >> - Swap MapMode and MapModes\n> >> - s/mmap_flags/mmapFlags/\n> >> - Remove and fix documentation regarding the modes\n> >> - add LIBCAMERA_FLAGS_ENABLE_OPERATORS()\n> >>\n> >> .../libcamera/internal/mapped_framebuffer.h | 16 +++++++--\n> >> src/android/jpeg/encoder_libjpeg.cpp | 2 +-\n> >> src/android/jpeg/thumbnailer.cpp | 2 +-\n> >> src/android/yuv/post_processor_yuv.cpp | 2 +-\n> >> src/ipa/ipu3/ipu3.cpp | 2 +-\n> >> src/ipa/raspberrypi/raspberrypi.cpp | 3 +-\n> >> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++---\n> >> test/mapped-buffer.cpp | 6 ++--\n> >> 8 files changed, 52 insertions(+), 15 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> >> index 41e587364260..e8234ebf76ff 100644\n> >> --- a/include/libcamera/internal/mapped_framebuffer.h\n> >> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> >> @@ -7,10 +7,11 @@\n> >> #ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> >> #define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> >> \n> >> -#include \n> > \n> > I trust that you've looked through the code base to ensure no other\n> > unnecessary inclusion of sys/mman.h is left.\n> \n> No ;-)\n> \n> But now I have at least removed them all and added back only the ones\n> that break compilation:\n> \n> Which leaves:\n> \n> \tmodified: src/android/camera_device.cpp\n> \tmodified: src/android/jpeg/encoder_libjpeg.cpp\n> \tmodified: src/ipa/ipu3/ipu3.cpp\n> \tmodified: src/libcamera/framebuffer.cpp\n> \tmodified: src/libcamera/ipa_module.cpp\n> \tmodified: src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> \tmodified: src/libcamera/v4l2_videodevice.cpp\n> \tmodified: src/v4l2/v4l2_camera_proxy.h\n> \tmodified: src/v4l2/v4l2_compat.cpp\n> \tmodified: src/v4l2/v4l2_compat_manager.h\n> \n> that can potentially have mman.h removed, but those are not all\n> necessarily caused by this series, nor no longer required if they simply\n> pass compilation due to other components including the requirements for\n> them ...\n> \n> Especially given that I can remove all those references to mman.h before\n> my MappedFrameBuffer patch - and have a successful build just the same.\n> \n> \n> It might be better to sweep through things like that with a tool like\n> include-what-you-use as a more general task, or inclusion in checkstyle.py.\n> \n> \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -44,9 +45,20 @@ private:\n> >> class MappedFrameBuffer : public MappedBuffer\n> >> {\n> >> public:\n> >> -\tMappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> >> +\tenum class MapMode {\n> >> +\t\tRead = 0 << 1,\n> >> +\t\tWrite = 1 << 1,\n> >> +\n> > \n> > I'd drop this blank line.\n> \n> Dropped.\n> \n> >> +\t\tReadWrite = Read | Write,\n> >> +\t};\n> >> +\n> >> +\tusing MapModes = Flags;\n> >> +\n> >> +\tMappedFrameBuffer(const FrameBuffer *buffer, MapModes flags);\n> > \n> > I'll be annoying again I'm afraid, but \"MapModes flags\" bothers me :-/\n> > Looking at the two example from File, we could have\n> > \n> > \tenum class MapModeFlag {\n> > \t\t...\n> > \t};\n> > \n> > \tusing MapMode = Flags;\n> > \n> > \tMappedFrameBuffer(const FrameBuffer *buffer, MapMode mode);\n> > \n> > or\n> > \tenum class MapFlag {\n> > \t\t...\n> > \t};\n> > \n> > \tusing MapFlags = Flags;\n> > \n> > \tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n> > \n> > Other options are possible (and it may make sense to standardize naming\n> > schemes once and for all), but naming the type \"modes\" and the argument\n> > \"flags\" lacks consistency.\n> \n> Yes, I think I introduced 'Modes' because Read/Write/ReadWrite are more\n> modes to me than flags.\n> \n> But as they are still MapFlags, I'm going to pick your second option.\n> \n> I sort of dislike the enum being singular - because that's where the\n> flag(*s*) are stored.\n\nIt bothers me slightly too, but if I then think about how enums are used\nin the general case, I conclude that using the plural would be worse.\n\nenum Status {\n\tBeautiful,\n\tUgly,\n};\n\nStatus analyzeCode(...);\n\nvs.\n\nenum Statuses {\n\tBeautiful,\n\tUgly,\n};\n\nStatuses analyzeCode(...);\n\nI try to remind myself that an enum is a type that stores a single\nvalue, so the singular makes sense.\n\n> > Sorry for being nitpicking, it's the first usage of Flags<> outside of\n> > the File class, so it's affected by the 0, 1, N generalization theorem.\n> > I'm also thinking that MappedBuffer may graduate as a public API, hence\n> > the particular attention.\n> > \n> >> };\n> >> \n> >> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode)\n> >> +\n> >> } /* namespace libcamera */\n> >> \n> >> #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n> >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> >> index 372018d2207f..34472e8da6a2 100644\n> >> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> >> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> >> @@ -182,7 +182,7 @@ void EncoderLibJpeg::compressNV(Span frame)\n> >> int EncoderLibJpeg::encode(const FrameBuffer &source, Span dest,\n> >> \t\t\t Span exifData, unsigned int quality)\n> >> {\n> >> -\tMappedFrameBuffer frame(&source, PROT_READ);\n> >> +\tMappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);\n> >> \tif (!frame.isValid()) {\n> >> \t\tLOG(JPEG, Error) << \"Failed to map FrameBuffer : \"\n> >> \t\t\t\t << strerror(frame.error());\n> >> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> >> index 535e2cece914..66b4e696b107 100644\n> >> --- a/src/android/jpeg/thumbnailer.cpp\n> >> +++ b/src/android/jpeg/thumbnailer.cpp\n> >> @@ -41,7 +41,7 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,\n> >> \t\t\t\t const Size &targetSize,\n> >> \t\t\t\t std::vector *destination)\n> >> {\n> >> -\tMappedFrameBuffer frame(&source, PROT_READ);\n> >> +\tMappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);\n> >> \tif (!frame.isValid()) {\n> >> \t\tLOG(Thumbnailer, Error)\n> >> \t\t\t<< \"Failed to map FrameBuffer : \"\n> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> >> index 509d4244d614..6d156c20646c 100644\n> >> --- a/src/android/yuv/post_processor_yuv.cpp\n> >> +++ b/src/android/yuv/post_processor_yuv.cpp\n> >> @@ -57,7 +57,7 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> >> \tif (!isValidBuffers(source, *destination))\n> >> \t\treturn -EINVAL;\n> >> \n> >> -\tconst MappedFrameBuffer sourceMapped(&source, PROT_READ);\n> >> +\tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapMode::Read);\n> >> \tif (!sourceMapped.isValid()) {\n> >> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> >> \t\treturn -EINVAL;\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 2647bf2f3b96..c8fc26525707 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -211,7 +211,7 @@ void IPAIPU3::mapBuffers(const std::vector &buffers)\n> >> \tfor (const IPABuffer &buffer : buffers) {\n> >> \t\tconst FrameBuffer fb(buffer.planes);\n> >> \t\tbuffers_.emplace(buffer.id,\n> >> -\t\t\t\t MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));\n> >> +\t\t\t\t MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));\n> >> \t}\n> >> }\n> >> \n> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> index 76f67dd4567a..54e22d91084b 100644\n> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> @@ -411,7 +411,8 @@ void IPARPi::mapBuffers(const std::vector &buffers)\n> >> {\n> >> \tfor (const IPABuffer &buffer : buffers) {\n> >> \t\tconst FrameBuffer fb(buffer.planes);\n> >> -\t\tbuffers_.emplace(buffer.id, MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));\n> >> +\t\tbuffers_.emplace(buffer.id,\n> >> +\t\t\t\t MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));\n> >> \t}\n> >> }\n> >> \n> >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> >> index 0e30fc542154..207c80b5aba2 100644\n> >> --- a/src/libcamera/mapped_framebuffer.cpp\n> >> +++ b/src/libcamera/mapped_framebuffer.cpp\n> >> @@ -142,21 +142,45 @@ MappedBuffer::~MappedBuffer()\n> >> * \\brief Map a FrameBuffer using the MappedBuffer interface\n> >> */\n> >> \n> >> +/**\n> >> + * \\enum MappedFrameBuffer::MapMode\n> >> + * \\brief Specify the mapping mode for the FrameBuffer\n> >> + * \\var MappedFrameBuffer::Read\n> >> + * \\brief Create a Read-only mapping\n> > \n> > Maybe s/Read/read/ ? Same for write.\n> > \n> >> + * \\var MappedFrameBuffer::Write\n> >> + * \\brief Create a Write-only mapping\n> >> + * \\var MappedFrameBuffer::ReadWrite\n> >> + * \\brief Create a mapping with both read and write capabilities\n> > \n> > Capabilities sound a bit weird to me in this context, I would have\n> > written \"Create a mapping that can be both read and written\".\n> \n> All updated/taken.\n> \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\typedef MappedFrameBuffer::MapModes\n> >> + * \\brief A bitwise combination of MappedFrameBuffer::MapMode values\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> >> + * Construct an object to map a frame buffer for CPU access. The mapping can be\n> >> + * made as Read only, Write only or support Read and Write operations by setting\n> >> + * the MapMode flags accordingly.\n> >> */\n> >> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> >> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags)\n> >> {\n> >> \tmaps_.reserve(buffer->planes().size());\n> >> \n> >> +\tint mmapFlags = 0;\n> >> +\n> >> +\tif (flags & MapMode::Read)\n> >> +\t\tmmapFlags |= PROT_READ;\n> >> +\n> >> +\tif (flags & MapMode::Write)\n> >> +\t\tmmapFlags |= PROT_WRITE;\n> >> +\n> >> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> >> -\t\tvoid *address = mmap(nullptr, plane.length, flags,\n> >> +\t\tvoid *address = mmap(nullptr, plane.length, mmapFlags,\n> >> \t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n> >> \t\tif (address == MAP_FAILED) {\n> >> \t\t\terror_ = -errno;\n> >> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> >> index a3d1511b74ce..63d20e7bc9c4 100644\n> >> --- a/test/mapped-buffer.cpp\n> >> +++ b/test/mapped-buffer.cpp\n> >> @@ -71,7 +71,7 @@ protected:\n> >> \t\tconst std::unique_ptr &buffer = allocator_->buffers(stream_).front();\n> >> \t\tstd::vector maps;\n> >> \n> >> -\t\tMappedFrameBuffer map(buffer.get(), PROT_READ);\n> >> +\t\tMappedFrameBuffer map(buffer.get(), MappedFrameBuffer::MapMode::Read);\n> >> \t\tif (!map.isValid()) {\n> >> \t\t\tcout << \"Failed to successfully map buffer\" << endl;\n> >> \t\t\treturn TestFail;\n> >> @@ -90,13 +90,13 @@ protected:\n> >> \t\t}\n> >> \n> >> \t\t/* Test for multiple successful maps on the same buffer. */\n> >> -\t\tMappedFrameBuffer write_map(buffer.get(), PROT_WRITE);\n> >> +\t\tMappedFrameBuffer write_map(buffer.get(), MappedFrameBuffer::MapMode::Write);\n> >> \t\tif (!write_map.isValid()) {\n> >> \t\t\tcout << \"Failed to map write buffer\" << endl;\n> >> \t\t\treturn TestFail;\n> >> \t\t}\n> >> \n> >> -\t\tMappedFrameBuffer rw_map(buffer.get(), PROT_READ | PROT_WRITE);\n> >> +\t\tMappedFrameBuffer rw_map(buffer.get(), MappedFrameBuffer::MapMode::ReadWrite);\n> >> \t\tif (!rw_map.isValid()) {\n> >> \t\t\tcout << \"Failed to map RW buffer\" << endl;\n> >> \t\t\treturn TestFail;","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 C72D6BD87D\n\tfor ;\n\tMon, 9 Aug 2021 14:45:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40C1F68826;\n\tMon, 9 Aug 2021 16:45:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2FC4060269\n\tfor ;\n\tMon, 9 Aug 2021 16:45:41 +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 A08B1DD;\n\tMon, 9 Aug 2021 16:45:40 +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=\"wgi22SSY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628520340;\n\tbh=mpuQIL93zSCgrTbKb28gCO4in5cbKqYtnEQWatMFVG8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wgi22SSYya/SEkme0704/4WUpVxino+o/J3KdH1UX8MBSHX68rFdfcUbT6+2TPJ8+\n\teYoFXd+V2KRuQxV/yORhIPLhakIYa+Ya5pghw77hR2Z7vHkaRNUxz+bU6p0lZ8UCgx\n\tP76auh5bgUYoA+0vUfixdqziLdvsodG04hb97yW0=","Date":"Mon, 9 Aug 2021 17:45:39 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t<20210806135315.743684-1-kieran.bingham@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use\n\ttyped Flags","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\" "}}]