<div dir="ltr"><div dir="ltr">Thanks Kieran!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 16, 2023 at 7:21 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@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">Quoting Harvey Yang via libcamera-devel (2023-05-16 09:03:18)<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>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> src/libcamera/heap_allocator.cpp | 106 +++++++++++++++++++++++++++++++<br>
> 1 file changed, 106 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp<br>
> index e9476de5..682a7e01 100644<br>
> --- a/src/libcamera/heap_allocator.cpp<br>
> +++ b/src/libcamera/heap_allocator.cpp<br>
> @@ -10,10 +10,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>
> @@ -52,6 +56,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>
> @@ -103,9 +115,103 @@ 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>
> +<br>
> + if (ret == EACCES) {<br>
> + LOG(HeapAllocator, Info)<br>
> + << "UdmaHeap: Consider making /dev/udmabuf accessible by the video group";<br>
> + LOG(HeapAllocator, Info)<br>
> + << "UdmaHeap: Alternatively, add your user to the kvm group.";<br>
<br>
I think I wrote this code, but I'm torn here for upstream. These are<br>
probably very 'distro' specific comments which we probably shouldn't<br>
include here.<br>
<br>
I like that they help inform the user what they need to do - but I fear<br>
maybe we should drop these comments for now.<br>
<br></blockquote><div><br></div><div>I see. Removed.</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">
> + }<br>
> +<br>
> + } else {<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>
<br>
I think if we've failed to create a DmaHeap - that's probably important<br>
to report to the user.<br>
<br>
I'm weary of this error path being taken for instance on an RPi or real<br>
device that wouldn't necessarily work with UdmaHeap ? (Maybe it will, I<br>
don't know) - but the fact we're using a different heap should probably<br>
be reported in that instance.<br>
<br>
Perhaps at least just a<br>
LOG(HeapAllocator, Info) << "Using UdmaHeap allocator";<br>
<br>
Which would give us an indicator if we have issues reported on hardware<br>
pipelines that can't use the buffers.<br>
<br>
Otherwise, the allocation code all looks about how I remember it being<br>
so <br>
<br></blockquote><div><br></div><div>Yes, your worry totally makes sense. I've considered letting users decide</div><div>which heap to be used actually. For instance, we can create an enum list</div><div>that contains all the options (Dma & Udma) for now, and let users choose.</div><div>WDYT?</div><div><br></div><div>For the log though, actually we'll get informed by "Dmaheap could not</div><div>open any dmaHeap device" in DmaHeap's c'tor, when it fails to initialize.</div><div>I've added corresponiding Info log in DmaHeap's and UdmaHeap's c'tors</div><div>though, to make it clearer.</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">
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
<br>
> + heap_ = std::make_unique<UdmaHeap>();<br>
> }<br>
> <br>
> HeapAllocator::~HeapAllocator() = default;<br>
> -- <br>
> 2.40.1.606.ga4b1b128d6-goog<br>
><br>
</blockquote></div></div>