[v7,1/7] libcamera: add DmaBufAllocation::exportFrameBuffers()
diff mbox series

Message ID 20240801073339.4061027-2-chenghaoyang@google.com
State Superseded
Headers show
Series
  • Add VirtualPipelineHandler
Related show

Commit Message

Harvey Yang Aug. 1, 2024, 7:30 a.m. UTC 
Add a helper function exportFrameBuffers in DmaBufAllocator to make it
easier to use.

It'll be used in Virtual Pipeline Handler specifically.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 .../libcamera/internal/dma_buf_allocator.h    | 10 +++
 src/libcamera/dma_buf_allocator.cpp           | 68 ++++++++++++++++++-
 2 files changed, 76 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Aug. 3, 2024, 8:20 p.m. UTC | #1 
Hi Chenghao,

Thank you for the patch.

On Thu, Aug 01, 2024 at 07:30:57AM +0000, Harvey Yang wrote:
> Add a helper function exportFrameBuffers in DmaBufAllocator to make it
> easier to use.
> 
> It'll be used in Virtual Pipeline Handler specifically.
> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  .../libcamera/internal/dma_buf_allocator.h    | 10 +++
>  src/libcamera/dma_buf_allocator.cpp           | 68 ++++++++++++++++++-
>  2 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> index 36ec1696..dd2cc237 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,13 @@ public:
>  	bool isValid() const { return providerHandle_.isValid(); }
>  	UniqueFD alloc(const char *name, std::size_t size);
>  
> +	int exportFrameBuffers(
> +		const StreamConfiguration &config,
> +		std::vector<std::unique_ptr<FrameBuffer>> *buffers);

I wonder if the DmaBufAllocator class is really the best place to
implement this. It would help reviewing how generic the implementation
is if we had two users as part of this series. Maybe converting the soft
ISP to the new function could be such a second user.

The part that bothers me the most is, I think, usage of
StreamConfiguration in this function. Maybe the createBuffer() function
could be kept, and be passed a pixel format and a size, whie the
exportFrameBuffers() could be moved to the pipeline handler.

> +
>  private:
> +	std::unique_ptr<FrameBuffer> createBuffer(const StreamConfiguration &config);
> +
>  	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 c06eca7d..6b406880 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

Unrelated change.

>  #endif
>  #endif
>  
> @@ -243,4 +248,63 @@ 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] config The config of the stream to allocate buffers for
> + * \param[out] buffers Array of buffers successfully allocated
> + *
> + * Allocates buffers for a stream from the DmaBufAllocator. It's a helper
> + * function that'll be used in PipelineHandler::exportFrameBuffers().
> + *
> + * \return The number of allocated buffers on success or a negative error code
> + * otherwise
> + */
> +int DmaBufAllocator::exportFrameBuffers(
> +	const StreamConfiguration &config,
> +	std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	unsigned int count = config.bufferCount;
> +
> +	for (unsigned i = 0; i < count; ++i) {
> +		std::unique_ptr<FrameBuffer> buffer = createBuffer(config);
> +		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(
> +	const StreamConfiguration &config)
> +{
> +	std::vector<FrameBuffer::Plane> planes;
> +
> +	auto info = PixelFormatInfo::info(config.pixelFormat);
> +	for (size_t i = 0; i < info.planes.size(); ++i) {
> +		unsigned int planeSize = info.planeSize(config.size, i);
> +		if (planeSize == 0)
> +			continue;
> +
> +		UniqueFD fd = alloc("FrameBuffer", planeSize);
> +		if (!fd.isValid())
> +			return nullptr;
> +
> +		SharedFD sharedFd(std::move(fd));
> +
> +		FrameBuffer::Plane plane;
> +		plane.fd = sharedFd;

		plane.fd = SharedFD(std::move(fd));
	
and drop the sharedFd variable.

> +		plane.offset = 0;
> +		plane.length = planeSize;
> +		planes.push_back(std::move(plane));
> +	}
> +
> +	return std::make_unique<FrameBuffer>(planes);
> +}
> +
>  } /* namespace libcamera */
Harvey Yang Aug. 5, 2024, 2 p.m. UTC | #2 
Hi Laurent,

Thanks for the review. Updated in v8.

On Sat, Aug 3, 2024 at 10:21 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Chenghao,
>
> Thank you for the patch.
>
> On Thu, Aug 01, 2024 at 07:30:57AM +0000, Harvey Yang wrote:
> > Add a helper function exportFrameBuffers in DmaBufAllocator to make it
> > easier to use.
> >
> > It'll be used in Virtual Pipeline Handler specifically.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  .../libcamera/internal/dma_buf_allocator.h    | 10 +++
> >  src/libcamera/dma_buf_allocator.cpp           | 68 ++++++++++++++++++-
> >  2 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/internal/dma_buf_allocator.h
> b/include/libcamera/internal/dma_buf_allocator.h
> > index 36ec1696..dd2cc237 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,13 @@ public:
> >       bool isValid() const { return providerHandle_.isValid(); }
> >       UniqueFD alloc(const char *name, std::size_t size);
> >
> > +     int exportFrameBuffers(
> > +             const StreamConfiguration &config,
> > +             std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> I wonder if the DmaBufAllocator class is really the best place to
> implement this. It would help reviewing how generic the implementation
> is if we had two users as part of this series. Maybe converting the soft
> ISP to the new function could be such a second user.
>
>
Right, it's a good idea that we add the helper functions in a way that works
for soft ISP as well.


> The part that bothers me the most is, I think, usage of
> StreamConfiguration in this function. Maybe the createBuffer() function
> could be kept, and be passed a pixel format and a size, whie the
> exportFrameBuffers() could be moved to the pipeline handler.
>
>
Hmm, as |SoftwareIsp::exportFrames| only uses `debayer_->frameSize()`,
I updated the DmaBufAllocator's helper function with `count` and a list of
`frameSize`s. Please check if it's better.



> > +
> >  private:
> > +     std::unique_ptr<FrameBuffer> createBuffer(const
> StreamConfiguration &config);
> > +
> >       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 c06eca7d..6b406880 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
>
> Unrelated change.
>
>
Sorry, my linter auto-corrected it. Removed.


> >  #endif
> >  #endif
> >
> > @@ -243,4 +248,63 @@ 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] config The config of the stream to allocate buffers for
> > + * \param[out] buffers Array of buffers successfully allocated
> > + *
> > + * Allocates buffers for a stream from the DmaBufAllocator. It's a
> helper
> > + * function that'll be used in PipelineHandler::exportFrameBuffers().
> > + *
> > + * \return The number of allocated buffers on success or a negative
> error code
> > + * otherwise
> > + */
> > +int DmaBufAllocator::exportFrameBuffers(
> > +     const StreamConfiguration &config,
> > +     std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > +     unsigned int count = config.bufferCount;
> > +
> > +     for (unsigned i = 0; i < count; ++i) {
> > +             std::unique_ptr<FrameBuffer> buffer = createBuffer(config);
> > +             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(
> > +     const StreamConfiguration &config)
> > +{
> > +     std::vector<FrameBuffer::Plane> planes;
> > +
> > +     auto info = PixelFormatInfo::info(config.pixelFormat);
> > +     for (size_t i = 0; i < info.planes.size(); ++i) {
> > +             unsigned int planeSize = info.planeSize(config.size, i);
> > +             if (planeSize == 0)
> > +                     continue;
> > +
> > +             UniqueFD fd = alloc("FrameBuffer", planeSize);
> > +             if (!fd.isValid())
> > +                     return nullptr;
> > +
> > +             SharedFD sharedFd(std::move(fd));
> > +
> > +             FrameBuffer::Plane plane;
> > +             plane.fd = sharedFd;
>
>                 plane.fd = SharedFD(std::move(fd));
>
> and drop the sharedFd variable.
>
>
Done.


> > +             plane.offset = 0;
> > +             plane.length = planeSize;
> > +             planes.push_back(std::move(plane));
> > +     }
> > +
> > +     return std::make_unique<FrameBuffer>(planes);
> > +}
> > +
> >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
>

Thanks!
Harvey

Patch
diff mbox series 

diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
index 36ec1696..dd2cc237 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,13 @@  public:
 	bool isValid() const { return providerHandle_.isValid(); }
 	UniqueFD alloc(const char *name, std::size_t size);
 
+	int exportFrameBuffers(
+		const StreamConfiguration &config,
+		std::vector<std::unique_ptr<FrameBuffer>> *buffers);
+
 private:
+	std::unique_ptr<FrameBuffer> createBuffer(const StreamConfiguration &config);
+
 	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 c06eca7d..6b406880 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,63 @@  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] config The config of the stream to allocate buffers for
+ * \param[out] buffers Array of buffers successfully allocated
+ *
+ * Allocates buffers for a stream from the DmaBufAllocator. It's a helper
+ * function that'll be used in PipelineHandler::exportFrameBuffers().
+ *
+ * \return The number of allocated buffers on success or a negative error code
+ * otherwise
+ */
+int DmaBufAllocator::exportFrameBuffers(
+	const StreamConfiguration &config,
+	std::vector<std::unique_ptr<FrameBuffer>> *buffers)
+{
+	unsigned int count = config.bufferCount;
+
+	for (unsigned i = 0; i < count; ++i) {
+		std::unique_ptr<FrameBuffer> buffer = createBuffer(config);
+		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(
+	const StreamConfiguration &config)
+{
+	std::vector<FrameBuffer::Plane> planes;
+
+	auto info = PixelFormatInfo::info(config.pixelFormat);
+	for (size_t i = 0; i < info.planes.size(); ++i) {
+		unsigned int planeSize = info.planeSize(config.size, i);
+		if (planeSize == 0)
+			continue;
+
+		UniqueFD fd = alloc("FrameBuffer", planeSize);
+		if (!fd.isValid())
+			return nullptr;
+
+		SharedFD sharedFd(std::move(fd));
+
+		FrameBuffer::Plane plane;
+		plane.fd = sharedFd;
+		plane.offset = 0;
+		plane.length = planeSize;
+		planes.push_back(std::move(plane));
+	}
+
+	return std::make_unique<FrameBuffer>(planes);
+}
+
 } /* namespace libcamera */