[libcamera-devel] [RFC PATCH 03/10] libcamera: mapped_framebuffer: Return plane begin address by MappedBuffer::maps()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 20 14:10:21 CEST 2021
Hi Hiro,
On Fri, Aug 20, 2021 at 05:19:59PM +0900, Hirokazu Honda wrote:
> On Wed, Aug 18, 2021 at 8:46 AM Laurent Pinchart wrote:
> > On Mon, Aug 16, 2021 at 01:31:31PM +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 | 51 +++++++++++++++----
> > > 2 files changed, 44 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(). */
> >
> > I'm fine with a todo comment in this patch, but could you do the rename
> > as part of the series, maybe as a last patch on top ?
> >
> > > + 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..b0ba89b0 100644
> > > --- a/src/libcamera/mapped_framebuffer.cpp
> > > +++ b/src/libcamera/mapped_framebuffer.cpp
> > > @@ -9,6 +9,7 @@
> > >
> > > #include <errno.h>
> > > #include <sys/mman.h>
> > > +#include <unistd.h>
> > >
> > > #include <libcamera/base/log.h>
> > >
> > > @@ -79,6 +80,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 +129,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 +177,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,18 +188,38 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> > > if (flags & MapFlag::Write)
> > > mmapFlags |= PROT_WRITE;
> > >
> > > + int prevFd = -1;
> > > 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();
> > > + if (prevFd != fd) {
> >
> > Could there be a case where plane 1 and 3 share the same dmabuf but
> > plane 2 uses a different once ? It may be a bit far-fetched, but how
> > about turning the maps_ vector into a std::map<int, Plane> ? That way
> > we'll correctly support this case, with no overhead and an
> > implementation that shouldn't be more complicated.
> >
> > > + const size_t length = lseek(fd, 0, SEEK_END);
> > > + void *address = mmap(nullptr, length, mmapFlags,
> > > + MAP_SHARED, fd, 0);
> >
> > Hmmm... I could imagine that in some case the dmabuf would be larger
> > than the sum of the planes, and it may then be possible to only map part
> > of the dmabuf. Let's consider this for a possible future enhancement, as
> > virtual address space should hopefully not be a limiting factor in most
> > case. Maybe a todo comment would be appropriate ?
> >
> > /**
> > * \todo Should we try to only map the portions of the
> > * dmabuf that are used by planes ?
> > */
> >
> > > + if (address == MAP_FAILED) {
> > > + error_ = -errno;
> > > + LOG(Buffer, Error) << "Failed to mmap plane: "
> > > + << strerror(-error_);
> > > + return;
> > > + }
> > > + maps_.emplace_back(static_cast<uint8_t *>(address),
> > > + length);
> > > + prevFd = fd;
> > > }
> > >
> > > - maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> > > + const size_t length = maps_.back().size();
> > > + if (plane.offset + plane.length > length) {
> >
> > Should we protect ourselves against arithmetic overflows here ?
> >
> > if (plane.offset > length ||
> > plane.offset + plane.length > length) {
> >
>
> Thanks. By the way, ideally we should introduce something like
> base/numerics in chromium.
> https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/README.md
I was actually thinking about adding helpers to utils.h for this when
reviewing your patch :-) I think we'll end up doing so at some point.
> So I can do like
> size_t planeEnd;
> if (!CheckAdd(plane.offset, plane.length).AssignIfValid(planeEnd)) {
> // Overflow!
> }
> Or more aggressively if (base::checked_cast<size_t>(plane.offset,
> plane.length) > length), which causes process crash if overflow
> happens.
>
> > > + LOG(Buffer, Fatal) << "plane is out of buffer: "
> > > + << "buffer length=" << length
> > > + << ", plane offset=" << plane.offset
> > > + << ", plane length=" << plane.length;
> > > + return;
> > > + }
> > > +
> > > + uint8_t *buf = maps_.back().data();
> > > + planes_.emplace_back(buf + plane.offset, plane.length);
> > > }
> > > +
> > > + ASSERT(maps_.size() == 1u);
> >
> > I'm not sure to understand this. Won't a multi-planar frame buffer with
> > different dmabufs fail this assertion ?
>
> Right, this is brought from your comment to my previous solution.
> We should remove now.
>
> > > }
> > >
> > > } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list