[libcamera-devel] [PATCH] libcamera: framebuffer: Return plane begin address by MappedBuffer::maps()

[Laurent Pinchart]

Hello,

On Fri, Aug 06, 2021 at 02:18:10PM +0100, Kieran Bingham wrote:
> On 05/08/2021 18:49, 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.
> 
> If we can handle the logic in MappedBuffer to get the correct addresses
> that would certainly be beneficial indeed, so this sounds like good
> development.
> 
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  include/libcamera/internal/framebuffer.h |  1 +
> >  src/libcamera/framebuffer.cpp            | 41 +++++++++++++++++-------
> >  2 files changed, 31 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > index 8c187adf..2eb3b7c9 100644
> > --- a/include/libcamera/internal/framebuffer.h
> > +++ b/include/libcamera/internal/framebuffer.h
> > @@ -36,6 +36,7 @@ protected:
> >  
> >  	int error_;
> >  	std::vector<Plane> maps_;
> > +	std::vector<Plane> chunks_;
> >  
> >  private:
> >  	LIBCAMERA_DISABLE_COPY(MappedBuffer)
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index a59e93fb..8231c1df 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -310,6 +310,7 @@ MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> >  {
> >  	error_ = other.error_;
> >  	maps_ = std::move(other.maps_);
> > +	chunks_ = std::move(other.chunks_);
> >  	other.error_ = -ENOENT;
> >  
> >  	return *this;
> > @@ -317,8 +318,8 @@ MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> >  
> >  MappedBuffer::~MappedBuffer()
> >  {
> > -	for (Plane &map : maps_)
> > -		munmap(map.data(), map.size());
> > +	for (Plane &chunk : chunks_)
> > +		munmap(chunk.data(), chunk.size());

The names are a bit confusing, I would expect maps_ to contained the
mapped memory. How about doing that, and renaming chunks_ to planes_ ?

> >  }
> >  
> >  /**
> > @@ -381,18 +382,36 @@ MappedBuffer::~MappedBuffer()
> >   */
> >  MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> >  {
> > +	if (buffer->planes().empty())
> > +		return;
> 
> Can we have a buffer with zero planes? Is this something that should
> generate an Error or Warning message?
> 
> (Or if it really implies there's a bug, a Fatal?)

It's not supposed to happen, so I think an ASSERT() would be enough.

> >  	maps_.reserve(buffer->planes().size());
> >  
> > +	/*
> > +	 * Below assumes planes are in the same buffer and planes are
> > +	 * consecutive.
> > +	 * \todo remove this assumption.
> 
> To do that, will we need to store the PixelFormat information as part of
> the FrameBuffer? Or at least some additional data there so that we know
> it's layout?
>
> And - now I've seen Laurent posted a patch to expose a Format for that
> purpose...

I'd really like to avoid coupling FrameBuffer and FrameFormat. What we
need if an offset field in FrameBuffer::Plane. With that,
MappedFrameBuffer can check if the fds in different planes are
identical, and if so, map the fd once only and use the offsets to
compute addresses. I think that would be enough.

Until we get that, could we verify that the assertion holds with an
ASSERT() here ? An ASSERT(buffer->planes().size() == 1) should suffice.

> > +	 */
> > +	const int fd = buffer->planes()[0].fd.fd();
> > +	const off_t length = lseek(fd, 0, SEEK_END);
> > +	if (length < 0) {
> > +		error_ = -errno;
> > +		LOG(Buffer, Error) << "Failed to lseek buffer";
> > +		return;
> > +	}
> > +
> > +	void *address = mmap(nullptr, length, flags, MAP_SHARED, fd, 0);
> > +	if (address == MAP_FAILED) {
> > +		error_ = -errno;
> > +		LOG(Buffer, Error) << "Failed to mmap plane";
> > +		return;
> > +	}
> > +	chunks_ = { MappedBuffer::Plane(static_cast<uint8_t *>(address),
> > +					static_cast<size_t>(length)) };
> > +	size_t offset = 0;
> >  	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(Buffer, Error) << "Failed to mmap plane";
> > -			break;
> > -		}
> > -
> > -		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> > +		maps_.emplace_back(static_cast<uint8_t *>(address) + offset,
> > +				   plane.length);
> > +		offset += plane.length;
> >  	}
> >  }
> >  

-- 
Regards,

Laurent Pinchart