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

441 diff mbox series

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Provide classes that represent frame buffers and pools of frame buffers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@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

Hi Kieran,
   thanks for the patch

On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Provide classes that represent frame buffers and pools of frame buffers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On 31/01/2019 18:34, Jacopo Mondi wrote:
> Hi Kieran,
>    thanks for the patch
> 
> On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:
>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> Provide classes that represent frame buffers and pools of frame buffers.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@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 ?

Laurent created this patch, in 2018. I've just been adding on top of his
patch. (except for one small issue I must have seen with checkstyle).

I guess I can update the year too ...


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

ah yes - I spotted this before - I thought I fixed it.
Looks like I must have dropped/lost the fix.

I'll update now so it doesn't get lost.


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

Playing this through - I'm becoming less convinced that we need a
Memory::Internal.

I'd like to be able restrict the creation of pools to the 'Device' which
will own them. And at that point, it can store a pointer of that device.

Then - if Pool.device == Device Buffers are internal.
Else Buffers are external.


I haven't yet seen what benefit setting the memory_ provides - but I'm
having issues creating the BufferPools anyway. It's a WIP. (see below)



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

I think that's sort of the way that my current branch is heading. I'm
trying to get the V4L2Device to create the objects directly. Having some
issues with casting and storage though.

I have a V4L2Buffer which allows instantiation of the Buffer object from
the V4L2 device.

But if I create a V4L2BufferPool - I'm hitting issues with how to
express a vector of Buffer objects - that are also (or rather are in
fact) V4L2Buffer objects.


In summary - I believe I agree with you here. The interface at Buffer.h
should be just that. The interface exposed to the application, with as
generic implementation as possible.



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

This function is named free(). It's not a destructor. So at this stage,
I think this .clear() needs to be kept.

Granted, it's only use is in the destructor - but if someone were to
call free() directly - it should clear() the vector.



> 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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran,

On Thu, Jan 31, 2019 at 07:00:31PM +0000, Kieran Bingham wrote:
> On 31/01/2019 18:34, Jacopo Mondi wrote:
> > On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:
> >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> Provide classes that represent frame buffers and pools of frame buffers.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@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

[snip]

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

[snip]

> >> +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?
> 
> Playing this through - I'm becoming less convinced that we need a
> Memory::Internal.
> 
> I'd like to be able restrict the creation of pools to the 'Device' which
> will own them. And at that point, it can store a pointer of that device.
> 
> Then - if Pool.device == Device Buffers are internal.
> Else Buffers are external.
> 
> 
> I haven't yet seen what benefit setting the memory_ provides - but I'm
> having issues creating the BufferPools anyway. It's a WIP. (see below)

Looking at this from an application point of view, the application needs
a pool of buffers per stream. The pool shall be provided by libcamera,
not by the application, but the memory backing the buffers could be
provided by either libcamera (internal allocation) or the application
(external allocation). We thus need a way for an application to request
a pool with a given number of buffers, and to specify whether buffers
should be allocated by libcamera (in which case the pool will be
associated with a V4L2 device) or will be provided externally (in which
case the pool doesn't need to be associated with a V4L2 device).

You will obviously need, in both cases, to request V4L2 buffers of type
MMAP or DMABUF, but that should be decoupled from the buffers pool
itself. Please keep in mind that the buffers pool class will be used
internally to share buffers between devices (for instance between the
IPU3 CIO2 and ImgU), where you will have a single buffers pool but two
V4L2 buffers queues. The pool is thus distinct from the queues.

This being said, when an application requests a buffers pool with
internal allocation, the allocation of buffers will be delegated to the
V4L2 device, so you may certainly implement a V4L2BufferPool class for
this case.

> >> +		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?
> 
> I think that's sort of the way that my current branch is heading. I'm
> trying to get the V4L2Device to create the objects directly. Having some
> issues with casting and storage though.
> 
> I have a V4L2Buffer which allows instantiation of the Buffer object from
> the V4L2 device.
> 
> But if I create a V4L2BufferPool - I'm hitting issues with how to
> express a vector of Buffer objects - that are also (or rather are in
> fact) V4L2Buffer objects.
> 
> 
> In summary - I believe I agree with you here. The interface at Buffer.h
> should be just that. The interface exposed to the application, with as
> generic implementation as possible.
> 
> >> +		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.
> > 
> 
> This function is named free(). It's not a destructor. So at this stage,
> I think this .clear() needs to be kept.
> 
> Granted, it's only use is in the destructor - but if someone were to
> call free() directly - it should clear() the vector.
> 
> >> +}
> >> +
> >> +} /* 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',

Hi Laurent,


On 01/02/2019 01:52, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Thu, Jan 31, 2019 at 07:00:31PM +0000, Kieran Bingham wrote:
>> On 31/01/2019 18:34, Jacopo Mondi wrote:
>>> On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:
>>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> Provide classes that represent frame buffers and pools of frame buffers.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@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
> 
> [snip]
> 
>>>> 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
> 
> [snip]
> 
>>>> +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?
>>
>> Playing this through - I'm becoming less convinced that we need a
>> Memory::Internal.
>>
>> I'd like to be able restrict the creation of pools to the 'Device' which
>> will own them. And at that point, it can store a pointer of that device.
>>
>> Then - if Pool.device == Device Buffers are internal.
>> Else Buffers are external.
>>
>>
>> I haven't yet seen what benefit setting the memory_ provides - but I'm
>> having issues creating the BufferPools anyway. It's a WIP. (see below)
> 
> Looking at this from an application point of view, the application needs
> a pool of buffers per stream. The pool shall be provided by libcamera,
> not by the application, but the memory backing the buffers could be
> provided by either libcamera (internal allocation) or the application
> (external allocation). We thus need a way for an application to request
> a pool with a given number of buffers, and to specify whether buffers
> should be allocated by libcamera (in which case the pool will be
> associated with a V4L2 device) or will be provided externally (in which
> case the pool doesn't need to be associated with a V4L2 device).
> 
> You will obviously need, in both cases, to request V4L2 buffers of type
> MMAP or DMABUF, but that should be decoupled from the buffers pool
> itself. Please keep in mind that the buffers pool class will be used
> internally to share buffers between devices (for instance between the
> IPU3 CIO2 and ImgU), where you will have a single buffers pool but two
> V4L2 buffers queues. The pool is thus distinct from the queues.
> 
> This being said, when an application requests a buffers pool with
> internal allocation, the allocation of buffers will be delegated to the
> V4L2 device, so you may certainly implement a V4L2BufferPool class for
> this case.



Ah yes - I was getting distracted by internal sharing of buffers between
V4L2Devices inside the library (i.e. CIO2->IMGU)

Providing buffers externally will need some design to be able to import
the buffers.

Do we need that /now/ or is this a feature for later?


At the moment, my branch was progressing towards the V4L2 device being
able to operate in either two modes.

Either it creates the buffers - and exports a BufferPool, or it imports
a BufferPool. ...


>>>> +		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?
>>
>> I think that's sort of the way that my current branch is heading. I'm
>> trying to get the V4L2Device to create the objects directly. Having some
>> issues with casting and storage though.
>>
>> I have a V4L2Buffer which allows instantiation of the Buffer object from
>> the V4L2 device.
>>
>> But if I create a V4L2BufferPool - I'm hitting issues with how to
>> express a vector of Buffer objects - that are also (or rather are in
>> fact) V4L2Buffer objects.
>>
>>
>> In summary - I believe I agree with you here. The interface at Buffer.h
>> should be just that. The interface exposed to the application, with as
>> generic implementation as possible.
>>
>>>> +		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.
>>>
>>
>> This function is named free(). It's not a destructor. So at this stage,
>> I think this .clear() needs to be kept.
>>
>> Granted, it's only use is in the destructor - but if someone were to
>> call free() directly - it should clear() the vector.
>>
>>>> +}
>>>> +
>>>> +} /* 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',
>