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

18945 diff mbox series

As it is written, the MappedFrameBuffer causes a failure in mmap when
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
this is not the case.

Signed-off-by: Gabby George <gabbymg94@gmail.com>
---
 .../libcamera/internal/mapped_framebuffer.h   |  3 ++-
 src/libcamera/mapped_framebuffer.cpp          | 20 ++++++++++++++-----
 2 files changed, 17 insertions(+), 6 deletions(-)

Hello Gabby,

Thanks for your patch !

On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94@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@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

Hello,

On Mon, Aug 21, 2023 at 07:31:29PM +0530, Vedant Paranjape via libcamera-devel wrote:
> On Mon, Aug 21, 2023 at 6:40 PM Gabby George 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

I think you meant dmabuf file descriptors here. The precision is
important, as file descriptors can be many things.

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

I don't think I like this :-( It feels a bit of an abuse of this class.
I'd rather implement the mapping manually in the UVC pipeline handler.

> > 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@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);
> >         }
> >  }
> >