| Message ID | 20210826112539.170694-3-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Thu, Aug 26, 2021 at 08:25:32PM +0900, Hirokazu Honda wrote: > MappedBuffer::maps() returns std::vector<MappedBuffer::Plane>. > Plane has the address, but the address points the beginning of the > buffer containing the plane. > This makes the Plane point the beginning of the plane. So > MappedBuffer::maps()[i].data() returns the address of i-th plane. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../libcamera/internal/mapped_framebuffer.h | 4 +- > src/android/mm/generic_camera_buffer.cpp | 2 - > src/libcamera/mapped_framebuffer.cpp | 71 ++++++++++++++++--- > src/libcamera/v4l2_videodevice.cpp | 6 ++ > 4 files changed, 70 insertions(+), 13 deletions(-) > > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > index 3401a9fc..42479541 100644 > --- a/include/libcamera/internal/mapped_framebuffer.h > +++ b/include/libcamera/internal/mapped_framebuffer.h > @@ -30,12 +30,14 @@ public: > > bool isValid() const { return error_ == 0; } > int error() const { return error_; } > - const std::vector<Plane> &maps() const { return maps_; } > + /* \todo rename to planes(). */ > + const std::vector<Plane> &maps() const { return planes_; } > > protected: > MappedBuffer(); > > int error_; > + std::vector<Plane> planes_; > std::vector<Plane> maps_; > > private: > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp > index d4f3f22b..299a5496 100644 > --- a/src/android/mm/generic_camera_buffer.cpp > +++ b/src/android/mm/generic_camera_buffer.cpp > @@ -53,8 +53,6 @@ private: > int flags_; > off_t bufferLength_; > std::vector<PlaneInfo> planeInfo_; > - /* \todo Remove planes_ when it will be added to MappedBuffer */ > - std::vector<Span<uint8_t>> planes_; > }; > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > index 2ebe9fdb..1dc88647 100644 > --- a/src/libcamera/mapped_framebuffer.cpp > +++ b/src/libcamera/mapped_framebuffer.cpp > @@ -7,8 +7,11 @@ > > #include "libcamera/internal/mapped_framebuffer.h" > > +#include <algorithm> > #include <errno.h> > +#include <map> > #include <sys/mman.h> > +#include <unistd.h> > > #include <libcamera/base/log.h> > > @@ -79,6 +82,7 @@ MappedBuffer::MappedBuffer(MappedBuffer &&other) > MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other) > { > error_ = other.error_; > + planes_ = std::move(other.planes_); > maps_ = std::move(other.maps_); > other.error_ = -ENOENT; > > @@ -127,10 +131,18 @@ MappedBuffer::~MappedBuffer() > */ > > /** > - * \var MappedBuffer::maps_ > + * \var MappedBuffer::planes_ > * \brief Stores the internal mapped planes > * > * MappedBuffer derived classes shall store the mappings they create in this > + * vector which points the beginning of mapped plane addresses. > + */ > + > +/** > + * \var MappedBuffer::maps_ > + * \brief Stores the mapped buffer > + * > + * MappedBuffer derived classes shall store the mappings they create in this > * vector which is parsed during destruct to unmap any memory mappings which > * completed successfully. > */ > @@ -167,7 +179,8 @@ MappedBuffer::~MappedBuffer() > */ > MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) > { > - maps_.reserve(buffer->planes().size()); > + ASSERT(!buffer->planes().empty()); > + planes_.reserve(buffer->planes().size()); > > int mmapFlags = 0; > > @@ -177,17 +190,55 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) > if (flags & MapFlag::Write) > mmapFlags |= PROT_WRITE; > > + struct MappedBufferInfo { > + uint8_t *address = nullptr; > + size_t mapLength = 0; > + size_t dmabufLength = 0; > + }; > + std::map<int, MappedBufferInfo> mappedBuffers; > + > + for (const FrameBuffer::Plane &plane : buffer->planes()) { > + const int fd = plane.fd.fd(); > + if (mappedBuffers.find(fd) == mappedBuffers.end()) { > + const size_t length = lseek(fd, 0, SEEK_END); > + mappedBuffers[fd] = MappedBufferInfo{ nullptr, 0, length }; > + } > + > + const size_t length = mappedBuffers[fd].dmabufLength; > + > + if (plane.offset > length || > + plane.offset + plane.length > length) { > + LOG(Buffer, Fatal) << "plane is out of buffer: " > + << "buffer length=" << length > + << ", plane offset=" << plane.offset > + << ", plane length=" << plane.length; > + return; > + } > + size_t &mapLength = mappedBuffers[fd].mapLength; > + mapLength = std::max(mapLength, > + static_cast<size_t>(plane.offset + plane.length)); > + } > + > for (const FrameBuffer::Plane &plane : buffer->planes()) { > - void *address = mmap(nullptr, plane.length, mmapFlags, > - MAP_SHARED, plane.fd.fd(), 0); > - if (address == MAP_FAILED) { > - error_ = -errno; > - LOG(Buffer, Error) << "Failed to mmap plane: " > - << strerror(-error_); > - break; > + const int fd = plane.fd.fd(); > + auto &info = mappedBuffers[fd]; > + if (!info.address) { > + void *address = > + static_cast<uint8_t *>( You can drop the static cast here, the variable is a void * and mmap() returns a void *. I can fix this when pushing the patches. > + mmap(nullptr, info.mapLength, mmapFlags, > + MAP_SHARED, fd, 0)); > + if (address == MAP_FAILED) { > + error_ = -errno; > + LOG(Buffer, Error) << "Failed to mmap plane: " > + << strerror(-error_); > + return; > + } > + > + info.address = static_cast<uint8_t *>(address); > + maps_.emplace_back(info.address, info.mapLength); > } > > - maps_.emplace_back(static_cast<uint8_t *>(address), plane.length); > + planes_.emplace_back(info.address + plane.offset, plane.length); > } > } > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index ce60dff6..2ff25af2 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1283,6 +1283,12 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > FrameBuffer::Plane plane; > plane.fd = std::move(fd); > + /* > + * V4L2 API doesn't provide dmabuf offset information of plane. > + * Set 0 as a placeholder offset. > + * \todo Set the right offset once V4L2 API provides a way. > + */ > + plane.offset = 0; > plane.length = multiPlanar ? > buf.m.planes[nplane].length : buf.length; >
diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h index 3401a9fc..42479541 100644 --- a/include/libcamera/internal/mapped_framebuffer.h +++ b/include/libcamera/internal/mapped_framebuffer.h @@ -30,12 +30,14 @@ public: bool isValid() const { return error_ == 0; } int error() const { return error_; } - const std::vector<Plane> &maps() const { return maps_; } + /* \todo rename to planes(). */ + const std::vector<Plane> &maps() const { return planes_; } protected: MappedBuffer(); int error_; + std::vector<Plane> planes_; std::vector<Plane> maps_; private: diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp index d4f3f22b..299a5496 100644 --- a/src/android/mm/generic_camera_buffer.cpp +++ b/src/android/mm/generic_camera_buffer.cpp @@ -53,8 +53,6 @@ private: int flags_; off_t bufferLength_; std::vector<PlaneInfo> planeInfo_; - /* \todo Remove planes_ when it will be added to MappedBuffer */ - std::vector<Span<uint8_t>> planes_; }; CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp index 2ebe9fdb..1dc88647 100644 --- a/src/libcamera/mapped_framebuffer.cpp +++ b/src/libcamera/mapped_framebuffer.cpp @@ -7,8 +7,11 @@ #include "libcamera/internal/mapped_framebuffer.h" +#include <algorithm> #include <errno.h> +#include <map> #include <sys/mman.h> +#include <unistd.h> #include <libcamera/base/log.h> @@ -79,6 +82,7 @@ MappedBuffer::MappedBuffer(MappedBuffer &&other) MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other) { error_ = other.error_; + planes_ = std::move(other.planes_); maps_ = std::move(other.maps_); other.error_ = -ENOENT; @@ -127,10 +131,18 @@ MappedBuffer::~MappedBuffer() */ /** - * \var MappedBuffer::maps_ + * \var MappedBuffer::planes_ * \brief Stores the internal mapped planes * * MappedBuffer derived classes shall store the mappings they create in this + * vector which points the beginning of mapped plane addresses. + */ + +/** + * \var MappedBuffer::maps_ + * \brief Stores the mapped buffer + * + * MappedBuffer derived classes shall store the mappings they create in this * vector which is parsed during destruct to unmap any memory mappings which * completed successfully. */ @@ -167,7 +179,8 @@ MappedBuffer::~MappedBuffer() */ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) { - maps_.reserve(buffer->planes().size()); + ASSERT(!buffer->planes().empty()); + planes_.reserve(buffer->planes().size()); int mmapFlags = 0; @@ -177,17 +190,55 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) if (flags & MapFlag::Write) mmapFlags |= PROT_WRITE; + struct MappedBufferInfo { + uint8_t *address = nullptr; + size_t mapLength = 0; + size_t dmabufLength = 0; + }; + std::map<int, MappedBufferInfo> mappedBuffers; + + for (const FrameBuffer::Plane &plane : buffer->planes()) { + const int fd = plane.fd.fd(); + if (mappedBuffers.find(fd) == mappedBuffers.end()) { + const size_t length = lseek(fd, 0, SEEK_END); + mappedBuffers[fd] = MappedBufferInfo{ nullptr, 0, length }; + } + + const size_t length = mappedBuffers[fd].dmabufLength; + + if (plane.offset > length || + plane.offset + plane.length > length) { + LOG(Buffer, Fatal) << "plane is out of buffer: " + << "buffer length=" << length + << ", plane offset=" << plane.offset + << ", plane length=" << plane.length; + return; + } + size_t &mapLength = mappedBuffers[fd].mapLength; + mapLength = std::max(mapLength, + static_cast<size_t>(plane.offset + plane.length)); + } + for (const FrameBuffer::Plane &plane : buffer->planes()) { - void *address = mmap(nullptr, plane.length, mmapFlags, - MAP_SHARED, plane.fd.fd(), 0); - if (address == MAP_FAILED) { - error_ = -errno; - LOG(Buffer, Error) << "Failed to mmap plane: " - << strerror(-error_); - break; + const int fd = plane.fd.fd(); + auto &info = mappedBuffers[fd]; + if (!info.address) { + void *address = + static_cast<uint8_t *>( + mmap(nullptr, info.mapLength, mmapFlags, + MAP_SHARED, fd, 0)); + if (address == MAP_FAILED) { + error_ = -errno; + LOG(Buffer, Error) << "Failed to mmap plane: " + << strerror(-error_); + return; + } + + info.address = static_cast<uint8_t *>(address); + maps_.emplace_back(info.address, info.mapLength); } - maps_.emplace_back(static_cast<uint8_t *>(address), plane.length); + planes_.emplace_back(info.address + plane.offset, plane.length); } } diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index ce60dff6..2ff25af2 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1283,6 +1283,12 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) FrameBuffer::Plane plane; plane.fd = std::move(fd); + /* + * V4L2 API doesn't provide dmabuf offset information of plane. + * Set 0 as a placeholder offset. + * \todo Set the right offset once V4L2 API provides a way. + */ + plane.offset = 0; plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;