[PATCH] libcamera: dma_heaps: Add support for using udmabuf to alloc DMA heaps

[Hans de Goede]

Hi,

On 5/29/24 6:43 PM, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-05-29 17:26:50)
>> Hi,
>>
>> On 5/28/24 11:37 PM, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:
>>>> Quoting Hans de Goede (2024-05-27 15:16:47)
>>>>> Add support for using udmabuf to alloc DMA heaps, this is basically:
>>>>> https://patchwork.libcamera.org/patch/18922/
>>>>>
>>>>> ported from the never merged HeapAllocator class to the current DmaHeap
>>>>> class.
>>>>
>>>> True statements, but not really specific to the content of the patch?
>>>>
>>>> Perhaps:
>>>>
>>>> """
>>>> The DMA heap allocator currently allocates from CMA and system heaps.
>>>>
>>>> These provide contiguous device memory but require dedicated and
>>>> reserved memory regions to be configured on the platform and may require
>>>> elevated privilidges to access.
>>>
>>> The system heap doesn't require reserved memory regions, and also
>>> doesn't provide contiguous device memory. The CMA heap also doesn't
>>> required reserved memory regions actually.
>>>
>>>> Extend the dma_heap allocator to support falling back to the memfd
>>>> allocator backed by the udmabuf framework to prepare buffers that are
>>>> compatible with linux drivers and the libcamera API.
>>>
>>> When reading the patch, my very first thought is that udmabuf doesn't
>>> make use of DMA heaps, so the allocator should belong to a different
>>> class.
>>>
>>> Thinking a bit more about it, I understand the appeal of hiding this
>>> fallback from the DmaHeap user. I'm however a bit concerned that the
>>> dmabuf instances allocated from the DMA system heap and through udmabuf
>>> will not operate in a fully identical way. Returning one or the other
>>> without the user being aware of what has been allocated may lead to
>>> issues.
>>>
>>> I may worry needlessly though. I haven't compared the system heap and
>>> the udmabuf implementations, to see if the dmabuf operations differ in
>>> ways that are significant. I think this should at least be checked
>>> before we make a decision.
>>>
>>>> The buffers allocated through memfd/udmabuf will not be contiguous and
>>>> are likely not suitable for directly rendering to a display or encoder
>>>> but may facilitate more usecases for allocating memory in pipelines that
>>>
>>> Let's make a stronger statement : "are not suitable for zero-copy buffer
>>> sharing with other devices".
>>
>> Sounds good I'll come up with a new commit message for v2 taking all the various
>> remarks into account.
>>
>>>> make use of the CPU for processing including virtual pipeline handlers
>>>> or the SoftISP.
>>>> """
>>>>
>>>> Feel free to adapt the above of course if I've made any errors.
>>>>
>>>>> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
>>>>> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
>>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>>> ---
>>>>>  include/libcamera/internal/dma_heaps.h |   3 +
>>>>>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
>>>>>  2 files changed, 109 insertions(+), 21 deletions(-)
> 
> ... snip ...
> 
>>>>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
>>>>> +{
>>>>> +       /* size must be a multiple of the page-size round it up */
>>>
>>> s/size/Size/
>>> s/ up/ up./
>>
>> Ack.
>>
>>>>> +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>>>>> +       size = (size + pageMask) & ~pageMask;
>>>>> +
>>>>> +       int ret = memfd_create(name, MFD_ALLOW_SEALING);
>>>
>>> Needs sys/mman.h.
>>
>> Ack.
> 
> ... snip ...
> 
>>
>>>>> +               return {};
>>>>> +       }
>>>>> +
>>>>> +       if (create.size != size)
>>>>> +               LOG(DmaHeap, Warning)
>>>>> +                       << "UdmaHeap allocated " << create.size << " bytes, "
>>>>> +                       << "which is greater than requested : " << size << " bytes";
>>>
>>> Why/when does this happen ?
>>
>> This was taken from the original patch which I based this on. Shall I drop
>> the check ?
> 
> I'm fine dropping it. I bet it was from my prototyping code where I was
> likely investigating why I didn't get the size I requested,

Ok, I'll drop it.

> which
> probably led to the comment "size must be a multiple of the page-size
> round it up" at the top.

The page-rounding of size is actually an addition by me, otherwise the
UDMABUF_CREATE call fails with errno=EINVAL.

Regards,

Hans