[libcamera-devel,v2,04/25] libcamera: buffer: Switch from Plane to FrameBuffer::Plane

Message ID 20191230120510.938333-5-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Rework buffer API
Related show

Commit Message

Niklas Söderlund Dec. 30, 2019, 12:04 p.m. UTC
It is not libcameras responsibility to handle memory mappings. Switch
from the soon to be removed Plane class which deals with memory
mappings to FrameBuffer::Plane which just describes it. This makes the
transition to the full FrameBuffer easier.

As the full FrameBuffer interface have not yet spread to all parts of
libcamera core it is hard to create efficient caching of memory mappings
in the qcam application. This will be fixed in a later patch, for now
the dmabuf is mapped and unmapped each time it is seen by the
application.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/buffer.h               |  6 +++---
 src/cam/buffer_writer.cpp                | 10 +++++++---
 src/libcamera/buffer.cpp                 |  4 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  8 ++++----
 src/libcamera/stream.cpp                 |  6 ++++--
 src/libcamera/v4l2_videodevice.cpp       | 13 +++++++------
 src/qcam/main_window.cpp                 | 14 ++++++++++----
 test/camera/buffer_import.cpp            |  2 +-
 8 files changed, 38 insertions(+), 25 deletions(-)

Comments

Jacopo Mondi Jan. 7, 2020, 10:53 a.m. UTC | #1
Hi Niklas,

On Mon, Dec 30, 2019 at 01:04:49PM +0100, Niklas Söderlund wrote:
> It is not libcameras responsibility to handle memory mappings. Switch
> from the soon to be removed Plane class which deals with memory
> mappings to FrameBuffer::Plane which just describes it. This makes the

mapping

> transition to the full FrameBuffer easier.
>
> As the full FrameBuffer interface have not yet spread to all parts of

s/have/has

> libcamera core it is hard to create efficient caching of memory mappings
> in the qcam application. This will be fixed in a later patch, for now
> the dmabuf is mapped and unmapped each time it is seen by the
> application.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/buffer.h               |  6 +++---
>  src/cam/buffer_writer.cpp                | 10 +++++++---
>  src/libcamera/buffer.cpp                 |  4 ++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  8 ++++----
>  src/libcamera/stream.cpp                 |  6 ++++--
>  src/libcamera/v4l2_videodevice.cpp       | 13 +++++++------
>  src/qcam/main_window.cpp                 | 14 ++++++++++----
>  test/camera/buffer_import.cpp            |  2 +-
>  8 files changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 862031123b4b510c..71633492e4752efb 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -89,11 +89,11 @@ private:
>  class BufferMemory final
>  {
>  public:
> -	const std::vector<Plane> &planes() const { return planes_; }
> -	std::vector<Plane> &planes() { return planes_; }
> +	const std::vector<FrameBuffer::Plane> &planes() const { return planes_; }
> +	std::vector<FrameBuffer::Plane> &planes() { return planes_; }
>
>  private:
> -	std::vector<Plane> planes_;
> +	std::vector<FrameBuffer::Plane> planes_;
>  };
>
>  class BufferPool final
> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> index c33e99c5f8173db8..3e84068e66bb4dd7 100644
> --- a/src/cam/buffer_writer.cpp
> +++ b/src/cam/buffer_writer.cpp
> @@ -10,6 +10,7 @@
>  #include <iostream>
>  #include <sstream>
>  #include <string.h>
> +#include <sys/mman.h>
>  #include <unistd.h>
>
>  #include "buffer_writer.h"
> @@ -43,9 +44,10 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
>  		return -errno;
>
>  	BufferMemory *mem = buffer->mem();
> -	for (Plane &plane : mem->planes()) {
> -		void *data = plane.mem();
> -		unsigned int length = plane.length();
> +	for (const FrameBuffer::Plane &plane : mem->planes()) {
> +		void *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> +				  plane.fd.fd(), 0);
> +		unsigned int length = plane.length;
>
>  		ret = ::write(fd, data, length);
>  		if (ret < 0) {
> @@ -59,6 +61,8 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
>  				  << length << std::endl;
>  			break;
>  		}
> +
> +		munmap(data, length);
>  	}
>
>  	close(fd);
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 7c78b9b8f1e90aa5..fde0b33511c64c9e 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -246,13 +246,13 @@ void *Plane::mem()
>  /**
>   * \fn BufferMemory::planes() const
>   * \brief Retrieve the planes within the buffer
> - * \return A const reference to a vector holding all Planes within the buffer
> + * \return A const reference to a vector holding all planes within the buffer
>   */
>
>  /**
>   * \fn BufferMemory::planes()
>   * \brief Retrieve the planes within the buffer
> - * \return A reference to a vector holding all Planes within the buffer
> + * \return A reference to a vector holding all planes within the buffer
>   */
>
>  /**
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index d7ee95ded0f76027..bb652d0da9c6df52 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -688,8 +688,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>
>  	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
>  		FrameBuffer::Plane plane;
> -		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());
> -		plane.length = paramPool_.buffers()[i].planes()[0].length();
> +		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());

Isn't paramPool_.buffers()[i].planes()[0].fd a FileDescriptor already ?
You have a copy constructor for it..

Although, this will later go away so...

> +		plane.length = paramPool_.buffers()[i].planes()[0].length;
>
>  		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
>  					      .planes = { plane } });
> @@ -698,8 +698,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>
>  	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
>  		FrameBuffer::Plane plane;
> -		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());
> -		plane.length = statPool_.buffers()[i].planes()[0].length();
> +		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());
> +		plane.length = statPool_.buffers()[i].planes()[0].length;
>
>  		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
>  					      .planes = { plane } });
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 45f31ae1e2daeb53..a6adc0de5da40063 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -577,8 +577,10 @@ int Stream::mapBuffer(const Buffer *buffer)
>  		if (dmabufs[i] == -1)
>  			break;
>
> -		mem->planes().emplace_back();
> -		mem->planes().back().setDmabuf(dmabufs[i], 0);
> +		FrameBuffer::Plane plane;
> +		plane.fd = FileDescriptor(dmabufs[i]);
> +		plane.length = 0;
> +		mem->planes().emplace_back(plane);

Emplacing an already constructed object calls the copy constructor.
You could just push_back here I think.

>  	}
>
>  	/* Remove the buffer from the cache and return its index. */
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 13e023237dab0daf..f3f5303b7f470f63 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -921,9 +921,10 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
>  		return ret;
>  	}
>
> -	buffer->planes().emplace_back();
> -	Plane &plane = buffer->planes().back();
> -	plane.setDmabuf(expbuf.fd, length);
> +	FrameBuffer::Plane plane;
> +	plane.fd = FileDescriptor(expbuf.fd);
> +	plane.length = length;
> +	buffer->planes().emplace_back(plane);

Same.

Or you could provide a constructor for FrameBuffer::Plane that accepts
and unsigned int fd and an unisgned in length, then you can actually
emplace.

Nits apart
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>  	::close(expbuf.fd);
>
>  	return 0;
> @@ -986,14 +987,14 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
>
>  	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
>  	BufferMemory *mem = &bufferPool_->buffers()[buf.index];
> -	const std::vector<Plane> &planes = mem->planes();
> +	const std::vector<FrameBuffer::Plane> &planes = mem->planes();
>
>  	if (buf.memory == V4L2_MEMORY_DMABUF) {
>  		if (multiPlanar) {
>  			for (unsigned int p = 0; p < planes.size(); ++p)
> -				v4l2Planes[p].m.fd = planes[p].dmabuf();
> +				v4l2Planes[p].m.fd = planes[p].fd.fd();
>  		} else {
> -			buf.m.fd = planes[0].dmabuf();
> +			buf.m.fd = planes[0].fd.fd();
>  		}
>  	}
>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 0c7ca61ac12ec41c..11fb67a509e973f5 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -8,6 +8,7 @@
>  #include <iomanip>
>  #include <iostream>
>  #include <string>
> +#include <sys/mman.h>
>
>  #include <QCoreApplication>
>  #include <QInputDialog>
> @@ -296,13 +297,18 @@ void MainWindow::requestComplete(Request *request)
>
>  int MainWindow::display(Buffer *buffer)
>  {
> -	BufferMemory *mem = buffer->mem();
> -	if (mem->planes().size() != 1)
> +	if (buffer->mem()->planes().size() != 1)
>  		return -EINVAL;
>
> -	Plane &plane = mem->planes().front();
> -	unsigned char *raw = static_cast<unsigned char *>(plane.mem());
> +	/* \todo: Once the FrameBuffer is done cache mapped memory. */
> +	const FrameBuffer::Plane &plane = buffer->mem()->planes().front();
> +	void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> +			    plane.fd.fd(), 0);
> +
> +	unsigned char *raw = static_cast<unsigned char *>(memory);
>  	viewfinder_->display(raw, buffer->bytesused());
>
> +	munmap(memory, plane.length);
> +
>  	return 0;
>  }
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 3efe02704c02f691..171540edd96f9fca 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -178,7 +178,7 @@ private:
>
>  		uint64_t cookie = index;
>  		BufferMemory &mem = pool_.buffers()[index];
> -		int dmabuf = mem.planes()[0].dmabuf();
> +		int dmabuf = mem.planes()[0].fd.fd();
>
>  		requestReady.emit(cookie, dmabuf);
>
> --
> 2.24.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 7, 2020, 4:56 p.m. UTC | #2
Hi Niklas,

Thank you for the patch.

On Tue, Jan 07, 2020 at 11:53:56AM +0100, Jacopo Mondi wrote:
> On Mon, Dec 30, 2019 at 01:04:49PM +0100, Niklas Söderlund wrote:
> > It is not libcameras responsibility to handle memory mappings. Switch

s/libcameras/libcamera's/

> > from the soon to be removed Plane class which deals with memory
> > mappings to FrameBuffer::Plane which just describes it. This makes the
> 
> mapping

Both "mapping" and "mappings" work here.

> > transition to the full FrameBuffer easier.
> >
> > As the full FrameBuffer interface have not yet spread to all parts of
> 
> s/have/has
> 
> > libcamera core it is hard to create efficient caching of memory mappings
> > in the qcam application. This will be fixed in a later patch, for now
> > the dmabuf is mapped and unmapped each time it is seen by the
> > application.

Ouch :-) But it's ok for now if it gets fixed later.

> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/buffer.h               |  6 +++---
> >  src/cam/buffer_writer.cpp                | 10 +++++++---
> >  src/libcamera/buffer.cpp                 |  4 ++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  8 ++++----
> >  src/libcamera/stream.cpp                 |  6 ++++--
> >  src/libcamera/v4l2_videodevice.cpp       | 13 +++++++------
> >  src/qcam/main_window.cpp                 | 14 ++++++++++----
> >  test/camera/buffer_import.cpp            |  2 +-
> >  8 files changed, 38 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index 862031123b4b510c..71633492e4752efb 100644
> > --- a/include/libcamera/buffer.h
> > +++ b/include/libcamera/buffer.h
> > @@ -89,11 +89,11 @@ private:
> >  class BufferMemory final
> >  {
> >  public:
> > -	const std::vector<Plane> &planes() const { return planes_; }
> > -	std::vector<Plane> &planes() { return planes_; }
> > +	const std::vector<FrameBuffer::Plane> &planes() const { return planes_; }
> > +	std::vector<FrameBuffer::Plane> &planes() { return planes_; }
> >
> >  private:
> > -	std::vector<Plane> planes_;
> > +	std::vector<FrameBuffer::Plane> planes_;
> >  };
> >
> >  class BufferPool final
> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> > index c33e99c5f8173db8..3e84068e66bb4dd7 100644
> > --- a/src/cam/buffer_writer.cpp
> > +++ b/src/cam/buffer_writer.cpp
> > @@ -10,6 +10,7 @@
> >  #include <iostream>
> >  #include <sstream>
> >  #include <string.h>
> > +#include <sys/mman.h>
> >  #include <unistd.h>
> >
> >  #include "buffer_writer.h"
> > @@ -43,9 +44,10 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
> >  		return -errno;
> >
> >  	BufferMemory *mem = buffer->mem();
> > -	for (Plane &plane : mem->planes()) {
> > -		void *data = plane.mem();
> > -		unsigned int length = plane.length();
> > +	for (const FrameBuffer::Plane &plane : mem->planes()) {

Can you add a

	/* \todo Cache memory mappings. */

and add a patch that fixes this ?

> > +		void *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> > +				  plane.fd.fd(), 0);
> > +		unsigned int length = plane.length;
> >
> >  		ret = ::write(fd, data, length);
> >  		if (ret < 0) {
> > @@ -59,6 +61,8 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
> >  				  << length << std::endl;
> >  			break;
> >  		}
> > +
> > +		munmap(data, length);
> >  	}
> >
> >  	close(fd);
> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > index 7c78b9b8f1e90aa5..fde0b33511c64c9e 100644
> > --- a/src/libcamera/buffer.cpp
> > +++ b/src/libcamera/buffer.cpp
> > @@ -246,13 +246,13 @@ void *Plane::mem()
> >  /**
> >   * \fn BufferMemory::planes() const
> >   * \brief Retrieve the planes within the buffer
> > - * \return A const reference to a vector holding all Planes within the buffer
> > + * \return A const reference to a vector holding all planes within the buffer
> >   */
> >
> >  /**
> >   * \fn BufferMemory::planes()
> >   * \brief Retrieve the planes within the buffer
> > - * \return A reference to a vector holding all Planes within the buffer
> > + * \return A reference to a vector holding all planes within the buffer
> >   */
> >
> >  /**
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index d7ee95ded0f76027..bb652d0da9c6df52 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -688,8 +688,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> >
> >  	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> >  		FrameBuffer::Plane plane;
> > -		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());
> > -		plane.length = paramPool_.buffers()[i].planes()[0].length();
> > +		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());
> 
> Isn't paramPool_.buffers()[i].planes()[0].fd a FileDescriptor already ?
> You have a copy constructor for it..
> 
> Although, this will later go away so...
> 
> > +		plane.length = paramPool_.buffers()[i].planes()[0].length;

I think we can even simplify it further.

		plane = paramPool_.buffers()[i].planes()[0];

> >
> >  		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
> >  					      .planes = { plane } });

Or even

  		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
  					      .planes = paramPool_.buffers()[i].planes() });

As Jacopo said the code will indeed go away later, but if it's not too
painful to fix, let's keep a good history.

> > @@ -698,8 +698,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> >
> >  	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> >  		FrameBuffer::Plane plane;
> > -		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());
> > -		plane.length = statPool_.buffers()[i].planes()[0].length();
> > +		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());
> > +		plane.length = statPool_.buffers()[i].planes()[0].length;
> >
> >  		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
> >  					      .planes = { plane } });

Same here.

> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index 45f31ae1e2daeb53..a6adc0de5da40063 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -577,8 +577,10 @@ int Stream::mapBuffer(const Buffer *buffer)
> >  		if (dmabufs[i] == -1)
> >  			break;
> >
> > -		mem->planes().emplace_back();
> > -		mem->planes().back().setDmabuf(dmabufs[i], 0);
> > +		FrameBuffer::Plane plane;
> > +		plane.fd = FileDescriptor(dmabufs[i]);
> > +		plane.length = 0;
> > +		mem->planes().emplace_back(plane);
> 
> Emplacing an already constructed object calls the copy constructor.
> You could just push_back here I think.

push_back is worse is it calls the default constructor followed by the
copy assignment operator.

> >  	}
> >
> >  	/* Remove the buffer from the cache and return its index. */
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 13e023237dab0daf..f3f5303b7f470f63 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -921,9 +921,10 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
> >  		return ret;
> >  	}
> >
> > -	buffer->planes().emplace_back();
> > -	Plane &plane = buffer->planes().back();
> > -	plane.setDmabuf(expbuf.fd, length);
> > +	FrameBuffer::Plane plane;
> > +	plane.fd = FileDescriptor(expbuf.fd);
> > +	plane.length = length;
> > +	buffer->planes().emplace_back(plane);
> 
> Same.
> 
> Or you could provide a constructor for FrameBuffer::Plane that accepts
> and unsigned int fd and an unisgned in length, then you can actually
> emplace.

It could make sense to do so, but I would then provide a const FileDescriptor
&, not an int fd.

> Nits apart
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  	::close(expbuf.fd);
> >
> >  	return 0;
> > @@ -986,14 +987,14 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> >
> >  	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> >  	BufferMemory *mem = &bufferPool_->buffers()[buf.index];
> > -	const std::vector<Plane> &planes = mem->planes();
> > +	const std::vector<FrameBuffer::Plane> &planes = mem->planes();
> >
> >  	if (buf.memory == V4L2_MEMORY_DMABUF) {
> >  		if (multiPlanar) {
> >  			for (unsigned int p = 0; p < planes.size(); ++p)
> > -				v4l2Planes[p].m.fd = planes[p].dmabuf();
> > +				v4l2Planes[p].m.fd = planes[p].fd.fd();
> >  		} else {
> > -			buf.m.fd = planes[0].dmabuf();
> > +			buf.m.fd = planes[0].fd.fd();
> >  		}
> >  	}
> >
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 0c7ca61ac12ec41c..11fb67a509e973f5 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -8,6 +8,7 @@
> >  #include <iomanip>
> >  #include <iostream>
> >  #include <string>
> > +#include <sys/mman.h>
> >
> >  #include <QCoreApplication>
> >  #include <QInputDialog>
> > @@ -296,13 +297,18 @@ void MainWindow::requestComplete(Request *request)
> >
> >  int MainWindow::display(Buffer *buffer)
> >  {
> > -	BufferMemory *mem = buffer->mem();
> > -	if (mem->planes().size() != 1)
> > +	if (buffer->mem()->planes().size() != 1)
> >  		return -EINVAL;
> >
> > -	Plane &plane = mem->planes().front();
> > -	unsigned char *raw = static_cast<unsigned char *>(plane.mem());
> > +	/* \todo: Once the FrameBuffer is done cache mapped memory. */

s/todo:/todo/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +	const FrameBuffer::Plane &plane = buffer->mem()->planes().front();
> > +	void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> > +			    plane.fd.fd(), 0);
> > +
> > +	unsigned char *raw = static_cast<unsigned char *>(memory);
> >  	viewfinder_->display(raw, buffer->bytesused());
> >
> > +	munmap(memory, plane.length);
> > +
> >  	return 0;
> >  }
> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > index 3efe02704c02f691..171540edd96f9fca 100644
> > --- a/test/camera/buffer_import.cpp
> > +++ b/test/camera/buffer_import.cpp
> > @@ -178,7 +178,7 @@ private:
> >
> >  		uint64_t cookie = index;
> >  		BufferMemory &mem = pool_.buffers()[index];
> > -		int dmabuf = mem.planes()[0].dmabuf();
> > +		int dmabuf = mem.planes()[0].fd.fd();
> >
> >  		requestReady.emit(cookie, dmabuf);
> >
Laurent Pinchart Jan. 7, 2020, 11:26 p.m. UTC | #3
On Tue, Jan 07, 2020 at 06:56:29PM +0200, Laurent Pinchart wrote:
> On Tue, Jan 07, 2020 at 11:53:56AM +0100, Jacopo Mondi wrote:
> > On Mon, Dec 30, 2019 at 01:04:49PM +0100, Niklas Söderlund wrote:
> > > It is not libcameras responsibility to handle memory mappings. Switch
> 
> s/libcameras/libcamera's/
> 
> > > from the soon to be removed Plane class which deals with memory
> > > mappings to FrameBuffer::Plane which just describes it. This makes the
> > 
> > mapping
> 
> Both "mapping" and "mappings" work here.
> 
> > > transition to the full FrameBuffer easier.
> > >
> > > As the full FrameBuffer interface have not yet spread to all parts of
> > 
> > s/have/has
> > 
> > > libcamera core it is hard to create efficient caching of memory mappings
> > > in the qcam application. This will be fixed in a later patch, for now
> > > the dmabuf is mapped and unmapped each time it is seen by the
> > > application.
> 
> Ouch :-) But it's ok for now if it gets fixed later.
> 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > >  include/libcamera/buffer.h               |  6 +++---
> > >  src/cam/buffer_writer.cpp                | 10 +++++++---
> > >  src/libcamera/buffer.cpp                 |  4 ++--
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  8 ++++----
> > >  src/libcamera/stream.cpp                 |  6 ++++--
> > >  src/libcamera/v4l2_videodevice.cpp       | 13 +++++++------
> > >  src/qcam/main_window.cpp                 | 14 ++++++++++----
> > >  test/camera/buffer_import.cpp            |  2 +-
> > >  8 files changed, 38 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > > index 862031123b4b510c..71633492e4752efb 100644
> > > --- a/include/libcamera/buffer.h
> > > +++ b/include/libcamera/buffer.h
> > > @@ -89,11 +89,11 @@ private:
> > >  class BufferMemory final
> > >  {
> > >  public:
> > > -	const std::vector<Plane> &planes() const { return planes_; }
> > > -	std::vector<Plane> &planes() { return planes_; }
> > > +	const std::vector<FrameBuffer::Plane> &planes() const { return planes_; }
> > > +	std::vector<FrameBuffer::Plane> &planes() { return planes_; }
> > >
> > >  private:
> > > -	std::vector<Plane> planes_;
> > > +	std::vector<FrameBuffer::Plane> planes_;
> > >  };
> > >
> > >  class BufferPool final
> > > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> > > index c33e99c5f8173db8..3e84068e66bb4dd7 100644
> > > --- a/src/cam/buffer_writer.cpp
> > > +++ b/src/cam/buffer_writer.cpp
> > > @@ -10,6 +10,7 @@
> > >  #include <iostream>
> > >  #include <sstream>
> > >  #include <string.h>
> > > +#include <sys/mman.h>
> > >  #include <unistd.h>
> > >
> > >  #include "buffer_writer.h"
> > > @@ -43,9 +44,10 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
> > >  		return -errno;
> > >
> > >  	BufferMemory *mem = buffer->mem();
> > > -	for (Plane &plane : mem->planes()) {
> > > -		void *data = plane.mem();
> > > -		unsigned int length = plane.length();
> > > +	for (const FrameBuffer::Plane &plane : mem->planes()) {
> 
> Can you add a
> 
> 	/* \todo Cache memory mappings. */
> 
> and add a patch that fixes this ?
> 
> > > +		void *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> > > +				  plane.fd.fd(), 0);
> > > +		unsigned int length = plane.length;
> > >
> > >  		ret = ::write(fd, data, length);
> > >  		if (ret < 0) {
> > > @@ -59,6 +61,8 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
> > >  				  << length << std::endl;
> > >  			break;
> > >  		}
> > > +
> > > +		munmap(data, length);
> > >  	}
> > >
> > >  	close(fd);
> > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > > index 7c78b9b8f1e90aa5..fde0b33511c64c9e 100644
> > > --- a/src/libcamera/buffer.cpp
> > > +++ b/src/libcamera/buffer.cpp
> > > @@ -246,13 +246,13 @@ void *Plane::mem()
> > >  /**
> > >   * \fn BufferMemory::planes() const
> > >   * \brief Retrieve the planes within the buffer
> > > - * \return A const reference to a vector holding all Planes within the buffer
> > > + * \return A const reference to a vector holding all planes within the buffer
> > >   */
> > >
> > >  /**
> > >   * \fn BufferMemory::planes()
> > >   * \brief Retrieve the planes within the buffer
> > > - * \return A reference to a vector holding all Planes within the buffer
> > > + * \return A reference to a vector holding all planes within the buffer
> > >   */
> > >
> > >  /**
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index d7ee95ded0f76027..bb652d0da9c6df52 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -688,8 +688,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> > >
> > >  	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> > >  		FrameBuffer::Plane plane;
> > > -		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());
> > > -		plane.length = paramPool_.buffers()[i].planes()[0].length();
> > > +		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());
> > 
> > Isn't paramPool_.buffers()[i].planes()[0].fd a FileDescriptor already ?
> > You have a copy constructor for it..
> > 
> > Although, this will later go away so...
> > 
> > > +		plane.length = paramPool_.buffers()[i].planes()[0].length;
> 
> I think we can even simplify it further.
> 
> 		plane = paramPool_.buffers()[i].planes()[0];
> 
> > >
> > >  		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
> > >  					      .planes = { plane } });
> 
> Or even
> 
>   		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
>   					      .planes = paramPool_.buffers()[i].planes() });
> 
> As Jacopo said the code will indeed go away later, but if it's not too
> painful to fix, let's keep a good history.
> 
> > > @@ -698,8 +698,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> > >
> > >  	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> > >  		FrameBuffer::Plane plane;
> > > -		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());
> > > -		plane.length = statPool_.buffers()[i].planes()[0].length();
> > > +		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());
> > > +		plane.length = statPool_.buffers()[i].planes()[0].length;
> > >
> > >  		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
> > >  					      .planes = { plane } });
> 
> Same here.
> 
> > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > > index 45f31ae1e2daeb53..a6adc0de5da40063 100644
> > > --- a/src/libcamera/stream.cpp
> > > +++ b/src/libcamera/stream.cpp
> > > @@ -577,8 +577,10 @@ int Stream::mapBuffer(const Buffer *buffer)
> > >  		if (dmabufs[i] == -1)
> > >  			break;
> > >
> > > -		mem->planes().emplace_back();
> > > -		mem->planes().back().setDmabuf(dmabufs[i], 0);
> > > +		FrameBuffer::Plane plane;
> > > +		plane.fd = FileDescriptor(dmabufs[i]);
> > > +		plane.length = 0;
> > > +		mem->planes().emplace_back(plane);
> > 
> > Emplacing an already constructed object calls the copy constructor.
> > You could just push_back here I think.
> 
> push_back is worse is it calls the default constructor followed by the
> copy assignment operator.

I have to apologize for this, I was wrong. push_back() doesn't call the
default constructor, so Jacopo was right.

> > >  	}
> > >
> > >  	/* Remove the buffer from the cache and return its index. */
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 13e023237dab0daf..f3f5303b7f470f63 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -921,9 +921,10 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
> > >  		return ret;
> > >  	}
> > >
> > > -	buffer->planes().emplace_back();
> > > -	Plane &plane = buffer->planes().back();
> > > -	plane.setDmabuf(expbuf.fd, length);
> > > +	FrameBuffer::Plane plane;
> > > +	plane.fd = FileDescriptor(expbuf.fd);
> > > +	plane.length = length;
> > > +	buffer->planes().emplace_back(plane);
> > 
> > Same.
> > 
> > Or you could provide a constructor for FrameBuffer::Plane that accepts
> > and unsigned int fd and an unisgned in length, then you can actually
> > emplace.
> 
> It could make sense to do so, but I would then provide a const FileDescriptor
> &, not an int fd.
> 
> > Nits apart
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > >  	::close(expbuf.fd);
> > >
> > >  	return 0;
> > > @@ -986,14 +987,14 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > >
> > >  	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> > >  	BufferMemory *mem = &bufferPool_->buffers()[buf.index];
> > > -	const std::vector<Plane> &planes = mem->planes();
> > > +	const std::vector<FrameBuffer::Plane> &planes = mem->planes();
> > >
> > >  	if (buf.memory == V4L2_MEMORY_DMABUF) {
> > >  		if (multiPlanar) {
> > >  			for (unsigned int p = 0; p < planes.size(); ++p)
> > > -				v4l2Planes[p].m.fd = planes[p].dmabuf();
> > > +				v4l2Planes[p].m.fd = planes[p].fd.fd();
> > >  		} else {
> > > -			buf.m.fd = planes[0].dmabuf();
> > > +			buf.m.fd = planes[0].fd.fd();
> > >  		}
> > >  	}
> > >
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 0c7ca61ac12ec41c..11fb67a509e973f5 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include <iomanip>
> > >  #include <iostream>
> > >  #include <string>
> > > +#include <sys/mman.h>
> > >
> > >  #include <QCoreApplication>
> > >  #include <QInputDialog>
> > > @@ -296,13 +297,18 @@ void MainWindow::requestComplete(Request *request)
> > >
> > >  int MainWindow::display(Buffer *buffer)
> > >  {
> > > -	BufferMemory *mem = buffer->mem();
> > > -	if (mem->planes().size() != 1)
> > > +	if (buffer->mem()->planes().size() != 1)
> > >  		return -EINVAL;
> > >
> > > -	Plane &plane = mem->planes().front();
> > > -	unsigned char *raw = static_cast<unsigned char *>(plane.mem());
> > > +	/* \todo: Once the FrameBuffer is done cache mapped memory. */
> 
> s/todo:/todo/
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > > +	const FrameBuffer::Plane &plane = buffer->mem()->planes().front();
> > > +	void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> > > +			    plane.fd.fd(), 0);
> > > +
> > > +	unsigned char *raw = static_cast<unsigned char *>(memory);
> > >  	viewfinder_->display(raw, buffer->bytesused());
> > >
> > > +	munmap(memory, plane.length);
> > > +
> > >  	return 0;
> > >  }
> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > > index 3efe02704c02f691..171540edd96f9fca 100644
> > > --- a/test/camera/buffer_import.cpp
> > > +++ b/test/camera/buffer_import.cpp
> > > @@ -178,7 +178,7 @@ private:
> > >
> > >  		uint64_t cookie = index;
> > >  		BufferMemory &mem = pool_.buffers()[index];
> > > -		int dmabuf = mem.planes()[0].dmabuf();
> > > +		int dmabuf = mem.planes()[0].fd.fd();
> > >
> > >  		requestReady.emit(cookie, dmabuf);
> > >

Patch

diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
index 862031123b4b510c..71633492e4752efb 100644
--- a/include/libcamera/buffer.h
+++ b/include/libcamera/buffer.h
@@ -89,11 +89,11 @@  private:
 class BufferMemory final
 {
 public:
-	const std::vector<Plane> &planes() const { return planes_; }
-	std::vector<Plane> &planes() { return planes_; }
+	const std::vector<FrameBuffer::Plane> &planes() const { return planes_; }
+	std::vector<FrameBuffer::Plane> &planes() { return planes_; }
 
 private:
-	std::vector<Plane> planes_;
+	std::vector<FrameBuffer::Plane> planes_;
 };
 
 class BufferPool final
diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
index c33e99c5f8173db8..3e84068e66bb4dd7 100644
--- a/src/cam/buffer_writer.cpp
+++ b/src/cam/buffer_writer.cpp
@@ -10,6 +10,7 @@ 
 #include <iostream>
 #include <sstream>
 #include <string.h>
+#include <sys/mman.h>
 #include <unistd.h>
 
 #include "buffer_writer.h"
@@ -43,9 +44,10 @@  int BufferWriter::write(Buffer *buffer, const std::string &streamName)
 		return -errno;
 
 	BufferMemory *mem = buffer->mem();
-	for (Plane &plane : mem->planes()) {
-		void *data = plane.mem();
-		unsigned int length = plane.length();
+	for (const FrameBuffer::Plane &plane : mem->planes()) {
+		void *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
+				  plane.fd.fd(), 0);
+		unsigned int length = plane.length;
 
 		ret = ::write(fd, data, length);
 		if (ret < 0) {
@@ -59,6 +61,8 @@  int BufferWriter::write(Buffer *buffer, const std::string &streamName)
 				  << length << std::endl;
 			break;
 		}
+
+		munmap(data, length);
 	}
 
 	close(fd);
diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
index 7c78b9b8f1e90aa5..fde0b33511c64c9e 100644
--- a/src/libcamera/buffer.cpp
+++ b/src/libcamera/buffer.cpp
@@ -246,13 +246,13 @@  void *Plane::mem()
 /**
  * \fn BufferMemory::planes() const
  * \brief Retrieve the planes within the buffer
- * \return A const reference to a vector holding all Planes within the buffer
+ * \return A const reference to a vector holding all planes within the buffer
  */
 
 /**
  * \fn BufferMemory::planes()
  * \brief Retrieve the planes within the buffer
- * \return A reference to a vector holding all Planes within the buffer
+ * \return A reference to a vector holding all planes within the buffer
  */
 
 /**
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index d7ee95ded0f76027..bb652d0da9c6df52 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -688,8 +688,8 @@  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
 
 	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
 		FrameBuffer::Plane plane;
-		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].dmabuf());
-		plane.length = paramPool_.buffers()[i].planes()[0].length();
+		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());
+		plane.length = paramPool_.buffers()[i].planes()[0].length;
 
 		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
 					      .planes = { plane } });
@@ -698,8 +698,8 @@  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
 
 	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
 		FrameBuffer::Plane plane;
-		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].dmabuf());
-		plane.length = statPool_.buffers()[i].planes()[0].length();
+		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());
+		plane.length = statPool_.buffers()[i].planes()[0].length;
 
 		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
 					      .planes = { plane } });
diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 45f31ae1e2daeb53..a6adc0de5da40063 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -577,8 +577,10 @@  int Stream::mapBuffer(const Buffer *buffer)
 		if (dmabufs[i] == -1)
 			break;
 
-		mem->planes().emplace_back();
-		mem->planes().back().setDmabuf(dmabufs[i], 0);
+		FrameBuffer::Plane plane;
+		plane.fd = FileDescriptor(dmabufs[i]);
+		plane.length = 0;
+		mem->planes().emplace_back(plane);
 	}
 
 	/* Remove the buffer from the cache and return its index. */
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 13e023237dab0daf..f3f5303b7f470f63 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -921,9 +921,10 @@  int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
 		return ret;
 	}
 
-	buffer->planes().emplace_back();
-	Plane &plane = buffer->planes().back();
-	plane.setDmabuf(expbuf.fd, length);
+	FrameBuffer::Plane plane;
+	plane.fd = FileDescriptor(expbuf.fd);
+	plane.length = length;
+	buffer->planes().emplace_back(plane);
 	::close(expbuf.fd);
 
 	return 0;
@@ -986,14 +987,14 @@  int V4L2VideoDevice::queueBuffer(Buffer *buffer)
 
 	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
 	BufferMemory *mem = &bufferPool_->buffers()[buf.index];
-	const std::vector<Plane> &planes = mem->planes();
+	const std::vector<FrameBuffer::Plane> &planes = mem->planes();
 
 	if (buf.memory == V4L2_MEMORY_DMABUF) {
 		if (multiPlanar) {
 			for (unsigned int p = 0; p < planes.size(); ++p)
-				v4l2Planes[p].m.fd = planes[p].dmabuf();
+				v4l2Planes[p].m.fd = planes[p].fd.fd();
 		} else {
-			buf.m.fd = planes[0].dmabuf();
+			buf.m.fd = planes[0].fd.fd();
 		}
 	}
 
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index 0c7ca61ac12ec41c..11fb67a509e973f5 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -8,6 +8,7 @@ 
 #include <iomanip>
 #include <iostream>
 #include <string>
+#include <sys/mman.h>
 
 #include <QCoreApplication>
 #include <QInputDialog>
@@ -296,13 +297,18 @@  void MainWindow::requestComplete(Request *request)
 
 int MainWindow::display(Buffer *buffer)
 {
-	BufferMemory *mem = buffer->mem();
-	if (mem->planes().size() != 1)
+	if (buffer->mem()->planes().size() != 1)
 		return -EINVAL;
 
-	Plane &plane = mem->planes().front();
-	unsigned char *raw = static_cast<unsigned char *>(plane.mem());
+	/* \todo: Once the FrameBuffer is done cache mapped memory. */
+	const FrameBuffer::Plane &plane = buffer->mem()->planes().front();
+	void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
+			    plane.fd.fd(), 0);
+
+	unsigned char *raw = static_cast<unsigned char *>(memory);
 	viewfinder_->display(raw, buffer->bytesused());
 
+	munmap(memory, plane.length);
+
 	return 0;
 }
diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
index 3efe02704c02f691..171540edd96f9fca 100644
--- a/test/camera/buffer_import.cpp
+++ b/test/camera/buffer_import.cpp
@@ -178,7 +178,7 @@  private:
 
 		uint64_t cookie = index;
 		BufferMemory &mem = pool_.buffers()[index];
-		int dmabuf = mem.planes()[0].dmabuf();
+		int dmabuf = mem.planes()[0].fd.fd();
 
 		requestReady.emit(cookie, dmabuf);