[libcamera-devel] [RFC PATCH 03/10] libcamera: mapped_framebuffer: Return plane begin address by MappedBuffer::maps()

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Aug 16, 2021 at 01:31:31PM +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          | 51 +++++++++++++++----
>  2 files changed, 44 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(). */

I'm fine with a todo comment in this patch, but could you do the rename
as part of the series, maybe as a last patch on top ?

> +	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..b0ba89b0 100644
> --- a/src/libcamera/mapped_framebuffer.cpp
> +++ b/src/libcamera/mapped_framebuffer.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <errno.h>
>  #include <sys/mman.h>
> +#include <unistd.h>
>  
>  #include <libcamera/base/log.h>
>  
> @@ -79,6 +80,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 +129,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 +177,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,18 +188,38 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
>  	if (flags & MapFlag::Write)
>  		mmapFlags |= PROT_WRITE;
>  
> +	int prevFd = -1;
>  	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();
> +		if (prevFd != fd) {

Could there be a case where plane 1 and 3 share the same dmabuf but
plane 2 uses a different once ? It may be a bit far-fetched, but how
about turning the maps_ vector into a std::map<int, Plane> ? That way
we'll correctly support this case, with no overhead and an
implementation that shouldn't be more complicated.

> +			const size_t length = lseek(fd, 0, SEEK_END);
> +			void *address = mmap(nullptr, length, mmapFlags,
> +					     MAP_SHARED, fd, 0);

Hmmm... I could imagine that in some case the dmabuf would be larger
than the sum of the planes, and it may then be possible to only map part
of the dmabuf. Let's consider this for a possible future enhancement, as
virtual address space should hopefully not be a limiting factor in most
case. Maybe a todo comment would be appropriate ?

			/**
			 * \todo Should we try to only map the portions of the
			 * dmabuf that are used by planes ?
			 */

> +			if (address == MAP_FAILED) {
> +				error_ = -errno;
> +				LOG(Buffer, Error) << "Failed to mmap plane: "
> +						   << strerror(-error_);
> +				return;
> +			}
> +			maps_.emplace_back(static_cast<uint8_t *>(address),
> +					   length);
> +			prevFd = fd;
>  		}
>  
> -		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> +		const size_t length = maps_.back().size();
> +		if (plane.offset + plane.length > length) {

Should we protect ourselves against arithmetic overflows here ?

		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;
> +		}
> +
> +		uint8_t *buf = maps_.back().data();
> +		planes_.emplace_back(buf + plane.offset, plane.length);
>  	}
> +
> +	ASSERT(maps_.size() == 1u);

I'm not sure to understand this. Won't a multi-planar frame buffer with
different dmabufs fail this assertion ?

>  }
>  
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart