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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart