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

[Kieran Bingham]

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 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
> 
> [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',
> 

-- 
Regards
--
Kieran