| Message ID | 20210816043138.957984-4-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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) { > + 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 ? > } > > } /* namespace libcamera */
Hi Laurent, thank you for the comment. On Wed, Aug 18, 2021 at 8:46 AM Laurent Pinchart <laurent.pinchart@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@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
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@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 */
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..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) { + const size_t length = lseek(fd, 0, SEEK_END); + void *address = mmap(nullptr, length, mmapFlags, + MAP_SHARED, fd, 0); + 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) { + 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); } } /* namespace libcamera */
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@chromium.org> --- .../libcamera/internal/mapped_framebuffer.h | 4 +- src/libcamera/mapped_framebuffer.cpp | 51 +++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-)