| Message ID | 20240820172202.526547-2-chenghaoyang@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + > 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); > + std::size_t count, #include <cstddef> > + std::vector<std::size_t> frameSize, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers); #include <memory> > + > private: > + std::unique_ptr<FrameBuffer> createBuffer( > + std::string name, std::vector<std::size_t> frameSizes); #include <string> > + > 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 > + > +#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 > #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 > + * \param[in] count The number of FrameBuffers required "of requested FrameBuffers" ? > + * \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 > + std::vector<std::size_t> frameSizes, can this be passed as a const reference ? > + 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 !! ) > +{ > + 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 ?) > + 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 > +} > + > +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 ? > +{ > + 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 > + > + 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. > + > + return std::make_unique<FrameBuffer>(planes); > +} > + > } /* namespace libcamera */ > -- > 2.46.0.184.g6999bdac58-goog >
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@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@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 > > >
Hi Jacopo, On Sat, Aug 31, 2024 at 8:39 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Harvey > > On Tue, Aug 27, 2024 at 05:34:43PM GMT, Cheng-Hao Yang wrote: > > 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@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@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 > > Sometimes you replies are indendented at the same level as the > previous email you replied to, making it really hard to distinguish > the two. Could you check why it happens ? > > Ah, thanks for letting me know. I normally wouldn't leave an empty line before my replies, and if the reply line above is an empty line, this issue happens. I'll leave one extra line above every reply from now on. > > > > > > > + 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. > > Never assume how an API could be wrongly used :) > > > > > We can't stop users allocating too many buffers with or without this > > helper function, right :p ? > > > > True indeed, but a good API should minimize the surface for making it > used wrongly, even more when it gives access to system resource. > > See below. > > > > > > > > > > + 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. > > > > I think it's fine > > > > > > > > > > +} > > > > + > > > > +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. > > > > As long as they don't :) > > I'm particularly concerned about udmabuf, as it uses memfd as backing > storage, and we allow to allocate a lof of memory (we did already to > be completely honest, as a user can call alloc with any size). > > Also, I've now noticed the dma_buf_allocator API are not public, so > this makes me way less concerned. > Yes, pipeline handlers' developers should be more careful than the applications'. So let's say we keep the API as is for now, until there are more guidelines on FrameBuffer's limitation? > > > > > > > > > > + > > > > + 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 > > > > > > > >
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; + 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( + std::size_t count, + std::vector<std::size_t> frameSize, + std::vector<std::unique_ptr<FrameBuffer>> *buffers); + private: + std::unique_ptr<FrameBuffer> createBuffer( + std::string name, std::vector<std::size_t> 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 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> + +#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 #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 + * \param[in] count The number of FrameBuffers required + * \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, + std::vector<std::size_t> frameSizes, + std::vector<std::unique_ptr<FrameBuffer>> *buffers) +{ + for (unsigned 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, std::vector<std::size_t> frameSizes) +{ + std::vector<FrameBuffer::Plane> planes; + + std::size_t 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) { + FrameBuffer::Plane plane; + plane.fd = fd; + plane.offset = offset; + plane.length = frameSize; + planes.push_back(std::move(plane)); + offset += plane.length; + } + + return std::make_unique<FrameBuffer>(planes); +} + } /* namespace libcamera */
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> --- .../libcamera/internal/dma_buf_allocator.h | 12 ++++ src/libcamera/dma_buf_allocator.cpp | 64 ++++++++++++++++++- 2 files changed, 74 insertions(+), 2 deletions(-)