| Message ID | 20230814112849.176943-3-gabbymg94@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Gabby George (2023-08-14 12:28:46) > When calling mmap, the mapped frame buffer class hard-codes the offset value to 0. TODO: EDIT ME WHEN YOU HAVE MORE VERBAL REASONING SKILLS Interesting TODO item ;-) > > Signed-off-by: Gabby George <gabbymg94@gmail.com> > --- > include/libcamera/internal/mapped_framebuffer.h | 2 +- > src/libcamera/mapped_framebuffer.cpp | 16 ++++++++++++---- > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > index fb39adbf..fac86344 100644 > --- a/include/libcamera/internal/mapped_framebuffer.h > +++ b/include/libcamera/internal/mapped_framebuffer.h > @@ -54,7 +54,7 @@ public: > > using MapFlags = Flags<MapFlag>; > > - MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); > + MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags, bool usePlaneOffset = false); > }; > > LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag) > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > index 6860069b..fcbb38ec 100644 > --- a/src/libcamera/mapped_framebuffer.cpp > +++ b/src/libcamera/mapped_framebuffer.cpp > @@ -172,12 +172,13 @@ MappedBuffer::~MappedBuffer() > * \brief Map all planes of a FrameBuffer > * \param[in] buffer FrameBuffer to be mapped > * \param[in] flags Protection flags to apply to map > + * \param[in] usePlaneOffset Use offset stored in buffer's plane; default false > * > * Construct an object to map a frame buffer for CPU access. The mapping can be > * made as Read only, Write only or support Read and Write operations by setting > * the MapFlag flags accordingly. > */ > -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) > +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags, bool usePlaneOffset) > { > ASSERT(!buffer->planes().empty()); > planes_.reserve(buffer->planes().size()); > @@ -223,8 +224,14 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) > const int fd = plane.fd.get(); > auto &info = mappedBuffers[fd]; > if (!info.address) { > - void *address = mmap(nullptr, info.mapLength, mmapFlags, > - MAP_SHARED, fd, 0); > + void *address; > + if (usePlaneOffset) { I don't think we should hide this behind a flag. We should trust the offset in the FrameBuffer. If the offset is not to be set, or rather is truely 0, it should be correctly initialised to 0 whenever a FrameBuffer is constructed. Hrm ... seeing the storedAddress calculation below worries me that what I've written above might not be true - but I hope we can work through and find what uses the offsets either way and make sure they're all consistent. I still hope we don't need to hide behind a flag here. > + address = mmap(nullptr, plane.length, mmapFlags, > + MAP_SHARED, fd, plane.offset); > + } else { > + address = mmap(nullptr, info.mapLength, mmapFlags, > + MAP_SHARED, fd, 0); > + } > if (address == MAP_FAILED) { > error_ = -errno; > LOG(Buffer, Error) << "Failed to mmap plane: " > @@ -236,7 +243,8 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) > maps_.emplace_back(info.address, info.mapLength); > } > > - planes_.emplace_back(info.address + plane.offset, plane.length); > + uint8_t *storedAddress = usePlaneOffset ? info.address : info.address + plane.offset; Hrm ... what's going on here though. I suspect if we are always mapping with the plane.offset, then here we should no longer 'add' the offset at all... Did this trigger anything in the unit tests ? Have you run the unit tests yet? - if not - please run ... sudo modprobe vimc sudo modprobe vivid sudo modprobe vim2m and then from your build directory: ninja -C build test Though now I check, you might need to make sure your libcamera configuration has testing enabled: perhaps (if you need it) meson --reconfigure build -Dtest=true > + planes_.emplace_back(storedAddress, plane.length); > } > } > > -- > 2.34.1 >
diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h index fb39adbf..fac86344 100644 --- a/include/libcamera/internal/mapped_framebuffer.h +++ b/include/libcamera/internal/mapped_framebuffer.h @@ -54,7 +54,7 @@ public: using MapFlags = Flags<MapFlag>; - MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); + MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags, bool usePlaneOffset = false); }; LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag) diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp index 6860069b..fcbb38ec 100644 --- a/src/libcamera/mapped_framebuffer.cpp +++ b/src/libcamera/mapped_framebuffer.cpp @@ -172,12 +172,13 @@ MappedBuffer::~MappedBuffer() * \brief Map all planes of a FrameBuffer * \param[in] buffer FrameBuffer to be mapped * \param[in] flags Protection flags to apply to map + * \param[in] usePlaneOffset Use offset stored in buffer's plane; default false * * Construct an object to map a frame buffer for CPU access. The mapping can be * made as Read only, Write only or support Read and Write operations by setting * the MapFlag flags accordingly. */ -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags, bool usePlaneOffset) { ASSERT(!buffer->planes().empty()); planes_.reserve(buffer->planes().size()); @@ -223,8 +224,14 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) const int fd = plane.fd.get(); auto &info = mappedBuffers[fd]; if (!info.address) { - void *address = mmap(nullptr, info.mapLength, mmapFlags, - MAP_SHARED, fd, 0); + void *address; + if (usePlaneOffset) { + address = mmap(nullptr, plane.length, mmapFlags, + MAP_SHARED, fd, plane.offset); + } else { + address = mmap(nullptr, info.mapLength, mmapFlags, + MAP_SHARED, fd, 0); + } if (address == MAP_FAILED) { error_ = -errno; LOG(Buffer, Error) << "Failed to mmap plane: " @@ -236,7 +243,8 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags) maps_.emplace_back(info.address, info.mapLength); } - planes_.emplace_back(info.address + plane.offset, plane.length); + uint8_t *storedAddress = usePlaneOffset ? info.address : info.address + plane.offset; + planes_.emplace_back(storedAddress, plane.length); } }
When calling mmap, the mapped frame buffer class hard-codes the offset value to 0. TODO: EDIT ME WHEN YOU HAVE MORE VERBAL REASONING SKILLS Signed-off-by: Gabby George <gabbymg94@gmail.com> --- include/libcamera/internal/mapped_framebuffer.h | 2 +- src/libcamera/mapped_framebuffer.cpp | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-)