[libcamera-devel] [RFC 09/12] libcamera: v4l2_videodevice: Add a buffer cache class

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Oct 28, 2019 at 03:25:22AM +0100, Niklas Söderlund wrote:
> In preparation for the new buffer handling add a class which will deal
> with keeping the cache between dmafds and V4L2 video device buffer
> indexes. Previously this responsibility have been split between multiple

s/have been/was/

> classes.
> 
> This initial implement 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>
> ---
>  src/libcamera/include/v4l2_videodevice.h | 19 ++++++++++
>  src/libcamera/v4l2_videodevice.cpp       | 47 ++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 4b8cf9394eb9516f..5b178339d0ce7e2c 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -104,6 +104,25 @@ struct V4L2Capability final : v4l2_capability {
>  	}
>  };
>  
> +class V4L2BufferCache
> +{
> +public:
> +	V4L2BufferCache(unsigned int size);
> +
> +	void populate(unsigned int index, const std::array<int, 3> &fds);
> +
> +	int pop(const std::array<int, 3> &fds);
> +	void push(unsigned int index);

I understand why there's no doc at this stage, but you then need to name
methods in a self-documenting way :-) Jokes aside, I think better names
are needed here. I also fail to see why you need both populate() and
pop(), but maybe I'll figure it out in a later patch.

> +
> +private:
> +	struct CacheInfo {
> +		bool free;
> +		std::array<int, 3> last;
> +	};
> +
> +	std::map<unsigned int, CacheInfo> cache_;

As indices start at zero and are consecutive, how about using an
std::vector<CacheInfo> instead ?

> +};
> +
>  class V4L2DeviceFormat
>  {
>  public:
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index a2a9eab2bcc0d7e8..8bc2e439e4faeb68 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -132,6 +132,53 @@ LOG_DECLARE_CATEGORY(V4L2)
>   * \return True if the video device provides Streaming I/O IOCTLs
>   */
>  
> +V4L2BufferCache::V4L2BufferCache(unsigned int size)
> +{
> +	for (unsigned int i = 0; i < size; i++)
> +		cache_[i] = { .free = true, .last = { -1, -1, -1 } };

If you turn cache_ into a vector, this could just become

	cache_.resize(size, { .free = true, .last = { -1, -1, -1 } });

> +}
> +
> +void V4L2BufferCache::populate(unsigned int index, const std::array<int, 3> &fds)
> +{
> +	ASSERT(index < cache_.size());
> +	cache_[index].last = fds;
> +}
> +
> +int V4L2BufferCache::pop(const std::array<int, 3> &fds)
> +{
> +	int use = -1;
> +
> +	for (auto &it : cache_) {
> +		int index = it.first;
> +		CacheInfo &info = it.second;
> +
> +		if (!info.free)
> +			continue;
> +
> +		if (use < 0)
> +			use = index;
> +
> +		if (info.last == fds) {
> +			use = index;
> +			break;
> +		}
> +	}

Or maybe cache_ should be a map from fds to { index, free } to optimize
this ?

> +
> +	if (use < 0)
> +		return -ENOENT;
> +
> +	cache_[use].free = false;
> +	cache_[use].last = fds;
> +
> +	return use;
> +}
> +
> +void V4L2BufferCache::push(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