| Message ID | 20230328095534.3584-4-harveyycyang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel wrote: > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > Add a helper function exportFrameBuffers in HeapAllocator to make it > easier to use. Will you use these with the Virtual pipeline handler ? > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > include/libcamera/heap_allocator.h | 11 ++++++ > src/libcamera/heap_allocator.cpp | 56 ++++++++++++++++++++++++++++++ > 2 files changed, 67 insertions(+) > > diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h > index cd7ed1a3..076f0951 100644 > --- a/include/libcamera/heap_allocator.h > +++ b/include/libcamera/heap_allocator.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <stddef.h> > +#include <vector> > > #include <libcamera/base/unique_fd.h> > > @@ -15,6 +16,11 @@ > > namespace libcamera { > > +class Camera; > +class Stream; > +class FrameBuffer; > +class PixelFormatInfo; Not used > + > class HeapAllocator > { > public: > @@ -23,7 +29,12 @@ public: > bool isValid() const { return heap_->isValid(); } > UniqueFD alloc(const char *name, std::size_t size); > > + int exportFrameBuffers(Camera *camera, Stream *stream, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers); > + > private: > + std::unique_ptr<FrameBuffer> createBuffer(Stream *stream); > + > std::unique_ptr<Heap> heap_; > }; > > diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp > index 179c2c21..33e9eaca 100644 > --- a/src/libcamera/heap_allocator.cpp > +++ b/src/libcamera/heap_allocator.cpp > @@ -17,7 +17,11 @@ > > #include <libcamera/base/log.h> > > +#include <libcamera/camera.h> > #include <libcamera/dma_heap.h> > +#include <libcamera/framebuffer.h> > +#include <libcamera/internal/formats.h> > +#include <libcamera/stream.h> > #include <libcamera/udma_heap.h> > > namespace libcamera { > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name, std::size_t size) > return heap_->alloc(name, size); > } > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > +{ > + unsigned int count = stream->configuration().bufferCount; > + > + /** \todo: Support multiplanar allocations */ no : after \todo > + > + for (unsigned i = 0; i < count; ++i) { > + std::unique_ptr<FrameBuffer> buffer = createBuffer(stream); > + if (!buffer) { > + LOG(HeapAllocator, Error) << "Unable to create buffer"; > + > + buffers->clear(); > + return -EINVAL; > + } > + > + buffers->push_back(std::move(buffer)); > + } > + > + return count; > +} > + > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream) > +{ > + std::vector<FrameBuffer::Plane> planes; > + > + int num_page = (stream->configuration().frameSize + getpagesize() - 1) / getpagesize(); Could you break this rather long line ? Can num_page be unsigned ? libcamera uses camelCase and not snake_case for variables unsigned int numPage = (stream->configuration().frameSize + getpagesize() - 1) / getpagesize(); What are the implications of overallocating ? The last plane will be slightly larger, is this an issue at all ? > + > + UniqueFD fd = alloc("Buffer", num_page * getpagesize()); What are the implications of using the same name ? > + if (!fd.isValid()) > + return nullptr; > + > + auto info = PixelFormatInfo::info(stream->configuration().pixelFormat); > + SharedFD shared_fd(std::move(fd)); > + unsigned int offset = 0; > + for (long unsigned int i = 0; i < info.planes.size(); ++i) { Does this need to be long ? > + unsigned int planeSize = info.planeSize(stream->configuration().size, i); configuration().size is the size in pixel, is this intended ? > + if (planeSize == 0) > + continue; > + > + FrameBuffer::Plane plane; I would note down a \todo to remind that we don't support allocating buffers with a dedicated fd per plane Sorry, lot of questions :) Cheers j > + plane.fd = shared_fd; > + plane.offset = offset; > + plane.length = planeSize; > + planes.push_back(std::move(plane)); > + > + offset += planeSize; > + } > + > + return std::make_unique<FrameBuffer>(planes); > +} > + > } /* namespace libcamera */ > -- > 2.40.0 >
Thanks Jacopo! On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Harvey > > On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel > wrote: > > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > > > Add a helper function exportFrameBuffers in HeapAllocator to make it > > easier to use. > > Will you use these with the Virtual pipeline handler ? > > Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|. Ref: https://patchwork.libcamera.org/patch/18533/ > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > include/libcamera/heap_allocator.h | 11 ++++++ > > src/libcamera/heap_allocator.cpp | 56 ++++++++++++++++++++++++++++++ > > 2 files changed, 67 insertions(+) > > > > diff --git a/include/libcamera/heap_allocator.h > b/include/libcamera/heap_allocator.h > > index cd7ed1a3..076f0951 100644 > > --- a/include/libcamera/heap_allocator.h > > +++ b/include/libcamera/heap_allocator.h > > @@ -8,6 +8,7 @@ > > #pragma once > > > > #include <stddef.h> > > +#include <vector> > > > > #include <libcamera/base/unique_fd.h> > > > > @@ -15,6 +16,11 @@ > > > > namespace libcamera { > > > > +class Camera; > > +class Stream; > > +class FrameBuffer; > > +class PixelFormatInfo; > > Not used > > Removed. > > + > > class HeapAllocator > > { > > public: > > @@ -23,7 +29,12 @@ public: > > bool isValid() const { return heap_->isValid(); } > > UniqueFD alloc(const char *name, std::size_t size); > > > > + int exportFrameBuffers(Camera *camera, Stream *stream, > > + std::vector<std::unique_ptr<FrameBuffer>> > *buffers); > > + > > private: > > + std::unique_ptr<FrameBuffer> createBuffer(Stream *stream); > > + > > std::unique_ptr<Heap> heap_; > > }; > > > > diff --git a/src/libcamera/heap_allocator.cpp > b/src/libcamera/heap_allocator.cpp > > index 179c2c21..33e9eaca 100644 > > --- a/src/libcamera/heap_allocator.cpp > > +++ b/src/libcamera/heap_allocator.cpp > > @@ -17,7 +17,11 @@ > > > > #include <libcamera/base/log.h> > > > > +#include <libcamera/camera.h> > > #include <libcamera/dma_heap.h> > > +#include <libcamera/framebuffer.h> > > +#include <libcamera/internal/formats.h> > > +#include <libcamera/stream.h> > > #include <libcamera/udma_heap.h> > > > > namespace libcamera { > > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name, > std::size_t size) > > return heap_->alloc(name, size); > > } > > > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera, > Stream *stream, > > + > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > +{ > > + unsigned int count = stream->configuration().bufferCount; > > + > > + /** \todo: Support multiplanar allocations */ > > no : after \todo > > Done > + > > + for (unsigned i = 0; i < count; ++i) { > > + std::unique_ptr<FrameBuffer> buffer = createBuffer(stream); > > + if (!buffer) { > > + LOG(HeapAllocator, Error) << "Unable to create > buffer"; > > + > > + buffers->clear(); > > + return -EINVAL; > > + } > > + > > + buffers->push_back(std::move(buffer)); > > + } > > + > > + return count; > > +} > > + > > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream) > > +{ > > + std::vector<FrameBuffer::Plane> planes; > > + > > + int num_page = (stream->configuration().frameSize + getpagesize() > - 1) / getpagesize(); > > Could you break this rather long line ? > Defined "frameSize" and "pageSize" beforehand. Should be shorter now. > Can num_page be unsigned ? > Sure. > libcamera uses camelCase and not snake_case for variables > > unsigned int numPage = (stream->configuration().frameSize + > getpagesize() - 1) > / getpagesize(); > > What are the implications of overallocating ? The last plane will be > slightly larger, is this an issue at all ? > > The allocation only works if the requested size is a direct multiple of |getpagesize()|. Added a comment. Ref: https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72 > > + > > + UniqueFD fd = alloc("Buffer", num_page * getpagesize()); > > What are the implications of using the same name ? > > Since it's a private function that is only called by |exportFrameBuffer|, renamed it to "FrameBuffer" instead. > > + if (!fd.isValid()) > > + return nullptr; > > + > > + auto info = > PixelFormatInfo::info(stream->configuration().pixelFormat); > > + SharedFD shared_fd(std::move(fd)); > > + unsigned int offset = 0; > > + for (long unsigned int i = 0; i < info.planes.size(); ++i) { > > Does this need to be long ? > > I should use size_t instead. > > + unsigned int planeSize = > info.planeSize(stream->configuration().size, i); > > configuration().size is the size in pixel, is this intended ? > > Yes, I guess so, according to the comment of PixelFormatInfo: https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056 > > + if (planeSize == 0) > > + continue; > > + > > + FrameBuffer::Plane plane; > > I would note down a \todo to remind that we don't support allocating > buffers with a dedicated fd per plane > > Is it preferred to do so? I guess we can also support that by calling |alloc()| multiple times...? > Sorry, lot of questions :) > > Thanks for doing so :) > Cheers > j > > > > + plane.fd = shared_fd; > > + plane.offset = offset; > > + plane.length = planeSize; > > + planes.push_back(std::move(plane)); > > + > > + offset += planeSize; > > + } > > + > > + return std::make_unique<FrameBuffer>(planes); > > +} > > + > > } /* namespace libcamera */ > > -- > > 2.40.0 > > >
Hi Harvey On Tue, May 16, 2023 at 04:12:08PM +0800, Cheng-Hao Yang via libcamera-devel wrote: > Thanks Jacopo! > > On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> > wrote: > > > Hi Harvey > > > > On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel > > wrote: > > > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > > > > > Add a helper function exportFrameBuffers in HeapAllocator to make it > > > easier to use. > > > > Will you use these with the Virtual pipeline handler ? > > > > > Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|. > Ref: https://patchwork.libcamera.org/patch/18533/ > > > > > > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > --- > > > include/libcamera/heap_allocator.h | 11 ++++++ > > > src/libcamera/heap_allocator.cpp | 56 ++++++++++++++++++++++++++++++ > > > 2 files changed, 67 insertions(+) > > > > > > diff --git a/include/libcamera/heap_allocator.h > > b/include/libcamera/heap_allocator.h > > > index cd7ed1a3..076f0951 100644 > > > --- a/include/libcamera/heap_allocator.h > > > +++ b/include/libcamera/heap_allocator.h > > > @@ -8,6 +8,7 @@ > > > #pragma once > > > > > > #include <stddef.h> > > > +#include <vector> > > > > > > #include <libcamera/base/unique_fd.h> > > > > > > @@ -15,6 +16,11 @@ > > > > > > namespace libcamera { > > > > > > +class Camera; > > > +class Stream; > > > +class FrameBuffer; > > > +class PixelFormatInfo; > > > > Not used > > > > > Removed. > > > > > + > > > class HeapAllocator > > > { > > > public: > > > @@ -23,7 +29,12 @@ public: > > > bool isValid() const { return heap_->isValid(); } > > > UniqueFD alloc(const char *name, std::size_t size); > > > > > > + int exportFrameBuffers(Camera *camera, Stream *stream, > > > + std::vector<std::unique_ptr<FrameBuffer>> > > *buffers); > > > + > > > private: > > > + std::unique_ptr<FrameBuffer> createBuffer(Stream *stream); > > > + > > > std::unique_ptr<Heap> heap_; > > > }; > > > > > > diff --git a/src/libcamera/heap_allocator.cpp > > b/src/libcamera/heap_allocator.cpp > > > index 179c2c21..33e9eaca 100644 > > > --- a/src/libcamera/heap_allocator.cpp > > > +++ b/src/libcamera/heap_allocator.cpp > > > @@ -17,7 +17,11 @@ > > > > > > #include <libcamera/base/log.h> > > > > > > +#include <libcamera/camera.h> > > > #include <libcamera/dma_heap.h> > > > +#include <libcamera/framebuffer.h> > > > +#include <libcamera/internal/formats.h> > > > +#include <libcamera/stream.h> > > > #include <libcamera/udma_heap.h> > > > > > > namespace libcamera { > > > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name, > > std::size_t size) > > > return heap_->alloc(name, size); > > > } > > > > > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera, > > Stream *stream, > > > + > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > > +{ > > > + unsigned int count = stream->configuration().bufferCount; > > > + > > > + /** \todo: Support multiplanar allocations */ > > > > no : after \todo > > > > > Done > > > + > > > + for (unsigned i = 0; i < count; ++i) { > > > + std::unique_ptr<FrameBuffer> buffer = createBuffer(stream); > > > + if (!buffer) { > > > + LOG(HeapAllocator, Error) << "Unable to create > > buffer"; > > > + > > > + buffers->clear(); > > > + return -EINVAL; > > > + } > > > + > > > + buffers->push_back(std::move(buffer)); > > > + } > > > + > > > + return count; > > > +} > > > + > > > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream) > > > +{ > > > + std::vector<FrameBuffer::Plane> planes; > > > + > > > + int num_page = (stream->configuration().frameSize + getpagesize() > > - 1) / getpagesize(); > > > > Could you break this rather long line ? > > > > Defined "frameSize" and "pageSize" beforehand. Should be shorter now. > > > > Can num_page be unsigned ? > > > > Sure. > > > > libcamera uses camelCase and not snake_case for variables > > > > unsigned int numPage = (stream->configuration().frameSize + > > getpagesize() - 1) > > / getpagesize(); > > > > What are the implications of overallocating ? The last plane will be > > slightly larger, is this an issue at all ? > > > > > The allocation only works if the requested size is a direct multiple of > |getpagesize()|. Added a comment. > Ref: > https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72 > Yes, I understand the requirement, I was wondering about the implications of overallocation, but I presume this is safe ? > > > > + > > > + UniqueFD fd = alloc("Buffer", num_page * getpagesize()); > > > > What are the implications of using the same name ? > > > > > Since it's a private function that is only called by |exportFrameBuffer|, > renamed it to "FrameBuffer" instead. > > > > > + if (!fd.isValid()) > > > + return nullptr; > > > + > > > + auto info = > > PixelFormatInfo::info(stream->configuration().pixelFormat); > > > + SharedFD shared_fd(std::move(fd)); > > > + unsigned int offset = 0; > > > + for (long unsigned int i = 0; i < info.planes.size(); ++i) { > > > > Does this need to be long ? > > > > > I should use size_t instead. > > > > > + unsigned int planeSize = > > info.planeSize(stream->configuration().size, i); > > > > configuration().size is the size in pixel, is this intended ? > > > > > Yes, I guess so, according to the comment of PixelFormatInfo: > https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056 > Ah you're right \param[in] size The size of the frame, in pixels > > > > + if (planeSize == 0) > > > + continue; > > > + > > > + FrameBuffer::Plane plane; > > > > I would note down a \todo to remind that we don't support allocating > > buffers with a dedicated fd per plane > > > > > Is it preferred to do so? I guess we can also support that by calling > |alloc()| multiple times...? > I don't think it's specifically preferred, but might be required for fully-planar formats where each plane lives in its own possibly non-contiguous memory area. I would for now record a todo item We have a similar issue here iirc https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/mm/cros_frame_buffer_allocator.cpp#n57 > > > Sorry, lot of questions :) > > > > > Thanks for doing so :) > > > > Cheers > > j > > > > > > > + plane.fd = shared_fd; > > > + plane.offset = offset; > > > + plane.length = planeSize; > > > + planes.push_back(std::move(plane)); > > > + > > > + offset += planeSize; > > > + } > > > + > > > + return std::make_unique<FrameBuffer>(planes); > > > +} > > > + > > > } /* namespace libcamera */ > > > -- > > > 2.40.0 > > > > >
Thanks Jacopo. On Tue, May 16, 2023 at 6:21 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Harvey > > On Tue, May 16, 2023 at 04:12:08PM +0800, Cheng-Hao Yang via > libcamera-devel wrote: > > Thanks Jacopo! > > > > On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi < > jacopo.mondi@ideasonboard.com> > > wrote: > > > > > Hi Harvey > > > > > > On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via > libcamera-devel > > > wrote: > > > > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > > > > > > > Add a helper function exportFrameBuffers in HeapAllocator to make it > > > > easier to use. > > > > > > Will you use these with the Virtual pipeline handler ? > > > > > > > > Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|. > > Ref: https://patchwork.libcamera.org/patch/18533/ > > > > > > > > > > > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > --- > > > > include/libcamera/heap_allocator.h | 11 ++++++ > > > > src/libcamera/heap_allocator.cpp | 56 > ++++++++++++++++++++++++++++++ > > > > 2 files changed, 67 insertions(+) > > > > > > > > diff --git a/include/libcamera/heap_allocator.h > > > b/include/libcamera/heap_allocator.h > > > > index cd7ed1a3..076f0951 100644 > > > > --- a/include/libcamera/heap_allocator.h > > > > +++ b/include/libcamera/heap_allocator.h > > > > @@ -8,6 +8,7 @@ > > > > #pragma once > > > > > > > > #include <stddef.h> > > > > +#include <vector> > > > > > > > > #include <libcamera/base/unique_fd.h> > > > > > > > > @@ -15,6 +16,11 @@ > > > > > > > > namespace libcamera { > > > > > > > > +class Camera; > > > > +class Stream; > > > > +class FrameBuffer; > > > > +class PixelFormatInfo; > > > > > > Not used > > > > > > > > Removed. > > > > > > > > + > > > > class HeapAllocator > > > > { > > > > public: > > > > @@ -23,7 +29,12 @@ public: > > > > bool isValid() const { return heap_->isValid(); } > > > > UniqueFD alloc(const char *name, std::size_t size); > > > > > > > > + int exportFrameBuffers(Camera *camera, Stream *stream, > > > > + > std::vector<std::unique_ptr<FrameBuffer>> > > > *buffers); > > > > + > > > > private: > > > > + std::unique_ptr<FrameBuffer> createBuffer(Stream *stream); > > > > + > > > > std::unique_ptr<Heap> heap_; > > > > }; > > > > > > > > diff --git a/src/libcamera/heap_allocator.cpp > > > b/src/libcamera/heap_allocator.cpp > > > > index 179c2c21..33e9eaca 100644 > > > > --- a/src/libcamera/heap_allocator.cpp > > > > +++ b/src/libcamera/heap_allocator.cpp > > > > @@ -17,7 +17,11 @@ > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > +#include <libcamera/camera.h> > > > > #include <libcamera/dma_heap.h> > > > > +#include <libcamera/framebuffer.h> > > > > +#include <libcamera/internal/formats.h> > > > > +#include <libcamera/stream.h> > > > > #include <libcamera/udma_heap.h> > > > > > > > > namespace libcamera { > > > > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name, > > > std::size_t size) > > > > return heap_->alloc(name, size); > > > > } > > > > > > > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera > *camera, > > > Stream *stream, > > > > + > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > > > +{ > > > > + unsigned int count = stream->configuration().bufferCount; > > > > + > > > > + /** \todo: Support multiplanar allocations */ > > > > > > no : after \todo > > > > > > > > Done > > > > > + > > > > + for (unsigned i = 0; i < count; ++i) { > > > > + std::unique_ptr<FrameBuffer> buffer = > createBuffer(stream); > > > > + if (!buffer) { > > > > + LOG(HeapAllocator, Error) << "Unable to create > > > buffer"; > > > > + > > > > + buffers->clear(); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + buffers->push_back(std::move(buffer)); > > > > + } > > > > + > > > > + return count; > > > > +} > > > > + > > > > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream > *stream) > > > > +{ > > > > + std::vector<FrameBuffer::Plane> planes; > > > > + > > > > + int num_page = (stream->configuration().frameSize + > getpagesize() > > > - 1) / getpagesize(); > > > > > > Could you break this rather long line ? > > > > > > > Defined "frameSize" and "pageSize" beforehand. Should be shorter now. > > > > > > > Can num_page be unsigned ? > > > > > > > Sure. > > > > > > > libcamera uses camelCase and not snake_case for variables > > > > > > unsigned int numPage = (stream->configuration().frameSize + > > > getpagesize() - 1) > > > / getpagesize(); > > > > > > What are the implications of overallocating ? The last plane will be > > > slightly larger, is this an issue at all ? > > > > > > > > The allocation only works if the requested size is a direct multiple of > > |getpagesize()|. Added a comment. > > Ref: > > > https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72 > > > > Yes, I understand the requirement, I was wondering about the > implications of overallocation, but I presume this is safe ? > > Ah sorry to misunderstand your question. I assume the plane size is determined by |FrameBuffer::Plane::length|, not the remaining space of the allocated buffer. It's specified in line 282. Please correct me if I'm wrong :) > > > > > > + > > > > + UniqueFD fd = alloc("Buffer", num_page * getpagesize()); > > > > > > What are the implications of using the same name ? > > > > > > > > Since it's a private function that is only called by |exportFrameBuffer|, > > renamed it to "FrameBuffer" instead. > > > > > > > > + if (!fd.isValid()) > > > > + return nullptr; > > > > + > > > > + auto info = > > > PixelFormatInfo::info(stream->configuration().pixelFormat); > > > > + SharedFD shared_fd(std::move(fd)); > > > > + unsigned int offset = 0; > > > > + for (long unsigned int i = 0; i < info.planes.size(); ++i) { > > > > > > Does this need to be long ? > > > > > > > > I should use size_t instead. > > > > > > > > + unsigned int planeSize = > > > info.planeSize(stream->configuration().size, i); > > > > > > configuration().size is the size in pixel, is this intended ? > > > > > > > > Yes, I guess so, according to the comment of PixelFormatInfo: > > > https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056 > > > > Ah you're right > > \param[in] size The size of the frame, in pixels > > > > > > > > + if (planeSize == 0) > > > > + continue; > > > > + > > > > + FrameBuffer::Plane plane; > > > > > > I would note down a \todo to remind that we don't support allocating > > > buffers with a dedicated fd per plane > > > > > > > > Is it preferred to do so? I guess we can also support that by calling > > |alloc()| multiple times...? > > > > I don't think it's specifically preferred, but might be required for > fully-planar formats where each plane lives in its own possibly > non-contiguous memory area. I would for now record a todo item > > We have a similar issue here iirc > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/mm/cros_frame_buffer_allocator.cpp#n57 > > > Okay. Added a comment. Feel free to modify it when landing. Also let me know if a dedicated fd is needed here, as I think it's not that hard to do so :) > > > > > Sorry, lot of questions :) > > > > > > > > Thanks for doing so :) > > > > > > > Cheers > > > j > > > > > > > > > > + plane.fd = shared_fd; > > > > + plane.offset = offset; > > > > + plane.length = planeSize; > > > > + planes.push_back(std::move(plane)); > > > > + > > > > + offset += planeSize; > > > > + } > > > > + > > > > + return std::make_unique<FrameBuffer>(planes); > > > > +} > > > > + > > > > } /* namespace libcamera */ > > > > -- > > > > 2.40.0 > > > > > > > >
diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h index cd7ed1a3..076f0951 100644 --- a/include/libcamera/heap_allocator.h +++ b/include/libcamera/heap_allocator.h @@ -8,6 +8,7 @@ #pragma once #include <stddef.h> +#include <vector> #include <libcamera/base/unique_fd.h> @@ -15,6 +16,11 @@ namespace libcamera { +class Camera; +class Stream; +class FrameBuffer; +class PixelFormatInfo; + class HeapAllocator { public: @@ -23,7 +29,12 @@ public: bool isValid() const { return heap_->isValid(); } UniqueFD alloc(const char *name, std::size_t size); + int exportFrameBuffers(Camera *camera, Stream *stream, + std::vector<std::unique_ptr<FrameBuffer>> *buffers); + private: + std::unique_ptr<FrameBuffer> createBuffer(Stream *stream); + std::unique_ptr<Heap> heap_; }; diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp index 179c2c21..33e9eaca 100644 --- a/src/libcamera/heap_allocator.cpp +++ b/src/libcamera/heap_allocator.cpp @@ -17,7 +17,11 @@ #include <libcamera/base/log.h> +#include <libcamera/camera.h> #include <libcamera/dma_heap.h> +#include <libcamera/framebuffer.h> +#include <libcamera/internal/formats.h> +#include <libcamera/stream.h> #include <libcamera/udma_heap.h> namespace libcamera { @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name, std::size_t size) return heap_->alloc(name, size); } +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream, + std::vector<std::unique_ptr<FrameBuffer>> *buffers) +{ + unsigned int count = stream->configuration().bufferCount; + + /** \todo: Support multiplanar allocations */ + + for (unsigned i = 0; i < count; ++i) { + std::unique_ptr<FrameBuffer> buffer = createBuffer(stream); + if (!buffer) { + LOG(HeapAllocator, Error) << "Unable to create buffer"; + + buffers->clear(); + return -EINVAL; + } + + buffers->push_back(std::move(buffer)); + } + + return count; +} + +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream) +{ + std::vector<FrameBuffer::Plane> planes; + + int num_page = (stream->configuration().frameSize + getpagesize() - 1) / getpagesize(); + + UniqueFD fd = alloc("Buffer", num_page * getpagesize()); + if (!fd.isValid()) + return nullptr; + + auto info = PixelFormatInfo::info(stream->configuration().pixelFormat); + SharedFD shared_fd(std::move(fd)); + unsigned int offset = 0; + for (long unsigned int i = 0; i < info.planes.size(); ++i) { + unsigned int planeSize = info.planeSize(stream->configuration().size, i); + if (planeSize == 0) + continue; + + FrameBuffer::Plane plane; + plane.fd = shared_fd; + plane.offset = offset; + plane.length = planeSize; + planes.push_back(std::move(plane)); + + offset += planeSize; + } + + return std::make_unique<FrameBuffer>(planes); +} + } /* namespace libcamera */