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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Aug 16, 2021 at 01:31:29PM +0900, Hirokazu Honda wrote:
> This adds offset to FrameBuffer::Plane. It enables represents V4L2
> multi-planar format and mapping with MappedFrameBuffer.

The change is good, but this isn't just about V4L2, especially given
that the FrameBuffer class doesn't know if V4L2 or a different API is
used by the pipeline handler. Could you write this to avoid the V4L2
reference, or at least to mention is as only an example ? Maybe
something as this:

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>
> ---
>  include/libcamera/framebuffer.h |  1 +
>  src/libcamera/framebuffer.cpp   | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 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/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 42c73134..1d7d5ea3 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,8 +151,8 @@ 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
> + * region by a dmabuf file descriptor, an offset and a length. A FrameBuffer

I'd write s/offset/offset within the dmabuf/

> + * then contains one or multiple planes, depending on the pixel format of the
>   * frames it is meant to store.

Now that we have offsets, I think they deserve a bit of a more detailed
explanation. How about adding the following paragraphs after the
following one, just before the todo comment ?

 * 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 Specify how an application shall decide whether to use a single or
 * multiple dmabufs, based on the camera requirements.

>   *
>   * To support DMA access, planes are associated with dmabuf objects represented

There's a todo comment at the end of this documentation block related to
addition of the offset field. You can drop it.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

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