[libcamera-devel,IPU3-IPA] libcamera-helpers: Integrate latest MappedFrameBuffer

14157 diff mbox series

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@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%)

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@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@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',
>  ])