[PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Aug 27 17:34:43 CEST 2024
Thanks Jacopo for the review.
The corresponding updates are included in v9.1. Please check.
On Tue, Aug 27, 2024 at 4:13 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> Hi Harvey
>
> On Tue, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:
> > Add a helper function exportBuffers in DmaBufAllocator to make it easier
> > to use.
> >
> > It'll be used in Virtual Pipeline Handler and SoftwareIsp.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> > .../libcamera/internal/dma_buf_allocator.h | 12 ++++
> > src/libcamera/dma_buf_allocator.cpp | 64 ++++++++++++++++++-
> > 2 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/internal/dma_buf_allocator.h
> b/include/libcamera/internal/dma_buf_allocator.h
> > index 36ec1696b..3a9b56b1c 100644
> > --- a/include/libcamera/internal/dma_buf_allocator.h
> > +++ b/include/libcamera/internal/dma_buf_allocator.h
> > @@ -8,12 +8,16 @@
> > #pragma once
> >
> > #include <stddef.h>
> > +#include <vector>
> >
> > #include <libcamera/base/flags.h>
> > #include <libcamera/base/unique_fd.h>
> >
> > namespace libcamera {
> >
> > +class FrameBuffer;
> > +struct StreamConfiguration;
>
> Where do you use this ?
>
>
Right, it's only used in the previous versions. Removed.
> > +
> > class DmaBufAllocator
> > {
> > public:
> > @@ -30,7 +34,15 @@ public:
> > bool isValid() const { return providerHandle_.isValid(); }
> > UniqueFD alloc(const char *name, std::size_t size);
> >
> > + int exportBuffers(
>
> weird indent
>
> int exportBuffers(std::size_t count,
> const std::vector<std::size_t> &frameSize,
> std::vector<std::unique_ptr<FrameBuffer>>
> *buffers);
>
> Done
> > + std::size_t count,
>
> #include <cstddef>
>
Removed std::size_t.
>
> > + std::vector<std::size_t> frameSize,
> > + std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> #include <memory>
>
> Done
> > +
> > private:
> > + std::unique_ptr<FrameBuffer> createBuffer(
> > + std::string name, std::vector<std::size_t> frameSizes);
>
> #include <string>
>
Done
> > +
> > UniqueFD allocFromHeap(const char *name, std::size_t size);
> > UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
> > UniqueFD providerHandle_;
> > diff --git a/src/libcamera/dma_buf_allocator.cpp
> b/src/libcamera/dma_buf_allocator.cpp
> > index c06eca7d0..2d88b8b2b 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -23,6 +23,11 @@
> >
> > #include <libcamera/base/log.h>
> >
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/stream.h>
>
> Not used
>
Removed
>
> > +
> > +#include "libcamera/internal/formats.h"
> > +
> > /**
> > * \file dma_buf_allocator.cpp
> > * \brief dma-buf allocator
> > @@ -130,8 +135,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;
> > /* uClibc doesn't provide the file sealing API. */
> > #ifndef __DOXYGEN__
> > #if not HAVE_FILE_SEALS
> > -#define F_ADD_SEALS 1033
> > -#define F_SEAL_SHRINK 0x0002
> > +#define F_ADD_SEALS 1033
> > +#define F_SEAL_SHRINK 0x0002
>
> Unrelated
>
> Removed the changes
> > #endif
> > #endif
> >
> > @@ -243,4 +248,59 @@ UniqueFD DmaBufAllocator::alloc(const char *name,
> std::size_t size)
> > return allocFromHeap(name, size);
> > }
> >
> > +/**
> > + * \brief Allocate and export buffers for \a stream from the
> DmaBufAllocator
>
> There's no \a stream
>
Removed.
>
> > + * \param[in] count The number of FrameBuffers required
>
> "of requested FrameBuffers" ?
>
Thanks! Adopted.
>
> > + * \param[in] frameSizes The sizes of planes in the FrameBuffer
> > + * \param[out] buffers Array of buffers successfully allocated
> > + *
> > + * \return The number of allocated buffers on success or a negative
> error code
> > + * otherwise
> > + */
> > +int DmaBufAllocator::exportBuffers(
> > + std::size_t count,
>
> nit: This can be an unsigned int maybe
>
Done
>
> > + std::vector<std::size_t> frameSizes,
>
> can this be passed as a const reference ?
>
Done
>
> > + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>
> weird indentation, I think
>
> int DmaBufAllocator::exportBuffers(std::size_t count,
> const std::vector<std::size_t>
> &frameSizes,
>
> std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>
> would do (some heretics consider 120 cols to be ok for libcamera, and
> nowadays it is accepted for linux as well !! )
>
Thanks! Adopted.
>
> > +{
> > + for (unsigned i = 0; i < count; ++i) {
>
> I wonder if we should be stricter when defining this interface: is
> there a maximum number of buffers that can be allocated ? How many
> planes can a buffer have ? (I don't see this addressed in our
> FrameBuffer class, so it can be left out from here too ?)
>
Hmm, as it's just a helper function, I think the users should know what
they're doing, unless we have more specific rules, like the maximum
number of planes in FrameBuffer.
We can't stop users allocating too many buffers with or without this
helper function, right :p ?
>
> > + std::unique_ptr<FrameBuffer> buffer =
> > + createBuffer("frame-" + std::to_string(i),
> frameSizes);
> > + if (!buffer) {
> > + LOG(DmaBufAllocator, Error) << "Unable to create
> buffer";
> > +
> > + buffers->clear();
> > + return -EINVAL;
> > + }
> > +
> > + buffers->push_back(std::move(buffer));
> > + }
> > +
> > + return count;
>
> if you want to have all the requested buffers to be allocated, and do
> not allow less than that, there's no point in returning count here, you
> can return 0
>
Hmm, you're right, while I wonder if it's the same case for
V4L2VideoDevice::exportBuffers/createBuffers? I want to align with
other exportBuffers API.
>
> > +}
> > +
> > +std::unique_ptr<FrameBuffer> DmaBufAllocator::createBuffer(
> > + std::string name, std::vector<std::size_t> frameSizes)
>
> Or
>
> std::unique_ptr<FrameBuffer>
> DmaBufAllocator::createBuffer(std::string name,
> const std::vector<std::size_t> &frameSizes)
>
> can frameSize be passed as a const reference ?
>
Thanks! Adopted.
>
>
> > +{
> > + std::vector<FrameBuffer::Plane> planes;
> > +
> > + std::size_t bufferSize = 0, offset = 0;
> > + for (auto frameSize : frameSizes)
> > + bufferSize += frameSize;
>
> This API allows unbounded size allocation, just sayin', maybe it's
> fine
>
Yeah... as it's just a helper function, I still think that the users should
know what they're doing.
>
> > +
> > + SharedFD fd(alloc(name.c_str(), bufferSize));
> > + if (!fd.isValid())
> > + return nullptr;
> > +
> > + for (auto frameSize : frameSizes) {
> > + FrameBuffer::Plane plane;
> > + plane.fd = fd;
> > + plane.offset = offset;
> > + plane.length = frameSize;
> > + planes.push_back(std::move(plane));
> > + offset += plane.length;
> > + }
>
> With a little implicit type casting, you can
>
> unsigned int offset = 0;
> for (unsigned int frameSize : frameSizes) {
> planes.emplace_back(FrameBuffer::Plane{fd, offset,
> frameSize});
> offset += frameSize;
> }
>
> But it won't save you going a temporary object I'm afraid. I'm not
> sure it's actually any better, up to you.
>
> Thanks, adopted. Also, as the offset's and length's types are both
unsigned int, I changed `frameSize` to be an array of unsigned as
well.
> > +
> > + return std::make_unique<FrameBuffer>(planes);
> > +}
> > +
> > } /* namespace libcamera */
> > --
> > 2.46.0.184.g6999bdac58-goog
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240827/2fe37f26/attachment.htm>
More information about the libcamera-devel
mailing list