| Message ID | 20210806135315.743684-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:53:15PM +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() > > .../libcamera/internal/mapped_framebuffer.h | 16 +++++++-- > 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 | 3 +- > src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++--- > test/mapped-buffer.cpp | 6 ++-- > 8 files changed, 52 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > index 41e587364260..e8234ebf76ff 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> I trust that you've looked through the code base to ensure no other unnecessary inclusion of sys/mman.h is left. > +#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,20 @@ private: > class MappedFrameBuffer : public MappedBuffer > { > public: > - MappedFrameBuffer(const FrameBuffer *buffer, int flags); > + enum class MapMode { > + Read = 0 << 1, > + Write = 1 << 1, > + I'd drop this blank line. > + ReadWrite = Read | Write, > + }; > + > + using MapModes = Flags<MapMode>; > + > + MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags); I'll be annoying again I'm afraid, but "MapModes flags" bothers me :-/ Looking at the two example from File, we could have enum class MapModeFlag { ... }; using MapMode = Flags<MapModeFlag>; MappedFrameBuffer(const FrameBuffer *buffer, MapMode mode); or enum class MapFlag { ... }; using MapFlags = Flags<MapFlag>; MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); Other options are possible (and it may make sense to standardize naming schemes once and for all), but naming the type "modes" and the argument "flags" lacks consistency. Sorry for being nitpicking, it's the first usage of Flags<> outside of the File class, so it's affected by the 0, 1, N generalization theorem. I'm also thinking that MappedBuffer may graduate as a public API, hence the particular attention. > }; > > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode) > + > } /* 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..34472e8da6a2 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::MapMode::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..66b4e696b107 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::MapMode::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..6d156c20646c 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::MapMode::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..c8fc26525707 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::MapMode::ReadWrite)); > } > } > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 76f67dd4567a..54e22d91084b 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::MapMode::ReadWrite)); > } > } > > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > index 0e30fc542154..207c80b5aba2 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::MapMode > + * \brief Specify the mapping mode for the FrameBuffer > + * \var MappedFrameBuffer::Read > + * \brief Create a Read-only mapping Maybe s/Read/read/ ? Same for write. > + * \var MappedFrameBuffer::Write > + * \brief Create a Write-only mapping > + * \var MappedFrameBuffer::ReadWrite > + * \brief Create a mapping with both read and write capabilities Capabilities sound a bit weird to me in this context, I would have written "Create a mapping that can be both read and written". > + */ > + > +/** > + * \typedef MappedFrameBuffer::MapModes > + * \brief A bitwise combination of MappedFrameBuffer::MapMode 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 MapMode flags accordingly. > */ > -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags) > +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags) > { > maps_.reserve(buffer->planes().size()); > > + int mmapFlags = 0; > + > + if (flags & MapMode::Read) > + mmapFlags |= PROT_READ; > + > + if (flags & MapMode::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..63d20e7bc9c4 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::MapMode::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::MapMode::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::MapMode::ReadWrite); > if (!rw_map.isValid()) { > cout << "Failed to map RW buffer" << endl; > return TestFail;
Hi Laurent, On 06/08/2021 16:19, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Aug 06, 2021 at 02:53:15PM +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() >> >> .../libcamera/internal/mapped_framebuffer.h | 16 +++++++-- >> 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 | 3 +- >> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++--- >> test/mapped-buffer.cpp | 6 ++-- >> 8 files changed, 52 insertions(+), 15 deletions(-) >> >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h >> index 41e587364260..e8234ebf76ff 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> > > I trust that you've looked through the code base to ensure no other > unnecessary inclusion of sys/mman.h is left. No ;-) But now I have at least removed them all and added back only the ones that break compilation: Which leaves: modified: src/android/camera_device.cpp modified: src/android/jpeg/encoder_libjpeg.cpp modified: src/ipa/ipu3/ipu3.cpp modified: src/libcamera/framebuffer.cpp modified: src/libcamera/ipa_module.cpp modified: src/libcamera/pipeline/raspberrypi/raspberrypi.cpp modified: src/libcamera/v4l2_videodevice.cpp modified: src/v4l2/v4l2_camera_proxy.h modified: src/v4l2/v4l2_compat.cpp modified: src/v4l2/v4l2_compat_manager.h that can potentially have mman.h removed, but those are not all necessarily caused by this series, nor no longer required if they simply pass compilation due to other components including the requirements for them ... Especially given that I can remove all those references to mman.h before my MappedFrameBuffer patch - and have a successful build just the same. It might be better to sweep through things like that with a tool like include-what-you-use as a more general task, or inclusion in checkstyle.py. >> +#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,20 @@ private: >> class MappedFrameBuffer : public MappedBuffer >> { >> public: >> - MappedFrameBuffer(const FrameBuffer *buffer, int flags); >> + enum class MapMode { >> + Read = 0 << 1, >> + Write = 1 << 1, >> + > > I'd drop this blank line. Dropped. > >> + ReadWrite = Read | Write, >> + }; >> + >> + using MapModes = Flags<MapMode>; >> + >> + MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags); > > I'll be annoying again I'm afraid, but "MapModes flags" bothers me :-/ > Looking at the two example from File, we could have > > enum class MapModeFlag { > ... > }; > > using MapMode = Flags<MapModeFlag>; > > MappedFrameBuffer(const FrameBuffer *buffer, MapMode mode); > > or > enum class MapFlag { > ... > }; > > using MapFlags = Flags<MapFlag>; > > MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); > > Other options are possible (and it may make sense to standardize naming > schemes once and for all), but naming the type "modes" and the argument > "flags" lacks consistency. Yes, I think I introduced 'Modes' because Read/Write/ReadWrite are more modes to me than flags. But as they are still MapFlags, I'm going to pick your second option. I sort of dislike the enum being singular - because that's where the flag(*s*) are stored. > > Sorry for being nitpicking, it's the first usage of Flags<> outside of > the File class, so it's affected by the 0, 1, N generalization theorem. > I'm also thinking that MappedBuffer may graduate as a public API, hence > the particular attention. > >> }; >> >> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode) >> + >> } /* 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..34472e8da6a2 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::MapMode::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..66b4e696b107 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::MapMode::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..6d156c20646c 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::MapMode::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..c8fc26525707 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::MapMode::ReadWrite)); >> } >> } >> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp >> index 76f67dd4567a..54e22d91084b 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::MapMode::ReadWrite)); >> } >> } >> >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp >> index 0e30fc542154..207c80b5aba2 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::MapMode >> + * \brief Specify the mapping mode for the FrameBuffer >> + * \var MappedFrameBuffer::Read >> + * \brief Create a Read-only mapping > > Maybe s/Read/read/ ? Same for write. > >> + * \var MappedFrameBuffer::Write >> + * \brief Create a Write-only mapping >> + * \var MappedFrameBuffer::ReadWrite >> + * \brief Create a mapping with both read and write capabilities > > Capabilities sound a bit weird to me in this context, I would have > written "Create a mapping that can be both read and written". All updated/taken. > >> + */ >> + >> +/** >> + * \typedef MappedFrameBuffer::MapModes >> + * \brief A bitwise combination of MappedFrameBuffer::MapMode 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 MapMode flags accordingly. >> */ >> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags) >> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags) >> { >> maps_.reserve(buffer->planes().size()); >> >> + int mmapFlags = 0; >> + >> + if (flags & MapMode::Read) >> + mmapFlags |= PROT_READ; >> + >> + if (flags & MapMode::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..63d20e7bc9c4 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::MapMode::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::MapMode::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::MapMode::ReadWrite); >> if (!rw_map.isValid()) { >> cout << "Failed to map RW buffer" << endl; >> return TestFail; >
Hi Kieran, On Mon, Aug 09, 2021 at 01:47:42PM +0100, Kieran Bingham wrote: > On 06/08/2021 16:19, Laurent Pinchart wrote: > > On Fri, Aug 06, 2021 at 02:53:15PM +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() > >> > >> .../libcamera/internal/mapped_framebuffer.h | 16 +++++++-- > >> 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 | 3 +- > >> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++--- > >> test/mapped-buffer.cpp | 6 ++-- > >> 8 files changed, 52 insertions(+), 15 deletions(-) > >> > >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > >> index 41e587364260..e8234ebf76ff 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> > > > > I trust that you've looked through the code base to ensure no other > > unnecessary inclusion of sys/mman.h is left. > > No ;-) > > But now I have at least removed them all and added back only the ones > that break compilation: > > Which leaves: > > modified: src/android/camera_device.cpp > modified: src/android/jpeg/encoder_libjpeg.cpp > modified: src/ipa/ipu3/ipu3.cpp > modified: src/libcamera/framebuffer.cpp > modified: src/libcamera/ipa_module.cpp > modified: src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > modified: src/libcamera/v4l2_videodevice.cpp > modified: src/v4l2/v4l2_camera_proxy.h > modified: src/v4l2/v4l2_compat.cpp > modified: src/v4l2/v4l2_compat_manager.h > > that can potentially have mman.h removed, but those are not all > necessarily caused by this series, nor no longer required if they simply > pass compilation due to other components including the requirements for > them ... > > Especially given that I can remove all those references to mman.h before > my MappedFrameBuffer patch - and have a successful build just the same. > > > It might be better to sweep through things like that with a tool like > include-what-you-use as a more general task, or inclusion in checkstyle.py. > > > >> +#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,20 @@ private: > >> class MappedFrameBuffer : public MappedBuffer > >> { > >> public: > >> - MappedFrameBuffer(const FrameBuffer *buffer, int flags); > >> + enum class MapMode { > >> + Read = 0 << 1, > >> + Write = 1 << 1, > >> + > > > > I'd drop this blank line. > > Dropped. > > >> + ReadWrite = Read | Write, > >> + }; > >> + > >> + using MapModes = Flags<MapMode>; > >> + > >> + MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags); > > > > I'll be annoying again I'm afraid, but "MapModes flags" bothers me :-/ > > Looking at the two example from File, we could have > > > > enum class MapModeFlag { > > ... > > }; > > > > using MapMode = Flags<MapModeFlag>; > > > > MappedFrameBuffer(const FrameBuffer *buffer, MapMode mode); > > > > or > > enum class MapFlag { > > ... > > }; > > > > using MapFlags = Flags<MapFlag>; > > > > MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); > > > > Other options are possible (and it may make sense to standardize naming > > schemes once and for all), but naming the type "modes" and the argument > > "flags" lacks consistency. > > Yes, I think I introduced 'Modes' because Read/Write/ReadWrite are more > modes to me than flags. > > But as they are still MapFlags, I'm going to pick your second option. > > I sort of dislike the enum being singular - because that's where the > flag(*s*) are stored. It bothers me slightly too, but if I then think about how enums are used in the general case, I conclude that using the plural would be worse. enum Status { Beautiful, Ugly, }; Status analyzeCode(...); vs. enum Statuses { Beautiful, Ugly, }; Statuses analyzeCode(...); I try to remind myself that an enum is a type that stores a single value, so the singular makes sense. > > Sorry for being nitpicking, it's the first usage of Flags<> outside of > > the File class, so it's affected by the 0, 1, N generalization theorem. > > I'm also thinking that MappedBuffer may graduate as a public API, hence > > the particular attention. > > > >> }; > >> > >> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode) > >> + > >> } /* 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..34472e8da6a2 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::MapMode::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..66b4e696b107 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::MapMode::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..6d156c20646c 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::MapMode::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..c8fc26525707 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::MapMode::ReadWrite)); > >> } > >> } > >> > >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > >> index 76f67dd4567a..54e22d91084b 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::MapMode::ReadWrite)); > >> } > >> } > >> > >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > >> index 0e30fc542154..207c80b5aba2 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::MapMode > >> + * \brief Specify the mapping mode for the FrameBuffer > >> + * \var MappedFrameBuffer::Read > >> + * \brief Create a Read-only mapping > > > > Maybe s/Read/read/ ? Same for write. > > > >> + * \var MappedFrameBuffer::Write > >> + * \brief Create a Write-only mapping > >> + * \var MappedFrameBuffer::ReadWrite > >> + * \brief Create a mapping with both read and write capabilities > > > > Capabilities sound a bit weird to me in this context, I would have > > written "Create a mapping that can be both read and written". > > All updated/taken. > > >> + */ > >> + > >> +/** > >> + * \typedef MappedFrameBuffer::MapModes > >> + * \brief A bitwise combination of MappedFrameBuffer::MapMode 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 MapMode flags accordingly. > >> */ > >> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags) > >> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags) > >> { > >> maps_.reserve(buffer->planes().size()); > >> > >> + int mmapFlags = 0; > >> + > >> + if (flags & MapMode::Read) > >> + mmapFlags |= PROT_READ; > >> + > >> + if (flags & MapMode::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..63d20e7bc9c4 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::MapMode::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::MapMode::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::MapMode::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..e8234ebf76ff 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,20 @@ private: class MappedFrameBuffer : public MappedBuffer { public: - MappedFrameBuffer(const FrameBuffer *buffer, int flags); + enum class MapMode { + Read = 0 << 1, + Write = 1 << 1, + + ReadWrite = Read | Write, + }; + + using MapModes = Flags<MapMode>; + + MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags); }; +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode) + } /* 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..34472e8da6a2 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::MapMode::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..66b4e696b107 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::MapMode::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..6d156c20646c 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::MapMode::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..c8fc26525707 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::MapMode::ReadWrite)); } } diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 76f67dd4567a..54e22d91084b 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::MapMode::ReadWrite)); } } diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp index 0e30fc542154..207c80b5aba2 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::MapMode + * \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::MapModes + * \brief A bitwise combination of MappedFrameBuffer::MapMode 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 MapMode flags accordingly. */ -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags) +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags) { maps_.reserve(buffer->planes().size()); + int mmapFlags = 0; + + if (flags & MapMode::Read) + mmapFlags |= PROT_READ; + + if (flags & MapMode::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..63d20e7bc9c4 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::MapMode::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::MapMode::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::MapMode::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() .../libcamera/internal/mapped_framebuffer.h | 16 +++++++-- 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 | 3 +- src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++--- test/mapped-buffer.cpp | 6 ++-- 8 files changed, 52 insertions(+), 15 deletions(-)