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

[Vedant Paranjape]

Hello Gabby,

Thanks for your patch !

On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94 at gmail.com> wrote:
>
> As it is written, the MappedFrameBuffer causes a failure in mmap when

s/As it is written/ /
s/the/The

> attempting to map UVC metadata planes.  UVC metadata (four character
> code UVCH) does not support exporting buffers as file descriptors, so
> mmap can be used to give libcamera access to the metadata buffer
> planes. It is convenient to use the already-existing
> MappedFrameBuffers class, but the class must be modified to support
> mapping using file descriptors of the video node itself.
>
> To do this, mmap needs information obtained through a call to
> QUERYBUF, namely, the plane offset for buffer planes. Modify the
> constructor of a MappedFrameBuffer to use the plane offset directly in
> the call to mmap, rather than the hard-coded 0 value.
>
> The current version does not work when a buffer cannot be exported as
> a dma buf fd. The fd argument to mmap must be the one obtained on a
> call to open() for the video node (ie, /dev/videoX). This method of
> mapping buffer planes requires the arguments to mmap to be exactly the
> length and offset QUERYBUF provides.  Mmap will return a -EINVAL if

s/Mmap/mmap

> this is not the case.
>
> Signed-off-by: Gabby George <gabbymg94 at gmail.com>
> ---
>  .../libcamera/internal/mapped_framebuffer.h   |  3 ++-
>  src/libcamera/mapped_framebuffer.cpp          | 20 ++++++++++++++-----
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> index fb39adbf..04096d1b 100644
> --- a/include/libcamera/internal/mapped_framebuffer.h
> +++ b/include/libcamera/internal/mapped_framebuffer.h
> @@ -54,7 +54,8 @@ 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..3d2cc332 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);
> +                       }

You could make this an oneliner using ternary operators, it would look
more compact I believe.

size_t length = usePlaneOffset ? plane.length : info.mapLength ;
off_t offset = usePlaneOffset ? plane.offset : 0
void *address = mmap(.., length, ..., offset);

>                         if (address == MAP_FAILED) {
>                                 error_ = -errno;
>                                 LOG(Buffer, Error) << "Failed to mmap plane: "
> @@ -233,10 +240,13 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
>                         }
>
>                         info.address = static_cast<uint8_t *>(address);
> -                       maps_.emplace_back(info.address, info.mapLength);
> +                       maps_.emplace_back(info.address, usePlaneOffset ? plane.length :
> +                                                                                                                         info.mapLength);
>                 }
>
> -               planes_.emplace_back(info.address + plane.offset, plane.length);
> +               uint8_t *storedAddress = usePlaneOffset ? info.address :
> +                                                         info.address + plane.offset;

I maybe wrong here, but looking at the code, shouldn't the operators
be opposite ? something like this ?

uint8_t *storedAddress = usePlaneOffset ? info.address + plane.offset
: info.address;

> +               planes_.emplace_back(storedAddress, plane.length);
>         }
>  }
>
> --
> 2.34.1
>

Regards,
Vedant Paranjape