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

2564 diff mbox series

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@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

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@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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