[libcamera-devel,v2,14/16] libcamera: stream: Map external buffers to indexes

1695 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

Add and use an operation to assign to Buffer representing external
memory locations an index at queueRequest() time. The index is used to
identify the memory buffer to be queued to the video device once the
buffer will be queued in a Request.

In order to minimize relocations in the V4L2 backend, this method
provides a best-effort caching mechanisms that attempts to reuse
BufferMemory previously mapped to the buffer's dmabuf file descriptors,
if any.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/buffer.h |  2 +
 include/libcamera/stream.h |  6 +++
 src/libcamera/camera.cpp   | 20 +++++++-
 src/libcamera/stream.cpp   | 96 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+), 1 deletion(-)

Hi Jacopo and Laurent.

Thanks for your work.

On 2019-07-13 20:23:49 +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Add and use an operation to assign to Buffer representing external
> memory locations an index at queueRequest() time. The index is used to
> identify the memory buffer to be queued to the video device once the
> buffer will be queued in a Request.
> 
> In order to minimize relocations in the V4L2 backend, this method
> provides a best-effort caching mechanisms that attempts to reuse
> BufferMemory previously mapped to the buffer's dmabuf file descriptors,
> if any.
> 
> 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 |  2 +
>  include/libcamera/stream.h |  6 +++
>  src/libcamera/camera.cpp   | 20 +++++++-
>  src/libcamera/stream.cpp   | 96 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index fc5c7d4c41b6..7b657509ab5f 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -30,6 +30,8 @@ public:
>  	unsigned int length() const { return length_; }
>  
>  private:
> +	friend class Stream;
> +
>  	int mmap();
>  	int munmap();
>  
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 1883d9e9b9c5..2e619cdf0e89 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -85,12 +85,18 @@ public:
>  protected:
>  	friend class Camera;
>  
> +	int mapBuffer(const Buffer *buffer);
> +	void unmapBuffer(const Buffer *buffer);
> +
>  	void createBuffers(MemoryType memory, unsigned int count);
>  	void destroyBuffers();
>  
>  	BufferPool bufferPool_;
>  	StreamConfiguration configuration_;
>  	MemoryType memoryType_;
> +
> +private:
> +	std::vector<std::pair<std::array<int, 3>, unsigned int>> bufferCache_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index db15fd46cd51..f2deb38da367 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -800,6 +800,7 @@ Request *Camera::createRequest(uint64_t cookie)
>   * \retval -ENODEV The camera has been disconnected from the system
>   * \retval -EACCES The camera is not running so requests can't be queued
>   * \retval -EINVAL The request is invalid
> + * \retval -ENOMEM No buffer memory was available to handle the request
>   */
>  int Camera::queueRequest(Request *request)
>  {
> @@ -818,6 +819,16 @@ int Camera::queueRequest(Request *request)
>  			return -EINVAL;
>  		}
>  
> +		if (stream->memoryType() == ExternalMemory) {
> +			int index = stream->mapBuffer(buffer);
> +			if (index < 0) {
> +				LOG(Camera, Error) << "No buffer memory available";
> +				return -ENOMEM;
> +			}
> +
> +			buffer->index_ = index;
> +		}
> +
>  		buffer->mem_ = &stream->buffers()[buffer->index_];
>  	}
>  
> @@ -901,7 +912,14 @@ int Camera::stop()
>   */
>  void Camera::requestComplete(Request *request)
>  {
> -	requestCompleted.emit(request, request->bufferMap_);
> +	for (auto it : request->buffers()) {
> +		Stream *stream = it.first;
> +		Buffer *buffer = it.second;
> +		if (stream->memoryType() == ExternalMemory)
> +			stream->unmapBuffer(buffer);
> +	}
> +
> +	requestCompleted.emit(request, request->buffers());
>  	delete request;
>  }
>  
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index e6aa1b643a37..729d36b31712 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -13,6 +13,8 @@
>  #include <iomanip>
>  #include <sstream>
>  
> +#include <libcamera/request.h>
> +
>  #include "log.h"
>  
>  /**
> @@ -476,6 +478,8 @@ std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)
>   * will return a null pointer when called on streams using the ExternalMemory
>   * type.
>   *
> + * \sa Stream::mapBuffer()
> + *
>   * \return A newly created Buffer on success or nullptr otherwise
>   */
>  std::unique_ptr<Buffer> Stream::createBuffer(const std::array<int, 3> &fds)
> @@ -521,6 +525,86 @@ std::unique_ptr<Buffer> Stream::createBuffer(const std::array<int, 3> &fds)
>   * \return The memory type used by the stream
>   */
>  
> +/**
> + * \brief Map a Buffer to a buffer memory index
> + * \param[in] buffer The buffer to map to a buffer memory index
> + *
> + * Streams configured to use externally allocated memory need to maintain a
> + * best-effort association between the memory area the \a buffer represents
> + * and the associated buffer memory in the Stream's pool.
> + *
> + * The buffer memory to use, once the \a buffer reaches the video device,
> + * is selected using the index assigned to the \a buffer and to minimize
> + * relocations in the V4L2 back-end, this operation provides a best-effort
> + * caching mechanism that associates to the dmabuf file descriptors contained
> + * in the \a buffer the index of the buffer memory that was lastly queued with
> + * those file descriptors set.
> + *
> + * If the Stream uses internally allocated memory, the index of the memory
> + * buffer to use will match the one request at Stream::createBuffer(unsigned int)
> + * time, and no mapping is thus required.
> + *
> + * \return The buffer memory index for the buffer on success, or a negative
> + * error code otherwise
> + * \retval -ENOMEM No buffer memory was available to map the buffer
> + */
> +int Stream::mapBuffer(const Buffer *buffer)
> +{
> +	ASSERT(memoryType_ == ExternalMemory);
> +
> +	if (bufferCache_.empty())
> +		return -ENOMEM;
> +
> +	const std::array<int, 3> &dmabufs = buffer->dmabufs();
> +
> +	/*
> +	 * Try to find a previously mapped buffer in the cache. If we miss, use
> +	 * the oldest entry in the cache.
> +	 */
> +	auto map = std::find_if(bufferCache_.begin(), bufferCache_.end(),
> +				[&](std::pair<std::array<int, 3>, unsigned int> &entry) {
> +					return entry.first == dmabufs;
> +				});
> +	if (map == bufferCache_.end())
> +		map = bufferCache_.begin();
> +
> +	/*
> +	 * Update the dmabuf file descriptors of the entry. We can't assume that
> +	 * identical file descriptor numbers refer to the same dmabuf object as
> +	 * it may have been closed and its file descriptor reused. We thus need
> +	 * to update the plane's internally cached mmap()ed memory.
> +	 */
> +	unsigned int index = map->second;
> +	BufferMemory *mem = &bufferPool_.buffers()[index];
> +	mem->planes().clear();
> +
> +	for (unsigned int i = 0; i < dmabufs.size(); ++i) {
> +		if (dmabufs[i] == -1)
> +			break;
> +
> +		mem->planes().emplace_back();
> +		mem->planes().back().setDmabuf(dmabufs[i], 0);
> +	}
> +
> +	/* Remove the buffer from the cache and return its index. */
> +	bufferCache_.erase(map);
> +	return index;
> +}
> +
> +/**
> + * \brief Unmap a Buffer from its buffer memory
> + * \param[in] buffer The buffer to unmap
> + *
> + * This method releases the buffer memory entry that was mapped by mapBuffer(),
> + * making it available for new mappings.
> + */
> +void Stream::unmapBuffer(const Buffer *buffer)
> +{
> +	ASSERT(memoryType_ == ExternalMemory);
> +
> +	bufferCache_.emplace_back(buffer->dmabufs(), buffer->index());
> +}
> +
>  /**
>   * \brief Create buffers for the stream
>   * \param count The number of buffers to create
> @@ -536,6 +620,18 @@ void Stream::createBuffers(MemoryType memory, unsigned int count)
>  
>  	memoryType_ = memory;
>  	bufferPool_.createBuffers(count);
> +
> +	/* Streams with internal memory usage do not need buffer mapping. */
> +	if (memoryType_ == InternalMemory)
> +		return;
> +
> +	/*
> +	 * Prepare for buffer mapping by adding all buffer memory entries to the
> +	 * cache.
> +	 */
> +	bufferCache_.clear();
> +	for (unsigned int i = 0; i < bufferPool_.count(); ++i)
> +		bufferCache_.emplace_back(std::array<int, 3>{ -1, -1, -1 }, i);
>  }
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel