<div dir="ltr">Also, if we let users decide which heap implementations to be used<div>directly, maybe there's no point having class HeapAllocator at all.</div><div>We can just give users (pipeline handlers) access to each Heap's</div><div>implementation directly.</div><div><br></div><div>WDYT?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 30, 2023 at 6:14 PM Cheng-Hao Yang <<a href="mailto:chenghaoyang@chromium.org">chenghaoyang@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Hi Laurent, thanks for pointing it out. I'm a newbie to buffer allocation<div>in linux, so here are my confusions before updating the patch:<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 30, 2023 at 2:42 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:<br>
> From: Cheng-Hao Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> <br>
> If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.<br>
<br>
udmabuf and the DMA heaps are two completely different things, meant for<br>
different use cases. An allocator that would use "whatever is available"<br>
isn't a good generic helper.<br>
<br></blockquote><div><br></div><div>I assume you mean udmabuf and DMA buffers, unless you mean the</div><div>difference between buffers and heaps:</div><div>I saw [1], which states udmabuf as "User space mappable DMA buffer",</div><div>so I assume that it's one kind of DMA buffer, especially accessible for</div><div>userspace.</div><div><br></div><div>I agree that we shouldn't use "whatever is available" logic though. How</div><div>about an enum passed into HeapAllocator's c'tor that lets the user to</div><div>specify which heap(s) (with an order or not) to be used?</div><div><br></div><div>[1]: <a href="https://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md" target="_blank">https://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I expect writing the documentation will make you realize that there's a<br>
design issue here, as it's difficult to document clearly an API that<br>
doesn't have a clear design.<br>
<br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++<br>
> 1 file changed, 100 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp<br>
> index 69f65062..8f99f590 100644<br>
> --- a/src/libcamera/heap_allocator.cpp<br>
> +++ b/src/libcamera/heap_allocator.cpp<br>
> @@ -11,10 +11,14 @@<br>
> #include <array><br>
> #include <fcntl.h><br>
> #include <sys/ioctl.h><br>
> +#include <sys/mman.h><br>
> +#include <sys/stat.h><br>
> +#include <sys/types.h><br>
> #include <unistd.h><br>
> <br>
> #include <linux/dma-buf.h><br>
> #include <linux/dma-heap.h><br>
> +#include <linux/udmabuf.h><br>
> <br>
> #include <libcamera/base/log.h><br>
> <br>
> @@ -54,6 +58,14 @@ public:<br>
> UniqueFD alloc(const char *name, std::size_t size) override;<br>
> };<br>
> <br>
> +class UdmaHeap : public Heap<br>
> +{<br>
> +public:<br>
> + UdmaHeap();<br>
> + ~UdmaHeap();<br>
> + UniqueFD alloc(const char *name, std::size_t size) override;<br>
> +};<br>
> +<br>
> DmaHeap::DmaHeap()<br>
> {<br>
> for (const char *name : dmaHeapNames) {<br>
> @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()<br>
> continue;<br>
> }<br>
> <br>
> + LOG(HeapAllocator, Info) << "Using DmaHeap allocator";<br>
> handle_ = UniqueFD(ret);<br>
> break;<br>
> }<br>
> @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)<br>
> return allocFd;<br>
> }<br>
> <br>
> +UdmaHeap::UdmaHeap()<br>
> +{<br>
> + int ret = ::open("/dev/udmabuf", O_RDWR, 0);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(HeapAllocator, Error)<br>
> + << "UdmaHeap failed to open allocator: " << strerror(ret);<br>
> + } else {<br>
> + LOG(HeapAllocator, Info) << "Using UdmaHeap allocator";<br>
> + handle_ = UniqueFD(ret);<br>
> + }<br>
> +}<br>
> +<br>
> +UdmaHeap::~UdmaHeap() = default;<br>
> +<br>
> +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)<br>
> +{<br>
> + if (!isValid()) {<br>
> + LOG(HeapAllocator, Fatal) << "UdmaHeap: Allocation attempted without allocator" << name;<br>
> + return {};<br>
> + }<br>
> +<br>
> + int ret;<br>
> +<br>
> + ret = memfd_create(name, MFD_ALLOW_SEALING);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(HeapAllocator, Error)<br>
> + << "UdmaHeap failed to allocate memfd storage: "<br>
> + << strerror(ret);<br>
> + return {};<br>
> + }<br>
> +<br>
> + UniqueFD memfd(ret);<br>
> +<br>
> + ret = ftruncate(memfd.get(), size);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(HeapAllocator, Error)<br>
> + << "UdmaHeap failed to set memfd size: " << strerror(ret);<br>
> + return {};<br>
> + }<br>
> +<br>
> + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */<br>
> + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(HeapAllocator, Error)<br>
> + << "UdmaHeap failed to seal the memfd: " << strerror(ret);<br>
> + return {};<br>
> + }<br>
> +<br>
> + struct udmabuf_create create;<br>
> +<br>
> + create.memfd = memfd.get();<br>
> + create.flags = UDMABUF_FLAGS_CLOEXEC;<br>
> + create.offset = 0;<br>
> + create.size = size;<br>
> +<br>
> + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(HeapAllocator, Error)<br>
> + << "UdmaHeap failed to allocate " << size << " bytes: "<br>
> + << strerror(ret);<br>
> + return {};<br>
> + }<br>
> +<br>
> + /* The underlying memfd is kept as as a reference in the kernel */<br>
> + UniqueFD uDma(ret);<br>
> +<br>
> + if (create.size != size)<br>
> + LOG(HeapAllocator, Warning)<br>
> + << "UdmaHeap allocated " << create.size << " bytes instead of "<br>
> + << size << " bytes";<br>
> +<br>
> + /* Fail if not suitable, the allocation will be free'd by UniqueFD */<br>
> + if (create.size < size)<br>
> + return {};<br>
> +<br>
> + LOG(HeapAllocator, Debug) << "UdmaHeap allocated " << create.size << " bytes";<br>
> +<br>
> + return uDma;<br>
> +}<br>
> +<br>
> HeapAllocator::HeapAllocator()<br>
> {<br>
> heap_ = std::make_unique<DmaHeap>();<br>
> + if (!isValid())<br>
> + heap_ = std::make_unique<UdmaHeap>();<br>
> }<br>
> <br>
> HeapAllocator::~HeapAllocator() = default;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>
</blockquote></div>