[libcamera-devel] [PATCH v2 04/25] libcamera: buffer: Switch from Plane to FrameBuffer::Plane
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 7 11:53:56 CET 2020
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 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()) {
> + 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200107/11e03138/attachment.sig>
More information about the libcamera-devel
mailing list