[libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch from Plane to FrameBuffer::Plane
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 8 00:26:42 CET 2020
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 at 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 at 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 at 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);
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list