[{"id":26430,"web_url":"https://patchwork.libcamera.org/comment/26430/","msgid":"","date":"2023-02-08T10:47:29","subject":"Re: [libcamera-devel] [PATCH v1 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Harvey\n\nOn Wed, 8 Feb 2023 at 09:59, Harvey Yang via libcamera-devel\n wrote:\n>\n> As other components (like the WIP virtual pipeline handler) also need a\n> heap allocator, move DmaHeap from raspberry pi pipeline handler to a\n> general HeapAllocator as a base class.\n>\n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/dma_heap.h | 20 +++++++\n> include/libcamera/heap.h | 27 ++++++++++\n> include/libcamera/heap_allocator.h | 30 +++++++++++\n> include/libcamera/meson.build | 3 ++\n> .../dma_heaps.cpp => dma_heap.cpp} | 54 ++++++++++++-------\n> src/libcamera/heap_allocator.cpp | 43 +++++++++++++++\n> src/libcamera/meson.build | 2 +\n> .../pipeline/raspberrypi/dma_heaps.h | 32 -----------\n> .../pipeline/raspberrypi/meson.build | 1 -\n> .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++--\n> 10 files changed, 164 insertions(+), 58 deletions(-)\n> create mode 100644 include/libcamera/dma_heap.h\n> create mode 100644 include/libcamera/heap.h\n> create mode 100644 include/libcamera/heap_allocator.h\n> rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp => dma_heap.cpp} (56%)\n> create mode 100644 src/libcamera/heap_allocator.cpp\n> delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h\n>\n> diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h\n> new file mode 100644\n> index 00000000..96e311d3\n> --- /dev/null\n> +++ b/include/libcamera/dma_heap.h\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * dma_heap.h - Dma Heap implementation.\n> + */\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class DmaHeap : public Heap\n> +{\n> +public:\n> + DmaHeap();\n> + ~DmaHeap();\n> + UniqueFD alloc(const char *name, std::size_t size) override;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h\n> new file mode 100644\n> index 00000000..c49a2ac3\n> --- /dev/null\n> +++ b/include/libcamera/heap.h\n> @@ -0,0 +1,27 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * heap.h - Heap interface.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Heap\n> +{\n> +public:\n> + virtual ~Heap() = default;\n> + bool isValid() const { return handle_.isValid(); }\n> + virtual UniqueFD alloc(const char *name, std::size_t size) = 0;\n> +\n> +protected:\n> + UniqueFD handle_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h\n> new file mode 100644\n> index 00000000..cd7ed1a3\n> --- /dev/null\n> +++ b/include/libcamera/heap_allocator.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * heap_allocator.h - Helper class for heap buffer allocations.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class HeapAllocator\n> +{\n> +public:\n> + HeapAllocator();\n> + ~HeapAllocator();\n> + bool isValid() const { return heap_->isValid(); }\n> + UniqueFD alloc(const char *name, std::size_t size);\n> +\n> +private:\n> + std::unique_ptr heap_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 408b7acf..f486630a 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -7,10 +7,13 @@ libcamera_public_headers = files([\n> 'camera_manager.h',\n> 'color_space.h',\n> 'controls.h',\n> + 'dma_heap.h',\n> 'fence.h',\n> 'framebuffer.h',\n> 'framebuffer_allocator.h',\n> 'geometry.h',\n> + 'heap.h',\n> + 'heap_allocator.h',\n> 'logging.h',\n> 'pixel_format.h',\n> 'request.h',\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/dma_heap.cpp\n> similarity index 56%\n> rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> rename to src/libcamera/dma_heap.cpp\n> index 6b644406..8b67e9d0 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> +++ b/src/libcamera/dma_heap.cpp\n> @@ -1,19 +1,20 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2020, Raspberry Pi Ltd\n> + * Copyright (C) 2023, Google Inc.\n\nMoving and then editing a file warrants claiming sole copyright?\nSounds legally dubious to me.\n\n Dave\n\n> *\n> - * dma_heaps.h - Helper class for dma-heap allocations.\n> + * dma_heap.h - Dma Heap implementation.\n> */\n>\n> -#include \"dma_heaps.h\"\n> +#include \n>\n> #include \n> #include \n> -#include \n> -#include \n> #include \n> #include \n>\n> +#include \n> +#include \n> +\n> #include \n>\n> /*\n> @@ -30,9 +31,7 @@ static constexpr std::array heapNames = {\n>\n> namespace libcamera {\n>\n> -LOG_DECLARE_CATEGORY(RPI)\n> -\n> -namespace RPi {\n> +LOG_DEFINE_CATEGORY(DmaHeap)\n>\n> DmaHeap::DmaHeap()\n> {\n> @@ -40,17 +39,34 @@ DmaHeap::DmaHeap()\n> int ret = ::open(name, O_RDWR, 0);\n> if (ret < 0) {\n> ret = errno;\n> - LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> - << strerror(ret);\n> + LOG(DmaHeap, Debug) << \"Failed to open \" << name << \": \"\n> + << strerror(ret);\n> continue;\n> }\n>\n> - dmaHeapHandle_ = UniqueFD(ret);\n> + handle_ = UniqueFD(ret);\n> break;\n> }\n>\n> - if (!dmaHeapHandle_.isValid())\n> - LOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> + if (!handle_.isValid())\n> + LOG(DmaHeap, Error) << \"Could not open any dmaHeap device\";\n> +\n> + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(DmaHeap, Error)\n> + << \"Failed to open allocator: \" << strerror(ret);\n> +\n> + if (ret == EACCES) {\n> + LOG(DmaHeap, Info)\n> + << \"Consider making /dev/udmabuf accessible by the video group\";\n> + LOG(DmaHeap, Info)\n> + << \"Alternatively, add your user to the kvm group.\";\n> + }\n> +\n> + } else {\n> + handle_ = UniqueFD(ret);\n> + }\n> }\n>\n> DmaHeap::~DmaHeap() = default;\n> @@ -67,24 +83,22 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)\n> alloc.len = size;\n> alloc.fd_flags = O_CLOEXEC | O_RDWR;\n>\n> - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> if (ret < 0) {\n> - LOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> - << name;\n> + LOG(DmaHeap, Error) << \"dmaHeap allocation failure for \"\n> + << name;\n> return {};\n> }\n>\n> UniqueFD allocFd(alloc.fd);\n> ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> if (ret < 0) {\n> - LOG(RPI, Error) << \"dmaHeap naming failure for \"\n> - << name;\n> + LOG(DmaHeap, Error) << \"dmaHeap naming failure for \"\n> + << name;\n> return {};\n> }\n>\n> return allocFd;\n> }\n>\n> -} /* namespace RPi */\n> -\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> new file mode 100644\n> index 00000000..594b1d6a\n> --- /dev/null\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Google Inc.\n> + *\n> + * heap_allocator.cpp - Helper class for heap buffer allocations.\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(HeapAllocator)\n> +\n> +HeapAllocator::HeapAllocator()\n> +{\n> + heap_ = std::make_unique();\n> +}\n> +\n> +HeapAllocator::~HeapAllocator() = default;\n> +\n> +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)\n> +{\n> + if (!isValid()) {\n> + LOG(HeapAllocator, Fatal) << \"Allocation attempted without allocator\" << name;\n> + return {};\n> + }\n> +\n> + return heap_->alloc(name, size);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 9869bfe7..ee586c0d 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -17,11 +17,13 @@ libcamera_sources = files([\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> + 'dma_heap.cpp',\n> 'fence.cpp',\n> 'formats.cpp',\n> 'framebuffer.cpp',\n> 'framebuffer_allocator.cpp',\n> 'geometry.cpp',\n> + 'heap_allocator.cpp',\n> 'ipa_controls.cpp',\n> 'ipa_data_serializer.cpp',\n> 'ipa_interface.cpp',\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> deleted file mode 100644\n> index 0a4a8d86..00000000\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> +++ /dev/null\n> @@ -1,32 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Raspberry Pi Ltd\n> - *\n> - * dma_heaps.h - Helper class for dma-heap allocations.\n> - */\n> -\n> -#pragma once\n> -\n> -#include \n> -\n> -#include \n> -\n> -namespace libcamera {\n> -\n> -namespace RPi {\n> -\n> -class DmaHeap\n> -{\n> -public:\n> - DmaHeap();\n> - ~DmaHeap();\n> - bool isValid() const { return dmaHeapHandle_.isValid(); }\n> - UniqueFD alloc(const char *name, std::size_t size);\n> -\n> -private:\n> - UniqueFD dmaHeapHandle_;\n> -};\n> -\n> -} /* namespace RPi */\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index 6064a3f0..42b96286 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -2,7 +2,6 @@\n>\n> libcamera_sources += files([\n> 'delayed_controls.cpp',\n> - 'dma_heaps.cpp',\n> 'raspberrypi.cpp',\n> 'rpi_stream.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 8569df17..18325faa 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -20,6 +20,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -41,7 +42,6 @@\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n>\n> #include \"delayed_controls.h\"\n> -#include \"dma_heaps.h\"\n> #include \"rpi_stream.h\"\n>\n> using namespace std::chrono_literals;\n> @@ -240,7 +240,7 @@ public:\n> std::vector, MediaLink *>> bridgeDevices_;\n>\n> /* DMAHEAP allocation helper. */\n> - RPi::DmaHeap dmaHeap_;\n> + HeapAllocator heapAllocator_;\n> SharedFD lsTable_;\n>\n> std::unique_ptr delayedCtrls_;\n> @@ -1206,7 +1206,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> {\n> std::unique_ptr data = std::make_unique(this);\n>\n> - if (!data->dmaHeap_.isValid())\n> + if (!data->heapAllocator_.isValid())\n> return -ENOMEM;\n>\n> MediaEntity *unicamImage = unicam->getEntityByName(\"unicam-image\");\n> @@ -1595,9 +1595,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA\n> /* Always send the user transform to the IPA. */\n> ipaConfig.transform = static_cast(config->transform);\n>\n> - /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> + /* Allocate the lens shading table via heapAllocator and pass to the IPA. */\n> if (!lsTable_.isValid()) {\n> - lsTable_ = SharedFD(dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize));\n> + lsTable_ = SharedFD(heapAllocator_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize));\n> if (!lsTable_.isValid())\n> return -ENOMEM;\n>\n> --\n> 2.39.1.519.gcb327c4b5f-goog\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 26D64BDB1C\n\tfor ;\n\tWed, 8 Feb 2023 10:47:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8479D625E3;\n\tWed, 8 Feb 2023 11:47:48 +0100 (CET)","from mail-vk1-xa30.google.com (mail-vk1-xa30.google.com\n\t[IPv6:2607:f8b0:4864:20::a30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0EC7E625DD\n\tfor ;\n\tWed, 8 Feb 2023 11:47:46 +0100 (CET)","by mail-vk1-xa30.google.com with SMTP id bs10so9435790vkb.3\n\tfor ;\n\tWed, 08 Feb 2023 02:47:45 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675853268;\n\tbh=54O+is5dAsuOTXm4XGo8dq40czYjaMLhT48/SjDxuXY=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=UdnwmGh/+CxMMxYNlQfYo85KpqAMtTnUG+BNks6sVvI4g3I/1k8fJaKisTQLhZaCf\n\t+393WWy7FtdbTCbcvZHeUtmS4KhMGtyYToE/VhEzSxHD08MTMxSpW0sfBNYQ7Mpm0l\n\tPgQMzlPDSyOc7MAB7w9SQ3i2d6trRA9ZdkEjiAWN/p2q+77C7PDeOcb0D47M95/rb0\n\tzZSjgaZUM9kCJN/89zAYHor2Udj7GV1XBMdfX5h27ulF1+P6W9sqh18JdMUy8hKQE4\n\tLW0kvmWjBY00HlX98wHv4VOu/3DeqsT48NcdF3z4ssIpZLhSlMKSQIZwzeOgZtxD0V\n\tIpV/VDIRRB7vw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=qSeoLz1g0zM2U08HqwxTFfJal7rpTPsk1wCV/UyDrbk=;\n\tb=WAxbNH5t/fkf2lAXXOKzacb9gPrk28o6PdU/A51EleBL0ZVHVZV/i4QmUHXvgKQyUg\n\ta1frCPLQd7dqypbZaGf4PdQ9ZiPWw7Lvl9KtAh6bxxANnN+mWIdAl4j/LH/TQx+e5hLd\n\tgkmgcwDiixWSZd27ppTKKp2JhBu/+LScNKBMd7eRlBPGUqeTX3+Xb8g5pRmviHLzzusd\n\tNnmsehkr0YznnqibyxYKnWk1eMJu0HbA4A1TX2baKlSJwhV6sQ/7d8/4LnJHxlM4/nnQ\n\tR1yHMk0vAuDniFKJcQ/pX0K5FhhSH3kdM2AYulOwroqR5fj24Bk5txJh0RHKxdE/fORt\n\tTAyA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"WAxbNH5t\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=qSeoLz1g0zM2U08HqwxTFfJal7rpTPsk1wCV/UyDrbk=;\n\tb=weudtS1L2djiZ37/IKOFRCkLt//BbG/YO6o7ibK3WmRK59TffJY++6GWBVawcPjx6Y\n\th5kRv+sEVi79qDa1rsQ7UShV/IykWMqCHLLe0zuDNN0Vo4iYzIVsXwUntLSJm2iu5HN3\n\tFouFkkc5gpMoQbTZjmQ8HT/OQmwsL1eI4OEBMa82sf4kkYOIjD5mqtpHqPmr0kL8nLDv\n\tNhtPNesWe/EZS3Oxve9VGn2JAoOeDo68UqDzD1ZFcdZPF+TsLRLNlWjDZPpeVe//oqF1\n\tfNhKqAkapDxYricKpY8m4nFaliWf4bzHxZYlfTdKsx7dO8qn+PL6XagauPPX49BsBK/L\n\te4jQ==","X-Gm-Message-State":"AO0yUKVPrRigU5R3lEEGrsHGzFltsuXuYSyKmkVHy64cSY+6cj2MvWql\n\tbieaWOl8tTVxnWK/65wzdzdQRHMKhx71sqxundXQAw==","X-Google-Smtp-Source":"AK7set/RaUA4hzVUzcc14lO+xhT7eSmtHQwWGr3hdip8HUgOuPgDVXHMJVD7Q9OLh/bUIYGjIZw6Gx2OaCaerBfU3r8=","X-Received":"by 2002:a1f:2b91:0:b0:3f8:12a2:ea4e with SMTP id\n\tr139-20020a1f2b91000000b003f812a2ea4emr1534244vkr.36.1675853264787;\n\tWed, 08 Feb 2023 02:47:44 -0800 (PST)","MIME-Version":"1.0","References":"<20230208095922.1471175-1-chenghaoyang@google.com>\n\t<20230208095922.1471175-2-chenghaoyang@google.com>","In-Reply-To":"<20230208095922.1471175-2-chenghaoyang@google.com>","Date":"Wed, 8 Feb 2023 10:47:29 +0000","Message-ID":"","To":"Harvey Yang ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","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","From":"Dave Stevenson via libcamera-devel\n\t","Reply-To":"Dave Stevenson ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26431,"web_url":"https://patchwork.libcamera.org/comment/26431/","msgid":"","date":"2023-02-08T11:58:50","subject":"Re: [libcamera-devel] [PATCH v1 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","submitter":{"id":148,"url":"https://patchwork.libcamera.org/api/people/148/","name":"Cheng-Hao Yang","email":"chenghaoyang@google.com"},"content":"Hi Dave,\n\nThanks for pointing it out. Will update in the next patch.\n\nOn Wed, Feb 8, 2023 at 6:47 PM Dave Stevenson <\ndave.stevenson@raspberrypi.com> wrote:\n\n> Hi Harvey\n>\n> On Wed, 8 Feb 2023 at 09:59, Harvey Yang via libcamera-devel\n> wrote:\n> >\n> > As other components (like the WIP virtual pipeline handler) also need a\n> > heap allocator, move DmaHeap from raspberry pi pipeline handler to a\n> > general HeapAllocator as a base class.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > include/libcamera/dma_heap.h | 20 +++++++\n> > include/libcamera/heap.h | 27 ++++++++++\n> > include/libcamera/heap_allocator.h | 30 +++++++++++\n> > include/libcamera/meson.build | 3 ++\n> > .../dma_heaps.cpp => dma_heap.cpp} | 54 ++++++++++++-------\n> > src/libcamera/heap_allocator.cpp | 43 +++++++++++++++\n> > src/libcamera/meson.build | 2 +\n> > .../pipeline/raspberrypi/dma_heaps.h | 32 -----------\n> > .../pipeline/raspberrypi/meson.build | 1 -\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++--\n> > 10 files changed, 164 insertions(+), 58 deletions(-)\n> > create mode 100644 include/libcamera/dma_heap.h\n> > create mode 100644 include/libcamera/heap.h\n> > create mode 100644 include/libcamera/heap_allocator.h\n> > rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp =>\n> dma_heap.cpp} (56%)\n> > create mode 100644 src/libcamera/heap_allocator.cpp\n> > delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> >\n> > diff --git a/include/libcamera/dma_heap.h b/include/libcamera/dma_heap.h\n> > new file mode 100644\n> > index 00000000..96e311d3\n> > --- /dev/null\n> > +++ b/include/libcamera/dma_heap.h\n> > @@ -0,0 +1,20 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * dma_heap.h - Dma Heap implementation.\n> > + */\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class DmaHeap : public Heap\n> > +{\n> > +public:\n> > + DmaHeap();\n> > + ~DmaHeap();\n> > + UniqueFD alloc(const char *name, std::size_t size) override;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h\n> > new file mode 100644\n> > index 00000000..c49a2ac3\n> > --- /dev/null\n> > +++ b/include/libcamera/heap.h\n> > @@ -0,0 +1,27 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * heap.h - Heap interface.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class Heap\n> > +{\n> > +public:\n> > + virtual ~Heap() = default;\n> > + bool isValid() const { return handle_.isValid(); }\n> > + virtual UniqueFD alloc(const char *name, std::size_t size) = 0;\n> > +\n> > +protected:\n> > + UniqueFD handle_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/heap_allocator.h\n> b/include/libcamera/heap_allocator.h\n> > new file mode 100644\n> > index 00000000..cd7ed1a3\n> > --- /dev/null\n> > +++ b/include/libcamera/heap_allocator.h\n> > @@ -0,0 +1,30 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * heap_allocator.h - Helper class for heap buffer allocations.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class HeapAllocator\n> > +{\n> > +public:\n> > + HeapAllocator();\n> > + ~HeapAllocator();\n> > + bool isValid() const { return heap_->isValid(); }\n> > + UniqueFD alloc(const char *name, std::size_t size);\n> > +\n> > +private:\n> > + std::unique_ptr heap_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/meson.build\n> b/include/libcamera/meson.build\n> > index 408b7acf..f486630a 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -7,10 +7,13 @@ libcamera_public_headers = files([\n> > 'camera_manager.h',\n> > 'color_space.h',\n> > 'controls.h',\n> > + 'dma_heap.h',\n> > 'fence.h',\n> > 'framebuffer.h',\n> > 'framebuffer_allocator.h',\n> > 'geometry.h',\n> > + 'heap.h',\n> > + 'heap_allocator.h',\n> > 'logging.h',\n> > 'pixel_format.h',\n> > 'request.h',\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> b/src/libcamera/dma_heap.cpp\n> > similarity index 56%\n> > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > rename to src/libcamera/dma_heap.cpp\n> > index 6b644406..8b67e9d0 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > +++ b/src/libcamera/dma_heap.cpp\n> > @@ -1,19 +1,20 @@\n> > /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > /*\n> > - * Copyright (C) 2020, Raspberry Pi Ltd\n> > + * Copyright (C) 2023, Google Inc.\n>\n> Moving and then editing a file warrants claiming sole copyright?\n> Sounds legally dubious to me.\n>\n> Dave\n>\n> > *\n> > - * dma_heaps.h - Helper class for dma-heap allocations.\n> > + * dma_heap.h - Dma Heap implementation.\n> > */\n> >\n> > -#include \"dma_heaps.h\"\n> > +#include \n> >\n> > #include \n> > #include \n> > -#include \n> > -#include \n> > #include \n> > #include \n> >\n> > +#include \n> > +#include \n> > +\n> > #include \n> >\n> > /*\n> > @@ -30,9 +31,7 @@ static constexpr std::array heapNames\n> = {\n> >\n> > namespace libcamera {\n> >\n> > -LOG_DECLARE_CATEGORY(RPI)\n> > -\n> > -namespace RPi {\n> > +LOG_DEFINE_CATEGORY(DmaHeap)\n> >\n> > DmaHeap::DmaHeap()\n> > {\n> > @@ -40,17 +39,34 @@ DmaHeap::DmaHeap()\n> > int ret = ::open(name, O_RDWR, 0);\n> > if (ret < 0) {\n> > ret = errno;\n> > - LOG(RPI, Debug) << \"Failed to open \" << name <<\n> \": \"\n> > - << strerror(ret);\n> > + LOG(DmaHeap, Debug) << \"Failed to open \" << name\n> << \": \"\n> > + << strerror(ret);\n> > continue;\n> > }\n> >\n> > - dmaHeapHandle_ = UniqueFD(ret);\n> > + handle_ = UniqueFD(ret);\n> > break;\n> > }\n> >\n> > - if (!dmaHeapHandle_.isValid())\n> > - LOG(RPI, Error) << \"Could not open any dmaHeap device\";\n> > + if (!handle_.isValid())\n> > + LOG(DmaHeap, Error) << \"Could not open any dmaHeap\n> device\";\n> > +\n> > + int ret = ::open(\"/dev/udmabuf\", O_RDWR, 0);\n> > + if (ret < 0) {\n> > + ret = errno;\n> > + LOG(DmaHeap, Error)\n> > + << \"Failed to open allocator: \" << strerror(ret);\n> > +\n> > + if (ret == EACCES) {\n> > + LOG(DmaHeap, Info)\n> > + << \"Consider making /dev/udmabuf\n> accessible by the video group\";\n> > + LOG(DmaHeap, Info)\n> > + << \"Alternatively, add your user to the\n> kvm group.\";\n> > + }\n> > +\n> > + } else {\n> > + handle_ = UniqueFD(ret);\n> > + }\n> > }\n> >\n> > DmaHeap::~DmaHeap() = default;\n> > @@ -67,24 +83,22 @@ UniqueFD DmaHeap::alloc(const char *name,\n> std::size_t size)\n> > alloc.len = size;\n> > alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> >\n> > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC,\n> &alloc);\n> > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > if (ret < 0) {\n> > - LOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> > - << name;\n> > + LOG(DmaHeap, Error) << \"dmaHeap allocation failure for \"\n> > + << name;\n> > return {};\n> > }\n> >\n> > UniqueFD allocFd(alloc.fd);\n> > ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);\n> > if (ret < 0) {\n> > - LOG(RPI, Error) << \"dmaHeap naming failure for \"\n> > - << name;\n> > + LOG(DmaHeap, Error) << \"dmaHeap naming failure for \"\n> > + << name;\n> > return {};\n> > }\n> >\n> > return allocFd;\n> > }\n> >\n> > -} /* namespace RPi */\n> > -\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/heap_allocator.cpp\n> b/src/libcamera/heap_allocator.cpp\n> > new file mode 100644\n> > index 00000000..594b1d6a\n> > --- /dev/null\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Google Inc.\n> > + *\n> > + * heap_allocator.cpp - Helper class for heap buffer allocations.\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(HeapAllocator)\n> > +\n> > +HeapAllocator::HeapAllocator()\n> > +{\n> > + heap_ = std::make_unique();\n> > +}\n> > +\n> > +HeapAllocator::~HeapAllocator() = default;\n> > +\n> > +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)\n> > +{\n> > + if (!isValid()) {\n> > + LOG(HeapAllocator, Fatal) << \"Allocation attempted\n> without allocator\" << name;\n> > + return {};\n> > + }\n> > +\n> > + return heap_->alloc(name, size);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 9869bfe7..ee586c0d 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -17,11 +17,13 @@ libcamera_sources = files([\n> > 'delayed_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > + 'dma_heap.cpp',\n> > 'fence.cpp',\n> > 'formats.cpp',\n> > 'framebuffer.cpp',\n> > 'framebuffer_allocator.cpp',\n> > 'geometry.cpp',\n> > + 'heap_allocator.cpp',\n> > 'ipa_controls.cpp',\n> > 'ipa_data_serializer.cpp',\n> > 'ipa_interface.cpp',\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > deleted file mode 100644\n> > index 0a4a8d86..00000000\n> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > +++ /dev/null\n> > @@ -1,32 +0,0 @@\n> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Raspberry Pi Ltd\n> > - *\n> > - * dma_heaps.h - Helper class for dma-heap allocations.\n> > - */\n> > -\n> > -#pragma once\n> > -\n> > -#include \n> > -\n> > -#include \n> > -\n> > -namespace libcamera {\n> > -\n> > -namespace RPi {\n> > -\n> > -class DmaHeap\n> > -{\n> > -public:\n> > - DmaHeap();\n> > - ~DmaHeap();\n> > - bool isValid() const { return dmaHeapHandle_.isValid(); }\n> > - UniqueFD alloc(const char *name, std::size_t size);\n> > -\n> > -private:\n> > - UniqueFD dmaHeapHandle_;\n> > -};\n> > -\n> > -} /* namespace RPi */\n> > -\n> > -} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build\n> b/src/libcamera/pipeline/raspberrypi/meson.build\n> > index 6064a3f0..42b96286 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > @@ -2,7 +2,6 @@\n> >\n> > libcamera_sources += files([\n> > 'delayed_controls.cpp',\n> > - 'dma_heaps.cpp',\n> > 'raspberrypi.cpp',\n> > 'rpi_stream.cpp',\n> > ])\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 8569df17..18325faa 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -20,6 +20,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -41,7 +42,6 @@\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> >\n> > #include \"delayed_controls.h\"\n> > -#include \"dma_heaps.h\"\n> > #include \"rpi_stream.h\"\n> >\n> > using namespace std::chrono_literals;\n> > @@ -240,7 +240,7 @@ public:\n> > std::vector, MediaLink\n> *>> bridgeDevices_;\n> >\n> > /* DMAHEAP allocation helper. */\n> > - RPi::DmaHeap dmaHeap_;\n> > + HeapAllocator heapAllocator_;\n> > SharedFD lsTable_;\n> >\n> > std::unique_ptr delayedCtrls_;\n> > @@ -1206,7 +1206,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> > {\n> > std::unique_ptr data =\n> std::make_unique(this);\n> >\n> > - if (!data->dmaHeap_.isValid())\n> > + if (!data->heapAllocator_.isValid())\n> > return -ENOMEM;\n> >\n> > MediaEntity *unicamImage =\n> unicam->getEntityByName(\"unicam-image\");\n> > @@ -1595,9 +1595,9 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config, ipa::RPi::IPA\n> > /* Always send the user transform to the IPA. */\n> > ipaConfig.transform = static_cast int>(config->transform);\n> >\n> > - /* Allocate the lens shading table via dmaHeap and pass to the\n> IPA. */\n> > + /* Allocate the lens shading table via heapAllocator and pass to\n> the IPA. */\n> > if (!lsTable_.isValid()) {\n> > - lsTable_ = SharedFD(dmaHeap_.alloc(\"ls_grid\",\n> ipa::RPi::MaxLsGridSize));\n> > + lsTable_ = SharedFD(heapAllocator_.alloc(\"ls_grid\",\n> ipa::RPi::MaxLsGridSize));\n> > if (!lsTable_.isValid())\n> > return -ENOMEM;\n> >\n> > --\n> > 2.39.1.519.gcb327c4b5f-goog\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 3772FBEFBE\n\tfor ;\n\tWed, 8 Feb 2023 15:42:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6A0361EEF;\n\tWed, 8 Feb 2023 16:42:51 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675870971;\n\tbh=68Cd9O5UyW5gkqZgox8T8+IrnmBBE8klB37ZG39n44g=;\n\th=References:In-Reply-To:Date:To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=nDWIBFA7sqozZat2IUHVUtoI4/HYXeBDZJVRrXk+M6l6gJXdhzBQo+PTM7uH5eUuM\n\t3xytzwyMVzu3eYvO9D3rzsLtdzh2YBo70a8wAXLoOQ0D/ejhegRSd474A+BH9FLcbg\n\tq6RCWTaB/M6vJTa5i5Z+1CyvTokxhdtllZNq9CaU+dfYF+oPjFH4IR3aJTW+IR/2vD\n\tO8cbLVzRD4fDRr6g84vVStkluljsuDwOeOG+S9xG+JQyhN0zi4Rx7SFCP6qXKkNGgL\n\tqlpq6G32WnvabBmOt+dkKRh8mOnA8huFET9n/xTsGwutXoJYUzwK087jOQwpjzU431\n\tCVc9mLhvxZXHQ==","References":"<20230208095922.1471175-1-chenghaoyang@google.com>\n\t<20230208095922.1471175-2-chenghaoyang@google.com>\n\t","In-Reply-To":"","Date":"Wed, 8 Feb 2023 19:58:50 +0800","To":"Dave Stevenson ","X-Mailman-Approved-At":"Wed, 08 Feb 2023 16:42:49 +0100","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Cheng-Hao Yang via libcamera-devel\n\t","Precedence":"list","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Cheng-Hao Yang ","List-Help":"","Subject":"Re: [libcamera-devel] [PATCH v1 1/3] libcamera: Move DmaHeap to\n\tHeapAllocator as a base class","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]