[libcamera-devel] [PATCH v3 3/3] libcamera: MappedFrameBuffer: Use typed Flags<MapFlags>
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 9 18:26:43 CEST 2021
Hi Laurent,
On 09/08/2021 15:20, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Mon, Aug 09, 2021 at 02:29:29PM +0100, Kieran Bingham wrote:
>> Remove the need for callers to reference PROT_READ/PROT_WRITE directly
>> from <sys/mman.h> by instead exposing the Read/Write mapping options as
>> flags from the MappedFrameBuffer class itself.
>>
>> While here, introduce the <stdint.h> header which is required for the
>> uint8_t as part of the Plane.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> ---
>> v2
>> - Use fully scoped enum class
>> - Swap MapMode and MapModes
>> - s/mmap_flags/mmapFlags/
>> - Remove and fix documentation regarding the modes
>> - add LIBCAMERA_FLAGS_ENABLE_OPERATORS()
>>
>> v3:
>> - Remove no longer necessary sys/mman.h inclusions throughout
>> - fix documentation
>> - rename MapMode as MapFlag and MapModes as MapFlags
>> ---
>> .../libcamera/internal/mapped_framebuffer.h | 15 ++++++--
>> src/android/jpeg/encoder_libjpeg.cpp | 3 +-
>> src/android/jpeg/thumbnailer.cpp | 2 +-
>> src/android/yuv/post_processor_yuv.cpp | 2 +-
>> src/ipa/ipu3/ipu3.cpp | 3 +-
>> src/ipa/raspberrypi/raspberrypi.cpp | 3 +-
>> src/libcamera/framebuffer.cpp | 1 -
>> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++---
>> test/mapped-buffer.cpp | 6 ++--
>> 9 files changed, 51 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
>> index 41e587364260..68c8a43c35dc 100644
>> --- a/include/libcamera/internal/mapped_framebuffer.h
>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>> @@ -7,10 +7,11 @@
>> #ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>> #define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>>
>> -#include <sys/mman.h>
>> +#include <stdint.h>
>> #include <vector>
>>
>> #include <libcamera/base/class.h>
>> +#include <libcamera/base/flags.h>
>> #include <libcamera/base/span.h>
>>
>> #include <libcamera/framebuffer.h>
>> @@ -44,9 +45,19 @@ private:
>> class MappedFrameBuffer : public MappedBuffer
>> {
>> public:
>> - MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>> + enum class MapFlag {
>> + Read = 0 << 1,
>> + Write = 1 << 1,
>
> Did you mean
>
> Read = 1 << 0,
> Write = 1 << 1,
>
Yes, of course.
Maybe we should have my favourite BIT() macro somewhere ...
> ? Otherwise, Read == 0, Write == 1 and ReadWrite == 1.
>
> With this fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Fixed up, thanks.
>
>> + ReadWrite = Read | Write,
>> + };
>> +
>> + using MapFlags = Flags<MapFlag>;
>> +
>> + MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
>> };
>>
>> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)
>> +
>> } /* namespace libcamera */
>>
>> #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
>> index 372018d2207f..a7a636011045 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
>> @@ -12,7 +12,6 @@
>> #include <iostream>
>> #include <sstream>
>> #include <string.h>
>> -#include <sys/mman.h>
>> #include <unistd.h>
>> #include <vector>
>>
>> @@ -182,7 +181,7 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame)
>> int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
>> Span<const uint8_t> exifData, unsigned int quality)
>> {
>> - MappedFrameBuffer frame(&source, PROT_READ);
>> + MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
>> if (!frame.isValid()) {
>> LOG(JPEG, Error) << "Failed to map FrameBuffer : "
>> << strerror(frame.error());
>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
>> index 535e2cece914..79d83926af5c 100644
>> --- a/src/android/jpeg/thumbnailer.cpp
>> +++ b/src/android/jpeg/thumbnailer.cpp
>> @@ -41,7 +41,7 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,
>> const Size &targetSize,
>> std::vector<unsigned char> *destination)
>> {
>> - MappedFrameBuffer frame(&source, PROT_READ);
>> + MappedFrameBuffer frame(&source, MappedFrameBuffer::MapFlag::Read);
>> if (!frame.isValid()) {
>> LOG(Thumbnailer, Error)
>> << "Failed to map FrameBuffer : "
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index 509d4244d614..3e793a580377 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -57,7 +57,7 @@ int PostProcessorYuv::process(const FrameBuffer &source,
>> if (!isValidBuffers(source, *destination))
>> return -EINVAL;
>>
>> - const MappedFrameBuffer sourceMapped(&source, PROT_READ);
>> + const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
>> if (!sourceMapped.isValid()) {
>> LOG(YUV, Error) << "Failed to mmap camera frame buffer";
>> return -EINVAL;
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 2647bf2f3b96..10093acdad30 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -6,7 +6,6 @@
>> */
>>
>> #include <stdint.h>
>> -#include <sys/mman.h>
>>
>> #include <linux/intel-ipu3.h>
>> #include <linux/v4l2-controls.h>
>> @@ -211,7 +210,7 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>> for (const IPABuffer &buffer : buffers) {
>> const FrameBuffer fb(buffer.planes);
>> buffers_.emplace(buffer.id,
>> - MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));
>> + MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite));
>> }
>> }
>>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 76f67dd4567a..592e2fe41617 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -411,7 +411,8 @@ void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
>> {
>> for (const IPABuffer &buffer : buffers) {
>> const FrameBuffer fb(buffer.planes);
>> - buffers_.emplace(buffer.id, MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));
>> + buffers_.emplace(buffer.id,
>> + MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite));
>> }
>> }
>>
>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
>> index 3d98affb20f9..2223e24321ca 100644
>> --- a/src/libcamera/framebuffer.cpp
>> +++ b/src/libcamera/framebuffer.cpp
>> @@ -10,7 +10,6 @@
>>
>> #include <errno.h>
>> #include <string.h>
>> -#include <sys/mman.h>
>> #include <unistd.h>
>>
>> #include <libcamera/base/log.h>
>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
>> index 0e30fc542154..5a70c18147f2 100644
>> --- a/src/libcamera/mapped_framebuffer.cpp
>> +++ b/src/libcamera/mapped_framebuffer.cpp
>> @@ -142,21 +142,45 @@ MappedBuffer::~MappedBuffer()
>> * \brief Map a FrameBuffer using the MappedBuffer interface
>> */
>>
>> +/**
>> + * \enum MappedFrameBuffer::MapFlag
>> + * \brief Specify the mapping mode for the FrameBuffer
>> + * \var MappedFrameBuffer::Read
>> + * \brief Create a read-only mapping
>> + * \var MappedFrameBuffer::Write
>> + * \brief Create a write-only mapping
>> + * \var MappedFrameBuffer::ReadWrite
>> + * \brief Create a mapping that can be both read and written
>> + */
>> +
>> +/**
>> + * \typedef MappedFrameBuffer::MapFlags
>> + * \brief A bitwise combination of MappedFrameBuffer::MapFlag values
>> + */
>> +
>> /**
>> * \brief Map all planes of a FrameBuffer
>> * \param[in] buffer FrameBuffer to be mapped
>> * \param[in] flags Protection flags to apply to map
>> *
>> - * Construct an object to map a frame buffer for CPU access.
>> - * The flags are passed directly to mmap and should be either PROT_READ,
>> - * PROT_WRITE, or a bitwise-or combination of both.
>> + * Construct an object to map a frame buffer for CPU access. The mapping can be
>> + * made as Read only, Write only or support Read and Write operations by setting
>> + * the MapFlag flags accordingly.
>> */
>> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
>> {
>> maps_.reserve(buffer->planes().size());
>>
>> + int mmapFlags = 0;
>> +
>> + if (flags & MapFlag::Read)
>> + mmapFlags |= PROT_READ;
>> +
>> + if (flags & MapFlag::Write)
>> + mmapFlags |= PROT_WRITE;
>> +
>> for (const FrameBuffer::Plane &plane : buffer->planes()) {
>> - void *address = mmap(nullptr, plane.length, flags,
>> + void *address = mmap(nullptr, plane.length, mmapFlags,
>> MAP_SHARED, plane.fd.fd(), 0);
>> if (address == MAP_FAILED) {
>> error_ = -errno;
>> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
>> index a3d1511b74ce..97571945cbca 100644
>> --- a/test/mapped-buffer.cpp
>> +++ b/test/mapped-buffer.cpp
>> @@ -71,7 +71,7 @@ protected:
>> const std::unique_ptr<FrameBuffer> &buffer = allocator_->buffers(stream_).front();
>> std::vector<MappedBuffer> maps;
>>
>> - MappedFrameBuffer map(buffer.get(), PROT_READ);
>> + MappedFrameBuffer map(buffer.get(), MappedFrameBuffer::MapFlag::Read);
>> if (!map.isValid()) {
>> cout << "Failed to successfully map buffer" << endl;
>> return TestFail;
>> @@ -90,13 +90,13 @@ protected:
>> }
>>
>> /* Test for multiple successful maps on the same buffer. */
>> - MappedFrameBuffer write_map(buffer.get(), PROT_WRITE);
>> + MappedFrameBuffer write_map(buffer.get(), MappedFrameBuffer::MapFlag::Write);
>> if (!write_map.isValid()) {
>> cout << "Failed to map write buffer" << endl;
>> return TestFail;
>> }
>>
>> - MappedFrameBuffer rw_map(buffer.get(), PROT_READ | PROT_WRITE);
>> + MappedFrameBuffer rw_map(buffer.get(), MappedFrameBuffer::MapFlag::ReadWrite);
>> if (!rw_map.isValid()) {
>> cout << "Failed to map RW buffer" << endl;
>> return TestFail;
>
More information about the libcamera-devel
mailing list