| Message ID | 20210809132929.1824114-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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, ? Otherwise, Read == 0, Write == 1 and ReadWrite == 1. With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + 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;
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@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@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; >
Hi Kieran, thank you for the patch. On Tue, Aug 10, 2021 at 1:26 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > 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@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@ideasonboard.com> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > 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; > >
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, + 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;
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@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(-)