[libcamera-devel,v2,12/16] libcamera: buffer: Add dmabuf file descriptors
diff mbox series

Message ID 20190713172351.25452-13-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Add support for external buffers
Related show

Commit Message

Laurent Pinchart July 13, 2019, 5:23 p.m. UTC 
From: Jacopo Mondi <jacopo@jmondi.org>

In addition to referencing buffer memory by index, add support to
referencing it using dmabuf file descriptors. This will be used to
reference buffer memory allocated outside of libcamera and import it.

The dmabuf file descriptors are stored in an array in the Buffer class,
and a new Stream::createBuffer() overload is added to construct a buffer
from dmabuf file descriptor.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/buffer.h |  3 +++
 include/libcamera/stream.h |  1 +
 src/libcamera/buffer.cpp   | 13 ++++++++++-
 src/libcamera/request.cpp  |  5 ++++
 src/libcamera/stream.cpp   | 47 +++++++++++++++++++++++++++++++++++++-
 5 files changed, 67 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi July 14, 2019, 7:22 a.m. UTC | #1 
Hi Laurent,
   thanks for the update

On Sat, Jul 13, 2019 at 08:23:47PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
>
> In addition to referencing buffer memory by index, add support to
> referencing it using dmabuf file descriptors. This will be used to
> reference buffer memory allocated outside of libcamera and import it.
>
> The dmabuf file descriptors are stored in an array in the Buffer class,
> and a new Stream::createBuffer() overload is added to construct a buffer
> from dmabuf file descriptor.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/buffer.h |  3 +++
>  include/libcamera/stream.h |  1 +
>  src/libcamera/buffer.cpp   | 13 ++++++++++-
>  src/libcamera/request.cpp  |  5 ++++
>  src/libcamera/stream.cpp   | 47 +++++++++++++++++++++++++++++++++++++-
>  5 files changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index f5ba6207bcef..f8569a6b67f3 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_BUFFER_H__
>  #define __LIBCAMERA_BUFFER_H__
>
> +#include <array>
>  #include <stdint.h>
>  #include <vector>
>
> @@ -75,6 +76,7 @@ public:
>  	Buffer &operator=(const Buffer &) = delete;
>
>  	unsigned int index() const { return index_; }
> +	const std::array<int, 3> &dmabufs() const { return dmabuf_; }
>
>  	unsigned int bytesused() const { return bytesused_; }
>  	uint64_t timestamp() const { return timestamp_; }
> @@ -95,6 +97,7 @@ private:
>  	void setRequest(Request *request) { request_ = request; }
>
>  	unsigned int index_;
> +	std::array<int, 3> dmabuf_;
>
>  	unsigned int bytesused_;
>  	uint64_t timestamp_;
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 08eb8cc7d5c7..1883d9e9b9c5 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -75,6 +75,7 @@ public:
>  	Stream();
>
>  	std::unique_ptr<Buffer> createBuffer(unsigned int index);
> +	std::unique_ptr<Buffer> createBuffer(const std::array<int, 3> &fds);
>
>  	BufferPool &bufferPool() { return bufferPool_; }
>  	std::vector<BufferMemory> &buffers() { return bufferPool_.buffers(); }
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index ecbf25246a55..99358633a088 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -269,7 +269,8 @@ void BufferPool::destroyBuffers()
>   * for a stream with Stream::createBuffer().
>   */
>  Buffer::Buffer(unsigned int index, const Buffer *metadata)
> -	: index_(index), status_(Buffer::BufferSuccess), request_(nullptr),
> +	: index_(index), dmabuf_({ -1, -1, -1 }),
> +	  status_(Buffer::BufferSuccess), request_(nullptr),
>  	  stream_(nullptr)
>  {
>  	if (metadata) {
> @@ -289,6 +290,16 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
>   * \return The buffer index
>   */
>
> +/**
> + * \fn Buffer::dmabufs()
> + * \brief Retrieve the dmabuf file descriptors for all buffer planes
> + *
> + * The dmabufs array contains one dmabuf file descriptor per plane. Unused
> + * entries are set to -1.
> + *
> + * \return The dmabuf file descriptors
> + */
> +
>  /**
>   * \fn Buffer::bytesused()
>   * \brief Retrieve the number of bytes occupied by the data in the buffer
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 19131472710b..ee2158fc7a9c 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -106,10 +106,15 @@ Request::~Request()
>   *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -EEXIST The request already contains a buffer for the stream
> + * \retval -EINVAL The buffer does not reference a valid Stream
>   */
>  int Request::addBuffer(std::unique_ptr<Buffer> buffer)
>  {
>  	Stream *stream = buffer->stream();
> +	if (!stream) {
> +		LOG(Request, Error) << "Invalid stream reference";
> +		return -EINVAL;
> +	}
>
>  	auto it = bufferMap_.find(stream);
>  	if (it != bufferMap_.end()) {
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 94aa4810f6b9..e6aa1b643a37 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -424,17 +424,26 @@ Stream::Stream()
>  }
>
>  /**
> - * \brief Create a Buffer instance
> + * \brief Create a Buffer instance referencing the memory buffer \a index
>   * \param[in] index The desired buffer index
>   *
>   * This method creates a Buffer instance that references a BufferMemory from
>   * the stream's buffers pool by its \a index. The index shall be lower than the
>   * number of buffers in the pool.
>   *
> + * This method is only valid for streams that use the InternalMemory type. It
> + * will return a null pointer when called on streams using the ExternalMemory
> + * type.
> + *
>   * \return A newly created Buffer on success or nullptr otherwise
>   */
>  std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)
>  {
> +	if (memoryType_ != InternalMemory) {
> +		LOG(Stream, Error) << "Invalid stream memory type";
> +		return nullptr;
> +	}
> +
>  	if (index >= bufferPool_.count()) {
>  		LOG(Stream, Error) << "Invalid buffer index " << index;
>  		return nullptr;
> @@ -447,6 +456,42 @@ std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)
>  	return std::unique_ptr<Buffer>(buffer);
>  }
>
> +/**
> + * \brief Create a Buffer instance that represents a memory area identified by
> + * dmabuf file descriptors
> + * \param[in] fds The dmabuf file descriptors for each plane
> + *
> + * This method creates a Buffer instance that references buffer memory
> + * allocated outside of libcamera through dmabuf file descriptors. The \a
> + * dmabuf array shall contain a file descriptor for each plane in the buffer,
> + * and unused entries shall be set to -1.
> + *
> + * The buffer is created without a valid index, as it does not yet map to any of
> + * the stream's BufferMemory instances. An index will be assigned at the time
> + * the buffer is queued to the camera in a request. Applications may thus
> + * create any number of Buffer instances, providing that no more than the
> + * number of buffers allocated for the stream are queued at any given time.
> + *
> + * This method is only valid for streams that use the InternalMemory type. It
> + * will return a null pointer when called on streams using the ExternalMemory
> + * type.

It's actually the other way around!

Thanks
  j

> + *
> + * \return A newly created Buffer on success or nullptr otherwise
> + */
> +std::unique_ptr<Buffer> Stream::createBuffer(const std::array<int, 3> &fds)
> +{
> +	if (memoryType_ != ExternalMemory) {
> +		LOG(Stream, Error) << "Invalid stream memory type";
> +		return nullptr;
> +	}
> +
> +	Buffer *buffer = new Buffer();
> +	buffer->dmabuf_ = fds;
> +	buffer->stream_ = this;
> +
> +	return std::unique_ptr<Buffer>(buffer);
> +}
> +
>  /**
>   * \fn Stream::bufferPool()
>   * \brief Retrieve the buffer pool for the stream
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund July 14, 2019, 10:52 a.m. UTC | #2 
Hi Jacopo and Laurent,

Thanks for your patch.

On 2019-07-13 20:23:47 +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> In addition to referencing buffer memory by index, add support to
> referencing it using dmabuf file descriptors. This will be used to
> reference buffer memory allocated outside of libcamera and import it.
> 
> The dmabuf file descriptors are stored in an array in the Buffer class,
> and a new Stream::createBuffer() overload is added to construct a buffer
> from dmabuf file descriptor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/buffer.h |  3 +++
>  include/libcamera/stream.h |  1 +
>  src/libcamera/buffer.cpp   | 13 ++++++++++-
>  src/libcamera/request.cpp  |  5 ++++
>  src/libcamera/stream.cpp   | 47 +++++++++++++++++++++++++++++++++++++-
>  5 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index f5ba6207bcef..f8569a6b67f3 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_BUFFER_H__
>  #define __LIBCAMERA_BUFFER_H__
>  
> +#include <array>
>  #include <stdint.h>
>  #include <vector>
>  
> @@ -75,6 +76,7 @@ public:
>  	Buffer &operator=(const Buffer &) = delete;
>  
>  	unsigned int index() const { return index_; }
> +	const std::array<int, 3> &dmabufs() const { return dmabuf_; }
>  
>  	unsigned int bytesused() const { return bytesused_; }
>  	uint64_t timestamp() const { return timestamp_; }
> @@ -95,6 +97,7 @@ private:
>  	void setRequest(Request *request) { request_ = request; }
>  
>  	unsigned int index_;
> +	std::array<int, 3> dmabuf_;
>  
>  	unsigned int bytesused_;
>  	uint64_t timestamp_;
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 08eb8cc7d5c7..1883d9e9b9c5 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -75,6 +75,7 @@ public:
>  	Stream();
>  
>  	std::unique_ptr<Buffer> createBuffer(unsigned int index);
> +	std::unique_ptr<Buffer> createBuffer(const std::array<int, 3> &fds);
>  
>  	BufferPool &bufferPool() { return bufferPool_; }
>  	std::vector<BufferMemory> &buffers() { return bufferPool_.buffers(); }
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index ecbf25246a55..99358633a088 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -269,7 +269,8 @@ void BufferPool::destroyBuffers()
>   * for a stream with Stream::createBuffer().
>   */
>  Buffer::Buffer(unsigned int index, const Buffer *metadata)
> -	: index_(index), status_(Buffer::BufferSuccess), request_(nullptr),
> +	: index_(index), dmabuf_({ -1, -1, -1 }),
> +	  status_(Buffer::BufferSuccess), request_(nullptr),
>  	  stream_(nullptr)
>  {
>  	if (metadata) {
> @@ -289,6 +290,16 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
>   * \return The buffer index
>   */
>  
> +/**
> + * \fn Buffer::dmabufs()
> + * \brief Retrieve the dmabuf file descriptors for all buffer planes
> + *
> + * The dmabufs array contains one dmabuf file descriptor per plane. Unused
> + * entries are set to -1.
> + *
> + * \return The dmabuf file descriptors
> + */
> +
>  /**
>   * \fn Buffer::bytesused()
>   * \brief Retrieve the number of bytes occupied by the data in the buffer
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 19131472710b..ee2158fc7a9c 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -106,10 +106,15 @@ Request::~Request()
>   *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -EEXIST The request already contains a buffer for the stream
> + * \retval -EINVAL The buffer does not reference a valid Stream
>   */
>  int Request::addBuffer(std::unique_ptr<Buffer> buffer)
>  {
>  	Stream *stream = buffer->stream();
> +	if (!stream) {
> +		LOG(Request, Error) << "Invalid stream reference";
> +		return -EINVAL;
> +	}
>  
>  	auto it = bufferMap_.find(stream);
>  	if (it != bufferMap_.end()) {
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 94aa4810f6b9..e6aa1b643a37 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -424,17 +424,26 @@ Stream::Stream()
>  }
>  
>  /**
> - * \brief Create a Buffer instance
> + * \brief Create a Buffer instance referencing the memory buffer \a index
>   * \param[in] index The desired buffer index
>   *
>   * This method creates a Buffer instance that references a BufferMemory from
>   * the stream's buffers pool by its \a index. The index shall be lower than the
>   * number of buffers in the pool.
>   *
> + * This method is only valid for streams that use the InternalMemory type. It
> + * will return a null pointer when called on streams using the ExternalMemory
> + * type.
> + *
>   * \return A newly created Buffer on success or nullptr otherwise
>   */
>  std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)
>  {
> +	if (memoryType_ != InternalMemory) {
> +		LOG(Stream, Error) << "Invalid stream memory type";
> +		return nullptr;
> +	}
> +
>  	if (index >= bufferPool_.count()) {
>  		LOG(Stream, Error) << "Invalid buffer index " << index;
>  		return nullptr;
> @@ -447,6 +456,42 @@ std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)
>  	return std::unique_ptr<Buffer>(buffer);
>  }
>  
> +/**
> + * \brief Create a Buffer instance that represents a memory area identified by
> + * dmabuf file descriptors
> + * \param[in] fds The dmabuf file descriptors for each plane
> + *
> + * This method creates a Buffer instance that references buffer memory
> + * allocated outside of libcamera through dmabuf file descriptors. The \a
> + * dmabuf array shall contain a file descriptor for each plane in the buffer,
> + * and unused entries shall be set to -1.
> + *
> + * The buffer is created without a valid index, as it does not yet map to any of
> + * the stream's BufferMemory instances. An index will be assigned at the time
> + * the buffer is queued to the camera in a request. Applications may thus
> + * create any number of Buffer instances, providing that no more than the
> + * number of buffers allocated for the stream are queued at any given time.
> + *
> + * This method is only valid for streams that use the InternalMemory type. It
> + * will return a null pointer when called on streams using the ExternalMemory
> + * type.
> + *
> + * \return A newly created Buffer on success or nullptr otherwise
> + */
> +std::unique_ptr<Buffer> Stream::createBuffer(const std::array<int, 3> &fds)
> +{
> +	if (memoryType_ != ExternalMemory) {
> +		LOG(Stream, Error) << "Invalid stream memory type";
> +		return nullptr;
> +	}
> +
> +	Buffer *buffer = new Buffer();
> +	buffer->dmabuf_ = fds;
> +	buffer->stream_ = this;
> +
> +	return std::unique_ptr<Buffer>(buffer);
> +}
> +
>  /**
>   * \fn Stream::bufferPool()
>   * \brief Retrieve the buffer pool for the stream
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
index f5ba6207bcef..f8569a6b67f3 100644
--- a/include/libcamera/buffer.h
+++ b/include/libcamera/buffer.h
@@ -7,6 +7,7 @@ 
 #ifndef __LIBCAMERA_BUFFER_H__
 #define __LIBCAMERA_BUFFER_H__
 
+#include <array>
 #include <stdint.h>
 #include <vector>
 
@@ -75,6 +76,7 @@  public:
 	Buffer &operator=(const Buffer &) = delete;
 
 	unsigned int index() const { return index_; }
+	const std::array<int, 3> &dmabufs() const { return dmabuf_; }
 
 	unsigned int bytesused() const { return bytesused_; }
 	uint64_t timestamp() const { return timestamp_; }
@@ -95,6 +97,7 @@  private:
 	void setRequest(Request *request) { request_ = request; }
 
 	unsigned int index_;
+	std::array<int, 3> dmabuf_;
 
 	unsigned int bytesused_;
 	uint64_t timestamp_;
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
index 08eb8cc7d5c7..1883d9e9b9c5 100644
--- a/include/libcamera/stream.h
+++ b/include/libcamera/stream.h
@@ -75,6 +75,7 @@  public:
 	Stream();
 
 	std::unique_ptr<Buffer> createBuffer(unsigned int index);
+	std::unique_ptr<Buffer> createBuffer(const std::array<int, 3> &fds);
 
 	BufferPool &bufferPool() { return bufferPool_; }
 	std::vector<BufferMemory> &buffers() { return bufferPool_.buffers(); }
diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
index ecbf25246a55..99358633a088 100644
--- a/src/libcamera/buffer.cpp
+++ b/src/libcamera/buffer.cpp
@@ -269,7 +269,8 @@  void BufferPool::destroyBuffers()
  * for a stream with Stream::createBuffer().
  */
 Buffer::Buffer(unsigned int index, const Buffer *metadata)
-	: index_(index), status_(Buffer::BufferSuccess), request_(nullptr),
+	: index_(index), dmabuf_({ -1, -1, -1 }),
+	  status_(Buffer::BufferSuccess), request_(nullptr),
 	  stream_(nullptr)
 {
 	if (metadata) {
@@ -289,6 +290,16 @@  Buffer::Buffer(unsigned int index, const Buffer *metadata)
  * \return The buffer index
  */
 
+/**
+ * \fn Buffer::dmabufs()
+ * \brief Retrieve the dmabuf file descriptors for all buffer planes
+ *
+ * The dmabufs array contains one dmabuf file descriptor per plane. Unused
+ * entries are set to -1.
+ *
+ * \return The dmabuf file descriptors
+ */
+
 /**
  * \fn Buffer::bytesused()
  * \brief Retrieve the number of bytes occupied by the data in the buffer
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index 19131472710b..ee2158fc7a9c 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -106,10 +106,15 @@  Request::~Request()
  *
  * \return 0 on success or a negative error code otherwise
  * \retval -EEXIST The request already contains a buffer for the stream
+ * \retval -EINVAL The buffer does not reference a valid Stream
  */
 int Request::addBuffer(std::unique_ptr<Buffer> buffer)
 {
 	Stream *stream = buffer->stream();
+	if (!stream) {
+		LOG(Request, Error) << "Invalid stream reference";
+		return -EINVAL;
+	}
 
 	auto it = bufferMap_.find(stream);
 	if (it != bufferMap_.end()) {
diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 94aa4810f6b9..e6aa1b643a37 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -424,17 +424,26 @@  Stream::Stream()
 }
 
 /**
- * \brief Create a Buffer instance
+ * \brief Create a Buffer instance referencing the memory buffer \a index
  * \param[in] index The desired buffer index
  *
  * This method creates a Buffer instance that references a BufferMemory from
  * the stream's buffers pool by its \a index. The index shall be lower than the
  * number of buffers in the pool.
  *
+ * This method is only valid for streams that use the InternalMemory type. It
+ * will return a null pointer when called on streams using the ExternalMemory
+ * type.
+ *
  * \return A newly created Buffer on success or nullptr otherwise
  */
 std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)
 {
+	if (memoryType_ != InternalMemory) {
+		LOG(Stream, Error) << "Invalid stream memory type";
+		return nullptr;
+	}
+
 	if (index >= bufferPool_.count()) {
 		LOG(Stream, Error) << "Invalid buffer index " << index;
 		return nullptr;
@@ -447,6 +456,42 @@  std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)
 	return std::unique_ptr<Buffer>(buffer);
 }
 
+/**
+ * \brief Create a Buffer instance that represents a memory area identified by
+ * dmabuf file descriptors
+ * \param[in] fds The dmabuf file descriptors for each plane
+ *
+ * This method creates a Buffer instance that references buffer memory
+ * allocated outside of libcamera through dmabuf file descriptors. The \a
+ * dmabuf array shall contain a file descriptor for each plane in the buffer,
+ * and unused entries shall be set to -1.
+ *
+ * The buffer is created without a valid index, as it does not yet map to any of
+ * the stream's BufferMemory instances. An index will be assigned at the time
+ * the buffer is queued to the camera in a request. Applications may thus
+ * create any number of Buffer instances, providing that no more than the
+ * number of buffers allocated for the stream are queued at any given time.
+ *
+ * This method is only valid for streams that use the InternalMemory type. It
+ * will return a null pointer when called on streams using the ExternalMemory
+ * type.
+ *
+ * \return A newly created Buffer on success or nullptr otherwise
+ */
+std::unique_ptr<Buffer> Stream::createBuffer(const std::array<int, 3> &fds)
+{
+	if (memoryType_ != ExternalMemory) {
+		LOG(Stream, Error) << "Invalid stream memory type";
+		return nullptr;
+	}
+
+	Buffer *buffer = new Buffer();
+	buffer->dmabuf_ = fds;
+	buffer->stream_ = this;
+
+	return std::unique_ptr<Buffer>(buffer);
+}
+
 /**
  * \fn Stream::bufferPool()
  * \brief Retrieve the buffer pool for the stream