[libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch from Plane to FrameBuffer::Plane
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 7 17:56:29 CET 2020
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 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.
> > }
> >
> > /* 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