[libcamera-devel] [PATCH v3 1/3] libcamera: Move DmaHeap to HeapAllocator as a base class
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Mar 17 22:48:38 CET 2023
Quoting Cheng-Hao Yang (2023-03-16 15:50:29)
> Thanks Kieran for the input. I found it difficult to enable dma/udma in
> Google's linux distribution though, so I tested it in my personal Arch
> linux instead (udmabuf is available).
>
> However, I found that udmabuf only works if the requested size is power of
> 2 (ex: 4194304 for 1920*1080). Is it expected, or there's something wrong
> with my linux udmabuf support?
I would expect that it can only allocate a multiple of the PAGE_SIZE
configured on the Kernel.
You could investigate that and decide to round up to the next PAGE_SIZE,
or if the kernel does this, just let the allocation succeed as long as
it is equal or larger than the requested size?
--
Kieran
>
> BR,
> Harvey
>
> On Thu, Mar 2, 2023 at 8:01 PM Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
>
> > Quoting Cheng-Hao Yang (2023-03-02 08:25:00)
> > > Hi Kieran,
> > >
> > > Sorry that I'm new to dma heap: I was trying to test virtual pipeline
> > > handler on dma heap
> > > (or the udma heap I add in the following patch), while I find that my
> > > workstation doesn't
> > > have the dma heap directory (`/dev/dma_heap` or `/dev/udmabuf`). I heard
> > > from Han-lin
> > > that we might need to enable it in the linux kernel. Is that right? Is
> > > there a doc/tutorial that
> > > I can follow?
> >
> > It could be kernel specific indeed. I guess it's up to your
> > distribution?
> >
> > UDMABUF
> > -
> > https://github.com/torvalds/linux/blob/ee3f96b164688dae21e2466a57f2e806b64e8a37/drivers/dma-buf/Kconfig#L33
> >
> > DMABUF_HEAPS
> > -
> > https://github.com/torvalds/linux/blob/ee3f96b164688dae21e2466a57f2e806b64e8a37/drivers/dma-buf/Kconfig#L68
> >
> >
> > --
> > Regards
> >
> > Kieran
> >
> >
> >
> > >
> > > Thanks!
> > > Harvey
> > >
> > >
> > >
> > > On Thu, Mar 2, 2023 at 4:13 PM Harvey Yang <chenghaoyang at chromium.org>
> > > wrote:
> > >
> > > > As other components (like the WIP virtual pipeline handler) also need a
> > > > heap allocator, move DmaHeap from raspberry pi pipeline handler to a
> > > > general HeapAllocator as a base class.
> > > >
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > ---
> > > > include/libcamera/dma_heap.h | 20 +++++++++
> > > > include/libcamera/heap.h | 27 ++++++++++++
> > > > include/libcamera/heap_allocator.h | 30 +++++++++++++
> > > > include/libcamera/meson.build | 3 ++
> > > > .../dma_heaps.cpp => dma_heap.cpp} | 35 +++++++--------
> > > > src/libcamera/heap_allocator.cpp | 43 +++++++++++++++++++
> > > > src/libcamera/meson.build | 2 +
> > > > .../pipeline/raspberrypi/dma_heaps.h | 32 --------------
> > > > .../pipeline/raspberrypi/meson.build | 1 -
> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 10 ++---
> > > > 10 files changed, 146 insertions(+), 57 deletions(-)
> > > > create mode 100644 include/libcamera/dma_heap.h
> > > > create mode 100644 include/libcamera/heap.h
> > > > create mode 100644 include/libcamera/heap_allocator.h
> > > > rename src/libcamera/{pipeline/raspberrypi/dma_heaps.cpp =>
> > dma_heap.cpp}
> > > > (69%)
> > > > create mode 100644 src/libcamera/heap_allocator.cpp
> > > > delete mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h
> > > >
> > > > diff --git a/include/libcamera/dma_heap.h
> > b/include/libcamera/dma_heap.h
> > > > new file mode 100644
> > > > index 00000000..a663c317
> > > > --- /dev/null
> > > > +++ b/include/libcamera/dma_heap.h
> > > > @@ -0,0 +1,20 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2020, Raspberry Pi Ltd
> > > > + *
> > > > + * dma_heap.h - Dma Heap implementation.
> > > > + */
> > > > +
> > > > +#include <libcamera/heap.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +class DmaHeap : public Heap
> > > > +{
> > > > +public:
> > > > + DmaHeap();
> > > > + ~DmaHeap();
> > > > + UniqueFD alloc(const char *name, std::size_t size) override;
> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/include/libcamera/heap.h b/include/libcamera/heap.h
> > > > new file mode 100644
> > > > index 00000000..c49a2ac3
> > > > --- /dev/null
> > > > +++ b/include/libcamera/heap.h
> > > > @@ -0,0 +1,27 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2023, Google Inc.
> > > > + *
> > > > + * heap.h - Heap interface.
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <stddef.h>
> > > > +
> > > > +#include <libcamera/base/unique_fd.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +class Heap
> > > > +{
> > > > +public:
> > > > + virtual ~Heap() = default;
> > > > + bool isValid() const { return handle_.isValid(); }
> > > > + virtual UniqueFD alloc(const char *name, std::size_t size) = 0;
> > > > +
> > > > +protected:
> > > > + UniqueFD handle_;
> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/include/libcamera/heap_allocator.h
> > > > b/include/libcamera/heap_allocator.h
> > > > new file mode 100644
> > > > index 00000000..cd7ed1a3
> > > > --- /dev/null
> > > > +++ b/include/libcamera/heap_allocator.h
> > > > @@ -0,0 +1,30 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2023, Google Inc.
> > > > + *
> > > > + * heap_allocator.h - Helper class for heap buffer allocations.
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <stddef.h>
> > > > +
> > > > +#include <libcamera/base/unique_fd.h>
> > > > +
> > > > +#include <libcamera/heap.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +class HeapAllocator
> > > > +{
> > > > +public:
> > > > + HeapAllocator();
> > > > + ~HeapAllocator();
> > > > + bool isValid() const { return heap_->isValid(); }
> > > > + UniqueFD alloc(const char *name, std::size_t size);
> > > > +
> > > > +private:
> > > > + std::unique_ptr<Heap> heap_;
> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/include/libcamera/meson.build
> > b/include/libcamera/meson.build
> > > > index 408b7acf..f486630a 100644
> > > > --- a/include/libcamera/meson.build
> > > > +++ b/include/libcamera/meson.build
> > > > @@ -7,10 +7,13 @@ libcamera_public_headers = files([
> > > > 'camera_manager.h',
> > > > 'color_space.h',
> > > > 'controls.h',
> > > > + 'dma_heap.h',
> > > > 'fence.h',
> > > > 'framebuffer.h',
> > > > 'framebuffer_allocator.h',
> > > > 'geometry.h',
> > > > + 'heap.h',
> > > > + 'heap_allocator.h',
> > > > 'logging.h',
> > > > 'pixel_format.h',
> > > > 'request.h',
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > > > b/src/libcamera/dma_heap.cpp
> > > > similarity index 69%
> > > > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > > > rename to src/libcamera/dma_heap.cpp
> > > > index 6b644406..02415975 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > > > +++ b/src/libcamera/dma_heap.cpp
> > > > @@ -2,18 +2,19 @@
> > > > /*
> > > > * Copyright (C) 2020, Raspberry Pi Ltd
> > > > *
> > > > - * dma_heaps.h - Helper class for dma-heap allocations.
> > > > + * dma_heap.h - Dma Heap implementation.
> > > > */
> > > >
> > > > -#include "dma_heaps.h"
> > > > +#include <libcamera/dma_heap.h>
> > > >
> > > > #include <array>
> > > > #include <fcntl.h>
> > > > -#include <linux/dma-buf.h>
> > > > -#include <linux/dma-heap.h>
> > > > #include <sys/ioctl.h>
> > > > #include <unistd.h>
> > > >
> > > > +#include <linux/dma-buf.h>
> > > > +#include <linux/dma-heap.h>
> > > > +
> > > > #include <libcamera/base/log.h>
> > > >
> > > > /*
> > > > @@ -30,9 +31,7 @@ static constexpr std::array<const char *, 2>
> > heapNames =
> > > > {
> > > >
> > > > namespace libcamera {
> > > >
> > > > -LOG_DECLARE_CATEGORY(RPI)
> > > > -
> > > > -namespace RPi {
> > > > +LOG_DEFINE_CATEGORY(DmaHeap)
> > > >
> > > > DmaHeap::DmaHeap()
> > > > {
> > > > @@ -40,17 +39,17 @@ DmaHeap::DmaHeap()
> > > > int ret = ::open(name, O_RDWR, 0);
> > > > if (ret < 0) {
> > > > ret = errno;
> > > > - LOG(RPI, Debug) << "Failed to open " << name
> > << ":
> > > > "
> > > > - << strerror(ret);
> > > > + LOG(DmaHeap, Debug) << "Failed to open " <<
> > name
> > > > << ": "
> > > > + << strerror(ret);
> > > > continue;
> > > > }
> > > >
> > > > - dmaHeapHandle_ = UniqueFD(ret);
> > > > + handle_ = UniqueFD(ret);
> > > > break;
> > > > }
> > > >
> > > > - if (!dmaHeapHandle_.isValid())
> > > > - LOG(RPI, Error) << "Could not open any dmaHeap device";
> > > > + if (!handle_.isValid())
> > > > + LOG(DmaHeap, Error) << "Could not open any dmaHeap
> > device";
> > > > }
> > > >
> > > > DmaHeap::~DmaHeap() = default;
> > > > @@ -67,24 +66,22 @@ UniqueFD DmaHeap::alloc(const char *name,
> > std::size_t
> > > > size)
> > > > alloc.len = size;
> > > > alloc.fd_flags = O_CLOEXEC | O_RDWR;
> > > >
> > > > - ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC,
> > &alloc);
> > > > + ret = ::ioctl(handle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);
> > > > if (ret < 0) {
> > > > - LOG(RPI, Error) << "dmaHeap allocation failure for "
> > > > - << name;
> > > > + LOG(DmaHeap, Error) << "dmaHeap allocation failure for
> > "
> > > > + << name;
> > > > return {};
> > > > }
> > > >
> > > > UniqueFD allocFd(alloc.fd);
> > > > ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);
> > > > if (ret < 0) {
> > > > - LOG(RPI, Error) << "dmaHeap naming failure for "
> > > > - << name;
> > > > + LOG(DmaHeap, Error) << "dmaHeap naming failure for "
> > > > + << name;
> > > > return {};
> > > > }
> > > >
> > > > return allocFd;
> > > > }
> > > >
> > > > -} /* namespace RPi */
> > > > -
> > > > } /* namespace libcamera */
> > > > diff --git a/src/libcamera/heap_allocator.cpp
> > > > b/src/libcamera/heap_allocator.cpp
> > > > new file mode 100644
> > > > index 00000000..594b1d6a
> > > > --- /dev/null
> > > > +++ b/src/libcamera/heap_allocator.cpp
> > > > @@ -0,0 +1,43 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2023, Google Inc.
> > > > + *
> > > > + * heap_allocator.cpp - Helper class for heap buffer allocations.
> > > > + */
> > > > +
> > > > +#include <libcamera/heap_allocator.h>
> > > > +
> > > > +#include <array>
> > > > +#include <fcntl.h>
> > > > +#include <sys/ioctl.h>
> > > > +#include <unistd.h>
> > > > +
> > > > +#include <linux/dma-buf.h>
> > > > +#include <linux/dma-heap.h>
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +#include <libcamera/dma_heap.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +LOG_DEFINE_CATEGORY(HeapAllocator)
> > > > +
> > > > +HeapAllocator::HeapAllocator()
> > > > +{
> > > > + heap_ = std::make_unique<DmaHeap>();
> > > > +}
> > > > +
> > > > +HeapAllocator::~HeapAllocator() = default;
> > > > +
> > > > +UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)
> > > > +{
> > > > + if (!isValid()) {
> > > > + LOG(HeapAllocator, Fatal) << "Allocation attempted
> > without
> > > > allocator" << name;
> > > > + return {};
> > > > + }
> > > > +
> > > > + return heap_->alloc(name, size);
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index 9869bfe7..ee586c0d 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -17,11 +17,13 @@ libcamera_sources = files([
> > > > 'delayed_controls.cpp',
> > > > 'device_enumerator.cpp',
> > > > 'device_enumerator_sysfs.cpp',
> > > > + 'dma_heap.cpp',
> > > > 'fence.cpp',
> > > > 'formats.cpp',
> > > > 'framebuffer.cpp',
> > > > 'framebuffer_allocator.cpp',
> > > > 'geometry.cpp',
> > > > + 'heap_allocator.cpp',
> > > > 'ipa_controls.cpp',
> > > > 'ipa_data_serializer.cpp',
> > > > 'ipa_interface.cpp',
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h
> > > > b/src/libcamera/pipeline/raspberrypi/dma_heaps.h
> > > > deleted file mode 100644
> > > > index 0a4a8d86..00000000
> > > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h
> > > > +++ /dev/null
> > > > @@ -1,32 +0,0 @@
> > > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > -/*
> > > > - * Copyright (C) 2020, Raspberry Pi Ltd
> > > > - *
> > > > - * dma_heaps.h - Helper class for dma-heap allocations.
> > > > - */
> > > > -
> > > > -#pragma once
> > > > -
> > > > -#include <stddef.h>
> > > > -
> > > > -#include <libcamera/base/unique_fd.h>
> > > > -
> > > > -namespace libcamera {
> > > > -
> > > > -namespace RPi {
> > > > -
> > > > -class DmaHeap
> > > > -{
> > > > -public:
> > > > - DmaHeap();
> > > > - ~DmaHeap();
> > > > - bool isValid() const { return dmaHeapHandle_.isValid(); }
> > > > - UniqueFD alloc(const char *name, std::size_t size);
> > > > -
> > > > -private:
> > > > - UniqueFD dmaHeapHandle_;
> > > > -};
> > > > -
> > > > -} /* namespace RPi */
> > > > -
> > > > -} /* namespace libcamera */
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build
> > > > b/src/libcamera/pipeline/raspberrypi/meson.build
> > > > index 59e8fb18..7322f0e8 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/meson.build
> > > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build
> > > > @@ -2,7 +2,6 @@
> > > >
> > > > libcamera_sources += files([
> > > > 'delayed_controls.cpp',
> > > > - 'dma_heaps.cpp',
> > > > 'raspberrypi.cpp',
> > > > 'rpi_stream.cpp',
> > > > ])
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index 84120954..b7e0d031 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -21,6 +21,7 @@
> > > > #include <libcamera/camera.h>
> > > > #include <libcamera/control_ids.h>
> > > > #include <libcamera/formats.h>
> > > > +#include <libcamera/heap_allocator.h>
> > > > #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> > > > #include <libcamera/ipa/raspberrypi_ipa_proxy.h>
> > > > #include <libcamera/logging.h>
> > > > @@ -44,7 +45,6 @@
> > > > #include "libcamera/internal/yaml_parser.h"
> > > >
> > > > #include "delayed_controls.h"
> > > > -#include "dma_heaps.h"
> > > > #include "rpi_stream.h"
> > > >
> > > > using namespace std::chrono_literals;
> > > > @@ -245,7 +245,7 @@ public:
> > > > std::vector<std::pair<std::unique_ptr<V4L2Subdevice>, MediaLink
> > > > *>> bridgeDevices_;
> > > >
> > > > /* DMAHEAP allocation helper. */
> > > > - RPi::DmaHeap dmaHeap_;
> > > > + HeapAllocator heapAllocator_;
> > > > SharedFD lsTable_;
> > > >
> > > > std::unique_ptr<RPi::DelayedControls> delayedCtrls_;
> > > > @@ -1293,7 +1293,7 @@ int
> > PipelineHandlerRPi::registerCamera(MediaDevice
> > > > *unicam, MediaDevice *isp, Me
> > > > {
> > > > std::unique_ptr<RPiCameraData> data =
> > > > std::make_unique<RPiCameraData>(this);
> > > >
> > > > - if (!data->dmaHeap_.isValid())
> > > > + if (!data->heapAllocator_.isValid())
> > > > return -ENOMEM;
> > > >
> > > > MediaEntity *unicamImage =
> > unicam->getEntityByName("unicam-image");
> > > > @@ -1680,9 +1680,9 @@ int RPiCameraData::configureIPA(const
> > > > CameraConfiguration *config, ipa::RPi::IPA
> > > > /* Always send the user transform to the IPA. */
> > > > ipaConfig.transform = static_cast<unsigned
> > int>(config->transform);
> > > >
> > > > - /* Allocate the lens shading table via dmaHeap and pass to the
> > > > IPA. */
> > > > + /* Allocate the lens shading table via heapAllocator and pass
> > to
> > > > the IPA. */
> > > > if (!lsTable_.isValid()) {
> > > > - lsTable_ = SharedFD(dmaHeap_.alloc("ls_grid",
> > > > ipa::RPi::MaxLsGridSize));
> > > > + lsTable_ = SharedFD(heapAllocator_.alloc("ls_grid",
> > > > ipa::RPi::MaxLsGridSize));
> > > > if (!lsTable_.isValid())
> > > > return -ENOMEM;
> > > >
> > > > --
> > > > 2.39.2.722.g9855ee24e9-goog
> > > >
> > > >
> >
More information about the libcamera-devel
mailing list