[libcamera-devel] [PATCH 18/30] libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping
Jacopo Mondi
jacopo at jmondi.org
Mon Dec 2 11:45:29 CET 2019
Hi Niklas,
On Wed, Nov 27, 2019 at 12:36:08AM +0100, Niklas Söderlund wrote:
> In preparation for the FrameBuffer interface add a class which will deal
s/FrameBuffer interface/switch to use the FrameBuffer interface/ ?
> with keeping the cache between dmafds and V4L2 video device buffer
> indexes.
>
> 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 | 20 ++++-
> src/libcamera/v4l2_videodevice.cpp | 105 ++++++++++++++++++++++-
> 2 files changed, 123 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 34bbff41760753bd..254f8797af42dd8a 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -12,6 +12,7 @@
>
> #include <linux/videodev2.h>
>
> +#include <libcamera/buffer.h>
> #include <libcamera/geometry.h>
> #include <libcamera/pixelformats.h>
> #include <libcamera/signal.h>
> @@ -22,7 +23,6 @@
>
> namespace libcamera {
>
> -class Buffer;
Is this and the removal of the buffer.h inclusion in
v4l2_videodevice.cpp related with the introduction of this class ?
> class BufferMemory;
> class BufferPool;
> class EventNotifier;
> @@ -105,6 +105,24 @@ struct V4L2Capability final : v4l2_capability {
> }
> };
>
> +class V4L2BufferCache
> +{
> +public:
> + V4L2BufferCache(unsigned int size);
> + V4L2BufferCache(const std::vector<FrameBuffer *> buffers);
> +
> + int fetch(const FrameBuffer *buffer);
Not sure what's best, as I've not seen this in use, but what about a
const FrameBuffer & ?
> + void put(unsigned int index);
> +
> +private:
> + struct CacheInfo {
CacheEntry ?
> + bool free;
> + std::vector<FrameBuffer::Plane> last;
> + };
> +
> + std::vector<CacheInfo> cache_;
> +};
> +
> class V4L2DeviceFormat
> {
> public:
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index a05dd6a1f7d86eaa..c82f2829601bd14c 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,110 @@ LOG_DECLARE_CATEGORY(V4L2)
> * \return True if the video device provides Streaming I/O IOCTLs
> */
>
> +/**
> + * \class V4L2BufferCache
> + * \brief Hot cache of associations between V4L2 index and FrameBuffer
V4L2 buffer indexes ?
> + *
> + * There is performance to be gained if the same V4L2 buffer index can be
> + * reused for the same FrameBuffer object as the kernel don't have to redo
the kernel -doesn't-
> + * the mapping. The V4L2BufferCache tries to keep a hot-cache of mappings
> + * between the two.
> + *
> + * If there is a cache miss is not critical, everything still works as expected.
> + */
> +
> +/**
> + * \brief Create a empty cache of a given size
an empty
of a give \a size
> + * \param[in] size Size of cache to create
Number of entries to reserve in the cache ?
> + *
> + * Create a cold cache with \a size entries. The cache will be populated as
> + * it's being used.
not sure, but this sounds better to me as
"as it is used"
> + */
> +V4L2BufferCache::V4L2BufferCache(unsigned int size)
> +{
> + cache_.resize(size, { .free = true, .last = {} });
> +}
> +
> +/**
> + * \brief Create a pre-populated cache
> + * \param[in] buffers Array of buffers to pre-populated with
> + *
> + * Create a warm cache from \a buffers.
> + */
> +V4L2BufferCache::V4L2BufferCache(const std::vector<FrameBuffer *> buffers)
> +{
> + for (const FrameBuffer *buffer : buffers)
> + cache_.push_back({ .free = true, .last = buffer->planes() });
> +}
Here and in all over this patch, are you copying vectors around ?
> +
> +/**
> + * \brief Fetch a index from the cache
an index
I would:
"Retrieve the V4L2 buffer index associated with \a buffer"
> + * \param[in] buffer FrameBuffer to match
The FrameBuffer
> + *
> + * Try to find \a buffer in cache and if it's free reuse the last used index
> + * for this buffer. If the buffer have never been seen or if have been evinced
the buffer -has-
> + * from the cache the first free index is pieced instead. Likewise if the last
not sure about what pieced means.
> + * used index is in use a new free index is picked.
I would just
"If no index is associated with \a buffer in the cache, or the index
has been re-assigned, use the first available one, if any"
> + *
> + * When an index is picked it is marked as in-use and returned to the caller.
> + * The association is also recorded so it if possible can reused the next time
> + * the FrameBuffer is seen.
That's the purpose of a cache :)
> + *
> + * \return V4L2 buffer index
or -ENOENT
> + */
> +int V4L2BufferCache::fetch(const FrameBuffer *buffer)
> +{
> + int use = -1;
> +
> + for (unsigned int index = 0; index < cache_.size(); index++) {
> + if (!cache_[index].free)
> + continue;
> +
> + if (use < 0)
> + use = index;
I wonder if we shouldn't keep the V4L2 buffer index in the CacheInfo
instead of using the CacheInfo entry position in the vector.
> +
> + /* Try to find a cache hit by comparing the planes. */
> + std::vector<FrameBuffer::Plane> planes = buffer->planes();
Are you copying the vector ?
> + if (cache_[index].last.size() != planes.size())
> + continue;
> +
> + bool match = true;
> + for (unsigned int i = 0; i < planes.size(); i++) {
> + if (cache_[index].last[i].fd != planes[i].fd ||
> + cache_[index].last[i].length != planes[i].length) {
> + match = false;
> + break;
> + }
I would here just use the address of the planes() vector as the key
instead of comparing it's content. Or is there a risk or relocation?
> + }
> +
> + if (!match)
> + continue;
> +
> + use = index;
> + break;
> + }
> +
> + if (use < 0)
> + return -ENOENT;
> +
> + cache_[use].free = false;
> + cache_[use].last = buffer->planes();
This seems to be a copy as well
> +
> + return use;
> +}
> +
> +/**
> + * \brief Pit a V4L2 index back in the cache
s/Pit/Put
I would:
"Make the V4L2 buffer \a idex available again in the cache"
> + * \param[in] index V4L2 index
The V4L2 buffer index
> + *
> + * Mark the \a index as free in the cache so it can be reused.
the v4l2 buffer \a index
> + */
> +void V4L2BufferCache::put(unsigned int index)
put would suggest a get() counterpart ? Or fetch/put is a good enough
operation names pair ?
> +{
> + ASSERT(index < cache_.size());
> + cache_[index].free = true;
If you keep the v4l2 index in the CacheInfo class as I've suggested
you would here pay the price of iterating through the vector. How
often is a v4l2 buffer index given back to the cache ?
Thanks
j
> +}
> +
> /**
> * \class V4L2DeviceFormat
> * \brief The V4L2 video device image format and sizes
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191202/f1d9f670/attachment.sig>
More information about the libcamera-devel
mailing list