[PATCH] libcamera: dma_heaps: Add support for using udmabuf to alloc DMA heaps
Hans de Goede
hdegoede at redhat.com
Wed May 29 18:26:50 CEST 2024
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(-)
>>>
>>> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
>>> index f0a8aa5d..7e1271ed 100644
>>> --- a/include/libcamera/internal/dma_heaps.h
>>> +++ b/include/libcamera/internal/dma_heaps.h
>>> @@ -30,7 +30,10 @@ public:
>>> UniqueFD alloc(const char *name, std::size_t size);
>>>
>>> private:
>>> + UniqueFD allocFromHeap(const char *name, std::size_t size);
>>> + UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
>>> UniqueFD dmaHeapHandle_;
>>> + bool useUDmaBuf_;
>>> };
>>>
>>> LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
>>> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
>>> index d4cb880b..bb707786 100644
>>> --- a/src/libcamera/dma_heaps.cpp
>>> +++ b/src/libcamera/dma_heaps.cpp
>>> @@ -10,10 +10,14 @@
>>> #include <array>
>>> #include <fcntl.h>
>>> #include <sys/ioctl.h>
>>> +#include <sys/mman.h>
>>> +#include <sys/stat.h>
>>> +#include <sys/types.h>
>>> #include <unistd.h>
>>>
>>> #include <linux/dma-buf.h>
>>> #include <linux/dma-heap.h>
>>> +#include <linux/udmabuf.h>
>>>
>>> #include <libcamera/base/log.h>
>>>
>>> @@ -36,13 +40,15 @@ namespace libcamera {
>>> struct DmaHeapInfo {
>>> DmaHeap::DmaHeapFlag type;
>>> const char *deviceNodeName;
>>> + bool useUDmaBuf;
>>> };
>>> #endif
>>>
>>> -static constexpr std::array<DmaHeapInfo, 3> heapInfos = { {
>>> - { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
>>> - { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
>>> - { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" },
>>> +static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
>>> + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
>>> + { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
>>> + { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
>>> + { DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },
>>
>> Should we distinguise UDMA with a different flag? or just simply keep it
>> as a fall back?
>>
>> If we do use a different DmaHeapFlag then I think we don't need the
>> boolean...
>
> Exposing the difference to the user may alleviate some of my concerns.
> Should we then rename the DmaHeap class to DmaBufAllocator ?
That sounds like a good idea. I'll add a patch to rename the class
as first patch in the series for v2 of the series.
>
>>> } };
>>>
>>> LOG_DEFINE_CATEGORY(DmaHeap)
>>> @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)
>>>
>>> LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName;
>>> dmaHeapHandle_ = UniqueFD(ret);
>>> + useUDmaBuf_ = info.useUDmaBuf;
>>> break;
>>> }
>>>
>>> @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;
>>> * \return True if the DmaHeap is valid, false otherwise
>>> */
>>>
>>> -/**
>>> - * \brief Allocate a dma-buf from the DmaHeap
>>> - * \param [in] name The name to set for the allocated buffer
>>> - * \param [in] size The size of the buffer to allocate
>>> - *
>>> - * Allocates a dma-buf with read/write access.
>>> - *
>>> - * If the allocation fails, return an invalid UniqueFD.
>>> - *
>>> - * \return The UniqueFD of the allocated buffer
>>> - */
>>> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>> +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)
>>> {
>>> - int ret;
>>> -
>>> - if (!name)
>>> - return {};
>>> -
>>> struct dma_heap_allocation_data alloc = {};
>>> + int ret;
>>>
>>> alloc.len = size;
>>> alloc.fd_flags = O_CLOEXEC | O_RDWR;
>>> @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>> return allocFd;
>>> }
>>>
>>> +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.
>>> + if (ret < 0) {
>>> + ret = errno;
>>> + LOG(DmaHeap, Error)
>>> + << "UdmaHeap failed to allocate memfd storage: "
>
> UdmaHeap sounds like a frankenstein monster :-) Did you mean UDmaBuf
> here ? I would just drop the prefix from this message and the ones below
> actually.
I'll drop the prefix from the log message for v2.
>>> + << strerror(ret);
>>
>> Completely optional/just an idea - I wonder if we should use 'name' in
>> the log messages here and below.
>>
>> Would probably apply to allocFromHeap() too so likely a patch on top -
>> not an update here.
>>
>>> + return {};
>>> + }
>>> +
>>> + UniqueFD memfd(ret);
>>> +
>>> + ret = ftruncate(memfd.get(), size);
>>> + if (ret < 0) {
>>> + ret = errno;
>>> + LOG(DmaHeap, Error)
>>> + << "UdmaHeap failed to set memfd size: " << strerror(ret);
>>> + return {};
>>> + }
>>> +
>>> + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */
>
> s/seal/seal./
>
>>> + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);
>>> + if (ret < 0) {
>>> + ret = errno;
>>> + LOG(DmaHeap, Error)
>>> + << "UdmaHeap failed to seal the memfd: " << strerror(ret);
>>> + return {};
>>> + }
>>> +
>>> + struct udmabuf_create create;
>>> +
>>> + create.memfd = memfd.get();
>>> + create.flags = UDMABUF_FLAGS_CLOEXEC;
>>> + create.offset = 0;
>>> + create.size = size;
>>> +
>>> + ret = ::ioctl(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);
>>> + if (ret < 0) {
>>> + ret = errno;
>>> + LOG(DmaHeap, Error)
>>> + << "UdmaHeap failed to allocate " << size << " bytes: "
>>> + << strerror(ret);
>>> + return {};
>>> + }
>>> +
>>> + if (create.size < size) {
>>> + LOG(DmaHeap, Error)
>>> + << "UdmaHeap allocated " << create.size << " bytes instead of "
>>> + << size << " bytes";
>>
>> Hrm, I think this path doesn't handle the now successfully created
>> UDMABuf handle. Do we need to free it?
>>
>> Maybe we should move the 'UniqueFD uDma(ret);' above this statement so
>> that the UniqueFD handles it correctly for both paths?
>
> Sounds good.
Ack.
>>> + 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 ?
>
>>> +
>>> + /* Fail if not suitable, the allocation will be free'd by UniqueFD */
>
> s/UniqueFD/UniqueFD./
>
> That comment doesn't seem correct though, there are no further checks.
> Is it a leftover ?
If we move the UniqueFD declaration + this comment up then it does seem like
a valid comment.
>
>>> + LOG(DmaHeap, Debug) << "UdmaHeap allocated " << create.size << " bytes";
>>> +
>>> + /* The underlying memfd is kept as as a reference in the kernel */
>>> + UniqueFD uDma(ret);
>>> +
>>> + return uDma;
>>> +}
>>> +
>>> +/**
>>> + * \brief Allocate a dma-buf from the DmaHeap
>>
>> I think this would now change from 'the DmaHeap' to 'a DmaHeap' but
>> that's just a nit.
>
> I don't think that's correct. The allocator still allocates the buffer
> from one specific backend, selected when the allocator is constructed.
>
>> Otherwise / with the above considered I'd put my name to this ;-)
>> (err... I think I co-wrote it in the past so I'm biased ;D)
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> + * \param [in] name The name to set for the allocated buffer
>>> + * \param [in] size The size of the buffer to allocate
>>> + *
>>> + * Allocates a dma-buf with read/write access.
>>> + *
>>> + * If the allocation fails, return an invalid UniqueFD.
>>> + *
>>> + * \return The UniqueFD of the allocated buffer
>>> + */
>>> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>> +{
>>> + if (!name)
>>> + return {};
>>> +
>>> + if (useUDmaBuf_)
>>> + return allocFromUDmaBuf(name, size);
>>> + else
>>> + return allocFromHeap(name, size);
>>> +}
>>> +
>>> } /* namespace libcamera */
>
Regards,
Hans
More information about the libcamera-devel
mailing list