Hi Hiro,
Thank you for the patch.
On Mon, Aug 23, 2021 at 10:12:14PM +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 at chromium.org>
> ---
> .../libcamera/internal/mapped_framebuffer.h | 4 +-
> src/libcamera/mapped_framebuffer.cpp | 69 ++++++++++++++++---
> 2 files changed, 62 insertions(+), 11 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/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> index 2ebe9fdb..03425dea 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,53 @@ 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) {
> + info.address =
> + static_cast<uint8_t *>(
> + mmap(nullptr, info.mapLength, mmapFlags,
> + MAP_SHARED, fd, 0));
> + if (info.address == MAP_FAILED) {
I think the following would be more readable:
void *addr = mmap(nullptr, info.mapLength, mmapFlags,
MAP_SHARED, fd, 0));
if (addr == MAP_FAILED) {
...
}
info.address = static_cast<uint8_t *>(addr);
Overall the patch looks quite elegant to me :-)
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + error_ = -errno;
> + LOG(Buffer, Error) << "Failed to mmap plane: "
> + << strerror(-error_);
> + return;
> + }
> +
> + 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);
> }
> }
>
--
Regards,
Laurent Pinchart