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

13444 diff mbox series

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@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

Hi Hiro,

Thank you for the patch.

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@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.

> + * 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

On Tue, Aug 24, 2021 at 7:09 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> 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@chromium.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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.

>
> > + * 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

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@chromium.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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

On Mon, Aug 30, 2021 at 11:59 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@chromium.org>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
>

Well, isn't it the buffer allocator ultimately who only knows about
the nature of the buffer?

> > > > + * 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

Hi Tomasz,

On Tue, Aug 31, 2021 at 11:10:35AM +0900, Tomasz Figa wrote:
> On Mon, Aug 30, 2021 at 11:59 PM Laurent Pinchart wrote:
> > 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@chromium.org>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
> 
> Well, isn't it the buffer allocator ultimately who only knows about
> the nature of the buffer?

Exactly my point :-) libcamera imports buffers. It has a convenience API
to also allocate and export them if the application has no other way to
allocate buffers, but the main use case is import. When using the camera
HAL, libcamera operates in import mode only.

> > > > > + * 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

On Tue, Aug 31, 2021 at 11:44 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Tue, Aug 31, 2021 at 11:10:35AM +0900, Tomasz Figa wrote:
> > On Mon, Aug 30, 2021 at 11:59 PM Laurent Pinchart wrote:
> > > 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@chromium.org>
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
> >
> > Well, isn't it the buffer allocator ultimately who only knows about
> > the nature of the buffer?
>
> Exactly my point :-) libcamera imports buffers. It has a convenience API
> to also allocate and export them if the application has no other way to
> allocate buffers, but the main use case is import. When using the camera
> HAL, libcamera operates in import mode only.

Right, but within libcamera we still have consumers which may want to
map those buffers, but they are not aware of the allocator itself. So
we need some kind of abstraction to get back to the allocator to
obtain the mapping.

>
> > > > > > + * 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