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

[Niklas Söderlund]

Hi Jacopo and Laurent.

Thanks for your work.

On 2019-07-13 20:23:49 +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo at 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 at jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund