| Message ID | 20210806131511.712042-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Aug 06, 2021 at 02:15:11PM +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 > int8_t as part of the Plane. s/int8_t/uint8_t/ > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../libcamera/internal/mapped_framebuffer.h | 14 ++++++++-- > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > src/android/jpeg/thumbnailer.cpp | 2 +- > src/android/yuv/post_processor_yuv.cpp | 2 +- > src/ipa/ipu3/ipu3.cpp | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 2 +- > src/libcamera/mapped_framebuffer.cpp | 28 +++++++++++++++++-- > test/mapped-buffer.cpp | 6 ++-- > 8 files changed, 46 insertions(+), 12 deletions(-) > > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > index 41e587364260..24bd2c50cd27 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,7 +45,16 @@ private: > class MappedFrameBuffer : public MappedBuffer > { > public: > - MappedFrameBuffer(const FrameBuffer *buffer, int flags); > + enum MapModes { To get the full benefits from Flags<>, this should be a scoped enum (enum class). Don't forget LIBCAMERA_FLAGS_ENABLE_OPERATORS(). Yes, it will be a bit more verbose in the caller, but I think it's worth it to ensure type safety. > + Read = 0 << 1, > + Write = 1 << 1, > + > + ReadWrite = Read | Write, > + }; > + > + using MapMode = Flags<MapModes>; The File class uses MapFlags = Flags<MapFlag> and OpenMode = Flags<OpenModeFlag>. Now we have yet another naming scheme :-) I think it would be nice to try and be consistent. > + > + MappedFrameBuffer(const FrameBuffer *buffer, MapMode flags); > }; > > } /* namespace libcamera */ > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index 372018d2207f..c37611f6b6cd 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -182,7 +182,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::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..17196b2dd257 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::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..6f599a82a3a0 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::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..6028638c314c 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -211,7 +211,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::ReadWrite)); > } > } > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 76f67dd4567a..b905a9ca1dbf 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -411,7 +411,7 @@ 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::ReadWrite)); > } > } > > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > index 0e30fc542154..65bdf9249133 100644 > --- a/src/libcamera/mapped_framebuffer.cpp > +++ b/src/libcamera/mapped_framebuffer.cpp > @@ -142,6 +142,22 @@ MappedBuffer::~MappedBuffer() > * \brief Map a FrameBuffer using the MappedBuffer interface > */ > > +/** > + * \enum MappedFrameBuffer::MapModes > + * \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 with both read and write capabilities > + */ > + > +/** > + * \typedef MappedFrameBuffer::MapMode > + * \brief A bitwise combination of MappedFrameBuffer::MapModes values > + */ > + > /** > * \brief Map all planes of a FrameBuffer > * \param[in] buffer FrameBuffer to be mapped > @@ -151,12 +167,20 @@ MappedBuffer::~MappedBuffer() > * The flags are passed directly to mmap and should be either PROT_READ, > * PROT_WRITE, or a bitwise-or combination of both. The documentation should be updated. > */ > -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags) > +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapMode flags) > { > maps_.reserve(buffer->planes().size()); > > + int mmap_flags = 0; s/mmap_flags/mmapFlags/ > + > + if (flags & Read) > + mmap_flags |= PROT_READ; > + > + if (flags & Write) > + mmap_flags |= PROT_WRITE; > + > for (const FrameBuffer::Plane &plane : buffer->planes()) { > - void *address = mmap(nullptr, plane.length, flags, > + void *address = mmap(nullptr, plane.length, mmap_flags, > 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..ce75d9a9dbbd 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::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::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::ReadWrite); > if (!rw_map.isValid()) { > cout << "Failed to map RW buffer" << endl; > return TestFail;
diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h index 41e587364260..24bd2c50cd27 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,7 +45,16 @@ private: class MappedFrameBuffer : public MappedBuffer { public: - MappedFrameBuffer(const FrameBuffer *buffer, int flags); + enum MapModes { + Read = 0 << 1, + Write = 1 << 1, + + ReadWrite = Read | Write, + }; + + using MapMode = Flags<MapModes>; + + MappedFrameBuffer(const FrameBuffer *buffer, MapMode flags); }; } /* namespace libcamera */ diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index 372018d2207f..c37611f6b6cd 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -182,7 +182,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::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..17196b2dd257 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::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..6f599a82a3a0 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::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..6028638c314c 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -211,7 +211,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::ReadWrite)); } } diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 76f67dd4567a..b905a9ca1dbf 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -411,7 +411,7 @@ 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::ReadWrite)); } } diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp index 0e30fc542154..65bdf9249133 100644 --- a/src/libcamera/mapped_framebuffer.cpp +++ b/src/libcamera/mapped_framebuffer.cpp @@ -142,6 +142,22 @@ MappedBuffer::~MappedBuffer() * \brief Map a FrameBuffer using the MappedBuffer interface */ +/** + * \enum MappedFrameBuffer::MapModes + * \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 with both read and write capabilities + */ + +/** + * \typedef MappedFrameBuffer::MapMode + * \brief A bitwise combination of MappedFrameBuffer::MapModes values + */ + /** * \brief Map all planes of a FrameBuffer * \param[in] buffer FrameBuffer to be mapped @@ -151,12 +167,20 @@ MappedBuffer::~MappedBuffer() * The flags are passed directly to mmap and should be either PROT_READ, * PROT_WRITE, or a bitwise-or combination of both. */ -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags) +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapMode flags) { maps_.reserve(buffer->planes().size()); + int mmap_flags = 0; + + if (flags & Read) + mmap_flags |= PROT_READ; + + if (flags & Write) + mmap_flags |= PROT_WRITE; + for (const FrameBuffer::Plane &plane : buffer->planes()) { - void *address = mmap(nullptr, plane.length, flags, + void *address = mmap(nullptr, plane.length, mmap_flags, 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..ce75d9a9dbbd 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::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::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::ReadWrite); if (!rw_map.isValid()) { cout << "Failed to map RW buffer" << endl; return TestFail;
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 int8_t as part of the Plane. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- .../libcamera/internal/mapped_framebuffer.h | 14 ++++++++-- src/android/jpeg/encoder_libjpeg.cpp | 2 +- src/android/jpeg/thumbnailer.cpp | 2 +- src/android/yuv/post_processor_yuv.cpp | 2 +- src/ipa/ipu3/ipu3.cpp | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 2 +- src/libcamera/mapped_framebuffer.cpp | 28 +++++++++++++++++-- test/mapped-buffer.cpp | 6 ++-- 8 files changed, 46 insertions(+), 12 deletions(-)