| Message ID | 20191230120510.938333-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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
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); > >
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); > > >
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);
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(-)