[libcamera-devel] [RFC 6/8] libcamera: stream: Add operation to map buffers

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-06-30 20:10:47 +0200, Jacopo Mondi wrote:
> Add and operation to map external buffers provided by applications in
> a Request to the Stream's internal buffers used by the pipeline handlers
> to interact with the video device.
> 
> For streams using internal memory allocation, the two buffers are the
> same as applications effectively use Buffers from the Stream's pool
> where the video device memory has been exported to.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/buffer.h |  1 +
>  include/libcamera/stream.h |  5 +++
>  src/libcamera/stream.cpp   | 66 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 260a62e9e77e..d5d3dc90a096 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -59,6 +59,7 @@ private:
>  	friend class BufferPool;
>  	friend class PipelineHandler;
>  	friend class Request;
> +	friend class Stream;
>  	friend class V4L2VideoDevice;
>  
>  	void cancel();
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 796f1aff2602..4c034c113ddb 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -85,6 +85,8 @@ public:
>  	const StreamConfiguration &configuration() const { return configuration_; }
>  	MemoryType memoryType() const { return memoryType_; }
>  
> +	Buffer *mapBuffer(Buffer *requestBuffer);
> +
>  protected:
>  	friend class Camera;
>  
> @@ -94,6 +96,9 @@ protected:
>  	BufferPool bufferPool_;
>  	StreamConfiguration configuration_;
>  	MemoryType memoryType_;
> +
> +	std::vector<Buffer *> mappableBuffers_;
> +	std::map<unsigned int, Buffer *> bufferMaps_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index b6292427d3a2..f36336857ad6 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"
>  
>  /**
> @@ -445,6 +447,26 @@ void Stream::createBuffers(unsigned int count)
>  		return;
>  
>  	bufferPool_.createBuffers(count);
> +
> +	/* Streams with internal memory usage do not need buffer mapping. */
> +	if (memoryType_ == InternalMemory)
> +		return;
> +
> +	/*
> +	 * Prepare for buffer mapping by queuing all buffers from the internal
> +	 * pool. Each external buffer presented by application will be mapped
> +	 * on an internal one.
> +	 */
> +	mappableBuffers_.clear();
> +	for (Buffer &buffer : bufferPool_.buffers()) {
> +		/* Reserve all planes to support mapping multiplanar buffers. */
> +		buffer.planes().clear();
> +		/* \todo: I would use VIDEO_MAX_PLANES but that's V4L2 stuff.. */
> +		for (unsigned int i = 0; i < 3; ++i)
> +			buffer.planes().emplace_back();
> +
> +		mappableBuffers_.push_back(&buffer);
> +	}
>  }
>  
>  /**
> @@ -457,6 +479,50 @@ void Stream::destroyBuffers()
>  	createBuffers(0);
>  }
>  
> +/**
> + * \brief Map the buffer an application has associated with a Request to an
> + * internl one
> + *
> + * \todo Rewrite documentation
> + * If the Stream uses external memory, we need to map the externally
> + * provided buffer to an internal one, trying to keep a best effort
> + * association based on the Buffer's last usage time.

I would not use the word last here as it (at lest to me) implies that 
the buffer selected is the most recently used buffer and not the oldest 
buffer.

> + * External and internal buffers are associated by using the dmabuf
> + * fds as key.
> + */
> +Buffer *Stream::mapBuffer(Buffer *requestBuffer)
> +{
> +	/*
> +	 * \todo Multiplane APIs have one fd per plane, the key should be
> +	 * hashed using all the planes fds.
> +	 */
> +	unsigned int key = requestBuffer->planes()[0].dmabuf();
> +
> +	/* If the buffer has already been mapped, just return it. */
> +	auto mapped = bufferMaps_.find(key);
> +	if (mapped != bufferMaps_.end())
> +		return mapped->second;

On a hit should you not update the buffers position in mappableBuffers_ 
to increase the hit rate and try to keep the cache as hot as possible?  

One idea is to make mappableBuffers_ a std::set and index it on a 
timestmap property, that way you could just update the timestamp when a 
buffer is resued and when you need to evict a buffer from the list the 
oldes buffer will always be at the front (or end) of the set.

> +
> +	/*
> +	 * Remove the last recently used buffer from the circular list and
> +	 * use it for mapping.
> +	 */
> +	auto mappable = mappableBuffers_.begin();
> +	Buffer *buffer = *mappable;
> +	mappableBuffers_.erase(mappable);
> +	mappableBuffers_.push_back(buffer);

It's simpler to rotate the vector,

  std::rotate(mappableBuffers_.begin(), mappableBuffers_.begin() + 1, mappableBuffers_.end());

> +
> +	/* \todo: Support multiplanar external buffers. */
> +	buffer->planes()[0].setDmabuf(key, 0);
> +
> +	/* Pipeline handlers use request_ at buffer completion time. */
> +	buffer->request_ = requestBuffer->request();
> +
> +	bufferMaps_[key] = buffer;

I'm no expert on our buffer handling, but when you evict a buffer from 
mappableBuffers_ should it not also be removed from bufferMaps_? Else if 
every buffer imported is a new dmabuf we will consume a lot of memory.

> +
> +	return buffer;
> +}
> +
>  /**
>   * \var Stream::bufferPool_
>   * \brief The pool of buffers associated with the stream
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund