[libcamera-devel] [PATCH v3 17/33] libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 10, 2020 at 08:37:52PM +0100, Niklas Söderlund wrote:
> In preparation for the FrameBuffer interface add a class that will deal
> with keeping the cache between dmabuf file descriptors and V4L2 video
> device buffer indexes.
> 
> This initial implementation ensures that no hot association is lost
> while its eviction strategy could be improved in the future.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> * Changes since v2
> - Entry constructor and operator== takes FrameBuffer instead of
>   std::vector<FrameBuffer::Plane>.
> - Move Entry inline functions to from .h to .cpp file
> - Make Entry::Plane a struct
> - Add a miss counter
> 
> * 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 |  45 +++++++-
>  src/libcamera/v4l2_videodevice.cpp       | 136 ++++++++++++++++++++++-
>  2 files changed, 177 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 1f52fe0120831fa3..820b0cb297744828 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,46 @@ struct V4L2Capability final : v4l2_capability {
>  	}
>  };
>  
> +class V4L2BufferCache
> +{
> +public:
> +	V4L2BufferCache(unsigned int numEntries);
> +	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
> +	~V4L2BufferCache();
> +
> +	int get(const FrameBuffer &buffer);
> +	void put(unsigned int index);
> +
> +private:
> +	class Entry
> +	{
> +	public:
> +		Entry();
> +		Entry(bool free, const FrameBuffer &buffer);
> +
> +		bool operator==(const FrameBuffer &buffer);
> +
> +		bool free;
> +
> +	private:
> +		struct Plane {
> +			Plane(const FrameBuffer::Plane &plane)
> +				: fd(plane.fd.fd()), length(plane.length)
> +			{
> +			}
> +
> +			int fd;
> +			unsigned int length;
> +		};
> +
> +		std::vector<Plane> planes_;
> +	};
> +
> +	std::vector<Entry> cache_;
> +	/* \todo Expose the miss counter through an instrumentation API. */
> +	unsigned int missCounter_;
> +};
> +
>  class V4L2DeviceFormat
>  {
>  public:
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index f60490cc9a1b1771..474849846bddca8b 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -20,7 +20,6 @@
>  
>  #include <linux/drm_fourcc.h>
>  
> -#include <libcamera/buffer.h>
>  #include <libcamera/event_notifier.h>
>  
>  #include "log.h"
> @@ -135,6 +134,141 @@ 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)
> +	: missCounter_(0)
> +{
> +	cache_.resize(numEntries);
> +}
> +
> +/**
> + * \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)
> +	: missCounter_(0)
> +{
> +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> +		cache_.emplace_back(true, buffer->planes());
> +}
> +
> +V4L2BufferCache::~V4L2BufferCache()
> +{
> +	if (missCounter_)

I think this should be

	if (missCounter_ > cache_.size())

as we will typically have an initial miss for every entry.

> +		LOG(V4L2, Debug) << "Cached misses " << missCounter_;

s/Cached misses/Cache misses:/

> +}
> +
> +/**
> + * \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)
> +{
> +	bool miss = true;

How about calling this hit and initializing it to false, ...

> +	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] == buffer) {
> +			miss = false;

... and setting it to true here ...

> +			use = index;
> +			break;
> +		}
> +	}
> +
> +	if (miss)

and checking if (!hit) ? I think the logic would be slightly easier to
read.

> +		missCounter_++;
> +
> +	if (use < 0)
> +		return -ENOENT;
> +
> +	cache_[use] = Entry(false, buffer);
> +
> +	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;
> +}
> +
> +V4L2BufferCache::Entry::Entry()
> +	: free(true)
> +{
> +}
> +
> +V4L2BufferCache::Entry::Entry(bool free, const FrameBuffer &buffer)
> +	: free(free)
> +{
> +	for (const FrameBuffer::Plane &plane : buffer.planes())
> +		planes_.emplace_back(plane);
> +}
> +
> +bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer)
> +{
> +	const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
> +
> +	if (planes_.size() != planes.size())
> +		return false;
> +
> +	for (unsigned int i = 0; i < planes.size(); i++)
> +		if (planes_[i].fd != planes[i].fd.fd() ||
> +		    planes_[i].length != planes[i].length)
> +			return false;
> +	return true;
> +}
> +
>  /**
>   * \class V4L2DeviceFormat
>   * \brief The V4L2 video device image format and sizes

-- 
Regards,

Laurent Pinchart