[libcamera-devel] [IPU3-IPA PATCH] libcamera-helpers: Integrate latest MappedFrameBuffer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Oct 16 01:00:40 CEST 2021
Hi Kieran,
Thank you for the patch.
On Fri, Oct 15, 2021 at 03:16:56PM +0100, Kieran Bingham wrote:
> The MappedFrameBuffer helper class has been updated in the libcamera
> source code.
>
> This makes use of the new enum MapFlags type, and corrects the mapping
> changes that were made during 8708904fad6f ("libcamera:
> mapped_framebuffer: Return plane begin address by MappedBuffer::maps()")
>
> This update also brings back isolated IPA functionality to this external
> IPA, which is otherwise broken due to the offset/plane changes.
>
> The files are renamed to mapped_framebuffer to match the naming in
> libcamera, but are kept within the 'libcamera-helper' hierarchy of the
> IPA.
>
> Also, a minor todo is added to IPAIPU3::mapBuffers, to highlight that we
> could potentially map Statistics buffers as read only rather than
> read/write if we could correctly identify them.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> include/libcamera-helpers/mapped_buffer.h | 53 ---------
> .../libcamera-helpers/mapped_framebuffer.h | 65 +++++++++++
> ipu3.cpp | 8 +-
> ...pped_buffer.cpp => mapped_framebuffer.cpp} | 110 +++++++++++++++---
> src/libcamera-helpers/meson.build | 2 +-
> 5 files changed, 163 insertions(+), 75 deletions(-)
> delete mode 100644 include/libcamera-helpers/mapped_buffer.h
> create mode 100644 include/libcamera-helpers/mapped_framebuffer.h
> rename src/libcamera-helpers/{mapped_buffer.cpp => mapped_framebuffer.cpp} (56%)
>
> diff --git a/include/libcamera-helpers/mapped_buffer.h b/include/libcamera-helpers/mapped_buffer.h
> deleted file mode 100644
> index 6cfc57217d75..000000000000
> --- a/include/libcamera-helpers/mapped_buffer.h
> +++ /dev/null
> @@ -1,53 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * buffer.h - Internal buffer handling
> - */
> -#ifndef __LIBCAMERA_MAPPED_BUFFER_H__
> -#define __LIBCAMERA_MAPPED_BUFFER_H__
> -
> -#include <sys/mman.h>
> -#include <vector>
> -
> -#include <libcamera/base/class.h>
> -#include <libcamera/base/span.h>
> -
> -#include <libcamera/framebuffer.h>
> -
> -namespace libcamera {
> -
> -class MappedBuffer
> -{
> -public:
> - using Plane = Span<uint8_t>;
> -
> - ~MappedBuffer();
> -
> - MappedBuffer(MappedBuffer &&other);
> - MappedBuffer &operator=(MappedBuffer &&other);
> -
> - bool isValid() const { return error_ == 0; }
> - int error() const { return error_; }
> - const std::vector<Plane> &maps() const { return maps_; }
> -
> -protected:
> - MappedBuffer();
> -
> - int error_;
> - std::vector<Plane> maps_;
> -
> -private:
> - LIBCAMERA_DISABLE_COPY(MappedBuffer)
> -};
> -
> -class MappedFrameBuffer : public MappedBuffer
> -{
> -public:
> - MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> -};
> -
> -} /* namespace libcamera */
> -
> -#endif /* __LIBCAMERA_MAPPED_BUFFER_H__ */
> -
> diff --git a/include/libcamera-helpers/mapped_framebuffer.h b/include/libcamera-helpers/mapped_framebuffer.h
> new file mode 100644
> index 000000000000..42155279d44d
> --- /dev/null
> +++ b/include/libcamera-helpers/mapped_framebuffer.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * mapped_framebuffer.h - Frame buffer memory mapping support
> + */
> +#ifndef __LIBCAMERA_HELPER_MAPPED_FRAMEBUFFER_H__
> +#define __LIBCAMERA_HELPER_MAPPED_FRAMEBUFFER_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>
> +
> +namespace libcamera {
> +
> +class MappedBuffer
> +{
> +public:
> + using Plane = Span<uint8_t>;
> +
> + ~MappedBuffer();
> +
> + MappedBuffer(MappedBuffer &&other);
> + MappedBuffer &operator=(MappedBuffer &&other);
> +
> + bool isValid() const { return error_ == 0; }
> + int error() const { return error_; }
> + /* \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:
> + LIBCAMERA_DISABLE_COPY(MappedBuffer)
> +};
> +
> +class MappedFrameBuffer : public MappedBuffer
> +{
> +public:
> + enum class MapFlag {
> + Read = 1 << 0,
> + 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_HELPER_MAPPED_FRAMEBUFFER_H__ */
> diff --git a/ipu3.cpp b/ipu3.cpp
> index 3e89e6dd4e02..c1a254517845 100644
> --- a/ipu3.cpp
> +++ b/ipu3.cpp
> @@ -20,7 +20,7 @@
>
> #include <libcamera/base/log.h>
>
> -#include "libcamera-helpers/mapped_buffer.h"
> +#include "libcamera-helpers/mapped_framebuffer.h"
>
> /* IA AIQ Wrapper API */
> #include "aic/aic.h"
> @@ -258,10 +258,14 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>
> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> {
> + /*
> + * todo: Statistics buffers could be mapped read-only if they
s/todo:/\todo/
It's a good point, it could make sense to pass usage information along
with the buffers.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + * could be easily identified.
> + */
> 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/libcamera-helpers/mapped_buffer.cpp b/src/libcamera-helpers/mapped_framebuffer.cpp
> similarity index 56%
> rename from src/libcamera-helpers/mapped_buffer.cpp
> rename to src/libcamera-helpers/mapped_framebuffer.cpp
> index 6f3248e1b31b..a65740831331 100644
> --- a/src/libcamera-helpers/mapped_buffer.cpp
> +++ b/src/libcamera-helpers/mapped_framebuffer.cpp
> @@ -2,26 +2,27 @@
> /*
> * Copyright (C) 2021, Google Inc.
> *
> - * mapped_buffer.cpp - Mapped Buffer handling
> + * mapped_framebuffer.cpp - Mapped Framebuffer support
> */
>
> -#include "libcamera-helpers/mapped_buffer.h"
> +#include "libcamera-helpers/mapped_framebuffer.h"
>
> +#include <algorithm>
> #include <errno.h>
> -#include <string.h>
> +#include <map>
> #include <sys/mman.h>
> #include <unistd.h>
>
> #include <libcamera/base/log.h>
>
> /**
> - * \file libcamera-helpers/mapped_buffer.h
> - * \brief Mapped Buffer handling
> + * \file libcamera-helpers/mapped_framebuffer.h
> + * \brief Frame buffer memory mapping support
> */
>
> namespace libcamera {
>
> -LOG_DEFINE_CATEGORY(MappedBuffer)
> +LOG_DECLARE_CATEGORY(Buffer)
>
> /**
> * \class MappedBuffer
> @@ -81,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;
>
> @@ -129,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.
> */
> @@ -142,29 +152,91 @@ 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());
> + ASSERT(!buffer->planes().empty());
> + planes_.reserve(buffer->planes().size());
> +
> + int mmapFlags = 0;
> +
> + if (flags & MapFlag::Read)
> + mmapFlags |= PROT_READ;
> +
> + 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, flags,
> - MAP_SHARED, plane.fd.fd(), 0);
> - if (address == MAP_FAILED) {
> - error_ = -errno;
> - LOG(MappedBuffer, Error) << "Failed to mmap plane";
> - break;
> + const int fd = plane.fd.fd();
> + auto &info = mappedBuffers[fd];
> + if (!info.address) {
> + void *address = 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-helpers/meson.build b/src/libcamera-helpers/meson.build
> index 444f21204d5d..084bf65345ae 100644
> --- a/src/libcamera-helpers/meson.build
> +++ b/src/libcamera-helpers/meson.build
> @@ -2,5 +2,5 @@
>
> # Implementation of internal libcamera internals
> libcamera_helpers = files([
> - 'mapped_buffer.cpp',
> + 'mapped_framebuffer.cpp',
> ])
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list