[{"id":29629,"web_url":"https://patchwork.libcamera.org/comment/29629/","msgid":"<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>","date":"2024-05-28T11:32:10","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hans,\n\nThanks for refreshing this work!\n\nQuoting Hans de Goede (2024-05-27 15:16:47)\n> Add support for using udmabuf to alloc DMA heaps, this is basically:\n> https://patchwork.libcamera.org/patch/18922/\n> \n> ported from the never merged HeapAllocator class to the current DmaHeap\n> class.\n\nTrue statements, but not really specific to the content of the patch?\n\nPerhaps:\n\n\"\"\"\nThe DMA heap allocator currently allocates from CMA and system heaps.\n\nThese provide contiguous device memory but require dedicated and\nreserved memory regions to be configured on the platform and may require\nelevated privilidges to access.\n\nExtend the dma_heap allocator to support falling back to the memfd\nallocator backed by the udmabuf framework to prepare buffers that are\ncompatible with linux drivers and the libcamera API.\n\nThe buffers allocated through memfd/udmabuf will not be contiguous and\nare likely not suitable for directly rendering to a display or encoder\nbut may facilitate more usecases for allocating memory in pipelines that\nmake use of the CPU for processing including virtual pipeline handlers\nor the SoftISP.\n\"\"\"\n\nFeel free to adapt the above of course if I've made any errors.\n\n\n\n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> Signed-off-by: Hans de Goede \n> ---\n> include/libcamera/internal/dma_heaps.h | 3 +\n> src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n> 2 files changed, 109 insertions(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n> index f0a8aa5d..7e1271ed 100644\n> --- a/include/libcamera/internal/dma_heaps.h\n> +++ b/include/libcamera/internal/dma_heaps.h\n> @@ -30,7 +30,10 @@ public:\n> UniqueFD alloc(const char *name, std::size_t size);\n> \n> private:\n> + UniqueFD allocFromHeap(const char *name, std::size_t size);\n> + UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);\n> UniqueFD dmaHeapHandle_;\n> + bool useUDmaBuf_;\n> };\n> \n> LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> index d4cb880b..bb707786 100644\n> --- a/src/libcamera/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heaps.cpp\n> @@ -10,10 +10,14 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> +#include \n> #include \n> \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -36,13 +40,15 @@ namespace libcamera {\n> struct DmaHeapInfo {\n> DmaHeap::DmaHeapFlag type;\n> const char *deviceNodeName;\n> + bool useUDmaBuf;\n> };\n> #endif\n> \n> -static constexpr std::array heapInfos = { {\n> - { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\" },\n> - { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\" },\n> - { DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\" },\n> +static constexpr std::array heapInfos = { {\n> + { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\", false },\n> + { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\", false },\n> + { DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\", false },\n> + { DmaHeap::DmaHeapFlag::System, \"/dev/udmabuf\", true },\n\nShould we distinguise UDMA with a different flag? or just simply keep it\nas a fall back?\n\nIf we do use a different DmaHeapFlag then I think we don't need the\nboolean...\n\n\n> } };\n> \n> LOG_DEFINE_CATEGORY(DmaHeap)\n> @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)\n> \n> LOG(DmaHeap, Debug) << \"Using \" << info.deviceNodeName;\n> dmaHeapHandle_ = UniqueFD(ret);\n> + useUDmaBuf_ = info.useUDmaBuf;\n> break;\n> }\n> \n> @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;\n> * \\return True if the DmaHeap is valid, false otherwise\n> */\n> \n> -/**\n> - * \\brief Allocate a dma-buf from the DmaHeap\n> - * \\param [in] name The name to set for the allocated buffer\n> - * \\param [in] size The size of the buffer to allocate\n> - *\n> - * Allocates a dma-buf with read/write access.\n> - *\n> - * If the allocation fails, return an invalid UniqueFD.\n> - *\n> - * \\return The UniqueFD of the allocated buffer\n> - */\n> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)\n> {\n> - int ret;\n> -\n> - if (!name)\n> - return {};\n> -\n> struct dma_heap_allocation_data alloc = {};\n> + int ret;\n> \n> alloc.len = size;\n> alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> return allocFd;\n> }\n> \n> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)\n> +{\n> + /* size must be a multiple of the page-size round it up */\n> + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n> + size = (size + pageMask) & ~pageMask;\n> +\n> + int ret = memfd_create(name, MFD_ALLOW_SEALING);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(DmaHeap, Error)\n> + << \"UdmaHeap failed to allocate memfd storage: \"\n> + << strerror(ret);\n\nCompletely optional/just an idea - I wonder if we should use 'name' in\nthe log messages here and below.\n\nWould probably apply to allocFromHeap() too so likely a patch on top -\nnot an update here.\n\n\n> + return {};\n> + }\n> +\n> + UniqueFD memfd(ret);\n> +\n> + ret = ftruncate(memfd.get(), size);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(DmaHeap, Error)\n> + << \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> + return {};\n> + }\n> +\n> + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(DmaHeap, Error)\n> + << \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> + return {};\n> + }\n> +\n> + struct udmabuf_create create;\n> +\n> + create.memfd = memfd.get();\n> + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> + create.offset = 0;\n> + create.size = size;\n> +\n> + ret = ::ioctl(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(DmaHeap, Error)\n> + << \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n> + << strerror(ret);\n> + return {};\n> + }\n> +\n> + if (create.size < size) {\n> + LOG(DmaHeap, Error)\n> + << \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> + << size << \" bytes\";\n\nHrm, I think this path doesn't handle the now successfully created\nUDMABuf handle. Do we need to free it?\n\nMaybe we should move the 'UniqueFD uDma(ret);' above this statement so\nthat the UniqueFD handles it correctly for both paths?\n\n\n> + return {};\n> + }\n> +\n> + if (create.size != size)\n> + LOG(DmaHeap, Warning)\n> + << \"UdmaHeap allocated \" << create.size << \" bytes, \"\n> + << \"which is greater than requested : \" << size << \" bytes\";\n> +\n> + /* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> + LOG(DmaHeap, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> +\n> + /* The underlying memfd is kept as as a reference in the kernel */\n> + UniqueFD uDma(ret);\n> +\n> + return uDma;\n> +}\n> +\n> +/**\n> + * \\brief Allocate a dma-buf from the DmaHeap\n\nI think this would now change from 'the DmaHeap' to 'a DmaHeap' but\nthat's just a nit.\n\n\nOtherwise / with the above considered I'd put my name to this ;-)\n(err... I think I co-wrote it in the past so I'm biased ;D)\n\n\nReviewed-by: Kieran Bingham \n\n\n> + * \\param [in] name The name to set for the allocated buffer\n> + * \\param [in] size The size of the buffer to allocate\n> + *\n> + * Allocates a dma-buf with read/write access.\n> + *\n> + * If the allocation fails, return an invalid UniqueFD.\n> + *\n> + * \\return The UniqueFD of the allocated buffer\n> + */\n> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> +{\n> + if (!name)\n> + return {};\n> +\n> + if (useUDmaBuf_)\n> + return allocFromUDmaBuf(name, size);\n> + else\n> + return allocFromHeap(name, size);\n> +}\n> +\n> } /* namespace libcamera */\n> -- \n> 2.45.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F37D5BD87C\n\tfor ;\n\tTue, 28 May 2024 11:32:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99B8C634B0;\n\tTue, 28 May 2024 13:32:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E16AB61A41\n\tfor ;\n\tTue, 28 May 2024 13:32:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE2C84CD;\n\tTue, 28 May 2024 13:32:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FWytedUl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716895930;\n\tbh=/uqPUvqpI9Ot+w3YMm4PILhiRNUJf7BOqHI8fmez8xw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=FWytedUlD9StAmk40SyIWIwUpKGGORK8UvI7EEW2wHetqFTuvGEiAW7OX+sKT44xq\n\t4VqVnoOTGj7j6VVQ7CYSu+9VU5DXmnjCiXAjd8/bQ2jsWpv5UGzVkpmZS24ukz1jtb\n\tDR2r/S89+uRVdEaeAdLQkds4jKfDX2HBz2sYoWlU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240527141647.336633-1-hdegoede@redhat.com>","References":"<20240527141647.336633-1-hdegoede@redhat.com>","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","From":"Kieran Bingham ","Cc":"Maxime Ripard , Milan Zamazal , \n\tDennis Bonke , Hans de Goede ,\n\tHarvey Yang ","To":"Hans de Goede ,\n\tSakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 28 May 2024 12:32:10 +0100","Message-ID":"<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29630,"web_url":"https://patchwork.libcamera.org/comment/29630/","msgid":"<20240528-loud-turkey-of-hurricane-07f26d@houat>","date":"2024-05-28T11:46:24","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":197,"url":"https://patchwork.libcamera.org/api/people/197/","name":"Maxime Ripard","email":"mripard@redhat.com"},"content":"Hi Kieran,\n\nOn Tue, May 28, 2024 at 12:32:10PM GMT, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-05-27 15:16:47)\n> > Add support for using udmabuf to alloc DMA heaps, this is basically:\n> > https://patchwork.libcamera.org/patch/18922/\n> > \n> > ported from the never merged HeapAllocator class to the current DmaHeap\n> > class.\n> \n> True statements, but not really specific to the content of the patch?\n> \n> Perhaps:\n> \n> \"\"\"\n> The DMA heap allocator currently allocates from CMA and system heaps.\n> \n> These provide contiguous device memory but require dedicated and\n> reserved memory regions to be configured on the platform and may require\n> elevated privilidges to access.\n\nOnly the CMA allocator (and heap) reserves memory. The system heap uses\nthe page allocator.\n\nMaxime","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7552CBD87C\n\tfor ;\n\tTue, 28 May 2024 12:16:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2A066349B;\n\tTue, 28 May 2024 14:16:01 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C112361A41\n\tfor ;\n\tTue, 28 May 2024 13:46:31 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-531-WSEe8VnOP3uyDRkcKRmC6A-1; Tue, 28 May 2024 07:46:27 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-35559d30617so560739f8f.2\n\tfor ;\n\tTue, 28 May 2024 04:46:27 -0700 (PDT)","from localhost ([2a01:e0a:b25:f902::ff])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-35579d7de7esm11381601f8f.14.2024.05.28.04.46.24\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 28 May 2024 04:46:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"d3BlTaVe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1716896790;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=HLclBGTOA3ASP7QyFmr8ey+Xo0C1GHBpR3NqV5M2Ja0=;\n\tb=d3BlTaVewJemXXWLOhibz3KQfGBsaYqDAByhKk7a1FYluvOctDDAbi+kZCyGbiNPP/5DqQ\n\tWR3d4MZDXJeNGiqhitR9QU2ulvyC39TxaNgCnbicRsexKcV4u75uDXEovvqA3wuRSB1gHG\n\tG4gBVKFHsYdbc4+UhzXnJNcP9+TFuXo=","X-MC-Unique":"WSEe8VnOP3uyDRkcKRmC6A-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1716896786; x=1717501586;\n\th=in-reply-to:content-disposition:mime-version:references:message-id\n\t:subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=HLclBGTOA3ASP7QyFmr8ey+Xo0C1GHBpR3NqV5M2Ja0=;\n\tb=gimmlDz54Gc1+CWrOtSbVy5HwrFr9jZ7DCmqYKNsUjkZ1JKbSV6+4JHvAhdonF/Gc1\n\tmhsXaItWAAHHJy0V08RTkhuGFCZ16tBSZrSQ8W0Uxjn+MyBIEvikYek2yR71mghf6bXi\n\tGk1YAO8nMjazfMzsc5VBC52LC1p9kqoBz0Krk/Wj1fECfbKelsSHaiAPNhuHrI5aunsw\n\tojEtMwAbjFVictb0teE33NsQOKVlsGKMyyV3b3lnNvLnX3xzGEqiJFdEbSqOV7NSmbA+\n\tOF7l0hRzhNpDgtFpQMZTBDCke/PwvlfLUzCt/MlhP6WgclpLXkRuawztd/s2aWYTaIKk\n\tT87Q==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXql2OLaWhTWWVKsoyUhrCdcT341zNi1yNxTu8xG6l1XLSMjTBghNrMP9hIGXNe7Qv9BP9eqVarmQJXV/nS8PeQt3ZxA31Z2/+TmPM4mc4/s/kPEQ==","X-Gm-Message-State":"AOJu0Yy9fLLParINd5BXErth9snrntBb6KYmszb1QP/wjndCQ6/GEcI3\n\tVfi0UUQvVNeuusAdiE8HwkPRtUEAkL+xqUZy+tcOqmzuMHbzwqrH72AUAN+xvHSqpASDib9PN4i\n\tcP8kPBIRLxqAvVylJdT0Df4BLR8UYSj+jZhC1RAAgzI1Xwhvl5bpf4KH0Hv8MBQ+oOfHZ0Nw=","X-Received":["by 2002:adf:f00f:0:b0:354:f304:cd36 with SMTP id\n\tffacd0b85a97d-35538a61547mr10995558f8f.70.1716896785734; \n\tTue, 28 May 2024 04:46:25 -0700 (PDT)","by 2002:adf:f00f:0:b0:354:f304:cd36 with SMTP id\n\tffacd0b85a97d-35538a61547mr10995542f8f.70.1716896785247; \n\tTue, 28 May 2024 04:46:25 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEhVMhhmvgPnzvy2BfWqtJK3OKksVGUGxJbMTppQdNl6QHmh//TgPalFvfVEPIjU5/cC0I3rw==","Date":"Tue, 28 May 2024 13:46:24 +0200","From":"Maxime Ripard ","To":"Kieran Bingham ","Cc":"Hans de Goede , \n\tSakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org, \n\tMilan Zamazal ,\n\tDennis Bonke , \n\tHarvey Yang ","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","Message-ID":"<20240528-loud-turkey-of-hurricane-07f26d@houat>","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha384;\n\tprotocol=\"application/pgp-signature\"; boundary=\"4cgxg4rsw2k543uq\"","Content-Disposition":"inline","In-Reply-To":"<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>","X-Mailman-Approved-At":"Tue, 28 May 2024 14:15:59 +0200","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29636,"web_url":"https://patchwork.libcamera.org/comment/29636/","msgid":"<20240528213746.GG8500@pendragon.ideasonboard.com>","date":"2024-05-28T21:37:46","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-05-27 15:16:47)\n> > Add support for using udmabuf to alloc DMA heaps, this is basically:\n> > https://patchwork.libcamera.org/patch/18922/\n> > \n> > ported from the never merged HeapAllocator class to the current DmaHeap\n> > class.\n> \n> True statements, but not really specific to the content of the patch?\n> \n> Perhaps:\n> \n> \"\"\"\n> The DMA heap allocator currently allocates from CMA and system heaps.\n> \n> These provide contiguous device memory but require dedicated and\n> reserved memory regions to be configured on the platform and may require\n> elevated privilidges to access.\n\nThe system heap doesn't require reserved memory regions, and also\ndoesn't provide contiguous device memory. The CMA heap also doesn't\nrequired reserved memory regions actually.\n\n> Extend the dma_heap allocator to support falling back to the memfd\n> allocator backed by the udmabuf framework to prepare buffers that are\n> compatible with linux drivers and the libcamera API.\n\nWhen reading the patch, my very first thought is that udmabuf doesn't\nmake use of DMA heaps, so the allocator should belong to a different\nclass.\n\nThinking a bit more about it, I understand the appeal of hiding this\nfallback from the DmaHeap user. I'm however a bit concerned that the\ndmabuf instances allocated from the DMA system heap and through udmabuf\nwill not operate in a fully identical way. Returning one or the other\nwithout the user being aware of what has been allocated may lead to\nissues.\n\nI may worry needlessly though. I haven't compared the system heap and\nthe udmabuf implementations, to see if the dmabuf operations differ in\nways that are significant. I think this should at least be checked\nbefore we make a decision.\n\n> The buffers allocated through memfd/udmabuf will not be contiguous and\n> are likely not suitable for directly rendering to a display or encoder\n> but may facilitate more usecases for allocating memory in pipelines that\n\nLet's make a stronger statement : \"are not suitable for zero-copy buffer\nsharing with other devices\".\n\n> make use of the CPU for processing including virtual pipeline handlers\n> or the SoftISP.\n> \"\"\"\n> \n> Feel free to adapt the above of course if I've made any errors.\n> \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > Signed-off-by: Hans de Goede \n> > ---\n> > include/libcamera/internal/dma_heaps.h | 3 +\n> > src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n> > 2 files changed, 109 insertions(+), 21 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n> > index f0a8aa5d..7e1271ed 100644\n> > --- a/include/libcamera/internal/dma_heaps.h\n> > +++ b/include/libcamera/internal/dma_heaps.h\n> > @@ -30,7 +30,10 @@ public:\n> > UniqueFD alloc(const char *name, std::size_t size);\n> > \n> > private:\n> > + UniqueFD allocFromHeap(const char *name, std::size_t size);\n> > + UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);\n> > UniqueFD dmaHeapHandle_;\n> > + bool useUDmaBuf_;\n> > };\n> > \n> > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n> > diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> > index d4cb880b..bb707786 100644\n> > --- a/src/libcamera/dma_heaps.cpp\n> > +++ b/src/libcamera/dma_heaps.cpp\n> > @@ -10,10 +10,14 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > +#include \n> > +#include \n> > #include \n> > \n> > #include \n> > #include \n> > +#include \n> > \n> > #include \n> > \n> > @@ -36,13 +40,15 @@ namespace libcamera {\n> > struct DmaHeapInfo {\n> > DmaHeap::DmaHeapFlag type;\n> > const char *deviceNodeName;\n> > + bool useUDmaBuf;\n> > };\n> > #endif\n> > \n> > -static constexpr std::array heapInfos = { {\n> > - { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\" },\n> > - { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\" },\n> > - { DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\" },\n> > +static constexpr std::array heapInfos = { {\n> > + { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\", false },\n> > + { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\", false },\n> > + { DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\", false },\n> > + { DmaHeap::DmaHeapFlag::System, \"/dev/udmabuf\", true },\n> \n> Should we distinguise UDMA with a different flag? or just simply keep it\n> as a fall back?\n> \n> If we do use a different DmaHeapFlag then I think we don't need the\n> boolean...\n\nExposing the difference to the user may alleviate some of my concerns.\nShould we then rename the DmaHeap class to DmaBufAllocator ?\n\n> > } };\n> > \n> > LOG_DEFINE_CATEGORY(DmaHeap)\n> > @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)\n> > \n> > LOG(DmaHeap, Debug) << \"Using \" << info.deviceNodeName;\n> > dmaHeapHandle_ = UniqueFD(ret);\n> > + useUDmaBuf_ = info.useUDmaBuf;\n> > break;\n> > }\n> > \n> > @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;\n> > * \\return True if the DmaHeap is valid, false otherwise\n> > */\n> > \n> > -/**\n> > - * \\brief Allocate a dma-buf from the DmaHeap\n> > - * \\param [in] name The name to set for the allocated buffer\n> > - * \\param [in] size The size of the buffer to allocate\n> > - *\n> > - * Allocates a dma-buf with read/write access.\n> > - *\n> > - * If the allocation fails, return an invalid UniqueFD.\n> > - *\n> > - * \\return The UniqueFD of the allocated buffer\n> > - */\n> > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)\n> > {\n> > - int ret;\n> > -\n> > - if (!name)\n> > - return {};\n> > -\n> > struct dma_heap_allocation_data alloc = {};\n> > + int ret;\n> > \n> > alloc.len = size;\n> > alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > return allocFd;\n> > }\n> > \n> > +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)\n> > +{\n> > + /* size must be a multiple of the page-size round it up */\n\ns/size/Size/\ns/ up/ up./\n\n> > + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n> > + size = (size + pageMask) & ~pageMask;\n> > +\n> > + int ret = memfd_create(name, MFD_ALLOW_SEALING);\n\nNeeds sys/mman.h.\n\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(DmaHeap, Error)\n> > + << \"UdmaHeap failed to allocate memfd storage: \"\n\nUdmaHeap sounds like a frankenstein monster :-) Did you mean UDmaBuf\nhere ? I would just drop the prefix from this message and the ones below\nactually.\n\n> > + << strerror(ret);\n> \n> Completely optional/just an idea - I wonder if we should use 'name' in\n> the log messages here and below.\n> \n> Would probably apply to allocFromHeap() too so likely a patch on top -\n> not an update here.\n> \n> > + return {};\n> > + }\n> > +\n> > + UniqueFD memfd(ret);\n> > +\n> > + ret = ftruncate(memfd.get(), size);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(DmaHeap, Error)\n> > + << \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n\ns/seal/seal./\n\n> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(DmaHeap, Error)\n> > + << \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + struct udmabuf_create create;\n> > +\n> > + create.memfd = memfd.get();\n> > + create.flags = UDMABUF_FLAGS_CLOEXEC;\n> > + create.offset = 0;\n> > + create.size = size;\n> > +\n> > + ret = ::ioctl(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(DmaHeap, Error)\n> > + << \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n> > + << strerror(ret);\n> > + return {};\n> > + }\n> > +\n> > + if (create.size < size) {\n> > + LOG(DmaHeap, Error)\n> > + << \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n> > + << size << \" bytes\";\n> \n> Hrm, I think this path doesn't handle the now successfully created\n> UDMABuf handle. Do we need to free it?\n> \n> Maybe we should move the 'UniqueFD uDma(ret);' above this statement so\n> that the UniqueFD handles it correctly for both paths?\n\nSounds good.\n\n> > + return {};\n> > + }\n> > +\n> > + if (create.size != size)\n> > + LOG(DmaHeap, Warning)\n> > + << \"UdmaHeap allocated \" << create.size << \" bytes, \"\n> > + << \"which is greater than requested : \" << size << \" bytes\";\n\nWhy/when does this happen ?\n\n> > +\n> > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */\n\ns/UniqueFD/UniqueFD./\n\nThat comment doesn't seem correct though, there are no further checks.\nIs it a leftover ?\n\n> > + LOG(DmaHeap, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n> > +\n> > + /* The underlying memfd is kept as as a reference in the kernel */\n> > + UniqueFD uDma(ret);\n> > +\n> > + return uDma;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Allocate a dma-buf from the DmaHeap\n> \n> I think this would now change from 'the DmaHeap' to 'a DmaHeap' but\n> that's just a nit.\n\nI don't think that's correct. The allocator still allocates the buffer\nfrom one specific backend, selected when the allocator is constructed.\n\n> Otherwise / with the above considered I'd put my name to this ;-)\n> (err... I think I co-wrote it in the past so I'm biased ;D)\n> \n> Reviewed-by: Kieran Bingham \n> \n> > + * \\param [in] name The name to set for the allocated buffer\n> > + * \\param [in] size The size of the buffer to allocate\n> > + *\n> > + * Allocates a dma-buf with read/write access.\n> > + *\n> > + * If the allocation fails, return an invalid UniqueFD.\n> > + *\n> > + * \\return The UniqueFD of the allocated buffer\n> > + */\n> > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> > +{\n> > + if (!name)\n> > + return {};\n> > +\n> > + if (useUDmaBuf_)\n> > + return allocFromUDmaBuf(name, size);\n> > + else\n> > + return allocFromHeap(name, size);\n> > +}\n> > +\n> > } /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8E1F8BDE6B\n\tfor ;\n\tTue, 28 May 2024 21:38:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF331634B0;\n\tTue, 28 May 2024 23:38:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDE4261A41\n\tfor ;\n\tTue, 28 May 2024 23:37:58 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3D40E3A2;\n\tTue, 28 May 2024 23:37:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Uos+RulZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716932275;\n\tbh=uvUZKZ5SDd70/3sSX4Mj1O3C9IafQR0iyY+qXEi50ms=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Uos+RulZJnKBlUFyRdt3IG6KgsWENqCHthNKrvjpPCtNOWeR+qLMmyteOAu5L0a/+\n\tcVmwz9IYMvAGgiWHxAj3rLUqh2/pyoQOHxxX1+bisg17RrdETGVYr7lz6AOv6Naizm\n\tRMa0c4LGYuT3GxFkQFfNmiVdrGGMnGzymp2bgMow=","Date":"Wed, 29 May 2024 00:37:46 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Hans de Goede ,\n\tSakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ,\n\tDennis Bonke , \n\tHarvey Yang ","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","Message-ID":"<20240528213746.GG8500@pendragon.ideasonboard.com>","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29638,"web_url":"https://patchwork.libcamera.org/comment/29638/","msgid":"<171693762805.1680360.4565387653970693104@ping.linuxembedded.co.uk>","date":"2024-05-28T23:07:08","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-05-27 15:16:47)\n> Add support for using udmabuf to alloc DMA heaps, this is basically:\n> https://patchwork.libcamera.org/patch/18922/\n> \n> ported from the never merged HeapAllocator class to the current DmaHeap\n> class.\n> \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> Signed-off-by: Hans de Goede \n> ---\n> include/libcamera/internal/dma_heaps.h | 3 +\n> src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n> 2 files changed, 109 insertions(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n> index f0a8aa5d..7e1271ed 100644\n> --- a/include/libcamera/internal/dma_heaps.h\n> +++ b/include/libcamera/internal/dma_heaps.h\n> @@ -30,7 +30,10 @@ public:\n> UniqueFD alloc(const char *name, std::size_t size);\n> \n> private:\n> + UniqueFD allocFromHeap(const char *name, std::size_t size);\n> + UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);\n> UniqueFD dmaHeapHandle_;\n> + bool useUDmaBuf_;\n> };\n> \n> LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> index d4cb880b..bb707786 100644\n> --- a/src/libcamera/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heaps.cpp\n> @@ -10,10 +10,14 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> +#include \n> #include \n> \n> #include \n> #include \n> +#include \n\nHrm. this fails on CI. Perhaps it's a new header, and we might have to\nbring this into include/linux in libcamera.\n\n - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59258127\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 27A1BBDE6B\n\tfor ;\n\tTue, 28 May 2024 23:07:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7D6F634B0;\n\tWed, 29 May 2024 01:07:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 235A76349B\n\tfor ;\n\tWed, 29 May 2024 01:07:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B615F4AB;\n\tWed, 29 May 2024 01:07:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"B8SQ8RY5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716937627;\n\tbh=k7E7XEWbMr4bNvUCc9PDVRehMSsoEn0kEqnSE6x/i1Y=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=B8SQ8RY5wJ3RYWu0vqX7w3cvdVWxdkVW5weHDS6BdasMXire0uKHaDoAIy3Ano6K6\n\tpzwmyNf81O2TNIoaIHMVo2eS0V49lS5gAQ1yxKvGLqPtonzDQR9eo9/O0/EXeaZM7S\n\trt0bAN9MfsouES1ROUALV1goZsPcqp0uEE9SsvTU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240527141647.336633-1-hdegoede@redhat.com>","References":"<20240527141647.336633-1-hdegoede@redhat.com>","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","From":"Kieran Bingham ","Cc":"Maxime Ripard , Milan Zamazal , \n\tDennis Bonke , Hans de Goede ,\n\tHarvey Yang ","To":"Hans de Goede ,\n\tSakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 29 May 2024 00:07:08 +0100","Message-ID":"<171693762805.1680360.4565387653970693104@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29640,"web_url":"https://patchwork.libcamera.org/comment/29640/","msgid":"<20240528232531.GA11144@pendragon.ideasonboard.com>","date":"2024-05-28T23:25:31","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, May 29, 2024 at 12:07:08AM +0100, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-05-27 15:16:47)\n> > Add support for using udmabuf to alloc DMA heaps, this is basically:\n> > https://patchwork.libcamera.org/patch/18922/\n> > \n> > ported from the never merged HeapAllocator class to the current DmaHeap\n> > class.\n> > \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > Signed-off-by: Hans de Goede \n> > ---\n> > include/libcamera/internal/dma_heaps.h | 3 +\n> > src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n> > 2 files changed, 109 insertions(+), 21 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n> > index f0a8aa5d..7e1271ed 100644\n> > --- a/include/libcamera/internal/dma_heaps.h\n> > +++ b/include/libcamera/internal/dma_heaps.h\n> > @@ -30,7 +30,10 @@ public:\n> > UniqueFD alloc(const char *name, std::size_t size);\n> > \n> > private:\n> > + UniqueFD allocFromHeap(const char *name, std::size_t size);\n> > + UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);\n> > UniqueFD dmaHeapHandle_;\n> > + bool useUDmaBuf_;\n> > };\n> > \n> > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n> > diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n> > index d4cb880b..bb707786 100644\n> > --- a/src/libcamera/dma_heaps.cpp\n> > +++ b/src/libcamera/dma_heaps.cpp\n> > @@ -10,10 +10,14 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > +#include \n> > +#include \n> > #include \n> > \n> > #include \n> > #include \n> > +#include \n> \n> Hrm. this fails on CI. Perhaps it's a new header, and we might have to\n> bring this into include/linux in libcamera.\n> \n> - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59258127\n\nPlease update and use utils/update-kernel-headers.sh to fix that.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F32C9BD87C\n\tfor ;\n\tTue, 28 May 2024 23:25:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45767634A8;\n\tWed, 29 May 2024 01:25:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4ED556349B\n\tfor ;\n\tWed, 29 May 2024 01:25:45 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 94EE122A;\n\tWed, 29 May 2024 01:25:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"B8qXdovO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716938741;\n\tbh=2Yja/dxrMDu+N0k7bMMOpVmkwbAhOWUhrf/KudPFtFQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B8qXdovOD1I3g27/4fUJHj4OiE5JfqbsDXM/aEtNWPVobBesPrXIYV7kYFKPbKHLD\n\t0EJkgL0zid1y3dPXKtab1eaTr5T4bP8cTSfkpaucC2igj/PmK/HpBO4hCZP8ArrmaL\n\tHFB25LOLDowyIGr9QPtyspJpNmUnMnhCknfaW/WQ=","Date":"Wed, 29 May 2024 02:25:31 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Hans de Goede ,\n\tSakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ,\n\tDennis Bonke , \n\tHarvey Yang ","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","Message-ID":"<20240528232531.GA11144@pendragon.ideasonboard.com>","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<171693762805.1680360.4565387653970693104@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171693762805.1680360.4565387653970693104@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29644,"web_url":"https://patchwork.libcamera.org/comment/29644/","msgid":"<75af9ddb-1e22-4413-bfa8-0bfe0335663c@linaro.org>","date":"2024-05-29T00:23:03","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":175,"url":"https://patchwork.libcamera.org/api/people/175/","name":"Bryan O'Donoghue","email":"bryan.odonoghue@linaro.org"},"content":"On 27/05/2024 15:16, Hans de Goede wrote:\n> @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)\n> \n> \t\tLOG(DmaHeap, Debug) << \"Using \" << info.deviceNodeName;\n> \t\tdmaHeapHandle_ = UniqueFD(ret);\n> +\t\tuseUDmaBuf_ = info.useUDmaBuf;\n> \t\tbreak;\n\nThe comment at the top of this function needs to be updated\n\n87ee158ec\n\n * The DMA heap type is selected with the \\a type parameter, which \ndefaults to\n * the CMA heap. If no heap of the given type can be accessed, the \nconstructed\n * DmaHeap instance is invalid as indicated by the isValid() function.\n *\n\n---\nbod","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1D24EBD87C\n\tfor ;\n\tWed, 29 May 2024 00:23:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6BE8634BA;\n\tWed, 29 May 2024 02:23:06 +0200 (CEST)","from mail-wm1-x333.google.com (mail-wm1-x333.google.com\n\t[IPv6:2a00:1450:4864:20::333])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2EFFA634B0\n\tfor ;\n\tWed, 29 May 2024 02:23:05 +0200 (CEST)","by mail-wm1-x333.google.com with SMTP id\n\t5b1f17b1804b1-4202c1d19d5so13152845e9.2\n\tfor ;\n\tTue, 28 May 2024 17:23:05 -0700 (PDT)","from [192.168.0.14] ([176.61.106.227])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-42108970967sm161175865e9.17.2024.05.28.17.23.03\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 28 May 2024 17:23:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"G9crBnxM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=linaro.org; s=google; t=1716942185; x=1717546985;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=bB9gWdiD0YNtV1W2V/AL39nSG9p7v6PZmaPDMccIDIg=;\n\tb=G9crBnxMS9QNpM0Ujnhfl4iD3REDAx0UTxL2cNTxDxDAzG1GMZzfhCGyUzNtg01521\n\tRHmJ76rqLVh6M4epNSCdfXUrs8nZnwSUAQEFJHFUrNaHKjDuZktlZsOnroPQm3FUbBoK\n\trErFGGV97D8imqVIF5e78n6FqIu/i7QspIgAjNpouM6OwjIZFGjaJNvYtog9xNa2qzhI\n\tynCkgD/RiVw9Vxab3/lKXJawx/MtgVUNVLScLzkOcNGqxtuWQvVvVbdZx4M+bEpyWL2O\n\ti5FwxVWO3K7tlSyPuZy4uZ/bW4sHjn3qrXyUOTwQXf0GzBP8oCSwcQPAcC/gEGPXS/Zo\n\tkU8g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1716942185; x=1717546985;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=bB9gWdiD0YNtV1W2V/AL39nSG9p7v6PZmaPDMccIDIg=;\n\tb=YQihdk3tkaVqgeu+iTtm2/FkPqwQ9sAgbKiFpof9RrkM1PolCv/leZEtRzB8Em6o+W\n\tBznWDwjb2Y4RmKtvQo/T3wYV/9cJLpgZCBRNTX3WXtE1BUAE8Pl0rzBfpoQjq2zlDXo+\n\t5bX0zuGiIM1i2NrHahoQjKKza7qKVW+iLGa5VGqd6OevP1RH9oVIG6HNg4dcj/ME6HTe\n\tSr/c2aBAfFKJQofmLBEXR7dzZQjueIUX9b3/xI13wlOlOaP/SIU7G8vIWC7maZskua+L\n\t01IBCOJfD0soRnYm4M5ZbH16543yc1aeZaxjtkETvoiq1SMbPMQjodFsBw8Io6BShhA2\n\t8xmg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCU1D67uPmSUezs0uBr5s/df6TKacvvUWgEjeG7dM4eqis5NCayo27Aggw9N691BY7qIgrTRW9lwgXinG0IUh8G5GqGRLSawrVMVTwdHHo/UQMU1wA==","X-Gm-Message-State":"AOJu0Yz0lW2vFEjHjrzeSYbwmMqVox27kBNOdmzZlPFSfB0wU7jHjB8w\n\tooZczcwe7Pzv8Nv7mRp/hqqteE7sqHRMZnjMnu606NF0HbfBFBBt92Jh8qtlcq8=","X-Google-Smtp-Source":"AGHT+IHnQYjJVbb0jQEAwXtSxhZ/5cHIf+ergrB7HHY7R6+2S/+tmI527JKFBvwCStHneiv4XgZbSw==","X-Received":"by 2002:a05:600c:3b1d:b0:421:129e:98a0 with SMTP id\n\t5b1f17b1804b1-421129e9a97mr68648215e9.19.1716942184676; \n\tTue, 28 May 2024 17:23:04 -0700 (PDT)","Message-ID":"<75af9ddb-1e22-4413-bfa8-0bfe0335663c@linaro.org>","Date":"Wed, 29 May 2024 01:23:03 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","To":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tSakari Ailus ","Cc":"Maxime Ripard , Milan Zamazal , \n\tDennis Bonke ,\n\tHarvey Yang ","References":"<20240527141647.336633-1-hdegoede@redhat.com>","Content-Language":"en-US","From":"Bryan O'Donoghue ","In-Reply-To":"<20240527141647.336633-1-hdegoede@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29645,"web_url":"https://patchwork.libcamera.org/comment/29645/","msgid":"<363e109d-439b-4ce4-81ff-ba063774a274@linaro.org>","date":"2024-05-29T00:29:22","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":175,"url":"https://patchwork.libcamera.org/api/people/175/","name":"Bryan O'Donoghue","email":"bryan.odonoghue@linaro.org"},"content":"On 27/05/2024 15:16, Hans de Goede wrote:\n> +static constexpr std::array heapInfos = { {\n> +\t{ DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\", false },\n> +\t{ DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\", false },\n> +\t{ DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\", false },\n> +\t{ DmaHeap::DmaHeapFlag::System, \"/dev/udmabuf\", true },\n> } };\n\nI have\n\ndeckard@x13s-linux:~/Development/linux$ ls /dev/udmabuf\n/dev/udmabuf\ndeckard@x13s-linux:~/Development/linux$ ls /dev/d\ndisk/ dma_heap/ dri/\ndeckard@x13s-linux:~/Development/linux$ ls /dev/dma_heap/\nlinux,cma system\n\nLIBCAMERA_LOG_LEVELS=*:DEBUG ./build.master.dbg/src/apps/qcam/qcam\n\nGives\n\n[0:15:09.128108059] [6874] DEBUG DmaHeap dma_heaps.cpp:112 Using \n/dev/dma_heap/linux,cma\n\nI thought we said udmabuf should be default over cma_heap ?\n\nAt a risk of displaying a lack of knowledge, I tried forcing the flag to \nuseUDmaBuf = true;\n\nI get\n\nZero-copy enabled\n[0:18:13.834305484] [7388] ERROR DmaHeap dma_heaps.cpp:201 UdmaHeap \nfailed to allocate 15073280 bytes: Invalid argument\n[0:18:13.834365480] [7388] ERROR SoftwareIsp software_isp.cpp:246 failed \nto allocate a dma_buf\n\nDo I need to do something more than CONFIG_UDAMBUF=y ?\n\n---\nbod","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AB99ABDE6B\n\tfor ;\n\tWed, 29 May 2024 00:29:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EFD0634B9;\n\tWed, 29 May 2024 02:29:25 +0200 (CEST)","from mail-wm1-x329.google.com (mail-wm1-x329.google.com\n\t[IPv6:2a00:1450:4864:20::329])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61BE9634AD\n\tfor ;\n\tWed, 29 May 2024 02:29:24 +0200 (CEST)","by mail-wm1-x329.google.com with SMTP id\n\t5b1f17b1804b1-42108856c33so1722935e9.1\n\tfor ;\n\tTue, 28 May 2024 17:29:24 -0700 (PDT)","from [192.168.0.14] ([176.61.106.227])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-4212278b24dsm5030895e9.0.2024.05.28.17.29.22\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 28 May 2024 17:29:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"MlDbPiwo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=linaro.org; s=google; t=1716942564; x=1717547364;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Csef6DSZ0y0L6MQy7MkKWhQgW578UTMWc3zylY3gdx0=;\n\tb=MlDbPiwo8mKrFOnrGitNS9u2G4XFjeAJGRjtkmbTGT6GfjMYvkGUJsXlRdEgC0DkVa\n\t3qF4M7QjPfzFudTStseMgUyJWE35wZcpBJ93WMke+XpXZf3na3Tqgn0v86j9diFelxOB\n\tovareynIkLYfTr5/VlKsBKdk+dj22h2b82vz4fINm+xrlmE6Yr5MTzxEhwiJ8IaHMKWP\n\tW7PWSnFOz5VETAmcsAg9vPhqaIN+ogI3TFIQ9JTYNOc0y3Tdsf31FhMLowR0BFwUHjgR\n\tBi0GWuAnvXATSPdt4834qzNRgsTXDuCvQNqBESHgZrVa7+r1lAAydzu3sYGm6jR/H6sk\n\tXZSg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1716942564; x=1717547364;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Csef6DSZ0y0L6MQy7MkKWhQgW578UTMWc3zylY3gdx0=;\n\tb=bJbumKI7938RNkhEBrbXOJKQ3Dcpnso1fNmAkfm9l/IiEwY+ZH659nVD6sGx3ELHjS\n\tJkeDVfNC7ldvRf0ZkZw7ELQfiBQuJVEhVjwTuPfTtkL84bslXi6UrKS1fy++a10FuD2s\n\tVwRbNU73QVAB/Sj6d11qrhsi+fopF6bqyvdxVuTV6XPVQP13PUqRPt8FsWjvloA2MGL2\n\tQo7HDl2vvF1BgMBVdWK0M74DTHTGtXSAbESpI9QOrSqEire4TLK5p7aoqcgIRliFFHd1\n\tOVULufmm8my5HmMs58O/n88GlV/7oQQZFUc5jiBytvQtSbeiy/kZKrNqFyXirmWIfqmp\n\tMEAA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUE4iLe5HqnrqheRsxu0oQ6TTES2z+dXwjA70g5S2txvGVrJEraRTowx+yQqG9SGf/V4JJ5P6PyASRlMO7nZ6YUMXlf2cv/7KpEirUyXkqt3GL2qA==","X-Gm-Message-State":"AOJu0YwMgp+CJyFbSWpforq4GTaBgqFyQKPhELUQiBxYHS7AEhTehx2N\n\tKY6SRVZza4ubKRPPsJozUx34jWnEppBsGKE0DY7L/JKrtXDnOTjtQo89xkPFLI8=","X-Google-Smtp-Source":"AGHT+IGFNGs0xoJuPpIc3NcV8SwHzWlMm7Y8fzBZ3C/tz2nkTta846Nihff1f5pLyzpelMHdAXkYpA==","X-Received":"by 2002:a05:600c:4fd1:b0:41b:d6ca:eefa with SMTP id\n\t5b1f17b1804b1-42122b01e91mr3883395e9.16.1716942563663; \n\tTue, 28 May 2024 17:29:23 -0700 (PDT)","Message-ID":"<363e109d-439b-4ce4-81ff-ba063774a274@linaro.org>","Date":"Wed, 29 May 2024 01:29:22 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","To":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tSakari Ailus ","Cc":"Maxime Ripard , Milan Zamazal , \n\tDennis Bonke ,\n\tHarvey Yang ","References":"<20240527141647.336633-1-hdegoede@redhat.com>","Content-Language":"en-US","From":"Bryan O'Donoghue ","In-Reply-To":"<20240527141647.336633-1-hdegoede@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29667,"web_url":"https://patchwork.libcamera.org/comment/29667/","msgid":"<0d4390d5-e466-4da1-a67d-feca891af92f@redhat.com>","date":"2024-05-29T16:26:50","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 5/28/24 11:37 PM, Laurent Pinchart wrote:\n> Hello,\n> \n> On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:\n>> Quoting Hans de Goede (2024-05-27 15:16:47)\n>>> Add support for using udmabuf to alloc DMA heaps, this is basically:\n>>> https://patchwork.libcamera.org/patch/18922/\n>>>\n>>> ported from the never merged HeapAllocator class to the current DmaHeap\n>>> class.\n>>\n>> True statements, but not really specific to the content of the patch?\n>>\n>> Perhaps:\n>>\n>> \"\"\"\n>> The DMA heap allocator currently allocates from CMA and system heaps.\n>>\n>> These provide contiguous device memory but require dedicated and\n>> reserved memory regions to be configured on the platform and may require\n>> elevated privilidges to access.\n> \n> The system heap doesn't require reserved memory regions, and also\n> doesn't provide contiguous device memory. The CMA heap also doesn't\n> required reserved memory regions actually.\n> \n>> Extend the dma_heap allocator to support falling back to the memfd\n>> allocator backed by the udmabuf framework to prepare buffers that are\n>> compatible with linux drivers and the libcamera API.\n> \n> When reading the patch, my very first thought is that udmabuf doesn't\n> make use of DMA heaps, so the allocator should belong to a different\n> class.\n> \n> Thinking a bit more about it, I understand the appeal of hiding this\n> fallback from the DmaHeap user. I'm however a bit concerned that the\n> dmabuf instances allocated from the DMA system heap and through udmabuf\n> will not operate in a fully identical way. Returning one or the other\n> without the user being aware of what has been allocated may lead to\n> issues.\n> \n> I may worry needlessly though. I haven't compared the system heap and\n> the udmabuf implementations, to see if the dmabuf operations differ in\n> ways that are significant. I think this should at least be checked\n> before we make a decision.\n> \n>> The buffers allocated through memfd/udmabuf will not be contiguous and\n>> are likely not suitable for directly rendering to a display or encoder\n>> but may facilitate more usecases for allocating memory in pipelines that\n> \n> Let's make a stronger statement : \"are not suitable for zero-copy buffer\n> sharing with other devices\".\n\nSounds good I'll come up with a new commit message for v2 taking all the various\nremarks into account.\n\n>> make use of the CPU for processing including virtual pipeline handlers\n>> or the SoftISP.\n>> \"\"\"\n>>\n>> Feel free to adapt the above of course if I've made any errors.\n>>\n>>> Co-developed-by: Harvey Yang \n>>> Signed-off-by: Harvey Yang \n>>> Signed-off-by: Hans de Goede \n>>> ---\n>>> include/libcamera/internal/dma_heaps.h | 3 +\n>>> src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n>>> 2 files changed, 109 insertions(+), 21 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h\n>>> index f0a8aa5d..7e1271ed 100644\n>>> --- a/include/libcamera/internal/dma_heaps.h\n>>> +++ b/include/libcamera/internal/dma_heaps.h\n>>> @@ -30,7 +30,10 @@ public:\n>>> UniqueFD alloc(const char *name, std::size_t size);\n>>> \n>>> private:\n>>> + UniqueFD allocFromHeap(const char *name, std::size_t size);\n>>> + UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);\n>>> UniqueFD dmaHeapHandle_;\n>>> + bool useUDmaBuf_;\n>>> };\n>>> \n>>> LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)\n>>> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp\n>>> index d4cb880b..bb707786 100644\n>>> --- a/src/libcamera/dma_heaps.cpp\n>>> +++ b/src/libcamera/dma_heaps.cpp\n>>> @@ -10,10 +10,14 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> #include \n>>> \n>>> #include \n>>> #include \n>>> +#include \n>>> \n>>> #include \n>>> \n>>> @@ -36,13 +40,15 @@ namespace libcamera {\n>>> struct DmaHeapInfo {\n>>> DmaHeap::DmaHeapFlag type;\n>>> const char *deviceNodeName;\n>>> + bool useUDmaBuf;\n>>> };\n>>> #endif\n>>> \n>>> -static constexpr std::array heapInfos = { {\n>>> - { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\" },\n>>> - { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\" },\n>>> - { DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\" },\n>>> +static constexpr std::array heapInfos = { {\n>>> + { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\", false },\n>>> + { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\", false },\n>>> + { DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\", false },\n>>> + { DmaHeap::DmaHeapFlag::System, \"/dev/udmabuf\", true },\n>>\n>> Should we distinguise UDMA with a different flag? or just simply keep it\n>> as a fall back?\n>>\n>> If we do use a different DmaHeapFlag then I think we don't need the\n>> boolean...\n> \n> Exposing the difference to the user may alleviate some of my concerns.\n> Should we then rename the DmaHeap class to DmaBufAllocator ?\n\nThat sounds like a good idea. I'll add a patch to rename the class\nas first patch in the series for v2 of the series.\n\n> \n>>> } };\n>>> \n>>> LOG_DEFINE_CATEGORY(DmaHeap)\n>>> @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)\n>>> \n>>> LOG(DmaHeap, Debug) << \"Using \" << info.deviceNodeName;\n>>> dmaHeapHandle_ = UniqueFD(ret);\n>>> + useUDmaBuf_ = info.useUDmaBuf;\n>>> break;\n>>> }\n>>> \n>>> @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;\n>>> * \\return True if the DmaHeap is valid, false otherwise\n>>> */\n>>> \n>>> -/**\n>>> - * \\brief Allocate a dma-buf from the DmaHeap\n>>> - * \\param [in] name The name to set for the allocated buffer\n>>> - * \\param [in] size The size of the buffer to allocate\n>>> - *\n>>> - * Allocates a dma-buf with read/write access.\n>>> - *\n>>> - * If the allocation fails, return an invalid UniqueFD.\n>>> - *\n>>> - * \\return The UniqueFD of the allocated buffer\n>>> - */\n>>> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n>>> +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)\n>>> {\n>>> - int ret;\n>>> -\n>>> - if (!name)\n>>> - return {};\n>>> -\n>>> struct dma_heap_allocation_data alloc = {};\n>>> + int ret;\n>>> \n>>> alloc.len = size;\n>>> alloc.fd_flags = O_CLOEXEC | O_RDWR;\n>>> @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n>>> return allocFd;\n>>> }\n>>> \n>>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)\n>>> +{\n>>> + /* size must be a multiple of the page-size round it up */\n> \n> s/size/Size/\n> s/ up/ up./\n\nAck.\n\n>>> + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n>>> + size = (size + pageMask) & ~pageMask;\n>>> +\n>>> + int ret = memfd_create(name, MFD_ALLOW_SEALING);\n> \n> Needs sys/mman.h.\n\nAck.\n\n>>> + if (ret < 0) {\n>>> + ret = errno;\n>>> + LOG(DmaHeap, Error)\n>>> + << \"UdmaHeap failed to allocate memfd storage: \"\n> \n> UdmaHeap sounds like a frankenstein monster :-) Did you mean UDmaBuf\n> here ? I would just drop the prefix from this message and the ones below\n> actually.\n\nI'll drop the prefix from the log message for v2.\n\n>>> + << strerror(ret);\n>>\n>> Completely optional/just an idea - I wonder if we should use 'name' in\n>> the log messages here and below.\n>>\n>> Would probably apply to allocFromHeap() too so likely a patch on top -\n>> not an update here.\n>>\n>>> + return {};\n>>> + }\n>>> +\n>>> + UniqueFD memfd(ret);\n>>> +\n>>> + ret = ftruncate(memfd.get(), size);\n>>> + if (ret < 0) {\n>>> + ret = errno;\n>>> + LOG(DmaHeap, Error)\n>>> + << \"UdmaHeap failed to set memfd size: \" << strerror(ret);\n>>> + return {};\n>>> + }\n>>> +\n>>> + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */\n> \n> s/seal/seal./\n> \n>>> + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n>>> + if (ret < 0) {\n>>> + ret = errno;\n>>> + LOG(DmaHeap, Error)\n>>> + << \"UdmaHeap failed to seal the memfd: \" << strerror(ret);\n>>> + return {};\n>>> + }\n>>> +\n>>> + struct udmabuf_create create;\n>>> +\n>>> + create.memfd = memfd.get();\n>>> + create.flags = UDMABUF_FLAGS_CLOEXEC;\n>>> + create.offset = 0;\n>>> + create.size = size;\n>>> +\n>>> + ret = ::ioctl(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);\n>>> + if (ret < 0) {\n>>> + ret = errno;\n>>> + LOG(DmaHeap, Error)\n>>> + << \"UdmaHeap failed to allocate \" << size << \" bytes: \"\n>>> + << strerror(ret);\n>>> + return {};\n>>> + }\n>>> +\n>>> + if (create.size < size) {\n>>> + LOG(DmaHeap, Error)\n>>> + << \"UdmaHeap allocated \" << create.size << \" bytes instead of \"\n>>> + << size << \" bytes\";\n>>\n>> Hrm, I think this path doesn't handle the now successfully created\n>> UDMABuf handle. Do we need to free it?\n>>\n>> Maybe we should move the 'UniqueFD uDma(ret);' above this statement so\n>> that the UniqueFD handles it correctly for both paths?\n> \n> Sounds good.\n\nAck.\n\n>>> + return {};\n>>> + }\n>>> +\n>>> + if (create.size != size)\n>>> + LOG(DmaHeap, Warning)\n>>> + << \"UdmaHeap allocated \" << create.size << \" bytes, \"\n>>> + << \"which is greater than requested : \" << size << \" bytes\";\n> \n> Why/when does this happen ?\n\nThis was taken from the original patch which I based this on. Shall I drop\nthe check ?\n\n> \n>>> +\n>>> + /* Fail if not suitable, the allocation will be free'd by UniqueFD */\n> \n> s/UniqueFD/UniqueFD./\n> \n> That comment doesn't seem correct though, there are no further checks.\n> Is it a leftover ?\n\nIf we move the UniqueFD declaration + this comment up then it does seem like\na valid comment.\n\n\n> \n>>> + LOG(DmaHeap, Debug) << \"UdmaHeap allocated \" << create.size << \" bytes\";\n>>> +\n>>> + /* The underlying memfd is kept as as a reference in the kernel */\n>>> + UniqueFD uDma(ret);\n>>> +\n>>> + return uDma;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Allocate a dma-buf from the DmaHeap\n>>\n>> I think this would now change from 'the DmaHeap' to 'a DmaHeap' but\n>> that's just a nit.\n> \n> I don't think that's correct. The allocator still allocates the buffer\n> from one specific backend, selected when the allocator is constructed.\n> \n>> Otherwise / with the above considered I'd put my name to this ;-)\n>> (err... I think I co-wrote it in the past so I'm biased ;D)\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> + * \\param [in] name The name to set for the allocated buffer\n>>> + * \\param [in] size The size of the buffer to allocate\n>>> + *\n>>> + * Allocates a dma-buf with read/write access.\n>>> + *\n>>> + * If the allocation fails, return an invalid UniqueFD.\n>>> + *\n>>> + * \\return The UniqueFD of the allocated buffer\n>>> + */\n>>> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n>>> +{\n>>> + if (!name)\n>>> + return {};\n>>> +\n>>> + if (useUDmaBuf_)\n>>> + return allocFromUDmaBuf(name, size);\n>>> + else\n>>> + return allocFromHeap(name, size);\n>>> +}\n>>> +\n>>> } /* namespace libcamera */\n> \n\n\nRegards,\n\nHans","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 37738BD87C\n\tfor ;\n\tWed, 29 May 2024 16:27:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E885D634B6;\n\tWed, 29 May 2024 18:26:59 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B57D6347E\n\tfor ;\n\tWed, 29 May 2024 18:26:57 +0200 (CEST)","from mail-lj1-f199.google.com (mail-lj1-f199.google.com\n\t[209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-277-vDfZU9WsOZWlsa-iPFd9kQ-1; Wed, 29 May 2024 12:26:54 -0400","by mail-lj1-f199.google.com with SMTP id\n\t38308e7fff4ca-2e95aba2b88so17627561fa.2\n\tfor ;\n\tWed, 29 May 2024 09:26:53 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-57872da9dfbsm6323528a12.2.2024.05.29.09.26.51\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 29 May 2024 09:26:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"RbKOvyuN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1717000016;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=hRwWlLM3sq/uVZFL60JyihmzW7TtI173T32v17mFF20=;\n\tb=RbKOvyuNc0sqBrS0BATdZg2YinyOYFLhgLmYv1LICu5IgBKllFFlbeVwIbeJnYTNHaTX9K\n\to6EUK2KI1AH6E5LwEzrb+MiP1NFivsGZQ6EhPClyIqyF1VuPZ3SGlFQlSPGaaWGswESNgn\n\t99qihva/11HLCP4zz12RZ+WSSGGE2z4=","X-MC-Unique":"vDfZU9WsOZWlsa-iPFd9kQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1717000013; x=1717604813;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=hRwWlLM3sq/uVZFL60JyihmzW7TtI173T32v17mFF20=;\n\tb=nCc6TQIxM1QLeXusLe7aZCM5XNuUarghUwiksdo47EoFcj0wF+6TayQOF1toXsa8T0\n\t7CwEa2d0boNbV2gN1/+nIhEeN0dIIbuNiqyjHnmPqY+rTTSY9SD6maHYwiBraZ0KUuXY\n\tMhSnws5GXgRKQ6qq/sOMJSmkkPQMGzWWBP1G2cizoSl+tpWwzuvDx2/oXGPL26mYU5Ab\n\t+5ajycKPw7fQT/Y1Bdtx/wI+hFXZ4TKNQ2K341OfYZYvZ68k/XLCKc0IUpouz88SiAim\n\txTUwnf8vKKJxawml9ExUZ5HoBmKwydey9PsQCfYwZNzBIPqkzwxZ5rU5Je/mWXaOsU++\n\tGa8A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWpOjOqvb11c/mM9lEH7UIeWtTDoQhk0X9Ndm6maJ+R9xlZ7TqinKMzA+oh97OPoVcBFojvKDrlcgeRBvC1MiDjwJLt/NMqFL5szJGsBRvjnMSo9g==","X-Gm-Message-State":"AOJu0Yw9p1DUyN8hUvrHi/FF0zEvENu585UpjYEfgrO+lBfMGk33/YRg\n\tlSnkpI/0d5PMIlDxwDcNFJVG0JeHGiQaChfw2siOsDxe4I3gzKF2l+6fZex8Mz66WJdlcWLS6+6\n\tXGPGZvvb15kq1t6d+tplfht65sYeBtSwYUx5w4RHKl2EdXhVIpUbU/WUpEcZZ3cZF+scRc4w=","X-Received":["by 2002:a2e:9c8f:0:b0:2e6:f59e:226f with SMTP id\n\t38308e7fff4ca-2e95b0bce90mr103702151fa.5.1717000012641; \n\tWed, 29 May 2024 09:26:52 -0700 (PDT)","by 2002:a2e:9c8f:0:b0:2e6:f59e:226f with SMTP id\n\t38308e7fff4ca-2e95b0bce90mr103702001fa.5.1717000012096; \n\tWed, 29 May 2024 09:26:52 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFwT9Py4R+lx8MOquff7E/P42/WK/4bRbmBjTW2uHbFe9816uKSz8WLdSdg2pyWy0o5QGBb2w==","Message-ID":"<0d4390d5-e466-4da1-a67d-feca891af92f@redhat.com>","Date":"Wed, 29 May 2024 18:26:50 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","To":"Laurent Pinchart ,\n\tKieran Bingham ","Cc":"Sakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ,\n\tDennis Bonke , \n\tHarvey Yang ","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>\n\t<20240528213746.GG8500@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240528213746.GG8500@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29668,"web_url":"https://patchwork.libcamera.org/comment/29668/","msgid":"<171700103567.2555695.9309583119077395852@ping.linuxembedded.co.uk>","date":"2024-05-29T16:43:55","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-05-29 17:26:50)\n> Hi,\n> \n> On 5/28/24 11:37 PM, Laurent Pinchart wrote:\n> > Hello,\n> > \n> > On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:\n> >> Quoting Hans de Goede (2024-05-27 15:16:47)\n> >>> Add support for using udmabuf to alloc DMA heaps, this is basically:\n> >>> https://patchwork.libcamera.org/patch/18922/\n> >>>\n> >>> ported from the never merged HeapAllocator class to the current DmaHeap\n> >>> class.\n> >>\n> >> True statements, but not really specific to the content of the patch?\n> >>\n> >> Perhaps:\n> >>\n> >> \"\"\"\n> >> The DMA heap allocator currently allocates from CMA and system heaps.\n> >>\n> >> These provide contiguous device memory but require dedicated and\n> >> reserved memory regions to be configured on the platform and may require\n> >> elevated privilidges to access.\n> > \n> > The system heap doesn't require reserved memory regions, and also\n> > doesn't provide contiguous device memory. The CMA heap also doesn't\n> > required reserved memory regions actually.\n> > \n> >> Extend the dma_heap allocator to support falling back to the memfd\n> >> allocator backed by the udmabuf framework to prepare buffers that are\n> >> compatible with linux drivers and the libcamera API.\n> > \n> > When reading the patch, my very first thought is that udmabuf doesn't\n> > make use of DMA heaps, so the allocator should belong to a different\n> > class.\n> > \n> > Thinking a bit more about it, I understand the appeal of hiding this\n> > fallback from the DmaHeap user. I'm however a bit concerned that the\n> > dmabuf instances allocated from the DMA system heap and through udmabuf\n> > will not operate in a fully identical way. Returning one or the other\n> > without the user being aware of what has been allocated may lead to\n> > issues.\n> > \n> > I may worry needlessly though. I haven't compared the system heap and\n> > the udmabuf implementations, to see if the dmabuf operations differ in\n> > ways that are significant. I think this should at least be checked\n> > before we make a decision.\n> > \n> >> The buffers allocated through memfd/udmabuf will not be contiguous and\n> >> are likely not suitable for directly rendering to a display or encoder\n> >> but may facilitate more usecases for allocating memory in pipelines that\n> > \n> > Let's make a stronger statement : \"are not suitable for zero-copy buffer\n> > sharing with other devices\".\n> \n> Sounds good I'll come up with a new commit message for v2 taking all the various\n> remarks into account.\n> \n> >> make use of the CPU for processing including virtual pipeline handlers\n> >> or the SoftISP.\n> >> \"\"\"\n> >>\n> >> Feel free to adapt the above of course if I've made any errors.\n> >>\n> >>> Co-developed-by: Harvey Yang \n> >>> Signed-off-by: Harvey Yang \n> >>> Signed-off-by: Hans de Goede \n> >>> ---\n> >>> include/libcamera/internal/dma_heaps.h | 3 +\n> >>> src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n> >>> 2 files changed, 109 insertions(+), 21 deletions(-)\n\n... snip ...\n\n> >>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)\n> >>> +{\n> >>> + /* size must be a multiple of the page-size round it up */\n> > \n> > s/size/Size/\n> > s/ up/ up./\n> \n> Ack.\n> \n> >>> + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n> >>> + size = (size + pageMask) & ~pageMask;\n> >>> +\n> >>> + int ret = memfd_create(name, MFD_ALLOW_SEALING);\n> > \n> > Needs sys/mman.h.\n> \n> Ack.\n\n... snip ...\n\n> \n> >>> + return {};\n> >>> + }\n> >>> +\n> >>> + if (create.size != size)\n> >>> + LOG(DmaHeap, Warning)\n> >>> + << \"UdmaHeap allocated \" << create.size << \" bytes, \"\n> >>> + << \"which is greater than requested : \" << size << \" bytes\";\n> > \n> > Why/when does this happen ?\n> \n> This was taken from the original patch which I based this on. Shall I drop\n> the check ?\n\nI'm fine dropping it. I bet it was from my prototyping code where I was\nlikely investigating why I didn't get the size I requested, which\nprobably led to the comment \"size must be a multiple of the page-size\nround it up\" at the top.\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 49A96BDE6B\n\tfor ;\n\tWed, 29 May 2024 16:44:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E356634B6;\n\tWed, 29 May 2024 18:44:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 825596347E\n\tfor ;\n\tWed, 29 May 2024 18:43:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 911694CD;\n\tWed, 29 May 2024 18:43:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cKFk+8sH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717001035;\n\tbh=qOfwCPirICJZX0zQz+O6faMZ+PksKmPkREaLA4WfqFw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=cKFk+8sHtCRWRSofWZKjI01Lxg07b9ed3lTuPuEavACEFG6A3ei8uZ0/N1pBwpSJ5\n\tp2huwN+3woSyXJBGYYODWBiZG9JbM5N56tpQrcFgY7kn5HvE1WetM+xET6CaxzjS7M\n\tW1atNcjZIwur7uuwm4N5c5hbZB+GywC7cRoonw8Q=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<0d4390d5-e466-4da1-a67d-feca891af92f@redhat.com>","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>\n\t<20240528213746.GG8500@pendragon.ideasonboard.com>\n\t<0d4390d5-e466-4da1-a67d-feca891af92f@redhat.com>","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","From":"Kieran Bingham ","Cc":"Sakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ,\n\tDennis Bonke , \n\tHarvey Yang ","To":"Hans de Goede ,\n\tLaurent Pinchart ","Date":"Wed, 29 May 2024 17:43:55 +0100","Message-ID":"<171700103567.2555695.9309583119077395852@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29669,"web_url":"https://patchwork.libcamera.org/comment/29669/","msgid":"","date":"2024-05-29T17:07:01","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 5/29/24 6:43 PM, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-05-29 17:26:50)\n>> Hi,\n>>\n>> On 5/28/24 11:37 PM, Laurent Pinchart wrote:\n>>> Hello,\n>>>\n>>> On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:\n>>>> Quoting Hans de Goede (2024-05-27 15:16:47)\n>>>>> Add support for using udmabuf to alloc DMA heaps, this is basically:\n>>>>> https://patchwork.libcamera.org/patch/18922/\n>>>>>\n>>>>> ported from the never merged HeapAllocator class to the current DmaHeap\n>>>>> class.\n>>>>\n>>>> True statements, but not really specific to the content of the patch?\n>>>>\n>>>> Perhaps:\n>>>>\n>>>> \"\"\"\n>>>> The DMA heap allocator currently allocates from CMA and system heaps.\n>>>>\n>>>> These provide contiguous device memory but require dedicated and\n>>>> reserved memory regions to be configured on the platform and may require\n>>>> elevated privilidges to access.\n>>>\n>>> The system heap doesn't require reserved memory regions, and also\n>>> doesn't provide contiguous device memory. The CMA heap also doesn't\n>>> required reserved memory regions actually.\n>>>\n>>>> Extend the dma_heap allocator to support falling back to the memfd\n>>>> allocator backed by the udmabuf framework to prepare buffers that are\n>>>> compatible with linux drivers and the libcamera API.\n>>>\n>>> When reading the patch, my very first thought is that udmabuf doesn't\n>>> make use of DMA heaps, so the allocator should belong to a different\n>>> class.\n>>>\n>>> Thinking a bit more about it, I understand the appeal of hiding this\n>>> fallback from the DmaHeap user. I'm however a bit concerned that the\n>>> dmabuf instances allocated from the DMA system heap and through udmabuf\n>>> will not operate in a fully identical way. Returning one or the other\n>>> without the user being aware of what has been allocated may lead to\n>>> issues.\n>>>\n>>> I may worry needlessly though. I haven't compared the system heap and\n>>> the udmabuf implementations, to see if the dmabuf operations differ in\n>>> ways that are significant. I think this should at least be checked\n>>> before we make a decision.\n>>>\n>>>> The buffers allocated through memfd/udmabuf will not be contiguous and\n>>>> are likely not suitable for directly rendering to a display or encoder\n>>>> but may facilitate more usecases for allocating memory in pipelines that\n>>>\n>>> Let's make a stronger statement : \"are not suitable for zero-copy buffer\n>>> sharing with other devices\".\n>>\n>> Sounds good I'll come up with a new commit message for v2 taking all the various\n>> remarks into account.\n>>\n>>>> make use of the CPU for processing including virtual pipeline handlers\n>>>> or the SoftISP.\n>>>> \"\"\"\n>>>>\n>>>> Feel free to adapt the above of course if I've made any errors.\n>>>>\n>>>>> Co-developed-by: Harvey Yang \n>>>>> Signed-off-by: Harvey Yang \n>>>>> Signed-off-by: Hans de Goede \n>>>>> ---\n>>>>> include/libcamera/internal/dma_heaps.h | 3 +\n>>>>> src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n>>>>> 2 files changed, 109 insertions(+), 21 deletions(-)\n> \n> ... snip ...\n> \n>>>>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)\n>>>>> +{\n>>>>> + /* size must be a multiple of the page-size round it up */\n>>>\n>>> s/size/Size/\n>>> s/ up/ up./\n>>\n>> Ack.\n>>\n>>>>> + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n>>>>> + size = (size + pageMask) & ~pageMask;\n>>>>> +\n>>>>> + int ret = memfd_create(name, MFD_ALLOW_SEALING);\n>>>\n>>> Needs sys/mman.h.\n>>\n>> Ack.\n> \n> ... snip ...\n> \n>>\n>>>>> + return {};\n>>>>> + }\n>>>>> +\n>>>>> + if (create.size != size)\n>>>>> + LOG(DmaHeap, Warning)\n>>>>> + << \"UdmaHeap allocated \" << create.size << \" bytes, \"\n>>>>> + << \"which is greater than requested : \" << size << \" bytes\";\n>>>\n>>> Why/when does this happen ?\n>>\n>> This was taken from the original patch which I based this on. Shall I drop\n>> the check ?\n> \n> I'm fine dropping it. I bet it was from my prototyping code where I was\n> likely investigating why I didn't get the size I requested,\n\nOk, I'll drop it.\n\n> which\n> probably led to the comment \"size must be a multiple of the page-size\n> round it up\" at the top.\n\nThe page-rounding of size is actually an addition by me, otherwise the\nUDMABUF_CREATE call fails with errno=EINVAL.\n\nRegards,\n\nHans","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9B2A6BD87C\n\tfor ;\n\tWed, 29 May 2024 17:07:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D081634B6;\n\tWed, 29 May 2024 19:07:08 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00E686347E\n\tfor ;\n\tWed, 29 May 2024 19:07:06 +0200 (CEST)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-401-hlr9NDPyPh2A8S64MExucQ-1; Wed, 29 May 2024 13:07:04 -0400","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-a62b36bc187so117190466b.3\n\tfor ;\n\tWed, 29 May 2024 10:07:04 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a626cc51debsm732770366b.114.2024.05.29.10.07.01\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 29 May 2024 10:07:02 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"JO8FCD0/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1717002425;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=mY7CKZ/1nRdWhtKoi1Da0DgVrtCZVpcRTkyhKbZWTiU=;\n\tb=JO8FCD0/OgS7Ht+2X+ymagw9dxxkwAaybuJ6bgBpi3LRTpAGnY1coEIUp8nYfiSTvpt2lB\n\t7f6LcvmUyUi0m4xFi8DbCfPExgEh01wyUOIa5cL57gJ1HXjgHugaAV0OV8QSHp0jQpinvx\n\trbAj/VtONlhavdgs1wsAekIqPOuFduY=","X-MC-Unique":"hlr9NDPyPh2A8S64MExucQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1717002423; x=1717607223;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=mY7CKZ/1nRdWhtKoi1Da0DgVrtCZVpcRTkyhKbZWTiU=;\n\tb=VQFwor7ajVhyUG8mQKX7MAhnRAa6ctDtg23TsvHPhD6z/Vb2TVSC9csVcwbzMF2kpf\n\to89s5ELWKjG3+v49j8G61USMpN6AHMhtkOjL/1sLOvXT0lKQKR6Zny/WQ6VFoVpM0B9y\n\tl90n24ztEq+YOElk9MYJZltAVbcLvC+e8M6SR+44f2j7WRZRQT0KpOTac1aQoFCxfGqN\n\tfjuMC7eip3mq3v9FfF2lvh5lhvFB8Pqt5RhYpWoiQGTabpqSKP5nQtL70oESjvF2o77h\n\tN/rHw9f0we4DSBuU3GCn9rgE5INC/1bdK02waDyJE+UYPZQCmOWHOE2gF/mRM/KUJ89G\n\tHqfw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUUaFY9kLrGqC+MJbupW0PGADDdOWb75S2fTxGt0jS2gDGNAqcRRAlEhVahsoaiVdr3zHt2sjGpONWkg03WZMcMxqeKs5vdgumokjBXGfqjIZ2/qg==","X-Gm-Message-State":"AOJu0Yyir2D/ZpOUonPbZWUzFBx63DVcS3JYmXQx/zQMu2m8XdGwVcDp\n\tfpvfS2encjo8f5DS4TovdUi0c3pZZW0DG7GSEeHJ1ctDbgdTRs648xxU55foemkZDsPfZ7rQ9Hr\n\tJP8RBmztfllMFNOSbTttZt5ab0jifKCOGWDoFG1DXFeaCDK2IFvNuFZARBQ3O6lXASZh6SmY=","X-Received":["by 2002:a17:906:e219:b0:a65:4268:dfca with SMTP id\n\ta640c23a62f3a-a654268e0f0mr150108266b.65.1717002422913; \n\tWed, 29 May 2024 10:07:02 -0700 (PDT)","by 2002:a17:906:e219:b0:a65:4268:dfca with SMTP id\n\ta640c23a62f3a-a654268e0f0mr150106766b.65.1717002422403; \n\tWed, 29 May 2024 10:07:02 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEz4OzgvGXBjjGn+GxrbbYVzwsKdXrNj4NcaS6mFPOxJNnnbsvrVmSGgxTgzlv8CcD9o2zOlA==","Message-ID":"","Date":"Wed, 29 May 2024 19:07:01 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Cc":"Sakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ,\n\tDennis Bonke , \n\tHarvey Yang ","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>\n\t<20240528213746.GG8500@pendragon.ideasonboard.com>\n\t<0d4390d5-e466-4da1-a67d-feca891af92f@redhat.com>\n\t<171700103567.2555695.9309583119077395852@ping.linuxembedded.co.uk>","From":"Hans de Goede ","In-Reply-To":"<171700103567.2555695.9309583119077395852@ping.linuxembedded.co.uk>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29670,"web_url":"https://patchwork.libcamera.org/comment/29670/","msgid":"<171700305087.2248009.17101743862208257433@ping.linuxembedded.co.uk>","date":"2024-05-29T17:17:30","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-05-29 18:07:01)\n> Hi,\n> \n> On 5/29/24 6:43 PM, Kieran Bingham wrote:\n> > Quoting Hans de Goede (2024-05-29 17:26:50)\n> >> Hi,\n> >>\n> >> On 5/28/24 11:37 PM, Laurent Pinchart wrote:\n> >>> Hello,\n> >>>\n> >>> On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:\n> >>>> Quoting Hans de Goede (2024-05-27 15:16:47)\n> >>>>> Add support for using udmabuf to alloc DMA heaps, this is basically:\n> >>>>> https://patchwork.libcamera.org/patch/18922/\n> >>>>>\n> >>>>> ported from the never merged HeapAllocator class to the current DmaHeap\n> >>>>> class.\n> >>>>\n> >>>> True statements, but not really specific to the content of the patch?\n> >>>>\n> >>>> Perhaps:\n> >>>>\n> >>>> \"\"\"\n> >>>> The DMA heap allocator currently allocates from CMA and system heaps.\n> >>>>\n> >>>> These provide contiguous device memory but require dedicated and\n> >>>> reserved memory regions to be configured on the platform and may require\n> >>>> elevated privilidges to access.\n> >>>\n> >>> The system heap doesn't require reserved memory regions, and also\n> >>> doesn't provide contiguous device memory. The CMA heap also doesn't\n> >>> required reserved memory regions actually.\n> >>>\n> >>>> Extend the dma_heap allocator to support falling back to the memfd\n> >>>> allocator backed by the udmabuf framework to prepare buffers that are\n> >>>> compatible with linux drivers and the libcamera API.\n> >>>\n> >>> When reading the patch, my very first thought is that udmabuf doesn't\n> >>> make use of DMA heaps, so the allocator should belong to a different\n> >>> class.\n> >>>\n> >>> Thinking a bit more about it, I understand the appeal of hiding this\n> >>> fallback from the DmaHeap user. I'm however a bit concerned that the\n> >>> dmabuf instances allocated from the DMA system heap and through udmabuf\n> >>> will not operate in a fully identical way. Returning one or the other\n> >>> without the user being aware of what has been allocated may lead to\n> >>> issues.\n> >>>\n> >>> I may worry needlessly though. I haven't compared the system heap and\n> >>> the udmabuf implementations, to see if the dmabuf operations differ in\n> >>> ways that are significant. I think this should at least be checked\n> >>> before we make a decision.\n> >>>\n> >>>> The buffers allocated through memfd/udmabuf will not be contiguous and\n> >>>> are likely not suitable for directly rendering to a display or encoder\n> >>>> but may facilitate more usecases for allocating memory in pipelines that\n> >>>\n> >>> Let's make a stronger statement : \"are not suitable for zero-copy buffer\n> >>> sharing with other devices\".\n> >>\n> >> Sounds good I'll come up with a new commit message for v2 taking all the various\n> >> remarks into account.\n> >>\n> >>>> make use of the CPU for processing including virtual pipeline handlers\n> >>>> or the SoftISP.\n> >>>> \"\"\"\n> >>>>\n> >>>> Feel free to adapt the above of course if I've made any errors.\n> >>>>\n> >>>>> Co-developed-by: Harvey Yang \n> >>>>> Signed-off-by: Harvey Yang \n> >>>>> Signed-off-by: Hans de Goede \n> >>>>> ---\n> >>>>> include/libcamera/internal/dma_heaps.h | 3 +\n> >>>>> src/libcamera/dma_heaps.cpp | 127 +++++++++++++++++++++----\n> >>>>> 2 files changed, 109 insertions(+), 21 deletions(-)\n> > \n> > ... snip ...\n> > \n> >>>>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)\n> >>>>> +{\n> >>>>> + /* size must be a multiple of the page-size round it up */\n> >>>\n> >>> s/size/Size/\n> >>> s/ up/ up./\n> >>\n> >> Ack.\n> >>\n> >>>>> + std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n> >>>>> + size = (size + pageMask) & ~pageMask;\n> >>>>> +\n> >>>>> + int ret = memfd_create(name, MFD_ALLOW_SEALING);\n> >>>\n> >>> Needs sys/mman.h.\n> >>\n> >> Ack.\n> > \n> > ... snip ...\n> > \n> >>\n> >>>>> + return {};\n> >>>>> + }\n> >>>>> +\n> >>>>> + if (create.size != size)\n> >>>>> + LOG(DmaHeap, Warning)\n> >>>>> + << \"UdmaHeap allocated \" << create.size << \" bytes, \"\n> >>>>> + << \"which is greater than requested : \" << size << \" bytes\";\n> >>>\n> >>> Why/when does this happen ?\n> >>\n> >> This was taken from the original patch which I based this on. Shall I drop\n> >> the check ?\n> > \n> > I'm fine dropping it. I bet it was from my prototyping code where I was\n> > likely investigating why I didn't get the size I requested,\n> \n> Ok, I'll drop it.\n\nI dug up the original WIP:\n - https://github.com/kbingham/libcamera/commit/8d79af53eaf461bab246c2fdeb1d9fdcbb288743#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R102-R109\n\nSo it probably started there indeed. Anyway - I think this code has come\na long way (for the better) since then ;-)\n\n\n> > which\n> > probably led to the comment \"size must be a multiple of the page-size\n> > round it up\" at the top.\n> \n> The page-rounding of size is actually an addition by me, otherwise the\n> UDMABUF_CREATE call fails with errno=EINVAL.\n\nAha, well then it's a great addition ;-)\n\nThanks!\n\n--\nKieran\n\n> \n> Regards,\n> \n> Hans\n> \n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4D036BDE6B\n\tfor ;\n\tWed, 29 May 2024 17:17:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A181634B6;\n\tWed, 29 May 2024 19:17:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBA326347E\n\tfor ;\n\tWed, 29 May 2024 19:17:33 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C97CEA27;\n\tWed, 29 May 2024 19:17:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ct7ArrRE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717003049;\n\tbh=mLl5vsEMYSHvE3P8Iui0kuHP9AEfrTsOCvWymu3ugw8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ct7ArrREhaK2pAo1BdcsAjyMlidJHaDWfItRLwcbXpBwuQXS7h7iRlT9JTSl4Epbv\n\t5W45uwUw2jKh83+/hX6eBX8ytFU9PnFCTrRMATfszOgzq6E0F/6dOGjKNinfacWFIO\n\tSkiGOtC8GuRNH7sMA8z6DB8gPb+VZ3fEn9SHlFXw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<171689593044.668709.2617407682457935851@ping.linuxembedded.co.uk>\n\t<20240528213746.GG8500@pendragon.ideasonboard.com>\n\t<0d4390d5-e466-4da1-a67d-feca891af92f@redhat.com>\n\t<171700103567.2555695.9309583119077395852@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","From":"Kieran Bingham ","Cc":"Sakari Ailus ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ,\n\tDennis Bonke , \n\tHarvey Yang ","To":"Hans de Goede ,\n\tLaurent Pinchart ","Date":"Wed, 29 May 2024 18:17:30 +0100","Message-ID":"<171700305087.2248009.17101743862208257433@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29675,"web_url":"https://patchwork.libcamera.org/comment/29675/","msgid":"<24a4dea8-5bf3-4042-8b22-8fe398a90acf@redhat.com>","date":"2024-05-30T17:08:52","subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Brian,\n\nOn 5/29/24 2:29 AM, Bryan O'Donoghue wrote:\n> On 27/05/2024 15:16, Hans de Goede wrote:\n>> +static constexpr std::array heapInfos = { {\n>> +    { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\", false },\n>> +    { DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\", false },\n>> +    { DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\", false },\n>> +    { DmaHeap::DmaHeapFlag::System, \"/dev/udmabuf\", true },\n>>   } };\n> \n> I have\n> \n> deckard@x13s-linux:~/Development/linux$ ls /dev/udmabuf\n> /dev/udmabuf\n> deckard@x13s-linux:~/Development/linux$ ls /dev/d\n> disk/     dma_heap/ dri/\n> deckard@x13s-linux:~/Development/linux$ ls /dev/dma_heap/\n> linux,cma  system\n> \n> LIBCAMERA_LOG_LEVELS=*:DEBUG ./build.master.dbg/src/apps/qcam/qcam\n> \n> Gives\n> \n> [0:15:09.128108059] [6874] DEBUG DmaHeap dma_heaps.cpp:112 Using /dev/dma_heap/linux,cma\n> \n> I thought we said udmabuf should be default over cma_heap ?\n> \n> At a risk of displaying a lack of knowledge, I tried forcing the flag to useUDmaBuf = true;\n> \n> I get\n> \n> Zero-copy enabled\n> [0:18:13.834305484] [7388] ERROR DmaHeap dma_heaps.cpp:201 UdmaHeap failed to allocate 15073280 bytes: Invalid argument\n> [0:18:13.834365480] [7388] ERROR SoftwareIsp software_isp.cpp:246 failed to allocate a dma_buf\n> \n> Do I need to do something more than CONFIG_UDAMBUF=y ?\n\nIf you just set \"useUDmaBuf = true;\" then the DmaHeap code will still\nopen /dev/dma_heap/linux,cma but you are now using that fd with\ncode-paths which expect an fd for /dev/udmabuf so it is unsurprising\nthat that does not work.\n\nTo test the udmabuf code you can either modify:\n\nstatic constexpr std::array heapInfos = { {\n\t{ DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/linux,cma\", false },\n\t{ DmaHeap::DmaHeapFlag::Cma, \"/dev/dma_heap/reserved\", false },\n\t{ DmaHeap::DmaHeapFlag::System, \"/dev/dma_heap/system\", false },\n\t{ DmaHeap::DmaHeapFlag::System, \"/dev/udmabuf\", true },\n} };\n\nTo only list the /dev/udmabuf line; or just do:\n\nsudo rm /dev/dma_heap/*\n\nNote I'm posting a v2 of the udmabuf series in a couple of minutes,\nif you are going to re-test please test that.\n\nRegards,\n\nHans\n\n\n \n\n> \n> ---\n> bod\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 99173BDE6B\n\tfor ;\n\tThu, 30 May 2024 17:09:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77F57634B6;\n\tThu, 30 May 2024 19:09:01 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F194361A43\n\tfor ;\n\tThu, 30 May 2024 19:08:59 +0200 (CEST)","from mail-lf1-f72.google.com (mail-lf1-f72.google.com\n\t[209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-691-2bZ1FU3AMFqT3tL11e--vQ-1; Thu, 30 May 2024 13:08:56 -0400","by mail-lf1-f72.google.com with SMTP id\n\t2adb3069b0e04-52b799b71b5so754681e87.2\n\tfor ;\n\tThu, 30 May 2024 10:08:56 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a66a6d4b05fsm48433166b.1.2024.05.30.10.08.52\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tThu, 30 May 2024 10:08:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"fK10U0JW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1717088938;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=sIGoMgWx9Q4Gp3+aQgi1XYKerx9qcaDAhabOZzcFXE4=;\n\tb=fK10U0JWUk+addDp+JI7WpdmJVtO1J+v16S+BvyqCnBXoupr6ybkItTcLpPPEeAzsUsAtt\n\tlz8rIHac2X3RE6bdl96M8d4Vlr6HVgW9aOyKfJ6y5izX1NYHVKWVnEs0sM8wLHHAxd4hiW\n\trDfylDPgOVB6SHIxgQDNY7ZFIaSLCOE=","X-MC-Unique":"2bZ1FU3AMFqT3tL11e--vQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1717088935; x=1717693735;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=sIGoMgWx9Q4Gp3+aQgi1XYKerx9qcaDAhabOZzcFXE4=;\n\tb=lj+KCET+mUb8nP9Ul9XD7/G7OGIqCyJq0SGTxApFNbrFL2fIMvNlYdb+ebfqpCVblK\n\t+tih3sSMzhjHoEupUviFSE2yfWekL+p/g3HfYvfUfp4f1FyPwMC/sL0CdZ5G+oA/6wc7\n\tgz04MgBPilbiDMvXWKv0kLBvHvCICgNWGMiBPVPZlFRZM/F0n/1ykqYv91x+wESkqGLt\n\tsZ2kXUNnLBIGtNFj48k+n1bL3n7CC6l/SwgcIBSPRmRD50SNAwXYnI1ZWtXoQcXyh/NX\n\tqCzXm3GsdJFDUX9xn1aivrs/qDPWrA3oFdntFRo8bDli7nj23e4fQGp+HdAISxPJzf8R\n\tj7CQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWKysszgOWaDIHX8hr3NwJz2z4TPE7XGOzqNZ1PLeXH0huFazKtc9vmHODMJ9YNcDt63MMD7Tuxl0VyPZYj1Qs3geMP6fvZPZ0DUQcrAPrMdnvC1A==","X-Gm-Message-State":"AOJu0YwQwS4j1zFE4Q2VsYPm0AC4k5ctpujUR6rkeZiecv9KswOupqcO\n\t1s2CnQ7x1aUsZWi6gGoLv5vxFc2MVNm7m6sx0voAuRoTF/7SKEpGwIvWie2uDeTjfutH5ABkRxz\n\t7H723z2HKOc6D6/iVTmQAZKqSp6fIwcDvtSOCAXo4Inlsvp3OVaYfW5BTVJ1sa7Q4gHCVNDO4YI\n\th8qfE=","X-Received":["by 2002:ac2:4c23:0:b0:52b:796f:8af5 with SMTP id\n\t2adb3069b0e04-52b7d428a90mr1763295e87.34.1717088934877; \n\tThu, 30 May 2024 10:08:54 -0700 (PDT)","by 2002:ac2:4c23:0:b0:52b:796f:8af5 with SMTP id\n\t2adb3069b0e04-52b7d428a90mr1763277e87.34.1717088934088; \n\tThu, 30 May 2024 10:08:54 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHJ8qc/KjY2hXcJe3T4xjCQQWLekcOd/MvKmq2TYyk/WdJHI+y+NtUHI+LPkExGKq766uSQEg==","Message-ID":"<24a4dea8-5bf3-4042-8b22-8fe398a90acf@redhat.com>","Date":"Thu, 30 May 2024 19:08:52 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: dma_heaps: Add support for using udmabuf to\n\talloc DMA heaps","To":"Bryan O'Donoghue ,\n\tlibcamera-devel@lists.libcamera.org,\n\tSakari Ailus ","Cc":"Maxime Ripard , Milan Zamazal , \n\tDennis Bonke ,\n\tHarvey Yang ","References":"<20240527141647.336633-1-hdegoede@redhat.com>\n\t<363e109d-439b-4ce4-81ff-ba063774a274@linaro.org>","From":"Hans de Goede ","In-Reply-To":"<363e109d-439b-4ce4-81ff-ba063774a274@linaro.org>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]