[libcamera-devel] [RFC PATCH v2 01/10] libcamera: framebuffer: Add offset to FrameBuffer::Plane

[Laurent Pinchart]

Hi Tomasz,

On Fri, Aug 27, 2021 at 06:49:41PM +0900, Tomasz Figa wrote:
> On Tue, Aug 24, 2021 at 7:09 AM Laurent Pinchart wrote:
> > On Mon, Aug 23, 2021 at 10:12:12PM +0900, Hirokazu Honda wrote:
> > > This adds offset to FrameBuffer::Plane. It enables representing frame
> > > buffers that store planes in the same dmabuf at different offsets, as
> > > for instance required by the V4L2 NV12 pixel format.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  include/libcamera/framebuffer.h          |  1 +
> > >  src/android/mm/generic_camera_buffer.cpp |  2 --
> > >  src/libcamera/framebuffer.cpp            | 27 ++++++++++++++----------
> > >  3 files changed, 17 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > index 28307890..5de3c744 100644
> > > --- a/include/libcamera/framebuffer.h
> > > +++ b/include/libcamera/framebuffer.h
> > > @@ -42,6 +42,7 @@ class FrameBuffer final : public Extensible
> > >  public:
> > >       struct Plane {
> > >               FileDescriptor fd;
> > > +             unsigned int offset;
> > >               unsigned int length;
> > >       };
> > >
> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > > index b296b3e3..73fbd97a 100644
> > > --- a/src/android/mm/generic_camera_buffer.cpp
> > > +++ b/src/android/mm/generic_camera_buffer.cpp
> > > @@ -52,8 +52,6 @@ private:
> > >       int fd_;
> > >       int flags_;
> > >       std::vector<PlaneInfo> planeInfo_;
> > > -     /* \todo remove planes_ is added to MappedBuffer. */
> > > -     std::vector<Span<uint8_t>> planes_;
> >
> > This breaks compilation, as the planes_ member is added to MappedBuffer
> > in "libcamera: mapped_framebuffer: Return plane begin address by
> > MappedBuffer::maps()".
> >
> > >  };
> > >
> > >  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > index b401c50e..e9467aa0 100644
> > > --- a/src/libcamera/framebuffer.cpp
> > > +++ b/src/libcamera/framebuffer.cpp
> > > @@ -131,7 +131,7 @@ FrameBuffer::Private::Private()
> > >   *
> > >   * The static information describes the memory planes that make a frame. The
> > >   * planes are specified when creating the FrameBuffer and are expressed as a set
> > > - * of dmabuf file descriptors and length.
> > > + * of dmabuf file descriptors, offset and length.
> > >   *
> > >   * The dynamic information is grouped in a FrameMetadata instance. It is updated
> > >   * during the processing of a queued capture request, and is valid from the
> > > @@ -151,18 +151,18 @@ FrameBuffer::Private::Private()
> > >   *
> > >   * Planar pixel formats use multiple memory regions to store the different
> > >   * colour components of a frame. The Plane structure describes such a memory
> > > - * region by a dmabuf file descriptor and a length. A FrameBuffer then
> > > - * contains one or multiple planes, depending on the pixel format of the
> > > - * frames it is meant to store.
> > > + * region by a dmabuf file descriptor, an offset within the dmabuf and a length.
> > > + * A FrameBuffer then contains one or multiple planes, depending on the pixel
> > > + * format of the frames it is meant to store.
> > >   *
> > > - * To support DMA access, planes are associated with dmabuf objects represented
> > > - * by FileDescriptor handles. The Plane class doesn't handle mapping of the
> > > - * memory to the CPU, but applications and IPAs may use the dmabuf file
> > > - * descriptors to map the plane memory with mmap() and access its contents.
> >
> > I think this paragraph should be kept, it's still valid.
> 
> mmap() doesn't always lead to a meaningful view of the buffer. There
> are buffers which use compressed or tiled layouts which have to be
> either decompressed/detiled first or mapped through a translation unit
> with a specific driver API (often called GTT in the DRM world).
> 
> I think it might be a better choice to provide a mapping API and
> handle those special cases appropriately.

Isn't that the job of a buffer allocator API ? It's a big can of worms,
and not something specific to libcamera.

> > > + * The offset identifies the location of the plane data from the start of the
> > > + * memory referenced by the dmabuf file descriptor. Multiple planes may be
> > > + * stored in the same dmabuf, in which case they will reference the same dmabuf
> > > + * and different offsets. No two planes may overlap, as specified by their
> > > + * offset and length.
> > >   *
> > > - * \todo Once we have a Kernel API which can express offsets within a plane
> > > - * this structure shall be extended to contain this information. See commit
> > > - * 83148ce8be55e for initial documentation of this feature.
> > > + * \todo Specify how an application shall decide whether to use a single or
> > > + * multiple dmabufs, based on the camera requirements.
> > >   */
> > >
> > >  /**
> > > @@ -170,6 +170,11 @@ FrameBuffer::Private::Private()
> > >   * \brief The dmabuf file descriptor
> > >   */
> > >
> > > +/**
> > > + * \var FrameBuffer::Plane::offset
> > > + * \brief The plane offset in bytes
> > > +*/
> > > +
> > >  /**
> > >   * \var FrameBuffer::Plane::length
> > >   * \brief The plane length in bytes

-- 
Regards,

Laurent Pinchart