[libcamera-devel,v5,3/3] libcamera: Add exportFrameBuffers in HeapAllocator
diff mbox series

Message ID 20230516080459.711347-4-chenghaoyang@google.com
State Superseded
Headers show
Series
  • Add HeapAllocator
Related show

Commit Message

Harvey Yang May 16, 2023, 8:03 a.m. UTC 
From: Cheng-Hao Yang <chenghaoyang@chromium.org>

Add a helper function exportFrameBuffers in HeapAllocator to make it
easier to use.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/internal/heap_allocator.h |  9 +++
 src/libcamera/heap_allocator.cpp            | 61 +++++++++++++++++++++
 2 files changed, 70 insertions(+)

Comments

Jacopo Mondi May 16, 2023, 10:32 a.m. UTC | #1 
Hi Harvey

On Tue, May 16, 2023 at 08:03:19AM +0000, 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.
>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  include/libcamera/internal/heap_allocator.h |  9 +++
>  src/libcamera/heap_allocator.cpp            | 61 +++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>
> diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h
> index d061fdce..b6488fbc 100644
> --- a/include/libcamera/internal/heap_allocator.h
> +++ b/include/libcamera/internal/heap_allocator.h
> @@ -8,12 +8,16 @@
>  #pragma once
>
>  #include <stddef.h>
> +#include <vector>
>
>  #include <libcamera/base/unique_fd.h>
>
>  namespace libcamera {
>
> +class Camera;
> +class FrameBuffer;
>  class Heap;
> +class Stream;
>
>  class HeapAllocator
>  {
> @@ -23,7 +27,12 @@ public:
>  	bool isValid() const;
>  	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 682a7e01..ce3a815d 100644
> --- a/src/libcamera/heap_allocator.cpp
> +++ b/src/libcamera/heap_allocator.cpp
> @@ -21,6 +21,12 @@
>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/camera.h>
> +#include <libcamera/framebuffer.h>
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/formats.h"
> +
>  namespace libcamera {
>
>  /*
> @@ -231,4 +237,59 @@ 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;
> +
> +	unsigned int frameSize = stream->configuration().frameSize;
> +	int pageSize = getpagesize();
> +	// Allocation size need to be a direct multiple of |pageSize|.

no c++ style comments, please

> +	unsigned int numPage = (frameSize + pageSize - 1) / pageSize;
> +
> +	UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);
> +	if (!fd.isValid())
> +		return nullptr;
> +

And I would add here a comment as reported in the review of v4

Can be fixed when applying
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> +	auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);
> +	SharedFD shared_fd(std::move(fd));
> +	unsigned int offset = 0;
> +	for (size_t 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 */
> --
> 2.40.1.606.ga4b1b128d6-goog
>
Harvey Yang May 22, 2023, 8:36 a.m. UTC | #2 
Thanks Jacopo.

On Tue, May 16, 2023 at 6:32 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

> Hi Harvey
>
> On Tue, May 16, 2023 at 08:03:19AM +0000, 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.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  include/libcamera/internal/heap_allocator.h |  9 +++
> >  src/libcamera/heap_allocator.cpp            | 61 +++++++++++++++++++++
> >  2 files changed, 70 insertions(+)
> >
> > diff --git a/include/libcamera/internal/heap_allocator.h
> b/include/libcamera/internal/heap_allocator.h
> > index d061fdce..b6488fbc 100644
> > --- a/include/libcamera/internal/heap_allocator.h
> > +++ b/include/libcamera/internal/heap_allocator.h
> > @@ -8,12 +8,16 @@
> >  #pragma once
> >
> >  #include <stddef.h>
> > +#include <vector>
> >
> >  #include <libcamera/base/unique_fd.h>
> >
> >  namespace libcamera {
> >
> > +class Camera;
> > +class FrameBuffer;
> >  class Heap;
> > +class Stream;
> >
> >  class HeapAllocator
> >  {
> > @@ -23,7 +27,12 @@ public:
> >       bool isValid() const;
> >       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 682a7e01..ce3a815d 100644
> > --- a/src/libcamera/heap_allocator.cpp
> > +++ b/src/libcamera/heap_allocator.cpp
> > @@ -21,6 +21,12 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > +#include <libcamera/camera.h>
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "libcamera/internal/formats.h"
> > +
> >  namespace libcamera {
> >
> >  /*
> > @@ -231,4 +237,59 @@ 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;
> > +
> > +     unsigned int frameSize = stream->configuration().frameSize;
> > +     int pageSize = getpagesize();
> > +     // Allocation size need to be a direct multiple of |pageSize|.
>
> no c++ style comments, please
>
>
You mean to use "/**" and "*/", right?


> > +     unsigned int numPage = (frameSize + pageSize - 1) / pageSize;
> > +
> > +     UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);
> > +     if (!fd.isValid())
> > +             return nullptr;
> > +
>
> And I would add here a comment as reported in the review of v4
>
> Can be fixed when applying
>

You mean the todo of the dedicated fd for each plane, right?


> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>   j
>
> > +     auto info =
> PixelFormatInfo::info(stream->configuration().pixelFormat);
> > +     SharedFD shared_fd(std::move(fd));
> > +     unsigned int offset = 0;
> > +     for (size_t 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 */
> > --
> > 2.40.1.606.ga4b1b128d6-goog
> >
>
Jacopo Mondi May 22, 2023, 1:19 p.m. UTC | #3 
Hello Harvey

On Mon, May 22, 2023 at 04:36:32PM +0800, Cheng-Hao Yang via libcamera-devel wrote:
> Thanks Jacopo.
>
> On Tue, May 16, 2023 at 6:32 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> wrote:
>
> > Hi Harvey
> >
> > On Tue, May 16, 2023 at 08:03:19AM +0000, 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.
> > >
> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > ---
> > >  include/libcamera/internal/heap_allocator.h |  9 +++
> > >  src/libcamera/heap_allocator.cpp            | 61 +++++++++++++++++++++
> > >  2 files changed, 70 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/heap_allocator.h
> > b/include/libcamera/internal/heap_allocator.h
> > > index d061fdce..b6488fbc 100644
> > > --- a/include/libcamera/internal/heap_allocator.h
> > > +++ b/include/libcamera/internal/heap_allocator.h
> > > @@ -8,12 +8,16 @@
> > >  #pragma once
> > >
> > >  #include <stddef.h>
> > > +#include <vector>
> > >
> > >  #include <libcamera/base/unique_fd.h>
> > >
> > >  namespace libcamera {
> > >
> > > +class Camera;
> > > +class FrameBuffer;
> > >  class Heap;
> > > +class Stream;
> > >
> > >  class HeapAllocator
> > >  {
> > > @@ -23,7 +27,12 @@ public:
> > >       bool isValid() const;
> > >       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 682a7e01..ce3a815d 100644
> > > --- a/src/libcamera/heap_allocator.cpp
> > > +++ b/src/libcamera/heap_allocator.cpp
> > > @@ -21,6 +21,12 @@
> > >
> > >  #include <libcamera/base/log.h>
> > >
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/framebuffer.h>
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include "libcamera/internal/formats.h"
> > > +
> > >  namespace libcamera {
> > >
> > >  /*
> > > @@ -231,4 +237,59 @@ 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;
> > > +
> > > +     unsigned int frameSize = stream->configuration().frameSize;
> > > +     int pageSize = getpagesize();
> > > +     // Allocation size need to be a direct multiple of |pageSize|.
> >
> > no c++ style comments, please
> >
> >
> You mean to use "/**" and "*/", right?
>

/** is (one of the) doxygen markers

/* is the single line comment stlye. */

/*
 * while this one is a multi line comment
 * split on multiple lines.
 */

>
> > > +     unsigned int numPage = (frameSize + pageSize - 1) / pageSize;
> > > +
> > > +     UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);
> > > +     if (!fd.isValid())
> > > +             return nullptr;
> > > +
> >
> > And I would add here a comment as reported in the review of v4
> >
> > Can be fixed when applying
> >
>
> You mean the todo of the dedicated fd for each plane, right?
>

If I recall correctly, yes :)

>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > Thanks
> >   j
> >
> > > +     auto info =
> > PixelFormatInfo::info(stream->configuration().pixelFormat);
> > > +     SharedFD shared_fd(std::move(fd));
> > > +     unsigned int offset = 0;
> > > +     for (size_t 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 */
> > > --
> > > 2.40.1.606.ga4b1b128d6-goog
> > >
> >

Patch
diff mbox series 

diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h
index d061fdce..b6488fbc 100644
--- a/include/libcamera/internal/heap_allocator.h
+++ b/include/libcamera/internal/heap_allocator.h
@@ -8,12 +8,16 @@ 
 #pragma once
 
 #include <stddef.h>
+#include <vector>
 
 #include <libcamera/base/unique_fd.h>
 
 namespace libcamera {
 
+class Camera;
+class FrameBuffer;
 class Heap;
+class Stream;
 
 class HeapAllocator
 {
@@ -23,7 +27,12 @@  public:
 	bool isValid() const;
 	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 682a7e01..ce3a815d 100644
--- a/src/libcamera/heap_allocator.cpp
+++ b/src/libcamera/heap_allocator.cpp
@@ -21,6 +21,12 @@ 
 
 #include <libcamera/base/log.h>
 
+#include <libcamera/camera.h>
+#include <libcamera/framebuffer.h>
+#include <libcamera/stream.h>
+
+#include "libcamera/internal/formats.h"
+
 namespace libcamera {
 
 /*
@@ -231,4 +237,59 @@  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;
+
+	unsigned int frameSize = stream->configuration().frameSize;
+	int pageSize = getpagesize();
+	// Allocation size need to be a direct multiple of |pageSize|.
+	unsigned int numPage = (frameSize + pageSize - 1) / pageSize;
+
+	UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);
+	if (!fd.isValid())
+		return nullptr;
+
+	auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);
+	SharedFD shared_fd(std::move(fd));
+	unsigned int offset = 0;
+	for (size_t 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 */