[libcamera-devel,RFC,v2,03/10] libcamera: mapped_framebuffer: Return plane begin address by MappedBuffer::maps()
diff mbox series

Message ID 20210823131221.1034059-4-hiroh@chromium.org
State Superseded
Headers show
Series
  • Add offset to FrameBuffer::Plane
Related show

Commit Message

Hirokazu Honda Aug. 23, 2021, 1:12 p.m. UTC 
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>
---
 .../libcamera/internal/mapped_framebuffer.h   |  4 +-
 src/libcamera/mapped_framebuffer.cpp          | 69 ++++++++++++++++---
 2 files changed, 62 insertions(+), 11 deletions(-)

Comments

Laurent Pinchart Aug. 23, 2021, 10:30 p.m. UTC | #1 
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@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@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);
>  	}
>  }
>
Laurent Pinchart Aug. 24, 2021, 3:49 p.m. UTC | #2 
Hi Hiro,

On Tue, Aug 24, 2021 at 01:30:32AM +0300, Laurent Pinchart wrote:
> 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@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;

By the way, this patch breaks multiple unit tests:

[192:35:55.294272353] [19738] FATAL Buffer mapped_framebuffer.cpp:210 plane is out of buffer: buffer length=6221824, plane offset=969921872, plane length=6220800

The issue is fixed later on in the series, in patch 08/10. It would be
nice to avoid breaking bisection, which I think can simply be done by
moving the plane.offset = 0 in v4l2_videodevice.cpp from patch 08/10 to
01/10.

The reason I noticed is that the V4L2 buffer cache unit test is broken
by the series in 08/10, and I tried to check if 07/10 worked properly.

> > +			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@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);
> >  	}
> >  }
> >

Patch
diff mbox series 

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) {
+				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);
 	}
 }