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

[Laurent Pinchart]

Hi Niklas,

One more thing.

On Wed, Jan 08, 2020 at 01:04:21AM +0200, Laurent Pinchart wrote:
> 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.

Now that the index won't be visible to applications, the tests won't be
able to catch cache trashing :-S I think we need to instrument this
class. It's a bit out of scope for this series, but could you at least
add a miss counter, a LOG(Debug) in the destructor to print it, and a
"\todo Expose the miss counter through an instrumentation API" ?

> > 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