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

2269 diff mbox series

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
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@ragnatech.se>
---
 src/libcamera/include/v4l2_videodevice.h | 19 ++++++++++
 src/libcamera/v4l2_videodevice.cpp       | 47 ++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

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