[libcamera-devel] [PATCH v2 11/25] libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 30, 2019 at 01:04:56PM +0100, Niklas Söderlund wrote:
> In preparation for the FrameBuffer interface add a class that will deal
> with keeping the cache between dmafds and V4L2 video device buffer

s/dmafds/dmabuf file descriptors/

> indexes.
> 
> This initial implement ensures that no hot association is lost while its

s/implement/implementation/

> eviction strategy could be improved in the future.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - fetch() take a const reference instead of a const pointer
> - Rename argument for V4L2BufferCache()
> - Rename V4L2BufferCache::CacheInfo to V4L2BufferCache::Entry.
> - Turn V4L2BufferCache::Entry into a class with constructors for
>   foo.emplace_back().
> - Rename V4L2BufferCache::fetch() to V4L2BufferCache::get()
> - Make use of operator==
> - Large updates of documentation.
> ---
>  src/libcamera/include/v4l2_videodevice.h | 58 +++++++++++++-
>  src/libcamera/v4l2_videodevice.cpp       | 98 +++++++++++++++++++++++-
>  2 files changed, 152 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 1f52fe0120831fa3..9fc2082b2e7f7219 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -11,7 +11,9 @@
>  #include <vector>
>  
>  #include <linux/videodev2.h>
> +#include <memory>
>  
> +#include <libcamera/buffer.h>
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixelformats.h>
>  #include <libcamera/signal.h>
> @@ -22,9 +24,6 @@
>  
>  namespace libcamera {
>  
> -class Buffer;
> -class BufferMemory;
> -class BufferPool;
>  class EventNotifier;
>  class MediaDevice;
>  class MediaEntity;
> @@ -105,6 +104,59 @@ struct V4L2Capability final : v4l2_capability {
>  	}
>  };
>  
> +class V4L2BufferCache
> +{
> +public:
> +	V4L2BufferCache(unsigned int numEntries);
> +	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
> +
> +	int get(const FrameBuffer &buffer);
> +	void put(unsigned int index);
> +
> +private:
> +	class Entry
> +	{
> +	public:
> +		Entry(bool free, const std::vector<FrameBuffer::Plane> &planes)

Would it make sense to pass a const reference to a FrameBuffer here ? It
would simplify the callers.

> +			: free(free)
> +		{
> +			for (const FrameBuffer::Plane &plane : planes)
> +				last.emplace_back(plane);

last isn't a very descriptive name I think, should this be called planes
?

> +		}
> +
> +		bool operator==(const std::vector<FrameBuffer::Plane> &planes)

Same here, const reference to a FrameBuffer ?

> +		{
> +			if (last.size() != planes.size())
> +				return false;
> +
> +			for (unsigned int i = 0; i < planes.size(); i++)
> +				if (last[i].fd != planes[i].fd.fd() ||
> +				    last[i].length != planes[i].length)
> +					return false;
> +			return true;
> +		}
> +

Could we avoid making these two methods inline ?

> +		bool free;
> +
> +	private:
> +		class Plane

You can make this a struct.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		{
> +		public:
> +			Plane(const FrameBuffer::Plane &plane)
> +				: fd(plane.fd.fd()), length(plane.length)
> +			{
> +			}
> +
> +			int fd;
> +			unsigned int length;
> +		};
> +
> +		std::vector<Plane> last;
> +	};
> +
> +	std::vector<Entry> cache_;
> +};
> +
>  class V4L2DeviceFormat
>  {
>  public:
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 51112d197068cf0a..c9e17c44bfe17c3b 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -19,7 +19,6 @@
>  
>  #include <linux/drm_fourcc.h>
>  
> -#include <libcamera/buffer.h>
>  #include <libcamera/event_notifier.h>
>  
>  #include "log.h"
> @@ -134,6 +133,103 @@ LOG_DECLARE_CATEGORY(V4L2)
>   * \return True if the video device provides Streaming I/O IOCTLs
>   */
>  
> +/**
> + * \class V4L2BufferCache
> + * \brief Hot cache of associations between V4L2 buffer indexes and FrameBuffer
> + *
> + * When importing buffers, V4L2 performs lazy mapping of dmabuf instances at
> + * VIDIOC_QBUF (or VIDIOC_PREPARE_BUF) time and keeps the mapping associated
> + * with the V4L2 buffer, as identified by its index. If the same V4L2 buffer is
> + * then reused and queued with different dmabufs, the old dmabufs will be
> + * unmapped and the new ones mapped. To keep this process efficient, it is
> + * crucial to consistently use the same V4L2 buffer for given dmabufs through
> + * the whole duration of a capture cycle.
> + *
> + * The V4L2BufferCache class keeps a map of previous dmabufs to V4L2 buffer
> + * index associations to help selecting V4L2 buffers. It tracks, for every
> + * entry, if the V4L2 buffer is in use, and offers lookup of the best free V4L2
> + * buffer for a set of dmabufs.
> + */
> +
> +/**
> + * \brief Create an empty cache with \a numEntries entries
> + * \param[in] numEntries Number of entries to reserve in the cache
> + *
> + * Create a cache with \a numEntries entries all marked as unused. The entries
> + * will be populated as the cache is used. This is typically used to implement
> + * buffer import, with buffers added to the cache as they are queued.
> + */
> +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> +{
> +	cache_.resize(numEntries, Entry(true, {}));
> +}
> +
> +/**
> + * \brief Create a pre-populated cache
> + * \param[in] buffers Array of buffers to pre-populated with
> + *
> + * Create a cache pre-populated with \a buffers. This is typically used to
> + * implement buffer export, with all buffers added to the cache when they are
> + * allocated.
> + */
> +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> +{
> +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> +		cache_.emplace_back(true, buffer->planes());
> +}
> +
> +/**
> + * \brief Find the best V4L2 buffer for a FrameBuffer
> + * \param[in] buffer The FrameBuffer
> + *
> + * Find the best V4L2 buffer index to be used for the FrameBuffer \a buffer
> + * based on previous mappings of frame buffers to V4L2 buffers. If a free V4L2
> + * buffer previously used with the same dmabufs as \a buffer is found in the
> + * cache, return its index. Otherwise return the index of the first free V4L2
> + * buffer and record its association with the dmabufs of \a buffer.
> + *
> + * \return The index of the best V4L2 buffer, or -ENOENT if no free V4L2 buffer
> + * is available
> + */
> +int V4L2BufferCache::get(const FrameBuffer &buffer)
> +{
> +	const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
> +	int use = -1;
> +
> +	for (unsigned int index = 0; index < cache_.size(); index++) {
> +		const Entry &entry = cache_[index];
> +
> +		if (!entry.free)
> +			continue;
> +
> +		if (use < 0)
> +			use = index;
> +
> +		/* Try to find a cache hit by comparing the planes. */
> +		if (cache_[index] == planes) {
> +			use = index;
> +			break;
> +		}
> +	}
> +
> +	if (use < 0)
> +		return -ENOENT;
> +
> +	cache_[use] = Entry(false, planes);
> +
> +	return use;
> +}
> +
> +/**
> + * \brief Mark buffer \a index as free in the cache
> + * \param[in] index The V4L2 buffer index
> + */
> +void V4L2BufferCache::put(unsigned int index)
> +{
> +	ASSERT(index < cache_.size());
> +	cache_[index].free = true;
> +}
> +
>  /**
>   * \class V4L2DeviceFormat
>   * \brief The V4L2 video device image format and sizes

-- 
Regards,

Laurent Pinchart