[libcamera-devel] [RFC PATCH v2 03/10] libcamera: mapped_framebuffer: Return plane begin address by MappedBuffer::maps()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 24 17:49:32 CEST 2021
Hi Hiro,
On Tue, Aug 24, 2021 at 01:30:32AM +0300, Laurent Pinchart wrote:
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Aug 23, 2021 at 10:12:14PM +0900, Hirokazu Honda wrote:
> > MappedBuffer::maps() returns std::vector<MappedBuffer::Plane>.
> > Plane has the address, but the address points the beginning of the
> > buffer containing the plane.
> > This makes the Plane point the beginning of the plane. So
> > MappedBuffer::maps()[i].data() returns the address of i-th plane.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > .../libcamera/internal/mapped_framebuffer.h | 4 +-
> > src/libcamera/mapped_framebuffer.cpp | 69 ++++++++++++++++---
> > 2 files changed, 62 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> > index 3401a9fc..42479541 100644
> > --- a/include/libcamera/internal/mapped_framebuffer.h
> > +++ b/include/libcamera/internal/mapped_framebuffer.h
> > @@ -30,12 +30,14 @@ public:
> >
> > bool isValid() const { return error_ == 0; }
> > int error() const { return error_; }
> > - const std::vector<Plane> &maps() const { return maps_; }
> > + /* \todo rename to planes(). */
> > + const std::vector<Plane> &maps() const { return planes_; }
> >
> > protected:
> > MappedBuffer();
> >
> > int error_;
> > + std::vector<Plane> planes_;
> > std::vector<Plane> maps_;
> >
> > private:
> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> > index 2ebe9fdb..03425dea 100644
> > --- a/src/libcamera/mapped_framebuffer.cpp
> > +++ b/src/libcamera/mapped_framebuffer.cpp
> > @@ -7,8 +7,11 @@
> >
> > #include "libcamera/internal/mapped_framebuffer.h"
> >
> > +#include <algorithm>
> > #include <errno.h>
> > +#include <map>
> > #include <sys/mman.h>
> > +#include <unistd.h>
> >
> > #include <libcamera/base/log.h>
> >
> > @@ -79,6 +82,7 @@ MappedBuffer::MappedBuffer(MappedBuffer &&other)
> > MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> > {
> > error_ = other.error_;
> > + planes_ = std::move(other.planes_);
> > maps_ = std::move(other.maps_);
> > other.error_ = -ENOENT;
> >
> > @@ -127,10 +131,18 @@ MappedBuffer::~MappedBuffer()
> > */
> >
> > /**
> > - * \var MappedBuffer::maps_
> > + * \var MappedBuffer::planes_
> > * \brief Stores the internal mapped planes
> > *
> > * MappedBuffer derived classes shall store the mappings they create in this
> > + * vector which points the beginning of mapped plane addresses.
> > + */
> > +
> > +/**
> > + * \var MappedBuffer::maps_
> > + * \brief Stores the mapped buffer
> > + *
> > + * MappedBuffer derived classes shall store the mappings they create in this
> > * vector which is parsed during destruct to unmap any memory mappings which
> > * completed successfully.
> > */
> > @@ -167,7 +179,8 @@ MappedBuffer::~MappedBuffer()
> > */
> > MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> > {
> > - maps_.reserve(buffer->planes().size());
> > + ASSERT(!buffer->planes().empty());
> > + planes_.reserve(buffer->planes().size());
> >
> > int mmapFlags = 0;
> >
> > @@ -177,17 +190,53 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> > if (flags & MapFlag::Write)
> > mmapFlags |= PROT_WRITE;
> >
> > + struct MappedBufferInfo {
> > + uint8_t *address = nullptr;
> > + size_t mapLength = 0;
> > + size_t dmabufLength = 0;
> > + };
> > + std::map<int, MappedBufferInfo> mappedBuffers;
> > + for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > + const int fd = plane.fd.fd();
> > + if (mappedBuffers.find(fd) == mappedBuffers.end()) {
> > + const size_t length = lseek(fd, 0, SEEK_END);
> > + mappedBuffers[fd] = MappedBufferInfo{ nullptr, 0, length };
> > + }
> > +
> > + const size_t length = mappedBuffers[fd].dmabufLength;
> > +
> > + if (plane.offset > length ||
> > + plane.offset + plane.length > length) {
> > + LOG(Buffer, Fatal) << "plane is out of buffer: "
> > + << "buffer length=" << length
> > + << ", plane offset=" << plane.offset
> > + << ", plane length=" << plane.length;
By the way, this patch breaks multiple unit tests:
[192:35:55.294272353] [19738] FATAL Buffer mapped_framebuffer.cpp:210 plane is out of buffer: buffer length=6221824, plane offset=969921872, plane length=6220800
The issue is fixed later on in the series, in patch 08/10. It would be
nice to avoid breaking bisection, which I think can simply be done by
moving the plane.offset = 0 in v4l2_videodevice.cpp from patch 08/10 to
01/10.
The reason I noticed is that the V4L2 buffer cache unit test is broken
by the series in 08/10, and I tried to check if 07/10 worked properly.
> > + return;
> > + }
> > + size_t &mapLength = mappedBuffers[fd].mapLength;
> > + mapLength = std::max(mapLength,
> > + static_cast<size_t>(plane.offset + plane.length));
> > + }
> > +
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > - void *address = mmap(nullptr, plane.length, mmapFlags,
> > - MAP_SHARED, plane.fd.fd(), 0);
> > - if (address == MAP_FAILED) {
> > - error_ = -errno;
> > - LOG(Buffer, Error) << "Failed to mmap plane: "
> > - << strerror(-error_);
> > - break;
> > + const int fd = plane.fd.fd();
> > + auto &info = mappedBuffers[fd];
> > + if (!info.address) {
> > + info.address =
> > + static_cast<uint8_t *>(
> > + mmap(nullptr, info.mapLength, mmapFlags,
> > + MAP_SHARED, fd, 0));
> > + if (info.address == MAP_FAILED) {
>
> I think the following would be more readable:
>
> void *addr = mmap(nullptr, info.mapLength, mmapFlags,
> MAP_SHARED, fd, 0));
> if (addr == MAP_FAILED) {
> ...
> }
>
> info.address = static_cast<uint8_t *>(addr);
>
> Overall the patch looks quite elegant to me :-)
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + error_ = -errno;
> > + LOG(Buffer, Error) << "Failed to mmap plane: "
> > + << strerror(-error_);
> > + return;
> > + }
> > +
> > + maps_.emplace_back(info.address, info.mapLength);
> > }
> >
> > - maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> > + planes_.emplace_back(info.address + plane.offset, plane.length);
> > }
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list