[libcamera-devel] [RFC PATCH 03/10] libcamera: mapped_framebuffer: Return plane begin address by MappedBuffer::maps()

[Hirokazu Honda]

Hi Laurent, thank you for the comment.

On Wed, Aug 18, 2021 at 8:46 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> 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

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.

Best Regards,
-Hiro

> >  }
> >
> >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart