<div dir="ltr"><div dir="ltr">Thanks Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 29, 2023 at 3:58 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@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>
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>
> 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>
<br>
Should you add O_CLOEXEC or does it generate issues ? Same question as<br>
in 1/3 on the third argument<br>
<br></blockquote><div><br></div><div>Removed the third argument for now.</div><div>Kieran, I copied and pasted from your code [1]. Maybe you know better than I</div><div>do?</div><div><br></div><div>[1]: <a href="https://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R29" target="_blank">https://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R29</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">
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(HeapAllocator, Error)<br>
> + << "UdmaHeap failed to open allocator: " << strerror(ret);<br>
<br>
You could return here and save an...<br>
<br>
> + } else {<br>
<br>
... indendation level here<br>
<br></blockquote><div><br></div><div>Yeah thanks!</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">
> + 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>
UdmaHeap::alloc() is called by HeapAllocator::alloc() which already<br>
checks for validity with a Fatal error. You can drop this change<br></blockquote><div><br></div><div>Right, removed.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + int ret;<br>
> +<br>
> + ret = memfd_create(name, MFD_ALLOW_SEALING);<br>
<br>
You can declare and assign ret in one line<br>
<br></blockquote><div><br></div><div>Done</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">
> + 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>
Should we continue or is it an error ? Also I would move this check<br>
before the creation of uDma(ret);<br>
<br></blockquote><div><br></div><div>Yeah creating |uDma| when we're returning should be better.<br></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>
> + /* Fail if not suitable, the allocation will be free'd by UniqueFD */<br>
> + if (create.size < size)<br>
> + return {};<br>
<br>
Ah so larger size is ok, smaller it's not.<br>
<br>
So I would check for this before the previous one<br>
<br></blockquote><div><br></div><div>Hmm, do you mean we should generate different logs for the two cases?</div><div>Updated the order and the logs. Please check.</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>
> + 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>
> 2.40.1.698.g37aff9b760-goog<br>
><br>
</blockquote></div></div>