[libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and BufferPool classes

[Jacopo Mondi]

Hi Kieran,
   thanks for the patch

On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Provide classes that represent frame buffers and pools of frame buffers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> ---
> Kieran
>  - Fix checkstyle.py diff
> ---
>  include/libcamera/buffer.h    |  55 ++++++++++++++++++
>  include/libcamera/libcamera.h |   1 +
>  include/libcamera/meson.build |   1 +
>  src/libcamera/buffer.cpp      | 102 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  5 files changed, 160 insertions(+)
>  create mode 100644 include/libcamera/buffer.h
>  create mode 100644 src/libcamera/buffer.cpp
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> new file mode 100644
> index 000000000000..97c8025d9e77
> --- /dev/null
> +++ b/include/libcamera/buffer.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.

2019 ?

> + *
> + * buffer.h - Buffer handling
> + */
> +#ifndef __LIBCAMERA_BUFFER_H__
> +#define __LIBCAMERA_BUFFER_H__
> +
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class Buffer
> +{
> +public:
> +	Buffer();
> +	~Buffer();
> +
> +	int dmabuf() const { return fd_; };
> +	int setDmabuf(int fd);
> +
> +	int mmap();
> +	void munmap();
> +
> +private:
> +	int fd_;
> +};
> +
> +class BufferPool
> +{
> +public:
> +	enum Memory {
> +		Internal,
> +		External,
> +	};
> +
> +	BufferPool(Memory memory);
> +	virtual ~BufferPool();
> +
> +	int allocate(unsigned int count);
> +	void free();
> +
> +	unsigned int count() const { return buffers_.size(); };
> +
> +private:
> +	virtual int allocateMemory() = 0;
> +
> +	BufferPool::Memory memory_;
> +	std::vector<Buffer *> buffers_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_BUFFER_H__ */
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index c0511cf6d662..6619e556ebbd 100644
> --- a/include/libcamera/libcamera.h
> +++ b/include/libcamera/libcamera.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_LIBCAMERA_H__
>  #define __LIBCAMERA_LIBCAMERA_H__
>
> +#include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/event_dispatcher.h>
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index d7cb55ba4a49..8fcbbf7892ea 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,4 +1,5 @@
>  libcamera_api = files([
> +    'buffer.h',
>      'camera.h',
>      'camera_manager.h',
>      'event_dispatcher.h',
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> new file mode 100644
> index 000000000000..6dfebfc6bb28
> --- /dev/null
> +++ b/src/libcamera/buffer.cpp
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.

2019 ?

> + *
> + * buffer.cpp - Buffer handling
> + */
> +
> +#include <errno.h>
> +#include <unistd.h>
> +
> +#include <libcamera/buffer.h>
> +
> +/**
> + * \file buffer.h
> + * \brief Buffer handling
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Buffer
> + * \brief A memory buffer to store a frame
> + *
> + * The Buffer class represents a memory buffer used to store a frame.
> + */
> +Buffer::Buffer()
> +	: fd_(-1)
> +{
> +}
> +
> +Buffer::~Buffer()
> +{
> +	if (fd_ != -1)
> +		close(fd_);
> +}
> +
> +/**
> + * \fn Buffer::dmabuf()
> + * \brief Get the dmabuf file handle backing the buffer
> + */
> +
> +/**
> + * \brief Set the dmabuf file handle backing the buffer
> + *
> + * The \a fd dmabuf file handle is duplicated and stored.
> + */
> +int Buffer::setDmabuf(int fd)
> +{
> +	if (fd_ != -1) {
> +		close(fd_);
> +		fd_ = -1;
> +	}
> +
> +	if (fd != -1)
> +		return 0;

I didn't get this... Shouldn't we exit if (fd == -1) ?

> +
> +	fd_ = dup(fd);
> +	if (fd_ == -1)

A more approriate error handling sequence might be considered here.

> +		return -errno;
> +
> +	return 0;
> +}
> +
> +/**
> + * \class BufferPool
> + * \brief A pool of buffers
> + */
> +BufferPool::BufferPool(BufferPool::Memory memory)
> +	: memory_(memory)
> +{
> +}
> +
> +BufferPool::~BufferPool()
> +{
> +	free();
> +}
> +
> +int BufferPool::allocate(unsigned int count)
> +{
> +	for (unsigned int i = 0; i < count; ++i) {
> +		Buffer *buffer = new Buffer();
> +		buffers_.push_back(buffer);
> +	}
> +
> +	if (memory_ == Memory::Internal) {

The Internal vs External distinction matches the "exporter" vs
"importer" one for the DMABUF use case, or has other purposes?
Shouldn't we use then use Importer vs Exporter not to add another
term?

> +		int ret = allocateMemory();

This is left to subclasses, which I do expect to use MMAP or DMABUF,
and each subclass will have its own specific fields. I wonder if it is
worth to provide a base class like Buffer which is not pure virtual.

In example, importer vs exporter does not apply to mmap buffers, am I
right? Also, why would a DMABUF implementation expose a mmap() and
unmap() method?

Have you considered having Buffer as pure virtual, and use it keep the
vector<Buffers *> in the pool, but move most of the details and
implementations in subclasses?

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void BufferPool::free()
> +{
> +	for (Buffer *buffer : buffers_)
> +		delete buffer;
> +
> +	buffers_.clear();

I think buffers_ will be destroyed anyway, so you could drop this.

Thanks
  j

> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index f9f25c0ecf15..b5b25965fd12 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,4 +1,5 @@
>  libcamera_sources = files([
> +    'buffer.cpp',
>      'camera.cpp',
>      'camera_manager.cpp',
>      'device_enumerator.cpp',
> --
> 2.19.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190131/60be8ef3/attachment.sig>