[libcamera-devel] [RFC PATCH 2/5] libcamera: MappedFrameBuffer: Use stored plane offset

[Kieran Bingham]

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 at 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
>