Message ID | 20240930063342.3014837-2-chenghaoyang@google.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Kieran and Jacopo, On Mon, Sep 30, 2024 at 2:33 PM Harvey Yang <chenghaoyang@chromium.org> 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@chromium.org> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > .../libcamera/internal/dma_buf_allocator.h | 13 +++++ > src/libcamera/dma_buf_allocator.cpp | 57 +++++++++++++++++++ > 2 files changed, 70 insertions(+) > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > index d2a0a0d1..66d3b419 100644 > --- a/include/libcamera/internal/dma_buf_allocator.h > +++ b/include/libcamera/internal/dma_buf_allocator.h > @@ -7,11 +7,17 @@ > > #pragma once > > +#include <memory> > +#include <string> > +#include <vector> > + > #include <libcamera/base/flags.h> > #include <libcamera/base/unique_fd.h> > > namespace libcamera { > > +class FrameBuffer; > + > class DmaBufAllocator > { > public: > @@ -28,7 +34,14 @@ public: > bool isValid() const { return providerHandle_.isValid(); } > UniqueFD alloc(const char *name, std::size_t size); > > + int exportBuffers(unsigned int count, > + const std::vector<unsigned int> &frameSize, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers); > + > private: > + std::unique_ptr<FrameBuffer> createBuffer( > + std::string name, const std::vector<unsigned int> &frameSizes); > + > 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 be6efb89..f8ed3de2 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -22,6 +22,9 @@ > > #include <libcamera/base/log.h> > #include <libcamera/base/memfd.h> > +#include <libcamera/base/shared_fd.h> > + > +#include <libcamera/framebuffer.h> > > /** > * \file dma_buf_allocator.cpp > @@ -205,4 +208,58 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > return allocFromHeap(name, size); > } > > +/** > + * \brief Allocate and export buffers from the DmaBufAllocator > + * \param[in] count The number of requested FrameBuffers > + * \param[in] frameSizes The sizes of planes in each FrameBuffer > + * \param[out] buffers Array of buffers successfully allocated > + * > + * Planes in a FrameBuffer are allocated with a single dma buf. > + * \todo Add the option to allocate each plane with a dma buf respectively. > + * > + * \return The number of allocated buffers on success or a negative error code > + * otherwise > + */ > +int DmaBufAllocator::exportBuffers(unsigned int count, > + const std::vector<unsigned int> &frameSizes, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > +{ > + for (unsigned int i = 0; i < count; ++i) { > + 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; > +} > + > +std::unique_ptr<FrameBuffer> > +DmaBufAllocator::createBuffer(std::string name, > + const std::vector<unsigned int> &frameSizes) > +{ > + std::vector<FrameBuffer::Plane> planes; > + > + unsigned int bufferSize = 0, offset = 0; > + for (auto frameSize : frameSizes) > + bufferSize += frameSize; Maybe it's a bit late: Do you think I should rename `frameSizes` as `planeSizes`, and `bufferSize` as `frameSize`? BR, Harvey > + > + SharedFD fd(alloc(name.c_str(), bufferSize)); > + if (!fd.isValid()) > + return nullptr; > + > + for (auto frameSize : frameSizes) { > + planes.emplace_back(FrameBuffer::Plane{ fd, offset, frameSize }); > + offset += frameSize; > + } > + > + return std::make_unique<FrameBuffer>(planes); > +} > + > } /* namespace libcamera */ > -- > 2.46.1.824.gd892dcdcdd-goog >
Quoting Cheng-Hao Yang (2024-10-03 09:06:28) > Hi Kieran and Jacopo, > > On Mon, Sep 30, 2024 at 2:33 PM Harvey Yang <chenghaoyang@chromium.org> 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@chromium.org> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > .../libcamera/internal/dma_buf_allocator.h | 13 +++++ > > src/libcamera/dma_buf_allocator.cpp | 57 +++++++++++++++++++ > > 2 files changed, 70 insertions(+) > > > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > > index d2a0a0d1..66d3b419 100644 > > --- a/include/libcamera/internal/dma_buf_allocator.h > > +++ b/include/libcamera/internal/dma_buf_allocator.h > > @@ -7,11 +7,17 @@ > > > > #pragma once > > > > +#include <memory> > > +#include <string> > > +#include <vector> > > + > > #include <libcamera/base/flags.h> > > #include <libcamera/base/unique_fd.h> > > > > namespace libcamera { > > > > +class FrameBuffer; > > + > > class DmaBufAllocator > > { > > public: > > @@ -28,7 +34,14 @@ public: > > bool isValid() const { return providerHandle_.isValid(); } > > UniqueFD alloc(const char *name, std::size_t size); > > > > + int exportBuffers(unsigned int count, > > + const std::vector<unsigned int> &frameSize, > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > + > > private: > > + std::unique_ptr<FrameBuffer> createBuffer( > > + std::string name, const std::vector<unsigned int> &frameSizes); > > + > > 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 be6efb89..f8ed3de2 100644 > > --- a/src/libcamera/dma_buf_allocator.cpp > > +++ b/src/libcamera/dma_buf_allocator.cpp > > @@ -22,6 +22,9 @@ > > > > #include <libcamera/base/log.h> > > #include <libcamera/base/memfd.h> > > +#include <libcamera/base/shared_fd.h> > > + > > +#include <libcamera/framebuffer.h> > > > > /** > > * \file dma_buf_allocator.cpp > > @@ -205,4 +208,58 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > > return allocFromHeap(name, size); > > } > > > > +/** > > + * \brief Allocate and export buffers from the DmaBufAllocator > > + * \param[in] count The number of requested FrameBuffers > > + * \param[in] frameSizes The sizes of planes in each FrameBuffer > > + * \param[out] buffers Array of buffers successfully allocated > > + * > > + * Planes in a FrameBuffer are allocated with a single dma buf. > > + * \todo Add the option to allocate each plane with a dma buf respectively. > > + * > > + * \return The number of allocated buffers on success or a negative error code > > + * otherwise > > + */ > > +int DmaBufAllocator::exportBuffers(unsigned int count, > > + const std::vector<unsigned int> &frameSizes, > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > +{ > > + for (unsigned int i = 0; i < count; ++i) { > > + 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; > > +} > > + > > +std::unique_ptr<FrameBuffer> > > +DmaBufAllocator::createBuffer(std::string name, > > + const std::vector<unsigned int> &frameSizes) > > +{ > > + std::vector<FrameBuffer::Plane> planes; > > + > > + unsigned int bufferSize = 0, offset = 0; > > + for (auto frameSize : frameSizes) > > + bufferSize += frameSize; > > Maybe it's a bit late: > Do you think I should rename `frameSizes` as `planeSizes`, > and `bufferSize` as `frameSize`? plane sizes would probably be better, but I don't object either way. I've just checked CI - and I see the unit tests are passing again, so I'll see if I can find some time to re-test this later tonight if no one else beats me to it. This small rename could be handled when applying if this series is close to merge. > > BR, > Harvey > > > + > > + SharedFD fd(alloc(name.c_str(), bufferSize)); > > + if (!fd.isValid()) > > + return nullptr; > > + > > + for (auto frameSize : frameSizes) { > > + planes.emplace_back(FrameBuffer::Plane{ fd, offset, frameSize }); > > + offset += frameSize; > > + } > > + > > + return std::make_unique<FrameBuffer>(planes); > > +} > > + > > } /* namespace libcamera */ > > -- > > 2.46.1.824.gd892dcdcdd-goog > >
diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h index d2a0a0d1..66d3b419 100644 --- a/include/libcamera/internal/dma_buf_allocator.h +++ b/include/libcamera/internal/dma_buf_allocator.h @@ -7,11 +7,17 @@ #pragma once +#include <memory> +#include <string> +#include <vector> + #include <libcamera/base/flags.h> #include <libcamera/base/unique_fd.h> namespace libcamera { +class FrameBuffer; + class DmaBufAllocator { public: @@ -28,7 +34,14 @@ public: bool isValid() const { return providerHandle_.isValid(); } UniqueFD alloc(const char *name, std::size_t size); + int exportBuffers(unsigned int count, + const std::vector<unsigned int> &frameSize, + std::vector<std::unique_ptr<FrameBuffer>> *buffers); + private: + std::unique_ptr<FrameBuffer> createBuffer( + std::string name, const std::vector<unsigned int> &frameSizes); + 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 be6efb89..f8ed3de2 100644 --- a/src/libcamera/dma_buf_allocator.cpp +++ b/src/libcamera/dma_buf_allocator.cpp @@ -22,6 +22,9 @@ #include <libcamera/base/log.h> #include <libcamera/base/memfd.h> +#include <libcamera/base/shared_fd.h> + +#include <libcamera/framebuffer.h> /** * \file dma_buf_allocator.cpp @@ -205,4 +208,58 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) return allocFromHeap(name, size); } +/** + * \brief Allocate and export buffers from the DmaBufAllocator + * \param[in] count The number of requested FrameBuffers + * \param[in] frameSizes The sizes of planes in each FrameBuffer + * \param[out] buffers Array of buffers successfully allocated + * + * Planes in a FrameBuffer are allocated with a single dma buf. + * \todo Add the option to allocate each plane with a dma buf respectively. + * + * \return The number of allocated buffers on success or a negative error code + * otherwise + */ +int DmaBufAllocator::exportBuffers(unsigned int count, + const std::vector<unsigned int> &frameSizes, + std::vector<std::unique_ptr<FrameBuffer>> *buffers) +{ + for (unsigned int i = 0; i < count; ++i) { + 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; +} + +std::unique_ptr<FrameBuffer> +DmaBufAllocator::createBuffer(std::string name, + const std::vector<unsigned int> &frameSizes) +{ + std::vector<FrameBuffer::Plane> planes; + + unsigned int bufferSize = 0, offset = 0; + for (auto frameSize : frameSizes) + bufferSize += frameSize; + + SharedFD fd(alloc(name.c_str(), bufferSize)); + if (!fd.isValid()) + return nullptr; + + for (auto frameSize : frameSizes) { + planes.emplace_back(FrameBuffer::Plane{ fd, offset, frameSize }); + offset += frameSize; + } + + return std::make_unique<FrameBuffer>(planes); +} + } /* namespace libcamera */